[PATCH v9 0/7] PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()

[PATCH v9 0/7] PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()

[Frank Li]

== RC side:

            ┌─────────┐                    ┌────────────┐
 ┌─────┐    │         │ 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
                                           └────────────┘

Current dwc implimemnt, pci_fixup_addr() call back is needed when bus
fabric convert cpu address before send to PCIe controller.

    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>;
            ...
            };
    };

Device tree already can descript all address translate. Some hardware
driver implement fixup function by mask some bits of cpu address. Last
pci-imx6.c are little bit better by fetch memory resource's offset to do
fixup.

static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
{
	...
	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
	return cpu_addr - entry->offset;
}

But it is not good by using IORESOURCE_MEM to fix up io/cfg address map
although address translate is the same as IORESOURCE_MEM.

This patches to fetch untranslate range information for PCIe controller
(pcie@5f010000: ranges). So current config ATU without cpu_fixup_addr().

== EP side:

                   Endpoint
  ┌───────────────────────────────────────────────┐
  │                             pcie-ep@5f010000  │
  │                             ┌────────────────┐│
  │                             │   Endpoint     ││
  │                             │   PCIe         ││
  │                             │   Controller   ││
  │           bus@5f000000      │                ││
  │           ┌──────────┐      │                ││
  │           │          │ Outbound Transfer     ││
  │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
  ││     │    │  Fabric  │Bus   │                ││PCI Addr
  ││ CPU ├───►│          │Addr  │                ││0xA000_0000
  ││     │CPU │          │0x8000_0000            ││
  │└─────┘Addr└──────────┘      │                ││
  │       0x7000_0000           └────────────────┘│
  └───────────────────────────────────────────────┘

bus@5f000000 {
        compatible = "simple-bus";
        ranges = <0x80000000 0x0 0x70000000 0x10000000>;

        pcie-ep@5f010000 {
                reg = <0x5f010000 0x00010000>,
                      <0x80000000 0x10000000>;
                reg-names = "dbi", "addr_space";
                ...                ^^^^
        };
        ...
};

Add `bus_addr_base` to configure the outbound window address for CPU write.
The BUS fabric generally passes the same address to the PCIe EP controller,
but some BUS fabrics convert the address before sending it to the PCIe EP
controller.

Above diagram, CPU write data to outbound windows address 0x7000_0000,
Bus fabric convert it to 0x8000_0000. ATU should use BUS address
0x8000_0000 as input address and convert to PCI address 0xA000_0000.

Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
the device tree provides this information.

The both pave the road to eliminate ugle cpu_fixup_addr() callback function.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v9:
- There are some change in patches, if need drop review-tags, let me known.
- DTS part: https://lore.kernel.org/imx/20250128211559.1582598-2-Frank.Li@nxp.com/T/#u
- Keep "use_parent_dt_ranges" flags because need below combine logic

cpu_addr_fixup  use_parent_dt_ranges
NULL		X			No difference.
!NULL		true			Use device tree parent_address informaion [1]
!NULL		false			Keep use lagency method	[2]

Generally DTS is in different maintainer tree. It need two steps to cleanup
cpu_address_fixup() to avoid function block.
1. Update dts, which reflect the correct bus fabric behavior.
2. set "use_parent_dt_ranges" to true, then remove "cpu_address_fixup()" callback
in platform driver.

Bjorn's comments in https://lore.kernel.org/imx/20250123190900.GA650360@bhelgaas/

> After all cpu_address_fixup() removed, we can remove use_parent_dt_ranges
> in one clean up patches.
>
>
  ...
>  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.

io_base is parent_bus_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_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.

Fixed at this version.

- Link to v8: https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-0-c4bfa5193288@nxp.com

Changes in v8:
- Add mani's review tages
- use rename use_dt_ranges to use_parent_dt_ranges
- Add dev_warn_once to reminder to fix their dt file and remove
cpu_fixup_addr() callback.
- rename dw_pcie_get_untranslate_addr() to dw_pcie_get_parent_addr()
- Link to v7: https://lore.kernel.org/r/20241029-pci_fixup_addr-v7-0-8310dc24fb7c@nxp.com

Changes in v7:
- fix
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410291546.kvgEWJv7-lkp@intel.com/
- Link to v6: https://lore.kernel.org/r/20241028-pci_fixup_addr-v6-0-ebebcd8fd4ff@nxp.com

Changes in v6:
- merge RC and EP to one thread!
- Link to v5: https://lore.kernel.org/r/20241015-pci_fixup_addr-v5-0-ced556c85270@nxp.com

Changes in v5:
- update address order in diagram patches.
- remove confused 0x5f00_0000 range
- update patch1's commit message.
- Link to v4: https://lore.kernel.org/r/20241008-pci_fixup_addr-v4-0-25e5200657bc@nxp.com

Changes in v4:
- Improve commit message by add driver source code path.
- Link to v3: https://lore.kernel.org/r/20240930-pci_fixup_addr-v3-0-80ee70352fc7@nxp.com

Changes in v3:
- see each patch
- Link to v2: https://lore.kernel.org/r/20240926-pci_fixup_addr-v2-0-e4524541edf4@nxp.com

Changes in v2:
- see each patch
- Link to v1: https://lore.kernel.org/r/20240924-pci_fixup_addr-v1-0-57d14a91ec4f@nxp.com

---
Frank Li (7):
      PCI: dwc: Use resource start as iomap() input in dw_pcie_pme_turn_off()
      PCI: dwc: Rename cpu_addr to parent_bus_addr for ATU configuration
      PCI: Add parent_bus_offset to resource_entry
      PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
      PCI: dwc: ep: Add parent_bus_addr for outbound window
      PCI: dwc: ep: Ensure proper iteration over outbound map windows
      PCI: imx6: Remove cpu_addr_fixup()

 drivers/pci/bus.c                                 | 11 +++++-
 drivers/pci/controller/dwc/pci-imx6.c             | 18 +--------
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 29 +++++++++++---
 drivers/pci/controller/dwc/pcie-designware-host.c | 46 +++++++++++++++++++----
 drivers/pci/controller/dwc/pcie-designware.c      | 43 ++++++++++++---------
 drivers/pci/controller/dwc/pcie-designware.h      | 11 +++++-
 drivers/pci/of.c                                  | 12 +++++-
 include/linux/pci.h                               |  2 +
 include/linux/resource_ext.h                      |  1 +
 9 files changed, 121 insertions(+), 52 deletions(-)
---
base-commit: 5fcc194143ce5918ea0790a16323a844c5ab9499
change-id: 20240924-pci_fixup_addr-a8568f9bbb34

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v9 1/7] PCI: dwc: Use resource start as iomap() input in dw_pcie_pme_turn_off()

[Frank Li]

Pass resource start as the input argument to iomap() instead of
atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
be the same here, it actually represents the parent bus address before ATU,
which may not always align with the CPU address. Using resource start
ensures correctness and clarity.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v9 to v10
- new patch
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ffaded8f2df7b..ae3fd2a5dbf85 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
 	if (ret)
 		return ret;
 
-	mem = ioremap(atu.cpu_addr, pci->region_align);
+	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
 	if (!mem)
 		return -ENOMEM;
 

-- 
2.34.1

Re: [PATCH v9 1/7] PCI: dwc: Use resource start as iomap() input in dw_pcie_pme_turn_off()

[Bjorn Helgaas]

On Tue, Jan 28, 2025 at 05:07:34PM -0500, Frank Li wrote:
> Pass resource start as the input argument to iomap() instead of
> atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
> be the same here, it actually represents the parent bus address before ATU,
> which may not always align with the CPU address. Using resource start
> ensures correctness and clarity.

1) "atu.cpu_address happens to be the same here" is currently true
   but only because this function is buggy and assumes the ATU input
   address is the same as a CPU physical address.

2) We're trying to make a correctness fix, not a clarity fix.  This
   patch by itself isn't a functional change because of the cpu_addr
   bug, but without this patch, fixing the cpu_addr bug would break
   the iomap.

3) "Pass resource start as the input to iomap()" just restates the
   patch.  We should say *why* this is important.  Something like
   this:

     The msg_res region translates writes into PCIe Message TLPs.
     Previously we mapped this region using atu.cpu_addr, the input
     address programmed into the ATU.

     "cpu_addr" is a misnomer because when a bus fabric translates
     addresses between the CPU and the ATU, the ATU input address is
     different from the CPU address.  A future patch will rename
     "cpu_addr" and correct the value to be the ATU input address
     instead of the CPU physical address.

     Map the msg_res region before writing to it using the msg_res
     resource start, a CPU physical address.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change from v9 to v10
> - new patch
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ffaded8f2df7b..ae3fd2a5dbf85 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  	if (ret)
>  		return ret;
>  
> -	mem = ioremap(atu.cpu_addr, pci->region_align);
> +	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
>  	if (!mem)
>  		return -ENOMEM;
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v9 1/7] PCI: dwc: Use resource start as iomap() input in dw_pcie_pme_turn_off()

[Frank Li]

On Wed, Jan 29, 2025 at 05:19:37PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 05:07:34PM -0500, Frank Li wrote:
> > Pass resource start as the input argument to iomap() instead of
> > atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
> > be the same here, it actually represents the parent bus address before ATU,
> > which may not always align with the CPU address. Using resource start
> > ensures correctness and clarity.
>
> 1) "atu.cpu_address happens to be the same here" is currently true
>    but only because this function is buggy and assumes the ATU input
>    address is the same as a CPU physical address.
>
> 2) We're trying to make a correctness fix, not a clarity fix.  This
>    patch by itself isn't a functional change because of the cpu_addr
>    bug, but without this patch, fixing the cpu_addr bug would break
>    the iomap.
>
> 3) "Pass resource start as the input to iomap()" just restates the
>    patch.  We should say *why* this is important.  Something like
>    this:
>
>      The msg_res region translates writes into PCIe Message TLPs.
>      Previously we mapped this region using atu.cpu_addr, the input
>      address programmed into the ATU.
>
>      "cpu_addr" is a misnomer because when a bus fabric translates
>      addresses between the CPU and the ATU, the ATU input address is
>      different from the CPU address.  A future patch will rename
>      "cpu_addr" and correct the value to be the ATU input address
>      instead of the CPU physical address.
>
>      Map the msg_res region before writing to it using the msg_res
>      resource start, a CPU physical address.

Thanks, will update commit message. The key change is patch 3

PCI: Add parent_bus_offset to resource_entry

Frank

