All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] r8169: reinstate ALDPS for power saving
@ 2017-11-15  9:00 Kai-Heng Feng
  2017-11-15  9:00 ` [PATCH 2/2] r8169: reinstate internal ASPM and clock request settings Kai-Heng Feng
  2017-11-15 10:53 ` [PATCH 1/2] r8169: reinstate ALDPS for power saving David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2017-11-15  9:00 UTC (permalink / raw)
  To: nic_swsd
  Cc: davem, netdev, linux-kernel, Kai-Heng Feng, Francois Romieu,
	Hayes Wang, Jörg Otte

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 <romieu@fr.zoreil.com>
Cc: Hayes Wang <hayeswang@realtek.com>
Cc: Jörg Otte <jrg.otte@gmail.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 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, 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_w0w1_phy(tp, 0x15, 0x1000, 0x0000);
+}
+
 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, 0x0000, 0x0400);
 	rtl_writephy(tp, 0x1f, 0x0000);
 
+	/* 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, 0x0000);
 	rtl_writephy(tp, 0x1f, 0x0000);
+
+	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, 0x0000, 0x0004);
 
 	rtl_writephy(tp, 0x1f, 0x0000);
+
+	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, 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);
 
@@ -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, 0x0000);
+
+	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, 0x0000);
-	rtl_writephy(tp, 0x18, 0x0310);
-	msleep(100);
+	r810x_aldps_disable(tp);
 
 	rtl_apply_firmware(tp);
 
@@ -4234,6 +4270,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)
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] r8169: reinstate internal ASPM and clock request settings
  2017-11-15  9:00 [PATCH 1/2] r8169: reinstate ALDPS for power saving Kai-Heng Feng
@ 2017-11-15  9:00 ` Kai-Heng Feng
  2017-11-15 10:53 ` [PATCH 1/2] r8169: reinstate ALDPS for power saving David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2017-11-15  9:00 UTC (permalink / raw)
  To: nic_swsd
  Cc: davem, netdev, linux-kernel, Kai-Heng Feng, Francois Romieu,
	Hayes Wang, Jörg Otte

Commit ("r8169: enable internal ASPM and clock request settings") caused
a regression on RTL8168evl/8111evl [1], so it got reverted.

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

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

Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Hayes Wang <hayeswang@realtek.com>
Cc: Jörg Otte <jrg.otte@gmail.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/net/ethernet/realtek/r8169.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 54bb9b15d541..6d6f3079320a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -510,6 +510,7 @@ enum rtl8168_registers {
 #define PWM_EN				(1 << 22)
 #define RXDV_GATED_EN			(1 << 19)
 #define EARLY_TALLY_EN			(1 << 16)
+#define FORCE_CLK			(1 << 15) /* force clock request */
 };
 
 enum rtl_register_content {
@@ -5992,6 +5993,11 @@ static void rtl_hw_start_8168e_2(struct rtl8169_private *tp)
 
 	RTL_W8(DLLPR, RTL_R8(DLLPR) | PFM_EN);
 	RTL_W32(MISC, RTL_R32(MISC) | PWM_EN);
+
+	/* Don't enable ASPM for RTL8168evl/8111evl.
+	 * https://lkml.org/lkml/2013/1/5/36
+	 */
+
 	RTL_W8(Config5, RTL_R8(Config5) & ~Spi_en);
 }
 
@@ -6017,13 +6023,12 @@ static void rtl_hw_start_8168f(struct rtl8169_private *tp)
 
 	RTL_W8(MaxTxPacketSize, EarlySize);
 
-	rtl_disable_clock_request(pdev);
-
 	RTL_W32(TxConfig, RTL_R32(TxConfig) | TXCFG_AUTO_FIFO);
 	RTL_W8(MCU, RTL_R8(MCU) & ~NOW_IS_OOB);
 	RTL_W8(DLLPR, RTL_R8(DLLPR) | PFM_EN);
-	RTL_W32(MISC, RTL_R32(MISC) | PWM_EN);
-	RTL_W8(Config5, RTL_R8(Config5) & ~Spi_en);
+	RTL_W32(MISC, RTL_R32(MISC) | PWM_EN | FORCE_CLK);
+	RTL_W8(Config5, (RTL_R8(Config5) & ~Spi_en) | ASPM_en);
+	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
 }
 
 static void rtl_hw_start_8168f_1(struct rtl8169_private *tp)
@@ -6083,8 +6088,10 @@ static void rtl_hw_start_8168g(struct rtl8169_private *tp)
 	rtl_w0w1_eri(tp, 0xdc, ERIAR_MASK_0001, 0x01, 0x00, ERIAR_EXGMAC);
 	rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
 
-	RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
+	RTL_W32(MISC, (RTL_R32(MISC) | FORCE_CLK) & ~RXDV_GATED_EN);
 	RTL_W8(MaxTxPacketSize, EarlySize);
+	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
 
 	rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
 	rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
@@ -6594,6 +6601,9 @@ static void rtl_hw_start_8105e_1(struct rtl8169_private *tp)
 
 	RTL_W8(MCU, RTL_R8(MCU) | EN_NDP | EN_OOB_RESET);
 	RTL_W8(DLLPR, RTL_R8(DLLPR) | PFM_EN);
