[PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.

Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
Substates, for all devices powered on at the time the controller driver
enumerates them.

ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
the user enabled ASPM via module parameter or sysfs).

After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
devicetree platforms"), the PCI core enabled all ASPM states for all
devices whether powered on initially or by pwrctrl, so a729c1664619 was
unnecessary and reverted.

But f3ac2ff14834 was too aggressive and broke platforms that didn't support
CLKREQ# or required device-specific configuration for L1 Substates, so
df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
enabled only L0s and L1.

On Qualcomm platforms, this left L1 Substates disabled, which was a
regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
that are initially powered on.  Devices powered on by pwrctrl will be
addressed later.

Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 32 ++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6948824642dc..c48a20602d7f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -247,6 +247,7 @@ struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
 	int (*post_init)(struct qcom_pcie *pcie);
+	void (*host_post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
@@ -1038,6 +1039,25 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
+{
+	/*
+	 * Downstream devices need to be in D0 state before enabling PCI PM
+	 * substates.
+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+
+	return 0;
+}
+
+static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
+{
+	struct dw_pcie_rp *pp = &pcie->pci->pp;
+
+	pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_aspm, NULL);
+}
+
 static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
@@ -1312,9 +1332,19 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
 	pcie->cfg->ops->deinit(pcie);
 }
 
+static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+	if (pcie->cfg->ops->host_post_init)
+		pcie->cfg->ops->host_post_init(pcie);
+}
+
 static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
 	.init		= qcom_pcie_host_init,
 	.deinit		= qcom_pcie_host_deinit,
+	.post_init	= qcom_pcie_host_post_init,
 };
 
 /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
@@ -1376,6 +1406,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.get_resources = qcom_pcie_get_resources_2_7_0,
 	.init = qcom_pcie_init_2_7_0,
 	.post_init = qcom_pcie_post_init_2_7_0,
+	.host_post_init = qcom_pcie_host_post_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.config_sid = qcom_pcie_config_sid_1_9_0,
@@ -1386,6 +1417,7 @@ static const struct qcom_pcie_ops ops_1_21_0 = {
 	.get_resources = qcom_pcie_get_resources_2_7_0,
 	.init = qcom_pcie_init_2_7_0,
 	.post_init = qcom_pcie_post_init_2_7_0,
+	.host_post_init = qcom_pcie_host_post_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 };
-- 
2.43.0

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Manivannan Sadhasivam]

On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> 
> Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> Substates, for all devices powered on at the time the controller driver
> enumerates them.
> 
> ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> the user enabled ASPM via module parameter or sysfs).
> 
> After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> devicetree platforms"), the PCI core enabled all ASPM states for all
> devices whether powered on initially or by pwrctrl, so a729c1664619 was
> unnecessary and reverted.
> 
> But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> CLKREQ# or required device-specific configuration for L1 Substates, so
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> enabled only L0s and L1.
> 
> On Qualcomm platforms, this left L1 Substates disabled, which was a
> regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> that are initially powered on.  Devices powered on by pwrctrl will be
> addressed later.
> 

Can we rather have platform specific APIs [1] to enable ASPM states instead of just
re-introducing this half-baked solution? (yes, I introduced it, but it is still
imprefect).

I think we have learned by hard way that enabling ASPM by default can have
catastrophic effects for reasons we do not certainly know. So how about having
this platform specific API that enables individual platforms to enable the ASPM
states?

Sure, relying on the 'supports-clkreq' DT property will work for some platforms,
but this property is not widely used by all DT platforms (except Nvidia). For
instance, all our Qcom platforms support all ASPM states (except some known
platforms that do not support L0s due to incorrect PHY register settings) and we
do not set 'supports-clkreq' in DT. We can add them now, but old DTs which users
do not update will still suffer.

And we do not have a solution for VMD yet. So IMO, it is best to leave the ASPM
enablement to host controller drivers if they have the knowledge about it.

- Mani

[1] https://lore.kernel.org/linux-pci/20250825203542.3502368-1-david.e.box@linux.intel.com/

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Bjorn Helgaas]

On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> > 
> > Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> > the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> > Substates, for all devices powered on at the time the controller driver
> > enumerates them.
> > 
> > ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> > kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> > the user enabled ASPM via module parameter or sysfs).
> > 
> > After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> > devicetree platforms"), the PCI core enabled all ASPM states for all
> > devices whether powered on initially or by pwrctrl, so a729c1664619 was
> > unnecessary and reverted.
> > 
> > But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> > CLKREQ# or required device-specific configuration for L1 Substates, so
> > df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> > enabled only L0s and L1.
> > 
> > On Qualcomm platforms, this left L1 Substates disabled, which was a
> > regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> > that are initially powered on.  Devices powered on by pwrctrl will be
> > addressed later.
> 
> Can we rather have platform specific APIs [1] to enable ASPM states
> instead of just re-introducing this half-baked solution? (yes, I
> introduced it, but it is still imperfect).

I intend this (reverting "PCI: qcom: Remove custom ASPM enablement
code") for v6.18 to avoid regressing Qualcomm: v6.17 enabled L1 PM
Substates, and v6.18-rc3 does not.

Adding pci_host_set_default_pcie_link_state() with [1] (along with a
follow-up qcom patch using it) is another possible way to enable L1 PM
Substates, but I think the revert is the safest post-merge window
regression fix.

I have some heartburn about both the revert and the
pci_host_set_default_pcie_link_state() approach because they apply to
the entire hierarchy under a qcom or VMD root port, potentially
including add-in cards with switches.  CLKREQ# (and possibly more) is
required to enable L1SS, and I don't know if we can assume it's
supported on add-in links.

> I think we have learned by hard way that enabling ASPM by default
> can have catastrophic effects for reasons we do not certainly know.
> So how about having this platform specific API that enables
> individual platforms to enable the ASPM states?

As far as I know, it's L1SS that has catastrophic effects.  I haven't
seen anything for L0s or L1.

> [1]
> https://lore.kernel.org/linux-pci/20250825203542.3502368-1-david.e.box@linux.intel.com/

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Johan Hovold]

On Sun, Oct 26, 2025 at 02:37:54PM -0500, Bjorn Helgaas wrote:
> On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:

> As far as I know, it's L1SS that has catastrophic effects.  I haven't
> seen anything for L0s or L1.

Enabling L0s unconditionally certainly blew up on some Qualcomm
machines. See commit d1997c987814 ("PCI: qcom: Disable ASPM L0s for
sc8280xp, sa8540p and sa8295p").

Johan

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Manivannan Sadhasivam]

On Sun, Oct 26, 2025 at 02:37:54PM -0500, Bjorn Helgaas wrote:
> On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > 
> > > This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> > > 
> > > Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> > > the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> > > Substates, for all devices powered on at the time the controller driver
> > > enumerates them.
> > > 
> > > ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> > > kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> > > the user enabled ASPM via module parameter or sysfs).
> > > 
> > > After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> > > devicetree platforms"), the PCI core enabled all ASPM states for all
> > > devices whether powered on initially or by pwrctrl, so a729c1664619 was
> > > unnecessary and reverted.
> > > 
> > > But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> > > CLKREQ# or required device-specific configuration for L1 Substates, so
> > > df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> > > enabled only L0s and L1.
> > > 
> > > On Qualcomm platforms, this left L1 Substates disabled, which was a
> > > regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> > > that are initially powered on.  Devices powered on by pwrctrl will be
> > > addressed later.
> > 
> > Can we rather have platform specific APIs [1] to enable ASPM states
> > instead of just re-introducing this half-baked solution? (yes, I
> > introduced it, but it is still imperfect).
> 
> I intend this (reverting "PCI: qcom: Remove custom ASPM enablement
> code") for v6.18 to avoid regressing Qualcomm: v6.17 enabled L1 PM
> Substates, and v6.18-rc3 does not.
> 
> Adding pci_host_set_default_pcie_link_state() with [1] (along with a
> follow-up qcom patch using it) is another possible way to enable L1 PM
> Substates, but I think the revert is the safest post-merge window
> regression fix.
> 

I'm fine with the revert as a stopgap solution.

> I have some heartburn about both the revert and the
> pci_host_set_default_pcie_link_state() approach because they apply to
> the entire hierarchy under a qcom or VMD root port, potentially
> including add-in cards with switches.  CLKREQ# (and possibly more) is
> required to enable L1SS, and I don't know if we can assume it's
> supported on add-in links.
> 

I don't think we can assume, but at the same time, I don't think we will ever be
able to come up with a logical way to enable L1ss on all devices. But if we
leave the decision to the host controller drivers, they can at least guarantee
that the CLKREQ# and other requirements are satisfied from the host perspective
for L1ss. Then, if any device exhibit erratic behavior, we will for sure know
that the device is at fault and we can quirk them.

> > I think we have learned by hard way that enabling ASPM by default
> > can have catastrophic effects for reasons we do not certainly know.
> > So how about having this platform specific API that enables
> > individual platforms to enable the ASPM states?
> 
> As far as I know, it's L1SS that has catastrophic effects.  I haven't
> seen anything for L0s or L1.
> 

As Johan mentioned, we did see a near-catastrophic effect with enabling L0s on
some Qcom SoCs and we disabled the L0s CAP for them.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Manivannan Sadhasivam]

On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> 
> Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> Substates, for all devices powered on at the time the controller driver
> enumerates them.
> 
> ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> the user enabled ASPM via module parameter or sysfs).
> 
> After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> devicetree platforms"), the PCI core enabled all ASPM states for all
> devices whether powered on initially or by pwrctrl, so a729c1664619 was
> unnecessary and reverted.
> 
> But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> CLKREQ# or required device-specific configuration for L1 Substates, so
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> enabled only L0s and L1.
> 
> On Qualcomm platforms, this left L1 Substates disabled, which was a
> regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> that are initially powered on.  Devices powered on by pwrctrl will be
> addressed later.
> 
> Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 32 ++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6948824642dc..c48a20602d7f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -247,6 +247,7 @@ struct qcom_pcie_ops {
>  	int (*get_resources)(struct qcom_pcie *pcie);
>  	int (*init)(struct qcom_pcie *pcie);
>  	int (*post_init)(struct qcom_pcie *pcie);
> +	void (*host_post_init)(struct qcom_pcie *pcie);
>  	void (*deinit)(struct qcom_pcie *pcie);
>  	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  	int (*config_sid)(struct qcom_pcie *pcie);
> @@ -1038,6 +1039,25 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
> +{
> +	/*
> +	 * Downstream devices need to be in D0 state before enabling PCI PM
> +	 * substates.
> +	 */
> +	pci_set_power_state_locked(pdev, PCI_D0);
> +	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> +
> +	pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_aspm, NULL);
> +}
> +
>  static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> @@ -1312,9 +1332,19 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>  	pcie->cfg->ops->deinit(pcie);
>  }
>  
> +static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> +	if (pcie->cfg->ops->host_post_init)
> +		pcie->cfg->ops->host_post_init(pcie);
> +}
> +
>  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>  	.init		= qcom_pcie_host_init,
>  	.deinit		= qcom_pcie_host_deinit,
> +	.post_init	= qcom_pcie_host_post_init,
>  };
>  
>  /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
> @@ -1376,6 +1406,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.get_resources = qcom_pcie_get_resources_2_7_0,
>  	.init = qcom_pcie_init_2_7_0,
>  	.post_init = qcom_pcie_post_init_2_7_0,
> +	.host_post_init = qcom_pcie_host_post_init_2_7_0,
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  	.config_sid = qcom_pcie_config_sid_1_9_0,
> @@ -1386,6 +1417,7 @@ static const struct qcom_pcie_ops ops_1_21_0 = {
>  	.get_resources = qcom_pcie_get_resources_2_7_0,
>  	.init = qcom_pcie_init_2_7_0,
>  	.post_init = qcom_pcie_post_init_2_7_0,
> +	.host_post_init = qcom_pcie_host_post_init_2_7_0,
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  };
> -- 
> 2.43.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Johan Hovold]

