From: Kai-Heng Feng <kai.heng.feng@canonical.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
Heiner Kallweit <hkallweit1@gmail.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
"David S. Miller" <davem@davemloft.net>,
Krzysztof Wilczynski <kw@linux.com>,
"open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] PCI/ASPM: Enable ASPM for root complex <-> bridge <-> bridge case
Date: Tue, 5 May 2020 22:00:44 +0800 [thread overview]
Message-ID: <B6977248-C345-466D-AE8B-600088B73FA8@canonical.com> (raw)
In-Reply-To: <20200505133812.GA353121@bjorn-Precision-5520>
> 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
next prev parent reply other threads:[~2020-05-05 14:03 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B6977248-C345-466D-AE8B-600088B73FA8@canonical.com \
--to=kai.heng.feng@canonical.com \
--cc=bhelgaas@google.com \
--cc=davem@davemloft.net \
--cc=helgaas@kernel.org \
--cc=hkallweit1@gmail.com \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).