| Message ID | 20200505173423.26968-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link | expand |
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>
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>
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.
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 >
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) {
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(-)