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

From: Krishna Chaitanya Chundru

Date: Thu Jan 29 2026 - 00:29:58 EST

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@xxxxxxxxxxxxxxxx>
---
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