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

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 <[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 ?
>>
>> 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
>