Re: [PATCH 5/6] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Frank Li <Frank.li@nxp.com>]

On Tue, Sep 09, 2025 at 04:14:48PM +0800, Randolph Lin wrote:
> Hi Bjorn and Frank,
>
> On Mon, Sep 08, 2025 at 05:07:37PM -0500, Bjorn Helgaas wrote:
> > [EXTERNAL MAIL]
> >
> > [+cc Frank, who can probably answer faster than I can]
> >
...
> > > >
> > > > Sorry, we can't do this.  We're removing .cpu_addr_fixup() because
> > > > it's a workaround for defects in the DT description.  See these
> > > > commits, for example:
> > > >
> > > >     befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> > > >     b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
> > > >     07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")
> > >
> > > I’m a bit confused about the following question:
> > > After removing cpu_addr_fixup, should we use pci->parent_bus_offset
> > > to store the offset value, or should pci->parent_bus_offset remain
> > > 0?
> >
> > If you needed qilai_pcie_cpu_addr_fixup(), I would expect your
> > dw_pcie.parent_bus_offset to be non-zero because parent_bus_offset is
> > used instead of ->cpu_addr_fixup().
> >
>
> In this SoC, the dw_pcie.parent_bus_offset should be set to the value of the
> config register base address. Setting the dw_pcie.parent_bus_offset to a
> non-zero value seems to occur only in the path that uses ->cpu_addr_fixup().
>
> The parent_bus_offset is generally used instead of ->cpu_addr_fixup() throughout
> most of the code, but its assignment still seems to rely on ->cpu_addr_fixup()
> when it needs to be set to a non-zero value.
> If no other method exists to set up dw_pcie.parent_bus_offset, can we keep using
> ->cpu_addr_fixup() for the assignment?

Correct your dts file. See below

>
> > > In the commit message:
> > > befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> > >     We know the parent_bus_offset, either computed from a DT reg
> > >     property (the offset is the CPU physical addr - the
> > >     'config'/'addr_space' address on the parent bus) or from a
> > >     .cpu_addr_fixup() (which may have used a host bridge window
> > >     offset).
> > >
> > > We know that "the offset is the CPU physical addr - the
> > > 'config'/'addr_space' address on the parent bus".
> > >
> > > However, in dw_pcie_host_get_resources(), it passes pp->cfg0_base,
> > > which is parsed from the device tree using "config", as the
> > > cpu_phys_addr parameter to dw_pcie_parent_bus_offset(). It also
> > > passes "config" as the 2nd parameter to dw_pcie_parent_bus_offset().
> > >
> > > In dw_pcie_parent_bus_offset(), the 2nd parameter is used to get the
> > > index from the devicetree "reg-names" field, and the result is used
> > > as the 'config'/'addr_space' address.
> > >
> > > It seems that the same value is being obtained through a different
> > > method, and the return value appears to be 0.  Could I be
> > > misunderstanding something?

I missed prevous discussion. Actually address convert happen at PCIe's parent
bus fabric. If your DTS correct reflect this address convert, you needn't
cpu_addr_fixup() at all.

bus {
	ranges = (0x2000_00000, 0x1000_0000, 0x1000_0000)
	^^^^^

	pcie@xxxx {
		reg = <0x2000_00000, 0x1000>;
		reg-names = <config>;
	}
}

0x2000_0000 is actaully address input to your PCI controller although
CPU use 0x1000_0000.  "bus" fabric convert 0x1000_0000 to 0x2000_0000

So dwc common code already can handle this to auto calucate
parent_bus_offset base on ranges of bus.

Frank

> >
>
> Sincerely,
> Randolph