[PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[Frank Li]

This patches basic on
https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-0-3c4bb506f665@nxp.com/

I have not hardware to test.

Look for driver owner, who help test this and start move forward to remove
cpu_addr_fixup() work.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (2):
      ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes
      PCI: dra7xx: Use use_parent_dt_ranges and clean up dra7xx_pcie_cpu_addr_fixup()

 arch/arm/boot/dts/ti/omap/dra7.dtsi     | 18 +++++++++---------
 drivers/pci/controller/dwc/pci-dra7xx.c |  8 +-------
 2 files changed, 10 insertions(+), 16 deletions(-)
---
base-commit: 1552be4855dacca5ea39b15b1ef0b96c91dbea0d
change-id: 20250305-dra-a7fea1c035bd

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

[PATCH PATCH RFC NOT TESTED 1/2] ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes

[Frank Li]

According to code in drivers/pci/controller/dwc/pci-dra7xx.c

dra7xx_pcie_cpu_addr_fixup()
{
	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;  //0x0FFFFFFF
}

PCI parent bus trim high 4 bits address to 0. Correct ranges in
target-module@51000000 to algin hardware behavior, which translate PCIe
outbound address 0..0x0fff_ffff to 0x2000_0000..0x2fff_ffff.

Set 'config' and 'addr_space' reg values to 0.
Change parent bus address of downstream I/O and non-prefetchable memory to
0.

Ensure no functional impact on the final address translation result.

Prepare for the removal of the driver’s cpu_addr_fixup().

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm/boot/dts/ti/omap/dra7.dtsi | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
index b709703f6c0d4..9213fdd25330b 100644
--- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
+++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
@@ -196,7 +196,7 @@ axi0: target-module@51000000 {
 			#size-cells = <1>;
 			#address-cells = <1>;
 			ranges = <0x51000000 0x51000000 0x3000>,
-				 <0x20000000 0x20000000 0x10000000>;
+				 <0x00000000 0x20000000 0x10000000>;
 			dma-ranges;
 			/**
 			 * To enable PCI endpoint mode, disable the pcie1_rc
@@ -205,14 +205,14 @@ axi0: target-module@51000000 {
 			pcie1_rc: pcie@51000000 {
 				reg = <0x51000000 0x2000>,
 				      <0x51002000 0x14c>,
-				      <0x20001000 0x2000>;
+				      <0x00001000 0x2000>;
 				reg-names = "rc_dbics", "ti_conf", "config";
 				interrupts = <0 232 0x4>, <0 233 0x4>;
 				#address-cells = <3>;
 				#size-cells = <2>;
 				device_type = "pci";
-				ranges = <0x81000000 0 0x00000000 0x20003000 0 0x00010000>,
-					 <0x82000000 0 0x20013000 0x20013000 0 0x0ffed000>;
+				ranges = <0x81000000 0 0x00000000 0x00003000 0 0x00010000>,
+					 <0x82000000 0 0x20013000 0x00013000 0 0x0ffed000>;
 				bus-range = <0x00 0xff>;
 				#interrupt-cells = <1>;
 				num-lanes = <1>;
@@ -238,7 +238,7 @@ pcie1_ep: pcie_ep@51000000 {
 				reg = <0x51000000 0x28>,
 				      <0x51002000 0x14c>,
 				      <0x51001000 0x28>,
-				      <0x20001000 0x10000000>;
+				      <0x00001000 0x10000000>;
 				reg-names = "ep_dbics", "ti_conf", "ep_dbics2", "addr_space";
 				interrupts = <0 232 0x4>;
 				num-lanes = <1>;
@@ -270,20 +270,20 @@ axi1: target-module@51800000 {
 			#size-cells = <1>;
 			#address-cells = <1>;
 			ranges = <0x51800000 0x51800000 0x3000>,
-				 <0x30000000 0x30000000 0x10000000>;
+				 <0x00000000 0x30000000 0x10000000>;
 			dma-ranges;
 			status = "disabled";
 			pcie2_rc: pcie@51800000 {
 				reg = <0x51800000 0x2000>,
 				      <0x51802000 0x14c>,
-				      <0x30001000 0x2000>;
+				      <0x00001000 0x2000>;
 				reg-names = "rc_dbics", "ti_conf", "config";
 				interrupts = <0 355 0x4>, <0 356 0x4>;
 				#address-cells = <3>;
 				#size-cells = <2>;
 				device_type = "pci";
-				ranges = <0x81000000 0 0x00000000 0x30003000 0 0x00010000>,
-					 <0x82000000 0 0x30013000 0x30013000 0 0x0ffed000>;
+				ranges = <0x81000000 0 0x00000000 0x00003000 0 0x00010000>,
+					 <0x82000000 0 0x30013000 0x00013000 0 0x0ffed000>;
 				bus-range = <0x00 0xff>;
 				#interrupt-cells = <1>;
 				num-lanes = <1>;

-- 
2.34.1

[PATCH PATCH RFC NOT TESTED 2/2] PCI: dra7xx: Use use_parent_dt_ranges and clean up dra7xx_pcie_cpu_addr_fixup()

[Frank Li]

Remove dra7xx_pcie_cpu_addr_fixup() as the DT bus fabric should provide correct
address translation. Set use_parent_dt_ranges to allow the DWC core driver to
fetch address translation from the device tree

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-dra7xx.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index 33d6bf460ffe5..d6e0bf67a07b3 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -72,7 +72,6 @@
 
 #define	PCIECTRL_DRA7XX_CONF_PHY_CS			0x010C
 #define	LINK_UP						BIT(16)
-#define	DRA7XX_CPU_TO_BUS_ADDR				0x0FFFFFFF
 
 #define	PCIECTRL_TI_CONF_INTX_ASSERT			0x0124
 #define	PCIECTRL_TI_CONF_INTX_DEASSERT			0x0128
@@ -113,11 +112,6 @@ static inline void dra7xx_pcie_writel(struct dra7xx_pcie *pcie, u32 offset,
 	writel(value, pcie->base + offset);
 }
 
-static u64 dra7xx_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
-{
-	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;
-}
-
 static int dra7xx_pcie_link_up(struct dw_pcie *pci)
 {
 	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
@@ -514,7 +508,6 @@ static int dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
 }
 
 static const struct dw_pcie_ops dw_pcie_ops = {
-	.cpu_addr_fixup = dra7xx_pcie_cpu_addr_fixup,
 	.start_link = dra7xx_pcie_establish_link,
 	.stop_link = dra7xx_pcie_stop_link,
 	.link_up = dra7xx_pcie_link_up,
@@ -712,6 +705,7 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
 
 	pci->dev = dev;
 	pci->ops = &dw_pcie_ops;
+	pci->use_parent_dt_ranges = true;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)

-- 
2.34.1

Re: [PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[Manivannan Sadhasivam]

On Wed, Mar 05, 2025 at 11:20:21AM -0500, Frank Li wrote:
> This patches basic on
> https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-0-3c4bb506f665@nxp.com/
> 
> I have not hardware to test.
> 
> Look for driver owner, who help test this and start move forward to remove
> cpu_addr_fixup() work.
> 

If you remove cpu_addr_fixup() callback, it will break backwards compatibility
with old DTs.

You should fix the existing DTs and continue carrying the callback for a while.

- Mani

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

Re: [PATCH PATCH RFC NOT TESTED 1/2] ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes

[Manivannan Sadhasivam]

On Wed, Mar 05, 2025 at 11:20:22AM -0500, Frank Li wrote:

If you want a specific patch to be tested, you can add [PATCH RFT] tag.C

> According to code in drivers/pci/controller/dwc/pci-dra7xx.c
> 
> dra7xx_pcie_cpu_addr_fixup()
> {
> 	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;  //0x0FFFFFFF
> }
> 
> PCI parent bus trim high 4 bits address to 0. Correct ranges in
> target-module@51000000 to algin hardware behavior, which translate PCIe
> outbound address 0..0x0fff_ffff to 0x2000_0000..0x2fff_ffff.
> 
> Set 'config' and 'addr_space' reg values to 0.
> Change parent bus address of downstream I/O and non-prefetchable memory to
> 0.
> 
> Ensure no functional impact on the final address translation result.
> 
> Prepare for the removal of the driver’s cpu_addr_fixup().
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  arch/arm/boot/dts/ti/omap/dra7.dtsi | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> index b709703f6c0d4..9213fdd25330b 100644
> --- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> @@ -196,7 +196,7 @@ axi0: target-module@51000000 {
>  			#size-cells = <1>;
>  			#address-cells = <1>;
>  			ranges = <0x51000000 0x51000000 0x3000>,
> -				 <0x20000000 0x20000000 0x10000000>;
> +				 <0x00000000 0x20000000 0x10000000>;

I'm not able to interpret this properly. So this essentially means that the
parent address 0x20000000 is mapped to child address 0x00000000. And the child
address is same for other controller as well.

Also, the cpu_addr_fixup() is doing the same by masking out the upper 4 bits. I
tried looking into the DRA7 TRM, but it says (ECAM_Param_Base_Addr +
0x20000000) where ECAM_Param_Base_Addr = 0x0000_0000 to 0x0FFF_F000.

I couldn't relate TRM with the cpu_addr_fixup() callback. Can someone from TI
shed light on this?

- Mani

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

Re: [PATCH PATCH RFC NOT TESTED 1/2] ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes

[Siddharth Vadapalli]

On Thu, Mar 13, 2025 at 10:23:11PM +0530, Manivannan Sadhasivam wrote:

Hello Mani,

> On Wed, Mar 05, 2025 at 11:20:22AM -0500, Frank Li wrote:
> 
> If you want a specific patch to be tested, you can add [PATCH RFT] tag.C
> 
> > According to code in drivers/pci/controller/dwc/pci-dra7xx.c
> > 
> > dra7xx_pcie_cpu_addr_fixup()
> > {
> > 	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;  //0x0FFFFFFF
> > }
> > 
> > PCI parent bus trim high 4 bits address to 0. Correct ranges in
> > target-module@51000000 to algin hardware behavior, which translate PCIe
> > outbound address 0..0x0fff_ffff to 0x2000_0000..0x2fff_ffff.
> > 
> > Set 'config' and 'addr_space' reg values to 0.
> > Change parent bus address of downstream I/O and non-prefetchable memory to
> > 0.
> > 
> > Ensure no functional impact on the final address translation result.
> > 
> > Prepare for the removal of the driver’s cpu_addr_fixup().
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  arch/arm/boot/dts/ti/omap/dra7.dtsi | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > index b709703f6c0d4..9213fdd25330b 100644
> > --- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > +++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > @@ -196,7 +196,7 @@ axi0: target-module@51000000 {
> >  			#size-cells = <1>;
> >  			#address-cells = <1>;
> >  			ranges = <0x51000000 0x51000000 0x3000>,
> > -				 <0x20000000 0x20000000 0x10000000>;
> > +				 <0x00000000 0x20000000 0x10000000>;
> 
> I'm not able to interpret this properly. So this essentially means that the
> parent address 0x20000000 is mapped to child address 0x00000000. And the child
> address is same for other controller as well.
> 
> Also, the cpu_addr_fixup() is doing the same by masking out the upper 4 bits. I
> tried looking into the DRA7 TRM, but it says (ECAM_Param_Base_Addr +
> 0x20000000) where ECAM_Param_Base_Addr = 0x0000_0000 to 0x0FFF_F000.
> 
> I couldn't relate TRM with the cpu_addr_fixup() callback. Can someone from TI
> shed light on this?

A "git blame" on the line being modified in dra7.dtsi gives the
following commit:
https://github.com/torvalds/linux/commit/c761028ef5e2
prior to which the ranges is exactly the same as the one being added by
this patch.

The cpu_addr_fixup() function was introduced by the following commit:
https://github.com/torvalds/linux/commit/2ed6cc71e6f7
with the reason described in
Section 24.9.4.3.2 PCIe Controller Slave Port
of the T.R.M. at:
https://www.ti.com/lit/ug/spruic2d/spruic2d.pdf
---------------------------------------------------------------------------
NOTE:
The PCIe controller remains fully functional, and able to send transactions
to, for example, anywhere within the 64-bit PCIe memory space, with the
appropriate remapping of the 28-bit address by the outbound address
translation unit (iATU). The limitation is that the total size of addressed
PCIe regions (in config, memory, IO spaces) must be less than 2^28 bytes.
---------------------------------------------------------------------------

The entire sequence is:
0) dra7.dtsi had ranges which match the ranges in the current patch.
1) cpu_addr_fixup() was added by
https://github.com/torvalds/linux/commit/2ed6cc71e6f7
2) ranges was updated to <0x20000000 0x20000000 0x10000000> by:
https://github.com/torvalds/linux/commit/c761028ef5e2
3) ranges is being changed back to its original state of "0)" above.

cpu_addr_fixup() was introduced to remove the following:
	pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
	pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
	pp->cfg0_base &= DRA7XX_CPU_TO_BUS_ADDR;
	pp->cfg1_base &= DRA7XX_CPU_TO_BUS_ADDR;
in dra7xx_pcie_host_init(). The reason for the above is mentioned in the
"NOTE" as:
---------------------------------------------------------------------------
The limitation is that the total size of addressed PCIe regions
(in config, memory, IO spaces) must be less than 2^28 bytes.
---------------------------------------------------------------------------

I am not sure if Frank is accounting for all of this in the current patch
as well as the dependent patch series associated with removing
cpu_addr_fixup().

Regarding testing the series, I unfortunately don't have the hardware so
I cannot test it.

Regards,
Siddharth.

Re: [PATCH PATCH RFC NOT TESTED 1/2] ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes

[Frank Li]

On Fri, Mar 14, 2025 at 12:16:42PM +0530, Siddharth Vadapalli wrote:
> On Thu, Mar 13, 2025 at 10:23:11PM +0530, Manivannan Sadhasivam wrote:
>
> Hello Mani,
>
> > On Wed, Mar 05, 2025 at 11:20:22AM -0500, Frank Li wrote:
> >
> > If you want a specific patch to be tested, you can add [PATCH RFT] tag.C
> >
> > > According to code in drivers/pci/controller/dwc/pci-dra7xx.c
> > >
> > > dra7xx_pcie_cpu_addr_fixup()
> > > {
> > > 	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;  //0x0FFFFFFF
> > > }
> > >
> > > PCI parent bus trim high 4 bits address to 0. Correct ranges in
> > > target-module@51000000 to algin hardware behavior, which translate PCIe
> > > outbound address 0..0x0fff_ffff to 0x2000_0000..0x2fff_ffff.
> > >
> > > Set 'config' and 'addr_space' reg values to 0.
> > > Change parent bus address of downstream I/O and non-prefetchable memory to
> > > 0.
> > >
> > > Ensure no functional impact on the final address translation result.
> > >
> > > Prepare for the removal of the driver’s cpu_addr_fixup().
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  arch/arm/boot/dts/ti/omap/dra7.dtsi | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > index b709703f6c0d4..9213fdd25330b 100644
> > > --- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > +++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > @@ -196,7 +196,7 @@ axi0: target-module@51000000 {
> > >  			#size-cells = <1>;
> > >  			#address-cells = <1>;
> > >  			ranges = <0x51000000 0x51000000 0x3000>,
> > > -				 <0x20000000 0x20000000 0x10000000>;
> > > +				 <0x00000000 0x20000000 0x10000000>;
> >
> > I'm not able to interpret this properly. So this essentially means that the
> > parent address 0x20000000 is mapped to child address 0x00000000. And the child
> > address is same for other controller as well.
> >
> > Also, the cpu_addr_fixup() is doing the same by masking out the upper 4 bits. I
> > tried looking into the DRA7 TRM, but it says (ECAM_Param_Base_Addr +
> > 0x20000000) where ECAM_Param_Base_Addr = 0x0000_0000 to 0x0FFF_F000.
> >
> > I couldn't relate TRM with the cpu_addr_fixup() callback. Can someone from TI
> > shed light on this?
>
> A "git blame" on the line being modified in dra7.dtsi gives the
> following commit:
> https://github.com/torvalds/linux/commit/c761028ef5e2
> prior to which the ranges is exactly the same as the one being added by
> this patch.

Okay, original one correct reflect hardware behavior.

>
> The cpu_addr_fixup() function was introduced by the following commit:
> https://github.com/torvalds/linux/commit/2ed6cc71e6f7
> with the reason described in
> Section 24.9.4.3.2 PCIe Controller Slave Port
> of the T.R.M. at:
> https://www.ti.com/lit/ug/spruic2d/spruic2d.pdf
> ---------------------------------------------------------------------------
> NOTE:
> The PCIe controller remains fully functional, and able to send transactions
> to, for example, anywhere within the 64-bit PCIe memory space, with the
> appropriate remapping of the 28-bit address by the outbound address
> translation unit (iATU). The limitation is that the total size of addressed
> PCIe regions (in config, memory, IO spaces) must be less than 2^28 bytes.
> ---------------------------------------------------------------------------
>
> The entire sequence is:
> 0) dra7.dtsi had ranges which match the ranges in the current patch.
> 1) cpu_addr_fixup() was added by
> https://github.com/torvalds/linux/commit/2ed6cc71e6f7
> 2) ranges was updated to <0x20000000 0x20000000 0x10000000> by:
> https://github.com/torvalds/linux/commit/c761028ef5e2

Actually this patch is not necessary, with cpu_addr_fixup(), it should
work with/without c761028ef5e2 change because it just the use CPU physical
address, the finial result is exact same.

> 3) ranges is being changed back to its original state of "0)" above.
>
> cpu_addr_fixup() was introduced to remove the following:
> 	pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
> 	pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
> 	pp->cfg0_base &= DRA7XX_CPU_TO_BUS_ADDR;
> 	pp->cfg1_base &= DRA7XX_CPU_TO_BUS_ADDR;
> in dra7xx_pcie_host_init(). The reason for the above is mentioned in the
> "NOTE" as:
> ---------------------------------------------------------------------------
> The limitation is that the total size of addressed PCIe regions
> (in config, memory, IO spaces) must be less than 2^28 bytes.
> ---------------------------------------------------------------------------
>

That is functional equal.

> I am not sure if Frank is accounting for all of this in the current patch
> as well as the dependent patch series associated with removing
> cpu_addr_fixup().

I have not track back the history. I think before
https://github.com/torvalds/linux/commit/c761028ef5e2 is correct reflect
hardware behavor. axi@0 trim down 4 bits before send to PCI controller.

The commit message of c761028ef5e2

"In order to update pcie to probe with ti-sysc and genpd, let's update the
pcie ranges to not use address 0 for 0x20000000 and 0x30000000. The range
for 0 is typically used for child devices as the offset from the module
base. In the following patches, we will update pcie to probe with ti-sysc,
and the patches become a bit confusing to read compared to other similar
modules unless we update the ranges first. So let's just use the full
addresses for ranges for the 0x20000000 and 0x30000000 ranges."

I think maybe only ti's bus fabric do address translation at that time, and
DT team and dwc pci driver never consider that before. Now more vendor bus
fabric do address translation. So needn't every platform driver consider it
but it require DTB reflect hardware behavor correctly.

We may revert patch c761028ef5e2 firstly, after some time later, we can
cleanup cpu_addr_fixup().

It will be wondful, if someone help test it.

Frank
>
> Regarding testing the series, I unfortunately don't have the hardware so
> I cannot test it.
>
> Regards,
> Siddharth.

Re: [PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[Bjorn Helgaas]

On Thu, Mar 13, 2025 at 11:35:21AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 05, 2025 at 11:20:21AM -0500, Frank Li wrote:
> > This patches basic on
> > https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-0-3c4bb506f665@nxp.com/
> > 
> > I have not hardware to test.
> > 
> > Look for driver owner, who help test this and start move forward to remove
> > cpu_addr_fixup() work.
> 
> If you remove cpu_addr_fixup() callback, it will break backwards
> compatibility with old DTs.

Do you have any pointers to DTs that will be broken?  Or to commits
where they were fixed?

> You should fix the existing DTs and continue carrying the callback
> for a while.

Any insight into where these existing DTs are used and how long
they're likely to live?

Bjorn

Re: [PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[Manivannan Sadhasivam]

On Mon, Mar 17, 2025 at 12:30:08PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 13, 2025 at 11:35:21AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Mar 05, 2025 at 11:20:21AM -0500, Frank Li wrote:
> > > This patches basic on
> > > https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-0-3c4bb506f665@nxp.com/
> > > 
> > > I have not hardware to test.
> > > 
> > > Look for driver owner, who help test this and start move forward to remove
> > > cpu_addr_fixup() work.
> > 
> > If you remove cpu_addr_fixup() callback, it will break backwards
> > compatibility with old DTs.
> 
> Do you have any pointers to DTs that will be broken?  Or to commits
> where they were fixed?
> 

Any patch that fixes issues in DT and then makes the required changes in the
driver without accounting for the old DTs will break backwards compatibility.

> > You should fix the existing DTs and continue carrying the callback
> > for a while.
> 
> Any insight into where these existing DTs are used and how long
> they're likely to live?
> 

There is no fixed rule in this afaik. Just like we continue to support old
hardwares, we need to continue supporting old DTs for some time. The best we can
do is provide some warning so that users can update their DTBs. Then we can get
rid of the old DT support in the driver after some time.

That's why I asked Frank to add the warning previously.

- Mani

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

Re: [PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[Bjorn Helgaas]

On Tue, Mar 18, 2025 at 12:14:27AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 17, 2025 at 12:30:08PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 13, 2025 at 11:35:21AM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Mar 05, 2025 at 11:20:21AM -0500, Frank Li wrote:
> > > > This patches basic on
> > > > https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-0-3c4bb506f665@nxp.com/
> > > > 
> > > > I have not hardware to test.
> > > > 
> > > > Look for driver owner, who help test this and start move
> > > > forward to remove cpu_addr_fixup() work.
> > > 
> > > If you remove cpu_addr_fixup() callback, it will break backwards
> > > compatibility with old DTs.
> > 
> > Do you have any pointers to DTs that will be broken?  Or to
> > commits where they were fixed?
> 
> Any patch that fixes issues in DT and then makes the required
> changes in the driver without accounting for the old DTs will break
> backwards compatibility.

Right, I guess the rule is that if we have patches that fix DT issues,
we should apply them as soon as possible.

And later if we ever have confidence that unfixed DTs no longer exist
(or if we can identify and work around them in the kernel), we can
remove the .cpu_addr_fixup().

Bjorn

Re: [PATCH PATCH RFC NOT TESTED 1/2] ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes

[Manivannan Sadhasivam]

On Fri, Mar 14, 2025 at 12:16:42PM +0530, Siddharth Vadapalli wrote:
> On Thu, Mar 13, 2025 at 10:23:11PM +0530, Manivannan Sadhasivam wrote:
> 
> Hello Mani,
> 
> > On Wed, Mar 05, 2025 at 11:20:22AM -0500, Frank Li wrote:
> > 
> > If you want a specific patch to be tested, you can add [PATCH RFT] tag.C
> > 
> > > According to code in drivers/pci/controller/dwc/pci-dra7xx.c
> > > 
> > > dra7xx_pcie_cpu_addr_fixup()
> > > {
> > > 	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;  //0x0FFFFFFF
> > > }
> > > 
> > > PCI parent bus trim high 4 bits address to 0. Correct ranges in
> > > target-module@51000000 to algin hardware behavior, which translate PCIe
> > > outbound address 0..0x0fff_ffff to 0x2000_0000..0x2fff_ffff.
> > > 
> > > Set 'config' and 'addr_space' reg values to 0.
> > > Change parent bus address of downstream I/O and non-prefetchable memory to
> > > 0.
> > > 
> > > Ensure no functional impact on the final address translation result.
> > > 
> > > Prepare for the removal of the driver’s cpu_addr_fixup().
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  arch/arm/boot/dts/ti/omap/dra7.dtsi | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > index b709703f6c0d4..9213fdd25330b 100644
> > > --- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > +++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > @@ -196,7 +196,7 @@ axi0: target-module@51000000 {
> > >  			#size-cells = <1>;
> > >  			#address-cells = <1>;
> > >  			ranges = <0x51000000 0x51000000 0x3000>,
> > > -				 <0x20000000 0x20000000 0x10000000>;
> > > +				 <0x00000000 0x20000000 0x10000000>;
> > 
> > I'm not able to interpret this properly. So this essentially means that the
> > parent address 0x20000000 is mapped to child address 0x00000000. And the child
> > address is same for other controller as well.
> > 
> > Also, the cpu_addr_fixup() is doing the same by masking out the upper 4 bits. I
> > tried looking into the DRA7 TRM, but it says (ECAM_Param_Base_Addr +
> > 0x20000000) where ECAM_Param_Base_Addr = 0x0000_0000 to 0x0FFF_F000.
> > 
> > I couldn't relate TRM with the cpu_addr_fixup() callback. Can someone from TI
> > shed light on this?
> 
> A "git blame" on the line being modified in dra7.dtsi gives the
> following commit:
> https://github.com/torvalds/linux/commit/c761028ef5e2
> prior to which the ranges is exactly the same as the one being added by
> this patch.
> 
> The cpu_addr_fixup() function was introduced by the following commit:
> https://github.com/torvalds/linux/commit/2ed6cc71e6f7
> with the reason described in
> Section 24.9.4.3.2 PCIe Controller Slave Port
> of the T.R.M. at:
> https://www.ti.com/lit/ug/spruic2d/spruic2d.pdf
> ---------------------------------------------------------------------------
> NOTE:
> The PCIe controller remains fully functional, and able to send transactions
> to, for example, anywhere within the 64-bit PCIe memory space, with the
> appropriate remapping of the 28-bit address by the outbound address
> translation unit (iATU). The limitation is that the total size of addressed
> PCIe regions (in config, memory, IO spaces) must be less than 2^28 bytes.
> ---------------------------------------------------------------------------
> 
> The entire sequence is:
> 0) dra7.dtsi had ranges which match the ranges in the current patch.
> 1) cpu_addr_fixup() was added by
> https://github.com/torvalds/linux/commit/2ed6cc71e6f7
> 2) ranges was updated to <0x20000000 0x20000000 0x10000000> by:
> https://github.com/torvalds/linux/commit/c761028ef5e2
> 3) ranges is being changed back to its original state of "0)" above.
> 

Thanks a lot for the reference.

> cpu_addr_fixup() was introduced to remove the following:
> 	pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
> 	pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
> 	pp->cfg0_base &= DRA7XX_CPU_TO_BUS_ADDR;
> 	pp->cfg1_base &= DRA7XX_CPU_TO_BUS_ADDR;
> in dra7xx_pcie_host_init(). The reason for the above is mentioned in the
> "NOTE" as:
> ---------------------------------------------------------------------------
> The limitation is that the total size of addressed PCIe regions
> (in config, memory, IO spaces) must be less than 2^28 bytes.
> ---------------------------------------------------------------------------
> 

I don't think so. This note is for the *size* of the addressed regions. The
fixup corresponds to the sentence in your above note from TRM:

"able to send transactions to, for example, anywhere within the 64-bit PCIe
memory space, with the appropriate remapping of the 28-bit address by the
outbound address translation unit (iATU)"

I think the limitation is due to the 29-bit address bus width of the PCIe
controller slave port as mentionend in section, 24.9.4.3.2. And that correlates
to the truncation of the upper 4 bits of the IO/CFG/MEM addresses. So even if
the CPU passes address range relative to offset 0, PCIe controller converts it
to 0x20000000/0x30000000 based on the instance.

If my understanding is correct, then commit, c761028ef5e2 should be reverted and
this patch can be applied.

> I am not sure if Frank is accounting for all of this in the current patch
> as well as the dependent patch series associated with removing
> cpu_addr_fixup().
> 
> Regarding testing the series, I unfortunately don't have the hardware so
> I cannot test it.
> 

That's a pity. If TI employee doesn't have access to an upstream supported
platform, then I'm not sure whom we should ask for testing.

Could you please ask around and see if you can get access to one? I'm sure that
the hardware will be available somewhere :)

- Mani

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

Re: [PATCH RFC NOT TESTED 0/2] PCI: dra7xx: Try to clean up dra7xx_pcie_cpu_addr_fixup()

[Manivannan Sadhasivam]

On Mon, Mar 17, 2025 at 02:45:39PM -0500, Bjorn Helgaas wrote:
> On Tue, Mar 18, 2025 at 12:14:27AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 17, 2025 at 12:30:08PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 13, 2025 at 11:35:21AM +0530, Manivannan Sadhasivam wrote:
> > > > On Wed, Mar 05, 2025 at 11:20:21AM -0500, Frank Li wrote:
> > > > > This patches basic on
> > > > > https://lore.kernel.org/imx/20250128-pci_fixup_addr-v9-0-3c4bb506f665@nxp.com/
> > > > > 
> > > > > I have not hardware to test.
> > > > > 
> > > > > Look for driver owner, who help test this and start move
> > > > > forward to remove cpu_addr_fixup() work.
> > > > 
> > > > If you remove cpu_addr_fixup() callback, it will break backwards
> > > > compatibility with old DTs.
> > > 
> > > Do you have any pointers to DTs that will be broken?  Or to
> > > commits where they were fixed?
> > 
> > Any patch that fixes issues in DT and then makes the required
> > changes in the driver without accounting for the old DTs will break
> > backwards compatibility.
> 
> Right, I guess the rule is that if we have patches that fix DT issues,
> we should apply them as soon as possible.
> 

Right, and those patches should not break old DTs.

> And later if we ever have confidence that unfixed DTs no longer exist
> (or if we can identify and work around them in the kernel), we can
> remove the .cpu_addr_fixup().
> 

Yeah. Unfortunately, we do not have a fixed deadline or process. Just like
supporting the legacy broken hw, we have to keep supporting the old DTs for some
time and then get rid of them.

- Mani

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

Re: [PATCH PATCH RFC NOT TESTED 1/2] ARM: dts: ti: dra7: Correct ranges for PCIe and parent bus nodes

[Manivannan Sadhasivam]

On Fri, Mar 14, 2025 at 11:03:00AM -0400, Frank Li wrote:
> On Fri, Mar 14, 2025 at 12:16:42PM +0530, Siddharth Vadapalli wrote:
> > On Thu, Mar 13, 2025 at 10:23:11PM +0530, Manivannan Sadhasivam wrote:
> >
> > Hello Mani,
> >
> > > On Wed, Mar 05, 2025 at 11:20:22AM -0500, Frank Li wrote:
> > >
> > > If you want a specific patch to be tested, you can add [PATCH RFT] tag.C
> > >
> > > > According to code in drivers/pci/controller/dwc/pci-dra7xx.c
> > > >
> > > > dra7xx_pcie_cpu_addr_fixup()
> > > > {
> > > > 	return cpu_addr & DRA7XX_CPU_TO_BUS_ADDR;  //0x0FFFFFFF
> > > > }
> > > >
> > > > PCI parent bus trim high 4 bits address to 0. Correct ranges in
> > > > target-module@51000000 to algin hardware behavior, which translate PCIe
> > > > outbound address 0..0x0fff_ffff to 0x2000_0000..0x2fff_ffff.
> > > >
> > > > Set 'config' and 'addr_space' reg values to 0.
> > > > Change parent bus address of downstream I/O and non-prefetchable memory to
> > > > 0.
> > > >
> > > > Ensure no functional impact on the final address translation result.
> > > >
> > > > Prepare for the removal of the driver’s cpu_addr_fixup().
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  arch/arm/boot/dts/ti/omap/dra7.dtsi | 18 +++++++++---------
> > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/arch/arm/boot/dts/ti/omap/dra7.dtsi b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > > index b709703f6c0d4..9213fdd25330b 100644
> > > > --- a/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > > +++ b/arch/arm/boot/dts/ti/omap/dra7.dtsi
> > > > @@ -196,7 +196,7 @@ axi0: target-module@51000000 {
> > > >  			#size-cells = <1>;
> > > >  			#address-cells = <1>;
> > > >  			ranges = <0x51000000 0x51000000 0x3000>,
> > > > -				 <0x20000000 0x20000000 0x10000000>;
> > > > +				 <0x00000000 0x20000000 0x10000000>;
> > >
> > > I'm not able to interpret this properly. So this essentially means that the
> > > parent address 0x20000000 is mapped to child address 0x00000000. And the child
> > > address is same for other controller as well.
> > >
> > > Also, the cpu_addr_fixup() is doing the same by masking out the upper 4 bits. I
> > > tried looking into the DRA7 TRM, but it says (ECAM_Param_Base_Addr +
> > > 0x20000000) where ECAM_Param_Base_Addr = 0x0000_0000 to 0x0FFF_F000.
> > >
> > > I couldn't relate TRM with the cpu_addr_fixup() callback. Can someone from TI
> > > shed light on this?
> >
> > A "git blame" on the line being modified in dra7.dtsi gives the
> > following commit:
> > https://github.com/torvalds/linux/commit/c761028ef5e2
> > prior to which the ranges is exactly the same as the one being added by
> > this patch.
> 
> Okay, original one correct reflect hardware behavior.
> 
> >
> > The cpu_addr_fixup() function was introduced by the following commit:
> > https://github.com/torvalds/linux/commit/2ed6cc71e6f7
> > with the reason described in
> > Section 24.9.4.3.2 PCIe Controller Slave Port
> > of the T.R.M. at:
> > https://www.ti.com/lit/ug/spruic2d/spruic2d.pdf
> > ---------------------------------------------------------------------------
> > NOTE:
> > The PCIe controller remains fully functional, and able to send transactions
> > to, for example, anywhere within the 64-bit PCIe memory space, with the
> > appropriate remapping of the 28-bit address by the outbound address
> > translation unit (iATU). The limitation is that the total size of addressed
> > PCIe regions (in config, memory, IO spaces) must be less than 2^28 bytes.
> > ---------------------------------------------------------------------------
> >
> > The entire sequence is:
> > 0) dra7.dtsi had ranges which match the ranges in the current patch.
> > 1) cpu_addr_fixup() was added by
> > https://github.com/torvalds/linux/commit/2ed6cc71e6f7
> > 2) ranges was updated to <0x20000000 0x20000000 0x10000000> by:
> > https://github.com/torvalds/linux/commit/c761028ef5e2
> 
> Actually this patch is not necessary, with cpu_addr_fixup(), it should
> work with/without c761028ef5e2 change because it just the use CPU physical
> address, the finial result is exact same.
> 
> > 3) ranges is being changed back to its original state of "0)" above.
> >
> > cpu_addr_fixup() was introduced to remove the following:
> > 	pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
> > 	pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
> > 	pp->cfg0_base &= DRA7XX_CPU_TO_BUS_ADDR;
> > 	pp->cfg1_base &= DRA7XX_CPU_TO_BUS_ADDR;
> > in dra7xx_pcie_host_init(). The reason for the above is mentioned in the
> > "NOTE" as:
> > ---------------------------------------------------------------------------
> > The limitation is that the total size of addressed PCIe regions
> > (in config, memory, IO spaces) must be less than 2^28 bytes.
> > ---------------------------------------------------------------------------
> >
> 
> That is functional equal.
> 
> > I am not sure if Frank is accounting for all of this in the current patch
> > as well as the dependent patch series associated with removing
> > cpu_addr_fixup().
> 
> I have not track back the history. I think before
> https://github.com/torvalds/linux/commit/c761028ef5e2 is correct reflect
> hardware behavor. axi@0 trim down 4 bits before send to PCI controller.
> 
> The commit message of c761028ef5e2
> 
> "In order to update pcie to probe with ti-sysc and genpd, let's update the
> pcie ranges to not use address 0 for 0x20000000 and 0x30000000. The range
> for 0 is typically used for child devices as the offset from the module
> base. In the following patches, we will update pcie to probe with ti-sysc,
> and the patches become a bit confusing to read compared to other similar
> modules unless we update the ranges first. So let's just use the full
> addresses for ranges for the 0x20000000 and 0x30000000 ranges."
> 
> I think maybe only ti's bus fabric do address translation at that time, and
> DT team and dwc pci driver never consider that before. Now more vendor bus
> fabric do address translation. So needn't every platform driver consider it
> but it require DTB reflect hardware behavor correctly.
> 
> We may revert patch c761028ef5e2 firstly, after some time later, we can
> cleanup cpu_addr_fixup().
> 

I agree. Commit, c761028ef5e2 is not going to break anything afaik (even the
cpu_addr_fixup), but it would be good to get it tested.

> It will be wondful, if someone help test it.
> 

Let's see if Siddharth can come up with some good news.

- Mani

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