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

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

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is a v12 based on Frank's v11 series.

v11 https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-0-01d2313502ab@nxp.com
    
Changes from v11:
  - Call devm_pci_alloc_host_bridge() early in dw_pcie_host_init(), before
    any devicetree-related code
  - Call devm_pci_epc_create() early in dw_pcie_ep_init(), before any
    devicetree-related code
  - Consolidate devicetree-related code in dw_pcie_host_get_resources() and
    dw_pcie_ep_get_resources()
  - Integrate dw_pcie_cfg0_setup() into dw_pcie_host_get_resources()
  - Convert dw_pcie_init_parent_bus_offset() to dw_pcie_parent_bus_offset()
    which returns the offset rather than setting it internally
  - Split the debug comparison of devicetree info with .cpu_addr_fixup() to
    separate patch so we can easily revert it later
  - Drop "cpu_addr_fixup() usage detected" warning since we always warn
    about something in that case anyway

Any comments welcome.


Bjorn Helgaas (3):
  PCI: dwc: Consolidate devicetree handling in
    dw_pcie_host_get_resources()
  PCI: dwc: ep: Call epc_create() early in dw_pcie_ep_init()
  PCI: dwc: ep: Consolidate devicetree handling in
    dw_pcie_ep_get_resources()

Frank Li (10):
  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: Call devm_pci_alloc_host_bridge() early in
    dw_pcie_host_init()
  PCI: dwc: Add dw_pcie_parent_bus_offset()
  PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug
  PCI: dwc: Use devicetree 'reg[config]' to derive CPU -> ATU addr
    offset
  PCI: dwc: ep: Use devicetree 'reg[addr_space]' to derive CPU -> ATU
    addr offset
  PCI: dwc: ep: Ensure proper iteration over outbound map windows
  PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()
  PCI: imx6: Remove cpu_addr_fixup()

 drivers/pci/controller/dwc/pci-imx6.c         | 18 +---
 .../pci/controller/dwc/pcie-designware-ep.c   | 74 +++++++++++------
 .../pci/controller/dwc/pcie-designware-host.c | 57 ++++++++-----
 drivers/pci/controller/dwc/pcie-designware.c  | 82 ++++++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.h  | 24 +++++-
 5 files changed, 171 insertions(+), 84 deletions(-)

-- 
2.34.1

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

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

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.

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-1-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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 ffaded8f2df7..ae3fd2a5dbf8 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 v12 02/13] PCI: dwc: Rename cpu_addr to parent_bus_addr for ATU configuration

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

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.

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-2-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   |  8 ++---
 .../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 8e07d432e74f..80ac2f9e88eb 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 ae3fd2a5dbf8..1206b26bff3f 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 145e7f579072..9d0a5f75effc 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 501d9ddfea16..d0d8c622a6e8 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 v12 03/13] PCI: dwc: Call devm_pci_alloc_host_bridge() early in dw_pcie_host_init()

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

Move devm_pci_alloc_host_bridge() to the beginning of dw_pcie_host_init().

devm_pci_alloc_host_bridge() is generic code that doesn't depend on any DWC
resource, so moving it earlier keeps all the subsequent devicetree-related
code together.

[bhelgaas: reorder earlier in series]
Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-4-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.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 1206b26bff3f..5636243fb90e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -431,6 +431,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 -ENOMEM;
+
+	pp->bridge = bridge;
+
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
 		return ret;
@@ -448,12 +454,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (IS_ERR(pp->va_cfg0_base))
 		return PTR_ERR(pp->va_cfg0_base);
 
-	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 v12 04/13] PCI: dwc: Consolidate devicetree handling in dw_pcie_host_get_resources()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Consolidate devicetree resource handling in dw_pcie_host_get_resources().
No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 37 +++++++++++++------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 5636243fb90e..9ce06b1ee266 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -418,25 +418,15 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 	}
 }
 
-int dw_pcie_host_init(struct dw_pcie_rp *pp)
+static int dw_pcie_host_get_resources(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);
-
-	bridge = devm_pci_alloc_host_bridge(dev, 0);
-	if (!bridge)
-		return -ENOMEM;
-
-	pp->bridge = bridge;
-
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
 		return ret;
@@ -455,13 +445,36 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		return PTR_ERR(pp->va_cfg0_base);
 
 	/* Get the I/O range from DT */
-	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
+	win = resource_list_first_type(&pp->bridge->windows, IORESOURCE_IO);
 	if (win) {
 		pp->io_size = resource_size(win->res);
 		pp->io_bus_addr = win->res->start - win->offset;
 		pp->io_base = pci_pio_to_address(win->res->start);
 	}
 
+	return 0;
+}
+
+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 pci_host_bridge *bridge;
+	int ret;
+
+	raw_spin_lock_init(&pp->lock);
+
+	bridge = devm_pci_alloc_host_bridge(dev, 0);
+	if (!bridge)
+		return -ENOMEM;
+
+	pp->bridge = bridge;
+
+	ret = dw_pcie_host_get_resources(pp);
+	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 v12 04/13] PCI: dwc: Consolidate devicetree handling in dw_pcie_host_get_resources()

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:39PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Consolidate devicetree resource handling in dw_pcie_host_get_resources().
> No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 37 +++++++++++++------
>  1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 5636243fb90e..9ce06b1ee266 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -418,25 +418,15 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
>  	}
>  }
>
> -int dw_pcie_host_init(struct dw_pcie_rp *pp)
> +static int dw_pcie_host_get_resources(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);
> -
> -	bridge = devm_pci_alloc_host_bridge(dev, 0);
> -	if (!bridge)
> -		return -ENOMEM;
> -
> -	pp->bridge = bridge;
> -
>  	ret = dw_pcie_get_resources(pci);
>  	if (ret)
>  		return ret;
> @@ -455,13 +445,36 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  		return PTR_ERR(pp->va_cfg0_base);
>
>  	/* Get the I/O range from DT */
> -	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
> +	win = resource_list_first_type(&pp->bridge->windows, IORESOURCE_IO);
>  	if (win) {
>  		pp->io_size = resource_size(win->res);
>  		pp->io_bus_addr = win->res->start - win->offset;
>  		pp->io_base = pci_pio_to_address(win->res->start);
>  	}
>
> +	return 0;
> +}
> +
> +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 pci_host_bridge *bridge;
> +	int ret;
> +
> +	raw_spin_lock_init(&pp->lock);
> +
> +	bridge = devm_pci_alloc_host_bridge(dev, 0);
> +	if (!bridge)
> +		return -ENOMEM;
> +
> +	pp->bridge = bridge;
> +
> +	ret = dw_pcie_host_get_resources(pp);
> +	if (ret)
> +		return ret;
> +
>  	/* Set default bus ops */
>  	bridge->ops = &dw_pcie_ops;
>  	bridge->child_ops = &dw_child_pcie_ops;
> --
> 2.34.1
>

[PATCH v12 05/13] PCI: dwc: Add dw_pcie_parent_bus_offset()

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

Return the offset from CPU physical address to the parent bus address of
the specified element of the devicetree 'reg' property.

[bhelgaas: return offset, split .cpu_addr_fixup() checking and debug to
separate patch]
Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 23 ++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h |  3 +++
 2 files changed, 26 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 9d0a5f75effc..0a35e36da703 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,25 @@ void dw_pcie_setup(struct dw_pcie *pci)
 
 	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
 }
+
+resource_size_t dw_pcie_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;
+	int index;
+	u64 reg_addr;
+
+	/* Look up reg_name address on parent bus */
+	index = of_property_match_string(np, "reg-names", reg_name);
+
+	if (index < 0) {
+		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);
+		return 0;
+	}
+
+	of_property_read_reg(np, index, &reg_addr, NULL);
+
+	return cpu_phy_addr - reg_addr;
+}
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index d0d8c622a6e8..16548b01347d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -500,6 +500,9 @@ 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);
+resource_size_t dw_pcie_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

Re: [PATCH v12 05/13] PCI: dwc: Add dw_pcie_parent_bus_offset()

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:40PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.com>
>
> Return the offset from CPU physical address to the parent bus address of
> the specified element of the devicetree 'reg' property.
>
> [bhelgaas: return offset, split .cpu_addr_fixup() checking and debug to
> separate patch]
> Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---

look good!

>  drivers/pci/controller/dwc/pcie-designware.c | 23 ++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h |  3 +++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 9d0a5f75effc..0a35e36da703 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,25 @@ void dw_pcie_setup(struct dw_pcie *pci)
>
>  	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
>  }
> +
> +resource_size_t dw_pcie_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;
> +	int index;
> +	u64 reg_addr;
> +
> +	/* Look up reg_name address on parent bus */
> +	index = of_property_match_string(np, "reg-names", reg_name);
> +
> +	if (index < 0) {
> +		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);
> +		return 0;
> +	}
> +
> +	of_property_read_reg(np, index, &reg_addr, NULL);
> +
> +	return cpu_phy_addr - reg_addr;
> +}
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index d0d8c622a6e8..16548b01347d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -500,6 +500,9 @@ 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);
> +resource_size_t dw_pcie_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
>

Re: [PATCH v12 05/13] PCI: dwc: Add dw_pcie_parent_bus_offset()

[Manivannan Sadhasivam]

