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

[Siddharth Vadapalli <s-vadapalli@ti.com>]

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.