[PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Chunhao Lin]

This patch will enable RTL8125 ASPM L1.2 on the platforms that have
tested RTL8125 with ASPM L1.2 enabled.
Register mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
on L1.2 enabled platform. If it is, this register will be set to 0xf.
If not, this register will be default value 0.

Signed-off-by: Chunhao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 99 ++++++++++++++++++-----
 1 file changed, 79 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 19e2621e0645..b1e013969d4c 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp)
 			AcceptBroadcast | AcceptMulticast | AcceptMyPhys);
 }
 
-static void rtl_prepare_power_down(struct rtl8169_private *tp)
-{
-	if (tp->dash_type != RTL_DASH_NONE)
-		return;
-
-	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
-	    tp->mac_version == RTL_GIGA_MAC_VER_33)
-		rtl_ephy_write(tp, 0x19, 0xff64);
-
-	if (device_may_wakeup(tp_to_dev(tp))) {
-		phy_speed_down(tp->phydev, false);
-		rtl_wol_enable_rx(tp);
-	}
-}
-
 static void rtl_init_rxcfg(struct rtl8169_private *tp)
 {
 	switch (tp->mac_version) {
@@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
 	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);
 }
 
+static void rtl_disable_exit_l1(struct rtl8169_private *tp)
+{
+	/* Bits control which events trigger ASPM L1 exit:
+	 * Bit 12: rxdv
+	 * Bit 11: ltr_msg
+	 * Bit 10: txdma_poll
+	 * Bit  9: xadm
+	 * Bit  8: pktavi
+	 * Bit  7: txpla
+	 */
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
+		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
+		break;
+	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
+		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
+		break;
+	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
+		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
+		break;
+	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
+		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
+		break;
+	default:
+		break;
+	}
+}
+
 static void rtl_enable_exit_l1(struct rtl8169_private *tp)
 {
 	/* Bits control which events trigger ASPM L1 exit:
@@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 	udelay(10);
 }
 
+static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool enable)
+{
+	/* Don't enable L1.2 in the chip if OS can't control ASPM */
+	if (enable && tp->aspm_manageable) {
+		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
+		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
+	} else {
+		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
+	}
+}
+
+static void rtl_prepare_power_down(struct rtl8169_private *tp)
+{
+	if (tp->dash_type != RTL_DASH_NONE)
+		return;
+
+	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
+	    tp->mac_version == RTL_GIGA_MAC_VER_33)
+		rtl_ephy_write(tp, 0x19, 0xff64);
+
+	if (device_may_wakeup(tp_to_dev(tp))) {
+		rtl_disable_exit_l1(tp);
+		phy_speed_down(tp->phydev, false);
+		rtl_wol_enable_rx(tp);
+	}
+}
+
 static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
 			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)
 {
@@ -3675,6 +3715,7 @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
 	rtl_ephy_init(tp, e_info_8125b);
 	rtl_hw_start_8125_common(tp);
 
+	rtl_hw_aspm_l12_enable(tp, true);
 	rtl_hw_aspm_clkreq_enable(tp, true);
 }
 
@@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
 	rtl_rar_set(tp, mac_addr);
 }
 
+/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
+ * on L1.2 enabled platform. If it is, this register will be set to 0xf.
+ * If not, this register will be default value 0.
+ */
+static bool rtl_platform_l12_enabled(struct rtl8169_private *tp)
+{
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
+		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
+	default:
+		return false;
+	}
+}
+
 static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct rtl8169_private *tp;
@@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 * Chips from RTL8168h partially have issues with L1.2, but seem
 	 * to work fine with L1 and L1.1.
 	 */
-	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
-		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
-	else
-		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
-	tp->aspm_manageable = !rc;
+	if (!rtl_platform_l12_enabled(tp)) {
+		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
+			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
+		else
+			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
+		tp->aspm_manageable = !rc;
+	} else {
+		tp->aspm_manageable = pcie_aspm_enabled(pdev);
+	}
 
 	tp->dash_type = rtl_check_dash(tp);
 
-- 
2.25.1

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

Hi Hau,

On 24.01.2022 19:19, Chunhao Lin wrote:
> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> tested RTL8125 with ASPM L1.2 enabled.
> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> on L1.2 enabled platform. If it is, this register will be set to 0xf.
> If not, this register will be default value 0.
> 
Who and what defines which value this register has? The BIOS? ACPI? Mainboard vendors
test and can control the flagging? How about add-on cards and systems with other
boot loaders, e.g. SBC's with RTL8125 like Odroid H2+?

What is actually the critical component that makes L1.2 work or not with
RTL8125 on a particular system? The chipset? Or electrical characteristics?

The difference in power consumption between L1.1 and L1.2 is a few mW ([0]).
So I wonder whether it's worth it to add this flagging mechanism.
Or does it also impact reaching certain package power saving states?

[0] https://pcisig.com/making-most-pcie%C2%AE-low-power-features

> Signed-off-by: Chunhao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 99 ++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 19e2621e0645..b1e013969d4c 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp)
>  			AcceptBroadcast | AcceptMulticast | AcceptMyPhys);
>  }
>  
> -static void rtl_prepare_power_down(struct rtl8169_private *tp)
> -{
> -	if (tp->dash_type != RTL_DASH_NONE)
> -		return;
> -
> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> -		rtl_ephy_write(tp, 0x19, 0xff64);
> -
> -	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> -		rtl_wol_enable_rx(tp);
> -	}
> -}
> -
>  static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  {
>  	switch (tp->mac_version) {
> @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);
>  }
>  
> +static void rtl_disable_exit_l1(struct rtl8169_private *tp)
> +{

Why is this function needed? The chip should be quiet anyway.
IOW: What could be the impact of not having this function currently?
If it fixes something then it should be a separate patch.

> +	/* Bits control which events trigger ASPM L1 exit:
> +	 * Bit 12: rxdv
> +	 * Bit 11: ltr_msg
> +	 * Bit 10: txdma_poll
> +	 * Bit  9: xadm
> +	 * Bit  8: pktavi
> +	 * Bit  7: txpla
> +	 */
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> +		break;
> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> +		break;
> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> +		break;
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)
>  {
>  	/* Bits control which events trigger ASPM L1 exit:
> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  	udelay(10);
>  }
>  
> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool enable)
> +{

I assume this code works on RTL8125 only. Then this should be reflected in
the function naming, like we do it for other version-specific functions.

> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> +	if (enable && tp->aspm_manageable) {
> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> +	} else {
> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> +	}
> +}
> +
> +static void rtl_prepare_power_down(struct rtl8169_private *tp)
> +{
> +	if (tp->dash_type != RTL_DASH_NONE)
> +		return;
> +
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> +		rtl_ephy_write(tp, 0x19, 0xff64);
> +
> +	if (device_may_wakeup(tp_to_dev(tp))) {
> +		rtl_disable_exit_l1(tp);
> +		phy_speed_down(tp->phydev, false);
> +		rtl_wol_enable_rx(tp);
> +	}
> +}
> +
>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)
>  {
> @@ -3675,6 +3715,7 @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>  	rtl_ephy_init(tp, e_info_8125b);
>  	rtl_hw_start_8125_common(tp);
>  
> +	rtl_hw_aspm_l12_enable(tp, true);
>  	rtl_hw_aspm_clkreq_enable(tp, true);
>  }
>  
> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
>  	rtl_rar_set(tp, mac_addr);
>  }
>  
> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> + * If not, this register will be default value 0.
> + */
> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp)
> +{

The function name is misleading. It could be read as checking whether
the platform supports L1.2.

> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct rtl8169_private *tp;
> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>  	 * to work fine with L1 and L1.1.
>  	 */
> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> -	else
> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> -	tp->aspm_manageable = !rc;
> +	if (!rtl_platform_l12_enabled(tp)) {
> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> +			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> +		else
> +			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> +		tp->aspm_manageable = !rc;
> +	} else {
> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> +	}
>  

Better readable may be the following:

if (rtl_platform_l12_enabled(tp)) {
	tp->aspm_manageable = pcie_aspm_enabled(pdev);
} else if (tp->mac_version >= RTL_GIGA_MAC_VER_45) {
	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
	tp->aspm_manageable = !rc;
} else {
	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
	tp->aspm_manageable = !rc;
}

>  	tp->dash_type = rtl_check_dash(tp);
>  

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> On 24.01.2022 19:19, Chunhao Lin wrote:
> > This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> > tested RTL8125 with ASPM L1.2 enabled.
> > Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> > tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> > If not, this register will be default value 0.
> >
> Who and what defines which value this register has? The BIOS? ACPI?
> Mainboard vendors test and can control the flagging? How about add-on
> cards and systems with other boot loaders, e.g. SBC's with RTL8125 like
> Odroid H2+?
>
   Soc vendor can opt-in to enable these bits to enable L1.2 through programming tool/bios/uboot.
   Right now, there is no plan for set these bits for add-on card.

> What is actually the critical component that makes L1.2 work or not with
> RTL8125 on a particular system? The chipset? Or electrical characteristics?
>
   RTL8125 can support L1.2, but it disabled by r8169. So we create an option
   to let soc vendor can opn-in to enabled L1.2 with r8169.
   
> The difference in power consumption between L1.1 and L1.2 is a few mW
> ([0]).
> So I wonder whether it's worth it to add this flagging mechanism.
> Or does it also impact reaching certain package power saving states?
> 
   Upstream port also can save power when rtl8125 L1.2 is enabled.

> [0] https://pcisig.com/making-most-pcie%C2%AE-low-power-features
> 
> > Signed-off-by: Chunhao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 99
> > ++++++++++++++++++-----
> >  1 file changed, 79 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index 19e2621e0645..b1e013969d4c 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> rtl8169_private *tp)
> >  			AcceptBroadcast | AcceptMulticast |
> AcceptMyPhys);  }
> >
> > -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> > -	if (tp->dash_type != RTL_DASH_NONE)
> > -		return;
> > -
> > -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > -		rtl_ephy_write(tp, 0x19, 0xff64);
> > -
> > -	if (device_may_wakeup(tp_to_dev(tp))) {
> > -		phy_speed_down(tp->phydev, false);
> > -		rtl_wol_enable_rx(tp);
> > -	}
> > -}
> > -
> >  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >  	switch (tp->mac_version) {
> > @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct
> rtl8169_private *tp)
> >  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >
> > +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> 
> Why is this function needed? The chip should be quiet anyway.
> IOW: What could be the impact of not having this function currently?
> If it fixes something then it should be a separate patch.
> 
> > +	/* Bits control which events trigger ASPM L1 exit:
> > +	 * Bit 12: rxdv
> > +	 * Bit 11: ltr_msg
> > +	 * Bit 10: txdma_poll
> > +	 * Bit  9: xadm
> > +	 * Bit  8: pktavi
> > +	 * Bit  7: txpla
> > +	 */
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> > +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >  	/* Bits control which events trigger ASPM L1 exit:
> > @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
> rtl8169_private *tp, bool enable)
> >  	udelay(10);
> >  }
> >
> > +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
> > +enable) {
> 
> I assume this code works on RTL8125 only. Then this should be reflected in
> the function naming, like we do it for other version-specific functions.
> 
> > +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> > +	if (enable && tp->aspm_manageable) {
> > +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> > +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> > +	} else {
> > +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> > +	}
> > +}
> > +
> > +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> > +	if (tp->dash_type != RTL_DASH_NONE)
> > +		return;
> > +
> > +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > +		rtl_ephy_write(tp, 0x19, 0xff64);
> > +
> > +	if (device_may_wakeup(tp_to_dev(tp))) {
> > +		rtl_disable_exit_l1(tp);
> > +		phy_speed_down(tp->phydev, false);
> > +		rtl_wol_enable_rx(tp);
> > +	}
> > +}
> > +
> >  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> 3675,6 +3715,7
> > @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >  	rtl_ephy_init(tp, e_info_8125b);
> >  	rtl_hw_start_8125_common(tp);
> >
> > +	rtl_hw_aspm_l12_enable(tp, true);
> >  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >
> > @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> rtl8169_private *tp)
> >  	rtl_rar_set(tp, mac_addr);
> >  }
> >
> > +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> > + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> > + * If not, this register will be default value 0.
> > + */
> > +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> 
> The function name is misleading. It could be read as checking whether the
> platform supports L1.2.
> 
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> > +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static int rtl_init_one(struct pci_dev *pdev, const struct
> > pci_device_id *ent)  {
> >  	struct rtl8169_private *tp;
> > @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> >  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >  	 * to work fine with L1 and L1.1.
> >  	 */
> > -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> > -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> > -	else
> > -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > -	tp->aspm_manageable = !rc;
> > +	if (!rtl_platform_l12_enabled(tp)) {
> > +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> > +			rc = pci_disable_link_state(pdev,
> PCIE_LINK_STATE_L1_2);
> > +		else
> > +			rc = pci_disable_link_state(pdev,
> PCIE_LINK_STATE_L1);
> > +		tp->aspm_manageable = !rc;
> > +	} else {
> > +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> > +	}
> >
> 
> Better readable may be the following:
> 
> if (rtl_platform_l12_enabled(tp)) {
> 	tp->aspm_manageable = pcie_aspm_enabled(pdev); } else if (tp-
> >mac_version >= RTL_GIGA_MAC_VER_45) {
> 	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> 	tp->aspm_manageable = !rc;
> } else {
> 	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> 	tp->aspm_manageable = !rc;
> }
> 
> >  	tp->dash_type = rtl_check_dash(tp);
> >
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

On 25.01.2022 09:51, Hau wrote:
>> On 24.01.2022 19:19, Chunhao Lin wrote:
>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
>>> tested RTL8125 with ASPM L1.2 enabled.
>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>> If not, this register will be default value 0.
>>>
>> Who and what defines which value this register has? The BIOS? ACPI?
>> Mainboard vendors test and can control the flagging? How about add-on
>> cards and systems with other boot loaders, e.g. SBC's with RTL8125 like
>> Odroid H2+?
>>
>    Soc vendor can opt-in to enable these bits to enable L1.2 through programming tool/bios/uboot.
>    Right now, there is no plan for set these bits for add-on card.
> 
>> What is actually the critical component that makes L1.2 work or not with
>> RTL8125 on a particular system? The chipset? Or electrical characteristics?
>>
>    RTL8125 can support L1.2, but it disabled by r8169. So we create an option
>    to let soc vendor can opn-in to enabled L1.2 with r8169.
>    

Thanks, Hau. Still the question is open what's the root cause of L1.2 not working
with RTL8125 on *some* systems. I can't imagine that it just by chance works or not.
If we know which component conflicts with RTL8125 then maybe a PCI quirk could
be used.

>> The difference in power consumption between L1.1 and L1.2 is a few mW
>> ([0]).
>> So I wonder whether it's worth it to add this flagging mechanism.
>> Or does it also impact reaching certain package power saving states?
>>
>    Upstream port also can save power when rtl8125 L1.2 is enabled.
> 
>> [0] https://pcisig.com/making-most-pcie%C2%AE-low-power-features
>>
>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
>>> ++++++++++++++++++-----
>>>  1 file changed, 79 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>> index 19e2621e0645..b1e013969d4c 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
>> rtl8169_private *tp)
>>>  			AcceptBroadcast | AcceptMulticast |
>> AcceptMyPhys);  }
>>>
>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
>>> -	if (tp->dash_type != RTL_DASH_NONE)
>>> -		return;
>>> -
>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
>>> -
>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
>>> -		phy_speed_down(tp->phydev, false);
>>> -		rtl_wol_enable_rx(tp);
>>> -	}
>>> -}
>>> -
>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
>>>  	switch (tp->mac_version) {
>>> @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct
>> rtl8169_private *tp)
>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
>>>
>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
>>
>> Why is this function needed? The chip should be quiet anyway.
>> IOW: What could be the impact of not having this function currently?
>> If it fixes something then it should be a separate patch.
>>
This question would still be open.