On Sat, Mar 15, 2025 at 03:15:40PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.com>
> 
> Return the offset from CPU physical address to the parent bus address of
> the specified element of the devicetree 'reg' property.
> 
> [bhelgaas: return offset, split .cpu_addr_fixup() checking and debug to
> separate patch]
> Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 23 ++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h |  3 +++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 9d0a5f75effc..0a35e36da703 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,25 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  
>  	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
>  }
> +
> +resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> +					  const char *reg_name,
> +					  resource_size_t cpu_phy_addr)
> +{

s/cpu_phy_addr/cpu_phys_addr/g

'phy' usually refers to the physical layer IP block. So 'cpu_phy_addr' sounds
like the address of the CPU PHY.

> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
> +	int index;
> +	u64 reg_addr;
> +
> +	/* Look up reg_name address on parent bus */

'parent bus' is not accurate as the below code checks for the 'reg_name' in
current PCI controller node.

> +	index = of_property_match_string(np, "reg-names", reg_name);
> +
> +	if (index < 0) {
> +		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);

Both of these callers are checking for the existence of the 'reg_name' property
before calling this API. So this check seems to be redundant (for now).

- Mani

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

Re: [PATCH v12 05/13] PCI: dwc: Add dw_pcie_parent_bus_offset()

[Bjorn Helgaas]

On Mon, Mar 24, 2025 at 10:48:23PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Mar 15, 2025 at 03:15:40PM -0500, Bjorn Helgaas wrote:
> > From: Frank Li <Frank.Li@nxp.com>
> > 
> > Return the offset from CPU physical address to the parent bus address of
> > the specified element of the devicetree 'reg' property.

> > +resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > +					  const char *reg_name,
> > +					  resource_size_t cpu_phy_addr)
> > +{
> 
> s/cpu_phy_addr/cpu_phys_addr/g

Fixed, thanks!

> > +	struct device *dev = pci->dev;
> > +	struct device_node *np = dev->of_node;
> > +	int index;
> > +	u64 reg_addr;
> > +
> > +	/* Look up reg_name address on parent bus */
> 
> 'parent bus' is not accurate as the below code checks for the 'reg_name' in
> current PCI controller node.

We want the address of "reg_name" on the node's primary side.  We've
been calling that the "parent bus address", I guess because it's the
address on the "parent bus" of the node.

I'm not sure what the best term is for this.  Do you have a
suggestion?

If "parent bus address" is the wrong term, maybe we need to rename
dw_pcie_parent_bus_offset() itself?  

Currently we pass in cpu_phys_addr, but this function doesn't need it
except for the debug code added later.  I would really rather have
something like this in the callers:

  pci->parent_bus_offset = pp->cfg0_base -
      dw_pcie_parent_bus_addr(pci, "config");

because then the offset is computed sort of at the same level where
it's used, and a grep for "cfg0_base" would find both the set and the
use and they would be easy to match up.

> > +	index = of_property_match_string(np, "reg-names", reg_name);
> > +
> > +	if (index < 0) {
> > +		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);
> 
> Both of these callers are checking for the existence of the
> 'reg_name' property before calling this API. So this check seems to
> be redundant (for now).

True, but I don't see a way to enforce the caller checks.  I don't
like the idea of calling of_property_read_reg(np, index, ...) where we
have to look the caller to verify that "index" is valid.

Bjorn

Re: [PATCH v12 05/13] PCI: dwc: Add dw_pcie_parent_bus_offset()

[Manivannan Sadhasivam]

On Mon, Mar 24, 2025 at 01:28:27PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 24, 2025 at 10:48:23PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Mar 15, 2025 at 03:15:40PM -0500, Bjorn Helgaas wrote:
> > > From: Frank Li <Frank.Li@nxp.com>
> > > 
> > > Return the offset from CPU physical address to the parent bus address of
> > > the specified element of the devicetree 'reg' property.
> 
> > > +resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > > +					  const char *reg_name,
> > > +					  resource_size_t cpu_phy_addr)
> > > +{
> > 
> > s/cpu_phy_addr/cpu_phys_addr/g
> 
> Fixed, thanks!
> 
> > > +	struct device *dev = pci->dev;
> > > +	struct device_node *np = dev->of_node;
> > > +	int index;
> > > +	u64 reg_addr;
> > > +
> > > +	/* Look up reg_name address on parent bus */
> > 
> > 'parent bus' is not accurate as the below code checks for the 'reg_name' in
> > current PCI controller node.
> 
> We want the address of "reg_name" on the node's primary side.  We've
> been calling that the "parent bus address", I guess because it's the
> address on the "parent bus" of the node.
> 

Yeah, 'parent bus address' sounds bogus to me. 'ranges' property is described
as:

	ranges = <child_addr parent_addr child_size>

Here, child_addr refers to the PCIe host controller's view of the address space
and parent_addr refers to the CPU's view of the address space.

So the register address described in the PCIe controller node is not a 'parent
bus address'.

> I'm not sure what the best term is for this.  Do you have a
> suggestion?
> 

We are just extracting the offset between translated (cpu_phy_addr) and
untranslated (reg_addr) addresses of a specific register. Maybe the function
should just return the 'untranslated address' and the caller should compute the
offset to make it simple?

> If "parent bus address" is the wrong term, maybe we need to rename
> dw_pcie_parent_bus_offset() itself?  
> 

Yes!

> Currently we pass in cpu_phys_addr, but this function doesn't need it
> except for the debug code added later.  I would really rather have
> something like this in the callers:
> 
>   pci->parent_bus_offset = pp->cfg0_base -
>       dw_pcie_parent_bus_addr(pci, "config");
> 

I agree. This should become, dw_pcie_get_untranslated_addr().

> because then the offset is computed sort of at the same level where
> it's used, and a grep for "cfg0_base" would find both the set and the
> use and they would be easy to match up.
> 
> > > +	index = of_property_match_string(np, "reg-names", reg_name);
> > > +
> > > +	if (index < 0) {
> > > +		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);
> > 
> > Both of these callers are checking for the existence of the
> > 'reg_name' property before calling this API. So this check seems to
> > be redundant (for now).
> 
> True, but I don't see a way to enforce the caller checks.  I don't
> like the idea of calling of_property_read_reg(np, index, ...) where we
> have to look the caller to verify that "index" is valid.
> 

Ok.

- Mani

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

Re: [PATCH v12 05/13] PCI: dwc: Add dw_pcie_parent_bus_offset()

[Frank Li]

On Tue, Mar 25, 2025 at 11:59:14PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 24, 2025 at 01:28:27PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 24, 2025 at 10:48:23PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Mar 15, 2025 at 03:15:40PM -0500, Bjorn Helgaas wrote:
> > > > From: Frank Li <Frank.Li@nxp.com>
> > > >
> > > > Return the offset from CPU physical address to the parent bus address of
> > > > the specified element of the devicetree 'reg' property.
> >
> > > > +resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > > > +					  const char *reg_name,
> > > > +					  resource_size_t cpu_phy_addr)
> > > > +{
> > >
> > > s/cpu_phy_addr/cpu_phys_addr/g
> >
> > Fixed, thanks!
> >
> > > > +	struct device *dev = pci->dev;
> > > > +	struct device_node *np = dev->of_node;
> > > > +	int index;
> > > > +	u64 reg_addr;
> > > > +
> > > > +	/* Look up reg_name address on parent bus */
> > >
> > > 'parent bus' is not accurate as the below code checks for the 'reg_name' in
> > > current PCI controller node.
> >
> > We want the address of "reg_name" on the node's primary side.  We've
> > been calling that the "parent bus address", I guess because it's the
> > address on the "parent bus" of the node.
> >
>
> Yeah, 'parent bus address' sounds bogus to me. 'ranges' property is described
> as:
>
> 	ranges = <child_addr parent_addr child_size>
>
> Here, child_addr refers to the PCIe host controller's view of the address space
> and parent_addr refers to the CPU's view of the address space.
>
> So the register address described in the PCIe controller node is not a 'parent
> bus address'.


All should be parent bus address. See Rob's comments

https://lore.kernel.org/imx/20240927221831.GA135061-robh@kernel.org/


bus{
	ranges = <child_addr parent_addr child_size>
	pcie {

		All address here, will be translated by bus's ranges, which
use 1:1 map if out of ranges by default.

		from pcie node (children node of bus) view, the 'child_addr'
		is parent node (bus)'s output address.

	}
}


bus may not only one layer to CPU.

bus1 {
	ranges = <...>

	bus2 {
		ranges = <...>

		bus3 {
			ranges = <...>

			All address here is parent's node (bus3)'s bus address
			So, 'parent bus address' means the parent node's
			output bus address.
		};
	};
};

Frank

>
> > I'm not sure what the best term is for this.  Do you have a
> > suggestion?
> >
>
> We are just extracting the offset between translated (cpu_phy_addr) and
> untranslated (reg_addr) addresses of a specific register. Maybe the function
> should just return the 'untranslated address' and the caller should compute the
> offset to make it simple?
>
> > If "parent bus address" is the wrong term, maybe we need to rename
> > dw_pcie_parent_bus_offset() itself?
> >
>
> Yes!
>
> > Currently we pass in cpu_phys_addr, but this function doesn't need it
> > except for the debug code added later.  I would really rather have
> > something like this in the callers:
> >
> >   pci->parent_bus_offset = pp->cfg0_base -
> >       dw_pcie_parent_bus_addr(pci, "config");
> >
>
> I agree. This should become, dw_pcie_get_untranslated_addr().
>
> > because then the offset is computed sort of at the same level where
> > it's used, and a grep for "cfg0_base" would find both the set and the
> > use and they would be easy to match up.
> >
> > > > +	index = of_property_match_string(np, "reg-names", reg_name);
> > > > +
> > > > +	if (index < 0) {
> > > > +		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);
> > >
> > > Both of these callers are checking for the existence of the
> > > 'reg_name' property before calling this API. So this check seems to
> > > be redundant (for now).
> >
> > True, but I don't see a way to enforce the caller checks.  I don't
> > like the idea of calling of_property_read_reg(np, index, ...) where we
> > have to look the caller to verify that "index" is valid.
> >
>
> Ok.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

[PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
is a hard-coded way to get the parent bus address corresponding to a CPU
physical address.

Add debug code to compare the address from .cpu_addr_fixup() with the
address from devicetree.  If they match, warn that .cpu_addr_fixup() is
redundant and should be removed; if they differ, warn that something is
wrong with the devicetree.

If .cpu_addr_fixup() is not implemented, the parent bus address should be
identical to the CPU physical address because we previously ignored the
parent bus address from devicetree.  If the devicetree has a different
parent bus address, warn about it being broken.

[bhelgaas: split debug to separate patch for easier future revert, commit
log]
Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 0a35e36da703..985264c88b92 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
 	int index;
-	u64 reg_addr;
+	u64 reg_addr, fixup_addr;
+	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
 
 	/* Look up reg_name address on parent bus */
 	index = of_property_match_string(np, "reg-names", reg_name);
@@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
 
 	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; devicetree 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, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
+				 cpu_phy_addr, reg_addr);
+			return 0;
+		}
+	}
+
+	dev_info(dev, "%s parent bus offset is %#010llx\n",
+		 reg_name, cpu_phy_addr - reg_addr);
 	return cpu_phy_addr - reg_addr;
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 16548b01347d..f08d2852cfd5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -465,6 +465,19 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+
+	/*
+	 * If iATU input addresses are offset from CPU physical addresses,
+	 * we previously required .cpu_addr_fixup() to convert them.  We
+	 * now rely on the devicetree instead.  If .cpu_addr_fixup()
+	 * exists, we compare its results with devicetree.
+	 *
+	 * If .cpu_addr_fixup() does not exist, we assume the offset is
+	 * zero and warn if devicetree claims otherwise.  If we know all
+	 * devicetrees correctly describe the offset, set
+	 * use_parent_dt_ranges to true to avoid this warning.
+	 */
+	bool			use_parent_dt_ranges;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
-- 
2.34.1

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.com>
>
> dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> is a hard-coded way to get the parent bus address corresponding to a CPU
> physical address.
>
> Add debug code to compare the address from .cpu_addr_fixup() with the
> address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> redundant and should be removed; if they differ, warn that something is
> wrong with the devicetree.
>
> If .cpu_addr_fixup() is not implemented, the parent bus address should be
> identical to the CPU physical address because we previously ignored the
> parent bus address from devicetree.  If the devicetree has a different
> parent bus address, warn about it being broken.
>
> [bhelgaas: split debug to separate patch for easier future revert, commit
> log]
> Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 0a35e36da703..985264c88b92 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  	struct device *dev = pci->dev;
>  	struct device_node *np = dev->of_node;
>  	int index;
> -	u64 reg_addr;
> +	u64 reg_addr, fixup_addr;
> +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
>
>  	/* Look up reg_name address on parent bus */
>  	index = of_property_match_string(np, "reg-names", reg_name);
> @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>
>  	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; devicetree 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, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> +				 cpu_phy_addr, reg_addr);
> +			return 0;
> +		}
> +	}
> +
> +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> +		 reg_name, cpu_phy_addr - reg_addr);
>  	return cpu_phy_addr - reg_addr;
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 16548b01347d..f08d2852cfd5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -465,6 +465,19 @@ struct dw_pcie {
>  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
>  	struct gpio_desc		*pe_rst;
>  	bool			suspended;
> +
> +	/*
> +	 * If iATU input addresses are offset from CPU physical addresses,
> +	 * we previously required .cpu_addr_fixup() to convert them.  We
> +	 * now rely on the devicetree instead.  If .cpu_addr_fixup()
> +	 * exists, we compare its results with devicetree.
> +	 *
> +	 * If .cpu_addr_fixup() does not exist, we assume the offset is
> +	 * zero and warn if devicetree claims otherwise.  If we know all
> +	 * devicetrees correctly describe the offset, set
> +	 * use_parent_dt_ranges to true to avoid this warning.
> +	 */
> +	bool			use_parent_dt_ranges;

Look good.

Frank

>  };
>
>  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> --
> 2.34.1
>

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Bjorn Helgaas]

