* [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge @ 2020-05-04 7:02 Kai-Heng Feng 2020-05-04 14:15 ` Bjorn Helgaas 2020-05-05 12:27 ` [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case Kai-Heng Feng 0 siblings, 2 replies; 11+ messages in thread From: Kai-Heng Feng @ 2020-05-04 7:02 UTC (permalink / raw) To: bhelgaas; +Cc: Kai-Heng Feng, open list:PCI SUBSYSTEM, open list 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge 2020-05-04 7:02 [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge Kai-Heng Feng @ 2020-05-04 14:15 ` Bjorn Helgaas 2020-05-05 12:27 ` [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case Kai-Heng Feng 1 sibling, 0 replies; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-04 14:15 UTC (permalink / raw) To: Kai-Heng Feng; +Cc: bhelgaas, open list:PCI SUBSYSTEM, open list 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case 2020-05-04 7:02 [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge Kai-Heng Feng 2020-05-04 14:15 ` Bjorn Helgaas @ 2020-05-05 12:27 ` Kai-Heng Feng 2020-05-05 13:38 ` Bjorn Helgaas 2020-05-05 17:34 ` [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link Kai-Heng Feng 1 sibling, 2 replies; 11+ messages in thread From: Kai-Heng Feng @ 2020-05-05 12:27 UTC (permalink / raw) To: bhelgaas Cc: Kai-Heng Feng, Heiner Kallweit, Rafael J. Wysocki, David S. Miller, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open 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. 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case 2020-05-05 12:27 ` [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case Kai-Heng Feng @ 2020-05-05 13:38 ` Bjorn Helgaas 2020-05-05 14:00 ` Kai-Heng Feng 2020-05-05 17:34 ` [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link Kai-Heng Feng 1 sibling, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-05 13:38 UTC (permalink / raw) To: Kai-Heng Feng Cc: bhelgaas, Heiner Kallweit, Rafael J. Wysocki, David S. Miller, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case 2020-05-05 13:38 ` Bjorn Helgaas @ 2020-05-05 14:00 ` Kai-Heng Feng 2020-05-05 15:45 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Kai-Heng Feng @ 2020-05-05 14:00 UTC (permalink / raw) To: Bjorn Helgaas Cc: Bjorn Helgaas, Heiner Kallweit, Rafael J. Wysocki, David S. Miller, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case 2020-05-05 14:00 ` Kai-Heng Feng @ 2020-05-05 15:45 ` Bjorn Helgaas 0 siblings, 0 replies; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-05 15:45 UTC (permalink / raw) To: Kai-Heng Feng Cc: Bjorn Helgaas, Heiner Kallweit, Rafael J. Wysocki, David S. Miller, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link 2020-05-05 12:27 ` [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case Kai-Heng Feng 2020-05-05 13:38 ` Bjorn Helgaas @ 2020-05-05 17:34 ` Kai-Heng Feng 2020-05-06 6:14 ` Mika Westerberg 2020-05-07 22:19 ` Bjorn Helgaas 1 sibling, 2 replies; 11+ messages in thread From: Kai-Heng Feng @ 2020-05-05 17:34 UTC (permalink / raw) To: bhelgaas Cc: Kai-Heng Feng, Heiner Kallweit, Rafael J. Wysocki, Keith Busch, Mika Westerberg, Chris Packham, Yicong Yang, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open 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(-) 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link 2020-05-05 17:34 ` [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link Kai-Heng Feng @ 2020-05-06 6:14 ` Mika Westerberg 2020-05-06 21:29 ` Bjorn Helgaas 2020-05-07 22:19 ` Bjorn Helgaas 1 sibling, 1 reply; 11+ messages in thread From: Mika Westerberg @ 2020-05-06 6:14 UTC (permalink / raw) To: Kai-Heng Feng Cc: bhelgaas, Heiner Kallweit, Rafael J. Wysocki, Keith Busch, Chris Packham, Yicong Yang, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link 2020-05-06 6:14 ` Mika Westerberg @ 2020-05-06 21:29 ` Bjorn Helgaas 2020-05-07 11:50 ` Mika Westerberg 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-06 21:29 UTC (permalink / raw) To: Mika Westerberg Cc: Kai-Heng Feng, bhelgaas, Heiner Kallweit, Rafael J. Wysocki, Keith Busch, Chris Packham, Yicong Yang, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link 2020-05-06 21:29 ` Bjorn Helgaas @ 2020-05-07 11:50 ` Mika Westerberg 0 siblings, 0 replies; 11+ messages in thread From: Mika Westerberg @ 2020-05-07 11:50 UTC (permalink / raw) To: Bjorn Helgaas Cc: Kai-Heng Feng, bhelgaas, Heiner Kallweit, Rafael J. Wysocki, Keith Busch, Chris Packham, Yicong Yang, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link 2020-05-05 17:34 ` [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link Kai-Heng Feng 2020-05-06 6:14 ` Mika Westerberg @ 2020-05-07 22:19 ` Bjorn Helgaas 1 sibling, 0 replies; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-07 22:19 UTC (permalink / raw) To: Kai-Heng Feng Cc: bhelgaas, Heiner Kallweit, Rafael J. Wysocki, Keith Busch, Mika Westerberg, Chris Packham, Yicong Yang, Krzysztof Wilczynski, open list:PCI SUBSYSTEM, open list 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-07 22:19 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 7:02 [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge Kai-Heng Feng 2020-05-04 14:15 ` Bjorn Helgaas 2020-05-05 12:27 ` [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case Kai-Heng Feng 2020-05-05 13:38 ` Bjorn Helgaas 2020-05-05 14:00 ` Kai-Heng Feng 2020-05-05 15:45 ` Bjorn Helgaas 2020-05-05 17:34 ` [PATCH v3] PCI/ASPM: Enable ASPM for bridge-to-bridge link Kai-Heng Feng 2020-05-06 6:14 ` Mika Westerberg 2020-05-06 21:29 ` Bjorn Helgaas 2020-05-07 11:50 ` Mika Westerberg 2020-05-07 22:19 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).