>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v9 to v10
> > - new patch
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index ffaded8f2df7b..ae3fd2a5dbf85 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
> >  	if (ret)
> >  		return ret;
> >
> > -	mem = ioremap(atu.cpu_addr, pci->region_align);
> > +	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
> >  	if (!mem)
> >  		return -ENOMEM;
> >
> >
> > --
> > 2.34.1
> >

[PATCH v9 2/7] PCI: dwc: Rename cpu_addr to parent_bus_addr for ATU configuration

[Frank Li]

Rename `cpu_addr` to `parent_bus_addr` in the DesignWare ATU configuration.
The ATU translates parent bus addresses to PCI addresses, which are often
the same as CPU addresses but can differ in systems where the bus fabric
translates addresses before passing them to the PCIe controller. This
renaming clarifies the purpose and avoids confusion.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v9 to v10
- new patch
---
 drivers/pci/controller/dwc/pcie-designware-ep.c   |  8 +++---
 drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++----
 drivers/pci/controller/dwc/pcie-designware.c      | 34 +++++++++++------------
 drivers/pci/controller/dwc/pcie-designware.h      |  2 +-
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 8e07d432e74f2..80ac2f9e88eb5 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -128,7 +128,7 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 }
 
 static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
-				  dma_addr_t cpu_addr, enum pci_barno bar,
+				  dma_addr_t parent_bus_addr, enum pci_barno bar,
 				  size_t size)
 {
 	int ret;
@@ -146,7 +146,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
 	}
 
 	ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
-					  cpu_addr, bar, size);
+					  parent_bus_addr, bar, size);
 	if (ret < 0) {
 		dev_err(pci->dev, "Failed to program IB window\n");
 		return ret;
@@ -181,7 +181,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep,
 		return ret;
 
 	set_bit(free_win, ep->ob_window_map);
-	ep->outbound_addr[free_win] = atu->cpu_addr;
+	ep->outbound_addr[free_win] = atu->parent_bus_addr;
 
 	return 0;
 }
@@ -333,7 +333,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 
 	atu.func_no = func_no;
 	atu.type = PCIE_ATU_TYPE_MEM;
-	atu.cpu_addr = addr;
+	atu.parent_bus_addr = addr;
 	atu.pci_addr = pci_addr;
 	atu.size = size;
 	ret = dw_pcie_ep_outbound_atu(ep, &atu);
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ae3fd2a5dbf85..1206b26bff3f2 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -616,7 +616,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
 		type = PCIE_ATU_TYPE_CFG1;
 
 	atu.type = type;
-	atu.cpu_addr = pp->cfg0_base;
+	atu.parent_bus_addr = pp->cfg0_base;
 	atu.pci_addr = busdev;
 	atu.size = pp->cfg0_size;
 
@@ -641,7 +641,7 @@ static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
 
 	if (pp->cfg0_io_shared) {
 		atu.type = PCIE_ATU_TYPE_IO;
-		atu.cpu_addr = pp->io_base;
+		atu.parent_bus_addr = pp->io_base;
 		atu.pci_addr = pp->io_bus_addr;
 		atu.size = pp->io_size;
 
@@ -667,7 +667,7 @@ static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
 
 	if (pp->cfg0_io_shared) {
 		atu.type = PCIE_ATU_TYPE_IO;
-		atu.cpu_addr = pp->io_base;
+		atu.parent_bus_addr = pp->io_base;
 		atu.pci_addr = pp->io_bus_addr;
 		atu.size = pp->io_size;
 
@@ -736,7 +736,7 @@ 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;
+		atu.parent_bus_addr = entry->res->start;
 		atu.pci_addr = entry->res->start - entry->offset;
 
 		/* Adjust iATU size if MSG TLP region was allocated before */
@@ -758,7 +758,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 		if (pci->num_ob_windows > ++i) {
 			atu.index = i;
 			atu.type = PCIE_ATU_TYPE_IO;
-			atu.cpu_addr = pp->io_base;
+			atu.parent_bus_addr = pp->io_base;
 			atu.pci_addr = pp->io_bus_addr;
 			atu.size = pp->io_size;
 
@@ -902,7 +902,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
 	atu.size = resource_size(pci->pp.msg_res);
 	atu.index = pci->pp.msg_atu_index;
 
-	atu.cpu_addr = pci->pp.msg_res->start;
+	atu.parent_bus_addr = pci->pp.msg_res->start;
 
 	ret = dw_pcie_prog_outbound_atu(pci, &atu);
 	if (ret)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 145e7f579072c..9d0a5f75effcc 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -470,25 +470,25 @@ static inline u32 dw_pcie_enable_ecrc(u32 val)
 int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 			      const struct dw_pcie_ob_atu_cfg *atu)
 {
-	u64 cpu_addr = atu->cpu_addr;
+	u64 parent_bus_addr = atu->parent_bus_addr;
 	u32 retries, val;
 	u64 limit_addr;
 
 	if (pci->ops && pci->ops->cpu_addr_fixup)
-		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
+		parent_bus_addr = pci->ops->cpu_addr_fixup(pci, parent_bus_addr);
 
-	limit_addr = cpu_addr + atu->size - 1;
+	limit_addr = parent_bus_addr + atu->size - 1;
 
-	if ((limit_addr & ~pci->region_limit) != (cpu_addr & ~pci->region_limit) ||
-	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
+	if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||
+	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
 	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
 		return -EINVAL;
 	}
 
 	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
-			      lower_32_bits(cpu_addr));
+			      lower_32_bits(parent_bus_addr));
 	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_BASE,
-			      upper_32_bits(cpu_addr));
+			      upper_32_bits(parent_bus_addr));
 
 	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LIMIT,
 			      lower_32_bits(limit_addr));
@@ -502,7 +502,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 			      upper_32_bits(atu->pci_addr));
 
 	val = atu->type | atu->routing | PCIE_ATU_FUNC_NUM(atu->func_no);
-	if (upper_32_bits(limit_addr) > upper_32_bits(cpu_addr) &&
+	if (upper_32_bits(limit_addr) > upper_32_bits(parent_bus_addr) &&
 	    dw_pcie_ver_is_ge(pci, 460A))
 		val |= PCIE_ATU_INCREASE_REGION_SIZE;
 	if (dw_pcie_ver_is(pci, 490A))
@@ -545,13 +545,13 @@ static inline void dw_pcie_writel_atu_ib(struct dw_pcie *pci, u32 index, u32 reg
 }
 
 int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
-			     u64 cpu_addr, u64 pci_addr, u64 size)
+			     u64 parent_bus_addr, u64 pci_addr, u64 size)
 {
 	u64 limit_addr = pci_addr + size - 1;
 	u32 retries, val;
 
 	if ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit) ||
-	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
+	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
 	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
 		return -EINVAL;
 	}
@@ -568,9 +568,9 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
 				      upper_32_bits(limit_addr));
 
 	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
-			      lower_32_bits(cpu_addr));
+			      lower_32_bits(parent_bus_addr));
 	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
-			      upper_32_bits(cpu_addr));
+			      upper_32_bits(parent_bus_addr));
 
 	val = type;
 	if (upper_32_bits(limit_addr) > upper_32_bits(pci_addr) &&
@@ -597,18 +597,18 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
 }
 
 int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-				int type, u64 cpu_addr, u8 bar, size_t size)
+				int type, u64 parent_bus_addr, u8 bar, size_t size)
 {
 	u32 retries, val;
 
-	if (!IS_ALIGNED(cpu_addr, pci->region_align) ||
-	    !IS_ALIGNED(cpu_addr, size))
+	if (!IS_ALIGNED(parent_bus_addr, pci->region_align) ||
+	    !IS_ALIGNED(parent_bus_addr, size))
 		return -EINVAL;
 
 	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
-			      lower_32_bits(cpu_addr));
+			      lower_32_bits(parent_bus_addr));
 	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
-			      upper_32_bits(cpu_addr));
+			      upper_32_bits(parent_bus_addr));
 
 	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL1, type |
 			      PCIE_ATU_FUNC_NUM(func_no));
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 501d9ddfea163..ac23604c829f4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -343,7 +343,7 @@ struct dw_pcie_ob_atu_cfg {
 	u8 func_no;
 	u8 code;
 	u8 routing;
-	u64 cpu_addr;
+	u64 parent_bus_addr;
 	u64 pci_addr;
 	u64 size;
 };

-- 
2.34.1

Re: [PATCH v9 2/7] PCI: dwc: Rename cpu_addr to parent_bus_addr for ATU configuration

[Bjorn Helgaas]

On Tue, Jan 28, 2025 at 05:07:35PM -0500, Frank Li wrote:
> Rename `cpu_addr` to `parent_bus_addr` in the DesignWare ATU configuration.
> The ATU translates parent bus addresses to PCI addresses, which are often
> the same as CPU addresses but can differ in systems where the bus fabric
> translates addresses before passing them to the PCIe controller. This
> renaming clarifies the purpose and avoids confusion.

Based on dw_pcie_ep_inbound_atu() below, I guess the ATU can also
translate PCI addresses from incoming DMA to parent bus addresses?

It's worth noting here that this patch only renames the member, and
IIUC, parent_bus_addr still incorrectly contains CPU physical
addresses.

> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -128,7 +128,7 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  }
>  
>  static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> -				  dma_addr_t cpu_addr, enum pci_barno bar,
> +				  dma_addr_t parent_bus_addr, enum pci_barno bar,
>  				  size_t size)

Re: [PATCH v9 2/7] PCI: dwc: Rename cpu_addr to parent_bus_addr for ATU configuration

[Frank Li]

On Wed, Jan 29, 2025 at 05:23:50PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 05:07:35PM -0500, Frank Li wrote:
> > Rename `cpu_addr` to `parent_bus_addr` in the DesignWare ATU configuration.
> > The ATU translates parent bus addresses to PCI addresses, which are often
> > the same as CPU addresses but can differ in systems where the bus fabric
> > translates addresses before passing them to the PCIe controller. This
> > renaming clarifies the purpose and avoids confusion.
>
> Based on dw_pcie_ep_inbound_atu() below, I guess the ATU can also
> translate PCI addresses from incoming DMA to parent bus addresses?

Yes, but root complex don't use it. Only EP use it. because most PCI root
complex system doesn't transfer incoming address, which generally use iommu
to do that. Linux already allow a simple map by use dt's dma-ranges and dma
API already handle it.

previous 'cpu_addr' is actually 'dma_bus_addr'.