On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> 
> Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> Substates, for all devices powered on at the time the controller driver
> enumerates them.
> 
> ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> the user enabled ASPM via module parameter or sysfs).
> 
> After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> devicetree platforms"), the PCI core enabled all ASPM states for all
> devices whether powered on initially or by pwrctrl, so a729c1664619 was
> unnecessary and reverted.
> 
> But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> CLKREQ# or required device-specific configuration for L1 Substates, so
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> enabled only L0s and L1.
> 
> On Qualcomm platforms, this left L1 Substates disabled, which was a
> regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> that are initially powered on.  Devices powered on by pwrctrl will be
> addressed later.
> 
> Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

This indeed re-enables L1SS for the X13s NVMe:

Reported-by: Johan Hovold <johan@kernel.org>
Link: https://lore.kernel.org/lkml/aPuXZlaawFmmsLmX@hovoldconsulting.com/
Tested-by: Johan Hovold <johan@kernel.org>

Johan

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Bjorn Helgaas]

On Mon, Oct 27, 2025 at 11:07:51AM +0100, Johan Hovold wrote:
> On Sun, Oct 26, 2025 at 02:37:54PM -0500, Bjorn Helgaas wrote:
> > On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> 
> > As far as I know, it's L1SS that has catastrophic effects.  I haven't
> > seen anything for L0s or L1.
> 
> Enabling L0s unconditionally certainly blew up on some Qualcomm
> machines. See commit d1997c987814 ("PCI: qcom: Disable ASPM L0s for
> sc8280xp, sa8540p and sa8295p").

Ah, right, thanks for that reminder.

IIUC the qcom_pcie_clear_aspm_l0s() quirk that removes L0s from the
advertised Link Capabilities is still there and prevents df5192d9bb0e
("PCI/ASPM: Enable only L0s and L1 for devicetree platforms") from
being a problem on those platforms, right?

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Bjorn Helgaas]

