[PATCH v3 0/6] PCI: dwc: Do not enumerate bus before endpoint devices are ready

[PATCH v3 0/6] PCI: dwc: Do not enumerate bus before endpoint devices are ready

[Niklas Cassel]

Hello all,

The DWC PCIe controller driver currently does not follow the PCIe
specification with regards to the delays after link training, before
sending out configuration requests. This series fixes this.

At the same time, PATCH 1/4 addresses a regression where a Plextor
NVMe drive fails to be configured correctly. With this series, the
Plextor NVMe drive works once again.


Kind regards,
Niklas


Changes since v2:
-Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS.


Niklas Cassel (6):
  PCI: Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to
    PCIE_RESET_CONFIG_WAIT_MS
  PCI: rockchip-host: Use macro PCIE_RESET_CONFIG_WAIT_MS
  PCI: dw-rockchip: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ
  PCI: qcom: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ
  PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link
    up
  PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

 drivers/pci/controller/dwc/pcie-designware.c  | 13 ++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.h  | 13 +++++++++----
 drivers/pci/controller/dwc/pcie-dw-rockchip.c |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c        |  1 +
 drivers/pci/controller/pcie-rockchip-host.c   |  2 +-
 drivers/pci/controller/plda/pcie-starfive.c   |  2 +-
 drivers/pci/pci.h                             |  9 +--------
 7 files changed, 26 insertions(+), 15 deletions(-)

-- 
2.49.0

[PATCH v3 1/6] PCI: Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS

[Niklas Cassel]

Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/plda/pcie-starfive.c | 2 +-
 drivers/pci/pci.h                           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index e73c1b7bc8ef..3caf53c6c082 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -368,7 +368,7 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 	 * of 100ms following exit from a conventional reset before
 	 * sending a configuration request to the device.
 	 */
-	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);
+	msleep(PCIE_RESET_CONFIG_WAIT_MS);
 
 	if (starfive_pcie_host_wait_for_link(pcie))
 		dev_info(dev, "port link down\n");
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12215ee72afb..98d6fccb383e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -61,7 +61,7 @@ struct pcie_tlp_log;
  *    completes before sending a Configuration Request to the device
  *    immediately below that Port."
  */
-#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	100
+#define PCIE_RESET_CONFIG_WAIT_MS	100
 
 /* Message Routing (r[2:0]); PCIe r6.0, sec 2.2.8 */
 #define PCIE_MSG_TYPE_R_RC	0
-- 
2.49.0

[PATCH v3 2/6] PCI: rockchip-host: Use macro PCIE_RESET_CONFIG_WAIT_MS

[Niklas Cassel]

Macro PCIE_RESET_CONFIG_WAIT_MS was added to pci.h in commit d5ceb9496c56
("PCI: Add PCIE_RESET_CONFIG_DEVICE_WAIT_MS waiting time value").