>
> It's worth noting here that this patch only renames the member, and
> IIUC, parent_bus_addr still incorrectly contains CPU physical
> addresses.

Anyway, call 'cpu_addr' for dw_pcie_ep_inbound_atu is wrong. Only one place
call dw_pcie_ep_inbound_atu(), that's dw_pcie_ep_set_bar(), which use
epf_bar->phys_addr, I think name 'phys_addr' is okay because most case it
is refer to dma address space.

Frank
>
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -128,7 +128,7 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >  }
> >
> >  static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > -				  dma_addr_t cpu_addr, enum pci_barno bar,
> > +				  dma_addr_t parent_bus_addr, enum pci_barno bar,
> >  				  size_t size)

Re: [PATCH v9 2/7] PCI: dwc: Rename cpu_addr to parent_bus_addr for ATU configuration

[Frank Li]

On Thu, Jan 30, 2025 at 11:02:06AM -0500, Frank Li wrote:
> On Wed, Jan 29, 2025 at 05:23:50PM -0600, Bjorn Helgaas wrote:
> > On Tue, Jan 28, 2025 at 05:07:35PM -0500, Frank Li wrote:
> > > Rename `cpu_addr` to `parent_bus_addr` in the DesignWare ATU configuration.
> > > The ATU translates parent bus addresses to PCI addresses, which are often
> > > the same as CPU addresses but can differ in systems where the bus fabric
> > > translates addresses before passing them to the PCIe controller. This
> > > renaming clarifies the purpose and avoids confusion.
> >
> > Based on dw_pcie_ep_inbound_atu() below, I guess the ATU can also
> > translate PCI addresses from incoming DMA to parent bus addresses?
>
> Yes, but root complex don't use it. Only EP use it. because most PCI root
> complex system doesn't transfer incoming address, which generally use iommu
> to do that. Linux already allow a simple map by use dt's dma-ranges and dma
> API already handle it.
>
> previous 'cpu_addr' is actually 'dma_bus_addr'.
>
> >
> > It's worth noting here that this patch only renames the member, and
> > IIUC, parent_bus_addr still incorrectly contains CPU physical
> > addresses.
>
> Anyway, call 'cpu_addr' for dw_pcie_ep_inbound_atu is wrong. Only one place
> call dw_pcie_ep_inbound_atu(), that's dw_pcie_ep_set_bar(), which use
> epf_bar->phys_addr, I think name 'phys_addr' is okay because most case it
> is refer to dma address space.

Bjoin:
	Do you any concern/comments about my reply?  how to move forward?
keep old cpu_addr or rename to parent_bus_addr, or futher work need be done.

Frank

>
> Frank
> >
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -128,7 +128,7 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > >  }
> > >
> > >  static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > > -				  dma_addr_t cpu_addr, enum pci_barno bar,
> > > +				  dma_addr_t parent_bus_addr, enum pci_barno bar,
> > >  				  size_t size)

[PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Frank Li]

Introduce `parent_bus_offset` in `resource_entry` and a new API,
`pci_add_resource_parent_bus_offset()`, to provide necessary information
for PCI controllers with address translation units.

Typical PCI data flow involves:
  CPU (CPU address) -> Bus Fabric (Intermediate address) ->
  PCI Controller (PCI bus address) -> PCI Bus.

While most bus fabrics preserve address consistency, some modify addresses
to intermediate values. The `parent_bus_offset` enables PCI controllers to
translate these intermediate addresses correctly to PCI bus addresses.

Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
PCI controller drivers.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v9 to v10
- new patch
---
 drivers/pci/bus.c            | 11 +++++++++--
 drivers/pci/of.c             | 12 +++++++++++-
 include/linux/pci.h          |  2 ++
 include/linux/resource_ext.h |  1 +
 4 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 98910bc0fcc4e..52e88c391e256 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -31,8 +31,8 @@ struct pci_bus_resource {
 	struct resource		*res;
 };
 
-void pci_add_resource_offset(struct list_head *resources, struct resource *res,
-			     resource_size_t offset)
+void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res,
+					resource_size_t offset, resource_size_t parent_bus_offset)
 {
 	struct resource_entry *entry;
 
@@ -43,8 +43,15 @@ void pci_add_resource_offset(struct list_head *resources, struct resource *res,
 	}
 
 	entry->offset = offset;
+	entry->parent_bus_offset = parent_bus_offset;
 	resource_list_add_tail(entry, resources);
 }
+
+void pci_add_resource_offset(struct list_head *resources, struct resource *res,
+			     resource_size_t offset)
+{
+	pci_add_resource_parent_bus_offset(resources, res, offset, 0);
+}
 EXPORT_SYMBOL(pci_add_resource_offset);
 
 void pci_add_resource(struct list_head *resources, struct resource *res)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 7a806f5c0d201..aa4a2e266c55e 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
 			res->flags &= ~IORESOURCE_MEM_64;
 		}
 
-		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
+		/*
+		 * IORESOURCE_IO res->start is io space start address.
+		 * IORESOURCE_MEM res->start is cpu start address, which is the
+		 * same as range.cpu_addr.
+		 *
+		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
+		 * IO and MEM's parent_bus_offset always offset to cpu address.
+		 */
+
+		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
+						   range.cpu_addr - range.parent_bus_addr);
 	}
 
 	/* Check for dma-ranges property */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 47b31ad724fa5..0d7e67b47be47 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev,
 void pci_add_resource(struct list_head *resources, struct resource *res);
 void pci_add_resource_offset(struct list_head *resources, struct resource *res,
 			     resource_size_t offset);
+void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res,
+					resource_size_t offset, resource_size_t parent_bus_offset);
 void pci_free_resource_list(struct list_head *resources);
 void pci_bus_add_resource(struct pci_bus *bus, struct resource *res);
 struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n);
diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
index ff0339df56afc..b6ec6cc318203 100644
--- a/include/linux/resource_ext.h
+++ b/include/linux/resource_ext.h
@@ -24,6 +24,7 @@ struct resource_entry {
 	struct list_head	node;
 	struct resource		*res;	/* In master (CPU) address space */
 	resource_size_t		offset;	/* Translation offset for bridge */
+	resource_size_t		parent_bus_offset; /* Parent bus address offset for bridge */
 	struct resource		__res;	/* Default storage for res */
 };
 

-- 
2.34.1

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Frank Li]

On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> Introduce `parent_bus_offset` in `resource_entry` and a new API,
> `pci_add_resource_parent_bus_offset()`, to provide necessary information
> for PCI controllers with address translation units.
>
> Typical PCI data flow involves:
>   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
>   PCI Controller (PCI bus address) -> PCI Bus.
>
> While most bus fabrics preserve address consistency, some modify addresses
> to intermediate values. The `parent_bus_offset` enables PCI controllers to
> translate these intermediate addresses correctly to PCI bus addresses.
>
> Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> PCI controller drivers.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---


Bjorn:

	How do you think this method to handle parent_bus_offset? so other
pci host rc driver should be easy to use it.

Frank

> change from v9 to v10
> - new patch
> ---
>  drivers/pci/bus.c            | 11 +++++++++--
>  drivers/pci/of.c             | 12 +++++++++++-
>  include/linux/pci.h          |  2 ++
>  include/linux/resource_ext.h |  1 +
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 98910bc0fcc4e..52e88c391e256 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -31,8 +31,8 @@ struct pci_bus_resource {
>  	struct resource		*res;
>  };
>
> -void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> -			     resource_size_t offset)
> +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res,
> +					resource_size_t offset, resource_size_t parent_bus_offset)
>  {
>  	struct resource_entry *entry;
>
> @@ -43,8 +43,15 @@ void pci_add_resource_offset(struct list_head *resources, struct resource *res,
>  	}
>
>  	entry->offset = offset;
> +	entry->parent_bus_offset = parent_bus_offset;
>  	resource_list_add_tail(entry, resources);
>  }
> +
> +void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> +			     resource_size_t offset)
> +{
> +	pci_add_resource_parent_bus_offset(resources, res, offset, 0);
> +}
>  EXPORT_SYMBOL(pci_add_resource_offset);
>
>  void pci_add_resource(struct list_head *resources, struct resource *res)
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 7a806f5c0d201..aa4a2e266c55e 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
>  			res->flags &= ~IORESOURCE_MEM_64;
>  		}
>
> -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> +		/*
> +		 * IORESOURCE_IO res->start is io space start address.
> +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> +		 * same as range.cpu_addr.
> +		 *
> +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> +		 */
> +
> +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> +						   range.cpu_addr - range.parent_bus_addr);
>  	}
>
>  	/* Check for dma-ranges property */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa5..0d7e67b47be47 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev,
>  void pci_add_resource(struct list_head *resources, struct resource *res);
>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
>  			     resource_size_t offset);
> +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res,
> +					resource_size_t offset, resource_size_t parent_bus_offset);
>  void pci_free_resource_list(struct list_head *resources);
>  void pci_bus_add_resource(struct pci_bus *bus, struct resource *res);
>  struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n);
> diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
> index ff0339df56afc..b6ec6cc318203 100644
> --- a/include/linux/resource_ext.h
> +++ b/include/linux/resource_ext.h
> @@ -24,6 +24,7 @@ struct resource_entry {
>  	struct list_head	node;
>  	struct resource		*res;	/* In master (CPU) address space */
>  	resource_size_t		offset;	/* Translation offset for bridge */
> +	resource_size_t		parent_bus_offset; /* Parent bus address offset for bridge */
>  	struct resource		__res;	/* Default storage for res */
>  };
>
>
> --
> 2.34.1
>

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Bjorn Helgaas]

On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> Introduce `parent_bus_offset` in `resource_entry` and a new API,
> `pci_add_resource_parent_bus_offset()`, to provide necessary information
> for PCI controllers with address translation units.
> 
> Typical PCI data flow involves:
>   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
>   PCI Controller (PCI bus address) -> PCI Bus.
> 
> While most bus fabrics preserve address consistency, some modify addresses
> to intermediate values. 

s/modify/translate/

Specifically, they *translate* addresses, which means the same offset
is added to every address in the range, as opposed to masking or some
other transformation.

I think we can take advantage of this to simplify the callers of
.cpu_addr_fixup() later.

Ironically, most of the .cpu_addr_fixup() implementations *do* mask
the address, e.g., cdns_plat_cpu_addr_fixup() masks with 0x0fffffff.
But I think this is actually incorrect because masking results in a
many-to-one mapping, e.g.,

  0x42000000 & 0x0fffffff == 0x02000000
  0x52000000 & 0x0fffffff == 0x02000000