+	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
+	RTL_W32(MISC, RTL_R32(MISC) | FORCE_CLK);
 
 	rtl_ephy_init(tp, e_info_8105e_1, ARRAY_SIZE(e_info_8105e_1));
 
@@ -6621,6 +6631,9 @@ static void rtl_hw_start_8402(struct rtl8169_private *tp)
 
 	RTL_W32(TxConfig, RTL_R32(TxConfig) | TXCFG_AUTO_FIFO);
 	RTL_W8(MCU, RTL_R8(MCU) & ~NOW_IS_OOB);
+	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
+	RTL_W32(MISC, RTL_R32(MISC) | FORCE_CLK);
 
 	rtl_ephy_init(tp, e_info_8402, ARRAY_SIZE(e_info_8402));
 
@@ -6644,7 +6657,10 @@ static void rtl_hw_start_8106(struct rtl8169_private *tp)
 	/* Force LAN exit from ASPM if Rx/Tx are not idle */
 	RTL_W32(FuncEvent, RTL_R32(FuncEvent) | 0x002800);
 
-	RTL_W32(MISC, (RTL_R32(MISC) | DISABLE_LAN_EN) & ~EARLY_TALLY_EN);
+	RTL_W32(MISC,
+		(RTL_R32(MISC) | DISABLE_LAN_EN | FORCE_CLK) & ~EARLY_TALLY_EN);
+	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
 	RTL_W8(MCU, RTL_R8(MCU) | EN_NDP | EN_OOB_RESET);
 	RTL_W8(DLLPR, RTL_R8(DLLPR) & ~PFM_EN);
 
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving
  2017-11-15  9:00 [PATCH 1/2] r8169: reinstate ALDPS for power saving Kai-Heng Feng
  2017-11-15  9:00 ` [PATCH 2/2] r8169: reinstate internal ASPM and clock request settings Kai-Heng Feng
@ 2017-11-15 10:53 ` David Miller
  2017-11-15 23:33   ` Francois Romieu
  2017-11-16  4:29   ` Kai Heng Feng
  1 sibling, 2 replies; 6+ messages in thread
From: David Miller @ 2017-11-15 10:53 UTC (permalink / raw)
  To: kai.heng.feng; +Cc: nic_swsd, netdev, linux-kernel, romieu, hayeswang, jrg.otte

From: Kai-Heng Feng <kai.heng.feng@canonical.com>
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 <romieu@fr.zoreil.com>
> Cc: Hayes Wang <hayeswang@realtek.com>
> Cc: Jörg Otte <jrg.otte@gmail.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving
  2017-11-15 10:53 ` [PATCH 1/2] r8169: reinstate ALDPS for power saving David Miller
@ 2017-11-15 23:33   ` Francois Romieu
  2017-11-16  4:29   ` Kai Heng Feng
  1 sibling, 0 replies; 6+ messages in thread
From: Francois Romieu @ 2017-11-15 23:33 UTC (permalink / raw)
  To: David Miller
  Cc: kai.heng.feng, nic_swsd, netdev, linux-kernel, hayeswang, jrg.otte

David Miller <davem@davemloft.net> :
[...]
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving
  2017-11-15 10:53 ` [PATCH 1/2] r8169: reinstate ALDPS for power saving David Miller
  2017-11-15 23:33   ` Francois Romieu
@ 2017-11-16  4:29   ` Kai Heng Feng
  2018-03-22 10:19     ` Kai-Heng Feng
  1 sibling, 1 reply; 6+ messages in thread
From: Kai Heng Feng @ 2017-11-16  4:29 UTC (permalink / raw)
  To: David Miller; +Cc: nic_swsd, netdev, linux-kernel, romieu, hayeswang, jrg.otte



> On 15 Nov 2017, at 6:53 PM, David Miller <davem@davemloft.net> wrote:
> 
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 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 <romieu@fr.zoreil.com>
>> Cc: Hayes Wang <hayeswang@realtek.com>
>> Cc: Jörg Otte <jrg.otte@gmail.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving
  2017-11-16  4:29   ` Kai Heng Feng
@ 2018-03-22 10:19     ` Kai-Heng Feng
  0 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2018-03-22 10:19 UTC (permalink / raw)
  To: Hayes Wang
  Cc: nic_swsd, Linux Netdev List, Linux Kernel Mailing List, romieu,
	jrg.otte, David Miller

Kai Heng Feng <kai.heng.feng@canonical.com> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-03-22 10:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-15  9:00 [PATCH 1/2] r8169: reinstate ALDPS for power saving Kai-Heng Feng
2017-11-15  9:00 ` [PATCH 2/2] r8169: reinstate internal ASPM and clock request settings Kai-Heng Feng
2017-11-15 10:53 ` [PATCH 1/2] r8169: reinstate ALDPS for power saving David Miller
2017-11-15 23:33   ` Francois Romieu
2017-11-16  4:29   ` Kai Heng Feng
2018-03-22 10:19     ` Kai-Heng Feng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.