[PATCH 0/2] PCI/ASPM: Enable ASPM and Clock PM by default on devicetree platforms

[PATCH 0/2] PCI/ASPM: Enable ASPM and Clock PM by default on devicetree platforms

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series is one of the 'let's bite the bullet' kind, where we have decided to
enable all ASPM and Clock PM states by default on devicetree platforms [1]. The
reason why devicetree platforms were chosen because, it will be of minimal
impact compared to the ACPI platforms. So seemed ideal to test the waters.

Problem Statement
=================

Historically, PCI subsystem relied on the BIOS to enable ASPM and Clock PM
states for PCI devices before the kernel boot. This was done to avoid enabling
ASPM for the buggy devices that are known to create issues with ASPM (even
though they advertise the ASPM capability). But BIOS is not at all a thing on
most of the non-x86 platforms. For instance, the majority of the Embedded and
Compute ARM based platforms using devicetree have something called bootloader,
which is not anyway near the standard BIOS used in x86 based platforms. And
these bootloaders wouldn't touch PCIe at all, unless they boot using PCIe
storage, even then there would be no guarantee that the ASPM states will get
enabled. Another example is the Intel's VMD domain that is not at all configured
by the BIOS. But, this series is not enabling ASPM/Clock PM for VMD domain. I
hope it will be done similarly in the future patches.

Solution
========

So to avoid relying on BIOS, it was agreed [2] that the PCI subsystem has to
enable ASPM and Clock PM states based on the device capability. If any devices
misbehave, then they should be quirked accordingly.

First patch of this series introduces two helper functions to enable all ASPM
and Clock PM states if CONFIG_OF is enabled. Second patch drops the custom ASPM
enablement code from the pcie-qcom driver as it is no longer needed.

Testing
=======

This series is tested on Lenovo Thinkpad T14s based on Snapdragon X1 SoC. All
supported ASPM states are getting enabled for both the NVMe and WLAN devices by
default.

[1] https://lore.kernel.org/linux-pci/a47sg5ahflhvzyzqnfxvpk3dw4clkhqlhznjxzwqpf4nyjx5dk@bcghz5o6zolk
[2] https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Manivannan Sadhasivam (2):
      PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms
      PCI: qcom: Remove the custom ASPM enablement code

 drivers/pci/controller/dwc/pcie-qcom.c | 32 -----------------------
 drivers/pci/pcie/aspm.c                | 48 ++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 37 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250916-pci-dt-aspm-8b3a7e8d2cf1

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

[PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

So far, the PCI subsystem has honored the ASPM and Clock PM states set by
the BIOS (through LNKCTL) during device initialization. This was done
conservatively to avoid issues with the buggy devices that advertise
ASPM capabilities, but behave erratically if the ASPM states are enabled.
So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
states that were known to work for the devices.

But this turned out to be a problem for devicetree platforms, especially
the ARM based devicetree platforms powering Embedded and *some* Compute
devices as they tend to run without any standard BIOS. So the ASPM states
on these platforms were left disabled during boot and the PCI subsystem
never bothered to enable them, unless the user has forcefully enabled the
ASPM states through Kconfig, cmdline, and sysfs or the device drivers
themselves, enabling the ASPM states through pci_enable_link_state() APIs.

This caused runtime power issues on those platforms. So a couple of
approaches were tried to mitigate this BIOS dependency without user
intervention by enabling the ASPM states in the PCI controller drivers
after device enumeration, and overriding the ASPM/Clock PM states
by the PCI controller drivers through an API before enumeration.

But it has been concluded that none of these mitigations should really be
required and the PCI subsystem should enable the ASPM states advertised by
the devices without relying on BIOS or the PCI controller drivers. If any
device is found to be misbehaving after enabling ASPM states that they
advertised, then those devices should be quirked to disable the problematic
ASPM/Clock PM states.

In an effort to do so, start by overriding the ASPM and Clock PM states set
by the BIOS for devicetree platforms first. Separate helper functions are
introduced to set the default ASPM and Clock PM states and they will
override the BIOS set states by enabling all of them if CONFIG_OF is
enabled. To aid debugging, print the overridden ASPM and Clock PM states.

In the future, these helpers could be extended to allow other platforms
like VMD, newer ACPI systems with a cutoff year etc... to follow the path.

Link: https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pcie/aspm.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 919a05b9764791c3cc469c9ada62ba5b2c405118..1e7218c5e9127699fdbf172c277aad3f847c43be 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -235,13 +235,15 @@ struct pcie_link_state {
 	u32 aspm_support:7;		/* Supported ASPM state */
 	u32 aspm_enabled:7;		/* Enabled ASPM state */
 	u32 aspm_capable:7;		/* Capable ASPM state with latency */
-	u32 aspm_default:7;		/* Default ASPM state by BIOS */
+	u32 aspm_default:7;		/* Default ASPM state by BIOS or
+					   override */
 	u32 aspm_disable:7;		/* Disabled ASPM state */
 
 	/* Clock PM state */
 	u32 clkpm_capable:1;		/* Clock PM capable? */
 	u32 clkpm_enabled:1;		/* Current Clock PM state */
-	u32 clkpm_default:1;		/* Default Clock PM state by BIOS */
+	u32 clkpm_default:1;		/* Default Clock PM state by BIOS or
+					   override */
 	u32 clkpm_disable:1;		/* Clock PM disabled */
 };
 
@@ -373,6 +375,20 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
 	pcie_set_clkpm_nocheck(link, enable);
 }
 
