[PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[Manivannan Sadhasivam via B4 Relay]

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

Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
enumerated at the time of the controller driver probe. It proved to be
useful for devices already powered on by the bootloader, as it allowed
devices to enter ASPM without user intervention.

However, it could not enable ASPM for the hotplug capable devices i.e.,
devices enumerated *after* the controller driver probe. This limitation
mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
and also the bootloader has been enabling the PCI devices before Linux
Kernel boots (mostly on the Qcom compute platforms which users use on a
daily basis).

But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
started to block the PCI device enumeration until it had been probed.
Though, the intention of the commit was to avoid race between the pwrctrl
driver and PCI client driver, it also meant that the pwrctrl controlled PCI
devices may get probed after the controller driver and will no longer have
ASPM enabled. So users started noticing high runtime power consumption with
WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
T14s, etc...

Obviously, it is the pwrctrl change that caused regression, but it
ultimately uncovered a flaw in the ASPM enablement logic of the controller
driver. So to address the actual issue, use the newly introduced
pci_host_set_default_pcie_link_state() API, which allows the controller
drivers to set the default ASPM state for *all* devices. This default state
will be honored by the ASPM driver while enabling ASPM for all the devices.

So with this change, we can get rid of the controller driver specific
'qcom_pcie_ops::host_post_init' callback.

Also with this change, ASPM is now enabled by default on all Qcom
platforms as I haven't heard of any reported issues (apart from the
unsupported L0s on some platorms, which is still handled separately).

Cc: stable+noautosel@kernel.org # API dependency
Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
Reported-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Changes in v3:

- Moved the ASPM change to qcom_pcie_host_init()
- Link to v2: https://lore.kernel.org/r/20250823-qcom_aspm_fix-v2-1-7cef4066663c@oss.qualcomm.com

Changes in v2:

* Used the new API introduced in this patch instead of bus notifier:
https://lore.kernel.org/linux-pci/20250822031159.4005529-1-david.e.box@linux.intel.com/

* Enabled ASPM on all platforms as there is no specific reason to limit it to a
few.
---
 drivers/pci/controller/dwc/pcie-qcom.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..0c6741b9a9585bd5566b23ba68ef12f1febab56b 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;
@@ -1336,6 +1316,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_assert_reset;
 	}
 
+	pci_host_set_default_pcie_link_state(pp->bridge, PCIE_LINK_STATE_ALL);
+
 	return 0;
 
 err_assert_reset:
@@ -1358,19 +1340,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 +1404,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 +1414,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,
 };

---
base-commit: 681accdad91923ef4938b96ff3eea62e29af74c3
change-id: 20250823-qcom_aspm_fix-3264926b5b14

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

Re: [PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[Krishna Chaitanya Chundru]

On 8/25/2025 1:52 PM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> enumerated at the time of the controller driver probe. It proved to be
> useful for devices already powered on by the bootloader, as it allowed
> devices to enter ASPM without user intervention.
> 
> However, it could not enable ASPM for the hotplug capable devices i.e.,
> devices enumerated *after* the controller driver probe. This limitation
> mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> and also the bootloader has been enabling the PCI devices before Linux
> Kernel boots (mostly on the Qcom compute platforms which users use on a
> daily basis).
> 
> But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> started to block the PCI device enumeration until it had been probed.
> Though, the intention of the commit was to avoid race between the pwrctrl
> driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> devices may get probed after the controller driver and will no longer have
> ASPM enabled. So users started noticing high runtime power consumption with
> WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> T14s, etc...
> 
> Obviously, it is the pwrctrl change that caused regression, but it
> ultimately uncovered a flaw in the ASPM enablement logic of the controller
> driver. So to address the actual issue, use the newly introduced
> pci_host_set_default_pcie_link_state() API, which allows the controller
> drivers to set the default ASPM state for *all* devices. This default state
> will be honored by the ASPM driver while enabling ASPM for all the devices.
> 
> So with this change, we can get rid of the controller driver specific
> 'qcom_pcie_ops::host_post_init' callback.
> 
> Also with this change, ASPM is now enabled by default on all Qcom
> platforms as I haven't heard of any reported issues (apart from the
> unsupported L0s on some platorms, which is still handled separately).
> 
> Cc: stable+noautosel@kernel.org # API dependency
> Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> Reported-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
Reviewed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

