Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2018-03-22 Thread Kai-Heng Feng 

Kai Heng Feng  wrote:


Hopefully Hayes (or Realtek) can shed more lights on the issue. Apparently
ALDPS and ASPM for r8169 is enabled in different commercial products, just
not in Linux mainline.


Hayes and Realtek folks,

How do we make this patch going forward?
Do you find the root cause that make this patch got reverted?

I guess ALDPS is no longer needed after commit a92a08499b1f ("r8169:  
improve runtime pm in general and suspend unused ports"), now the device  
gets runtime suspended when link is down.


OTOH, ASPM is still quite useful though. When it's enabled, it can save 1W  
power usage, which is quite substantial for a laptop.


So, I'd like to hear your feedback and make ASPM for r8169 eventually gets  
upstreamed.


Kai-Heng

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2018-03-22 Thread Kai-Heng Feng 

Kai Heng Feng  wrote:


Hopefully Hayes (or Realtek) can shed more lights on the issue. Apparently
ALDPS and ASPM for r8169 is enabled in different commercial products, just
not in Linux mainline.


Hayes and Realtek folks,

How do we make this patch going forward?
Do you find the root cause that make this patch got reverted?

I guess ALDPS is no longer needed after commit a92a08499b1f ("r8169:  
improve runtime pm in general and suspend unused ports"), now the device  
gets runtime suspended when link is down.


OTOH, ASPM is still quite useful though. When it's enabled, it can save 1W  
power usage, which is quite substantial for a laptop.


So, I'd like to hear your feedback and make ASPM for r8169 eventually gets  
upstreamed.


Kai-Heng

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread Kai Heng Feng 


> On 15 Nov 2017, at 6:53 PM, David Miller  wrote:
> 
> From: Kai-Heng Feng 
> Date: Wed, 15 Nov 2017 04:00:18 -0500
> 
>> Commit ("r8169: enable ALDPS for power saving") caused a regression on
>> RTL8168evl/8111evl [1], so it got reverted.
>> 
>> Instead of reverting the whole commit, let's reinstate ALDPS for all
>> models other than RTL8168evl/8111evl.
>> 
>> [1] https://lkml.org/lkml/2013/1/5/36
>> 
>> Cc: Francois Romieu 
>> Cc: Hayes Wang 
>> Cc: Jörg Otte 
>> Signed-off-by: Kai-Heng Feng 
> 
> Sorry, this isn't going to work.
> 
> The problem is that we have no idea what chips this really
> works for.  All we know is that it definitely does not work
> for one particular chip variant.

Another guy tried to enable ASPM before [1], with extra knobs,
which is discouraged.
I want to do the same without the knobs, with the intention not to
re-introduce the regression to RTL8168evl/8111evl.

> The amount of coverage this change is going to get is very small as
> well, meaning an even greater change of regressions.
> 
> Therefore the only acceptable way to handle this is to have
> a white-list, specific chips that have been explicitly tested
> and are known to work with this feature, rather than the other
> way around.

I think a chip-specific whitelist will be quite hard. That requires *both*
Realtek and OEM/ODM to do the test. IIUC, the very same chip can be used
in different Laptops/Motherboards, regression may happen on a very specific 
combination.

A more plausible solution is model specific whitelist. In this case, 
extra knobs are desirable, so users can do the testing themselves before
adding new models to whitelist.

Currently users report that by enabling r8169’s ASPM on Dell Inspiron 7559,
power consumption can be drastically reduced.

> Furthermore, you're not even checking the chip version, you're
> checking instead whether the firmware is loaded or not.  That
> doesn't seem like a safe way to guard this at all.

Hopefully Hayes (or Realtek) can shed more lights on the issue. Apparently
ALDPS and ASPM for r8169 is enabled in different commercial products, just
not in Linux mainline.

Kai-Heng

[1] https://www.spinics.net/lists/netdev/msg403994.html

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread Kai Heng Feng 


> On 15 Nov 2017, at 6:53 PM, David Miller  wrote:
> 
> From: Kai-Heng Feng 
> Date: Wed, 15 Nov 2017 04:00:18 -0500
> 
>> Commit ("r8169: enable ALDPS for power saving") caused a regression on
>> RTL8168evl/8111evl [1], so it got reverted.
>> 
>> Instead of reverting the whole commit, let's reinstate ALDPS for all
>> models other than RTL8168evl/8111evl.
>> 
>> [1] https://lkml.org/lkml/2013/1/5/36
>> 
>> Cc: Francois Romieu 
>> Cc: Hayes Wang 
>> Cc: Jörg Otte 
>> Signed-off-by: Kai-Heng Feng 
> 
> Sorry, this isn't going to work.
> 
> The problem is that we have no idea what chips this really
> works for.  All we know is that it definitely does not work
> for one particular chip variant.

Another guy tried to enable ASPM before [1], with extra knobs,
which is discouraged.
I want to do the same without the knobs, with the intention not to
re-introduce the regression to RTL8168evl/8111evl.

> The amount of coverage this change is going to get is very small as
> well, meaning an even greater change of regressions.
> 
> Therefore the only acceptable way to handle this is to have
> a white-list, specific chips that have been explicitly tested
> and are known to work with this feature, rather than the other
> way around.

I think a chip-specific whitelist will be quite hard. That requires *both*
Realtek and OEM/ODM to do the test. IIUC, the very same chip can be used
in different Laptops/Motherboards, regression may happen on a very specific 
combination.

A more plausible solution is model specific whitelist. In this case, 
extra knobs are desirable, so users can do the testing themselves before
adding new models to whitelist.

Currently users report that by enabling r8169’s ASPM on Dell Inspiron 7559,
power consumption can be drastically reduced.

> Furthermore, you're not even checking the chip version, you're
> checking instead whether the firmware is loaded or not.  That
> doesn't seem like a safe way to guard this at all.

Hopefully Hayes (or Realtek) can shed more lights on the issue. Apparently
ALDPS and ASPM for r8169 is enabled in different commercial products, just
not in Linux mainline.

Kai-Heng

[1] https://www.spinics.net/lists/netdev/msg403994.html

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread Francois Romieu 
David Miller  :
[...]
> The amount of coverage this change is going to get is very small as
> well, meaning an even greater chance of regressions.

Yes.

> Therefore the only acceptable way to handle this is to have
> a white-list, specific chips that have been explicitly tested
> and are known to work with this feature, rather than the other
> way around.
> 
> Furthermore, you're not even checking the chip version, you're
> checking instead whether the firmware is loaded or not.  That
> doesn't seem like a safe way to guard this at all.

Actually the chip specific xyz_hw_phy_config methods call the relevant
aldps enabling helper _but_ the 8168evl dedicated xyz_hw_phy_config
doesn't. The firmware loaded check is just a distraction for the
busy reviewer.

-- 
Ueimor

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread Francois Romieu 
David Miller  :
[...]
> The amount of coverage this change is going to get is very small as
> well, meaning an even greater chance of regressions.

Yes.

> Therefore the only acceptable way to handle this is to have
> a white-list, specific chips that have been explicitly tested
> and are known to work with this feature, rather than the other
> way around.
> 
> Furthermore, you're not even checking the chip version, you're
> checking instead whether the firmware is loaded or not.  That
> doesn't seem like a safe way to guard this at all.

Actually the chip specific xyz_hw_phy_config methods call the relevant
aldps enabling helper _but_ the 8168evl dedicated xyz_hw_phy_config
doesn't. The firmware loaded check is just a distraction for the
busy reviewer.

-- 
Ueimor

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread David Miller 
From: Kai-Heng Feng 
Date: Wed, 15 Nov 2017 04:00:18 -0500

> Commit ("r8169: enable ALDPS for power saving") caused a regression on
> RTL8168evl/8111evl [1], so it got reverted.
> 
> Instead of reverting the whole commit, let's reinstate ALDPS for all
> models other than RTL8168evl/8111evl.
> 
> [1] https://lkml.org/lkml/2013/1/5/36
> 
> Cc: Francois Romieu 
> Cc: Hayes Wang 
> Cc: Jörg Otte 
> Signed-off-by: Kai-Heng Feng 

Sorry, this isn't going to work.

The problem is that we have no idea what chips this really
works for.  All we know is that it definitely does not work
for one particular chip variant.

The amount of coverage this change is going to get is very small as
well, meaning an even greater change of regressions.

Therefore the only acceptable way to handle this is to have
a white-list, specific chips that have been explicitly tested
and are known to work with this feature, rather than the other
way around.

Furthermore, you're not even checking the chip version, you're
checking instead whether the firmware is loaded or not.  That
doesn't seem like a safe way to guard this at all.

Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread David Miller 
From: Kai-Heng Feng 
Date: Wed, 15 Nov 2017 04:00:18 -0500

> Commit ("r8169: enable ALDPS for power saving") caused a regression on
> RTL8168evl/8111evl [1], so it got reverted.
> 
> Instead of reverting the whole commit, let's reinstate ALDPS for all
> models other than RTL8168evl/8111evl.
> 
> [1] https://lkml.org/lkml/2013/1/5/36
> 
> Cc: Francois Romieu 
> Cc: Hayes Wang 
> Cc: Jörg Otte 
> Signed-off-by: Kai-Heng Feng 

Sorry, this isn't going to work.

The problem is that we have no idea what chips this really
works for.  All we know is that it definitely does not work
for one particular chip variant.

The amount of coverage this change is going to get is very small as
well, meaning an even greater change of regressions.

Therefore the only acceptable way to handle this is to have
a white-list, specific chips that have been explicitly tested
and are known to work with this feature, rather than the other
way around.

Furthermore, you're not even checking the chip version, you're
checking instead whether the firmware is loaded or not.  That
doesn't seem like a safe way to guard this at all.

[PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread Kai-Heng Feng 
Commit ("r8169: enable ALDPS for power saving") caused a regression on
RTL8168evl/8111evl [1], so it got reverted.

Instead of reverting the whole commit, let's reinstate ALDPS for all
models other than RTL8168evl/8111evl.

[1] https://lkml.org/lkml/2013/1/5/36

Cc: Francois Romieu 
Cc: Hayes Wang 
Cc: Jörg Otte 
Signed-off-by: Kai-Heng Feng 
---
 drivers/net/ethernet/realtek/r8169.c | 58 +---
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c 
b/drivers/net/ethernet/realtek/r8169.c
index e03fcf914690..54bb9b15d541 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -733,6 +733,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 {
@@ -2785,8 +2786,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)
@@ -2797,6 +2800,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, 0x);
+   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, 0x);
+   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, 0x);
+   rtl_w0w1_phy(tp, 0x15, 0x1000, 0x);
+}
+
 static void rtl8169s_hw_phy_config(struct rtl8169_private *tp)
 {
static const struct phy_reg phy_reg_init[] = {
@@ -3587,6 +3615,10 @@ static void rtl8168e_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_w0w1_phy(tp, 0x10, 0x, 0x0400);
rtl_writephy(tp, 0x1f, 0x);
 
+   /* Don't enable ALDPS for RTL8168evl/8111evl.
+* https://lkml.org/lkml/2013/1/5/36
+*/
+
/* Broken BIOS workaround: feed GigaMAC registers with MAC address. */
rtl_rar_exgmac_set(tp, tp->dev->dev_addr);
 }
@@ -3661,6 +3693,8 @@ static void rtl8168f_1_hw_phy_config(struct 
rtl8169_private *tp)
rtl_writephy(tp, 0x05, 0x8b85);
rtl_w0w1_phy(tp, 0x06, 0x4000, 0x);
rtl_writephy(tp, 0x1f, 0x);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp)
@@ -3668,6 +3702,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)
@@ -4019,6 +4055,8 @@ static void rtl8168h_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_w0w1_phy(tp, 0x10, 0x, 0x0004);
 
