[PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Niklas Cassel]

Hello there,

Commit 8d3bf19f1b58 ("PCI: dwc: Don't wait for link up if driver can detect
Link Up event") added support for DWC to not wait for link up before
enumerating the bus. However, we cannot simply enumerate the bus after
receiving a Link Up IRQ, we still need to wait PCIE_T_RRS_READY_MS time
to allow a device to become ready after deasserting PERST. To avoid
bringing back an conditional delay during probe, perform the wait in the
threaded IRQ handler instead.

Please review.


Kind regards,
Niklas


Changes since v1:
-Added missing include pci.h that was lost during rebase.


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: dw-rockchip: Replace PERST sleep time with proper macro
  PCI: qcom: Replace PERST sleep time with proper macro

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 4 +++-
 drivers/pci/controller/dwc/pcie-qcom.c        | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.49.0

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

Before 36971d6c5a9a, 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
(qcom already has an unconditional 1 ms sleep after deasserting PERST),
that would essentially bring back an unconditional delay during 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, for qcom SoCs that has a link up IRQ, we will not
have a 100 ms unconditional delay during boot for unpopulated PCIe slots.

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 | 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 dc98ae63362d..01a60d1f372a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1565,6 +1565,7 @@ 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");
+		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 v2 4/4] PCI: qcom: Replace PERST sleep time with proper macro

[Niklas Cassel]

Replace the PERST sleep time with the proper macro (PCIE_T_PVPERL_MS).
No functional change.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 01a60d1f372a..fa689e29145f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -289,7 +289,7 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
 {
 	/* Ensure that PERST has been asserted for at least 100 ms */
-	msleep(100);
+	msleep(PCIE_T_PVPERL_MS);
 	gpiod_set_value_cansleep(pcie->reset, 0);
 	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
 }
-- 
2.49.0

Re: [PATCH v2 4/4] PCI: qcom: Replace PERST sleep time with proper macro

[Hans Zhang]

On 2025/5/6 15:39, Niklas Cassel wrote:
> Replace the PERST sleep time with the proper macro (PCIE_T_PVPERL_MS).
> No functional change.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 01a60d1f372a..fa689e29145f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -289,7 +289,7 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>   static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>   {
>   	/* Ensure that PERST has been asserted for at least 100 ms */
> -	msleep(100);
> +	msleep(PCIE_T_PVPERL_MS);
>   	gpiod_set_value_cansleep(pcie->reset, 0);
>   	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>   }


Reviewed-by: Hans Zhang <18255117159@163.com>

Best regards,
Hans

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

[Krishna Chaitanya Chundru]

On 5/6/2025 1:09 PM, 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 when receiving a Link Up IRQ.
> 
> Before 36971d6c5a9a, 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
> (qcom already has an unconditional 1 ms sleep after deasserting PERST),
> that would essentially bring back an unconditional delay during 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, for qcom SoCs that has a link up IRQ, we will not
> have a 100 ms unconditional delay during boot for unpopulated PCIe slots.
> 
> Fixes: 36971d6c5a9a ("PCI: qcom: Don't wait for link if we can detect Link Up")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>   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 dc98ae63362d..01a60d1f372a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1565,6 +1565,7 @@ 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");
> +		msleep(PCIE_T_RRS_READY_MS);
>   		/* Rescan the bus to enumerate endpoint devices */
>   		pci_lock_rescan_remove();
>   		pci_rescan_bus(pp->bridge->bus);

Re: [PATCH v2 4/4] PCI: qcom: Replace PERST sleep time with proper macro

[Krishna Chaitanya Chundru]

On 5/6/2025 1:09 PM, Niklas Cassel wrote:
> Replace the PERST sleep time with the proper macro (PCIE_T_PVPERL_MS).
> No functional change.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>   drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 01a60d1f372a..fa689e29145f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -289,7 +289,7 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
>   static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>   {
>   	/* Ensure that PERST has been asserted for at least 100 ms */
> -	msleep(100);
> +	msleep(PCIE_T_PVPERL_MS);
>   	gpiod_set_value_cansleep(pcie->reset, 0);
>   	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>   }

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

