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

[PATCH v4 0/7] 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 3/7 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 v3:
-Move LINK_WAIT_MAX_RETRIES and LINK_WAIT_SLEEP_MS to pci.h (Mani)
-Only wait PCIE_RESET_CONFIG_WAIT_MS if > 5 GT/s (Mani)
-Fix nit in commit log (Mani)


Niklas Cassel (7):
  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: Move link up wait time and max retries macros to pci.h
  PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS

 drivers/pci/controller/dwc/pcie-designware.c  | 20 +++++++++++++++----
 drivers/pci/controller/dwc/pcie-designware.h  |  4 ----
 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                             | 18 +++++++++--------
 7 files changed, 30 insertions(+), 18 deletions(-)

-- 
2.49.0

[PATCH v4 1/7] 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 v4 2/7] PCI: rockchip-host: Use macro PCIE_RESET_CONFIG_WAIT_MS

[Niklas Cassel]

Macro PCIE_RESET_CONFIG_DEVICE_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

[PATCH v4 3/7] 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 v4 4/7] 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

[PATCH v4 5/7] 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 | 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);
+
 	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
 
-- 
2.49.0

[PATCH v4 6/7] PCI: Move link up wait time and max retries macros to pci.h

[Niklas Cassel]

Move the LINK_WAIT_SLEEP_MS and LINK_WAIT_MAX_RETRIES macros to pci.h.
Prefix the macros with PCIE_ in order to avoid redefining these for
drivers that already have macros named like this.

No functional changes.

Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware.c | 6 +++---
 drivers/pci/controller/dwc/pcie-designware.h | 4 ----
 drivers/pci/pci.h                            | 4 ++++
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 053e9c540439..89aad5a08928 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -702,14 +702,14 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 	int retries;
 
 	/* Check if the link is up or not */
-	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
+	for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++) {
 		if (dw_pcie_link_up(pci))
 			break;
 
-		msleep(LINK_WAIT_SLEEP_MS);
+		msleep(PCIE_LINK_WAIT_SLEEP_MS);
 	}
 
-	if (retries >= LINK_WAIT_MAX_RETRIES) {
+	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
 		dev_info(pci->dev, "Phy link never came up\n");
 		return -ETIMEDOUT;
 	}
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ce9e18554e42..1bf1e08ab4c3 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -62,10 +62,6 @@
 #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 the waiting for iATU enabled routine */
 #define LINK_WAIT_MAX_IATU_RETRIES	5
 #define LINK_WAIT_IATU			9
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 819833e57590..43cb77c27ac0 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -56,6 +56,10 @@ struct pcie_tlp_log;
  */
 #define PCIE_RESET_CONFIG_WAIT_MS	100
 
+/* Parameters for the waiting for link up routine */
+#define PCIE_LINK_WAIT_MAX_RETRIES	10
+#define PCIE_LINK_WAIT_SLEEP_MS		90
+
 /* Message Routing (r[2:0]); PCIe r6.0, sec 2.2.8 */
 #define PCIE_MSG_TYPE_R_RC	0
 #define PCIE_MSG_TYPE_R_ADDR	1
-- 
2.49.0

[PATCH v4 7/7] PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS

[Niklas Cassel]