>>> +	/* Bits control which events trigger ASPM L1 exit:
>>> +	 * Bit 12: rxdv
>>> +	 * Bit 11: ltr_msg
>>> +	 * Bit 10: txdma_poll
>>> +	 * Bit  9: xadm
>>> +	 * Bit  8: pktavi
>>> +	 * Bit  7: txpla
>>> +	 */
>>> +	switch (tp->mac_version) {
>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>> +}
>>> +
>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
>>>  	/* Bits control which events trigger ASPM L1 exit:
>>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
>> rtl8169_private *tp, bool enable)
>>>  	udelay(10);
>>>  }
>>>
>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
>>> +enable) {
>>
>> I assume this code works on RTL8125 only. Then this should be reflected in
>> the function naming, like we do it for other version-specific functions.
>>
>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
>>> +	if (enable && tp->aspm_manageable) {
>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>> +	} else {
>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>> +	}
>>> +}
>>> +
>>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
>>> +	if (tp->dash_type != RTL_DASH_NONE)
>>> +		return;
>>> +
>>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>> +		rtl_ephy_write(tp, 0x19, 0xff64);
>>> +
>>> +	if (device_may_wakeup(tp_to_dev(tp))) {
>>> +		rtl_disable_exit_l1(tp);
>>> +		phy_speed_down(tp->phydev, false);
>>> +		rtl_wol_enable_rx(tp);
>>> +	}
>>> +}
>>> +
>>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
>> 3675,6 +3715,7
>>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>>>  	rtl_ephy_init(tp, e_info_8125b);
>>>  	rtl_hw_start_8125_common(tp);
>>>
>>> +	rtl_hw_aspm_l12_enable(tp, true);
>>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
>>>
>>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
>> rtl8169_private *tp)
>>>  	rtl_rar_set(tp, mac_addr);
>>>  }
>>>
>>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
>>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>> + * If not, this register will be default value 0.
>>> + */
>>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
>>
>> The function name is misleading. It could be read as checking whether the
>> platform supports L1.2.
>>
>>> +	switch (tp->mac_version) {
>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>>  static int rtl_init_one(struct pci_dev *pdev, const struct
>>> pci_device_id *ent)  {
>>>  	struct rtl8169_private *tp;
>>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>>>  	 * to work fine with L1 and L1.1.
>>>  	 */
>>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>>> -	else
>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>>> -	tp->aspm_manageable = !rc;
>>> +	if (!rtl_platform_l12_enabled(tp)) {
>>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>> +			rc = pci_disable_link_state(pdev,
>> PCIE_LINK_STATE_L1_2);
>>> +		else
>>> +			rc = pci_disable_link_state(pdev,
>> PCIE_LINK_STATE_L1);
>>> +		tp->aspm_manageable = !rc;
>>> +	} else {
>>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
>>> +	}
>>>
>>
>> Better readable may be the following:
>>
>> if (rtl_platform_l12_enabled(tp)) {
>> 	tp->aspm_manageable = pcie_aspm_enabled(pdev); } else if (tp-
>>> mac_version >= RTL_GIGA_MAC_VER_45) {
>> 	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>> 	tp->aspm_manageable = !rc;
>> } else {
>> 	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>> 	tp->aspm_manageable = !rc;
>> }
>>
>>>  	tp->dash_type = rtl_check_dash(tp);
>>>
>>
>> ------Please consider the environment before printing this e-mail.

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> On 25.01.2022 09:51, Hau wrote:
> >> On 24.01.2022 19:19, Chunhao Lin wrote:
> >>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> >>> tested RTL8125 with ASPM L1.2 enabled.
> >>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>> If not, this register will be default value 0.
> >>>
> >> Who and what defines which value this register has? The BIOS? ACPI?
> >> Mainboard vendors test and can control the flagging? How about add-on
> >> cards and systems with other boot loaders, e.g. SBC's with RTL8125
> >> like Odroid H2+?
> >>
> >    Soc vendor can opt-in to enable these bits to enable L1.2 through
> programming tool/bios/uboot.
> >    Right now, there is no plan for set these bits for add-on card.
> >
> >> What is actually the critical component that makes L1.2 work or not
> >> with
> >> RTL8125 on a particular system? The chipset? Or electrical characteristics?
> >>
> >    RTL8125 can support L1.2, but it disabled by r8169. So we create an
> option
> >    to let soc vendor can opn-in to enabled L1.2 with r8169.
> >
> 
> Thanks, Hau. Still the question is open what's the root cause of L1.2 not
> working with RTL8125 on *some* systems. I can't imagine that it just by
> chance works or not.
> If we know which component conflicts with RTL8125 then maybe a PCI quirk
> could be used.
> 
  Most of them are hardware compatibility issues. It may related to refclk/clkreq/ltr... setting.
  Without the platform, it is hard to debug this kind of issue.

> >> The difference in power consumption between L1.1 and L1.2 is a few mW
> >> ([0]).
> >> So I wonder whether it's worth it to add this flagging mechanism.
> >> Or does it also impact reaching certain package power saving states?
> >>
> >    Upstream port also can save power when rtl8125 L1.2 is enabled.
> >
> >> [0] https://pcisig.com/making-most-pcie%C2%AE-low-power-features
> >>
> >>> Signed-off-by: Chunhao Lin <hau@realtek.com>
> >>> ---
> >>>  drivers/net/ethernet/realtek/r8169_main.c | 99
> >>> ++++++++++++++++++-----
> >>>  1 file changed, 79 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>> index 19e2621e0645..b1e013969d4c 100644
> >>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> >> rtl8169_private *tp)
> >>>  			AcceptBroadcast | AcceptMulticast |
> >> AcceptMyPhys);  }
> >>>
> >>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> >>> -	if (tp->dash_type != RTL_DASH_NONE)
> >>> -		return;
> >>> -
> >>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>> -		rtl_ephy_write(tp, 0x19, 0xff64);
> >>> -
> >>> -	if (device_may_wakeup(tp_to_dev(tp))) {
> >>> -		phy_speed_down(tp->phydev, false);
> >>> -		rtl_wol_enable_rx(tp);
> >>> -	}
> >>> -}
> >>> -
> >>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >>>  	switch (tp->mac_version) {
> >>> @@ -2650,6 +2635,34 @@ static void
> >>> rtl_pcie_state_l2l3_disable(struct
> >> rtl8169_private *tp)
> >>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >>>
> >>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> >>
> >> Why is this function needed? The chip should be quiet anyway.
> >> IOW: What could be the impact of not having this function currently?
> >> If it fixes something then it should be a separate patch.
> >>
> This question would still be open.
> 
> >>> +	/* Bits control which events trigger ASPM L1 exit:
> >>> +	 * Bit 12: rxdv
> >>> +	 * Bit 11: ltr_msg
> >>> +	 * Bit 10: txdma_poll
> >>> +	 * Bit  9: xadm
> >>> +	 * Bit  8: pktavi
> >>> +	 * Bit  7: txpla
> >>> +	 */
> >>> +	switch (tp->mac_version) {
> >>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> >>> +		break;
> >>> +	default:
> >>> +		break;
> >>> +	}
> >>> +}
> >>> +
> >>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >>>  	/* Bits control which events trigger ASPM L1 exit:
> >>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
> >> rtl8169_private *tp, bool enable)
> >>>  	udelay(10);
> >>>  }
> >>>
> >>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
> >>> +enable) {
> >>
> >> I assume this code works on RTL8125 only. Then this should be
> >> reflected in the function naming, like we do it for other version-specific
> functions.
> >>
> >>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> >>> +	if (enable && tp->aspm_manageable) {
> >>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>> +	} else {
> >>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>> +	}
> >>> +}
> >>> +
> >>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> >>> +	if (tp->dash_type != RTL_DASH_NONE)
> >>> +		return;
> >>> +
> >>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>> +		rtl_ephy_write(tp, 0x19, 0xff64);
> >>> +
> >>> +	if (device_may_wakeup(tp_to_dev(tp))) {
> >>> +		rtl_disable_exit_l1(tp);
> >>> +		phy_speed_down(tp->phydev, false);
> >>> +		rtl_wol_enable_rx(tp);
> >>> +	}
> >>> +}
> >>> +
> >>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> >> 3675,6 +3715,7
> >>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >>>  	rtl_ephy_init(tp, e_info_8125b);
> >>>  	rtl_hw_start_8125_common(tp);
> >>>
> >>> +	rtl_hw_aspm_l12_enable(tp, true);
> >>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >>>
> >>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> >> rtl8169_private *tp)
> >>>  	rtl_rar_set(tp, mac_addr);
> >>>  }
> >>>
> >>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> >>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>> + * If not, this register will be default value 0.
> >>> + */
> >>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> >>
> >> The function name is misleading. It could be read as checking whether
> >> the platform supports L1.2.
> >>
> >>> +	switch (tp->mac_version) {
> >>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> >>> +	default:
> >>> +		return false;
> >>> +	}
> >>> +}
> >>> +
> >>>  static int rtl_init_one(struct pci_dev *pdev, const struct
> >>> pci_device_id *ent)  {
> >>>  	struct rtl8169_private *tp;
> >>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev
> >>> *pdev,
> >> const struct pci_device_id *ent)
> >>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >>>  	 * to work fine with L1 and L1.1.
> >>>  	 */
> >>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> >>> -	else
> >>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> >>> -	tp->aspm_manageable = !rc;
> >>> +	if (!rtl_platform_l12_enabled(tp)) {
> >>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>> +			rc = pci_disable_link_state(pdev,
> >> PCIE_LINK_STATE_L1_2);
> >>> +		else
> >>> +			rc = pci_disable_link_state(pdev,
> >> PCIE_LINK_STATE_L1);
> >>> +		tp->aspm_manageable = !rc;
> >>> +	} else {
> >>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> >>> +	}
> >>>
> >>
> >> Better readable may be the following:
> >>
> >> if (rtl_platform_l12_enabled(tp)) {
> >> 	tp->aspm_manageable = pcie_aspm_enabled(pdev); } else if (tp-
> >>> mac_version >= RTL_GIGA_MAC_VER_45) {
> >> 	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> >> 	tp->aspm_manageable = !rc;
> >> } else {
> >> 	rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> >> 	tp->aspm_manageable = !rc;
> >> }
> >>
> >>>  	tp->dash_type = rtl_check_dash(tp);
> >>>
> >>
> >> ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

