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

[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 when receiving a Link Up IRQ.

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.

It seems that we are enumerating the bus before the endpoint is ready.
Adding a msleep(PCIE_T_RRS_READY_MS) before enumerating the bus in the
threaded IRQ handler makes the SSD functional once again.

What appears to happen is that before ec9fd499b9c6, we called
dw_pcie_wait_for_link(), and in the first iteration of the loop, the link
will never be up (because the link was just started),
dw_pcie_wait_for_link() will then sleep for LINK_WAIT_SLEEP_MS (90 ms),
before trying again.

This means that even if a driver was missing a msleep(PCIE_T_RRS_READY_MS)
(100 ms), because of the call to dw_pcie_wait_for_link(), enumerating the
bus would essentially be delayed by that time anyway (because of the sleep
LINK_WAIT_SLEEP_MS (90 ms)).

While we could add the msleep(PCIE_T_RRS_READY_MS) after deasserting PERST,
that would essentially bring back an unconditional delay during synchronous
probe (the whole reason to use a Link Up IRQ was to avoid an unconditional
delay during probe).

Thus, add the msleep(PCIE_T_RRS_READY_MS) before enumerating the bus in the
IRQ handler. This way, we will not have an unconditional delay during boot
for unpopulated PCIe slots.

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>
---
Hello Laszlo,

I know you have already tested this, but could you please send
your Tested-by tag to this patch?

 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 7a6a95dc877a..ed8e3dfe80e0 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -471,6 +471,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)) {
 			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
+			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 Mon, May 05, 2025 at 11:26:05AM +0200, Niklas Cassel wrote:
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 7a6a95dc877a..ed8e3dfe80e0 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -471,6 +471,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)) {
>  			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
> +			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
> 

Bah.. this patch is missing
+#include "../../pci.h"

missed during rebase.

(pcie-qcom.c already includes this.)

Will send a V2.


Kind regards,
Niklas

[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

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

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