But presumably the addresses we pass to cdns_plat_cpu_addr_fixup()
don't cross a 256MB (0x10000000) boundary, so we could accomplish the
same by subtracting 0x40000000:

  0x42000000 - 0x40000000 == 0x02000000

> +++ b/drivers/pci/of.c
> @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
>  			res->flags &= ~IORESOURCE_MEM_64;
>  		}
>  
> -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> +		/*
> +		 * IORESOURCE_IO res->start is io space start address.
> +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> +		 * same as range.cpu_addr.
> +		 *
> +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> +		 */
> +
> +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> +						   range.cpu_addr - range.parent_bus_addr);

Wrap to fit in 80 columns like the rest of the file.  Will have to
unindent the two lines of arguments to make this work.

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Bjorn Helgaas]

On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> Introduce `parent_bus_offset` in `resource_entry` and a new API,
> `pci_add_resource_parent_bus_offset()`, to provide necessary information
> for PCI controllers with address translation units.
> 
> Typical PCI data flow involves:
>   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
>   PCI Controller (PCI bus address) -> PCI Bus.
> 
> While most bus fabrics preserve address consistency, some modify addresses
> to intermediate values. The `parent_bus_offset` enables PCI controllers to
> translate these intermediate addresses correctly to PCI bus addresses.
> 
> Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> PCI controller drivers.
> ...

> +++ b/drivers/pci/of.c
> @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
>  			res->flags &= ~IORESOURCE_MEM_64;
>  		}
>  
> -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> +		/*
> +		 * IORESOURCE_IO res->start is io space start address.
> +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> +		 * same as range.cpu_addr.
> +		 *
> +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> +		 */
> +
> +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> +						   range.cpu_addr - range.parent_bus_addr);

I don't know exactly where it needs to go, but I think we can call
.cpu_addr_fixup() once at startup on the base of the region.  This
will tell us the offset that applies to the entire region, i.e.,
parent_bus_offset.

Then we can remove all the .cpu_addr_fixup() calls in
cdns_pcie_host_init_address_translation(),
cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().

Until we can get rid of all the .cpu_addr_fixup() implementations,
We'll still have that single call at startup (I guess once for cadence
and another for designware), but it should simplify the current
callers quite a bit.

>  	}
>  
>  	/* Check for dma-ranges property */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa5..0d7e67b47be47 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev,
>  void pci_add_resource(struct list_head *resources, struct resource *res);
>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
>  			     resource_size_t offset);
> +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res,
> +					resource_size_t offset, resource_size_t parent_bus_offset);
>  void pci_free_resource_list(struct list_head *resources);
>  void pci_bus_add_resource(struct pci_bus *bus, struct resource *res);
>  struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n);
> diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
> index ff0339df56afc..b6ec6cc318203 100644
> --- a/include/linux/resource_ext.h
> +++ b/include/linux/resource_ext.h
> @@ -24,6 +24,7 @@ struct resource_entry {
>  	struct list_head	node;
>  	struct resource		*res;	/* In master (CPU) address space */
>  	resource_size_t		offset;	/* Translation offset for bridge */
> +	resource_size_t		parent_bus_offset; /* Parent bus address offset for bridge */
>  	struct resource		__res;	/* Default storage for res */
>  };
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Frank Li]

On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> > Introduce `parent_bus_offset` in `resource_entry` and a new API,
> > `pci_add_resource_parent_bus_offset()`, to provide necessary information
> > for PCI controllers with address translation units.
> >
> > Typical PCI data flow involves:
> >   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
> >   PCI Controller (PCI bus address) -> PCI Bus.
> >
> > While most bus fabrics preserve address consistency, some modify addresses
> > to intermediate values. The `parent_bus_offset` enables PCI controllers to
> > translate these intermediate addresses correctly to PCI bus addresses.
> >
> > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> > PCI controller drivers.
> > ...
>
> > +++ b/drivers/pci/of.c
> > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> >  			res->flags &= ~IORESOURCE_MEM_64;
> >  		}
> >
> > -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> > +		/*
> > +		 * IORESOURCE_IO res->start is io space start address.
> > +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> > +		 * same as range.cpu_addr.
> > +		 *
> > +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> > +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> > +		 */
> > +
> > +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> > +						   range.cpu_addr - range.parent_bus_addr);
>
> I don't know exactly where it needs to go, but I think we can call
> .cpu_addr_fixup() once at startup on the base of the region.  This
> will tell us the offset that applies to the entire region, i.e.,
> parent_bus_offset.
>
> Then we can remove all the .cpu_addr_fixup() calls in
> cdns_pcie_host_init_address_translation(),
> cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().
>
> Until we can get rid of all the .cpu_addr_fixup() implementations,
> We'll still have that single call at startup (I guess once for cadence
> and another for designware), but it should simplify the current
> callers quite a bit.

I don't think it can simple code. cdns_pcie_set_outbound_region() and
dw_pcie_prog_outbound_atu() are called by EP functions, which have not use
"resource" to manage outbound windows. And there are IO/CFG/MEM space at
host side, It will involve more .cpu_addr_fixup() calls to setup these
range.

Frank
>
> >  	}
> >
> >  	/* Check for dma-ranges property */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 47b31ad724fa5..0d7e67b47be47 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1510,6 +1510,8 @@ static inline void pci_release_config_region(struct pci_dev *pdev,
> >  void pci_add_resource(struct list_head *resources, struct resource *res);
> >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> >  			     resource_size_t offset);
> > +void pci_add_resource_parent_bus_offset(struct list_head *resources, struct resource *res,
> > +					resource_size_t offset, resource_size_t parent_bus_offset);
> >  void pci_free_resource_list(struct list_head *resources);
> >  void pci_bus_add_resource(struct pci_bus *bus, struct resource *res);
> >  struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n);
> > diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
> > index ff0339df56afc..b6ec6cc318203 100644
> > --- a/include/linux/resource_ext.h
> > +++ b/include/linux/resource_ext.h
> > @@ -24,6 +24,7 @@ struct resource_entry {
> >  	struct list_head	node;
> >  	struct resource		*res;	/* In master (CPU) address space */
> >  	resource_size_t		offset;	/* Translation offset for bridge */
> > +	resource_size_t		parent_bus_offset; /* Parent bus address offset for bridge */
> >  	struct resource		__res;	/* Default storage for res */
> >  };
> >
> >
> > --
> > 2.34.1
> >

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Bjorn Helgaas]

On Mon, Mar 03, 2025 at 04:57:29PM -0500, Frank Li wrote:
> On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote:
> > On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> > > Introduce `parent_bus_offset` in `resource_entry` and a new API,
> > > `pci_add_resource_parent_bus_offset()`, to provide necessary information
> > > for PCI controllers with address translation units.
> > >
> > > Typical PCI data flow involves:
> > >   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
> > >   PCI Controller (PCI bus address) -> PCI Bus.
> > >
> > > While most bus fabrics preserve address consistency, some modify addresses
> > > to intermediate values. The `parent_bus_offset` enables PCI controllers to
> > > translate these intermediate addresses correctly to PCI bus addresses.
> > >
> > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> > > PCI controller drivers.
> > > ...
> >
> > > +++ b/drivers/pci/of.c
> > > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> > >  			res->flags &= ~IORESOURCE_MEM_64;
> > >  		}
> > >
> > > -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> > > +		/*
> > > +		 * IORESOURCE_IO res->start is io space start address.
> > > +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> > > +		 * same as range.cpu_addr.
> > > +		 *
> > > +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> > > +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> > > +		 */
> > > +
> > > +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> > > +						   range.cpu_addr - range.parent_bus_addr);
> >
> > I don't know exactly where it needs to go, but I think we can call
> > .cpu_addr_fixup() once at startup on the base of the region.  This
> > will tell us the offset that applies to the entire region, i.e.,
> > parent_bus_offset.
> >
> > Then we can remove all the .cpu_addr_fixup() calls in
> > cdns_pcie_host_init_address_translation(),
> > cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().
> >
> > Until we can get rid of all the .cpu_addr_fixup() implementations,
> > We'll still have that single call at startup (I guess once for cadence
> > and another for designware), but it should simplify the current
> > callers quite a bit.
> 
> I don't think it can simple code. cdns_pcie_set_outbound_region() and
> dw_pcie_prog_outbound_atu() are called by EP functions, which have not use
> "resource" to manage outbound windows. 

Let's ignore cadence for now.  I don't think we need to solve that
until later.

dw_pcie_prog_outbound_atu() is called by:

  - dw_pcie_other_conf_map_bus(): atu.parent_bus_addr = pp->cfg0_base

    I think dw_pcie_host_init() can set pp->cfg0_base with the correct
    intermediate address, either via the the of_property_read_reg() or
    .cpu_addr_fixup().

    If dw_pcie_host_init() does this, then we don't need
    .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().

  - dw_pcie_rd_other_conf(): atu.parent_bus_addr = pp->io_base

    Similarly, dw_pcie_host_init() should be able to set pp->io_base
    to the intermediate address, so we don't need .cpu_addr_fixup() in
    dw_pcie_prog_outbound_atu().

  - dw_pcie_iatu_setup(): atu.parent_bus_addr = entry->res->start

    Here "entry" iterates through bridge->windows, and we should be
    able to set entry->parent_bus_offset at init-time, using
    .cpu_addr_fixup() if necessary, so we can apply that offset
    unconditionally, regardless of use_parent_dt_ranges, and we won't
    need .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().

  - dw_pcie_pme_turn_off:
    atu.parent_bus_addr = pci->pp.msg_res->start - pci->pp.msg_parent_bus_offset

    This should be the same as dw_pcie_iatu_setup() since
    msg_parent_bus_offset comes from the window iteration in
    dw_pcie_host_request_msg_tlp_res().  As long as the windows have
    the correct parent_bus_offset at init-time, we should be all set.

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Frank Li]