On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.com>
> 
> dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> is a hard-coded way to get the parent bus address corresponding to a CPU
> physical address.
> 
> Add debug code to compare the address from .cpu_addr_fixup() with the
> address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> redundant and should be removed; if they differ, warn that something is
> wrong with the devicetree.
> 
> If .cpu_addr_fixup() is not implemented, the parent bus address should be
> identical to the CPU physical address because we previously ignored the
> parent bus address from devicetree.  If the devicetree has a different
> parent bus address, warn about it being broken.

> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  	struct device *dev = pci->dev;
>  	struct device_node *np = dev->of_node;
>  	int index;
> -	u64 reg_addr;
> +	u64 reg_addr, fixup_addr;
> +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
>  
>  	/* Look up reg_name address on parent bus */
>  	index = of_property_match_string(np, "reg-names", reg_name);
> @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  
>  	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",

On second thought, I think this one should be a dev_info(), not a
dev_warn().  We know the *current* devicetree describes the offset
correctly, but there may be other devicetrees that do not describe it,
and we need to keep .cpu_addr_fixup() for those other devicetrees.

So there's nothing the current user can or should do about this.

> +				 cpu_phy_addr, reg_name, index,
> +				 fixup_addr, fixup);
> +		} else {
> +			dev_warn(dev, "%#010llx %s reg[%d] != %#010llx fixed up addr; devicetree 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, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> +				 cpu_phy_addr, reg_addr);
> +			return 0;
> +		}
> +	}
> +
> +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> +		 reg_name, cpu_phy_addr - reg_addr);
>  	return cpu_phy_addr - reg_addr;
>  }

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Frank Li]

On Tue, Mar 18, 2025 at 10:38:20AM -0500, Bjorn Helgaas wrote:
> On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> > From: Frank Li <Frank.Li@nxp.com>
> >
> > dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> > controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> > is a hard-coded way to get the parent bus address corresponding to a CPU
> > physical address.
> >
> > Add debug code to compare the address from .cpu_addr_fixup() with the
> > address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> > redundant and should be removed; if they differ, warn that something is
> > wrong with the devicetree.
> >
> > If .cpu_addr_fixup() is not implemented, the parent bus address should be
> > identical to the CPU physical address because we previously ignored the
> > parent bus address from devicetree.  If the devicetree has a different
> > parent bus address, warn about it being broken.
>
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >  	struct device *dev = pci->dev;
> >  	struct device_node *np = dev->of_node;
> >  	int index;
> > -	u64 reg_addr;
> > +	u64 reg_addr, fixup_addr;
> > +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> >
> >  	/* Look up reg_name address on parent bus */
> >  	index = of_property_match_string(np, "reg-names", reg_name);
> > @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >
> >  	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",
>
> On second thought, I think this one should be a dev_info(), not a
> dev_warn().  We know the *current* devicetree describes the offset
> correctly, but there may be other devicetrees that do not describe it,
> and we need to keep .cpu_addr_fixup() for those other devicetrees.
>
> So there's nothing the current user can or should do about this.

Okay

Frank
>
> > +				 cpu_phy_addr, reg_name, index,
> > +				 fixup_addr, fixup);
> > +		} else {
> > +			dev_warn(dev, "%#010llx %s reg[%d] != %#010llx fixed up addr; devicetree 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, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> > +				 cpu_phy_addr, reg_addr);
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> > +		 reg_name, cpu_phy_addr - reg_addr);
> >  	return cpu_phy_addr - reg_addr;
> >  }

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Manivannan Sadhasivam]

On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.com>
> 
> dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> is a hard-coded way to get the parent bus address corresponding to a CPU
> physical address.
> 
> Add debug code to compare the address from .cpu_addr_fixup() with the
> address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> redundant and should be removed; if they differ, warn that something is
> wrong with the devicetree.
> 
> If .cpu_addr_fixup() is not implemented, the parent bus address should be
> identical to the CPU physical address because we previously ignored the
> parent bus address from devicetree.  If the devicetree has a different
> parent bus address, warn about it being broken.
> 
> [bhelgaas: split debug to separate patch for easier future revert, commit
> log]
> Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 0a35e36da703..985264c88b92 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  	struct device *dev = pci->dev;
>  	struct device_node *np = dev->of_node;
>  	int index;
> -	u64 reg_addr;
> +	u64 reg_addr, fixup_addr;
> +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
>  
>  	/* Look up reg_name address on parent bus */
>  	index = of_property_match_string(np, "reg-names", reg_name);
> @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  
>  	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; devicetree is broken\n",
> +				 cpu_phy_addr, reg_name,
> +				 index, fixup_addr);
> +			reg_addr = fixup_addr;
> +		}
> +	} else if (!pci->use_parent_dt_ranges) {

Is this check still valid? 'use_parent_dt_ranges' is only used here for
validation. Moreover, if the fixup is not available, we should be able to
safely return 'cpu_phy_addr - reg_addr' unconditionally.

> +		if (reg_addr != cpu_phy_addr) {
> +			dev_warn(dev, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> +				 cpu_phy_addr, reg_addr);
> +			return 0;
> +		}
> +	}
> +
> +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> +		 reg_name, cpu_phy_addr - reg_addr);