- Krishna Chaitanya.
> ---
> Changes in v3:
> 
> - Moved the ASPM change to qcom_pcie_host_init()
> - Link to v2: https://lore.kernel.org/r/20250823-qcom_aspm_fix-v2-1-7cef4066663c@oss.qualcomm.com
> 
> Changes in v2:
> 
> * Used the new API introduced in this patch instead of bus notifier:
> https://lore.kernel.org/linux-pci/20250822031159.4005529-1-david.e.box@linux.intel.com/
> 
> * Enabled ASPM on all platforms as there is no specific reason to limit it to a
> few.
> ---
>   drivers/pci/controller/dwc/pcie-qcom.c | 34 ++--------------------------------
>   1 file changed, 2 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..0c6741b9a9585bd5566b23ba68ef12f1febab56b 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;
> @@ -1336,6 +1316,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>   			goto err_assert_reset;
>   	}
>   
> +	pci_host_set_default_pcie_link_state(pp->bridge, PCIE_LINK_STATE_ALL);
> +
>   	return 0;
>   
>   err_assert_reset:
> @@ -1358,19 +1340,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 +1404,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 +1414,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,
>   };
> 
> ---
> base-commit: 681accdad91923ef4938b96ff3eea62e29af74c3
> change-id: 20250823-qcom_aspm_fix-3264926b5b14
> 
> Best regards,

Re: [PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[Bjorn Helgaas]

On Mon, Aug 25, 2025 at 01:52:57PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> enumerated at the time of the controller driver probe. It proved to be
> useful for devices already powered on by the bootloader, as it allowed
> devices to enter ASPM without user intervention.
> 
> However, it could not enable ASPM for the hotplug capable devices i.e.,
> devices enumerated *after* the controller driver probe. This limitation
> mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> and also the bootloader has been enabling the PCI devices before Linux
> Kernel boots (mostly on the Qcom compute platforms which users use on a
> daily basis).
> 
> But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> started to block the PCI device enumeration until it had been probed.
> Though, the intention of the commit was to avoid race between the pwrctrl
> driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> devices may get probed after the controller driver and will no longer have
> ASPM enabled. So users started noticing high runtime power consumption with
> WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> T14s, etc...
> 
> Obviously, it is the pwrctrl change that caused regression, but it
> ultimately uncovered a flaw in the ASPM enablement logic of the controller
> driver. So to address the actual issue, use the newly introduced
> pci_host_set_default_pcie_link_state() API, which allows the controller
> drivers to set the default ASPM state for *all* devices. This default state
> will be honored by the ASPM driver while enabling ASPM for all the devices.

So I guess this fixes a power consumption regression that became
visible with b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are
probed before PCI client drivers").  Arguably we should have a Fixes:
tag for that, and maybe even skip the tag for 9f4f3dfad8cf, since if
you have 9f4f3dfad8cf but not b458ff7e8176, it doesn't sound like you
would need to backport this change?

I *love* getting rid of the bus walk and solving the hotplug issue.

> So with this change, we can get rid of the controller driver specific
> 'qcom_pcie_ops::host_post_init' callback.
> 
> Also with this change, ASPM is now enabled by default on all Qcom
> platforms as I haven't heard of any reported issues (apart from the
> unsupported L0s on some platorms, which is still handled separately).

If ASPM hasn't been enabled by default, how would you hear about
issues?  Is ASPM commonly enabled in some other way?

If you think the risk of ASPM issues is nil, I guess it's OK to do at
the same time.  From a maintenance perspective it might be nice to
separate that change so if there happened to be a regression, we could
identify and revert that part by itself if necessary.  It looks like
previously, ASPM was only enabled for one part:

  ops_2_7_0   # cfg_2_7_0 qcom,pcie-sdm845

