[PATCH net-next 0/3] r8169: enable more devices ASPM and LTR support

[PATCH net-next 0/3] r8169: enable more devices ASPM and LTR support

[ChunHao Lin]

This series of patches will enable more devices ASPM and LTR support.
It also fix a RTL8126 cannot enter L1 substate issue when ASPM is
enabled.

ChunHao Lin (3):
  r8169: enable RTL8168H/RTL8168EP/RTL8168FP ASPM support
  r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
  r8169: disable RTL8126 ZRX-DC timeout

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

-- 
2.43.0

[PATCH net-next 1/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP ASPM support

[ChunHao Lin]

This patch will enable RTL8168H/RTL8168EP/RTL8168FP ASPM support on
the platforms that have tested with ASPM enabled.

Signed-off-by: ChunHao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index fa339bd8c775..731302361989 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5394,7 +5394,7 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
 /* 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_61 &&
+	if (tp->mac_version >= RTL_GIGA_MAC_VER_46 &&
 	    r8168_mac_ocp_read(tp, 0xc0b2) & 0xf)
 		return true;
 
-- 
2.43.0

[PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[ChunHao Lin]

This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR
support on the platforms that have tested with LTR enabled.

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

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 731302361989..9953eaa01c9d 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
 	}
 }
 
+static void rtl_set_ltr_latency(struct rtl8169_private *tp)
+{
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_70:
+	case RTL_GIGA_MAC_VER_71:
+		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
+		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
+		r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
+		r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
+		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
+		r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
+		break;
+	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
+		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
+		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
+		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
+		r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
+		r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
+		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
+		break;
+	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
+		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
+		r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
+		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
+		break;
+	default:
+		break;
+	}
+}
+
+static void rtl_reset_pci_ltr(struct rtl8169_private *tp)
+{
+	struct pci_dev *pdev = tp->pci_dev;
+	u16 cap;
+
+	pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
+	if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
+		pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
+					   PCI_EXP_DEVCTL2_LTR_EN);
+		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_LTR_EN);
+	}
+}
+
+static void rtl_enable_ltr(struct rtl8169_private *tp)
+{
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
+		r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
+		r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
+		r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
+		break;
+	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
+	case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
+		r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
+		RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
+		fallthrough;
+	case RTL_GIGA_MAC_VER_51:
+		r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
+		r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
+		r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
+		break;
+	default:
+		return;
+	}
+
+	rtl_set_ltr_latency(tp);
+
+	/* chip can trigger LTR */
+	r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
+
+	/* reset LTR to notify host */
+	rtl_reset_pci_ltr(tp);
+}
+
+static void rtl_disable_ltr(struct rtl8169_private *tp)
+{
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
+		r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
+		break;
+	default:
+		break;
+	}
+}
+
 static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 {
 	u8 val8;
@@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 		    tp->mac_version == RTL_GIGA_MAC_VER_43)
 			return;
 
+		rtl_enable_ltr(tp);
+
 		rtl_mod_config5(tp, 0, ASPM_en);
 		switch (tp->mac_version) {
 		case RTL_GIGA_MAC_VER_70:
@@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private *tp)
 
 	rtl8169_cleanup(tp);
 	rtl_disable_exit_l1(tp);
+	rtl_disable_ltr(tp);
 	rtl_prepare_power_down(tp);
 
 	if (tp->dash_type != RTL_DASH_NONE)
-- 
2.43.0

[PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

[ChunHao Lin]

Disable it due to it dose not meet ZRX-DC specification. If it is enabled,
device will exit L1 substate every 100ms. Disable it for saving more power
in L1 substate.

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

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 9953eaa01c9d..7a5b99d54e12 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2851,6 +2851,21 @@ static u32 rtl_csi_read(struct rtl8169_private *tp, int addr)
 		RTL_R32(tp, CSIDR) : ~0;
 }
 
