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.
>
>> 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,
>
>[ ... ]
>
>> 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
>
>[ ... ]
>
>> @@ -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.
>
>> 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,
>>       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@xxxxxxx/
>
>> +     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)  {
>
>[ ... ]