Message-ID: <20250917215722.GA1876729@bhelgaas>
Date: Wed, 17 Sep 2025 16:57:22 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Randolph Lin <randolph@...estech.com>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
	jingoohan1@...il.com, mani@...nel.org, lpieralisi@...nel.org,
	kwilczynski@...nel.org, robh@...nel.org, bhelgaas@...gle.com,
	krzk+dt@...nel.org, conor+dt@...nel.org, alex@...ti.fr,
	aou@...s.berkeley.edu, palmer@...belt.com, paul.walmsley@...ive.com,
	ben717@...estech.com, inochiama@...il.com,
	thippeswamy.havalige@....com, namcao@...utronix.de,
	shradha.t@...sung.com, randolph.sklin@...il.com,
	tim609@...estech.com
Subject: Re: [PATCH v2 4/5] PCI: andes: Add Andes QiLai SoC PCIe host driver
 support

On Wed, Sep 17, 2025 at 08:16:25PM +0800, Randolph Lin wrote:
> On Tue, Sep 16, 2025 at 09:46:52AM -0500, Bjorn Helgaas wrote:
> > On Tue, Sep 16, 2025 at 06:04:16PM +0800, Randolph Lin wrote:
> > > Add driver support for DesignWare based PCIe controller in Andes
> > > QiLai SoC. The driver only supports the Root Complex mode.

> > > +          Say Y here to enable PCIe controller support on Andes QiLai SoCs,
> > > +       which operate in Root Complex mode. The Andes QiLai SoCs PCIe
> > > +       controller is based on DesignWare IP (5.97a version) and therefore
> > > +       the driver re-uses the DesignWare core functions to implement the
> > > +       driver. The Andes QiLai SoC has three Root Complexes (RCs): one
> > > +       operates on PCIe 4.0 with 4 lanes at 0x80000000, while the other
> > > +       two operate on PCIe 4.0 with 2 lanes at 0xA0000000 and 0xC0000000,
> > > +       respectively.
> > 
> > I assume these addresses come from devicetree, so I don't think
> > there's any need to include them here.
> 
> I will add num-lanes property in the devicetree.

Don't add num-lanes to devicetree unless your driver requires it.
dw_pcie_host_init() uses dw_pcie_link_get_max_link_width() try to get
the lane width from PCI_EXP_LNKCAP.

> > > +static
> > > +bool qilai_pcie_outbound_atu_addr_valid(struct dw_pcie *pci,
> > > +                                     const struct dw_pcie_ob_atu_cfg *atu,
> > > +                                     u64 *limit_addr)
> > > +{
> > > +     u64 parent_bus_addr = atu->parent_bus_addr;
> > > +
> > > +     *limit_addr = parent_bus_addr + atu->size - 1;
> > > +
> > > +     /*
> > > +      * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> > > +      * only need to ensure addresses below 4 GB match pci->region_limit.
> > > +      */
> > > +     if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> > > +         lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> > > +         !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> > > +         !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> > > +             return false;
> > 
> > Seems a little bit strange.  Is this something that could be expressed
> > via devicetree?  Or something peculiar about QiLai that's different
> > from all the other DWC-based controllers?
> 
> After reviewing both the code history and the bug tracking system,
> it turns out that this code doesn't even qualify as a valid
> workaround.  Apologies for having submitted it as a patch.
> 
> The root cause is that the iATU limits were not configured
> correctly.  The original design assumed at least 32GB or 128GB of
> BAR resource assignment, but the actual chip sets the iATU limit to
> only 4GB.  As a result, region_limit is always constrained by this
> 4GB boundary.
> 
> The correct workaround should be to program the iATU only for the
> 32-bit address space and skip iATU programming for the 64-bit space.
> A simple way to implement this workaround is to parse the
> num-viewport property from the devicetree and use this value
> directly, instead of relying on the result of reading
> PCIE_ATU_VIEWPORT.
> 
> I will attempt to implement it this way, but the correct method is
> not yet well-defined. Do you have any suggestions on how to modify
> the num-viewport property from the devicetree for use in the driver?
> It seems it will be modified in pcie-designware.c.

Nope, I don't know enough about DWC to give any advice, sorry!

> > > +static struct platform_driver qilai_pcie_driver = {
> > > +     .probe = qilai_pcie_probe,
> > > +     .driver = {
> > > +             .name   = "qilai-pcie",
> > > +             .of_match_table = qilai_pcie_of_match,
> > > +             /* only test passed at PROBE_DEFAULT_STRATEGY */
> > > +             .probe_type = PROBE_DEFAULT_STRATEGY,
> > 
> > This is the only use of PROBE_DEFAULT_STRATEGY in the entire tree, so
> > I doubt you need it.  If you do, please explain why in more detail.
> 
> In the V1 patch, the reviewer, Manivannan, suggested:
> "You should start using PROBE_PREFER_ASYNCHRONOUS."
> However, after setting up PROBE_PREFER_ASYNCHRONOUS, numerous errors
> were encountered during the EP device probe flow.
> Therefore, we would prefer to continue using PROBE_DEFAULT_STRATEGY.

I don't really know the details of probe_type.  But it sounds like the
errors with PROBE_PREFER_ASYNCHRONOUS should probably be debugged and
fixed.

If PROBE_PREFER_ASYNCHRONOUS can't be made to work, it sounds like you
should specify PROBE_FORCE_SYNCHRONOUS, because the comments suggest
that using PROBE_DEFAULT_STRATEGY means the driver should work with
either PROBE_FORCE_SYNCHRONOUS or PROBE_PREFER_ASYNCHRONOUS:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/device/driver.h?id=v6.16#n24

Powered by blists - more mailing lists