Hi Hau,

On 24.01.2022 19:19, Chunhao Lin wrote:
> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> tested RTL8125 with ASPM L1.2 enabled.
> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> on L1.2 enabled platform. If it is, this register will be set to 0xf.
> If not, this register will be default value 0.
> 
> Signed-off-by: Chunhao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 99 ++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 19e2621e0645..b1e013969d4c 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp)
>  			AcceptBroadcast | AcceptMulticast | AcceptMyPhys);
>  }
>  
> -static void rtl_prepare_power_down(struct rtl8169_private *tp)
> -{
> -	if (tp->dash_type != RTL_DASH_NONE)
> -		return;
> -
> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> -		rtl_ephy_write(tp, 0x19, 0xff64);
> -
> -	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> -		rtl_wol_enable_rx(tp);
> -	}
> -}
> -
>  static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  {
>  	switch (tp->mac_version) {
> @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);
>  }
>  
> +static void rtl_disable_exit_l1(struct rtl8169_private *tp)
> +{
> +	/* Bits control which events trigger ASPM L1 exit:
> +	 * Bit 12: rxdv
> +	 * Bit 11: ltr_msg
> +	 * Bit 10: txdma_poll
> +	 * Bit  9: xadm
> +	 * Bit  8: pktavi
> +	 * Bit  7: txpla
> +	 */
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> +		break;
> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> +		break;
> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> +		break;
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)
>  {
>  	/* Bits control which events trigger ASPM L1 exit:
> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  	udelay(10);
>  }
>  
> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool enable)
> +{
> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> +	if (enable && tp->aspm_manageable) {
> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> +	} else {
> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> +	}
> +}
> +

Register E094 bits 0..15 are cleared when enabling, but not touched
on disabling. I this correct?

And for basically the same purpose we have the following function.
"don't enable L1.2 in the chip" is not covered by ASPM_en in Config5?


static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
{
	/* Don't enable ASPM in the chip if OS can't control ASPM */
	if (enable && tp->aspm_manageable) {
		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
	} else {
		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
	}

	udelay(10);
}


> +static void rtl_prepare_power_down(struct rtl8169_private *tp)
> +{
> +	if (tp->dash_type != RTL_DASH_NONE)
> +		return;
> +
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> +		rtl_ephy_write(tp, 0x19, 0xff64);
> +
> +	if (device_may_wakeup(tp_to_dev(tp))) {
> +		rtl_disable_exit_l1(tp);
> +		phy_speed_down(tp->phydev, false);
> +		rtl_wol_enable_rx(tp);
> +	}
> +}
> +
>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)
>  {
> @@ -3675,6 +3715,7 @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>  	rtl_ephy_init(tp, e_info_8125b);
>  	rtl_hw_start_8125_common(tp);
>  
> +	rtl_hw_aspm_l12_enable(tp, true);
>  	rtl_hw_aspm_clkreq_enable(tp, true);
>  }
>  
> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
>  	rtl_rar_set(tp, mac_addr);
>  }
>  
> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> + * If not, this register will be default value 0.
> + */
> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp)
> +{
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct rtl8169_private *tp;
> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>  	 * to work fine with L1 and L1.1.
>  	 */
> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> -	else
> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> -	tp->aspm_manageable = !rc;
> +	if (!rtl_platform_l12_enabled(tp)) {
> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> +			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> +		else
> +			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> +		tp->aspm_manageable = !rc;
> +	} else {
> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> +	}
>  
>  	tp->dash_type = rtl_check_dash(tp);
>  

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

On 24.01.2022 19:19, Chunhao Lin wrote:
> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> tested RTL8125 with ASPM L1.2 enabled.
> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> on L1.2 enabled platform. If it is, this register will be set to 0xf.
> If not, this register will be default value 0.
> 
> Signed-off-by: Chunhao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 99 ++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 19e2621e0645..b1e013969d4c 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp)
>  			AcceptBroadcast | AcceptMulticast | AcceptMyPhys);
>  }
>  
> -static void rtl_prepare_power_down(struct rtl8169_private *tp)
> -{
> -	if (tp->dash_type != RTL_DASH_NONE)
> -		return;
> -
> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> -		rtl_ephy_write(tp, 0x19, 0xff64);
> -
> -	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> -		rtl_wol_enable_rx(tp);
> -	}
> -}
> -
>  static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  {
>  	switch (tp->mac_version) {
> @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);
>  }
>  
> +static void rtl_disable_exit_l1(struct rtl8169_private *tp)
> +{
> +	/* Bits control which events trigger ASPM L1 exit:
> +	 * Bit 12: rxdv
> +	 * Bit 11: ltr_msg
> +	 * Bit 10: txdma_poll
> +	 * Bit  9: xadm
> +	 * Bit  8: pktavi
> +	 * Bit  7: txpla
> +	 */
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> +		break;
> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> +		break;
> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> +		break;
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)
>  {
>  	/* Bits control which events trigger ASPM L1 exit:
> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  	udelay(10);
>  }
>  
> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool enable)
> +{
> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> +	if (enable && tp->aspm_manageable) {
> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> +	} else {
> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> +	}
> +}
> +
> +static void rtl_prepare_power_down(struct rtl8169_private *tp)
> +{
> +	if (tp->dash_type != RTL_DASH_NONE)
> +		return;
> +
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> +		rtl_ephy_write(tp, 0x19, 0xff64);
> +
> +	if (device_may_wakeup(tp_to_dev(tp))) {
> +		rtl_disable_exit_l1(tp);
> +		phy_speed_down(tp->phydev, false);
> +		rtl_wol_enable_rx(tp);
> +	}
> +}
> +
>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)
>  {
> @@ -3675,6 +3715,7 @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>  	rtl_ephy_init(tp, e_info_8125b);
>  	rtl_hw_start_8125_common(tp);
>  
> +	rtl_hw_aspm_l12_enable(tp, true);
>  	rtl_hw_aspm_clkreq_enable(tp, true);
>  }
>  
> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
>  	rtl_rar_set(tp, mac_addr);
>  }
>  
> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> + * If not, this register will be default value 0.
> + */
> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp)
> +{
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct rtl8169_private *tp;
> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>  	 * to work fine with L1 and L1.1.
>  	 */
> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> -	else
> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> -	tp->aspm_manageable = !rc;
> +	if (!rtl_platform_l12_enabled(tp)) {
> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> +			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> +		else
> +			rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> +		tp->aspm_manageable = !rc;
> +	} else {
> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> +	}
>  
>  	tp->dash_type = rtl_check_dash(tp);
>  

Hi Hau,

the following is a stripped-down version of the patch. Could you please check/test?
If function rtl_disable_exit_l1() is actually needed, I'd prefer to add it
in a separate patch (to facilitate bisecting).


 drivers/net/ethernet/realtek/r8169_main.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ca95e9266..890a64245 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2684,7 +2684,15 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 	if (enable && tp->aspm_manageable) {
 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
+
+		if (tp->mac_version == RTL_GIGA_MAC_VER_63) {
+			r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
+			r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
+		}
 	} else {
+		if (tp->mac_version == RTL_GIGA_MAC_VER_63)
+			r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
+
 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
 	}
@@ -5251,6 +5259,16 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
 	rtl_rar_set(tp, mac_addr);
 }
 
+/* register is set if system vendor successfully tested ASPM 1.2 */
+static bool rtl_aspm_is_safe(struct rtl8169_private *tp)
+{
+	if (tp->mac_version >= RTL_GIGA_MAC_VER_60 &&
+	    r8168_mac_ocp_read(tp, 0xc0b2) & 0xf)
+		return true;
+
+	return false;
+}
+
 static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct rtl8169_private *tp;
@@ -5329,7 +5347,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 * Chips from RTL8168h partially have issues with L1.2, but seem
 	 * to work fine with L1 and L1.1.
 	 */
-	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
+	if (rtl_aspm_is_safe(tp))
+		rc = 0;
+	else if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
 		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
 	else
 		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
