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

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

[Frank Li]

== RC side:

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

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

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

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

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

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

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

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

== EP side:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

---
Frank Li (7):
      of: address: Add parent_bus_addr to struct of_pci_range
      PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
      PCI: dwc: ep: Add bus_addr_base for outbound window
      PCI: imx6: Remove cpu_addr_fixup()
      dt-bindings: PCI: fsl,imx6q-pcie-ep: Add compatible string fsl,imx8q-pcie-ep
      PCI: imx6: Pass correct sub mode when calling phy_set_mode_ext()
      PCI: imx6: Add i.MX8Q PCIe Endpoint (EP) support

 .../devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml | 38 ++++++++++++++-
 drivers/of/address.c                               |  2 +
 drivers/pci/controller/dwc/pci-imx6.c              | 46 +++++++++--------
 drivers/pci/controller/dwc/pcie-designware-ep.c    | 18 ++++++-
 drivers/pci/controller/dwc/pcie-designware-host.c  | 57 +++++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.c       |  9 ++++
 drivers/pci/controller/dwc/pcie-designware.h       |  8 +++
 include/linux/of_address.h                         |  1 +
 8 files changed, 155 insertions(+), 24 deletions(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20240924-pci_fixup_addr-a8568f9bbb34

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

[PATCH v8 1/7] of: address: Add parent_bus_addr to struct of_pci_range

[Frank Li]

Introduce field 'parent_bus_addr' in struct of_pci_range to retrieve parent
bus address information.

Refer to the diagram below to understand that the bus fabric in some
systems (like i.MX8QXP) does not use a 1:1 address map between input and
output.

Currently, many controller drivers use .cpu_addr_fixup() callback hardcodes
that translation in the code, e.g., "cpu_addr & CDNS_PLAT_CPU_TO_BUS_ADDR"
(drivers/pci/controller/cadence/pcie-cadence-plat.c),
"cpu_addr + BUS_IATU_OFFSET"(drivers/pci/controller/dwc/pcie-intel-gw.c),
etc, even though those translations *should* be described via DT.

The .cpu_addr_fixup() can be eliminated if DT correct reflect hardware
behavior and driver use 'parent_bus_addr' in struct of_pci_range.

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

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

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

'parent_bus_addr' in struct of_pci_range can indicate above diagram internal
address (IA) address information.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v7 to v8
- add mani's ACK tag

Change from v5 to v7
-none

Change from v4 to v5
- remove confused  <0x5f000000 0x0 0x5f000000 0x21000000>
- change address order to 7ff8_0000, 7ff0_0000, 7000_0000
- In commit message use parent bus addres

Change from v3 to v4
- improve commit message by driver source code path.

Change from v2 to v3
- cpu_untranslate_addr -> parent_bus_addr
- Add Rob's review tag
  I changed commit message base on Bjorn, if you have concern about review
added tag, let me know.

Change from v1 to v2
- add parent_bus_addr in struct of_pci_range, instead adding new API.
---
 drivers/of/address.c       | 2 ++
 include/linux/of_address.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 286f0c161e332..1a0229ee4e0b2 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -811,6 +811,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 	else
 		range->cpu_addr = of_translate_address(parser->node,
 				parser->range + na);
+
+	range->parent_bus_addr = of_read_number(parser->range + na, parser->pna);
 	range->size = of_read_number(parser->range + parser->pna + na, ns);
 
 	parser->range += np;
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 26a19daf0d092..13dd79186d02c 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -26,6 +26,7 @@ struct of_pci_range {
 		u64 bus_addr;
 	};
 	u64 cpu_addr;
+	u64 parent_bus_addr;
 	u64 size;
 	u32 flags;
 };

-- 
2.34.1

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

[Frank Li]

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

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

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

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

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

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

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

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

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

Change from v3 to v4
- none

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

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

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

-- 
2.34.1

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

[Manivannan Sadhasivam]

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

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

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

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

- Mani

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

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

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

[Krzysztof Wilczyński]

Hello,

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

The commit message will include the following:

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

Let me know if anything needs to be changed.

	Krzysztof

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

[Frank Li]

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

Good, thanks

Frank

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

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

[Bjorn Helgaas]

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

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

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

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

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

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

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

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

[Bjorn Helgaas]

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

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

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

Bjorn

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

[Frank Li]

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

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

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

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

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

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

I am not sure I understand your means.

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

Frank
>
> Bjorn

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

[Frank Li]

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

Bjorn:

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

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

Frank

> >
> > Bjorn

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

[Bjorn Helgaas]

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

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

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

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

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

A few of the things I wonder about:

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

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

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

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

Bjorn

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

[Frank Li]

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

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

Frank
>
> Bjorn

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

[Niklas Cassel]

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


Small nit that I saw when looking at this series.

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


Kind regards,
Niklas

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

[Bjorn Helgaas]

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

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

The callers of dw_pcie_prog_outbound_atu() I see are:

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

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

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

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

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

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

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

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

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

Bjorn

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

[Frank Li]

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

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

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

Yes, we can improve it later.

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

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

Frank

[PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li]

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

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

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

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

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

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

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

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

Add 'using_dtbus_info' to indicate device tree reflect correctly bus
address translation in case break compatibility.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v7 to v8
- Add Mani's reviewedby tag
- s/convert/map in commit message
- update comments for of_property_read_reg()
- use 'use_parent_dt_ranges'

Change from v6 to v7
- none

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

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

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

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 43ba5c6738df1..42719ad263b11 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -9,6 +9,7 @@
 #include <linux/align.h>
 #include <linux/bitfield.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include "pcie-designware.h"
@@ -294,7 +295,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.cpu_addr = addr - ep->phys_base + ep->bus_addr_base;
 	atu.pci_addr = pci_addr;
 	atu.size = size;
 	ret = dw_pcie_ep_outbound_atu(ep, &atu);
@@ -861,6 +862,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	struct device *dev = pci->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
+	int index;
 
 	INIT_LIST_HEAD(&ep->func_list);
 
@@ -873,6 +875,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 
 	ep->phys_base = res->start;
+	ep->bus_addr_base = ep->phys_base;
+
+	if (pci->use_parent_dt_ranges) {
+		index = of_property_match_string(np, "reg-names", "addr_space");
+		if (index < 0)
+			return -EINVAL;
+
+		/*
+		 * Get the untranslated bus address from devicetree to use it
+		 * as the iATU CPU address in dw_pcie_ep_map_addr().
+		 */
+		of_property_read_reg(np, index, &ep->bus_addr_base, NULL);
+	}
+
 	ep->addr_size = resource_size(res);
 
 	if (ep->ops->pre_init)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 4f31d4259a0de..5c14ed2cb91ed 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -410,6 +410,7 @@ struct dw_pcie_ep {
 	struct list_head	func_list;
 	const struct dw_pcie_ep_ops *ops;
 	phys_addr_t		phys_base;
+	u64			bus_addr_base;
 	size_t			addr_size;
 	size_t			page_size;
 	u8			bar_to_atu[PCI_STD_NUM_BARS];

-- 
2.34.1

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Bjorn Helgaas]

On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
>                    Endpoint
>   ┌───────────────────────────────────────────────┐
>   │                             pcie-ep@5f010000  │
>   │                             ┌────────────────┐│
>   │                             │   Endpoint     ││
>   │                             │   PCIe         ││
>   │                             │   Controller   ││
>   │           bus@5f000000      │                ││
>   │           ┌──────────┐      │                ││
>   │           │          │ Outbound Transfer     ││
>   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
>   ││     │    │  Fabric  │Bus   │                ││PCI Addr
>   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
>   ││     │CPU │          │0x8000_0000            ││
>   │└─────┘Addr└──────────┘      │                ││
>   │       0x7000_0000           └────────────────┘│
>   └───────────────────────────────────────────────┘
> 
> Use 'ranges' property in DT to configure the iATU outbound window address.
> The bus fabric generally passes the same address to the PCIe EP controller,
> but some bus fabrics map the address before sending it to the PCIe EP
> controller.
> 
> Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> input address and map to PCI address 0xA000_0000.
> 
> Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
> the device tree provides this information, preferring a common method.
> 
> bus@5f000000 {
> 	compatible = "simple-bus";
> 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
> 	pcie-ep@5f010000 {
> 		reg = <0x80000000 0x10000000>;
> 		reg-names ="addr_space";
> 		...
> 	};
> 	...
> };
> 
> 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> address.

Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
describe the translation for the window from bus addr 0x8000_0000 to
PCI addr 0xA000_0000?

I assume the pcie-ep@5f010000 controller also has its own registers in
the bus addr space, separate from the window to PCI, and its 'reg'
property would describe those?

