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

[Paolo Abeni <pabeni@redhat.com>]

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.

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

> +	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)
>  {

[ ... ]