Re: [PATCH net-next v1 1/2] net: phy: realtek: add support for RTL8261

[Nicolai Buchwitz <nb@tipi-net.de>]

Hi Javen

On 28.5.2026 09:52, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> This patch adds support for Realtek phy chip RTL8261. Its PHY id is
> 0x001cc898 and 0x001cc899.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
> ---
>  drivers/net/phy/realtek/realtek_main.c | 315 +++++++++++++++++++++++++
>  1 file changed, 315 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c 
> b/drivers/net/phy/realtek/realtek_main.c
> index 27268811f564..fe743fd0421b 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -22,6 +22,9 @@
>  #include "../phylib.h"
>  #include "realtek.h"
> 
> +#define RTL_8261C_CG			0x001cc898
> +#define RTL_8261CE_CG			0x001cc899

Imho these should be added below RTL_8261C and not at the top.

> +
>  #define RTL8201F_IER_PAGE			0x07
>  #define RTL8201F_IER				0x13
>  #define RTL8201F_IER_LINK			BIT(13)
> @@ -141,6 +144,10 @@
>  #define RTL8211F_PHYSICAL_ADDR_WORD1		17
>  #define RTL8211F_PHYSICAL_ADDR_WORD2		18
> 
> +#define RTL8261X_EXT_ADDR_REG			0xa436
> +#define RTL8261X_EXT_DATA_REG			0xa438
> +#define RTL_8261X_SUB_PHY_ID_ADDR		0x801d
> +
>  #define RTL822X_VND1_SERDES_OPTION			0x697a
>  #define RTL822X_VND1_SERDES_OPTION_MODE_MASK		GENMASK(5, 0)
>  #define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII		0
> @@ -252,6 +259,57 @@
>  #define RTL_8251B				0x001cc862
>  #define RTL_8261C				0x001cc890
> 
> +#define RTL8261C_CE_MODEL		0x00
> +#define RTL8261D_MODEL			0x81
> +#define RTL8261X_PHYSR_REG		0xa434
> +#define RTL8261X_GBCR_REG		0xa412
> +#define RTL8261X_IMR			0xa4d2
> +#define RTL8261X_ISR			0xa4d4
> +#define RTL8261X_INT_AUTONEG_ERROR	BIT(0)
> +#define RTL8261X_INT_PAGE_RECV		BIT(2)
> +#define RTL8261X_INT_AUTONEG_DONE	BIT(3)
> +#define RTL8261X_INT_LINK_CHG		BIT(4)
> +#define RTL8261X_INT_PHY_REG_ACCESS	BIT(5)
> +#define RTL8261X_INT_PME		BIT(7)
> +#define RTL8261X_INT_ALDPS_CHG		BIT(9)
> +#define RTL8261X_INT_JABBER		BIT(10)
> +#define RTL8261X_PHYSR_LINK		BIT(2)
> +#define RTL8261X_PHYSR_DUPLEX		BIT(3)
> +#define RTL8261X_PHYSR_SPEED_L		GENMASK(5, 4)
> +#define RTL8261X_PHYSR_SPEED_H		GENMASK(10, 9)
> +
> +/* Concatenated 4-bit speed code values (SPD_H << 2 | SPD_L) */
> +#define RTL8261X_SPEED_CODE_500M	0x3	/* H=0, L=3 */
> +#define RTL8261X_SPEED_CODE_1000M	0x7	/* H=1, L=3 */
> +#define RTL8261X_SPEED_CODE_2500M	0x8	/* H=2, L=0 */
> +#define RTL8261X_SPEED_CODE_5000M	0x9	/* H=2, L=1 */
> +#define RTL8261X_SPEED_500		500

500 MBit/s isnt a standard rate which also isnt represented in ethtool.
Is this just for documentation or intended to be used? If yes, how?

> [...]

> +
> +static int rtl8261x_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_c45_pma_read_abilities(phydev);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Supplement Multi-Gig speeds that may not be automatically detected
> +	 * RTL8261X supports 2.5G/5G in addition to standard 10G
> +	 */
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			 phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> +			 phydev->supported);

Feature bits for 2.5 and 5 GBit/s are set unconditionally - even for the 
generic variant.
Do all variants have these capabilities? If not better add check for at 
least the generic one.

> +
> +	return 0;
> +}
> +
> +static int rtl8261x_config_master_slave(struct phy_device *phydev)
> +{
> +	u16 val;
> +	/*
> +	 * Configure bits 15:14 of MMD 7.0x0020
> +	 *
> +	 * Bit 15 (Enable) | Bit 14 (Value) | Mode
> +	 * ----------------|----------------|-------------
> +	 *       0         |       0        | Auto (disabled)
> +	 *       1         |       0        | Force Slave
> +	 *       1         |       1        | Force Master
> +	 */
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		val = RTL8261X_MS_MASTER;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		val = RTL8261X_MS_SLAVE;
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +	default:
> +		val = RTL8261X_MS_AUTO;
> +		break;
> +	}
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_AN, RTL8261X_MULTIG_CTRL,
> +			      RTL8261X_MASTER_SLAVE_MASK, val);
> +}

