[PATCH net-next 1/2] r8169: enable ALDPS for power saving

[PATCH net-next 1/2] r8169: enable ALDPS for power saving

[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

Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving

[David Miller]

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?

Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving

[Francois Romieu]

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

RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving

[hayeswang]

 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

[PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving

[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

[PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G

[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

Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving

[Francois Romieu]

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

Re: [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving

[Francois Romieu]

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

Re: [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G

[Francois Romieu]

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

RE: [PATCH net-next 1/2] r8169: enable ALDPS for power saving

[hayeswang]

 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

RE: [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving

[hayeswang]

 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

[PATCH v3 net-next] r8169: enable ALDPS for power saving

[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

Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving

[Francois Romieu]

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

Re: [PATCH v3 net-next] r8169: enable ALDPS for power saving

[Francois Romieu]

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>

Re: [PATCH v3 net-next] r8169: enable ALDPS for power saving

[David Miller]

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.