On Mon, Aug 25, 2025 at 09:56:41AM GMT, Krishna Chaitanya Chundru wrote:
>
>
> On 8/23/2025 11:14 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx # API dependency
> > Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> > Reported-by: Johan Hovold <johan@xxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx/
> >
> > * 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..71f14bc3a06ade1da1374e88b0ebef8c4e266064 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,
> > };
> > @@ -1979,6 +1947,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > if (pcie->mhi)
> > qcom_pcie_init_debugfs(pcie);
> > + pci_host_set_default_pcie_link_state(pp->bridge, PCIE_LINK_STATE_ALL);
> > +
> It is better to call this before starting link training,
Not after link training, but before enumeration... But you are right to spot the
issue.
> if the endpoint
> is already powered on by the time execution comes here link enumeration
> and ASPM init might be already done. This might not have any impact.
>
I've sent v3 after moving this call to qcom_pcie_host_init() which gets called
before attempting the enumeration.
- Mani
--
மணிவண்ணன் சதாசிவம்
