[PATCH v4 0/4] PCI: ep: dwc/imx6: Add bus address support for PCI endpoint devices

[PATCH v4 0/4] PCI: ep: dwc/imx6: Add bus address support for PCI endpoint devices

[Frank Li]

Endpoint          Root complex
                             ┌───────┐        ┌─────────┐
               ┌─────┐       │ EP    │        │         │      ┌─────┐
               │     │       │ Ctrl  │        │         │      │ CPU │
               │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
               │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
               │     │       │       │        │ └────┘  │ Outbound Transfer
               └─────┘       │       │        │         │
                             │       │        │         │
                             │       │        │         │
                             │       │        │         │ Inbound Transfer
                             │       │        │         │      ┌──▼──┐
              ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
              │       │ outbound Transfer*    │ │       │      └─────┘
   ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
   │     │    │ Fabric│Bus   │       │ PCI Addr         │
   │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
   │     │CPU │       │0x8000_0000   │        │         │
   └─────┘Addr└───────┘      │       │        │         │
          0x7000_0000        └───────┘        └─────────┘

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, preferring a common method.

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

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

'ranges' in bus@5f000000 descript how address convert 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().

The 1st patch implement above method in dwc common driver.
The 2nd patch update imx6's binding doc to add fsl,imx8q-pcie-ep.
The 3rd patch fix a pci-mx6's a bug
The 4th patch enable pci ep function.

The imx8q's dts is usptreaming, the pcie-ep part is below.

hsio_subsys: bus@5f000000 {
        compatible = "simple-bus";
        #address-cells = <1>;
        #size-cells = <1>;
        /* Only supports up to 32bits DMA, map all possible DDR as inbound ranges */
        dma-ranges = <0x80000000 0 0x80000000 0x80000000>;
        ranges = <0x5f000000 0x0 0x5f000000 0x01000000>,
                 <0x80000000 0x0 0x70000000 0x10000000>;

	pcieb_ep: pcie-ep@5f010000 {
                compatible = "fsl,imx8q-pcie-ep";
                reg = <0x5f010000 0x00010000>,
                      <0x80000000 0x10000000>;
                reg-names = "dbi", "addr_space";
                num-lanes = <1>;
                interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
                interrupt-names = "dma";
                clocks = <&pcieb_lpcg IMX_LPCG_CLK_6>,
                         <&pcieb_lpcg IMX_LPCG_CLK_4>,
                         <&pcieb_lpcg IMX_LPCG_CLK_5>;
                clock-names = "dbi", "mstr", "slv";
                power-domains = <&pd IMX_SC_R_PCIE_B>;
                fsl,max-link-speed = <3>;
                num-ib-windows = <6>;
                num-ob-windows = <6>;
        };
};

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v4:
- 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/
- Link to v3: https://lore.kernel.org/r/20241021-pcie_ep_range-v3-0-b13526eb0089@nxp.com

Changes in v3:
- Add mani' review tag for patch 3,4
- Add varible using_dtbus_info to control use bus range information instead
cpu_address_fixup().
- Link to v2: https://lore.kernel.org/r/20240923-pcie_ep_range-v2-0-78d2ea434d9f@nxp.com

Changes in v2:
- Totally rewrite with difference method. 'range' should in bus node
instead pcie-ep node because address convert happen at bus fabric. Needn't
add 'range' property at pci-ep node.
- Link to v1: https://lore.kernel.org/r/20240919-pcie_ep_range-v1-0-b3e9d62780b7@nxp.com

---
Frank Li (4):
      PCI: dwc: ep: Add bus_addr_base for outbound window
      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/pci/controller/dwc/pci-imx6.c              | 26 ++++++++++++++-
 drivers/pci/controller/dwc/pcie-designware-ep.c    | 14 +++++++-
 drivers/pci/controller/dwc/pcie-designware.h       |  9 +++++
 4 files changed, 84 insertions(+), 3 deletions(-)
---
base-commit: afb15ca28055352101286046c1f9f01fdaa1ace1
change-id: 20240918-pcie_ep_range-4c5c5e300e19

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