But after this patch, ASPM will be enabled for many more parts:

  ops_2_1_0   # cfg_2_1_0 qcom,pcie-apq8064 qcom,pcie-ipq8064 qcom,pcie-ipq8064-v2
  ops_1_0_0   # cfg_1_0_0 qcom,pcie-apq8084
  ops_2_3_2   # cfg_2_3_2 qcom,pcie-msm8996
  ops_2_4_0   # cfg_2_4_0 qcom,pcie-ipq4019 qcom,pcie-qcs404
  ops_2_3_3   # cfg_2_3_3 qcom,pcie-ipq8074
  ops_1_9_0   # cfg_1_9_0 qcom,pcie-sc7280 qcom,pcie-sc8180x qcom,pcie-sdx55 qcom,pcie-sm8150 qcom,pcie-sm8250 qcom,pcie-sm8350 qcom,pcie-sm8450-pcie0 qcom,pcie-sm8450-pcie1 qcom,pcie-sm8550
	      # cfg_1_34_0 qcom,pcie-sa8775p
  ops_1_21_0  # cfg_sc8280xp qcom,pcie-sa8540p qcom,pcie-sc8280xp qcom,pcie-x1e80100
  ops_2_9_0   # cfg_2_9_0 qcom,pcie-ipq5018 qcom,pcie-ipq6018 qcom,pcie-ipq8074-gen3 qcom,pcie-ipq9574

> Cc: stable+noautosel@kernel.org # API dependency
> Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> Reported-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> Changes in v3:
> 
> - Moved the ASPM change to qcom_pcie_host_init()
> - Link to v2: https://lore.kernel.org/r/20250823-qcom_aspm_fix-v2-1-7cef4066663c@oss.qualcomm.com
> 
> Changes in v2:
> 
> * Used the new API introduced in this patch instead of bus notifier:
> https://lore.kernel.org/linux-pci/20250822031159.4005529-1-david.e.box@linux.intel.com/
> 
> * Enabled ASPM on all platforms as there is no specific reason to limit it to a
> few.
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 34 ++--------------------------------
>  1 file changed, 2 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..0c6741b9a9585bd5566b23ba68ef12f1febab56b 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;
> @@ -1336,6 +1316,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  			goto err_assert_reset;
>  	}
>  
> +	pci_host_set_default_pcie_link_state(pp->bridge, PCIE_LINK_STATE_ALL);
> +
>  	return 0;
>  
>  err_assert_reset:
> @@ -1358,19 +1340,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 +1404,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 +1414,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,
>  };
> 
> ---
> base-commit: 681accdad91923ef4938b96ff3eea62e29af74c3
> change-id: 20250823-qcom_aspm_fix-3264926b5b14
> 
> Best regards,
> -- 
> Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> 

Re: [PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[Manivannan Sadhasivam]

On Wed, Aug 27, 2025 at 10:47:39AM GMT, Bjorn Helgaas wrote:
> On Mon, Aug 25, 2025 at 01:52:57PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> > ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> > enumerated at the time of the controller driver probe. It proved to be
> > useful for devices already powered on by the bootloader, as it allowed
> > devices to enter ASPM without user intervention.
> > 
> > However, it could not enable ASPM for the hotplug capable devices i.e.,
> > devices enumerated *after* the controller driver probe. This limitation
> > mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> > and also the bootloader has been enabling the PCI devices before Linux
> > Kernel boots (mostly on the Qcom compute platforms which users use on a
> > daily basis).
> > 
> > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> > pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> > started to block the PCI device enumeration until it had been probed.
> > Though, the intention of the commit was to avoid race between the pwrctrl
> > driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> > devices may get probed after the controller driver and will no longer have
> > ASPM enabled. So users started noticing high runtime power consumption with
> > WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> > T14s, etc...
> > 
> > Obviously, it is the pwrctrl change that caused regression, but it
> > ultimately uncovered a flaw in the ASPM enablement logic of the controller
> > driver. So to address the actual issue, use the newly introduced
> > pci_host_set_default_pcie_link_state() API, which allows the controller
> > drivers to set the default ASPM state for *all* devices. This default state
> > will be honored by the ASPM driver while enabling ASPM for all the devices.
> 
> So I guess this fixes a power consumption regression that became
> visible with b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are
> probed before PCI client drivers").  Arguably we should have a Fixes:
> tag for that, and maybe even skip the tag for 9f4f3dfad8cf, since if
> you have 9f4f3dfad8cf but not b458ff7e8176, it doesn't sound like you
> would need to backport this change?
> 

9f4f3dfad8cf is the culprit which added a half baked solution for enabling ASPM
and b458ff7e8176 made the issue more obvious since instead of requiring people
to connect a device after boot, it allowed the device to enumerate after the
probe of the controller driver, thereby making it more visible.

It would make sense to add a Fixes tag for b458ff7e8176 too, but 9f4f3dfad8cf
should also be present IMO.

For backport, I do not want the AI tools to do the job since they will fail
anyway because of the API dependency. I may send a separate backport patch later
once this patch gets merged into mainline.

> I *love* getting rid of the bus walk and solving the hotplug issue.
> 
> > So with this change, we can get rid of the controller driver specific
> > 'qcom_pcie_ops::host_post_init' callback.
> > 
> > Also with this change, ASPM is now enabled by default on all Qcom
> > platforms as I haven't heard of any reported issues (apart from the
> > unsupported L0s on some platorms, which is still handled separately).
> 
> If ASPM hasn't been enabled by default, how would you hear about
> issues?  Is ASPM commonly enabled in some other way?
> 