rtl_writephy(tp, 0x1f, 0x);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp)
@@ -4188,21 +4226,19 @@ static void rtl8105e_hw_phy_config(struct 
rtl8169_private *tp)
};
 
/* Disable ALDPS before ram code */
-   rtl_writephy(tp, 0x1f, 0x);
-   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, 0x);
-   rtl_writephy(tp, 0x18, 0x0310);
-   msleep(20);
+   r810x_aldps_disable(tp);
 
rtl_apply_firmware(tp);
 
@@ -4212,6 +4248,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, 0x);
+
+   r810x_aldps_enable(tp);
 }
 
 static void rtl8106e_hw_phy_config(struct rtl8169_private *tp)
@@ -4224,9 +4262,7 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private 
*tp)
};
 
/* Disable ALDPS before ram code */
-   rtl_writephy(tp, 0x1f, 0x);
-   rtl_writephy(tp, 0x18, 0x0310);
-   msleep(100);
+

[PATCH 1/2] r8169: reinstate ALDPS for power saving

2017-11-15 Thread Kai-Heng Feng 
Commit ("r8169: enable ALDPS for power saving") caused a regression on
RTL8168evl/8111evl [1], so it got reverted.

Instead of reverting the whole commit, let's reinstate ALDPS for all
models other than RTL8168evl/8111evl.