On Tue, Mar 04, 2025 at 11:50:10AM -0600, Bjorn Helgaas wrote:
> On Mon, Mar 03, 2025 at 04:57:29PM -0500, Frank Li wrote:
> > On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> > > > Introduce `parent_bus_offset` in `resource_entry` and a new API,
> > > > `pci_add_resource_parent_bus_offset()`, to provide necessary information
> > > > for PCI controllers with address translation units.
> > > >
> > > > Typical PCI data flow involves:
> > > >   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
> > > >   PCI Controller (PCI bus address) -> PCI Bus.
> > > >
> > > > While most bus fabrics preserve address consistency, some modify addresses
> > > > to intermediate values. The `parent_bus_offset` enables PCI controllers to
> > > > translate these intermediate addresses correctly to PCI bus addresses.
> > > >
> > > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> > > > PCI controller drivers.
> > > > ...
> > >
> > > > +++ b/drivers/pci/of.c
> > > > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> > > >  			res->flags &= ~IORESOURCE_MEM_64;
> > > >  		}
> > > >
> > > > -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> > > > +		/*
> > > > +		 * IORESOURCE_IO res->start is io space start address.
> > > > +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> > > > +		 * same as range.cpu_addr.
> > > > +		 *
> > > > +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> > > > +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> > > > +		 */
> > > > +
> > > > +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> > > > +						   range.cpu_addr - range.parent_bus_addr);
> > >
> > > I don't know exactly where it needs to go, but I think we can call
> > > .cpu_addr_fixup() once at startup on the base of the region.  This
> > > will tell us the offset that applies to the entire region, i.e.,
> > > parent_bus_offset.
> > >
> > > Then we can remove all the .cpu_addr_fixup() calls in
> > > cdns_pcie_host_init_address_translation(),
> > > cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().
> > >
> > > Until we can get rid of all the .cpu_addr_fixup() implementations,
> > > We'll still have that single call at startup (I guess once for cadence
> > > and another for designware), but it should simplify the current
> > > callers quite a bit.
> >
> > I don't think it can simple code. cdns_pcie_set_outbound_region() and
> > dw_pcie_prog_outbound_atu() are called by EP functions, which have not use
> > "resource" to manage outbound windows.
>
> Let's ignore cadence for now.  I don't think we need to solve that
> until later.
>
> dw_pcie_prog_outbound_atu() is called by:
>
>   - dw_pcie_other_conf_map_bus(): atu.parent_bus_addr = pp->cfg0_base
>
>     I think dw_pcie_host_init() can set pp->cfg0_base with the correct
>     intermediate address, either via the the of_property_read_reg() or
>     .cpu_addr_fixup().
>
>     If dw_pcie_host_init() does this, then we don't need
>     .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().
>
>   - dw_pcie_rd_other_conf(): atu.parent_bus_addr = pp->io_base
>
>     Similarly, dw_pcie_host_init() should be able to set pp->io_base
>     to the intermediate address, so we don't need .cpu_addr_fixup() in
>     dw_pcie_prog_outbound_atu().

I found some driver's cpu_addr_fixup()'s implement depend on the
initilize sequence.

for example:
	pcie-artpec6.c

static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
{
        struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
        struct dw_pcie_rp *pp = &pci->pp;
        struct dw_pcie_ep *ep = &pci->ep;

        switch (artpec6_pcie->mode) {
        case DW_PCIE_RC_TYPE:
                return cpu_addr - pp->cfg0_base;
        case DW_PCIE_EP_TYPE:
                return cpu_addr - ep->phys_base;
        default:
                dev_err(pci->dev, "UNKNOWN device type\n");
        }
        return cpu_addr;
}

This implement require *cfg0_base* and *phys_base*, pp/ep, need set before
call artpec6_pcie_cpu_addr_fixup().

static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
{
        struct dw_pcie_rp *pp = &pci->pp;

        return cpu_addr & ~pp->io_base;
}

this one require *io_base* and *pp* need be set before call
visconti_pcie_cpu_addr_fixup()

Because I have not such hardware platform, it is not trivial change and
it is hard to involve bugs.

If move .cpu_addr_fixup() too early, it will cause kernel dump.

I suggest keep current overall sequent and try to clean up these driver's
cpu_addr_fixup() firstly.

Frank

>
>   - dw_pcie_iatu_setup(): atu.parent_bus_addr = entry->res->start
>
>     Here "entry" iterates through bridge->windows, and we should be
>     able to set entry->parent_bus_offset at init-time, using
>     .cpu_addr_fixup() if necessary, so we can apply that offset
>     unconditionally, regardless of use_parent_dt_ranges, and we won't
>     need .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().
>
>   - dw_pcie_pme_turn_off:
>     atu.parent_bus_addr = pci->pp.msg_res->start - pci->pp.msg_parent_bus_offset
>
>     This should be the same as dw_pcie_iatu_setup() since
>     msg_parent_bus_offset comes from the window iteration in
>     dw_pcie_host_request_msg_tlp_res().  As long as the windows have
>     the correct parent_bus_offset at init-time, we should be all set.

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Frank Li]

On Tue, Mar 04, 2025 at 05:11:54PM -0500, Frank Li wrote:
> On Tue, Mar 04, 2025 at 11:50:10AM -0600, Bjorn Helgaas wrote:
> > On Mon, Mar 03, 2025 at 04:57:29PM -0500, Frank Li wrote:
> > > On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> > > > > Introduce `parent_bus_offset` in `resource_entry` and a new API,
> > > > > `pci_add_resource_parent_bus_offset()`, to provide necessary information
> > > > > for PCI controllers with address translation units.
> > > > >
> > > > > Typical PCI data flow involves:
> > > > >   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
> > > > >   PCI Controller (PCI bus address) -> PCI Bus.
> > > > >
> > > > > While most bus fabrics preserve address consistency, some modify addresses
> > > > > to intermediate values. The `parent_bus_offset` enables PCI controllers to
> > > > > translate these intermediate addresses correctly to PCI bus addresses.
> > > > >
> > > > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> > > > > PCI controller drivers.
> > > > > ...
> > > >
> > > > > +++ b/drivers/pci/of.c
> > > > > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> > > > >  			res->flags &= ~IORESOURCE_MEM_64;
> > > > >  		}
> > > > >
> > > > > -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> > > > > +		/*
> > > > > +		 * IORESOURCE_IO res->start is io space start address.
> > > > > +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> > > > > +		 * same as range.cpu_addr.
> > > > > +		 *
> > > > > +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> > > > > +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> > > > > +		 */
> > > > > +
> > > > > +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> > > > > +						   range.cpu_addr - range.parent_bus_addr);
> > > >
> > > > I don't know exactly where it needs to go, but I think we can call
> > > > .cpu_addr_fixup() once at startup on the base of the region.  This
> > > > will tell us the offset that applies to the entire region, i.e.,
> > > > parent_bus_offset.
> > > >
> > > > Then we can remove all the .cpu_addr_fixup() calls in
> > > > cdns_pcie_host_init_address_translation(),
> > > > cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().
> > > >
> > > > Until we can get rid of all the .cpu_addr_fixup() implementations,
> > > > We'll still have that single call at startup (I guess once for cadence
> > > > and another for designware), but it should simplify the current
> > > > callers quite a bit.
> > >
> > > I don't think it can simple code. cdns_pcie_set_outbound_region() and
> > > dw_pcie_prog_outbound_atu() are called by EP functions, which have not use
> > > "resource" to manage outbound windows.
> >
> > Let's ignore cadence for now.  I don't think we need to solve that
> > until later.
> >
> > dw_pcie_prog_outbound_atu() is called by:
> >
> >   - dw_pcie_other_conf_map_bus(): atu.parent_bus_addr = pp->cfg0_base
> >
> >     I think dw_pcie_host_init() can set pp->cfg0_base with the correct
> >     intermediate address, either via the the of_property_read_reg() or
> >     .cpu_addr_fixup().

And chicken and egg problem here for artpec6_pcie_cpu_addr_fixup(), which
need cfg0_base. But try to use .cpu_addr_fixup() to get cfg0_base's
intermediate address.

Frank

> >
> >     If dw_pcie_host_init() does this, then we don't need
> >     .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().
> >
> >   - dw_pcie_rd_other_conf(): atu.parent_bus_addr = pp->io_base
> >
> >     Similarly, dw_pcie_host_init() should be able to set pp->io_base
> >     to the intermediate address, so we don't need .cpu_addr_fixup() in
> >     dw_pcie_prog_outbound_atu().
>
> I found some driver's cpu_addr_fixup()'s implement depend on the
> initilize sequence.
>
> for example:
> 	pcie-artpec6.c
>
> static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> {
>         struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
>         struct dw_pcie_rp *pp = &pci->pp;
>         struct dw_pcie_ep *ep = &pci->ep;
>
>         switch (artpec6_pcie->mode) {
>         case DW_PCIE_RC_TYPE:
>                 return cpu_addr - pp->cfg0_base;
>         case DW_PCIE_EP_TYPE:
>                 return cpu_addr - ep->phys_base;
>         default:
>                 dev_err(pci->dev, "UNKNOWN device type\n");
>         }
>         return cpu_addr;
> }
>
> This implement require *cfg0_base* and *phys_base*, pp/ep, need set before
> call artpec6_pcie_cpu_addr_fixup().
>
> static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> {
>         struct dw_pcie_rp *pp = &pci->pp;
>
>         return cpu_addr & ~pp->io_base;
> }
>
> this one require *io_base* and *pp* need be set before call
> visconti_pcie_cpu_addr_fixup()
>
> Because I have not such hardware platform, it is not trivial change and
> it is hard to involve bugs.
>
> If move .cpu_addr_fixup() too early, it will cause kernel dump.
>
> I suggest keep current overall sequent and try to clean up these driver's
> cpu_addr_fixup() firstly.
>
> Frank
>
> >
> >   - dw_pcie_iatu_setup(): atu.parent_bus_addr = entry->res->start
> >
> >     Here "entry" iterates through bridge->windows, and we should be
> >     able to set entry->parent_bus_offset at init-time, using
> >     .cpu_addr_fixup() if necessary, so we can apply that offset
> >     unconditionally, regardless of use_parent_dt_ranges, and we won't
> >     need .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().
> >
> >   - dw_pcie_pme_turn_off:
> >     atu.parent_bus_addr = pci->pp.msg_res->start - pci->pp.msg_parent_bus_offset
> >
> >     This should be the same as dw_pcie_iatu_setup() since
> >     msg_parent_bus_offset comes from the window iteration in
> >     dw_pcie_host_request_msg_tlp_res().  As long as the windows have
> >     the correct parent_bus_offset at init-time, we should be all set.

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Frank Li]

