[PATCH v2 0/2] PCI: qcom: Enable ASPM on host bridge and devices

[PATCH v2 0/2] PCI: qcom: Enable ASPM on host bridge and devices

[Manivannan Sadhasivam]

Hi,

This series enables ASPM by default on the host bridge and devices of selected
Qcom platforms.

The motivation behind enabling ASPM in the controller driver is provided in the
commit message of patch 2/2.

This series has been tested on SC8280-CRD and Lenovo Thinkpad X13s laptop
and it helped save ~0.6W of power during runtime.

- Mani

Changes in v2:

* Rebased on top of v6.6-rc1

Manivannan Sadhasivam (2):
  PCI: dwc: Add host_post_init() callback
  PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops

 .../pci/controller/dwc/pcie-designware-host.c |  3 ++
 drivers/pci/controller/dwc/pcie-designware.h  |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c        | 28 +++++++++++++++++++
 3 files changed, 32 insertions(+)

-- 
2.25.1

[PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

[Manivannan Sadhasivam]

This callback can be used by the platform drivers to do configuration once
all the devices are scanned. Like changing LNKCTL of all downstream devices
to enable ASPM etc...

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index a7170fd0e847..7991f0e179b2 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		goto err_stop_link;
 
+	if (pp->ops->host_post_init)
+		pp->ops->host_post_init(pp);
+
 	return 0;
 
 err_stop_link:
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ef0b2efa9f93..efb4d4754fc8 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -301,6 +301,7 @@ enum dw_pcie_ltssm {
 struct dw_pcie_host_ops {
 	int (*host_init)(struct dw_pcie_rp *pp);
 	void (*host_deinit)(struct dw_pcie_rp *pp);
+	void (*host_post_init)(struct dw_pcie_rp *pp);
 	int (*msi_host_init)(struct dw_pcie_rp *pp);
 	void (*pme_turn_off)(struct dw_pcie_rp *pp);
 };
-- 
2.25.1

[PATCH v2 2/2] PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops

[Manivannan Sadhasivam]

ASPM is supported by Qcom host controllers/bridges on most of the recent
platforms and so the devices tested so far. But for enabling ASPM by
default (without Kconfig/cmdline/sysfs), BIOS has to enable ASPM on both
host bridge and downstream devices during boot. Unfortunately, none of the
BIOS available on Qcom platforms enables ASPM. Due to this, the platforms
making use of Qcom SoCs draw high power during runtime.

To fix this power issue, users/distros have to enable ASPM using configs
such as (Kconfig/cmdline/sysfs) or the BIOS has to start enabling ASPM.
The latter may happen in the future, but that won't address the issue on
current platforms. Also, asking users/distros to enable a feature to get
the power management right would provide an unpleasant out-of-the-box
experience.

So the apt solution is to enable ASPM in the controller driver itself. And
this is being accomplished by calling pci_enable_link_state() in the newly
introduced host_post_init() callback for all the devices connected to the
bus. This function enables all supported link low power states for both
host bridge and the downstream devices.

Due to limited testing, ASPM is only enabled for platforms making use of
ops_1_9_0 callbacks.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 367acb419a2b..c324c3daaa5a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -222,6 +222,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);
@@ -967,6 +968,22 @@ 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(pdev, PCI_D0);
+	pci_enable_link_state(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;
@@ -1219,9 +1236,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 = {
 	.host_init	= qcom_pcie_host_init,
 	.host_deinit	= qcom_pcie_host_deinit,
+	.host_post_init	= qcom_pcie_host_post_init,
 };
 
 /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
@@ -1283,6 +1310,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,
-- 
2.25.1

Re: [PATCH v2 0/2] PCI: qcom: Enable ASPM on host bridge and devices

[Konrad Dybcio]

On 10/10/23 17:59, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series enables ASPM by default on the host bridge and devices of selected
> Qcom platforms.
> 
> The motivation behind enabling ASPM in the controller driver is provided in the
> commit message of patch 2/2.
> 
> This series has been tested on SC8280-CRD and Lenovo Thinkpad X13s laptop
> and it helped save ~0.6W of power during runtime.
That's a lot of power, thanks for looking into this!

Konrad

Re: [PATCH v2 2/2] PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops

[Bjorn Helgaas]

On Tue, Oct 10, 2023 at 09:29:14PM +0530, Manivannan Sadhasivam wrote:
> ASPM is supported by Qcom host controllers/bridges on most of the recent
> platforms and so the devices tested so far. But for enabling ASPM by
> default (without Kconfig/cmdline/sysfs), BIOS has to enable ASPM on both
> host bridge and downstream devices during boot. Unfortunately, none of the
> BIOS available on Qcom platforms enables ASPM.

I think this covers over a PCI core defect.  If the devices advertise
ASPM support, which both the qcom host controller and the endpoint
devices do, the PCI core should be able to enable it without being
prodded as this patch does.

We had a long conversation about this at [1], but never came to a good
resolution.  Since we don't know how to fix the PCI core issue, I
guess we have no choice but to do things like this patch, at least for
now.

If/when we ever *do* fix the PCI core issue, it would likely result in
enabling ASPM (if advertised by both ends of the link) for *all* qcom
controllers, not just the 1.9.0 ones.

And I think that even today, users can enable ASPM on non-1.9.0
controllers via sysfs.  So if you are concerned about ASPM not being
tested on those controllers, you may want to make them not advertise
ASPM support.

Even with this patch, I guess hot-added devices don't get ASPM
enabled?  That's basically what [1] is about.

Bjorn

[1] https://lore.kernel.org/linux-pci/20230615070421.1704133-1-kai.heng.feng@canonical.com/

> Due to this, the platforms
> making use of Qcom SoCs draw high power during runtime.
> 
> To fix this power issue, users/distros have to enable ASPM using configs
> such as (Kconfig/cmdline/sysfs) or the BIOS has to start enabling ASPM.
> The latter may happen in the future, but that won't address the issue on
> current platforms. Also, asking users/distros to enable a feature to get
> the power management right would provide an unpleasant out-of-the-box
> experience.
> 
> So the apt solution is to enable ASPM in the controller driver itself. And
> this is being accomplished by calling pci_enable_link_state() in the newly
> introduced host_post_init() callback for all the devices connected to the
> bus. This function enables all supported link low power states for both
> host bridge and the downstream devices.
> 
> Due to limited testing, ASPM is only enabled for platforms making use of
> ops_1_9_0 callbacks.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 28 ++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 367acb419a2b..c324c3daaa5a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -222,6 +222,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);
> @@ -967,6 +968,22 @@ 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(pdev, PCI_D0);
> +	pci_enable_link_state(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;
> @@ -1219,9 +1236,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 = {
>  	.host_init	= qcom_pcie_host_init,
>  	.host_deinit	= qcom_pcie_host_deinit,
> +	.host_post_init	= qcom_pcie_host_post_init,
>  };
>  
>  /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
> @@ -1283,6 +1310,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,
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/2] PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops

[Konrad Dybcio]

On 10/10/23 17:59, Manivannan Sadhasivam wrote:
> ASPM is supported by Qcom host controllers/bridges on most of the recent
> platforms and so the devices tested so far. But for enabling ASPM by
> default (without Kconfig/cmdline/sysfs), BIOS has to enable ASPM on both
> host bridge and downstream devices during boot. Unfortunately, none of the
> BIOS available on Qcom platforms enables ASPM. Due to this, the platforms
> making use of Qcom SoCs draw high power during runtime.
> 
> To fix this power issue, users/distros have to enable ASPM using configs
> such as (Kconfig/cmdline/sysfs) or the BIOS has to start enabling ASPM.
> The latter may happen in the future, but that won't address the issue on
> current platforms. Also, asking users/distros to enable a feature to get
> the power management right would provide an unpleasant out-of-the-box
> experience.
> 
> So the apt solution is to enable ASPM in the controller driver itself. And
> this is being accomplished by calling pci_enable_link_state() in the newly
> introduced host_post_init() callback for all the devices connected to the
> bus. This function enables all supported link low power states for both
> host bridge and the downstream devices.
> 
> Due to limited testing, ASPM is only enabled for platforms making use of
> ops_1_9_0 callbacks.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
[...]

> +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(pdev, PCI_D0);
> +	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
Do we not care about retval here?

> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
post_init_enable_aspm?

Konrad

Re: [PATCH v2 2/2] PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops

[Manivannan Sadhasivam]

On Tue, Oct 10, 2023 at 11:29:45AM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 10, 2023 at 09:29:14PM +0530, Manivannan Sadhasivam wrote:
> > ASPM is supported by Qcom host controllers/bridges on most of the recent
> > platforms and so the devices tested so far. But for enabling ASPM by
> > default (without Kconfig/cmdline/sysfs), BIOS has to enable ASPM on both
> > host bridge and downstream devices during boot. Unfortunately, none of the
> > BIOS available on Qcom platforms enables ASPM.
> 
> I think this covers over a PCI core defect.  If the devices advertise
> ASPM support, which both the qcom host controller and the endpoint
> devices do, the PCI core should be able to enable it without being
> prodded as this patch does.
> 

Right.

> We had a long conversation about this at [1], but never came to a good
> resolution.  Since we don't know how to fix the PCI core issue, I
> guess we have no choice but to do things like this patch, at least for
> now.
> 
> If/when we ever *do* fix the PCI core issue, it would likely result in
> enabling ASPM (if advertised by both ends of the link) for *all* qcom
> controllers, not just the 1.9.0 ones.
> 

That would be a welcome move IMO :)

