PCI: dwc: Do not enumerate bus before endpoint devices are ready

PATCH Superseded Niklas Cassel 2 participants 15 messages Jun 13, 2025 – Jun 25, 2025

Subsystems (8)

Arm Qualcomm mailing list PCI Arm Rockchip PCI native host bridge and endpoint drivers +4 more subsystems

lore

Share & tools

lore

Niklas Cassel

author

2025-06-13 12:49 UTC #0 lore raw

v3 cover PCI: dwc: Do not enumerate bus before endpoint devices are ready

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(-)

Signature · 1 line

2.49.0

Niklas Cassel

author

2025-06-13 12:49 UTC #1 lore raw

PATCH v3 1/6 PCI: Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS

Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS.

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/plda/pcie-starfive.c | 2 +-
 drivers/pci/pci.h                           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/pci/controller/plda/pcie-starfive.c +1 -1

index e73c1b7bc8ef..3caf53c6c082 100644
@@ -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");

drivers/pci/pci.h +1 -1

index 12215ee72afb..98d6fccb383e 100644
@@ -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

Signature · 1 line

2.49.0

Niklas Cassel

author

2025-06-13 12:49 UTC #2 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/pcie-rockchip-host.c | 2 +-
 drivers/pci/pci.h                           | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

drivers/pci/controller/pcie-rockchip-host.c +1 -1

index b9e7a8710cf0..c11ed45c25f6 100644
@@ -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,

drivers/pci/pci.h +0 -7

index 98d6fccb383e..819833e57590 100644
@@ -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.

Signature · 1 line

2.49.0

Manivannan Sadhasivam

maintainer

2025-06-23 14:25 UTC #3 lore raw

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

Quoted 1 line · Niklas Cassel

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

Quoted 8 lines

("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.

Quoted 7 lines

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 <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>

LGTM!

- Mani

Quoted 39 lines

---
 drivers/pci/controller/pcie-rockchip-host.c | 2 +-
 drivers/pci/pci.h                           | 7 -------
 2 files changed, 1 insertion(+), 8 deletions(-)

drivers/pci/controller/pcie-rockchip-host.c +1 -1

index b9e7a8710cf0..c11ed45c25f6 100644
@@ -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,

drivers/pci/pci.h +0 -7

index 98d6fccb383e..819833e57590 100644
@@ -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.

Signature · 1 line

2.49.0

Signature · 1 line

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

Niklas Cassel

author

2025-06-13 12:49 UTC #4 lore raw

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

In reply to 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 <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Reviewed-by Wilfred Mallawa <[email protected]>

Fixes 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/controller/dwc/pcie-dw-rockchip.c +1 -0

index 93171a392879..108d30637920 100644
@@ -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();

Signature · 1 line

2.49.0

Niklas Cassel

author

2025-06-13 12:49 UTC #5 lore raw

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

In reply to 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 <[email protected]>

Reviewed-by Wilfred Mallawa <[email protected]>

Fixes 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-qcom.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/controller/dwc/pcie-qcom.c +1 -0

index c789e3f85655..9b12f2f02042 100644
@@ -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();

Signature · 1 line

2.49.0

Manivannan Sadhasivam

maintainer

2025-06-23 14:27 UTC #6 lore raw

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

Quoted 14 lines · 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.

Quoted 6 lines

Delay PCIE_RESET_CONFIG_WAIT_MS after the link-up IRQ before starting enumeration.
Reviewed-by Damien Le Moal <[email protected]>
Reviewed-by Wilfred Mallawa <[email protected]>
Fixes 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")

Shouldn't 36971d6c5a9a be the fixes commit?

- Mani

Quoted 20 lines

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-qcom.c | 1 +
 1 file changed, 1 insertion(+)

drivers/pci/controller/dwc/pcie-qcom.c +1 -0

index c789e3f85655..9b12f2f02042 100644
@@ -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();

Signature · 1 line

2.49.0

Signature · 1 line

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

Niklas Cassel

author

2025-06-25 09:06 UTC #7 lore raw

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

Quoted 15 lines · Manivannan Sadhasivam

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 6 lines
Delay PCIE_RESET_CONFIG_WAIT_MS after the link-up IRQ before starting enumeration.
Reviewed-by Damien Le Moal <[email protected]>
Reviewed-by Wilfred Mallawa <[email protected]>
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

Niklas Cassel

author

2025-06-13 12:49 UTC #8 lore raw

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

In reply to 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 <[email protected]>

Reviewed-by Wilfred Mallawa <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/pci/controller/dwc/pcie-designware.c +7 -0

index 4d794964fa0f..24903f67d724 100644
@@ -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);