[PATCH v4 1/4] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li]

                               Endpoint          Root complex
                             ┌───────┐        ┌─────────┐
               ┌─────┐       │ EP    │        │         │      ┌─────┐
               │     │       │ Ctrl  │        │         │      │ CPU │
               │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
               │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
               │     │       │       │        │ └────┘  │ Outbound Transfer
               └─────┘       │       │        │         │
                             │       │        │         │
                             │       │        │         │
                             │       │        │         │ Inbound Transfer
                             │       │        │         │      ┌──▼──┐
              ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
              │       │ outbound Transfer*    │ │       │      └─────┘
   ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
   │     │    │ Fabric│Bus   │       │ PCI Addr         │
   │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
   │     │CPU │       │0x8000_0000   │        │         │
   └─────┘Addr└───────┘      │       │        │         │
          0x7000_0000        └───────┘        └─────────┘

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, preferring a common method.

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

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

'ranges' in bus@5f000000 descript how address convert 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.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
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 | 14 +++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.h    |  9 +++++++++
 2 files changed, 22 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..81b4057befa62 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,16 @@ 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->using_dtbus_info) {
+		index = of_property_match_string(np, "reg-names", "addr_space");
+		if (index < 0)
+			return -EINVAL;
+
+		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 347ab74ac35aa..f10b533b04f77 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];
@@ -463,6 +464,14 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+	/*
+	 * Use device tree 'ranges' property of bus node instead using
+	 * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not
+	 * reflect real hardware's behavior. In case break these platform back
+	 * compatibility, add below flags. Set it true if dts already correct
+	 * indicate bus fabric address convert.
+	 */
+	bool			using_dtbus_info;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)

-- 
2.34.1

[PATCH v4 2/4] 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 v4
- 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 v4 3/4] 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->v4
- 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 808d1f1054173..bdc2b372e6c13 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -961,7 +961,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 v4 4/4] 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 v4
- none
change from v2 to v3
- add Mani's review tag
- Add pci->using_dtbus_info = true;
---
 drivers/pci/controller/dwc/pci-imx6.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index bdc2b372e6c13..5be9bac6206a7 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,
 };
 
@@ -1079,6 +1080,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
  * ================================================================================================
@@ -1448,6 +1459,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pci->using_dtbus_info = true;
+
 	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
 		ret = imx_add_pcie_ep(imx_pcie, pdev);
 		if (ret < 0)
@@ -1645,6 +1658,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 |
@@ -1674,6 +1695,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 v4 0/4] PCI: ep: dwc/imx6: Add bus address support for PCI endpoint devices

[Bjorn Helgaas]

On Thu, Oct 24, 2024 at 04:41:42PM -0400, Frank Li wrote:
> Endpoint          Root complex
>                              ┌───────┐        ┌─────────┐
>                ┌─────┐       │ EP    │        │         │      ┌─────┐
>                │     │       │ Ctrl  │        │         │      │ CPU │
>                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
>                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
>                │     │       │       │        │ └────┘  │ Outbound Transfer
>                └─────┘       │       │        │         │
>                              │       │        │         │
>                              │       │        │         │
>                              │       │        │         │ Inbound Transfer
>                              │       │        │         │      ┌──▼──┐
>               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
>               │       │ outbound Transfer*    │ │       │      └─────┘
>    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
>    │     │    │ Fabric│Bus   │       │ PCI Addr         │
>    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
>    │     │CPU │       │0x8000_0000   │        │         │
>    └─────┘Addr└───────┘      │       │        │         │
>           0x7000_0000        └───────┘        └─────────┘
> 
> 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.

The above doesn't match what's in patch 1/4, and I think the version
in 1/4 is better, so I'll comment there.

To avoid confusion, it might be better not to duplicate it in 0/4 and
1/4.

