Re: [PATCH 1/2] PCI: cadence: Ensure that cdns_pcie_host_wait_for_link() waits 100 ms after link up

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, May 04, 2026 at 02:23:34PM +0800, Hans Zhang wrote:
> On 5/4/26 13:08, Siddharth Vadapalli wrote:
> > On 03/05/26 21:16, Hans Zhang wrote:
> > > On 5/2/26 13:18, Siddharth Vadapalli wrote:
> > > > On 01/05/26 21:05, Hans Zhang wrote:
> > > > > As per PCIe r6.0, sec 6.6.1, a Downstream Port that supports
> > > > > Link speeds
> > > > > greater than 5.0 GT/s, software must wait a minimum of 100
> > > > > ms after Link
> > > > > training completes before sending a Configuration Request.
> > > > > 
> > > > > Add a new 'max_link_speed' field in struct cdns_pcie to record the
> > > > > maximum supported (or currently configured) link speed of
> > > > > the controller.
> > > > > 
> > > > > In cdns_pcie_host_wait_for_link(), after the link is reported as up,
> > > > > insert a 100 ms delay if max_link_speed > 2 (i.e., > 5 GT/s). This
> > > > > implements the required delay at the common Cadence host layer.
> > > > > 
> > > > > Currently max_link_speed is zero-initialized, so the delay is not yet
> > > > > active. Glue drivers must set max_link_speed appropriately to enable
> > > > > the delay. This matches the approach taken for the Synopsys DWC
> > > > > controller in commit 80dc18a0cba8d ("PCI: dwc: Ensure that
> > > > > dw_pcie_wait_for_link() waits 100 ms after link up").
> > > > > 
> > > > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > > > ---
> > > > >   .../pci/controller/cadence/pcie-cadence-host-common.c    |
> > > > > 9 +++++ ++++
> > > > >   drivers/pci/controller/cadence/pcie-cadence.h            | 2 ++
> > > > >   2 files changed, 11 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/drivers/pci/controller/cadence/pcie-cadence-host- common.c
> > > > > b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> > > > > index 2b0211870f02..d4ae762f423f 100644
> > > > > --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> > > > > +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> > > > > @@ -14,6 +14,7 @@
> > > > >   #include "pcie-cadence.h"
> > > > >   #include "pcie-cadence-host-common.h"
> > > > > +#include "../../pci.h"
> > > > >   #define LINK_RETRAIN_TIMEOUT HZ
> > > > > @@ -55,6 +56,14 @@ int cdns_pcie_host_wait_for_link(struct
> > > > > cdns_pcie *pcie,
> > > > >       /* Check if the link is up or not */
> > > > >       for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> > > > >           if (pcie_link_up(pcie)) {
> > > > > +            /*
> > > > > +             * As per PCIe r6.0, sec 6.6.1, a Downstream Port that
> > > > > +             * supports Link speeds greater than 5.0 GT/s, software
> > > > > +             * must wait a minimum of 100 ms after Link training
> > > > > +             * completes before sending a Configuration Request.
> > > > > +             */
> > > > > +            if (pcie->max_link_speed > 2)
> > > > > +                msleep(PCIE_RESET_CONFIG_WAIT_MS);
> > > > 
> > > > I think the above could be moved to cdns_pcie_host_start_link()
> > > > as follows:
> > > > 
> > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-
> > > > common.c b/
> > > > drivers/pci/controller/cadence/pcie-cadence-host-common.c
> > > > index 2b0211870f02..0f885dcbdb12 100644
> > > > --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> > > > +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> > > > @@ -115,6 +115,15 @@ int cdns_pcie_host_start_link(struct
> > > > cdns_pcie_rc *rc,
> > > >       if (!ret && rc->quirk_retrain_flag)
> > > >           ret = cdns_pcie_retrain(pcie, pcie_link_up);
> > > > 
> > > > +    /*
> > > > +     * As per PCIe r6.0, sec 6.6.1, a Downstream Port that
> > > > +     * supports Link speeds greater than 5.0 GT/s, software
> > > > +     * must wait a minimum of 100 ms after Link training
> > > > +     * completes before sending a Configuration Request.
> > > > +     */
> > > > +    if (!ret && pcie->max_link_speed > 2)
> > > > +        msleep(PCIE_RESET_CONFIG_WAIT_MS);
> > > > +
> > > >       return ret;
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
> > > > 
> > > > This will avoid an additional and unnecessary delay when
> > > > 'cdns_pcie_retrain()' retrains the link.
> > > > 
> > > > Instead of checking for the link being up using
> > > > "pcie_link_up(pcie)", checking for 'ret' being zero should also
> > > > work (ret being zero indicates that the link is up).
> > > > 
> > > > Since configuration space accesses will not be performed until
> > > > cdns_pcie_host_start_link() completes executing, it should be
> > > > safe to switch to the above implementation.
> > > 
> > > Hi Siddharth,
> > > 
> > > I think this is applicable to LGA IP as per the method you
> > > mentioned. However, for HPA IP, additional repetitive code needs to
> > > be added in the following code.
> > 
> > Yes, additional code is required as you rightly pointed out, but the
> > problem I was trying to address with your patch is the following:
> >      cdns_pcie_host_start_link()
> >        calls cdns_pcie_host_wait_for_link()
> >          Link is Up and we wait for 100 ms here
> >        calls cdns_pcie_retrain()
> >            calls cdns_pcie_host_wait_for_link() a second time
> >              Link is Up again after retraining and we wait and
> >              we wait an additional 100 ms here.
> > 
> > Instead, it will be sufficient if we could wait just once after
> > cdns_pcie_retrain() returns.
> 
> Hi Siddharth,
> 
> Yes, I looked at the code and indeed it works this way.
> 
> Because of the abundance of redundant comments. I'm wondering if it's
> possible to encapsulate a helper function in the file
> drivers/pci/controller/pci-host-common.c, so that controller drivers like
> dwc and cadence can call this API. Or do you know where it would be
> appropriate to place it?
> 
> Hello, Bjorn and Mani, I wonder what your opinions are.

Make a proposal.  Sounds fine to remove redundant comments if they
cause confusion.  Adding a helper to make things more consistent
across drivers also sounds fine, but it would be better to have a
straw-man proposal to respond to.

Bjorn