[Laszlo Fiat]

On Tuesday, May 6th, 2025 at 9:39 AM, Niklas Cassel <cassel@kernel.org> 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 when receiving a Link Up IRQ.
> 
> Before 36971d6c5a9a, 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
> (qcom already has an unconditional 1 ms sleep after deasserting PERST),
> that would essentially bring back an unconditional delay during 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, for qcom SoCs that has a link up IRQ, we will not
> have a 100 ms unconditional delay during boot for unpopulated PCIe slots.
> 
> 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 | 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 dc98ae63362d..01a60d1f372a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1565,6 +1565,7 @@ 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");
> + 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

Tested-by: Laszlo Fiat <laszlo.fiat@proton.me>

Re: [PATCH v2 4/4] PCI: qcom: Replace PERST sleep time with proper macro

[Laszlo Fiat]

On Tuesday, May 6th, 2025 at 9:40 AM, Niklas Cassel <cassel@kernel.org> wrote:

> Replace the PERST sleep time with the proper macro (PCIE_T_PVPERL_MS).
> No functional change.
> 
> Signed-off-by: Niklas Cassel cassel@kernel.org
> 
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 01a60d1f372a..fa689e29145f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -289,7 +289,7 @@ static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> static void qcom_ep_reset_deassert(struct qcom_pcie pcie)
> {
> / Ensure that PERST has been asserted for at least 100 ms */
> - msleep(100);
> + msleep(PCIE_T_PVPERL_MS);
> gpiod_set_value_cansleep(pcie->reset, 0);
> 
> usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
> --
> 2.49.0

Tested-by: Laszlo Fiat <laszlo.fiat@proton.me>

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Niklas Cassel]

On Tue, May 06, 2025 at 09:39:35AM +0200, Niklas Cassel wrote:
> Hello there,
> 
> Commit 8d3bf19f1b58 ("PCI: dwc: Don't wait for link up if driver can detect
> Link Up event") added support for DWC to not wait for link up before
> enumerating the bus. However, we cannot simply enumerate the bus after
> receiving a Link Up IRQ, we still need to wait PCIE_T_RRS_READY_MS time
> to allow a device to become ready after deasserting PERST. To avoid
> bringing back an conditional delay during probe, perform the wait in the
> threaded IRQ handler instead.
> 
> Please review.
> 
> 
> Kind regards,
> Niklas

Hello Laszlo,

Since you have a RK3588 based board, sending a Tested-by
tag on testing patches 1-2 makes sense.

However, patches 3-4 are for Qualcomm based boards.
I'm not sure if you also have a Qualcomm board at your disposal.

If you don't, Manivannan can probably drop your Tested-by tags
on patches 3-4, but it might be good to know until next time.

Thank you for testing!


Kind regards,
Niklas

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Laszlo Fiat]

-------- Original Message --------
On 06/05/2025 16:33, Niklas Cassel <cassel@kernel.org> wrote:

