Re: [PATCH v4 5/7] PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link up

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jul 01, 2025 at 06:31:50PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 01, 2025 at 01:55:43PM GMT, Niklas Cassel wrote:
> > On Mon, Jun 30, 2025 at 03:19:02PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Jun 25, 2025 at 12:23:51PM +0200, Niklas Cassel 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 this delay in dw_pcie_wait_for_link(), after the link is reported as
> > > > up. The delay will only be performed in the success case where the link
> > > > came up.
> > > > 
> > > > DWC glue drivers that have a link up IRQ (drivers that set
> > > > use_linkup_irq = true) do not call dw_pcie_wait_for_link(), instead they
> > > > perform this delay in their threaded link up IRQ handler.
> > > > 
> > > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > > > Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 4d794964fa0f..053e9c540439 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -714,6 +714,14 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > >  		return -ETIMEDOUT;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * 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 (pci->max_link_speed > 2)
> > > > +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> > > 
> > > Sec 6.6.1 also requires "100 ms following exit from a Conventional
> > > Reset before sending a Configuration Request to the device immediately
> > > below that Port" for Downstream Ports that do *not* support Link
> > > speeds greater than 5.0 GT/s.
> > > 
> > > Where does that delay happen?
> > 
> > Argh...
> > 
> > In version 3 of this series, the wait was unconditional, so it handled both
> > cases:
> > https://lore.kernel.org/linux-pci/20250613124839.2197945-13-cassel@kernel.org/
> > 
> > But I got a review comment from Mani:
> > https://lore.kernel.org/linux-pci/hmkx6vjoqshthk5rqakcyzneredcg6q45tqhnaoqvmvs36zmsk@tzd7f44qkydq/
> > 
> > That I should only care about the greater than 5.0 GT/s case.
> > 
> > Perhaps the confusion came about since the comment only mentioned
> > one of the two the cases specified by PCIe r6.0, sec 6.6.1.
> 
> Yes, that was intentional since the DWC core code only deals with
> the link up and not PERST# (which is handled by the glue drivers).
> 
> > So I think the best thing is either (on top of pci/next):
> 
> No. The PERST# delay should be handled by the glue drivers since
> they are the ones controlling the PERST# line. Doing an
> unconditional wait for both the cases in DWC core, seems wrong to
> me.

It ends up being a little bit weird that the delay is in the DWC core
(dw_pcie_wait_for_link()) for ports that support fast links, and in
the glue drivers otherwise.  It would be easier to verify and maintain
if the delay were always in the DWC core.

If we had a dw_pcie_host_ops callback for PERST# deassertion, the
delay could be in the DWC core, but I don't know if there's enough
consistency across drivers for that to be practical.

And of course, we also have the "Link-up IRQ" drivers where the delay
is currently in the glue, although I could imagine a DWC core API that
includes both the delay and the lock/rescan logic.

I feel a little bit exposed here because none of the glue drivers
include this delay for slow ports.

Bjorn