-- 
2.35.0

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> On 24.01.2022 19:19, Chunhao Lin wrote:
> > This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> > tested RTL8125 with ASPM L1.2 enabled.
> > Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> > tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> > If not, this register will be default value 0.
> >
> > Signed-off-by: Chunhao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 99
> > ++++++++++++++++++-----
> >  1 file changed, 79 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index 19e2621e0645..b1e013969d4c 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> rtl8169_private *tp)
> >  			AcceptBroadcast | AcceptMulticast |
> AcceptMyPhys);  }
> >
> > -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> > -	if (tp->dash_type != RTL_DASH_NONE)
> > -		return;
> > -
> > -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > -		rtl_ephy_write(tp, 0x19, 0xff64);
> > -
> > -	if (device_may_wakeup(tp_to_dev(tp))) {
> > -		phy_speed_down(tp->phydev, false);
> > -		rtl_wol_enable_rx(tp);
> > -	}
> > -}
> > -
> >  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >  	switch (tp->mac_version) {
> > @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct
> rtl8169_private *tp)
> >  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >
> > +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> > +	/* Bits control which events trigger ASPM L1 exit:
> > +	 * Bit 12: rxdv
> > +	 * Bit 11: ltr_msg
> > +	 * Bit 10: txdma_poll
> > +	 * Bit  9: xadm
> > +	 * Bit  8: pktavi
> > +	 * Bit  7: txpla
> > +	 */
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> > +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >  	/* Bits control which events trigger ASPM L1 exit:
> > @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
> rtl8169_private *tp, bool enable)
> >  	udelay(10);
> >  }
> >
> > +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
> > +enable) {
> > +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> > +	if (enable && tp->aspm_manageable) {
> > +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> > +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> > +	} else {
> > +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> > +	}
> > +}
> > +
> 
> Register E094 bits 0..15 are cleared when enabling, but not touched on
> disabling. I this correct?
   Register E094 bits 8...15 is a timer counter that is used to control when to disable ephy tx/rx.
   Set it to 0 means disable ephy tx/rx immediately when certain condition meet. 
   It has no meaning when register E092 bit 2 is set to 0.

> And for basically the same purpose we have the following function.
> "don't enable L1.2 in the chip" is not covered by ASPM_en in Config5?
   Register E092 is like  ASPM_en in Config5. But it controls L1 substate (L1.1/L1.2) enable status.

> 
> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool
> enable) {
> 	/* Don't enable ASPM in the chip if OS can't control ASPM */
> 	if (enable && tp->aspm_manageable) {
> 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> 	} else {
> 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> 	}
> 
> 	udelay(10);
> }
> 
> 
> > +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> > +	if (tp->dash_type != RTL_DASH_NONE)
> > +		return;
> > +
> > +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > +		rtl_ephy_write(tp, 0x19, 0xff64);
> > +
> > +	if (device_may_wakeup(tp_to_dev(tp))) {
> > +		rtl_disable_exit_l1(tp);
> > +		phy_speed_down(tp->phydev, false);
> > +		rtl_wol_enable_rx(tp);
> > +	}
> > +}
> > +
> >  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> 3675,6 +3715,7
> > @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >  	rtl_ephy_init(tp, e_info_8125b);
> >  	rtl_hw_start_8125_common(tp);
> >
> > +	rtl_hw_aspm_l12_enable(tp, true);
> >  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >
> > @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> rtl8169_private *tp)
> >  	rtl_rar_set(tp, mac_addr);
> >  }
> >
> > +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> > + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> > + * If not, this register will be default value 0.
> > + */
> > +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> > +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static int rtl_init_one(struct pci_dev *pdev, const struct
> > pci_device_id *ent)  {
> >  	struct rtl8169_private *tp;
> > @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> >  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >  	 * to work fine with L1 and L1.1.
> >  	 */
> > -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> > -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> > -	else
> > -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > -	tp->aspm_manageable = !rc;
> > +	if (!rtl_platform_l12_enabled(tp)) {
> > +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> > +			rc = pci_disable_link_state(pdev,
> PCIE_LINK_STATE_L1_2);
> > +		else
> > +			rc = pci_disable_link_state(pdev,
> PCIE_LINK_STATE_L1);
> > +		tp->aspm_manageable = !rc;
> > +	} else {
> > +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> > +	}
> >
> >  	tp->dash_type = rtl_check_dash(tp);
> >
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

On 26.01.2022 10:02, Hau wrote:
> 
> 
>> On 24.01.2022 19:19, Chunhao Lin wrote:
>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
>>> tested RTL8125 with ASPM L1.2 enabled.
>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>> If not, this register will be default value 0.
>>>
>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
>>> ++++++++++++++++++-----
>>>  1 file changed, 79 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>> index 19e2621e0645..b1e013969d4c 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
>> rtl8169_private *tp)
>>>  			AcceptBroadcast | AcceptMulticast |
>> AcceptMyPhys);  }
>>>
>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
>>> -	if (tp->dash_type != RTL_DASH_NONE)
>>> -		return;
>>> -
>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
>>> -
>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
>>> -		phy_speed_down(tp->phydev, false);
>>> -		rtl_wol_enable_rx(tp);
>>> -	}
>>> -}
>>> -
>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
>>>  	switch (tp->mac_version) {
>>> @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct
>> rtl8169_private *tp)
>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
>>>
>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
>>> +	/* Bits control which events trigger ASPM L1 exit:
>>> +	 * Bit 12: rxdv
>>> +	 * Bit 11: ltr_msg
>>> +	 * Bit 10: txdma_poll
>>> +	 * Bit  9: xadm
>>> +	 * Bit  8: pktavi
>>> +	 * Bit  7: txpla
>>> +	 */
>>> +	switch (tp->mac_version) {
>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>> +}
>>> +
>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
>>>  	/* Bits control which events trigger ASPM L1 exit:
>>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
>> rtl8169_private *tp, bool enable)
>>>  	udelay(10);
>>>  }
>>>
>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
>>> +enable) {
>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
>>> +	if (enable && tp->aspm_manageable) {
>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>> +	} else {
>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>> +	}
>>> +}
>>> +
>>
>> Register E094 bits 0..15 are cleared when enabling, but not touched on
>> disabling. I this correct?
>    Register E094 bits 8...15 is a timer counter that is used to control when to disable ephy tx/rx.
>    Set it to 0 means disable ephy tx/rx immediately when certain condition meet. 
>    It has no meaning when register E092 bit 2 is set to 0.
> 
Thanks for the explanation.

>> And for basically the same purpose we have the following function.
>> "don't enable L1.2 in the chip" is not covered by ASPM_en in Config5?
>    Register E092 is like  ASPM_en in Config5. But it controls L1 substate (L1.1/L1.2) enable status.
> 
How is this handled for the RTL8168 chip versions supporting L1 sub-states (RTL8168h)?
Is there a similar register or does Config5 ASPM_en control also the L1 substates
on these chip versions?

>>
>> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool
>> enable) {
>> 	/* Don't enable ASPM in the chip if OS can't control ASPM */
>> 	if (enable && tp->aspm_manageable) {
>> 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
>> 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
>> 	} else {
>> 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>> 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>> 	}
>>
>> 	udelay(10);
>> }
>>
>>
...

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> On 26.01.2022 10:02, Hau wrote:
> >
> >
> >> On 24.01.2022 19:19, Chunhao Lin wrote:
> >>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> >>> tested RTL8125 with ASPM L1.2 enabled.
> >>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>> If not, this register will be default value 0.
> >>>
> >>> Signed-off-by: Chunhao Lin <hau@realtek.com>
> >>> ---
> >>>  drivers/net/ethernet/realtek/r8169_main.c | 99
> >>> ++++++++++++++++++-----
> >>>  1 file changed, 79 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>> index 19e2621e0645..b1e013969d4c 100644
> >>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> >> rtl8169_private *tp)
> >>>  			AcceptBroadcast | AcceptMulticast |
> >> AcceptMyPhys);  }
> >>>
> >>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> >>> -	if (tp->dash_type != RTL_DASH_NONE)
> >>> -		return;
> >>> -
> >>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>> -		rtl_ephy_write(tp, 0x19, 0xff64);
> >>> -
> >>> -	if (device_may_wakeup(tp_to_dev(tp))) {
> >>> -		phy_speed_down(tp->phydev, false);
> >>> -		rtl_wol_enable_rx(tp);
> >>> -	}
> >>> -}
> >>> -
> >>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >>>  	switch (tp->mac_version) {
> >>> @@ -2650,6 +2635,34 @@ static void
> >>> rtl_pcie_state_l2l3_disable(struct
> >> rtl8169_private *tp)
> >>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >>>
> >>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> >>> +	/* Bits control which events trigger ASPM L1 exit:
> >>> +	 * Bit 12: rxdv
> >>> +	 * Bit 11: ltr_msg
> >>> +	 * Bit 10: txdma_poll
> >>> +	 * Bit  9: xadm
> >>> +	 * Bit  8: pktavi
> >>> +	 * Bit  7: txpla
> >>> +	 */
> >>> +	switch (tp->mac_version) {
> >>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> >>> +		break;
> >>> +	default:
> >>> +		break;
> >>> +	}
> >>> +}
> >>> +
> >>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >>>  	/* Bits control which events trigger ASPM L1 exit:
> >>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
> >> rtl8169_private *tp, bool enable)
> >>>  	udelay(10);
> >>>  }
> >>>
> >>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
> >>> +enable) {
> >>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> >>> +	if (enable && tp->aspm_manageable) {
> >>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>> +	} else {
> >>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>> +	}
> >>> +}
> >>> +
> >>
> >> Register E094 bits 0..15 are cleared when enabling, but not touched
> >> on disabling. I this correct?
> >    Register E094 bits 8...15 is a timer counter that is used to control when to
> disable ephy tx/rx.
> >    Set it to 0 means disable ephy tx/rx immediately when certain condition
> meet.
> >    It has no meaning when register E092 bit 2 is set to 0.
> >
> Thanks for the explanation.
> 
> >> And for basically the same purpose we have the following function.
> >> "don't enable L1.2 in the chip" is not covered by ASPM_en in Config5?
> >    Register E092 is like  ASPM_en in Config5. But it controls L1 substate
> (L1.1/L1.2) enable status.
> >
> How is this handled for the RTL8168 chip versions supporting L1 sub-states
> (RTL8168h)?
> Is there a similar register or does Config5 ASPM_en control also the L1
> substates on these chip versions?
> 
You could apply the same setting on RTL8168H.

> >>
> >> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp,
> >> bool
> >> enable) {
> >> 	/* Don't enable ASPM in the chip if OS can't control ASPM */
> >> 	if (enable && tp->aspm_manageable) {
> >> 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> >> 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> >> 	} else {
> >> 		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> >> 		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> >> 	}
> >>
> >> 	udelay(10);
> >> }
> >>
> >>
> ...
> ------Please consider the environment before printing this e-mail.

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> On 24.01.2022 19:19, Chunhao Lin wrote:
> > This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> > tested RTL8125 with ASPM L1.2 enabled.
> > Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> > tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> > If not, this register will be default value 0.
> >
> > Signed-off-by: Chunhao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 99
> > ++++++++++++++++++-----
> >  1 file changed, 79 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index 19e2621e0645..b1e013969d4c 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> rtl8169_private *tp)
> >  			AcceptBroadcast | AcceptMulticast |
> AcceptMyPhys);  }
> >
> > -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> > -	if (tp->dash_type != RTL_DASH_NONE)
> > -		return;
> > -
> > -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > -		rtl_ephy_write(tp, 0x19, 0xff64);
> > -
> > -	if (device_may_wakeup(tp_to_dev(tp))) {
> > -		phy_speed_down(tp->phydev, false);
> > -		rtl_wol_enable_rx(tp);
> > -	}
> > -}
> > -
> >  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >  	switch (tp->mac_version) {
> > @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct
> rtl8169_private *tp)
> >  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >
> > +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> > +	/* Bits control which events trigger ASPM L1 exit:
> > +	 * Bit 12: rxdv
> > +	 * Bit 11: ltr_msg
> > +	 * Bit 10: txdma_poll
> > +	 * Bit  9: xadm
> > +	 * Bit  8: pktavi
> > +	 * Bit  7: txpla
> > +	 */
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> > +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> > +		break;
> > +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> > +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >  	/* Bits control which events trigger ASPM L1 exit:
> > @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
> rtl8169_private *tp, bool enable)
> >  	udelay(10);
> >  }
> >
> > +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
> > +enable) {
> > +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> > +	if (enable && tp->aspm_manageable) {
> > +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> > +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> > +	} else {
> > +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> > +	}
> > +}
> > +
> > +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> > +	if (tp->dash_type != RTL_DASH_NONE)
> > +		return;
> > +
> > +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> > +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> > +		rtl_ephy_write(tp, 0x19, 0xff64);
> > +
> > +	if (device_may_wakeup(tp_to_dev(tp))) {
> > +		rtl_disable_exit_l1(tp);
> > +		phy_speed_down(tp->phydev, false);
> > +		rtl_wol_enable_rx(tp);
> > +	}
> > +}
> > +
> >  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> 3675,6 +3715,7
> > @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >  	rtl_ephy_init(tp, e_info_8125b);
> >  	rtl_hw_start_8125_common(tp);
> >
> > +	rtl_hw_aspm_l12_enable(tp, true);
> >  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >
> > @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> rtl8169_private *tp)
> >  	rtl_rar_set(tp, mac_addr);
> >  }
> >
> > +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> > + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> > + * If not, this register will be default value 0.
> > + */
> > +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> > +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  static int rtl_init_one(struct pci_dev *pdev, const struct
> > pci_device_id *ent)  {
> >  	struct rtl8169_private *tp;
> > @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> >  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >  	 * to work fine with L1 and L1.1.
> >  	 */
> > -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> > -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> > -	else
> > -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> > -	tp->aspm_manageable = !rc;
> > +	if (!rtl_platform_l12_enabled(tp)) {
> > +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> > +			rc = pci_disable_link_state(pdev,
> PCIE_LINK_STATE_L1_2);
> > +		else
> > +			rc = pci_disable_link_state(pdev,
> PCIE_LINK_STATE_L1);
> > +		tp->aspm_manageable = !rc;
> > +	} else {
> > +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> > +	}
> >
> >  	tp->dash_type = rtl_check_dash(tp);
> >
> 
> Hi Hau,
> 
>the following is a stripped-down version of the patch. Could you please
> check/test?
This patch is ok. 
L1 substate lock can apply for both rtl8125a.rtl8125b.
if (enable && tp->aspm_manageable) {
	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);

	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
	}
} else {
	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);

	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
}

