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.
It turns out that these errors are always generated, but for some yet to
be determined reason, the AER interrupts are never received when using
the internal MSI controller, which makes the link errors harder to
notice.
On the X13s, there is a large number of errors generated when bringing
up the link on boot. This is related to the fact that UEFI firmware has
already enabled the Wi-Fi PCIe link at Gen2 speed and restarting the
link at Gen3 generates a massive amount of errors until the Wi-Fi
firmware is restarted. This has now also been shown to cause the Wi-Fi
to sometimes not start at all on boot for some users.
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.
Note that the X13s and CRD use the same Wi-Fi controller, but the errors
are only generated on the X13s. The NVMe controller on my X13s does not
support L0s so there are no issues there, unlike on the CRD which uses a
different controller. The modem on the CRD does not generate any errors,
but both the NVMe and modem keeps bouncing in and out of L0s/L1 also
when not used, which could indicate that there are bigger problems with
the ASPM implementation. I don't have a modem on my X13s so I have not
been able to test whether L0s causes any trouble there.
Note that disabling ASPM L0s for the X13s Wi-Fi does not seem to have a
significant impact on the power consumption (and there are indications
that this applies generally for L0s on these platforms).
***
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
Changes in v4
- drop the 'aspm-no-l0s' DT property and disable L0s for all sc8280xp
derivative platforms based on the compatible string for now
Changes in v3
- drop the two wifi link speed patches which have been picked up for
6.8
- rebase on binding rework in linux-next and add the properties also to
the new qcom,pcie-common.yaml
- https://lore.kernel.org/linux-pci/[email protected]/
- fix an 'L0s' typo in one commit message
Changes in v2
- drop RFC from ASPM patches and add stable tags
- reorder patches and move ITS patch last
- fix s/GB/MB/ typo in Gen2 speed commit messages
- fix an incorrect Fixes tag
- amend commit message X13 wifi link speed patch after user
confirmation that this fixes the wifi startup issue
- disable L0s also for modem and wifi on CRD
- disable L0s also for nvme and modem on X13s
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
arm64: dts: qcom: sc8280xp: add missing PCIe minimum OPP
arm64: dts: qcom: sc8280xp: enable GICv3 ITS for PCIe
.../bindings/pci/qcom,pcie-common.yaml | 4 ++-
.../devicetree/bindings/pci/qcom,pcie.yaml | 4 ++-
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 17 +++++++++-
drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++--
4 files changed, 51 insertions(+), 5 deletions(-)
--
2.43.0
Add the missing PCIe CX performance level votes to avoid relying on
other drivers (e.g. USB or UFS) to maintain the nominal performance
level required for Gen3 speeds.
Fixes: 813e83157001 ("arm64: dts: qcom: sc8280xp/sa8540p: add PCIe2-4 nodes")
Cc: [email protected] # 6.2
Reviewed-by: Konrad Dybcio <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 2fc8d3308844..a8279ba6a756 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1780,6 +1780,7 @@ pcie4: pcie@1c00000 {
reset-names = "pci";
power-domains = <&gcc PCIE_4_GDSC>;
+ required-opps = <&rpmhpd_opp_nom>;
phys = <&pcie4_phy>;
phy-names = "pciephy";
@@ -1878,6 +1879,7 @@ pcie3b: pcie@1c08000 {
reset-names = "pci";
power-domains = <&gcc PCIE_3B_GDSC>;
+ required-opps = <&rpmhpd_opp_nom>;
phys = <&pcie3b_phy>;
phy-names = "pciephy";
@@ -1976,6 +1978,7 @@ pcie3a: pcie@1c10000 {
reset-names = "pci";
power-domains = <&gcc PCIE_3A_GDSC>;
+ required-opps = <&rpmhpd_opp_nom>;
phys = <&pcie3a_phy>;
phy-names = "pciephy";
@@ -2077,6 +2080,7 @@ pcie2b: pcie@1c18000 {
reset-names = "pci";
power-domains = <&gcc PCIE_2B_GDSC>;
+ required-opps = <&rpmhpd_opp_nom>;
phys = <&pcie2b_phy>;
phy-names = "pciephy";
@@ -2175,6 +2179,7 @@ pcie2a: pcie@1c20000 {
reset-names = "pci";
power-domains = <&gcc PCIE_2A_GDSC>;
+ required-opps = <&rpmhpd_opp_nom>;
phys = <&pcie2a_phy>;
phy-names = "pciephy";
--
2.43.0
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: [email protected] # 6.7
Signed-off-by: Johan Hovold <[email protected]>
---
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
Some Qualcomm SoCs require a minimum performance level for the power
domain so add 'required-opps' to the binding.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml | 3 +++
Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 3 +++
2 files changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
index 125136176f93..8d570669650a 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
@@ -59,6 +59,9 @@ properties:
power-domains:
maxItems: 1
+ required-opps:
+ maxItems: 1
+
resets:
minItems: 1
maxItems: 12
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index c8f36978a94c..1d7a6a520fef 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -111,6 +111,9 @@ properties:
description: GPIO controlled connection to PERST# signal
maxItems: 1
+ required-opps:
+ maxItems: 1
+
wake-gpios:
description: GPIO controlled connection to WAKE# signal
maxItems: 1
--
2.43.0
The DWC PCIe controller can be used with its internal MSI controller or
with an external one such as the GICv3 Interrupt Translation Service
(ITS).
Add the msi-map properties needed to use the GIC ITS. This will also
make Linux switch to the ITS implementation, which allows for assigning
affinity to individual MSIs.
Note that using the GIC ITS on SC8280XP will cause Advanced Error
Reporting (AER) interrupts to be received on errors unlike when using
the internal MSI controller. This will specifically lead to
notifications about Correctable Errors being logged for the Wi-Fi
controller on the Lenovo ThinkPad X13s when ASPM L0s is enabled.
Suggested-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Konrad Dybcio <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index a8279ba6a756..906dd4a656a2 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1737,6 +1737,8 @@ pcie4: pcie@1c00000 {
linux,pci-domain = <6>;
num-lanes = <1>;
+ msi-map = <0x0 &its 0xe0000 0x10000>;
+
interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>,
@@ -1838,6 +1840,8 @@ pcie3b: pcie@1c08000 {
linux,pci-domain = <5>;
num-lanes = <2>;
+ msi-map = <0x0 &its 0xd0000 0x10000>;
+
interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>,
@@ -1937,6 +1941,8 @@ pcie3a: pcie@1c10000 {
linux,pci-domain = <4>;
num-lanes = <4>;
+ msi-map = <0x0 &its 0xc0000 0x10000>;
+
interrupts = <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>,
@@ -2039,6 +2045,8 @@ pcie2b: pcie@1c18000 {
linux,pci-domain = <3>;
num-lanes = <2>;
+ msi-map = <0x0 &its 0xb0000 0x10000>;
+
interrupts = <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>,
@@ -2138,6 +2146,8 @@ pcie2a: pcie@1c20000 {
linux,pci-domain = <2>;
num-lanes = <4>;
+ msi-map = <0x0 &its 0xa0000 0x10000>;
+
interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 523 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 524 IRQ_TYPE_LEVEL_HIGH>,
@@ -4424,7 +4434,7 @@ intc: interrupt-controller@17a00000 {
#size-cells = <2>;
ranges;
- msi-controller@17a40000 {
+ its: msi-controller@17a40000 {
compatible = "arm,gic-v3-its";
reg = <0 0x17a40000 0 0x20000>;
msi-controller;
--
2.43.0
Whether the 'msi-map-mask' property is needed or not depends on how the
MSI interrupts are mapped and it should therefore not be described as
required.
Note that the current schema fails to detect omissions of the mask
property if the internal MSI controller properties are also present.
Acked-by: Krzysztof Kozlowski <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml | 1 -
Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 1 -
2 files changed, 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
index 8d570669650a..0d1b23523f62 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
@@ -93,7 +93,6 @@ anyOf:
- "#interrupt-cells"
- required:
- msi-map
- - msi-map-mask
allOf:
- $ref: /schemas/pci/pci-bus.yaml#
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index 1d7a6a520fef..efaab5f82b47 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -134,7 +134,6 @@ anyOf:
- "#interrupt-cells"
- required:
- msi-map
- - msi-map-mask
allOf:
- $ref: /schemas/pci/pci-bus.yaml#
--
2.43.0
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: [email protected] # 6.7
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- 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: [email protected] # 6.7
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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,
--
Bjorn Andersson <[email protected]>
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