| Message ID | 1350893153-18320-1-git-send-email-hayeswang@realtek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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;
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(-)