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

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


Niklas Cassel (4):
  PCI: dw-rockchip: Do not enumerate bus before endpoint devices are
    ready
  PCI: qcom: Do not enumerate bus before endpoint devices are ready
  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  | 11 ++++++++---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c |  7 +++++++
 drivers/pci/controller/dwc/pcie-qcom.c        |  7 +++++++
 4 files changed, 34 insertions(+), 4 deletions(-)

-- 
2.49.0

[PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Niklas Cassel]

Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
and instead enumerate the bus directly after receiving the Link Up IRQ.

This means that there is no longer any delay between link up and the bus
getting enumerated.

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 the threaded link up IRQ handler in order to satisfy
the requirements of the PCIe spec.

Laszlo Fiat reported (off-list) that his PLEXTOR PX-256M8PeGN NVMe SSD is
no longer functional, and simply reverting commit ec9fd499b9c6 ("PCI:
dw-rockchip: Don't wait for link since we can detect Link Up") makes his
SSD functional again. Adding the 100 ms delay as required by the spec also
makes the SSD functional again.

Cc: Laszlo Fiat <laszlo.fiat@proton.me>
Fixes: ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 93171a392879..a941a239cbfc 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -459,6 +459,13 @@ 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)) {
 			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
+			/*
+			 * 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_T_RRS_READY_MS);
 			/* Rescan the bus to enumerate endpoint devices */
 			pci_lock_rescan_remove();
 			pci_rescan_bus(pp->bridge->bus);
-- 
2.49.0

[PATCH 2/4] PCI: qcom: Do not enumerate bus before endpoint devices are ready

[Niklas Cassel]

Commit 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect Link
Up") changed so that we no longer call dw_pcie_wait_for_link(), and instead
enumerate the bus directly after receiving the Link Up IRQ.

This means that there is no longer any delay between link up and the bus
getting enumerated.

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 the threaded link up IRQ handler in order to satisfy
the requirements of the PCIe spec.

Fixes: 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect Link Up")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f85655..0a627f3b5e2c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1565,6 +1565,13 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 
 	if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) {
 		dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
+		/*
+		 * 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_T_RRS_READY_MS);
 		/* Rescan the bus to enumerate endpoint devices */
 		pci_lock_rescan_remove();
 		pci_rescan_bus(pp->bridge->bus);
-- 
2.49.0

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

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..dbb21a9c93d7 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_T_RRS_READY_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 4/4] 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).

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware.c |  6 +++++-
 drivers/pci/controller/dwc/pcie-designware.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 dbb21a9c93d7..8ef1e42b7168 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..52daf9525bae 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -62,9 +62,14 @@
 #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 link up routine. 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 */
 #define LINK_WAIT_MAX_IATU_RETRIES	5
-- 
2.49.0

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Damien Le Moal]

On 6/11/25 19:51, Niklas Cassel wrote:
> Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> and instead enumerate the bus directly after receiving the Link Up IRQ.
> 
> This means that there is no longer any delay between link up and the bus
> getting enumerated.
> 
> 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 the threaded link up IRQ handler in order to satisfy
> the requirements of the PCIe spec.
> 
> Laszlo Fiat reported (off-list) that his PLEXTOR PX-256M8PeGN NVMe SSD is
> no longer functional, and simply reverting commit ec9fd499b9c6 ("PCI:
> dw-rockchip: Don't wait for link since we can detect Link Up") makes his
> SSD functional again. Adding the 100 ms delay as required by the spec also
> makes the SSD functional again.
> 
> Cc: Laszlo Fiat <laszlo.fiat@proton.me>
> Fixes: ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 93171a392879..a941a239cbfc 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -459,6 +459,13 @@ 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)) {
>  			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");

Should maybe this message be moved after the sleep ?

> +			/*
> +			 * 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_T_RRS_READY_MS);
>  			/* Rescan the bus to enumerate endpoint devices */
>  			pci_lock_rescan_remove();
>  			pci_rescan_bus(pp->bridge->bus);

Other than that, looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] PCI: qcom: Do not enumerate bus before endpoint devices are ready

[Damien Le Moal]

