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

[Aksh Garg <a-garg7@ti.com>]

On 15/07/26 11:31, Manivannan Sadhasivam wrote:
> On Tue, Jun 23, 2026 at 03:21:48PM +0530, 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 link_down_no_hotplug flag to track link state across suspend/resume
>> cycles (for the platforms that do not support hotplug). If link was
>> down before suspend in such platforms, skip the expensive polling in
>> resume since no endpoint was present.
>>
>> Reviewed-by: Chen Wang <unicorn_wang@outlook.com>
>> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Signed-off-by: Aksh Garg <a-garg7@ti.com>
>> ---
>>
>> Changes from v1 to v2:
>> - Updated the flag name from 'skip_link_polling' to 'link_down_no_hotplug'
>>
>> v1: https://lore.kernel.org/all/20260605071922.1724499-1-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..48db7a6cf754 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->link_down_no_hotplug = !j721e_pcie_link_up(pcie->cdns_pcie);
>> +
>>   		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
>>   		clk_disable_unprepare(pcie->refclk);
>>   	}
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> index 0f540bed58e8..31cf50cff8f8 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
>> @@ -301,6 +301,9 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc *rc)
>>   		return ret;
>>   	}
>>   
>> +	if (rc->link_down_no_hotplug)
>> +		return 0;
>> +
>>   	ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
>>   	if (ret)
>>   		dev_dbg(dev, "PCIe link never came up\n");
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 0bc9e6e90e0e..50abc657a871 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -352,6 +352,9 @@ int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
>>   		return ret;
>>   	}
>>   
>> +	if (rc->link_down_no_hotplug)
>> +		return 0;
>> +
>>   	ret = cdns_pcie_host_start_link(rc, cdns_pcie_link_up);
>>   	if (ret)
>>   		dev_dbg(dev, "PCIe link never came up\n");
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index 574e9cf4d003..1561022c1a8b 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -117,6 +117,8 @@ struct cdns_pcie {
>>    * @no_inbound_map: Whether inbound mapping is supported
>>    * @quirk_broken_aspm_l0s: Disable ASPM L0s support as quirk
>>    * @quirk_broken_aspm_l1: Disable ASPM L1 support as quirk
>> + * @link_down_no_hotplug: Skip link polling during resume on no-hotplug
>> + *			  platforms when link was down before suspend
>>    */
>>   struct cdns_pcie_rc {
>>   	struct cdns_pcie	pcie;
>> @@ -131,6 +133,7 @@ struct cdns_pcie_rc {
>>   	unsigned int            no_inbound_map:1;
>>   	unsigned int            quirk_broken_aspm_l0s:1;
>>   	unsigned int            quirk_broken_aspm_l1:1;
>> +	unsigned int		link_down_no_hotplug:1;
> 
> If the goal of this flag is to convey the information that the platform doesn't
> support hotplug, then it should be called 'no_hotplug'. Skipping the polling
> during LDn is the usecase.

Hi Mani,

The goal of the flag "link_down_no_hotplug" is to convey both: the
platform doesn't support hotplug, as well as the link is down (as both
of these conditions must be true to skip the polling). Hence, I chose to
name the flag this way.

If we choose to name the flag "no_hotplug", then we would need to add
another flag "link_down", and then skip the polling
"if (rc->link_down && rc->no_hotplug)".

Regards,
Aksh Garg

> 
> - Mani
>