>  On Tue, May 06, 2025 at 09:39:35AM +0200, Niklas Cassel wrote:
>  > Hello there,
>  >
>  > Commit 8d3bf19f1b58 ("PCI: dwc: Don't wait for link up if driver can detect
>  > Link Up event") added support for DWC to not wait for link up before
>  > enumerating the bus. However, we cannot simply enumerate the bus after
>  > receiving a Link Up IRQ, we still need to wait PCIE_T_RRS_READY_MS time
>  > to allow a device to become ready after deasserting PERST. To avoid
>  > bringing back an conditional delay during probe, perform the wait in the
>  > threaded IRQ handler instead.
>  >
>  > Please review.
>  >
>  >
>  > Kind regards,
>  > Niklas
>  
>  Hello Laszlo,
>  
>  Since you have a RK3588 based board, sending a Tested-by
>  tag on testing patches 1-2 makes sense.
>  
>  However, patches 3-4 are for Qualcomm based boards.
>  I'm not sure if you also have a Qualcomm board at your disposal.
>  
>  If you don't, Manivannan can probably drop your Tested-by tags
>  on patches 3-4, but it might be good to know until next time.

Hello Niklas,

You are right, I only have a Rockchip based board, which I have tested on. 
Mannivan should drop my Tested-by tags on the qcom patches. My mistake, I am sorry.

>  
>  Thank you for testing!
>
You are welcome! Thank you for making my board work again.

Bye,

Laszlo

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

[Wilfred Mallawa]

On Tue, 2025-05-06 at 09:39 +0200, 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 when receiving a Link Up IRQ.
> 
> Before 36971d6c5a9a, 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
> (qcom already has an unconditional 1 ms sleep after deasserting
> PERST),
> that would essentially bring back an unconditional delay during 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, for qcom SoCs that has a link up IRQ, we will
> not
> have a 100 ms unconditional delay during boot for unpopulated PCIe
> slots.
> 
> 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 | 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 dc98ae63362d..01a60d1f372a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1565,6 +1565,7 @@ 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");
> +		msleep(PCIE_T_RRS_READY_MS);
>  		/* Rescan the bus to enumerate endpoint devices */
>  		pci_lock_rescan_remove();
>  		pci_rescan_bus(pp->bridge->bus);
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Re: [PATCH v2 4/4] PCI: qcom: Replace PERST sleep time with proper macro

[Wilfred Mallawa]

On Tue, 2025-05-06 at 09:39 +0200, Niklas Cassel wrote:
> Replace the PERST sleep time with the proper macro
> (PCIE_T_PVPERL_MS).
> No functional change.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> index 01a60d1f372a..fa689e29145f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -289,7 +289,7 @@ static void qcom_ep_reset_assert(struct qcom_pcie
> *pcie)
>  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
>  {
>  	/* Ensure that PERST has been asserted for at least 100 ms
> */
> -	msleep(100);
> +	msleep(PCIE_T_PVPERL_MS);
>  	gpiod_set_value_cansleep(pcie->reset, 0);
>  	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
>  }
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Manivannan Sadhasivam]

On Tue, May 06, 2025 at 09:39:35AM +0200, Niklas Cassel wrote:
> Hello there,
> 
> Commit 8d3bf19f1b58 ("PCI: dwc: Don't wait for link up if driver can detect
> Link Up event") added support for DWC to not wait for link up before
> enumerating the bus. However, we cannot simply enumerate the bus after
> receiving a Link Up IRQ, we still need to wait PCIE_T_RRS_READY_MS time
> to allow a device to become ready after deasserting PERST. To avoid
> bringing back an conditional delay during probe, perform the wait in the
> threaded IRQ handler instead.
> 

This wait time is a grey area in the spec tbh. If the Readiness Notification
(RN) is not supported, then the spec suggests waiting 1s for the device to
become 'configuration ready'. That's why we have the 1s delay in dwc driver.

Also, it has the below in r6.0, sec 6.6.1:

```
* On the completion of Link Training (entering the DL_Active state, see §
Section 3.2 ), a component must be able to receive and process TLPs and DLLPs.
* Following exit from a Conventional Reset of a device, within 1.0 s the device
must be able to receive a Configuration Request and return a Successful
Completion if the Request is valid. This period is independent of how quickly
Link training completes. If Readiness Notifications mechanisms are used (see
§ Section 6.22 .), this period may be shorter.
```

As per the first note, once link training is completed, the device should be
ready to accept configuration requests from the host. So no delay should be
required.

But the second note says that the 1s delay is independent of how quickly the
link training completes. This essentially contradicts with the above point.

So I think it is not required to add delay after completing the LTSSM, unless
someone sees any issue.

- Mani

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

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Niklas Cassel]

Hello Mani,

