parent_bus_addr in struct of_range can indicate address information just
ahead of PCIe controller. Most system's bus fabric use 1:1 map between
input and output address. but some hardware like i.MX8QXP doesn't use 1:1
map. See below diagram:

            ┌─────────┐                    ┌────────────┐
 ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
 │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
 └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
  CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
0x7ff8_0000─┼───┘  │  │             │   │  │            │
            │      │  │             │   │  │            │   PCI Addr
0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
            │         │             │      │            │    0
0x7000_0000─┼────────►├─────────┐   │      │            │
            └─────────┘         │   └──────► CfgSpace  ─┼────────────►
             BUS Fabric         │          │            │    0
                                │          │            │
                                └──────────► MemSpace  ─┼────────────►
                        IA: 0x8000_0000    │            │  0x8000_0000
                                           └────────────┘

bus@5f000000 {
	compatible = "simple-bus";
	#address-cells = <1>;
	#size-cells = <1>;
	ranges = <0x80000000 0x0 0x70000000 0x10000000>;

	pcie@5f010000 {
		compatible = "fsl,imx8q-pcie";
		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
		reg-names = "dbi", "config";
		#address-cells = <3>;
		#size-cells = <2>;
		device_type = "pci";
		bus-range = <0x00 0xff>;
		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
	...
	};
};

Term internal address (IA) here means the address just before PCIe
controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
be removed.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v7 to v8
- Add dev_warning_once at dw_pcie_iatu_detect() to reminder
cpu_addr_fixup() user to correct their code
- use 'use_parent_dt_ranges' control enable use dt parent bus node ranges.
- rename dw_pcie_get_untranslate_addr to dw_pcie_get_parent_addr().
- of_property_read_reg() already have comments, so needn't add more.
- return actual err code from function

Change from v6 to v7
Add a resource_size_t parent_bus_addr local varible to fix 32bit build
error.
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410291546.kvgEWJv7-lkp@intel.com/

Chagne from v5 to v6
-add comments for of_property_read_reg().

Change from v4 to v5
- remove confused 0x5f00_0000 range in sample dts.
- reorder address at above diagram.

Change from v3 to v4
- none

Change from v2 to v3
- %s/cpu_untranslate_addr/parent_bus_addr/g
- update diagram.
- improve commit message.

Change from v1 to v2
- update because patch1 change get untranslate address method.
- add using_dtbus_info in case break back compatibility for exited platform.
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 57 ++++++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.c      |  9 ++++
 drivers/pci/controller/dwc/pcie-designware.h      |  7 +++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e41865c72904..f882b11fd7b94 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -418,6 +418,34 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 	}
 }
 