Later, in commit 70a7bfb1e515 ("PCI: rockchip-host: Wait 100ms after reset
before starting configuration"), PCIE_T_RRS_READY_MS was added to pci.h.

These macros are duplicates, and represent the exact same delay in the
PCIe specification.

Since the comment above PCIE_RESET_CONFIG_WAIT_MS is strictly more correct
than the comment above PCIE_T_RRS_READY_MS, change rockchip-host to use
PCIE_RESET_CONFIG_WAIT_MS, and remove PCIE_T_RRS_READY_MS, as
rockchip-host is the only user of this macro.

Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-host.c | 2 +-
 drivers/pci/pci.h                           | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index b9e7a8710cf0..c11ed45c25f6 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -325,7 +325,7 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 	msleep(PCIE_T_PVPERL_MS);
 	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
 
-	msleep(PCIE_T_RRS_READY_MS);
+	msleep(PCIE_RESET_CONFIG_WAIT_MS);
 
 	/* 500ms timeout value should be enough for Gen1/2 training */
 	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 98d6fccb383e..819833e57590 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -35,13 +35,6 @@ struct pcie_tlp_log;
  */
 #define PCIE_T_PERST_CLK_US		100
 
-/*
- * End of conventional reset (PERST# de-asserted) to first configuration
- * request (device able to respond with a "Request Retry Status" completion),
- * from PCIe r6.0, sec 6.6.1.
- */
-#define PCIE_T_RRS_READY_MS	100
-
 /*
  * PCIe r6.0, sec 5.3.3.2.1 <PME Synchronization>
  * Recommends 1ms to 10ms timeout to check L2 ready.
-- 
2.49.0

Re: [PATCH v3 2/6] PCI: rockchip-host: Use macro PCIE_RESET_CONFIG_WAIT_MS

[Manivannan Sadhasivam]

On Fri, Jun 13, 2025 at 02:48:41PM +0200, Niklas Cassel wrote:
> Macro PCIE_RESET_CONFIG_WAIT_MS was added to pci.h in commit d5ceb9496c56

s/PCIE_RESET_CONFIG_WAIT_MS/PCIE_RESET_CONFIG_DEVICE_WAIT_MS

> ("PCI: Add PCIE_RESET_CONFIG_DEVICE_WAIT_MS waiting time value").
> 
> Later, in commit 70a7bfb1e515 ("PCI: rockchip-host: Wait 100ms after reset
> before starting configuration"), PCIE_T_RRS_READY_MS was added to pci.h.
> 
> These macros are duplicates, and represent the exact same delay in the
> PCIe specification.
> 
> Since the comment above PCIE_RESET_CONFIG_WAIT_MS is strictly more correct
> than the comment above PCIE_T_RRS_READY_MS, change rockchip-host to use
> PCIE_RESET_CONFIG_WAIT_MS, and remove PCIE_T_RRS_READY_MS, as
> rockchip-host is the only user of this macro.
> 
> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

LGTM!

- Mani

> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 2 +-
>  drivers/pci/pci.h                           | 7 -------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index b9e7a8710cf0..c11ed45c25f6 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -325,7 +325,7 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  	msleep(PCIE_T_PVPERL_MS);
>  	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
>  
> -	msleep(PCIE_T_RRS_READY_MS);
> +	msleep(PCIE_RESET_CONFIG_WAIT_MS);
>  
>  	/* 500ms timeout value should be enough for Gen1/2 training */
>  	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 98d6fccb383e..819833e57590 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -35,13 +35,6 @@ struct pcie_tlp_log;
>   */
>  #define PCIE_T_PERST_CLK_US		100
>  
> -/*
> - * End of conventional reset (PERST# de-asserted) to first configuration
> - * request (device able to respond with a "Request Retry Status" completion),
> - * from PCIe r6.0, sec 6.6.1.
> - */
> -#define PCIE_T_RRS_READY_MS	100
> -
>  /*
>   * PCIe r6.0, sec 5.3.3.2.1 <PME Synchronization>
>   * Recommends 1ms to 10ms timeout to check L2 ready.
> -- 
> 2.49.0
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v3 3/6] PCI: dw-rockchip: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ

[Niklas Cassel]

Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
100ms (PCIE_RESET_CONFIG_WAIT_MS) after Link training completes before
sending a Configuration Request.

Prior to ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since
we can detect Link Up"), dw-rockchip used dw_pcie_wait_for_link(),
which waited between 0 and 90ms after the link came up before we
enumerate the bus, and this was apparently enough for most devices.

After ec9fd499b9c6, rockchip_pcie_rc_sys_irq_thread() started
enumeration immediately when handling the link-up IRQ, and devices
(e.g., Laszlo Fiat's PLEXTOR PX-256M8PeGN NVMe SSD) may not be ready
to handle config requests yet.

Delay PCIE_RESET_CONFIG_WAIT_MS after the link-up IRQ before starting
enumeration.

Cc: Laszlo Fiat <laszlo.fiat@proton.me>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 93171a392879..108d30637920 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -458,6 +458,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
 
 	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
 		if (rockchip_pcie_link_up(pci)) {
+			msleep(PCIE_RESET_CONFIG_WAIT_MS);
 			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
 			/* Rescan the bus to enumerate endpoint devices */
 			pci_lock_rescan_remove();
-- 
2.49.0

[PATCH v3 4/6] PCI: qcom: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ

[Niklas Cassel]

Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
100ms (PCIE_RESET_CONFIG_WAIT_MS) after Link training completes before
sending a Configuration Request.

Prior to 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect
Link Up"), qcom used dw_pcie_wait_for_link(), which waited between 0
and 90ms after the link came up before we enumerate the bus, and this
was apparently enough for most devices.

After 36971d6c5a9a, qcom_pcie_global_irq_thread() started enumeration
immediately when handling the link-up IRQ, and devices (e.g., Laszlo
Fiat's PLEXTOR PX-256M8PeGN NVMe SSD) may not be ready to handle config
requests yet.

Delay PCIE_RESET_CONFIG_WAIT_MS after the link-up IRQ before starting
enumeration.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f85655..9b12f2f02042 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1564,6 +1564,7 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 	writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR);
 
 	if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) {
+		msleep(PCIE_RESET_CONFIG_WAIT_MS);
 		dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
 		/* Rescan the bus to enumerate endpoint devices */
 		pci_lock_rescan_remove();
-- 
2.49.0

Re: [PATCH v3 4/6] PCI: qcom: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ

[Manivannan Sadhasivam]

On Fri, Jun 13, 2025 at 02:48:43PM +0200, Niklas Cassel wrote:
> Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
> 100ms (PCIE_RESET_CONFIG_WAIT_MS) after Link training completes before
> sending a Configuration Request.
> 
> Prior to 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect
> Link Up"), qcom used dw_pcie_wait_for_link(), which waited between 0
> and 90ms after the link came up before we enumerate the bus, and this
> was apparently enough for most devices.
> 
> After 36971d6c5a9a, qcom_pcie_global_irq_thread() started enumeration
> immediately when handling the link-up IRQ, and devices (e.g., Laszlo
> Fiat's PLEXTOR PX-256M8PeGN NVMe SSD) may not be ready to handle config
> requests yet.
> 
> Delay PCIE_RESET_CONFIG_WAIT_MS after the link-up IRQ before starting
> enumeration.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")

Shouldn't 36971d6c5a9a be the fixes commit?

- Mani

> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index c789e3f85655..9b12f2f02042 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1564,6 +1564,7 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  	writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR);
>  
>  	if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) {
> +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
>  		dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
>  		/* Rescan the bus to enumerate endpoint devices */
>  		pci_lock_rescan_remove();
> -- 
> 2.49.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 4/6] PCI: qcom: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ

[Niklas Cassel]

On Mon, Jun 23, 2025 at 08:27:19AM -0600, Manivannan Sadhasivam wrote:
> On Fri, Jun 13, 2025 at 02:48:43PM +0200, Niklas Cassel wrote:
> > Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
> > 100ms (PCIE_RESET_CONFIG_WAIT_MS) after Link training completes before
> > sending a Configuration Request.
> > 
> > Prior to 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect
> > Link Up"), qcom used dw_pcie_wait_for_link(), which waited between 0
> > and 90ms after the link came up before we enumerate the bus, and this
> > was apparently enough for most devices.
> > 
> > After 36971d6c5a9a, qcom_pcie_global_irq_thread() started enumeration
> > immediately when handling the link-up IRQ, and devices (e.g., Laszlo
> > Fiat's PLEXTOR PX-256M8PeGN NVMe SSD) may not be ready to handle config
> > requests yet.
> > 
> > Delay PCIE_RESET_CONFIG_WAIT_MS after the link-up IRQ before starting
> > enumeration.
> > 
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> 
> Shouldn't 36971d6c5a9a be the fixes commit?

See Bjorn's comment:
https://lore.kernel.org/linux-pci/20250611211456.GA869983@bhelgaas/

	I would argue that 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip
	RK356X host controller driver") is the right Fixes: commit here
	because dw_pcie_wait_for_link() *never* waited the required time, and
	it's quite possible that other devices don't work correctly.  The
	delay was about 90ms - <time required for link training>, so could be
	significantly less than 100ms.


Thus, following Bjorn's comment, to put the commit that introduced the driver
as the Fixes tag for dw-rockchip, I did the same thing for qcom.


Kind regards,
Niklas

[PATCH v3 5/6] PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link up

[Niklas Cassel]

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 4d794964fa0f..24903f67d724 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -714,6 +714,13 @@ 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.
+	 */
+	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);
 
-- 
2.49.0

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

[Manivannan Sadhasivam]

On Fri, Jun 13, 2025 at 02:48:44PM +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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 4d794964fa0f..24903f67d724 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -714,6 +714,13 @@ 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.
> +	 */

As the comment clearly states, we should only wait if the downstream port
supports link speed > 5.0 GT/s. So you should have the below check:

	if (pci->max_link_speed > 1)
		msleep(PCIE_RESET_CONFIG_WAIT_MS);

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Niklas Cassel]

On Mon, Jun 23, 2025 at 08:28:55AM -0600, Manivannan Sadhasivam wrote:
> On Fri, Jun 13, 2025 at 02:48:44PM +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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 4d794964fa0f..24903f67d724 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -714,6 +714,13 @@ 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.
> > +	 */
> 
> As the comment clearly states, we should only wait if the downstream port
> supports link speed > 5.0 GT/s. So you should have the below check:
> 
> 	if (pci->max_link_speed > 1)
> 		msleep(PCIE_RESET_CONFIG_WAIT_MS);

PCIe 1.0 has 2.5 GT/s
PCIe 2.0 has 5.0 GT/s

Thus will assume that you actually meant:

