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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>