+static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp)
+{
+	struct pci_dev *pdev = tp->pci_dev;
+	u8 val;
+
+	if (pdev->cfg_size > 0x0890 &&
+	    pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL &&
+	    pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL)
+		return;
+
+	netdev_notice_once(tp->dev,
+		"No native access to PCI extended config space, falling back to CSI\n");
+	rtl_csi_write(tp, 0x0890, rtl_csi_read(tp, 0x0890) & ~BIT(0));
+}
+
 static void rtl_set_aspm_entry_latency(struct rtl8169_private *tp, u8 val)
 {
 	struct pci_dev *pdev = tp->pci_dev;
@@ -3930,6 +3945,7 @@ static void rtl_hw_start_8125d(struct rtl8169_private *tp)
 
 static void rtl_hw_start_8126a(struct rtl8169_private *tp)
 {
+	rtl_disable_zrxdc_timeout(tp);
 	rtl_set_def_aspm_entry_latency(tp);
 	rtl_hw_start_8125_common(tp);
 }
-- 
2.43.0

Re: [PATCH net-next 1/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP ASPM support

[Heiner Kallweit]

On 21.02.2025 08:18, ChunHao Lin wrote:
> This patch will enable RTL8168H/RTL8168EP/RTL8168FP ASPM support on
> the platforms that have tested with ASPM enabled.
> 
> Signed-off-by: ChunHao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Heiner Kallweit]

On 21.02.2025 08:18, ChunHao Lin wrote:
> This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR
> support on the platforms that have tested with LTR enabled.
> 

Where in the code is the check whether platform has been tested with LTR?

> Signed-off-by: ChunHao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 108 ++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 731302361989..9953eaa01c9d 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
>  	}
>  }
>  
> +static void rtl_set_ltr_latency(struct rtl8169_private *tp)
> +{
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_70:
> +	case RTL_GIGA_MAC_VER_71:
> +		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
> +		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
> +		r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
> +		r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
> +		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
> +		r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
> +		break;
> +	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
> +		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
> +		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
> +		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
> +		r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
> +		r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
> +		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> +		break;
> +	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
> +		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> +		r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> +		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void rtl_reset_pci_ltr(struct rtl8169_private *tp)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	u16 cap;
> +
> +	pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
> +	if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
> +		pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> +					   PCI_EXP_DEVCTL2_LTR_EN);
> +		pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> +					 PCI_EXP_DEVCTL2_LTR_EN);

I'd prefer that only PCI core deals with these registers (functions like
pci_configure_ltr()). Any specific reason for this reset? Is it something
which could be applicable for other devices too, so that the PCI core
should be extended?

+Bjorn and PCI list, to get an opinion from the PCI folks.

