[PATCH v11 00/11] PCI: Use device bus range info to cleanup RC Host/EP pci_fixup_addr()

[PATCH v11 00/11] PCI: Use device bus range info to cleanup 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 v11:
- Add patch for Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()
- move PCI: dwc: ep: Ensure proper iteration over outbound map windows
ahead PCI: dwc: Use parent_bus_offset  to avoid broken bisect for some platfrom
- other  detail change each patches's change log.
- Link to v10: https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-0-409dafc950d1@nxp.com

Changes in v10:
- Remove patch PCI: Add parent_bus_offset to resource_entry because it
is detect by reg-names["config"] and reg-names["space_addr"];

- using Bjorn suggest method
https://lore.kernel.org/linux-pci/20250307233744.440476-1-helgaas@kernel.org/
- other detail change each patches's change log.
- PCI: dwc: Move cfg0 setup to dw_pcie_cfg0_setup() is not necessary, but
nice clean up, so keep it.

- Still keep use_parent_dt_ranges in case some platform without cpu_addr_fixup,
which use fake address transation at DTB. If no one report warning for
sometime, we can remove it safely. Bjoin, if you think this case is rare,
you can remove it.

- Link to v9: https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-0-3c4bb506f665@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

---
Bjorn Helgaas (3):
      PCI: dwc: Move cfg0 setup to dw_pcie_cfg0_setup()
      PCI: dwc: Add helper dw_pcie_init_parent_bus_offset()
      PCI: dwc: Use parent_bus_offset

Frank Li (8):
      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: dwc: Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()
      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: dwc: Print warning message when cpu_addr_fixup() exists
      PCI: imx6: Remove cpu_addr_fixup()

 drivers/pci/controller/dwc/pci-imx6.c             | 18 +----
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 21 ++++--
 drivers/pci/controller/dwc/pcie-designware-host.c | 75 +++++++++++++-------
 drivers/pci/controller/dwc/pcie-designware.c      | 85 ++++++++++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.h      | 19 ++++-
 5 files changed, 146 insertions(+), 72 deletions(-)
---
base-commit: 5fcc194143ce5918ea0790a16323a844c5ab9499
change-id: 20240924-pci_fixup_addr-a8568f9bbb34

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

[PATCH v11 01/11] PCI: dwc: Use resource start as iomap() input in dw_pcie_pme_turn_off()

[Frank Li]

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 v10 to v11
- none

change from v9 to v10
- use bjorn's suggested commit messaage.

change from v8 to v9
- 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 v11 02/11] 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 v10 to v11
- none

change from v9 to v10
- rename header file's cpu_addr for dw_pcie_prog_inbound_atu() and
dw_pcie_prog_ep_inbound_atu();

change from v8 to v9
- 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      |  7 +++--
 4 files changed, 31 insertions(+), 30 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..d0d8c622a6e8b 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;
 };
@@ -491,9 +491,10 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci);
 int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 			      const struct dw_pcie_ob_atu_cfg *atu);
 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);
 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);
 void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
 void dw_pcie_setup(struct dw_pcie *pci);
 void dw_pcie_iatu_detect(struct dw_pcie *pci);

-- 
2.34.1

[PATCH v11 03/11] PCI: dwc: Move cfg0 setup to dw_pcie_cfg0_setup()

[Frank Li]

From: Bjorn Helgaas <bhelgaas@google.com>

Move pp->cfg0 setup to dw_pcie_cfg0_setup(). No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v10 v0 v11
- none

Change from v9 to v10
v9 means https://lore.kernel.org/linux-pci/20250307233744.440476-1-helgaas@kernel.org/T/#maa0134c1826bffcccab6028c7732a13f7adcec4d

- move dw_pcie_cfg0_setup() ahead of dw_pcie_host_request_msg_tlp_res() to
nice git diff and easy to review.
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 40 +++++++++++++++--------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 1206b26bff3f2..c57831902686e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -392,6 +392,29 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 	return 0;
 }
 
+static int dw_pcie_cfg0_setup(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	if (!res) {
+		dev_err(dev, "Missing \"config\" reg space\n");
+		return -ENODEV;
+	}
+
+	pp->cfg0_size = resource_size(res);
+	pp->cfg0_base = res->start;
+
+	pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pp->va_cfg0_base))
+		return PTR_ERR(pp->va_cfg0_base);
+
+	return 0;
+}
+
 static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -423,10 +446,8 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
-	struct platform_device *pdev = to_platform_device(dev);
 	struct resource_entry *win;
 	struct pci_host_bridge *bridge;
-	struct resource *res;
 	int ret;
 
 	raw_spin_lock_init(&pp->lock);
@@ -435,18 +456,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		return ret;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
-	if (!res) {
-		dev_err(dev, "Missing \"config\" reg space\n");
-		return -ENODEV;
-	}
-
-	pp->cfg0_size = resource_size(res);
-	pp->cfg0_base = res->start;
-
-	pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
-	if (IS_ERR(pp->va_cfg0_base))
-		return PTR_ERR(pp->va_cfg0_base);
+	ret = dw_pcie_cfg0_setup(pp);
+	if (ret)
+		return ret;
 
 	bridge = devm_pci_alloc_host_bridge(dev, 0);
 	if (!bridge)

-- 
2.34.1

[PATCH v11 04/11] PCI: dwc: Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()

[Frank Li]

Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
any DWC resource, moving it earlier improves code logic and readability.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index c57831902686e..52a441662cabe 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 
 	raw_spin_lock_init(&pp->lock);
 
+	bridge = devm_pci_alloc_host_bridge(dev, 0);
+	if (!bridge)
+		return bridge;
+
+	pp->bridge = bridge;
+
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
 		return ret;
@@ -460,12 +466,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		return ret;
 