[1] https://lkml.org/lkml/2013/1/5/36

Cc: Francois Romieu 
Cc: Hayes Wang 
Cc: Jörg Otte 
Signed-off-by: Kai-Heng Feng 
---
 drivers/net/ethernet/realtek/r8169.c | 58 +---
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c 
b/drivers/net/ethernet/realtek/r8169.c
index e03fcf914690..54bb9b15d541 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -733,6 +733,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 {
@@ -2785,8 +2786,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)
@@ -2797,6 +2800,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, 0x);
+   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, 0x);
+   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, 0x);
+   rtl_w0w1_phy(tp, 0x15, 0x1000, 0x);
+}
+
 static void rtl8169s_hw_phy_config(struct rtl8169_private *tp)
 {
static const struct phy_reg phy_reg_init[] = {
@@ -3587,6 +3615,10 @@ static void rtl8168e_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_w0w1_phy(tp, 0x10, 0x, 0x0400);
rtl_writephy(tp, 0x1f, 0x);
 
+   /* Don't enable ALDPS for RTL8168evl/8111evl.
+* https://lkml.org/lkml/2013/1/5/36
+*/
+
/* Broken BIOS workaround: feed GigaMAC registers with MAC address. */
rtl_rar_exgmac_set(tp, tp->dev->dev_addr);
 }
@@ -3661,6 +3693,8 @@ static void rtl8168f_1_hw_phy_config(struct 
rtl8169_private *tp)
rtl_writephy(tp, 0x05, 0x8b85);
rtl_w0w1_phy(tp, 0x06, 0x4000, 0x);
rtl_writephy(tp, 0x1f, 0x);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8168f_2_hw_phy_config(struct rtl8169_private *tp)
@@ -3668,6 +3702,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)
@@ -4019,6 +4055,8 @@ static void rtl8168h_2_hw_phy_config(struct 
rtl8169_private *tp)
rtl_w0w1_phy(tp, 0x10, 0x, 0x0004);
 
rtl_writephy(tp, 0x1f, 0x);
+
+   r8168_aldps_enable_1(tp);
 }
 
 static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp)
@@ -4188,21 +4226,19 @@ static void rtl8105e_hw_phy_config(struct 
rtl8169_private *tp)
};
 
/* Disable ALDPS before ram code */
-   rtl_writephy(tp, 0x1f, 0x);
-   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, 0x);
-   rtl_writephy(tp, 0x18, 0x0310);
-   msleep(20);
+   r810x_aldps_disable(tp);
 
rtl_apply_firmware(tp);
 
@@ -4212,6 +4248,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, 0x);
+
+   r810x_aldps_enable(tp);
 }
 
 static void rtl8106e_hw_phy_config(struct rtl8169_private *tp)
@@ -4224,9 +4262,7 @@ static void rtl8106e_hw_phy_config(struct rtl8169_private 
*tp)
};
 
/* Disable ALDPS before ram code */
-   rtl_writephy(tp, 0x1f, 0x);
-   rtl_writephy(tp, 0x18, 0x0310);
-   msleep(100);
+   r810x_aldps_disable(tp);
 
rtl_apply_firmware(tp);
 
@@ -4234,6 +4270,8 @@ static