This info is useless on platforms having no translation between CPU and PCI
controller. The offset will always be 0.

- Mani

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

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Frank Li]

On Mon, Mar 24, 2025 at 11:00:24PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> > From: Frank Li <Frank.Li@nxp.com>
> >
> > dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> > controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> > is a hard-coded way to get the parent bus address corresponding to a CPU
> > physical address.
> >
> > Add debug code to compare the address from .cpu_addr_fixup() with the
> > address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> > redundant and should be removed; if they differ, warn that something is
> > wrong with the devicetree.
> >
> > If .cpu_addr_fixup() is not implemented, the parent bus address should be
> > identical to the CPU physical address because we previously ignored the
> > parent bus address from devicetree.  If the devicetree has a different
> > parent bus address, warn about it being broken.
> >
> > [bhelgaas: split debug to separate patch for easier future revert, commit
> > log]
> > Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
> >  2 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 0a35e36da703..985264c88b92 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >  	struct device *dev = pci->dev;
> >  	struct device_node *np = dev->of_node;
> >  	int index;
> > -	u64 reg_addr;
> > +	u64 reg_addr, fixup_addr;
> > +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> >
> >  	/* Look up reg_name address on parent bus */
> >  	index = of_property_match_string(np, "reg-names", reg_name);
> > @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >
> >  	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; devicetree is broken\n",
> > +				 cpu_phy_addr, reg_name,
> > +				 index, fixup_addr);
> > +			reg_addr = fixup_addr;
> > +		}
> > +	} else if (!pci->use_parent_dt_ranges) {
>
> Is this check still valid? 'use_parent_dt_ranges' is only used here for
> validation. Moreover, if the fixup is not available, we should be able to
> safely return 'cpu_phy_addr - reg_addr' unconditionally.

I worry about some platform use fake bus address translation in dtb file.
If none report below warn for some times, we can remove all
use_parent_dt_ranges.

Frank

>
> > +		if (reg_addr != cpu_phy_addr) {
> > +			dev_warn(dev, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> > +				 cpu_phy_addr, reg_addr);
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> > +		 reg_name, cpu_phy_addr - reg_addr);
>
> This info is useless on platforms having no translation between CPU and PCI
> controller. The offset will always be 0.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Bjorn Helgaas]

On Mon, Mar 24, 2025 at 11:00:24PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> > From: Frank Li <Frank.Li@nxp.com>
> > 
> > dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> > controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> > is a hard-coded way to get the parent bus address corresponding to a CPU
> > physical address.
> > 
> > Add debug code to compare the address from .cpu_addr_fixup() with the
> > address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> > redundant and should be removed; if they differ, warn that something is
> > wrong with the devicetree.
> > 
> > If .cpu_addr_fixup() is not implemented, the parent bus address should be
> > identical to the CPU physical address because we previously ignored the
> > parent bus address from devicetree.  If the devicetree has a different
> > parent bus address, warn about it being broken.
> > 
> > [bhelgaas: split debug to separate patch for easier future revert, commit
> > log]
> > Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
> >  2 files changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 0a35e36da703..985264c88b92 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >  	struct device *dev = pci->dev;
> >  	struct device_node *np = dev->of_node;
> >  	int index;
> > -	u64 reg_addr;
> > +	u64 reg_addr, fixup_addr;
> > +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> >  
> >  	/* Look up reg_name address on parent bus */
> >  	index = of_property_match_string(np, "reg-names", reg_name);
> > @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >  
> >  	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; devicetree is broken\n",
> > +				 cpu_phy_addr, reg_name,
> > +				 index, fixup_addr);
> > +			reg_addr = fixup_addr;
> > +		}
> > +	} else if (!pci->use_parent_dt_ranges) {
> 
> Is this check still valid? 'use_parent_dt_ranges' is only used here for
> validation. Moreover, if the fixup is not available, we should be able to
> safely return 'cpu_phy_addr - reg_addr' unconditionally.

Yes, that's true IF the devicetree has the correct 'ranges'
translation.  This is to avoid breaking platforms with broken
devicetrees.

> > +		if (reg_addr != cpu_phy_addr) {
> > +			dev_warn(dev, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> > +				 cpu_phy_addr, reg_addr);
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> > +		 reg_name, cpu_phy_addr - reg_addr);
> 
> This info is useless on platforms having no translation between CPU and PCI
> controller. The offset will always be 0.

You're right.  This was probably an overzealous message for any
possible issues.

What would you think of the below as a replacement?  It should emit at
most one message, and none for platforms where devicetree describes no
translation and there never was a .cpu_addr_fixup().

It's still pretty aggressive logging, but I'm just concerned about
being able to quickly debug and fix any regressions.  Ideally we can
revert the whole thing eventually.


diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 27b464a405a4..4b442d1aa55b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
 	int index;
-	u64 reg_addr;
+	u64 reg_addr, fixup_addr;
+	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
 
 	/* Look up reg_name address on parent bus */
 	index = of_property_match_string(np, "reg-names", reg_name);
@@ -1126,5 +1127,42 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
 
 	of_property_read_reg(np, index, &reg_addr, NULL);
 
+	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;
+	}
+
+	if (pci->use_parent_dt_ranges) {
+
+		/*
+		 * This platform once had a fixup, presumably because it
+		 * translates between CPU and PCI controller addresses.
+		 * Log a note if devicetree didn't describe a translation.
+		 */
+		if (reg_addr == cpu_phys_addr)
+			dev_info(dev, "%s reg[%d] %#010llx == cpu %#010llx\n; no fixup was ever needed for this devicetree\n",
+				 reg_name, index, reg_addr,
+				 (unsigned long long) cpu_phys_addr);
+	} else {
+		if (reg_addr != cpu_phys_addr) {
+			dev_warn(dev, "%s reg[%d] %#010llx != cpu %#010llx; no fixup and devicetree \"ranges\" is broken, assuming no translation\n",
+				 reg_name, index, reg_addr,
+				 (unsigned long long) cpu_phys_addr);
+			return 0;
+		}
+	}
+
 	return cpu_phys_addr - reg_addr;
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 16548b01347d..f08d2852cfd5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -465,6 +465,19 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+
+	/*
+	 * If iATU input addresses are offset from CPU physical addresses,
+	 * we previously required .cpu_addr_fixup() to convert them.  We
+	 * now rely on the devicetree instead.  If .cpu_addr_fixup()
+	 * exists, we compare its results with devicetree.
+	 *
+	 * If .cpu_addr_fixup() does not exist, we assume the offset is
+	 * zero and warn if devicetree claims otherwise.  If we know all
+	 * devicetrees correctly describe the offset, set
+	 * use_parent_dt_ranges to true to avoid this warning.
+	 */
+	bool			use_parent_dt_ranges;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Manivannan Sadhasivam]

