Re: [PATCH RFC net-next 1/6] net: phy: realtek: configure SerDes mode for rtl822x/8251b PHYs

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Feb 27, 2024 at 08:51:46AM +0100, Eric Woudstra wrote:
> From: Alexander Couzens <lynxis@fe80.eu>
> 
> The rtl822x series and rtl8251b support switching SerDes mode between
> 2500base-x and sgmii based on the negotiated copper speed.
> 
> Configure this switching mode according to SerDes modes supported by
> host.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> [ refactored, dropped HiSGMII mode and changed commit message ]
> Signed-off-by: Marek Behún <kabel@kernel.org>
> [ changed rtl822x_update_interface() to use vendor register ]
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> ---
>  drivers/net/phy/realtek.c | 96 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 1fa70427b2a2..67cffe9b7d5d 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -54,6 +54,16 @@
>  						 RTL8201F_ISR_LINK)
>  #define RTL8201F_IER				0x13
>  
> +#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
> +#define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX		2
> +
> +#define RTL822X_VND1_SERDES_CTRL3			0x7580
> +#define RTL822X_VND1_SERDES_CTRL3_MODE_MASK		GENMASK(5, 0)
> +#define RTL822X_VND1_SERDES_CTRL3_MODE_SGMII			0x02
> +#define RTL822X_VND1_SERDES_CTRL3_MODE_2500BASEX		0x16
> +
>  #define RTL8366RB_POWER_SAVE			0x15
>  #define RTL8366RB_POWER_SAVE_ON			BIT(12)
>  
> @@ -659,6 +669,60 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>  	return ret;
>  }
>  
> +static int rtl822x_config_init(struct phy_device *phydev)
> +{
> +	bool has_2500, has_sgmii;
> +	u16 mode;
> +	int ret;
> +
> +	has_2500 = test_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			    phydev->host_interfaces) ||
> +		   phydev->interface == PHY_INTERFACE_MODE_2500BASEX;
> +
> +	has_sgmii = test_bit(PHY_INTERFACE_MODE_SGMII,
> +			     phydev->host_interfaces) ||
> +		    phydev->interface == PHY_INTERFACE_MODE_SGMII;
> +
> +	if (!has_2500 && !has_sgmii)
> +		return 0;
> +
> +	/* fill in possible interfaces */
> +	__assign_bit(PHY_INTERFACE_MODE_2500BASEX, phydev->possible_interfaces,
> +		     has_2500);
> +	__assign_bit(PHY_INTERFACE_MODE_SGMII, phydev->possible_interfaces,
> +		     has_sgmii);

It would be nice to fill phydev->possible_interfaces even if
phydev->host_interfaces has not been populated. That means that the
"newer" paths in phylink can be always used during validation.

In other words, move the if() test just above this to below it.

> +
> +	/* determine SerDes option mode */
> +	if (has_2500 && !has_sgmii)
> +		mode = RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX;
> +	else
> +		mode = RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
> +	if (ret < 0)
> +		return ret;

It would be nice to know what this is doing.
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND1,
> +				     RTL822X_VND1_SERDES_OPTION,
> +				     RTL822X_VND1_SERDES_OPTION_MODE_MASK,
> +				     mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* the following 3 writes into SerDes control are needed for 2500base-x
> +	 * mode to work properly
> +	 */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6a04, 0x0503);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f10, 0xd455);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f11, 0x8020);

Also for these. "to work properly" is too vague - is it to do with the
inband signalling?

> +}
> +
>  static int rtl822x_get_features(struct phy_device *phydev)
>  {
>  	int val;
> @@ -695,6 +759,25 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
>  	return __genphy_config_aneg(phydev, ret);
>  }
>  
> +static void rtl822x_update_interface(struct phy_device *phydev)
> +{
> +	int val;
> +
> +	/* Change interface according to serdes mode */
> +	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, RTL822X_VND1_SERDES_CTRL3);
> +	if (val < 0)
> +		return;
> +
> +	switch (val & RTL822X_VND1_SERDES_CTRL3_MODE_MASK) {
> +	case RTL822X_VND1_SERDES_CTRL3_MODE_2500BASEX:
> +		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> +		break;
> +	case RTL822X_VND1_SERDES_CTRL3_MODE_SGMII:
> +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> +		break;
> +	}

Just to confirm that this doesn't change existing device behaviour?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!