| Message ID | 20240223152124.20042-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: PCIe fixes and GICv3 ITS enable | expand |
[+to Mani] On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: > This series addresses a few problems with the sc8280xp PCIe > implementation. > ... > A recent commit enabling ASPM on certain Qualcomm platforms introduced > further errors when using the Wi-Fi on the X13s as well as when > accessing the NVMe on the CRD. The exact reason for this has not yet > been identified, but disabling ASPM L0s makes the errors go away. This > could suggest that either the current ASPM implementation is incomplete > or that L0s is not supported with these devices. > ... > As this series fixes a regression in 6.7 (which enabled ASPM) and fixes > a user-reported problem with the Wi-Fi often not starting at boot, I > think we should merge this series for the 6.8 cycle. The final patch > enabling the GIC ITS should wait for 6.9. > > The DT bindings and PCI patch are expected to go through the PCI tree, > while Bjorn A takes the devicetree updates through the Qualcomm tree. > ... > Johan Hovold (12): > dt-bindings: PCI: qcom: Allow 'required-opps' > dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' > PCI: qcom: Add support for disabling ASPM L0s in devicetree The ASPM patches fix a v6.7 regression, so it would be good to fix that in v6.8. Mani, if you are OK with them, I can add them to for-linus for v6.8. What about the 'required-opps' and 'msi-map-mask' patches? If they're important, I can merge them for v6.8, too, but it's late in the cycle and it's not clear from the commit logs why they shouldn't wait for v6.9. Bjorn
On 2/28/24 23:08, Bjorn Helgaas wrote: > [+to Mani] > > On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: >> This series addresses a few problems with the sc8280xp PCIe >> implementation. >> ... > >> A recent commit enabling ASPM on certain Qualcomm platforms introduced >> further errors when using the Wi-Fi on the X13s as well as when >> accessing the NVMe on the CRD. The exact reason for this has not yet >> been identified, but disabling ASPM L0s makes the errors go away. This >> could suggest that either the current ASPM implementation is incomplete >> or that L0s is not supported with these devices. >> ... > >> As this series fixes a regression in 6.7 (which enabled ASPM) and fixes >> a user-reported problem with the Wi-Fi often not starting at boot, I >> think we should merge this series for the 6.8 cycle. The final patch >> enabling the GIC ITS should wait for 6.9. >> >> The DT bindings and PCI patch are expected to go through the PCI tree, >> while Bjorn A takes the devicetree updates through the Qualcomm tree. >> ... > >> Johan Hovold (12): >> dt-bindings: PCI: qcom: Allow 'required-opps' >> dt-bindings: PCI: qcom: Do not require 'msi-map-mask' >> dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' >> PCI: qcom: Add support for disabling ASPM L0s in devicetree > > The ASPM patches fix a v6.7 regression, so it would be good to fix > that in v6.8. > > Mani, if you are OK with them, I can add them to for-linus for v6.8. > > What about the 'required-opps' and 'msi-map-mask' patches? If they're > important, I can merge them for v6.8, too, but it's late in the cycle > and it's not clear from the commit logs why they shouldn't wait for > v6.9. Either way, I believe they should go through the qcom tree, as it's a very hot one with lots of different changes to a given file. Unless the soc-late window is already closed... Bjorn A will know. Konrad
On Wed, Feb 28, 2024 at 04:08:43PM -0600, Bjorn Helgaas wrote: > [+to Mani] > > On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: > > This series addresses a few problems with the sc8280xp PCIe > > implementation. > > ... > > > A recent commit enabling ASPM on certain Qualcomm platforms introduced > > further errors when using the Wi-Fi on the X13s as well as when > > accessing the NVMe on the CRD. The exact reason for this has not yet > > been identified, but disabling ASPM L0s makes the errors go away. This > > could suggest that either the current ASPM implementation is incomplete > > or that L0s is not supported with these devices. > > ... > > > As this series fixes a regression in 6.7 (which enabled ASPM) and fixes > > a user-reported problem with the Wi-Fi often not starting at boot, I > > think we should merge this series for the 6.8 cycle. The final patch > > enabling the GIC ITS should wait for 6.9. > > > > The DT bindings and PCI patch are expected to go through the PCI tree, > > while Bjorn A takes the devicetree updates through the Qualcomm tree. > > ... > > > Johan Hovold (12): > > dt-bindings: PCI: qcom: Allow 'required-opps' > > dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > > dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' > > PCI: qcom: Add support for disabling ASPM L0s in devicetree > > The ASPM patches fix a v6.7 regression, so it would be good to fix > that in v6.8. > > Mani, if you are OK with them, I can add them to for-linus for v6.8. > > What about the 'required-opps' and 'msi-map-mask' patches? If they're > important, I can merge them for v6.8, too, but it's late in the cycle > and it's not clear from the commit logs why they shouldn't wait for > v6.9. The 'required-opps' binding patch is a prerequisite for the corresponding DT fix, which is tagged for stable and that should go in 6.8. The 'msi-map-mask' binding update is strictly only needed for enabling the ITS, which is 6.9 material, even if it's arguably also a fix for the current binding. So this one could possibly wait for 6.9 even if it may make sense to just take all three now for 6.8 to only have to deal with the mentioned binding conflict once. Johan
On Thu, Feb 29, 2024 at 12:30:03AM +0100, Konrad Dybcio wrote: > On 2/28/24 23:08, Bjorn Helgaas wrote: > > On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: > >> Johan Hovold (12): > >> dt-bindings: PCI: qcom: Allow 'required-opps' > >> dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > >> dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' > >> PCI: qcom: Add support for disabling ASPM L0s in devicetree > > What about the 'required-opps' and 'msi-map-mask' patches? If they're > > important, I can merge them for v6.8, too, but it's late in the cycle > > and it's not clear from the commit logs why they shouldn't wait for > > v6.9. > > Either way, I believe they should go through the qcom tree, as it's > a very hot one with lots of different changes to a given file. I think Bjorn was just referring to the three binding patches listed above and which should go through the PCI tree (unlike the later DT fixes which will go through the Qualcomm SoC tree). Johan
On Wed, Feb 28, 2024 at 04:08:43PM -0600, Bjorn Helgaas wrote: > [+to Mani] > > On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: > > This series addresses a few problems with the sc8280xp PCIe > > implementation. > > ... > > > A recent commit enabling ASPM on certain Qualcomm platforms introduced > > further errors when using the Wi-Fi on the X13s as well as when > > accessing the NVMe on the CRD. The exact reason for this has not yet > > been identified, but disabling ASPM L0s makes the errors go away. This > > could suggest that either the current ASPM implementation is incomplete > > or that L0s is not supported with these devices. > > ... > > > As this series fixes a regression in 6.7 (which enabled ASPM) and fixes > > a user-reported problem with the Wi-Fi often not starting at boot, I > > think we should merge this series for the 6.8 cycle. The final patch > > enabling the GIC ITS should wait for 6.9. > > > > The DT bindings and PCI patch are expected to go through the PCI tree, > > while Bjorn A takes the devicetree updates through the Qualcomm tree. > > ... > > > Johan Hovold (12): > > dt-bindings: PCI: qcom: Allow 'required-opps' > > dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > > dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' > > PCI: qcom: Add support for disabling ASPM L0s in devicetree > > The ASPM patches fix a v6.7 regression, so it would be good to fix > that in v6.8. > > Mani, if you are OK with them, I can add them to for-linus for v6.8. > > What about the 'required-opps' and 'msi-map-mask' patches? If they're > important, I can merge them for v6.8, too, but it's late in the cycle > and it's not clear from the commit logs why they shouldn't wait for > v6.9. > I'm checking with Qcom HW team on the ASPM behavior. So please hold off the ASPM related patches until I get an answer. But 'required-opps' and 'msi-map-mask' patches can be applied for 6.9 (not strictly fixing anything in 6.8). - Mani
On Thu, Feb 29, 2024 at 03:38:53PM +0530, Manivannan Sadhasivam wrote: > On Wed, Feb 28, 2024 at 04:08:43PM -0600, Bjorn Helgaas wrote: > > On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: > > > Johan Hovold (12): > > > dt-bindings: PCI: qcom: Allow 'required-opps' > > > dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > > > dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' > > > PCI: qcom: Add support for disabling ASPM L0s in devicetree > > > > The ASPM patches fix a v6.7 regression, so it would be good to fix > > that in v6.8. > > > > Mani, if you are OK with them, I can add them to for-linus for v6.8. > > > > What about the 'required-opps' and 'msi-map-mask' patches? If they're > > important, I can merge them for v6.8, too, but it's late in the cycle > > and it's not clear from the commit logs why they shouldn't wait for > > v6.9. > > > > I'm checking with Qcom HW team on the ASPM behavior. So please hold off the ASPM > related patches until I get an answer. But 'required-opps' and 'msi-map-mask' > patches can be applied for 6.9 (not strictly fixing anything in 6.8). 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. 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. Johan
On Thu, Feb 29, 2024 at 11:25:48AM +0100, Johan Hovold wrote: > On Thu, Feb 29, 2024 at 03:38:53PM +0530, Manivannan Sadhasivam wrote: > > On Wed, Feb 28, 2024 at 04:08:43PM -0600, Bjorn Helgaas wrote: > > > On Fri, Feb 23, 2024 at 04:21:12PM +0100, Johan Hovold wrote: > > > > > Johan Hovold (12): > > > > dt-bindings: PCI: qcom: Allow 'required-opps' > > > > dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > > > > dt-bindings: PCI: qcom: Allow 'aspm-no-l0s' > > > > PCI: qcom: Add support for disabling ASPM L0s in devicetree > > > > > > The ASPM patches fix a v6.7 regression, so it would be good to fix > > > that in v6.8. > > > > > > Mani, if you are OK with them, I can add them to for-linus for v6.8. > > > > > > What about the 'required-opps' and 'msi-map-mask' patches? If they're > > > important, I can merge them for v6.8, too, but it's late in the cycle > > > and it's not clear from the commit logs why they shouldn't wait for > > > v6.9. > > > > > > > I'm checking with Qcom HW team on the ASPM behavior. So please hold off the ASPM > > related patches until I get an answer. But 'required-opps' and 'msi-map-mask' > > patches can be applied for 6.9 (not strictly fixing anything in 6.8). > > 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. > 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. Since this is not the hot target now (for Qcom), it takes time to check things. - Mani
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
On Thu, Feb 29, 2024 at 02:10:21PM +0100, Johan Hovold wrote: > 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. :) > Right. > > > 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. > Well, sometimes the init sequence published by qualcomm could turn out to be faulty. That's why they publish v2 sequence and such. And I want to rule out that possibility in this case. > 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. > This may be due to hardware design that I agree (possibly influenced by driver defect). > > 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. > Well the problem is, if you are not sure, then adding the DT properties is certainly not correct. As that implies a hardware defect, but it may not be. So let's wait for some time to find out the actual issue. - Mani
On Thu, Feb 29, 2024 at 07:24:07PM +0530, Manivannan Sadhasivam wrote: > On Thu, Feb 29, 2024 at 02:10:21PM +0100, Johan Hovold wrote: > > 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. > > Well the problem is, if you are not sure, then adding the DT properties is > certainly not correct. As that implies a hardware defect, but it may not be. > So let's wait for some time to find out the actual issue. Our devicetrees are always going to be a tentative description of the hardware, and this especially true for Qualcomm that don't publish any documentation so that people are forced to rely on informed guesses based on downstream devicetrees and drivers and reverse engineering. As far as I can tell, after having spent a lot of time on this and checking with sources inside Qualcomm, the hardware is to blame here. If this turns out not to be true, we can always revise later. We do this all the time, as you know. I'm all for digging further into this issue with the help of Qualcomm, but I don't think that should block this series as that would leave the link errors that we hit since 6.7 in place and effectively prevent us from enabling the ITS in 6.9. Johan
On Thu, Feb 29, 2024 at 02:10:21PM +0100, Johan Hovold wrote: > 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. I do prefer to merge only regression and important fixes after the merge window, so I want to be able to provide justification. > 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. > :) IIUC we're talking about: arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP dt-bindings: PCI: qcom: Allow 'required-opps' These don't look like a regression fix (correct me if I'm wrong), and I can't tell whether they fix a user-visible problem, since sc8280xp.dtsi does already contain 'required-opps' for ufs_mem_hc, usb_0, and usb_1, which are mentioned in the commit log as covering up the issue. If these patches wait until v6.9, what badness ensues? Bjorn
On Thu, Feb 29, 2024 at 02:52:40PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 29, 2024 at 02:10:21PM +0100, Johan Hovold wrote: > > 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. > > :) > > IIUC we're talking about: > > arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP > dt-bindings: PCI: qcom: Allow 'required-opps' Right. > These don't look like a regression fix (correct me if I'm wrong), and > I can't tell whether they fix a user-visible problem, since > sc8280xp.dtsi does already contain 'required-opps' for ufs_mem_hc, > usb_0, and usb_1, which are mentioned in the commit log as covering up > the issue. The issue has been there since PCIe support was added for this platform and does not cause any issues until the USB and UFS controllers are runtime suspended. When that happens nothing is currently making sure that we have enough power to run PCIe at gen3 speeds, something which can potentially result in system instability (e.g. resets). > If these patches wait until v6.9, what badness ensues? We'd have a few more weeks where users enabling runtime PM for USB on the X13s could hit this before we can get the fix backported to stable. I could have put some more details in the commit message for the DT patch, but I did not think that amending the PCIe binding would be controversial. (I guess we can also take the DT fix without waiting for the binding update as it has been acked by a DT maintainer even if that would result in some DT checker warnings until things are aligned again.) Let me know what you decide regarding getting the whole series into 6.8, and then I can spend some more time on rewording, splitting and rebasing this series if needed. Johan
On Thu, Feb 29, 2024 at 04:37:27PM +0100, Johan Hovold wrote: > On Thu, Feb 29, 2024 at 07:24:07PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Feb 29, 2024 at 02:10:21PM +0100, Johan Hovold wrote: > > > > 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. > > > > Well the problem is, if you are not sure, then adding the DT properties is > > certainly not correct. As that implies a hardware defect, but it may not be. > > So let's wait for some time to find out the actual issue. > > Our devicetrees are always going to be a tentative description of the > hardware, and this especially true for Qualcomm that don't publish any > documentation so that people are forced to rely on informed guesses > based on downstream devicetrees and drivers and reverse engineering. > > As far as I can tell, after having spent a lot of time on this and > checking with sources inside Qualcomm, the hardware is to blame here. If > this turns out not to be true, we can always revise later. We do this > all the time, as you know. > > I'm all for digging further into this issue with the help of Qualcomm, > but I don't think that should block this series as that would leave the > link errors that we hit since 6.7 in place and effectively prevent us > from enabling the ITS in 6.9. > Sounds fair. I will report back, perhaps with a fix based on what I get to know. But I think it is better to disable L0s in the SoC dtsi itself. That's not only because there are patches to essentially disable L0s in 2 of the available platforms making use of this Soc, but also you are enabling GIC ITS in the SoC dtsi and that may affect sa8540p which is making use of this dtsi. The users of that SoC may have not noticed the errors as you did before, but enabling GIC ITS will certainly make the issue visible to them (more likely). Also, if it turns out to be a hardware IP issue, then we can leave the patches as it is, otherwise we can revert them easily. - Mani
On Fri, Mar 01, 2024 at 05:54:06PM +0530, Manivannan Sadhasivam wrote: > On Thu, Feb 29, 2024 at 04:37:27PM +0100, Johan Hovold wrote: > > I'm all for digging further into this issue with the help of Qualcomm, > > but I don't think that should block this series as that would leave the > > link errors that we hit since 6.7 in place and effectively prevent us > > from enabling the ITS in 6.9. > > Sounds fair. I will report back, perhaps with a fix based on what I get to know. Sounds good, thanks. > But I think it is better to disable L0s in the SoC dtsi itself. That's not only > because there are patches to essentially disable L0s in 2 of the available > platforms making use of this Soc, but also you are enabling GIC ITS in the SoC > dtsi and that may affect sa8540p which is making use of this dtsi. I did not do so on purpose as I'm only disabling L0s on machines where I've confirmed the issue. And the assumption for now is that this is a machine-level issue. > The users of that SoC may have not noticed the errors as you did before, but > enabling GIC ITS will certainly make the issue visible to them (more likely). Sure and that would be good to know as that would give us another data point which may help determine where the problem lies. Enabling the ITS will (hopefully) be done in 6.9 so we'll have a whole cycle to disable L0s where needed. I don't think this should be done before then. Johan
On Fri, Mar 01, 2024 at 01:46:15PM +0100, Johan Hovold wrote: > On Fri, Mar 01, 2024 at 05:54:06PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Feb 29, 2024 at 04:37:27PM +0100, Johan Hovold wrote: > > > > I'm all for digging further into this issue with the help of Qualcomm, > > > but I don't think that should block this series as that would leave the > > > link errors that we hit since 6.7 in place and effectively prevent us > > > from enabling the ITS in 6.9. > > > > Sounds fair. I will report back, perhaps with a fix based on what I get to know. > > Sounds good, thanks. > > > But I think it is better to disable L0s in the SoC dtsi itself. That's not only > > because there are patches to essentially disable L0s in 2 of the available > > platforms making use of this Soc, but also you are enabling GIC ITS in the SoC > > dtsi and that may affect sa8540p which is making use of this dtsi. > > I did not do so on purpose as I'm only disabling L0s on machines where > I've confirmed the issue. And the assumption for now is that this is a > machine-level issue. > > > The users of that SoC may have not noticed the errors as you did before, but > > enabling GIC ITS will certainly make the issue visible to them (more likely). > > Sure and that would be good to know as that would give us another data > point which may help determine where the problem lies. Enabling the ITS > will (hopefully) be done in 6.9 so we'll have a whole cycle to disable > L0s where needed. I don't think this should be done before then. > Ok. Let's see what happens :) For the series: Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani
On Fri, Mar 01, 2024 at 06:44:45PM +0530, Manivannan Sadhasivam wrote: > For the series: > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks for reviewing. Johan
On Thu, Feb 29, 2024 at 02:52:40PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 29, 2024 at 02:10:21PM +0100, Johan Hovold wrote: > > 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. > > I do prefer to merge only regression and important fixes after the > merge window, so I want to be able to provide justification. > > > 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. > > :) > > IIUC we're talking about: > > arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP I'd prefer to take this one through my tree. I will double check the hardware documentation (there are differences in sc8280xp here) and decide how to proceed... > dt-bindings: PCI: qcom: Allow 'required-opps' Picking this for v6.9 is fine, no practical badness ensues. We would temporarily have a few additional DeviceTree validation warnings in the v6.8 release... Regards, Bjorn > > These don't look like a regression fix (correct me if I'm wrong), and > I can't tell whether they fix a user-visible problem, since > sc8280xp.dtsi does already contain 'required-opps' for ufs_mem_hc, > usb_0, and usb_1, which are mentioned in the commit log as covering up > the issue. > > If these patches wait until v6.9, what badness ensues? > > Bjorn
On Fri, 23 Feb 2024 16:21:12 +0100, Johan Hovold wrote: > This series addresses a few problems with the sc8280xp PCIe > implementation. > > The DWC PCIe controller can either use its internal MSI controller or an > external one such as the GICv3 ITS. Enabling the latter allows for > assigning affinity to individual interrupts, but results in a large > amount of Correctable Errors being logged on both the Lenovo ThinkPad > X13s and the sc8280xp-crd reference design. > > [...] Applied, thanks! [06/12] arm64: dts: qcom: sc8280xp-crd: limit pcie4 link speed commit: db8138845cebcdd0c709570b8217bd052757b8df [07/12] arm64: dts: qcom: sc8280xp-x13s: limit pcie4 link speed commit: 7a1c6a8bf47b0b290c79b9cc3ba6ee68be5522e8 Best regards,