Mostly from the downstream drivers. They do enable ASPM by default and I haven't
heard of any issues with that. So I assumed that would mean it will be safe for
us to enable ASPM for all platforms in upstream as well.

> If you think the risk of ASPM issues is nil, I guess it's OK to do at
> the same time.  From a maintenance perspective it might be nice to
> separate that change so if there happened to be a regression, we could
> identify and revert that part by itself if necessary.  It looks like
> previously, ASPM was only enabled for one part:
> 
>   ops_2_7_0   # cfg_2_7_0 qcom,pcie-sdm845
> 

No. Previously ASPM was enabled for ops_1_9_0 and ops_1_21_0 based platforms.

> But after this patch, ASPM will be enabled for many more parts:
> 
>   ops_2_1_0   # cfg_2_1_0 qcom,pcie-apq8064 qcom,pcie-ipq8064 qcom,pcie-ipq8064-v2
>   ops_1_0_0   # cfg_1_0_0 qcom,pcie-apq8084
>   ops_2_3_2   # cfg_2_3_2 qcom,pcie-msm8996
>   ops_2_4_0   # cfg_2_4_0 qcom,pcie-ipq4019 qcom,pcie-qcs404
>   ops_2_3_3   # cfg_2_3_3 qcom,pcie-ipq8074
>   ops_1_9_0   # cfg_1_9_0 qcom,pcie-sc7280 qcom,pcie-sc8180x qcom,pcie-sdx55 qcom,pcie-sm8150 qcom,pcie-sm8250 qcom,pcie-sm8350 qcom,pcie-sm8450-pcie0 qcom,pcie-sm8450-pcie1 qcom,pcie-sm8550
> 	      # cfg_1_34_0 qcom,pcie-sa8775p
>   ops_1_21_0  # cfg_sc8280xp qcom,pcie-sa8540p qcom,pcie-sc8280xp qcom,pcie-x1e80100
>   ops_2_9_0   # cfg_2_9_0 qcom,pcie-ipq5018 qcom,pcie-ipq6018 qcom,pcie-ipq8074-gen3 qcom,pcie-ipq9574
> 

If you insist, I can do that by calling pci_host_set_default_pcie_link_state()
from qcom_pcie_init_2_7_0() and later move it to qcom_pcie_host_init() in a
separate patch.

- Mani

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

