Re: [PATCH v2 00/12] arm64: dts: qcom: sc8280xp: PCIe fixes and GICv3 ITS enable

From: Johan Hovold <johan@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: "Bjorn Helgaas" <helgaas@kernel.org>,
	"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>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/12] arm64: dts: qcom: sc8280xp: PCIe fixes and GICv3 ITS enable
Date: Thu, 29 Feb 2024 14:10:21 +0100	[thread overview]
Message-ID: <ZeCCPRVvYCNfMYnd@hovoldconsulting.com> (raw)
In-Reply-To: <20240229122416.GD2999@thinkpad>

On Thu, Feb 29, 2024 at 05:54:16PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Feb 29, 2024 at 11:25:48AM +0100, Johan Hovold wrote:

> > As I mentioned, the 'required-opps' binding update is needed to fix the
> > missing OPP vote so blocking the binding patch would block merging the
> > DT fix which could otherwise go into 6.8.

> I agree that the fix gets the priority. But some maintainers perfer to merge fix
> patches _only_ if they are fixing the issue introduced in the ongoing release.
> But if Bjorn has no issues in merging these for 6.8, then it is fine.

It also depends on the severity of the issue and to some extent the
complexity of the fix. These binding fixes are certainly low risk. :)

> > The 'msi-map-mask' is arguably a fix of the binding which should never
> > have had that property, but sure, it's strictly only needed for 6.9.
> > 
> > And Bjorn A has already checked with the Qualcomm PCI team regarding
> > ASPM. It's also been two weeks since you said you were going to check
> > with your contacts. Is it really worth waiting more for an answer from
> > that part of the team? We can always amend the ASPM fixes later when/if
> > we learn more.
> > 
> > Note that this is also a blocker for merging ITS support for 6.9.

> I got it, but we cannot just merge the patches without finding the rootcause. I
> heard from Qcom that this AER error could also be due to PHY init sequence as
> spotted on some other platforms, so if that is the case then the DT property is
> not correct.

I've verified the PHY sequences both against what the UEFI firmware (and
hence Windows) uses as well as against the internal Qualcomm
documentation (with the help of Bjorn A). And Qualcomm did say that such
errors are also seen under Windows on these platforms.

But the fact that the symptoms differ between the CRD and X13s, which
use the same Atheros Wi-Fi controller (and the same PHY initialisation)
also suggests that this may to some extent be due to some property of
the machine.

Notably, on the X13s there are lot of errors when pushing data
(e.g. using iperf3), whereas on the CRD the are no errors when running
such tests.

When leaving the CRD on for long periods of time with the Wi-Fi
disconnected, I do however see occasional correctable errors.

> Since this is not the hot target now (for Qcom), it takes time to
> check things.

I think that based on the available data it's reasonable to go ahead and
merge these patches. In the event that this turns out to be a
configuration issue, we can just drop the 'aspm-no-l0s' properties
again.

Johan