+static int dw_pcie_get_parent_addr(struct dw_pcie *pci, resource_size_t pci_addr,
+				   resource_size_t *i_addr)
+{
+	struct device *dev = pci->dev;
+	struct device_node *np = dev->of_node;
+	struct of_range_parser parser;
+	struct of_range range;
+	int ret;
+
+	if (!pci->use_parent_dt_ranges) {
+		*i_addr = pci_addr;
+		return 0;
+	}
+
+	ret = of_range_parser_init(&parser, np);
+	if (ret)
+		return ret;
+
+	for_each_of_pci_range(&parser, &range) {
+		if (pci_addr == range.bus_addr) {
+			*i_addr = range.parent_bus_addr;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 int dw_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -427,6 +455,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	struct resource_entry *win;
 	struct pci_host_bridge *bridge;
 	struct resource *res;
+	int index;
 	int ret;
 
 	raw_spin_lock_init(&pp->lock);
@@ -440,6 +469,20 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->cfg0_size = resource_size(res);
 		pp->cfg0_base = res->start;
 
+		if (pci->use_parent_dt_ranges) {
+			index = of_property_match_string(np, "reg-names", "config");
+			if (index < 0)
+				return -EINVAL;
+			/*
+			 * Retrieve the parent bus address of PCI config space.
+			 * If the parent bus ranges in the device tree provide
+			 * the correct address conversion information, set
+			 * 'use_parent_dt_ranges' to true, The
+			 * 'cpu_addr_fixup()' can be eliminated.
+			 */
+			of_property_read_reg(np, index, &pp->cfg0_base, NULL);
+		}
+
 		pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
 		if (IS_ERR(pp->va_cfg0_base))
 			return PTR_ERR(pp->va_cfg0_base);
@@ -462,6 +505,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->io_base = pci_pio_to_address(win->res->start);
 	}
 
+	ret = dw_pcie_get_parent_addr(pci, pp->io_bus_addr, &pp->io_base);
+	if (ret)
+		return ret;
+
 	/* Set default bus ops */
 	bridge->ops = &dw_pcie_ops;
 	bridge->child_ops = &dw_child_pcie_ops;
@@ -722,6 +769,8 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 
 	i = 0;
 	resource_list_for_each_entry(entry, &pp->bridge->windows) {
+		resource_size_t parent_bus_addr;
+
 		if (resource_type(entry->res) != IORESOURCE_MEM)
 			continue;
 
@@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 
 		atu.index = i;
 		atu.type = PCIE_ATU_TYPE_MEM;
-		atu.cpu_addr = entry->res->start;
+		parent_bus_addr = entry->res->start;
 		atu.pci_addr = entry->res->start - entry->offset;
 
+		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
+		if (ret)
+			return ret;
+
+		atu.cpu_addr = parent_bus_addr;
+
 		/* Adjust iATU size if MSG TLP region was allocated before */
 		if (pp->msg_res && pp->msg_res->parent == entry->res)
 			atu.size = resource_size(entry->res) -
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 6d6cbc8b5b2c6..e1ac9c81ad531 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -840,6 +840,15 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
 	pci->region_align = 1 << fls(min);
 	pci->region_limit = (max << 32) | (SZ_4G - 1);
 
+	if (pci->ops && pci->ops->cpu_addr_fixup) {
+		/*
+		 * If the parent 'ranges' property in DT correctly describes
+		 * the address translation, cpu_addr_fixup() callback is not
+		 * needed.
+		 */
+		dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n");
+	}
+
 	dev_info(pci->dev, "iATU: unroll %s, %u ob, %u ib, align %uK, limit %lluG\n",
 		 dw_pcie_cap_is(pci, IATU_UNROLL) ? "T" : "F",
 		 pci->num_ob_windows, pci->num_ib_windows,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35aa..4f31d4259a0de 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -463,6 +463,13 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+	/*
+	 * This flag indicates that the vendor driver uses devicetree 'ranges'
+	 * property to allow iATU to use the Intermediate Address (IA) for
+	 * outbound mapping. Using this flag also avoids the usage of
+	 * 'cpu_addr_fixup' callback implementation in the driver.
+	 */
+	bool			use_parent_dt_ranges;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)

-- 
2.34.1

On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> parent_bus_addr in struct of_range can indicate address information just
> ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> map. See below diagram:
> 
>             ┌─────────┐                    ┌────────────┐
>  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
>  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
>  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
>   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> 0x7ff8_0000─┼───┘  │  │             │   │  │            │
>             │      │  │             │   │  │            │   PCI Addr
> 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
>             │         │             │      │            │    0
> 0x7000_0000─┼────────►├─────────┐   │      │            │
>             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
>              BUS Fabric         │          │            │    0
>                                 │          │            │
>                                 └──────────► MemSpace  ─┼────────────►
>                         IA: 0x8000_0000    │            │  0x8000_0000
>                                            └────────────┘
> 
> bus@5f000000 {
> 	compatible = "simple-bus";
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
> 	pcie@5f010000 {
> 		compatible = "fsl,imx8q-pcie";
> 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> 		reg-names = "dbi", "config";
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		device_type = "pci";
> 		bus-range = <0x00 0xff>;
> 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> 	...
> 	};
> };
> 
> Term internal address (IA) here means the address just before PCIe
> controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> be removed.

> @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  
>  		atu.index = i;
>  		atu.type = PCIE_ATU_TYPE_MEM;
> -		atu.cpu_addr = entry->res->start;
> +		parent_bus_addr = entry->res->start;
>  		atu.pci_addr = entry->res->start - entry->offset;
>  
> +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> +		if (ret)
> +			return ret;
> +
> +		atu.cpu_addr = parent_bus_addr;

Here you set atu.cpu_addr to the intermediate bus address instead
of the CPU physical address before calling
dw_pcie_prog_outbound_atu().

But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
all of them need to use the intermediate bus address?

Maybe struct dw_pcie_ob_atu_cfg.cpu_addr should be renamed since it is
not necessarily a CPU physical address?

> +	if (pci->ops && pci->ops->cpu_addr_fixup) {
> +		/*
> +		 * If the parent 'ranges' property in DT correctly describes
> +		 * the address translation, cpu_addr_fixup() callback is not
> +		 * needed.
> +		 */
> +		dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n");
> +	}

I kinda wish this warning were in a separate patch because it will be
a little cleaner if we ever want to revert or remove the warning.

On Thu, Jan 16, 2025 at 05:13:58PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > parent_bus_addr in struct of_range can indicate address information just
> > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > map. See below diagram:
> >
> >             ┌─────────┐                    ┌────────────┐
> >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> >             │      │  │             │   │  │            │   PCI Addr
> > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> >             │         │             │      │            │    0
> > 0x7000_0000─┼────────►├─────────┐   │      │            │
> >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> >              BUS Fabric         │          │            │    0
> >                                 │          │            │
> >                                 └──────────► MemSpace  ─┼────────────►
> >                         IA: 0x8000_0000    │            │  0x8000_0000
> >                                            └────────────┘
> >
> > bus@5f000000 {
> > 	compatible = "simple-bus";
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >
> > 	pcie@5f010000 {
> > 		compatible = "fsl,imx8q-pcie";
> > 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > 		reg-names = "dbi", "config";
> > 		#address-cells = <3>;
> > 		#size-cells = <2>;
> > 		device_type = "pci";
> > 		bus-range = <0x00 0xff>;
> > 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > 	...
> > 	};
> > };
> >
> > Term internal address (IA) here means the address just before PCIe
> > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > be removed.
>
> > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >
> >  		atu.index = i;
> >  		atu.type = PCIE_ATU_TYPE_MEM;
> > -		atu.cpu_addr = entry->res->start;
> > +		parent_bus_addr = entry->res->start;
> >  		atu.pci_addr = entry->res->start - entry->offset;
> >
> > +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > +		if (ret)
> > +			return ret;
> > +
> > +		atu.cpu_addr = parent_bus_addr;
>
> Here you set atu.cpu_addr to the intermediate bus address instead
> of the CPU physical address before calling
> dw_pcie_prog_outbound_atu().
>
> But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
> all of them need to use the intermediate bus address?

EP side change in follow patches. RC side only here call
dw_pcie_prog_outbound_atu()

>
> Maybe struct dw_pcie_ob_atu_cfg.cpu_addr should be renamed since it is
> not necessarily a CPU physical address?

Yes, we can improve it later.

>
> > +	if (pci->ops && pci->ops->cpu_addr_fixup) {
> > +		/*
> > +		 * If the parent 'ranges' property in DT correctly describes
> > +		 * the address translation, cpu_addr_fixup() callback is not
> > +		 * needed.
> > +		 */
> > +		dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n");
> > +	}
>
> I kinda wish this warning were in a separate patch because it will be
> a little cleaner if we ever want to revert or remove the warning.

I don't think it is possible to only revert one print warning patch in
future. I think only two path: one revert whole solution, the other is fix
all dts and remove whole cpu_addr_fixup.

Frank

On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > parent_bus_addr in struct of_range can indicate address information just
> > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > map. See below diagram:
> > 
> >             ┌─────────┐                    ┌────────────┐
> >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> >             │      │  │             │   │  │            │   PCI Addr
> > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> >             │         │             │      │            │    0
> > 0x7000_0000─┼────────►├─────────┐   │      │            │
> >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> >              BUS Fabric         │          │            │    0
> >                                 │          │            │
> >                                 └──────────► MemSpace  ─┼────────────►
> >                         IA: 0x8000_0000    │            │  0x8000_0000
> >                                            └────────────┘
> > 
> > bus@5f000000 {
> > 	compatible = "simple-bus";
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > 
> > 	pcie@5f010000 {
> > 		compatible = "fsl,imx8q-pcie";
> > 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > 		reg-names = "dbi", "config";
> > 		#address-cells = <3>;
> > 		#size-cells = <2>;
> > 		device_type = "pci";
> > 		bus-range = <0x00 0xff>;
> > 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > 	...
> > 	};
> > };
> > 
> > Term internal address (IA) here means the address just before PCIe
> > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > be removed.
> 
> > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >  
> >  		atu.index = i;
> >  		atu.type = PCIE_ATU_TYPE_MEM;
> > -		atu.cpu_addr = entry->res->start;
> > +		parent_bus_addr = entry->res->start;
> >  		atu.pci_addr = entry->res->start - entry->offset;
> >  
> > +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > +		if (ret)
> > +			return ret;
> > +
> > +		atu.cpu_addr = parent_bus_addr;
> 
> Here you set atu.cpu_addr to the intermediate bus address instead
> of the CPU physical address before calling
> dw_pcie_prog_outbound_atu().
> 
> But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
> all of them need to use the intermediate bus address?

Somehow I expected the patch to skip calling ->cpu_addr_fixup() if the
driver had set 'use_parent_dt_ranges'.  In fact, I think that's a
requirement.

Since dw_pcie_prog_outbound_atu() is the only dwc caller, maybe the
parent_bus_addr change should go *there* instead of in the callers?

Bjorn

On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> > On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > > parent_bus_addr in struct of_range can indicate address information just
> > > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > > map. See below diagram:
> > >
> > >             ┌─────────┐                    ┌────────────┐
> > >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> > >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> > >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> > >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> > >             │      │  │             │   │  │            │   PCI Addr
> > > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> > >             │         │             │      │            │    0
> > > 0x7000_0000─┼────────►├─────────┐   │      │            │
> > >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> > >              BUS Fabric         │          │            │    0
> > >                                 │          │            │
> > >                                 └──────────► MemSpace  ─┼────────────►
> > >                         IA: 0x8000_0000    │            │  0x8000_0000
> > >                                            └────────────┘
> > >
> > > bus@5f000000 {
> > > 	compatible = "simple-bus";
> > > 	#address-cells = <1>;
> > > 	#size-cells = <1>;
> > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > >
> > > 	pcie@5f010000 {
> > > 		compatible = "fsl,imx8q-pcie";
> > > 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > > 		reg-names = "dbi", "config";
> > > 		#address-cells = <3>;
> > > 		#size-cells = <2>;
> > > 		device_type = "pci";
> > > 		bus-range = <0x00 0xff>;
> > > 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > > 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > > 	...
> > > 	};
> > > };
> > >
> > > Term internal address (IA) here means the address just before PCIe
> > > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > > be removed.
> >
> > > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >
> > >  		atu.index = i;
> > >  		atu.type = PCIE_ATU_TYPE_MEM;
> > > -		atu.cpu_addr = entry->res->start;
> > > +		parent_bus_addr = entry->res->start;
> > >  		atu.pci_addr = entry->res->start - entry->offset;
> > >
> > > +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		atu.cpu_addr = parent_bus_addr;
> >
> > Here you set atu.cpu_addr to the intermediate bus address instead
> > of the CPU physical address before calling
> > dw_pcie_prog_outbound_atu().
> >
> > But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
> > all of them need to use the intermediate bus address?

All should use "intermediate bus address", RC side only call it here. EP

side is here
https://lore.kernel.org/imx/Z4p0fUAK1ONNjLst@lizhi-Precision-Tower-5810/T/#t

>
> Somehow I expected the patch to skip calling ->cpu_addr_fixup() if the
> driver had set 'use_parent_dt_ranges'.  In fact, I think that's a
> requirement.

It's fine to add check to call cpu_addr_fixup() although I think driver
owner should take responsiblity to make cpu_addr_fixup and
use_parent_dt_ranges exclusive.

>
> Since dw_pcie_prog_outbound_atu() is the only dwc caller, maybe the
> parent_bus_addr change should go *there* instead of in the callers?

I am not sure I understand your means.

EP and RC parts need call dw_pcie_prog_outbound_atu(). EP and RC use
difference method to get outbound windows informaiton. So can't move it
into dw_pcie_prog_outbound_atu().

Frank
>
> Bjorn

On Fri, Jan 17, 2025 at 10:42:37AM -0500, Frank Li wrote:
> On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > > > parent_bus_addr in struct of_range can indicate address information just
> > > > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > > > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > > > map. See below diagram:
> > > >
> > > >             ┌─────────┐                    ┌────────────┐
> > > >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> > > >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> > > >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> > > >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > > > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> > > >             │      │  │             │   │  │            │   PCI Addr
> > > > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> > > >             │         │             │      │            │    0
> > > > 0x7000_0000─┼────────►├─────────┐   │      │            │
> > > >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> > > >              BUS Fabric         │          │            │    0
> > > >                                 │          │            │
> > > >                                 └──────────► MemSpace  ─┼────────────►
> > > >                         IA: 0x8000_0000    │            │  0x8000_0000
> > > >                                            └────────────┘
> > > >
> > > > bus@5f000000 {
> > > > 	compatible = "simple-bus";
> > > > 	#address-cells = <1>;
> > > > 	#size-cells = <1>;
> > > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > > >
> > > > 	pcie@5f010000 {
> > > > 		compatible = "fsl,imx8q-pcie";
> > > > 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > > > 		reg-names = "dbi", "config";
> > > > 		#address-cells = <3>;
> > > > 		#size-cells = <2>;
> > > > 		device_type = "pci";
> > > > 		bus-range = <0x00 0xff>;
> > > > 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > > > 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > > > 	...
> > > > 	};
> > > > };
> > > >
> > > > Term internal address (IA) here means the address just before PCIe
> > > > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > > > be removed.
> > >
> > > > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > >
> > > >  		atu.index = i;
> > > >  		atu.type = PCIE_ATU_TYPE_MEM;
> > > > -		atu.cpu_addr = entry->res->start;
> > > > +		parent_bus_addr = entry->res->start;
> > > >  		atu.pci_addr = entry->res->start - entry->offset;
> > > >
> > > > +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		atu.cpu_addr = parent_bus_addr;
> > >
> > > Here you set atu.cpu_addr to the intermediate bus address instead
> > > of the CPU physical address before calling
> > > dw_pcie_prog_outbound_atu().
> > >
> > > But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
> > > all of them need to use the intermediate bus address?
> 
> All should use "intermediate bus address", RC side only call it here. EP
> side is here
> https://lore.kernel.org/imx/Z4p0fUAK1ONNjLst@lizhi-Precision-Tower-5810/T/#t

Currently dw_pcie_prog_outbound_atu() uses atu->cpu_addr, calls
ops->cpu_addr_fixup() if defined, and writes cpu_addr to
PCIE_ATU_LOWER_BASE/PCIE_ATU_UPPER_BASE.

The callers of dw_pcie_prog_outbound_atu() I see are:

  dw_pcie_ep_outbound_atu
    atu.cpu_addr came from dw_pcie_ep_map_addr(), so it's a CPU
    address.  Fixed by [1], which reads ep->bus_addr_base from the
    "addr_space" 'reg' property.

  dw_pcie_other_conf_map_bus
    atu.cpu_addr came from pp->cfg0_base, set by dw_pcie_host_init()
    to a CPU address (the "config" resource).  Fixed by [2], which
    overwrites pp->cfg0_base with the address from the "config" 'reg'
    property if 'use_parent_dt_ranges' is set.

  dw_pcie_rd_other_conf
  dw_pcie_wr_other_conf
    dw_pcie_prog_outbound_atu() only called if pp->cfg0_io_shared,
    after an ECAM map via dw_pcie_other_conf_map_bus() and subsequent
    successful access; atu.cpu_addr came from pp->io_base, set by
    dw_pcie_host_init() from pci_pio_to_address(), which I'm pretty
    sure returns a CPU address.

    So this still looks wrong to me.  In addition, I think doing the
    ATU setup in *_map() and restore in *rd/wr_other_conf() is ugly
    and looks unreliable.

  dw_pcie_iatu_setup
    atu.cpu_addr came from the struct resource in pp->bridge->windows,
    so it's a CPU address.  Also fixed by [2], which overwrites
    atu.cpu_addr with the address from dw_pcie_get_parent_addr(), which
    iterates through the PCI controller's 'ranges' property and
    returns the range.parent_bus_addr.

  dw_pcie_pme_turn_off
    atu.cpu_addr came from pp.msg_res, set by
    dw_pcie_host_request_msg_tlp_res() to a CPU address at the end of
    the first MMIO bridge window.  This one also looks wrong to me.

[1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-3-c4bfa5193288@nxp.com
[2] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-2-c4bfa5193288@nxp.com

> > Since dw_pcie_prog_outbound_atu() is the only dwc caller, maybe the
> > parent_bus_addr change should go *there* instead of in the callers?
> 
> I am not sure I understand your means.
> 
> EP and RC parts need call dw_pcie_prog_outbound_atu(). EP and RC use
> difference method to get outbound windows information. So can't move it
> into dw_pcie_prog_outbound_atu().

Yes, I think I see what you mean.  In the Endpoint case, the iATU
input comes from the "addr_space" 'reg' property.  In the Root Complex
case, it comes from the parent bus address of the 'ranges' property.

Bjorn

On Fri, Jan 17, 2025 at 10:42:37AM -0500, Frank Li wrote:
> On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > > > parent_bus_addr in struct of_range can indicate address information just
> > > > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > > > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > > > map. See below diagram:
> > > >
> > > >             ┌─────────┐                    ┌────────────┐
> > > >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> > > >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> > > >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> > > >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > > > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> > > >             │      │  │             │   │  │            │   PCI Addr
> > > > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> > > >             │         │             │      │            │    0
> > > > 0x7000_0000─┼────────►├─────────┐   │      │            │
> > > >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> > > >              BUS Fabric         │          │            │    0
> > > >                                 │          │            │
> > > >                                 └──────────► MemSpace  ─┼────────────►
> > > >                         IA: 0x8000_0000    │            │  0x8000_0000
> > > >                                            └────────────┘
> > > >
> > > > bus@5f000000 {
> > > > 	compatible = "simple-bus";
> > > > 	#address-cells = <1>;
> > > > 	#size-cells = <1>;
> > > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > > >
> > > > 	pcie@5f010000 {
> > > > 		compatible = "fsl,imx8q-pcie";
> > > > 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > > > 		reg-names = "dbi", "config";
> > > > 		#address-cells = <3>;
> > > > 		#size-cells = <2>;
> > > > 		device_type = "pci";
> > > > 		bus-range = <0x00 0xff>;
> > > > 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > > > 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > > > 	...
> > > > 	};
> > > > };
> > > >
> > > > Term internal address (IA) here means the address just before PCIe
> > > > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > > > be removed.
> > >
> > > > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > > >
> > > >  		atu.index = i;
> > > >  		atu.type = PCIE_ATU_TYPE_MEM;
> > > > -		atu.cpu_addr = entry->res->start;
> > > > +		parent_bus_addr = entry->res->start;
> > > >  		atu.pci_addr = entry->res->start - entry->offset;
> > > >
> > > > +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		atu.cpu_addr = parent_bus_addr;
> > >
> > > Here you set atu.cpu_addr to the intermediate bus address instead
> > > of the CPU physical address before calling
> > > dw_pcie_prog_outbound_atu().
> > >
> > > But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
> > > all of them need to use the intermediate bus address?
>
> All should use "intermediate bus address", RC side only call it here. EP
>
> side is here
> https://lore.kernel.org/imx/Z4p0fUAK1ONNjLst@lizhi-Precision-Tower-5810/T/#t
>
> >
> > Somehow I expected the patch to skip calling ->cpu_addr_fixup() if the
> > driver had set 'use_parent_dt_ranges'.  In fact, I think that's a
> > requirement.
>
> It's fine to add check to call cpu_addr_fixup() although I think driver
> owner should take responsiblity to make cpu_addr_fixup and
> use_parent_dt_ranges exclusive.
>
> >
> > Since dw_pcie_prog_outbound_atu() is the only dwc caller, maybe the
> > parent_bus_addr change should go *there* instead of in the callers?
>
> I am not sure I understand your means.
>
> EP and RC parts need call dw_pcie_prog_outbound_atu(). EP and RC use
> difference method to get outbound windows informaiton. So can't move it
> into dw_pcie_prog_outbound_atu().
>
> Frank

Bjorn:

	I saw you have not picked all of these patches during you rework
pci git branches.

	I know you are busy, do you have chance to pick left patch for 6.14.

Frank

> >
> > Bjorn

On Thu, Jan 23, 2025 at 10:21:36AM -0500, Frank Li wrote:
> On Fri, Jan 17, 2025 at 10:42:37AM -0500, Frank Li wrote:
> > On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> > > On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > > > > parent_bus_addr in struct of_range can indicate address information just
> > > > > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > > > > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > > > > map.
> ...

> 	I saw you have not picked all of these patches during you rework
> pci git branches.
> 
> 	I know you are busy, do you have chance to pick left patch for 6.14.

This series had a mix of things: several patches related to
.cpu_addr_fixup(), plus several unrelated ones for PHY mode and i.MX8Q
support.  I think I picked up all the unrelated ones.

.cpu_addr_fixup() is a generic problem that affects dwc (dra7xx, imx6,
artpec6, intel-gw, visconti), cadence (cadence-plat), and now
apparently microchip.

I deferred these because I'm hoping we can come up with a more generic
solution that's easier to apply across all these cases.  I don't
really want to merge something that immediately needs to be reworked
for other drivers.

A few of the things I wonder about:

  - dw_pcie_get_parent_addr() has no DWC dependencies, so it doesn't
    make sense to me to have it be DWC-specific and copy and pasted
    to other places that need something similar.

  - It doesn't seem elegant to iterate through for_each_pci_range() in
    devm_of_pci_get_host_bridge_resources(), then again in
    dw_pcie_host_init() for io_bus_addr, then again in
    dw_pcie_iatu_setup() for each window.  Maybe that's the best we
    can do, but maybe there's a way to capture what we need on the
    first time through.

  - The connection between .cpu_addr_fixup() and use_parent_dt_ranges
    is clear in the patches remove a .cpu_addr_fixup(), but not in the
    DWC patches on the other end.

  - Ideally, "use_parent_dt_ranges" would be the default and we
    wouldn't have a flag to indicate that, and drivers would have to
    opt out instead of opt in.  They basically already do that by
    implementing .cpu_addr_fixup(), so maybe we can take advantage of
    that fact.

Bjorn

On Thu, Jan 23, 2025 at 01:04:22PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 23, 2025 at 10:21:36AM -0500, Frank Li wrote:
> > On Fri, Jan 17, 2025 at 10:42:37AM -0500, Frank Li wrote:
> > > On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> > > > On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> > > > > On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > > > > > parent_bus_addr in struct of_range can indicate address information just
> > > > > > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > > > > > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > > > > > map.
> > ...
>
> > 	I saw you have not picked all of these patches during you rework
> > pci git branches.
> >
> > 	I know you are busy, do you have chance to pick left patch for 6.14.
>
> This series had a mix of things: several patches related to
> .cpu_addr_fixup(), plus several unrelated ones for PHY mode and i.MX8Q
> support.  I think I picked up all the unrelated ones.
>
> .cpu_addr_fixup() is a generic problem that affects dwc (dra7xx, imx6,
> artpec6, intel-gw, visconti), cadence (cadence-plat), and now
> apparently microchip.
>
> I deferred these because I'm hoping we can come up with a more generic
> solution that's easier to apply across all these cases.  I don't
> really want to merge something that immediately needs to be reworked
> for other drivers.
>
> A few of the things I wonder about:
>
>   - dw_pcie_get_parent_addr() has no DWC dependencies, so it doesn't
>     make sense to me to have it be DWC-specific and copy and pasted
>     to other places that need something similar.
>
>   - It doesn't seem elegant to iterate through for_each_pci_range() in
>     devm_of_pci_get_host_bridge_resources(), then again in
>     dw_pcie_host_init() for io_bus_addr, then again in
>     dw_pcie_iatu_setup() for each window.  Maybe that's the best we
>     can do, but maybe there's a way to capture what we need on the
>     first time through.
>
>   - The connection between .cpu_addr_fixup() and use_parent_dt_ranges
>     is clear in the patches remove a .cpu_addr_fixup(), but not in the
>     DWC patches on the other end.
>
>   - Ideally, "use_parent_dt_ranges" would be the default and we
>     wouldn't have a flag to indicate that, and drivers would have to
>     opt out instead of opt in.  They basically already do that by
>     implementing .cpu_addr_fixup(), so maybe we can take advantage of
>     that fact.

Okay, thanks. let me think how to improve it after 6.14.

Frank
>
> Bjorn

On Thu, Jan 23, 2025 at 02:15:10PM -0500, Frank Li wrote:
> On Thu, Jan 23, 2025 at 01:04:22PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 23, 2025 at 10:21:36AM -0500, Frank Li wrote:
> > > On Fri, Jan 17, 2025 at 10:42:37AM -0500, Frank Li wrote:
> > > > On Thu, Jan 16, 2025 at 05:29:16PM -0600, Bjorn Helgaas wrote:
> > > > > On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> > > > > > On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > > > > > > parent_bus_addr in struct of_range can indicate address information just
> > > > > > > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > > > > > > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > > > > > > map.
> > > ...
> >
> > > 	I saw you have not picked all of these patches during you rework
> > > pci git branches.
> > >
> > > 	I know you are busy, do you have chance to pick left patch for 6.14.
> >
> > This series had a mix of things: several patches related to
> > .cpu_addr_fixup(), plus several unrelated ones for PHY mode and i.MX8Q
> > support.  I think I picked up all the unrelated ones.
> >
> > .cpu_addr_fixup() is a generic problem that affects dwc (dra7xx, imx6,
> > artpec6, intel-gw, visconti), cadence (cadence-plat), and now
> > apparently microchip.
> >
> > I deferred these because I'm hoping we can come up with a more generic
> > solution that's easier to apply across all these cases.  I don't
> > really want to merge something that immediately needs to be reworked
> > for other drivers.
> >
> > A few of the things I wonder about:
> >
> >   - dw_pcie_get_parent_addr() has no DWC dependencies, so it doesn't
> >     make sense to me to have it be DWC-specific and copy and pasted
> >     to other places that need something similar.
> >
> >   - It doesn't seem elegant to iterate through for_each_pci_range() in
> >     devm_of_pci_get_host_bridge_resources(), then again in
> >     dw_pcie_host_init() for io_bus_addr, then again in
> >     dw_pcie_iatu_setup() for each window.  Maybe that's the best we
> >     can do, but maybe there's a way to capture what we need on the
> >     first time through.
> >
> >   - The connection between .cpu_addr_fixup() and use_parent_dt_ranges
> >     is clear in the patches remove a .cpu_addr_fixup(), but not in the
> >     DWC patches on the other end.
> >
> >   - Ideally, "use_parent_dt_ranges" would be the default and we
> >     wouldn't have a flag to indicate that, and drivers would have to
> >     opt out instead of opt in.  They basically already do that by
> >     implementing .cpu_addr_fixup(), so maybe we can take advantage of
> >     that fact.
> 
> Okay, thanks. let me think how to improve it after 6.14.


Small nit that I saw when looking at this series.

Some patches use "Internal Address (IA)", but other patches use
"Intermediate Address (IA)". For the re-spin, use one term consistently.


Kind regards,
Niklas

On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> parent_bus_addr in struct of_range can indicate address information just
> ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> map. See below diagram:
> 
>             ┌─────────┐                    ┌────────────┐
>  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
>  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
>  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
>   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> 0x7ff8_0000─┼───┘  │  │             │   │  │            │
>             │      │  │             │   │  │            │   PCI Addr
> 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
>             │         │             │      │            │    0
> 0x7000_0000─┼────────►├─────────┐   │      │            │
>             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
>              BUS Fabric         │          │            │    0
>                                 │          │            │
>                                 └──────────► MemSpace  ─┼────────────►
>                         IA: 0x8000_0000    │            │  0x8000_0000
>                                            └────────────┘
> 
> bus@5f000000 {
> 	compatible = "simple-bus";
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
> 	pcie@5f010000 {
> 		compatible = "fsl,imx8q-pcie";
> 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> 		reg-names = "dbi", "config";
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		device_type = "pci";
> 		bus-range = <0x00 0xff>;
> 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> 	...
> 	};
> };
> 
> Term internal address (IA) here means the address just before PCIe
> controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> be removed.
> 

The newly added warning should be mentioned in the commit message. But no need
to respin just for this. I hope Krzysztof can add it while applying.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> Change from v7 to v8
> - Add dev_warning_once at dw_pcie_iatu_detect() to reminder
> cpu_addr_fixup() user to correct their code
> - use 'use_parent_dt_ranges' control enable use dt parent bus node ranges.
> - rename dw_pcie_get_untranslate_addr to dw_pcie_get_parent_addr().
> - of_property_read_reg() already have comments, so needn't add more.
> - return actual err code from function
> 
> Change from v6 to v7
> Add a resource_size_t parent_bus_addr local varible to fix 32bit build
> error.
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410291546.kvgEWJv7-lkp@intel.com/
> 
> Chagne from v5 to v6
> -add comments for of_property_read_reg().
> 
> Change from v4 to v5
> - remove confused 0x5f00_0000 range in sample dts.
> - reorder address at above diagram.
> 
> Change from v3 to v4
> - none
> 
> Change from v2 to v3
> - %s/cpu_untranslate_addr/parent_bus_addr/g
> - update diagram.
> - improve commit message.
> 
> Change from v1 to v2
> - update because patch1 change get untranslate address method.
> - add using_dtbus_info in case break back compatibility for exited platform.
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 57 ++++++++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.c      |  9 ++++
>  drivers/pci/controller/dwc/pcie-designware.h      |  7 +++
>  3 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 3e41865c72904..f882b11fd7b94 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -418,6 +418,34 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
>  	}
>  }
>  
> +static int dw_pcie_get_parent_addr(struct dw_pcie *pci, resource_size_t pci_addr,
> +				   resource_size_t *i_addr)
> +{
> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
> +	struct of_range_parser parser;
> +	struct of_range range;
> +	int ret;
> +
> +	if (!pci->use_parent_dt_ranges) {
> +		*i_addr = pci_addr;
> +		return 0;
> +	}
> +
> +	ret = of_range_parser_init(&parser, np);
> +	if (ret)
> +		return ret;
> +
> +	for_each_of_pci_range(&parser, &range) {
> +		if (pci_addr == range.bus_addr) {
> +			*i_addr = range.parent_bus_addr;
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -427,6 +455,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	struct resource_entry *win;
>  	struct pci_host_bridge *bridge;
>  	struct resource *res;
> +	int index;
>  	int ret;
>  
>  	raw_spin_lock_init(&pp->lock);
> @@ -440,6 +469,20 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		pp->cfg0_size = resource_size(res);
>  		pp->cfg0_base = res->start;
>  
> +		if (pci->use_parent_dt_ranges) {
> +			index = of_property_match_string(np, "reg-names", "config");
> +			if (index < 0)
> +				return -EINVAL;
> +			/*
> +			 * Retrieve the parent bus address of PCI config space.
> +			 * If the parent bus ranges in the device tree provide
> +			 * the correct address conversion information, set
> +			 * 'use_parent_dt_ranges' to true, The
> +			 * 'cpu_addr_fixup()' can be eliminated.
> +			 */
> +			of_property_read_reg(np, index, &pp->cfg0_base, NULL);
> +		}
> +
>  		pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
>  		if (IS_ERR(pp->va_cfg0_base))
>  			return PTR_ERR(pp->va_cfg0_base);
> @@ -462,6 +505,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		pp->io_base = pci_pio_to_address(win->res->start);
>  	}
>  
> +	ret = dw_pcie_get_parent_addr(pci, pp->io_bus_addr, &pp->io_base);
> +	if (ret)
> +		return ret;
> +
>  	/* Set default bus ops */
>  	bridge->ops = &dw_pcie_ops;
>  	bridge->child_ops = &dw_child_pcie_ops;
> @@ -722,6 +769,8 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  
>  	i = 0;
>  	resource_list_for_each_entry(entry, &pp->bridge->windows) {
> +		resource_size_t parent_bus_addr;
> +
>  		if (resource_type(entry->res) != IORESOURCE_MEM)
>  			continue;
>  
> @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  
>  		atu.index = i;
>  		atu.type = PCIE_ATU_TYPE_MEM;
> -		atu.cpu_addr = entry->res->start;
> +		parent_bus_addr = entry->res->start;
>  		atu.pci_addr = entry->res->start - entry->offset;
>  
> +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> +		if (ret)
> +			return ret;
> +
> +		atu.cpu_addr = parent_bus_addr;
> +
>  		/* Adjust iATU size if MSG TLP region was allocated before */
>  		if (pp->msg_res && pp->msg_res->parent == entry->res)
>  			atu.size = resource_size(entry->res) -
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 6d6cbc8b5b2c6..e1ac9c81ad531 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -840,6 +840,15 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  	pci->region_align = 1 << fls(min);
>  	pci->region_limit = (max << 32) | (SZ_4G - 1);
>  
> +	if (pci->ops && pci->ops->cpu_addr_fixup) {
> +		/*
> +		 * If the parent 'ranges' property in DT correctly describes
> +		 * the address translation, cpu_addr_fixup() callback is not
> +		 * needed.
> +		 */
> +		dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n");
> +	}
> +
>  	dev_info(pci->dev, "iATU: unroll %s, %u ob, %u ib, align %uK, limit %lluG\n",
>  		 dw_pcie_cap_is(pci, IATU_UNROLL) ? "T" : "F",
>  		 pci->num_ob_windows, pci->num_ib_windows,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 347ab74ac35aa..4f31d4259a0de 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -463,6 +463,13 @@ struct dw_pcie {
>  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
>  	struct gpio_desc		*pe_rst;
>  	bool			suspended;
> +	/*
> +	 * This flag indicates that the vendor driver uses devicetree 'ranges'
> +	 * property to allow iATU to use the Intermediate Address (IA) for
> +	 * outbound mapping. Using this flag also avoids the usage of
> +	 * 'cpu_addr_fixup' callback implementation in the driver.
> +	 */
> +	bool			use_parent_dt_ranges;
>  };
>  
>  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Hello,

> The newly added warning should be mentioned in the commit message. But no need
> to respin just for this. I hope Krzysztof can add it while applying.

The commit message will include the following:

  To expedite deprecation of the cpu_addr_fixup() callback, warn its users
  if the 'ranges' property is missing from the Devicetree or the address
  translation information is somewhat incorrect.

Let me know if anything needs to be changed.

	Krzysztof

On Thu, Jan 16, 2025 at 10:47:47AM +0900, Krzysztof Wilczyński wrote:
> Hello,
>
> > The newly added warning should be mentioned in the commit message. But no need
> > to respin just for this. I hope Krzysztof can add it while applying.
>
> The commit message will include the following:
>
>   To expedite deprecation of the cpu_addr_fixup() callback, warn its users
>   if the 'ranges' property is missing from the Devicetree or the address
>   translation information is somewhat incorrect.

Good, thanks

Frank

>
> Let me know if anything needs to be changed.
>
> 	Krzysztof