> 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 = <0x5f010000 0x00010000>,
> 		      <0x80000000 0x10000000>;
> 		reg-names = "dbi", "addr_space";
> 		...
> 	};
> 	...
> };
> 
> 'ranges' in bus@5f000000 descript how address convert 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().
> 
> The 1st patch implement above method in dwc common driver.
> The 2nd patch update imx6's binding doc to add fsl,imx8q-pcie-ep.
> The 3rd patch fix a pci-mx6's a bug
> The 4th patch enable pci ep function.
> 
> The imx8q's dts is usptreaming, the pcie-ep part is below.
> 
> hsio_subsys: bus@5f000000 {
>         compatible = "simple-bus";
>         #address-cells = <1>;
>         #size-cells = <1>;
>         /* Only supports up to 32bits DMA, map all possible DDR as inbound ranges */
>         dma-ranges = <0x80000000 0 0x80000000 0x80000000>;
>         ranges = <0x5f000000 0x0 0x5f000000 0x01000000>,
>                  <0x80000000 0x0 0x70000000 0x10000000>;
> 
> 	pcieb_ep: pcie-ep@5f010000 {
>                 compatible = "fsl,imx8q-pcie-ep";
>                 reg = <0x5f010000 0x00010000>,
>                       <0x80000000 0x10000000>;
>                 reg-names = "dbi", "addr_space";
>                 num-lanes = <1>;
>                 interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
>                 interrupt-names = "dma";
>                 clocks = <&pcieb_lpcg IMX_LPCG_CLK_6>,
>                          <&pcieb_lpcg IMX_LPCG_CLK_4>,
>                          <&pcieb_lpcg IMX_LPCG_CLK_5>;
>                 clock-names = "dbi", "mstr", "slv";
>                 power-domains = <&pd IMX_SC_R_PCIE_B>;
>                 fsl,max-link-speed = <3>;
>                 num-ib-windows = <6>;
>                 num-ob-windows = <6>;
>         };
> };
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Changes in v4:
> - 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/
> - Link to v3: https://lore.kernel.org/r/20241021-pcie_ep_range-v3-0-b13526eb0089@nxp.com
> 
> Changes in v3:
> - Add mani' review tag for patch 3,4
> - Add varible using_dtbus_info to control use bus range information instead
> cpu_address_fixup().
> - Link to v2: https://lore.kernel.org/r/20240923-pcie_ep_range-v2-0-78d2ea434d9f@nxp.com
> 
> Changes in v2:
> - Totally rewrite with difference method. 'range' should in bus node
> instead pcie-ep node because address convert happen at bus fabric. Needn't
> add 'range' property at pci-ep node.
> - Link to v1: https://lore.kernel.org/r/20240919-pcie_ep_range-v1-0-b3e9d62780b7@nxp.com
> 
> ---
> Frank Li (4):
>       PCI: dwc: ep: Add bus_addr_base for outbound window
>       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/pci/controller/dwc/pci-imx6.c              | 26 ++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware-ep.c    | 14 +++++++-
>  drivers/pci/controller/dwc/pcie-designware.h       |  9 +++++
>  4 files changed, 84 insertions(+), 3 deletions(-)
> ---
> base-commit: afb15ca28055352101286046c1f9f01fdaa1ace1
> change-id: 20240918-pcie_ep_range-4c5c5e300e19
> 
> Best regards,
> ---
> Frank Li <Frank.Li@nxp.com>
> 

Re: [PATCH v4 0/4] PCI: ep: dwc/imx6: Add bus address support for PCI endpoint devices

[Frank Li]

On Fri, Oct 25, 2024 at 03:48:18PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 24, 2024 at 04:41:42PM -0400, Frank Li wrote:
> > Endpoint          Root complex
> >                              ┌───────┐        ┌─────────┐
> >                ┌─────┐       │ EP    │        │         │      ┌─────┐
> >                │     │       │ Ctrl  │        │         │      │ CPU │
> >                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
> >                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
> >                │     │       │       │        │ └────┘  │ Outbound Transfer
> >                └─────┘       │       │        │         │
> >                              │       │        │         │
> >                              │       │        │         │
> >                              │       │        │         │ Inbound Transfer
> >                              │       │        │         │      ┌──▼──┐
> >               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
> >               │       │ outbound Transfer*    │ │       │      └─────┘
> >    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
> >    │     │    │ Fabric│Bus   │       │ PCI Addr         │
> >    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
> >    │     │CPU │       │0x8000_0000   │        │         │
> >    └─────┘Addr└───────┘      │       │        │         │
> >           0x7000_0000        └───────┘        └─────────┘
> >
> > 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.
>
> The above doesn't match what's in patch 1/4, and I think the version
> in 1/4 is better, so I'll comment there.
>
> To avoid confusion, it might be better not to duplicate it in 0/4 and
> 1/4.