> +	}
> +}
> +
> +static void rtl_enable_ltr(struct rtl8169_private *tp)
> +{
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
> +		r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> +		r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> +		r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
> +		break;
> +	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
> +	case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
> +		r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> +		RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
> +		fallthrough;
> +	case RTL_GIGA_MAC_VER_51:
> +		r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> +		r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
> +		r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	rtl_set_ltr_latency(tp);
> +
> +	/* chip can trigger LTR */
> +	r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
> +
> +	/* reset LTR to notify host */
> +	rtl_reset_pci_ltr(tp);
> +}
> +
> +static void rtl_disable_ltr(struct rtl8169_private *tp)
> +{
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
> +		r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  {
>  	u8 val8;
> @@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  		    tp->mac_version == RTL_GIGA_MAC_VER_43)
>  			return;
>  
> +		rtl_enable_ltr(tp);
> +
>  		rtl_mod_config5(tp, 0, ASPM_en);
>  		switch (tp->mac_version) {
>  		case RTL_GIGA_MAC_VER_70:
> @@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private *tp)
>  
>  	rtl8169_cleanup(tp);
>  	rtl_disable_exit_l1(tp);
> +	rtl_disable_ltr(tp);

Any specific reason why LTR isn't configured just once, on driver load?

>  	rtl_prepare_power_down(tp);
>  
>  	if (tp->dash_type != RTL_DASH_NONE)

Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

[Heiner Kallweit]

On 21.02.2025 08:18, ChunHao Lin wrote:
> Disable it due to it dose not meet ZRX-DC specification. If it is enabled,
> device will exit L1 substate every 100ms. Disable it for saving more power
> in L1 substate.
> 
Is this compliant with the PCIe spec? Not being an expert on this topic,
but when I read e.g. the following then my understanding is that this wakeup
every 100ms is the expected behavior.

https://lore.kernel.org/all/1610033323-10560-4-git-send-email-shradha.t@samsung.com/T/


> Signed-off-by: ChunHao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 9953eaa01c9d..7a5b99d54e12 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2851,6 +2851,21 @@ static u32 rtl_csi_read(struct rtl8169_private *tp, int addr)
>  		RTL_R32(tp, CSIDR) : ~0;
>  }
>  
> +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	u8 val;
> +
> +	if (pdev->cfg_size > 0x0890 &&
> +	    pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL &&
> +	    pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL)
> +		return;
> +
> +	netdev_notice_once(tp->dev,
> +		"No native access to PCI extended config space, falling back to CSI\n");
> +	rtl_csi_write(tp, 0x0890, rtl_csi_read(tp, 0x0890) & ~BIT(0));
> +}
> +
>  static void rtl_set_aspm_entry_latency(struct rtl8169_private *tp, u8 val)
>  {
>  	struct pci_dev *pdev = tp->pci_dev;
> @@ -3930,6 +3945,7 @@ static void rtl_hw_start_8125d(struct rtl8169_private *tp)
>  
>  static void rtl_hw_start_8126a(struct rtl8169_private *tp)
>  {
> +	rtl_disable_zrxdc_timeout(tp);
>  	rtl_set_def_aspm_entry_latency(tp);
>  	rtl_hw_start_8125_common(tp);
>  }

Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

[Bjorn Helgaas]

On Fri, Feb 21, 2025 at 03:18:28PM +0800, ChunHao Lin wrote:
> Disable it due to it dose not meet ZRX-DC specification. If it is enabled,
> device will exit L1 substate every 100ms. Disable it for saving more power
> in L1 substate.

s/dose/does/

Is ZRX-DC a PCIe spec?  A Google search suggests that it might not be
completely Realtek-specific?

> +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	u8 val;
> +
> +	if (pdev->cfg_size > 0x0890 &&
> +	    pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL &&
> +	    pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL)

Is this a standard PCIe extended capability?  If so, it would be nice
to search for it with pci_find_ext_capability() and use standard
#defines.

Bjorn

Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

[Heiner Kallweit]

On 21.02.2025 21:01, Bjorn Helgaas wrote:
> On Fri, Feb 21, 2025 at 03:18:28PM +0800, ChunHao Lin wrote:
>> Disable it due to it dose not meet ZRX-DC specification. If it is enabled,
>> device will exit L1 substate every 100ms. Disable it for saving more power
>> in L1 substate.
> 
> s/dose/does/
> 
> Is ZRX-DC a PCIe spec?  A Google search suggests that it might not be
> completely Realtek-specific?
> 
ZRX-DC is the receiver DC impedance as specified in the PCIe Base Specification.
From what I've found after a quick search ASPM restrictions apply if this impedance
isn't in the range 40-60 ohm.

>> +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp)
>> +{
>> +	struct pci_dev *pdev = tp->pci_dev;
>> +	u8 val;
>> +
>> +	if (pdev->cfg_size > 0x0890 &&
>> +	    pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL &&
>> +	    pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL)
> 
> Is this a standard PCIe extended capability?  If so, it would be nice
> to search for it with pci_find_ext_capability() and use standard
> #defines.
> 
> Bjorn

Re: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

[Bjorn Helgaas]

On Fri, Feb 21, 2025 at 09:12:09PM +0100, Heiner Kallweit wrote:
> On 21.02.2025 21:01, Bjorn Helgaas wrote:
> > On Fri, Feb 21, 2025 at 03:18:28PM +0800, ChunHao Lin wrote:
> >> Disable it due to it dose not meet ZRX-DC specification. If it is enabled,
> >> device will exit L1 substate every 100ms. Disable it for saving more power
> >> in L1 substate.
> > 
> > s/dose/does/
> > 
> > Is ZRX-DC a PCIe spec?  A Google search suggests that it might not be
> > completely Realtek-specific?
> > 
> ZRX-DC is the receiver DC impedance as specified in the PCIe Base
> Specification.  From what I've found after a quick search ASPM
> restrictions apply if this impedance isn't in the range 40-60 ohm.

Ah, so it looks like PCIe r6.0, sec 4.2.7.6.1.2, is the sort of thing
this refers to:

  4.2.7.6.1.2 Rx_L0s.Idle §

    - Next state is Rx_L0s.FTS if the Receiver detects an exit from
      Electrical Idle on any Lane of the configured Link.

    - Next state is Rx_L0s.FTS after a 100 ms timeout if the current
      data rate is 8.0 GT/s or higher and the Port’s Receivers do not
      meet the ZRX-DC specification for 2.5 GT/s (see § Table 8-11).
      All Ports are permitted to implement the timeout and transition
      to Rx_L0s.FTS when the data rate is 8.0 GT/s or higher.

> >> +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp)
> >> +{
> >> +	struct pci_dev *pdev = tp->pci_dev;
> >> +	u8 val;
> >> +
> >> +	if (pdev->cfg_size > 0x0890 &&
> >> +	    pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL &&
> >> +	    pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) == PCIBIOS_SUCCESSFUL)
> > 
> > Is this a standard PCIe extended capability?  If so, it would be nice
> > to search for it with pci_find_ext_capability() and use standard
> > #defines.

I guess we could tell from "sudo lspci -vv" output whether this is a
standard capability.