+static void pcie_clkpm_set_default_link_state(struct pcie_link_state *link,
+					      int enabled)
+{
+	struct pci_dev *pdev = link->downstream;
+
+	link->clkpm_default = enabled;
+
+	/* Override the BIOS disabled Clock PM state for devicetree platforms */
+	if (IS_ENABLED(CONFIG_OF) && !enabled) {
+		link->clkpm_default = 1;
+		pci_info(pdev, "Clock PM state overridden\n");
+	}
+}
+
 static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	int capable = 1, enabled = 1;
@@ -394,7 +410,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 			enabled = 0;
 	}
 	link->clkpm_enabled = enabled;
-	link->clkpm_default = enabled;
+	pcie_clkpm_set_default_link_state(link, enabled);
 	link->clkpm_capable = capable;
 	link->clkpm_disable = blacklist ? 1 : 0;
 }
@@ -788,6 +804,29 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
+static void pcie_aspm_set_default_link_state(struct pcie_link_state *link)
+{
+	struct pci_dev *pdev = link->downstream;
+	u32 override;
+
+	/* Set BIOS enabled states as the default */
+	link->aspm_default = link->aspm_enabled;
+
+	/* Override the BIOS disabled ASPM states for devicetree platforms */
+	if (IS_ENABLED(CONFIG_OF)) {
+		link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
+		override = link->aspm_default & ~link->aspm_enabled;
+		if (override)
+			pci_info(pdev, "ASPM states overridden: %s%s%s%s%s%s\n",
+				 (override & PCIE_LINK_STATE_L0S) ? "L0s, " : "",
+				 (override & PCIE_LINK_STATE_L1) ? "L1, " : "",
+				 (override & PCIE_LINK_STATE_L1_1) ? "L1.1, " : "",
+				 (override & PCIE_LINK_STATE_L1_2) ? "L1.2, " : "",
+				 (override & PCIE_LINK_STATE_L1_1_PCIPM) ? "L1.1 PCI-PM, " : "",
+				 (override & PCIE_LINK_STATE_L1_2_PCIPM) ? "L1.2 PCI-PM" : "");
+	}
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -865,8 +904,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		pcie_capability_write_word(child, PCI_EXP_LNKCTL, child_lnkctl);
 	}
 
-	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	pcie_aspm_set_default_link_state(link);
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;

-- 
2.45.2

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Konrad Dybcio]

On 9/16/25 6:12 PM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> the BIOS (through LNKCTL) during device initialization. This was done
> conservatively to avoid issues with the buggy devices that advertise
> ASPM capabilities, but behave erratically if the ASPM states are enabled.
> So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> states that were known to work for the devices.
> 
> But this turned out to be a problem for devicetree platforms, especially
> the ARM based devicetree platforms powering Embedded and *some* Compute
> devices as they tend to run without any standard BIOS. So the ASPM states
> on these platforms were left disabled during boot and the PCI subsystem
> never bothered to enable them, unless the user has forcefully enabled the
> ASPM states through Kconfig, cmdline, and sysfs or the device drivers
> themselves, enabling the ASPM states through pci_enable_link_state() APIs.
> 
> This caused runtime power issues on those platforms. So a couple of
> approaches were tried to mitigate this BIOS dependency without user
> intervention by enabling the ASPM states in the PCI controller drivers
> after device enumeration, and overriding the ASPM/Clock PM states
> by the PCI controller drivers through an API before enumeration.
> 
> But it has been concluded that none of these mitigations should really be
> required and the PCI subsystem should enable the ASPM states advertised by
> the devices without relying on BIOS or the PCI controller drivers. If any
> device is found to be misbehaving after enabling ASPM states that they
> advertised, then those devices should be quirked to disable the problematic
> ASPM/Clock PM states.
> 
> In an effort to do so, start by overriding the ASPM and Clock PM states set
> by the BIOS for devicetree platforms first. Separate helper functions are
> introduced to set the default ASPM and Clock PM states and they will
> override the BIOS set states by enabling all of them if CONFIG_OF is
> enabled. To aid debugging, print the overridden ASPM and Clock PM states.
> 
> In the future, these helpers could be extended to allow other platforms
> like VMD, newer ACPI systems with a cutoff year etc... to follow the path.
> 
> Link: https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---

[...]

> +	/* Override the BIOS disabled Clock PM state for devicetree platforms */
> +	if (IS_ENABLED(CONFIG_OF) && !enabled) {

JFYI CONFIG_OF=y && CONFIG_ACPI=y is valid, at least on arm64
Maybe something like of_have_populated_dt()?

You can then choose which one to use with e.g. acpi=force

Konrad

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Manivannan Sadhasivam]