On Tue, Mar 04, 2025 at 05:25:45PM -0500, Frank Li wrote:
> On Tue, Mar 04, 2025 at 05:11:54PM -0500, Frank Li wrote:
> > On Tue, Mar 04, 2025 at 11:50:10AM -0600, Bjorn Helgaas wrote:
> > > On Mon, Mar 03, 2025 at 04:57:29PM -0500, Frank Li wrote:
> > > > On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote:
> > > > > On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> > > > > > Introduce `parent_bus_offset` in `resource_entry` and a new API,
> > > > > > `pci_add_resource_parent_bus_offset()`, to provide necessary information
> > > > > > for PCI controllers with address translation units.
> > > > > >
> > > > > > Typical PCI data flow involves:
> > > > > >   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
> > > > > >   PCI Controller (PCI bus address) -> PCI Bus.
> > > > > >
> > > > > > While most bus fabrics preserve address consistency, some modify addresses
> > > > > > to intermediate values. The `parent_bus_offset` enables PCI controllers to
> > > > > > translate these intermediate addresses correctly to PCI bus addresses.
> > > > > >
> > > > > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> > > > > > PCI controller drivers.
> > > > > > ...
> > > > >
> > > > > > +++ b/drivers/pci/of.c
> > > > > > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> > > > > >  			res->flags &= ~IORESOURCE_MEM_64;
> > > > > >  		}
> > > > > >
> > > > > > -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> > > > > > +		/*
> > > > > > +		 * IORESOURCE_IO res->start is io space start address.
> > > > > > +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> > > > > > +		 * same as range.cpu_addr.
> > > > > > +		 *
> > > > > > +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> > > > > > +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> > > > > > +		 */
> > > > > > +
> > > > > > +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> > > > > > +						   range.cpu_addr - range.parent_bus_addr);
> > > > >
> > > > > I don't know exactly where it needs to go, but I think we can call
> > > > > .cpu_addr_fixup() once at startup on the base of the region.  This
> > > > > will tell us the offset that applies to the entire region, i.e.,
> > > > > parent_bus_offset.
> > > > >
> > > > > Then we can remove all the .cpu_addr_fixup() calls in
> > > > > cdns_pcie_host_init_address_translation(),
> > > > > cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().
> > > > >
> > > > > Until we can get rid of all the .cpu_addr_fixup() implementations,
> > > > > We'll still have that single call at startup (I guess once for cadence
> > > > > and another for designware), but it should simplify the current
> > > > > callers quite a bit.
> > > >
> > > > I don't think it can simple code. cdns_pcie_set_outbound_region() and
> > > > dw_pcie_prog_outbound_atu() are called by EP functions, which have not use
> > > > "resource" to manage outbound windows.
> > >
> > > Let's ignore cadence for now.  I don't think we need to solve that
> > > until later.
> > >
> > > dw_pcie_prog_outbound_atu() is called by:
> > >
> > >   - dw_pcie_other_conf_map_bus(): atu.parent_bus_addr = pp->cfg0_base
> > >
> > >     I think dw_pcie_host_init() can set pp->cfg0_base with the correct
> > >     intermediate address, either via the the of_property_read_reg() or
> > >     .cpu_addr_fixup().
>
> And chicken and egg problem here for artpec6_pcie_cpu_addr_fixup(), which
> need cfg0_base. But try to use .cpu_addr_fixup() to get cfg0_base's
> intermediate address.

Bjorn:
	Do you have chance to check my reply? some dwc platform driver
.cpu_addr_fixup() implement have dependence with old initilize sequency.

	Can I use original method? If change each driver's .cpu_addr_fixup()
implement, it will involve more risk, even more than directly clean it as
my RFC patch.

Frank
>
> Frank
>
> > >
> > >     If dw_pcie_host_init() does this, then we don't need
> > >     .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().
> > >
> > >   - dw_pcie_rd_other_conf(): atu.parent_bus_addr = pp->io_base
> > >
> > >     Similarly, dw_pcie_host_init() should be able to set pp->io_base
> > >     to the intermediate address, so we don't need .cpu_addr_fixup() in
> > >     dw_pcie_prog_outbound_atu().
> >
> > I found some driver's cpu_addr_fixup()'s implement depend on the
> > initilize sequence.
> >
> > for example:
> > 	pcie-artpec6.c
> >
> > static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > {
> >         struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> >         struct dw_pcie_rp *pp = &pci->pp;
> >         struct dw_pcie_ep *ep = &pci->ep;
> >
> >         switch (artpec6_pcie->mode) {
> >         case DW_PCIE_RC_TYPE:
> >                 return cpu_addr - pp->cfg0_base;
> >         case DW_PCIE_EP_TYPE:
> >                 return cpu_addr - ep->phys_base;
> >         default:
> >                 dev_err(pci->dev, "UNKNOWN device type\n");
> >         }
> >         return cpu_addr;
> > }
> >
> > This implement require *cfg0_base* and *phys_base*, pp/ep, need set before
> > call artpec6_pcie_cpu_addr_fixup().
> >
> > static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > {
> >         struct dw_pcie_rp *pp = &pci->pp;
> >
> >         return cpu_addr & ~pp->io_base;
> > }
> >
> > this one require *io_base* and *pp* need be set before call
> > visconti_pcie_cpu_addr_fixup()
> >
> > Because I have not such hardware platform, it is not trivial change and
> > it is hard to involve bugs.
> >
> > If move .cpu_addr_fixup() too early, it will cause kernel dump.
> >
> > I suggest keep current overall sequent and try to clean up these driver's
> > cpu_addr_fixup() firstly.
> >
> > Frank
> >
> > >
> > >   - dw_pcie_iatu_setup(): atu.parent_bus_addr = entry->res->start
> > >
> > >     Here "entry" iterates through bridge->windows, and we should be
> > >     able to set entry->parent_bus_offset at init-time, using
> > >     .cpu_addr_fixup() if necessary, so we can apply that offset
> > >     unconditionally, regardless of use_parent_dt_ranges, and we won't
> > >     need .cpu_addr_fixup() in dw_pcie_prog_outbound_atu().
> > >
> > >   - dw_pcie_pme_turn_off:
> > >     atu.parent_bus_addr = pci->pp.msg_res->start - pci->pp.msg_parent_bus_offset
> > >
> > >     This should be the same as dw_pcie_iatu_setup() since
> > >     msg_parent_bus_offset comes from the window iteration in
> > >     dw_pcie_host_request_msg_tlp_res().  As long as the windows have
> > >     the correct parent_bus_offset at init-time, we should be all set.

Re: [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry

[Bjorn Helgaas]

On Fri, Mar 07, 2025 at 10:32:39AM -0500, Frank Li wrote:
> On Tue, Mar 04, 2025 at 05:25:45PM -0500, Frank Li wrote:
> > On Tue, Mar 04, 2025 at 05:11:54PM -0500, Frank Li wrote:
> > > On Tue, Mar 04, 2025 at 11:50:10AM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Mar 03, 2025 at 04:57:29PM -0500, Frank Li wrote:
> > > > > On Wed, Feb 26, 2025 at 06:23:26PM -0600, Bjorn Helgaas wrote:
> > > > > > On Tue, Jan 28, 2025 at 05:07:36PM -0500, Frank Li wrote:
> > > > > > > Introduce `parent_bus_offset` in `resource_entry` and a new API,
> > > > > > > `pci_add_resource_parent_bus_offset()`, to provide necessary information
> > > > > > > for PCI controllers with address translation units.
> > > > > > >
> > > > > > > Typical PCI data flow involves:
> > > > > > >   CPU (CPU address) -> Bus Fabric (Intermediate address) ->
> > > > > > >   PCI Controller (PCI bus address) -> PCI Bus.
> > > > > > >
> > > > > > > While most bus fabrics preserve address consistency, some modify addresses
> > > > > > > to intermediate values. The `parent_bus_offset` enables PCI controllers to
> > > > > > > translate these intermediate addresses correctly to PCI bus addresses.
> > > > > > >
> > > > > > > Pave the road to remove hardcoded cpu_addr_fixup() and similar patterns in
> > > > > > > PCI controller drivers.
> > > > > > > ...
> > > > > >
> > > > > > > +++ b/drivers/pci/of.c
> > > > > > > @@ -402,7 +402,17 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
> > > > > > >  			res->flags &= ~IORESOURCE_MEM_64;
> > > > > > >  		}
> > > > > > >
> > > > > > > -		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> > > > > > > +		/*
> > > > > > > +		 * IORESOURCE_IO res->start is io space start address.
> > > > > > > +		 * IORESOURCE_MEM res->start is cpu start address, which is the
> > > > > > > +		 * same as range.cpu_addr.
> > > > > > > +		 *
> > > > > > > +		 * Use (range.cpu_addr - range.parent_bus_addr) to align both
> > > > > > > +		 * IO and MEM's parent_bus_offset always offset to cpu address.
> > > > > > > +		 */
> > > > > > > +
> > > > > > > +		pci_add_resource_parent_bus_offset(resources, res, res->start - range.pci_addr,
> > > > > > > +						   range.cpu_addr - range.parent_bus_addr);
> > > > > >
> > > > > > I don't know exactly where it needs to go, but I think we can call
> > > > > > .cpu_addr_fixup() once at startup on the base of the region.  This
> > > > > > will tell us the offset that applies to the entire region, i.e.,
> > > > > > parent_bus_offset.
> > > > > >
> > > > > > Then we can remove all the .cpu_addr_fixup() calls in
> > > > > > cdns_pcie_host_init_address_translation(),
> > > > > > cdns_pcie_set_outbound_region(), and dw_pcie_prog_outbound_atu().
> > > > > >
> > > > > > Until we can get rid of all the .cpu_addr_fixup() implementations,
> > > > > > We'll still have that single call at startup (I guess once for cadence
> > > > > > and another for designware), but it should simplify the current
> > > > > > callers quite a bit.
> > > > >
> > > > > I don't think it can simple code. cdns_pcie_set_outbound_region() and
> > > > > dw_pcie_prog_outbound_atu() are called by EP functions, which have not use
> > > > > "resource" to manage outbound windows.
> > > >
> > > > Let's ignore cadence for now.  I don't think we need to solve that
> > > > until later.
> > > >
> > > > dw_pcie_prog_outbound_atu() is called by:
> > > >
> > > >   - dw_pcie_other_conf_map_bus(): atu.parent_bus_addr = pp->cfg0_base
> > > >
> > > >     I think dw_pcie_host_init() can set pp->cfg0_base with the correct
> > > >     intermediate address, either via the the of_property_read_reg() or
> > > >     .cpu_addr_fixup().
> >
> > And chicken and egg problem here for artpec6_pcie_cpu_addr_fixup(), which
> > need cfg0_base. But try to use .cpu_addr_fixup() to get cfg0_base's
> > intermediate address.
> 
> Bjorn:
> 	Do you have chance to check my reply? some dwc platform driver
> .cpu_addr_fixup() implement have dependence with old initilize sequency.