Yes, cover letter don't come into git tree. This part is common and
important, It is not good just said ref to patch1 commit message.

Add do you have addition comment about this and
https://lore.kernel.org/imx/20241015-pci_fixup_addr-v5-0-ced556c85270@nxp.com/T/#t

The both are the pave the road to clean up pci_fixup_addr().

Frank

>
> > 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 = <0x5f010000 0x00010000>,
> > 		      <0x80000000 0x10000000>;
> > 		reg-names = "dbi", "addr_space";
> > 		...
> > 	};
> > 	...
> > };
> >
> > 'ranges' in bus@5f000000 descript how address convert 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().
> >
> > The 1st patch implement above method in dwc common driver.
> > The 2nd patch update imx6's binding doc to add fsl,imx8q-pcie-ep.
> > The 3rd patch fix a pci-mx6's a bug
> > The 4th patch enable pci ep function.
> >
> > The imx8q's dts is usptreaming, the pcie-ep part is below.
> >
> > hsio_subsys: bus@5f000000 {
> >         compatible = "simple-bus";
> >         #address-cells = <1>;
> >         #size-cells = <1>;
> >         /* Only supports up to 32bits DMA, map all possible DDR as inbound ranges */
> >         dma-ranges = <0x80000000 0 0x80000000 0x80000000>;
> >         ranges = <0x5f000000 0x0 0x5f000000 0x01000000>,
> >                  <0x80000000 0x0 0x70000000 0x10000000>;
> >
> > 	pcieb_ep: pcie-ep@5f010000 {
> >                 compatible = "fsl,imx8q-pcie-ep";
> >                 reg = <0x5f010000 0x00010000>,
> >                       <0x80000000 0x10000000>;
> >                 reg-names = "dbi", "addr_space";
> >                 num-lanes = <1>;
> >                 interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
> >                 interrupt-names = "dma";
> >                 clocks = <&pcieb_lpcg IMX_LPCG_CLK_6>,
> >                          <&pcieb_lpcg IMX_LPCG_CLK_4>,
> >                          <&pcieb_lpcg IMX_LPCG_CLK_5>;
> >                 clock-names = "dbi", "mstr", "slv";
> >                 power-domains = <&pd IMX_SC_R_PCIE_B>;
> >                 fsl,max-link-speed = <3>;
> >                 num-ib-windows = <6>;
> >                 num-ob-windows = <6>;
> >         };
> > };
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Changes in v4:
> > - 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/
> > - Link to v3: https://lore.kernel.org/r/20241021-pcie_ep_range-v3-0-b13526eb0089@nxp.com
> >
> > Changes in v3:
> > - Add mani' review tag for patch 3,4
> > - Add varible using_dtbus_info to control use bus range information instead
> > cpu_address_fixup().
> > - Link to v2: https://lore.kernel.org/r/20240923-pcie_ep_range-v2-0-78d2ea434d9f@nxp.com
> >
> > Changes in v2:
> > - Totally rewrite with difference method. 'range' should in bus node
> > instead pcie-ep node because address convert happen at bus fabric. Needn't
> > add 'range' property at pci-ep node.
> > - Link to v1: https://lore.kernel.org/r/20240919-pcie_ep_range-v1-0-b3e9d62780b7@nxp.com
> >
> > ---
> > Frank Li (4):
> >       PCI: dwc: ep: Add bus_addr_base for outbound window
> >       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/pci/controller/dwc/pci-imx6.c              | 26 ++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware-ep.c    | 14 +++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h       |  9 +++++
> >  4 files changed, 84 insertions(+), 3 deletions(-)
> > ---
> > base-commit: afb15ca28055352101286046c1f9f01fdaa1ace1
> > change-id: 20240918-pcie_ep_range-4c5c5e300e19
> >
> > Best regards,
> > ---
> > Frank Li <Frank.Li@nxp.com>
> >

Re: [PATCH v4 0/4] PCI: ep: dwc/imx6: Add bus address support for PCI endpoint devices

[Bjorn Helgaas]