On Tue, Sep 16, 2025 at 06:28:58PM GMT, Konrad Dybcio wrote:
> On 9/16/25 6:12 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> > the BIOS (through LNKCTL) during device initialization. This was done
> > conservatively to avoid issues with the buggy devices that advertise
> > ASPM capabilities, but behave erratically if the ASPM states are enabled.
> > So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> > states that were known to work for the devices.
> > 
> > But this turned out to be a problem for devicetree platforms, especially
> > the ARM based devicetree platforms powering Embedded and *some* Compute
> > devices as they tend to run without any standard BIOS. So the ASPM states
> > on these platforms were left disabled during boot and the PCI subsystem
> > never bothered to enable them, unless the user has forcefully enabled the
> > ASPM states through Kconfig, cmdline, and sysfs or the device drivers
> > themselves, enabling the ASPM states through pci_enable_link_state() APIs.
> > 
> > This caused runtime power issues on those platforms. So a couple of
> > approaches were tried to mitigate this BIOS dependency without user
> > intervention by enabling the ASPM states in the PCI controller drivers
> > after device enumeration, and overriding the ASPM/Clock PM states
> > by the PCI controller drivers through an API before enumeration.
> > 
> > But it has been concluded that none of these mitigations should really be
> > required and the PCI subsystem should enable the ASPM states advertised by
> > the devices without relying on BIOS or the PCI controller drivers. If any
> > device is found to be misbehaving after enabling ASPM states that they
> > advertised, then those devices should be quirked to disable the problematic
> > ASPM/Clock PM states.
> > 
> > In an effort to do so, start by overriding the ASPM and Clock PM states set
> > by the BIOS for devicetree platforms first. Separate helper functions are
> > introduced to set the default ASPM and Clock PM states and they will
> > override the BIOS set states by enabling all of them if CONFIG_OF is
> > enabled. To aid debugging, print the overridden ASPM and Clock PM states.
> > 
> > In the future, these helpers could be extended to allow other platforms
> > like VMD, newer ACPI systems with a cutoff year etc... to follow the path.
> > 
> > Link: https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> 
> [...]
> 
> > +	/* Override the BIOS disabled Clock PM state for devicetree platforms */
> > +	if (IS_ENABLED(CONFIG_OF) && !enabled) {
> 
> JFYI CONFIG_OF=y && CONFIG_ACPI=y is valid, at least on arm64

Ouch! I didn't know this, thanks for pointing it out.

> Maybe something like of_have_populated_dt()?
> 

Yep, this looks like the correct API.

- Mani

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

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Bjorn Helgaas]

On Tue, Sep 16, 2025 at 09:42:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> the BIOS (through LNKCTL) during device initialization. This was done
> conservatively to avoid issues with the buggy devices that advertise
> ASPM capabilities, but behave erratically if the ASPM states are enabled.
> So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> states that were known to work for the devices.