Yes, I tried to sketch out what I was thinking to make it more
concrete.  I posted it and sent to you, but just for other readers of
this thread, it's at:

https://lore.kernel.org/linux-pci/20250307233744.440476-1-helgaas@kernel.org/T/#t

Bjorn

[PATCH v9 4/7] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Frank Li]

parent_bus_offset in resource_entry 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 Intermediate 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>
---
chagne from v8 to v9
- use resoure_entry parent_bus_offset to simple code logic
- add check for use_parent_dt_ranges and cpu_addr_fixup to make sure only
one set.

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 | 34 +++++++++++++++++++++--
 drivers/pci/controller/dwc/pcie-designware.c      |  9 ++++++
 drivers/pci/controller/dwc/pcie-designware.h      |  8 ++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 1206b26bff3f2..a0c8e6f66ec4d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -413,8 +413,10 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 		res->name = "msg";
 		res->flags = win->res->flags | IORESOURCE_BUSY;
 
-		if (!devm_request_resource(pci->dev, win->res, res))
+		if (!devm_request_resource(pci->dev, win->res, res)) {
 			pp->msg_res = res;
+			pp->msg_parent_bus_offset = win->parent_bus_offset;
+		}
 	}
 }
 
@@ -427,6 +429,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);
@@ -448,6 +451,26 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (IS_ERR(pp->va_cfg0_base))
 		return PTR_ERR(pp->va_cfg0_base);
 
+	if (pci->use_parent_dt_ranges) {
+		if (pci->ops->cpu_addr_fixup) {
+			dev_err(dev, "Use parent bus DT ranges, cpu_addr_fixup() must be removed\n");
+			return -EINVAL;
+		}
+
+		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);
+	}
+
 	bridge = devm_pci_alloc_host_bridge(dev, 0);
 	if (!bridge)
 		return -ENOMEM;
@@ -460,6 +483,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->io_size = resource_size(win->res);
 		pp->io_bus_addr = win->res->start - win->offset;
 		pp->io_base = pci_pio_to_address(win->res->start);
+		/* In case ranges in pci node provide wrong information */
+		if (pci->use_parent_dt_ranges)
+			pp->io_base -= win->parent_bus_offset;
 	}
 
 	/* Set default bus ops */
@@ -739,6 +765,10 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 		atu.parent_bus_addr = entry->res->start;
 		atu.pci_addr = entry->res->start - entry->offset;
 