if (pci->max_link_speed > 2)


Kind regards,
Niklas

[PATCH v3 6/6] PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

[Niklas Cassel]

There is no reason for the delay, in each loop iteration, while polling for
link up (LINK_WAIT_SLEEP_MS), to be so long as 90 ms.

PCIe r6.0, sec 6.6.1, still require us to wait for up to 1.0 s for the link
to come up, thus the number of retries (LINK_WAIT_MAX_RETRIES) is increased
to keep the total timeout to 1.0 s.

PCIe r6.0, sec 6.6.1, also mandates that there is a 100 ms delay, after the
link has been established, before performing configuration requests (this
delay already exists in dw_pcie_wait_for_link() and is unchanged).

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 |  6 +++++-
 drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 24903f67d724..ae6f0bfe3c56 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -701,7 +701,11 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 	u32 offset, val;
 	int retries;
 
-	/* Check if the link is up or not */
+	/*
+	 * Check if the link is up or not. As per PCIe r6.0, sec 6.6.1, software
+	 * must allow at least 1.0 s following exit from a Conventional Reset of
+	 * a device, before determining that the device is broken.
+	 */
 	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
 		if (dw_pcie_link_up(pci))
 			break;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ce9e18554e42..b225c4f3d36a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -62,11 +62,16 @@
 #define dw_pcie_cap_set(_pci, _cap) \
 	set_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
 
-/* Parameters for the waiting for link up routine */
-#define LINK_WAIT_MAX_RETRIES		10
-#define LINK_WAIT_SLEEP_MS		90
+/*
+ * Parameters for waiting for a link to be established. As per PCIe r6.0,
+ * sec 6.6.1, software must allow at least 1.0 s following exit from a
+ * Conventional Reset of a device, before determining that the device is broken.
+ * Therefore LINK_WAIT_MAX_RETRIES * LINK_WAIT_SLEEP_MS should equal 1.0 s.
+ */
+#define LINK_WAIT_MAX_RETRIES		100
+#define LINK_WAIT_SLEEP_MS		10
 
-/* Parameters for the waiting for iATU enabled routine */
+/* Parameters for waiting for iATU enabled routine */
 #define LINK_WAIT_MAX_IATU_RETRIES	5
 #define LINK_WAIT_IATU			9
 
-- 
2.49.0

Re: [PATCH v3 6/6] PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

[Manivannan Sadhasivam]

On Fri, Jun 13, 2025 at 02:48:45PM +0200, Niklas Cassel wrote:
> There is no reason for the delay, in each loop iteration, while polling for
> link up (LINK_WAIT_SLEEP_MS), to be so long as 90 ms.
> 
> PCIe r6.0, sec 6.6.1, still require us to wait for up to 1.0 s for the link
> to come up, thus the number of retries (LINK_WAIT_MAX_RETRIES) is increased
> to keep the total timeout to 1.0 s.
> 
> PCIe r6.0, sec 6.6.1, also mandates that there is a 100 ms delay, after the
> link has been established, before performing configuration requests (this
> delay already exists in dw_pcie_wait_for_link() and is unchanged).
> 
> 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 |  6 +++++-
>  drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 24903f67d724..ae6f0bfe3c56 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -701,7 +701,11 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  	u32 offset, val;
>  	int retries;
>  
> -	/* Check if the link is up or not */
> +	/*
> +	 * Check if the link is up or not. As per PCIe r6.0, sec 6.6.1, software
> +	 * must allow at least 1.0 s following exit from a Conventional Reset of
> +	 * a device, before determining that the device is broken.
> +	 */
>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
>  		if (dw_pcie_link_up(pci))
>  			break;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ce9e18554e42..b225c4f3d36a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -62,11 +62,16 @@
>  #define dw_pcie_cap_set(_pci, _cap) \
>  	set_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
>  
> -/* Parameters for the waiting for link up routine */
> -#define LINK_WAIT_MAX_RETRIES		10
> -#define LINK_WAIT_SLEEP_MS		90
> +/*
> + * Parameters for waiting for a link to be established. As per PCIe r6.0,
> + * sec 6.6.1, software must allow at least 1.0 s following exit from a
> + * Conventional Reset of a device, before determining that the device is broken.
> + * Therefore LINK_WAIT_MAX_RETRIES * LINK_WAIT_SLEEP_MS should equal 1.0 s.
> + */
> +#define LINK_WAIT_MAX_RETRIES		100
> +#define LINK_WAIT_SLEEP_MS		10

These are not DWC specific. So I'd recommend moving it to drivers/pci/pci.h.