-	bridge = devm_pci_alloc_host_bridge(dev, 0);
-	if (!bridge)
-		return -ENOMEM;
-
-	pp->bridge = bridge;
-
 	/* Get the I/O range from DT */
 	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
 	if (win) {

-- 
2.34.1

[PATCH v11 05/11] PCI: dwc: Add helper dw_pcie_init_parent_bus_offset()

[Frank Li]

From: Bjorn Helgaas <bhelgaas@google.com>

Set pci->parent_bus_offset based on the parent bus address from the
"config" (Root complex mode) and "addr_space" (Endpoint mode).

.cpu_addr_fixup(cpu_phy_addr). (if implemented) should return the parent
bus address corresponding according to cpu_phy_addr.

Sets pp->parent_bus_offset, but doesn't use it, so no functional change
intended yet.

Add use_parent_dt_ranges to detect some fake bus translation at platform,
which have not .cpu_addr_fixup(). Set use_parent_dt_ranges true explicitly
at platform that have .cpu_addr_fixup() and fixed DTB's range. If not one
report "fake bus translation" for sometime, this flags can be removed
safely.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v10 to v11
- add cpu physical and parent bus address information when no equal
- remove "Using this flag also avoids the usage of 'cpu_addr_fixup'
callback implementation in the driver." for use_parent_dt_ranges.
- change dev_warn_once to dev_warn because only call once.

change from v9 to v10
v9: https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-4-3c4bb506f665@nxp.com/

- use help funtion dw_pcie_init_parent_bus_offset() because both EP and RC
use simular logic.
- still use use_parent_dt_ranges to detect fake bus translation for
no .cpu_addr_fixup()'s platfrom incase block exist platform.
---
 drivers/pci/controller/dwc/pcie-designware.c | 48 ++++++++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h | 12 +++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 9d0a5f75effcc..f17a25fe55a5b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -16,6 +16,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/ioport.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/types.h>
@@ -1105,3 +1106,50 @@ void dw_pcie_setup(struct dw_pcie *pci)
 
 	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
 }
+
+int dw_pcie_init_parent_bus_offset(struct dw_pcie *pci, const char *reg_name,
+				   resource_size_t cpu_phy_addr)
+{
+	struct device *dev = pci->dev;
+	struct device_node *np = dev->of_node;
+	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
+	u64 reg_addr, fixup_addr;
+	int index;
+
+	/* Look up reg_name address on parent bus */
+	index = of_property_match_string(np, "reg-names", reg_name);
+
+	if (index < 0) {
+		dev_err(dev, "Missed reg-name: %s, Broken DTB file\n", reg_name);
+		return -EINVAL;
+	}
+
+	of_property_read_reg(np, index, &reg_addr, NULL);
+
+	fixup = pci->ops->cpu_addr_fixup;
+	if (fixup) {
+		fixup_addr = fixup(pci, cpu_phy_addr);
+		if (reg_addr == fixup_addr) {
+			dev_warn(dev, "%#010llx %s reg[%d] == %#010llx; %ps is redundant\n",
+				 cpu_phy_addr, reg_name, index,
+				 fixup_addr, fixup);
+		} else {
+			dev_warn(dev, "%#010llx %s reg[%d] != %#010llx fixed up addr; DT is broken\n",
+				 cpu_phy_addr, reg_name,
+				 index, fixup_addr);
+			reg_addr = fixup_addr;
+		}
+	} else if (!pci->use_parent_dt_ranges) {
+		if (reg_addr != cpu_phy_addr) {
+			dev_warn(dev, "Your DTB try to use fake translation, Please check parent's ranges property. cpu physical addr: %#010llx, parent bus addr: %#010llx",
+				 cpu_phy_addr, reg_addr);
+			return 0;
+		}
+	}
+
+	pci->parent_bus_offset = cpu_phy_addr - reg_addr;
+	dev_info(dev, "%s parent bus offset is %#010llx\n",
+		 reg_name, pci->parent_bus_offset);
+
+	return 0;
+}
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index d0d8c622a6e8b..bfed9d45aba9f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -445,6 +445,7 @@ struct dw_pcie {
 	void __iomem		*atu_base;
 	resource_size_t		atu_phys_addr;
 	size_t			atu_size;
+	resource_size_t		parent_bus_offset;
 	u32			num_ib_windows;
 	u32			num_ob_windows;
 	u32			region_align;
@@ -465,6 +466,15 @@ 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.
+	 *
+	 * If use_parent_dt_ranges is false, warning will print if IA is not
+	 * equal to cpu physical address. Indicate dtb use a fake translation.
+	 */
+	bool			use_parent_dt_ranges;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
@@ -500,6 +510,8 @@ void dw_pcie_setup(struct dw_pcie *pci);
 void dw_pcie_iatu_detect(struct dw_pcie *pci);
 int dw_pcie_edma_detect(struct dw_pcie *pci);
 void dw_pcie_edma_remove(struct dw_pcie *pci);
+int dw_pcie_init_parent_bus_offset(struct dw_pcie *pci, const char *reg_name,
+				   resource_size_t cpu_phy_addr);
 
 static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
 {

-- 
2.34.1

[PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Frank Li]

The 'ranges' property at PCI controller parent bus can indicate address
translation information. 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.

Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset
is the same for all kinds of address translation.

Just set parent_bus_offset, but doesn't use it, so no functional change
intended yet.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v10 to v11
- update commit message's first paragraph because switch to use 'config'
to get address translation.
- move dw_pcie_init_parent_bus_offset() ahead of bridge->ops = ...

change from v9 to v10
- call helper dw_pcie_init_parent_bus_offset()

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 52a441662cabe..482d8ff751526 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->io_base = pci_pio_to_address(win->res->start);
 	}
 
+	/*
+	 * visconti_pcie_cpu_addr_fixup() use pp->io_base,
+	 * so have to call dw_pcie_init_parent_bus_offset() after init
+	 * pp->io_base.
+	 */
+	ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base);
+	if (ret)
+		return ret;
+
 	/* Set default bus ops */
 	bridge->ops = &dw_pcie_ops;
 	bridge->child_ops = &dw_child_pcie_ops;

-- 
2.34.1

[PATCH v11 07/11] 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 reg-name "addr_space" to detect parent_bus_addr_offset.

Just set parent_bus_offset, but doesn't use it, so no functional change
intended yet.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v10 to v11
- none

Change from v9 to v10
- drop mani's review tag because big change.
- call help funciton dw_pcie_init_parent_bus_offset().

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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 80ac2f9e88eb5..d69d76c150d92 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -915,6 +915,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	ep->phys_base = res->start;
 	ep->addr_size = resource_size(res);
 