There is no reason for the delay, in each loop iteration, while polling for
link up (PCIE_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 (PCIE_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/pci.h                            | 11 ++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 89aad5a08928..d7278f6b84c1 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 < PCIE_LINK_WAIT_MAX_RETRIES; retries++) {
 		if (dw_pcie_link_up(pci))
 			break;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 43cb77c27ac0..9d20f0222fb1 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -56,9 +56,14 @@ struct pcie_tlp_log;
  */
 #define PCIE_RESET_CONFIG_WAIT_MS	100
 
-/* Parameters for the waiting for link up routine */
-#define PCIE_LINK_WAIT_MAX_RETRIES	10
-#define PCIE_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 PCIE_LINK_WAIT_MAX_RETRIES	100
+#define PCIE_LINK_WAIT_SLEEP_MS		10
 
 /* Message Routing (r[2:0]); PCIe r6.0, sec 2.2.8 */
 #define PCIE_MSG_TYPE_R_RC	0
-- 
2.49.0

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

[Manivannan Sadhasivam]

On Wed, 25 Jun 2025 12:23:46 +0200, Niklas Cassel wrote:
> 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 3/7 addresses a regression where a Plextor
> NVMe drive fails to be configured correctly. With this series, the
> Plextor NVMe drive works once again.
> 
> [...]

Applied, thanks!

[1/7] PCI: Rename PCIE_RESET_CONFIG_DEVICE_WAIT_MS to PCIE_RESET_CONFIG_WAIT_MS
      commit: 817f989700fddefa56e5e443e7d138018ca6709d
[2/7] PCI: rockchip-host: Use macro PCIE_RESET_CONFIG_WAIT_MS
      commit: bbc6a829ad3f054181d24a56944f944002e68898
[3/7] PCI: dw-rockchip: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ
      commit: c7eb9c5e1498882951b7583c56add0b77bfc162e
[4/7] PCI: qcom: Wait PCIE_RESET_CONFIG_WAIT_MS after link-up IRQ
      commit: 15b6b243cc2b1017cf89e2477aa0b4e1a306a82a
[5/7] PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link up
      commit: 80dc18a0cba8dea42614f021b20a04354b213d86
[6/7] PCI: Move link up wait time and max retries macros to pci.h
      commit: d7467bc72ce4e3f64062017d6c9ae3816e8a7b0e
[7/7] PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS
      commit: 470f10f18b482b3d46429c9e6723ff0f7854d049

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>

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

[Bjorn Helgaas]

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?

>  	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 v4 5/7] PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link up

[Niklas Cassel]

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

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

[Manivannan Sadhasivam]

On Tue, Jul 01, 2025 at 01:55:43PM GMT, Niklas Cassel wrote:
> 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.
> 

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.

- Mani

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

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

[Bjorn Helgaas]

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

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

[Niklas Cassel]

On Tue, Jul 01, 2025 at 11:38:44AM -0500, Bjorn Helgaas wrote:
> > 
> > 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.

Currently, there is not much consistency between the glue drivers, so
adding a DWC core API to assert/deassert PERST# sounds like a good idea
to me. The callback could even be supplied a struct gpio_desc pointer.

Like I mentioned in my previous email:
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

these drivers seem to have a 100 ms delay after PERST# has been deasserted,
but there are of course more glue drivers, so a lot of them will not have a
100 ms wait _after_ PERST# is deasserted. (All glue drivers seem to have a
delay between asserting and deasserting PERST#.)

Right now, e.g. qcom will have a 100 ms delay both after deasserting PERST#
and after link up. (However, based on the supported link speed, only one of
the delays should be needed.)

However, my main concern is not that qcom waits twice, it is those drivers
that do not have a 100 ms delay after PERST# has been deasserted, because
before commit 470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS"), those
drivers might have been "saved" by the ridiculously long
PCIE_LINK_WAIT_SLEEP_MS.

However, now when we sleep less in each iteration when polling for link up,
those drivers that do not have a 100 ms delay after PERST# has been
deasserted might actually see regressions, because (the now reduced)
PCIE_LINK_WAIT_SLEEP_MS time is no longer "saving" them.

If we don't want to make the PCIE_RESET_CONFIG_WAIT_MS wait unconditional
(not care about the supported link speed), then perhaps we should drop
470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS") from pci/next ?


Kind regards,
Niklas

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

[Manivannan Sadhasivam]

On Wed, Jul 02, 2025 at 11:43:11AM GMT, Niklas Cassel wrote:
> On Tue, Jul 01, 2025 at 11:38:44AM -0500, Bjorn Helgaas wrote:
> > > 
> > > 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.
> 
> Currently, there is not much consistency between the glue drivers, so
> adding a DWC core API to assert/deassert PERST# sounds like a good idea
> to me. The callback could even be supplied a struct gpio_desc pointer.
> 

+1 for consolidating the PERST# handling. But just FYI, I'm going to move the
PERST# deassert handling from controller drivers to pwrctrl drivers as once
pwrctrl drivers are used to control the power supplies, they should control
PERST# as well. Currently, this is the missing piece for pwrctrl framework.

Also, we cannot entirely get rid of the PERST# handling in controller drivers,
since they need to assert PERST# before resetting the RC during init.

> Like I mentioned in my previous email:
> 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
> 
> these drivers seem to have a 100 ms delay after PERST# has been deasserted,
> but there are of course more glue drivers, so a lot of them will not have a
> 100 ms wait _after_ PERST# is deasserted. (All glue drivers seem to have a
> delay between asserting and deasserting PERST#.)
> 
> Right now, e.g. qcom will have a 100 ms delay both after deasserting PERST#
> and after link up. (However, based on the supported link speed, only one of
> the delays should be needed.)
> 
> However, my main concern is not that qcom waits twice, it is those drivers
> that do not have a 100 ms delay after PERST# has been deasserted, because
> before commit 470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS"), those
> drivers might have been "saved" by the ridiculously long
> PCIE_LINK_WAIT_SLEEP_MS.
> 
> However, now when we sleep less in each iteration when polling for link up,
> those drivers that do not have a 100 ms delay after PERST# has been
> deasserted might actually see regressions, because (the now reduced)
> PCIE_LINK_WAIT_SLEEP_MS time is no longer "saving" them.
> 

Why can't you just add the delay to those drivers now? They can be consolidated
later.

- Mani

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

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

[Niklas Cassel]

Hello Mani,

On Wed, Jul 02, 2025 at 08:17:44PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 02, 2025 at 11:43:11AM GMT, Niklas Cassel wrote:
> > However, my main concern is not that qcom waits twice, it is those drivers
> > that do not have a 100 ms delay after PERST# has been deasserted, because
> > before commit 470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS"), those
> > drivers might have been "saved" by the ridiculously long
> > PCIE_LINK_WAIT_SLEEP_MS.
> > 
> > However, now when we sleep less in each iteration when polling for link up,
> > those drivers that do not have a 100 ms delay after PERST# has been
> > deasserted might actually see regressions, because (the now reduced)
> > PCIE_LINK_WAIT_SLEEP_MS time is no longer "saving" them.
>
> 
> Why can't you just add the delay to those drivers now? They can be consolidated
> later.

Right now, I don't have the extra cycles needed to read all of these
drivers:

$ git grep -l gpio drivers/pci/controller/dwc/
drivers/pci/controller/dwc/pci-dra7xx.c
drivers/pci/controller/dwc/pci-imx6.c
drivers/pci/controller/dwc/pci-keystone.c
drivers/pci/controller/dwc/pci-meson.c
drivers/pci/controller/dwc/pcie-amd-mdb.c
drivers/pci/controller/dwc/pcie-bt1.c
drivers/pci/controller/dwc/pcie-designware-plat.c
drivers/pci/controller/dwc/pcie-designware.c
drivers/pci/controller/dwc/pcie-designware.h
drivers/pci/controller/dwc/pcie-dw-rockchip.c
drivers/pci/controller/dwc/pcie-fu740.c
drivers/pci/controller/dwc/pcie-histb.c
drivers/pci/controller/dwc/pcie-intel-gw.c
drivers/pci/controller/dwc/pcie-keembay.c
drivers/pci/controller/dwc/pcie-kirin.c
drivers/pci/controller/dwc/pcie-qcom-ep.c
drivers/pci/controller/dwc/pcie-qcom.c
drivers/pci/controller/dwc/pcie-rcar-gen4.c
drivers/pci/controller/dwc/pcie-tegra194.c
drivers/pci/controller/dwc/pcie-visconti.c

then understanding them properly to feel that I am confident in my changes,
waiting for reviews from each glue driver maintainer, and then waiting for
someone to pick it up.

Also, all of the above has to be done when consolidating the PERST#
handling anyway.



This whole thing started because someone reported a regression on a random
Plextor NVMe drive. Since it was my commit ec9fd499b9c6 ("PCI: dw-rockchip:
Don't wait for link since we can detect Link Up") that introduced the
regression, I obviously helped debugging the issue.

The regression was solved by adding a 100 ms delay after receiving the link
up IRQ, before enumerating the bus.

This fix was sent May 5th:
https://lore.kernel.org/linux-pci/20250505092603.286623-7-cassel@kernel.org/
(This series had up to v2, the series was then renamed and had up to v4,
so basically v6 in total.)

While my responsibility was done two months ago, I still wanted to make
sure that no one else got bit by the same bug, thus I also improved the
generic dw_pcie_wait_for_link() (for those drivers not using link up IRQ):
https://lore.kernel.org/linux-pci/20250625102347.1205584-14-cassel@kernel.org/

Sure, that will only help PCIe controllers that support Link speeds greater
than 5.0 GT/s, and do not use a link up IRQ, but still something.


PCIe controllers that only support Link speeds <= 5.0 GT/s, and do not use
a link up IRQ, and do not have a delay after deasserting PERST#, can still
send config requests too early.

However, if we drop 470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS"),
PCIe controllers that only support Link speeds <= 5.0 GT/s, and do not use
a link up IRQ, and do not have a delay after deasserting PERST#, will be no
worse off than what is already in mainline.

PCIe controllers that support Link speeds greater than 5.0 GT/s, and do not
use a link up IRQ, will still be more robust than ever :)
(Rome wasn't built in a day.)


Kind regards,
Niklas

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

[Manivannan Sadhasivam]

On Wed, Jul 02, 2025 at 06:17:41PM GMT, Niklas Cassel wrote:
> Hello Mani,
> 
> On Wed, Jul 02, 2025 at 08:17:44PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jul 02, 2025 at 11:43:11AM GMT, Niklas Cassel wrote:
> > > However, my main concern is not that qcom waits twice, it is those drivers
> > > that do not have a 100 ms delay after PERST# has been deasserted, because
> > > before commit 470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS"), those
> > > drivers might have been "saved" by the ridiculously long
> > > PCIE_LINK_WAIT_SLEEP_MS.
> > > 
> > > However, now when we sleep less in each iteration when polling for link up,
> > > those drivers that do not have a 100 ms delay after PERST# has been
> > > deasserted might actually see regressions, because (the now reduced)
> > > PCIE_LINK_WAIT_SLEEP_MS time is no longer "saving" them.
> >
> > 
> > Why can't you just add the delay to those drivers now? They can be consolidated
> > later.
> 
> Right now, I don't have the extra cycles needed to read all of these
> drivers:
> 
> $ git grep -l gpio drivers/pci/controller/dwc/
> drivers/pci/controller/dwc/pci-dra7xx.c
> drivers/pci/controller/dwc/pci-imx6.c
> drivers/pci/controller/dwc/pci-keystone.c
> drivers/pci/controller/dwc/pci-meson.c
> drivers/pci/controller/dwc/pcie-amd-mdb.c
> drivers/pci/controller/dwc/pcie-bt1.c
> drivers/pci/controller/dwc/pcie-designware-plat.c
> drivers/pci/controller/dwc/pcie-designware.c
> drivers/pci/controller/dwc/pcie-designware.h
> drivers/pci/controller/dwc/pcie-dw-rockchip.c
> drivers/pci/controller/dwc/pcie-fu740.c
> drivers/pci/controller/dwc/pcie-histb.c
> drivers/pci/controller/dwc/pcie-intel-gw.c
> drivers/pci/controller/dwc/pcie-keembay.c
> drivers/pci/controller/dwc/pcie-kirin.c
> drivers/pci/controller/dwc/pcie-qcom-ep.c
> drivers/pci/controller/dwc/pcie-qcom.c
> drivers/pci/controller/dwc/pcie-rcar-gen4.c
> drivers/pci/controller/dwc/pcie-tegra194.c
> drivers/pci/controller/dwc/pcie-visconti.c
> 
> then understanding them properly to feel that I am confident in my changes,
> waiting for reviews from each glue driver maintainer, and then waiting for
> someone to pick it up.
> 
> Also, all of the above has to be done when consolidating the PERST#
> handling anyway.
> 
> 
> 
> This whole thing started because someone reported a regression on a random
> Plextor NVMe drive. Since it was my commit ec9fd499b9c6 ("PCI: dw-rockchip:
> Don't wait for link since we can detect Link Up") that introduced the
> regression, I obviously helped debugging the issue.
> 
> The regression was solved by adding a 100 ms delay after receiving the link
> up IRQ, before enumerating the bus.
> 
> This fix was sent May 5th:
> https://lore.kernel.org/linux-pci/20250505092603.286623-7-cassel@kernel.org/
> (This series had up to v2, the series was then renamed and had up to v4,
> so basically v6 in total.)
> 
> While my responsibility was done two months ago, I still wanted to make
> sure that no one else got bit by the same bug, thus I also improved the
> generic dw_pcie_wait_for_link() (for those drivers not using link up IRQ):
> https://lore.kernel.org/linux-pci/20250625102347.1205584-14-cassel@kernel.org/
> 
> Sure, that will only help PCIe controllers that support Link speeds greater
> than 5.0 GT/s, and do not use a link up IRQ, but still something.
> 
> 
> PCIe controllers that only support Link speeds <= 5.0 GT/s, and do not use
> a link up IRQ, and do not have a delay after deasserting PERST#, can still
> send config requests too early.
> 
> However, if we drop 470f10f18b48 ("PCI: Reduce PCIE_LINK_WAIT_SLEEP_MS"),
> PCIe controllers that only support Link speeds <= 5.0 GT/s, and do not use
> a link up IRQ, and do not have a delay after deasserting PERST#, will be no
> worse off than what is already in mainline.
> 
> PCIe controllers that support Link speeds greater than 5.0 GT/s, and do not
> use a link up IRQ, will still be more robust than ever :)
> (Rome wasn't built in a day.)
> 

Sounds fair! I've now dropped 470f10f18b48 from controller/linkup-fix branch.

Do you have cycles for consolidating PERST# handling?

- Mani

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

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

[Niklas Cassel]

Hello Mani,

On Mon, Jul 07, 2025 at 01:18:49PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 02, 2025 at 06:17:41PM GMT, Niklas Cassel wrote:
> 
> Sounds fair! I've now dropped 470f10f18b48 from controller/linkup-fix branch.
> 
> Do you have cycles for consolidating PERST# handling?

While I think that consolidating PERST# handling is a great idea, I already
have 3-4 things with higher priority on my TODO list (and some things have
been there for quite a while).

So unless something has rather high priority, I currently don't know when
I will have some spare cycles.

I really hope that someone will have some spare cycles to work on this,
because the PERST# handling in the controller drivers really is a mess.


Kind regards,
Niklas

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

[Bjorn Helgaas]

On Wed, Jul 02, 2025 at 08:17:44PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 02, 2025 at 11:43:11AM GMT, Niklas Cassel wrote:
> > On Tue, Jul 01, 2025 at 11:38:44AM -0500, Bjorn Helgaas wrote:
> > > > 
> > > > 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.
> > 
> > Currently, there is not much consistency between the glue drivers,
> > so adding a DWC core API to assert/deassert PERST# sounds like a
> > good idea to me. The callback could even be supplied a struct
> > gpio_desc pointer.
> 
> +1 for consolidating the PERST# handling. But just FYI, I'm going to
> move the PERST# deassert handling from controller drivers to pwrctrl
> drivers as once pwrctrl drivers are used to control the power
> supplies, they should control PERST# as well. Currently, this is the
> missing piece for pwrctrl framework.
> 
> Also, we cannot entirely get rid of the PERST# handling in
> controller drivers, since they need to assert PERST# before
> resetting the RC during init.

I thought pwrctrl was optional.  The PERST# management is not
optional, is it?

Bjorn

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

[Manivannan Sadhasivam]

On Mon, Jul 07, 2025 at 01:56:43PM GMT, Niklas Cassel wrote:
> Hello Mani,
> 
> On Mon, Jul 07, 2025 at 01:18:49PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jul 02, 2025 at 06:17:41PM GMT, Niklas Cassel wrote:
> > 
> > Sounds fair! I've now dropped 470f10f18b48 from controller/linkup-fix branch.
> > 
> > Do you have cycles for consolidating PERST# handling?
> 
> While I think that consolidating PERST# handling is a great idea, I already
> have 3-4 things with higher priority on my TODO list (and some things have
> been there for quite a while).
> 
> So unless something has rather high priority, I currently don't know when
> I will have some spare cycles.
> 
> I really hope that someone will have some spare cycles to work on this,
> because the PERST# handling in the controller drivers really is a mess.
> 

Yeah, I agree. Then I'll add it to my long list of TODO items :)

- Mani

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