diff mbox series

[net-next,2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

Message ID 20220209211312.7242-3-luizluca@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: realtek: add rtl8_4t tag | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
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 success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: linux@armlinux.org.uk
netdev/build_clang success Errors and warnings before: 0 this patch: 0
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 success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Luiz Angelo Daros de Luca Feb. 9, 2022, 9:13 p.m. UTC 
The tailing tag is also supported by this family. The default is still
rtl8_4 but now the switch supports changing the tag to rtl8_4t.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 63 +++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Vladimir Oltean Feb. 9, 2022, 9:57 p.m. UTC | #1 
On Wed, Feb 09, 2022 at 06:13:12PM -0300, Luiz Angelo Daros de Luca wrote:
> The tailing tag is also supported by this family. The default is still
> rtl8_4 but now the switch supports changing the tag to rtl8_4t.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 63 +++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index e0c7f64bc56f..02d1521887ae 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -853,9 +853,70 @@ static enum dsa_tag_protocol
>  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>  			   enum dsa_tag_protocol mp)
>  {
> +	struct realtek_priv *priv = ds->priv;
> +	u32 val;
> +
> +	/* No way to return error */
> +	regmap_read(priv->map, RTL8365MB_CPU_CTRL_REG, &val);
> +
> +	switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, val)) {
> +	case RTL8365MB_CPU_FORMAT_4BYTES:
> +		/* Similar to DSA_TAG_PROTO_RTL4_A but with 1-byte version
> +		 * To CPU: [0x88 0x99 0x04 portnum]. Not supported yet.
> +		 */
> +		break;
> +	case RTL8365MB_CPU_FORMAT_8BYTES:
> +		switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, val)) {
> +		case RTL8365MB_CPU_POS_BEFORE_CRC:
> +			return DSA_TAG_PROTO_RTL8_4T;
> +		case RTL8365MB_CPU_POS_AFTER_SA:
> +		default:
> +			return DSA_TAG_PROTO_RTL8_4;
> +		}
> +		break;
> +	}
> +
>  	return DSA_TAG_PROTO_RTL8_4;

Not great. dsa_get_tag_protocol gets called from dsa_port_parse_cpu,
which is earlier than rtl8365mb_cpu_config, so you may temporarily
report a tagging protocol which you don't expect (depending on what is
in hardware at the time). Can you not keep the current tagging protocol
in a variable? You could then avoid the need to return an error on
regmap_read too.

>  }
>  
> +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
> +					 enum dsa_tag_protocol proto)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	int tag_position;
> +	int tag_format;
> +	int ret;
> +
> +	switch (proto) {
> +	case DSA_TAG_PROTO_RTL8_4:
> +		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
> +		tag_position = RTL8365MB_CPU_POS_AFTER_SA;
> +		break;
> +	case DSA_TAG_PROTO_RTL8_4T:
> +		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
> +		tag_position = RTL8365MB_CPU_POS_BEFORE_CRC;
> +		break;
> +	/* The switch also supports a 4-byte format, similar to rtl4a but with
> +	 * the same 0x04 8-bit version and probably 8-bit port source/dest.
> +	 * There is no public doc about it. Not supported yet.
> +	 */
> +	default:
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> +				 RTL8365MB_CPU_CTRL_TAG_POSITION_MASK |
> +				 RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
> +				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK,
> +					    tag_position) |
> +				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
> +					    tag_format));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  				      phy_interface_t interface)
>  {
> @@ -2079,6 +2140,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  
>  static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  	.get_tag_protocol = rtl8365mb_get_tag_protocol,
> +	.change_tag_protocol = rtl8365mb_change_tag_protocol,
>  	.setup = rtl8365mb_setup,
>  	.teardown = rtl8365mb_teardown,
>  	.phylink_get_caps = rtl8365mb_phylink_get_caps,
> @@ -2097,6 +2159,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  
>  static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
>  	.get_tag_protocol = rtl8365mb_get_tag_protocol,
> +	.change_tag_protocol = rtl8365mb_change_tag_protocol,
>  	.setup = rtl8365mb_setup,
>  	.teardown = rtl8365mb_teardown,
>  	.phylink_get_caps = rtl8365mb_phylink_get_caps,
> -- 
> 2.35.1
>
Luiz Angelo Daros de Luca Feb. 18, 2022, 5:38 a.m. UTC | #2 
> >  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
> >                          enum dsa_tag_protocol mp)
> >  {
> > +     struct realtek_priv *priv = ds->priv;
> > +     u32 val;
> > +
> > +     /* No way to return error */
> > +     regmap_read(priv->map, RTL8365MB_CPU_CTRL_REG, &val);
> > +
> > +     switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, val)) {
> > +     case RTL8365MB_CPU_FORMAT_4BYTES:
> > +             /* Similar to DSA_TAG_PROTO_RTL4_A but with 1-byte version
> > +              * To CPU: [0x88 0x99 0x04 portnum]. Not supported yet.
> > +              */
> > +             break;
> > +     case RTL8365MB_CPU_FORMAT_8BYTES:
> > +             switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, val)) {
> > +             case RTL8365MB_CPU_POS_BEFORE_CRC:
> > +                     return DSA_TAG_PROTO_RTL8_4T;
> > +             case RTL8365MB_CPU_POS_AFTER_SA:
> > +             default:
> > +                     return DSA_TAG_PROTO_RTL8_4;
> > +             }
> > +             break;
> > +     }
> > +
> >       return DSA_TAG_PROTO_RTL8_4;
>
> Not great. dsa_get_tag_protocol gets called from dsa_port_parse_cpu,
> which is earlier than rtl8365mb_cpu_config, so you may temporarily
> report a tagging protocol which you don't expect (depending on what is
> in hardware at the time). Can you not keep the current tagging protocol
> in a variable? You could then avoid the need to return an error on
> regmap_read too.

