| Message ID | 20180605045812.17977-1-kai.heng.feng@canonical.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | r8169: Reinstate ALDPS and ASPM support | expand |
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 > 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") > > Cc: Ryankao <ryankao@realtek.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ > 1 file changed, 151 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 75dfac0248f4..a28ef20be221 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] = { > > MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); > > +static int enable_aspm = 1; > +static int enable_aldps = 1; > static int use_dac = -1; > static struct { > u32 msg_enable; > @@ -817,6 +819,10 @@ struct rtl8169_private { > > MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); > MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); > +module_param(enable_aspm, int, 0); > +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); > +module_param(enable_aldps, int, 0); > +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); > module_param(use_dac, int, 0); > MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot."); > module_param_named(debug, debug.msg_enable, int, 0); > @@ -1567,25 +1573,6 @@ static void rtl_link_chg_patch(struct > rtl8169_private *tp) > } > } > > -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. */ > - pm_request_resume(d); > - 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); > - } > -} > - > #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST) > > static u32 __rtl8169_get_wol(struct rtl8169_private *tp) > @@ -3520,6 +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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl_rar_exgmac_set(struct rtl8169_private *tp, u8 *addr) > @@ -3627,6 +3623,15 @@ static void rtl8168e_2_hw_phy_config(struct > rtl8169_private *tp) > > /* Broken BIOS workaround: feed GigaMAC registers with MAC address. */ > rtl_rar_exgmac_set(tp, tp->dev->dev_addr); > + > + /* 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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) > @@ -3649,6 +3654,15 @@ static void rtl8168f_hw_phy_config(struct > rtl8169_private *tp) > rtl_writephy(tp, 0x05, 0x8b86); > rtl_w0w1_phy(tp, 0x06, 0x0001, 0x0000); > rtl_writephy(tp, 0x1f, 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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) > @@ -3865,7 +3879,9 @@ static void rtl8168g_1_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -3874,6 +3890,14 @@ static void rtl8168g_1_hw_phy_config(struct > rtl8169_private *tp) > static void rtl8168g_2_hw_phy_config(struct rtl8169_private *tp) > { > rtl_apply_firmware(tp); > + > + rtl_writephy(tp, 0x1f, 0x0a43); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > + rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) > @@ -3980,7 +4004,9 @@ static void rtl8168h_1_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4053,7 +4079,9 @@ static void rtl8168h_2_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4095,7 +4123,9 @@ static void rtl8168ep_1_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4186,7 +4216,9 @@ static void rtl8168ep_2_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4233,6 +4265,15 @@ static void rtl8105e_hw_phy_config(struct > rtl8169_private *tp) > rtl_apply_firmware(tp); > > rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8402_hw_phy_config(struct rtl8169_private *tp) > @@ -4250,6 +4291,15 @@ static void rtl8402_hw_phy_config(struct > rtl8169_private *tp) > rtl_writephy(tp, 0x10, 0x401f); > rtl_writephy(tp, 0x19, 0x7030); > rtl_writephy(tp, 0x1f, 0x0000); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) > @@ -4272,6 +4322,15 @@ static void rtl8106e_hw_phy_config(struct > rtl8169_private *tp) > rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); > > rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl_hw_phy_config(struct net_device *dev) > @@ -5290,6 +5349,18 @@ static void rtl_pcie_state_l2l3_enable(struct > rtl8169_private *tp, bool enable) > RTL_W8(tp, Config3, data); > } > > +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp, > + bool enable) > +{ > + if (enable) { > + RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn); > + RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en); > + } else { > + RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > + RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + } > +} > + > static void rtl_hw_start_8168bb(struct rtl8169_private *tp) > { > RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en); > @@ -5646,9 +5717,10 @@ static void rtl_hw_start_8168g_1(struct > rtl8169_private *tp) > rtl_hw_start_8168g(tp); > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1)); > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168g_2(struct rtl8169_private *tp) > @@ -5681,9 +5753,10 @@ static void rtl_hw_start_8411_2(struct > rtl8169_private *tp) > rtl_hw_start_8168g(tp); > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2)); > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) > @@ -5700,8 +5773,7 @@ static void rtl_hw_start_8168h_1(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1)); > > RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO); > @@ -5780,6 +5852,9 @@ static void rtl_hw_start_8168h_1(struct > rtl8169_private *tp) > r8168_mac_ocp_write(tp, 0xe63e, 0x0000); > r8168_mac_ocp_write(tp, 0xc094, 0x0000); > r8168_mac_ocp_write(tp, 0xc09e, 0x0000); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep(struct rtl8169_private *tp) > @@ -5831,11 +5906,13 @@ static void rtl_hw_start_8168ep_1(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1)); > > rtl_hw_start_8168ep(tp); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) > @@ -5847,14 +5924,16 @@ static void rtl_hw_start_8168ep_2(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2)); > > rtl_hw_start_8168ep(tp); > > RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN); > RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) > @@ -5868,8 +5947,7 @@ static void rtl_hw_start_8168ep_3(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3)); > > rtl_hw_start_8168ep(tp); > @@ -5889,6 +5967,9 @@ static void rtl_hw_start_8168ep_3(struct > rtl8169_private *tp) > data = r8168_mac_ocp_read(tp, 0xe860); > data |= 0x0080; > r8168_mac_ocp_write(tp, 0xe860, data); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168(struct rtl8169_private *tp) > @@ -6364,7 +6445,7 @@ static void rtl8169_tx_clear(struct rtl8169_private > *tp) > tp->cur_tx = tp->dirty_tx = 0; > } > > -static void rtl_reset_work(struct rtl8169_private *tp) > +static void _rtl_reset_work(struct rtl8169_private *tp) > { > struct net_device *dev = tp->dev; > int i; > @@ -6384,6 +6465,33 @@ static void rtl_reset_work(struct rtl8169_private > *tp) > napi_enable(&tp->napi); > rtl_hw_start(tp); > netif_wake_queue(dev); > +} > + > +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); > + } > +} > + > +static void rtl_reset_work(struct rtl8169_private *tp) > +{ > + struct net_device *dev = tp->dev; > + > + _rtl_reset_work(tp); > rtl8169_check_link_status(dev, tp); > } > > @@ -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"); > + } > > /* enable device (incl. PCI PM wakeup and hotplug setup) */ > rc = pcim_enable_device(pdev); > -- > 2.17.0
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 > 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") > > Cc: Ryankao <ryankao@realtek.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/net/ethernet/realtek/r8169.c | 190 > +++++++++++++++++++++------ > 1 file changed, 151 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 75dfac0248f4..a28ef20be221 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -319,6 +319,8 @@ static const struct pci_device_id > rtl8169_pci_tbl[] = { > > MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); > > +static int enable_aspm = 1; > +static int enable_aldps = 1; > static int use_dac = -1; > static struct { > u32 msg_enable; > @@ -817,6 +819,10 @@ struct rtl8169_private { > > MODULE_AUTHOR("Realtek and the Linux r8169 crew > <netdev@vger.kernel.org>"); MODULE_DESCRIPTION("RealTek RTL-8169 > Gigabit Ethernet driver"); > +module_param(enable_aspm, int, 0); > +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); > +module_param(enable_aldps, int, 0); MODULE_PARM_DESC(enable_aldps, > +"Enable ALDPS"); > module_param(use_dac, int, 0); > MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI > slot."); module_param_named(debug, debug.msg_enable, int, 0); @@ > -1567,25 +1573,6 @@ static void rtl_link_chg_patch(struct > rtl8169_private *tp) > } > } > > -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. */ > - pm_request_resume(d); > - 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); > - } > -} > - > #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | > WAKE_MCAST) > > static u32 __rtl8169_get_wol(struct rtl8169_private *tp) @@ -3520,6 > +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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl_rar_exgmac_set(struct rtl8169_private *tp, u8 *addr) > @@ -3627,6 +3623,15 @@ static void rtl8168e_2_hw_phy_config(struct > rtl8169_private *tp) > > /* Broken BIOS workaround: feed GigaMAC registers with MAC address. */ > rtl_rar_exgmac_set(tp, tp->dev->dev_addr); > + > + /* 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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) @@ > -3649,6 +3654,15 @@ static void rtl8168f_hw_phy_config(struct > rtl8169_private *tp) > rtl_writephy(tp, 0x05, 0x8b86); > rtl_w0w1_phy(tp, 0x06, 0x0001, 0x0000); > rtl_writephy(tp, 0x1f, 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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) @@ > -3865,7 +3879,9 @@ static void rtl8168g_1_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -3874,6 +3890,14 @@ static void rtl8168g_1_hw_phy_config(struct > rtl8169_private *tp) static void rtl8168g_2_hw_phy_config(struct > rtl8169_private *tp) { > rtl_apply_firmware(tp); > + > + rtl_writephy(tp, 0x1f, 0x0a43); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > + rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) @@ > -3980,7 +4004,9 @@ static void rtl8168h_1_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4053,7 +4079,9 @@ static void rtl8168h_2_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4095,7 +4123,9 @@ static void rtl8168ep_1_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4186,7 +4216,9 @@ static void rtl8168ep_2_hw_phy_config(struct > rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4233,6 +4265,15 @@ static void rtl8105e_hw_phy_config(struct > rtl8169_private *tp) > rtl_apply_firmware(tp); > > rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8402_hw_phy_config(struct rtl8169_private *tp) @@ > -4250,6 +4291,15 @@ static void rtl8402_hw_phy_config(struct > rtl8169_private *tp) > rtl_writephy(tp, 0x10, 0x401f); > rtl_writephy(tp, 0x19, 0x7030); > rtl_writephy(tp, 0x1f, 0x0000); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) @@ > -4272,6 +4322,15 @@ static void rtl8106e_hw_phy_config(struct > rtl8169_private *tp) > rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); > > rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl_hw_phy_config(struct net_device *dev) @@ -5290,6 > +5349,18 @@ static void rtl_pcie_state_l2l3_enable(struct > rtl8169_private *tp, bool enable) > RTL_W8(tp, Config3, data); > } > > +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp, > + bool enable) > +{ > + if (enable) { > + RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn); > + RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en); > + } else { > + RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > + RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + } > +} > + > static void rtl_hw_start_8168bb(struct rtl8169_private *tp) { > RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en); @@ -5646,9 > +5717,10 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private > *tp) > rtl_hw_start_8168g(tp); > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1)); > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168g_2(struct rtl8169_private *tp) @@ > -5681,9 +5753,10 @@ static void rtl_hw_start_8411_2(struct > rtl8169_private *tp) > rtl_hw_start_8168g(tp); > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2)); > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) @@ > -5700,8 +5773,7 @@ static void rtl_hw_start_8168h_1(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1)); > > RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO); @@ > -5780,6 +5852,9 @@ static void rtl_hw_start_8168h_1(struct > rtl8169_private *tp) > r8168_mac_ocp_write(tp, 0xe63e, 0x0000); > r8168_mac_ocp_write(tp, 0xc094, 0x0000); > r8168_mac_ocp_write(tp, 0xc09e, 0x0000); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep(struct rtl8169_private *tp) @@ > -5831,11 +5906,13 @@ static void rtl_hw_start_8168ep_1(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1)); > > rtl_hw_start_8168ep(tp); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) @@ > -5847,14 +5924,16 @@ static void rtl_hw_start_8168ep_2(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2)); > > rtl_hw_start_8168ep(tp); > > RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN); > RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) @@ > -5868,8 +5947,7 @@ static void rtl_hw_start_8168ep_3(struct > rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3)); > > rtl_hw_start_8168ep(tp); > @@ -5889,6 +5967,9 @@ static void rtl_hw_start_8168ep_3(struct > rtl8169_private *tp) > data = r8168_mac_ocp_read(tp, 0xe860); > data |= 0x0080; > r8168_mac_ocp_write(tp, 0xe860, data); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168(struct rtl8169_private *tp) @@ -6364,7 > +6445,7 @@ static void rtl8169_tx_clear(struct rtl8169_private > *tp) > tp->cur_tx = tp->dirty_tx = 0; > } > > -static void rtl_reset_work(struct rtl8169_private *tp) > +static void _rtl_reset_work(struct rtl8169_private *tp) > { > struct net_device *dev = tp->dev; > int i; > @@ -6384,6 +6465,33 @@ static void rtl_reset_work(struct > rtl8169_private > *tp) > napi_enable(&tp->napi); > rtl_hw_start(tp); > netif_wake_queue(dev); > +} > + > +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); > + } > +} > + > +static void rtl_reset_work(struct rtl8169_private *tp) { > + struct net_device *dev = tp->dev; > + > + _rtl_reset_work(tp); > rtl8169_check_link_status(dev, tp); > } > > @@ -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"); > + } > > /* 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.
On Tue, Jun 05, 2018 at 12:58:12PM +0800, Kai-Heng Feng 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") > > Cc: Ryankao <ryankao@realtek.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ > 1 file changed, 151 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 75dfac0248f4..a28ef20be221 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] = { > > MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); > > +static int enable_aspm = 1; > +static int enable_aldps = 1; > static int use_dac = -1; > static struct { > u32 msg_enable; > @@ -817,6 +819,10 @@ struct rtl8169_private { > > MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); > MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); > +module_param(enable_aspm, int, 0); > +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); > +module_param(enable_aldps, int, 0); > +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); Hi Kai No module parameter please. Just turn it on by default. Assuming testing shows works. Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: Tue, 5 Jun 2018 16:11:14 +0200 > No module parameter please. Just turn it on by default. Assuming > testing shows works. Agreed.
On 06/05/2018 07:15 AM, David Miller wrote: > From: Andrew Lunn <andrew@lunn.ch> > Date: Tue, 5 Jun 2018 16:11:14 +0200 > >> No module parameter please. Just turn it on by default. Assuming >> testing shows works. > > Agreed. devlink would be a good candidate to add such configuration attributes, since you would be operating on the PCI function itself, thus allowing this to be on a per-device instance basis as opposed to global, which is what a module parameter is.
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. > > 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.
[+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. > > > > 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.
On 05.06.2018 06:58, Kai-Heng Feng 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") > > Cc: Ryankao <ryankao@realtek.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ > 1 file changed, 151 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 75dfac0248f4..a28ef20be221 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] = { > > MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); > > +static int enable_aspm = 1; > +static int enable_aldps = 1; > static int use_dac = -1; > static struct { > u32 msg_enable; > @@ -817,6 +819,10 @@ struct rtl8169_private { > > MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); > MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); > +module_param(enable_aspm, int, 0); > +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); > +module_param(enable_aldps, int, 0); > +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); > module_param(use_dac, int, 0); > MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot."); > module_param_named(debug, debug.msg_enable, int, 0); > @@ -1567,25 +1573,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp) > } > } > > -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. */ > - pm_request_resume(d); > - 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); > - } > -} > - > #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST) > > static u32 __rtl8169_get_wol(struct rtl8169_private *tp) > @@ -3520,6 +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); > + > + rtl_writephy(tp, 0x1f, 0x0000); Few remarks: - The comment isn't applicable any longer. - The second rtl_writephy(tp, 0x1f, 0x0000) isn't needed because you don't switch the page in between. - The code is a little hard to read, instead you could use the following and create a helper, ideally with register and bit number as parameters so that you can use it for all affected chip types. val = rtl_readphy(tp, 0x15); val &= ~BIT(12); if (enable_aldps) val |= BIT(12); rtl_writephy(tp, 0x15, val); > } > > static void rtl_rar_exgmac_set(struct rtl8169_private *tp, u8 *addr) > @@ -3627,6 +3623,15 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp) > > /* Broken BIOS workaround: feed GigaMAC registers with MAC address. */ > rtl_rar_exgmac_set(tp, tp->dev->dev_addr); > + > + /* 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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) > @@ -3649,6 +3654,15 @@ static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) > rtl_writephy(tp, 0x05, 0x8b86); > rtl_w0w1_phy(tp, 0x06, 0x0001, 0x0000); > rtl_writephy(tp, 0x1f, 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); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) > @@ -3865,7 +3879,9 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -3874,6 +3890,14 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) > static void rtl8168g_2_hw_phy_config(struct rtl8169_private *tp) > { > rtl_apply_firmware(tp); > + > + rtl_writephy(tp, 0x1f, 0x0a43); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > + rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) > @@ -3980,7 +4004,9 @@ static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4053,7 +4079,9 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4095,7 +4123,9 @@ static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4186,7 +4216,9 @@ static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp) > > /* Check ALDPS bit, disable it if enabled */ > rtl_writephy(tp, 0x1f, 0x0a43); > - if (rtl_readphy(tp, 0x10) & 0x0004) > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); > + else if (rtl_readphy(tp, 0x10) & 0x0004) > rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); > > rtl_writephy(tp, 0x1f, 0x0000); > @@ -4233,6 +4265,15 @@ static void rtl8105e_hw_phy_config(struct rtl8169_private *tp) > rtl_apply_firmware(tp); > > rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8402_hw_phy_config(struct rtl8169_private *tp) > @@ -4250,6 +4291,15 @@ static void rtl8402_hw_phy_config(struct rtl8169_private *tp) > rtl_writephy(tp, 0x10, 0x401f); > rtl_writephy(tp, 0x19, 0x7030); > rtl_writephy(tp, 0x1f, 0x0000); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) > @@ -4272,6 +4322,15 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) > rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); > > rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC); > + > + /* Check ALDPS bit, disable it if enabled */ > + rtl_writephy(tp, 0x1f, 0x0000); > + if (enable_aldps) > + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); > + else if (rtl_readphy(tp, 0x18) & 0x1000) > + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); > + > + rtl_writephy(tp, 0x1f, 0x0000); > } > > static void rtl_hw_phy_config(struct net_device *dev) > @@ -5290,6 +5349,18 @@ static void rtl_pcie_state_l2l3_enable(struct rtl8169_private *tp, bool enable) > RTL_W8(tp, Config3, data); > } > > +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp, > + bool enable) > +{ > + if (enable) { > + RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn); > + RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en); > + } else { > + RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > + RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + } > +} > + > static void rtl_hw_start_8168bb(struct rtl8169_private *tp) > { > RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en); > @@ -5646,9 +5717,10 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp) > rtl_hw_start_8168g(tp); > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1)); > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168g_2(struct rtl8169_private *tp) > @@ -5681,9 +5753,10 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp) > rtl_hw_start_8168g(tp); > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2)); > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) > @@ -5700,8 +5773,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1)); > > RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO); > @@ -5780,6 +5852,9 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) > r8168_mac_ocp_write(tp, 0xe63e, 0x0000); > r8168_mac_ocp_write(tp, 0xc094, 0x0000); > r8168_mac_ocp_write(tp, 0xc09e, 0x0000); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep(struct rtl8169_private *tp) > @@ -5831,11 +5906,13 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1)); > > rtl_hw_start_8168ep(tp); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) > @@ -5847,14 +5924,16 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2)); > > rtl_hw_start_8168ep(tp); > > RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN); > RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) > @@ -5868,8 +5947,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) > }; > > /* disable aspm and clock request before access ephy */ > - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); > - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); > + rtl_hw_internal_aspm_clkreq_enable(tp, false); > rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3)); > > rtl_hw_start_8168ep(tp); > @@ -5889,6 +5967,9 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) > data = r8168_mac_ocp_read(tp, 0xe860); > data |= 0x0080; > r8168_mac_ocp_write(tp, 0xe860, data); > + > + if (enable_aspm) > + rtl_hw_internal_aspm_clkreq_enable(tp, true); > } > > static void rtl_hw_start_8168(struct rtl8169_private *tp) > @@ -6364,7 +6445,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp) > tp->cur_tx = tp->dirty_tx = 0; > } > > -static void rtl_reset_work(struct rtl8169_private *tp) > +static void _rtl_reset_work(struct rtl8169_private *tp) > { > struct net_device *dev = tp->dev; > int i; > @@ -6384,6 +6465,33 @@ static void rtl_reset_work(struct rtl8169_private *tp) > napi_enable(&tp->napi); > rtl_hw_start(tp); > netif_wake_queue(dev); > +} > + > +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); This reset was added, what is it good for and how is it related to ASPM/ALDPS? It looks a little bogus, especially considering that pm_request_resume() can return also positive values. > + 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); > + } > +} > + > +static void rtl_reset_work(struct rtl8169_private *tp) > +{ > + struct net_device *dev = tp->dev; > + > + _rtl_reset_work(tp); > rtl8169_check_link_status(dev, tp); > } > > @@ -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"); You should use dev_info() here because the net_device isn't registered yet. > + } > > /* enable device (incl. PCI PM wakeup and hotplug setup) */ > rc = pcim_enable_device(pdev); >
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. >
On 05.06.2018 19:28, 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. > >>> 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. > Fully agree, but: There are no publicly available datasheets and only source for such magic is the r8168 vendor driver and trial&error. In addition the driver supports ~ 50 chip variants and not all variants (unfortunately nobody except Realtek knows which) are affected by the problem. Maybe the involved Realtek guys can shed some light on this. > 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. >
On 06/05/2018 12:17 PM, Heiner Kallweit wrote: > 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. Link as in Ethernet link or PCI(e) link? Sorry too lazy to let me google that for myself :)
On 05.06.2018 21:27, Florian Fainelli wrote: > On 06/05/2018 12:17 PM, Heiner Kallweit wrote: >> 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. > > Link as in Ethernet link or PCI(e) link? Sorry too lazy to let me google > that for myself :) > Sorry .. Link here refers to the Ethernet PHY.
On Tue, Jun 05, 2018 at 09:13:08PM +0200, Heiner Kallweit wrote: > On 05.06.2018 06:58, Kai-Heng Feng 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") > > > > Cc: Ryankao <ryankao@realtek.com> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ > > 1 file changed, 151 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > > index 75dfac0248f4..a28ef20be221 100644 > > --- a/drivers/net/ethernet/realtek/r8169.c > > +++ b/drivers/net/ethernet/realtek/r8169.c > > @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] = { > > > > MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); > > > > +static int enable_aspm = 1; > > +static int enable_aldps = 1; > > static int use_dac = -1; > > static struct { > > u32 msg_enable; > > @@ -817,6 +819,10 @@ struct rtl8169_private { > > > > MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); > > MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); > > +module_param(enable_aspm, int, 0); > > +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); > > +module_param(enable_aldps, int, 0); > > +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); > > module_param(use_dac, int, 0); > > MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot."); > > module_param_named(debug, debug.msg_enable, int, 0); I'm a little surprised to see new module parameters being added.
at 22:11, Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Jun 05, 2018 at 12:58:12PM +0800, Kai-Heng Feng 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") >> >> Cc: Ryankao <ryankao@realtek.com> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ >> 1 file changed, 151 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169.c >> b/drivers/net/ethernet/realtek/r8169.c >> index 75dfac0248f4..a28ef20be221 100644 >> --- a/drivers/net/ethernet/realtek/r8169.c >> +++ b/drivers/net/ethernet/realtek/r8169.c >> @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] >> = { >> >> MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); >> >> +static int enable_aspm = 1; >> +static int enable_aldps = 1; >> static int use_dac = -1; >> static struct { >> u32 msg_enable; >> @@ -817,6 +819,10 @@ struct rtl8169_private { >> >> MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); >> MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); >> +module_param(enable_aspm, int, 0); >> +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); >> +module_param(enable_aldps, int, 0); >> +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); > > Hi Kai > > No module parameter please. Just turn it on by default. Assuming > testing shows works. Hi Andrew, Sure. Do you think we should also strip out the enable/disable logic? Or just remove the parameter? Kai-Heng > > Andrew
Hi, Bjorn, at 01:28, Bjorn Helgaas <helgaas@kernel.org> 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. Sure, I'll split them in two. I'll also consult with Realtek to explain what ALDPS actually does. > >>> 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. You are right. Will do. > >>> +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. It actually added a new condition to check the return value of pm_request_resume(). It's probably a bogus check though. I'll ask Realtek why they did this. > >>> @@ -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. You are right. I think calling pci_disable_link_state() is unnecessary, I'll also consult with Realtek about this. I'll also do some testing and send a separate patch to remove pci_disable_link_state() for -rc kernel to test if this is something really needed. Kai-Heng > >>> /* 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.
at 03:13, Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 05.06.2018 06:58, Kai-Heng Feng 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") >> >> Cc: Ryankao <ryankao@realtek.com> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ >> 1 file changed, 151 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169.c >> b/drivers/net/ethernet/realtek/r8169.c >> index 75dfac0248f4..a28ef20be221 100644 >> --- a/drivers/net/ethernet/realtek/r8169.c >> +++ b/drivers/net/ethernet/realtek/r8169.c >> @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] >> = { >> >> MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); >> >> +static int enable_aspm = 1; >> +static int enable_aldps = 1; >> static int use_dac = -1; >> static struct { >> u32 msg_enable; >> @@ -817,6 +819,10 @@ struct rtl8169_private { >> >> MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); >> MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); >> +module_param(enable_aspm, int, 0); >> +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); >> +module_param(enable_aldps, int, 0); >> +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); >> module_param(use_dac, int, 0); >> MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot."); >> module_param_named(debug, debug.msg_enable, int, 0); >> @@ -1567,25 +1573,6 @@ static void rtl_link_chg_patch(struct >> rtl8169_private *tp) >> } >> } >> >> -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. */ >> - pm_request_resume(d); >> - 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); >> - } >> -} >> - >> #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST) >> >> static u32 __rtl8169_get_wol(struct rtl8169_private *tp) >> @@ -3520,6 +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); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); > > Few remarks: > - The comment isn't applicable any longer. > - The second rtl_writephy(tp, 0x1f, 0x0000) isn't needed because you don't > switch the page in between. > - The code is a little hard to read, instead you could use the following > and create a helper, ideally with register and bit number as > parameters so that you can use it for all affected chip types. > > val = rtl_readphy(tp, 0x15); > val &= ~BIT(12); > if (enable_aldps) > val |= BIT(12); > rtl_writephy(tp, 0x15, val); This is much cleaner. Thanks! > >> } >> >> static void rtl_rar_exgmac_set(struct rtl8169_private *tp, u8 *addr) >> @@ -3627,6 +3623,15 @@ static void rtl8168e_2_hw_phy_config(struct >> rtl8169_private *tp) >> >> /* Broken BIOS workaround: feed GigaMAC registers with MAC address. */ >> rtl_rar_exgmac_set(tp, tp->dev->dev_addr); >> + >> + /* 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); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); >> } >> >> static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) >> @@ -3649,6 +3654,15 @@ static void rtl8168f_hw_phy_config(struct >> rtl8169_private *tp) >> rtl_writephy(tp, 0x05, 0x8b86); >> rtl_w0w1_phy(tp, 0x06, 0x0001, 0x0000); >> rtl_writephy(tp, 0x1f, 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); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); >> } >> >> static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) >> @@ -3865,7 +3879,9 @@ static void rtl8168g_1_hw_phy_config(struct >> rtl8169_private *tp) >> >> /* Check ALDPS bit, disable it if enabled */ >> rtl_writephy(tp, 0x1f, 0x0a43); >> - if (rtl_readphy(tp, 0x10) & 0x0004) >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); >> + else if (rtl_readphy(tp, 0x10) & 0x0004) >> rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); >> >> rtl_writephy(tp, 0x1f, 0x0000); >> @@ -3874,6 +3890,14 @@ static void rtl8168g_1_hw_phy_config(struct >> rtl8169_private *tp) >> static void rtl8168g_2_hw_phy_config(struct rtl8169_private *tp) >> { >> rtl_apply_firmware(tp); >> + >> + rtl_writephy(tp, 0x1f, 0x0a43); >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); >> + else if (rtl_readphy(tp, 0x10) & 0x0004) >> + rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); >> } >> >> static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) >> @@ -3980,7 +4004,9 @@ static void rtl8168h_1_hw_phy_config(struct >> rtl8169_private *tp) >> >> /* Check ALDPS bit, disable it if enabled */ >> rtl_writephy(tp, 0x1f, 0x0a43); >> - if (rtl_readphy(tp, 0x10) & 0x0004) >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); >> + else if (rtl_readphy(tp, 0x10) & 0x0004) >> rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); >> >> rtl_writephy(tp, 0x1f, 0x0000); >> @@ -4053,7 +4079,9 @@ static void rtl8168h_2_hw_phy_config(struct >> rtl8169_private *tp) >> >> /* Check ALDPS bit, disable it if enabled */ >> rtl_writephy(tp, 0x1f, 0x0a43); >> - if (rtl_readphy(tp, 0x10) & 0x0004) >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); >> + else if (rtl_readphy(tp, 0x10) & 0x0004) >> rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); >> >> rtl_writephy(tp, 0x1f, 0x0000); >> @@ -4095,7 +4123,9 @@ static void rtl8168ep_1_hw_phy_config(struct >> rtl8169_private *tp) >> >> /* Check ALDPS bit, disable it if enabled */ >> rtl_writephy(tp, 0x1f, 0x0a43); >> - if (rtl_readphy(tp, 0x10) & 0x0004) >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); >> + else if (rtl_readphy(tp, 0x10) & 0x0004) >> rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); >> >> rtl_writephy(tp, 0x1f, 0x0000); >> @@ -4186,7 +4216,9 @@ static void rtl8168ep_2_hw_phy_config(struct >> rtl8169_private *tp) >> >> /* Check ALDPS bit, disable it if enabled */ >> rtl_writephy(tp, 0x1f, 0x0a43); >> - if (rtl_readphy(tp, 0x10) & 0x0004) >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); >> + else if (rtl_readphy(tp, 0x10) & 0x0004) >> rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); >> >> rtl_writephy(tp, 0x1f, 0x0000); >> @@ -4233,6 +4265,15 @@ static void rtl8105e_hw_phy_config(struct >> rtl8169_private *tp) >> rtl_apply_firmware(tp); >> >> rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); >> + >> + /* Check ALDPS bit, disable it if enabled */ >> + rtl_writephy(tp, 0x1f, 0x0000); >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); >> + else if (rtl_readphy(tp, 0x18) & 0x1000) >> + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); >> } >> >> static void rtl8402_hw_phy_config(struct rtl8169_private *tp) >> @@ -4250,6 +4291,15 @@ static void rtl8402_hw_phy_config(struct >> rtl8169_private *tp) >> rtl_writephy(tp, 0x10, 0x401f); >> rtl_writephy(tp, 0x19, 0x7030); >> rtl_writephy(tp, 0x1f, 0x0000); >> + >> + /* Check ALDPS bit, disable it if enabled */ >> + rtl_writephy(tp, 0x1f, 0x0000); >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); >> + else if (rtl_readphy(tp, 0x18) & 0x1000) >> + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); >> } >> >> static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) >> @@ -4272,6 +4322,15 @@ static void rtl8106e_hw_phy_config(struct >> rtl8169_private *tp) >> rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); >> >> rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC); >> + >> + /* Check ALDPS bit, disable it if enabled */ >> + rtl_writephy(tp, 0x1f, 0x0000); >> + if (enable_aldps) >> + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); >> + else if (rtl_readphy(tp, 0x18) & 0x1000) >> + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); >> + >> + rtl_writephy(tp, 0x1f, 0x0000); >> } >> >> static void rtl_hw_phy_config(struct net_device *dev) >> @@ -5290,6 +5349,18 @@ static void rtl_pcie_state_l2l3_enable(struct >> rtl8169_private *tp, bool enable) >> RTL_W8(tp, Config3, data); >> } >> >> +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private >> *tp, >> + bool enable) >> +{ >> + if (enable) { >> + RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn); >> + RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en); >> + } else { >> + RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> + RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + } >> +} >> + >> static void rtl_hw_start_8168bb(struct rtl8169_private *tp) >> { >> RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en); >> @@ -5646,9 +5717,10 @@ static void rtl_hw_start_8168g_1(struct >> rtl8169_private *tp) >> rtl_hw_start_8168g(tp); >> >> /* disable aspm and clock request before access ephy */ >> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + rtl_hw_internal_aspm_clkreq_enable(tp, false); >> rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1)); >> + if (enable_aspm) >> + rtl_hw_internal_aspm_clkreq_enable(tp, true); >> } >> >> static void rtl_hw_start_8168g_2(struct rtl8169_private *tp) >> @@ -5681,9 +5753,10 @@ static void rtl_hw_start_8411_2(struct >> rtl8169_private *tp) >> rtl_hw_start_8168g(tp); >> >> /* disable aspm and clock request before access ephy */ >> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + rtl_hw_internal_aspm_clkreq_enable(tp, false); >> rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2)); >> + if (enable_aspm) >> + rtl_hw_internal_aspm_clkreq_enable(tp, true); >> } >> >> static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) >> @@ -5700,8 +5773,7 @@ static void rtl_hw_start_8168h_1(struct >> rtl8169_private *tp) >> }; >> >> /* disable aspm and clock request before access ephy */ >> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + rtl_hw_internal_aspm_clkreq_enable(tp, false); >> rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1)); >> >> RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO); >> @@ -5780,6 +5852,9 @@ static void rtl_hw_start_8168h_1(struct >> rtl8169_private *tp) >> r8168_mac_ocp_write(tp, 0xe63e, 0x0000); >> r8168_mac_ocp_write(tp, 0xc094, 0x0000); >> r8168_mac_ocp_write(tp, 0xc09e, 0x0000); >> + >> + if (enable_aspm) >> + rtl_hw_internal_aspm_clkreq_enable(tp, true); >> } >> >> static void rtl_hw_start_8168ep(struct rtl8169_private *tp) >> @@ -5831,11 +5906,13 @@ static void rtl_hw_start_8168ep_1(struct >> rtl8169_private *tp) >> }; >> >> /* disable aspm and clock request before access ephy */ >> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + rtl_hw_internal_aspm_clkreq_enable(tp, false); >> rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1)); >> >> rtl_hw_start_8168ep(tp); >> + >> + if (enable_aspm) >> + rtl_hw_internal_aspm_clkreq_enable(tp, true); >> } >> >> static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) >> @@ -5847,14 +5924,16 @@ static void rtl_hw_start_8168ep_2(struct >> rtl8169_private *tp) >> }; >> >> /* disable aspm and clock request before access ephy */ >> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + rtl_hw_internal_aspm_clkreq_enable(tp, false); >> rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2)); >> >> rtl_hw_start_8168ep(tp); >> >> RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN); >> RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN); >> + >> + if (enable_aspm) >> + rtl_hw_internal_aspm_clkreq_enable(tp, true); >> } >> >> static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) >> @@ -5868,8 +5947,7 @@ static void rtl_hw_start_8168ep_3(struct >> rtl8169_private *tp) >> }; >> >> /* disable aspm and clock request before access ephy */ >> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); >> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); >> + rtl_hw_internal_aspm_clkreq_enable(tp, false); >> rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3)); >> >> rtl_hw_start_8168ep(tp); >> @@ -5889,6 +5967,9 @@ static void rtl_hw_start_8168ep_3(struct >> rtl8169_private *tp) >> data = r8168_mac_ocp_read(tp, 0xe860); >> data |= 0x0080; >> r8168_mac_ocp_write(tp, 0xe860, data); >> + >> + if (enable_aspm) >> + rtl_hw_internal_aspm_clkreq_enable(tp, true); >> } >> >> static void rtl_hw_start_8168(struct rtl8169_private *tp) >> @@ -6364,7 +6445,7 @@ static void rtl8169_tx_clear(struct >> rtl8169_private *tp) >> tp->cur_tx = tp->dirty_tx = 0; >> } >> >> -static void rtl_reset_work(struct rtl8169_private *tp) >> +static void _rtl_reset_work(struct rtl8169_private *tp) >> { >> struct net_device *dev = tp->dev; >> int i; >> @@ -6384,6 +6465,33 @@ static void rtl_reset_work(struct rtl8169_private >> *tp) >> napi_enable(&tp->napi); >> rtl_hw_start(tp); >> netif_wake_queue(dev); >> +} >> + >> +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); > > This reset was added, what is it good for and how is it related to > ASPM/ALDPS? It looks a little bogus, especially considering that > pm_request_resume() can return also positive values. Probably. I'll do some testing and discuss with Realtek. > >> + 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); >> + } >> +} >> + >> +static void rtl_reset_work(struct rtl8169_private *tp) >> +{ >> + struct net_device *dev = tp->dev; >> + >> + _rtl_reset_work(tp); >> rtl8169_check_link_status(dev, tp); >> } >> >> @@ -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"); > > You should use dev_info() here because the net_device isn't registered yet. Thanks. I will change that. Kai-Heng > >> + } >> >> /* enable device (incl. PCI PM wakeup and hotplug setup) */ >> rc = pcim_enable_device(pdev);
> Hi Andrew, > > Sure. > > Do you think we should also strip out the enable/disable logic? > > Or just remove the parameter? Hi Kai-Heng How would you use the logic if you remove the parameter? Andrew
> On Jun 6, 2018, at 8:34 PM, Andrew Lunn <andrew@lunn.ch> wrote: > >> Hi Andrew, >> >> Sure. >> >> Do you think we should also strip out the enable/disable logic? >> >> Or just remove the parameter? > > Hi Kai-Heng > > How would you use the logic if you remove the parameter? Hi Andrew, Just in case someone needs it. But I get what you mean, I'll remove them completely. Kai-Heng > > Andrew
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 75dfac0248f4..a28ef20be221 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -319,6 +319,8 @@ static const struct pci_device_id rtl8169_pci_tbl[] = { MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl); +static int enable_aspm = 1; +static int enable_aldps = 1; static int use_dac = -1; static struct { u32 msg_enable; @@ -817,6 +819,10 @@ struct rtl8169_private { MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>"); MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver"); +module_param(enable_aspm, int, 0); +MODULE_PARM_DESC(enable_aspm, "Enable ASPM"); +module_param(enable_aldps, int, 0); +MODULE_PARM_DESC(enable_aldps, "Enable ALDPS"); module_param(use_dac, int, 0); MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot."); module_param_named(debug, debug.msg_enable, int, 0); @@ -1567,25 +1573,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp) } } -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. */ - pm_request_resume(d); - 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); - } -} - #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST) static u32 __rtl8169_get_wol(struct rtl8169_private *tp) @@ -3520,6 +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); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl_rar_exgmac_set(struct rtl8169_private *tp, u8 *addr) @@ -3627,6 +3623,15 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp) /* Broken BIOS workaround: feed GigaMAC registers with MAC address. */ rtl_rar_exgmac_set(tp, tp->dev->dev_addr); + + /* 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); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) @@ -3649,6 +3654,15 @@ static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) rtl_writephy(tp, 0x05, 0x8b86); rtl_w0w1_phy(tp, 0x06, 0x0001, 0x0000); rtl_writephy(tp, 0x1f, 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); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) @@ -3865,7 +3879,9 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) /* Check ALDPS bit, disable it if enabled */ rtl_writephy(tp, 0x1f, 0x0a43); - if (rtl_readphy(tp, 0x10) & 0x0004) + if (enable_aldps) + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); + else if (rtl_readphy(tp, 0x10) & 0x0004) rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); rtl_writephy(tp, 0x1f, 0x0000); @@ -3874,6 +3890,14 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) static void rtl8168g_2_hw_phy_config(struct rtl8169_private *tp) { rtl_apply_firmware(tp); + + rtl_writephy(tp, 0x1f, 0x0a43); + if (enable_aldps) + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); + else if (rtl_readphy(tp, 0x10) & 0x0004) + rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) @@ -3980,7 +4004,9 @@ static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp) /* Check ALDPS bit, disable it if enabled */ rtl_writephy(tp, 0x1f, 0x0a43); - if (rtl_readphy(tp, 0x10) & 0x0004) + if (enable_aldps) + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); + else if (rtl_readphy(tp, 0x10) & 0x0004) rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); rtl_writephy(tp, 0x1f, 0x0000); @@ -4053,7 +4079,9 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp) /* Check ALDPS bit, disable it if enabled */ rtl_writephy(tp, 0x1f, 0x0a43); - if (rtl_readphy(tp, 0x10) & 0x0004) + if (enable_aldps) + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); + else if (rtl_readphy(tp, 0x10) & 0x0004) rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); rtl_writephy(tp, 0x1f, 0x0000); @@ -4095,7 +4123,9 @@ static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp) /* Check ALDPS bit, disable it if enabled */ rtl_writephy(tp, 0x1f, 0x0a43); - if (rtl_readphy(tp, 0x10) & 0x0004) + if (enable_aldps) + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); + else if (rtl_readphy(tp, 0x10) & 0x0004) rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); rtl_writephy(tp, 0x1f, 0x0000); @@ -4186,7 +4216,9 @@ static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp) /* Check ALDPS bit, disable it if enabled */ rtl_writephy(tp, 0x1f, 0x0a43); - if (rtl_readphy(tp, 0x10) & 0x0004) + if (enable_aldps) + rtl_w0w1_phy(tp, 0x10, 0x0004, 0x0000); + else if (rtl_readphy(tp, 0x10) & 0x0004) rtl_w0w1_phy(tp, 0x10, 0x0000, 0x0004); rtl_writephy(tp, 0x1f, 0x0000); @@ -4233,6 +4265,15 @@ static void rtl8105e_hw_phy_config(struct rtl8169_private *tp) rtl_apply_firmware(tp); rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); + + /* Check ALDPS bit, disable it if enabled */ + rtl_writephy(tp, 0x1f, 0x0000); + if (enable_aldps) + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); + else if (rtl_readphy(tp, 0x18) & 0x1000) + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl8402_hw_phy_config(struct rtl8169_private *tp) @@ -4250,6 +4291,15 @@ static void rtl8402_hw_phy_config(struct rtl8169_private *tp) rtl_writephy(tp, 0x10, 0x401f); rtl_writephy(tp, 0x19, 0x7030); rtl_writephy(tp, 0x1f, 0x0000); + + /* Check ALDPS bit, disable it if enabled */ + rtl_writephy(tp, 0x1f, 0x0000); + if (enable_aldps) + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); + else if (rtl_readphy(tp, 0x18) & 0x1000) + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) @@ -4272,6 +4322,15 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC); + + /* Check ALDPS bit, disable it if enabled */ + rtl_writephy(tp, 0x1f, 0x0000); + if (enable_aldps) + rtl_w0w1_phy(tp, 0x18, 0x1000, 0x0000); + else if (rtl_readphy(tp, 0x18) & 0x1000) + rtl_w0w1_phy(tp, 0x18, 0x0000, 0x1000); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl_hw_phy_config(struct net_device *dev) @@ -5290,6 +5349,18 @@ static void rtl_pcie_state_l2l3_enable(struct rtl8169_private *tp, bool enable) RTL_W8(tp, Config3, data); } +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp, + bool enable) +{ + if (enable) { + RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn); + RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en); + } else { + RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); + RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + } +} + static void rtl_hw_start_8168bb(struct rtl8169_private *tp) { RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en); @@ -5646,9 +5717,10 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp) rtl_hw_start_8168g(tp); /* disable aspm and clock request before access ephy */ - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + rtl_hw_internal_aspm_clkreq_enable(tp, false); rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1)); + if (enable_aspm) + rtl_hw_internal_aspm_clkreq_enable(tp, true); } static void rtl_hw_start_8168g_2(struct rtl8169_private *tp) @@ -5681,9 +5753,10 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp) rtl_hw_start_8168g(tp); /* disable aspm and clock request before access ephy */ - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + rtl_hw_internal_aspm_clkreq_enable(tp, false); rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2)); + if (enable_aspm) + rtl_hw_internal_aspm_clkreq_enable(tp, true); } static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) @@ -5700,8 +5773,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) }; /* disable aspm and clock request before access ephy */ - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + rtl_hw_internal_aspm_clkreq_enable(tp, false); rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1)); RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO); @@ -5780,6 +5852,9 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp) r8168_mac_ocp_write(tp, 0xe63e, 0x0000); r8168_mac_ocp_write(tp, 0xc094, 0x0000); r8168_mac_ocp_write(tp, 0xc09e, 0x0000); + + if (enable_aspm) + rtl_hw_internal_aspm_clkreq_enable(tp, true); } static void rtl_hw_start_8168ep(struct rtl8169_private *tp) @@ -5831,11 +5906,13 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp) }; /* disable aspm and clock request before access ephy */ - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + rtl_hw_internal_aspm_clkreq_enable(tp, false); rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1)); rtl_hw_start_8168ep(tp); + + if (enable_aspm) + rtl_hw_internal_aspm_clkreq_enable(tp, true); } static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) @@ -5847,14 +5924,16 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp) }; /* disable aspm and clock request before access ephy */ - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + rtl_hw_internal_aspm_clkreq_enable(tp, false); rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2)); rtl_hw_start_8168ep(tp); RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN); RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN); + + if (enable_aspm) + rtl_hw_internal_aspm_clkreq_enable(tp, true); } static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) @@ -5868,8 +5947,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) }; /* disable aspm and clock request before access ephy */ - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn); - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); + rtl_hw_internal_aspm_clkreq_enable(tp, false); rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3)); rtl_hw_start_8168ep(tp); @@ -5889,6 +5967,9 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp) data = r8168_mac_ocp_read(tp, 0xe860); data |= 0x0080; r8168_mac_ocp_write(tp, 0xe860, data); + + if (enable_aspm) + rtl_hw_internal_aspm_clkreq_enable(tp, true); } static void rtl_hw_start_8168(struct rtl8169_private *tp) @@ -6364,7 +6445,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp) tp->cur_tx = tp->dirty_tx = 0; } -static void rtl_reset_work(struct rtl8169_private *tp) +static void _rtl_reset_work(struct rtl8169_private *tp) { struct net_device *dev = tp->dev; int i; @@ -6384,6 +6465,33 @@ static void rtl_reset_work(struct rtl8169_private *tp) napi_enable(&tp->napi); rtl_hw_start(tp); netif_wake_queue(dev); +} + +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); + } +} + +static void rtl_reset_work(struct rtl8169_private *tp) +{ + struct net_device *dev = tp->dev; + + _rtl_reset_work(tp); rtl8169_check_link_status(dev, tp); } @@ -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"); + } /* enable device (incl. PCI PM wakeup and hotplug setup) */ rc = pcim_enable_device(pdev);
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") Cc: Ryankao <ryankao@realtek.com> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/net/ethernet/realtek/r8169.c | 190 +++++++++++++++++++++------ 1 file changed, 151 insertions(+), 39 deletions(-)