> If function rtl_disable_exit_l1() is actually needed, I'd prefer to add it in a
> separate patch (to facilitate bisecting).
> 
If exit l1 mask is enabled, hardware will prone to exit l1. That will prevent hardware from
entering l1 substate. So It needs to disable l1 exist mask when device go to d3 state
for entering l1 substate..

>  drivers/net/ethernet/realtek/r8169_main.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> b/drivers/net/ethernet/realtek/r8169_main.c
> index ca95e9266..890a64245 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2684,7 +2684,15 @@ static void rtl_hw_aspm_clkreq_enable(struct
> rtl8169_private *tp, bool enable)
>  	if (enable && tp->aspm_manageable) {
>  		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
>  		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> +
> +		if (tp->mac_version == RTL_GIGA_MAC_VER_63) {
> +			r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> +			r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> +		}
>  	} else {
> +		if (tp->mac_version == RTL_GIGA_MAC_VER_63)
> +			r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> +
>  		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>  		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
>  	}
> @@ -5251,6 +5259,16 @@ static void rtl_init_mac_address(struct
> rtl8169_private *tp)
>  	rtl_rar_set(tp, mac_addr);
>  }
> 
> +/* register is set if system vendor successfully tested ASPM 1.2 */
> +static bool rtl_aspm_is_safe(struct rtl8169_private *tp) {
> +	if (tp->mac_version >= RTL_GIGA_MAC_VER_60 &&
> +	    r8168_mac_ocp_read(tp, 0xc0b2) & 0xf)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)  {
>  	struct rtl8169_private *tp;
> @@ -5329,7 +5347,9 @@ static int rtl_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>  	 * to work fine with L1 and L1.1.
>  	 */
> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> +	if (rtl_aspm_is_safe(tp))
> +		rc = 0;
> +	else if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>  		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>  	else
>  		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> --
> 2.35.0
> 
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

On 26.01.2022 14:00, Hau wrote:
>> On 24.01.2022 19:19, Chunhao Lin wrote:
>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
>>> tested RTL8125 with ASPM L1.2 enabled.
>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>> If not, this register will be default value 0.
>>>
>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
>>> ++++++++++++++++++-----
>>>  1 file changed, 79 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>> index 19e2621e0645..b1e013969d4c 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
>> rtl8169_private *tp)
>>>  			AcceptBroadcast | AcceptMulticast |
>> AcceptMyPhys);  }
>>>
>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
>>> -	if (tp->dash_type != RTL_DASH_NONE)
>>> -		return;
>>> -
>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
>>> -
>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
>>> -		phy_speed_down(tp->phydev, false);
>>> -		rtl_wol_enable_rx(tp);
>>> -	}
>>> -}
>>> -
>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
>>>  	switch (tp->mac_version) {
>>> @@ -2650,6 +2635,34 @@ static void rtl_pcie_state_l2l3_disable(struct
>> rtl8169_private *tp)
>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
>>>
>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
>>> +	/* Bits control which events trigger ASPM L1 exit:
>>> +	 * Bit 12: rxdv
>>> +	 * Bit 11: ltr_msg
>>> +	 * Bit 10: txdma_poll
>>> +	 * Bit  9: xadm
>>> +	 * Bit  8: pktavi
>>> +	 * Bit  7: txpla
>>> +	 */
>>> +	switch (tp->mac_version) {
>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
>>> +		break;
>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
>>> +}
>>> +
>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
>>>  	/* Bits control which events trigger ASPM L1 exit:
>>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
>> rtl8169_private *tp, bool enable)
>>>  	udelay(10);
>>>  }
>>>
>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
>>> +enable) {
>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
>>> +	if (enable && tp->aspm_manageable) {
>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>> +	} else {
>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>> +	}
>>> +}
>>> +
>>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
>>> +	if (tp->dash_type != RTL_DASH_NONE)
>>> +		return;
>>> +
>>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>> +		rtl_ephy_write(tp, 0x19, 0xff64);
>>> +
>>> +	if (device_may_wakeup(tp_to_dev(tp))) {
>>> +		rtl_disable_exit_l1(tp);
>>> +		phy_speed_down(tp->phydev, false);
>>> +		rtl_wol_enable_rx(tp);
>>> +	}
>>> +}
>>> +
>>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
>> 3675,6 +3715,7
>>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>>>  	rtl_ephy_init(tp, e_info_8125b);
>>>  	rtl_hw_start_8125_common(tp);
>>>
>>> +	rtl_hw_aspm_l12_enable(tp, true);
>>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
>>>
>>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
>> rtl8169_private *tp)
>>>  	rtl_rar_set(tp, mac_addr);
>>>  }
>>>
>>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
>>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>> + * If not, this register will be default value 0.
>>> + */
>>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
>>> +	switch (tp->mac_version) {
>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>>  static int rtl_init_one(struct pci_dev *pdev, const struct
>>> pci_device_id *ent)  {
>>>  	struct rtl8169_private *tp;
>>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>>>  	 * to work fine with L1 and L1.1.
>>>  	 */
>>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>>> -	else
>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>>> -	tp->aspm_manageable = !rc;
>>> +	if (!rtl_platform_l12_enabled(tp)) {
>>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>> +			rc = pci_disable_link_state(pdev,
>> PCIE_LINK_STATE_L1_2);
>>> +		else
>>> +			rc = pci_disable_link_state(pdev,
>> PCIE_LINK_STATE_L1);
>>> +		tp->aspm_manageable = !rc;
>>> +	} else {
>>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
>>> +	}
>>>
>>>  	tp->dash_type = rtl_check_dash(tp);
>>>
>>
>> Hi Hau,
>>
>> the following is a stripped-down version of the patch. Could you please
>> check/test?
> This patch is ok. 
> L1 substate lock can apply for both rtl8125a.rtl8125b.
> if (enable && tp->aspm_manageable) {
> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> 
> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
> 		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> 	}
> } else {
> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> 
> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> }
> 
>> If function rtl_disable_exit_l1() is actually needed, I'd prefer to add it in a
>> separate patch (to facilitate bisecting).
>>
> If exit l1 mask is enabled, hardware will prone to exit l1. That will prevent hardware from
> entering l1 substate. So It needs to disable l1 exist mask when device go to d3 state
> for entering l1 substate..
> 
My understanding of PCI power management may be incomplete, but:
If a device goes to D3, then doesn't the bus go to L2/L3?
L1 exit criteria would be irrelevant then.

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> Sent: Wednesday, January 26, 2022 9:47 PM
> To: Hau <hau@realtek.com>; netdev@vger.kernel.org
> Cc: nic_swsd <nic_swsd@realtek.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2
> 
> On 26.01.2022 14:00, Hau wrote:
> >> On 24.01.2022 19:19, Chunhao Lin wrote:
> >>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
> >>> tested RTL8125 with ASPM L1.2 enabled.
> >>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>> If not, this register will be default value 0.
> >>>
> >>> Signed-off-by: Chunhao Lin <hau@realtek.com>
> >>> ---
> >>>  drivers/net/ethernet/realtek/r8169_main.c | 99
> >>> ++++++++++++++++++-----
> >>>  1 file changed, 79 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>> index 19e2621e0645..b1e013969d4c 100644
> >>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> >> rtl8169_private *tp)
> >>>  			AcceptBroadcast | AcceptMulticast |
> >> AcceptMyPhys);  }
> >>>
> >>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> >>> -	if (tp->dash_type != RTL_DASH_NONE)
> >>> -		return;
> >>> -
> >>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>> -		rtl_ephy_write(tp, 0x19, 0xff64);
> >>> -
> >>> -	if (device_may_wakeup(tp_to_dev(tp))) {
> >>> -		phy_speed_down(tp->phydev, false);
> >>> -		rtl_wol_enable_rx(tp);
> >>> -	}
> >>> -}
> >>> -
> >>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >>>  	switch (tp->mac_version) {
> >>> @@ -2650,6 +2635,34 @@ static void
> >>> rtl_pcie_state_l2l3_disable(struct
> >> rtl8169_private *tp)
> >>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >>>
> >>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> >>> +	/* Bits control which events trigger ASPM L1 exit:
> >>> +	 * Bit 12: rxdv
> >>> +	 * Bit 11: ltr_msg
> >>> +	 * Bit 10: txdma_poll
> >>> +	 * Bit  9: xadm
> >>> +	 * Bit  8: pktavi
> >>> +	 * Bit  7: txpla
> >>> +	 */
> >>> +	switch (tp->mac_version) {
> >>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> >>> +		break;
> >>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> >>> +		break;
> >>> +	default:
> >>> +		break;
> >>> +	}
> >>> +}
> >>> +
> >>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >>>  	/* Bits control which events trigger ASPM L1 exit:
> >>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
> >> rtl8169_private *tp, bool enable)
> >>>  	udelay(10);
> >>>  }
> >>>
> >>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
> >>> +enable) {
> >>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> >>> +	if (enable && tp->aspm_manageable) {
> >>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>> +	} else {
> >>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>> +	}
> >>> +}
> >>> +
> >>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> >>> +	if (tp->dash_type != RTL_DASH_NONE)
> >>> +		return;
> >>> +
> >>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>> +		rtl_ephy_write(tp, 0x19, 0xff64);
> >>> +
> >>> +	if (device_may_wakeup(tp_to_dev(tp))) {
> >>> +		rtl_disable_exit_l1(tp);
> >>> +		phy_speed_down(tp->phydev, false);
> >>> +		rtl_wol_enable_rx(tp);
> >>> +	}
> >>> +}
> >>> +
> >>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> >> 3675,6 +3715,7
> >>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >>>  	rtl_ephy_init(tp, e_info_8125b);
> >>>  	rtl_hw_start_8125_common(tp);
> >>>
> >>> +	rtl_hw_aspm_l12_enable(tp, true);
> >>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >>>
> >>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> >> rtl8169_private *tp)
> >>>  	rtl_rar_set(tp, mac_addr);
> >>>  }
> >>>
> >>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
> >>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>> + * If not, this register will be default value 0.
> >>> + */
> >>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> >>> +	switch (tp->mac_version) {
> >>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
> >>> +	default:
> >>> +		return false;
> >>> +	}
> >>> +}
> >>> +
> >>>  static int rtl_init_one(struct pci_dev *pdev, const struct
> >>> pci_device_id *ent)  {
> >>>  	struct rtl8169_private *tp;
> >>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev
> >>> *pdev,
> >> const struct pci_device_id *ent)
> >>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >>>  	 * to work fine with L1 and L1.1.
> >>>  	 */
> >>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> >>> -	else
> >>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> >>> -	tp->aspm_manageable = !rc;
> >>> +	if (!rtl_platform_l12_enabled(tp)) {
> >>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>> +			rc = pci_disable_link_state(pdev,
> >> PCIE_LINK_STATE_L1_2);
> >>> +		else
> >>> +			rc = pci_disable_link_state(pdev,
> >> PCIE_LINK_STATE_L1);
> >>> +		tp->aspm_manageable = !rc;
> >>> +	} else {
> >>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> >>> +	}
> >>>
> >>>  	tp->dash_type = rtl_check_dash(tp);
> >>>
> >>
> >> Hi Hau,
> >>
> >> the following is a stripped-down version of the patch. Could you
> >> please check/test?
> > This patch is ok.
> > L1 substate lock can apply for both rtl8125a.rtl8125b.
> > if (enable && tp->aspm_manageable) {
> > 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> > 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> >
> > 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
> > 		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> > 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> > 	}
> > } else {
> > 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
> > 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >
> > 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> > 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); }
> >
> >> If function rtl_disable_exit_l1() is actually needed, I'd prefer to
> >> add it in a separate patch (to facilitate bisecting).
> >>
> > If exit l1 mask is enabled, hardware will prone to exit l1. That will
> > prevent hardware from entering l1 substate. So It needs to disable l1
> > exist mask when device go to d3 state for entering l1 substate..
> >
> My understanding of PCI power management may be incomplete, but:
> If a device goes to D3, then doesn't the bus go to L2/L3?
> L1 exit criteria would be irrelevant then.
Your understanding is correct.
D3 is divided to two substate, D3hot and D3cold. D3cold will enter L2/L3.
D3hot may enter L1 or L2/L3 ready.  In D3hot case, enable exit l1 mask will
prevent hardware from entering PM L1. That is our hardware issue.
So we disable exit l1 mask before hardware enter D3.



> ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

On 26.01.2022 16:03, Hau wrote:
> 
> 
>> -----Original Message-----
>> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
>> Sent: Wednesday, January 26, 2022 9:47 PM
>> To: Hau <hau@realtek.com>; netdev@vger.kernel.org
>> Cc: nic_swsd <nic_swsd@realtek.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2
>>
>> On 26.01.2022 14:00, Hau wrote:
>>>> On 24.01.2022 19:19, Chunhao Lin wrote:
>>>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
>>>>> tested RTL8125 with ASPM L1.2 enabled.
>>>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
>>>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>>>> If not, this register will be default value 0.
>>>>>
>>>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
>>>>> ---
>>>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
>>>>> ++++++++++++++++++-----
>>>>>  1 file changed, 79 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>>>> index 19e2621e0645..b1e013969d4c 100644
>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
>>>> rtl8169_private *tp)
>>>>>  			AcceptBroadcast | AcceptMulticast |
>>>> AcceptMyPhys);  }
>>>>>
>>>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
>>>>> -	if (tp->dash_type != RTL_DASH_NONE)
>>>>> -		return;
>>>>> -
>>>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
>>>>> -
>>>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
>>>>> -		phy_speed_down(tp->phydev, false);
>>>>> -		rtl_wol_enable_rx(tp);
>>>>> -	}
>>>>> -}
>>>>> -
>>>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
>>>>>  	switch (tp->mac_version) {
>>>>> @@ -2650,6 +2635,34 @@ static void
>>>>> rtl_pcie_state_l2l3_disable(struct
>>>> rtl8169_private *tp)
>>>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
>>>>>
>>>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
>>>>> +	/* Bits control which events trigger ASPM L1 exit:
>>>>> +	 * Bit 12: rxdv
>>>>> +	 * Bit 11: ltr_msg
>>>>> +	 * Bit 10: txdma_poll
>>>>> +	 * Bit  9: xadm
>>>>> +	 * Bit  8: pktavi
>>>>> +	 * Bit  7: txpla
>>>>> +	 */
>>>>> +	switch (tp->mac_version) {
>>>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
>>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
>>>>> +		break;
>>>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
>>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
>>>>> +		break;
>>>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
>>>>> +		break;
>>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
>>>>> +		break;
>>>>> +	default:
>>>>> +		break;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
>>>>>  	/* Bits control which events trigger ASPM L1 exit:
>>>>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
>>>> rtl8169_private *tp, bool enable)
>>>>>  	udelay(10);
>>>>>  }
>>>>>
>>>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
>>>>> +enable) {
>>>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
>>>>> +	if (enable && tp->aspm_manageable) {
>>>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>>>> +	} else {
>>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
>>>>> +	if (tp->dash_type != RTL_DASH_NONE)
>>>>> +		return;
>>>>> +
>>>>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>>>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>>>> +		rtl_ephy_write(tp, 0x19, 0xff64);
>>>>> +
>>>>> +	if (device_may_wakeup(tp_to_dev(tp))) {
>>>>> +		rtl_disable_exit_l1(tp);
>>>>> +		phy_speed_down(tp->phydev, false);
>>>>> +		rtl_wol_enable_rx(tp);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>>>>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
>>>> 3675,6 +3715,7
>>>>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>>>>>  	rtl_ephy_init(tp, e_info_8125b);
>>>>>  	rtl_hw_start_8125_common(tp);
>>>>>
>>>>> +	rtl_hw_aspm_l12_enable(tp, true);
>>>>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
>>>>>
>>>>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
>>>> rtl8169_private *tp)
>>>>>  	rtl_rar_set(tp, mac_addr);
>>>>>  }
>>>>>
>>>>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
>>>>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>>>> + * If not, this register will be default value 0.
>>>>> + */
>>>>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
>>>>> +	switch (tp->mac_version) {
>>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>>>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static int rtl_init_one(struct pci_dev *pdev, const struct
>>>>> pci_device_id *ent)  {
>>>>>  	struct rtl8169_private *tp;
>>>>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev
>>>>> *pdev,
>>>> const struct pci_device_id *ent)
>>>>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>>>>>  	 * to work fine with L1 and L1.1.
>>>>>  	 */
>>>>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>>>>> -	else
>>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>>>>> -	tp->aspm_manageable = !rc;
>>>>> +	if (!rtl_platform_l12_enabled(tp)) {
>>>>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>>>> +			rc = pci_disable_link_state(pdev,
>>>> PCIE_LINK_STATE_L1_2);
>>>>> +		else
>>>>> +			rc = pci_disable_link_state(pdev,
>>>> PCIE_LINK_STATE_L1);
>>>>> +		tp->aspm_manageable = !rc;
>>>>> +	} else {
>>>>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
>>>>> +	}
>>>>>
>>>>>  	tp->dash_type = rtl_check_dash(tp);
>>>>>
>>>>
>>>> Hi Hau,
>>>>
>>>> the following is a stripped-down version of the patch. Could you
>>>> please check/test?
>>> This patch is ok.
>>> L1 substate lock can apply for both rtl8125a.rtl8125b.
>>> if (enable && tp->aspm_manageable) {
>>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
>>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
>>>
>>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
>>> 		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>> 	}
>>> } else {
>>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
>>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>>
>>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); }
>>>
>>>> If function rtl_disable_exit_l1() is actually needed, I'd prefer to
>>>> add it in a separate patch (to facilitate bisecting).
>>>>
>>> If exit l1 mask is enabled, hardware will prone to exit l1. That will
>>> prevent hardware from entering l1 substate. So It needs to disable l1
>>> exist mask when device go to d3 state for entering l1 substate..
>>>
>> My understanding of PCI power management may be incomplete, but:
>> If a device goes to D3, then doesn't the bus go to L2/L3?
>> L1 exit criteria would be irrelevant then.
> Your understanding is correct.
> D3 is divided to two substate, D3hot and D3cold. D3cold will enter L2/L3.
> D3hot may enter L1 or L2/L3 ready.  In D3hot case, enable exit l1 mask will
> prevent hardware from entering PM L1. That is our hardware issue.
> So we disable exit l1 mask before hardware enter D3.
> 

Thanks! One, hopefully last, question:
Are you aware of any boards/systems setting this "L1.2 was tested and is safe" flag?
Then this could be mentioned in the commit description.

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> 
> On 26.01.2022 16:03, Hau wrote:
> >
> >
> >> -----Original Message-----
> >> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> >> Sent: Wednesday, January 26, 2022 9:47 PM
> >> To: Hau <hau@realtek.com>; netdev@vger.kernel.org
> >> Cc: nic_swsd <nic_swsd@realtek.com>; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2
> >>
> >> On 26.01.2022 14:00, Hau wrote:
> >>>> On 24.01.2022 19:19, Chunhao Lin wrote:
> >>>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that
> >>>>> have tested RTL8125 with ASPM L1.2 enabled.
> >>>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>>>> If not, this register will be default value 0.
> >>>>>
> >>>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
> >>>>> ---
> >>>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
> >>>>> ++++++++++++++++++-----
> >>>>>  1 file changed, 79 insertions(+), 20 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> index 19e2621e0645..b1e013969d4c 100644
> >>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> >>>> rtl8169_private *tp)
> >>>>>  			AcceptBroadcast | AcceptMulticast |
> >>>> AcceptMyPhys);  }
> >>>>>
> >>>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> >>>>> -	if (tp->dash_type != RTL_DASH_NONE)
> >>>>> -		return;
> >>>>> -
> >>>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
> >>>>> -
> >>>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
> >>>>> -		phy_speed_down(tp->phydev, false);
> >>>>> -		rtl_wol_enable_rx(tp);
> >>>>> -	}
> >>>>> -}
> >>>>> -
> >>>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >>>>>  	switch (tp->mac_version) {
> >>>>> @@ -2650,6 +2635,34 @@ static void
> >>>>> rtl_pcie_state_l2l3_disable(struct
> >>>> rtl8169_private *tp)
> >>>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >>>>>
> >>>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> >>>>> +	/* Bits control which events trigger ASPM L1 exit:
> >>>>> +	 * Bit 12: rxdv
> >>>>> +	 * Bit 11: ltr_msg
> >>>>> +	 * Bit 10: txdma_poll
> >>>>> +	 * Bit  9: xadm
> >>>>> +	 * Bit  8: pktavi
> >>>>> +	 * Bit  7: txpla
> >>>>> +	 */
> >>>>> +	switch (tp->mac_version) {
> >>>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> >>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> >>>>> +		break;
> >>>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> >>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> >>>>> +		break;
> >>>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> >>>>> +		break;
> >>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> >>>>> +		break;
> >>>>> +	default:
> >>>>> +		break;
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >>>>>  	/* Bits control which events trigger ASPM L1 exit:
> >>>>> @@ -2692,6 +2705,33 @@ static void
> >>>>> rtl_hw_aspm_clkreq_enable(struct
> >>>> rtl8169_private *tp, bool enable)
> >>>>>  	udelay(10);
> >>>>>  }
> >>>>>
> >>>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp,
> >>>>> +bool
> >>>>> +enable) {
> >>>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> >>>>> +	if (enable && tp->aspm_manageable) {
> >>>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>>>> +	} else {
> >>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> >>>>> +	if (tp->dash_type != RTL_DASH_NONE)
> >>>>> +		return;
> >>>>> +
> >>>>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>>>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>>>> +		rtl_ephy_write(tp, 0x19, 0xff64);
> >>>>> +
> >>>>> +	if (device_may_wakeup(tp_to_dev(tp))) {
> >>>>> +		rtl_disable_exit_l1(tp);
> >>>>> +		phy_speed_down(tp->phydev, false);
> >>>>> +		rtl_wol_enable_rx(tp);
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >>>>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> >>>> 3675,6 +3715,7
> >>>>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >>>>>  	rtl_ephy_init(tp, e_info_8125b);
> >>>>>  	rtl_hw_start_8125_common(tp);
> >>>>>
> >>>>> +	rtl_hw_aspm_l12_enable(tp, true);
> >>>>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >>>>>
> >>>>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> >>>> rtl8169_private *tp)
> >>>>>  	rtl_rar_set(tp, mac_addr);
> >>>>>  }
> >>>>>
> >>>>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>>>> +tested
> >>>>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>>>> + * If not, this register will be default value 0.
> >>>>> + */
> >>>>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> >>>>> +	switch (tp->mac_version) {
> >>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>>>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ?
> true : false;
> >>>>> +	default:
> >>>>> +		return false;
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static int rtl_init_one(struct pci_dev *pdev, const struct
> >>>>> pci_device_id *ent)  {
> >>>>>  	struct rtl8169_private *tp;
> >>>>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev
> >>>>> *pdev,
> >>>> const struct pci_device_id *ent)
> >>>>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >>>>>  	 * to work fine with L1 and L1.1.
> >>>>>  	 */
> >>>>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> >>>>> -	else
> >>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> >>>>> -	tp->aspm_manageable = !rc;
> >>>>> +	if (!rtl_platform_l12_enabled(tp)) {
> >>>>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>>>> +			rc = pci_disable_link_state(pdev,
> >>>> PCIE_LINK_STATE_L1_2);
> >>>>> +		else
> >>>>> +			rc = pci_disable_link_state(pdev,
> >>>> PCIE_LINK_STATE_L1);
> >>>>> +		tp->aspm_manageable = !rc;
> >>>>> +	} else {
> >>>>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> >>>>> +	}
> >>>>>
> >>>>>  	tp->dash_type = rtl_check_dash(tp);
> >>>>>
> >>>>
> >>>> Hi Hau,
> >>>>
> >>>> the following is a stripped-down version of the patch. Could you
> >>>> please check/test?
> >>> This patch is ok.
> >>> L1 substate lock can apply for both rtl8125a.rtl8125b.
> >>> if (enable && tp->aspm_manageable) {
> >>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> >>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> >>>
> >>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
> >>> 		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>> 	}
> >>> } else {
> >>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
> >>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>>
> >>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> >>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); }
> >>>
> >>>> If function rtl_disable_exit_l1() is actually needed, I'd prefer to
> >>>> add it in a separate patch (to facilitate bisecting).
> >>>>
> >>> If exit l1 mask is enabled, hardware will prone to exit l1. That
> >>> will prevent hardware from entering l1 substate. So It needs to
> >>> disable l1 exist mask when device go to d3 state for entering l1 substate..
> >>>
> >> My understanding of PCI power management may be incomplete, but:
> >> If a device goes to D3, then doesn't the bus go to L2/L3?
> >> L1 exit criteria would be irrelevant then.
> > Your understanding is correct.
> > D3 is divided to two substate, D3hot and D3cold. D3cold will enter L2/L3.
> > D3hot may enter L1 or L2/L3 ready.  In D3hot case, enable exit l1 mask
> > will prevent hardware from entering PM L1. That is our hardware issue.
> > So we disable exit l1 mask before hardware enter D3.
> >
> 
> Thanks! One, hopefully last, question:
> Are you aware of any boards/systems setting this "L1.2 was tested and is
> safe" flag?
> Then this could be mentioned in the commit description.
Google chromebox will be the first one to use this flag.

> ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Heiner Kallweit]

