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

[Randolph Lin <randolph@andestech.com>]

Hi Bjorn,

On Wed, Sep 17, 2025 at 04:57:22PM -0500, Bjorn Helgaas wrote:
> [EXTERNAL MAIL]
> 
> 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.
>

ok.

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

refer to the following patches:
    91703041697c ("PCI: Allow built-in drivers to use async initial probing")
    8e77d3d59d7b ("Revert "usb: xhci-pci: Set PROBE_PREFER_ASYNCHRONOUS"")

We can continue using PROBE_PREFER_ASYNCHRONOUS.
When used with drivers that support it, such as NVMe, the system behaves as
expected. In the past, we primarily used Renesas xHCI controller.

Sincerely,
Randolph