[PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge

[PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge

[Kai-Heng Feng]

The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
state deeper than PC3, consumes lots of unnecessary power.

On Windows ASPM L1 is enabled on the device and its upstream bridge,
so it can make the Intel SoC reach PC8 or PC10 to save lots of power.

So enable ASPM L1 like Windows does, to save additional power.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/quirks.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index ca9ed5774eb1..ac7eccf34f87 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2330,6 +2330,27 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
 
+static void quirk_enable_aspm_l1(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	u16 lnkctl;
+
+	pci_info(dev, "Enabling L1\n");
+	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
+	if (!(lnkctl & PCI_EXP_LNKCTL_ASPM_L1))
+		pcie_capability_write_word(dev, PCI_EXP_LNKCTL,
+					   lnkctl | PCI_EXP_LNKCTL_ASPM_L1);
+
+	if (!bridge)
+		return;
+
+	pcie_capability_read_word(bridge, PCI_EXP_LNKCTL, &lnkctl);
+	if (!(lnkctl & PCI_EXP_LNKCTL_ASPM_L1))
+		pcie_capability_write_word(bridge, PCI_EXP_LNKCTL,
+					   lnkctl | PCI_EXP_LNKCTL_ASPM_L1);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_TI, 0x8240, quirk_enable_aspm_l1);
+
 /*
  * Some Pericom PCIe-to-PCI bridges in reverse mode need the PCIe Retrain
  * Link bit cleared after starting the link retrain process to allow this
-- 
2.17.1

Re: [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge

[Bjorn Helgaas]

On Mon, May 04, 2020 at 03:02:59PM +0800, Kai-Heng Feng wrote:
> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> state deeper than PC3, consumes lots of unnecessary power.
> 
> On Windows ASPM L1 is enabled on the device and its upstream bridge,
> so it can make the Intel SoC reach PC8 or PC10 to save lots of power.

Does this work around some kind of hardware or firmware defect?  If
ASPM on this device works correctly, the generic code in aspm.c should
enable L1 automatically as it does for all other devices.

I don't think we should add quirks to set the ASPM configuration
directly for random devices.

> So enable ASPM L1 like Windows does, to save additional power.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index ca9ed5774eb1..ac7eccf34f87 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2330,6 +2330,27 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
>  
> +static void quirk_enable_aspm_l1(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge = pci_upstream_bridge(dev);
> +	u16 lnkctl;
> +
> +	pci_info(dev, "Enabling L1\n");
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
> +	if (!(lnkctl & PCI_EXP_LNKCTL_ASPM_L1))
> +		pcie_capability_write_word(dev, PCI_EXP_LNKCTL,
> +					   lnkctl | PCI_EXP_LNKCTL_ASPM_L1);
> +
> +	if (!bridge)
> +		return;
> +
> +	pcie_capability_read_word(bridge, PCI_EXP_LNKCTL, &lnkctl);
> +	if (!(lnkctl & PCI_EXP_LNKCTL_ASPM_L1))
> +		pcie_capability_write_word(bridge, PCI_EXP_LNKCTL,
> +					   lnkctl | PCI_EXP_LNKCTL_ASPM_L1);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_TI, 0x8240, quirk_enable_aspm_l1);
> +
>  /*
>   * Some Pericom PCIe-to-PCI bridges in reverse mode need the PCIe Retrain
>   * Link bit cleared after starting the link retrain process to allow this
> -- 
> 2.17.1
> 

[PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case

[Kai-Heng Feng]

The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
power. On Windows ASPM L1 is enabled on the device and its upstream
bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
power.

Currently, ASPM is disabled if downstream has bridge function. It was
introduced by commit 7d715a6c1ae5 ("PCI: add PCI Express ASPM support").
The commit introduced PCIe ASPM support, but didn't explain why ASPM
needs to be in that case.

So relax the condition a bit to let bridge which connects to root
complex enables ASPM, instead of removing it completely, to avoid
regression.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/aspm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 2378ed692534..af5e22d78101 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -629,13 +629,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
 	/*
-	 * If the downstream component has pci bridge function, don't
-	 * do ASPM for now.
+	 * If upstream bridge isn't connected to root complex and the
+	 * downstream component has pci bridge function, don't do ASPM for now.
 	 */
-	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
-			link->aspm_disable = ASPM_STATE_ALL;
-			break;
+	if (parent->bus->parent) {
+		list_for_each_entry(child, &linkbus->devices, bus_list) {
+			if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
+				link->aspm_disable = ASPM_STATE_ALL;
+				break;
+			}
 		}
 	}
 
-- 
2.17.1

Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case

[Bjorn Helgaas]

On Tue, May 05, 2020 at 08:27:59PM +0800, Kai-Heng Feng wrote:
> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
> power. On Windows ASPM L1 is enabled on the device and its upstream
> bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
> power.

The above is a benefit, but leading off with it suggests that this
change is specifically for that config, which it isn't.

> Currently, ASPM is disabled if downstream has bridge function. It was
> introduced by commit 7d715a6c1ae5 ("PCI: add PCI Express ASPM support").
> The commit introduced PCIe ASPM support, but didn't explain why ASPM
> needs to be in that case.

s/needs to be in that case/needs to be disabled in that case/ ?

> So relax the condition a bit to let bridge which connects to root
> complex enables ASPM, instead of removing it completely, to avoid
> regression.

If this is a regression, that means it used to work correctly.  So are
you saying 7d715a6c1ae5^ works correctly?  That seems doubtful since
7d715a6c1ae5 appeared in v2.6.26 and added ASPM support in the first
place.

> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/pcie/aspm.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2378ed692534..af5e22d78101 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -629,13 +629,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	/* Setup initial capable state. Will be updated later */
>  	link->aspm_capable = link->aspm_support;
>  	/*
> -	 * If the downstream component has pci bridge function, don't
> -	 * do ASPM for now.

I agree, that comment is missing the essential information about *why*
we don't do ASPM.

> +	 * If upstream bridge isn't connected to root complex and the
> +	 * downstream component has pci bridge function, don't do ASPM for now.

But this comment just perpetuates it and makes the special case even
more special.  I think we should either remove that special case
completely or figure out what the real issue is.

I know we weren't always very good about computing the acceptable
latencies (and we still don't handle LTR correctly, though that's an
L1 Substates issue that wouldn't have applied in the 7d715a6c1ae5
timeframe).

>  	 */
> -	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
> -			link->aspm_disable = ASPM_STATE_ALL;
> -			break;
> +	if (parent->bus->parent) {
> +		list_for_each_entry(child, &linkbus->devices, bus_list) {
> +			if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
> +				link->aspm_disable = ASPM_STATE_ALL;
> +				break;
> +			}
>  		}
>  	}
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case