On Fri, Oct 25, 2024 at 05:05:03PM -0400, Frank Li wrote:
> On Fri, Oct 25, 2024 at 03:48:18PM -0500, Bjorn Helgaas wrote:
> > On Thu, Oct 24, 2024 at 04:41:42PM -0400, Frank Li wrote:
> > > Endpoint          Root complex
> > >                              ┌───────┐        ┌─────────┐
> > >                ┌─────┐       │ EP    │        │         │      ┌─────┐
> > >                │     │       │ Ctrl  │        │         │      │ CPU │
> > >                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
> > >                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
> > >                │     │       │       │        │ └────┘  │ Outbound Transfer
> > >                └─────┘       │       │        │         │
> > >                              │       │        │         │
> > >                              │       │        │         │
> > >                              │       │        │         │ Inbound Transfer
> > >                              │       │        │         │      ┌──▼──┐
> > >               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
> > >               │       │ outbound Transfer*    │ │       │      └─────┘
> > >    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
> > >    │     │    │ Fabric│Bus   │       │ PCI Addr         │
> > >    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
> > >    │     │CPU │       │0x8000_0000   │        │         │
> > >    └─────┘Addr└───────┘      │       │        │         │
> > >           0x7000_0000        └───────┘        └─────────┘
> > >
> > > 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.
> >
> > The above doesn't match what's in patch 1/4, and I think the version
> > in 1/4 is better, so I'll comment there.
> >
> > To avoid confusion, it might be better not to duplicate it in 0/4 and
> > 1/4.
> 
> Yes, cover letter don't come into git tree. This part is common and
> important, It is not good just said ref to patch1 commit message.
> 
> Add do you have addition comment about this and
> https://lore.kernel.org/imx/20241015-pci_fixup_addr-v5-0-ced556c85270@nxp.com/T/#t
> 
> The both are the pave the road to clean up pci_fixup_addr().

I think it would be helpful to combine the "PCI: dwc: optimize RC host
pci_fixup_addr()" series and the "bus_addr_base" parts of this series
together into a single series because they are doing very similar
things, and it's easier to review them together.

And split the dt-bindings, PHY sub mode, and new endpoint support
parts to their own series because they're not related to the address
translation changes.

Bjorn

Re: [PATCH v4 1/4] PCI: dwc: ep: Add bus_addr_base for outbound window

[Bjorn Helgaas]

On Thu, Oct 24, 2024 at 04:41:43PM -0400, Frank Li wrote:
>                                Endpoint          Root complex
>                              ┌───────┐        ┌─────────┐
>                ┌─────┐       │ EP    │        │         │      ┌─────┐
>                │     │       │ Ctrl  │        │         │      │ CPU │
>                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
>                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
>                │     │       │       │        │ └────┘  │ Outbound Transfer
>                └─────┘       │       │        │         │
>                              │       │        │         │
>                              │       │        │         │
>                              │       │        │         │ Inbound Transfer
>                              │       │        │         │      ┌──▼──┐
>               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
>               │       │ outbound Transfer*    │ │       │      └─────┘
>    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
>    │     │    │ Fabric│Bus   │       │ PCI Addr         │
>    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
>    │     │CPU │       │0x8000_0000   │        │         │
>    └─────┘Addr└───────┘      │       │        │         │
>           0x7000_0000        └───────┘        └─────────┘
> 
> 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.

Thanks for the diagram and description.  I don't think the top half is
relevant to *this* patch, is it?  I think this patch is only concerned
with the address translations between the CPU in the endpoint and the
PCI bus address.  In this case it happens in two steps: the bus fabric
applies one offset, and the ATU applies a second offset.

Unless the top half is relevant, I would omit it and simply use
something like this:

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

If you don't want a big "Endpoint" box including the CPU and bus
fabric, that's OK with me, too.  I added it because everything on the
PCI side only sees TLPs that contain PCI bus addresses, and can't tell
anything about the internal implementation of the Endpoint.

> 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 = <0x5f010000 0x00010000>,
> 		      <0x80000000 0x10000000>;
> 		reg-names = "dbi", "addr_space";
> 		...
> 	};
> 	...
> };

I guess bus@5f000000 includes a "ranges" property because that
translation from 0x7000_0000 -> 0x8000_0000 is fixed or at least
not touched by Linux?