+	/*
+	 * artpec6_pcie_cpu_addr_fixup() use ep->phys_base. so call
+	 * dw_pcie_init_parent_bus_offset after init ep->phys_base.
+	 */
+	ret = dw_pcie_init_parent_bus_offset(pci, "addr_space", ep->phys_base);
+	if (ret)
+		return ret;
+
 	if (ep->ops->pre_init)
 		ep->ops->pre_init(ep);
 

-- 
2.34.1

[PATCH v11 08/11] 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 after following commit
("PCI: dwc: Use parent_bus_offset").

'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 v10 to v11
- change refer commit

change from v9 to v10
- remove commit hash value

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 d69d76c150d92..4ecddab131b33 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -282,7 +282,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] != addr)
 			continue;
 		*atu_index = index;

-- 
2.34.1

[PATCH v11 09/11] PCI: dwc: Use parent_bus_offset

[Frank Li]

From: Bjorn Helgaas <bhelgaas@google.com>

We know the parent_bus_offset, either computed from a DT reg property (the
offset is the CPU physical addr - the 'config'/'addr_space' address on the
parent bus) or from a .cpu_addr_fixup() (which may have used a host bridge
window offset).

Apply that parent_bus_offset instead of calling .cpu_addr_fixup() again.

This assumes that all intermediate addresses are at the same offset from
the CPU physical addresses.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v10 to v11
- none

change from v9 to v10
v9: https://lore.kernel.org/linux-pci/20250307233744.440476-5-helgaas@kernel.org/#R
- pp -> pci
- add ep side support
---
 drivers/pci/controller/dwc/pcie-designware-ep.c   |  5 +++--
 drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
 drivers/pci/controller/dwc/pcie-designware.c      |  3 ---
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 4ecddab131b33..e333855633a77 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -314,7 +314,8 @@ static void dw_pcie_ep_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
-	ret = dw_pcie_find_index(ep, addr, &atu_index);
+	ret = dw_pcie_find_index(ep, addr - pci->parent_bus_offset,
+				 &atu_index);
 	if (ret < 0)
 		return;
 
@@ -333,7 +334,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 - pci->parent_bus_offset;
 	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 482d8ff751526..3e7df3d2ac269 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -637,7 +637,7 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
 		type = PCIE_ATU_TYPE_CFG1;
 
 	atu.type = type;
-	atu.parent_bus_addr = pp->cfg0_base;
+	atu.parent_bus_addr = pp->cfg0_base - pci->parent_bus_offset;
 	atu.pci_addr = busdev;
 	atu.size = pp->cfg0_size;
 
@@ -662,7 +662,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.parent_bus_addr = pp->io_base;
+		atu.parent_bus_addr = pp->io_base - pci->parent_bus_offset;
 		atu.pci_addr = pp->io_bus_addr;
 		atu.size = pp->io_size;
 
@@ -688,7 +688,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.parent_bus_addr = pp->io_base;
+		atu.parent_bus_addr = pp->io_base - pci->parent_bus_offset;
 		atu.pci_addr = pp->io_bus_addr;
 		atu.size = pp->io_size;
 
@@ -757,7 +757,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 
 		atu.index = i;
 		atu.type = PCIE_ATU_TYPE_MEM;
-		atu.parent_bus_addr = entry->res->start;
+		atu.parent_bus_addr = entry->res->start - pci->parent_bus_offset;
 		atu.pci_addr = entry->res->start - entry->offset;
 
 		/* Adjust iATU size if MSG TLP region was allocated before */
@@ -779,7 +779,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.parent_bus_addr = pp->io_base;
+			atu.parent_bus_addr = pp->io_base - pci->parent_bus_offset;
 			atu.pci_addr = pp->io_bus_addr;
 			atu.size = pp->io_size;
 
@@ -923,7 +923,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->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 f17a25fe55a5b..8b546131b97f6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -475,9 +475,6 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 	u32 retries, val;
 	u64 limit_addr;
 
-	if (pci->ops && pci->ops->cpu_addr_fixup)
-		parent_bus_addr = pci->ops->cpu_addr_fixup(pci, parent_bus_addr);
-
 	limit_addr = parent_bus_addr + atu->size - 1;
 
 	if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||

-- 
2.34.1

[PATCH v11 10/11] PCI: dwc: Print warning message when cpu_addr_fixup() exists

[Frank Li]

If the parent 'ranges' property in DT correctly describes the address
translation, the cpu_addr_fixup() callback is not needed. Print a warning
message to inform users to correct their DTB files and prepare to remove
cpu_addr_fixup().

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v10 to v11
- change to dev_warn()
- Bjorn: this is opitional patches to encourage user fix their dtb file.

