[PATCH v4 0/7] PCI: Add common helper for 100 ms delay after link training

[PATCH v4 0/7] PCI: Add common helper for 100 ms delay after link training

[Hans Zhang]

PCIe r6.0, sec 6.6.1 (Conventional Reset) requires that for a Downstream
Port supporting Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending any
Configuration Request.

Several PCIe host controller drivers currently omit this 100 ms delay
when the negotiated link speed is Gen3 (8 GT/s) or higher. Only the DWC
driver already implements it. The missing delay can lead to violations
of the PCIe specification and cause enumeration failures with high-speed
devices (e.g., NVIDIA RTX5070 GPU, PCIe 5.0 NVMe SSDs).

To fix this consistently and avoid code duplication, this series:

  1. Adds a static inline helper `pci_host_common_link_train_delay()`
     in `drivers/pci/controller/pci-host-common.h`. The helper checks
     the given max_link_speed and calls msleep(100) if speed > 5 GT/s.

  2. Converts the DWC driver to use this helper.

  3. Adds the missing 100 ms delay to the Cadence PCIe controller
     (both LGA and HPA IPs). A `max_link_speed` field is introduced
     in `struct cdns_pcie`. The j721e glue driver sets this field;
     other LGA glue drivers fall back to reading DT property
     "max-link-speed". HPA also reads the DT property.

  4. Adds the delay to the Aardvark and MediaTek Gen3 host drivers,
     and replaces the existing unconditional delay in the Renesas
     RZ/G3S driver with the conditional helper (per spec).

All changes are placed immediately after link training completes and
before any Configuration Request would be issued.

---
Our company's product is based on the HPA IP from Cadence. When connecting
to different devices, we encountered issues with the enumeration failure
when connecting to the NVIDIA RTX5070 GPU and the NVMe SSD with PCIe 5.0
interface. Our code is based on: 80dc18a0cba8d ("PCI: dwc: Ensure that
dw_pcie_wait_for_link() waits 100 ms after link up").
---
Changes since v4:
- Resolve the pointer reference error in patch 0002. (sashiko)

Changes since v3:
- Renamed helper from pcie_wait_after_link_train() to
  pci_host_common_link_train_delay() and moved to pci-host-common.h. (Mani)
- Reorganized Cadence patches: LGA + j721e glue in one patch, HPA separate.
- Added missing DT property reading for HPA and LGA fallback.
- Replace the existing msleep(100) call with the common helper
  pci_host_common_link_train_delay(). (Claudiu)

Changes since v2:
- Renamed helper from pcie_wait_after_link_train() to
  pci_host_common_link_train_delay() and moved to pci-host-common.h. (Mani)
- Reorganized Cadence patches: LGA + j721e glue in one patch, HPA separate.
- Added missing DT property reading for HPA and LGA fallback.
- Replace the existing msleep(100) call with the common helper
  pci_host_common_link_train_delay(). (Claudiu)

Changes since v1:
- Add pcie_wait_after_link_train() helper
- Reduce repetitive code comments and have each Root Port driver use the
  helper function instead.
- Increase the delay to 100ms after enabling the link-up that distinguishes
  between Cadence LGA and HPA IPs.
- Add the Aardvark, MediaTek Gen3, and Renesas RZ/G3S Root Port driver. When
  the speed is greater than GEN2, a delay of 100ms should be applied.

v3:
https://patchwork.kernel.org/project/linux-pci/patch/20260511055923.37117-1-18255117159@163.com/

v2:
https://patchwork.kernel.org/project/linux-pci/cover/20260506152346.166056-1-18255117159@163.com/

v1:
https://patchwork.kernel.org/project/linux-pci/patch/20260501153553.66382-1-18255117159@163.com/
---

Hans Zhang (7):
  PCI: Add pci_host_common_link_train_delay() helper
  PCI: cadence: Add post-link delay for LGA and j721e glue driver
  PCI: cadence: HPA: Add post-link delay
  PCI: dwc: Use common pci_host_common_link_train_delay() helper
  PCI: aardvark: Add 100 ms delay after link training
  PCI: mediatek-gen3: Add 100 ms delay after link up
  PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper

 drivers/pci/controller/cadence/pci-j721e.c      |  1 +
 .../cadence/pcie-cadence-host-common.c          |  4 ++++
 .../controller/cadence/pcie-cadence-host-hpa.c  |  8 ++++++++
 .../pci/controller/cadence/pcie-cadence-host.c  |  4 ++++
 drivers/pci/controller/cadence/pcie-cadence.h   |  2 ++
 drivers/pci/controller/dwc/pcie-designware.c    |  9 ++-------
 drivers/pci/controller/pci-aardvark.c           |  5 ++++-
 drivers/pci/controller/pci-host-common.h        | 17 +++++++++++++++++
 drivers/pci/controller/pcie-mediatek-gen3.c     |  3 +++
 drivers/pci/controller/pcie-rzg3s-host.c        |  3 ++-
 10 files changed, 47 insertions(+), 9 deletions(-)


base-commit: 50897c955902c93ae71c38698abb910525ebdc89
-- 
2.43.0

[PATCH v4 1/7] PCI: Add pci_host_common_link_train_delay() helper

[Hans Zhang]

PCIe r6.0, sec 6.6.1 (Conventional Reset) requires that for a Downstream
Port supporting Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending any
Configuration Request.

Introduce a static inline helper pci_host_common_link_train_delay() that
checks the given max_link_speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, etc.) and
calls msleep(100) only when the speed is greater than 5.0 GT/s.

This allows multiple host controller drivers to share the same mandatory
delay without duplicating the logic.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/pci-host-common.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
index b5075d4bd7eb..d709f7e3e11a 100644
--- a/drivers/pci/controller/pci-host-common.h
+++ b/drivers/pci/controller/pci-host-common.h
@@ -10,6 +10,9 @@
 #ifndef _PCI_HOST_COMMON_H
 #define _PCI_HOST_COMMON_H
 
+#include <linux/delay.h>
+#include "../pci.h"
+
 struct pci_ecam_ops;
 
 int pci_host_common_probe(struct platform_device *pdev);
@@ -20,4 +23,18 @@ void pci_host_common_remove(struct platform_device *pdev);
 
 struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
 	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
+
+/**
+ * pci_host_common_link_train_delay - Wait 100 ms if link speed > 5 GT/s
+ * @max_link_speed: the maximum link speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, ...)
+ *
+ * Must be called after Link training completes and before the first
+ * Configuration Request is sent.
+ */
+static inline void pci_host_common_link_train_delay(int max_link_speed)
+{
+	if (max_link_speed > 2)
+		msleep(PCIE_RESET_CONFIG_WAIT_MS);
+}
+
 #endif
-- 
2.43.0

[PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[Hans Zhang]

The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
lacks the mandatory 100 ms delay after link training completes for speeds
> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.

Add a 'max_link_speed' field to struct cdns_pcie. In the common host
layer function cdns_pcie_host_start_link(), after the link has been
successfully established, call pci_host_common_link_train_delay() to
insert the required delay.

For the j721e glue driver, set cdns_pcie.max_link_speed from the existing
link speed logic. For other LGA-based glue drivers (sky1, sg2042), the
common LGA host setup (pcie-cadence-host.c) provides a fallback reading
of the device tree property "max-link-speed" when available. This ensures
that the delay is not missed on those platforms once they enable the
property.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/cadence/pci-j721e.c                | 1 +
 drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
 drivers/pci/controller/cadence/pcie-cadence-host.c        | 4 ++++
 drivers/pci/controller/cadence/pcie-cadence.h             | 2 ++
 4 files changed, 11 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index bfdfe98d5aba..ae916e7b1927 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
 	    (pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
 		link_speed = 2;
 
+	pcie->cdns_pcie->max_link_speed = link_speed;
 	val = link_speed - 1;
 	ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK, val);
 	if (ret)
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
index 2b0211870f02..18e4b6c760b5 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-host-common.h"
 
 #define LINK_RETRAIN_TIMEOUT HZ
 
@@ -115,6 +116,9 @@ 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);
 
+	if (!ret)
+		pci_host_common_link_train_delay(pcie->max_link_speed);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 0bc9e6e90e0e..058e4e619654 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -13,6 +13,7 @@
 
 #include "pcie-cadence.h"
 #include "pcie-cadence-host-common.h"
+#include "../../pci.h"
 
 static u8 bar_aperture_mask[] = {
 	[RP_BAR0] = 0x1F,
@@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	rc->device_id = 0xffff;
 	of_property_read_u32(np, "device-id", &rc->device_id);
 
+	if (pcie->max_link_speed < 1)
+		pcie->max_link_speed = of_pci_get_max_link_speed(np);
+
 	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
 	if (IS_ERR(pcie->reg_base)) {
 		dev_err(dev, "missing \"reg\"\n");
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 574e9cf4d003..042a4c49bb9a 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
  * @ops: Platform-specific ops to control various inputs from Cadence PCIe
  *       wrapper
  * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
+ * @max_link_speed: Maximum supported link speed
  */
 struct cdns_pcie {
 	void __iomem		             *reg_base;
@@ -98,6 +99,7 @@ struct cdns_pcie {
 	struct device_link	             **link;
 	const  struct cdns_pcie_ops          *ops;
 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
+	int				     max_link_speed;
 };
 
 /**
-- 
2.43.0

[PATCH v4 3/7] PCI: cadence: HPA: Add post-link delay

[Hans Zhang]

The Cadence HPA (High Performance Architecture IP) specific link setup
function cdns_pcie_hpa_host_link_setup() waits for the link to come up
but does not implement the required 100 ms delay after link training
completes for speeds > 5.0 GT/s (PCIe r6.0 sec 6.6.1).

Add a call to pci_host_common_link_train_delay() immediately after the
link is confirmed to be up, using the max_link_speed field. Also, in the
HPA host setup function, read the device tree property "max-link-speed"
to initialize max_link_speed if not already set by a glue driver.

This ensures compliance for HPA-based platforms.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host-hpa.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
index 0f540bed58e8..8ef58ed01daa 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
@@ -15,6 +15,8 @@
 
 #include "pcie-cadence.h"
 #include "pcie-cadence-host-common.h"
+#include "../pci-host-common.h"
+#include "../../pci.h"
 
 static u8 bar_aperture_mask[] = {
 	[RP_BAR0] = 0x3F,
@@ -304,6 +306,8 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc *rc)
 	ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
 	if (ret)
 		dev_dbg(dev, "PCIe link never came up\n");
+	else
+		pci_host_common_link_train_delay(pcie->max_link_speed);
 
 	return ret;
 }
@@ -313,6 +317,7 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
 {
 	struct device *dev = rc->pcie.dev;
 	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *np = dev->of_node;
 	struct pci_host_bridge *bridge;
 	enum   cdns_pcie_rp_bar bar;
 	struct cdns_pcie *pcie;
@@ -343,6 +348,9 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
 		rc->cfg_res = res;
 	}
 
+	if (pcie->max_link_speed < 1)
+		pcie->max_link_speed = of_pci_get_max_link_speed(np);
+
 	/* Put EROM Bar aperture to 0 */
 	cdns_pcie_hpa_writel(pcie, REG_BANK_IP_CFG_CTRL_REG, CDNS_PCIE_EROM, 0x0);
 
-- 
2.43.0

[PATCH v4 4/7] PCI: dwc: Use common pci_host_common_link_train_delay() helper

[Hans Zhang]

The DWC driver already implements the 100 ms delay required by PCIe
r6.0 sec 6.6.1 by checking pci->max_link_speed and calling msleep(100).

Replace the open-coded msleep() with the new common helper
pci_host_common_link_train_delay() to reduce code duplication and
improve maintainability. No functional change intended.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index c11cf61b8319..7021d21bb601 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -22,6 +22,7 @@
 #include <linux/sizes.h>
 #include <linux/types.h>
 
+#include "../pci-host-common.h"
 #include "../../pci.h"
 #include "pcie-designware.h"
 
@@ -799,13 +800,7 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 		return -ETIMEDOUT;
 	}
 
-	/*
-	 * As per PCIe r6.0, sec 6.6.1, a Downstream Port that supports Link
-	 * speeds greater than 5.0 GT/s, software must wait a minimum of 100 ms
-	 * after Link training completes before sending a Configuration Request.
-	 */
-	if (pci->max_link_speed > 2)
-		msleep(PCIE_RESET_CONFIG_WAIT_MS);
+	pci_host_common_link_train_delay(pci->max_link_speed);
 
 	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
-- 
2.43.0

[PATCH v4 5/7] PCI: aardvark: Add 100 ms delay after link training

[Hans Zhang]

The Aardvark PCIe controller driver waits for the link to come up but
does not implement the mandatory 100 ms delay after link training
completes for speeds greater than 5.0 GT/s (PCIe r6.0 sec 6.6.1).

The driver already maintains a 'link_gen' field that holds the negotiated
link speed. Use it together with pci_host_common_link_train_delay() to
insert the required delay immediately after confirming that the link
is up.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/pci-aardvark.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e34bea1ff0ac..fd9c7d53e8a7 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -26,6 +26,7 @@
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
 
+#include "pci-host-common.h"
 #include "../pci.h"
 #include "../pci-bridge-emul.h"
 
@@ -350,8 +351,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
 
 	/* check if the link is up or not */
 	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
-		if (advk_pcie_link_up(pcie))
+		if (advk_pcie_link_up(pcie)) {
+			pci_host_common_link_train_delay(pcie->link_gen);
 			return 0;
+		}
 
 		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
 	}
-- 
2.43.0

[PATCH v4 6/7] PCI: mediatek-gen3: Add 100 ms delay after link up

[Hans Zhang]

The MediaTek Gen3 PCIe host driver lacks the required 100 ms delay after
link training completes for speeds > 5.0 GT/s, as specified in PCIe r6.0
sec 6.6.1.

The driver already stores max_link_speed (from the device tree). After
mtk_pcie_startup_port() successfully brings up the link, call
pci_host_common_link_train_delay() to comply with the specification.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index b0accd828589..5abddec4e9be 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -30,6 +30,7 @@
 #include <linux/regmap.h>
 #include <linux/reset.h>
 
+#include "pci-host-common.h"
 #include "../pci.h"
 
 #define PCIE_BASE_CFG_REG		0x14
@@ -570,6 +571,8 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 		goto err_power_down_device;
 	}
 