> And I think that even today, users can enable ASPM on non-1.9.0
> controllers via sysfs.  So if you are concerned about ASPM not being
> tested on those controllers, you may want to make them not advertise
> ASPM support.
> 

That will completely take away the power saving if users want it at some time.
I'm planning to enable ASPM for other configs as well in the coming days once I
get hold of the test platforms. For platforms where ASPM is not really desired,
like IPQ SoCs used in routers, I will just disable it.

> Even with this patch, I guess hot-added devices don't get ASPM
> enabled?  That's basically what [1] is about.
> 

Yeah, that's a limitation like the BIOS enablement. Fortunately, most of the
platforms based on Qcom SoCs do not have hot pluggable PCIe slots like in PCs.
But we should fix it too.

- Mani

> Bjorn
> 
> [1] https://lore.kernel.org/linux-pci/20230615070421.1704133-1-kai.heng.feng@canonical.com/
> 
> > Due to this, the platforms
> > making use of Qcom SoCs draw high power during runtime.
> > 
> > To fix this power issue, users/distros have to enable ASPM using configs
> > such as (Kconfig/cmdline/sysfs) or the BIOS has to start enabling ASPM.
> > The latter may happen in the future, but that won't address the issue on
> > current platforms. Also, asking users/distros to enable a feature to get
> > the power management right would provide an unpleasant out-of-the-box
> > experience.
> > 
> > So the apt solution is to enable ASPM in the controller driver itself. And
> > this is being accomplished by calling pci_enable_link_state() in the newly
> > introduced host_post_init() callback for all the devices connected to the
> > bus. This function enables all supported link low power states for both
> > host bridge and the downstream devices.
> > 
> > Due to limited testing, ASPM is only enabled for platforms making use of
> > ops_1_9_0 callbacks.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 28 ++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 367acb419a2b..c324c3daaa5a 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -222,6 +222,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);
> > @@ -967,6 +968,22 @@ 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(pdev, PCI_D0);
> > +	pci_enable_link_state(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;
> > @@ -1219,9 +1236,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 = {
> >  	.host_init	= qcom_pcie_host_init,
> >  	.host_deinit	= qcom_pcie_host_deinit,
> > +	.host_post_init	= qcom_pcie_host_post_init,
> >  };
> >  
> >  /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
> > @@ -1283,6 +1310,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,
> > -- 
> > 2.25.1
> > 

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

Re: [PATCH v2 2/2] PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops

[Manivannan Sadhasivam]

On Tue, Oct 10, 2023 at 06:33:52PM +0200, Konrad Dybcio wrote:
> 
> 
> On 10/10/23 17:59, Manivannan Sadhasivam wrote:
> > ASPM is supported by Qcom host controllers/bridges on most of the recent
> > platforms and so the devices tested so far. But for enabling ASPM by
> > default (without Kconfig/cmdline/sysfs), BIOS has to enable ASPM on both
> > host bridge and downstream devices during boot. Unfortunately, none of the
> > BIOS available on Qcom platforms enables ASPM. Due to this, the platforms
> > making use of Qcom SoCs draw high power during runtime.
> > 
> > To fix this power issue, users/distros have to enable ASPM using configs
> > such as (Kconfig/cmdline/sysfs) or the BIOS has to start enabling ASPM.
> > The latter may happen in the future, but that won't address the issue on
> > current platforms. Also, asking users/distros to enable a feature to get
> > the power management right would provide an unpleasant out-of-the-box
> > experience.
> > 
> > So the apt solution is to enable ASPM in the controller driver itself. And
> > this is being accomplished by calling pci_enable_link_state() in the newly
> > introduced host_post_init() callback for all the devices connected to the
> > bus. This function enables all supported link low power states for both
> > host bridge and the downstream devices.
> > 
> > Due to limited testing, ASPM is only enabled for platforms making use of
> > ops_1_9_0 callbacks.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> [...]
> 
> > +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(pdev, PCI_D0);
> > +	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> Do we not care about retval here?
> 

No. Even if it fails, we shouldn't care about it.

> > +
> > +	return 0;
> > +}
> > +
> > +static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
> post_init_enable_aspm?
> 

