Re: [PATCH] PCI: qcom: Prevent GDSC power down on suspend

[Manivannan Sadhasivam]

On Wed, Jan 28, 2026 at 08:13:48AM -0600, Bjorn Andersson wrote:
> On Wed, Jan 28, 2026 at 05:52:42PM +0530, Krishna Chaitanya Chundru wrote:
> > Currently, the driver expects the devices to remain in D0 across system
> > suspend, but the genpd framework may still power down the associated
> > GDSC during suspend. When that happens, the PCIe link goes down and
> > cannot be recovered on resume.
> > 
> 
> The GDSC is a child of CX, so by keeping it always-on, you effectively
> put an always-on vote on CX, forever preventing CXPC.
> 
> In fact, this is one of the reasons why the PCIe GDSCs on most targets
> is marked PWRSTS_RET_ON (in the clock driver) so that the "off state"
> doesn't actually turn off the GDSC, but it relinquishes the inherited
> vote on CX.
> 

So this means, you favor the patch that marks the PCIe GDSCs as PWRSTS_RET_ON?

> > Prevent genpd from turning off the PCIe GDSC by using
> > dev_pm_genpd_rpm_always_on() so that the power domain stays on while
> > the controller is suspended. This preserves the link state across
> > suspend/resume and avoids unrecoverable link failures.
> > 
> 
> We are able to suspend/resume a whole bunch of platforms today, which
> one are you on?
> 
> That said, while we can suspend/resume, we're not allowing CXPC today.
> On many systems the main culprit is the icc_set_bw() vote in
> qcom_pcie_suspend_noirq().
> 

Yeah, I still need to look deeply into this part. The stray vote keeps the PCIe
link active as irq core tries to masks the MSIs at the very end of suspend. This
design works fine for firmware controlled suspends, but not for kernel
controlled ones.

- Mani

> Regards,
> Bjorn
> 
> > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 5a318487b2b3f6c61d8f5b1fd5cdf2738a1f1dcd..314cf334a313dff35efaf0c023597e6eef483925 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/pci-ecam.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/pm_domain.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/phy/pcie.h>
> > @@ -2052,6 +2053,11 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> >  		pcie->suspended = true;
> >  	}
> >  
> > +	if (pcie->suspended)
> > +		dev_pm_genpd_rpm_always_on(dev, false);
> > +	else
> > +		dev_pm_genpd_rpm_always_on(dev, true);
> > +
> >  	/*
> >  	 * Only disable CPU-PCIe interconnect path if the suspend is non-S2RAM.
> >  	 * Because on some platforms, DBI access can happen very late during the
> > 
> > ---
> > base-commit: 1f97d9dcf53649c41c33227b345a36902cbb08ad
> > change-id: 20260128-genpd_fix-3aa413d9a383
> > 
> > Best regards,
> > -- 
> > Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
> > 
> > 

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