+	pci_host_common_link_train_delay(pcie->max_link_speed);
+
 	return 0;
 
 err_power_down_device:
-- 
2.43.0

[PATCH v4 7/7] PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper

[Hans Zhang]

Replace the unconditional msleep(100) with the common helper
pci_host_common_link_train_delay(). The helper only waits when
max_link_speed > 2, as required by PCIe r6.0 sec 6.6.1.

This avoids unnecessary delay for Gen1/Gen2 links while retaining
the mandatory 100 ms for higher speeds.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/pcie-rzg3s-host.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index d86e7516dcc2..66f687304c1c 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -35,6 +35,7 @@
 #include <linux/slab.h>
 #include <linux/units.h>
 
+#include "pci-host-common.h"
 #include "../pci.h"
 
 /* AXI registers */
@@ -1663,7 +1664,7 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 	if (ret)
 		dev_info(dev, "Failed to set max link speed\n");
 
-	msleep(PCIE_RESET_CONFIG_WAIT_MS);
+	pci_host_common_link_train_delay(host->max_link_speed);
 
 	return 0;
 
-- 
2.43.0

Re: [PATCH v4 1/7] PCI: Add pci_host_common_link_train_delay() helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Generic PCIe link training logic is incorrectly scoped to a framework-specific header, causing unnecessary coupling.
- [Medium] Including `../pci.h` inside a shared header file breaks subsystem encapsulation.
--

commit e409ebd1e8d25418f5cc0346099d7e22ffac79dd
Author: Hans Zhang <18255117159@163.com>

PCI: Add pci_host_common_link_train_delay() helper

This commit introduces a static inline helper to wait 100 ms after link
training if the link speed is greater than 5.0 GT/s, in accordance with
PCIe r6.0 sec 6.6.1. The helper is placed in pci-host-common.h to allow
multiple host controller drivers to share the logic.

> diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
> index b5075d4bd7eb3..d709f7e3e11a3 100644
> --- a/drivers/pci/controller/pci-host-common.h
> +++ b/drivers/pci/controller/pci-host-common.h
> @@ -10,6 +10,9 @@
>  #ifndef _PCI_HOST_COMMON_H
>  #define _PCI_HOST_COMMON_H
>  
> +#include <linux/delay.h>
> +#include "../pci.h"

Does including ../pci.h inside a shared header file create a regression
in subsystem encapsulation? 

drivers/pci/pci.h is a private header meant exclusively for internal PCI
core subsystem definitions. Exposing it in this shared header could pollute
the namespace of any driver that includes it. Moving the helper directly
into drivers/pci/pci.h might resolve this regression without requiring
additional includes.

[ ... ]

> @@ -20,4 +23,18 @@ void pci_host_common_remove(struct platform_device *pdev);
>  
>  struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
>  	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
> +
> +/**
> + * pci_host_common_link_train_delay - Wait 100 ms if link speed > 5 GT/s
> + * @max_link_speed: the maximum link speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, ...)
> + *
> + * Must be called after Link training completes and before the first
> + * Configuration Request is sent.
> + */
> +static inline void pci_host_common_link_train_delay(int max_link_speed)

Since the 100 ms delay requirement for downstream ports supporting > 5.0 GT/s
is a universal PCIe specification rule (PCIe r6.0 sec 6.6.1), does placing
this helper in the ECAM-specific pci-host-common.h introduce an
architectural regression?

Placing it here forces non-ECAM host controllers to include an unrelated
ECAM header just to use it. Could this be named generically and placed in
a central location like drivers/pci/pci.h instead?

> +{
> +	if (max_link_speed > 2)
> +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +}
> +
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=1

Re: [PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unhandled error code and failure to enforce mandatory PCIe link delay
--

commit b73c41b22775891786c97fcc245ece6f4b38b290
Author: Hans Zhang <18255117159@163.com>

PCI: cadence: Add post-link delay for LGA and j721e glue driver

The Cadence LGA PCIe host controller is missing the mandatory 100 ms delay
after link training for speeds > 5.0 GT/s. This patch adds a max_link_speed
field to struct cdns_pcie and inserts the required delay after link
establishment.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 0bc9e6e90e0e0..058e4e619654c 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
[ ... ]
> @@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	rc->device_id = 0xffff;
>  	of_property_read_u32(np, "device-id", &rc->device_id);
>  
> +	if (pcie->max_link_speed < 1)
> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);
> +

If the max-link-speed property is missing from the device tree,
of_pci_get_max_link_speed() returns -EINVAL. Does this code handle that
error correctly?

It looks like the negative error code (-22) will be assigned directly to
pcie->max_link_speed.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> index 2b0211870f02a..18e4b6c760b51 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
[ ... ]
> @@ -115,6 +116,9 @@ 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);
>  
> +	if (!ret)
> +		pci_host_common_link_train_delay(pcie->max_link_speed);
> +
>  	return ret;

Since pci_host_common_link_train_delay() checks if max_link_speed > 2,
would a negative value silently bypass the required 100ms delay on
platforms where the device tree property is missing?

Could we instead fall back to querying the hardware's actual link capability
from the PCI_EXP_LNKCAP register when the device tree property is absent,
so the hardware trains correctly and the delay is enforced without requiring
device tree updates?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=2

Re: [PATCH v4 3/7] PCI: cadence: HPA: Add post-link delay

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unchecked error return from `of_pci_get_max_link_speed()` disables the mandatory link training delay when the DT property is missing.
--

commit b05c1e9fe77d90401c0d9f6da4d6922526b3a553
Author: Hans Zhang <18255117159@163.com>

PCI: cadence: HPA: Add post-link delay
    
This commit introduces a mandatory 100 ms post-link training delay for PCIe
speeds > 5.0 GT/s to comply with the PCIe r6.0 specification. It reads the
max-link-speed property from the device tree to determine the appropriate
delay, defaulting to the value from the glue driver if set.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c

[ ... ]

> @@ -304,6 +306,8 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc *rc)
>  	ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
>  	if (ret)
>  		dev_dbg(dev, "PCIe link never came up\n");
> +	else
> +		pci_host_common_link_train_delay(pcie->max_link_speed);

Will this delay be skipped entirely if the max-link-speed device tree
property is missing?

[ ... ]

> @@ -343,6 +348,9 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
>  		rc->cfg_res = res;
>  	}
>  
> +	if (pcie->max_link_speed < 1)
> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);

