Re: [PATCH v7 1/5] PCI: qcom: Add system suspend and resume support

From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: <linux-pci@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <mka@chromium.org>,
	<quic_vbadigan@quicinc.com>, <quic_hemantk@quicinc.com>,
	<quic_nitegupt@quicinc.com>, <quic_skananth@quicinc.com>,
	<quic_ramkri@quicinc.com>, <manivannan.sadhasivam@linaro.org>,
	<swboyd@chromium.org>, <dmitry.baryshkov@linaro.org>,
	<svarbanov@mm-sol.com>, <agross@kernel.org>,
	<andersson@kernel.org>, <konrad.dybcio@somainline.org>,
	<lpieralisi@kernel.org>, <robh@kernel.org>, <kw@linux.com>,
	<bhelgaas@google.com>, <linux-phy@lists.infradead.org>,
	<vkoul@kernel.org>, <kishon@ti.com>, <mturquette@baylibre.com>,
	<linux-clk@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [PATCH v7 1/5] PCI: qcom: Add system suspend and resume support
Date: Wed, 21 Sep 2022 15:23:35 +0530	[thread overview]
Message-ID: <ed1a1260-0ef4-203a-f073-f5232bfc8466@quicinc.com> (raw)
In-Reply-To: <20220920181637.GA1121154@bhelgaas>

On 9/20/2022 11:46 PM, Bjorn Helgaas wrote:
> On Tue, Sep 20, 2022 at 03:52:23PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume syscore ops.
>>
>> Few PCIe endpoints like NVMe and WLANs are always expecting the device
>> to be in D0 state and the link to be active (or in l1ss) all the time
>> (including in S3 state).
> What does this have to do with the patch?  I don't see any NVMe or
> WLAN patches here.
Existing NVMe driver expecting NVMe device to be in D0 during S3 also. 
If we turn off the link in
suspend, the NVMe resume path is broken as the state machine is getting 
reset in the NVMe device.
Due to this, the host driver state machine and the device state machine 
are going out of sync, and all NVMe commands
after resumes are getting timed out.

IIRC, Tegra is also facing this issue with NVMe.

This issue has been discussed below threads:

https://lore.kernel.org/all/Yl+6V3pWuyRYuVV8@infradead.org/T/

https://lore.kernel.org/linux-nvme/20220201165006.3074615-1-kbusch@kernel.org/

>> In qcom platform PCIe resources( clocks, phy etc..) can released
>> when the link is in L1ss to reduce the power consumption. So if the link
>> is in L1ss, release the PCIe resources. And when the system resumes,
>> enable the PCIe resources if they released in the suspend path.
> What's the connection with L1.x?  Links enter L1.x based on activity
> and timing.  That doesn't seem like a reliable indicator to turn PHYs
> off and disable clocks.
This is a Qcom PHY-specific feature (retaining the link state in L1.x 
with clocks turned off).
It is possible only with the link being in l1.x. PHY can't retain the 
link state in L0 with the
clocks turned off and we need to re-train the link if it's in L2 or L3. 
So we can support this feature only with L1.x.
That is the reason we are taking l1.x as the trigger to turn off clocks 
(in only suspend path).
>> is_suspended flag indicates if the PCIe resources are released or not
>> in the suspend path.
> Why is "is_suspended" important for the commit log?  It looks like
> just a standard implementation detail.
Someone in one of the previous patch asked to include this in the commit 
text.
>> Its observed that access to Ep PCIe space to mask MSI/MSIX is happening
>> at the very late stage of suspend path (access by affinity changes while
>> making CPUs offline during suspend, this will happen after devices are
>> suspended (after all phases of suspend ops)). If we turn off clocks in
>> any PM callback, afterwards running into crashes due to un-clocked access
>> due to above mentioned MSI/MSIx access.
>> So, we are making use of syscore framework to turn off the PCIe clocks
>> which will be called after making CPUs offline.
> Add blank lines between paragraphs.  Or rewrap into a single paragraph.
>
> s/Its observed/It's observed/
> s/MSIX/MSI-X/ throughout
> s/MSIx/MSI-X/ throughout
>
> Bjorn