Signature · 1 line

2.49.0

Manivannan Sadhasivam

maintainer

2025-06-23 14:28 UTC #9 lore raw

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

Quoted 24 lines · 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 <[email protected]>
Reviewed-by Wilfred Mallawa <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/controller/dwc/pcie-designware.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/pci/controller/dwc/pcie-designware.c +0 -0
index 4d794964fa0f..24903f67d724 100644

drivers/pci/controller/dwc/pcie-designware.c int dw_pcie_wait_for_link(struct dw_pcie *pci) · line 721

Quoted 8 lines

drivers/pci/controller/dwc/pcie-designware.c +5 -0

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

Signature · 1 line

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

Niklas Cassel

author

2025-06-25 09:20 UTC #10 lore raw

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

Quoted 37 lines · Manivannan Sadhasivam

Quoted 24 lines · 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 <[email protected]>
Reviewed-by Wilfred Mallawa <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/controller/dwc/pcie-designware.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/pci/controller/dwc/pcie-designware.c +0 -0
index 4d794964fa0f..24903f67d724 100644
drivers/pci/controller/dwc/pcie-designware.c int dw_pcie_wait_for_link(struct dw_pcie *pci) · line 721
Quoted 8 lines
drivers/pci/controller/dwc/pcie-designware.c +5 -0
@@ -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:

Quoted 2 lines

	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

Niklas Cassel

author

2025-06-13 12:49 UTC #11 lore raw

PATCH v3 6/6 PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

In reply to 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 <[email protected]>

Reviewed-by Wilfred Mallawa <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware.c |  6 +++++-
 drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

drivers/pci/controller/dwc/pcie-designware.c +5 -1

index 24903f67d724..ae6f0bfe3c56 100644
@@ -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;

drivers/pci/controller/dwc/pcie-designware.h +9 -4

index ce9e18554e42..b225c4f3d36a 100644
@@ -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

Signature · 1 line

2.49.0

Manivannan Sadhasivam

maintainer

2025-06-23 14:52 UTC #12 lore raw

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

Quoted 45 lines · 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 <[email protected]>
Reviewed-by Wilfred Mallawa <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/controller/dwc/pcie-designware.c |  6 +++++-
 drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)
drivers/pci/controller/dwc/pcie-designware.c +5 -1
index 24903f67d724..ae6f0bfe3c56 100644
@@ -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;
drivers/pci/controller/dwc/pcie-designware.h +0 -1
index ce9e18554e42..b225c4f3d36a 100644
@@ -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 */

drivers/pci/controller/dwc/pcie-designware.h line 72

Quoted 10 lines

drivers/pci/controller/dwc/pcie-designware.h +8 -2

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

Signature · 1 line

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

Niklas Cassel

author

2025-06-25 09:02 UTC #13 lore raw

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

Quoted 45 lines · Manivannan Sadhasivam

Quoted 44 lines · 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 <[email protected]>
Reviewed-by Wilfred Mallawa <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/pci/controller/dwc/pcie-designware.c |  6 +++++-
 drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)
drivers/pci/controller/dwc/pcie-designware.c +5 -1
index 24903f67d724..ae6f0bfe3c56 100644
@@ -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;
drivers/pci/controller/dwc/pcie-designware.h +0 -0
index ce9e18554e42..b225c4f3d36a 100644
@@ -62,11 +62,16 @@
 #define dw_pcie_cap_set(_pci, _cap) \
 	set_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
 

Quoted 13 lines

drivers/pci/controller/dwc/pcie-designware.h line 72

Quoted 11 lines

drivers/pci/controller/dwc/pcie-designware.h +8 -3

-/* 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

Niklas Cassel

author

2025-06-23 10:12 UTC #14 lore raw

In reply to Niklas Cassel

Quoted 29 lines · 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

Quoted 12 lines

 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(-)

Signature · 1 line

2.49.0

Gentle ping