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

>>>>>
>>>>> #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
>>