[PATCH 1/2] r8169: Don't disable ASPM in the driver

[PATCH 1/2] r8169: Don't disable ASPM in the driver

[Kai-Heng Feng]

Enable or disable ASPM should be done in PCI core instead of in the
device driver.

Commit ba04c7c93bbc ("r8169: disable ASPM") uses
pci_disable_link_state() to disable ASPM. This is incorrect, if the
device really needs to disable ASPM, we should use a quirk in PCI core
to prevent the PCI core from setting ASPM altogether.

Let's remove pci_disable_link_state() for now. Use PCI core quirks if
any regression happens.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
- Remove module parameter.
- Remove pci_disable_link_state().

 drivers/net/ethernet/realtek/r8169.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 75dfac0248f4..9b55ce513a36 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -25,7 +25,6 @@
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/firmware.h>
-#include <linux/pci-aspm.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
 #include <net/ip6_checksum.h>
@@ -7647,10 +7646,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mii->reg_num_mask = 0x1f;
 	mii->supports_gmii = cfg->has_gmii;
 
-	/* disable ASPM completely as that cause random device stop working
-	 * problems as well as full system hangs for some PCIe devices users */
-	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
-				     PCIE_LINK_STATE_CLKPM);
 
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
 	rc = pcim_enable_device(pdev);
-- 
2.17.0

[PATCH 2/2] r8169: Reinstate ASPM Support

[Kai-Heng Feng]

On newer Intel platforms, ASPM support in r8169 is the last missing
puzzle to let Package C-State achieves PC8. Without ASPM support, the
deepest Package C-State can hit is PC3.
PC8 can save additional ~3W in comparison with PC3 on my testing
platform.

The original patch is from Realtek.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
- Remove module parameter.
- Remove pci_disable_link_state().

 drivers/net/ethernet/realtek/r8169.c | 41 +++++++++++++++++++---------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 9b55ce513a36..85f4e746b040 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5289,6 +5289,18 @@ static void rtl_pcie_state_l2l3_enable(struct rtl8169_private *tp, bool enable)
 	RTL_W8(tp, Config3, data);
 }
 
+static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp,
+					       bool enable)
+{
+	if (enable) {
+		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
+		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
+	} else {
+		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
+		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	}
+}
+
 static void rtl_hw_start_8168bb(struct rtl8169_private *tp)
 {
 	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
@@ -5645,9 +5657,9 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
 	rtl_hw_start_8168g(tp);
 
 	/* disable aspm and clock request before access ephy */
-	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	rtl_hw_internal_aspm_clkreq_enable(tp, false);
 	rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1));
+	rtl_hw_internal_aspm_clkreq_enable(tp, true);
 }
 
 static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
@@ -5680,9 +5692,9 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
 	rtl_hw_start_8168g(tp);
 
 	/* disable aspm and clock request before access ephy */
-	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	rtl_hw_internal_aspm_clkreq_enable(tp, false);
 	rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2));
+	rtl_hw_internal_aspm_clkreq_enable(tp, true);
 }
 
 static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
@@ -5699,8 +5711,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
 	};
 
 	/* disable aspm and clock request before access ephy */
-	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	rtl_hw_internal_aspm_clkreq_enable(tp, false);
 	rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1));
 
 	RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO);
@@ -5779,6 +5790,8 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
 	r8168_mac_ocp_write(tp, 0xe63e, 0x0000);
 	r8168_mac_ocp_write(tp, 0xc094, 0x0000);
 	r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
+
+	rtl_hw_internal_aspm_clkreq_enable(tp, true);
 }
 
 static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
@@ -5830,11 +5843,12 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp)
 	};
 
 	/* disable aspm and clock request before access ephy */
-	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	rtl_hw_internal_aspm_clkreq_enable(tp, false);
 	rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1));
 
 	rtl_hw_start_8168ep(tp);
+
+	rtl_hw_internal_aspm_clkreq_enable(tp, true);
 }
 
 static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
@@ -5846,14 +5860,15 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
 	};
 
 	/* disable aspm and clock request before access ephy */
-	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	rtl_hw_internal_aspm_clkreq_enable(tp, false);
 	rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2));
 
 	rtl_hw_start_8168ep(tp);
 
 	RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN);
 	RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN);
+
+	rtl_hw_internal_aspm_clkreq_enable(tp, true);
 }
 
 static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
@@ -5867,8 +5882,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
 	};
 
 	/* disable aspm and clock request before access ephy */
-	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+	rtl_hw_internal_aspm_clkreq_enable(tp, false);
 	rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3));
 
 	rtl_hw_start_8168ep(tp);
@@ -5888,6 +5902,8 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
 	data = r8168_mac_ocp_read(tp, 0xe860);
 	data |= 0x0080;
 	r8168_mac_ocp_write(tp, 0xe860, data);
