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@xxxxxxx>
> > > ---
> > > 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.
