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