Questions about what exactly "honoring ASPM states set by BIOS" means:

  - I *think* honoring ASPM states set by BIOS means that Linux
    doesn't change ASPM config at boot-time, and this only applies
    when:

    * CONFIG_PCIEASPM_DEFAULT=y, or
    * booting with "pcie_aspm=off", or
    * FADT has ACPI_FADT_NO_ASPM set, or
    * platform retained control of PCIe Capability via _OSC (I'm not
      sure we enforce this today, but I think we should)

  - IIUC we always save the pre-Linux config in link->aspm_default,
    but when CONFIG_PCIEASPM_POWERSAVE, CONFIG_PCIEASPM_POWER_SUPERSAVE,
    or CONFIG_PCIEASPM_PERFORMANCE are set, Linux immediately
    reconfigures ASPM.

  - But I *think* the config option is not restrictive, and users can
    do more aggressive ASPM configuration at run-time via sysfs, right?
    (Assuming the platform hasn't prevented Linux from doing so)

If users can configure ASPM differently than BIOS did, we're liable to
trip over issues later even though we claim to honor ASPM states set
by BIOS, so I think CONFIG_PCIEASPM_DEFAULT is kind of a fig leaf that
only defers issues.

I'd really like to get rid of all those CONFIG_PCIEASPM_* options
because I don't think they have any business being build-time things,
but I don't think that would have to be in this series.

> But this turned out to be a problem for devicetree platforms, especially
> the ARM based devicetree platforms powering Embedded and *some* Compute
> devices as they tend to run without any standard BIOS. So the ASPM states
> on these platforms were left disabled during boot and the PCI subsystem
> never bothered to enable them, unless the user has forcefully enabled the
> ASPM states through Kconfig, cmdline, and sysfs or the device drivers
> themselves, enabling the ASPM states through pci_enable_link_state() APIs.
> 
> This caused runtime power issues on those platforms. So a couple of
> approaches were tried to mitigate this BIOS dependency without user
> intervention by enabling the ASPM states in the PCI controller drivers
> after device enumeration, and overriding the ASPM/Clock PM states
> by the PCI controller drivers through an API before enumeration.
> 
> But it has been concluded that none of these mitigations should really be
> required and the PCI subsystem should enable the ASPM states advertised by
> the devices without relying on BIOS or the PCI controller drivers. If any
> device is found to be misbehaving after enabling ASPM states that they
> advertised, then those devices should be quirked to disable the problematic
> ASPM/Clock PM states.
> 
> In an effort to do so, start by overriding the ASPM and Clock PM states set
> by the BIOS for devicetree platforms first. Separate helper functions are
> introduced to set the default ASPM and Clock PM states and they will
> override the BIOS set states by enabling all of them if CONFIG_OF is
> enabled. To aid debugging, print the overridden ASPM and Clock PM states.
> 
> In the future, these helpers could be extended to allow other platforms
> like VMD, newer ACPI systems with a cutoff year etc... to follow the path.
> 
> Link: https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/pcie/aspm.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 919a05b9764791c3cc469c9ada62ba5b2c405118..1e7218c5e9127699fdbf172c277aad3f847c43be 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -235,13 +235,15 @@ struct pcie_link_state {
>  	u32 aspm_support:7;		/* Supported ASPM state */
>  	u32 aspm_enabled:7;		/* Enabled ASPM state */
>  	u32 aspm_capable:7;		/* Capable ASPM state with latency */
> -	u32 aspm_default:7;		/* Default ASPM state by BIOS */
> +	u32 aspm_default:7;		/* Default ASPM state by BIOS or
> +					   override */
>  	u32 aspm_disable:7;		/* Disabled ASPM state */
>  
>  	/* Clock PM state */
>  	u32 clkpm_capable:1;		/* Clock PM capable? */
>  	u32 clkpm_enabled:1;		/* Current Clock PM state */
> -	u32 clkpm_default:1;		/* Default Clock PM state by BIOS */
> +	u32 clkpm_default:1;		/* Default Clock PM state by BIOS or
> +					   override */
>  	u32 clkpm_disable:1;		/* Clock PM disabled */
>  };
>  
> @@ -373,6 +375,20 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>  	pcie_set_clkpm_nocheck(link, enable);
>  }
>  
> +static void pcie_clkpm_set_default_link_state(struct pcie_link_state *link,
> +					      int enabled)
> +{
> +	struct pci_dev *pdev = link->downstream;
> +
> +	link->clkpm_default = enabled;
> +
> +	/* Override the BIOS disabled Clock PM state for devicetree platforms */
> +	if (IS_ENABLED(CONFIG_OF) && !enabled) {
> +		link->clkpm_default = 1;
> +		pci_info(pdev, "Clock PM state overridden\n");

It's obvious from the code that this message means we're going to
*enable* ClockPM, but I want to know from the message itself what the
resulting state is, not just that it was overridden.

Maybe "ClockPM+" or "ClockPM-" like lspci does?

Maybe we should have a pci_dbg() for the current state?

For debuggability, I wonder if we should have a pci_dbg() at the point
where we actually update PCI_EXP_LNKCTL, PCI_L1SS_CTL1, etc?  I could
even argue for pci_info() since this should be a low-frequency and
relatively high-risk event.

> +	}
> +}
> +
>  static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
>  	int capable = 1, enabled = 1;
> @@ -394,7 +410,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>  			enabled = 0;
>  	}
>  	link->clkpm_enabled = enabled;
> -	link->clkpm_default = enabled;

I think both the patch and the resulting code would be easier to read
if we preserved the link->clkpm_enabled here and simply added the call
to pcie_clkpm_set_default_link_state().

> +	pcie_clkpm_set_default_link_state(link, enabled);
>  	link->clkpm_capable = capable;
>  	link->clkpm_disable = blacklist ? 1 : 0;
>  }
> @@ -788,6 +804,29 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
>  		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
>  }
>  
> +static void pcie_aspm_set_default_link_state(struct pcie_link_state *link)
> +{
> +	struct pci_dev *pdev = link->downstream;
> +	u32 override;
> +
> +	/* Set BIOS enabled states as the default */
> +	link->aspm_default = link->aspm_enabled;
> +
> +	/* Override the BIOS disabled ASPM states for devicetree platforms */
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
> +		override = link->aspm_default & ~link->aspm_enabled;
> +		if (override)
> +			pci_info(pdev, "ASPM states overridden: %s%s%s%s%s%s\n",
> +				 (override & PCIE_LINK_STATE_L0S) ? "L0s, " : "",
> +				 (override & PCIE_LINK_STATE_L1) ? "L1, " : "",
> +				 (override & PCIE_LINK_STATE_L1_1) ? "L1.1, " : "",
> +				 (override & PCIE_LINK_STATE_L1_2) ? "L1.2, " : "",
> +				 (override & PCIE_LINK_STATE_L1_1_PCIPM) ? "L1.1 PCI-PM, " : "",
> +				 (override & PCIE_LINK_STATE_L1_2_PCIPM) ? "L1.2 PCI-PM" : "");

Same here.

> +	}
> +}
> +
>  static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
> @@ -865,8 +904,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  		pcie_capability_write_word(child, PCI_EXP_LNKCTL, child_lnkctl);
>  	}
>  
> -	/* Save default state */
> -	link->aspm_default = link->aspm_enabled;

Same here.

> +	pcie_aspm_set_default_link_state(link);
>  
>  	/* Setup initial capable state. Will be updated later */
>  	link->aspm_capable = link->aspm_support;
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Manivannan Sadhasivam]