On Mon, Mar 24, 2025 at 03:04:37PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 24, 2025 at 11:00:24PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> > > From: Frank Li <Frank.Li@nxp.com>
> > > 
> > > dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> > > controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> > > is a hard-coded way to get the parent bus address corresponding to a CPU
> > > physical address.
> > > 
> > > Add debug code to compare the address from .cpu_addr_fixup() with the
> > > address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> > > redundant and should be removed; if they differ, warn that something is
> > > wrong with the devicetree.
> > > 
> > > If .cpu_addr_fixup() is not implemented, the parent bus address should be
> > > identical to the CPU physical address because we previously ignored the
> > > parent bus address from devicetree.  If the devicetree has a different
> > > parent bus address, warn about it being broken.
> > > 
> > > [bhelgaas: split debug to separate patch for easier future revert, commit
> > > log]
> > > Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
> > >  drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
> > >  2 files changed, 38 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 0a35e36da703..985264c88b92 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > >  	struct device *dev = pci->dev;
> > >  	struct device_node *np = dev->of_node;
> > >  	int index;
> > > -	u64 reg_addr;
> > > +	u64 reg_addr, fixup_addr;
> > > +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> > >  
> > >  	/* Look up reg_name address on parent bus */
> > >  	index = of_property_match_string(np, "reg-names", reg_name);
> > > @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > >  
> > >  	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; devicetree is broken\n",
> > > +				 cpu_phy_addr, reg_name,
> > > +				 index, fixup_addr);
> > > +			reg_addr = fixup_addr;
> > > +		}
> > > +	} else if (!pci->use_parent_dt_ranges) {
> > 
> > Is this check still valid? 'use_parent_dt_ranges' is only used here for
> > validation. Moreover, if the fixup is not available, we should be able to
> > safely return 'cpu_phy_addr - reg_addr' unconditionally.
> 
> Yes, that's true IF the devicetree has the correct 'ranges'
> translation.  This is to avoid breaking platforms with broken
> devicetrees.
> 

You mean the driver without cpu_addr_fixup() and devicetree with broken 'ranges'
property? So the existing platforms... Not a bad idea though.

> > > +		if (reg_addr != cpu_phy_addr) {
> > > +			dev_warn(dev, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> > > +				 cpu_phy_addr, reg_addr);
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> > > +		 reg_name, cpu_phy_addr - reg_addr);
> > 
> > This info is useless on platforms having no translation between CPU and PCI
> > controller. The offset will always be 0.
> 
> You're right.  This was probably an overzealous message for any
> possible issues.
> 
> What would you think of the below as a replacement?  It should emit at
> most one message, and none for platforms where devicetree describes no
> translation and there never was a .cpu_addr_fixup().
> 
> It's still pretty aggressive logging, but I'm just concerned about
> being able to quickly debug and fix any regressions.  Ideally we can
> revert the whole thing eventually.
> 
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 27b464a405a4..4b442d1aa55b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  	struct device *dev = pci->dev;
>  	struct device_node *np = dev->of_node;
>  	int index;
> -	u64 reg_addr;
> +	u64 reg_addr, fixup_addr;
> +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
>  
>  	/* Look up reg_name address on parent bus */
>  	index = of_property_match_string(np, "reg-names", reg_name);
> @@ -1126,5 +1127,42 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
>  
>  	of_property_read_reg(np, index, &reg_addr, NULL);
>  
> +	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;
> +	}
> +
> +	if (pci->use_parent_dt_ranges) {
> +
> +		/*
> +		 * This platform once had a fixup, presumably because it
> +		 * translates between CPU and PCI controller addresses.
> +		 * Log a note if devicetree didn't describe a translation.
> +		 */
> +		if (reg_addr == cpu_phys_addr)
> +			dev_info(dev, "%s reg[%d] %#010llx == cpu %#010llx\n; no fixup was ever needed for this devicetree\n",
> +				 reg_name, index, reg_addr,
> +				 (unsigned long long) cpu_phys_addr);

So this check is to detect the usage of old DTs with new kernel without
cpu_addr_fixup()? If so:

(1) The log is not accurate
(2) The driver would be broken, so the log should be an error. This condition
should not be met (if we do not remove the fixup for some time). But I think
this check should be moved ahead of cpu_addr_fixup() so that the correct DTs
would be honored first and the fixup would be ignored for them.

- Mani

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

Re: [PATCH v12 06/13] PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug

[Frank Li]

On Tue, Mar 25, 2025 at 11:39:15PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 24, 2025 at 03:04:37PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 24, 2025 at 11:00:24PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Mar 15, 2025 at 03:15:41PM -0500, Bjorn Helgaas wrote:
> > > > From: Frank Li <Frank.Li@nxp.com>
> > > >
> > > > dw_pcie_parent_bus_offset() looks up the parent bus address of a PCI
> > > > controller 'reg' property in devicetree.  If implemented, .cpu_addr_fixup()
> > > > is a hard-coded way to get the parent bus address corresponding to a CPU
> > > > physical address.
> > > >
> > > > Add debug code to compare the address from .cpu_addr_fixup() with the
> > > > address from devicetree.  If they match, warn that .cpu_addr_fixup() is
> > > > redundant and should be removed; if they differ, warn that something is
> > > > wrong with the devicetree.
> > > >
> > > > If .cpu_addr_fixup() is not implemented, the parent bus address should be
> > > > identical to the CPU physical address because we previously ignored the
> > > > parent bus address from devicetree.  If the devicetree has a different
> > > > parent bus address, warn about it being broken.
> > > >
> > > > [bhelgaas: split debug to separate patch for easier future revert, commit
> > > > log]
> > > > Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-5-01d2313502ab@nxp.com
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-designware.c | 26 +++++++++++++++++++-
> > > >  drivers/pci/controller/dwc/pcie-designware.h | 13 ++++++++++
> > > >  2 files changed, 38 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 0a35e36da703..985264c88b92 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > > >  	struct device *dev = pci->dev;
> > > >  	struct device_node *np = dev->of_node;
> > > >  	int index;
> > > > -	u64 reg_addr;
> > > > +	u64 reg_addr, fixup_addr;
> > > > +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> > > >
> > > >  	/* Look up reg_name address on parent bus */
> > > >  	index = of_property_match_string(np, "reg-names", reg_name);
> > > > @@ -1126,5 +1127,28 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> > > >
> > > >  	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; devicetree is broken\n",
> > > > +				 cpu_phy_addr, reg_name,
> > > > +				 index, fixup_addr);
> > > > +			reg_addr = fixup_addr;
> > > > +		}
> > > > +	} else if (!pci->use_parent_dt_ranges) {
> > >
> > > Is this check still valid? 'use_parent_dt_ranges' is only used here for
> > > validation. Moreover, if the fixup is not available, we should be able to
> > > safely return 'cpu_phy_addr - reg_addr' unconditionally.
> >
> > Yes, that's true IF the devicetree has the correct 'ranges'
> > translation.  This is to avoid breaking platforms with broken
> > devicetrees.
> >
>
> You mean the driver without cpu_addr_fixup() and devicetree with broken 'ranges'
> property? So the existing platforms... Not a bad idea though.

I worry about some platform without cpu_addr_fixup() use fake translation
by ranges property.

like
bus{
	...
	ranges = <fake_addr, real_addr, size>
	pcie {
		regs = <fake_addr + offset>
		reg-names = "config".
		...
	}
}

So add above check, we will remove it if none report it for sometimes.

Frank

>
> > > > +		if (reg_addr != cpu_phy_addr) {
> > > > +			dev_warn(dev, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
> > > > +				 cpu_phy_addr, reg_addr);
> > > > +			return 0;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	dev_info(dev, "%s parent bus offset is %#010llx\n",
> > > > +		 reg_name, cpu_phy_addr - reg_addr);
> > >
> > > This info is useless on platforms having no translation between CPU and PCI
> > > controller. The offset will always be 0.
> >
> > You're right.  This was probably an overzealous message for any
> > possible issues.
> >
> > What would you think of the below as a replacement?  It should emit at
> > most one message, and none for platforms where devicetree describes no
> > translation and there never was a .cpu_addr_fixup().
> >
> > It's still pretty aggressive logging, but I'm just concerned about
> > being able to quickly debug and fix any regressions.  Ideally we can
> > revert the whole thing eventually.
> >
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 27b464a405a4..4b442d1aa55b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -1114,7 +1114,8 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >  	struct device *dev = pci->dev;
> >  	struct device_node *np = dev->of_node;
> >  	int index;
> > -	u64 reg_addr;
> > +	u64 reg_addr, fixup_addr;
> > +	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> >
> >  	/* Look up reg_name address on parent bus */
> >  	index = of_property_match_string(np, "reg-names", reg_name);
> > @@ -1126,5 +1127,42 @@ resource_size_t dw_pcie_parent_bus_offset(struct dw_pcie *pci,
> >
> >  	of_property_read_reg(np, index, &reg_addr, NULL);
> >
> > +	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;
> > +	}
> > +
> > +	if (pci->use_parent_dt_ranges) {
> > +
> > +		/*
> > +		 * This platform once had a fixup, presumably because it
> > +		 * translates between CPU and PCI controller addresses.
> > +		 * Log a note if devicetree didn't describe a translation.
> > +		 */
> > +		if (reg_addr == cpu_phys_addr)
> > +			dev_info(dev, "%s reg[%d] %#010llx == cpu %#010llx\n; no fixup was ever needed for this devicetree\n",
> > +				 reg_name, index, reg_addr,
> > +				 (unsigned long long) cpu_phys_addr);
>
> So this check is to detect the usage of old DTs with new kernel without
> cpu_addr_fixup()? If so:
>
> (1) The log is not accurate
> (2) The driver would be broken, so the log should be an error. This condition
> should not be met (if we do not remove the fixup for some time). But I think
> this check should be moved ahead of cpu_addr_fixup() so that the correct DTs
> would be honored first and the fixup would be ignored for them.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

[PATCH v12 07/13] PCI: dwc: Use devicetree 'reg[config]' to derive CPU -> ATU addr offset

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

The 'ranges' property of a PCI controller's parent can indicate address
translation information. Most system use 1:1 map between CPU physical and
PCI controller input addresses.

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

Intermediate address (IA) here means the PCIe controller input address.
The pcie@5f010000 'reg[config]' address is the parent bus (PCIe controller
input) address of CfgSpace.

The ATU in MemSpace is not explicitly described via devicetree, so we
assume the offset from CPU address to intermediate MemSpace address is the
same as that for CfgSpace.

We could use bus@5f000000 'ranges' for the same purpose.

Set parent_bus_offset using dw_pcie_init_parent_bus_offset().  The
parent_bus_offset is not used yet, so no functional change intended.

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-6-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9ce06b1ee266..9e38ac7d1bcb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -452,6 +452,12 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
 		pp->io_base = pci_pio_to_address(win->res->start);
 	}
 
+	/*
+	 * visconti_pcie_cpu_addr_fixup() uses pp->io_base, so we have to
+	 * call dw_pcie_parent_bus_offset() after setting pp->io_base.
+	 */
+	pci->parent_bus_offset = dw_pcie_parent_bus_offset(pci, "config",
+							   pp->cfg0_base);
 	return 0;
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index f08d2852cfd5..741c46926ce2 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;
-- 
2.34.1

[PATCH v12 08/13] PCI: dwc: ep: Call epc_create() early in dw_pcie_ep_init()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Move devm_pci_epc_create() to the beginning of dw_pcie_ep_init().

devm_pci_epc_create() is generic code that doesn't depend on any DWC
resource, so moving it earlier keeps all the subsequent devicetree-related
code together.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c    | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 80ac2f9e88eb..100d26466f05 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -904,6 +904,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	INIT_LIST_HEAD(&ep->func_list);
 
+	epc = devm_pci_epc_create(dev, &epc_ops);
+	if (IS_ERR(epc)) {
+		dev_err(dev, "Failed to create epc device\n");
+		return PTR_ERR(epc);
+	}
+
+	ep->epc = epc;
+	epc_set_drvdata(epc, ep);
+
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
 		return ret;
@@ -918,15 +927,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	if (ep->ops->pre_init)
 		ep->ops->pre_init(ep);
 
-	epc = devm_pci_epc_create(dev, &epc_ops);
-	if (IS_ERR(epc)) {
-		dev_err(dev, "Failed to create epc device\n");
-		return PTR_ERR(epc);
-	}
-
-	ep->epc = epc;
-	epc_set_drvdata(epc, ep);
-
 	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
 	if (ret < 0)
 		epc->max_functions = 1;
-- 
2.34.1

Re: [PATCH v12 08/13] PCI: dwc: ep: Call epc_create() early in dw_pcie_ep_init()

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:43PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Move devm_pci_epc_create() to the beginning of dw_pcie_ep_init().
>
> devm_pci_epc_create() is generic code that doesn't depend on any DWC
> resource, so moving it earlier keeps all the subsequent devicetree-related
> code together.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  .../pci/controller/dwc/pcie-designware-ep.c    | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 80ac2f9e88eb..100d26466f05 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -904,6 +904,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>
>  	INIT_LIST_HEAD(&ep->func_list);
>
> +	epc = devm_pci_epc_create(dev, &epc_ops);
> +	if (IS_ERR(epc)) {
> +		dev_err(dev, "Failed to create epc device\n");
> +		return PTR_ERR(epc);
> +	}
> +
> +	ep->epc = epc;
> +	epc_set_drvdata(epc, ep);
> +
>  	ret = dw_pcie_get_resources(pci);
>  	if (ret)
>  		return ret;
> @@ -918,15 +927,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (ep->ops->pre_init)
>  		ep->ops->pre_init(ep);
>
> -	epc = devm_pci_epc_create(dev, &epc_ops);
> -	if (IS_ERR(epc)) {
> -		dev_err(dev, "Failed to create epc device\n");
> -		return PTR_ERR(epc);
> -	}
> -
> -	ep->epc = epc;
> -	epc_set_drvdata(epc, ep);
> -
>  	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>  	if (ret < 0)
>  		epc->max_functions = 1;
> --
> 2.34.1
>

[PATCH v12 09/13] PCI: dwc: ep: Consolidate devicetree handling in dw_pcie_ep_get_resources()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Consolidate devicetree resource handling in dw_pcie_ep_get_resources().
No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 68 +++++++++++--------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 100d26466f05..2db834345ec2 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -883,35 +883,15 @@ void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown);
 
-/**
- * dw_pcie_ep_init - Initialize the endpoint device
- * @ep: DWC EP device
- *
- * Initialize the endpoint device. Allocate resources and create the EPC
- * device with the endpoint framework.
- *
- * Return: 0 if success, errno otherwise.
- */
-int dw_pcie_ep_init(struct dw_pcie_ep *ep)
+static int dw_pcie_ep_get_resources(struct dw_pcie_ep *ep)
 {
-	int ret;
-	struct resource *res;
-	struct pci_epc *epc;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct device *dev = pci->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
-
-	INIT_LIST_HEAD(&ep->func_list);
-
-	epc = devm_pci_epc_create(dev, &epc_ops);
-	if (IS_ERR(epc)) {
-		dev_err(dev, "Failed to create epc device\n");
-		return PTR_ERR(epc);
-	}
-
-	ep->epc = epc;
-	epc_set_drvdata(epc, ep);
+	struct pci_epc *epc = ep->epc;
+	struct resource *res;
+	int ret;
 
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
@@ -924,13 +904,47 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	ep->phys_base = res->start;
 	ep->addr_size = resource_size(res);
 
-	if (ep->ops->pre_init)
-		ep->ops->pre_init(ep);
-
 	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
 	if (ret < 0)
 		epc->max_functions = 1;
 
+	return 0;
+}
+
+/**
+ * dw_pcie_ep_init - Initialize the endpoint device
+ * @ep: DWC EP device
+ *
+ * Initialize the endpoint device. Allocate resources and create the EPC
+ * device with the endpoint framework.
+ *
+ * Return: 0 if success, errno otherwise.
+ */
+int dw_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	int ret;
+	struct pci_epc *epc;
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct device *dev = pci->dev;
+
+	INIT_LIST_HEAD(&ep->func_list);
+
+	epc = devm_pci_epc_create(dev, &epc_ops);
+	if (IS_ERR(epc)) {
+		dev_err(dev, "Failed to create epc device\n");
+		return PTR_ERR(epc);
+	}
+
+	ep->epc = epc;
+	epc_set_drvdata(epc, ep);
+
+	ret = dw_pcie_ep_get_resources(ep);
+	if (ret)
+		return ret;
+
+	if (ep->ops->pre_init)
+		ep->ops->pre_init(ep);
+
 	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
 			       ep->page_size);
 	if (ret < 0) {
-- 
2.34.1

Re: [PATCH v12 09/13] PCI: dwc: ep: Consolidate devicetree handling in dw_pcie_ep_get_resources()

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:44PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Consolidate devicetree resource handling in dw_pcie_ep_get_resources().
> No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  .../pci/controller/dwc/pcie-designware-ep.c   | 68 +++++++++++--------
>  1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 100d26466f05..2db834345ec2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -883,35 +883,15 @@ void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown);
>
> -/**
> - * dw_pcie_ep_init - Initialize the endpoint device
> - * @ep: DWC EP device
> - *
> - * Initialize the endpoint device. Allocate resources and create the EPC
> - * device with the endpoint framework.
> - *
> - * Return: 0 if success, errno otherwise.
> - */
> -int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> +static int dw_pcie_ep_get_resources(struct dw_pcie_ep *ep)
>  {
> -	int ret;
> -	struct resource *res;
> -	struct pci_epc *epc;
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct device *dev = pci->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct device_node *np = dev->of_node;
> -
> -	INIT_LIST_HEAD(&ep->func_list);
> -
> -	epc = devm_pci_epc_create(dev, &epc_ops);
> -	if (IS_ERR(epc)) {
> -		dev_err(dev, "Failed to create epc device\n");
> -		return PTR_ERR(epc);
> -	}
> -
> -	ep->epc = epc;
> -	epc_set_drvdata(epc, ep);
> +	struct pci_epc *epc = ep->epc;
> +	struct resource *res;
> +	int ret;
>
>  	ret = dw_pcie_get_resources(pci);
>  	if (ret)
> @@ -924,13 +904,47 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	ep->phys_base = res->start;
>  	ep->addr_size = resource_size(res);
>
> -	if (ep->ops->pre_init)
> -		ep->ops->pre_init(ep);
> -
>  	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>  	if (ret < 0)
>  		epc->max_functions = 1;
>
> +	return 0;
> +}
> +
> +/**
> + * dw_pcie_ep_init - Initialize the endpoint device
> + * @ep: DWC EP device
> + *
> + * Initialize the endpoint device. Allocate resources and create the EPC
> + * device with the endpoint framework.
> + *
> + * Return: 0 if success, errno otherwise.
> + */
> +int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	int ret;
> +	struct pci_epc *epc;
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct device *dev = pci->dev;
> +
> +	INIT_LIST_HEAD(&ep->func_list);
> +
> +	epc = devm_pci_epc_create(dev, &epc_ops);
> +	if (IS_ERR(epc)) {
> +		dev_err(dev, "Failed to create epc device\n");
> +		return PTR_ERR(epc);
> +	}
> +
> +	ep->epc = epc;
> +	epc_set_drvdata(epc, ep);
> +
> +	ret = dw_pcie_ep_get_resources(ep);
> +	if (ret)
> +		return ret;
> +
> +	if (ep->ops->pre_init)
> +		ep->ops->pre_init(ep);
> +
>  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>  			       ep->page_size);
>  	if (ret < 0) {
> --
> 2.34.1
>

[PATCH v12 10/13] PCI: dwc: ep: Use devicetree 'reg[addr_space]' to derive CPU -> ATU addr offset

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

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

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

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

In the diagram above, CPU writes data to outbound window address
0x7000_0000, and the bus fabric maps it to 0x8000_0000. The ATU uses
bus address 0x8000_0000 as input address and maps to some PCI address
dynamically allocated by a PCI device driver on the host side.

The pcie-ep@5f010000 'reg[addr_space]' is the parent bus address of the
PCIe controller input, including the ATU.

Set parent_bus_offset, the offset from the CPU address to the PCIe
controller input address using dw_pcie_init_parent_bus_offset().  The
parent_bus_offset is not used yet, so no functional change intended.

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-7-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
[bhelgaas: commit log]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 2db834345ec2..bb87d0c5c665 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -904,6 +904,13 @@ static int dw_pcie_ep_get_resources(struct dw_pcie_ep *ep)
 	ep->phys_base = res->start;
 	ep->addr_size = resource_size(res);
 
+	/*
+	 * artpec6_pcie_cpu_addr_fixup() uses ep->phys_base, so call
+	 * dw_pcie_parent_bus_offset() after setting ep->phys_base.
+	 */
+	pci->parent_bus_offset = dw_pcie_parent_bus_offset(pci, "addr_space",
+							   ep->phys_base);
+
 	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
 	if (ret < 0)
 		epc->max_functions = 1;
-- 
2.34.1

Re: [PATCH v12 10/13] PCI: dwc: ep: Use devicetree 'reg[addr_space]' to derive CPU -> ATU addr offset

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:45PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.com>
>
>                    Endpoint
>   ┌───────────────────────────────────────────────┐
>   │                             pcie-ep@5f010000  │
>   │                             ┌────────────────┐│
>   │                             │   Endpoint     ││
>   │                             │   PCIe         ││
>   │                             │   Controller   ││
>   │           bus@5f000000      │             ┌────────►
>   │           ┌──────────┐      │             │  ││dynamically
>   │           │          │ Outbound Transfer  │  ││allocated
>   │┌─────┐    │  Bus     ┼─────►│ ATU  ───────┘  ││PCI Addr
>   ││     │    │  Fabric  │Bus   │                ││
>   ││ CPU ├───►│          │Addr  │                ││
>   ││     │CPU │          │0x8000_0000            ││
>   │└─────┘Addr└──────────┘      │                ││
>   │       0x7000_0000           └────────────────┘│
>   └───────────────────────────────────────────────┘
>
>   bus@5f000000 {
>           compatible = "simple-bus";
>           ranges = <0x80000000 0x0 0x70000000 0x10000000>;
>
>           pcie-ep@5f010000 {
>                   reg = <0x80000000 0x10000000>;
>                   reg-names ="addr_space";
>                   ...
>           };
>           ...
>   };
>
> In the diagram above, CPU writes data to outbound window address
> 0x7000_0000, and the bus fabric maps it to 0x8000_0000. The ATU uses
> bus address 0x8000_0000 as input address and maps to some PCI address
> dynamically allocated by a PCI device driver on the host side.
>
> The pcie-ep@5f010000 'reg[addr_space]' is the parent bus address of the
> PCIe controller input, including the ATU.

how about

The pcie-ep@5f010000 'reg[addr_space]' is the parent bus address, which is
input of PCIe controller including the ATU.

Frank

>
> Set parent_bus_offset, the offset from the CPU address to the PCIe
> controller input address using dw_pcie_init_parent_bus_offset().  The
> parent_bus_offset is not used yet, so no functional change intended.
>
> Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-7-01d2313502ab@nxp.com
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> [bhelgaas: commit log]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---

>  drivers/pci/controller/dwc/pcie-designware-ep.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 2db834345ec2..bb87d0c5c665 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -904,6 +904,13 @@ static int dw_pcie_ep_get_resources(struct dw_pcie_ep *ep)
>  	ep->phys_base = res->start;
>  	ep->addr_size = resource_size(res);
>
> +	/*
> +	 * artpec6_pcie_cpu_addr_fixup() uses ep->phys_base, so call
> +	 * dw_pcie_parent_bus_offset() after setting ep->phys_base.
> +	 */
> +	pci->parent_bus_offset = dw_pcie_parent_bus_offset(pci, "addr_space",
> +							   ep->phys_base);
> +
>  	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>  	if (ret < 0)
>  		epc->max_functions = 1;
> --
> 2.34.1
>

[PATCH v12 11/13] PCI: dwc: ep: Ensure proper iteration over outbound map windows

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

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.

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-8-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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 bb87d0c5c665..2ef9964fa080 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 v12 12/13] PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.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() when
programming the ATU.

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

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-9-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
[bhelgaas: commit log]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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 2ef9964fa080..c1feaadb046a 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 9e38ac7d1bcb..d760abcbb785 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -635,7 +635,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;
 