On Tue, May 13, 2025 at 11:53:29AM +0100, Manivannan Sadhasivam wrote:
> 
> This wait time is a grey area in the spec tbh. If the Readiness Notification
> (RN) is not supported, then the spec suggests waiting 1s for the device to
> become 'configuration ready'. That's why we have the 1s delay in dwc driver.
> 
> Also, it has the below in r6.0, sec 6.6.1:
> 
> ```
> * On the completion of Link Training (entering the DL_Active state, see §
> Section 3.2 ), a component must be able to receive and process TLPs and DLLPs.
> * Following exit from a Conventional Reset of a device, within 1.0 s the device
> must be able to receive a Configuration Request and return a Successful
> Completion if the Request is valid. This period is independent of how quickly
> Link training completes. If Readiness Notifications mechanisms are used (see
> § Section 6.22 .), this period may be shorter.
> ```
> 
> As per the first note, once link training is completed, the device should be
> ready to accept configuration requests from the host. So no delay should be
> required.
> 
> But the second note says that the 1s delay is independent of how quickly the
> link training completes. This essentially contradicts with the above point.
> 
> So I think it is not required to add delay after completing the LTSSM, unless
> someone sees any issue.

If you look at the commit message in patch 1/2, the whole reason for this
series is that someone has seen an issue :)

While I personally haven't seen any issue, the user reporting that commit
ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect
Link Up") regressed his system so that it can no longer mount rootfs
(which is on a PLEXTOR PX-256M8PeGN NVMe SSD) clearly has seen an issue.

It is possible that his device is not following the spec.
I simply compared the code before and after ec9fd499b9c6, to try to
figure out why it was actually working before, and came up with this,
which made his device functional again.

Perhaps we should add a comment above the sleep that says that this
should strictly not be needed as per the spec?
(And also add the same comment in the (single) controller driver in
mainline which already does an msleep(PCIE_T_RRS_READY_MS).)


Kind regards,
Niklas

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Laszlo Fiat]

Hello,

On Tuesday, May 13th, 2025 at 4:07 PM, Niklas Cassel <cassel@kernel.org> wrote:

> Hello Mani,
> 
> On Tue, May 13, 2025 at 11:53:29AM +0100, Manivannan Sadhasivam wrote:
> 
> > This wait time is a grey area in the spec tbh. If the Readiness Notification
> > (RN) is not supported, then the spec suggests waiting 1s for the device to
> > become 'configuration ready'. That's why we have the 1s delay in dwc driver.
> > 
> > Also, it has the below in r6.0, sec 6.6.1:
> > 
> > `* On the completion of Link Training (entering the DL_Active state, see § Section 3.2 ), a component must be able to receive and process TLPs and DLLPs. * Following exit from a Conventional Reset of a device, within 1.0 s the device must be able to receive a Configuration Request and return a Successful Completion if the Request is valid. This period is independent of how quickly Link training completes. If Readiness Notifications mechanisms are used (see § Section 6.22 .), this period may be shorter.`
> > 
> > As per the first note, once link training is completed, the device should be
> > ready to accept configuration requests from the host. So no delay should be
> > required.
> > 
> > But the second note says that the 1s delay is independent of how quickly the
> > link training completes. This essentially contradicts with the above point.
> > 
> > So I think it is not required to add delay after completing the LTSSM, unless
> > someone sees any issue.
> 
> 
> If you look at the commit message in patch 1/2, the whole reason for this
> series is that someone has seen an issue :)
> 
> While I personally haven't seen any issue, the user reporting that commit
> ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect
> Link Up") regressed his system so that it can no longer mount rootfs
> (which is on a PLEXTOR PX-256M8PeGN NVMe SSD) clearly has seen an issue.
> 
> It is possible that his device is not following the spec.
> I simply compared the code before and after ec9fd499b9c6, to try to
> figure out why it was actually working before, and came up with this,
> which made his device functional again.
> 
> Perhaps we should add a comment above the sleep that says that this
> should strictly not be needed as per the spec?
> (And also add the same comment in the (single) controller driver in
> mainline which already does an msleep(PCIE_T_RRS_READY_MS).)

I am the one experiencing the issue with my Orange PI 3B (RK3566, 8 GB RAM) and a PLEXTOR PX-256M8PeGN NVMe SSD. 

I first detected the problem while upgrading from 6.13.8 to 6.14.3, that my system cannot find the NVME SSD which contains the rootfs. After reverting the two patches:

- ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
- 0e0b45ab5d77 ("PCI: dw-rockchip: Enumerate endpoints based on dll_link_up IRQ")

my system booted fine again. 
After that I tested the patches sent by Niklas in this thread, which fixed the issue, so I sent Tested-by.

I did another test Today with 6.15.0-rc6, which in itself does not find my SSD. Niklas asked me to test with these 

- revert ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
- revert 0e0b45ab5d77 ("PCI: dw-rockchip: Enumerate endpoints based on dll_link_up IRQ")
- apply the following patch:

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index b3615d125942..5dee689ecd95 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -692,7 +692,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
                if (dw_pcie_link_up(pci))
                        break;

-               msleep(LINK_WAIT_SLEEP_MS);
+               usleep_range(100, 200);
        }

        if (retries >= LINK_WAIT_MAX_RETRIES) {


which restores the original behaviour to wait for link-up, then shorten the time. This resulted again a non booting system, this time with "Phy link never came up" error message.
So please allow to fix the regression that is already in 6.14.x. I now so far only I have reported this, but we cannot be sure how many SSDs have this timing issue. Most users use older, distribution packaged kernels, so others will face this later.

Bye,

Laszlo Fiat

> 
> 
> Kind regards,
> Niklas

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Niklas Cassel]

On Thu, May 15, 2025 at 05:33:41PM +0000, Laszlo Fiat wrote:
> I am the one experiencing the issue with my Orange PI 3B (RK3566, 8 GB RAM) and a PLEXTOR PX-256M8PeGN NVMe SSD. 
> 
> I first detected the problem while upgrading from 6.13.8 to 6.14.3, that my system cannot find the NVME SSD which contains the rootfs. After reverting the two patches:
> 
> - ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
> - 0e0b45ab5d77 ("PCI: dw-rockchip: Enumerate endpoints based on dll_link_up IRQ")
> 
> my system booted fine again. 
> After that I tested the patches sent by Niklas in this thread, which fixed the issue, so I sent Tested-by.
> 
> I did another test Today with 6.15.0-rc6, which in itself does not find my SSD. Niklas asked me to test with these 
> 
> - revert ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
> - revert 0e0b45ab5d77 ("PCI: dw-rockchip: Enumerate endpoints based on dll_link_up IRQ")
> - apply the following patch:
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index b3615d125942..5dee689ecd95 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -692,7 +692,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>                 if (dw_pcie_link_up(pci))
>                         break;
> 
> -               msleep(LINK_WAIT_SLEEP_MS);
> +               usleep_range(100, 200);
>         }
> 
>         if (retries >= LINK_WAIT_MAX_RETRIES) {
> 
> 
> which restores the original behaviour to wait for link-up, then shorten the time. This resulted again a non booting system, this time with "Phy link never came up" error message.

That message was unexpected.

What I expected to happen was that the link would come up, but by reducing
delay between "link is up" and device is accessed (from 90 ms -> 100 us),
I was that you would see the same problem on "older" kernels as you do with
the "link up IRQ" patches (which originally had no delay, but this series
basically re-added the same delay (PCIE_T_RRS_READY_MS, 100 ms) as we had
before (LINK_WAIT_SLEEP_MS, 90 ms).

But I see the problem with the test code that I asked you to test to verify
that this problem also existed before (if you had a shorter delay).
(By reducing the delay, the LINK_WAIT_MAX_RETRIES also need to be bumped.)

Could you please test:
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index b3615d125942..5dee689ecd95 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -692,7 +692,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
                if (dw_pcie_link_up(pci))
                        break;
 
-               msleep(LINK_WAIT_SLEEP_MS);
+               usleep_range(100, 200);
        }
 
        if (retries >= LINK_WAIT_MAX_RETRIES) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 4dd16aa4b39e..8422661b79d5 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -61,7 +61,7 @@
        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_MAX_RETRIES          10000
 #define LINK_WAIT_SLEEP_MS             90
 
 /* Parameters for the waiting for iATU enabled routine */


On top of an old kernel instead?

We expect the link to come up, but that you will not be able to mount rootfs.

If that is the case, we are certain that the this patch series is 100% needed
for your device to have the same functional behavior as before.


Kind regards,
Niklas

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Laszlo Fiat]