Re: [PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[Bjorn Helgaas]

On Wed, Aug 27, 2025 at 10:43:26PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Aug 27, 2025 at 10:47:39AM GMT, Bjorn Helgaas wrote:
> > On Mon, Aug 25, 2025 at 01:52:57PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms
> > > supporting 1.9.0 ops") allowed the Qcom controller driver to
> > > enable ASPM for all PCI devices enumerated at the time of the
> > > controller driver probe. It proved to be useful for devices
> > > already powered on by the bootloader, as it allowed devices to
> > > enter ASPM without user intervention.
> > > 
> > > However, it could not enable ASPM for the hotplug capable
> > > devices i.e., devices enumerated *after* the controller driver
> > > probe. This limitation mostly went unnoticed as the Qcom PCI
> > > controllers are not hotplug capable and also the bootloader has
> > > been enabling the PCI devices before Linux Kernel boots (mostly
> > > on the Qcom compute platforms which users use on a daily basis).
> > > 
> > > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl:
> > > Ensure that pwrctl drivers are probed before PCI client
> > > drivers"), the pwrctrl driver started to block the PCI device
> > > enumeration until it had been probed.  Though, the intention of
> > > the commit was to avoid race between the pwrctrl driver and PCI
> > > client driver, it also meant that the pwrctrl controlled PCI
> > > devices may get probed after the controller driver and will no
> > > longer have ASPM enabled. So users started noticing high runtime
> > > power consumption with WLAN chipsets on Qcom compute platforms
> > > like Thinkpad X13s, and Thinkpad T14s, etc...
> > > 
> > > Obviously, it is the pwrctrl change that caused regression, but
> > > it ultimately uncovered a flaw in the ASPM enablement logic of
> > > the controller driver. So to address the actual issue, use the
> > > newly introduced pci_host_set_default_pcie_link_state() API,
> > > which allows the controller drivers to set the default ASPM
> > > state for *all* devices. This default state will be honored by
> > > the ASPM driver while enabling ASPM for all the devices.
> > 
> > So I guess this fixes a power consumption regression that became
> > visible with b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers
> > are probed before PCI client drivers").  Arguably we should have a
> > Fixes: tag for that, and maybe even skip the tag for 9f4f3dfad8cf,
> > since if you have 9f4f3dfad8cf but not b458ff7e8176, it doesn't
> > sound like you would need to backport this change?
> 
> 9f4f3dfad8cf is the culprit which added a half baked solution for
> enabling ASPM and b458ff7e8176 made the issue more obvious since
> instead of requiring people to connect a device after boot, it
> allowed the device to enumerate after the probe of the controller
> driver, thereby making it more visible.
> 
> It would make sense to add a Fixes tag for b458ff7e8176 too, but
> 9f4f3dfad8cf should also be present IMO.

OK.  IIUC we would only land on 9f4f3dfad8cf for hot-plugged devices,
and you said these controllers don't support hotplug.  Backporting
something to fix a half-baked solution that doesn't cause an actual
problem is of marginal value, but we can keep the Fixes: 9f4f3dfad8cf.

> > > Also with this change, ASPM is now enabled by default on all
> > > Qcom platforms as I haven't heard of any reported issues (apart
> > > from the unsupported L0s on some platorms, which is still
> > > handled separately).
> > 
> > If ASPM hasn't been enabled by default, how would you hear about
> > issues?  Is ASPM commonly enabled in some other way?
> 
> Mostly from the downstream drivers. They do enable ASPM by default
> and I haven't heard of any issues with that. So I assumed that would
> mean it will be safe for us to enable ASPM for all platforms in
> upstream as well.
> 
> > If you think the risk of ASPM issues is nil, I guess it's OK to do
> > at the same time.  From a maintenance perspective it might be nice
> > to separate that change so if there happened to be a regression,
> > we could identify and revert that part by itself if necessary.  It
> > looks like previously, ASPM was only enabled for one part:
> > 
> >   ops_2_7_0   # cfg_2_7_0 qcom,pcie-sdm845
> 
> No. Previously ASPM was enabled for ops_1_9_0 and ops_1_21_0 based
> platforms.

Oops, my mistake.  Traversing all the ops_ and cfg_ structs was a
little confusing.  For posterity, I guess the corrected matrix is that
ASPM was previously enabled for these:

  ops_1_9_0   # cfg_1_9_0 qcom,pcie-sc7280 qcom,pcie-sc8180x qcom,pcie-sdx55 qcom,pcie-sm8150 qcom,pcie-sm8250 qcom,pcie-sm8350 qcom,pcie-sm8450-pcie0 qcom,pcie-sm8450-pcie1 qcom,pcie-sm8550
	      # cfg_1_34_0 qcom,pcie-sa8775p
  ops_1_21_0  # cfg_sc8280xp qcom,pcie-sa8540p qcom,pcie-sc8280xp qcom,pcie-x1e80100