The similar patch at [1] includes:

  pcie@5f010000 {
    reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;

I assumed that [bus 0x5f010000-0x5f01ffff] is PCIe controller register
space and [bus 0x8ff00000-0x8ff7ffff] is ECAM space.

But that can't be right because ECAM requires 1MB per bus, and
[bus 0x8ff00000-0x8ff7ffff] is only 512KB.

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

Why is this different from [1], where parent_bus_addr comes from the
'ranges' property?  Isn't this the same exact kind of address
translation for both RC and EP mode?

> Add 'using_dtbus_info' to indicate device tree reflect correctly bus
> address translation in case break compatibility.

'using_dtbus_info' doesn't exist; I assume this should be
'use_parent_dt_ranges'?

Sorry I'm so confused, please help me out :)

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

> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v7 to v8
> - Add Mani's reviewedby tag
> - s/convert/map in commit message
> - update comments for of_property_read_reg()
> - use 'use_parent_dt_ranges'
> 
> Change from v6 to v7
> - none
> 
> Change from v5 to v6
> - update diagram
> - Add comments for of_property_read_reg()
> - Remove unrelated 0x5f00_0000 in commit message
> 
> Change from v3 to v4
> - change bus_addr_base to u64 to fix 32bit build error
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410230328.BTHareG1-lkp@intel.com/
> 
> Change from v2 to v3
> - Add using_dtbus_info to control if use device tree bus ranges
> information.
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h    |  1 +
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 43ba5c6738df1..42719ad263b11 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -9,6 +9,7 @@
>  #include <linux/align.h>
>  #include <linux/bitfield.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  
>  #include "pcie-designware.h"
> @@ -294,7 +295,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.cpu_addr = addr - ep->phys_base + ep->bus_addr_base;
>  	atu.pci_addr = pci_addr;
>  	atu.size = size;
>  	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> @@ -861,6 +862,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	struct device *dev = pci->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct device_node *np = dev->of_node;
> +	int index;
>  
>  	INIT_LIST_HEAD(&ep->func_list);
>  
> @@ -873,6 +875,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		return -EINVAL;
>  
>  	ep->phys_base = res->start;
> +	ep->bus_addr_base = ep->phys_base;
> +
> +	if (pci->use_parent_dt_ranges) {
> +		index = of_property_match_string(np, "reg-names", "addr_space");
> +		if (index < 0)
> +			return -EINVAL;
> +
> +		/*
> +		 * Get the untranslated bus address from devicetree to use it
> +		 * as the iATU CPU address in dw_pcie_ep_map_addr().
> +		 */
> +		of_property_read_reg(np, index, &ep->bus_addr_base, NULL);
> +	}
> +
>  	ep->addr_size = resource_size(res);
>  
>  	if (ep->ops->pre_init)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 4f31d4259a0de..5c14ed2cb91ed 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -410,6 +410,7 @@ struct dw_pcie_ep {
>  	struct list_head	func_list;
>  	const struct dw_pcie_ep_ops *ops;
>  	phys_addr_t		phys_base;
> +	u64			bus_addr_base;
>  	size_t			addr_size;
>  	size_t			page_size;
>  	u8			bar_to_atu[PCI_STD_NUM_BARS];
> 
> -- 
> 2.34.1
> 

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li]

On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
> >                    Endpoint
> >   ┌───────────────────────────────────────────────┐
> >   │                             pcie-ep@5f010000  │
> >   │                             ┌────────────────┐│
> >   │                             │   Endpoint     ││
> >   │                             │   PCIe         ││
> >   │                             │   Controller   ││
> >   │           bus@5f000000      │                ││
> >   │           ┌──────────┐      │                ││
> >   │           │          │ Outbound Transfer     ││
> >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
> >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
> >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
> >   ││     │CPU │          │0x8000_0000            ││
> >   │└─────┘Addr└──────────┘      │                ││
> >   │       0x7000_0000           └────────────────┘│
> >   └───────────────────────────────────────────────┘
> >
> > Use 'ranges' property in DT to configure the iATU outbound window address.
> > The bus fabric generally passes the same address to the PCIe EP controller,
> > but some bus fabrics map the address before sending it to the PCIe EP
> > controller.
> >
> > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> > input address and map to PCI address 0xA000_0000.
> >
> > Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
> > the device tree provides this information, preferring a common method.
> >
> > bus@5f000000 {
> > 	compatible = "simple-bus";
> > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >
> > 	pcie-ep@5f010000 {
> > 		reg = <0x80000000 0x10000000>;
> > 		reg-names ="addr_space";
> > 		...
> > 	};
> > 	...
> > };
> >
> > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> > address.
>
> Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
> describe the translation for the window from bus addr 0x8000_0000 to
> PCI addr 0xA000_0000?

Needn't 'ragnes' under pcie-ep@5f010000 because history reason. DWC use
reg-names "addr_space" descript outbound windows space.

>
> I assume the pcie-ep@5f010000 controller also has its own registers in
> the bus addr space, separate from the window to PCI, and its 'reg'
> property would describe those?

Yes

>
> The similar patch at [1] includes:
>
>   pcie@5f010000 {
>     reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;

Yes, but "<0x5f010000 0x10000>" is not related with this outbound windows
translation. So I delete it.

>
> I assumed that [bus 0x5f010000-0x5f01ffff] is PCIe controller register
> space and [bus 0x8ff00000-0x8ff7ffff] is ECAM space.

For EP side, needn't export pci config space for dwc controller.

>
> But that can't be right because ECAM requires 1MB per bus, and
> [bus 0x8ff00000-0x8ff7ffff] is only 512KB.
>
> > Use `of_property_read_reg()` to obtain the bus address and set it to the
> > ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup().
>
> Why is this different from [1], where parent_bus_addr comes from the
> 'ranges' property?  Isn't this the same exact kind of address
> translation for both RC and EP mode?

The method is the same, but space means is difference.

RC side:
   regs, 1: controller register, 2: config space, (although it should be
in ranges)
   ranges, (IO range and Memory range).

EP side:
   regs, 1: controller register, 2: outbound windows space.("addr_space")

All regs need call of_property_read_reg() to get untranslated address.
ranges:  use "parent_bus_addr" in [1].

>
> > Add 'using_dtbus_info' to indicate device tree reflect correctly bus
> > address translation in case break compatibility.
>
> 'using_dtbus_info' doesn't exist; I assume this should be
> 'use_parent_dt_ranges'?

Yes, sorry, I forget updae it.

Frank

>
> Sorry I'm so confused, please help me out :)
>
> [1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-1-c4bfa5193288@nxp.com
>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v7 to v8
> > - Add Mani's reviewedby tag
> > - s/convert/map in commit message
> > - update comments for of_property_read_reg()
> > - use 'use_parent_dt_ranges'
> >
> > Change from v6 to v7
> > - none
> >
> > Change from v5 to v6
> > - update diagram
> > - Add comments for of_property_read_reg()
> > - Remove unrelated 0x5f00_0000 in commit message
> >
> > Change from v3 to v4
> > - change bus_addr_base to u64 to fix 32bit build error
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202410230328.BTHareG1-lkp@intel.com/
> >
> > Change from v2 to v3
> > - Add using_dtbus_info to control if use device tree bus ranges
> > information.
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 18 +++++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h    |  1 +
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 43ba5c6738df1..42719ad263b11 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/align.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/of.h>
> > +#include <linux/of_address.h>
> >  #include <linux/platform_device.h>
> >
> >  #include "pcie-designware.h"
> > @@ -294,7 +295,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.cpu_addr = addr - ep->phys_base + ep->bus_addr_base;
> >  	atu.pci_addr = pci_addr;
> >  	atu.size = size;
> >  	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > @@ -861,6 +862,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  	struct device *dev = pci->dev;
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct device_node *np = dev->of_node;
> > +	int index;
> >
> >  	INIT_LIST_HEAD(&ep->func_list);
> >
> > @@ -873,6 +875,20 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		return -EINVAL;
> >
> >  	ep->phys_base = res->start;
> > +	ep->bus_addr_base = ep->phys_base;
> > +
> > +	if (pci->use_parent_dt_ranges) {
> > +		index = of_property_match_string(np, "reg-names", "addr_space");
> > +		if (index < 0)
> > +			return -EINVAL;
> > +
> > +		/*
> > +		 * Get the untranslated bus address from devicetree to use it
> > +		 * as the iATU CPU address in dw_pcie_ep_map_addr().
> > +		 */
> > +		of_property_read_reg(np, index, &ep->bus_addr_base, NULL);
> > +	}
> > +
> >  	ep->addr_size = resource_size(res);
> >
> >  	if (ep->ops->pre_init)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 4f31d4259a0de..5c14ed2cb91ed 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -410,6 +410,7 @@ struct dw_pcie_ep {
> >  	struct list_head	func_list;
> >  	const struct dw_pcie_ep_ops *ops;
> >  	phys_addr_t		phys_base;
> > +	u64			bus_addr_base;
> >  	size_t			addr_size;
> >  	size_t			page_size;
> >  	u8			bar_to_atu[PCI_STD_NUM_BARS];
> >
> > --
> > 2.34.1
> >

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Bjorn Helgaas]

On Thu, Jan 16, 2025 at 01:04:16PM -0500, Frank Li wrote:
> On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
> > On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
> > >                    Endpoint
> > >   ┌───────────────────────────────────────────────┐
> > >   │                             pcie-ep@5f010000  │
> > >   │                             ┌────────────────┐│
> > >   │                             │   Endpoint     ││
> > >   │                             │   PCIe         ││
> > >   │                             │   Controller   ││
> > >   │           bus@5f000000      │                ││
> > >   │           ┌──────────┐      │                ││
> > >   │           │          │ Outbound Transfer     ││
> > >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
> > >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
> > >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
> > >   ││     │CPU │          │0x8000_0000            ││
> > >   │└─────┘Addr└──────────┘      │                ││
> > >   │       0x7000_0000           └────────────────┘│
> > >   └───────────────────────────────────────────────┘
> > >
> > > Use 'ranges' property in DT to configure the iATU outbound window address.
> > > The bus fabric generally passes the same address to the PCIe EP controller,
> > > but some bus fabrics map the address before sending it to the PCIe EP
> > > controller.
> > >
> > > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> > > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> > > input address and map to PCI address 0xA000_0000.
> > >
> > > Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
> > > the device tree provides this information, preferring a common method.
> > >
> > > bus@5f000000 {
> > > 	compatible = "simple-bus";
> > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > >
> > > 	pcie-ep@5f010000 {
> > > 		reg = <0x80000000 0x10000000>;
> > > 		reg-names ="addr_space";
> > > 		...
> > > 	};
> > > 	...
> > > };
> > >
> > > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> > > address.
> >
> > Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
> > describe the translation for the window from bus addr 0x8000_0000 to
> > PCI addr 0xA000_0000?
> 
> Needn't 'ranges' under pcie-ep@5f010000 because history reason. DWC use
> reg-names "addr_space" descript outbound windows space.

If reg-name "addr_space" is used instead of 'ranges' for some
historical reason, we should mention that in the commit log so people
don't assume that this difference is the way it's *supposed* to be
done.

I only see "addr_space" mentioned in
Documentation/devicetree/bindings/pci/*-ep.yaml, so I assume
this "addr_space" usage only applies to endpoints?  

> > > Use `of_property_read_reg()` to obtain the bus address and set it to the
> > > ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup().
> >
> > Why is this different from [1], where parent_bus_addr comes from the
> > 'ranges' property?  Isn't this the same exact kind of address
> > translation for both RC and EP mode?
> 
> The method is the same, but space means is difference.
> 
> RC side:
>    regs, 1: controller register, 2: config space, (although it should be
> in ranges)
>    ranges, (IO range and Memory range).
> 
> EP side:
>    regs, 1: controller register, 2: outbound windows space.("addr_space")
> 
> All regs need call of_property_read_reg() to get untranslated address.
> ranges:  use "parent_bus_addr" in [1].

I think we should at least use the same name ("parent_bus_addr", not
"bus_addr_base") and probably also figure out a wrapper or similar way
to use 'ranges' for future endpoint drivers and fall back to
"addr_space" for DWC.

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

> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -410,6 +410,7 @@ struct dw_pcie_ep {
> > >  	struct list_head	func_list;
> > >  	const struct dw_pcie_ep_ops *ops;
> > >  	phys_addr_t		phys_base;
> > > +	u64			bus_addr_base;
> > >  	size_t			addr_size;
> > >  	size_t			page_size;
> > >  	u8			bar_to_atu[PCI_STD_NUM_BARS];

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li]

On Thu, Jan 16, 2025 at 01:45:58PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 16, 2025 at 01:04:16PM -0500, Frank Li wrote:
> > On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
> > > On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
> > > >                    Endpoint
> > > >   ┌───────────────────────────────────────────────┐
> > > >   │                             pcie-ep@5f010000  │
> > > >   │                             ┌────────────────┐│
> > > >   │                             │   Endpoint     ││
> > > >   │                             │   PCIe         ││
> > > >   │                             │   Controller   ││
> > > >   │           bus@5f000000      │                ││
> > > >   │           ┌──────────┐      │                ││
> > > >   │           │          │ Outbound Transfer     ││
> > > >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
> > > >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
> > > >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
> > > >   ││     │CPU │          │0x8000_0000            ││
> > > >   │└─────┘Addr└──────────┘      │                ││
> > > >   │       0x7000_0000           └────────────────┘│
> > > >   └───────────────────────────────────────────────┘
> > > >
> > > > Use 'ranges' property in DT to configure the iATU outbound window address.
> > > > The bus fabric generally passes the same address to the PCIe EP controller,
> > > > but some bus fabrics map the address before sending it to the PCIe EP
> > > > controller.
> > > >
> > > > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> > > > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> > > > input address and map to PCI address 0xA000_0000.
> > > >
> > > > Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
> > > > the device tree provides this information, preferring a common method.
> > > >
> > > > bus@5f000000 {
> > > > 	compatible = "simple-bus";
> > > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > > >
> > > > 	pcie-ep@5f010000 {
> > > > 		reg = <0x80000000 0x10000000>;
> > > > 		reg-names ="addr_space";
> > > > 		...
> > > > 	};
> > > > 	...
> > > > };
> > > >
> > > > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> > > > address.
> > >
> > > Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
> > > describe the translation for the window from bus addr 0x8000_0000 to
> > > PCI addr 0xA000_0000?
> >
> > Needn't 'ranges' under pcie-ep@5f010000 because history reason. DWC use
> > reg-names "addr_space" descript outbound windows space.
>
> If reg-name "addr_space" is used instead of 'ranges' for some
> historical reason, we should mention that in the commit log so people
> don't assume that this difference is the way it's *supposed* to be
> done.

How about add comments after

reg-names ="addr_space"; // Indicate EP outbound windows space instead use
ranges by histortical reason.

>
> I only see "addr_space" mentioned in
> Documentation/devicetree/bindings/pci/*-ep.yaml, so I assume
> this "addr_space" usage only applies to endpoints?

Yes, "addr_space" usage only applies to endpoints.

>
> > > > Use `of_property_read_reg()` to obtain the bus address and set it to the
> > > > ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup().
> > >
> > > Why is this different from [1], where parent_bus_addr comes from the
> > > 'ranges' property?  Isn't this the same exact kind of address
> > > translation for both RC and EP mode?
> >
> > The method is the same, but space means is difference.
> >
> > RC side:
> >    regs, 1: controller register, 2: config space, (although it should be
> > in ranges)
> >    ranges, (IO range and Memory range).
> >
> > EP side:
> >    regs, 1: controller register, 2: outbound windows space.("addr_space")
> >
> > All regs need call of_property_read_reg() to get untranslated address.
> > ranges:  use "parent_bus_addr" in [1].
>
> I think we should at least use the same name ("parent_bus_addr", not
> "bus_addr_base") and probably also figure out a wrapper or similar way
> to use 'ranges' for future endpoint drivers and fall back to
> "addr_space" for DWC.

Okay for name parent_bus_addr.
Do you need me to respin it? Or you change it by yourself?

Frank

>
> > > [1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-1-c4bfa5193288@nxp.com
>
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -410,6 +410,7 @@ struct dw_pcie_ep {
> > > >  	struct list_head	func_list;
> > > >  	const struct dw_pcie_ep_ops *ops;
> > > >  	phys_addr_t		phys_base;
> > > > +	u64			bus_addr_base;
> > > >  	size_t			addr_size;
> > > >  	size_t			page_size;
> > > >  	u8			bar_to_atu[PCI_STD_NUM_BARS];

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Bjorn Helgaas]

On Thu, Jan 16, 2025 at 03:02:44PM -0500, Frank Li wrote:
> On Thu, Jan 16, 2025 at 01:45:58PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 16, 2025 at 01:04:16PM -0500, Frank Li wrote:
> > > On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
> > > > >                    Endpoint
> > > > >   ┌───────────────────────────────────────────────┐
> > > > >   │                             pcie-ep@5f010000  │
> > > > >   │                             ┌────────────────┐│
> > > > >   │                             │   Endpoint     ││
> > > > >   │                             │   PCIe         ││
> > > > >   │                             │   Controller   ││
> > > > >   │           bus@5f000000      │                ││
> > > > >   │           ┌──────────┐      │                ││
> > > > >   │           │          │ Outbound Transfer     ││
> > > > >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
> > > > >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
> > > > >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
> > > > >   ││     │CPU │          │0x8000_0000            ││
> > > > >   │└─────┘Addr└──────────┘      │                ││
> > > > >   │       0x7000_0000           └────────────────┘│
> > > > >   └───────────────────────────────────────────────┘
> > > > >
> > > > > Use 'ranges' property in DT to configure the iATU outbound window address.
> > > > > The bus fabric generally passes the same address to the PCIe EP controller,
> > > > > but some bus fabrics map the address before sending it to the PCIe EP
> > > > > controller.
> > > > >
> > > > > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> > > > > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> > > > > input address and map to PCI address 0xA000_0000.
> > > > >
> > > > > Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
> > > > > the device tree provides this information, preferring a common method.
> > > > >
> > > > > bus@5f000000 {
> > > > > 	compatible = "simple-bus";
> > > > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > > > >
> > > > > 	pcie-ep@5f010000 {
> > > > > 		reg = <0x80000000 0x10000000>;
> > > > > 		reg-names ="addr_space";
> > > > > 		...
> > > > > 	};
> > > > > 	...
> > > > > };
> > > > >
> > > > > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> > > > > address.
> > > >
> > > > Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
> > > > describe the translation for the window from bus addr 0x8000_0000 to
> > > > PCI addr 0xA000_0000?
> > >
> > > Needn't 'ranges' under pcie-ep@5f010000 because history reason. DWC use
> > > reg-names "addr_space" descript outbound windows space.
> >
> > If reg-name "addr_space" is used instead of 'ranges' for some
> > historical reason, we should mention that in the commit log so people
> > don't assume that this difference is the way it's *supposed* to be
> > done.
> 
> How about add comments after
> 
> reg-names ="addr_space"; // Indicate EP outbound windows space instead use
> ranges by histortical reason.

OK, that seems reasonable.

Where does the 0xA000_0000 PCI address come from?  I assume that's in
DT somewhere too?

Is there a binding in the tree that would take advantage of this patch
that I can look at?  arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi 
has bus@5f000000 that does this translation, but I don't see any
endpoint mode that uses it.

> > > All regs need call of_property_read_reg() to get untranslated address.
> > > ranges:  use "parent_bus_addr" in [1].
> >
> > I think we should at least use the same name ("parent_bus_addr", not
> > "bus_addr_base") and probably also figure out a wrapper or similar way
> > to use 'ranges' for future endpoint drivers and fall back to
> > "addr_space" for DWC.
> 
> Okay for name parent_bus_addr.
> Do you need me to respin it? Or you change it by yourself?

I can do that.

Bjorn

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

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Manivannan Sadhasivam]

On January 17, 2025 4:19:02 AM GMT+05:30, Bjorn Helgaas <helgaas@kernel.org> wrote:
>On Thu, Jan 16, 2025 at 03:02:44PM -0500, Frank Li wrote:
>> On Thu, Jan 16, 2025 at 01:45:58PM -0600, Bjorn Helgaas wrote:
>> > On Thu, Jan 16, 2025 at 01:04:16PM -0500, Frank Li wrote:
>> > > On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
>> > > > On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
>> > > > >                    Endpoint
>> > > > >   ┌───────────────────────────────────────────────┐
>> > > > >   │                             pcie-ep@5f010000  │
>> > > > >   │                             ┌────────────────┐│
>> > > > >   │                             │   Endpoint     ││
>> > > > >   │                             │   PCIe         ││
>> > > > >   │                             │   Controller   ││
>> > > > >   │           bus@5f000000      │                ││
>> > > > >   │           ┌──────────┐      │                ││
>> > > > >   │           │          │ Outbound Transfer     ││
>> > > > >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
>> > > > >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
>> > > > >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
>> > > > >   ││     │CPU │          │0x8000_0000            ││
>> > > > >   │└─────┘Addr└──────────┘      │                ││
>> > > > >   │       0x7000_0000           └────────────────┘│
>> > > > >   └───────────────────────────────────────────────┘
>> > > > >
>> > > > > Use 'ranges' property in DT to configure the iATU outbound window address.
>> > > > > The bus fabric generally passes the same address to the PCIe EP controller,
>> > > > > but some bus fabrics map the address before sending it to the PCIe EP
>> > > > > controller.
>> > > > >
>> > > > > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
>> > > > > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
>> > > > > input address and map to PCI address 0xA000_0000.
>> > > > >
>> > > > > Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
>> > > > > the device tree provides this information, preferring a common method.
>> > > > >
>> > > > > bus@5f000000 {
>> > > > > 	compatible = "simple-bus";
>> > > > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
>> > > > >
>> > > > > 	pcie-ep@5f010000 {
>> > > > > 		reg = <0x80000000 0x10000000>;
>> > > > > 		reg-names ="addr_space";
>> > > > > 		...
>> > > > > 	};
>> > > > > 	...
>> > > > > };
>> > > > >
>> > > > > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
>> > > > > address.
>> > > >
>> > > > Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
>> > > > describe the translation for the window from bus addr 0x8000_0000 to
>> > > > PCI addr 0xA000_0000?
>> > >
>> > > Needn't 'ranges' under pcie-ep@5f010000 because history reason. DWC use
>> > > reg-names "addr_space" descript outbound windows space.
>> >
>> > If reg-name "addr_space" is used instead of 'ranges' for some
>> > historical reason, we should mention that in the commit log so people
>> > don't assume that this difference is the way it's *supposed* to be
>> > done.
>> 
>> How about add comments after
>> 
>> reg-names ="addr_space"; // Indicate EP outbound windows space instead use
>> ranges by histortical reason.
>
>OK, that seems reasonable.
>

Unfortunately not. Please see below.

>Where does the 0xA000_0000 PCI address come from?  I assume that's in
>DT somewhere too?
>

No, PCI address is from host address space, hence it cannot be described in DT. That's the reason why 'addr_space' reg property is used to define outbound window region. iATU will map the host PCI address to endpoint outbound window region dynamically based on usecase (like mapping the buffer in host address space).

The translation between CPU:BUS address space is a hardware property, hence using 'ranges' to describe them makes sense.

But the same cannot be applied for BUS:PCI address space. Maybe the diagram had misled you thinking that PCI address is also static.

- Mani

>Is there a binding in the tree that would take advantage of this patch
>that I can look at?  arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi 
>has bus@5f000000 that does this translation, but I don't see any
>endpoint mode that uses it.
>
>> > > All regs need call of_property_read_reg() to get untranslated address.
>> > > ranges:  use "parent_bus_addr" in [1].
>> >
>> > I think we should at least use the same name ("parent_bus_addr", not
>> > "bus_addr_base") and probably also figure out a wrapper or similar way
>> > to use 'ranges' for future endpoint drivers and fall back to
>> > "addr_space" for DWC.
>> 
>> Okay for name parent_bus_addr.
>> Do you need me to respin it? Or you change it by yourself?
>
>I can do that.
>
>Bjorn
>
>> > > > [1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-1-c4bfa5193288@nxp.com

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

Re: [PATCH v8 3/7] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li]

On Fri, Jan 17, 2025 at 08:05:16PM +0530, Manivannan Sadhasivam wrote:
>
>
> On January 17, 2025 4:19:02 AM GMT+05:30, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >On Thu, Jan 16, 2025 at 03:02:44PM -0500, Frank Li wrote:
> >> On Thu, Jan 16, 2025 at 01:45:58PM -0600, Bjorn Helgaas wrote:
> >> > On Thu, Jan 16, 2025 at 01:04:16PM -0500, Frank Li wrote:
> >> > > On Thu, Jan 16, 2025 at 09:32:39AM -0600, Bjorn Helgaas wrote:
> >> > > > On Tue, Nov 19, 2024 at 02:44:21PM -0500, Frank Li wrote:
> >> > > > >                    Endpoint
> >> > > > >   ┌───────────────────────────────────────────────┐
> >> > > > >   │                             pcie-ep@5f010000  │
> >> > > > >   │                             ┌────────────────┐│
> >> > > > >   │                             │   Endpoint     ││
> >> > > > >   │                             │   PCIe         ││
> >> > > > >   │                             │   Controller   ││
> >> > > > >   │           bus@5f000000      │                ││
> >> > > > >   │           ┌──────────┐      │                ││
> >> > > > >   │           │          │ Outbound Transfer     ││
> >> > > > >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
> >> > > > >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
> >> > > > >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
> >> > > > >   ││     │CPU │          │0x8000_0000            ││
> >> > > > >   │└─────┘Addr└──────────┘      │                ││
> >> > > > >   │       0x7000_0000           └────────────────┘│
> >> > > > >   └───────────────────────────────────────────────┘
> >> > > > >
> >> > > > > Use 'ranges' property in DT to configure the iATU outbound window address.
> >> > > > > The bus fabric generally passes the same address to the PCIe EP controller,
> >> > > > > but some bus fabrics map the address before sending it to the PCIe EP
> >> > > > > controller.
> >> > > > >
> >> > > > > Above diagram, CPU write data to outbound windows address 0x7000_0000, Bus
> >> > > > > fabric map it to 0x8000_0000. ATU should use bus address 0x8000_0000 as
> >> > > > > input address and map to PCI address 0xA000_0000.
> >> > > > >
> >> > > > > Previously, 'cpu_addr_fixup()' was used to handle address conversion. Now,
> >> > > > > the device tree provides this information, preferring a common method.
> >> > > > >
> >> > > > > bus@5f000000 {
> >> > > > > 	compatible = "simple-bus";
> >> > > > > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >> > > > >
> >> > > > > 	pcie-ep@5f010000 {
> >> > > > > 		reg = <0x80000000 0x10000000>;
> >> > > > > 		reg-names ="addr_space";
> >> > > > > 		...
> >> > > > > 	};
> >> > > > > 	...
> >> > > > > };
> >> > > > >
> >> > > > > 'ranges' in bus@5f000000 descript how address map from CPU address to bus
> >> > > > > address.
> >> > > >
> >> > > > Shouldn't there also be a pcie-ep@5f010000 'ranges' property to
> >> > > > describe the translation for the window from bus addr 0x8000_0000 to
> >> > > > PCI addr 0xA000_0000?
> >> > >
> >> > > Needn't 'ranges' under pcie-ep@5f010000 because history reason. DWC use
> >> > > reg-names "addr_space" descript outbound windows space.
> >> >
> >> > If reg-name "addr_space" is used instead of 'ranges' for some
> >> > historical reason, we should mention that in the commit log so people
> >> > don't assume that this difference is the way it's *supposed* to be
> >> > done.
> >>
> >> How about add comments after
> >>
> >> reg-names ="addr_space"; // Indicate EP outbound windows space instead use
> >> ranges by histortical reason.
> >
> >OK, that seems reasonable.
> >
>
> Unfortunately not. Please see below.

Yes, you are right.

>
> >Where does the 0xA000_0000 PCI address come from?  I assume that's in
> >DT somewhere too?
> >
>
> No, PCI address is from host address space, hence it cannot be described in DT. That's the reason why 'addr_space' reg property is used to define outbound window region. iATU will map the host PCI address to endpoint outbound window region dynamically based on usecase (like mapping the buffer in host address space).
>
> The translation between CPU:BUS address space is a hardware property, hence using 'ranges' to describe them makes sense.
>
> But the same cannot be applied for BUS:PCI address space. Maybe the diagram had misled you thinking that PCI address is also static.

Yes, 0xA000_0000 come from PCI host side, generally allocate by dma API.
Use other channel (like registers) to told EP side how to map it.

>
> - Mani
>
> >Is there a binding in the tree that would take advantage of this patch
> >that I can look at?  arch/arm64/boot/dts/freescale/imx8-ss-hsio.dtsi
> >has bus@5f000000 that does this translation, but I don't see any
> >endpoint mode that uses it.

Not upstream yet. I plan do this after this patch merged.

Frank

> >
> >> > > All regs need call of_property_read_reg() to get untranslated address.
> >> > > ranges:  use "parent_bus_addr" in [1].
> >> >
> >> > I think we should at least use the same name ("parent_bus_addr", not
> >> > "bus_addr_base") and probably also figure out a wrapper or similar way
> >> > to use 'ranges' for future endpoint drivers and fall back to
> >> > "addr_space" for DWC.
> >>
> >> Okay for name parent_bus_addr.
> >> Do you need me to respin it? Or you change it by yourself?
> >
> >I can do that.
> >
> >Bjorn
> >
> >> > > > [1] https://lore.kernel.org/r/20241119-pci_fixup_addr-v8-1-c4bfa5193288@nxp.com
>
> மணிவண்ணன் சதாசிவம்

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

[Frank Li]

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

Acked-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v7 to v8
- add mani and richard's review/ack tag
- use varible 'use_parent_dt_ranges'

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 808d1f1054173..cf033e672dbde 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -81,7 +81,6 @@ enum imx_pcie_variants {
 #define IMX_PCIE_FLAG_HAS_PHY_RESET		BIT(5)
 #define IMX_PCIE_FLAG_HAS_SERDES		BIT(6)
 #define IMX_PCIE_FLAG_SUPPORT_64BIT		BIT(7)
-#define IMX_PCIE_FLAG_CPU_ADDR_FIXUP		BIT(8)
 
 #define imx_check_flag(pci, val)	(pci->drvdata->flags & val)
 
@@ -1012,22 +1011,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;
-}
-
 static const struct dw_pcie_host_ops imx_pcie_host_ops = {
 	.init = imx_pcie_host_init,
 	.deinit = imx_pcie_host_exit,
@@ -1036,7 +1019,6 @@ static const struct dw_pcie_host_ops imx_pcie_host_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)
@@ -1446,6 +1428,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)
@@ -1585,8 +1568,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 	},
 	[IMX8Q] = {
 		.variant = IMX8Q,
-		.flags = IMX_PCIE_FLAG_HAS_PHYDRV |
-			 IMX_PCIE_FLAG_CPU_ADDR_FIXUP,
+		.flags = IMX_PCIE_FLAG_HAS_PHYDRV,
 		.clk_names = imx8q_clks,
 		.clks_cnt = ARRAY_SIZE(imx8q_clks),
 	},

-- 
2.34.1

[PATCH v8 5/7] dt-bindings: PCI: fsl,imx6q-pcie-ep: Add compatible string fsl,imx8q-pcie-ep

[Frank Li]

Add new compatible string fsl,imx8q-pcie-ep for iMX8Q. reg-names only needs
'dbi' and 'addr_space' because the others are located at default offset.
The clock-names align Root Complex (RC)'s naming.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3 to v8
- none
Change from v2 to v3
- Add conor review tag
---
 .../devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml | 38 +++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
index 84ca12e8b25be..7bd00faa1f2c3 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
@@ -22,6 +22,7 @@ properties:
       - fsl,imx8mm-pcie-ep
       - fsl,imx8mq-pcie-ep
       - fsl,imx8mp-pcie-ep
+      - fsl,imx8q-pcie-ep
       - fsl,imx95-pcie-ep
 
   clocks:
@@ -74,6 +75,20 @@ allOf:
             - const: dbi2
             - const: atu
 
+  - if:
+      properties:
+        compatible:
+          enum:
+            - fsl,imx8q-pcie-ep
+    then:
+      properties:
+        reg:
+          maxItems: 2
+        reg-names:
+          items:
+            - const: dbi
+            - const: addr_space
+
   - if:
       properties:
         compatible:
@@ -109,7 +124,14 @@ allOf:
             - const: pcie_bus
             - const: pcie_phy
             - const: pcie_aux
-    else:
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - fsl,imx8mm-pcie-ep
+            - fsl,imx8mp-pcie-ep
+    then:
       properties:
         clocks:
           maxItems: 3
@@ -119,6 +141,20 @@ allOf:
             - const: pcie_bus
             - const: pcie_aux
 
+  - if:
+      properties:
+        compatible:
+          enum:
+            - fsl,imxq-pcie-ep
+    then:
+      properties:
+        clocks:
+          maxItems: 3
+        clock-names:
+          items:
+            - const: dbi
+            - const: mstr
+            - const: slv
 
 unevaluatedProperties: false
 

-- 
2.34.1

[PATCH v8 6/7] PCI: imx6: Pass correct sub mode when calling phy_set_mode_ext()

[Frank Li]

Fix hardcoding to Root Complex (RC) mode by adding a drvdata mode check.
Pass PHY_MODE_PCIE_EP if the PCI controller operates in Endpoint (EP) mode.

Fixes: 8026f2d8e8a9 ("PCI: imx6: Call common PHY API to set mode, speed, and submode")
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3->v8
- none
Change from v2->v3
- Add mani's review tag
---
 drivers/pci/controller/dwc/pci-imx6.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index cf033e672dbde..5303dfc3dbb41 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -960,7 +960,9 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_clk_disable;
 		}
 
-		ret = phy_set_mode_ext(imx_pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
+		ret = phy_set_mode_ext(imx_pcie->phy, PHY_MODE_PCIE,
+				       imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE ?
+						PHY_MODE_PCIE_EP : PHY_MODE_PCIE_RC);
 		if (ret) {
 			dev_err(dev, "unable to set PCIe PHY mode\n");
 			goto err_phy_exit;

-- 
2.34.1

[PATCH v8 7/7] PCI: imx6: Add i.MX8Q PCIe Endpoint (EP) support

[Frank Li]

Add support for i.MX8Q series (i.MX8QM, i.MX8QXP, and i.MX8DXL) PCIe
Endpoint (EP). On i.MX8Q platforms, the PCI bus addresses differ from the
CPU addresses. The DesignWare (DWC) driver already handles this in the
common code.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Chagne from v3 to v8
- none
change from v2 to v3
- add Mani's review tag
- Add pci->using_dtbus_info = true;
---
 drivers/pci/controller/dwc/pci-imx6.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5303dfc3dbb41..d457514d17485 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -70,6 +70,7 @@ enum imx_pcie_variants {
 	IMX8MQ_EP,
 	IMX8MM_EP,
 	IMX8MP_EP,
+	IMX8Q_EP,
 	IMX95_EP,
 };
 
@@ -1061,6 +1062,16 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
 	.align = SZ_64K,
 };
 
+static const struct pci_epc_features imx8q_pcie_epc_features = {
+	.linkup_notifier = false,
+	.msi_capable = true,
+	.msix_capable = false,
+	.bar[BAR_1] = { .type = BAR_RESERVED, },
+	.bar[BAR_3] = { .type = BAR_RESERVED, },
+	.bar[BAR_5] = { .type = BAR_RESERVED, },
+	.align = SZ_64K,
+};
+
 /*
  * BAR#	| Default BAR enable	| Default BAR Type	| Default BAR Size	| BAR Sizing Scheme
  * ================================================================================================
@@ -1627,6 +1638,14 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.epc_features = &imx8m_pcie_epc_features,
 		.enable_ref_clk = imx8mm_pcie_enable_ref_clk,
 	},
+	[IMX8Q_EP] = {
+		.variant = IMX8Q_EP,
+		.flags = IMX_PCIE_FLAG_HAS_PHYDRV,
+		.mode = DW_PCIE_EP_TYPE,
+		.epc_features = &imx8q_pcie_epc_features,
+		.clk_names = imx8q_clks,
+		.clks_cnt = ARRAY_SIZE(imx8q_clks),
+	},
 	[IMX95_EP] = {
 		.variant = IMX95_EP,
 		.flags = IMX_PCIE_FLAG_HAS_SERDES |
@@ -1656,6 +1675,7 @@ static const struct of_device_id imx_pcie_of_match[] = {
 	{ .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
 	{ .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },
 	{ .compatible = "fsl,imx8mp-pcie-ep", .data = &drvdata[IMX8MP_EP], },
+	{ .compatible = "fsl,imx8q-pcie-ep", .data = &drvdata[IMX8Q_EP], },
 	{ .compatible = "fsl,imx95-pcie-ep", .data = &drvdata[IMX95_EP], },
 	{},
 };

-- 
2.34.1

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

[Manivannan Sadhasivam]

On Tue, Nov 19, 2024 at 02:44:18PM -0500, Frank Li wrote:
> == RC side:
> 
>             ┌─────────┐                    ┌────────────┐
>  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
>  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
>  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
>   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> 0x7ff8_0000─┼───┘  │  │             │   │  │            │
>             │      │  │             │   │  │            │   PCI Addr
> 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
>             │         │             │      │            │    0
> 0x7000_0000─┼────────►├─────────┐   │      │            │
>             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
>              BUS Fabric         │          │            │    0
>                                 │          │            │
>                                 └──────────► MemSpace  ─┼────────────►
>                         IA: 0x8000_0000    │            │  0x8000_0000
>                                            └────────────┘
> 
> Current dwc implimemnt, pci_fixup_addr() call back is needed when bus
> fabric convert cpu address before send to PCIe controller.
> 
>     bus@5f000000 {
>             compatible = "simple-bus";
>             #address-cells = <1>;
>             #size-cells = <1>;
>             ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
>             pcie@5f010000 {
>                     compatible = "fsl,imx8q-pcie";
>                     reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
>                     reg-names = "dbi", "config";
>                     #address-cells = <3>;
>                     #size-cells = <2>;
>                     device_type = "pci";
>                     bus-range = <0x00 0xff>;
>                     ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
>                              <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
>             ...
>             };
>     };
> 
> Device tree already can descript all address translate. Some hardware
> driver implement fixup function by mask some bits of cpu address. Last
> pci-imx6.c are little bit better by fetch memory resource's offset to do
> fixup.
> 
> static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> {
> 	...
> 	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> 	return cpu_addr - entry->offset;
> }
> 
> But it is not good by using IORESOURCE_MEM to fix up io/cfg address map
> although address translate is the same as IORESOURCE_MEM.
> 
> This patches to fetch untranslate range information for PCIe controller
> (pcie@5f010000: ranges). So current config ATU without cpu_fixup_addr().
> 
> == EP side:
> 
>                    Endpoint
>   ┌───────────────────────────────────────────────┐
>   │                             pcie-ep@5f010000  │
>   │                             ┌────────────────┐│
>   │                             │   Endpoint     ││
>   │                             │   PCIe         ││
>   │                             │   Controller   ││
>   │           bus@5f000000      │                ││
>   │           ┌──────────┐      │                ││
>   │           │          │ Outbound Transfer     ││
>   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
>   ││     │    │  Fabric  │Bus   │                ││PCI Addr
>   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
>   ││     │CPU │          │0x8000_0000            ││
>   │└─────┘Addr└──────────┘      │                ││
>   │       0x7000_0000           └────────────────┘│
>   └───────────────────────────────────────────────┘
> 
> bus@5f000000 {
>         compatible = "simple-bus";
>         ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
>         pcie-ep@5f010000 {
>                 reg = <0x5f010000 0x00010000>,
>                       <0x80000000 0x10000000>;
>                 reg-names = "dbi", "addr_space";
>                 ...                ^^^^
>         };
>         ...
> };
> 
> Add `bus_addr_base` to configure the outbound window address for CPU write.
> The BUS fabric generally passes the same address to the PCIe EP controller,
> but some BUS fabrics convert the address before sending it to the PCIe EP
> controller.
> 
> Above diagram, CPU write data to outbound windows address 0x7000_0000,
> Bus fabric convert it to 0x8000_0000. ATU should use BUS address
> 0x8000_0000 as input address and convert to PCI address 0xA000_0000.
> 
> Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> the device tree provides this information.
> 
> The both pave the road to eliminate ugle cpu_fixup_addr() callback function.
> 

Series looks good to me. Thanks a lot for your persistence! But it missed 6.13
cycle. So let's get it merged early once 6.13-rc1 is out.

- Mani

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Changes in v8:
> - Add mani's review tages
> - use rename use_dt_ranges to use_parent_dt_ranges
> - Add dev_warn_once to reminder to fix their dt file and remove
> cpu_fixup_addr() callback.
> - rename dw_pcie_get_untranslate_addr() to dw_pcie_get_parent_addr()
> - Link to v7: https://lore.kernel.org/r/20241029-pci_fixup_addr-v7-0-8310dc24fb7c@nxp.com
> 
> Changes in v7:
> - fix
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410291546.kvgEWJv7-lkp@intel.com/
> - Link to v6: https://lore.kernel.org/r/20241028-pci_fixup_addr-v6-0-ebebcd8fd4ff@nxp.com
> 
> Changes in v6:
> - merge RC and EP to one thread!
> - Link to v5: https://lore.kernel.org/r/20241015-pci_fixup_addr-v5-0-ced556c85270@nxp.com
> 
> Changes in v5:
> - update address order in diagram patches.
> - remove confused 0x5f00_0000 range
> - update patch1's commit message.
> - Link to v4: https://lore.kernel.org/r/20241008-pci_fixup_addr-v4-0-25e5200657bc@nxp.com
> 
> Changes in v4:
> - Improve commit message by add driver source code path.
> - Link to v3: https://lore.kernel.org/r/20240930-pci_fixup_addr-v3-0-80ee70352fc7@nxp.com
> 
> Changes in v3:
> - see each patch
> - Link to v2: https://lore.kernel.org/r/20240926-pci_fixup_addr-v2-0-e4524541edf4@nxp.com
> 
> Changes in v2:
> - see each patch
> - Link to v1: https://lore.kernel.org/r/20240924-pci_fixup_addr-v1-0-57d14a91ec4f@nxp.com
> 
> ---
> Frank Li (7):
>       of: address: Add parent_bus_addr to struct of_pci_range
>       PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
>       PCI: dwc: ep: Add bus_addr_base for outbound window
>       PCI: imx6: Remove cpu_addr_fixup()
>       dt-bindings: PCI: fsl,imx6q-pcie-ep: Add compatible string fsl,imx8q-pcie-ep
>       PCI: imx6: Pass correct sub mode when calling phy_set_mode_ext()
>       PCI: imx6: Add i.MX8Q PCIe Endpoint (EP) support
> 
>  .../devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml | 38 ++++++++++++++-
>  drivers/of/address.c                               |  2 +
>  drivers/pci/controller/dwc/pci-imx6.c              | 46 +++++++++--------
>  drivers/pci/controller/dwc/pcie-designware-ep.c    | 18 ++++++-
>  drivers/pci/controller/dwc/pcie-designware-host.c  | 57 +++++++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.c       |  9 ++++
>  drivers/pci/controller/dwc/pcie-designware.h       |  8 +++
>  include/linux/of_address.h                         |  1 +
>  8 files changed, 155 insertions(+), 24 deletions(-)
> ---
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> change-id: 20240924-pci_fixup_addr-a8568f9bbb34
> 
> Best regards,
> ---
> Frank Li <Frank.Li@nxp.com>
> 

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

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

[Frank Li]

On Sun, Nov 24, 2024 at 08:08:39PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Nov 19, 2024 at 02:44:18PM -0500, Frank Li wrote:
> > == RC side:
> >
> >             ┌─────────┐                    ┌────────────┐
> >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> >             │      │  │             │   │  │            │   PCI Addr
> > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> >             │         │             │      │            │    0
> > 0x7000_0000─┼────────►├─────────┐   │      │            │
> >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> >              BUS Fabric         │          │            │    0
> >                                 │          │            │
> >                                 └──────────► MemSpace  ─┼────────────►
> >                         IA: 0x8000_0000    │            │  0x8000_0000
> >                                            └────────────┘
> >
> > Current dwc implimemnt, pci_fixup_addr() call back is needed when bus
> > fabric convert cpu address before send to PCIe controller.
> >
> >     bus@5f000000 {
> >             compatible = "simple-bus";
> >             #address-cells = <1>;
> >             #size-cells = <1>;
> >             ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >
> >             pcie@5f010000 {
> >                     compatible = "fsl,imx8q-pcie";
> >                     reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> >                     reg-names = "dbi", "config";
> >                     #address-cells = <3>;
> >                     #size-cells = <2>;
> >                     device_type = "pci";
> >                     bus-range = <0x00 0xff>;
> >                     ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> >                              <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> >             ...
> >             };
> >     };
> >
> > Device tree already can descript all address translate. Some hardware
> > driver implement fixup function by mask some bits of cpu address. Last
> > pci-imx6.c are little bit better by fetch memory resource's offset to do
> > fixup.
> >
> > static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> > {
> > 	...
> > 	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> > 	return cpu_addr - entry->offset;
> > }
> >
> > But it is not good by using IORESOURCE_MEM to fix up io/cfg address map
> > although address translate is the same as IORESOURCE_MEM.
> >
> > This patches to fetch untranslate range information for PCIe controller
> > (pcie@5f010000: ranges). So current config ATU without cpu_fixup_addr().
> >
> > == EP side:
> >
> >                    Endpoint
> >   ┌───────────────────────────────────────────────┐
> >   │                             pcie-ep@5f010000  │
> >   │                             ┌────────────────┐│
> >   │                             │   Endpoint     ││
> >   │                             │   PCIe         ││
> >   │                             │   Controller   ││
> >   │           bus@5f000000      │                ││
> >   │           ┌──────────┐      │                ││
> >   │           │          │ Outbound Transfer     ││
> >   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
> >   ││     │    │  Fabric  │Bus   │                ││PCI Addr
> >   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
> >   ││     │CPU │          │0x8000_0000            ││
> >   │└─────┘Addr└──────────┘      │                ││
> >   │       0x7000_0000           └────────────────┘│
> >   └───────────────────────────────────────────────┘
> >
> > bus@5f000000 {
> >         compatible = "simple-bus";
> >         ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> >
> >         pcie-ep@5f010000 {
> >                 reg = <0x5f010000 0x00010000>,
> >                       <0x80000000 0x10000000>;
> >                 reg-names = "dbi", "addr_space";
> >                 ...                ^^^^
> >         };
> >         ...
> > };
> >
> > Add `bus_addr_base` to configure the outbound window address for CPU write.
> > The BUS fabric generally passes the same address to the PCIe EP controller,
> > but some BUS fabrics convert the address before sending it to the PCIe EP
> > controller.
> >
> > Above diagram, CPU write data to outbound windows address 0x7000_0000,
> > Bus fabric convert it to 0x8000_0000. ATU should use BUS address
> > 0x8000_0000 as input address and convert to PCI address 0xA000_0000.
> >
> > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> > the device tree provides this information.
> >
> > The both pave the road to eliminate ugle cpu_fixup_addr() callback function.
> >
>
> Series looks good to me. Thanks a lot for your persistence! But it missed 6.13
> cycle. So let's get it merged early once 6.13-rc1 is out.

Krzysztof Wilczyński:

	Could you please pick these? all reviewed by mani? It pave the
road to clean up ugle cpu_fixup_addr().

Frank

>
> - Mani
>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Changes in v8:
> > - Add mani's review tages
> > - use rename use_dt_ranges to use_parent_dt_ranges
> > - Add dev_warn_once to reminder to fix their dt file and remove
> > cpu_fixup_addr() callback.
> > - rename dw_pcie_get_untranslate_addr() to dw_pcie_get_parent_addr()
> > - Link to v7: https://lore.kernel.org/r/20241029-pci_fixup_addr-v7-0-8310dc24fb7c@nxp.com
> >
> > Changes in v7:
> > - fix
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202410291546.kvgEWJv7-lkp@intel.com/
> > - Link to v6: https://lore.kernel.org/r/20241028-pci_fixup_addr-v6-0-ebebcd8fd4ff@nxp.com
> >
> > Changes in v6:
> > - merge RC and EP to one thread!
> > - Link to v5: https://lore.kernel.org/r/20241015-pci_fixup_addr-v5-0-ced556c85270@nxp.com
> >
> > Changes in v5:
> > - update address order in diagram patches.
> > - remove confused 0x5f00_0000 range
> > - update patch1's commit message.
> > - Link to v4: https://lore.kernel.org/r/20241008-pci_fixup_addr-v4-0-25e5200657bc@nxp.com
> >
> > Changes in v4:
> > - Improve commit message by add driver source code path.
> > - Link to v3: https://lore.kernel.org/r/20240930-pci_fixup_addr-v3-0-80ee70352fc7@nxp.com
> >
> > Changes in v3:
> > - see each patch
> > - Link to v2: https://lore.kernel.org/r/20240926-pci_fixup_addr-v2-0-e4524541edf4@nxp.com
> >
> > Changes in v2:
> > - see each patch
> > - Link to v1: https://lore.kernel.org/r/20240924-pci_fixup_addr-v1-0-57d14a91ec4f@nxp.com
> >
> > ---
> > Frank Li (7):
> >       of: address: Add parent_bus_addr to struct of_pci_range
> >       PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback
> >       PCI: dwc: ep: Add bus_addr_base for outbound window
> >       PCI: imx6: Remove cpu_addr_fixup()
> >       dt-bindings: PCI: fsl,imx6q-pcie-ep: Add compatible string fsl,imx8q-pcie-ep
> >       PCI: imx6: Pass correct sub mode when calling phy_set_mode_ext()
> >       PCI: imx6: Add i.MX8Q PCIe Endpoint (EP) support
> >
> >  .../devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml | 38 ++++++++++++++-
> >  drivers/of/address.c                               |  2 +
> >  drivers/pci/controller/dwc/pci-imx6.c              | 46 +++++++++--------
> >  drivers/pci/controller/dwc/pcie-designware-ep.c    | 18 ++++++-
> >  drivers/pci/controller/dwc/pcie-designware-host.c  | 57 +++++++++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.c       |  9 ++++
> >  drivers/pci/controller/dwc/pcie-designware.h       |  8 +++
> >  include/linux/of_address.h                         |  1 +
> >  8 files changed, 155 insertions(+), 24 deletions(-)
> > ---
> > base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> > change-id: 20240924-pci_fixup_addr-a8568f9bbb34
> >
> > Best regards,
> > ---
> > Frank Li <Frank.Li@nxp.com>
> >
>
> --
> மணிவண்ணன் சதாசிவம்

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

[Frank Li]

On Tue, Dec 10, 2024 at 05:16:24PM -0500, Frank Li wrote:
> On Sun, Nov 24, 2024 at 08:08:39PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Nov 19, 2024 at 02:44:18PM -0500, Frank Li wrote:
[...]
> > > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> > > the device tree provides this information.
> > >
> > > The both pave the road to eliminate ugle cpu_fixup_addr() callback function.
> > >
> >
> > Series looks good to me. Thanks a lot for your persistence! But it missed 6.13
> > cycle. So let's get it merged early once 6.13-rc1 is out.
>
> Krzysztof Wilczyński:
>
> 	Could you please pick these? all reviewed by mani? It pave the
> road to clean up ugle cpu_fixup_addr().

Krzysztof Wilczyński and Bjorn Helgaas

	Any update for this? All already reviewed by mani.

Frank

>
> Frank
>
> >
> > - Mani
> >
[...]
> >
> > --
> > மணிவண்ணன் சதாசிவம்

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

[Frank Li]

On Thu, Dec 19, 2024 at 02:55:08PM -0500, Frank Li wrote:
> On Tue, Dec 10, 2024 at 05:16:24PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 08:08:39PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Nov 19, 2024 at 02:44:18PM -0500, Frank Li wrote:
> [...]
> > > > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> > > > the device tree provides this information.
> > > >
> > > > The both pave the road to eliminate ugle cpu_fixup_addr() callback function.
> > > >
> > >
> > > Series looks good to me. Thanks a lot for your persistence! But it missed 6.13
> > > cycle. So let's get it merged early once 6.13-rc1 is out.
> >
> > Krzysztof Wilczyński:
> >
> > 	Could you please pick these? all reviewed by mani? It pave the
> > road to clean up ugle cpu_fixup_addr().
>
> Krzysztof Wilczyński and Bjorn Helgaas
>
> 	Any update for this? All already reviewed by mani.

Krzysztof Wilczyński and Bjorn Helgaas:

	Happy new year!, could you please take care this patches?

Frank

>
> Frank
>
> >
> > Frank
> >
> > >
> > > - Mani
> > >
> [...]
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்

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

[Frank Li]

On Mon, Jan 06, 2025 at 12:14:07PM -0500, Frank Li wrote:
> On Thu, Dec 19, 2024 at 02:55:08PM -0500, Frank Li wrote:
> > On Tue, Dec 10, 2024 at 05:16:24PM -0500, Frank Li wrote:
> > > On Sun, Nov 24, 2024 at 08:08:39PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Nov 19, 2024 at 02:44:18PM -0500, Frank Li wrote:
> > [...]
> > > > > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> > > > > the device tree provides this information.
> > > > >
> > > > > The both pave the road to eliminate ugle cpu_fixup_addr() callback function.
> > > > >
> > > >
> > > > Series looks good to me. Thanks a lot for your persistence! But it missed 6.13
> > > > cycle. So let's get it merged early once 6.13-rc1 is out.
> > >
> > > Krzysztof Wilczyński:
> > >
> > > 	Could you please pick these? all reviewed by mani? It pave the
> > > road to clean up ugle cpu_fixup_addr().
> >
> > Krzysztof Wilczyński and Bjorn Helgaas
> >
> > 	Any update for this? All already reviewed by mani.
>
> Krzysztof Wilczyński and Bjorn Helgaas:
>
> 	Happy new year!, could you please take care this patches?

Sorry, missed put bhelgaas@google.com in "TO" list.

Frank

>
> Frank
>
> >
> > Frank
> >
> > >
> > > Frank
> > >
> > > >
> > > > - Mani
> > > >
> > [...]
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்

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

[Krzysztof Wilczyński]

Hello,

> > > > Series looks good to me. Thanks a lot for your persistence! But it missed 6.13
> > > > cycle. So let's get it merged early once 6.13-rc1 is out.
> > >
> > > Krzysztof Wilczyński:
> > >
> > > 	Could you please pick these? all reviewed by mani? It pave the
> > > road to clean up ugle cpu_fixup_addr().
> >
> > Krzysztof Wilczyński and Bjorn Helgaas
> >
> > 	Any update for this? All already reviewed by mani.
> 
> Krzysztof Wilczyński and Bjorn Helgaas:
> 
> 	Happy new year!, could you please take care this patches?

Happy New Year 2025 to you, too!

I apologise for the delay, especially given you chasing few times.
  
However, we should be good to get the series out for the v6.14 release.
 
Again, sorry for the delay. 

	Krzysztof

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

[Krzysztof Wilczyński]

Hello,

> == RC side:
> 
>             ┌─────────┐                    ┌────────────┐
>  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
>  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
>  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
>   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> 0x7ff8_0000─┼───┘  │  │             │   │  │            │
>             │      │  │             │   │  │            │   PCI Addr
> 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
>             │         │             │      │            │    0
> 0x7000_0000─┼────────►├─────────┐   │      │            │
>             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
>              BUS Fabric         │          │            │    0
>                                 │          │            │
>                                 └──────────► MemSpace  ─┼────────────►
>                         IA: 0x8000_0000    │            │  0x8000_0000
>                                            └────────────┘
> 
> Current dwc implimemnt, pci_fixup_addr() call back is needed when bus
> fabric convert cpu address before send to PCIe controller.
> 
>     bus@5f000000 {
>             compatible = "simple-bus";
>             #address-cells = <1>;
>             #size-cells = <1>;
>             ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
>             pcie@5f010000 {
>                     compatible = "fsl,imx8q-pcie";
>                     reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
>                     reg-names = "dbi", "config";
>                     #address-cells = <3>;
>                     #size-cells = <2>;
>                     device_type = "pci";
>                     bus-range = <0x00 0xff>;
>                     ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
>                              <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
>             ...
>             };
>     };
> 
> Device tree already can descript all address translate. Some hardware
> driver implement fixup function by mask some bits of cpu address. Last
> pci-imx6.c are little bit better by fetch memory resource's offset to do
> fixup.
> 
> static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> {
> 	...
> 	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> 	return cpu_addr - entry->offset;
> }
> 
> But it is not good by using IORESOURCE_MEM to fix up io/cfg address map
> although address translate is the same as IORESOURCE_MEM.
> 
> This patches to fetch untranslate range information for PCIe controller
> (pcie@5f010000: ranges). So current config ATU without cpu_fixup_addr().
> 
> == EP side:
> 
>                    Endpoint
>   ┌───────────────────────────────────────────────┐
>   │                             pcie-ep@5f010000  │
>   │                             ┌────────────────┐│
>   │                             │   Endpoint     ││
>   │                             │   PCIe         ││
>   │                             │   Controller   ││
>   │           bus@5f000000      │                ││
>   │           ┌──────────┐      │                ││
>   │           │          │ Outbound Transfer     ││
>   │┌─────┐    │  Bus     ┼─────►│ ATU  ──────────┬┬─────►
>   ││     │    │  Fabric  │Bus   │                ││PCI Addr
>   ││ CPU ├───►│          │Addr  │                ││0xA000_0000
>   ││     │CPU │          │0x8000_0000            ││
>   │└─────┘Addr└──────────┘      │                ││
>   │       0x7000_0000           └────────────────┘│
>   └───────────────────────────────────────────────┘
> 
> bus@5f000000 {
>         compatible = "simple-bus";
>         ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> 
>         pcie-ep@5f010000 {
>                 reg = <0x5f010000 0x00010000>,
>                       <0x80000000 0x10000000>;
>                 reg-names = "dbi", "addr_space";
>                 ...                ^^^^
>         };
>         ...
> };
> 
> Add `bus_addr_base` to configure the outbound window address for CPU write.
> The BUS fabric generally passes the same address to the PCIe EP controller,
> but some BUS fabrics convert the address before sending it to the PCIe EP
> controller.
> 
> Above diagram, CPU write data to outbound windows address 0x7000_0000,
> Bus fabric convert it to 0x8000_0000. ATU should use BUS address
> 0x8000_0000 as input address and convert to PCI address 0xA000_0000.
> 
> Previously, `cpu_addr_fixup()` was used to handle address conversion. Now,
> the device tree provides this information.
> 
> The both pave the road to eliminate ugle cpu_fixup_addr() callback function.

Applied to controller/dwc for v6.14, thank you!

	Krzysztof