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

[Niklas Cassel <cassel@kernel.org>]

Hello Bjorn, Mani,

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.

So I think the best thing is either (on top of pci/next):

1) Make the wait unconditional and mention both cases in the comment:

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 89aad5a08928..c30b1d8d833c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -718,9 +718,15 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 	* 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.
+	*
+	* As per PCIe r6.0, sec 6.6.1, a Downstream Port that does not support
+	* Link speeds greater than 5.0 GT/s, software must wait a minimum of
+	* 100 ms following exit from a Conventional Reset before sending a
+	* Configuration Request to the device immediately below that Port.
+	*
+	* For either case, perform the wait here.
 	*/
-	if (pci->max_link_speed > 2)
-		msleep(PCIE_RESET_CONFIG_WAIT_MS);
+	msleep(PCIE_RESET_CONFIG_WAIT_MS);
 
 	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);


Or:

1) Make the wait unconditional and mention none of the cases in the comment:

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 89aad5a08928..ce3b5b319550 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -714,13 +714,7 @@ 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);
+	msleep(PCIE_RESET_CONFIG_WAIT_MS);
 
 	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);


Please tell me what you prefer and I can send a patch on top of pci/next.



Also note that some drivers already do an explicit wait after PERST# has been
deasserted (in addition to the wait while PERST# is asserted), e.g.:
https://github.com/torvalds/linux/blob/v6.16-rc4/drivers/pci/controller/dwc/pci-imx6.c#L885
https://github.com/torvalds/linux/blob/v6.16-rc4/drivers/pci/controller/dwc/pcie-qcom.c#L294
https://github.com/torvalds/linux/blob/v6.16-rc4/drivers/pci/controller/dwc/pcie-keembay.c#L89


Kind regards,
Niklas