And the pcie-ep@5f010000 address translation from 0x8000_0000 to
0xA000_0000 *is* programmed by Linux and therefore can't be described
by a DT?  But I guess Linux only programs the *PCI* side, and the
parent side (0x8000_0000) is fixed?

AFAICT, the "reg = <0x5f010000 0x00010000>" part is not relevant here.

I guess this implementation assumes there's only a single aperture
through the Bus Fabric, right?

And also a single ATU aperture through the endpoint PCIe controller?

And also that there's only one layer of Bus Fabric address
translation?  The fact that only a few DWC controllers have this
translation suggests that this part of the picture might be external
to the DWC IP and there could be more variation.  But I guess there's
no point in adding code for topologies that don't exist; we can deal
with that if the need ever arises.

> 'ranges' in bus@5f000000 descript how address convert 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.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 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 | 14 +++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h    |  9 +++++++++
>  2 files changed, 22 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..81b4057befa62 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;

Tangent: Maybe dw_pcie_ob_atu_cfg.cpu_addr isn't exactly the right
name, since it now contains an address that is not a CPU physical
address.  Not a question for *this* patch though.

>  	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,16 @@ 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->using_dtbus_info) {
> +		index = of_property_match_string(np, "reg-names", "addr_space");
> +		if (index < 0)
> +			return -EINVAL;
> +
> +		of_property_read_reg(np, index, &ep->bus_addr_base, NULL);
> +	}

If this translation were fixed, I suppose we'd extract something from
a "ranges" property that contains (child-bus-address,
parent-bus-address) information.  So I suppose "addr_space" contains a
fixed parent-bus-address, and is setting the child (PCI) bus address,
right?

If so, I might add a comment here for other readers who come this way.
(And me, because I won't remember the next time I read it :)

>  	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 347ab74ac35aa..f10b533b04f77 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];
> @@ -463,6 +464,14 @@ struct dw_pcie {
>  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
>  	struct gpio_desc		*pe_rst;
>  	bool			suspended;
> +	/*
> +	 * Use device tree 'ranges' property of bus node instead using
> +	 * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not
> +	 * reflect real hardware's behavior. In case break these platform back
> +	 * compatibility, add below flags. Set it true if dts already correct
> +	 * indicate bus fabric address convert.
> +	 */
> +	bool			using_dtbus_info;
>  };
>  
>  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 4/4] PCI: imx6: Add i.MX8Q PCIe Endpoint (EP) support

[Bjorn Helgaas]

On Thu, Oct 24, 2024 at 04:41:46PM -0400, Frank Li wrote:
> 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 v4
> - none
> change from v2 to v3
> - add Mani's review tag
> - Add pci->using_dtbus_info = true;
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index bdc2b372e6c13..5be9bac6206a7 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,
>  };
>  
> @@ -1079,6 +1080,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
>   * ================================================================================================
> @@ -1448,6 +1459,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	pci->using_dtbus_info = true;

I mentioned this elsewhere, but I think the using_dtbus_info part
should be part of a series that only deals with the address
translation, and adding IMX8Q_EP should be in a separate series.

>  	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
>  		ret = imx_add_pcie_ep(imx_pcie, pdev);
>  		if (ret < 0)
> @@ -1645,6 +1658,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 |
> @@ -1674,6 +1695,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 v4 0/4] PCI: ep: dwc/imx6: Add bus address support for PCI endpoint devices

[Frank Li]

