| Message ID | 20240306095651.4551-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: PCIe fixes and GICv3 ITS enable | expand |
On Wed, Mar 06, 2024 at 10:56:49AM +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 when L0s is enabled, > which could indicate an incomplete driver ASPM implementation or that > the hardware does in fact not support L0s. > > This has now been confirmed by Qualcomm to be the case for sc8280xp and > its derivate platforms (e.g. sa8540p and sa8295p). Specifically, the PHY > configuration used on these platforms is not correctly tuned for L0s and > there is currently no updated configuration available. > > Add a new flag to the driver configuration data and use it to disable > ASPM L0s on sc8280xp, sa8540p and sa8295p for now. > > Note that only the 1.9.0 ops enable ASPM currently. > > Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops") > Cc: stable@vger.kernel.org # 6.7 > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/pci/controller/dwc/pcie-qcom.c | 31 ++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 2ce2a3bd932b..9f83a1611a20 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -229,6 +229,7 @@ struct qcom_pcie_ops { > > struct qcom_pcie_cfg { > const struct qcom_pcie_ops *ops; > + bool no_l0s; > }; > > struct qcom_pcie { > @@ -272,6 +273,26 @@ static int qcom_pcie_start_link(struct dw_pcie *pci) > return 0; > } > > +static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci) > +{ > + struct qcom_pcie *pcie = to_qcom_pcie(pci); > + u16 offset; > + u32 val; > + > + if (!pcie->cfg->no_l0s) > + return; > + > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > + > + dw_pcie_dbi_ro_wr_en(pci); > + > + val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP); > + val &= ~PCI_EXP_LNKCAP_ASPM_L0S; > + writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP); > + > + dw_pcie_dbi_ro_wr_dis(pci); > +} > + > static void qcom_pcie_clear_hpc(struct dw_pcie *pci) > { > u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > @@ -961,6 +982,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > > static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie) > { > + qcom_pcie_clear_aspm_l0s(pcie->pci); > qcom_pcie_clear_hpc(pcie->pci); > > return 0; > @@ -1358,6 +1380,11 @@ static const struct qcom_pcie_cfg cfg_2_9_0 = { > .ops = &ops_2_9_0, > }; > > +static const struct qcom_pcie_cfg cfg_sc8280xp = { > + .ops = &ops_1_9_0, > + .no_l0s = true, > +}; > + > static const struct dw_pcie_ops dw_pcie_ops = { > .link_up = qcom_pcie_link_up, > .start_link = qcom_pcie_start_link, > @@ -1629,11 +1656,11 @@ static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-ipq8074-gen3", .data = &cfg_2_9_0 }, > { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 }, > { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 }, > - { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 }, > + { .compatible = "qcom,pcie-sa8540p", .data = &cfg_sc8280xp }, > { .compatible = "qcom,pcie-sa8775p", .data = &cfg_1_9_0}, > { .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 }, > { .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 }, > - { .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 }, > + { .compatible = "qcom,pcie-sc8280xp", .data = &cfg_sc8280xp }, > { .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 }, > { .compatible = "qcom,pcie-sdx55", .data = &cfg_1_9_0 }, > { .compatible = "qcom,pcie-sm8150", .data = &cfg_1_9_0 }, > -- > 2.43.0 >
On Wed, Mar 06, 2024 at 10:56:46AM +0100, Johan Hovold wrote: > This series addresses a few problems with the sc8280xp PCIe > implementation. > Qualcomm has now confirmed that this is an issue on sc8280xp and its > derivate platforms. Specifically, the PHY configuration used on these > platforms is not correctly tuned for L0s and there is currently no > updated configuration available. > > As we are now at 6.8-rc7, I've rebased this series on the Qualcomm PCIe > binding rework in linux-next so that the whole series can be merged for > 6.9 (the patch to disable l0s and the devicetree fix are both marked for > stable backport anyway). > > 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 (5): > dt-bindings: PCI: qcom: Allow 'required-opps' > dt-bindings: PCI: qcom: Do not require 'msi-map-mask' > PCI: qcom: Disable ASPM L0s for sc8280xp, sa8540p and sa8295p Bjorn H, could you pick up these three for 6.9-rc1? > arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP > arm64: dts: qcom: sc8280xp: enable GICv3 ITS for PCIe That way there's a small chance that Bjorn A can be able to squeeze in the patch to enable ITS in 6.9 too (e.g. if there's an rc8). Johan
On Wed, 06 Mar 2024 10:56:46 +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 to controller/qcom, thanks! [1/5] dt-bindings: PCI: qcom: Allow 'required-opps' https://git.kernel.org/pci/pci/c/c8073025c0e4 [2/5] dt-bindings: PCI: qcom: Do not require 'msi-map-mask' https://git.kernel.org/pci/pci/c/545e88cb41a6 [3/5] PCI: qcom: Disable ASPM L0s for sc8280xp, sa8540p and sa8295p https://git.kernel.org/pci/pci/c/d1997c987814 Thanks, Lorenzo
On Wed, Mar 06, 2024 at 10:56:49AM +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 when L0s is enabled, > which could indicate an incomplete driver ASPM implementation or that > the hardware does in fact not support L0s. > > This has now been confirmed by Qualcomm to be the case for sc8280xp and > its derivate platforms (e.g. sa8540p and sa8295p). Specifically, the PHY > configuration used on these platforms is not correctly tuned for L0s and > there is currently no updated configuration available. > > Add a new flag to the driver configuration data and use it to disable > ASPM L0s on sc8280xp, sa8540p and sa8295p for now. > > Note that only the 1.9.0 ops enable ASPM currently. > > Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops") > Cc: stable@vger.kernel.org # 6.7 > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn
On Wed, 06 Mar 2024 10:56:46 +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! [4/5] arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP commit: 2b621971554a94094cf489314dc1c2b65401965c [5/5] arm64: dts: qcom: sc8280xp: enable GICv3 ITS for PCIe commit: 81051f14a66c3913f1d219bd97e47002f1dc91de Best regards,
On Mon, Mar 18, 2024 at 09:48:30PM -0500, Bjorn Andersson wrote: > > On Wed, 06 Mar 2024 10:56:46 +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! > > [4/5] arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP > commit: 2b621971554a94094cf489314dc1c2b65401965c I noticed that you applied both of these for 6.10, but this one is a fix that should go into 6.9. > [5/5] arm64: dts: qcom: sc8280xp: enable GICv3 ITS for PCIe > commit: 81051f14a66c3913f1d219bd97e47002f1dc91de Johan
On 19/03/2024 08:25, Johan Hovold wrote: > On Mon, Mar 18, 2024 at 09:48:30PM -0500, Bjorn Andersson wrote: >> >> On Wed, 06 Mar 2024 10:56:46 +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! >> >> [4/5] arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP >> commit: 2b621971554a94094cf489314dc1c2b65401965c > > I noticed that you applied both of these for 6.10, but this one is a fix > that should go into 6.9. Well, mixing fixes for different cycles in one patchset was always discouraged. In case of some subsystems you would receive clear response, that you must split fixes out of the patchset. Fixes being first in the patchset would be probably accepted by the rest of subsystems, but putting it in the middle of the patchset is wrong. Best regards, Krzysztof
On Wed, Mar 20, 2024 at 09:09:02AM +0100, Krzysztof Kozlowski wrote: > On 19/03/2024 08:25, Johan Hovold wrote: > > On Mon, Mar 18, 2024 at 09:48:30PM -0500, Bjorn Andersson wrote: > >> On Wed, 06 Mar 2024 10:56:46 +0100, Johan Hovold wrote: > >>> This series addresses a few problems with the sc8280xp PCIe > >>> implementation. > >> Applied, thanks! > >> > >> [4/5] arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP > >> commit: 2b621971554a94094cf489314dc1c2b65401965c > > > > I noticed that you applied both of these for 6.10, but this one is a fix > > that should go into 6.9. > > Well, mixing fixes for different cycles in one patchset was always > discouraged. In case of some subsystems you would receive clear > response, that you must split fixes out of the patchset. > > Fixes being first in the patchset would be probably accepted by the rest > of subsystems, but putting it in the middle of the patchset is wrong. Perhaps you should not comment before reading up on the history of this series. This was all intended for 6.9, but merging was stalled for a number of reasons so here we are. The patches were also going in through different trees, so patch 4/5 is the first Qualcomm SoC patch. Johan
On 20/03/2024 09:18, Johan Hovold wrote: > On Wed, Mar 20, 2024 at 09:09:02AM +0100, Krzysztof Kozlowski wrote: >> On 19/03/2024 08:25, Johan Hovold wrote: >>> On Mon, Mar 18, 2024 at 09:48:30PM -0500, Bjorn Andersson wrote: >>>> On Wed, 06 Mar 2024 10:56:46 +0100, Johan Hovold wrote: >>>>> This series addresses a few problems with the sc8280xp PCIe >>>>> implementation. > >>>> Applied, thanks! >>>> >>>> [4/5] arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP >>>> commit: 2b621971554a94094cf489314dc1c2b65401965c >>> >>> I noticed that you applied both of these for 6.10, but this one is a fix >>> that should go into 6.9. >> >> Well, mixing fixes for different cycles in one patchset was always >> discouraged. In case of some subsystems you would receive clear >> response, that you must split fixes out of the patchset. >> >> Fixes being first in the patchset would be probably accepted by the rest >> of subsystems, but putting it in the middle of the patchset is wrong. > > Perhaps you should not comment before reading up on the history of this > series. > > This was all intended for 6.9, but merging was stalled for a number of > reasons so here we are. The patches were also going in through different > trees, so patch 4/5 is the first Qualcomm SoC patch. Again, well, you sent it at few days before merge window, so how do you imagine this being applied for v6.9 and still fulfilling "few linux-next cycles before merge window" requirement? Especially that arm-soc cut off is much earlier :/. I talk about patch 5, of course, because that is not a fix (at least not marked as one). Don't expect in general a arms-co patch to be applied four days before merge window, thus the actual fix - patch #4 - should be split. Best regards, Krzysztof
On Wed, Mar 20, 2024 at 09:24:54AM +0100, Krzysztof Kozlowski wrote: > On 20/03/2024 09:18, Johan Hovold wrote: > > Perhaps you should not comment before reading up on the history of this > > series. > > > > This was all intended for 6.9, but merging was stalled for a number of > > reasons so here we are. The patches were also going in through different > > trees, so patch 4/5 is the first Qualcomm SoC patch. > > Again, well, you sent it at few days before merge window, so how do you > imagine this being applied for v6.9 and still fulfilling "few linux-next > cycles before merge window" requirement? Especially that arm-soc cut off > is much earlier :/. I talk about patch 5, of course, because that is not > a fix (at least not marked as one). Don't expect in general a arms-co > patch to be applied four days before merge window, thus the actual fix - > patch #4 - should be split. At the time there was still hope that there may be an rc8, and the patch in question had been used by a large number of X13s users for several weeks, which is a lot more testing than the average Qualcomm patch receives, whether it's in linux-next or not. And patch 5 depends on the earlier patches in the series so it belongs in the series, which was also initially posted long before the merge window. I'm sure Bjorn A can handle this in general, even if he failed to notice the CC stable tag or had other reasons for applying the fix for 6.10 instead of 6.9. Johan
On 20/03/2024 09:40, Johan Hovold wrote: > On Wed, Mar 20, 2024 at 09:24:54AM +0100, Krzysztof Kozlowski wrote: >> On 20/03/2024 09:18, Johan Hovold wrote: > >>> Perhaps you should not comment before reading up on the history of this >>> series. >>> >>> This was all intended for 6.9, but merging was stalled for a number of >>> reasons so here we are. The patches were also going in through different >>> trees, so patch 4/5 is the first Qualcomm SoC patch. >> >> Again, well, you sent it at few days before merge window, so how do you >> imagine this being applied for v6.9 and still fulfilling "few linux-next >> cycles before merge window" requirement? Especially that arm-soc cut off >> is much earlier :/. I talk about patch 5, of course, because that is not >> a fix (at least not marked as one). Don't expect in general a arms-co >> patch to be applied four days before merge window, thus the actual fix - >> patch #4 - should be split. > > At the time there was still hope that there may be an rc8, and the patch > in question had been used by a large number of X13s users for several > weeks, which is a lot more testing than the average Qualcomm patch > receives, whether it's in linux-next or not. OK, it does solve some parts of our discussion but does not solve my earlier comment: Fixes should be separate series. Certain folks have quite strict requirement on this. Try sending a fix with non-fix (depending on fix somehow like here) to Mark Brown. He has even template for such case... > > And patch 5 depends on the earlier patches in the series so it belongs > in the series, which was also initially posted long before the merge > window. The dependency is might not be good enough reason to combine fixes and non-fixes into one series. Dependency should be explained (in 5th patch), but it's maintainer's judgement and job to handle this. And in all this, fact that Bjorn missed certain aspects and applied this differently than you wanted, is another argument that this should be split. Best regards, Krzysztof
On Wed, Mar 20, 2024 at 10:16:16AM +0100, Krzysztof Kozlowski wrote: > On 20/03/2024 09:40, Johan Hovold wrote: > > At the time there was still hope that there may be an rc8, and the patch > > in question had been used by a large number of X13s users for several > > weeks, which is a lot more testing than the average Qualcomm patch > > receives, whether it's in linux-next or not. > > OK, it does solve some parts of our discussion but does not solve my > earlier comment: Fixes should be separate series. Certain folks have > quite strict requirement on this. Try sending a fix with non-fix > (depending on fix somehow like here) to Mark Brown. He has even template > for such case... That's not a general rule at all. > > And patch 5 depends on the earlier patches in the series so it belongs > > in the series, which was also initially posted long before the merge > > window. > > The dependency is might not be good enough reason to combine fixes and > non-fixes into one series. Dependency should be explained (in 5th > patch), but it's maintainer's judgement and job to handle this. FFS, I'm posting a series adding a new feature, which depends on first addressing a few related bugs. I post everything together so that it can be evaluated and tested together. That's perfectly fine, and not that different from how we post driver and dts changes in one series to facilitate review. Some maintainers don't like that either, then we deal with that. Johan