@@ -660,7 +660,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;
 
@@ -686,7 +686,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;
 
@@ -755,7 +755,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 */
@@ -777,7 +777,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;
 
@@ -921,7 +921,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 985264c88b92..d9d2090f380c 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

Re: [PATCH v12 12/13] PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()

[Frank Li]

On Sat, Mar 15, 2025 at 03:15:47PM -0500, Bjorn Helgaas wrote:
> From: Frank Li <Frank.Li@nxp.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() when
> programming the ATU.
>
> This assumes all intermediate addresses are at the same offset from the CPU
> physical addresses.
>
> Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-9-01d2313502ab@nxp.com
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> [bhelgaas: commit log]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

look good

Frank
> ---
>  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 2ef9964fa080..c1feaadb046a 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 9e38ac7d1bcb..d760abcbb785 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -635,7 +635,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;
>
> @@ -660,7 +660,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;
>
> @@ -686,7 +686,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;
>
> @@ -755,7 +755,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 */
> @@ -777,7 +777,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;
>
> @@ -921,7 +921,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 985264c88b92..d9d2090f380c 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 v12 13/13] PCI: imx6: Remove cpu_addr_fixup()

[Bjorn Helgaas]

From: Frank Li <Frank.Li@nxp.com>

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

Link: https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-11-01d2313502ab@nxp.com
Signed-off-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 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 90ace941090f..d1eb535df73e 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 v12 00/13] PCI: Use device bus range info to cleanup RC Host/EP pci_fixup_addr()