Also, I'd have fancied a helper that does the link check with all delays taken
care of. But that involves creating a common link_up function and would take a
bit more work. So leaving that is fine for the moment.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 6/6] PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

[Niklas Cassel]

On Mon, Jun 23, 2025 at 08:52:24AM -0600, Manivannan Sadhasivam wrote:
> On Fri, Jun 13, 2025 at 02:48:45PM +0200, Niklas Cassel wrote:
> > There is no reason for the delay, in each loop iteration, while polling for
> > link up (LINK_WAIT_SLEEP_MS), to be so long as 90 ms.
> > 
> > PCIe r6.0, sec 6.6.1, still require us to wait for up to 1.0 s for the link
> > to come up, thus the number of retries (LINK_WAIT_MAX_RETRIES) is increased
> > to keep the total timeout to 1.0 s.
> > 
> > PCIe r6.0, sec 6.6.1, also mandates that there is a 100 ms delay, after the
> > link has been established, before performing configuration requests (this
> > delay already exists in dw_pcie_wait_for_link() and is unchanged).
> > 
> > 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 |  6 +++++-
> >  drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++----
> >  2 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 24903f67d724..ae6f0bfe3c56 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -701,7 +701,11 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >  	u32 offset, val;
> >  	int retries;
> >  
> > -	/* Check if the link is up or not */
> > +	/*
> > +	 * Check if the link is up or not. As per PCIe r6.0, sec 6.6.1, software
> > +	 * must allow at least 1.0 s following exit from a Conventional Reset of
> > +	 * a device, before determining that the device is broken.
> > +	 */
> >  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> >  		if (dw_pcie_link_up(pci))
> >  			break;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index ce9e18554e42..b225c4f3d36a 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -62,11 +62,16 @@
> >  #define dw_pcie_cap_set(_pci, _cap) \
> >  	set_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
> >  
> > -/* Parameters for the waiting for link up routine */
> > -#define LINK_WAIT_MAX_RETRIES		10
> > -#define LINK_WAIT_SLEEP_MS		90
> > +/*
> > + * Parameters for waiting for a link to be established. As per PCIe r6.0,
> > + * sec 6.6.1, software must allow at least 1.0 s following exit from a
> > + * Conventional Reset of a device, before determining that the device is broken.
> > + * Therefore LINK_WAIT_MAX_RETRIES * LINK_WAIT_SLEEP_MS should equal 1.0 s.
> > + */
> > +#define LINK_WAIT_MAX_RETRIES		100
> > +#define LINK_WAIT_SLEEP_MS		10
> 
> These are not DWC specific. So I'd recommend moving it to drivers/pci/pci.h.

The total time to wait (LINK_WAIT_MAX_RETRIES * LINK_WAIT_SLEEP_MS) is not DWC
specific.

However, that we choose to wait 10 ms between polls is definitely DWC specific.

But sure, I can move these to drivers/pci/pci.h.


Kind regards,
Niklas

Re: [PATCH v3 0/6] PCI: dwc: Do not enumerate bus before endpoint devices are ready

[Niklas Cassel]

On Fri, Jun 13, 2025 at 02:48:39PM +0200, Niklas Cassel wrote:
> Hello all,
> 
> The DWC PCIe controller driver currently does not follow the PCIe
> specification with regards to the delays after link training, before
> sending out configuration requests. This series fixes this.
> 
> At the same time, PATCH 1/4 addresses a regression where a Plextor
> NVMe drive fails to be configured correctly. With this series, the
> Plextor NVMe drive works once again.
> 
> 
> Kind regards,
> Niklas
> 
> 
> Changes since v2:
> -Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS.
> 
> 
> Niklas Cassel (6):
>   PCI: Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to
>     PCIE_RESET_CONFIG_WAIT_MS
>   PCI: rockchip-host: Use macro PCIE_RESET_CONFIG_WAIT_MS
>   PCI: dw-rockchip: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ
>   PCI: qcom: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ
>   PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link
>     up
>   PCI: dwc: Reduce LINK_WAIT_SLEEP_MS
> 
>  drivers/pci/controller/dwc/pcie-designware.c  | 13 ++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h  | 13 +++++++++----
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c |  1 +
>  drivers/pci/controller/dwc/pcie-qcom.c        |  1 +
>  drivers/pci/controller/pcie-rockchip-host.c   |  2 +-
>  drivers/pci/controller/plda/pcie-starfive.c   |  2 +-
>  drivers/pci/pci.h                             |  9 +--------
>  7 files changed, 26 insertions(+), 15 deletions(-)
> 
> -- 
> 2.49.0
> 

Gentle ping