RE: [net-next,v5] r8169: add support for RTL8125cp

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.



This is an AI-generated review of your patch. The human sending this email
has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
r8169: add support for RTL8125cp

This patch adds support for chip RTL8125cp with XID 0x708, applying different
firmware (rtl8125cp-1.fw) and a dedicated PHY configuration function.

quoted

diff --git a/drivers/net/ethernet/realtek/r8169.h

b/drivers/net/ethernet/realtek/r8169.h
index aed4cf852091..0b9c1d4eb48b 100644

--- a/drivers/net/ethernet/realtek/r8169.h
+++ b/drivers/net/ethernet/realtek/r8169.h

@@ -68,6 +68,7 @@ enum mac_version {
      RTL_GIGA_MAC_VER_61,
      RTL_GIGA_MAC_VER_63,
      RTL_GIGA_MAC_VER_64,
+     RTL_GIGA_MAC_VER_65,
      RTL_GIGA_MAC_VER_66,
      RTL_GIGA_MAC_VER_70,
      RTL_GIGA_MAC_VER_80,

[ ... ]

quoted

diff --git a/drivers/net/ethernet/realtek/r8169_main.c

b/drivers/net/ethernet/realtek/r8169_main.c
index 2f7d9809c373..d040f2074db6 100644

--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

quoted

@@ -4021,6 +4026,7 @@ static void rtl_hw_config(struct rtl8169_private *tp)
              [RTL_GIGA_MAC_VER_61] = rtl_hw_start_8125a_2,
              [RTL_GIGA_MAC_VER_63] = rtl_hw_start_8125b,
              [RTL_GIGA_MAC_VER_64] = rtl_hw_start_8125d,
+             [RTL_GIGA_MAC_VER_65] = rtl_hw_start_8125d,
              [RTL_GIGA_MAC_VER_66] = rtl_hw_start_8125d,
              [RTL_GIGA_MAC_VER_70] = rtl_hw_start_8126a,
              [RTL_GIGA_MAC_VER_80] = rtl_hw_start_8127a,

RTL_GIGA_MAC_VER_65 (RTL8125CP) maps to rtl_hw_start_8125d, which is
also used by VER_64 and VER_66. However, these versions need special
handling in
rtl_hw_start_8125():

static void rtl_hw_start_8125(struct rtl8169_private *tp) {
       ...
       switch (tp->mac_version) {
       case RTL_GIGA_MAC_VER_61:
       case RTL_GIGA_MAC_VER_64:
       case RTL_GIGA_MAC_VER_66:
       case RTL_GIGA_MAC_VER_80:
               for (i = 0xa00; i < 0xb00; i += 4)
                       RTL_W32(tp, i, 0);
               if (tp->mac_version == RTL_GIGA_MAC_VER_80)
                       RTL_W16(tp, INT_CFG1_8125, 0x0000);
               break;
       default:
               break;
       }
       ...
}

Should RTL_GIGA_MAC_VER_65 be added to this case group? Both VER_64
and VER_66, which share the same rtl_hw_start_8125d function as VER_65,
are explicitly listed here to disable interrupt coalescing (clearing registers
0xa00-0xb00). Without this, the RTL8125CP may operate with stale interrupt
coalescing settings, potentially causing incorrect interrupt delivery.

quoted

diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c

b/drivers/net/ethernet/realtek/r8169_phy_config.c
index 032d9d2cfa2a..f78c3dd36483 100644

--- a/drivers/net/ethernet/realtek/r8169_phy_config.c
+++ b/drivers/net/ethernet/realtek/r8169_phy_config.c

@@ -1102,6 +1102,27 @@ static void rtl8125d_hw_phy_config(struct

rtl8169_private *tp,

quoted

      rtl8125_config_eee_phy(phydev);
 }

+static void rtl8125cp_hw_phy_config(struct rtl8169_private *tp,
+                                 struct phy_device *phydev) {
+     r8169_apply_firmware(tp);
+     rtl8168g_enable_gphy_10m(phydev);
+
+     phy_modify_paged(phydev, 0xad0, 0x17, 0x007f, 0x000b);
+     phy_modify_paged(phydev, 0xad7, 0x14, 0x0000, BIT(4));
+     rtl8125_phy_param(phydev, 0x807f, 0xff00, 0x5300);
+     r8168g_phy_param(phydev, 0x81b8, 0xffff, 0x00b4);
+     r8168g_phy_param(phydev, 0x81ba, 0xffff, 0x00e4);
+     r8168g_phy_param(phydev, 0x81c5, 0xffff, 0x0104);
+     r8168g_phy_param(phydev, 0x81d0, 0xffff, 0x054d);
+     phy_modify_paged(phydev, 0x0a43, 0x10, 0x0000, 0x0003);

                                                          ^^^^^^

Andrew Lunn suggested in v4 review that magic bit values should be replaced
with named constants. The value 0x0003 sets bits for 'link speed 10m PLL OFF'
(bit 0) and 'ALDPS PLL OFF' (bit 1). This would improve code readability and
maintainability. Andrew suggested defines like
RTL8125CP_LINK_SPEED_10M_PLL_OFF BIT(0), RTL8125CP_ALDPS_PLL_OFF
BIT(1), etc.

Thanks for your advice. I will consider the two suggestions and resubmit it in v6.

Reference: https://lore.kernel.org/netdev/1a36b013-ae9b-4d51-8259-
92728644ceab@lunn.ch/

quoted

+     phy_modify_paged(phydev, 0x0a44, 0x11, 0x0000, BIT(7));
+
+     rtl8125_legacy_force_mode(phydev);
+     rtl8168g_disable_aldps(phydev);
+     rtl8125_config_eee_phy(phydev);
+}
+
 static void rtl8125bp_hw_phy_config(struct rtl8169_private *tp,
                                  struct phy_device *phydev)  {

[ ... ]