* [PATCH net-next 1/2] r8169: enable ALDPS for power saving @ 2012-10-22 8:05 Hayes Wang 2012-10-22 17:59 ` David Miller ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Hayes Wang @ 2012-10-22 8:05 UTC (permalink / raw) To: romieu; +Cc: netdev, linux-kernel, jean, Hayes Wang Enable ALDPS function to save power when link down. Note that the feature should be set after the other PHY settings. And the firmware is necessary. Don't enable it without loading the firmware. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 46 +++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index e7ff886..ba29e4d 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -687,6 +687,7 @@ enum features { RTL_FEATURE_WOL = (1 << 0), RTL_FEATURE_MSI = (1 << 1), RTL_FEATURE_GMII = (1 << 2), + RTL_FEATURE_EXTENDED = (1 << 3), }; struct rtl8169_counters { @@ -2394,8 +2395,10 @@ static void rtl_apply_firmware(struct rtl8169_private *tp) struct rtl_fw *rtl_fw = tp->rtl_fw; /* TODO: release firmware once rtl_phy_write_fw signals failures. */ - if (!IS_ERR_OR_NULL(rtl_fw)) + if (!IS_ERR_OR_NULL(rtl_fw)) { rtl_phy_write_fw(tp, rtl_fw); + tp->features |= RTL_FEATURE_EXTENDED; + } } static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 val) @@ -3178,6 +3181,12 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); rtl_writephy(tp, 0x1f, 0x0000); + + /* ALDPS enable */ + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); + } } static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) @@ -3250,6 +3259,12 @@ static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) rtl_writephy(tp, 0x05, 0x8b85); rtl_w1w0_phy(tp, 0x06, 0x4000, 0x0000); rtl_writephy(tp, 0x1f, 0x0000); + + /* ALDPS enable */ + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); + } } static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp) @@ -3257,6 +3272,12 @@ static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp) rtl_apply_firmware(tp); rtl8168f_hw_phy_config(tp); + + /* ALDPS enable */ + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); + } } static void rtl8411_hw_phy_config(struct rtl8169_private *tp) @@ -3354,6 +3375,12 @@ static void rtl8411_hw_phy_config(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); rtl_writephy(tp, 0x1f, 0x0000); + + /* ALDPS enable */ + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); + } } static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) @@ -3446,6 +3473,11 @@ 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)); + + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x8310); + } } static void rtl8402_hw_phy_config(struct rtl8169_private *tp) @@ -3463,6 +3495,11 @@ 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); + + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x8310); + } } static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) @@ -3485,6 +3522,11 @@ 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); + + if (tp->features & RTL_FEATURE_EXTENDED) { + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x8310); + } } static void rtl_hw_phy_config(struct net_device *dev) @@ -6391,6 +6433,8 @@ static void rtl8169_net_suspend(struct net_device *dev) { struct rtl8169_private *tp = netdev_priv(dev); + tp->features &= ~RTL_FEATURE_EXTENDED; + if (!netif_running(dev)) return; -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving 2012-10-22 8:05 [PATCH net-next 1/2] r8169: enable ALDPS for power saving Hayes Wang @ 2012-10-22 17:59 ` David Miller 2012-10-22 19:27 ` Francois Romieu ` (2 subsequent siblings) 3 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2012-10-22 17:59 UTC (permalink / raw) To: hayeswang; +Cc: romieu, netdev, linux-kernel, jean From: Hayes Wang <hayeswang@realtek.com> Date: Mon, 22 Oct 2012 16:05:53 +0800 > Enable ALDPS function to save power when link down. Note that the > feature should be set after the other PHY settings. And the firmware > is necessary. Don't enable it without loading the firmware. > > Signed-off-by: Hayes Wang <hayeswang@realtek.com> If there are two patches in this series, which the Subject line implies, where is the second patch? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving 2012-10-22 8:05 [PATCH net-next 1/2] r8169: enable ALDPS for power saving Hayes Wang 2012-10-22 17:59 ` David Miller @ 2012-10-22 19:27 ` Francois Romieu 2012-10-23 2:24 ` hayeswang 2012-10-23 6:47 ` [PATCH v2 " Hayes Wang 2012-10-24 6:24 ` [PATCH v3 net-next] " Hayes Wang 3 siblings, 1 reply; 15+ messages in thread From: Francois Romieu @ 2012-10-22 19:27 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, linux-kernel, jean Hayes Wang <hayeswang@realtek.com> : > Enable ALDPS function to save power when link down. Note that the > feature should be set after the other PHY settings. And the firmware > is necessary. Don't enable it without loading the firmware. > > Signed-off-by: Hayes Wang <hayeswang@realtek.com> > --- > drivers/net/ethernet/realtek/r8169.c | 46 +++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index e7ff886..ba29e4d 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -687,6 +687,7 @@ enum features { > RTL_FEATURE_WOL = (1 << 0), > RTL_FEATURE_MSI = (1 << 1), > RTL_FEATURE_GMII = (1 << 2), > + RTL_FEATURE_EXTENDED = (1 << 3), Is there a specific reason why it is not named RTL_FEATURE_ALDPS ? RTL_FEATURE_EXTENDED is anything but enlightning. > }; > > struct rtl8169_counters { > @@ -2394,8 +2395,10 @@ static void rtl_apply_firmware(struct rtl8169_private *tp) > struct rtl_fw *rtl_fw = tp->rtl_fw; > > /* TODO: release firmware once rtl_phy_write_fw signals failures. */ > - if (!IS_ERR_OR_NULL(rtl_fw)) > + if (!IS_ERR_OR_NULL(rtl_fw)) { > rtl_phy_write_fw(tp, rtl_fw); > + tp->features |= RTL_FEATURE_EXTENDED; > + } > } > > static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 val) > @@ -3178,6 +3181,12 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp) > rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); > rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); > rtl_writephy(tp, 0x1f, 0x0000); > + > + /* ALDPS enable */ > + if (tp->features & RTL_FEATURE_EXTENDED) { > + rtl_writephy(tp, 0x1f, 0x0000); > + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); > + } This same code fragment will be repeated 3 extra times. The patch duplicates a different fragment 3 times and the driver already repeats the disable ALDPS sequence 3 times. What about some factoring out frenzy ? :o) [...] > @@ -6391,6 +6433,8 @@ static void rtl8169_net_suspend(struct net_device *dev) > { > struct rtl8169_private *tp = netdev_priv(dev); > > + tp->features &= ~RTL_FEATURE_EXTENDED; > + The commit message does not explain this part. What are you trying to achieve ? After this patch the driver would look like: 1. disable ALDPS before setting firmware (unmodified by patch) RTL_GIGA_MAC_VER_29 "RTL8105e" RTL_GIGA_MAC_VER_30 "RTL8105e" RTL_GIGA_MAC_VER_37 "RTL8402" RTL_GIGA_MAC_VER_39 "RTL8106e" 2. apply_firmware (unmodified by patch) RTL_GIGA_MAC_VER_25 "RTL8168d/8111d" RTL_GIGA_MAC_VER_26 "RTL8168d/8111d" RTL_GIGA_MAC_VER_29 "RTL8105e" RTL_GIGA_MAC_VER_30 "RTL8105e" RTL_GIGA_MAC_VER_32 "RTL8168e/8111e" RTL_GIGA_MAC_VER_33 "RTL8168e/8111e" RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl" RTL_GIGA_MAC_VER_35 "RTL8168f/8111f" RTL_GIGA_MAC_VER_36 "RTL8168f/8111f" RTL_GIGA_MAC_VER_37 "RTL8402" RTL_GIGA_MAC_VER_38 "RTL8411" RTL_GIGA_MAC_VER_39 "RTL8106e" RTL_GIGA_MAC_VER_40 "RTL8168g/8111g" 3. enable ALDPS after firmware RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl" RTL_GIGA_MAC_VER_35 "RTL8168f/8111f" RTL_GIGA_MAC_VER_36 "RTL8168f/8111f" RTL_GIGA_MAC_VER_37 "RTL8402" RTL_GIGA_MAC_VER_38 "RTL8411" RTL_GIGA_MAC_VER_39 "RTL8106e" RTL_GIGA_MAC_VER_40 "RTL8168g/8111g" The disable/enable ALDPS code is not trivially balanced. Do we exactly perform the required ALDPS operations ? Nothing more, nothing less ? -- Ueimor ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving 2012-10-22 19:27 ` Francois Romieu @ 2012-10-23 2:24 ` hayeswang 2012-10-23 19:31 ` Francois Romieu 0 siblings, 1 reply; 15+ messages in thread From: hayeswang @ 2012-10-23 2:24 UTC (permalink / raw) To: 'Francois Romieu'; +Cc: netdev, linux-kernel Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Tuesday, October 23, 2012 3:28 AM > To: Hayeswang > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > jean@google.com > Subject: Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving > [...] > > @@ -687,6 +687,7 @@ enum features { > > RTL_FEATURE_WOL = (1 << 0), > > RTL_FEATURE_MSI = (1 << 1), > > RTL_FEATURE_GMII = (1 << 2), > > + RTL_FEATURE_EXTENDED = (1 << 3), > > Is there a specific reason why it is not named RTL_FEATURE_ALDPS ? > > RTL_FEATURE_EXTENDED is anything but enlightning. > The flag is determined if the firmware is loaded. It doesn't mean to enable ALDPS. Besides ALDPS, the firmware includes the other features about power saving, performance, hw behavior, and so on. Thus, I think it is the extended feature. Any suggestion? [...] > > @@ -6391,6 +6433,8 @@ static void > rtl8169_net_suspend(struct net_device *dev) > > { > > struct rtl8169_private *tp = netdev_priv(dev); > > > > + tp->features &= ~RTL_FEATURE_EXTENDED; > > + > > The commit message does not explain this part. > > What are you trying to achieve ? I don't sure if the firmware code still exists and works after hibernation. I clear the flag for safe. It would be set again after loading firmware. It is used to make sure to enable ALDPS with firmware. > > After this patch the driver would look like: > 1. disable ALDPS before setting firmware (unmodified by patch) > RTL_GIGA_MAC_VER_29 "RTL8105e" > RTL_GIGA_MAC_VER_30 "RTL8105e" > RTL_GIGA_MAC_VER_37 "RTL8402" > RTL_GIGA_MAC_VER_39 "RTL8106e" > > 2. apply_firmware (unmodified by patch) > RTL_GIGA_MAC_VER_25 "RTL8168d/8111d" > RTL_GIGA_MAC_VER_26 "RTL8168d/8111d" > RTL_GIGA_MAC_VER_29 "RTL8105e" > RTL_GIGA_MAC_VER_30 "RTL8105e" > RTL_GIGA_MAC_VER_32 "RTL8168e/8111e" > RTL_GIGA_MAC_VER_33 "RTL8168e/8111e" > RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl" > RTL_GIGA_MAC_VER_35 "RTL8168f/8111f" > RTL_GIGA_MAC_VER_36 "RTL8168f/8111f" > RTL_GIGA_MAC_VER_37 "RTL8402" > RTL_GIGA_MAC_VER_38 "RTL8411" > RTL_GIGA_MAC_VER_39 "RTL8106e" > RTL_GIGA_MAC_VER_40 "RTL8168g/8111g" > > 3. enable ALDPS after firmware > RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl" > RTL_GIGA_MAC_VER_35 "RTL8168f/8111f" > RTL_GIGA_MAC_VER_36 "RTL8168f/8111f" > RTL_GIGA_MAC_VER_37 "RTL8402" > RTL_GIGA_MAC_VER_38 "RTL8411" > RTL_GIGA_MAC_VER_39 "RTL8106e" > RTL_GIGA_MAC_VER_40 "RTL8168g/8111g" > > The disable/enable ALDPS code is not trivially balanced. > > Do we exactly perform the required ALDPS operations ? Nothing more, > nothing less ? > Because the different design between the giga nic and 10/100M nic, the behavior would be different. For example, you couldn't disable the ALDPS of the giga nic directly just like the 10/100M (8136 series) one. The giga nic would disable ALDPS automatically when loading firmware, but the 8136 series wouldn't. Best Regards, Hayes ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving 2012-10-23 2:24 ` hayeswang @ 2012-10-23 19:31 ` Francois Romieu 2012-10-24 5:55 ` hayeswang 0 siblings, 1 reply; 15+ messages in thread From: Francois Romieu @ 2012-10-23 19:31 UTC (permalink / raw) To: hayeswang; +Cc: netdev, linux-kernel hayeswang <hayeswang@realtek.com> : [...] > The flag is determined if the firmware is loaded. It doesn't mean to enable > ALDPS. Besides ALDPS, the firmware includes the other features about power > saving, performance, hw behavior, and so on. Thus, I think it is the extended > feature. Any suggestion? RTL_FEATURE_FW_LOADED [...] > I don't sure if the firmware code still exists and works after hibernation. I > clear the flag for safe. It would be set again after loading firmware. It is > used to make sure to enable ALDPS with firmware. I don't understand if you want to avoid a coding bug or a hardware problem. The code only enables ALDPS in the hw_phy_config helpers. Said helpers are the only places where firmware loading is requested. Loading the firmware always set RTL_FEATURE_XYZ. Whatever you are trying to achieve, the change in rtl8169_net_suspend seem rather useless. Please explain. (if you want to enforce that ALDPS does nothing when it is not called right after firmware loading, the ALDPS enable method itself is imho the natural place to reset the bit). [...] > > After this patch the driver would look like: > > 1. disable ALDPS before setting firmware (unmodified by patch) > > RTL_GIGA_MAC_VER_29 "RTL8105e" > > RTL_GIGA_MAC_VER_30 "RTL8105e" > > RTL_GIGA_MAC_VER_37 "RTL8402" > > RTL_GIGA_MAC_VER_39 "RTL8106e" > > > > 2. apply_firmware (unmodified by patch) > > RTL_GIGA_MAC_VER_25 "RTL8168d/8111d" > > RTL_GIGA_MAC_VER_26 "RTL8168d/8111d" > > RTL_GIGA_MAC_VER_29 "RTL8105e" > > RTL_GIGA_MAC_VER_30 "RTL8105e" > > RTL_GIGA_MAC_VER_32 "RTL8168e/8111e" > > RTL_GIGA_MAC_VER_33 "RTL8168e/8111e" > > RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl" > > RTL_GIGA_MAC_VER_35 "RTL8168f/8111f" > > RTL_GIGA_MAC_VER_36 "RTL8168f/8111f" > > RTL_GIGA_MAC_VER_37 "RTL8402" > > RTL_GIGA_MAC_VER_38 "RTL8411" > > RTL_GIGA_MAC_VER_39 "RTL8106e" > > RTL_GIGA_MAC_VER_40 "RTL8168g/8111g" > > > > 3. enable ALDPS after firmware > > RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl" > > RTL_GIGA_MAC_VER_35 "RTL8168f/8111f" > > RTL_GIGA_MAC_VER_36 "RTL8168f/8111f" > > RTL_GIGA_MAC_VER_37 "RTL8402" > > RTL_GIGA_MAC_VER_38 "RTL8411" > > RTL_GIGA_MAC_VER_39 "RTL8106e" > > RTL_GIGA_MAC_VER_40 "RTL8168g/8111g" > > > > The disable/enable ALDPS code is not trivially balanced. > > > > Do we exactly perform the required ALDPS operations ? Nothing more, > > nothing less ? > > > > Because the different design between the giga nic and 10/100M nic, the behavior > would be different. For example, you couldn't disable the ALDPS of the giga nic > directly just like the 10/100M (8136 series) one. The giga nic would disable > ALDPS automatically when loading firmware, but the 8136 series wouldn't. It would be nice to state these things in the commit message, namely: - ALDPS should never be enabled for the RTL8105e - none of the firmware-free chipsets support ALDPS - neither do the RTL8168d/8111d -- Ueimor ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving 2012-10-23 19:31 ` Francois Romieu @ 2012-10-24 5:55 ` hayeswang 2012-10-24 20:05 ` Francois Romieu 0 siblings, 1 reply; 15+ messages in thread From: hayeswang @ 2012-10-24 5:55 UTC (permalink / raw) To: 'Francois Romieu'; +Cc: netdev, linux-kernel Francois Romieu [mailto:romieu@fr.zoreil.com] [...] > It would be nice to state these things in the commit message, namely: > - ALDPS should never be enabled for the RTL8105e > - none of the firmware-free chipsets support ALDPS > - neither do the RTL8168d/8111d Excuse me. I don't understand why the RTL8105e shouldn't enable ALDPS. Best Regards, Hayes ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving 2012-10-24 5:55 ` hayeswang @ 2012-10-24 20:05 ` Francois Romieu 0 siblings, 0 replies; 15+ messages in thread From: Francois Romieu @ 2012-10-24 20:05 UTC (permalink / raw) To: hayeswang; +Cc: netdev, linux-kernel hayeswang <hayeswang@realtek.com> : > Francois Romieu [mailto:romieu@fr.zoreil.com] > [...] > > It would be nice to state these things in the commit message, namely: > > - ALDPS should never be enabled for the RTL8105e > > - none of the firmware-free chipsets support ALDPS > > - neither do the RTL8168d/8111d > > Excuse me. I don't understand why the RTL8105e shouldn't enable ALDPS. I'd appreciate to see such a statement if ALDPS can be enabled with the RTL8105e ! You have sent ALDPS related patches for some specific chipsets. Now would be a sensible time to check if ALDPS is correctly handled for _all_ supported chipsets. I don't have access to your authoritative resources. I can only outline something strange in the code wrt ALDPS and the 8105e. /me returns to 8169b RxOverflow oddities... -- Ueimor ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving 2012-10-22 8:05 [PATCH net-next 1/2] r8169: enable ALDPS for power saving Hayes Wang 2012-10-22 17:59 ` David Miller 2012-10-22 19:27 ` Francois Romieu @ 2012-10-23 6:47 ` Hayes Wang 2012-10-23 6:47 ` [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G Hayes Wang 2012-10-23 19:33 ` [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving Francois Romieu 2012-10-24 6:24 ` [PATCH v3 net-next] " Hayes Wang 3 siblings, 2 replies; 15+ messages in thread From: Hayes Wang @ 2012-10-23 6:47 UTC (permalink / raw) To: romieu; +Cc: netdev, linux-kernel, Hayes Wang Enable ALDPS function to save power when link down. Note that the feature should be set after the other PHY settings. And the firmware is necessary. Don't enable it without loading the firmware. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 62 ++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index e7ff886..cdd46de 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -687,6 +687,7 @@ enum features { RTL_FEATURE_WOL = (1 << 0), RTL_FEATURE_MSI = (1 << 1), RTL_FEATURE_GMII = (1 << 2), + RTL_FEATURE_EXTENDED = (1 << 3), }; struct rtl8169_counters { @@ -2394,8 +2395,10 @@ static void rtl_apply_firmware(struct rtl8169_private *tp) struct rtl_fw *rtl_fw = tp->rtl_fw; /* TODO: release firmware once rtl_phy_write_fw signals failures. */ - if (!IS_ERR_OR_NULL(rtl_fw)) + if (!IS_ERR_OR_NULL(rtl_fw)) { rtl_phy_write_fw(tp, rtl_fw); + tp->features |= RTL_FEATURE_EXTENDED; + } } static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 val) @@ -2406,6 +2409,31 @@ static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 val) rtl_apply_firmware(tp); } +static void r810x_aldps_disable(struct rtl8169_private *tp) +{ + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x0310); + msleep(100); +} + +static void r810x_aldps_enable(struct rtl8169_private *tp) +{ + if (!(tp->features & RTL_FEATURE_EXTENDED)) + return; + + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x8310); +} + +static void r8168_aldps_enable_1(struct rtl8169_private *tp) +{ + if (!(tp->features & RTL_FEATURE_EXTENDED)) + return; + + rtl_writephy(tp, 0x1f, 0x0000); + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); +} + static void rtl8169s_hw_phy_config(struct rtl8169_private *tp) { static const struct phy_reg phy_reg_init[] = { @@ -3178,6 +3206,9 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); rtl_writephy(tp, 0x1f, 0x0000); + + /* ALDPS enable */ + r8168_aldps_enable_1(tp); } static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) @@ -3250,6 +3281,9 @@ static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) rtl_writephy(tp, 0x05, 0x8b85); rtl_w1w0_phy(tp, 0x06, 0x4000, 0x0000); rtl_writephy(tp, 0x1f, 0x0000); + + /* ALDPS enable */ + r8168_aldps_enable_1(tp); } static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp) @@ -3257,6 +3291,9 @@ static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp) rtl_apply_firmware(tp); rtl8168f_hw_phy_config(tp); + + /* ALDPS enable */ + r8168_aldps_enable_1(tp); } static void rtl8411_hw_phy_config(struct rtl8169_private *tp) @@ -3354,6 +3391,9 @@ static void rtl8411_hw_phy_config(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); rtl_writephy(tp, 0x1f, 0x0000); + + /* ALDPS enable */ + r8168_aldps_enable_1(tp); } static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) @@ -3439,21 +3479,19 @@ static void rtl8105e_hw_phy_config(struct rtl8169_private *tp) }; /* Disable ALDPS before ram code */ - rtl_writephy(tp, 0x1f, 0x0000); - rtl_writephy(tp, 0x18, 0x0310); - msleep(100); + r810x_aldps_disable(tp); rtl_apply_firmware(tp); rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); + + r810x_aldps_enable(tp); } static void rtl8402_hw_phy_config(struct rtl8169_private *tp) { /* Disable ALDPS before setting firmware */ - rtl_writephy(tp, 0x1f, 0x0000); - rtl_writephy(tp, 0x18, 0x0310); - msleep(20); + r810x_aldps_disable(tp); rtl_apply_firmware(tp); @@ -3463,6 +3501,8 @@ 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); + + r810x_aldps_enable(tp); } static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) @@ -3475,9 +3515,7 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) }; /* Disable ALDPS before ram code */ - rtl_writephy(tp, 0x1f, 0x0000); - rtl_writephy(tp, 0x18, 0x0310); - msleep(100); + r810x_aldps_disable(tp); rtl_apply_firmware(tp); @@ -3485,6 +3523,8 @@ 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); + + r810x_aldps_enable(tp); } static void rtl_hw_phy_config(struct net_device *dev) @@ -6391,6 +6431,8 @@ static void rtl8169_net_suspend(struct net_device *dev) { struct rtl8169_private *tp = netdev_priv(dev); + tp->features &= ~RTL_FEATURE_EXTENDED; + if (!netif_running(dev)) return; -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G 2012-10-23 6:47 ` [PATCH v2 " Hayes Wang @ 2012-10-23 6:47 ` Hayes Wang 2012-10-23 19:41 ` Francois Romieu 2012-10-23 19:33 ` [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving Francois Romieu 1 sibling, 1 reply; 15+ messages in thread From: Hayes Wang @ 2012-10-23 6:47 UTC (permalink / raw) To: romieu; +Cc: netdev, linux-kernel, Hayes Wang Update the parameters of RTL8111G Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 118 ++++++++++++++++++++++++++++++----- 1 file changed, 101 insertions(+), 17 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index cdd46de..3a393f7 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -335,6 +335,7 @@ enum rtl_registers { #define RXCFG_FIFO_SHIFT 13 /* No threshold before first PCI xfer */ #define RX_FIFO_THRESH (7 << RXCFG_FIFO_SHIFT) +#define RX_EARLY_OFF (1 << 11) #define RXCFG_DMA_SHIFT 8 /* Unlimited maximum PCI burst. */ #define RX_DMA_BURST (7 << RXCFG_DMA_SHIFT) @@ -2434,6 +2435,15 @@ static void r8168_aldps_enable_1(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); } +static void r8168_aldps_enable_2(struct rtl8169_private *tp) +{ + if (!(tp->features & RTL_FEATURE_EXTENDED)) + return; + + rtl_writephy(tp, 0x1f, 0x0a43); + rtl_w1w0_phy(tp, 0x10, 0x0004, 0x0000); +} + static void rtl8169s_hw_phy_config(struct rtl8169_private *tp) { static const struct phy_reg phy_reg_init[] = { @@ -3400,29 +3410,57 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) { static const u16 mac_ocp_patch[] = { 0xe008, 0xe01b, 0xe01d, 0xe01f, - 0xe021, 0xe023, 0xe025, 0xe027, + 0xe022, 0xe025, 0xe031, 0xe04d, 0x49d2, 0xf10d, 0x766c, 0x49e2, 0xf00a, 0x1ec0, 0x8ee1, 0xc60a, - 0x77c0, 0x4870, 0x9fc0, 0x1ea0, 0xc707, 0x8ee1, 0x9d6c, 0xc603, 0xbe00, 0xb416, 0x0076, 0xe86c, - 0xc602, 0xbe00, 0x0000, 0xc602, - - 0xbe00, 0x0000, 0xc602, 0xbe00, - 0x0000, 0xc602, 0xbe00, 0x0000, - 0xc602, 0xbe00, 0x0000, 0xc602, - 0xbe00, 0x0000, 0xc602, 0xbe00, - - 0x0000, 0x0000, 0x0000, 0x0000 + 0xc602, 0xbe00, 0xa000, 0xc602, + 0xbe00, 0x0000, 0x1b76, 0xc202, + 0xba00, 0x059c, 0x1b76, 0xc602, + 0xbe00, 0x065a, 0x74e6, 0x1b78, + 0x46dc, 0x1300, 0xf005, 0x74f8, + 0x48c3, 0x48c4, 0x8cf8, 0x64e7, + 0xc302, 0xbb00, 0x06a0, 0x74e4, + 0x49c5, 0xf106, 0x49c6, 0xf107, + 0x48c8, 0x48c9, 0xe011, 0x48c9, + 0x4848, 0xe00e, 0x4848, 0x49c7, + 0xf00a, 0x48c9, 0xc60d, 0x1d1f, + 0x8dc2, 0x1d00, 0x8dc3, 0x1d11, + 0x8dc0, 0xe002, 0x4849, 0x94e5, + 0xc602, 0xbe00, 0x01f0, 0xe434, + 0x49d9, 0xf01b, 0xc31e, 0x7464, + 0x49c4, 0xf114, 0xc31b, 0x6460, + 0x14fa, 0xfa02, 0xe00f, 0xc317, + 0x7460, 0x49c0, 0xf10b, 0xc311, + 0x7462, 0x48c1, 0x9c62, 0x4841, + 0x9c62, 0xc30a, 0x1c04, 0x8c60, + 0xe004, 0x1c15, 0xc305, 0x8c60, + 0xc602, 0xbe00, 0x0384, 0xe434, + 0xe030, 0xe61c, 0xe906 }; u32 i; /* Patch code for GPHY reset */ + r8168_mac_ocp_write(tp, 0xfc26, 0x0000); + r8168_mac_ocp_write(tp, 0xfc28, 0x0000); + r8168_mac_ocp_write(tp, 0xfc2a, 0x0000); + r8168_mac_ocp_write(tp, 0xfc2c, 0x0000); + r8168_mac_ocp_write(tp, 0xfc2e, 0x0000); + r8168_mac_ocp_write(tp, 0xfc30, 0x0000); + r8168_mac_ocp_write(tp, 0xfc32, 0x0000); + r8168_mac_ocp_write(tp, 0xfc34, 0x0000); + r8168_mac_ocp_write(tp, 0xfc36, 0x0000); for (i = 0; i < ARRAY_SIZE(mac_ocp_patch); i++) - r8168_mac_ocp_write(tp, 0xf800 + 2*i, mac_ocp_patch[i]); + r8168_mac_ocp_write(tp, 0xf800 + 2 * i, mac_ocp_patch[i]); r8168_mac_ocp_write(tp, 0xfc26, 0x8000); r8168_mac_ocp_write(tp, 0xfc28, 0x0075); + r8168_mac_ocp_write(tp, 0xfc2e, 0x059b); + r8168_mac_ocp_write(tp, 0xfc30, 0x0659); + r8168_mac_ocp_write(tp, 0xfc32, 0x069f); + r8168_mac_ocp_write(tp, 0xfc34, 0x01cd); + r8168_mac_ocp_write(tp, 0xfc36, 0x0303); rtl_apply_firmware(tp); @@ -3436,13 +3474,46 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) else rtl_w1w0_phy_ocp(tp, 0xbcc4, 0x0000, 0x0002); - rtl_w1w0_phy_ocp(tp, 0xa442, 0x000c, 0x0000); - rtl_w1w0_phy_ocp(tp, 0xa4b2, 0x0004, 0x0000); + /* Enable PHY auto speed down */ + rtl_writephy(tp, 0x1f, 0x0a44); + rtl_w1w0_phy(tp, 0x11, 0x000c, 0x0000); + + rtl_writephy(tp, 0x1f, 0x0bcc); + rtl_w1w0_phy(tp, 0x14, 0x0100, 0x0000); + rtl_writephy(tp, 0x1f, 0x0a44); + rtl_w1w0_phy(tp, 0x11, 0x00c0, 0x0000); + rtl_writephy(tp, 0x1f, 0x0a43); + rtl_writephy(tp, 0x13, 0x8084); + rtl_w1w0_phy(tp, 0x14, 0x0000, 0x6000); + rtl_w1w0_phy(tp, 0x10, 0x1003, 0x0000); + + /* EEE auto-fallback function */ + rtl_writephy(tp, 0x1f, 0x0a4b); + rtl_w1w0_phy(tp, 0x11, 0x0004, 0x0000); + + /* Enable UC LPF tune function */ + rtl_writephy(tp, 0x1f, 0x0a43); + rtl_writephy(tp, 0x13, 0x8012); + rtl_w1w0_phy(tp, 0x14, 0x8000, 0x0000); + + rtl_writephy(tp, 0x1f, 0x0c42); + rtl_w1w0_phy(tp, 0x11, 0x4000, 0x2000); - r8168_phy_ocp_write(tp, 0xa436, 0x8012); - rtl_w1w0_phy_ocp(tp, 0xa438, 0x8000, 0x0000); + /* Improve SWR Efficiency */ + rtl_writephy(tp, 0x1f, 0x0bcd); + rtl_writephy(tp, 0x14, 0x5065); + rtl_writephy(tp, 0x14, 0xd065); + rtl_writephy(tp, 0x1f, 0x0bc8); + rtl_writephy(tp, 0x11, 0x5655); + rtl_writephy(tp, 0x1f, 0x0bcd); + rtl_writephy(tp, 0x14, 0x1065); + rtl_writephy(tp, 0x14, 0x9065); + rtl_writephy(tp, 0x14, 0x1065); - rtl_w1w0_phy_ocp(tp, 0xc422, 0x4000, 0x2000); + /* ALDPS enable */ + r8168_aldps_enable_2(tp); + + rtl_writephy(tp, 0x1f, 0x0000); } static void rtl8102e_hw_phy_config(struct rtl8169_private *tp) @@ -4047,6 +4118,10 @@ static void r8168_pll_power_down(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_33: RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80); break; + case RTL_GIGA_MAC_VER_40: + case RTL_GIGA_MAC_VER_41: + RTL_W8(PMCH, RTL_R8(PMCH) & ~0x40); + break; } } @@ -4064,6 +4139,10 @@ static void r8168_pll_power_up(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_33: RTL_W8(PMCH, RTL_R8(PMCH) | 0x80); break; + case RTL_GIGA_MAC_VER_40: + case RTL_GIGA_MAC_VER_41: + RTL_W8(PMCH, RTL_R8(PMCH) | 0x40); + break; } r8168_phy_power_up(tp); @@ -4169,6 +4248,10 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_34: RTL_W32(RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST); break; + case RTL_GIGA_MAC_VER_40: + case RTL_GIGA_MAC_VER_41: + RTL_W32(RxConfig, RX128_INT_EN | RX_DMA_BURST | RX_EARLY_OFF); + break; default: RTL_W32(RxConfig, RX128_INT_EN | RX_DMA_BURST); break; @@ -5157,7 +5240,8 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp) /* Adjust EEE LED frequency */ RTL_W8(EEE_LED, RTL_R8(EEE_LED) & ~0x07); - rtl_w1w0_eri(tp, 0x2fc, ERIAR_MASK_0001, 0x01, 0x02, ERIAR_EXGMAC); + rtl_w1w0_eri(tp, 0x2fc, ERIAR_MASK_0001, 0x01, 0x06, ERIAR_EXGMAC); + rtl_w1w0_eri(tp, 0x1b0, ERIAR_MASK_0011, 0x0000, 0x1000, ERIAR_EXGMAC); } static void rtl_hw_start_8168(struct net_device *dev) -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G 2012-10-23 6:47 ` [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G Hayes Wang @ 2012-10-23 19:41 ` Francois Romieu 0 siblings, 0 replies; 15+ messages in thread From: Francois Romieu @ 2012-10-23 19:41 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, linux-kernel Hayes Wang <hayeswang@realtek.com> : > Update the parameters of RTL8111G I am Jack's Broken Heart. :o/ In a not too distant future somebody may try to figure if things should be fed into one of the numerous -stable trees. He will appreciate figuring from the comment if this commit is supposed to fix anything or if it's a pure performance thing or whatever. The pll_power changes could be a bugfix for instance. The ALDPS change isn't. > Signed-off-by: Hayes Wang <hayeswang@realtek.com> > --- > drivers/net/ethernet/realtek/r8169.c | 118 ++++++++++++++++++++++++++++++----- > 1 file changed, 101 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index cdd46de..3a393f7 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > @@ -3400,29 +3410,57 @@ static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) > { > static const u16 mac_ocp_patch[] = { > 0xe008, 0xe01b, 0xe01d, 0xe01f, > - 0xe021, 0xe023, 0xe025, 0xe027, > + 0xe022, 0xe025, 0xe031, 0xe04d, > 0x49d2, 0xf10d, 0x766c, 0x49e2, > 0xf00a, 0x1ec0, 0x8ee1, 0xc60a, > - > 0x77c0, 0x4870, 0x9fc0, 0x1ea0, > 0xc707, 0x8ee1, 0x9d6c, 0xc603, > 0xbe00, 0xb416, 0x0076, 0xe86c, > - 0xc602, 0xbe00, 0x0000, 0xc602, > - > - 0xbe00, 0x0000, 0xc602, 0xbe00, > - 0x0000, 0xc602, 0xbe00, 0x0000, > - 0xc602, 0xbe00, 0x0000, 0xc602, > - 0xbe00, 0x0000, 0xc602, 0xbe00, > - > - 0x0000, 0x0000, 0x0000, 0x0000 > + 0xc602, 0xbe00, 0xa000, 0xc602, > + 0xbe00, 0x0000, 0x1b76, 0xc202, > + 0xba00, 0x059c, 0x1b76, 0xc602, > + 0xbe00, 0x065a, 0x74e6, 0x1b78, > + 0x46dc, 0x1300, 0xf005, 0x74f8, > + 0x48c3, 0x48c4, 0x8cf8, 0x64e7, > + 0xc302, 0xbb00, 0x06a0, 0x74e4, > + 0x49c5, 0xf106, 0x49c6, 0xf107, > + 0x48c8, 0x48c9, 0xe011, 0x48c9, > + 0x4848, 0xe00e, 0x4848, 0x49c7, > + 0xf00a, 0x48c9, 0xc60d, 0x1d1f, > + 0x8dc2, 0x1d00, 0x8dc3, 0x1d11, > + 0x8dc0, 0xe002, 0x4849, 0x94e5, > + 0xc602, 0xbe00, 0x01f0, 0xe434, > + 0x49d9, 0xf01b, 0xc31e, 0x7464, > + 0x49c4, 0xf114, 0xc31b, 0x6460, > + 0x14fa, 0xfa02, 0xe00f, 0xc317, > + 0x7460, 0x49c0, 0xf10b, 0xc311, > + 0x7462, 0x48c1, 0x9c62, 0x4841, > + 0x9c62, 0xc30a, 0x1c04, 0x8c60, > + 0xe004, 0x1c15, 0xc305, 0x8c60, > + 0xc602, 0xbe00, 0x0384, 0xe434, > + 0xe030, 0xe61c, 0xe906 > }; Do you remember why firmware loading was introduced in the first place ? I find it increasingly hard to believe that it does not apply here. [...] > /* Patch code for GPHY reset */ > + r8168_mac_ocp_write(tp, 0xfc26, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc28, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc2a, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc2c, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc2e, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc30, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc32, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc34, 0x0000); > + r8168_mac_ocp_write(tp, 0xfc36, 0x0000); > for (i = 0; i < ARRAY_SIZE(mac_ocp_patch); i++) > - r8168_mac_ocp_write(tp, 0xf800 + 2*i, mac_ocp_patch[i]); > + r8168_mac_ocp_write(tp, 0xf800 + 2 * i, mac_ocp_patch[i]); > r8168_mac_ocp_write(tp, 0xfc26, 0x8000); > r8168_mac_ocp_write(tp, 0xfc28, 0x0075); > + r8168_mac_ocp_write(tp, 0xfc2e, 0x059b); > + r8168_mac_ocp_write(tp, 0xfc30, 0x0659); > + r8168_mac_ocp_write(tp, 0xfc32, 0x069f); > + r8168_mac_ocp_write(tp, 0xfc34, 0x01cd); > + r8168_mac_ocp_write(tp, 0xfc36, 0x0303); Within a few weeks / months there will be a new update with a new set of completely obscure parameters. It will takes a kernel cycle or more to be distributed. There should be some higher level data struct that could use a mundane "for" loop and some indirection in place of this code bloat. It is getting out of control. -- Ueimor ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving 2012-10-23 6:47 ` [PATCH v2 " Hayes Wang 2012-10-23 6:47 ` [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G Hayes Wang @ 2012-10-23 19:33 ` Francois Romieu 2012-10-24 5:55 ` hayeswang 1 sibling, 1 reply; 15+ messages in thread From: Francois Romieu @ 2012-10-23 19:33 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, linux-kernel Hayes Wang <hayeswang@realtek.com> : > Enable ALDPS function to save power when link down. Note that the > feature should be set after the other PHY settings. And the firmware > is necessary. Don't enable it without loading the firmware. > > Signed-off-by: Hayes Wang <hayeswang@realtek.com> [...] Please see my just sent answer in yesterday's thread. > +static void r810x_aldps_disable(struct rtl8169_private *tp) > +{ > + rtl_writephy(tp, 0x1f, 0x0000); > + rtl_writephy(tp, 0x18, 0x0310); > + msleep(100); > +} rtl8402_hw_phy_config used a msleep(20). Meguesses it won't hurt, right ? [...] > + > + /* ALDPS enable */ > + r8168_aldps_enable_1(tp); The functions are literate enough: you can remove the comment. [...] > @@ -6391,6 +6431,8 @@ static void rtl8169_net_suspend(struct net_device *dev) > { > struct rtl8169_private *tp = netdev_priv(dev); > > + tp->features &= ~RTL_FEATURE_EXTENDED; > + > if (!netif_running(dev)) > return; No (as previously stated). -- Ueimor ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving 2012-10-23 19:33 ` [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving Francois Romieu @ 2012-10-24 5:55 ` hayeswang 0 siblings, 0 replies; 15+ messages in thread From: hayeswang @ 2012-10-24 5:55 UTC (permalink / raw) To: 'Francois Romieu'; +Cc: netdev, linux-kernel Francois Romieu [mailto:romieu@fr.zoreil.com] [...] > > +static void r810x_aldps_disable(struct rtl8169_private *tp) > > +{ > > + rtl_writephy(tp, 0x1f, 0x0000); > > + rtl_writephy(tp, 0x18, 0x0310); > > + msleep(100); > > +} > > rtl8402_hw_phy_config used a msleep(20). Meguesses it won't > hurt, right ? No, it won't hurt. The delay make suer there is enough time to pause ALDPS. Best Regards, Hayes ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 net-next] r8169: enable ALDPS for power saving 2012-10-22 8:05 [PATCH net-next 1/2] r8169: enable ALDPS for power saving Hayes Wang ` (2 preceding siblings ...) 2012-10-23 6:47 ` [PATCH v2 " Hayes Wang @ 2012-10-24 6:24 ` Hayes Wang 2012-10-24 21:20 ` Francois Romieu 3 siblings, 1 reply; 15+ messages in thread From: Hayes Wang @ 2012-10-24 6:24 UTC (permalink / raw) To: romieu; +Cc: netdev, linux-kernel, jean, Hayes Wang Enable ALDPS function to save power when link down. Note that the feature should be set after the other PHY settings. And the firmware is necessary. Don't enable it without loading the firmware. None of the firmware-free chipsets support ALDPS. Neither do the RTL8168d/8111d. For 8136 series, make sure the ALDPS is disabled before loading the firmware. For 8168 series, the ALDPS would be disabled automatically when loading firmware. You must not disable it directly. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 56 +++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index e7ff886..2317b8c 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -687,6 +687,7 @@ enum features { RTL_FEATURE_WOL = (1 << 0), RTL_FEATURE_MSI = (1 << 1), RTL_FEATURE_GMII = (1 << 2), + RTL_FEATURE_FW_LOADED = (1 << 3), }; struct rtl8169_counters { @@ -2394,8 +2395,10 @@ static void rtl_apply_firmware(struct rtl8169_private *tp) struct rtl_fw *rtl_fw = tp->rtl_fw; /* TODO: release firmware once rtl_phy_write_fw signals failures. */ - if (!IS_ERR_OR_NULL(rtl_fw)) + if (!IS_ERR_OR_NULL(rtl_fw)) { rtl_phy_write_fw(tp, rtl_fw); + tp->features |= RTL_FEATURE_FW_LOADED; + } } static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 val) @@ -2406,6 +2409,31 @@ static void rtl_apply_firmware_cond(struct rtl8169_private *tp, u8 reg, u16 val) rtl_apply_firmware(tp); } +static void r810x_aldps_disable(struct rtl8169_private *tp) +{ + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x0310); + msleep(100); +} + +static void r810x_aldps_enable(struct rtl8169_private *tp) +{ + if (!(tp->features & RTL_FEATURE_FW_LOADED)) + return; + + rtl_writephy(tp, 0x1f, 0x0000); + rtl_writephy(tp, 0x18, 0x8310); +} + +static void r8168_aldps_enable_1(struct rtl8169_private *tp) +{ + if (!(tp->features & RTL_FEATURE_FW_LOADED)) + return; + + rtl_writephy(tp, 0x1f, 0x0000); + rtl_w1w0_phy(tp, 0x15, 0x1000, 0x0000); +} + static void rtl8169s_hw_phy_config(struct rtl8169_private *tp) { static const struct phy_reg phy_reg_init[] = { @@ -3178,6 +3206,8 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); rtl_writephy(tp, 0x1f, 0x0000); + + r8168_aldps_enable_1(tp); } static void rtl8168f_hw_phy_config(struct rtl8169_private *tp) @@ -3250,6 +3280,8 @@ static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp) rtl_writephy(tp, 0x05, 0x8b85); rtl_w1w0_phy(tp, 0x06, 0x4000, 0x0000); rtl_writephy(tp, 0x1f, 0x0000); + + r8168_aldps_enable_1(tp); } static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp) @@ -3257,6 +3289,8 @@ static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp) rtl_apply_firmware(tp); rtl8168f_hw_phy_config(tp); + + r8168_aldps_enable_1(tp); } static void rtl8411_hw_phy_config(struct rtl8169_private *tp) @@ -3354,6 +3388,8 @@ static void rtl8411_hw_phy_config(struct rtl8169_private *tp) rtl_w1w0_phy(tp, 0x19, 0x0000, 0x0001); rtl_w1w0_phy(tp, 0x10, 0x0000, 0x0400); rtl_writephy(tp, 0x1f, 0x0000); + + r8168_aldps_enable_1(tp); } static void rtl8168g_1_hw_phy_config(struct rtl8169_private *tp) @@ -3439,21 +3475,19 @@ static void rtl8105e_hw_phy_config(struct rtl8169_private *tp) }; /* Disable ALDPS before ram code */ - rtl_writephy(tp, 0x1f, 0x0000); - rtl_writephy(tp, 0x18, 0x0310); - msleep(100); + r810x_aldps_disable(tp); rtl_apply_firmware(tp); rtl_writephy_batch(tp, phy_reg_init, ARRAY_SIZE(phy_reg_init)); + + r810x_aldps_enable(tp); } static void rtl8402_hw_phy_config(struct rtl8169_private *tp) { /* Disable ALDPS before setting firmware */ - rtl_writephy(tp, 0x1f, 0x0000); - rtl_writephy(tp, 0x18, 0x0310); - msleep(20); + r810x_aldps_disable(tp); rtl_apply_firmware(tp); @@ -3463,6 +3497,8 @@ 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); + + r810x_aldps_enable(tp); } static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) @@ -3475,9 +3511,7 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private *tp) }; /* Disable ALDPS before ram code */ - rtl_writephy(tp, 0x1f, 0x0000); - rtl_writephy(tp, 0x18, 0x0310); - msleep(100); + r810x_aldps_disable(tp); rtl_apply_firmware(tp); @@ -3485,6 +3519,8 @@ 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); + + r810x_aldps_enable(tp); } static void rtl_hw_phy_config(struct net_device *dev) -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 net-next] r8169: enable ALDPS for power saving 2012-10-24 6:24 ` [PATCH v3 net-next] " Hayes Wang @ 2012-10-24 21:20 ` Francois Romieu 2012-10-26 6:15 ` David Miller 0 siblings, 1 reply; 15+ messages in thread From: Francois Romieu @ 2012-10-24 21:20 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, linux-kernel, jean Hayes Wang <hayeswang@realtek.com> : > Enable ALDPS function to save power when link down. Note that the > feature should be set after the other PHY settings. And the firmware > is necessary. Don't enable it without loading the firmware. > > None of the firmware-free chipsets support ALDPS. Neither do the > RTL8168d/8111d. > > For 8136 series, make sure the ALDPS is disabled before loading the > firmware. For 8168 series, the ALDPS would be disabled automatically > when loading firmware. You must not disable it directly. > > Signed-off-by: Hayes Wang <hayeswang@realtek.com> Acked-by: Francois Romieu <romieu@fr.zoreil.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 net-next] r8169: enable ALDPS for power saving 2012-10-24 21:20 ` Francois Romieu @ 2012-10-26 6:15 ` David Miller 0 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2012-10-26 6:15 UTC (permalink / raw) To: romieu; +Cc: hayeswang, netdev, linux-kernel, jean From: Francois Romieu <romieu@fr.zoreil.com> Date: Wed, 24 Oct 2012 23:20:03 +0200 > Hayes Wang <hayeswang@realtek.com> : >> Enable ALDPS function to save power when link down. Note that the >> feature should be set after the other PHY settings. And the firmware >> is necessary. Don't enable it without loading the firmware. >> >> None of the firmware-free chipsets support ALDPS. Neither do the >> RTL8168d/8111d. >> >> For 8136 series, make sure the ALDPS is disabled before loading the >> firmware. For 8168 series, the ALDPS would be disabled automatically >> when loading firmware. You must not disable it directly. >> >> Signed-off-by: Hayes Wang <hayeswang@realtek.com> > > Acked-by: Francois Romieu <romieu@fr.zoreil.com> Applied, thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-10-26 6:15 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-10-22 8:05 [PATCH net-next 1/2] r8169: enable ALDPS for power saving Hayes Wang 2012-10-22 17:59 ` David Miller 2012-10-22 19:27 ` Francois Romieu 2012-10-23 2:24 ` hayeswang 2012-10-23 19:31 ` Francois Romieu 2012-10-24 5:55 ` hayeswang 2012-10-24 20:05 ` Francois Romieu 2012-10-23 6:47 ` [PATCH v2 " Hayes Wang 2012-10-23 6:47 ` [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G Hayes Wang 2012-10-23 19:41 ` Francois Romieu 2012-10-23 19:33 ` [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving Francois Romieu 2012-10-24 5:55 ` hayeswang 2012-10-24 6:24 ` [PATCH v3 net-next] " Hayes Wang 2012-10-24 21:20 ` Francois Romieu 2012-10-26 6:15 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).