On Fri, Oct 25, 2024 at 04:36:26PM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 25, 2024 at 05:05:03PM -0400, Frank Li wrote:
> > On Fri, Oct 25, 2024 at 03:48:18PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Oct 24, 2024 at 04:41:42PM -0400, Frank Li wrote:
> > > > Endpoint          Root complex
> > > >                              ┌───────┐        ┌─────────┐
> > > >                ┌─────┐       │ EP    │        │         │      ┌─────┐
> > > >                │     │       │ Ctrl  │        │         │      │ CPU │
> > > >                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
> > > >                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
> > > >                │     │       │       │        │ └────┘  │ Outbound Transfer
> > > >                └─────┘       │       │        │         │
> > > >                              │       │        │         │
> > > >                              │       │        │         │
> > > >                              │       │        │         │ Inbound Transfer
> > > >                              │       │        │         │      ┌──▼──┐
> > > >               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
> > > >               │       │ outbound Transfer*    │ │       │      └─────┘
> > > >    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
> > > >    │     │    │ Fabric│Bus   │       │ PCI Addr         │
> > > >    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
> > > >    │     │CPU │       │0x8000_0000   │        │         │
> > > >    └─────┘Addr└───────┘      │       │        │         │
> > > >           0x7000_0000        └───────┘        └─────────┘
> > > >
> > > > 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.
> > >
> > > The above doesn't match what's in patch 1/4, and I think the version
> > > in 1/4 is better, so I'll comment there.
> > >
> > > To avoid confusion, it might be better not to duplicate it in 0/4 and
> > > 1/4.
> >
> > Yes, cover letter don't come into git tree. This part is common and
> > important, It is not good just said ref to patch1 commit message.
> >
> > Add do you have addition comment about this and
> > https://lore.kernel.org/imx/20241015-pci_fixup_addr-v5-0-ced556c85270@nxp.com/T/#t
> >
> > The both are the pave the road to clean up pci_fixup_addr().
>
> I think it would be helpful to combine the "PCI: dwc: optimize RC host
> pci_fixup_addr()" series and the "bus_addr_base" parts of this series
> together into a single series because they are doing very similar
> things, and it's easier to review them together.
>
> And split the dt-bindings, PHY sub mode, and new endpoint support
> parts to their own series because they're not related to the address
> translation changes.

new endpoint support depend on patch 1. let me merge two thread together
to review easily.

Frank

>
> Bjorn

Re: [PATCH v4 1/4] PCI: dwc: ep: Add bus_addr_base for outbound window

[Frank Li]

On Fri, Oct 25, 2024 at 05:31:02PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 24, 2024 at 04:41:43PM -0400, Frank Li wrote:
> >                                Endpoint          Root complex
> >                              ┌───────┐        ┌─────────┐
> >                ┌─────┐       │ EP    │        │         │      ┌─────┐
> >                │     │       │ Ctrl  │        │         │      │ CPU │
> >                │ DDR │       │       │        │ ┌────┐  │      └──┬──┘
> >                │     │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘
> >                │     │       │       │        │ └────┘  │ Outbound Transfer
> >                └─────┘       │       │        │         │
> >                              │       │        │         │
> >                              │       │        │         │
> >                              │       │        │         │ Inbound Transfer
> >                              │       │        │         │      ┌──▼──┐
> >               ┌───────┐      │       │        │ ┌───────┼─────►│DDR  │
> >               │       │ outbound Transfer*    │ │       │      └─────┘
> >    ┌─────┐    │ Bus   ┼─────►│ ATU  ─┬────────┼─┘       │
> >    │     │    │ Fabric│Bus   │       │ PCI Addr         │
> >    │ CPU ├───►│       │Addr  │       │ 0xA000_0000      │
> >    │     │CPU │       │0x8000_0000   │        │         │
> >    └─────┘Addr└───────┘      │       │        │         │
> >           0x7000_0000        └───────┘        └─────────┘
> >
> > 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.
>
> Thanks for the diagram and description.  I don't think the top half is
> relevant to *this* patch, is it?  I think this patch is only concerned
> with the address translations between the CPU in the endpoint and the
> PCI bus address.  In this case it happens in two steps: the bus fabric
> applies one offset, and the ATU applies a second offset.
>
> Unless the top half is relevant, I would omit it and simply use
> something like this:
>
>                    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           └────────────────┘│
>   └───────────────────────────────────────────────┘
>
> If you don't want a big "Endpoint" box including the CPU and bus
> fabric, that's OK with me, too.  I added it because everything on the
> PCI side only sees TLPs that contain PCI bus addresses, and can't tell
> anything about the internal implementation of the Endpoint.
>
> > 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 = <0x5f010000 0x00010000>,
> > 		      <0x80000000 0x10000000>;
> > 		reg-names = "dbi", "addr_space";
> > 		...
> > 	};
> > 	...
> > };
>
> I guess bus@5f000000 includes a "ranges" property because that
> translation from 0x7000_0000 -> 0x8000_0000 is fixed or at least
> not touched by Linux?