+
+	rtl_hw_internal_aspm_clkreq_enable(tp, true);
 }
 
 static void rtl_hw_start_8168(struct rtl8169_private *tp)
@@ -7646,7 +7662,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mii->reg_num_mask = 0x1f;
 	mii->supports_gmii = cfg->has_gmii;
 
-
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
 	rc = pcim_enable_device(pdev);
 	if (rc < 0) {
-- 
2.17.0

Re: [PATCH 2/2] r8169: Reinstate ASPM Support

[Heiner Kallweit]

On 12.06.2018 11:57, Kai-Heng Feng wrote:
> On newer Intel platforms, ASPM support in r8169 is the last missing
> puzzle to let Package C-State achieves PC8. Without ASPM support, the
> deepest Package C-State can hit is PC3.
> PC8 can save additional ~3W in comparison with PC3 on my testing
> platform.
> 
Maybe we should replace PC8 with "beyond PC3". My system
(Haswell 2961Y) reaches 50% PC7 + 5% PC9 + 45% PC10 now.
It never seems to use PC8.

> The original patch is from Realtek.
> 
Please add a link to this original patch.

> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
> - Remove module parameter.
> - Remove pci_disable_link_state().
> 
>  drivers/net/ethernet/realtek/r8169.c | 41 +++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 9b55ce513a36..85f4e746b040 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -5289,6 +5289,18 @@ static void rtl_pcie_state_l2l3_enable(struct rtl8169_private *tp, bool enable)
>  	RTL_W8(tp, Config3, data);
>  }
>  
> +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp,
> +					       bool enable)

Do we need this hw_internal in the function name?