On 26.01.2022 16:03, Hau wrote:
> 
> 
>> -----Original Message-----
>> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
>> Sent: Wednesday, January 26, 2022 9:47 PM
>> To: Hau <hau@realtek.com>; netdev@vger.kernel.org
>> Cc: nic_swsd <nic_swsd@realtek.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2
>>
>> On 26.01.2022 14:00, Hau wrote:
>>>> On 24.01.2022 19:19, Chunhao Lin wrote:
>>>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that have
>>>>> tested RTL8125 with ASPM L1.2 enabled.
>>>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
>>>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>>>> If not, this register will be default value 0.
>>>>>
>>>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
>>>>> ---
>>>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
>>>>> ++++++++++++++++++-----
>>>>>  1 file changed, 79 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>>>> index 19e2621e0645..b1e013969d4c 100644
>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
>>>> rtl8169_private *tp)
>>>>>  			AcceptBroadcast | AcceptMulticast |
>>>> AcceptMyPhys);  }
>>>>>
>>>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
>>>>> -	if (tp->dash_type != RTL_DASH_NONE)
>>>>> -		return;
>>>>> -
>>>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
>>>>> -
>>>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
>>>>> -		phy_speed_down(tp->phydev, false);
>>>>> -		rtl_wol_enable_rx(tp);
>>>>> -	}
>>>>> -}
>>>>> -
>>>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
>>>>>  	switch (tp->mac_version) {
>>>>> @@ -2650,6 +2635,34 @@ static void
>>>>> rtl_pcie_state_l2l3_disable(struct
>>>> rtl8169_private *tp)
>>>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
>>>>>
>>>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
>>>>> +	/* Bits control which events trigger ASPM L1 exit:
>>>>> +	 * Bit 12: rxdv
>>>>> +	 * Bit 11: ltr_msg
>>>>> +	 * Bit 10: txdma_poll
>>>>> +	 * Bit  9: xadm
>>>>> +	 * Bit  8: pktavi
>>>>> +	 * Bit  7: txpla
>>>>> +	 */
>>>>> +	switch (tp->mac_version) {
>>>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
>>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
>>>>> +		break;
>>>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
>>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
>>>>> +		break;
>>>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
>>>>> +		break;
>>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
>>>>> +		break;
>>>>> +	default:
>>>>> +		break;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
>>>>>  	/* Bits control which events trigger ASPM L1 exit:
>>>>> @@ -2692,6 +2705,33 @@ static void rtl_hw_aspm_clkreq_enable(struct
>>>> rtl8169_private *tp, bool enable)
>>>>>  	udelay(10);
>>>>>  }
>>>>>
>>>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp, bool
>>>>> +enable) {
>>>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
>>>>> +	if (enable && tp->aspm_manageable) {
>>>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>>>> +	} else {
>>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
>>>>> +	if (tp->dash_type != RTL_DASH_NONE)
>>>>> +		return;
>>>>> +
>>>>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>>>>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
>>>>> +		rtl_ephy_write(tp, 0x19, 0xff64);
>>>>> +
>>>>> +	if (device_may_wakeup(tp_to_dev(tp))) {
>>>>> +		rtl_disable_exit_l1(tp);
>>>>> +		phy_speed_down(tp->phydev, false);
>>>>> +		rtl_wol_enable_rx(tp);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
>>>>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
>>>> 3675,6 +3715,7
>>>>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
>>>>>  	rtl_ephy_init(tp, e_info_8125b);
>>>>>  	rtl_hw_start_8125_common(tp);
>>>>>
>>>>> +	rtl_hw_aspm_l12_enable(tp, true);
>>>>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
>>>>>
>>>>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
>>>> rtl8169_private *tp)
>>>>>  	rtl_rar_set(tp, mac_addr);
>>>>>  }
>>>>>
>>>>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been tested
>>>>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
>>>>> + * If not, this register will be default value 0.
>>>>> + */
>>>>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
>>>>> +	switch (tp->mac_version) {
>>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
>>>>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ? true : false;
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static int rtl_init_one(struct pci_dev *pdev, const struct
>>>>> pci_device_id *ent)  {
>>>>>  	struct rtl8169_private *tp;
>>>>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev
>>>>> *pdev,
>>>> const struct pci_device_id *ent)
>>>>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
>>>>>  	 * to work fine with L1 and L1.1.
>>>>>  	 */
>>>>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>>>>> -	else
>>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>>>>> -	tp->aspm_manageable = !rc;
>>>>> +	if (!rtl_platform_l12_enabled(tp)) {
>>>>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
>>>>> +			rc = pci_disable_link_state(pdev,
>>>> PCIE_LINK_STATE_L1_2);
>>>>> +		else
>>>>> +			rc = pci_disable_link_state(pdev,
>>>> PCIE_LINK_STATE_L1);
>>>>> +		tp->aspm_manageable = !rc;
>>>>> +	} else {
>>>>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
>>>>> +	}
>>>>>
>>>>>  	tp->dash_type = rtl_check_dash(tp);
>>>>>
>>>>
>>>> Hi Hau,
>>>>
>>>> the following is a stripped-down version of the patch. Could you
>>>> please check/test?
>>> This patch is ok.
>>> L1 substate lock can apply for both rtl8125a.rtl8125b.
>>> if (enable && tp->aspm_manageable) {
>>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
>>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
>>>
>>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
>>> 		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
>>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
>>> 	}
>>> } else {
>>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
>>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>>>
>>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
>>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); }
>>>
>>>> If function rtl_disable_exit_l1() is actually needed, I'd prefer to
>>>> add it in a separate patch (to facilitate bisecting).
>>>>
>>> If exit l1 mask is enabled, hardware will prone to exit l1. That will
>>> prevent hardware from entering l1 substate. So It needs to disable l1
>>> exist mask when device go to d3 state for entering l1 substate..
>>>
>> My understanding of PCI power management may be incomplete, but:
>> If a device goes to D3, then doesn't the bus go to L2/L3?
>> L1 exit criteria would be irrelevant then.
> Your understanding is correct.
> D3 is divided to two substate, D3hot and D3cold. D3cold will enter L2/L3.
> D3hot may enter L1 or L2/L3 ready.  In D3hot case, enable exit l1 mask will
> prevent hardware from entering PM L1. That is our hardware issue.
> So we disable exit l1 mask before hardware enter D3.
> 
> 
I submitted the patch to enable L1.2 if tested with your Suggested-by.
One last question before submitting the disable_exit_l1 patch.

Depending on the chip version only certain L1 exit bits are set.

+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
+		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
+		break;
+	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
+		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
+		break;
+	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
+		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
+		break;
+	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
+		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
+		break;

Would it be safe to shorten this to the following? Or is some bit
in this range used for another purpose on certain chip versions?

+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38:
+	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
+		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
+		break;
+	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
+		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
+		break;

RE: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2

[Hau]

> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> Sent: Thursday, January 27, 2022 3:58 AM
> To: Hau <hau@realtek.com>; netdev@vger.kernel.org
> Cc: nic_swsd <nic_swsd@realtek.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2
> 
> On 26.01.2022 16:03, Hau wrote:
> >
> >
> >> -----Original Message-----
> >> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> >> Sent: Wednesday, January 26, 2022 9:47 PM
> >> To: Hau <hau@realtek.com>; netdev@vger.kernel.org
> >> Cc: nic_swsd <nic_swsd@realtek.com>; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH net-next 1/1] r8169: enable RTL8125 ASPM L1.2
> >>
> >> On 26.01.2022 14:00, Hau wrote:
> >>>> On 24.01.2022 19:19, Chunhao Lin wrote:
> >>>>> This patch will enable RTL8125 ASPM L1.2 on the platforms that
> >>>>> have tested RTL8125 with ASPM L1.2 enabled.
> >>>>> Register mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>>>> tested on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>>>> If not, this register will be default value 0.
> >>>>>
> >>>>> Signed-off-by: Chunhao Lin <hau@realtek.com>
> >>>>> ---
> >>>>>  drivers/net/ethernet/realtek/r8169_main.c | 99
> >>>>> ++++++++++++++++++-----
> >>>>>  1 file changed, 79 insertions(+), 20 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> index 19e2621e0645..b1e013969d4c 100644
> >>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>>>> @@ -2238,21 +2238,6 @@ static void rtl_wol_enable_rx(struct
> >>>> rtl8169_private *tp)
> >>>>>  			AcceptBroadcast | AcceptMulticast |
> >>>> AcceptMyPhys);  }
> >>>>>
> >>>>> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{
> >>>>> -	if (tp->dash_type != RTL_DASH_NONE)
> >>>>> -		return;
> >>>>> -
> >>>>> -	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>>>> -	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>>>> -		rtl_ephy_write(tp, 0x19, 0xff64);
> >>>>> -
> >>>>> -	if (device_may_wakeup(tp_to_dev(tp))) {
> >>>>> -		phy_speed_down(tp->phydev, false);
> >>>>> -		rtl_wol_enable_rx(tp);
> >>>>> -	}
> >>>>> -}
> >>>>> -
> >>>>>  static void rtl_init_rxcfg(struct rtl8169_private *tp)  {
> >>>>>  	switch (tp->mac_version) {
> >>>>> @@ -2650,6 +2635,34 @@ static void
> >>>>> rtl_pcie_state_l2l3_disable(struct
> >>>> rtl8169_private *tp)
> >>>>>  	RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Rdy_to_L23);  }
> >>>>>
> >>>>> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) {
> >>>>> +	/* Bits control which events trigger ASPM L1 exit:
> >>>>> +	 * Bit 12: rxdv
> >>>>> +	 * Bit 11: ltr_msg
> >>>>> +	 * Bit 10: txdma_poll
> >>>>> +	 * Bit  9: xadm
> >>>>> +	 * Bit  8: pktavi
> >>>>> +	 * Bit  7: txpla
> >>>>> +	 */
> >>>>> +	switch (tp->mac_version) {
> >>>>> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> >>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> >>>>> +		break;
> >>>>> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> >>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> >>>>> +		break;
> >>>>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> >>>>> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> >>>>> +		break;
> >>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>>>> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> >>>>> +		break;
> >>>>> +	default:
> >>>>> +		break;
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static void rtl_enable_exit_l1(struct rtl8169_private *tp)  {
> >>>>>  	/* Bits control which events trigger ASPM L1 exit:
> >>>>> @@ -2692,6 +2705,33 @@ static void
> >>>>> rtl_hw_aspm_clkreq_enable(struct
> >>>> rtl8169_private *tp, bool enable)
> >>>>>  	udelay(10);
> >>>>>  }
> >>>>>
> >>>>> +static void rtl_hw_aspm_l12_enable(struct rtl8169_private *tp,
> >>>>> +bool
> >>>>> +enable) {
> >>>>> +	/* Don't enable L1.2 in the chip if OS can't control ASPM */
> >>>>> +	if (enable && tp->aspm_manageable) {
> >>>>> +		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>>>> +	} else {
> >>>>> +		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>> +static void rtl_prepare_power_down(struct rtl8169_private *tp) {
> >>>>> +	if (tp->dash_type != RTL_DASH_NONE)
> >>>>> +		return;
> >>>>> +
> >>>>> +	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> >>>>> +	    tp->mac_version == RTL_GIGA_MAC_VER_33)
> >>>>> +		rtl_ephy_write(tp, 0x19, 0xff64);
> >>>>> +
> >>>>> +	if (device_may_wakeup(tp_to_dev(tp))) {
> >>>>> +		rtl_disable_exit_l1(tp);
> >>>>> +		phy_speed_down(tp->phydev, false);
> >>>>> +		rtl_wol_enable_rx(tp);
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
> >>>>>  			      u16 tx_stat, u16 rx_dyn, u16 tx_dyn)  { @@ -
> >>>> 3675,6 +3715,7
> >>>>> @@ static void rtl_hw_start_8125b(struct rtl8169_private *tp)
> >>>>>  	rtl_ephy_init(tp, e_info_8125b);
> >>>>>  	rtl_hw_start_8125_common(tp);
> >>>>>
> >>>>> +	rtl_hw_aspm_l12_enable(tp, true);
> >>>>>  	rtl_hw_aspm_clkreq_enable(tp, true);  }
> >>>>>
> >>>>> @@ -5255,6 +5296,20 @@ static void rtl_init_mac_address(struct
> >>>> rtl8169_private *tp)
> >>>>>  	rtl_rar_set(tp, mac_addr);
> >>>>>  }
> >>>>>
> >>>>> +/* mac ocp 0xc0b2 will help to identify if RTL8125 has been
> >>>>> +tested
> >>>>> + * on L1.2 enabled platform. If it is, this register will be set to 0xf.
> >>>>> + * If not, this register will be default value 0.
> >>>>> + */
> >>>>> +static bool rtl_platform_l12_enabled(struct rtl8169_private *tp) {
> >>>>> +	switch (tp->mac_version) {
> >>>>> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> >>>>> +		return (r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) ?
> true : false;
> >>>>> +	default:
> >>>>> +		return false;
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static int rtl_init_one(struct pci_dev *pdev, const struct
> >>>>> pci_device_id *ent)  {
> >>>>>  	struct rtl8169_private *tp;
> >>>>> @@ -5333,11 +5388,15 @@ static int rtl_init_one(struct pci_dev
> >>>>> *pdev,
> >>>> const struct pci_device_id *ent)
> >>>>>  	 * Chips from RTL8168h partially have issues with L1.2, but seem
> >>>>>  	 * to work fine with L1 and L1.1.
> >>>>>  	 */
> >>>>> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
> >>>>> -	else
> >>>>> -		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
> >>>>> -	tp->aspm_manageable = !rc;
> >>>>> +	if (!rtl_platform_l12_enabled(tp)) {
> >>>>> +		if (tp->mac_version >= RTL_GIGA_MAC_VER_45)
> >>>>> +			rc = pci_disable_link_state(pdev,
> >>>> PCIE_LINK_STATE_L1_2);
> >>>>> +		else
> >>>>> +			rc = pci_disable_link_state(pdev,
> >>>> PCIE_LINK_STATE_L1);
> >>>>> +		tp->aspm_manageable = !rc;
> >>>>> +	} else {
> >>>>> +		tp->aspm_manageable = pcie_aspm_enabled(pdev);
> >>>>> +	}
> >>>>>
> >>>>>  	tp->dash_type = rtl_check_dash(tp);
> >>>>>
> >>>>
> >>>> Hi Hau,
> >>>>
> >>>> the following is a stripped-down version of the patch. Could you
> >>>> please check/test?
> >>> This patch is ok.
> >>> L1 substate lock can apply for both rtl8125a.rtl8125b.
> >>> if (enable && tp->aspm_manageable) {
> >>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> >>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> >>>
> >>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60) {
> >>> 		r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> >>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, BIT(2));
> >>> 	}
> >>> } else {
> >>> 	if (tp->mac_version >= RTL_GIGA_MAC_VER_60)
> >>> 		r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
> >>>
> >>> 	RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> >>> 	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en); }
> >>>
> >>>> If function rtl_disable_exit_l1() is actually needed, I'd prefer to
> >>>> add it in a separate patch (to facilitate bisecting).
> >>>>
> >>> If exit l1 mask is enabled, hardware will prone to exit l1. That
> >>> will prevent hardware from entering l1 substate. So It needs to
> >>> disable l1 exist mask when device go to d3 state for entering l1 substate..
> >>>
> >> My understanding of PCI power management may be incomplete, but:
> >> If a device goes to D3, then doesn't the bus go to L2/L3?
> >> L1 exit criteria would be irrelevant then.
> > Your understanding is correct.
> > D3 is divided to two substate, D3hot and D3cold. D3cold will enter L2/L3.
> > D3hot may enter L1 or L2/L3 ready.  In D3hot case, enable exit l1 mask
> > will prevent hardware from entering PM L1. That is our hardware issue.
> > So we disable exit l1 mask before hardware enter D3.
> >
> >
> I submitted the patch to enable L1.2 if tested with your Suggested-by.
> One last question before submitting the disable_exit_l1 patch.
> 
> Depending on the chip version only certain L1 exit bits are set.
> 
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_36:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
> +		break;
> +	case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x0c00);
> +		break;
> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> +		break;
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> +		break;
> 
> Would it be safe to shorten this to the following? Or is some bit in this range
> used for another purpose on certain chip versions?

Bit7 has different purpose for chip ver 34 to chip ver 38.
But for chip after rtl8168g, eri 0xd4 is mapped to mac ocp 0xc0ac.
The code can be shorten as following.
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38:
			 rtl_eri_clear_bits(tp, 0xd4, 0x1f00);
			 break;
case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
			 r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
			 break;

> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38:
> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
> +		rtl_eri_clear_bits(tp, 0xd4, 0x1f80);
> +		break;
> +	case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63:
> +		r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0);
> +		break;
> ------Please consider the environment before printing this e-mail.