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

[Randolph Lin <randolph@andestech.com>]

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]
> 
> On Fri, Aug 29, 2025 at 05:41:17PM +0800, Randolph Lin wrote:
> > Rn Wed, Aug 20, 2025 at 10:54:44AM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 20, 2025 at 07:18:42PM +0800, Randolph Lin wrote:
> > > > Add driver support for DesignWare based PCIe controller in Andes
> > > > QiLai SoC. The driver only supports the Root Complex mode.
> 
> > > > +#define PCIE_LOGIC_COHERENCY_CONTROL3                0x8e8
> > > > +/* Write-Back, Read and Write Allocate */
> > > > +#define IOCP_ARCACHE                         0xf
> > > > +/* Write-Back, Read and Write Allocate */
> > > > +#define IOCP_AWCACHE                         0xf
> > >
> > > Are IOCP_ARCACHE and IOCP_AWCACHE supposed to be identical values with
> > > identical comments?
> 
> Just pointing this out since you didn't respond to it.
> 

Sorry, I originally intended to add a comment in the V2 patch.

The following shows the modified version:
/*
 * Refer to Table A4-5 (Memory type encoding) in the
 * AMBA AXI and ACE Protocol Specification.
 * The selected value corresponds to the Memory type field:
 * "Write-back, Read and Write-allocate".
 */
#define IOCP_ARCACHE				0b1111
#define IOCP_AWCACHE				0b1111


Refer to the AMBA AXI and ACE Protocol Specification.
The last three rows in the table A4-5:
ARCACHE         AWCACHE         Memory type
1111 (0111)     0111            Write-back Read-allocate
1011            1111 (1011)     Write-back Write-allocate
1111            1111            Write-back Read and Write-allocate

I know that the following are meaningless.
1. Read-allocate in AWCACHE 
2. Write-allocate in ARCACHE
However, I want to set it up as "Write-back, Read and Write-allocate";
therefore, it seems they have the same comment.

> > > > +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > > > +{
> > > > +     struct dw_pcie_rp *pp = &pci->pp;
> > > > +
> > > > +     return cpu_addr - pp->cfg0_base;
> > >
> > > 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?

> > 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?
> 

Sincerely,
Randolph