Re: [PATCH v2 04/12] PCI: qcom: Add support for disabling ASPM L0s in devicetree

From: Bjorn Helgaas <helgaas@kernel.org>
To: Johan Hovold <johan@kernel.org>
Cc: "Johan Hovold" <johan+linaro@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v2 04/12] PCI: qcom: Add support for disabling ASPM L0s in devicetree
Date: Wed, 28 Feb 2024 16:02:15 -0600	[thread overview]
Message-ID: <20240228220215.GA308296@bhelgaas> (raw)
In-Reply-To: <Zd3_y_jbY8Ic2F0Y@hovoldconsulting.com>

On Tue, Feb 27, 2024 at 04:29:15PM +0100, Johan Hovold wrote:
> On Fri, Feb 23, 2024 at 04:10:00PM -0600, Bjorn Helgaas wrote:
> > On Fri, Feb 23, 2024 at 04:21:16PM +0100, Johan Hovold wrote:
> > > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting
> > > 1.9.0 ops") started enabling ASPM unconditionally when the hardware
> > > claims to support it. This triggers Correctable Errors for some PCIe
> > > devices on machines like the Lenovo ThinkPad X13s, which could indicate
> > > an incomplete driver ASPM implementation or that the hardware does in
> > > fact not support L0s.
> > 
> > Are there any more details about this?  Do the errors occur around
> > suspend/resume, a power state transition, or some other event?  Might
> > other DWC-based devices be susceptible?  Is there a specific driver
> > you suspect might be incomplete?
> 
> I see these errors when the devices in question are active as well as
> idle (not during suspend/resume). For example, when running iperf3 or
> fio to test the wifi and nvme, but I also see this occasionally for a
> wifi device which is (supposedly) not active (e.g. a handful errors over
> night).
> 
> I skimmed Qualcomm's driver and noted that there are some registers
> related to ASPM which that driver updates, while the mainline driver
> leaves them at their default settings, but I essentially only mentioned
> that the ASPM implementation may be incomplete as a theoretical
> possibility. The somewhat erratic ASPM behaviour for one of the modems
> also suggests that some further tweak/quirk may be needed, and I was
> hoping to catch Mani's interest by reporting it.
> 
> But based on what I've since heard from Qualcomm, it seems like these
> correctable error may be a known issue with the hardware (e.g. seen
> also with Windows), which is also why we decided to disable it for all
> controllers on these two platforms where I've seen this in v2.
>  
> > Do you want the DT approach because the problem is believed to be
> > platform-specific?  Otherwise, maybe we should consider reverting
> > 9f4f3dfad8cf until the problem is understood?
> 
> Enabling ASPM gave a very significant improvement in battery life on the
> Lenovo ThinkPad X13s, from 10.5 h to 15 h, so reverting is not really an
> option there.

Ah, I missed that you're only disabling L0s, but leaving L1 enabled,
thanks!

And given that the v1.9.0 ops that enable ASPM are used on a bunch of
platforms, and L0s seems to work fine on most of them, we wouldn't
want to disable L0s for everybody, so this seems like the right
solution.

Bjorn