From: Heiner Kallweit <hkallweit1@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>, Ryankao <ryankao@realtek.com>
Cc: Kai Heng Feng <kai.heng.feng@canonical.com>,
"jrg.otte@gmail.com" <jrg.otte@gmail.com>,
David Miller <davem@davemloft.net>,
Hayes Wang <hayeswang@realtek.com>,
"romieu@fr.zoreil.com" <romieu@fr.zoreil.com>,
Linux Netdev List <netdev@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Hau <hau@realtek.com>,
linux-pci@vger.kernel.org
Subject: Re: [PATCH] r8169: Reinstate ALDPS and ASPM support
Date: Tue, 5 Jun 2018 21:17:19 +0200 [thread overview]
Message-ID: <6c6b579a-5be4-ccc0-f64c-9998e51145f5@gmail.com> (raw)
In-Reply-To: <20180605191142.GA214338@bhelgaas-glaptop.roam.corp.google.com>
On 05.06.2018 21:11, Bjorn Helgaas wrote:
> [+cc linux-pci]
>
> On Tue, Jun 05, 2018 at 12:28:05PM -0500, Bjorn Helgaas wrote:
>> On Tue, Jun 05, 2018 at 06:34:09AM +0000, Ryankao wrote:
>>> Add realtek folk Hau
>>>
>>> -----Original Message-----
>>> From: Kai Heng Feng [mailto:kai.heng.feng@canonical.com]
>>> Sent: Tuesday, June 05, 2018 1:02 PM
>>> To: jrg.otte@gmail.com
>>> Cc: David Miller <davem@davemloft.net>; Hayes Wang <hayeswang@realtek.com>; hkallweit1@gmail.com; romieu@fr.zoreil.com; Linux Netdev List <netdev@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Ryankao <ryankao@realtek.com>
>>> Subject: Re: [PATCH] r8169: Reinstate ALDPS and ASPM support
>>>
>>> Hi Jörg Otte,
>>>
>>> Can you give this patch a try?
>>>
>>> Since you are the only one that reported ALDPS/ASPM regression,
>>>
>>> And I think this patch should solve the issue you had [1].
>>>
>>> Hopefully we don't need to go down the rabbit hole of blacklist/whitelist...
>>>
>>> Kai-Heng
>>>
>>> [1] https://lkml.org/lkml/2013/1/5/36
>>
>> I have no idea what ALDPS is. It's not mentioned in the PCIe spec, so
>> presumably it's some Realtek-specific thing. ASPM is a generic PCIe
>> thing. Changes to these two things should be in separate patches so
>> they don't get tangled up.
>>
ALDPS = Advanced Link Down Power Saving
And yes, it's a Realtek feature.
>>>> On Jun 5, 2018, at 12:58 PM, Kai-Heng Feng
>>>> <kai.heng.feng@canonical.com>
>>>> wrote:
>>>>
>>>> This patch reinstate ALDPS and ASPM support on r8169.
>>>>
>>>> On some Intel platforms, ASPM support on r8169 is the key factor to
>>>> let Package C-State achieve PC8. Without ASPM support, the deepest
>>>> Package C-State can hit is PC3. PC8 can save additional ~3W in
>>>> comparison with PC3.
>>>>
>>>> This patch is from Realtek.
>>>>
>>>> Fixes: e0c075577965 ("r8169: enable ALDPS for power saving")
>>>> Fixes: d64ec841517a ("r8169: enable internal ASPM and clock request
>>>> settings")
>>
>>>> +3507,15 @@ static void rtl8168e_1_hw_phy_config(struct
>>>> rtl8169_private *tp)
>>>> rtl_writephy(tp, 0x0d, 0x4007);
>>>> rtl_writephy(tp, 0x0e, 0x0000);
>>>> rtl_writephy(tp, 0x0d, 0x0000);
>>>> +
>>>> + /* Check ALDPS bit, disable it if enabled */
>>>> + rtl_writephy(tp, 0x1f, 0x0000);
>>>> + if (enable_aldps)
>>>> + rtl_w0w1_phy(tp, 0x15, 0x1000, 0x0000);
>>>> + else if (rtl_readphy(tp, 0x15) & 0x1000)
>>>> + rtl_w0w1_phy(tp, 0x15, 0x0000, 0x1000);
>>
>> There's a lot of repetition of this code with minor variations. You
>> could probably factor it out and make it more concise and more
>> readable.
>>
>>>> +static void rtl8169_check_link_status(struct net_device *dev,
>>>> + struct rtl8169_private *tp) {
>>>> + struct device *d = tp_to_dev(tp);
>>>> +
>>>> + if (tp->link_ok(tp)) {
>>>> + rtl_link_chg_patch(tp);
>>>> + /* This is to cancel a scheduled suspend if there's one. */
>>>> + if (pm_request_resume(d))
>>>> + _rtl_reset_work(tp);
>>>> + netif_carrier_on(dev);
>>>> + if (net_ratelimit())
>>>> + netif_info(tp, ifup, dev, "link up\n");
>>>> + } else {
>>>> + netif_carrier_off(dev);
>>>> + netif_info(tp, ifdown, dev, "link down\n");
>>>> + pm_runtime_idle(d);
>>>> + }
>>>> +}
>>
>> This function apparently just got moved around without changing
>> anything. That's fine, but the move should be in a separate patch to
>> make the real changes easier to review.
>>
>>>> @@ -7649,8 +7757,12 @@ static int rtl_init_one(struct pci_dev *pdev,
>>>> const struct pci_device_id *ent)
>>>>
>>>> /* disable ASPM completely as that cause random device stop working
>>>> * problems as well as full system hangs for some PCIe devices users */
>>>> - pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
>>>> - PCIE_LINK_STATE_CLKPM);
>>>> + if (!enable_aspm) {
>>>> + pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S |
>>>> + PCIE_LINK_STATE_L1 |
>>>> + PCIE_LINK_STATE_CLKPM);
>>>> + netif_info(tp, probe, dev, "ASPM disabled\n");
>>>> + }
>>
>> ASPM is a generic PCIe feature that should be configured by the PCI
>> core without any help from the device driver.
>>
>> If code in the driver is needed, that means either the PCI core is
>> doing it wrong and we should fix it there, or the device is broken and
>> the driver is working around the erratum.
>>
>> If this is an erratum, you should include details about exactly what's
>> broken and (ideally) a URL to the published erratum. Otherwise this
>> is just unmaintainable black magic and likely to be broken by future
>> ASPM changes in the PCI core.
>>
>> ASPM configuration is done by the PCI core before drivers are bound to
>> the device. If you need device-specific workarounds, they should
>> probably be in quirks so they're done before the core does that ASPM
>> configuration.
>>
>>>> /* enable device (incl. PCI PM wakeup and hotplug setup) */
>>>> rc = pcim_enable_device(pdev);
>>>> --
>>>> 2.17.0
>>>
>>> ------Please consider the environment before printing this e-mail.
>
next prev parent reply other threads:[~2018-06-05 19:24 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-05 4:58 [PATCH] r8169: Reinstate ALDPS and ASPM support Kai-Heng Feng
2018-06-05 5:02 ` Kai Heng Feng
2018-06-05 6:34 ` Ryankao
2018-06-05 17:28 ` Bjorn Helgaas
2018-06-05 19:11 ` Bjorn Helgaas
2018-06-05 19:17 ` Heiner Kallweit [this message]
2018-06-05 19:27 ` Florian Fainelli
2018-06-05 19:41 ` Heiner Kallweit
2018-06-05 19:24 ` Heiner Kallweit
2018-06-06 6:58 ` Kai-Heng Feng
2018-06-05 14:11 ` Andrew Lunn
2018-06-05 14:15 ` David Miller
2018-06-05 16:47 ` Florian Fainelli
2018-06-06 6:47 ` Kai-Heng Feng
2018-06-06 12:34 ` Andrew Lunn
2018-06-07 4:15 ` Kai Heng Feng
2018-06-05 19:13 ` Heiner Kallweit
2018-06-06 6:12 ` Simon Horman
2018-06-06 7:03 ` Kai-Heng Feng
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=6c6b579a-5be4-ccc0-f64c-9998e51145f5@gmail.com \
--to=hkallweit1@gmail.com \
--cc=davem@davemloft.net \
--cc=hau@realtek.com \
--cc=hayeswang@realtek.com \
--cc=helgaas@kernel.org \
--cc=jrg.otte@gmail.com \
--cc=kai.heng.feng@canonical.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=romieu@fr.zoreil.com \
--cc=ryankao@realtek.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).