Re: [PATCH 1/2] PCI: cadence: Ensure that cdns_pcie_host_wait_for_link() waits 100 ms after link up

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On 01/05/26 21:05, Hans Zhang 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 a new 'max_link_speed' field in struct cdns_pcie to record the
> maximum supported (or currently configured) link speed of the controller.
> 
> In cdns_pcie_host_wait_for_link(), after the link is reported as up,
> insert a 100 ms delay if max_link_speed > 2 (i.e., > 5 GT/s). This
> implements the required delay at the common Cadence host layer.
> 
> Currently max_link_speed is zero-initialized, so the delay is not yet
> active. Glue drivers must set max_link_speed appropriately to enable
> the delay. This matches the approach taken for the Synopsys DWC
> controller in commit 80dc18a0cba8d ("PCI: dwc: Ensure that
> dw_pcie_wait_for_link() waits 100 ms after link up").
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>   .../pci/controller/cadence/pcie-cadence-host-common.c    | 9 +++++++++
>   drivers/pci/controller/cadence/pcie-cadence.h            | 2 ++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> index 2b0211870f02..d4ae762f423f 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> @@ -14,6 +14,7 @@
>   
>   #include "pcie-cadence.h"
>   #include "pcie-cadence-host-common.h"
> +#include "../../pci.h"
>   
>   #define LINK_RETRAIN_TIMEOUT HZ
>   
> @@ -55,6 +56,14 @@ int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie,
>   	/* Check if the link is up or not */
>   	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
>   		if (pcie_link_up(pcie)) {
> +			/*
> +			 * As per PCIe r6.0, sec 6.6.1, a Downstream Port that
> +			 * supports Link speeds greater than 5.0 GT/s, software
> +			 * must wait a minimum of 100 ms after Link training
> +			 * completes before sending a Configuration Request.
> +			 */
> +			if (pcie->max_link_speed > 2)
> +				msleep(PCIE_RESET_CONFIG_WAIT_MS);

I think the above could be moved to cdns_pcie_host_start_link() as follows:

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c 
b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
index 2b0211870f02..0f885dcbdb12 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
@@ -115,6 +115,15 @@ int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc,
  	if (!ret && rc->quirk_retrain_flag)
  		ret = cdns_pcie_retrain(pcie, pcie_link_up);

+	/*
+	 * As per PCIe r6.0, sec 6.6.1, a Downstream Port that
+	 * supports Link speeds greater than 5.0 GT/s, software
+	 * must wait a minimum of 100 ms after Link training
+	 * completes before sending a Configuration Request.
+	 */
+	if (!ret && pcie->max_link_speed > 2)
+		msleep(PCIE_RESET_CONFIG_WAIT_MS);
+
  	return ret;
  }
  EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);

This will avoid an additional and unnecessary delay when 
'cdns_pcie_retrain()' retrains the link.

Instead of checking for the link being up using "pcie_link_up(pcie)", 
checking for 'ret' being zero should also work (ret being zero indicates 
that the link is up).

Since configuration space accesses will not be performed until 
cdns_pcie_host_start_link() completes executing, it should be safe to 
switch to the above implementation.


>   			dev_info(dev, "Link up\n");
>   			return 0;
>   		}

[TRIMMED]

Regards,
Siddharth.