On Mon, Oct 27, 2025 at 05:21:30PM +0530, Manivannan Sadhasivam wrote:
> On Sun, Oct 26, 2025 at 02:37:54PM -0500, Bjorn Helgaas wrote:
> > On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > 
> > > > This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> > > > 
> > > > Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> > > > the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> > > > Substates, for all devices powered on at the time the controller driver
> > > > enumerates them.
> > > > 
> > > > ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> > > > kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> > > > the user enabled ASPM via module parameter or sysfs).
> > > > 
> > > > After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> > > > devicetree platforms"), the PCI core enabled all ASPM states for all
> > > > devices whether powered on initially or by pwrctrl, so a729c1664619 was
> > > > unnecessary and reverted.
> > > > 
> > > > But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> > > > CLKREQ# or required device-specific configuration for L1 Substates, so
> > > > df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> > > > enabled only L0s and L1.
> > > > 
> > > > On Qualcomm platforms, this left L1 Substates disabled, which was a
> > > > regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> > > > that are initially powered on.  Devices powered on by pwrctrl will be
> > > > addressed later.
> ...

> > I have some heartburn about both the revert and the
> > pci_host_set_default_pcie_link_state() approach because they apply to
> > the entire hierarchy under a qcom or VMD root port, potentially
> > including add-in cards with switches.  CLKREQ# (and possibly more) is
> > required to enable L1SS, and I don't know if we can assume it's
> > supported on add-in links.
> 
> I don't think we can assume, but at the same time, I don't think we
> will ever be able to come up with a logical way to enable L1ss on
> all devices. But if we leave the decision to the host controller
> drivers, they can at least guarantee that the CLKREQ# and other
> requirements are satisfied from the host perspective for L1ss. Then,
> if any device exhibit erratic behavior, we will for sure know that
> the device is at fault and we can quirk them.