The scope of this callback may get extended in the future. So I'd keep it as it
is.

- Mani

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

Re: [PATCH v2 0/2] PCI: qcom: Enable ASPM on host bridge and devices

[Krzysztof Wilczyński]

Hello,

> This series enables ASPM by default on the host bridge and devices of selected
> Qcom platforms.
> 
> The motivation behind enabling ASPM in the controller driver is provided in the
> commit message of patch 2/2.
> 
> This series has been tested on SC8280-CRD and Lenovo Thinkpad X13s laptop
> and it helped save ~0.6W of power during runtime.

Applied to controller/aspm, thank you!

[1/2] PCI: dwc: Add host_post_init() callback
      https://git.kernel.org/pci/pci/c/a78794562fcb
[2/2] PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops
      https://git.kernel.org/pci/pci/c/9f4f3dfad8cf

	Krzysztof

Re: [PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

[Bjorn Helgaas]

On Tue, Oct 10, 2023 at 09:29:13PM +0530, Manivannan Sadhasivam wrote:
> This callback can be used by the platform drivers to do configuration once
> all the devices are scanned. Like changing LNKCTL of all downstream devices
> to enable ASPM etc...
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a7170fd0e847..7991f0e179b2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		goto err_stop_link;
>  
> +	if (pp->ops->host_post_init)
> +		pp->ops->host_post_init(pp);

I know we talked about this a little bit in the context of enabling
ASPM for devices below qcom 1.9.0 controllers at
https://lore.kernel.org/r/20231011050058.GC3508@thinkpad

But I didn't realize at the time that this callback adds a fundamental
difference between devices present at boot-time (these devices can be
affected by this callback) and any devices hot-added later (we never
run this callback again, so anything done by .host_post_init() never
applies to them).

We merged this for now, and it helps enable ASPM for builtin devices
on qcom, but I don't feel good about this from a larger DWC
perspective.  If other drivers use this and they support hot-add, I
think we're going to have problems.

>  	return 0;
>  
>  err_stop_link:
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ef0b2efa9f93..efb4d4754fc8 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -301,6 +301,7 @@ enum dw_pcie_ltssm {
>  struct dw_pcie_host_ops {
>  	int (*host_init)(struct dw_pcie_rp *pp);
>  	void (*host_deinit)(struct dw_pcie_rp *pp);
> +	void (*host_post_init)(struct dw_pcie_rp *pp);
>  	int (*msi_host_init)(struct dw_pcie_rp *pp);
>  	void (*pme_turn_off)(struct dw_pcie_rp *pp);
>  };
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

[Manivannan Sadhasivam]

On Mon, Oct 16, 2023 at 03:58:49PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 10, 2023 at 09:29:13PM +0530, Manivannan Sadhasivam wrote:
> > This callback can be used by the platform drivers to do configuration once
> > all the devices are scanned. Like changing LNKCTL of all downstream devices
> > to enable ASPM etc...
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
> >  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index a7170fd0e847..7991f0e179b2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  	if (ret)
> >  		goto err_stop_link;
> >  
> > +	if (pp->ops->host_post_init)
> > +		pp->ops->host_post_init(pp);
> 
> I know we talked about this a little bit in the context of enabling
> ASPM for devices below qcom 1.9.0 controllers at
> https://lore.kernel.org/r/20231011050058.GC3508@thinkpad
> 
> But I didn't realize at the time that this callback adds a fundamental
> difference between devices present at boot-time (these devices can be
> affected by this callback) and any devices hot-added later (we never
> run this callback again, so anything done by .host_post_init() never
> applies to them).
> 
> We merged this for now, and it helps enable ASPM for builtin devices
> on qcom, but I don't feel good about this from a larger DWC
> perspective.  If other drivers use this and they support hot-add, I
> think we're going to have problems.
> 

If someone is going to add same ASPM code in host_post_init() callback, they
will most likely aware of the hotplug issue. I see this as an interim solution
overall and we should fix the PCI core to handle this. But I do not see any
straightforward way to enable ASPM by default in PCI core as the misbehaving
devices can pull the system down (atleast in some x86 cases).

- Mani

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

Re: [PATCH v2 1/2] PCI: dwc: Add host_post_init() callback

[Bjorn Helgaas]

On Tue, Oct 17, 2023 at 01:29:52PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 16, 2023 at 03:58:49PM -0500, Bjorn Helgaas wrote:
> > On Tue, Oct 10, 2023 at 09:29:13PM +0530, Manivannan Sadhasivam wrote:
> > > This callback can be used by the platform drivers to do configuration once
> > > all the devices are scanned. Like changing LNKCTL of all downstream devices
> > > to enable ASPM etc...
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
> > >  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
> > >  2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index a7170fd0e847..7991f0e179b2 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -502,6 +502,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	if (ret)
> > >  		goto err_stop_link;
> > >  
> > > +	if (pp->ops->host_post_init)
> > > +		pp->ops->host_post_init(pp);
> > 
> > I know we talked about this a little bit in the context of enabling
> > ASPM for devices below qcom 1.9.0 controllers at
> > https://lore.kernel.org/r/20231011050058.GC3508@thinkpad
> > 
> > But I didn't realize at the time that this callback adds a fundamental
> > difference between devices present at boot-time (these devices can be
> > affected by this callback) and any devices hot-added later (we never
> > run this callback again, so anything done by .host_post_init() never
> > applies to them).
> > 
> > We merged this for now, and it helps enable ASPM for builtin devices
> > on qcom, but I don't feel good about this from a larger DWC
> > perspective.  If other drivers use this and they support hot-add, I
> > think we're going to have problems.
> 
> If someone is going to add same ASPM code in host_post_init()
> callback, they will most likely aware of the hotplug issue. I see
> this as an interim solution overall and we should fix the PCI core
> to handle this. But I do not see any straightforward way to enable
> ASPM by default in PCI core as the misbehaving devices can pull the
> system down (atleast in some x86 cases).

Definitely an interim solution, but the interim is going to be long :)

I don't see the PCI core ASPM issue being resolved very soon; it's
been this way ever since the beginning.  I guess there's no point in
me whining without any proposals.

Bjorn