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

>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 <[email protected]>
>---
> 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