On 6/11/25 19:51, Niklas Cassel wrote:
> Commit 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect Link
> Up") changed so that we no longer call dw_pcie_wait_for_link(), and instead
> enumerate the bus directly after receiving the Link Up IRQ.
> 
> This means that there is no longer any delay between link up and the bus
> getting enumerated.
> 
> 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 the threaded link up IRQ handler in order to satisfy
> the requirements of the PCIe spec.
> 
> Fixes: 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect Link Up")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index c789e3f85655..0a627f3b5e2c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1565,6 +1565,13 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  
>  	if (FIELD_GET(PARF_INT_ALL_LINK_UP, status)) {
>  		dev_dbg(dev, "Received Link up event. Starting enumeration!\n");

Same comment here as for the dw-rockchip. Sleep before printing the message ?

> +		/*
> +		 * 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_T_RRS_READY_MS);
>  		/* Rescan the bus to enumerate endpoint devices */
>  		pci_lock_rescan_remove();
>  		pci_rescan_bus(pp->bridge->bus);

Either way,

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] PCI: dwc: Ensure that dw_pcie_wait_for_link() waits 100 ms after link up

[Damien Le Moal]

On 6/11/25 19:51, 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.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

[Damien Le Moal]

On 6/11/25 19:51, 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).
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c |  6 +++++-
>  drivers/pci/controller/dwc/pcie-designware.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 dbb21a9c93d7..8ef1e42b7168 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..52daf9525bae 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -62,9 +62,14 @@
>  #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 link up routine. As per PCIe r6.0, sec 6.6.1,

s/for the/for

and maybe reword this to something like:

* Parameters for waiting for a link to be established.

> + * 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 */
>  #define LINK_WAIT_MAX_IATU_RETRIES	5

Other than that, looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] PCI: dwc: Reduce LINK_WAIT_SLEEP_MS

[Niklas Cassel]

On Wed, Jun 11, 2025 at 09:38:31PM +0900, Damien Le Moal wrote:

(snip)

> > -/* Parameters for the waiting for link up routine */
> > + * Parameters for the waiting for link up routine. As per PCIe r6.0, sec 6.6.1,
> 
> s/for the/for
> 
> and maybe reword this to something like:
> 
> * Parameters for waiting for a link to be established.

This sentence was there before I appended to the comment.
But sure, can fix it in V2.


Kind regards,
Niklas

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Bjorn Helgaas]

On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> and instead enumerate the bus directly after receiving the Link Up IRQ.
> 
> This means that there is no longer any delay between link up and the bus
> getting enumerated.

Minor quibble about "no longer any delay": dw_pcie_wait_for_link()
doesn't contain any explicit delay *after* we notice the link is up,
so we didn't guarantee sufficient delay even before ec9fd499b9c6.

If the link came up before the first check, dw_pcie_wait_for_link()
didn't delay at all.  Otherwise, it delayed 90ms * N, and we have no
idea when in the 90ms period the link came up, so the post link-up
delay was effectively some random amount between 0 and 90ms.

I would propose something like:

  PCI: dw-rockchip: Wait PCIE_T_RRS_READY_MS after link-up IRQ

  Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
  100ms (PCIE_T_RRS_READY_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_T_RRS_READY_MS after the link-up IRQ before starting
  enumeration.

> 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 the threaded link up IRQ handler in order to satisfy
> the requirements of the PCIe spec.
> 
> Laszlo Fiat reported (off-list) that his PLEXTOR PX-256M8PeGN NVMe SSD is
> no longer functional, and simply reverting commit ec9fd499b9c6 ("PCI:
> dw-rockchip: Don't wait for link since we can detect Link Up") makes his
> SSD functional again. Adding the 100 ms delay as required by the spec also
> makes the SSD functional again.
> 
> Cc: Laszlo Fiat <laszlo.fiat@proton.me>
> Fixes: ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")

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.

> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 93171a392879..a941a239cbfc 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -459,6 +459,13 @@ 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)) {
>  			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
> +			/*
> +			 * 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.
> +			 */

I think the comment at the PCIE_T_RRS_READY_MS definition should be
enough (although it might need to be updated to mention link-up).
This delay is going to be a standard piece of every driver, so it
won't require special notice.

> +			msleep(PCIE_T_RRS_READY_MS);
>  			/* Rescan the bus to enumerate endpoint devices */
>  			pci_lock_rescan_remove();
>  			pci_rescan_bus(pp->bridge->bus);
> -- 
> 2.49.0
> 

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Niklas Cassel]

