Re: [PATCH 2/4] net: dsa: realtek: rtl8365mb: Get chip option

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

On Mon, May 09, 2022 at 12:48:46AM +0200, Hauke Mehrtens wrote:
> Read the option register in addition to the other registers to identify
> the chip. The SGMII initialization is different for the different chip
> options.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 43 +++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2cb722a9e096..be64cfdeccc7 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -127,6 +127,9 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  
>  #define RTL8365MB_CHIP_VER_REG		0x1301
>  
> +#define RTL8365MB_CHIP_OPTION_REG	0x13C1
> +
> +#define RTL8365MB_MAGIC_OPT_REG		0x13C0

Realtek's driver calls this register PROTECT_ID:

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files/drivers/net/phy/rtk/rtl8367c/include/rtl8367c_reg.h;h=eb4f48b83e02ce0625d70b337c445c36f071686c;hb=HEAD
18050 #define    RTL8367C_REG_PROTECT_ID    0x13c0

Consider renaming it, or at least be consistent with _OPT and _OPTION. I see
both opt and option throughout the patch.

>  #define RTL8365MB_MAGIC_REG		0x13C2
>  #define   RTL8365MB_MAGIC_VALUE		0x0249

Can you repeat this for MAGIC_OPT_REG? It's OK that it's the same.

>  
> @@ -579,6 +582,7 @@ struct rtl8365mb {
>  	int irq;
>  	u32 chip_id;
>  	u32 chip_ver;
> +	u32 chip_option;

Moreover, do you know what option is supposed to mean here? Likewise the
register map from Realtek calls this CHIP_VER_INTL (internal maybe?):

#define    RTL8367C_REG_CHIP_VER_INTL    0x13c1

>  	u32 port_mask;
>  	u32 learn_limit_max;
>  	struct rtl8365mb_cpu cpu;
> @@ -1959,7 +1963,7 @@ static void rtl8365mb_teardown(struct dsa_switch *ds)
>  	rtl8365mb_irq_teardown(priv);
>  }
>  
> -static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
> +static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver, u32 *option)

Likewise for consistency, consider the name of this function as you add a new
argument to it.

>  {
>  	int ret;
>  
> @@ -1983,6 +1987,19 @@ static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, RTL8365MB_MAGIC_VALUE);

Might deserve a comment here too, "the ver2/ver_intl/option/etc. register is
also protected with magic".

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, RTL8365MB_CHIP_OPTION_REG, option);
> +	if (ret)
> +		return ret;
> +
> +	/* Reset magic register */
> +	ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, 0);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> @@ -1991,9 +2008,10 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  	struct rtl8365mb *mb = priv->chip_data;
>  	u32 chip_id;
>  	u32 chip_ver;
> +	u32 chip_option;
>  	int ret;
>  
> -	ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver);
> +	ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver, &chip_option);
>  	if (ret) {
>  		dev_err(priv->dev, "failed to read chip id and version: %d\n",
>  			ret);
> @@ -2005,22 +2023,22 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		switch (chip_ver) {
>  		case RTL8365MB_CHIP_VER_8365MB_VC:
>  			dev_info(priv->dev,
> -				 "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> -				 chip_ver);
> +				 "found an RTL8365MB-VC switch (ver=0x%04x, opt=0x%04x)\n",
> +				 chip_ver, chip_option);
>  			break;
>  		case RTL8365MB_CHIP_VER_8367RB:
>  			dev_info(priv->dev,
> -				 "found an RTL8367RB-VB switch (ver=0x%04x)\n",
> -				 chip_ver);
> +				 "found an RTL8367RB-VB switch (ver=0x%04x, opt=0x%04x)\n",
> +				 chip_ver, chip_option);
>  			break;
>  		case RTL8365MB_CHIP_VER_8367S:
>  			dev_info(priv->dev,
> -				 "found an RTL8367S switch (ver=0x%04x)\n",
> -				 chip_ver);
> +				 "found an RTL8367S switch (ver=0x%04x, opt=0x%04x)\n",
> +				 chip_ver, chip_option);
>  			break;
>  		default:
> -			dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
> -				chip_ver);
> +			dev_err(priv->dev, "unrecognized switch version (ver=0x%04x, opt=0x%04x)",
> +				chip_ver, chip_option);
>  			return -ENODEV;
>  		}
>  
> @@ -2029,6 +2047,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		mb->priv = priv;
>  		mb->chip_id = chip_id;
>  		mb->chip_ver = chip_ver;
> +		mb->chip_option = chip_option;
>  		mb->port_mask = GENMASK(priv->num_ports - 1, 0);
>  		mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
>  		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> @@ -2043,8 +2062,8 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		break;
>  	default:
>  		dev_err(priv->dev,
> -			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x)\n",
> -			chip_id, chip_ver);
> +			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x, opt=0x%04x)\n",
> +			chip_id, chip_ver, chip_option);
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.30.2
>