On Tue, Sep 16, 2025 at 12:15:46PM GMT, Bjorn Helgaas wrote:
> On Tue, Sep 16, 2025 at 09:42:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> > the BIOS (through LNKCTL) during device initialization. This was done
> > conservatively to avoid issues with the buggy devices that advertise
> > ASPM capabilities, but behave erratically if the ASPM states are enabled.
> > So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> > states that were known to work for the devices.
> 
> Questions about what exactly "honoring ASPM states set by BIOS" means:
> 
>   - I *think* honoring ASPM states set by BIOS means that Linux
>     doesn't change ASPM config at boot-time, and this only applies
>     when:
> 
>     * CONFIG_PCIEASPM_DEFAULT=y, or
>     * booting with "pcie_aspm=off", or
>     * FADT has ACPI_FADT_NO_ASPM set, or
>     * platform retained control of PCIe Capability via _OSC (I'm not
>       sure we enforce this today, but I think we should)
> 
>   - IIUC we always save the pre-Linux config in link->aspm_default,
>     but when CONFIG_PCIEASPM_POWERSAVE, CONFIG_PCIEASPM_POWER_SUPERSAVE,
>     or CONFIG_PCIEASPM_PERFORMANCE are set, Linux immediately
>     reconfigures ASPM.
> 
>   - But I *think* the config option is not restrictive, and users can
>     do more aggressive ASPM configuration at run-time via sysfs, right?
>     (Assuming the platform hasn't prevented Linux from doing so)
> 
> If users can configure ASPM differently than BIOS did, we're liable to
> trip over issues later even though we claim to honor ASPM states set
> by BIOS, so I think CONFIG_PCIEASPM_DEFAULT is kind of a fig leaf that
> only defers issues.
> 

True. Similar to how we allow users to fiddle with the PCI config space through
sysfs even when a driver is bound to the device.

> I'd really like to get rid of all those CONFIG_PCIEASPM_* options
> because I don't think they have any business being build-time things,
> but I don't think that would have to be in this series.
> 
> > But this turned out to be a problem for devicetree platforms, especially
> > the ARM based devicetree platforms powering Embedded and *some* Compute
> > devices as they tend to run without any standard BIOS. So the ASPM states
> > on these platforms were left disabled during boot and the PCI subsystem
> > never bothered to enable them, unless the user has forcefully enabled the
> > ASPM states through Kconfig, cmdline, and sysfs or the device drivers
> > themselves, enabling the ASPM states through pci_enable_link_state() APIs.
> > 
> > This caused runtime power issues on those platforms. So a couple of
> > approaches were tried to mitigate this BIOS dependency without user
> > intervention by enabling the ASPM states in the PCI controller drivers
> > after device enumeration, and overriding the ASPM/Clock PM states
> > by the PCI controller drivers through an API before enumeration.
> > 
> > But it has been concluded that none of these mitigations should really be
> > required and the PCI subsystem should enable the ASPM states advertised by
> > the devices without relying on BIOS or the PCI controller drivers. If any
> > device is found to be misbehaving after enabling ASPM states that they
> > advertised, then those devices should be quirked to disable the problematic
> > ASPM/Clock PM states.
> > 
> > In an effort to do so, start by overriding the ASPM and Clock PM states set
> > by the BIOS for devicetree platforms first. Separate helper functions are
> > introduced to set the default ASPM and Clock PM states and they will
> > override the BIOS set states by enabling all of them if CONFIG_OF is
> > enabled. To aid debugging, print the overridden ASPM and Clock PM states.
> > 
> > In the future, these helpers could be extended to allow other platforms
> > like VMD, newer ACPI systems with a cutoff year etc... to follow the path.
> > 
> > Link: https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 43 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 919a05b9764791c3cc469c9ada62ba5b2c405118..1e7218c5e9127699fdbf172c277aad3f847c43be 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -235,13 +235,15 @@ struct pcie_link_state {
> >  	u32 aspm_support:7;		/* Supported ASPM state */
> >  	u32 aspm_enabled:7;		/* Enabled ASPM state */
> >  	u32 aspm_capable:7;		/* Capable ASPM state with latency */
> > -	u32 aspm_default:7;		/* Default ASPM state by BIOS */
> > +	u32 aspm_default:7;		/* Default ASPM state by BIOS or
> > +					   override */
> >  	u32 aspm_disable:7;		/* Disabled ASPM state */
> >  
> >  	/* Clock PM state */
> >  	u32 clkpm_capable:1;		/* Clock PM capable? */
> >  	u32 clkpm_enabled:1;		/* Current Clock PM state */
> > -	u32 clkpm_default:1;		/* Default Clock PM state by BIOS */
> > +	u32 clkpm_default:1;		/* Default Clock PM state by BIOS or
> > +					   override */
> >  	u32 clkpm_disable:1;		/* Clock PM disabled */
> >  };
> >  
> > @@ -373,6 +375,20 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> >  	pcie_set_clkpm_nocheck(link, enable);
> >  }
> >  
> > +static void pcie_clkpm_set_default_link_state(struct pcie_link_state *link,
> > +					      int enabled)
> > +{
> > +	struct pci_dev *pdev = link->downstream;
> > +
> > +	link->clkpm_default = enabled;
> > +
> > +	/* Override the BIOS disabled Clock PM state for devicetree platforms */
> > +	if (IS_ENABLED(CONFIG_OF) && !enabled) {
> > +		link->clkpm_default = 1;
> > +		pci_info(pdev, "Clock PM state overridden\n");
> 
> It's obvious from the code that this message means we're going to
> *enable* ClockPM, but I want to know from the message itself what the
> resulting state is, not just that it was overridden.
> 
> Maybe "ClockPM+" or "ClockPM-" like lspci does?
> 

We are not disabling ClockPM here, but just enabling it. So adding 'ClockPM+'
would make sense.

> Maybe we should have a pci_dbg() for the current state?
> 

Since we are always enabling ClockPM if it was disabled, printing the current
state is redundant.

> For debuggability, I wonder if we should have a pci_dbg() at the point
> where we actually update PCI_EXP_LNKCTL, PCI_L1SS_CTL1, etc?  I could
> even argue for pci_info() since this should be a low-frequency and
> relatively high-risk event.
> 

I don't know why we should print register settings since we are explicitly
printing out what states are getting enabled.

> > +	}
> > +}
> > +
> >  static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> >  {
> >  	int capable = 1, enabled = 1;
> > @@ -394,7 +410,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> >  			enabled = 0;
> >  	}
> >  	link->clkpm_enabled = enabled;
> > -	link->clkpm_default = enabled;
> 
> I think both the patch and the resulting code would be easier to read
> if we preserved the link->clkpm_enabled here and simply added the call
> to pcie_clkpm_set_default_link_state().
> 

