diff mbox series

[net-next,24/30] net: dsa: mt7530: rename MT7530_MFC to MT753X_MFC

Message ID 20230522121532.86610-25-arinc.unal@arinc9.com (mailing list archive)
State New, archived
Headers show
Series net: dsa: mt7530: improve, trap BPDU & LLDP, and prefer CPU port | expand

Commit Message

Arınç ÜNAL May 22, 2023, 12:15 p.m. UTC 
From: Arınç ÜNAL <arinc.unal@arinc9.com>

The MT7530_MFC register is on MT7530, MT7531, and the switch on the MT7988
SoC. Some bits are for MT7530 only. Call the shared ones MT753X, the
MT7530-specific ones MT7530.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 30 +++++++++++++++------------
 drivers/net/dsa/mt7530.h | 44 ++++++++++++++++++++--------------------
 2 files changed, 39 insertions(+), 35 deletions(-)

Comments

Vladimir Oltean May 26, 2023, 3:42 p.m. UTC | #1 
On Mon, May 22, 2023 at 03:15:26PM +0300, arinc9.unal@gmail.com wrote:
>  	/* Disable flooding on all ports */
> -	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
> -		     UNU_FFP_MASK);
> +	mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
> +		     | MT753X_UNU_FFP_MASK);

The preferred coding style is not to start new lines with operators.

> +/* Register for CPU forward control */
>  #define MT7531_CFC			0x4
>  #define  MT7531_MIRROR_EN		BIT(19)
> -#define  MT7531_MIRROR_MASK		(MIRROR_MASK << 16)
> -#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & MIRROR_MASK)
> -#define  MT7531_MIRROR_PORT_SET(x)	(((x) & MIRROR_MASK) << 16)
> +#define  MT7531_MIRROR_MASK		(0x7 << 16)

minor nitpick: if you express this as GENMASK(18, 16), it will be a bit
easier to cross-check with the datasheet, since both 18 and 16 are more
representative than 0x7.

> +#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & 0x7)

also here: (((x) & GENMASK(18, 16)) >> 16)

> +#define  MT7531_MIRROR_PORT_SET(x)	(((x) & 0x7) << 16)

and here: (((x) << 16) & GENMASK(18, 16))
Russell King (Oracle) May 26, 2023, 3:50 p.m. UTC | #2 
On Fri, May 26, 2023 at 06:42:58PM +0300, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:26PM +0300, arinc9.unal@gmail.com wrote:
> >  	/* Disable flooding on all ports */
> > -	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
> > -		     UNU_FFP_MASK);
> > +	mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
> > +		     | MT753X_UNU_FFP_MASK);
> 
> The preferred coding style is not to start new lines with operators.
> 
> > +/* Register for CPU forward control */
> >  #define MT7531_CFC			0x4
> >  #define  MT7531_MIRROR_EN		BIT(19)
> > -#define  MT7531_MIRROR_MASK		(MIRROR_MASK << 16)
> > -#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & MIRROR_MASK)
> > -#define  MT7531_MIRROR_PORT_SET(x)	(((x) & MIRROR_MASK) << 16)
> > +#define  MT7531_MIRROR_MASK		(0x7 << 16)
> 
> minor nitpick: if you express this as GENMASK(18, 16), it will be a bit
> easier to cross-check with the datasheet, since both 18 and 16 are more
> representative than 0x7.
> 
> > +#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & 0x7)
> 
> also here: (((x) & GENMASK(18, 16)) >> 16)

Even better are:
#define  MT7531_MIRROR_PORT_GET(x)	FIELD_GET(MT7531_MIRROR_MASK, x)

> 
> > +#define  MT7531_MIRROR_PORT_SET(x)	(((x) & 0x7) << 16)
> 
> and here: (((x) << 16) & GENMASK(18, 16))

#define  MT7531_MIRROR_PORT_SET(x)	FIELD_PREP(MT7531_MIRROR_MASK, x)