Hello,
-------- Original Message --------
On 16/05/2025 12:00, Niklas Cassel <cassel@kernel.org> wrote:

>  On Thu, May 15, 2025 at 05:33:41PM +0000, Laszlo Fiat wrote:
>  > I am the one experiencing the issue with my Orange PI 3B (RK3566, 8 GB RAM) and a PLEXTOR PX-256M8PeGN NVMe SSD.
>  >
>  > I first detected the problem while upgrading from 6.13.8 to 6.14.3, that my system cannot find the NVME SSD which contains the rootfs. After reverting the two patches:
>  >
>  > - ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
>  > - 0e0b45ab5d77 ("PCI: dw-rockchip: Enumerate endpoints based on dll_link_up IRQ")
>  >
>  > my system booted fine again.
>  > After that I tested the patches sent by Niklas in this thread, which fixed the issue, so I sent Tested-by.
>  >
>  > I did another test Today with 6.15.0-rc6, which in itself does not find my SSD. Niklas asked me to test with these
>  >
>  > - revert ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can detect Link Up")
>  > - revert 0e0b45ab5d77 ("PCI: dw-rockchip: Enumerate endpoints based on dll_link_up IRQ")
>  > - apply the following patch:
>  >
>  > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>  > index b3615d125942..5dee689ecd95 100644
>  > --- a/drivers/pci/controller/dwc/pcie-designware.c
>  > +++ b/drivers/pci/controller/dwc/pcie-designware.c
>  > @@ -692,7 +692,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>  >                 if (dw_pcie_link_up(pci))
>  >                         break;
>  >
>  > -               msleep(LINK_WAIT_SLEEP_MS);
>  > +               usleep_range(100, 200);
>  >         }
>  >
>  >         if (retries >= LINK_WAIT_MAX_RETRIES) {
>  >
>  >
>  > which restores the original behaviour to wait for link-up, then shorten the time. This resulted again a non booting system, this time with "Phy link never came up" error message.
>  
>  That message was unexpected.
>  
>  What I expected to happen was that the link would come up, but by reducing
>  delay between "link is up" and device is accessed (from 90 ms -> 100 us),
>  I was that you would see the same problem on "older" kernels as you do with
>  the "link up IRQ" patches (which originally had no delay, but this series
>  basically re-added the same delay (PCIE_T_RRS_READY_MS, 100 ms) as we had
>  before (LINK_WAIT_SLEEP_MS, 90 ms).
>  
>  But I see the problem with the test code that I asked you to test to verify
>  that this problem also existed before (if you had a shorter delay).
>  (By reducing the delay, the LINK_WAIT_MAX_RETRIES also need to be bumped.)
>  
>  Could you please test:
>  diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>  index b3615d125942..5dee689ecd95 100644
>  --- a/drivers/pci/controller/dwc/pcie-designware.c
>  +++ b/drivers/pci/controller/dwc/pcie-designware.c
>  @@ -692,7 +692,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>                  if (dw_pcie_link_up(pci))
>                          break;
>  
>  -               msleep(LINK_WAIT_SLEEP_MS);
>  +               usleep_range(100, 200);
>          }
>  
>          if (retries >= LINK_WAIT_MAX_RETRIES) {
>  diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>  index 4dd16aa4b39e..8422661b79d5 100644
>  --- a/drivers/pci/controller/dwc/pcie-designware.h
>  +++ b/drivers/pci/controller/dwc/pcie-designware.h
>  @@ -61,7 +61,7 @@
>          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_MAX_RETRIES          10000
>   #define LINK_WAIT_SLEEP_MS             90
>  
>   /* Parameters for the waiting for iATU enabled routine */
>  
>  
>  On top of an old kernel instead?
>

I have compiled a vanilla 6.12.28, that booted fine, as expeced. Then compiled a  version with the patch directly above.

>  We expect the link to come up, but that you will not be able to mount rootfs.
>  

That is exactly what happened. 

>  If that is the case, we are certain that the this patch series is 100% needed
>  for your device to have the same functional behavior as before.

That is the case.

Bye,

Laszlo Fiat 
>  
>  
>  Kind regards,
>  Niklas
>

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Niklas Cassel]

On Fri, May 16, 2025 at 06:48:59PM +0000, Laszlo Fiat wrote:
> 
> I have compiled a vanilla 6.12.28, that booted fine, as expeced. Then compiled a  version with the patch directly above.
> 
> >  We expect the link to come up, but that you will not be able to mount rootfs.
> >  
> 
> That is exactly what happened. 
> 
> >  If that is the case, we are certain that the this patch series is 100% needed
> >  for your device to have the same functional behavior as before.
> 
> That is the case.

Laszlo,

Thank you for verifying!

> 
> Bye,
> 
> Laszlo Fiat 

Mani, Krzysztof,

It does indeed seem like the reason for this regression is that this weird
NVMe drive requires some additional time after link up to be ready.

This series does re-add a similar delay, but in the 'link up' IRQ thread,
so even with this series, things are nicer than before (where we had an
unconditional delay after starting the link).

Do you require any additional changes?

Otherwise, I think it would be nice if we could get this queued up before
the merge window.


Kind regards,
Niklas

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Manivannan Sadhasivam]