Bjorn

RE: [PATCH net-next 3/3] r8169: disable RTL8126 ZRX-DC timeout

[Hau]

> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> On 21.02.2025 08:18, ChunHao Lin wrote:
> > Disable it due to it dose not meet ZRX-DC specification. If it is
> > enabled, device will exit L1 substate every 100ms. Disable it for
> > saving more power in L1 substate.
> >
> Is this compliant with the PCIe spec? Not being an expert on this topic, but
> when I read e.g. the following then my understanding is that this wakeup
> every 100ms is the expected behavior.
> 
> https://lore.kernel.org/all/1610033323-10560-4-git-send-email-
> shradha.t@samsung.com/T/
>
ZRX-DC timeout is for L1.Idle, not for L1 Substate. And according to the spec below, because it will reduce power saving, it is not encouraged to implement.
So we disable it for saving power.

Next state is Recovery after a 100 ms timeout if the current data rate is 8.0 GT/s or higher and the Port’s
Receivers do not meet the ZRX-DC specification for 2.5 GT/s). All Ports are permitted, but not encouraged, to
implement the timeout and transition to Recovery when the data rate is 8.0 GT/s or higher.

> > Signed-off-by: ChunHao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index 9953eaa01c9d..7a5b99d54e12 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -2851,6 +2851,21 @@ static u32 rtl_csi_read(struct rtl8169_private *tp,
> int addr)
> >               RTL_R32(tp, CSIDR) : ~0;  }
> >
> > +static void rtl_disable_zrxdc_timeout(struct rtl8169_private *tp) {
> > +     struct pci_dev *pdev = tp->pci_dev;
> > +     u8 val;
> > +
> > +     if (pdev->cfg_size > 0x0890 &&
> > +         pci_read_config_byte(pdev, 0x0890, &val) == PCIBIOS_SUCCESSFUL
> &&
> > +         pci_write_config_byte(pdev, 0x0890, val & ~BIT(0)) ==
> PCIBIOS_SUCCESSFUL)
> > +             return;
> > +
> > +     netdev_notice_once(tp->dev,
> > +             "No native access to PCI extended config space, falling back to
> CSI\n");
> > +     rtl_csi_write(tp, 0x0890, rtl_csi_read(tp, 0x0890) & ~BIT(0)); }
> > +
> >  static void rtl_set_aspm_entry_latency(struct rtl8169_private *tp, u8
> > val)  {
> >       struct pci_dev *pdev = tp->pci_dev; @@ -3930,6 +3945,7 @@ static
> > void rtl_hw_start_8125d(struct rtl8169_private *tp)
> >
> >  static void rtl_hw_start_8126a(struct rtl8169_private *tp)  {
> > +     rtl_disable_zrxdc_timeout(tp);
> >       rtl_set_def_aspm_entry_latency(tp);
> >       rtl_hw_start_8125_common(tp);
> >  }

RE: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Hau]

> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> On 21.02.2025 08:18, ChunHao Lin wrote:
> > This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
> > LTR support on the platforms that have tested with LTR enabled.
> >
> 
> Where in the code is the check whether platform has been tested with LTR?
> 
LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe() return false. So LTR needs rtl_aspm_is_safe()
to return true.

> > Signed-off-by: ChunHao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 108
> > ++++++++++++++++++++++
> >  1 file changed, 108 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index 731302361989..9953eaa01c9d 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
> rtl8169_private *tp)
> >       }
> >  }
> >
> > +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
> > +     switch (tp->mac_version) {
> > +     case RTL_GIGA_MAC_VER_70:
> > +     case RTL_GIGA_MAC_VER_71:
> > +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
> > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
> > +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
> > +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
> > +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
> > +             r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
> > +             break;
> > +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
> > +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
> > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
> > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
> > +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
> > +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
> > +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> > +             break;
> > +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
> > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +}
> > +
> > +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
> > +     struct pci_dev *pdev = tp->pci_dev;
> > +     u16 cap;
> > +
> > +     pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
> > +     if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
> > +             pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> > +                                        PCI_EXP_DEVCTL2_LTR_EN);
> > +             pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> > +                                      PCI_EXP_DEVCTL2_LTR_EN);
> 
> I'd prefer that only PCI core deals with these registers (functions like
> pci_configure_ltr()). Any specific reason for this reset? Is it something which
> could be applicable for other devices too, so that the PCI core should be
> extended?
> 
It is for specific platform. On that platform driver needs to do this to let LTR works.