[Kai-Heng Feng]

> On May 5, 2020, at 21:38, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Tue, May 05, 2020 at 08:27:59PM +0800, Kai-Heng Feng wrote:
>> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
>> state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
>> power. On Windows ASPM L1 is enabled on the device and its upstream
>> bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
>> power.
> 
> The above is a benefit, but leading off with it suggests that this
> change is specifically for that config, which it isn't.

Yes, it applies all devices that meet the condition.

> 
>> Currently, ASPM is disabled if downstream has bridge function. It was
>> introduced by commit 7d715a6c1ae5 ("PCI: add PCI Express ASPM support").
>> The commit introduced PCIe ASPM support, but didn't explain why ASPM
>> needs to be in that case.
> 
> s/needs to be in that case/needs to be disabled in that case/ ?

Yes indeed I missed that word...

> 
>> So relax the condition a bit to let bridge which connects to root
>> complex enables ASPM, instead of removing it completely, to avoid
>> regression.
> 
> If this is a regression, that means it used to work correctly.  So are
> you saying 7d715a6c1ae5^ works correctly?  That seems doubtful since
> 7d715a6c1ae5 appeared in v2.6.26 and added ASPM support in the first
> place.

Clearly I didn't express my intention well enough.
What I meant was, we can either remove the "disable ASPM on bridge" case completely, or do what this patch does.

> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/pcie/aspm.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 2378ed692534..af5e22d78101 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -629,13 +629,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>> 	/* Setup initial capable state. Will be updated later */
>> 	link->aspm_capable = link->aspm_support;
>> 	/*
>> -	 * If the downstream component has pci bridge function, don't
>> -	 * do ASPM for now.
> 
> I agree, that comment is missing the essential information about *why*
> we don't do ASPM.

Or missing a part to re-enable ASPM in later time.

> 
>> +	 * If upstream bridge isn't connected to root complex and the
>> +	 * downstream component has pci bridge function, don't do ASPM for now.
> 
> But this comment just perpetuates it and makes the special case even
> more special.  I think we should either remove that special case
> completely or figure out what the real issue is.

I do prefer remote it completely, but I was afraid of introducing any regression so I just made the case more "special".

> 
> I know we weren't always very good about computing the acceptable
> latencies (and we still don't handle LTR correctly, though that's an
> L1 Substates issue that wouldn't have applied in the 7d715a6c1ae5
> timeframe).

Seems like Windows doesn't disable ASPM on bridge to bridge case, can we take the risk and remove the special case completely?

Kai-Heng

> 
>> 	 */
>> -	list_for_each_entry(child, &linkbus->devices, bus_list) {
>> -		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
>> -			link->aspm_disable = ASPM_STATE_ALL;
>> -			break;
>> +	if (parent->bus->parent) {
>> +		list_for_each_entry(child, &linkbus->devices, bus_list) {
>> +			if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
>> +				link->aspm_disable = ASPM_STATE_ALL;
>> +				break;
>> +			}
>> 		}
>> 	}
>> 
>> -- 
>> 2.17.1

Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case

[Bjorn Helgaas]

On Tue, May 05, 2020 at 10:00:44PM +0800, Kai-Heng Feng wrote:
> > On May 5, 2020, at 21:38, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, May 05, 2020 at 08:27:59PM +0800, Kai-Heng Feng wrote:
> >> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> >> state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
> >> power. On Windows ASPM L1 is enabled on the device and its upstream
> >> bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
> >> power.
> > 
> > The above is a benefit, but leading off with it suggests that this
> > change is specifically for that config, which it isn't.
> 
> Yes, it applies all devices that meet the condition.
> 
> >> Currently, ASPM is disabled if downstream has bridge function. It was
> >> introduced by commit 7d715a6c1ae5 ("PCI: add PCI Express ASPM support").
> >> The commit introduced PCIe ASPM support, but didn't explain why ASPM
> >> needs to be in that case.
> > 
> > s/needs to be in that case/needs to be disabled in that case/ ?
> 
> Yes indeed I missed that word...
> 
> >> So relax the condition a bit to let bridge which connects to root
> >> complex enables ASPM, instead of removing it completely, to avoid
> >> regression.
> > 
> > If this is a regression, that means it used to work correctly.  So are
> > you saying 7d715a6c1ae5^ works correctly?  That seems doubtful since
> > 7d715a6c1ae5 appeared in v2.6.26 and added ASPM support in the first
> > place.
> 
> Clearly I didn't express my intention well enough.
> What I meant was, we can either remove the "disable ASPM on bridge"
> case completely, or do what this patch does.

Ah, that makes sense, thanks.

> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> >> ---
> >> drivers/pci/pcie/aspm.c | 14 ++++++++------
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> >> index 2378ed692534..af5e22d78101 100644
> >> --- a/drivers/pci/pcie/aspm.c
> >> +++ b/drivers/pci/pcie/aspm.c
> >> @@ -629,13 +629,15 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> >> 	/* Setup initial capable state. Will be updated later */
> >> 	link->aspm_capable = link->aspm_support;
> >> 	/*
> >> -	 * If the downstream component has pci bridge function, don't
> >> -	 * do ASPM for now.
> > 
> > I agree, that comment is missing the essential information about *why*
> > we don't do ASPM.
> 
> Or missing a part to re-enable ASPM in later time.
> 
> >> +	 * If upstream bridge isn't connected to root complex and the
> >> +	 * downstream component has pci bridge function, don't do ASPM for now.
> > 
> > But this comment just perpetuates it and makes the special case even
> > more special.  I think we should either remove that special case
> > completely or figure out what the real issue is.
> 
> I do prefer remote it completely, but I was afraid of introducing
> any regression so I just made the case more "special".
> 
> > I know we weren't always very good about computing the acceptable
> > latencies (and we still don't handle LTR correctly, though that's an
> > L1 Substates issue that wouldn't have applied in the 7d715a6c1ae5
> > timeframe).
> 
> Seems like Windows doesn't disable ASPM on bridge to bridge case,
> can we take the risk and remove the special case completely?

I think we should remove the special case completely.  The spec
clearly envisions the possibility of ASPM being enabled on links
between switches, e.g., PCIe r5.0, sec 5.4.1.3.1, says:

  software examines the Endpoint L0s/L1 Acceptable Latency ... and
  enables or disables L0s/L1 entry ... in some or all of the
  intervening device Ports on that hierarchy.

We might break something, but if we do, we'll learn something concrete
about what we need to avoid.

> >> 	 */
> >> -	list_for_each_entry(child, &linkbus->devices, bus_list) {
> >> -		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
> >> -			link->aspm_disable = ASPM_STATE_ALL;
> >> -			break;
> >> +	if (parent->bus->parent) {
> >> +		list_for_each_entry(child, &linkbus->devices, bus_list) {
> >> +			if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
> >> +				link->aspm_disable = ASPM_STATE_ALL;
> >> +				break;
> >> +			}
> >> 		}
> >> 	}
> >> 
> >> -- 
> >> 2.17.1
> 

[PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link

[Kai-Heng Feng]

The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
power. On Windows ASPM L1 is enabled on the device and its upstream
bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
power.

In short, ASPM always gets disabled on bridge-to-bridge link.

The special case was part of first ASPM introduction patch, commit
7d715a6c1ae5 ("PCI: add PCI Express ASPM support"). However, it didn't
explain why ASPM needs to be disabled in special bridge-to-bridge case.

Let's remove the the special case, as PCIe spec already envisioned ASPM
on bridge-to-bridge link.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v3:
 - Remove the special case completely.

v2: 
 - Enable ASPM on root complex <-> bridge <-> bridge, instead of using
   quirk.
 drivers/pci/pcie/aspm.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 2378ed692534..b17e5ffd31b1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -628,16 +628,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
-	/*
-	 * If the downstream component has pci bridge function, don't
-	 * do ASPM for now.
-	 */
-	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
-			link->aspm_disable = ASPM_STATE_ALL;
-			break;
-		}
-	}
 
 	/* Get and check endpoint acceptable latencies */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-- 
2.17.1

Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link

[Mika Westerberg]

On Wed, May 06, 2020 at 01:34:21AM +0800, Kai-Heng Feng wrote:
> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
> power. On Windows ASPM L1 is enabled on the device and its upstream
> bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
> power.
> 
> In short, ASPM always gets disabled on bridge-to-bridge link.

Excelent finding :) I've heard several reports complaining that we can't
enter PC10 when TBT is enabled and I guess this explains it.

> The special case was part of first ASPM introduction patch, commit
> 7d715a6c1ae5 ("PCI: add PCI Express ASPM support"). However, it didn't
> explain why ASPM needs to be disabled in special bridge-to-bridge case.
> 
> Let's remove the the special case, as PCIe spec already envisioned ASPM
> on bridge-to-bridge link.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link

[Bjorn Helgaas]

On Wed, May 06, 2020 at 09:14:38AM +0300, Mika Westerberg wrote:
> On Wed, May 06, 2020 at 01:34:21AM +0800, Kai-Heng Feng wrote:
> > The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> > state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
> > power. On Windows ASPM L1 is enabled on the device and its upstream
> > bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
> > power.
> > 
> > In short, ASPM always gets disabled on bridge-to-bridge link.
> 
> Excelent finding :) I've heard several reports complaining that we can't
> enter PC10 when TBT is enabled and I guess this explains it.

I'm curious about this.  I first read this patch as affecting
garden-variety Links between a Root Port or Downstream Port and the
Upstream Port of a switch.  But the case we're talking about is
specifically when the downstream device is PCI_EXP_TYPE_PCI_BRIDGE,
i.e., a PCIe to PCI/PCI-X bridge, not a switch.