+		/* In case ranges in pci node provide wrong information */
+		if (pci->use_parent_dt_ranges)
+			atu.parent_bus_addr -= entry->parent_bus_offset;
+
 		/* 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) -
@@ -902,7 +932,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
 	atu.size = resource_size(pci->pp.msg_res);
 	atu.index = pci->pp.msg_atu_index;
 
-	atu.parent_bus_addr = pci->pp.msg_res->start;
+	atu.parent_bus_addr = pci->pp.msg_res->start - pci->pp.msg_parent_bus_offset;
 
 	ret = dw_pcie_prog_outbound_atu(pci, &atu);
 	if (ret)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 9d0a5f75effcc..909b14986660c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -841,6 +841,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 ac23604c829f4..483911ab9e629 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -380,6 +380,7 @@ struct dw_pcie_rp {
 	bool			use_atu_msg;
 	int			msg_atu_index;
 	struct resource		*msg_res;
+	resource_size_t		msg_parent_bus_offset;
 	bool			use_linkup_irq;
 };
 
@@ -465,6 +466,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

Re: [PATCH v9 4/7] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Bjorn Helgaas]

On Tue, Jan 28, 2025 at 05:07:37PM -0500, Frank Li wrote:
> parent_bus_offset in resource_entry 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:
> ...

> @@ -448,6 +451,26 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (IS_ERR(pp->va_cfg0_base))
>  		return PTR_ERR(pp->va_cfg0_base);
>  
> +	if (pci->use_parent_dt_ranges) {
> +		if (pci->ops->cpu_addr_fixup) {
> +			dev_err(dev, "Use parent bus DT ranges, cpu_addr_fixup() must be removed\n");
> +			return -EINVAL;
> +		}
> +
> +		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);
> +	}

I think all this code dealing with the "config" resource could go in a
helper function.  It's kind of a lot of clutter in
dw_pcie_host_init().

It would be nice to assign pp->cfg0_base once, not assign res->start
to it and then possibly overwrite it later.

> @@ -841,6 +841,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");
> +	}

Can you split the warnings out to a separate patch?  I think we should
warn once in every initialization path where .cpu_addr_fixup() could
be used, i.e.,

  dw_pcie_host_init()
  dw_pcie_ep_init()
  cdns_pcie_host_setup()
  cdns_pcie_ep_setup()

IMO these should warn if .cpu_addr_fixup() is implemented, regardless
of use_parent_dt_ranges.

I'm still puzzling over some of the rest of this, so no need to post a
revised series yet.

Bjorn

Re: [PATCH v9 4/7] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Frank Li]

On Wed, Feb 26, 2025 at 05:33:39PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 05:07:37PM -0500, Frank Li wrote:
> > parent_bus_offset in resource_entry 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:
> > ...
>
> > @@ -448,6 +451,26 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  	if (IS_ERR(pp->va_cfg0_base))
> >  		return PTR_ERR(pp->va_cfg0_base);
> >
> > +	if (pci->use_parent_dt_ranges) {
> > +		if (pci->ops->cpu_addr_fixup) {
> > +			dev_err(dev, "Use parent bus DT ranges, cpu_addr_fixup() must be removed\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		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);
> > +	}
>
> I think all this code dealing with the "config" resource could go in a
> helper function.  It's kind of a lot of clutter in
> dw_pcie_host_init().
>
> It would be nice to assign pp->cfg0_base once, not assign res->start
> to it and then possibly overwrite it later.
>
> > @@ -841,6 +841,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");
> > +	}
>
> Can you split the warnings out to a separate patch?  I think we should
> warn once in every initialization path where .cpu_addr_fixup() could
> be used, i.e.,
>
>   dw_pcie_host_init()
>   dw_pcie_ep_init()
>   cdns_pcie_host_setup()
>   cdns_pcie_ep_setup()
>
> IMO these should warn if .cpu_addr_fixup() is implemented, regardless
> of use_parent_dt_ranges.
>
> I'm still puzzling over some of the rest of this, so no need to post a
> revised series yet.

Do you have additional comments, so I can post a revised series?

Frank

>
> Bjorn

[PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window

[Frank Li]

                   Endpoint
  ┌───────────────────────────────────────────────┐
  │                             pcie-ep@5f010000  │
  │                             ┌────────────────┐│
  │                             │   Endpoint     ││
  │                             │   PCIe         ││
  │                             │   Controller   ││
  │           bus@5f000000      │                ││
  │           ┌──────────┐      │                ││
  │           │          │ Outbound Transfer     ││
  │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
  ││     │    │  Fabric  │Bus   │                ││PCI Addr
  ││ CPU ├───►│          │Addr  │                ││0xA000_0000
  ││     │CPU │          │0x8000_0000            ││
  │└─────┘Addr└──────────┘      │                ││
  │       0x7000_0000           └────────────────┘│
  └───────────────────────────────────────────────┘

Use 'ranges' property in DT to configure the iATU outbound window address.
The bus fabric generally passes the same address to the PCIe EP controller,
but some bus fabrics map the address before sending it to the PCIe EP
controller.

Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
input address and map to PCI address 0xA000_0000 (dynamic alloc and assign
from pci device driver in host side).

Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
the device tree provides this information, preferring a common method.

bus@5f000000 {
	compatible = "simple-bus";
	ranges = <0x80000000 0x0 0x70000000 0x10000000>;

	pcie-ep@5f010000 {
		reg = <0x80000000 0x10000000>;
		reg-names ="addr_space";
		...
	};
	...
};

'ranges' in bus@5f000000 descript how address map from CPU address to bus
address.

Use `of_property_read_reg()` to obtain the bus address and set it to the
ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup().

Use 'use_parent_dt_ranges' to indicate device tree reflect correctly bus
address translation in case break compatibility.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v8 to v9
- change bus_addr_base to parent_bus_addr
- fix dw_pcie_find_index() address compare, which cause atu allocate
failure when run many time test.

Change from v7 to v8
- Add Mani's reviewedby tag
- s/convert/map in commit message
- update comments for of_property_read_reg()
- use 'use_parent_dt_ranges'

Change from v6 to v7
- none

Change from v5 to v6
- update diagram
- Add comments for of_property_read_reg()
- Remove unrelated 0x5f00_0000 in commit message

Change from v3 to v4
- change parent_bus_addr to u64 to fix 32bit build error
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410230328.BTHareG1-lkp@intel.com/

Change from v2 to v3
- Add using_dtbus_info to control if use device tree bus ranges
information.
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 21 +++++++++++++++++++--
 drivers/pci/controller/dwc/pcie-designware.h    |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 80ac2f9e88eb5..d0d6c4e8df80c 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -9,6 +9,7 @@
 #include <linux/align.h>
 #include <linux/bitfield.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include "pcie-designware.h"
@@ -279,11 +280,12 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
 			      u32 *atu_index)
 {
+	phys_addr_t parent_bus_addr = addr - ep->phys_base + ep->parent_bus_addr;
 	u32 index;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
 	for (index = 0; index < pci->num_ob_windows; index++) {
-		if (ep->outbound_addr[index] != addr)
+		if (ep->outbound_addr[index] != parent_bus_addr)
 			continue;
 		*atu_index = index;
 		return 0;
@@ -333,7 +335,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 
 	atu.func_no = func_no;
 	atu.type = PCIE_ATU_TYPE_MEM;
-	atu.parent_bus_addr = addr;
+	atu.parent_bus_addr = addr - ep->phys_base + ep->parent_bus_addr;
 	atu.pci_addr = pci_addr;
 	atu.size = size;
 	ret = dw_pcie_ep_outbound_atu(ep, &atu);
@@ -901,6 +903,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	struct device *dev = pci->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
+	int index;
 
 	INIT_LIST_HEAD(&ep->func_list);
 
@@ -913,6 +916,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 
 	ep->phys_base = res->start;
+	ep->parent_bus_addr = ep->phys_base;
+
+	if (pci->use_parent_dt_ranges) {
+		index = of_property_match_string(np, "reg-names", "addr_space");
+		if (index < 0)
+			return -EINVAL;
+
+		/*
+		 * Get the untranslated bus address from devicetree to use it
+		 * as the iATU CPU address in dw_pcie_ep_map_addr().
+		 */
+		of_property_read_reg(np, index, &ep->parent_bus_addr, NULL);
+	}
+
 	ep->addr_size = resource_size(res);
 
 	if (ep->ops->pre_init)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 483911ab9e629..3d9bf2b43bcf2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -413,6 +413,7 @@ struct dw_pcie_ep {
 	struct list_head	func_list;
 	const struct dw_pcie_ep_ops *ops;
 	phys_addr_t		phys_base;
+	u64			parent_bus_addr;
 	size_t			addr_size;
 	size_t			page_size;
 	u8			bar_to_atu[PCI_STD_NUM_BARS];

-- 
2.34.1

[PATCH v9 6/7] PCI: dwc: ep: Ensure proper iteration over outbound map windows

[Frank Li]

Most systems' PCIe outbound map windows have non-zero physical addresses,
but the possibility of encountering zero increased with commit
700cafbb642b ("PCI: dwc: ep: Add bus_addr_base for outbound window").

'ep->outbound_addr[n]', representing 'parent_bus_address', might be 0 on
some hardware, which trims high address bits through bus fabric before
sending to the PCIe controller.

Replace the iteration logic with 'for_each_set_bit()' to ensure only
allocated map windows are iterated when determining the ATU index from a
given address.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v8 to v9
- new patch
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index d0d6c4e8df80c..c1767450541a4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -284,7 +284,7 @@ static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
 	u32 index;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
-	for (index = 0; index < pci->num_ob_windows; index++) {
+	for_each_set_bit(index, ep->ob_window_map, pci->num_ob_windows) {
 		if (ep->outbound_addr[index] != parent_bus_addr)
 			continue;
 		*atu_index = index;

-- 
2.34.1

Re: [PATCH v9 6/7] PCI: dwc: ep: Ensure proper iteration over outbound map windows

[Bjorn Helgaas]

On Tue, Jan 28, 2025 at 05:07:39PM -0500, Frank Li wrote:
> Most systems' PCIe outbound map windows have non-zero physical addresses,
> but the possibility of encountering zero increased with commit
> 700cafbb642b ("PCI: dwc: ep: Add bus_addr_base for outbound window").

I don't know what commit 700cafbb642b is.  It doesn't appear
upstream, and there's no subject line or patch match for
bus_addr_base.

Bjorn

Re: [PATCH v9 6/7] PCI: dwc: ep: Ensure proper iteration over outbound map windows

[Bjorn Helgaas]

On Wed, Feb 26, 2025 at 06:12:13PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 05:07:39PM -0500, Frank Li wrote:
> > Most systems' PCIe outbound map windows have non-zero physical addresses,
> > but the possibility of encountering zero increased with commit
> > 700cafbb642b ("PCI: dwc: ep: Add bus_addr_base for outbound window").
> 
> I don't know what commit 700cafbb642b is.  It doesn't appear
> upstream, and there's no subject line or patch match for
> bus_addr_base.

Never mind, I see it's patch 5/7 of *this* series.  I looked at the
0/7 list, but missed it.

In any case, the commit ID will be different since we apply patches
from email, so the ID from your tree won't be useful.

Bjorn

[PATCH v9 7/7] PCI: imx6: Remove cpu_addr_fixup()

[Frank Li]

Remove cpu_addr_fixup() because dwc common driver already handle address
translate.

Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v8 to v9
- rebase latest linux-next (close enough to v6.14-rc1)

Change from v7 to v8
- add mani and richard's review/ack tag
- use varible 'use_parent_dt_ranges'

Change from v2 to v7
- none
Change from v1 to v2
- set using_dtbus_info true
---
 drivers/pci/controller/dwc/pci-imx6.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 90ace941090f9..d1eb535df73e1 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1217,22 +1217,6 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
 		regulator_disable(imx_pcie->vpcie);
 }
 
-static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
-{
-	struct imx_pcie *imx_pcie = to_imx_pcie(pcie);
-	struct dw_pcie_rp *pp = &pcie->pp;
-	struct resource_entry *entry;
-
-	if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_CPU_ADDR_FIXUP))
-		return cpu_addr;
-
-	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
-	if (!entry)
-		return cpu_addr;
-
-	return cpu_addr - entry->offset;
-}
-
 /*
  * In old DWC implementations, PCIE_ATU_INHIBIT_PAYLOAD in iATU Ctrl2
  * register is reserved, so the generic DWC implementation of sending the
@@ -1263,7 +1247,6 @@ static const struct dw_pcie_host_ops imx_pcie_host_dw_pme_ops = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = imx_pcie_start_link,
 	.stop_link = imx_pcie_stop_link,
-	.cpu_addr_fixup = imx_pcie_cpu_addr_fixup,
 };
 
 static void imx_pcie_ep_init(struct dw_pcie_ep *ep)
@@ -1645,6 +1628,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pci->use_parent_dt_ranges = true;
 	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
 		ret = imx_add_pcie_ep(imx_pcie, pdev);
 		if (ret < 0)

-- 
2.34.1

Re: [PATCH v9 0/7] PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()

[Niklas Cassel]

Hello Frank,

Typo in subject:
s/opitimaze/optimize/


On Tue, Jan 28, 2025 at 05:07:33PM -0500, Frank Li wrote:
> 
> Bjorn's comments in https://lore.kernel.org/imx/20250123190900.GA650360@bhelgaas/
> 
> > After all cpu_address_fixup() removed, we can remove use_parent_dt_ranges
> > in one clean up patches.
> >
> >
>   ...
> >  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.
> 
> io_base is parent_bus_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_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.
> 
> Fixed at this version.


I feel like it would have been easier if you replied to Bjorn's comments
directly in the thread instead of pasting them here (to a cover letter).


Please don't shoot the messenger, but I don't see any reply to (what I
assume is the biggest reason why Bjorn did not merge this series):

""
.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.
""

It should probably state in the cover letter how v9 addresses Bjorn's
concern when it comes to other PCI controller drivers, especially those
that are not DWC based.


Kind regards,
Niklas

Re: [PATCH v9 0/7] PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()

[Frank Li]

On Wed, Jan 29, 2025 at 11:13:42AM +0100, Niklas Cassel wrote:
> Hello Frank,
>
> Typo in subject:
> s/opitimaze/optimize/
>
>
> On Tue, Jan 28, 2025 at 05:07:33PM -0500, Frank Li wrote:
> >
> > Bjorn's comments in https://lore.kernel.org/imx/20250123190900.GA650360@bhelgaas/
> >
> > > After all cpu_address_fixup() removed, we can remove use_parent_dt_ranges
> > > in one clean up patches.
> > >
> > >
> >   ...
> > >  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.
> >
> > io_base is parent_bus_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_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.
> >
> > Fixed at this version.
>
>
> I feel like it would have been easier if you replied to Bjorn's comments
> directly in the thread instead of pasting them here (to a cover letter).
>
>
> Please don't shoot the messenger, but I don't see any reply to (what I
> assume is the biggest reason why Bjorn did not merge this series):
>
> ""
> .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.
> ""
>
> It should probably state in the cover letter how v9 addresses Bjorn's
> concern when it comes to other PCI controller drivers, especially those
> that are not DWC based.

Thank you for your reminder, I forget mentions this in cover letter. I
create new patch to figure out this Bjorn's problem.

PCI: Add parent_bus_offset to resource_entry

With above patch, other platform will be easy apply the same method.

Frank
>
>
> Kind regards,
> Niklas

Re: [PATCH v9 0/7] PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()

[Niklas Cassel]

Hello Frank,

On Wed, Jan 29, 2025 at 10:28:23AM -0500, Frank Li wrote:
> On Wed, Jan 29, 2025 at 11:13:42AM +0100, Niklas Cassel wrote:
> >
> > Please don't shoot the messenger, but I don't see any reply to (what I
> > assume is the biggest reason why Bjorn did not merge this series):
> >
> > ""
> > .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.
> > ""
> >
> > It should probably state in the cover letter how v9 addresses Bjorn's
> > concern when it comes to other PCI controller drivers, especially those
> > that are not DWC based.
> 
> Thank you for your reminder, I forget mentions this in cover letter. I
> create new patch to figure out this Bjorn's problem.
> 
> PCI: Add parent_bus_offset to resource_entry

I see.

If you are solving this problem in a generic way, then it would make sense
if the generic patch comes first in the series, and then the driver (DWC)
specific patches come after that.

Your cover letter, including the subject is also written in a DWC specific
manner.

If you are solving a generic problem, then describe first how you solve
the generic problem, followed by DWC specific details.

See e.g. my cover letter here:
https://lore.kernel.org/linux-pci/20250119050850.2kogtpl5hatpp2tv@thinkpad/T/#t


Kind regards,
Niklas

Re: [PATCH v9 0/7] PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()

[Frank Li]

On Wed, Jan 29, 2025 at 05:39:18PM +0100, Niklas Cassel wrote:
> Hello Frank,
>
> On Wed, Jan 29, 2025 at 10:28:23AM -0500, Frank Li wrote:
> > On Wed, Jan 29, 2025 at 11:13:42AM +0100, Niklas Cassel wrote:
> > >
> > > Please don't shoot the messenger, but I don't see any reply to (what I
> > > assume is the biggest reason why Bjorn did not merge this series):
> > >
> > > ""
> > > .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.
> > > ""
> > >
> > > It should probably state in the cover letter how v9 addresses Bjorn's
> > > concern when it comes to other PCI controller drivers, especially those
> > > that are not DWC based.
> >
> > Thank you for your reminder, I forget mentions this in cover letter. I
> > create new patch to figure out this Bjorn's problem.
> >
> > PCI: Add parent_bus_offset to resource_entry
>
> I see.
>
> If you are solving this problem in a generic way, then it would make sense
> if the generic patch comes first in the series, and then the driver (DWC)
> specific patches come after that.
>
> Your cover letter, including the subject is also written in a DWC specific
> manner.
>
> If you are solving a generic problem, then describe first how you solve
> the generic problem, followed by DWC specific details.
>
> See e.g. my cover letter here:
> https://lore.kernel.org/linux-pci/20250119050850.2kogtpl5hatpp2tv@thinkpad/T/#t

Thanks, let's wait for Bjorn's comments about new method. I will do that
next time if need respin.

Frank

>
>
> Kind regards,
> Niklas