If the max-link-speed property is not present in the device tree,
of_pci_get_max_link_speed() returns -EINVAL (-22):

drivers/pci/pci.h:of_pci_get_max_link_speed() {
    ...
    return -EINVAL;
}

This negative value is then assigned to pcie->max_link_speed. Later, when
pci_host_common_link_train_delay() is called, it checks:

drivers/pci/controller/pci-host-common.h:pci_host_common_link_train_delay() {
    if (max_link_speed > 2)
        msleep(PCIE_RESET_CONFIG_WAIT_MS);
}

Since -22 is not greater than 2, the delay would be bypassed. Should the
code check for a negative return value from of_pci_get_max_link_speed()
before assigning it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=3

Re: [PATCH v4 4/7] PCI: dwc: Use common pci_host_common_link_train_delay() helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Including the private header of the `pci-host-common` framework into the DesignWare Core (DWC) framework introduces an inappropriate cross-driver dependency.
--

commit 826f6ff69b866addd978e8c94219d9a3f4ffea60
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: Use common pci_host_common_link_train_delay() helper

This commit replaces an open-coded 100ms link training delay in the
DesignWare Core (DWC) driver with the pci_host_common_link_train_delay()
helper. It adds a relative path include to access the helper from the
generic host framework.

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index c11cf61b8319e..7021d21bb6011 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -22,6 +22,7 @@
>  #include <linux/sizes.h>
>  #include <linux/types.h>
>  
> +#include "../pci-host-common.h"
>  #include "../../pci.h"
>  #include "pcie-designware.h"

This isn't a bug, but does including a sibling framework's private header
using a relative path violate component isolation?

Since pci-host-common.h is specific to the generic ECAM host controller
architecture, and DWC is a completely separate controller architecture,
reusing this helper directly introduces a cross-driver dependency.

Because the 100 ms link training delay is a generic PCIe specification
requirement (PCIe r6.0 sec 6.6.1), should this helper be relocated to a
shared PCI core header such as drivers/pci/pci.h instead of remaining in
the pci-host-common framework?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=4

Re: [PATCH v4 5/7] PCI: aardvark: Add 100 ms delay after link training

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failure to enforce the mandatory 100ms post-reset delay for Gen 1 and Gen 2 devices.
- [Medium] Commit message incorrectly claims that `link_gen` holds the "negotiated link speed".
--

commit 82256a299ecbac6749b1ecf25bbecc74415b47f2
Author: Hans Zhang <18255117159@163.com>

PCI: aardvark: Add 100 ms delay after link training

This commit introduces a mandatory 100ms delay after link training completes
for Aardvark PCIe controllers operating at speeds greater than 5.0 GT/s. It
utilizes pci_host_common_link_train_delay() along with the existing link_gen
field to insert this delay once the link is confirmed up.

> The driver already maintains a 'link_gen' field that holds the negotiated
> link speed. Use it together with pci_host_common_link_train_delay() to

This isn't a bug, but does link_gen actually hold the negotiated link speed?

Looking at advk_pcie_probe(), pcie->link_gen is populated using
of_pci_get_max_link_speed(), which reads the static max-link-speed
capability from the device tree rather than the dynamically negotiated speed.

While passing this static capability correctly implements the PCIe spec,
could the commit message be updated to accurately describe the variable's
contents?

> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index e34bea1ff0ac6..fd9c7d53e8a78 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c

[ ... ]

> @@ -350,8 +351,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  
>  	/* check if the link is up or not */
>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> -		if (advk_pcie_link_up(pcie))
> +		if (advk_pcie_link_up(pcie)) {
> +			pci_host_common_link_train_delay(pcie->link_gen);
>  			return 0;
> +		}
>  
>  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
>  	}

Does this code enforce the mandatory 100ms post-reset delay for Gen 1 and
Gen 2 devices?