On Mon, May 19, 2025 at 11:44:17AM +0200, Niklas Cassel wrote:
> On Fri, May 16, 2025 at 06:48:59PM +0000, Laszlo Fiat wrote:
> > 
> > I have compiled a vanilla 6.12.28, that booted fine, as expeced. Then compiled a  version with the patch directly above.
> > 
> > >  We expect the link to come up, but that you will not be able to mount rootfs.
> > >  
> > 
> > That is exactly what happened. 
> > 
> > >  If that is the case, we are certain that the this patch series is 100% needed
> > >  for your device to have the same functional behavior as before.
> > 
> > That is the case.
> 
> Laszlo,
> 
> Thank you for verifying!
> 
> > 
> > Bye,
> > 
> > Laszlo Fiat 
> 
> Mani, Krzysztof,
> 
> It does indeed seem like the reason for this regression is that this weird
> NVMe drive requires some additional time after link up to be ready.
> 
> This series does re-add a similar delay, but in the 'link up' IRQ thread,
> so even with this series, things are nicer than before (where we had an
> unconditional delay after starting the link).
> 
> Do you require any additional changes?
> 

No. Sorry for the delay in getting back. I was at Linaro Connect last week and
had to take some vacation for the first half of this week.

It is fine with me to add the delay only in the Rockchip platform where the
issue was reported (next time please mention the issue that triggered the series
in the cover letter). But I do not want to add the same in Qcom platform as the
delay is not strictly required as per the spec (if I understood it properly).

So I'll merge rest of the 3 patches.

- Mani

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

Re: [PATCH v2 0/4] PCI: dwc: Link Up IRQ fixes

[Manivannan Sadhasivam]

On Tue, 06 May 2025 09:39:35 +0200, Niklas Cassel wrote:
> Commit 8d3bf19f1b58 ("PCI: dwc: Don't wait for link up if driver can detect
> Link Up event") added support for DWC to not wait for link up before
> enumerating the bus. However, we cannot simply enumerate the bus after
> receiving a Link Up IRQ, we still need to wait PCIE_T_RRS_READY_MS time
> to allow a device to become ready after deasserting PERST. To avoid
> bringing back an conditional delay during probe, perform the wait in the
> threaded IRQ handler instead.
> 
> [...]

Applied to controller/dw-rockchip, thanks!

[1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready
      commit: c1c9c4b6130defb671c45b53ec12fcd92afea971
[3/4] PCI: dw-rockchip: Replace PERST sleep time with proper macro
      commit: 9ad44a6f6f8775786a9543427c82625d96a340b4
[4/4] PCI: qcom: Replace PERST sleep time with proper macro
      commit: be0b42befa87d81780aa0f003644b0ad520b0234

(I do need to change the name of the topic-branch now...)

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>