If we can figure out that an endpoint is defective, a quirk is great.
But the issue might be something in the path, e.g., some connector in
the path leading to the endpoint doesn't include CLKREQ#, and we can't
quirk the endpoint then.

To me it sounds like the mainline kernel should be safe and only
enable L1SS when it has a clear signal that it is safe, either via
devicetree, ACPI, or L1SS configuration inherited from firmware.  I
don't want a future of telling users to boot with "pcie_aspm=off" if
a device doesn't work.

Enabling L1SS *without* such a clear signal feels like something
downstream kernels might have to do when they know more about the
topology.

Bjorn

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Bjorn Helgaas]

On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> 
> Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> Substates, for all devices powered on at the time the controller driver
> enumerates them.
> 
> ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> the user enabled ASPM via module parameter or sysfs).
> 
> After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> devicetree platforms"), the PCI core enabled all ASPM states for all
> devices whether powered on initially or by pwrctrl, so a729c1664619 was
> unnecessary and reverted.
> 
> But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> CLKREQ# or required device-specific configuration for L1 Substates, so
> df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> enabled only L0s and L1.
> 
> On Qualcomm platforms, this left L1 Substates disabled, which was a
> regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> that are initially powered on.  Devices powered on by pwrctrl will be
> addressed later.
> 
> Fixes: df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Applied to pci/for-linus for v6.18.

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 32 ++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 6948824642dc..c48a20602d7f 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -247,6 +247,7 @@ struct qcom_pcie_ops {
>  	int (*get_resources)(struct qcom_pcie *pcie);
>  	int (*init)(struct qcom_pcie *pcie);
>  	int (*post_init)(struct qcom_pcie *pcie);
> +	void (*host_post_init)(struct qcom_pcie *pcie);
>  	void (*deinit)(struct qcom_pcie *pcie);
>  	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  	int (*config_sid)(struct qcom_pcie *pcie);
> @@ -1038,6 +1039,25 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
> +{
> +	/*
> +	 * Downstream devices need to be in D0 state before enabling PCI PM
> +	 * substates.
> +	 */
> +	pci_set_power_state_locked(pdev, PCI_D0);
> +	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> +
> +	pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_aspm, NULL);
> +}
> +
>  static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> @@ -1312,9 +1332,19 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
>  	pcie->cfg->ops->deinit(pcie);
>  }
>  
> +static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> +	if (pcie->cfg->ops->host_post_init)
> +		pcie->cfg->ops->host_post_init(pcie);
> +}
> +
>  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>  	.init		= qcom_pcie_host_init,
>  	.deinit		= qcom_pcie_host_deinit,
> +	.post_init	= qcom_pcie_host_post_init,
>  };
>  
>  /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
> @@ -1376,6 +1406,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.get_resources = qcom_pcie_get_resources_2_7_0,
>  	.init = qcom_pcie_init_2_7_0,
>  	.post_init = qcom_pcie_post_init_2_7_0,
> +	.host_post_init = qcom_pcie_host_post_init_2_7_0,
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  	.config_sid = qcom_pcie_config_sid_1_9_0,
> @@ -1386,6 +1417,7 @@ static const struct qcom_pcie_ops ops_1_21_0 = {
>  	.get_resources = qcom_pcie_get_resources_2_7_0,
>  	.init = qcom_pcie_init_2_7_0,
>  	.post_init = qcom_pcie_post_init_2_7_0,
> +	.host_post_init = qcom_pcie_host_post_init_2_7_0,
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  };
> -- 
> 2.43.0
> 

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Manivannan Sadhasivam]