Ok, in that case the helper should be renamed to
pcie_{clkpm/aspm}_override_default_link_state().

> > +	pcie_clkpm_set_default_link_state(link, enabled);
> >  	link->clkpm_capable = capable;
> >  	link->clkpm_disable = blacklist ? 1 : 0;
> >  }
> > @@ -788,6 +804,29 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
> >  		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
> >  }
> >  
> > +static void pcie_aspm_set_default_link_state(struct pcie_link_state *link)
> > +{
> > +	struct pci_dev *pdev = link->downstream;
> > +	u32 override;
> > +
> > +	/* Set BIOS enabled states as the default */
> > +	link->aspm_default = link->aspm_enabled;
> > +
> > +	/* Override the BIOS disabled ASPM states for devicetree platforms */
> > +	if (IS_ENABLED(CONFIG_OF)) {
> > +		link->aspm_default = PCIE_LINK_STATE_ASPM_ALL;
> > +		override = link->aspm_default & ~link->aspm_enabled;
> > +		if (override)
> > +			pci_info(pdev, "ASPM states overridden: %s%s%s%s%s%s\n",
> > +				 (override & PCIE_LINK_STATE_L0S) ? "L0s, " : "",
> > +				 (override & PCIE_LINK_STATE_L1) ? "L1, " : "",
> > +				 (override & PCIE_LINK_STATE_L1_1) ? "L1.1, " : "",
> > +				 (override & PCIE_LINK_STATE_L1_2) ? "L1.2, " : "",
> > +				 (override & PCIE_LINK_STATE_L1_1_PCIPM) ? "L1.1 PCI-PM, " : "",
> > +				 (override & PCIE_LINK_STATE_L1_2_PCIPM) ? "L1.2 PCI-PM" : "");
> 
> Same here.
> 

I will add '+' to make it clear that these states are getting enabled.

- Mani

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

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Bjorn Helgaas]

[+cc Kai-Heng, Rafael, Heiner, AceLan; response to
https://lore.kernel.org/r/20250916-pci-dt-aspm-v1-1-778fe907c9ad@oss.qualcomm.com]

On Wed, Sep 17, 2025 at 04:14:42PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Sep 16, 2025 at 12:15:46PM GMT, Bjorn Helgaas wrote:
> > On Tue, Sep 16, 2025 at 09:42:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> > > the BIOS (through LNKCTL) during device initialization. This was done
> > > conservatively to avoid issues with the buggy devices that advertise
> > > ASPM capabilities, but behave erratically if the ASPM states are enabled.
> > > So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> > > states that were known to work for the devices.
> ...

> > For debuggability, I wonder if we should have a pci_dbg() at the point
> > where we actually update PCI_EXP_LNKCTL, PCI_L1SS_CTL1, etc?  I could
> > even argue for pci_info() since this should be a low-frequency and
> > relatively high-risk event.
> 
> I don't know why we should print register settings since we are explicitly
> printing out what states are getting enabled.

My thinking here is that we care about is what is actually written to
the device, not what we *intend* to write to the device.

There's a lot of complicated aspm.c code between setting
link->clkpm_default/aspm_default and actually programming the device,
and when debugging a problem, I don't want to have to parse all that
code to derive the register values.

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Manivannan Sadhasivam]

On Wed, Sep 17, 2025 at 06:22:18AM GMT, Bjorn Helgaas wrote:
> [+cc Kai-Heng, Rafael, Heiner, AceLan; response to
> https://lore.kernel.org/r/20250916-pci-dt-aspm-v1-1-778fe907c9ad@oss.qualcomm.com]
> 
> On Wed, Sep 17, 2025 at 04:14:42PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Sep 16, 2025 at 12:15:46PM GMT, Bjorn Helgaas wrote:
> > > On Tue, Sep 16, 2025 at 09:42:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> > > > the BIOS (through LNKCTL) during device initialization. This was done
> > > > conservatively to avoid issues with the buggy devices that advertise
> > > > ASPM capabilities, but behave erratically if the ASPM states are enabled.
> > > > So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> > > > states that were known to work for the devices.
> > ...
> 
> > > For debuggability, I wonder if we should have a pci_dbg() at the point
> > > where we actually update PCI_EXP_LNKCTL, PCI_L1SS_CTL1, etc?  I could
> > > even argue for pci_info() since this should be a low-frequency and
> > > relatively high-risk event.
> > 
> > I don't know why we should print register settings since we are explicitly
> > printing out what states are getting enabled.
> 
> My thinking here is that we care about is what is actually written to
> the device, not what we *intend* to write to the device.
> 
> There's a lot of complicated aspm.c code between setting
> link->clkpm_default/aspm_default and actually programming the device,
> and when debugging a problem, I don't want to have to parse all that
> code to derive the register values.

Sure, but that is not strictly related to this series I believe. I will add a
patch for that at the start of this series so that you can merge it
independently.

- Mani

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

Re: [PATCH 1/2] PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms

[Manivannan Sadhasivam]

On Wed, Sep 17, 2025 at 06:33:53PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Sep 17, 2025 at 06:22:18AM GMT, Bjorn Helgaas wrote:
> > [+cc Kai-Heng, Rafael, Heiner, AceLan; response to
> > https://lore.kernel.org/r/20250916-pci-dt-aspm-v1-1-778fe907c9ad@oss.qualcomm.com]
> > 
> > On Wed, Sep 17, 2025 at 04:14:42PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Sep 16, 2025 at 12:15:46PM GMT, Bjorn Helgaas wrote:
> > > > On Tue, Sep 16, 2025 at 09:42:52PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > > So far, the PCI subsystem has honored the ASPM and Clock PM states set by
> > > > > the BIOS (through LNKCTL) during device initialization. This was done
> > > > > conservatively to avoid issues with the buggy devices that advertise
> > > > > ASPM capabilities, but behave erratically if the ASPM states are enabled.
> > > > > So the PCI subsystem ended up trusting the BIOS to enable only the ASPM
> > > > > states that were known to work for the devices.
> > > ...
> > 
> > > > For debuggability, I wonder if we should have a pci_dbg() at the point
> > > > where we actually update PCI_EXP_LNKCTL, PCI_L1SS_CTL1, etc?  I could
> > > > even argue for pci_info() since this should be a low-frequency and
> > > > relatively high-risk event.
> > > 
> > > I don't know why we should print register settings since we are explicitly
> > > printing out what states are getting enabled.
> > 
> > My thinking here is that we care about is what is actually written to
> > the device, not what we *intend* to write to the device.
> > 
> > There's a lot of complicated aspm.c code between setting
> > link->clkpm_default/aspm_default and actually programming the device,
> > and when debugging a problem, I don't want to have to parse all that
> > code to derive the register values.
> 
> Sure, but that is not strictly related to this series I believe. I will add a
> patch for that at the start of this series so that you can merge it
> independently.
> 

I'm going to send this patch separately from this series.

- Mani

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

[PATCH 2/2] PCI: qcom: Remove the custom ASPM enablement code

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Since the PCI subsystem has started enabling all ASPM states for all
devicetree based platforms, the ASPM enablement code from this driver can
now be dropped.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..a1c4a9c31f9241e9ca679533323e33c0b972e678 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -247,7 +247,6 @@ 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);
@@ -1040,25 +1039,6 @@ 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;
@@ -1358,19 +1338,9 @@ 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 */
@@ -1432,7 +1402,6 @@ 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,
@@ -1443,7 +1412,6 @@ 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.45.2

Re: [PATCH 0/2] PCI/ASPM: Enable ASPM and Clock PM by default on devicetree platforms

[Bjorn Helgaas]

[+cc Kai-Heng, Rafael; thread at
https://lore.kernel.org/r/20250916-pci-dt-aspm-v1-0-778fe907c9ad@oss.qualcomm.com]

On Tue, Sep 16, 2025 at 09:42:51PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> Hi,
> 
> This series is one of the 'let's bite the bullet' kind, where we have decided to
> enable all ASPM and Clock PM states by default on devicetree platforms [1]. The
> reason why devicetree platforms were chosen because, it will be of minimal
> impact compared to the ACPI platforms. So seemed ideal to test the waters.
> 
> Problem Statement
> =================
> 
> Historically, PCI subsystem relied on the BIOS to enable ASPM and Clock PM
> states for PCI devices before the kernel boot. This was done to avoid enabling
> ASPM for the buggy devices that are known to create issues with ASPM (even
> though they advertise the ASPM capability). But BIOS is not at all a thing on
> most of the non-x86 platforms. For instance, the majority of the Embedded and
> Compute ARM based platforms using devicetree have something called bootloader,
> which is not anyway near the standard BIOS used in x86 based platforms. And
> these bootloaders wouldn't touch PCIe at all, unless they boot using PCIe
> storage, even then there would be no guarantee that the ASPM states will get
> enabled. Another example is the Intel's VMD domain that is not at all configured
> by the BIOS. But, this series is not enabling ASPM/Clock PM for VMD domain. I
> hope it will be done similarly in the future patches.
> 
> Solution
> ========
> 
> So to avoid relying on BIOS, it was agreed [2] that the PCI subsystem has to
> enable ASPM and Clock PM states based on the device capability. If any devices
> misbehave, then they should be quirked accordingly.
> 
> First patch of this series introduces two helper functions to enable all ASPM
> and Clock PM states if CONFIG_OF is enabled. Second patch drops the custom ASPM
> enablement code from the pcie-qcom driver as it is no longer needed.
> 
> Testing
> =======
> 
> This series is tested on Lenovo Thinkpad T14s based on Snapdragon X1 SoC. All
> supported ASPM states are getting enabled for both the NVMe and WLAN devices by
> default.
> 
> [1] https://lore.kernel.org/linux-pci/a47sg5ahflhvzyzqnfxvpk3dw4clkhqlhznjxzwqpf4nyjx5dk@bcghz5o6zolk
> [2] https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> Manivannan Sadhasivam (2):
>       PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms
>       PCI: qcom: Remove the custom ASPM enablement code
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 32 -----------------------
>  drivers/pci/pcie/aspm.c                | 48 ++++++++++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 37 deletions(-)
> ---
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> change-id: 20250916-pci-dt-aspm-8b3a7e8d2cf1
> 
> Best regards,
> -- 
> Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> 

Re: [PATCH 0/2] PCI/ASPM: Enable ASPM and Clock PM by default on devicetree platforms

[Bjorn Helgaas]

[+cc Heiner, AceLan in case it's of interest to you]

On Tue, Sep 16, 2025 at 12:21:18PM -0500, Bjorn Helgaas wrote:
> [+cc Kai-Heng, Rafael; thread at
> https://lore.kernel.org/r/20250916-pci-dt-aspm-v1-0-778fe907c9ad@oss.qualcomm.com]
> 
> On Tue, Sep 16, 2025 at 09:42:51PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > Hi,
> > 
> > This series is one of the 'let's bite the bullet' kind, where we have decided to
> > enable all ASPM and Clock PM states by default on devicetree platforms [1]. The
> > reason why devicetree platforms were chosen because, it will be of minimal
> > impact compared to the ACPI platforms. So seemed ideal to test the waters.
> > 
> > Problem Statement
> > =================
> > 
> > Historically, PCI subsystem relied on the BIOS to enable ASPM and Clock PM
> > states for PCI devices before the kernel boot. This was done to avoid enabling
> > ASPM for the buggy devices that are known to create issues with ASPM (even
> > though they advertise the ASPM capability). But BIOS is not at all a thing on
> > most of the non-x86 platforms. For instance, the majority of the Embedded and
> > Compute ARM based platforms using devicetree have something called bootloader,
> > which is not anyway near the standard BIOS used in x86 based platforms. And
> > these bootloaders wouldn't touch PCIe at all, unless they boot using PCIe
> > storage, even then there would be no guarantee that the ASPM states will get
> > enabled. Another example is the Intel's VMD domain that is not at all configured
> > by the BIOS. But, this series is not enabling ASPM/Clock PM for VMD domain. I
> > hope it will be done similarly in the future patches.
> > 
> > Solution
> > ========
> > 
> > So to avoid relying on BIOS, it was agreed [2] that the PCI subsystem has to
> > enable ASPM and Clock PM states based on the device capability. If any devices
> > misbehave, then they should be quirked accordingly.
> > 
> > First patch of this series introduces two helper functions to enable all ASPM
> > and Clock PM states if CONFIG_OF is enabled. Second patch drops the custom ASPM
> > enablement code from the pcie-qcom driver as it is no longer needed.
> > 
> > Testing
> > =======
> > 
> > This series is tested on Lenovo Thinkpad T14s based on Snapdragon X1 SoC. All
> > supported ASPM states are getting enabled for both the NVMe and WLAN devices by
> > default.
> > 
> > [1] https://lore.kernel.org/linux-pci/a47sg5ahflhvzyzqnfxvpk3dw4clkhqlhznjxzwqpf4nyjx5dk@bcghz5o6zolk
> > [2] https://lore.kernel.org/linux-pci/20250828204345.GA958461@bhelgaas
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> > Manivannan Sadhasivam (2):
> >       PCI/ASPM: Override the ASPM and Clock PM states set by BIOS for devicetree platforms
> >       PCI: qcom: Remove the custom ASPM enablement code
> > 
> >  drivers/pci/controller/dwc/pcie-qcom.c | 32 -----------------------
> >  drivers/pci/pcie/aspm.c                | 48 ++++++++++++++++++++++++++++++----
> >  2 files changed, 43 insertions(+), 37 deletions(-)
> > ---
> > base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> > change-id: 20250916-pci-dt-aspm-8b3a7e8d2cf1
> > 
> > Best regards,
> > -- 
> > Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> >