On Fri, Feb 23, 2024 at 04:21:16PM +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, which could indicate an incomplete driver ASPM implementation or that the hardware does in fact not support L0s.
Are there any more details about this? Do the errors occur around suspend/resume, a power state transition, or some other event? Might other DWC-based devices be susceptible? Is there a specific driver you suspect might be incomplete?
Do you want the DT approach because the problem is believed to be platform-specific? Otherwise, maybe we should consider reverting 9f4f3dfad8cf until the problem is understood?
Could this be done via a quirk like quirk_disable_aspm_l0s()? That currently uses pci_disable_link_state(), which I don't think is completely safe because it leaves the possibility that drivers or users could re-enable L0s, e.g., via sysfs.
This patch is nice because IIUC it directly changes PCI_EXP_LNKCAP, which avoids that issue, but quirk_disable_aspm_l0s() could conceivably be reimplemented to cache PCI_EXP_LNKCAP in struct pci_dev so quirks could override it, as we do with struct pci_dev.devcap.
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: 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
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 09d485df34b9..0fb5dc06d2ef 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