change from v9 to v10
- new patch
---
 drivers/pci/controller/dwc/pcie-designware.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 8b546131b97f6..d4dc8bf06d4c1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -1125,6 +1125,8 @@ int dw_pcie_init_parent_bus_offset(struct dw_pcie *pci, const char *reg_name,
 
 	fixup = pci->ops->cpu_addr_fixup;
 	if (fixup) {
+		dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");
+
 		fixup_addr = fixup(pci, cpu_phy_addr);
 		if (reg_addr == fixup_addr) {
 			dev_warn(dev, "%#010llx %s reg[%d] == %#010llx; %ps is redundant\n",

-- 
2.34.1

[PATCH v11 11/11] 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 v9 to v11
- none

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 v11 04/11] PCI: dwc: Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()

[Bjorn Helgaas]

On Thu, Mar 13, 2025 at 11:38:40AM -0400, Frank Li wrote:
> Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
> Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
> any DWC resource, moving it earlier improves code logic and readability.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index c57831902686e..52a441662cabe 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	raw_spin_lock_init(&pp->lock);
>  
> +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> +	if (!bridge)
> +		return bridge;

This returns NULL (0) where it previously returned -ENOMEM.  Callers
interpret zero as "success", so I think it should stil return -ENOMEM.

I tentatively changed it back to -ENOMEM locally, let me know if
that's wrong.

> +	pp->bridge = bridge;
> +
>  	ret = dw_pcie_get_resources(pci);
>  	if (ret)
>  		return ret;
> @@ -460,12 +466,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		return ret;
>  
> -	bridge = devm_pci_alloc_host_bridge(dev, 0);
> -	if (!bridge)
> -		return -ENOMEM;
> -
> -	pp->bridge = bridge;
> -
>  	/* Get the I/O range from DT */
>  	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
>  	if (win) {
> 
> -- 
> 2.34.1
> 

Re: [PATCH v11 04/11] PCI: dwc: Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()

[Frank Li]

On Thu, Mar 13, 2025 at 02:22:54PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 13, 2025 at 11:38:40AM -0400, Frank Li wrote:
> > Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
> > Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
> > any DWC resource, moving it earlier improves code logic and readability.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c57831902686e..52a441662cabe 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >
> >  	raw_spin_lock_init(&pp->lock);
> >
> > +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > +	if (!bridge)
> > +		return bridge;
>
> This returns NULL (0) where it previously returned -ENOMEM.  Callers
> interpret zero as "success", so I think it should stil return -ENOMEM.

It should be -ENOMEM. Sorry for that. Strange, not sure what happen when
I copy/past code.

Do you need respin it or you can fix it?

Frank

>
> I tentatively changed it back to -ENOMEM locally, let me know if
> that's wrong.
>
> > +	pp->bridge = bridge;
> > +
> >  	ret = dw_pcie_get_resources(pci);
> >  	if (ret)
> >  		return ret;
> > @@ -460,12 +466,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  	if (ret)
> >  		return ret;
> >
> > -	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > -	if (!bridge)
> > -		return -ENOMEM;
> > -
> > -	pp->bridge = bridge;
> > -
> >  	/* Get the I/O range from DT */
> >  	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
> >  	if (win) {
> >
> > --
> > 2.34.1
> >

Re: [PATCH v11 04/11] PCI: dwc: Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init()

[Bjorn Helgaas]

On Thu, Mar 13, 2025 at 04:45:26PM -0400, Frank Li wrote:
> On Thu, Mar 13, 2025 at 02:22:54PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 13, 2025 at 11:38:40AM -0400, Frank Li wrote:
> > > Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().
> > > Since devm_pci_alloc_host_bridge() is common code that doesn't depend on
> > > any DWC resource, moving it earlier improves code logic and readability.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index c57831902686e..52a441662cabe 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -452,6 +452,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >
> > >  	raw_spin_lock_init(&pp->lock);
> > >
> > > +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> > > +	if (!bridge)
> > > +		return bridge;
> >
> > This returns NULL (0) where it previously returned -ENOMEM.  Callers
> > interpret zero as "success", so I think it should stil return -ENOMEM.
> 
> It should be -ENOMEM. Sorry for that. Strange, not sure what happen when
> I copy/past code.
> 
> Do you need respin it or you can fix it?

I fixed it locally.  But you should fix it, too, in case we do another
spin for other reasons.

> > I tentatively changed it back to -ENOMEM locally, let me know if
> > that's wrong.
> >
> > > +	pp->bridge = bridge;
> > > +

Re: [PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Bjorn Helgaas]

On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote:
> The 'ranges' property at PCI controller parent bus can indicate address
> translation information. Most system's bus fabric use 1:1 map between input
> and output address. but some hardware like i.MX8QXP doesn't use 1:1 map.

I think you've used reg["addr_space"] to get the offset for Endpoints
forever.

I just noticed that through v9, you used 'ranges' to get the offset
for the Root Complex (with "Add parent_bus_offset to resource_entry"),
and I think v10 switched to use reg["config"] instead.

I think I originally proposed the idea of "Add parent_bus_offset to
resource_entry" patch, but I think it turned out to be kind of an ugly
approach.

Anyway, IIUC this v11 patch actually uses reg["config"] to compute the
offset, not 'ranges', so we should probably update the subject and
commit log to reflect that, and maybe remove the now-unused bits of
the devicetree example.

I do worry a little bit about the assumption that the offset of
reg["config"] is the same as the offset of the other pieces.  The main
place we use the offset on RCs is for the ATU, and isn't the ATU in
the MemSpace area at 0x8000_0000 below?

It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config"
offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset,
but I don't know that this is guaranteed for all designs.

v9:
  [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry
    https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com
  [PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window
    https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com

v10:
  [PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
    https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com
  [PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window
    https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com

> 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.
> 
> Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset
> is the same for all kinds of address translation.
> 
> Just set parent_bus_offset, but doesn't use it, so no functional change
> intended yet.

> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		pp->io_base = pci_pio_to_address(win->res->start);
>  	}
>  
> +	/*
> +	 * visconti_pcie_cpu_addr_fixup() use pp->io_base,
> +	 * so have to call dw_pcie_init_parent_bus_offset() after init
> +	 * pp->io_base.
> +	 */
> +	ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base);
> +	if (ret)
> +		return ret;
> +
>  	/* Set default bus ops */
>  	bridge->ops = &dw_pcie_ops;
>  	bridge->child_ops = &dw_child_pcie_ops;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Frank Li]

On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote:
> > The 'ranges' property at PCI controller parent bus can indicate address
> > translation information. Most system's bus fabric use 1:1 map between input
> > and output address. but some hardware like i.MX8QXP doesn't use 1:1 map.
>
> I think you've used reg["addr_space"] to get the offset for Endpoints
> forever.

Yes, it still need ranges informaiton at parent bus.

	bus@000
	{
		ranges = <...>; [1] /* still need this */
		pcie {
			ranges = <...>;[2]
		};
		pcie-ep {};
	}

>
> I just noticed that through v9, you used 'ranges' to get the offset
> for the Root Complex (with "Add parent_bus_offset to resource_entry"),
> and I think v10 switched to use reg["config"] instead.
>
> I think I originally proposed the idea of "Add parent_bus_offset to
> resource_entry" patch, but I think it turned out to be kind of an ugly
> approach.
>
> Anyway, IIUC this v11 patch actually uses reg["config"] to compute the
> offset, not 'ranges', so we should probably update the subject and
> commit log to reflect that, and maybe remove the now-unused bits of
> the devicetree example.

We use reg["config"] to detect offset, but still need parent dts's ranges.
There are two ranges, one is at parent pci bus [1], the other is under
'pci bus' [2].

Although use reg["config"], but still need ranges [1]. And information at
ranges [2] also need be correct.

The whole devicetree example also validate to help write address translate
informaiton.

>
> I do worry a little bit about the assumption that the offset of
> reg["config"] is the same as the offset of the other pieces.  The main
> place we use the offset on RCs is for the ATU, and isn't the ATU in
> the MemSpace area at 0x8000_0000 below?

No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT".
Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never
happen.

>
> It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config"
> offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset,
> but I don't know that this is guaranteed for all designs.

So far, it is the same for use dwc chips. If we meet difference, we can
add later.

reg["config"] only simplied our implement base on the offset is the same.
But whole concept is unchanged.

Frank

>
> v9:
>   [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry
>     https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com
>   [PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window
>     https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com
>
> v10:
>   [PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
>     https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com
>   [PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window
>     https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com
>
> > 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.
> >
> > Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset
> > is the same for all kinds of address translation.
> >
> > Just set parent_bus_offset, but doesn't use it, so no functional change
> > intended yet.
>
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  		pp->io_base = pci_pio_to_address(win->res->start);
> >  	}
> >
> > +	/*
> > +	 * visconti_pcie_cpu_addr_fixup() use pp->io_base,
> > +	 * so have to call dw_pcie_init_parent_bus_offset() after init
> > +	 * pp->io_base.
> > +	 */
> > +	ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* Set default bus ops */
> >  	bridge->ops = &dw_pcie_ops;
> >  	bridge->child_ops = &dw_child_pcie_ops;
> >
> > --
> > 2.34.1
> >

Re: [PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Frank Li]

On Thu, Mar 13, 2025 at 06:56:17PM -0400, Frank Li wrote:
> On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote:
> > > The 'ranges' property at PCI controller parent bus can indicate address
> > > translation information. Most system's bus fabric use 1:1 map between input
> > > and output address. but some hardware like i.MX8QXP doesn't use 1:1 map.
> >
> > I think you've used reg["addr_space"] to get the offset for Endpoints
> > forever.
>
> Yes, it still need ranges informaiton at parent bus.
>
> 	bus@000
> 	{
> 		ranges = <...>; [1] /* still need this */
> 		pcie {
> 			ranges = <...>;[2]
> 		};
> 		pcie-ep {};
> 	}
>
> >
> > I just noticed that through v9, you used 'ranges' to get the offset
> > for the Root Complex (with "Add parent_bus_offset to resource_entry"),
> > and I think v10 switched to use reg["config"] instead.
> >
> > I think I originally proposed the idea of "Add parent_bus_offset to
> > resource_entry" patch, but I think it turned out to be kind of an ugly
> > approach.
> >
> > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the
> > offset, not 'ranges', so we should probably update the subject and
> > commit log to reflect that, and maybe remove the now-unused bits of
> > the devicetree example.
>
> We use reg["config"] to detect offset, but still need parent dts's ranges.
> There are two ranges, one is at parent pci bus [1], the other is under
> 'pci bus' [2].

Beside, luckly dwc use reg["config"] to indicate config space. but dt also
define ranges [2] under pcie node, which can also include 'config's space.

cadence also use reg["cfg"] to do that.
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");

I am not sure why both choose use reg[] instead of [2]ranges under
pcie node. But the result make our situation simpler.

Frank

>
> Although use reg["config"], but still need ranges [1]. And information at
> ranges [2] also need be correct.
>
> The whole devicetree example also validate to help write address translate
> informaiton.
>
> >
> > I do worry a little bit about the assumption that the offset of
> > reg["config"] is the same as the offset of the other pieces.  The main
> > place we use the offset on RCs is for the ATU, and isn't the ATU in
> > the MemSpace area at 0x8000_0000 below?
>
> No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT".
> Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never
> happen.
>
> >
> > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config"
> > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset,
> > but I don't know that this is guaranteed for all designs.
>
> So far, it is the same for use dwc chips. If we meet difference, we can
> add later.
>
> reg["config"] only simplied our implement base on the offset is the same.
> But whole concept is unchanged.
>
> Frank
>
> >
> > v9:
> >   [PATCH v9 3/7] PCI: Add parent_bus_offset to resource_entry
> >     https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-3-3c4bb506f665@nxp.com
> >   [PATCH v9 5/7] PCI: dwc: ep: Add parent_bus_addr for outbound window
> >     https://lore.kernel.org/r/20250128-pci_fixup_addr-v9-5-3c4bb506f665@nxp.com
> >
> > v10:
> >   [PATCH v10 05/10] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
> >     https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-5-409dafc950d1@nxp.com
> >   [PATCH v10 06/10] PCI: dwc: ep: Add parent_bus_addr for outbound window
> >     https://lore.kernel.org/r/20250310-pci_fixup_addr-v10-6-409dafc950d1@nxp.com
> >
> > > 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.
> > >
> > > Use reg-name "config" to detect parent_bus_addr_offset. Suppose the offset
> > > is the same for all kinds of address translation.
> > >
> > > Just set parent_bus_offset, but doesn't use it, so no functional change
> > > intended yet.
> >
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -474,6 +474,15 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  		pp->io_base = pci_pio_to_address(win->res->start);
> > >  	}
> > >
> > > +	/*
> > > +	 * visconti_pcie_cpu_addr_fixup() use pp->io_base,
> > > +	 * so have to call dw_pcie_init_parent_bus_offset() after init
> > > +	 * pp->io_base.
> > > +	 */
> > > +	ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	/* Set default bus ops */
> > >  	bridge->ops = &dw_pcie_ops;
> > >  	bridge->child_ops = &dw_child_pcie_ops;
> > >
> > > --
> > > 2.34.1
> > >

Re: [PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Bjorn Helgaas]

On Fri, Mar 14, 2025 at 11:21:19AM -0400, Frank Li wrote:
> On Thu, Mar 13, 2025 at 06:56:17PM -0400, Frank Li wrote:
> > On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote:
> > > > The 'ranges' property at PCI controller parent bus can indicate address
> > > > translation information. Most system's bus fabric use 1:1 map between input
> > > > and output address. but some hardware like i.MX8QXP doesn't use 1:1 map.
> > >
> > > I think you've used reg["addr_space"] to get the offset for Endpoints
> > > forever.
> >
> > Yes, it still need ranges informaiton at parent bus.
> >
> > 	bus@000
> > 	{
> > 		ranges = <...>; [1] /* still need this */
> > 		pcie {
> > 			ranges = <...>;[2]
> > 		};
> > 		pcie-ep {};
> > 	}

Yes, of course. I'm just making the point that the subject/commit log
says this patch uses 'ranges' but in fact it uses 'reg'.

> > > I just noticed that through v9, you used 'ranges' to get the offset
> > > for the Root Complex (with "Add parent_bus_offset to resource_entry"),
> > > and I think v10 switched to use reg["config"] instead.
> > >
> > > I think I originally proposed the idea of "Add parent_bus_offset to
> > > resource_entry" patch, but I think it turned out to be kind of an ugly
> > > approach.
> > >
> > > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the
> > > offset, not 'ranges', so we should probably update the subject and
> > > commit log to reflect that, and maybe remove the now-unused bits of
> > > the devicetree example.
> >
> > We use reg["config"] to detect offset, but still need parent dts's ranges.
> > There are two ranges, one is at parent pci bus [1], the other is under
> > 'pci bus' [2].
> 
> Beside, luckly dwc use reg["config"] to indicate config space. but dt also
> define ranges [2] under pcie node, which can also include 'config's space.
> 
> cadence also use reg["cfg"] to do that.
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> 
> I am not sure why both choose use reg[] instead of [2]ranges under
> pcie node. But the result make our situation simpler.
> 
> > Although use reg["config"], but still need ranges [1]. And information at
> > ranges [2] also need be correct.
> >
> > The whole devicetree example also validate to help write address translate
> > informaiton.
> >
> > > I do worry a little bit about the assumption that the offset of
> > > reg["config"] is the same as the offset of the other pieces.  The main
> > > place we use the offset on RCs is for the ATU, and isn't the ATU in
> > > the MemSpace area at 0x8000_0000 below?
> >
> > No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT".
> > Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never
> > happen.

Minor miscommunication, I think.   I didn't mean there were addresses
smaller than 0x8000_0000; I meant that in the picture, MemSpace at
0x8000_0000 is below CfgSpace at 0x8ff0_0000.  The important point is
that CfgSpace is a separate region from MemSpace, and we're applying
the CfgSpace offset to the ATU in MemSpace.

I think it's OK to assume that for now.  AFAICS there is nothing in
devicetree that explicitly mentions the ATU input address space; it's
just implicitly part of the intermediate address space described by
the bus@5f000000 'ranges'.

> > > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config"
> > > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset,
> > > but I don't know that this is guaranteed for all designs.
> >
> > So far, it is the same for use dwc chips. If we meet difference, we can
> > add later.
> >
> > reg["config"] only simplied our implement base on the offset is the same.
> > But whole concept is unchanged.

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

Of course we need this 'ranges' to describe the translation between
intermediate addresses and PCI bus addresses.  My point is that this
is not relevant to the parent bus offset we're computing in this
patch.

So I think for purposes of this patch, we can omit pcie@5f010000
#address-cells and everything after it.

Bjorn

Re: [PATCH v11 06/11] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback

[Frank Li]

On Fri, Mar 14, 2025 at 05:10:59PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 14, 2025 at 11:21:19AM -0400, Frank Li wrote:
> > On Thu, Mar 13, 2025 at 06:56:17PM -0400, Frank Li wrote:
> > > On Thu, Mar 13, 2025 at 05:04:50PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Mar 13, 2025 at 11:38:42AM -0400, Frank Li wrote:
> > > > > The 'ranges' property at PCI controller parent bus can indicate address
> > > > > translation information. Most system's bus fabric use 1:1 map between input
> > > > > and output address. but some hardware like i.MX8QXP doesn't use 1:1 map.
> > > >
> > > > I think you've used reg["addr_space"] to get the offset for Endpoints
> > > > forever.
> > >
> > > Yes, it still need ranges informaiton at parent bus.
> > >
> > > 	bus@000
> > > 	{
> > > 		ranges = <...>; [1] /* still need this */
> > > 		pcie {
> > > 			ranges = <...>;[2]
> > > 		};
> > > 		pcie-ep {};
> > > 	}
>
> Yes, of course. I'm just making the point that the subject/commit log
> says this patch uses 'ranges' but in fact it uses 'reg'.

Actaully my means: 'ranges' in subject means parent bus's ranges. Anyway
how about:

Use reg['config'] dettect parent_bus_offset to get rid of cpu_addr_fixup() callback

>
> > > > I just noticed that through v9, you used 'ranges' to get the offset
> > > > for the Root Complex (with "Add parent_bus_offset to resource_entry"),
> > > > and I think v10 switched to use reg["config"] instead.
> > > >
> > > > I think I originally proposed the idea of "Add parent_bus_offset to
> > > > resource_entry" patch, but I think it turned out to be kind of an ugly
> > > > approach.
> > > >
> > > > Anyway, IIUC this v11 patch actually uses reg["config"] to compute the
> > > > offset, not 'ranges', so we should probably update the subject and
> > > > commit log to reflect that, and maybe remove the now-unused bits of
> > > > the devicetree example.
> > >
> > > We use reg["config"] to detect offset, but still need parent dts's ranges.
> > > There are two ranges, one is at parent pci bus [1], the other is under
> > > 'pci bus' [2].
> >
> > Beside, luckly dwc use reg["config"] to indicate config space. but dt also
> > define ranges [2] under pcie node, which can also include 'config's space.
> >
> > cadence also use reg["cfg"] to do that.
> > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> >
> > I am not sure why both choose use reg[] instead of [2]ranges under
> > pcie node. But the result make our situation simpler.
> >
> > > Although use reg["config"], but still need ranges [1]. And information at
> > > ranges [2] also need be correct.
> > >
> > > The whole devicetree example also validate to help write address translate
> > > informaiton.
> > >
> > > > I do worry a little bit about the assumption that the offset of
> > > > reg["config"] is the same as the offset of the other pieces.  The main
> > > > place we use the offset on RCs is for the ATU, and isn't the ATU in
> > > > the MemSpace area at 0x8000_0000 below?
> > >
> > > No, "Bus fabric" only decode input address from "0x7000_0000..UPLIMIT".
> > > Then output address to 0x8000_0000..UPLIMIT. So below 0x8000_0000 never
> > > happen.
>
> Minor miscommunication, I think.   I didn't mean there were addresses
> smaller than 0x8000_0000; I meant that in the picture, MemSpace at
> 0x8000_0000 is below CfgSpace at 0x8ff0_0000.  The important point is
> that CfgSpace is a separate region from MemSpace, and we're applying
> the CfgSpace offset to the ATU in MemSpace.
>
> I think it's OK to assume that for now.  AFAICS there is nothing in
> devicetree that explicitly mentions the ATU input address space; it's
> just implicitly part of the intermediate address space described by
> the bus@5f000000 'ranges'.

Actually 'ranges' means address tranlation from child node to parent node.
that should means address to ATU input. It will be good if docuemnt
somewhere. Do you think where should document?

>
> > > > It's great that in this case the 0x7ff0_0000 to 0x8ff0_0000 "config"
> > > > offset is the same as the 0x7000_0000 to 0x8000_0000 MemSpace offset,
> > > > but I don't know that this is guaranteed for all designs.
> > >
> > > So far, it is the same for use dwc chips. If we meet difference, we can
> > > add later.
> > >
> > > reg["config"] only simplied our implement base on the offset is the same.
> > > But whole concept is unchanged.
>
> > > > > 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>;
>
> Of course we need this 'ranges' to describe the translation between
> intermediate addresses and PCI bus addresses.  My point is that this
> is not relevant to the parent bus offset we're computing in this
> patch.
>
> So I think for purposes of this patch, we can omit pcie@5f010000
> #address-cells and everything after it.
>

Okay, we can remove it.

Frank

> Bjorn

Re: [PATCH v11 10/11] PCI: dwc: Print warning message when cpu_addr_fixup() exists

[Manivannan Sadhasivam]

On Thu, Mar 13, 2025 at 11:38:46AM -0400, Frank Li wrote:
> If the parent 'ranges' property in DT correctly describes the address
> translation, the cpu_addr_fixup() callback is not needed. Print a warning
> message to inform users to correct their DTB files and prepare to remove
> cpu_addr_fixup().
> 

This patch seem to have dropped, but I do see a value in printing the warning to
encourage developers/users to fix the DTB in some way. Since we fixed the driver
to parse the DT 'ranges' properly, the presence of cpu_addr_fixup() callback
indicates that the translation is not properly described in DT. So DT has to be
fixed.

Bjorn, thoughts?

- Mani

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change from v10 to v11
> - change to dev_warn()
> - Bjorn: this is opitional patches to encourage user fix their dtb file.
> 
> change from v9 to v10
> - new patch
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 8b546131b97f6..d4dc8bf06d4c1 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1125,6 +1125,8 @@ int dw_pcie_init_parent_bus_offset(struct dw_pcie *pci, const char *reg_name,
>  
>  	fixup = pci->ops->cpu_addr_fixup;
>  	if (fixup) {
> +		dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");
> +
>  		fixup_addr = fixup(pci, cpu_phy_addr);
>  		if (reg_addr == fixup_addr) {
>  			dev_warn(dev, "%#010llx %s reg[%d] == %#010llx; %ps is redundant\n",
> 
> -- 
> 2.34.1
> 

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

Re: [PATCH v11 10/11] PCI: dwc: Print warning message when cpu_addr_fixup() exists

[Frank Li]

On Thu, Jun 12, 2025 at 08:16:03PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 13, 2025 at 11:38:46AM -0400, Frank Li wrote:
> > If the parent 'ranges' property in DT correctly describes the address
> > translation, the cpu_addr_fixup() callback is not needed. Print a warning
> > message to inform users to correct their DTB files and prepare to remove
> > cpu_addr_fixup().
> >
>
> This patch seem to have dropped, but I do see a value in printing the warning to
> encourage developers/users to fix the DTB in some way. Since we fixed the driver
> to parse the DT 'ranges' properly, the presence of cpu_addr_fixup() callback
> indicates that the translation is not properly described in DT. So DT has to be
> fixed.

This patch already in mainline with Bjorn's fine tuned at when merge.

	fixup = pci->ops ? pci->ops->cpu_addr_fixup : NULL;
        if (fixup) {
                fixup_addr = fixup(pci, cpu_phys_addr);
                if (reg_addr == fixup_addr) {
                        dev_info(dev, "%s reg[%d] %#010llx == %#010llx == fixup(cpu %#010llx); %ps is redundant with this devicetree\n",
                                 reg_name, index, reg_addr, fixup_addr,
                                 (unsigned long long) cpu_phys_addr, fixup);
                } else {
                        dev_warn(dev, "%s reg[%d] %#010llx != %#010llx == fixup(cpu %#010llx); devicetree is broken\n",
                                 reg_name, index, reg_addr, fixup_addr,
                                 (unsigned long long) cpu_phys_addr);
                        reg_addr = fixup_addr;
                }

                return cpu_phys_addr - reg_addr;
        }

I have not seen this "dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");"

Frank

>
> Bjorn, thoughts?
>
> - Mani
>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v10 to v11
> > - change to dev_warn()
> > - Bjorn: this is opitional patches to encourage user fix their dtb file.
> >
> > change from v9 to v10
> > - new patch
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 8b546131b97f6..d4dc8bf06d4c1 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -1125,6 +1125,8 @@ int dw_pcie_init_parent_bus_offset(struct dw_pcie *pci, const char *reg_name,
> >
> >  	fixup = pci->ops->cpu_addr_fixup;
> >  	if (fixup) {
> > +		dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");
> > +
> >  		fixup_addr = fixup(pci, cpu_phy_addr);
> >  		if (reg_addr == fixup_addr) {
> >  			dev_warn(dev, "%#010llx %s reg[%d] == %#010llx; %ps is redundant\n",
> >
> > --
> > 2.34.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v11 10/11] PCI: dwc: Print warning message when cpu_addr_fixup() exists

[Manivannan Sadhasivam]

On Thu, Jun 12, 2025 at 11:51:56AM -0400, Frank Li wrote:
> On Thu, Jun 12, 2025 at 08:16:03PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Mar 13, 2025 at 11:38:46AM -0400, Frank Li wrote:
> > > If the parent 'ranges' property in DT correctly describes the address
> > > translation, the cpu_addr_fixup() callback is not needed. Print a warning
> > > message to inform users to correct their DTB files and prepare to remove
> > > cpu_addr_fixup().
> > >
> >
> > This patch seem to have dropped, but I do see a value in printing the warning to
> > encourage developers/users to fix the DTB in some way. Since we fixed the driver
> > to parse the DT 'ranges' properly, the presence of cpu_addr_fixup() callback
> > indicates that the translation is not properly described in DT. So DT has to be
> > fixed.
> 
> This patch already in mainline with Bjorn's fine tuned at when merge.
> 
> 	fixup = pci->ops ? pci->ops->cpu_addr_fixup : NULL;
>         if (fixup) {
>                 fixup_addr = fixup(pci, cpu_phys_addr);
>                 if (reg_addr == fixup_addr) {
>                         dev_info(dev, "%s reg[%d] %#010llx == %#010llx == fixup(cpu %#010llx); %ps is redundant with this devicetree\n",
>                                  reg_name, index, reg_addr, fixup_addr,
>                                  (unsigned long long) cpu_phys_addr, fixup);
>                 } else {
>                         dev_warn(dev, "%s reg[%d] %#010llx != %#010llx == fixup(cpu %#010llx); devicetree is broken\n",
>                                  reg_name, index, reg_addr, fixup_addr,
>                                  (unsigned long long) cpu_phys_addr);
>                         reg_addr = fixup_addr;
>                 }
> 
>                 return cpu_phys_addr - reg_addr;
>         }
> 
> I have not seen this "dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");"
> 

This patch is supposed to add this warning and nothing else.

- Mani

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

Re: [PATCH v11 10/11] PCI: dwc: Print warning message when cpu_addr_fixup() exists

[Frank Li]

On Thu, Jun 12, 2025 at 09:38:32PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 12, 2025 at 11:51:56AM -0400, Frank Li wrote:
> > On Thu, Jun 12, 2025 at 08:16:03PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Mar 13, 2025 at 11:38:46AM -0400, Frank Li wrote:
> > > > If the parent 'ranges' property in DT correctly describes the address
> > > > translation, the cpu_addr_fixup() callback is not needed. Print a warning
> > > > message to inform users to correct their DTB files and prepare to remove
> > > > cpu_addr_fixup().
> > > >
> > >
> > > This patch seem to have dropped, but I do see a value in printing the warning to
> > > encourage developers/users to fix the DTB in some way. Since we fixed the driver
> > > to parse the DT 'ranges' properly, the presence of cpu_addr_fixup() callback
> > > indicates that the translation is not properly described in DT. So DT has to be
> > > fixed.
> >
> > This patch already in mainline with Bjorn's fine tuned at when merge.
> >
> > 	fixup = pci->ops ? pci->ops->cpu_addr_fixup : NULL;
> >         if (fixup) {
> >                 fixup_addr = fixup(pci, cpu_phys_addr);
> >                 if (reg_addr == fixup_addr) {
> >                         dev_info(dev, "%s reg[%d] %#010llx == %#010llx == fixup(cpu %#010llx); %ps is redundant with this devicetree\n",
> >                                  reg_name, index, reg_addr, fixup_addr,
> >                                  (unsigned long long) cpu_phys_addr, fixup);
> >                 } else {
> >                         dev_warn(dev, "%s reg[%d] %#010llx != %#010llx == fixup(cpu %#010llx); devicetree is broken\n",
> >                                  reg_name, index, reg_addr, fixup_addr,
> >                                  (unsigned long long) cpu_phys_addr);
> >                         reg_addr = fixup_addr;
> >                 }
> >
> >                 return cpu_phys_addr - reg_addr;
> >         }
> >
> > I have not seen this "dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");"
> >
>
> This patch is supposed to add this warning and nothing else.

We can forget this one. Can help check doorbell patch if you have time

https://lore.kernel.org/imx/202506101649.UpwblcVd-lkp@intel.com/T/#t

Frank

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

Re: [PATCH v11 10/11] PCI: dwc: Print warning message when cpu_addr_fixup() exists

[Manivannan Sadhasivam]

On Thu, Jun 12, 2025 at 12:19:04PM -0400, Frank Li wrote:
> On Thu, Jun 12, 2025 at 09:38:32PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 12, 2025 at 11:51:56AM -0400, Frank Li wrote:
> > > On Thu, Jun 12, 2025 at 08:16:03PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Mar 13, 2025 at 11:38:46AM -0400, Frank Li wrote:
> > > > > If the parent 'ranges' property in DT correctly describes the address
> > > > > translation, the cpu_addr_fixup() callback is not needed. Print a warning
> > > > > message to inform users to correct their DTB files and prepare to remove
> > > > > cpu_addr_fixup().
> > > > >
> > > >
> > > > This patch seem to have dropped, but I do see a value in printing the warning to
> > > > encourage developers/users to fix the DTB in some way. Since we fixed the driver
> > > > to parse the DT 'ranges' properly, the presence of cpu_addr_fixup() callback
> > > > indicates that the translation is not properly described in DT. So DT has to be
> > > > fixed.
> > >
> > > This patch already in mainline with Bjorn's fine tuned at when merge.
> > >
> > > 	fixup = pci->ops ? pci->ops->cpu_addr_fixup : NULL;
> > >         if (fixup) {
> > >                 fixup_addr = fixup(pci, cpu_phys_addr);
> > >                 if (reg_addr == fixup_addr) {
> > >                         dev_info(dev, "%s reg[%d] %#010llx == %#010llx == fixup(cpu %#010llx); %ps is redundant with this devicetree\n",
> > >                                  reg_name, index, reg_addr, fixup_addr,
> > >                                  (unsigned long long) cpu_phys_addr, fixup);
> > >                 } else {
> > >                         dev_warn(dev, "%s reg[%d] %#010llx != %#010llx == fixup(cpu %#010llx); devicetree is broken\n",
> > >                                  reg_name, index, reg_addr, fixup_addr,
> > >                                  (unsigned long long) cpu_phys_addr);
> > >                         reg_addr = fixup_addr;
> > >                 }
> > >
> > >                 return cpu_phys_addr - reg_addr;
> > >         }
> > >
> > > I have not seen this "dev_warn(pci->dev, "cpu_addr_fixup() usage detected. Please fix your DTB!\n");"
> > >
> >
> > This patch is supposed to add this warning and nothing else.
> 
> We can forget this one. Can help check doorbell patch if you have time
> 
> https://lore.kernel.org/imx/202506101649.UpwblcVd-lkp@intel.com/T/#t
> 

I'm going through the queue. Will get to it.

- Mani

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