On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > 
> > This means that there is no longer any delay between link up and the bus
> > getting enumerated.
> 
> Minor quibble about "no longer any delay": dw_pcie_wait_for_link()
> doesn't contain any explicit delay *after* we notice the link is up,
> so we didn't guarantee sufficient delay even before ec9fd499b9c6.
> 
> If the link came up before the first check, dw_pcie_wait_for_link()
> didn't delay at all.  Otherwise, it delayed 90ms * N, and we have no
> idea when in the 90ms period the link came up, so the post link-up
> delay was effectively some random amount between 0 and 90ms.
> 
> I would propose something like:
> 
>   PCI: dw-rockchip: Wait PCIE_T_RRS_READY_MS after link-up IRQ
> 
>   Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
>   100ms (PCIE_T_RRS_READY_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_T_RRS_READY_MS after the link-up IRQ before starting
>   enumeration.

Ok, I will shamelessly use this text verbatim.



> I think the comment at the PCIE_T_RRS_READY_MS definition should be
> enough (although it might need to be updated to mention link-up).
> This delay is going to be a standard piece of every driver, so it
> won't require special notice.

Looking at pci.h, we already have a comment mentioning exactly this
(link-up):
https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63

I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.


Kind regards,
Niklas

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > 
> > > This means that there is no longer any delay between link up and the bus
> > > getting enumerated.

> > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > enough (although it might need to be updated to mention link-up).
> > This delay is going to be a standard piece of every driver, so it
> > won't require special notice.
> 
> Looking at pci.h, we already have a comment mentioning exactly this
> (link-up):
> https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> 
> I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.

I'll more closely later, but I think PCIE_T_RRS_READY_MS and
PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
exist.  It looks like they got merged at about the same time by
different people, so we didn't notice.

Bjorn

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Niklas Cassel]

On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > 
> > > > This means that there is no longer any delay between link up and the bus
> > > > getting enumerated.
> 
> > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > enough (although it might need to be updated to mention link-up).
> > > This delay is going to be a standard piece of every driver, so it
> > > won't require special notice.
> > 
> > Looking at pci.h, we already have a comment mentioning exactly this
> > (link-up):
> > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > 
> > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> 
> I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> exist.  It looks like they got merged at about the same time by
> different people, so we didn't notice.

I came to the same conclusion, I will send a patch to remove
PCIE_T_RRS_READY_MS and convert the only existing user to use
PCIE_RESET_CONFIG_DEVICE_WAIT_MS.


Kind regards,
Niklas

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > > 
> > > > > This means that there is no longer any delay between link up and the bus
> > > > > getting enumerated.
> > 
> > > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > > enough (although it might need to be updated to mention link-up).
> > > > This delay is going to be a standard piece of every driver, so it
> > > > won't require special notice.
> > > 
> > > Looking at pci.h, we already have a comment mentioning exactly this
> > > (link-up):
> > > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > > 
> > > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> > 
> > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > exist.  It looks like they got merged at about the same time by
> > different people, so we didn't notice.
> 
> I came to the same conclusion, I will send a patch to remove
> PCIE_T_RRS_READY_MS and convert the only existing user to use
> PCIE_RESET_CONFIG_DEVICE_WAIT_MS.

I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
specifically.  It's not that the device is completely ready after
100ms; just that it should be able to respond with RRS if it needs
more time.

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Manivannan Sadhasivam]

On Thu, Jun 12, 2025 at 07:21:08AM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> > On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > > > 
> > > > > > This means that there is no longer any delay between link up and the bus
> > > > > > getting enumerated.
> > > 
> > > > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > > > enough (although it might need to be updated to mention link-up).
> > > > > This delay is going to be a standard piece of every driver, so it
> > > > > won't require special notice.
> > > > 
> > > > Looking at pci.h, we already have a comment mentioning exactly this
> > > > (link-up):
> > > > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > > > 
> > > > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> > > 
> > > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > > exist.  It looks like they got merged at about the same time by
> > > different people, so we didn't notice.
> > 
> > I came to the same conclusion, I will send a patch to remove
> > PCIE_T_RRS_READY_MS and convert the only existing user to use
> > PCIE_RESET_CONFIG_DEVICE_WAIT_MS.
> 
> I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
> specifically.  It's not that the device is completely ready after
> 100ms; just that it should be able to respond with RRS if it needs
> more time.

