Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 5 Jan 2023 19:03:21 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Johan Hovold <johan@kernel.org>
Cc:     lpieralisi@kernel.org, robh@kernel.org, andersson@kernel.org,
        konrad.dybcio@linaro.org, kw@linux.com, bhelgaas@google.com,
        linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, quic_krichai@quicinc.com,
        johan+linaro@kernel.org, steev@kali.org
Subject: Re: [PATCH 1/1] PCI: qcom: Add support for system suspend and resume
Message-ID: <20230105133321.GB4463@thinkpad>
References: <20230103074907.12784-1-manivannan.sadhasivam@linaro.org>
 <20230103074907.12784-2-manivannan.sadhasivam@linaro.org>
 <Y7Qqv+kyREvXdRu1@hovoldconsulting.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y7Qqv+kyREvXdRu1@hovoldconsulting.com>
Precedence: bulk

On Tue, Jan 03, 2023 at 02:16:47PM +0100, Johan Hovold wrote:
> On Tue, Jan 03, 2023 at 01:19:07PM +0530, Manivannan Sadhasivam wrote:
> > During the system suspend, vote for minimal interconnect bandwidth and
> > also turn OFF the resources like clock and PHY if there are no active
> > devices connected to the controller. For the controllers with active
> > devices, the resources are kept ON as removing the resources will
> > trigger access violation during the late end of suspend cycle as kernel
> > tries to access the config space of PCIe devices to mask the 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 storage devices like NVMe.
> > 
> > And finally, during resume, turn ON the resources if the controller was
> > truly suspended (resources OFF) and update the interconnect bandwidth
> > based on PCIe Gen speed.
> > 
> > Suggested-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> 
> I just gave this a quick spin on the sc8280xp-crd, and unfortunately
> this change appears to break suspend (e.g. hangs during suspend or
> resume). Setting a non-zero (250 MBps) peak bandwidth during suspend
> makes things work again.
> 
> Presumably something is relying on these interconnect clocks to remain
> enabled. And isn't that expected as we need to set a non-zero icc bw to
> enable the interconnect clocks during probe?
> 

After suspend, I assumed that there won't be any access to the controller
specific registers, so thought it should be fine. And it works on X13s too.
Maybe, the access to device config space is triggering issues on CRD? I will
check with Qcom.

> I'm afraid I won't have time to look into this for a while myself, but
> have you tried this on the CRD, Mani? 
> 

Thanks for testing, Johan!

I did not test this on CRD. Since both X13s and CRD are sharing the same
SoC, I thought it would work on CRD too. But since you have tested and
reported the issue, I will look into it.

> One obvious difference is the modem on the CRD which I believe neither
> of our X13s have, but this seems like more of a general problem.
> 

Yeah, this seems to be a platform issue. I will check on this behaviour and
report back.

Thanks,
Mani

> Johan

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