No need to add parens around "x" in either of these uses as we're not
doing anything with x other than passing it into another macro.
Arınç ÜNAL June 4, 2023, 8:06 a.m. UTC | #3 
On 26.05.2023 18:50, Russell King (Oracle) wrote:
> On Fri, May 26, 2023 at 06:42:58PM +0300, Vladimir Oltean wrote:
>> On Mon, May 22, 2023 at 03:15:26PM +0300, arinc9.unal@gmail.com wrote:
>>>   	/* Disable flooding on all ports */
>>> -	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
>>> -		     UNU_FFP_MASK);
>>> +	mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
>>> +		     | MT753X_UNU_FFP_MASK);
>>
>> The preferred coding style is not to start new lines with operators.
>>
>>> +/* Register for CPU forward control */
>>>   #define MT7531_CFC			0x4
>>>   #define  MT7531_MIRROR_EN		BIT(19)
>>> -#define  MT7531_MIRROR_MASK		(MIRROR_MASK << 16)
>>> -#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & MIRROR_MASK)
>>> -#define  MT7531_MIRROR_PORT_SET(x)	(((x) & MIRROR_MASK) << 16)
>>> +#define  MT7531_MIRROR_MASK		(0x7 << 16)
>>
>> minor nitpick: if you express this as GENMASK(18, 16), it will be a bit
>> easier to cross-check with the datasheet, since both 18 and 16 are more
>> representative than 0x7.
>>
>>> +#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & 0x7)
>>
>> also here: (((x) & GENMASK(18, 16)) >> 16)
> 
> Even better are:
> #define  MT7531_MIRROR_PORT_GET(x)	FIELD_GET(MT7531_MIRROR_MASK, x)
> 
>>
>>> +#define  MT7531_MIRROR_PORT_SET(x)	(((x) & 0x7) << 16)
>>
>> and here: (((x) << 16) & GENMASK(18, 16))
> 
> #define  MT7531_MIRROR_PORT_SET(x)	FIELD_PREP(MT7531_MIRROR_MASK, x)
> 
> No need to add parens around "x" in either of these uses as we're not
> doing anything with x other than passing it into another macro.

Thanks. I suppose the GENMASK, FIELD_PREP, and FIELD_GET macros can be 
widely used on mt7530.h? Like GENMASK(2, 0) on MT7530_MIRROR_MASK and 
FIELD_PREP(MT7530_MIRROR_MASK, x) on MT7530_MIRROR_PORT(x)?

Arınç
Vladimir Oltean June 4, 2023, 1:17 p.m. UTC | #4 
On Sun, Jun 04, 2023 at 11:06:32AM +0300, Arınç ÜNAL wrote:
> > Even better are:
> > #define  MT7531_MIRROR_PORT_GET(x)	FIELD_GET(MT7531_MIRROR_MASK, x)
> > 
> > > > +#define  MT7531_MIRROR_PORT_SET(x)	(((x) & 0x7) << 16)
> > > 
> > > and here: (((x) << 16) & GENMASK(18, 16))
> > 
> > #define  MT7531_MIRROR_PORT_SET(x)	FIELD_PREP(MT7531_MIRROR_MASK, x)
> > 
> > No need to add parens around "x" in either of these uses as we're not
> > doing anything with x other than passing it into another macro.
> 
> Thanks. I suppose the GENMASK, FIELD_PREP, and FIELD_GET macros can be
> widely used on mt7530.h? Like GENMASK(2, 0) on MT7530_MIRROR_MASK and
> FIELD_PREP(MT7530_MIRROR_MASK, x) on MT7530_MIRROR_PORT(x)?