> +Bjorn and PCI list, to get an opinion from the PCI folks.
> 
> > +     }
> > +}
> > +
> > +static void rtl_enable_ltr(struct rtl8169_private *tp) {
> > +     switch (tp->mac_version) {
> > +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
> > +             r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> > +             r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> > +             r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
> > +             break;
> > +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
> > +     case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
> > +             r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
> > +             RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
> > +             fallthrough;
> > +     case RTL_GIGA_MAC_VER_51:
> > +             r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
> > +             r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
> > +             r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
> > +             break;
> > +     default:
> > +             return;
> > +     }
> > +
> > +     rtl_set_ltr_latency(tp);
> > +
> > +     /* chip can trigger LTR */
> > +     r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
> > +
> > +     /* reset LTR to notify host */
> > +     rtl_reset_pci_ltr(tp);
> > +}
> > +
> > +static void rtl_disable_ltr(struct rtl8169_private *tp) {
> > +     switch (tp->mac_version) {
> > +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
> > +             r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +}
> > +
> >  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp,
> > bool enable)  {
> >       u8 val8;
> > @@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct
> rtl8169_private *tp, bool enable)
> >                   tp->mac_version == RTL_GIGA_MAC_VER_43)
> >                       return;
> >
> > +             rtl_enable_ltr(tp);
> > +
> >               rtl_mod_config5(tp, 0, ASPM_en);
> >               switch (tp->mac_version) {
> >               case RTL_GIGA_MAC_VER_70:
> > @@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private
> > *tp)
> >
> >       rtl8169_cleanup(tp);
> >       rtl_disable_exit_l1(tp);
> > +     rtl_disable_ltr(tp);
> 
> Any specific reason why LTR isn't configured just once, on driver load?
> 
It is for device compatibility, I will check internally to see if we can remove it.


> >       rtl_prepare_power_down(tp);
> >
> >       if (tp->dash_type != RTL_DASH_NONE)

Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Bjorn Helgaas]

On Mon, Feb 24, 2025 at 04:33:50PM +0000, Hau wrote:
> > On 21.02.2025 08:18, ChunHao Lin wrote:
> > > This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
> > > LTR support on the platforms that have tested with LTR enabled.
> > 
> > Where in the code is the check whether platform has been tested with LTR?
> > 
> LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe()
> return false. So LTR needs rtl_aspm_is_safe() to return true.
> 
> > > Signed-off-by: ChunHao Lin <hau@realtek.com>
> > > ---
> > >  drivers/net/ethernet/realtek/r8169_main.c | 108
> > > ++++++++++++++++++++++
> > >  1 file changed, 108 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > > b/drivers/net/ethernet/realtek/r8169_main.c
> > > index 731302361989..9953eaa01c9d 100644
> > > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > > @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
> > rtl8169_private *tp)
> > >       }
> > >  }
> > >
> > > +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
> > > +     switch (tp->mac_version) {
> > > +     case RTL_GIGA_MAC_VER_70:
> > > +     case RTL_GIGA_MAC_VER_71:
> > > +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
> > > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
> > > +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
> > > +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
> > > +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
> > > +             r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
> > > +             break;
> > > +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
> > > +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
> > > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
> > > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
> > > +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
> > > +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
> > > +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> > > +             break;
> > > +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
> > > +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> > > +             r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> > > +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> > > +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> > > +             break;
> > > +     default:
> > > +             break;
> > > +     }
> > > +}
> > > +
> > > +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
> > > +     struct pci_dev *pdev = tp->pci_dev;
> > > +     u16 cap;
> > > +
> > > +     pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
> > > +     if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
> > > +             pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> > > +                                        PCI_EXP_DEVCTL2_LTR_EN);
> > > +             pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> > > +                                      PCI_EXP_DEVCTL2_LTR_EN);
> > 
> > I'd prefer that only PCI core deals with these registers
> > (functions like pci_configure_ltr()). Any specific reason for this
> > reset? Is it something which could be applicable for other devices
> > too, so that the PCI core should be extended?
> > 
> It is for specific platform. On that platform driver needs to do
> this to let LTR works.

This definitely looks like code that should not be in a driver.
Drivers shouldn't need to touch ASPM or LTR configuration unless
there's a device defect to work around, and that should use a PCI core
interface.  Depending on what the defect is, we may need to add a new
interface.

This clear/set of PCI_EXP_DEVCTL2_LTR_EN when it was already set could
work around some kind of device defect, or it could be a hint that
something in the PCI core is broken.  Maybe the core is configuring
ASPM/LTR incorrectly.

Bjorn

Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Heiner Kallweit]