> +{
> +	if (enable) {
> +		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> +		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> +	} else {
> +		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> +		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	}
> +}
> +
>  static void rtl_hw_start_8168bb(struct rtl8169_private *tp)
>  {
>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
> @@ -5645,9 +5657,9 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
>  	rtl_hw_start_8168g(tp);
>  
>  	/* disable aspm and clock request before access ephy */
> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>  	rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1));
> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>  }
>  
>  static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
> @@ -5680,9 +5692,9 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
>  	rtl_hw_start_8168g(tp);
>  
>  	/* disable aspm and clock request before access ephy */
> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>  	rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2));
> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>  }
>  
>  static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
> @@ -5699,8 +5711,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
>  	};
>  
>  	/* disable aspm and clock request before access ephy */
> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>  	rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1));
>  
>  	RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO);
> @@ -5779,6 +5790,8 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
>  	r8168_mac_ocp_write(tp, 0xe63e, 0x0000);
>  	r8168_mac_ocp_write(tp, 0xc094, 0x0000);
>  	r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
> +
> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>  }
>  
>  static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
> @@ -5830,11 +5843,12 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp)
>  	};
>  
>  	/* disable aspm and clock request before access ephy */
> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>  	rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1));
>  
>  	rtl_hw_start_8168ep(tp);
> +
> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>  }
>  
>  static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
> @@ -5846,14 +5860,15 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
>  	};
>  
>  	/* disable aspm and clock request before access ephy */
> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>  	rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2));
>  
>  	rtl_hw_start_8168ep(tp);
>  
>  	RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN);
>  	RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN);
> +
> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>  }
>  
>  static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
> @@ -5867,8 +5882,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>  	};
>  
>  	/* disable aspm and clock request before access ephy */
> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>  	rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3));
>  
>  	rtl_hw_start_8168ep(tp);
> @@ -5888,6 +5902,8 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>  	data = r8168_mac_ocp_read(tp, 0xe860);
>  	data |= 0x0080;
>  	r8168_mac_ocp_write(tp, 0xe860, data);
> +
> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>  }
>  
>  static void rtl_hw_start_8168(struct rtl8169_private *tp)
> @@ -7646,7 +7662,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	mii->reg_num_mask = 0x1f;
>  	mii->supports_gmii = cfg->has_gmii;
>  
> -
>  	/* enable device (incl. PCI PM wakeup and hotplug setup) */
>  	rc = pcim_enable_device(pdev);
>  	if (rc < 0) {
> 

Re: [PATCH 2/2] r8169: Reinstate ASPM Support

[Kai Heng Feng]

Hi Heiner,

> On Jun 13, 2018, at 3:35 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 12.06.2018 11:57, Kai-Heng Feng wrote:
>> On newer Intel platforms, ASPM support in r8169 is the last missing
>> puzzle to let Package C-State achieves PC8. Without ASPM support, the
>> deepest Package C-State can hit is PC3.
>> PC8 can save additional ~3W in comparison with PC3 on my testing
>> platform.
> Maybe we should replace PC8 with "beyond PC3". My system
> (Haswell 2961Y) reaches 50% PC7 + 5% PC9 + 45% PC10 now.
> It never seems to use PC8.

My original wording are really mouthful. I'll update them in next version.

The platform in question is Coffee Lake. This patch should make systems  
newer than Skylake to hit > PC3. Older systems may not see significant  
change.
I'll also state these info in the next version.

>
>> The original patch is from Realtek.
> Please add a link to this original patch.

Realtek sent me the patch privately. Is it okay to upload the patch to  
pastebin or gist?

Kai-Heng

>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> v2:
>> - Remove module parameter.
>> - Remove pci_disable_link_state().
>>
>>  drivers/net/ethernet/realtek/r8169.c | 41 +++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c  
>> b/drivers/net/ethernet/realtek/r8169.c
>> index 9b55ce513a36..85f4e746b040 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -5289,6 +5289,18 @@ static void rtl_pcie_state_l2l3_enable(struct  
>> rtl8169_private *tp, bool enable)
>>  	RTL_W8(tp, Config3, data);
>>  }
>>
>> +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private  
>> *tp,
>> +					       bool enable)
>
> Do we need this hw_internal in the function name?
>
>> +{
>> +	if (enable) {
>> +		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
>> +		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
>> +	} else {
>> +		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> +		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	}
>> +}
>> +
>>  static void rtl_hw_start_8168bb(struct rtl8169_private *tp)
>>  {
>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
>> @@ -5645,9 +5657,9 @@ static void rtl_hw_start_8168g_1(struct  
>> rtl8169_private *tp)
>>  	rtl_hw_start_8168g(tp);
>>
>>  	/* disable aspm and clock request before access ephy */
>> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>>  	rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1));
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>>  }
>>
>>  static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
>> @@ -5680,9 +5692,9 @@ static void rtl_hw_start_8411_2(struct  
>> rtl8169_private *tp)
>>  	rtl_hw_start_8168g(tp);
>>
>>  	/* disable aspm and clock request before access ephy */
>> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>>  	rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2));
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>>  }
>>
>>  static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
>> @@ -5699,8 +5711,7 @@ static void rtl_hw_start_8168h_1(struct  
>> rtl8169_private *tp)
>>  	};
>>
>>  	/* disable aspm and clock request before access ephy */
>> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>>  	rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1));
>>
>>  	RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO);
>> @@ -5779,6 +5790,8 @@ static void rtl_hw_start_8168h_1(struct  
>> rtl8169_private *tp)
>>  	r8168_mac_ocp_write(tp, 0xe63e, 0x0000);
>>  	r8168_mac_ocp_write(tp, 0xc094, 0x0000);
>>  	r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
>> +
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>>  }
>>
>>  static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
>> @@ -5830,11 +5843,12 @@ static void rtl_hw_start_8168ep_1(struct  
>> rtl8169_private *tp)
>>  	};
>>
>>  	/* disable aspm and clock request before access ephy */
>> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>>  	rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1));
>>
>>  	rtl_hw_start_8168ep(tp);
>> +
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>>  }
>>
>>  static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
>> @@ -5846,14 +5860,15 @@ static void rtl_hw_start_8168ep_2(struct  
>> rtl8169_private *tp)
>>  	};
>>
>>  	/* disable aspm and clock request before access ephy */
>> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>>  	rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2));
>>
>>  	rtl_hw_start_8168ep(tp);
>>
>>  	RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN);
>>  	RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN);
>> +
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>>  }
>>
>>  static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>> @@ -5867,8 +5882,7 @@ static void rtl_hw_start_8168ep_3(struct  
>> rtl8169_private *tp)
>>  	};
>>
>>  	/* disable aspm and clock request before access ephy */
>> -	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> -	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, false);
>>  	rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3));
>>
>>  	rtl_hw_start_8168ep(tp);
>> @@ -5888,6 +5902,8 @@ static void rtl_hw_start_8168ep_3(struct  
>> rtl8169_private *tp)
>>  	data = r8168_mac_ocp_read(tp, 0xe860);
>>  	data |= 0x0080;
>>  	r8168_mac_ocp_write(tp, 0xe860, data);
>> +
>> +	rtl_hw_internal_aspm_clkreq_enable(tp, true);
>>  }
>>
>>  static void rtl_hw_start_8168(struct rtl8169_private *tp)
>> @@ -7646,7 +7662,6 @@ static int rtl_init_one(struct pci_dev *pdev,  
>> const struct pci_device_id *ent)
>>  	mii->reg_num_mask = 0x1f;
>>  	mii->supports_gmii = cfg->has_gmii;
>>
>> -
>>  	/* enable device (incl. PCI PM wakeup and hotplug setup) */
>>  	rc = pcim_enable_device(pdev);
>>  	if (rc < 0) {

Re: [PATCH 1/2] r8169: Don't disable ASPM in the driver

[Heiner Kallweit]

On 12.06.2018 11:57, Kai-Heng Feng wrote:
> Enable or disable ASPM should be done in PCI core instead of in the
> device driver.
> 
> Commit ba04c7c93bbc ("r8169: disable ASPM") uses
> pci_disable_link_state() to disable ASPM. This is incorrect, if the
> device really needs to disable ASPM, we should use a quirk in PCI core
> to prevent the PCI core from setting ASPM altogether.
> 
I wouldn't call using pci_disable_link_state() in a driver incorrect
(as it works), there is just a better way which is more in line with
the PCI subsystem architecture.

> Let's remove pci_disable_link_state() for now. Use PCI core quirks if
> any regression happens.
> 
The vendor driver disables ASPM unconditionally for chip version 25
(there it's METHOD_9), so I think ASPM support is broken in this chip
version. I'll cook a PCI quirk.

> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Please note that netdev is closed currently. Once 4.18-RC1 is out it
will be re-opened. Then please re-submit properly annotating PATCH
with "net-next" (I've forgotten this often enough myself).

> ---
> v2:
> - Remove module parameter.
> - Remove pci_disable_link_state().
> 
>  drivers/net/ethernet/realtek/r8169.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 75dfac0248f4..9b55ce513a36 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -25,7 +25,6 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/firmware.h>
> -#include <linux/pci-aspm.h>
>  #include <linux/prefetch.h>
>  #include <linux/ipv6.h>
>  #include <net/ip6_checksum.h>
> @@ -7647,10 +7646,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	mii->reg_num_mask = 0x1f;
>  	mii->supports_gmii = cfg->has_gmii;
>  
> -	/* disable ASPM completely as that cause random device stop working
> -	 * problems as well as full system hangs for some PCIe devices users */
> -	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
> -				     PCIE_LINK_STATE_CLKPM);
>  
>  	/* enable device (incl. PCI PM wakeup and hotplug setup) */
>  	rc = pcim_enable_device(pdev);
> 

Re: [PATCH 1/2] r8169: Don't disable ASPM in the driver

[Kai Heng Feng]

Hi Heiner,

> On Jun 13, 2018, at 3:30 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 12.06.2018 11:57, Kai-Heng Feng wrote:
>> Enable or disable ASPM should be done in PCI core instead of in the
>> device driver.
>>
>> Commit ba04c7c93bbc ("r8169: disable ASPM") uses
>> pci_disable_link_state() to disable ASPM. This is incorrect, if the
>> device really needs to disable ASPM, we should use a quirk in PCI core
>> to prevent the PCI core from setting ASPM altogether.
> I wouldn't call using pci_disable_link_state() in a driver incorrect
> (as it works), there is just a better way which is more in line with
> the PCI subsystem architecture.

Ok, I'll amend the commit log in next version.

>
>> Let's remove pci_disable_link_state() for now. Use PCI core quirks if
>> any regression happens.
> The vendor driver disables ASPM unconditionally for chip version 25
> (there it's METHOD_9), so I think ASPM support is broken in this chip
> version. I'll cook a PCI quirk.

I actually asked Ryankao about this. He said that variant is more then a  
decades old and he can't find why it doesn't support ASPM.

Since METHOD_9 might be a platform issue instead, my intention was to  
enable ASPM for all variants. If users hit any issue, then we can introduce  
new PCI quirks.

>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> Please note that netdev is closed currently. Once 4.18-RC1 is out it
> will be re-opened. Then please re-submit properly annotating PATCH
> with "net-next" (I've forgotten this often enough myself).

Will do for next version. Thanks!

Kai-Heng

>
>> ---
>> v2:
>> - Remove module parameter.
>> - Remove pci_disable_link_state().
>>
>>  drivers/net/ethernet/realtek/r8169.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c  
>> b/drivers/net/ethernet/realtek/r8169.c
>> index 75dfac0248f4..9b55ce513a36 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -25,7 +25,6 @@
>>  #include <linux/dma-mapping.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/firmware.h>
>> -#include <linux/pci-aspm.h>
>>  #include <linux/prefetch.h>
>>  #include <linux/ipv6.h>
>>  #include <net/ip6_checksum.h>
>> @@ -7647,10 +7646,6 @@ static int rtl_init_one(struct pci_dev *pdev,  
>> const struct pci_device_id *ent)
>>  	mii->reg_num_mask = 0x1f;
>>  	mii->supports_gmii = cfg->has_gmii;
>>
>> -	/* disable ASPM completely as that cause random device stop working
>> -	 * problems as well as full system hangs for some PCIe devices users */
>> -	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
>> -				     PCIE_LINK_STATE_CLKPM);
>>
>>  	/* enable device (incl. PCI PM wakeup and hotplug setup) */
>>  	rc = pcim_enable_device(pdev);