Yes, but none of the drivers are checking for the RRS status currently. So using
PCIE_T_RRS_READY_MS gives a wrong impression that the driver is waiting for the
RRS status from the device.

- Mani

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

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 06:30:37PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 12, 2025 at 07:21:08AM -0500, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> > > On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > > > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > > > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > > > > 
> > > > > > > This means that there is no longer any delay between link up and the bus
> > > > > > > getting enumerated.
> > > > 
> > > > > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > > > > enough (although it might need to be updated to mention link-up).
> > > > > > This delay is going to be a standard piece of every driver, so it
> > > > > > won't require special notice.
> > > > > 
> > > > > Looking at pci.h, we already have a comment mentioning exactly this
> > > > > (link-up):
> > > > > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > > > > 
> > > > > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> > > > 
> > > > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > > > exist.  It looks like they got merged at about the same time by
> > > > different people, so we didn't notice.
> > > 
> > > I came to the same conclusion, I will send a patch to remove
> > > PCIE_T_RRS_READY_MS and convert the only existing user to use
> > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS.
> > 
> > I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
> > specifically.  It's not that the device is completely ready after
> > 100ms; just that it should be able to respond with RRS if it needs
> > more time.
> 
> Yes, but none of the drivers are checking for the RRS status
> currently. So using PCIE_T_RRS_READY_MS gives a wrong impression
> that the driver is waiting for the RRS status from the device.

There's 100ms immediately after reset or link-up when we can't send
config requests because the device may not be able to respond at all.

After 100ms, the device should be able to respond to config requests
with SC, UR, RRS, or CA status (sec 2.2.9.1).  If it responds with
RRS, the access should be retried either by hardware or (if RRS SV is
enabled) by software.  This is the origin of "RRS_READY" -- the device
can at least do RRS.

"CONFIG_READY" would make sense except that it would be confused with
the spec's usage of "Configuration Ready" (unfortunately not formally
defined).  The PCIe r6.0, sec 6.22 implementation note says devices
may take up to 1 second to become Configuration Ready, and that when a
device is Configuration Ready, system software can proceed without
further delay to configure the device.

"PCIE_RESET_CONFIG_DEVICE_WAIT_MS" seems a little long to me (we might
not need "DEVICE"), but it does include "CONFIG" which is definitely
relevant.  "PCIE_RESET_CONFIG_WAIT_MS"?

Bjorn

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Manivannan Sadhasivam]

On Thu, Jun 12, 2025 at 09:44:47AM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 12, 2025 at 06:30:37PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 12, 2025 at 07:21:08AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> > > > On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > > > > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > > > > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > > > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > > > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > > > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > > > > > 
> > > > > > > > This means that there is no longer any delay between link up and the bus
> > > > > > > > getting enumerated.
> > > > > 
> > > > > > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > > > > > enough (although it might need to be updated to mention link-up).
> > > > > > > This delay is going to be a standard piece of every driver, so it
> > > > > > > won't require special notice.
> > > > > > 
> > > > > > Looking at pci.h, we already have a comment mentioning exactly this
> > > > > > (link-up):
> > > > > > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > > > > > 
> > > > > > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> > > > > 
> > > > > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > > > > exist.  It looks like they got merged at about the same time by
> > > > > different people, so we didn't notice.
> > > > 
> > > > I came to the same conclusion, I will send a patch to remove
> > > > PCIE_T_RRS_READY_MS and convert the only existing user to use
> > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS.
> > > 
> > > I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
> > > specifically.  It's not that the device is completely ready after
> > > 100ms; just that it should be able to respond with RRS if it needs
> > > more time.
> > 
> > Yes, but none of the drivers are checking for the RRS status
> > currently. So using PCIE_T_RRS_READY_MS gives a wrong impression
> > that the driver is waiting for the RRS status from the device.
> 
> There's 100ms immediately after reset or link-up when we can't send
> config requests because the device may not be able to respond at all.
> 
> After 100ms, the device should be able to respond to config requests
> with SC, UR, RRS, or CA status (sec 2.2.9.1).  If it responds with
> RRS, the access should be retried either by hardware or (if RRS SV is
> enabled) by software.  This is the origin of "RRS_READY" -- the device
> can at least do RRS.
> 