On 24.02.2025 20:00, Bjorn Helgaas wrote:
> On Mon, Feb 24, 2025 at 04:33:50PM +0000, Hau wrote:
>>> On 21.02.2025 08:18, ChunHao Lin wrote:
>>>> This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
>>>> LTR support on the platforms that have tested with LTR enabled.
>>>
>>> Where in the code is the check whether platform has been tested with LTR?
>>>
>> LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe()
>> return false. So LTR needs rtl_aspm_is_safe() to return true.
>>
>>>> Signed-off-by: ChunHao Lin <hau@realtek.com>
>>>> ---
>>>>  drivers/net/ethernet/realtek/r8169_main.c | 108
>>>> ++++++++++++++++++++++
>>>>  1 file changed, 108 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>>> index 731302361989..9953eaa01c9d 100644
>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>>> @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
>>> rtl8169_private *tp)
>>>>       }
>>>>  }
>>>>
>>>> +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
>>>> +     switch (tp->mac_version) {
>>>> +     case RTL_GIGA_MAC_VER_70:
>>>> +     case RTL_GIGA_MAC_VER_71:
>>>> +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
>>>> +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
>>>> +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
>>>> +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
>>>> +             r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
>>>> +             break;
>>>> +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
>>>> +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
>>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
>>>> +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
>>>> +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
>>>> +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
>>>> +             break;
>>>> +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
>>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
>>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
>>>> +             break;
>>>> +     default:
>>>> +             break;
>>>> +     }
>>>> +}
>>>> +
>>>> +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
>>>> +     struct pci_dev *pdev = tp->pci_dev;
>>>> +     u16 cap;
>>>> +
>>>> +     pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
>>>> +     if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
>>>> +             pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
>>>> +                                        PCI_EXP_DEVCTL2_LTR_EN);
>>>> +             pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
>>>> +                                      PCI_EXP_DEVCTL2_LTR_EN);
>>>
>>> I'd prefer that only PCI core deals with these registers
>>> (functions like pci_configure_ltr()). Any specific reason for this
>>> reset? Is it something which could be applicable for other devices
>>> too, so that the PCI core should be extended?
>>>
>> It is for specific platform. On that platform driver needs to do
>> this to let LTR works.
> 
I interpret this in a way that the chip triggers some internal LTR
configuration activity if it detects bit PCI_EXP_DEVCTL2_LTR_EN
changing from 0 to 1. And this needed activity isn't triggered
if PCI_EXP_DEVCTL2_LTR_EN is set already and doesn't change.
Hau, is this correct?

So the PCI_EXP_DEVCTL2_LTR_EN reset is some kind of needed quirk.
However PCI quirks are applied too early, before we even detected
the chip version in probe(). Therefore I also think a helper for
this reset in PCI core would be best.

And what hasn't been mentioned yet: We have to skip the chip-specific
LTR configuration if pci_dev->ltr_path isn't set.

> This definitely looks like code that should not be in a driver.
> Drivers shouldn't need to touch ASPM or LTR configuration unless
> there's a device defect to work around, and that should use a PCI core
> interface.  Depending on what the defect is, we may need to add a new
> interface.
> 
> This clear/set of PCI_EXP_DEVCTL2_LTR_EN when it was already set could
> work around some kind of device defect, or it could be a hint that
> something in the PCI core is broken.  Maybe the core is configuring
> ASPM/LTR incorrectly.
> 
> Bjorn

Re: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Heiner Kallweit]