On Mon, Oct 27, 2025 at 05:35:17PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 27, 2025 at 11:07:51AM +0100, Johan Hovold wrote:
> > On Sun, Oct 26, 2025 at 02:37:54PM -0500, Bjorn Helgaas wrote:
> > > On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> > 
> > > As far as I know, it's L1SS that has catastrophic effects.  I haven't
> > > seen anything for L0s or L1.
> > 
> > Enabling L0s unconditionally certainly blew up on some Qualcomm
> > machines. See commit d1997c987814 ("PCI: qcom: Disable ASPM L0s for
> > sc8280xp, sa8540p and sa8295p").
> 
> Ah, right, thanks for that reminder.
> 
> IIUC the qcom_pcie_clear_aspm_l0s() quirk that removes L0s from the
> advertised Link Capabilities is still there and prevents df5192d9bb0e
> ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms") from
> being a problem on those platforms, right?

Yes. Since the capability itself is disabled, df5192d9bb0e doesn't affect our
platforms.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] Revert "PCI: qcom: Remove custom ASPM enablement code"

[Manivannan Sadhasivam]

On Mon, Oct 27, 2025 at 06:07:03PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 27, 2025 at 05:21:30PM +0530, Manivannan Sadhasivam wrote:
> > On Sun, Oct 26, 2025 at 02:37:54PM -0500, Bjorn Helgaas wrote:
> > > On Sun, Oct 26, 2025 at 08:58:29PM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Oct 24, 2025 at 04:04:57PM -0500, Bjorn Helgaas wrote:
> > > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > > 
> > > > > This reverts commit a729c16646198872e345bf6c48dbe540ad8a9753.
> > > > > 
> > > > > Prior to a729c1664619 ("PCI: qcom: Remove custom ASPM enablement code"),
> > > > > the qcom controller driver enabled ASPM, including L0s, L1, and L1 PM
> > > > > Substates, for all devices powered on at the time the controller driver
> > > > > enumerates them.
> > > > > 
> > > > > ASPM was *not* enabled for devices powered on later by pwrctrl (unless the
> > > > > kernel was built with PCIEASPM_POWERSAVE or PCIEASPM_POWER_SUPERSAVE, or
> > > > > the user enabled ASPM via module parameter or sysfs).
> > > > > 
> > > > > After f3ac2ff14834 ("PCI/ASPM: Enable all ClockPM and ASPM states for
> > > > > devicetree platforms"), the PCI core enabled all ASPM states for all
> > > > > devices whether powered on initially or by pwrctrl, so a729c1664619 was
> > > > > unnecessary and reverted.
> > > > > 
> > > > > But f3ac2ff14834 was too aggressive and broke platforms that didn't support
> > > > > CLKREQ# or required device-specific configuration for L1 Substates, so
> > > > > df5192d9bb0e ("PCI/ASPM: Enable only L0s and L1 for devicetree platforms")
> > > > > enabled only L0s and L1.
> > > > > 
> > > > > On Qualcomm platforms, this left L1 Substates disabled, which was a
> > > > > regression.  Revert a729c1664619 so L1 Substates will be enabled on devices
> > > > > that are initially powered on.  Devices powered on by pwrctrl will be
> > > > > addressed later.
> > ...
> 
> > > I have some heartburn about both the revert and the
> > > pci_host_set_default_pcie_link_state() approach because they apply to
> > > the entire hierarchy under a qcom or VMD root port, potentially
> > > including add-in cards with switches.  CLKREQ# (and possibly more) is
> > > required to enable L1SS, and I don't know if we can assume it's
> > > supported on add-in links.
> > 
> > I don't think we can assume, but at the same time, I don't think we
> > will ever be able to come up with a logical way to enable L1ss on
> > all devices. But if we leave the decision to the host controller
> > drivers, they can at least guarantee that the CLKREQ# and other
> > requirements are satisfied from the host perspective for L1ss. Then,
> > if any device exhibit erratic behavior, we will for sure know that
> > the device is at fault and we can quirk them.
> 
> If we can figure out that an endpoint is defective, a quirk is great.
> But the issue might be something in the path, e.g., some connector in
> the path leading to the endpoint doesn't include CLKREQ#, and we can't
> quirk the endpoint then.
> 

I got your concern, in this case, we can have board specific quirks for those
with broken connectors and such. Let's say if the SoC supports CLKREQ#, but one
of the downstream connectors of a switch connected to the Root Port doesn't,
then we can have a quirk based on the board compatible and the switch combo.
It is implied that the person who is designing the board is aware of this issue
and they can add the quirk by themselves (without affecting users).

> To me it sounds like the mainline kernel should be safe and only
> enable L1SS when it has a clear signal that it is safe, either via
> devicetree, ACPI, or L1SS configuration inherited from firmware.  I
> don't want a future of telling users to boot with "pcie_aspm=off" if
> a device doesn't work.
> 

It is a trade-off. I completely agree that we do not want to break users, but at
the same time, the kernel should provide optimum power savings by default also
(if it has the knowledge).

The SoCs targetting embedded/laptop usecases want to make use of L1ss to prevent
draining battery too fast. Currently, on Qcom platforms, we are asking our users
to enable ASPM through Kconfig/cmdline to make sure their laptop battery last
long. And we cannot ask distros to enable these for users as distros would only
use a common kernel/config for all platforms they support. So the burden
essentially lies on the user. All this could've been fixed if the BIOS enabled
L1ss, but it doesn't unfortunately.

Also, it is not possible to rely on DT/ACPI or other platform description to
tell the OS about CLKREQ# routing for all the devices in the topology. They can
give the information about Root Ports and other devices attached to the board in
the enclosure, but not about the devices that will get attached to the open slot
dynamically.

Also, for a discoverable bus like PCI, DT do not necessarily describe all
devices unless they requires some special handling like power sequencing.

- Mani

-- 
மணிவண்ணன் சதாசிவம்