Yeah, but the usage of 100ms is only valid if RRS SV is enabled by the software
as per sec 6.6.1:

"It is strongly recommended that software use 100 ms wait periods only if
software enables Configuration RRS Software Visibility".

So I guess we should only have 3 delays in the drivers:

1. 100ms after PERST# deassert for ports supporting link speed < 5.0 GT/s (I
believe once the gpiod_ call returns, it could be considered as the exit from
the conventional reset).

2. 1s to poll for the link up after PERST# deassert.

3. 100ms after link up (either by polling or interrupt) for ports supporting
link speed > 5.0 GT/s.

> "CONFIG_READY" would make sense except that it would be confused with
> the spec's usage of "Configuration Ready" (unfortunately not formally
> defined).  The PCIe r6.0, sec 6.22 implementation note says devices
> may take up to 1 second to become Configuration Ready, and that when a
> device is Configuration Ready, system software can proceed without
> further delay to configure the device.
> 
> "PCIE_RESET_CONFIG_DEVICE_WAIT_MS" seems a little long to me (we might
> not need "DEVICE"), but it does include "CONFIG" which is definitely
> relevant.  "PCIE_RESET_CONFIG_WAIT_MS"?
> 

Shorter the better :) Looks good to me.

- Mani

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

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 08:33:54PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 12, 2025 at 09:44:47AM -0500, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 06:30:37PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Jun 12, 2025 at 07:21:08AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> > > > > On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > > > > > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > > > > > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > > > > > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > > > > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > > > > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > > > > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > > > > > > 
> > > > > > > > > This means that there is no longer any delay between link up and the bus
> > > > > > > > > getting enumerated.
> > > > > > 
> > > > > > > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > > > > > > enough (although it might need to be updated to mention link-up).
> > > > > > > > This delay is going to be a standard piece of every driver, so it
> > > > > > > > won't require special notice.
> > > > > > > 
> > > > > > > Looking at pci.h, we already have a comment mentioning exactly this
> > > > > > > (link-up):
> > > > > > > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > > > > > > 
> > > > > > > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> > > > > > 
> > > > > > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > > > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > > > > > exist.  It looks like they got merged at about the same time by
> > > > > > different people, so we didn't notice.
> > > > > 
> > > > > I came to the same conclusion, I will send a patch to remove
> > > > > PCIE_T_RRS_READY_MS and convert the only existing user to use
> > > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS.
> > > > 
> > > > I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
> > > > specifically.  It's not that the device is completely ready after
> > > > 100ms; just that it should be able to respond with RRS if it needs
> > > > more time.
> > > 
> > > Yes, but none of the drivers are checking for the RRS status
> > > currently. So using PCIE_T_RRS_READY_MS gives a wrong impression
> > > that the driver is waiting for the RRS status from the device.
> > 
> > There's 100ms immediately after reset or link-up when we can't send
> > config requests because the device may not be able to respond at all.
> > 
> > After 100ms, the device should be able to respond to config requests
> > with SC, UR, RRS, or CA status (sec 2.2.9.1).  If it responds with
> > RRS, the access should be retried either by hardware or (if RRS SV is
> > enabled) by software.  This is the origin of "RRS_READY" -- the device
> > can at least do RRS.
> 
> Yeah, but the usage of 100ms is only valid if RRS SV is enabled by
> the software as per sec 6.6.1:
> 
> "It is strongly recommended that software use 100 ms wait periods
> only if software enables Configuration RRS Software Visibility".

I see that statement but don't understand it.  Do you think it's meant
as an exception to the first two paragraphs that say "software must
wait a minimum of 100 ms" following either "exit from Conventional
Reset" or "after Link training completes"?

I can't imagine that it means "if the Root Port doesn't support RRS SV
or software doesn't enable RRS SV, software needn't wait at all before
issuing config requests."

