diff mbox series

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

Message ID 20220508224848.2384723-3-hauke@hauke-m.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 1
netdev/cc_maintainers warning 2 maintainers not CCed: edumazet@google.com pabeni@redhat.com
netdev/build_clang fail Errors and warnings before: 0 this patch: 1
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 1
netdev/checkpatch warning WARNING: line length of 90 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns
netdev/kdoc fail Errors and warnings before: 0 this patch: 1
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Hauke Mehrtens May 8, 2022, 10:48 p.m. UTC 
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(-)

Comments

Luiz Angelo Daros de Luca May 9, 2022, 8:03 a.m. UTC | #1 
> Read the option register in addition to the other registers to identify
> the chip. The SGMII initialization is different for the different chip
> options.

Is it possible to have two different chip models that share the same
chip ip and version but differ only in the option?
If that is true and the driver still wishes to print the correct model
name, we should keep track of each option value for each supported
chip, just like we already do with id/version.
If not, I don't believe we need to print that out during probe because
it would be just a derived property from chip id/model.

Regards,

Luiz
Alvin Šipraga May 10, 2022, 4:32 p.m. UTC | #2 
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
>
diff mbox series

Patch

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
 #define RTL8365MB_MAGIC_REG		0x13C2
 #define   RTL8365MB_MAGIC_VALUE		0x0249
 
@@ -579,6 +582,7 @@  struct rtl8365mb {
 	int irq;
 	u32 chip_id;
 	u32 chip_ver;
+	u32 chip_option;
 	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)
 {
 	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);
+	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;
 	}