On 24.02.2025 17:33, Hau wrote:
>>
>> External mail : This email originated from outside the organization. Do not
>> reply, click links, or open attachments unless you recognize the sender and
>> know the content is safe.
>>
>>
>>
>> On 21.02.2025 08:18, ChunHao Lin wrote:
>>> This patch will enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
>>> LTR support on the platforms that have tested with LTR enabled.
>>>
>>
>> Where in the code is the check whether platform has been tested with LTR?
>>
> LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe() return false. So LTR needs rtl_aspm_is_safe()
> to return true.
> 
>>> Signed-off-by: ChunHao Lin <hau@realtek.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169_main.c | 108
>>> ++++++++++++++++++++++
>>>  1 file changed, 108 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>>> b/drivers/net/ethernet/realtek/r8169_main.c
>>> index 731302361989..9953eaa01c9d 100644
>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
>> rtl8169_private *tp)
>>>       }
>>>  }
>>>
>>> +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
>>> +     switch (tp->mac_version) {
>>> +     case RTL_GIGA_MAC_VER_70:
>>> +     case RTL_GIGA_MAC_VER_71:
>>> +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
>>> +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
>>> +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
>>> +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
>>> +             r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
>>> +             break;
>>> +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
>>> +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
>>> +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
>>> +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
>>> +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
>>> +             break;
>>> +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
>>> +             break;
>>> +     default:
>>> +             break;
>>> +     }
>>> +}
>>> +
>>> +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
>>> +     struct pci_dev *pdev = tp->pci_dev;
>>> +     u16 cap;
>>> +
>>> +     pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
>>> +     if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
>>> +             pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
>>> +                                        PCI_EXP_DEVCTL2_LTR_EN);
>>> +             pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
>>> +                                      PCI_EXP_DEVCTL2_LTR_EN);
>>
>> I'd prefer that only PCI core deals with these registers (functions like
>> pci_configure_ltr()). Any specific reason for this reset? Is it something which
>> could be applicable for other devices too, so that the PCI core should be
>> extended?
>>
> It is for specific platform. On that platform driver needs to do this to let LTR works.
> 
>> +Bjorn and PCI list, to get an opinion from the PCI folks.
>>
>>> +     }
>>> +}
>>> +
>>> +static void rtl_enable_ltr(struct rtl8169_private *tp) {
>>> +     switch (tp->mac_version) {
>>> +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_71:
>>> +             r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
>>> +             r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
>>> +             r8168_mac_ocp_modify(tp, 0xe032, 0x0000, BIT(14));
>>> +             break;
>>> +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
>>> +     case RTL_GIGA_MAC_VER_52 ... RTL_GIGA_MAC_VER_53:
>>> +             r8168_mac_ocp_modify(tp, 0xe0a2, 0x0000, BIT(0));
>>> +             RTL_W8(tp, 0xb6, RTL_R8(tp, 0xb6) | BIT(0));
>>> +             fallthrough;
>>> +     case RTL_GIGA_MAC_VER_51:
>>> +             r8168_mac_ocp_modify(tp, 0xe034, 0x0000, 0xc000);
>>> +             r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
>>> +             r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
>>> +             break;
>>> +     default:
>>> +             return;
>>> +     }
>>> +
>>> +     rtl_set_ltr_latency(tp);
>>> +
>>> +     /* chip can trigger LTR */
>>> +     r8168_mac_ocp_modify(tp, 0xe032, 0x0003, BIT(0));
>>> +
>>> +     /* reset LTR to notify host */
>>> +     rtl_reset_pci_ltr(tp);
>>> +}
>>> +
>>> +static void rtl_disable_ltr(struct rtl8169_private *tp) {
>>> +     switch (tp->mac_version) {
>>> +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_71:
>>> +             r8168_mac_ocp_modify(tp, 0xe032, 0x0003, 0);
>>> +             break;
>>> +     default:
>>> +             break;
>>> +     }
>>> +}
>>> +
>>>  static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp,
>>> bool enable)  {
>>>       u8 val8;
>>> @@ -2971,6 +3076,8 @@ static void rtl_hw_aspm_clkreq_enable(struct
>> rtl8169_private *tp, bool enable)
>>>                   tp->mac_version == RTL_GIGA_MAC_VER_43)
>>>                       return;
>>>
>>> +             rtl_enable_ltr(tp);
>>> +
>>>               rtl_mod_config5(tp, 0, ASPM_en);
>>>               switch (tp->mac_version) {
>>>               case RTL_GIGA_MAC_VER_70:
>>> @@ -4821,6 +4928,7 @@ static void rtl8169_down(struct rtl8169_private
>>> *tp)
>>>
>>>       rtl8169_cleanup(tp);
>>>       rtl_disable_exit_l1(tp);
>>> +     rtl_disable_ltr(tp);
>>
>> Any specific reason why LTR isn't configured just once, on driver load?
>>
> It is for device compatibility, I will check internally to see if we can remove it.
> 
Thanks. Complementing what I wrote before:
I would understand that reconfiguring LTR may be needed after a hw reset, when chip
"forgets" settings. But is there a reason to disable the internal LTR config?
IOW: What could happen if we omit rtl_disable_ltr()?

> 
>>>       rtl_prepare_power_down(tp);
>>>
>>>       if (tp->dash_type != RTL_DASH_NONE)
> 

RE: [PATCH net-next 2/3] r8169: enable RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support

[Hau]

> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> Sent: Saturday, March 1, 2025 6:58 AM
> To: Bjorn Helgaas <helgaas@kernel.org>; Hau <hau@realtek.com>
> Cc: nic_swsd <nic_swsd@realtek.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Bjorn Helgaas <bhelgaas@google.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH net-next 2/3] r8169: enable
> RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126 LTR support
> 
> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> On 24.02.2025 20:00, Bjorn Helgaas wrote:
> > On Mon, Feb 24, 2025 at 04:33:50PM +0000, Hau wrote:
> >>> On 21.02.2025 08:18, ChunHao Lin wrote:
> >>>> This patch will enable
> RTL8168H/RTL8168EP/RTL8168FP/RTL8125/RTL8126
> >>>> LTR support on the platforms that have tested with LTR enabled.
> >>>
> >>> Where in the code is the check whether platform has been tested with
> LTR?
> >>>
> >> LTR is for L1,2. But L1 will be disabled when rtl_aspm_is_safe()
> >> return false. So LTR needs rtl_aspm_is_safe() to return true.
> >>
> >>>> Signed-off-by: ChunHao Lin <hau@realtek.com>
> >>>> ---
> >>>>  drivers/net/ethernet/realtek/r8169_main.c | 108
> >>>> ++++++++++++++++++++++
> >>>>  1 file changed, 108 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> >>>> b/drivers/net/ethernet/realtek/r8169_main.c
> >>>> index 731302361989..9953eaa01c9d 100644
> >>>> --- a/drivers/net/ethernet/realtek/r8169_main.c
> >>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> >>>> @@ -2955,6 +2955,111 @@ static void rtl_disable_exit_l1(struct
> >>> rtl8169_private *tp)
> >>>>       }
> >>>>  }
> >>>>
> >>>> +static void rtl_set_ltr_latency(struct rtl8169_private *tp) {
> >>>> +     switch (tp->mac_version) {
> >>>> +     case RTL_GIGA_MAC_VER_70:
> >>>> +     case RTL_GIGA_MAC_VER_71:
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x8c09);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcde8, 0x887a);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdec, 0x8c09);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8a62);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdf4, 0x883e);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdf6, 0x9003);
> >>>> +             break;
> >>>> +     case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_66:
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x889c);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c30);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcde8, 0x883e);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdea, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdec, 0x889c);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdee, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdf0, 0x8C09);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
> >>>> +             break;
> >>>> +     case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_53:
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> >>>> +             r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> >>>> +             break;
> >>>> +     default:
> >>>> +             break;
> >>>> +     }
> >>>> +}
> >>>> +
> >>>> +static void rtl_reset_pci_ltr(struct rtl8169_private *tp) {
> >>>> +     struct pci_dev *pdev = tp->pci_dev;
> >>>> +     u16 cap;
> >>>> +
> >>>> +     pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &cap);
> >>>> +     if (cap & PCI_EXP_DEVCTL2_LTR_EN) {
> >>>> +             pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL2,
> >>>> +                                        PCI_EXP_DEVCTL2_LTR_EN);
> >>>> +             pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> >>>> +                                      PCI_EXP_DEVCTL2_LTR_EN);
> >>>
> >>> I'd prefer that only PCI core deals with these registers (functions
> >>> like pci_configure_ltr()). Any specific reason for this reset? Is it
> >>> something which could be applicable for other devices too, so that
> >>> the PCI core should be extended?
> >>>
> >> It is for specific platform. On that platform driver needs to do this
> >> to let LTR works.
> >
> I interpret this in a way that the chip triggers some internal LTR configuration
> activity if it detects bit PCI_EXP_DEVCTL2_LTR_EN changing from 0 to 1. And
> this needed activity isn't triggered if PCI_EXP_DEVCTL2_LTR_EN is set already
> and doesn't change.
> Hau, is this correct?
> 
It may be the reason for the LTR  of these platforms to work. But I will remove this
quirk from next version  patch.

> So the PCI_EXP_DEVCTL2_LTR_EN reset is some kind of needed quirk.
> However PCI quirks are applied too early, before we even detected the chip
> version in probe(). Therefore I also think a helper for this reset in PCI core
> would be best.
> 
> And what hasn't been mentioned yet: We have to skip the chip-specific LTR
> configuration if pci_dev->ltr_path isn't set.
> 
I will add checking pci_dev->ltr_path in next version patch.

> > This definitely looks like code that should not be in a driver.
> > Drivers shouldn't need to touch ASPM or LTR configuration unless
> > there's a device defect to work around, and that should use a PCI core
> > interface.  Depending on what the defect is, we may need to add a new
> > interface.
> >
> > This clear/set of PCI_EXP_DEVCTL2_LTR_EN when it was already set could
> > work around some kind of device defect, or it could be a hint that
> > something in the PCI core is broken.  Maybe the core is configuring
> > ASPM/LTR incorrectly.
> >
> > Bjorn