Message-ID: <qg7hyicnyqcoi7jktalyfecfgxnv4elilmmmfkzvb2jbzrhisd@bq3pngh5n2a7>
Date: Wed, 28 Jan 2026 08:47:24 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Jingoo Han <jingoohan1@...il.com>, 
	Manivannan Sadhasivam <mani@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kwilczynski@...nel.org>, Rob Herring <robh@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Will Deacon <will@...nel.org>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, jonathanh@...dia.com
Subject: Re: [PATCH 3/3] PCI: qcom: Add D3cold support

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

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

Powered by blists - more mailing lists