Re: [PATCH 3/3] PCI: qcom: Add D3cold support

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 1/28/2026 8:17 PM, Bjorn Andersson wrote:
> On Wed, Jan 28, 2026 at 05:10:43PM +0530, Krishna Chaitanya Chundru wrote:
>> Add pme_turn_off() support and use DWC common suspend resume methods
>> for device D3cold entry & exit. If the device is not kept in D3cold
>> use existing methods like keeping icc votes, opp votes etc.. intact.
>>
>> In qcom_pcie_deinit_2_7_0(), explicitly disable PCIe clocks and resets
>> in the controller.
>>
>> Remove suspended flag from qcom_pcie structure as it is no longer needed.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 114 ++++++++++++++++++++-------------
>>   1 file changed, 68 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> [..]
>> @@ -2016,53 +2030,51 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>>   	if (!pcie)
>>   		return 0;
>>   
>> -	/*
>> -	 * Set minimum bandwidth required to keep data path functional during
>> -	 * suspend.
>> -	 */
>> -	if (pcie->icc_mem) {
>> -		ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> -		if (ret) {
>> -			dev_err(dev,
>> -				"Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
>> -				ret);
>> -			return ret;
>> -		}
>> -	}
>> +	ret = dw_pcie_suspend_noirq(pcie->pci);
>> +	if (ret)
>> +		return ret;
>>   
>> -	/*
>> -	 * Turn OFF the resources only for controllers without active PCIe
>> -	 * devices. For controllers with active devices, the resources are kept
>> -	 * ON and the link is expected to be in L0/L1 (sub)states.
>> -	 *
>> -	 * Turning OFF the resources for controllers with active PCIe devices
>> -	 * will trigger access violation during the end of the suspend cycle,
>> -	 * as kernel tries to access the PCIe devices config space for masking
>> -	 * MSIs.
>> -	 *
>> -	 * Also, it is not desirable to put the link into L2/L3 state as that
>> -	 * implies VDD supply will be removed and the devices may go into
>> -	 * powerdown state. This will affect the lifetime of the storage devices
>> -	 * like NVMe.
>> -	 */
>> -	if (!dw_pcie_link_up(pcie->pci)) {
>> -		qcom_pcie_host_deinit(&pcie->pci->pp);
>> -		pcie->suspended = true;
>> -	}
>> +	if (pcie->pci->suspended) {
> I think this is okay for now, but I'd prefer changing the return value
> of dw_pcie_suspend_noirq() to indicate if it did stop the link or not
> (two different success values) - rather than deriving that information
> by peeking into the dw_pcie struct and conclude that
> dw_pcie_suspend_noirq() did reach the end.
>
>> +		ret = icc_disable(pcie->icc_mem);
>> +		if (ret)
>> +			dev_err(dev, "Failed to disable PCIe-MEM interconnect path: %d\n", ret);
>>   
>> -	/*
>> -	 * Only disable CPU-PCIe interconnect path if the suspend is non-S2RAM.
>> -	 * Because on some platforms, DBI access can happen very late during the
>> -	 * S2RAM and a non-active CPU-PCIe interconnect path may lead to NoC
>> -	 * error.
>> -	 */
>> -	if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>>   		ret = icc_disable(pcie->icc_cpu);
>>   		if (ret)
>>   			dev_err(dev, "Failed to disable CPU-PCIe interconnect path: %d\n", ret);
>>   
>>   		if (pcie->use_pm_opp)
>>   			dev_pm_opp_set_opp(pcie->pci->dev, NULL);
>> +	} else {
>> +		/*
>> +		 * Set minimum bandwidth required to keep data path functional during
>> +		 * suspend.
>> +		 */
>> +		if (pcie->icc_mem) {
>> +			ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> +			if (ret) {
>> +				dev_err(dev,
>> +					"Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
>> +					ret);
>> +				return ret;
>> +			}
>> +		}
>> +
>> +		/*
>> +		 * Only disable CPU-PCIe interconnect path if the suspend is non-S2RAM.
>> +		 * Because on some platforms, DBI access can happen very late during the
>> +		 * S2RAM and a non-active CPU-PCIe interconnect path may lead to NoC
>> +		 * error.
>> +		 */
>> +		if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> +			ret = icc_disable(pcie->icc_cpu);
>> +			if (ret)
>> +				dev_err(dev, "Failed to disable CPU-PCIe interconnect path: %d\n",
>> +					ret);
>> +
>> +			if (pcie->use_pm_opp)
>> +				dev_pm_opp_set_opp(pcie->pci->dev, NULL);
>> +		}
>>   	}
>>   	return ret;
>>   }
>> @@ -2076,20 +2088,30 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>>   	if (!pcie)
>>   		return 0;
>>   
>> -	if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> +	if (pcie->pci->suspended) {
>>   		ret = icc_enable(pcie->icc_cpu);
>>   		if (ret) {
>>   			dev_err(dev, "Failed to enable CPU-PCIe interconnect path: %d\n", ret);
>>   			return ret;
>>   		}
>> -	}
>>   
>> -	if (pcie->suspended) {
>> -		ret = qcom_pcie_host_init(&pcie->pci->pp);
>> +		ret = icc_enable(pcie->icc_mem);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to enable PCIe-MEM interconnect path: %d\n", ret);
> I think you should revert icc_enable(pcie->icc_cpu) here, to avoid
> leaving the bus voted for with the PCIe controller resume aborted.
>
>> +			return ret;
>> +		}
>> +		ret = dw_pcie_resume_noirq(pcie->pci);
>>   		if (ret)
> And Both icc_cpu and icc_mem here.
Ack, I will do this in V2.

- Krishna Chaitanya.
> Regards,
> Bjorn
>
>>   			return ret;
>> -
>> -		pcie->suspended = false;
>> +	} else {
>> +		if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> +			ret = icc_enable(pcie->icc_cpu);
>> +			if (ret) {
>> +				dev_err(dev, "Failed to enable CPU-PCIe interconnect path: %d\n",
>> +					ret);
>> +				return ret;
>> +			}
>> +		}
>>   	}
>>   
>>   	qcom_pcie_icc_opp_update(pcie);
>>
>> -- 
>> 2.34.1
>>
>>