Re: [PATCH v2 5/5] PCI: qcom: Add D3cold support

[Manivannan Sadhasivam]

On Thu, Mar 05, 2026 at 02:55:25PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 3/5/2026 2:44 PM, Manivannan Sadhasivam wrote:
> > On Thu, Mar 05, 2026 at 02:30:17PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > On 3/5/2026 1:28 PM, Manivannan Sadhasivam wrote:
> > > > On Tue, Feb 17, 2026 at 04:49:10PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > Add support for transitioning Qcom PCIe controllers into D3cold by
> > > > You cannot transition a 'PCIe controller' to D3Cold state, but only the
> > > > endpoints and bridges.
> > > > 
> > > > > integrating with the DWC core suspend/resume helpers.
> > > > > 
> > > > > Implement PME_TurnOff message generation via ELBI_SYS_CTRL and hook it
> > > > > into the DWC host operations so the controller follows the standard
> > > > > PME_TurnOff-based power-down sequence before entering D3cold.
> > > > > 
> > > > > When the link is suspended into D3cold, fully tear down interconnect
> > > > You cannot suspend a link into D3Cold. Link and D-State are different.
> > > > 
> > > > > bandwidth, OPP votes. If D3cold is not entered, retain existing behavior
> > > > > by keeping the required interconnect and OPP votes.
> > > > > 
> > > > > Drop the qcom_pcie::suspended flag and rely on the existing
> > > > > dw_pcie::suspended state, which now drives both the power-management
> > > > > flow and the interconnect/OPP handling.
> > > > > 
> > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >    drivers/pci/controller/dwc/pcie-qcom.c | 121 ++++++++++++++++++++-------------
> > > > >    1 file changed, 74 insertions(+), 47 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > index b02c19bbdf2ea5db252c2a0281a569bb3a0cc497..37442bbe588c36b0b0414cc4d0016da2d8424a87 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > @@ -145,6 +145,7 @@
> > > > [...]
> > > > 
> > > > > -	if (pcie->suspended) {
> > > > > -		ret = qcom_pcie_host_init(&pcie->pci->pp);
> > > > > -		if (ret)
> > > > > -			return ret;
> > > > > +		ret = icc_enable(pcie->icc_mem);
> > > > > +		if (ret) {
> > > > > +			dev_err(dev, "Failed to enable PCIe-MEM interconnect path: %d\n", ret);
> > > > > +			goto disable_icc_cpu;
> > > > > +		}
> > > > > -		pcie->suspended = false;
> > > > > +		/*
> > > > > +		 * Ignore -ETIMEDOUT here since it is expected when no endpoint is
> > > > > +		 * connected to the PCIe link.
> > > > > +		 */
> > > > > +		ret = dw_pcie_resume_noirq(pcie->pci);
> > > > > +		if (ret && (ret != -ETIMEDOUT))
> > > > No, dw_pcie_resume_noirq() was reworked to return -ETIMEDOUT to indicate a hard
> > > > failure. If the device is not found, it will return -ENODEV. So you should
> > > > fail the resume if -ETIMEDOUT is returned.
> > > Ack, didn't noticed the reworked changes, I will change -ETIMEDOUT to
> > > -ENODEV.
> > > 
> > No, that's what not I meant. I meant, you should do:
> > 
> > 	if (ret == -ETIMEDOUT)
> > 		goto fail;
> there can be other failures also right, where we should fail,
> like pci->pp.ops->init(&pci->pp); can return different error other than
> -ETIMEDOUT in that case we should fail here. - Krishna Chaitanya.

Hmm, I overlooked the init() callback. In that case, you should skip both
-ENODEV and -EIO and fail resume() for other errors.

- Mani

-- 
மணிவண்ணன் சதாசிவம்