AFAICT, a Link to a PCI bridge is still a normal Link and ASPM should
still work.  I'm sort of surprised that you'd find such a PCIe to
PCI/PCI-X bridge in a Thunderbolt topology, but maybe that's a common
thing?

I guess "PC8" and "PC10" are some sort of Intel-specific power states?

> > The special case was part of first ASPM introduction patch, commit
> > 7d715a6c1ae5 ("PCI: add PCI Express ASPM support"). However, it didn't
> > explain why ASPM needs to be disabled in special bridge-to-bridge case.
> > 
> > Let's remove the the special case, as PCIe spec already envisioned ASPM
> > on bridge-to-bridge link.
> > 
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link

[Mika Westerberg]

On Wed, May 06, 2020 at 04:29:47PM -0500, Bjorn Helgaas wrote:
> On Wed, May 06, 2020 at 09:14:38AM +0300, Mika Westerberg wrote:
> > On Wed, May 06, 2020 at 01:34:21AM +0800, Kai-Heng Feng wrote:
> > > The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> > > state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
> > > power. On Windows ASPM L1 is enabled on the device and its upstream
> > > bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
> > > power.
> > > 
> > > In short, ASPM always gets disabled on bridge-to-bridge link.
> > 
> > Excelent finding :) I've heard several reports complaining that we can't
> > enter PC10 when TBT is enabled and I guess this explains it.
> 
> I'm curious about this.  I first read this patch as affecting
> garden-variety Links between a Root Port or Downstream Port and the
> Upstream Port of a switch.  But the case we're talking about is
> specifically when the downstream device is PCI_EXP_TYPE_PCI_BRIDGE,
> i.e., a PCIe to PCI/PCI-X bridge, not a switch.
> 
> AFAICT, a Link to a PCI bridge is still a normal Link and ASPM should
> still work.  I'm sort of surprised that you'd find such a PCIe to
> PCI/PCI-X bridge in a Thunderbolt topology, but maybe that's a common
> thing?

It actually is not common and now that you mention I'm wondering how
this can help at all. I also thought this applies to all ports which
would explain the issue we have but if it only applies to PCIe to
PCI/PCI-X bridge it should not make any difference in TBT systems.

> I guess "PC8" and "PC10" are some sort of Intel-specific power states?

Package C-state 8 and Package C-state 10. These are power states the
whole (Intel) CPU package can enter when individual CPU cores are in
correct low power states.

Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link

[Bjorn Helgaas]

On Wed, May 06, 2020 at 01:34:21AM +0800, Kai-Heng Feng wrote:
> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> state deeper than PC3 due to disabled ASPM, consumes lots of unnecessary
> power. On Windows ASPM L1 is enabled on the device and its upstream
> bridge, so it can make the Intel SoC reach PC8 or PC10 to save lots of
> power.
> 
> In short, ASPM always gets disabled on bridge-to-bridge link.
> 
> The special case was part of first ASPM introduction patch, commit
> 7d715a6c1ae5 ("PCI: add PCI Express ASPM support"). However, it didn't
> explain why ASPM needs to be disabled in special bridge-to-bridge case.
> 
> Let's remove the the special case, as PCIe spec already envisioned ASPM
> on bridge-to-bridge link.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Applied to pci/aspm for v5.8, thanks!

I did keep your Reviewed-by, Mika.  If the fact that this applies only
to the PCIe-to-PCI/PCI-X case makes your reviewed-by invalid, just let
me know and I'll drop it.

> ---
> v3:
>  - Remove the special case completely.
> 
> v2: 
>  - Enable ASPM on root complex <-> bridge <-> bridge, instead of using
>    quirk.
>  drivers/pci/pcie/aspm.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2378ed692534..b17e5ffd31b1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -628,16 +628,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  
>  	/* Setup initial capable state. Will be updated later */
>  	link->aspm_capable = link->aspm_support;
> -	/*
> -	 * If the downstream component has pci bridge function, don't
> -	 * do ASPM for now.
> -	 */
> -	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
> -			link->aspm_disable = ASPM_STATE_ALL;
> -			break;
> -		}
> -	}
>  
>  	/* Get and check endpoint acceptable latencies */
>  	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -- 
> 2.17.1
>