I suppose the answer would be "yes, they can be used", but then, I'm not
really sure what answer you're expecting.
Arınç ÜNAL June 4, 2023, 1:25 p.m. UTC | #5 
On 4.06.2023 16:17, Vladimir Oltean wrote:
> On Sun, Jun 04, 2023 at 11:06:32AM +0300, Arınç ÜNAL wrote:
>>> Even better are:
>>> #define  MT7531_MIRROR_PORT_GET(x)	FIELD_GET(MT7531_MIRROR_MASK, x)
>>>
>>>>> +#define  MT7531_MIRROR_PORT_SET(x)	(((x) & 0x7) << 16)
>>>>
>>>> and here: (((x) << 16) & GENMASK(18, 16))
>>>
>>> #define  MT7531_MIRROR_PORT_SET(x)	FIELD_PREP(MT7531_MIRROR_MASK, x)
>>>
>>> No need to add parens around "x" in either of these uses as we're not
>>> doing anything with x other than passing it into another macro.
>>
>> Thanks. I suppose the GENMASK, FIELD_PREP, and FIELD_GET macros can be
>> widely used on mt7530.h? Like GENMASK(2, 0) on MT7530_MIRROR_MASK and
>> FIELD_PREP(MT7530_MIRROR_MASK, x) on MT7530_MIRROR_PORT(x)?
> 
> I suppose the answer would be "yes, they can be used", but then, I'm not
> really sure what answer you're expecting.

I was thinking of replacing all manual definitions with these macros on 
mt7530.h. For now, I'll just make sure my current changes use them.

Arınç
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9a4d4413287a..58d8738d94d3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -955,12 +955,13 @@  mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
 		     PORT_SPEC_TAG);
 
 	/* Enable flooding on the CPU port */
-	mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
-		   UNU_FFP(BIT(port)));
+	mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
+		   MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
 
 	/* Set CPU port number */
 	if (priv->id == ID_MT7621)
-		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
+		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
+			   MT7530_CPU_PORT(port));
 
 	/* CPU port gets connected to all user ports of
 	 * the switch.
@@ -1120,16 +1121,19 @@  mt7530_port_bridge_flags(struct dsa_switch *ds, int port,
 			   flags.val & BR_LEARNING ? 0 : SA_DIS);
 
 	if (flags.mask & BR_FLOOD)
-		mt7530_rmw(priv, MT7530_MFC, UNU_FFP(BIT(port)),
-			   flags.val & BR_FLOOD ? UNU_FFP(BIT(port)) : 0);
+		mt7530_rmw(priv, MT753X_MFC, MT753X_UNU_FFP(BIT(port)),
+			   flags.val & BR_FLOOD ?
+				MT753X_UNU_FFP(BIT(port)) : 0);
 
 	if (flags.mask & BR_MCAST_FLOOD)
-		mt7530_rmw(priv, MT7530_MFC, UNM_FFP(BIT(port)),
-			   flags.val & BR_MCAST_FLOOD ? UNM_FFP(BIT(port)) : 0);
+		mt7530_rmw(priv, MT753X_MFC, MT753X_UNM_FFP(BIT(port)),
+			   flags.val & BR_MCAST_FLOOD ?
+				MT753X_UNM_FFP(BIT(port)) : 0);
 
 	if (flags.mask & BR_BCAST_FLOOD)
-		mt7530_rmw(priv, MT7530_MFC, BC_FFP(BIT(port)),
-			   flags.val & BR_BCAST_FLOOD ? BC_FFP(BIT(port)) : 0);
+		mt7530_rmw(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)),
+			   flags.val & BR_BCAST_FLOOD ?
+				MT753X_BC_FFP(BIT(port)) : 0);
 
 	return 0;
 }
@@ -1667,13 +1671,13 @@  mt7530_port_vlan_del(struct dsa_switch *ds, int port,
 static int mt753x_mirror_port_get(unsigned int id, u32 val)
 {
 	return (id == ID_MT7531) ? MT7531_MIRROR_PORT_GET(val) :
-				   MIRROR_PORT(val);
+				   MT7530_MIRROR_PORT(val);
 }
 
 static int mt753x_mirror_port_set(unsigned int id, u32 val)
 {
 	return (id == ID_MT7531) ? MT7531_MIRROR_PORT_SET(val) :
-				   MIRROR_PORT(val);
+				   MT7530_MIRROR_PORT(val);
 }
 
 static int mt753x_port_mirror_add(struct dsa_switch *ds, int port,
@@ -2327,8 +2331,8 @@  mt7531_setup_common(struct dsa_switch *ds)
 	mt7530_mib_reset(ds);
 
 	/* Disable flooding on all ports */