And will now be enabled for these:

  ops_2_1_0   # cfg_2_1_0 qcom,pcie-apq8064 qcom,pcie-ipq8064 qcom,pcie-ipq8064-v2
  ops_1_0_0   # cfg_1_0_0 qcom,pcie-apq8084
  ops_2_3_2   # cfg_2_3_2 qcom,pcie-msm8996
  ops_2_4_0   # cfg_2_4_0 qcom,pcie-ipq4019 qcom,pcie-qcs404
  ops_2_3_3   # cfg_2_3_3 qcom,pcie-ipq8074
  ops_2_7_0   # cfg_2_7_0 qcom,pcie-sdm845
  ops_2_9_0   # cfg_2_9_0 qcom,pcie-ipq5018 qcom,pcie-ipq6018 qcom,pcie-ipq8074-gen3 qcom,pcie-ipq9574

> If you insist, I can do that by calling
> pci_host_set_default_pcie_link_state() from qcom_pcie_init_2_7_0()
> and later move it to qcom_pcie_host_init() in a separate patch.

I don't insist; that's why I said "if you think the risk of issues
is nil," since I didn't know that you had any experience with ASPM
being enabled on the others.  But it sounds like you do, so I'm ok
with this as-is.

Bjorn

Re: [PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state() API to enable ASPM for all platforms

[Manivannan Sadhasivam]

On Wed, Aug 27, 2025 at 01:22:58PM GMT, Bjorn Helgaas wrote:
> On Wed, Aug 27, 2025 at 10:43:26PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Aug 27, 2025 at 10:47:39AM GMT, Bjorn Helgaas wrote:
> > > On Mon, Aug 25, 2025 at 01:52:57PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > 
> > > > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms
> > > > supporting 1.9.0 ops") allowed the Qcom controller driver to
> > > > enable ASPM for all PCI devices enumerated at the time of the
> > > > controller driver probe. It proved to be useful for devices
> > > > already powered on by the bootloader, as it allowed devices to
> > > > enter ASPM without user intervention.
> > > > 
> > > > However, it could not enable ASPM for the hotplug capable
> > > > devices i.e., devices enumerated *after* the controller driver
> > > > probe. This limitation mostly went unnoticed as the Qcom PCI
> > > > controllers are not hotplug capable and also the bootloader has
> > > > been enabling the PCI devices before Linux Kernel boots (mostly
> > > > on the Qcom compute platforms which users use on a daily basis).
> > > > 
> > > > But with the advent of the commit b458ff7e8176 ("PCI/pwrctl:
> > > > Ensure that pwrctl drivers are probed before PCI client
> > > > drivers"), the pwrctrl driver started to block the PCI device
> > > > enumeration until it had been probed.  Though, the intention of
> > > > the commit was to avoid race between the pwrctrl driver and PCI
> > > > client driver, it also meant that the pwrctrl controlled PCI
> > > > devices may get probed after the controller driver and will no
> > > > longer have ASPM enabled. So users started noticing high runtime
> > > > power consumption with WLAN chipsets on Qcom compute platforms
> > > > like Thinkpad X13s, and Thinkpad T14s, etc...
> > > > 
> > > > Obviously, it is the pwrctrl change that caused regression, but
> > > > it ultimately uncovered a flaw in the ASPM enablement logic of
> > > > the controller driver. So to address the actual issue, use the
> > > > newly introduced pci_host_set_default_pcie_link_state() API,
> > > > which allows the controller drivers to set the default ASPM
> > > > state for *all* devices. This default state will be honored by
> > > > the ASPM driver while enabling ASPM for all the devices.
> > > 
> > > So I guess this fixes a power consumption regression that became
> > > visible with b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers
> > > are probed before PCI client drivers").  Arguably we should have a
> > > Fixes: tag for that, and maybe even skip the tag for 9f4f3dfad8cf,
> > > since if you have 9f4f3dfad8cf but not b458ff7e8176, it doesn't
> > > sound like you would need to backport this change?
> > 
> > 9f4f3dfad8cf is the culprit which added a half baked solution for
> > enabling ASPM and b458ff7e8176 made the issue more obvious since
> > instead of requiring people to connect a device after boot, it
> > allowed the device to enumerate after the probe of the controller
> > driver, thereby making it more visible.
> > 
> > It would make sense to add a Fixes tag for b458ff7e8176 too, but
> > 9f4f3dfad8cf should also be present IMO.
> 
> OK.  IIUC we would only land on 9f4f3dfad8cf for hot-plugged devices,
> and you said these controllers don't support hotplug.  Backporting
> something to fix a half-baked solution that doesn't cause an actual
> problem is of marginal value, but we can keep the Fixes: 9f4f3dfad8cf.
> 

Just to add more: It is true that our controllers doesn't support hotplug, but
with the addition of pwrctrl, the enumeration of the devices could be deferred
for a while (until the pwrctrl driver gets probed and does its job even if the
device was powered on by the bootloader), then we will trip over this issue. So
we will need to backport it to 6.13 where commit b458ff7e8176 got introduced.
But I added the noautosel tag since the backport requires manual work.

> > > > Also with this change, ASPM is now enabled by default on all
> > > > Qcom platforms as I haven't heard of any reported issues (apart
> > > > from the unsupported L0s on some platorms, which is still
> > > > handled separately).
> > > 
> > > If ASPM hasn't been enabled by default, how would you hear about
> > > issues?  Is ASPM commonly enabled in some other way?
> > 
> > Mostly from the downstream drivers. They do enable ASPM by default
> > and I haven't heard of any issues with that. So I assumed that would
> > mean it will be safe for us to enable ASPM for all platforms in
> > upstream as well.
> > 
> > > If you think the risk of ASPM issues is nil, I guess it's OK to do
> > > at the same time.  From a maintenance perspective it might be nice
> > > to separate that change so if there happened to be a regression,
> > > we could identify and revert that part by itself if necessary.  It
> > > looks like previously, ASPM was only enabled for one part:
> > > 
> > >   ops_2_7_0   # cfg_2_7_0 qcom,pcie-sdm845
> > 
> > No. Previously ASPM was enabled for ops_1_9_0 and ops_1_21_0 based
> > platforms.
> 
> Oops, my mistake.  Traversing all the ops_ and cfg_ structs was a
> little confusing.  For posterity, I guess the corrected matrix is that
> ASPM was previously enabled for these:
> 
>   ops_1_9_0   # cfg_1_9_0 qcom,pcie-sc7280 qcom,pcie-sc8180x qcom,pcie-sdx55 qcom,pcie-sm8150 qcom,pcie-sm8250 qcom,pcie-sm8350 qcom,pcie-sm8450-pcie0 qcom,pcie-sm8450-pcie1 qcom,pcie-sm8550
> 	      # cfg_1_34_0 qcom,pcie-sa8775p
>   ops_1_21_0  # cfg_sc8280xp qcom,pcie-sa8540p qcom,pcie-sc8280xp qcom,pcie-x1e80100
> 
> And will now be enabled for these:
> 
>   ops_2_1_0   # cfg_2_1_0 qcom,pcie-apq8064 qcom,pcie-ipq8064 qcom,pcie-ipq8064-v2
>   ops_1_0_0   # cfg_1_0_0 qcom,pcie-apq8084
>   ops_2_3_2   # cfg_2_3_2 qcom,pcie-msm8996
>   ops_2_4_0   # cfg_2_4_0 qcom,pcie-ipq4019 qcom,pcie-qcs404
>   ops_2_3_3   # cfg_2_3_3 qcom,pcie-ipq8074
>   ops_2_7_0   # cfg_2_7_0 qcom,pcie-sdm845
>   ops_2_9_0   # cfg_2_9_0 qcom,pcie-ipq5018 qcom,pcie-ipq6018 qcom,pcie-ipq8074-gen3 qcom,pcie-ipq9574
> 
> > If you insist, I can do that by calling
> > pci_host_set_default_pcie_link_state() from qcom_pcie_init_2_7_0()
> > and later move it to qcom_pcie_host_init() in a separate patch.
> 
> I don't insist; that's why I said "if you think the risk of issues
> is nil," since I didn't know that you had any experience with ASPM
> being enabled on the others.  But it sounds like you do, so I'm ok
> with this as-is.
> 

Ok. In any case, we need to sort out the discussion happening with the series
from David [1] first. Currently, ASPM is broken on couple of Snapdragon laptops
since v6.13 and I'd love to fix it asap.

- Mani

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

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