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

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
> 

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

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

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

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

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

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

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
>
> மணிவண்ணன் சதாசிவம்