While this issue wasn't introduced by this patch, the existing code
fundamentally violates the PCIe Base Specification. The spec requires a
minimum 100ms delay after Conventional Reset (PERST# deassertion) before
sending Configuration Requests for ports supporting <= 5.0 GT/s.

In advk_pcie_setup_hw(), after PERST# is deasserted, the code calls
advk_pcie_wait_for_link(). If a Gen 1 or Gen 2 device trains quickly, the
new pci_host_common_link_train_delay() adds no delay, and the function
returns immediately.

Consequently, the PCI core will proceed to pci_host_probe() and start
issuing Configuration Requests before the 100ms minimum window has elapsed.
Can this cause Gen 1 or Gen 2 devices to fail to enumerate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=5

RE: [PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[Manikandan Karunakaran Pillai]

>EXTERNAL MAIL
>
>
>The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
>lacks the mandatory 100 ms delay after link training completes for speeds
>> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.
>
>Add a 'max_link_speed' field to struct cdns_pcie. In the common host
>layer function cdns_pcie_host_start_link(), after the link has been
>successfully established, call pci_host_common_link_train_delay() to
>insert the required delay.
>
>For the j721e glue driver, set cdns_pcie.max_link_speed from the existing
>link speed logic. For other LGA-based glue drivers (sky1, sg2042), the
>common LGA host setup (pcie-cadence-host.c) provides a fallback reading
>of the device tree property "max-link-speed" when available. This ensures
>that the delay is not missed on those platforms once they enable the
>property.
>
>Signed-off-by: Hans Zhang <18255117159@163.com>
>---
> drivers/pci/controller/cadence/pci-j721e.c                | 1 +
> drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
> drivers/pci/controller/cadence/pcie-cadence-host.c        | 4 ++++
> drivers/pci/controller/cadence/pcie-cadence.h             | 2 ++
> 4 files changed, 11 insertions(+)
>
>diff --git a/drivers/pci/controller/cadence/pci-j721e.c
>b/drivers/pci/controller/cadence/pci-j721e.c
>index bfdfe98d5aba..ae916e7b1927 100644
>--- a/drivers/pci/controller/cadence/pci-j721e.c
>+++ b/drivers/pci/controller/cadence/pci-j721e.c
>@@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie
>*pcie,
> 	    (pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
> 		link_speed = 2;
>
>+	pcie->cdns_pcie->max_link_speed = link_speed;
> 	val = link_speed - 1;
> 	ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK,
>val);
> 	if (ret)
>diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>index 2b0211870f02..18e4b6c760b5 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-host-common.h"
>
> #define LINK_RETRAIN_TIMEOUT HZ
>
>@@ -115,6 +116,9 @@ 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);
>
>+	if (!ret)
>+		pci_host_common_link_train_delay(pcie->max_link_speed);
>+
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
>diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>b/drivers/pci/controller/cadence/pcie-cadence-host.c
>index 0bc9e6e90e0e..058e4e619654 100644
>--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>@@ -13,6 +13,7 @@
>
> #include "pcie-cadence.h"
> #include "pcie-cadence-host-common.h"
>+#include "../../pci.h"
>
> static u8 bar_aperture_mask[] = {
> 	[RP_BAR0] = 0x1F,
>@@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> 	rc->device_id = 0xffff;
> 	of_property_read_u32(np, "device-id", &rc->device_id);
>
>+	if (pcie->max_link_speed < 1)
>+		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>+
Why is the conditional if required here as during cdns_pcie_host_setup(), the value of
max_link_speed is expected to be '0', unless specifically initialized by the platform code separately.

What happens if the max_link_speed is not defined in the corresponding dts ? Would not the -EINVAL returned from the function create issues ?

> 	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev,
>"reg");
> 	if (IS_ERR(pcie->reg_base)) {
> 		dev_err(dev, "missing \"reg\"\n");
>diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
>b/drivers/pci/controller/cadence/pcie-cadence.h
>index 574e9cf4d003..042a4c49bb9a 100644
>--- a/drivers/pci/controller/cadence/pcie-cadence.h
>+++ b/drivers/pci/controller/cadence/pcie-cadence.h
>@@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
>  * @ops: Platform-specific ops to control various inputs from Cadence PCIe
>  *       wrapper
>  * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
>+ * @max_link_speed: Maximum supported link speed
>  */
> struct cdns_pcie {
> 	void __iomem		             *reg_base;
>@@ -98,6 +99,7 @@ struct cdns_pcie {
> 	struct device_link	             **link;
> 	const  struct cdns_pcie_ops          *ops;
> 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
>+	int				     max_link_speed;
> };
>
> /**
>--
>2.43.0

RE: [PATCH v4 3/7] PCI: cadence: HPA: Add post-link delay

[Manikandan Karunakaran Pillai]

>EXTERNAL MAIL
>
>
>The Cadence HPA (High Performance Architecture IP) specific link setup
>function cdns_pcie_hpa_host_link_setup() waits for the link to come up
>but does not implement the required 100 ms delay after link training
>completes for speeds > 5.0 GT/s (PCIe r6.0 sec 6.6.1).
>
>Add a call to pci_host_common_link_train_delay() immediately after the
>link is confirmed to be up, using the max_link_speed field. Also, in the
>HPA host setup function, read the device tree property "max-link-speed"
>to initialize max_link_speed if not already set by a glue driver.
>
>This ensures compliance for HPA-based platforms.
>
>Signed-off-by: Hans Zhang <18255117159@163.com>
>---
> drivers/pci/controller/cadence/pcie-cadence-host-hpa.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>index 0f540bed58e8..8ef58ed01daa 100644
>--- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>+++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>@@ -15,6 +15,8 @@
>
> #include "pcie-cadence.h"
> #include "pcie-cadence-host-common.h"
>+#include "../pci-host-common.h"
>+#include "../../pci.h"
>
> static u8 bar_aperture_mask[] = {
> 	[RP_BAR0] = 0x3F,
>@@ -304,6 +306,8 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc
>*rc)
> 	ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
> 	if (ret)
> 		dev_dbg(dev, "PCIe link never came up\n");
>+	else
>+		pci_host_common_link_train_delay(pcie->max_link_speed);
>
> 	return ret;
> }
>@@ -313,6 +317,7 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
> {
> 	struct device *dev = rc->pcie.dev;
> 	struct platform_device *pdev = to_platform_device(dev);
>+	struct device_node *np = dev->of_node;
> 	struct pci_host_bridge *bridge;
> 	enum   cdns_pcie_rp_bar bar;
> 	struct cdns_pcie *pcie;
>@@ -343,6 +348,9 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
> 		rc->cfg_res = res;
> 	}
>
>+	if (pcie->max_link_speed < 1)
>+		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>+

Similar queries as for Cadence LGA controllers. Why do you need the max_link_speed check for "<1" and
What would be the consequences of not defining the max-link-speed in dts ?

> 	/* Put EROM Bar aperture to 0 */
> 	cdns_pcie_hpa_writel(pcie, REG_BANK_IP_CFG_CTRL_REG,
>CDNS_PCIE_EROM, 0x0);
>
>--
>2.43.0

Re: [PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[Hans Zhang]

On 5/18/26 10:12, Manikandan Karunakaran Pillai wrote:
> 
> 
>> EXTERNAL MAIL
>>
>>
>> The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
>> lacks the mandatory 100 ms delay after link training completes for speeds
>>> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.
>>
>> Add a 'max_link_speed' field to struct cdns_pcie. In the common host
>> layer function cdns_pcie_host_start_link(), after the link has been
>> successfully established, call pci_host_common_link_train_delay() to
>> insert the required delay.
>>
>> For the j721e glue driver, set cdns_pcie.max_link_speed from the existing
>> link speed logic. For other LGA-based glue drivers (sky1, sg2042), the
>> common LGA host setup (pcie-cadence-host.c) provides a fallback reading
>> of the device tree property "max-link-speed" when available. This ensures
>> that the delay is not missed on those platforms once they enable the
>> property.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/controller/cadence/pci-j721e.c                | 1 +
>> drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
>> drivers/pci/controller/cadence/pcie-cadence-host.c        | 4 ++++
>> drivers/pci/controller/cadence/pcie-cadence.h             | 2 ++
>> 4 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c
>> b/drivers/pci/controller/cadence/pci-j721e.c
>> index bfdfe98d5aba..ae916e7b1927 100644
>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>> @@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie
>> *pcie,
>> 	    (pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
>> 		link_speed = 2;
>>
>> +	pcie->cdns_pcie->max_link_speed = link_speed;
>> 	val = link_speed - 1;
>> 	ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK,
>> val);
>> 	if (ret)
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>> b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>> index 2b0211870f02..18e4b6c760b5 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-host-common.h"
>>
>> #define LINK_RETRAIN_TIMEOUT HZ
>>
>> @@ -115,6 +116,9 @@ 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);
>>
>> +	if (!ret)
>> +		pci_host_common_link_train_delay(pcie->max_link_speed);
>> +
>> 	return ret;
>> }
>> EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 0bc9e6e90e0e..058e4e619654 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -13,6 +13,7 @@
>>
>> #include "pcie-cadence.h"
>> #include "pcie-cadence-host-common.h"
>> +#include "../../pci.h"
>>
>> static u8 bar_aperture_mask[] = {
>> 	[RP_BAR0] = 0x1F,
>> @@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>> 	rc->device_id = 0xffff;
>> 	of_property_read_u32(np, "device-id", &rc->device_id);
>>
>> +	if (pcie->max_link_speed < 1)
>> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>> +
> Why is the conditional if required here as during cdns_pcie_host_setup(), the value of
> max_link_speed is expected to be '0', unless specifically initialized by the platform code separately.
> 
> What happens if the max_link_speed is not defined in the corresponding dts ? Would not the -EINVAL returned from the function create issues ?

Hi Manikandan,

Please see:

https://github.com/torvalds/linux/blob/v7.1-rc4/drivers/pci/controller/dwc/pcie-designware.c#L191


Best regards,
Hans

> 
>> 	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev,
>> "reg");
>> 	if (IS_ERR(pcie->reg_base)) {
>> 		dev_err(dev, "missing \"reg\"\n");
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
>> b/drivers/pci/controller/cadence/pcie-cadence.h
>> index 574e9cf4d003..042a4c49bb9a 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
>>   * @ops: Platform-specific ops to control various inputs from Cadence PCIe
>>   *       wrapper
>>   * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
>> + * @max_link_speed: Maximum supported link speed
>>   */
>> struct cdns_pcie {
>> 	void __iomem		             *reg_base;
>> @@ -98,6 +99,7 @@ struct cdns_pcie {
>> 	struct device_link	             **link;
>> 	const  struct cdns_pcie_ops          *ops;
>> 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
>> +	int				     max_link_speed;
>> };
>>
>> /**
>> --
>> 2.43.0

Re: [PATCH v4 3/7] PCI: cadence: HPA: Add post-link delay

[Hans Zhang]

On 5/18/26 10:16, Manikandan Karunakaran Pillai wrote:
> 
> 
>> EXTERNAL MAIL
>>
>>
>> The Cadence HPA (High Performance Architecture IP) specific link setup
>> function cdns_pcie_hpa_host_link_setup() waits for the link to come up
>> but does not implement the required 100 ms delay after link training
>> completes for speeds > 5.0 GT/s (PCIe r6.0 sec 6.6.1).
>>
>> Add a call to pci_host_common_link_train_delay() immediately after the
>> link is confirmed to be up, using the max_link_speed field. Also, in the
>> HPA host setup function, read the device tree property "max-link-speed"
>> to initialize max_link_speed if not already set by a glue driver.
>>
>> This ensures compliance for HPA-based platforms.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/controller/cadence/pcie-cadence-host-hpa.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> index 0f540bed58e8..8ef58ed01daa 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> @@ -15,6 +15,8 @@
>>
>> #include "pcie-cadence.h"
>> #include "pcie-cadence-host-common.h"
>> +#include "../pci-host-common.h"
>> +#include "../../pci.h"
>>
>> static u8 bar_aperture_mask[] = {
>> 	[RP_BAR0] = 0x3F,
>> @@ -304,6 +306,8 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc
>> *rc)
>> 	ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
>> 	if (ret)
>> 		dev_dbg(dev, "PCIe link never came up\n");
>> +	else
>> +		pci_host_common_link_train_delay(pcie->max_link_speed);
>>
>> 	return ret;
>> }
>> @@ -313,6 +317,7 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
>> {
>> 	struct device *dev = rc->pcie.dev;
>> 	struct platform_device *pdev = to_platform_device(dev);
>> +	struct device_node *np = dev->of_node;
>> 	struct pci_host_bridge *bridge;
>> 	enum   cdns_pcie_rp_bar bar;
>> 	struct cdns_pcie *pcie;
>> @@ -343,6 +348,9 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
>> 		rc->cfg_res = res;
>> 	}
>>
>> +	if (pcie->max_link_speed < 1)
>> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>> +
> 
> Similar queries as for Cadence LGA controllers. Why do you need the max_link_speed check for "<1" and
> What would be the consequences of not defining the max-link-speed in dts ?

Hi Manikandan,

It has been replied in patch 0002.

Best regards,
Hans

> 
>> 	/* Put EROM Bar aperture to 0 */
>> 	cdns_pcie_hpa_writel(pcie, REG_BANK_IP_CFG_CTRL_REG,
>> CDNS_PCIE_EROM, 0x0);
>>
>> --
>> 2.43.0

Re: [PATCH v4 6/7] PCI: mediatek-gen3: Add 100 ms delay after link up

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The required 100ms link training delay is silently bypassed when the `max-link-speed` property is omitted from the device tree.
--

commit d0afff20acf31b3fe5b43926c6609e07c2edb1cf
Author: Hans Zhang <18255117159@163.com>

PCI: mediatek-gen3: Add 100 ms delay after link up

This commit adds a 100 ms delay after link training completes for speeds
> 5.0 GT/s to comply with the PCIe r6.0 specification. It calls
pci_host_common_link_train_delay() using the max_link_speed value from the
device tree.

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index b0accd8285892..5abddec4e9bec 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
[ ... ]
> @@ -570,6 +571,8 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  		goto err_power_down_device;
>  	}
>  
> +	pci_host_common_link_train_delay(pcie->max_link_speed);

Is the 100ms link training delay silently bypassed here when the
max-link-speed property is omitted from the device tree?

Since pcie->max_link_speed defaults to 0 and is only populated if the
max-link-speed property is explicitly defined in the device tree, a missing
property leaves pcie->max_link_speed as 0. The controller then trains at
its native maximum capability (Gen3), but
pci_host_common_link_train_delay(0) will evaluate 0 > 2 as false,
completely skipping the required 100ms delay.

Additionally, due to a pre-existing inverted check in mtk_pcie_setup()
(if (max_speed >= 0 && max_speed <= err)), even if the link speed is
limited via the device tree, the override is usually ignored, leaving
pcie->max_link_speed as 0.

Does the driver need to determine the actual maximum link speed of the
port (for example, via mtk_pcie_get_controller_max_link_speed()) or read
the negotiated link status, rather than relying exclusively on the
optional device tree override variable?

> +
>  	return 0;
>  
>  err_power_down_device:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=6

RE: [PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[Manikandan Karunakaran Pillai]

>EXTERNAL MAIL
>
>
>
>
>On 5/18/26 10:12, Manikandan Karunakaran Pillai wrote:
>>
>>
>>> EXTERNAL MAIL
>>>
>>>
>>> The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
>>> lacks the mandatory 100 ms delay after link training completes for speeds
>>>> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.
>>>
>>> Add a 'max_link_speed' field to struct cdns_pcie. In the common host
>>> layer function cdns_pcie_host_start_link(), after the link has been
>>> successfully established, call pci_host_common_link_train_delay() to
>>> insert the required delay.
>>>
>>> For the j721e glue driver, set cdns_pcie.max_link_speed from the existing
>>> link speed logic. For other LGA-based glue drivers (sky1, sg2042), the
>>> common LGA host setup (pcie-cadence-host.c) provides a fallback reading
>>> of the device tree property "max-link-speed" when available. This ensures
>>> that the delay is not missed on those platforms once they enable the
>>> property.
>>>
>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>> ---
>>> drivers/pci/controller/cadence/pci-j721e.c                | 1 +
>>> drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
>>> drivers/pci/controller/cadence/pcie-cadence-host.c        | 4 ++++
>>> drivers/pci/controller/cadence/pcie-cadence.h             | 2 ++
>>> 4 files changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c
>>> b/drivers/pci/controller/cadence/pci-j721e.c
>>> index bfdfe98d5aba..ae916e7b1927 100644
>>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>>> @@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct
>j721e_pcie
>>> *pcie,
>>> 	    (pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
>>> 		link_speed = 2;
>>>
>>> +	pcie->cdns_pcie->max_link_speed = link_speed;
>>> 	val = link_speed - 1;
>>> 	ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK,
>>> val);
>>> 	if (ret)
>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>> b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>> index 2b0211870f02..18e4b6c760b5 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-host-common.h"
>>>
>>> #define LINK_RETRAIN_TIMEOUT HZ
>>>
>>> @@ -115,6 +116,9 @@ 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);
>>>
>>> +	if (!ret)
>>> +		pci_host_common_link_train_delay(pcie->max_link_speed);
>>> +
>>> 	return ret;
>>> }
>>> EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> index 0bc9e6e90e0e..058e4e619654 100644
>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> @@ -13,6 +13,7 @@
>>>
>>> #include "pcie-cadence.h"
>>> #include "pcie-cadence-host-common.h"
>>> +#include "../../pci.h"
>>>
>>> static u8 bar_aperture_mask[] = {
>>> 	[RP_BAR0] = 0x1F,
>>> @@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>> 	rc->device_id = 0xffff;
>>> 	of_property_read_u32(np, "device-id", &rc->device_id);
>>>
>>> +	if (pcie->max_link_speed < 1)
>>> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>>> +
>> Why is the conditional if required here as during cdns_pcie_host_setup(), the
>value of
>> max_link_speed is expected to be '0', unless specifically initialized by the
>platform code separately.
>>
>> What happens if the max_link_speed is not defined in the corresponding dts
>? Would not the -EINVAL returned from the function create issues ?
>
>Hi Manikandan,
>
>Please see:
>
>https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/v7.1-
>rc4/drivers/pci/controller/dwc/pcie-
>designware.c*L191__;Iw!!EHscmS1ygiU1lA!EDHVakD3QN0gGza3V1__qzHgDG9
>RZlq7LzC5AFsYLV2i5FcoveNFsjWORRgRdHCAmOI-LizY5cJvGIWBOFJG$
>
>
>Best regards,
>Hans
>
That is how Designware has implemented it but that does not answer my query. Becos both these implementations do 
not take care of the error returned, and it could well be the case for many of the current implementations.

>>
>>> 	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev,
>>> "reg");
>>> 	if (IS_ERR(pcie->reg_base)) {
>>> 		dev_err(dev, "missing \"reg\"\n");
>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
>>> b/drivers/pci/controller/cadence/pcie-cadence.h
>>> index 574e9cf4d003..042a4c49bb9a 100644
>>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>>> @@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
>>>   * @ops: Platform-specific ops to control various inputs from Cadence PCIe
>>>   *       wrapper
>>>   * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
>>> + * @max_link_speed: Maximum supported link speed
>>>   */
>>> struct cdns_pcie {
>>> 	void __iomem		             *reg_base;
>>> @@ -98,6 +99,7 @@ struct cdns_pcie {
>>> 	struct device_link	             **link;
>>> 	const  struct cdns_pcie_ops          *ops;
>>> 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
>>> +	int				     max_link_speed;
>>> };
>>>
>>> /**
>>> --
>>> 2.43.0

Re: [PATCH v4 7/7] PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The mandatory PCIe link training delay is skipped when the 'max-link-speed' device tree property is omitted.
--

commit 66a7457779d07490b3340e0c36c8ac63f1f0a84f
Author: Hans Zhang <18255117159@163.com>

PCI: rzg3s-host: Use common pci_host_common_link_train_delay() helper

This commit replaces an unconditional 100ms msleep with the
pci_host_common_link_train_delay() helper. The delay is now only applied
when max_link_speed is greater than 2.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1663,7 +1664,7 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>  	if (ret)
>  		dev_info(dev, "Failed to set max link speed\n");
>  
> -	msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +	pci_host_common_link_train_delay(host->max_link_speed);

Could this regression skip the mandatory link training delay if the
max-link-speed device tree property is omitted?

If the property is missing, of_pci_get_max_link_speed() returns a negative
error code like -EINVAL, which gets stored in host->max_link_speed.

When rzg3s_pcie_set_max_link_speed() runs, it ignores negative values and
uses the hardware's native capability to configure the link, which could be
Gen3 or Gen4. However, host->max_link_speed is not updated to reflect this
actual link speed.

Because pci_host_common_link_train_delay() takes an int, passing the
negative error code causes the condition to evaluate to false:

drivers/pci/controller/pci-host-common.h:pci_host_common_link_train_delay() {
        if (max_link_speed > 2)
                msleep(PCIE_RESET_CONFIG_WAIT_MS);
}

This skips the 100ms delay even for Gen3+ links, which could cause
enumeration failures for downstream devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518004246.1384532-1-18255117159@163.com?part=7

Re: [PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[Hans Zhang]

On 5/18/26 10:38, Manikandan Karunakaran Pillai wrote:
> 
> 
>> EXTERNAL MAIL
>>
>>
>>
>>
>> On 5/18/26 10:12, Manikandan Karunakaran Pillai wrote:
>>>
>>>
>>>> EXTERNAL MAIL
>>>>
>>>>
>>>> The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
>>>> lacks the mandatory 100 ms delay after link training completes for speeds
>>>>> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.
>>>>
>>>> Add a 'max_link_speed' field to struct cdns_pcie. In the common host
>>>> layer function cdns_pcie_host_start_link(), after the link has been
>>>> successfully established, call pci_host_common_link_train_delay() to
>>>> insert the required delay.
>>>>
>>>> For the j721e glue driver, set cdns_pcie.max_link_speed from the existing
>>>> link speed logic. For other LGA-based glue drivers (sky1, sg2042), the
>>>> common LGA host setup (pcie-cadence-host.c) provides a fallback reading
>>>> of the device tree property "max-link-speed" when available. This ensures
>>>> that the delay is not missed on those platforms once they enable the
>>>> property.
>>>>
>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>> ---
>>>> drivers/pci/controller/cadence/pci-j721e.c                | 1 +
>>>> drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
>>>> drivers/pci/controller/cadence/pcie-cadence-host.c        | 4 ++++
>>>> drivers/pci/controller/cadence/pcie-cadence.h             | 2 ++
>>>> 4 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c
>>>> b/drivers/pci/controller/cadence/pci-j721e.c
>>>> index bfdfe98d5aba..ae916e7b1927 100644
>>>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>>>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>>>> @@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct
>> j721e_pcie
>>>> *pcie,
>>>> 	    (pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
>>>> 		link_speed = 2;
>>>>
>>>> +	pcie->cdns_pcie->max_link_speed = link_speed;
>>>> 	val = link_speed - 1;
>>>> 	ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK,
>>>> val);
>>>> 	if (ret)
>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>> b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
>>>> index 2b0211870f02..18e4b6c760b5 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-host-common.h"
>>>>
>>>> #define LINK_RETRAIN_TIMEOUT HZ
>>>>
>>>> @@ -115,6 +116,9 @@ 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);
>>>>
>>>> +	if (!ret)
>>>> +		pci_host_common_link_train_delay(pcie->max_link_speed);
>>>> +
>>>> 	return ret;
>>>> }
>>>> EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> index 0bc9e6e90e0e..058e4e619654 100644
>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> @@ -13,6 +13,7 @@
>>>>
>>>> #include "pcie-cadence.h"
>>>> #include "pcie-cadence-host-common.h"
>>>> +#include "../../pci.h"
>>>>
>>>> static u8 bar_aperture_mask[] = {
>>>> 	[RP_BAR0] = 0x1F,
>>>> @@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>> 	rc->device_id = 0xffff;
>>>> 	of_property_read_u32(np, "device-id", &rc->device_id);
>>>>
>>>> +	if (pcie->max_link_speed < 1)
>>>> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>>>> +
>>> Why is the conditional if required here as during cdns_pcie_host_setup(), the
>> value of
>>> max_link_speed is expected to be '0', unless specifically initialized by the
>> platform code separately.
>>>
>>> What happens if the max_link_speed is not defined in the corresponding dts
>> ? Would not the -EINVAL returned from the function create issues ?
>>
>> Hi Manikandan,
>>
>> Please see:
>>
>> https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/v7.1-
>> rc4/drivers/pci/controller/dwc/pcie-
>> designware.c*L191__;Iw!!EHscmS1ygiU1lA!EDHVakD3QN0gGza3V1__qzHgDG9
>> RZlq7LzC5AFsYLV2i5FcoveNFsjWORRgRdHCAmOI-LizY5cJvGIWBOFJG$
>>
>>
>> Best regards,
>> Hans
>>
> That is how Designware has implemented it but that does not answer my query. Becos both these implementations do
> not take care of the error returned, and it could well be the case for many of the current implementations.

Hi Manikandan,

If "max-link-speed" is not defined in the DT, then:

of_pci_get_max_link_speed
   of_property_read_u32
     of_property_read_u32_array
       of_property_read_variable_u32_array
         return -EINVAL;


For patch 0001, no actions will be executed. I wonder if this answers 
your question?

Best regards,
Hans


> 
>>>
>>>> 	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev,
>>>> "reg");
>>>> 	if (IS_ERR(pcie->reg_base)) {
>>>> 		dev_err(dev, "missing \"reg\"\n");
>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
>>>> b/drivers/pci/controller/cadence/pcie-cadence.h
>>>> index 574e9cf4d003..042a4c49bb9a 100644
>>>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>>>> @@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
>>>>    * @ops: Platform-specific ops to control various inputs from Cadence PCIe
>>>>    *       wrapper
>>>>    * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
>>>> + * @max_link_speed: Maximum supported link speed
>>>>    */
>>>> struct cdns_pcie {
>>>> 	void __iomem		             *reg_base;
>>>> @@ -98,6 +99,7 @@ struct cdns_pcie {
>>>> 	struct device_link	             **link;
>>>> 	const  struct cdns_pcie_ops          *ops;
>>>> 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
>>>> +	int				     max_link_speed;
>>>> };
>>>>
>>>> /**
>>>> --
>>>> 2.43.0
> 

RE: [PATCH v4 2/7] PCI: cadence: Add post-link delay for LGA and j721e glue driver

[Manikandan Karunakaran Pillai]

>>>>>
>>>>> #include "pcie-cadence.h"
>>>>> #include "pcie-cadence-host-common.h"
>>>>> +#include "../pci-host-common.h"
>>>>>
>>>>> #define LINK_RETRAIN_TIMEOUT HZ
>>>>>
>>>>> @@ -115,6 +116,9 @@ 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);
>>>>>
>>>>> +	if (!ret)
>>>>> +		pci_host_common_link_train_delay(pcie->max_link_speed);
>>>>> +
>>>>> 	return ret;
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> index 0bc9e6e90e0e..058e4e619654 100644
>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> @@ -13,6 +13,7 @@
>>>>>
>>>>> #include "pcie-cadence.h"
>>>>> #include "pcie-cadence-host-common.h"
>>>>> +#include "../../pci.h"
>>>>>
>>>>> static u8 bar_aperture_mask[] = {
>>>>> 	[RP_BAR0] = 0x1F,
>>>>> @@ -397,6 +398,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>> 	rc->device_id = 0xffff;
>>>>> 	of_property_read_u32(np, "device-id", &rc->device_id);
>>>>>
>>>>> +	if (pcie->max_link_speed < 1)
>>>>> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);
>>>>> +
>>>> Why is the conditional if required here as during cdns_pcie_host_setup(),
>the
>>> value of
>>>> max_link_speed is expected to be '0', unless specifically initialized by the
>>> platform code separately.
>>>>
>>>> What happens if the max_link_speed is not defined in the corresponding
>dts
>>> ? Would not the -EINVAL returned from the function create issues ?
>>>
>>> Hi Manikandan,
>>>
>>> Please see:
>>>
>>> https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/v7.1-
>>> rc4/drivers/pci/controller/dwc/pcie-
>>>
>designware.c*L191__;Iw!!EHscmS1ygiU1lA!EDHVakD3QN0gGza3V1__qzHgDG9
>>> RZlq7LzC5AFsYLV2i5FcoveNFsjWORRgRdHCAmOI-LizY5cJvGIWBOFJG$
>>>
>>>
>>> Best regards,
>>> Hans
>>>
>> That is how Designware has implemented it but that does not answer my
>query. Becos both these implementations do
>> not take care of the error returned, and it could well be the case for many of
>the current implementations.
>
>Hi Manikandan,
>
>If "max-link-speed" is not defined in the DT, then:
>
>of_pci_get_max_link_speed
>   of_property_read_u32
>     of_property_read_u32_array
>       of_property_read_variable_u32_array
>         return -EINVAL;
>
>
>For patch 0001, no actions will be executed. I wonder if this answers
>your question?
>
Yes, got it. Thanks Hans.

>Best regards,
>Hans
>
>
>>
>>>>
>>>>> 	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev,
>>>>> "reg");
>>>>> 	if (IS_ERR(pcie->reg_base)) {
>>>>> 		dev_err(dev, "missing \"reg\"\n");
>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
>>>>> b/drivers/pci/controller/cadence/pcie-cadence.h
>>>>> index 574e9cf4d003..042a4c49bb9a 100644
>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>>>>> @@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
>>>>>    * @ops: Platform-specific ops to control various inputs from Cadence
>PCIe
>>>>>    *       wrapper
>>>>>    * @cdns_pcie_reg_offsets: Register bank offsets for different SoC
>>>>> + * @max_link_speed: Maximum supported link speed
>>>>>    */
>>>>> struct cdns_pcie {
>>>>> 	void __iomem		             *reg_base;
>>>>> @@ -98,6 +99,7 @@ struct cdns_pcie {
>>>>> 	struct device_link	             **link;
>>>>> 	const  struct cdns_pcie_ops          *ops;
>>>>> 	const  struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
>>>>> +	int				     max_link_speed;
>>>>> };
>>>>>
>>>>> /**
>>>>> --
>>>>> 2.43.0
>>