Yes, it is fixed by hardware.

>
> And the pcie-ep@5f010000 address translation from 0x8000_0000 to
> 0xA000_0000 *is* programmed by Linux and therefore can't be described
> by a DT?  But I guess Linux only programs the *PCI* side, and the
> parent side (0x8000_0000) is fixed?

0x8000_0000 -> 0xA000_0000 is programmed by Linux RC side host driver tell
Linux EP side driver how to map it. 0x8000_0000 is fixed MDIO space.

>
> AFAICT, the "reg = <0x5f010000 0x00010000>" part is not relevant here.

Yes.

>
> I guess this implementation assumes there's only a single aperture
> through the Bus Fabric, right?
>
> And also a single ATU aperture through the endpoint PCIe controller?
>

There are 8 ATU provide by DWC PCI controller. Bus only provide a big
PCI map windows for example, 0x8000_0000..0x9000_0000.

we can slip it up to 8 small region, such as each 64K,  each region can
map to any PCI address by ATU.

0x8001_0000 -> 0xA000_00000
0x8002_0000 -> 0xB100_00000
0x8003_0000 -> 0xD000_00000
...

This part EPC driver already handle it. EPC driver only need know
"addr_space" information by above sample dts.

> And also that there's only one layer of Bus Fabric address
> translation?

Doesn't metter, but PCI controller only care about closest one. Others
translation is tranparent to drivers. As above example, PCI EP controller
only care about input address is 0x8000_0000,  and cpu send out address is
0x7000_0000. Even there are more translation,
0x7000_0000-> 0x9000_0000->0x8000_0000,
  ^^^^^^                   ^^^^

PCI EP controller only care input (0x7000_0000) and output (0x9000_0000),
don't care any internal translation (0x9000_0000).


> The fact that only a few DWC controllers have this
> translation suggests that this part of the picture might be external
> to the DWC IP and there could be more variation.  But I guess there's
> no point in adding code for topologies that don't exist; we can deal
> with that if the need ever arises.

Some cdns also have the same situation.

>
> > 'ranges' in bus@5f000000 descript how address convert 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.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 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 | 14 +++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h    |  9 +++++++++
> >  2 files changed, 22 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..81b4057befa62 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;
>
> Tangent: Maybe dw_pcie_ob_atu_cfg.cpu_addr isn't exactly the right
> name, since it now contains an address that is not a CPU physical
> address.  Not a question for *this* patch though.

yes, cpu_addr is not good name altough it is correct for most system.

>
> >  	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,16 @@ 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->using_dtbus_info) {
> > +		index = of_property_match_string(np, "reg-names", "addr_space");
> > +		if (index < 0)
> > +			return -EINVAL;
> > +
> > +		of_property_read_reg(np, index, &ep->bus_addr_base, NULL);
> > +	}
>
> If this translation were fixed, I suppose we'd extract something from
> a "ranges" property that contains (child-bus-address,
> parent-bus-address) information.

Yes, see below
	ranges = <0x80000000 0x0 0x70000000 0x10000000>;

> So I suppose "addr_space" contains a
> fixed parent-bus-address, and is setting the child (PCI) bus address,
> right?

"addr_space" hold PCI EP outbound MDIO space, which is parent-bus-address.
it is confused if called as PCI bus address, which most likely the address
after ATU covert.

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

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

History reasion, PCI EP use reg-names "addr_space" indicate outbound
windows informaiton.

>
> If so, I might add a comment here for other readers who come this way.
> (And me, because I won't remember the next time I read it :)
>
> >  	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 347ab74ac35aa..f10b533b04f77 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];
> > @@ -463,6 +464,14 @@ struct dw_pcie {
> >  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
> >  	struct gpio_desc		*pe_rst;
> >  	bool			suspended;
> > +	/*
> > +	 * Use device tree 'ranges' property of bus node instead using
> > +	 * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not
> > +	 * reflect real hardware's behavior. In case break these platform back
> > +	 * compatibility, add below flags. Set it true if dts already correct
> > +	 * indicate bus fabric address convert.
> > +	 */
> > +	bool			using_dtbus_info;
> >  };
> >
> >  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> >
> > --
> > 2.34.1
> >