[Bjorn Helgaas]

On Sat, Mar 15, 2025 at 03:15:35PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is a v12 based on Frank's v11 series.
> 
> v11 https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-0-01d2313502ab@nxp.com
>     
> Changes from v11:
>   - Call devm_pci_alloc_host_bridge() early in dw_pcie_host_init(), before
>     any devicetree-related code
>   - Call devm_pci_epc_create() early in dw_pcie_ep_init(), before any
>     devicetree-related code
>   - Consolidate devicetree-related code in dw_pcie_host_get_resources() and
>     dw_pcie_ep_get_resources()
>   - Integrate dw_pcie_cfg0_setup() into dw_pcie_host_get_resources()
>   - Convert dw_pcie_init_parent_bus_offset() to dw_pcie_parent_bus_offset()
>     which returns the offset rather than setting it internally
>   - Split the debug comparison of devicetree info with .cpu_addr_fixup() to
>     separate patch so we can easily revert it later
>   - Drop "cpu_addr_fixup() usage detected" warning since we always warn
>     about something in that case anyway

FWIW, here's the diff from v11 to v12:

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index e333855633a7..c1feaadb046a 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -884,26 +884,15 @@ void dw_pcie_ep_linkdown(struct dw_pcie_ep *ep)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_linkdown);
 
-/**
- * dw_pcie_ep_init - Initialize the endpoint device
- * @ep: DWC EP device
- *
- * Initialize the endpoint device. Allocate resources and create the EPC
- * device with the endpoint framework.
- *
- * Return: 0 if success, errno otherwise.
- */
-int dw_pcie_ep_init(struct dw_pcie_ep *ep)
+static int dw_pcie_ep_get_resources(struct dw_pcie_ep *ep)
 {
-	int ret;
-	struct resource *res;
-	struct pci_epc *epc;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct device *dev = pci->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
-
-	INIT_LIST_HEAD(&ep->func_list);
+	struct pci_epc *epc = ep->epc;
+	struct resource *res;
+	int ret;
 
 	ret = dw_pcie_get_resources(pci);
 	if (ret)
@@ -917,15 +906,36 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	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.
+	 * artpec6_pcie_cpu_addr_fixup() uses ep->phys_base, so call
+	 * dw_pcie_parent_bus_offset() after setting ep->phys_base.
 	 */
-	ret = dw_pcie_init_parent_bus_offset(pci, "addr_space", ep->phys_base);
-	if (ret)
-		return ret;
+	pci->parent_bus_offset = dw_pcie_parent_bus_offset(pci, "addr_space",
+							   ep->phys_base);
 
-	if (ep->ops->pre_init)
-		ep->ops->pre_init(ep);
+	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
+	if (ret < 0)
+		epc->max_functions = 1;
+
+	return 0;
+}
+
+/**
+ * dw_pcie_ep_init - Initialize the endpoint device
+ * @ep: DWC EP device
+ *
+ * Initialize the endpoint device. Allocate resources and create the EPC
+ * device with the endpoint framework.
+ *
+ * Return: 0 if success, errno otherwise.
+ */
+int dw_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	int ret;
+	struct pci_epc *epc;
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct device *dev = pci->dev;
+
+	INIT_LIST_HEAD(&ep->func_list);
 
 	epc = devm_pci_epc_create(dev, &epc_ops);
 	if (IS_ERR(epc)) {
@@ -936,9 +946,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	ep->epc = epc;
 	epc_set_drvdata(epc, ep);
 
-	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
-	if (ret < 0)
-		epc->max_functions = 1;
+	ret = dw_pcie_ep_get_resources(ep);
+	if (ret)
+		return ret;
+
+	if (ep->ops->pre_init)
+		ep->ops->pre_init(ep);
 
 	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
 			       ep->page_size);
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e7df3d2ac26..d760abcbb785 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -392,29 +392,6 @@ 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);
@@ -441,33 +418,34 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 	}
 }
 
-int dw_pcie_host_init(struct dw_pcie_rp *pp)
+static int dw_pcie_host_get_resources(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);
-
-	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;
 
-	ret = dw_pcie_cfg0_setup(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);
 
 	/* Get the I/O range from DT */
-	win = resource_list_first_type(&bridge->windows, IORESOURCE_IO);
+	win = resource_list_first_type(&pp->bridge->windows, IORESOURCE_IO);
 	if (win) {
 		pp->io_size = resource_size(win->res);
 		pp->io_bus_addr = win->res->start - win->offset;
@@ -475,11 +453,31 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	}
 
 	/*
-	 * visconti_pcie_cpu_addr_fixup() use pp->io_base,
-	 * so have to call dw_pcie_init_parent_bus_offset() after init
-	 * pp->io_base.
+	 * visconti_pcie_cpu_addr_fixup() uses pp->io_base, so we have to
+	 * call dw_pcie_parent_bus_offset() after setting pp->io_base.
 	 */