Thanks Vladimir.

Sure. I added the variable to the chip_data struct. With that, I can
also remove the tag-related code and variables from
rtl8365mb_cpu_config() and rtl8365mb_cpu. Now I simply call the same
rtl8365mb_change_tag_protocol() during setup. I believe it is better
to have a single place that touches that config.

Regards,

Luiz
diff mbox series

Patch

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index e0c7f64bc56f..02d1521887ae 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -853,9 +853,70 @@  static enum dsa_tag_protocol
 rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
 			   enum dsa_tag_protocol mp)
 {
+	struct realtek_priv *priv = ds->priv;
+	u32 val;
+
+	/* No way to return error */
+	regmap_read(priv->map, RTL8365MB_CPU_CTRL_REG, &val);
+
+	switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, val)) {
+	case RTL8365MB_CPU_FORMAT_4BYTES:
+		/* Similar to DSA_TAG_PROTO_RTL4_A but with 1-byte version
+		 * To CPU: [0x88 0x99 0x04 portnum]. Not supported yet.
+		 */
+		break;
+	case RTL8365MB_CPU_FORMAT_8BYTES:
+		switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, val)) {
+		case RTL8365MB_CPU_POS_BEFORE_CRC:
+			return DSA_TAG_PROTO_RTL8_4T;
+		case RTL8365MB_CPU_POS_AFTER_SA:
+		default:
+			return DSA_TAG_PROTO_RTL8_4;
+		}
+		break;
+	}
+
 	return DSA_TAG_PROTO_RTL8_4;
 }
 
+static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
+					 enum dsa_tag_protocol proto)
+{
+	struct realtek_priv *priv = ds->priv;
+	int tag_position;
+	int tag_format;
+	int ret;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_RTL8_4:
+		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
+		tag_position = RTL8365MB_CPU_POS_AFTER_SA;
+		break;
+	case DSA_TAG_PROTO_RTL8_4T:
+		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
+		tag_position = RTL8365MB_CPU_POS_BEFORE_CRC;
+		break;
+	/* The switch also supports a 4-byte format, similar to rtl4a but with
+	 * the same 0x04 8-bit version and probably 8-bit port source/dest.
+	 * There is no public doc about it. Not supported yet.
+	 */
+	default:
+		return -EPROTONOSUPPORT;
+	}
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
+				 RTL8365MB_CPU_CTRL_TAG_POSITION_MASK |
+				 RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
+				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK,
+					    tag_position) |
+				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
+					    tag_format));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 				      phy_interface_t interface)
 {
@@ -2079,6 +2140,7 @@  static int rtl8365mb_detect(struct realtek_priv *priv)
 
 static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
 	.teardown = rtl8365mb_teardown,
 	.phylink_get_caps = rtl8365mb_phylink_get_caps,
@@ -2097,6 +2159,7 @@  static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 
 static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
 	.teardown = rtl8365mb_teardown,
 	.phylink_get_caps = rtl8365mb_phylink_get_caps,