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

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