RTL8261X_MULTIG_CTRL is a IEEE-defined register, so it can use 
MDIO_AN_10GBT_CTRL instead.
Also the define for RTL8261X_MULTIG_CTRL can be dropped.

While at it I would suggest to add MDIO_AN_10GBT_CTRL_MS_ENABLE (bit 15) 
and
MDIO_AN_10GBT_CTRL_MS_VALUE (bit 14) to include/uapi/linux/mdio.h and 
also make use of it.
RTL8261X_MS_AUTO would become

   val = MDIO_AN_10GBT_CTRL_MS_ENABLE | MDIO_AN_10GBT_CTRL_MS_VALUE;

> [...]

> +
> +static int rtl8261x_read_status(struct phy_device *phydev)
> +{
> +	int ret, val, speed_code;
> +
> +	ret = genphy_c45_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_VEND2, RTL8261X_PHYSR_REG);
> +	if (val < 0)
> +		return val;
> +
> +	phydev->link = !!(val & RTL8261X_PHYSR_LINK);
> +	if (!phydev->link) {
> +		phydev->speed  = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		return 0;
> +	}

Why does it need to overreide the values from genphy_c45_read_status() ?

> +
> +	phydev->duplex = (val & RTL8261X_PHYSR_DUPLEX) ? DUPLEX_FULL : 
> DUPLEX_HALF;
> +	speed_code = (FIELD_GET(RTL8261X_PHYSR_SPEED_H, val) << 2) |
> +		      FIELD_GET(RTL8261X_PHYSR_SPEED_L, val);
> +	switch (speed_code) {
> +	case RTL8261X_SPEED_CODE_500M:
> +		phydev->speed = RTL8261X_SPEED_500;

See question above (defines).

> +		break;
> +	case RTL8261X_SPEED_CODE_1000M:
> +		phydev->speed = SPEED_1000;
> +		break;
> +	case RTL8261X_SPEED_CODE_2500M:
> +		phydev->speed = SPEED_2500;
> +		break;
> +	case RTL8261X_SPEED_CODE_5000M:
> +		phydev->speed = SPEED_5000;
> +		break;
> +	default:
> +		phydev_warn(phydev, "unknown speed code 0x%x (PHYSR=0x%04x)\n", 
> speed_code, val);
> +		phydev->speed = SPEED_UNKNOWN;
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtl8261x_config_aneg(struct phy_device *phydev)
> +{
> +	u16 adv_1g = 0;
> +	int ret;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return genphy_c45_pma_setup_forced(phydev);
> +
> +	ret = rtl8261x_config_master_slave(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = genphy_c45_config_aneg(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +			      phydev->advertising))
> +		adv_1g = BIT(9);
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND2,
> +				     RTL8261X_GBCR_REG,
> +				     BIT(9), adv_1g);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret > 0)
> +		return genphy_c45_restart_aneg(phydev);

This does not take rtl8261x_config_master_slave() into account. So maybe 
change rtl8261x_config_master_slave

    return phy_modify_mmd_changed ...

and keep the result in a bool and also trigger auto nego restart if 
needed.

> +
> +	return 0;
> +}



> +
>  static int rtl821x_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
> @@ -3001,6 +3292,30 @@ static struct phy_driver realtek_drvs[] = {
>  		.resume		= genphy_resume,
>  		.read_mmd	= genphy_read_mmd_unsupported,
>  		.write_mmd	= genphy_write_mmd_unsupported,
> +	}, {
> +		PHY_ID_MATCH_EXACT(RTL_8261C_CG),
> +		.name             = "Realtek RTL8261C_RTL8261D 10Gbps PHY",
> +		.probe            = rtl8261x_probe,

RTL8261C/RTL8261D? Or just the c variant as the phy id suggests?

> +		.get_features     = rtl8261x_get_features,
> +		.config_aneg      = rtl8261x_config_aneg,
> +		.read_status      = rtl8261x_read_status,
> +		.config_intr      = rtl8261x_config_intr,
> +		.handle_interrupt = rtl8261x_handle_interrupt,
> +		.soft_reset       = rtl8261x_soft_reset,
> +		.suspend          = genphy_c45_pma_suspend,
> +		.resume           = genphy_c45_pma_resume,
> +	}, {
> +		PHY_ID_MATCH_EXACT(RTL_8261CE_CG),
> +		.name             = "Realtek RTL8261CE 10Gbps PHY",
> +		.probe            = rtl8261x_probe,
> +		.get_features     = rtl8261x_get_features,
> +		.config_aneg      = rtl8261x_config_aneg,
> +		.read_status      = rtl8261x_read_status,
> +		.config_intr      = rtl8261x_config_intr,
> +		.handle_interrupt = rtl8261x_handle_interrupt,
> +		.soft_reset       = rtl8261x_soft_reset,
> +		.suspend          = genphy_c45_pma_suspend,
> +		.resume           = genphy_c45_pma_resume,
>  	},
>  };

Please also make sure for both patches that checkpatch has no further 
complaints. Thanks.

Nicolai