diff mbox series

[RFC,08/10] PCI: qcom: Add support for disabling ASPM L0s in devicetree

Message ID 20240212165043.26961-9-johan+linaro@kernel.org
State New
Headers
Series arm64: dts: qcom: sc8280xp: enable GICv3 ITS for PCIe |

Commit Message

Johan Hovold Feb. 12, 2024, 4:50 p.m. UTC 
  A recent commit 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.

Add support for disabling ASPM L0s in the devicetree when it is not
supported on a particular machine and controller.

Note that only the 1.9.0 ops enable ASPM currently.

Fixes: a9a023c05697 ("PCI: qcom: Add support for disabling ASPM L0s in devicetree")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Bjorn Helgaas Feb. 12, 2024, 7:34 p.m. UTC | #1 
On Mon, Feb 12, 2024 at 05:50:41PM +0100, Johan Hovold wrote:
> A recent commit 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.

I think it would be useful for debugging purposes to identify the
specific commit.  Maybe it's 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for
platforms supporting 1.9.0 ops") ?

> Add support for disabling ASPM L0s in the devicetree when it is not
> supported on a particular machine and controller.
> 
> Note that only the 1.9.0 ops enable ASPM currently.
> 
> Fixes: a9a023c05697 ("PCI: qcom: Add support for disabling ASPM L0s in devicetree")

I don't see this SHA1 in the PCI tree; is it a stable SHA1 from
somewhere else?

> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 2455decc574a..071741b81644 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -273,6 +273,25 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  	return 0;
>  }
>  
> +static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
> +{
> +	u16 offset;
> +	u32 val;
> +
> +	if (!of_property_read_bool(pci->dev->of_node, "aspm-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);
> @@ -962,6 +981,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;
> -- 
> 2.43.0
>
Johan Hovold Feb. 12, 2024, 8:21 p.m. UTC | #2 
On Mon, Feb 12, 2024 at 01:34:49PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 12, 2024 at 05:50:41PM +0100, Johan Hovold wrote:
> > A recent commit 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.
> 
> I think it would be useful for debugging purposes to identify the
> specific commit.  Maybe it's 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for
> platforms supporting 1.9.0 ops") ?
> 
> > Add support for disabling ASPM L0s in the devicetree when it is not
> > supported on a particular machine and controller.
> > 
> > Note that only the 1.9.0 ops enable ASPM currently.
> > 
> > Fixes: a9a023c05697 ("PCI: qcom: Add support for disabling ASPM L0s in devicetree")
> 
> I don't see this SHA1 in the PCI tree; is it a stable SHA1 from
> somewhere else?

Yes, sorry, this was indeed supposed to say

Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")

as it would be a dependency for the follow-on fixes.

Johan
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 2455decc574a..071741b81644 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -273,6 +273,25 @@  static int qcom_pcie_start_link(struct dw_pcie *pci)
 	return 0;
 }
 
+static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
+{
+	u16 offset;
+	u32 val;
+
+	if (!of_property_read_bool(pci->dev->of_node, "aspm-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);
@@ -962,6 +981,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;