Re: [PATCH] PCI: cadence: skip the link polling when endpoint not connected

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On 09/06/26 09:58, Aksh Garg wrote:
> 
> 
> On 09/06/26 09:12, Siddharth Vadapalli wrote:
>> On 05/06/26 12:49, Aksh Garg wrote:
>>> cdns_pcie_host_wait_for_link() polls on link-up for 10 retries with a
>>> delay of 90-100ms each (~1 second). A call to cdns_pcie_host_link_setup()
>>> during the resume operation blocks the resume operation unnecessarily for
>>> ~1s even when no endpoint device is connected.
>>>
>>> Add skip_link_polling flag to track link state across suspend/resume
>>> cycles. If link was down before suspend, skip the expensive polling
>>> in resume since no endpoint was present.
>>>
>>> Signed-off-by: Aksh Garg <a-garg7@ti.com>
>>> ---
>>>   drivers/pci/controller/cadence/pci-j721e.c             | 5 +++++
>>>   drivers/pci/controller/cadence/pcie-cadence-host-hpa.c | 3 +++
>>>   drivers/pci/controller/cadence/pcie-cadence-host.c     | 3 +++
>>>   drivers/pci/controller/cadence/pcie-cadence.h          | 3 +++
>>>   4 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/ 
>>> controller/cadence/pci-j721e.c
>>> index bfdfe98d5aba..849eb8bb9e45 100644
>>> --- a/drivers/pci/controller/cadence/pci-j721e.c
>>> +++ b/drivers/pci/controller/cadence/pci-j721e.c
>>> @@ -686,6 +686,11 @@ static int j721e_pcie_suspend_noirq(struct device 
>>> *dev)
>>>       struct j721e_pcie *pcie = dev_get_drvdata(dev);
>>>       if (pcie->mode == PCI_MODE_RC) {
>>> +        struct cdns_pcie_rc *rc = cdns_pcie_to_rc(pcie->cdns_pcie);
>>> +
>>> +        /* If link is down before suspend, skip polling in resume */
>>> +        rc->skip_link_polling = !j721e_pcie_link_up(pcie->cdns_pcie);
>>> +
>>
>> Please consider the following scenario:
>> 1. Endpoint device is not connected initially and we suspend with 
>> 'skip_link_polling' set to true.
>> 2. We resume and skip link polling.
>> 3. An Endpoint device is connected to the PCIe Connector a while later.
>> 4. We perform a bus rescan, but with skip_link_polling still set (in 
>> resume), wouldn't we skip polling for the link to be up?
>>
>> Shouldn't 'skip_link_polling' be set to false on resume, after we skip 
>> the polling initially, in order to allow waiting for a link up for a 
>> newly connected device? Although this may not be done in practice and the 
>> Endpoint device may always be left connected from the start, I am 
>> wondering if we should clear 'skip_link_polling' on resume to account for 
>> the aforementioned scenario. Let me know if I am overlooking something here.
> 
> This path is not used during bus rescan. This implies that during the 
> rescan, we do not call cdns_pcie_host_link_setup() or 
> cdns_pcie_hpa_host_link_setup().
Thank you for clarifying.

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Regards,
Siddharth.