-	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK |
-		     UNU_FFP_MASK);
+	mt7530_clear(priv, MT753X_MFC, MT753X_BC_FFP_MASK | MT753X_UNM_FFP_MASK
+		     | MT753X_UNU_FFP_MASK);
 
 	for (i = 0; i < MT7530_NUM_PORTS; i++) {
 		/* Disable forwarding by default on all ports */
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 216081fb1c12..5ebb942b07ef 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -32,35 +32,35 @@  enum mt753x_id {
 #define SYSC_REG_RSTCTRL		0x34
 #define  RESET_MCM			BIT(2)
 
-/* Registers to mac forward control for unknown frames */
-#define MT7530_MFC			0x10
-#define  BC_FFP(x)			(((x) & 0xff) << 24)
-#define  BC_FFP_MASK			BC_FFP(~0)
-#define  UNM_FFP(x)			(((x) & 0xff) << 16)
-#define  UNM_FFP_MASK			UNM_FFP(~0)
-#define  UNU_FFP(x)			(((x) & 0xff) << 8)
-#define  UNU_FFP_MASK			UNU_FFP(~0)
-#define  CPU_EN				BIT(7)
-#define  CPU_PORT(x)			((x) << 4)
-#define  CPU_MASK			(0xf << 4)
-#define  MIRROR_EN			BIT(3)
-#define  MIRROR_PORT(x)			((x) & 0x7)
-#define  MIRROR_MASK			0x7
-
-/* Registers for CPU forward control */
+/* Register for MAC forward control */
+#define MT753X_MFC			0x10
+#define  MT753X_BC_FFP(x)		(((x) & 0xff) << 24)
+#define  MT753X_BC_FFP_MASK		MT753X_BC_FFP(~0)
+#define  MT753X_UNM_FFP(x)		(((x) & 0xff) << 16)
+#define  MT753X_UNM_FFP_MASK		MT753X_UNM_FFP(~0)
+#define  MT753X_UNU_FFP(x)		(((x) & 0xff) << 8)
+#define  MT753X_UNU_FFP_MASK		MT753X_UNU_FFP(~0)
+#define  MT7530_CPU_EN			BIT(7)
+#define  MT7530_CPU_PORT(x)		((x) << 4)
+#define  MT7530_CPU_MASK		(0xf << 4)
+#define  MT7530_MIRROR_EN		BIT(3)
+#define  MT7530_MIRROR_PORT(x)		((x) & 0x7)
+#define  MT7530_MIRROR_MASK		0x7
+
+/* Register for CPU forward control */
 #define MT7531_CFC			0x4
 #define  MT7531_MIRROR_EN		BIT(19)
-#define  MT7531_MIRROR_MASK		(MIRROR_MASK << 16)
-#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & MIRROR_MASK)
-#define  MT7531_MIRROR_PORT_SET(x)	(((x) & MIRROR_MASK) << 16)
+#define  MT7531_MIRROR_MASK		(0x7 << 16)
+#define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & 0x7)
+#define  MT7531_MIRROR_PORT_SET(x)	(((x) & 0x7) << 16)
 #define  MT7531_CPU_PMAP_MASK		GENMASK(7, 0)
 
 #define MT753X_MIRROR_REG(id)		((((id) == ID_MT7531) || ((id) == ID_MT7988)) ?	\
-					 MT7531_CFC : MT7530_MFC)
+					 MT7531_CFC : MT753X_MFC)
 #define MT753X_MIRROR_EN(id)		((((id) == ID_MT7531) || ((id) == ID_MT7988)) ?	\
-					 MT7531_MIRROR_EN : MIRROR_EN)
+					 MT7531_MIRROR_EN : MT7530_MIRROR_EN)
 #define MT753X_MIRROR_MASK(id)		((((id) == ID_MT7531) || ((id) == ID_MT7988)) ?	\
-					 MT7531_MIRROR_MASK : MIRROR_MASK)
+					 MT7531_MIRROR_MASK : MT7530_MIRROR_MASK)
 
 /* Registers for BPDU and PAE frame control*/
 #define MT753X_BPC			0x24