This whole thing is about whether the Endpoint is ready to respond.
A Root Port property (RRS SV support or enablement) doesn't tell us
anything about the Endpoint.

Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Manivannan Sadhasivam]

On Thu, Jun 12, 2025 at 10:24:42AM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 12, 2025 at 08:33:54PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 12, 2025 at 09:44:47AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Jun 12, 2025 at 06:30:37PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Jun 12, 2025 at 07:21:08AM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Jun 12, 2025 at 01:40:23PM +0200, Niklas Cassel wrote:
> > > > > > On Thu, Jun 12, 2025 at 06:38:27AM -0500, Bjorn Helgaas wrote:
> > > > > > > On Thu, Jun 12, 2025 at 01:19:45PM +0200, Niklas Cassel wrote:
> > > > > > > > On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> > > > > > > > > On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > > > > > > > > > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > > > > > > > > > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > > > > > > > > > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > > > > > > > > > 
> > > > > > > > > > This means that there is no longer any delay between link up and the bus
> > > > > > > > > > getting enumerated.
> > > > > > > 
> > > > > > > > > I think the comment at the PCIE_T_RRS_READY_MS definition should be
> > > > > > > > > enough (although it might need to be updated to mention link-up).
> > > > > > > > > This delay is going to be a standard piece of every driver, so it
> > > > > > > > > won't require special notice.
> > > > > > > > 
> > > > > > > > Looking at pci.h, we already have a comment mentioning exactly this
> > > > > > > > (link-up):
> > > > > > > > https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63
> > > > > > > > 
> > > > > > > > I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.
> > > > > > > 
> > > > > > > I'll more closely later, but I think PCIE_T_RRS_READY_MS and
> > > > > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS are duplicates and only one should
> > > > > > > exist.  It looks like they got merged at about the same time by
> > > > > > > different people, so we didn't notice.
> > > > > > 
> > > > > > I came to the same conclusion, I will send a patch to remove
> > > > > > PCIE_T_RRS_READY_MS and convert the only existing user to use
> > > > > > PCIE_RESET_CONFIG_DEVICE_WAIT_MS.
> > > > > 
> > > > > I think PCIE_T_RRS_READY_MS expresses the purpose of the wait more
> > > > > specifically.  It's not that the device is completely ready after
> > > > > 100ms; just that it should be able to respond with RRS if it needs
> > > > > more time.
> > > > 
> > > > Yes, but none of the drivers are checking for the RRS status
> > > > currently. So using PCIE_T_RRS_READY_MS gives a wrong impression
> > > > that the driver is waiting for the RRS status from the device.
> > > 
> > > There's 100ms immediately after reset or link-up when we can't send
> > > config requests because the device may not be able to respond at all.
> > > 
> > > After 100ms, the device should be able to respond to config requests
> > > with SC, UR, RRS, or CA status (sec 2.2.9.1).  If it responds with
> > > RRS, the access should be retried either by hardware or (if RRS SV is
> > > enabled) by software.  This is the origin of "RRS_READY" -- the device
> > > can at least do RRS.
> > 
> > Yeah, but the usage of 100ms is only valid if RRS SV is enabled by
> > the software as per sec 6.6.1:
> > 
> > "It is strongly recommended that software use 100 ms wait periods
> > only if software enables Configuration RRS Software Visibility".
> 
> I see that statement but don't understand it.  Do you think it's meant
> as an exception to the first two paragraphs that say "software must
> wait a minimum of 100 ms" following either "exit from Conventional
> Reset" or "after Link training completes"?
> 

Maybe yes.

> I can't imagine that it means "if the Root Port doesn't support RRS SV
> or software doesn't enable RRS SV, software needn't wait at all before
> issuing config requests."
> 

Yeah, it cannot be.

> This whole thing is about whether the Endpoint is ready to respond.
> A Root Port property (RRS SV support or enablement) doesn't tell us
> anything about the Endpoint.

I think we should just leave the RRS for good :) I was having a feeling that the
RRS define we have didn't fit the purpose of waiting for the device to be ready
post link up. Hence, I looked it up in the spec and spotted the above statement,
just to confuse you and myself.

- Mani

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