Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

189 views
Skip to first unread message

Hayes Wang

unread,
Oct 22, 2012, 4:10:02 AM10/22/12
to
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 <haye...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

David Miller

unread,
Oct 22, 2012, 2:00:02 PM10/22/12
to
From: Hayes Wang <haye...@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 <haye...@realtek.com>

If there are two patches in this series, which the Subject line
implies, where is the second patch?

Francois Romieu

unread,
Oct 22, 2012, 3:50:02 PM10/22/12
to
Hayes Wang <haye...@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 <haye...@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

hayeswang

unread,
Oct 22, 2012, 10:30:02 PM10/22/12
to
Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Tuesday, October 23, 2012 3:28 AM
> To: Hayeswang
> Cc: net...@vger.kernel.org; linux-...@vger.kernel.org;
> je...@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.
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

Hayes Wang

unread,
Oct 23, 2012, 2:50:02 AM10/23/12
to
Update the parameters of RTL8111G

Signed-off-by: Hayes Wang <haye...@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)

Hayes Wang

unread,
Oct 23, 2012, 2:50:02 AM10/23/12
to
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 <haye...@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)

Francois Romieu

unread,
Oct 23, 2012, 4:00:01 PM10/23/12
to
hayeswang <haye...@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).

[...]
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

Francois Romieu

unread,
Oct 23, 2012, 4:00:02 PM10/23/12
to
Hayes Wang <haye...@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 <haye...@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

Francois Romieu

unread,
Oct 23, 2012, 4:10:02 PM10/23/12
to
Hayes Wang <haye...@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 <haye...@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
[...]
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

hayeswang

unread,
Oct 24, 2012, 2:00:01 AM10/24/12
to
Francois Romieu [mailto:rom...@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

hayeswang

unread,
Oct 24, 2012, 2:00:02 AM10/24/12
to
Francois Romieu [mailto:rom...@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

Hayes Wang

unread,
Oct 24, 2012, 2:30:02 AM10/24/12
to
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 <haye...@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)

Francois Romieu

unread,
Oct 24, 2012, 4:30:02 PM10/24/12
to
hayeswang <haye...@realtek.com> :
> Francois Romieu [mailto:rom...@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

Francois Romieu

unread,
Oct 24, 2012, 5:50:02 PM10/24/12
to
Hayes Wang <haye...@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 <haye...@realtek.com>

Acked-by: Francois Romieu <rom...@fr.zoreil.com>

David Miller

unread,
Oct 26, 2012, 2:20:02 AM10/26/12
to
From: Francois Romieu <rom...@fr.zoreil.com>
Date: Wed, 24 Oct 2012 23:20:03 +0200

> Hayes Wang <haye...@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 <haye...@realtek.com>
>
> Acked-by: Francois Romieu <rom...@fr.zoreil.com>

Applied, thanks.
0 new messages