-	ret = dw_pcie_init_parent_bus_offset(pci, "config", pp->cfg0_base);
+	pci->parent_bus_offset = dw_pcie_parent_bus_offset(pci, "config",
+							   pp->cfg0_base);
+	return 0;
+}
+
+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 pci_host_bridge *bridge;
+	int ret;
+
+	raw_spin_lock_init(&pp->lock);
+
+	bridge = devm_pci_alloc_host_bridge(dev, 0);
+	if (!bridge)
+		return -ENOMEM;
+
+	pp->bridge = bridge;
+
+	ret = dw_pcie_host_get_resources(pp);
 	if (ret)
 		return ret;
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index d4dc8bf06d4c..d9d2090f380c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -1104,51 +1104,48 @@ 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)
+resource_size_t dw_pcie_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;
+	u64 reg_addr, fixup_addr;
+	u64 (*fixup)(struct dw_pcie *pcie, u64 cpu_addr);
 
 	/* 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;
+		dev_err(dev, "No %s in devicetree \"reg\" property\n", reg_name);
+		return 0;
 	}
 
 	of_property_read_reg(np, index, &reg_addr, NULL);
 
 	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",
 				 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",
+			dev_warn(dev, "%#010llx %s reg[%d] != %#010llx fixed up addr; devicetree 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",
+			dev_warn(dev, "devicetree has incorrect translation; please check parent \"ranges\" property. CPU physical addr %#010llx, parent bus addr %#010llx\n",
 				 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;
+		 reg_name, cpu_phy_addr - reg_addr);
+	return cpu_phy_addr - reg_addr;
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index bfed9d45aba9..741c46926ce2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -466,13 +466,17 @@ 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 iATU input addresses are offset from CPU physical addresses,
+	 * we previously required .cpu_addr_fixup() to convert them.  We
+	 * now rely on the devicetree instead.  If .cpu_addr_fixup()
+	 * exists, we compare its results with devicetree.
 	 *
-	 * 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.
+	 * If .cpu_addr_fixup() does not exist, we assume the offset is
+	 * zero and warn if devicetree claims otherwise.  If we know all
+	 * devicetrees correctly describe the offset, set
+	 * use_parent_dt_ranges to true to avoid this warning.
 	 */
 	bool			use_parent_dt_ranges;
 };
@@ -510,8 +514,9 @@ 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);
+resource_size_t dw_pcie_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)
 {

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

[Bjorn Helgaas]

On Sat, Mar 15, 2025 at 03:15:35PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is a v12 based on Frank's v11 series.
> 
> v11 https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-0-01d2313502ab@nxp.com
>     
> Changes from v11:
>   - Call devm_pci_alloc_host_bridge() early in dw_pcie_host_init(), before
>     any devicetree-related code
>   - Call devm_pci_epc_create() early in dw_pcie_ep_init(), before any
>     devicetree-related code
>   - Consolidate devicetree-related code in dw_pcie_host_get_resources() and
>     dw_pcie_ep_get_resources()
>   - Integrate dw_pcie_cfg0_setup() into dw_pcie_host_get_resources()
>   - Convert dw_pcie_init_parent_bus_offset() to dw_pcie_parent_bus_offset()
>     which returns the offset rather than setting it internally
>   - Split the debug comparison of devicetree info with .cpu_addr_fixup() to
>     separate patch so we can easily revert it later
>   - Drop "cpu_addr_fixup() usage detected" warning since we always warn
>     about something in that case anyway
> 
> Any comments welcome.
> 
> 
> Bjorn Helgaas (3):
>   PCI: dwc: Consolidate devicetree handling in
>     dw_pcie_host_get_resources()
>   PCI: dwc: ep: Call epc_create() early in dw_pcie_ep_init()
>   PCI: dwc: ep: Consolidate devicetree handling in
>     dw_pcie_ep_get_resources()
> 
> Frank Li (10):
>   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: Call devm_pci_alloc_host_bridge() early in
>     dw_pcie_host_init()
>   PCI: dwc: Add dw_pcie_parent_bus_offset()
>   PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug
>   PCI: dwc: Use devicetree 'reg[config]' to derive CPU -> ATU addr
>     offset
>   PCI: dwc: ep: Use devicetree 'reg[addr_space]' to derive CPU -> ATU
>     addr offset
>   PCI: dwc: ep: Ensure proper iteration over outbound map windows
>   PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()
>   PCI: imx6: Remove cpu_addr_fixup()
> 
>  drivers/pci/controller/dwc/pci-imx6.c         | 18 +---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 74 +++++++++++------
>  .../pci/controller/dwc/pcie-designware-host.c | 57 ++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.c  | 82 ++++++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h  | 24 +++++-
>  5 files changed, 171 insertions(+), 84 deletions(-)

Applied to pci/controller/dwc-cpu-addr-fixup for v6.15.

Thank you very much, Frank, for all your hard work on driving this
series!  I think this is a major improvement.

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

[Manivannan Sadhasivam]

On Sat, Mar 15, 2025 at 03:15:35PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is a v12 based on Frank's v11 series.
> 

Mostly looks good to me. But I do have a few comments, that I'll share.

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

- Mani

> v11 https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-0-01d2313502ab@nxp.com
>     
> Changes from v11:
>   - Call devm_pci_alloc_host_bridge() early in dw_pcie_host_init(), before
>     any devicetree-related code
>   - Call devm_pci_epc_create() early in dw_pcie_ep_init(), before any
>     devicetree-related code
>   - Consolidate devicetree-related code in dw_pcie_host_get_resources() and
>     dw_pcie_ep_get_resources()
>   - Integrate dw_pcie_cfg0_setup() into dw_pcie_host_get_resources()
>   - Convert dw_pcie_init_parent_bus_offset() to dw_pcie_parent_bus_offset()
>     which returns the offset rather than setting it internally
>   - Split the debug comparison of devicetree info with .cpu_addr_fixup() to
>     separate patch so we can easily revert it later
>   - Drop "cpu_addr_fixup() usage detected" warning since we always warn
>     about something in that case anyway
> 
> Any comments welcome.
> 
> 
> Bjorn Helgaas (3):
>   PCI: dwc: Consolidate devicetree handling in
>     dw_pcie_host_get_resources()
>   PCI: dwc: ep: Call epc_create() early in dw_pcie_ep_init()
>   PCI: dwc: ep: Consolidate devicetree handling in
>     dw_pcie_ep_get_resources()
> 
> Frank Li (10):
>   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: Call devm_pci_alloc_host_bridge() early in
>     dw_pcie_host_init()
>   PCI: dwc: Add dw_pcie_parent_bus_offset()
>   PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug
>   PCI: dwc: Use devicetree 'reg[config]' to derive CPU -> ATU addr
>     offset
>   PCI: dwc: ep: Use devicetree 'reg[addr_space]' to derive CPU -> ATU
>     addr offset
>   PCI: dwc: ep: Ensure proper iteration over outbound map windows
>   PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()
>   PCI: imx6: Remove cpu_addr_fixup()
> 
>  drivers/pci/controller/dwc/pci-imx6.c         | 18 +---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 74 +++++++++++------
>  .../pci/controller/dwc/pcie-designware-host.c | 57 ++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.c  | 82 ++++++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h  | 24 +++++-
>  5 files changed, 171 insertions(+), 84 deletions(-)
> 
> -- 
> 2.34.1
> 

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

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

[Manivannan Sadhasivam]

On Sat, Mar 15, 2025 at 03:15:35PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is a v12 based on Frank's v11 series.
> 

Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> #SA8775P-RIDE

- Mani

> v11 https://lore.kernel.org/r/20250313-pci_fixup_addr-v11-0-01d2313502ab@nxp.com
>     
> Changes from v11:
>   - Call devm_pci_alloc_host_bridge() early in dw_pcie_host_init(), before
>     any devicetree-related code
>   - Call devm_pci_epc_create() early in dw_pcie_ep_init(), before any
>     devicetree-related code
>   - Consolidate devicetree-related code in dw_pcie_host_get_resources() and
>     dw_pcie_ep_get_resources()
>   - Integrate dw_pcie_cfg0_setup() into dw_pcie_host_get_resources()
>   - Convert dw_pcie_init_parent_bus_offset() to dw_pcie_parent_bus_offset()
>     which returns the offset rather than setting it internally
>   - Split the debug comparison of devicetree info with .cpu_addr_fixup() to
>     separate patch so we can easily revert it later
>   - Drop "cpu_addr_fixup() usage detected" warning since we always warn
>     about something in that case anyway
> 
> Any comments welcome.
> 
> 
> Bjorn Helgaas (3):
>   PCI: dwc: Consolidate devicetree handling in
>     dw_pcie_host_get_resources()
>   PCI: dwc: ep: Call epc_create() early in dw_pcie_ep_init()
>   PCI: dwc: ep: Consolidate devicetree handling in
>     dw_pcie_ep_get_resources()
> 
> Frank Li (10):
>   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: Call devm_pci_alloc_host_bridge() early in
>     dw_pcie_host_init()
>   PCI: dwc: Add dw_pcie_parent_bus_offset()
>   PCI: dwc: Add dw_pcie_parent_bus_offset() checking and debug
>   PCI: dwc: Use devicetree 'reg[config]' to derive CPU -> ATU addr
>     offset
>   PCI: dwc: ep: Use devicetree 'reg[addr_space]' to derive CPU -> ATU
>     addr offset
>   PCI: dwc: ep: Ensure proper iteration over outbound map windows
>   PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()
>   PCI: imx6: Remove cpu_addr_fixup()
> 
>  drivers/pci/controller/dwc/pci-imx6.c         | 18 +---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 74 +++++++++++------
>  .../pci/controller/dwc/pcie-designware-host.c | 57 ++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.c  | 82 ++++++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h  | 24 +++++-
>  5 files changed, 171 insertions(+), 84 deletions(-)
> 
> -- 
> 2.34.1
> 

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