[PATCH net-next v3 0/6] ar9331: mainline some parts of switch functionality

[PATCH net-next v3 0/6] ar9331: mainline some parts of switch functionality

[Oleksij Rempel]

changes v3:
- spell fixes
- remove debug print from ar9331_sw_port_fdb_add()
- fix reverse Christmas tree in ar9331_sw_port_fdb_rmw()
- rename _fdb to fdb in ar9331_sw_port_fdb_dump

Till now the ar9331 switch was supporting only port multiplexing mode.
With this patch set we should be able to bridging and VLAN

Oleksij Rempel (6):
  net: dsa: qca: ar9331: reorder MDIO write sequence
  net: dsa: qca: ar9331: make proper initial port defaults
  net: dsa: qca: ar9331: add forwarding database support
  net: dsa: qca: ar9331: add ageing time support
  net: dsa: qca: ar9331: add bridge support
  net: dsa: qca: ar9331: add vlan support

 drivers/net/dsa/qca/ar9331.c | 780 ++++++++++++++++++++++++++++++++++-
 1 file changed, 775 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH net-next v3 1/6] net: dsa: qca: ar9331: reorder MDIO write sequence

[Oleksij Rempel]

In case of this switch we work with 32bit registers on top of 16bit
bus. Some registers (for example access to forwarding database) have
trigger bit on the first 16bit half of request and the result +
configuration of request in the second half. Without this patch, we would
trigger database operation and overwrite result in one run.

To make it work properly, we should do the second part of transfer
before the first one is done.

So far, this rule seems to work for all registers on this switch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/ar9331.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index ca2ad77b71f1..6686192e1883 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -837,16 +837,24 @@ static int ar9331_mdio_write(void *ctx, u32 reg, u32 val)
 		return 0;
 	}
 
-	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
+	/* In case of this switch we work with 32bit registers on top of 16bit
+	 * bus. Some registers (for example access to forwarding database) have
+	 * trigger bit on the first 16bit half of request, the result and
+	 * configuration of request in the second half.
+	 * To make it work properly, we should do the second part of transfer
+	 * before the first one is done.
+	 */
+	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
+				  val >> 16);
 	if (ret < 0)
 		goto error;
 
-	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
-				  val >> 16);
+	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
 	if (ret < 0)
 		goto error;
 
 	return 0;
+
 error:
 	dev_err_ratelimited(&sbus->dev, "Bus error. Failed to write register.\n");
 	return ret;
-- 
2.30.2

Re: [PATCH net-next v3 1/6] net: dsa: qca: ar9331: reorder MDIO write sequence

[Vladimir Oltean]

On Mon, Aug 02, 2021 at 03:10:32PM +0200, Oleksij Rempel wrote:
> In case of this switch we work with 32bit registers on top of 16bit
> bus. Some registers (for example access to forwarding database) have
> trigger bit on the first 16bit half of request and the result +
> configuration of request in the second half. Without this patch, we would
> trigger database operation and overwrite result in one run.
> 
> To make it work properly, we should do the second part of transfer
> before the first one is done.
> 
> So far, this rule seems to work for all registers on this switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next v3 2/6] net: dsa: qca: ar9331: make proper initial port defaults

[Oleksij Rempel]

Make sure that all external port are actually isolated from each other,
so no packets are leaked.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 109 ++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 6686192e1883..2f5673ea3140 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -60,10 +60,20 @@
 
 #define AR9331_SW_REG_FLOOD_MASK		0x2c
 #define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU	BIT(26)
+#define AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP	GENMASK(20, 16)
+#define AR9331_SW_FLOOD_MASK_UNI_FLOOD_DP	GENMASK(4, 0)
 
 #define AR9331_SW_REG_GLOBAL_CTRL		0x30
 #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
 
+#define AR9331_SW_REG_ADDR_TABLE_CTRL		0x5c
+#define AR9331_SW_AT_ARP_EN			BIT(20)
+#define AR9331_SW_AT_LEARN_CHANGE_EN		BIT(18)
+#define AR9331_SW_AT_AGE_EN			BIT(17)
+#define AR9331_SW_AT_AGE_TIME			GENMASK(15, 0)
+/* AGE_TIME_COEF is not documented. This is "works for me" value */
+#define AR9331_SW_AT_AGE_TIME_COEF		6900
+
 #define AR9331_SW_REG_MDIO_CTRL			0x98
 #define AR9331_SW_MDIO_CTRL_BUSY		BIT(31)
 #define AR9331_SW_MDIO_CTRL_MASTER_EN		BIT(30)
@@ -101,6 +111,46 @@
 	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
 	 AR9331_SW_PORT_STATUS_SPEED_M)
 
+#define AR9331_SW_REG_PORT_CTRL(_port)			(0x104 + (_port) * 0x100)
+#define AR9331_SW_PORT_CTRL_ING_MIRROR_EN		BIT(17)
+#define AR9331_SW_PORT_CTRL_EG_MIRROR_EN		BIT(16)
+#define AR9331_SW_PORT_CTRL_DOUBLE_TAG_VLAN		BIT(15)
+#define AR9331_SW_PORT_CTRL_LEARN_EN			BIT(14)
+#define AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN		BIT(13)
+#define AR9331_SW_PORT_CTRL_MAC_LOOP_BACK		BIT(12)
+#define AR9331_SW_PORT_CTRL_HEAD_EN			BIT(11)
+#define AR9331_SW_PORT_CTRL_IGMP_MLD_EN			BIT(10)
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE		GENMASK(9, 8)
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_KEEP		0
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP		1
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD		2
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_DOUBLE		3
+#define AR9331_SW_PORT_CTRL_LEARN_ONE_LOCK		BIT(7)
+#define AR9331_SW_PORT_CTRL_PORT_LOCK_EN		BIT(6)
+#define AR9331_SW_PORT_CTRL_LOCK_DROP_EN		BIT(5)
+#define AR9331_SW_PORT_CTRL_PORT_STATE			GENMASK(2, 0)
+#define AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED		0
+#define AR9331_SW_PORT_CTRL_PORT_STATE_BLOCKING		1
+#define AR9331_SW_PORT_CTRL_PORT_STATE_LISTENING	2
+#define AR9331_SW_PORT_CTRL_PORT_STATE_LEARNING		3
+#define AR9331_SW_PORT_CTRL_PORT_STATE_FORWARD		4
+
+#define AR9331_SW_REG_PORT_VLAN(_port)			(0x108 + (_port) * 0x100)
+#define AR9331_SW_PORT_VLAN_8021Q_MODE			GENMASK(31, 30)
+#define AR9331_SW_8021Q_MODE_SECURE			3
+#define AR9331_SW_8021Q_MODE_CHECK			2
+#define AR9331_SW_8021Q_MODE_FALLBACK			1
+#define AR9331_SW_8021Q_MODE_NONE			0
+#define AR9331_SW_PORT_VLAN_ING_PORT_PRI		GENMASK(29, 27)
+#define AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN		BIT(26)
+#define AR9331_SW_PORT_VLAN_PORT_VID_MEMBER		GENMASK(25, 16)
+#define AR9331_SW_PORT_VLAN_ARP_LEAKY_EN		BIT(15)
+#define AR9331_SW_PORT_VLAN_UNI_LEAKY_EN		BIT(14)
+#define AR9331_SW_PORT_VLAN_MULTI_LEAKY_EN		BIT(13)
+#define AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN	BIT(12)
+#define AR9331_SW_PORT_VLAN_PORT_VID			GENMASK(11, 0)
+#define AR9331_SW_PORT_VLAN_PORT_VID_DEF		1
+
 /* MIB registers */
 #define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
 
@@ -371,12 +421,63 @@ static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
 	return 0;
 }
 
-static int ar9331_sw_setup(struct dsa_switch *ds)
+static int ar9331_sw_setup_port(struct dsa_switch *ds, int port)
 {
 	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
 	struct regmap *regmap = priv->regmap;
+	u32 port_mask, port_ctrl, val;
 	int ret;
 
+	/* Generate default port settings */
+	port_ctrl = FIELD_PREP(AR9331_SW_PORT_CTRL_PORT_STATE,
+			       AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED);
+
+	if (dsa_is_cpu_port(ds, port)) {
+		/* CPU port should be allowed to communicate with all user
+		 * ports.
+		 */
+		port_mask = dsa_user_ports(ds);
+		/* Enable Atheros header on CPU port. This will allow us
+		 * communicate with each port separately
+		 */
+		port_ctrl |= AR9331_SW_PORT_CTRL_HEAD_EN;
+	} else if (dsa_is_user_port(ds, port)) {
+		/* User ports should communicate only with the CPU port.
+		 */
+		port_mask = BIT(dsa_to_port(ds, port)->cpu_dp->index);
+		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN;
+	} else {
+		/* Other ports do not need to communicate at all */
+		port_mask = 0;
+	}
+
+	val = FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
+			 AR9331_SW_8021Q_MODE_NONE) |
+		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask) |
+		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
+			   AR9331_SW_PORT_VLAN_PORT_VID_DEF);
+
+	ret = regmap_write(regmap, AR9331_SW_REG_PORT_VLAN(port), val);
+	if (ret)
+		goto error;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_PORT_CTRL(port), port_ctrl);
+	if (ret)
+		goto error;
+
+	return 0;
+error:
+	dev_err(priv->dev, "%s: error: %i\n", __func__, ret);
+
+	return ret;
+}
+
+static int ar9331_sw_setup(struct dsa_switch *ds)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int ret, i;
+
 	ret = ar9331_sw_reset(priv);
 	if (ret)
 		return ret;
@@ -402,6 +503,12 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
 	if (ret)
 		goto error;
 
+	for (i = 0; i < ds->num_ports; i++) {
+		ret = ar9331_sw_setup_port(ds, i);
+		if (ret)
+			goto error;
+	}
+
 	ds->configure_vlan_while_not_filtering = false;
 
 	return 0;
-- 
2.30.2

Re: [PATCH net-next v3 2/6] net: dsa: qca: ar9331: make proper initial port defaults

[Vladimir Oltean]

On Mon, Aug 02, 2021 at 03:10:33PM +0200, Oleksij Rempel wrote:
> Make sure that all external port are actually isolated from each other,
> so no packets are leaked.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 109 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index 6686192e1883..2f5673ea3140 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -60,10 +60,20 @@
>  
>  #define AR9331_SW_REG_FLOOD_MASK		0x2c
>  #define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU	BIT(26)
> +#define AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP	GENMASK(20, 16)
> +#define AR9331_SW_FLOOD_MASK_UNI_FLOOD_DP	GENMASK(4, 0)
>  
>  #define AR9331_SW_REG_GLOBAL_CTRL		0x30
>  #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
>  
> +#define AR9331_SW_REG_ADDR_TABLE_CTRL		0x5c
> +#define AR9331_SW_AT_ARP_EN			BIT(20)
> +#define AR9331_SW_AT_LEARN_CHANGE_EN		BIT(18)
> +#define AR9331_SW_AT_AGE_EN			BIT(17)
> +#define AR9331_SW_AT_AGE_TIME			GENMASK(15, 0)
> +/* AGE_TIME_COEF is not documented. This is "works for me" value */
> +#define AR9331_SW_AT_AGE_TIME_COEF		6900

Not documented, not used either, it seems.
"Works for you" based on what?

> +
>  #define AR9331_SW_REG_MDIO_CTRL			0x98
>  #define AR9331_SW_MDIO_CTRL_BUSY		BIT(31)
>  #define AR9331_SW_MDIO_CTRL_MASTER_EN		BIT(30)
> @@ -101,6 +111,46 @@
>  	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
>  	 AR9331_SW_PORT_STATUS_SPEED_M)

Is this patch material for "net"? If standalone ports is all that ar9331
supports, then it would better not do packet forwarding in lack of a
bridge device.

Re: [PATCH net-next v3 2/6] net: dsa: qca: ar9331: make proper initial port defaults

[Andrew Lunn]

> > +/* AGE_TIME_COEF is not documented. This is "works for me" value */
> > +#define AR9331_SW_AT_AGE_TIME_COEF		6900
> 
> Not documented, not used either, it seems.
> "Works for you" based on what?

It is used in a later patch. Ideally it would of been introduced in
that patch to make this more obvious.

> >  #define AR9331_SW_REG_MDIO_CTRL			0x98
> >  #define AR9331_SW_MDIO_CTRL_BUSY		BIT(31)
> >  #define AR9331_SW_MDIO_CTRL_MASTER_EN		BIT(30)
> > @@ -101,6 +111,46 @@
> >  	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
> >  	 AR9331_SW_PORT_STATUS_SPEED_M)
> 
> Is this patch material for "net"? If standalone ports is all that ar9331
> supports, then it would better not do packet forwarding in lack of a
> bridge device.

It does seem like this patch should be considered for stable, if by
default all ports can talk with all ports when not part of a bridge.

	Andrew

Re: [PATCH net-next v3 2/6] net: dsa: qca: ar9331: make proper initial port defaults

[Oleksij Rempel]

On Mon, Aug 02, 2021 at 09:45:14PM +0200, Andrew Lunn wrote:
> > > +/* AGE_TIME_COEF is not documented. This is "works for me" value */
> > > +#define AR9331_SW_AT_AGE_TIME_COEF		6900
> > 
> > Not documented, not used either, it seems.
> > "Works for you" based on what?
> 
> It is used in a later patch. Ideally it would of been introduced in
> that patch to make this more obvious.

ack, i'll move it from this patch

> > >  #define AR9331_SW_REG_MDIO_CTRL			0x98
> > >  #define AR9331_SW_MDIO_CTRL_BUSY		BIT(31)
> > >  #define AR9331_SW_MDIO_CTRL_MASTER_EN		BIT(30)
> > > @@ -101,6 +111,46 @@
> > >  	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
> > >  	 AR9331_SW_PORT_STATUS_SPEED_M)
> > 
> > Is this patch material for "net"? If standalone ports is all that ar9331
> > supports, then it would better not do packet forwarding in lack of a
> > bridge device.
> 
> It does seem like this patch should be considered for stable, if by
> default all ports can talk with all ports when not part of a bridge.

ack, i'll split this patch set

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v3 2/6] net: dsa: qca: ar9331: make proper initial port defaults

[Andrew Lunn]

On Mon, Aug 02, 2021 at 03:10:33PM +0200, Oleksij Rempel wrote:
> Make sure that all external port are actually isolated from each other,
> so no packets are leaked.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v3 3/6] net: dsa: qca: ar9331: add forwarding database support

[Oleksij Rempel]

This switch provides simple address resolution table, without VLAN or
multicast specific information.
With this patch we are able now to read, modify unicast and multicast
addresses.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 349 +++++++++++++++++++++++++++++++++++
 1 file changed, 349 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 2f5673ea3140..d94c7ea163c4 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -66,6 +66,47 @@
 #define AR9331_SW_REG_GLOBAL_CTRL		0x30
 #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
 
+/* Size of the address resolution table (ARL) */
+#define AR9331_SW_NUM_ARL_RECORDS		1024
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION0	0x50
+#define AR9331_SW_AT_ADDR_BYTES4		GENMASK(31, 24)
+#define AR9331_SW_AT_ADDR_BYTES5		GENMASK(23, 16)
+#define AR9331_SW_AT_FULL_VIO			BIT(12)
+#define AR9331_SW_AT_PORT_NUM			GENMASK(11, 8)
+#define AR9331_SW_AT_FLUSH_STATIC_EN		BIT(4)
+#define AR9331_SW_AT_BUSY			BIT(3)
+#define AR9331_SW_AT_FUNC			GENMASK(2, 0)
+#define AR9331_SW_AT_FUNC_NOP			0
+#define AR9331_SW_AT_FUNC_FLUSH_ALL		1
+#define AR9331_SW_AT_FUNC_LOAD_ENTRY		2
+#define AR9331_SW_AT_FUNC_PURGE_ENTRY		3
+#define AR9331_SW_AT_FUNC_FLUSH_ALL_UNLOCKED	4
+#define AR9331_SW_AT_FUNC_FLUSH_PORT		5
+#define AR9331_SW_AT_FUNC_GET_NEXT		6
+#define AR9331_SW_AT_FUNC_FIND_MAC		7
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION1	0x54
+#define AR9331_SW_AT_ADDR_BYTES0		GENMASK(31, 24)
+#define AR9331_SW_AT_ADDR_BYTES1		GENMASK(23, 16)
+#define AR9331_SW_AT_ADDR_BYTES2		GENMASK(15, 8)
+#define AR9331_SW_AT_ADDR_BYTES3		GENMASK(7, 0)
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION2	0x58
+#define AR9331_SW_AT_COPY_TO_CPU		BIT(26)
+#define AR9331_SW_AT_REDIRECT_TOCPU		BIT(25)
+#define AR9331_SW_AT_LEAKY_EN			BIT(24)
+#define AR9331_SW_AT_STATUS			GENMASK(19, 16)
+#define AR9331_SW_AT_STATUS_EMPTY		0
+/* STATUS values from 7 to 1 are different aging levels */
+#define AR9331_SW_AT_STATUS_STATIC		0xf
+
+#define AR9331_SW_AT_SA_DROP_EN			BIT(14)
+#define AR9331_SW_AT_MIRROR_EN			BIT(13)
+#define AR9331_SW_AT_PRIORITY_EN		BIT(12)
+#define AR9331_SW_AT_PRIORITY			GENMASK(11, 10)
+#define AR9331_SW_AT_DES_PORT			GENMASK(5, 0)
+
 #define AR9331_SW_REG_ADDR_TABLE_CTRL		0x5c
 #define AR9331_SW_AT_ARP_EN			BIT(20)
 #define AR9331_SW_AT_LEARN_CHANGE_EN		BIT(18)
@@ -267,6 +308,12 @@ struct ar9331_sw_port {
 	struct spinlock stats_lock;
 };
 
+struct ar9331_sw_fdb {
+	u8 port_mask;
+	u8 aging;
+	u8 mac[ETH_ALEN];
+};
+
 struct ar9331_sw_priv {
 	struct device *dev;
 	struct dsa_switch ds;
@@ -731,6 +778,302 @@ static void ar9331_get_stats64(struct dsa_switch *ds, int port,
 	spin_unlock(&p->stats_lock);
 }
 
+static int ar9331_sw_fdb_wait(struct ar9331_sw_priv *priv, u32 *f0)
+{
+	struct regmap *regmap = priv->regmap;
+
+	return regmap_read_poll_timeout(regmap,
+					AR9331_SW_REG_ADDR_TABLE_FUNCTION0,
+					*f0, !(*f0 & AR9331_SW_AT_BUSY),
+					10, 2000);
+}
+
+static int ar9331_sw_port_fdb_write(struct ar9331_sw_priv *priv,
+				    u32 f0, u32 f1, u32 f2)
+{
+	struct regmap *regmap = priv->regmap;
+	int ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, f2);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION1, f1);
+	if (ret)
+		return ret;
+
+	return regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, f0);
+}
+
+static int ar9331_sw_fdb_next(struct ar9331_sw_priv *priv,
+			      struct ar9331_sw_fdb *fdb, int port)
+{
+	struct regmap *regmap = priv->regmap;
+	unsigned int status, ports;
+	u32 f0, f1, f2;
+	int ret;
+
+	/* Keep AT_ADDR_BYTES4/5 to search next entry after current */
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0,
+				 AR9331_SW_AT_FUNC | AR9331_SW_AT_BUSY,
+				 AR9331_SW_AT_BUSY |
+				 FIELD_PREP(AR9331_SW_AT_FUNC,
+					    AR9331_SW_AT_FUNC_GET_NEXT));
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, &f2);
+	if (ret)
+		return ret;
+
+	/* If the hardware returns an MAC != 0 and the AT_STATUS is zero, there
+	 * is no next valid entry in the address table.
+	 */
+	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
+	fdb->aging = status;
+	if (!status)
+		return 0;
+
+	ret = regmap_read(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION1, &f1);
+	if (ret)
+		return ret;
+
+	fdb->mac[0] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES0, f1);
+	fdb->mac[1] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES1, f1);
+	fdb->mac[2] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES2, f1);
+	fdb->mac[3] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES3, f1);
+	fdb->mac[4] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES4, f0);
+	fdb->mac[5] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES5, f0);
+
+	ports = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
+	if (!(ports & BIT(port)))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static void ar9331_sw_port_fdb_prepare(const unsigned char *mac, u32 *f0,
+				       u32 *f1, unsigned int func)
+{
+	*f1 = FIELD_PREP(AR9331_SW_AT_ADDR_BYTES0, mac[0]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES1, mac[1]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES2, mac[2]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES3, mac[3]);
+	*f0 = FIELD_PREP(AR9331_SW_AT_ADDR_BYTES4, mac[4]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES5, mac[5]) |
+	      FIELD_PREP(AR9331_SW_AT_FUNC, func) | AR9331_SW_AT_BUSY;
+}
+
+static int ar9331_sw_port_fdb_dump(struct dsa_switch *ds, int port,
+				   dsa_fdb_dump_cb_t *cb, void *data)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	int cnt = AR9331_SW_NUM_ARL_RECORDS;
+	struct ar9331_sw_fdb fdb = { 0 };
+	bool is_static;
+	int ret;
+	u32 f0;
+
+	/* Make sure no pending operation is in progress. Since no timeout and
+	 * interval values are documented, we use here "seems to be sane, works
+	 * for me" values.
+	 */
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	/* If the address and the AT_STATUS are both zero, the hardware will
+	 * search the first valid entry from entry0.
+	 * If the address is set to zero and the AT_STATUS is not zero, the
+	 * hardware will discover the next valid entry which has an address
+	 * of 0x0.
+	 */
+	ret = ar9331_sw_port_fdb_write(priv, 0, 0, 0);
+	if (ret)
+		return ret;
+
+	while (cnt--) {
+		ret = ar9331_sw_fdb_next(priv, &fdb, port);
+		if (ret == -EAGAIN)
+			continue;
+		else if (ret)
+			return ret;
+
+		if (!fdb.aging)
+			break;
+
+		is_static = (fdb.aging == AR9331_SW_AT_STATUS_STATIC);
+		ret = cb(fdb.mac, 0, is_static, data);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int ar9331_sw_port_fdb_rmw(struct ar9331_sw_priv *priv,
+				  const unsigned char *mac,
+				  u8 port_mask_set,
+				  u8 port_mask_clr)
+{
+	u8 port_mask, port_mask_new, status, func;
+	struct regmap *regmap = priv->regmap;
+	u32 f0, f1, f2 = 0;
+	int ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ar9331_sw_port_fdb_prepare(mac, &f0, &f1, AR9331_SW_AT_FUNC_FIND_MAC);
+
+	ret = ar9331_sw_port_fdb_write(priv, f0, f1, f2);
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, &f2);
+	if (ret)
+		return ret;
+
+	port_mask = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
+	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
+	if (status > 0 && status < AR9331_SW_AT_STATUS_STATIC) {
+		dev_dbg(priv->dev, "%s: found existing dynamic entry on %x\n",
+			__func__, port_mask);
+
+		if (port_mask_set && port_mask_set != port_mask)
+			dev_dbg(priv->dev, "%s: found existing dynamic entry on %x, replacing it with static on %x\n",
+				__func__, port_mask, port_mask_set);
+		port_mask = 0;
+	} else if (!status && !port_mask_set) {
+		return 0;
+	}
+
+	port_mask_new = port_mask & ~port_mask_clr;
+	port_mask_new |= port_mask_set;
+
+	if (port_mask_new == port_mask &&
+	    status == AR9331_SW_AT_STATUS_STATIC) {
+		dev_dbg(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
+			__func__, port_mask_new);
+		return 0;
+	}
+
+	if (port_mask_new) {
+		func = AR9331_SW_AT_FUNC_LOAD_ENTRY;
+	} else {
+		func = AR9331_SW_AT_FUNC_PURGE_ENTRY;
+		port_mask_new = port_mask;
+	}
+
+	f2 = FIELD_PREP(AR9331_SW_AT_DES_PORT, port_mask_new) |
+		FIELD_PREP(AR9331_SW_AT_STATUS, AR9331_SW_AT_STATUS_STATIC);
+
+	ar9331_sw_port_fdb_prepare(mac, &f0, &f1, func);
+
+	ret = ar9331_sw_port_fdb_write(priv, f0, f1, f2);
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	if (f0 & AR9331_SW_AT_FULL_VIO) {
+		/* cleanup error status */
+		regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, 0);
+		dev_err(priv->dev, "%s: can't add new entry, ATU is full\n", __func__);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int ar9331_sw_port_fdb_add(struct dsa_switch *ds, int port,
+				  const unsigned char *mac, u16 vid)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (vid)
+		return -EINVAL;
+
+	return ar9331_sw_port_fdb_rmw(priv, mac, port_mask, 0);
+}
+
+static int ar9331_sw_port_fdb_del(struct dsa_switch *ds, int port,
+				  const unsigned char *mac, u16 vid)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (vid)
+		return -EINVAL;
+
+	return ar9331_sw_port_fdb_rmw(priv, mac, 0, port_mask);
+}
+
+static int ar9331_sw_port_mdb_add(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+
+	return ar9331_sw_port_fdb_rmw(priv, mdb->addr, port_mask, 0);
+}
+
+static int ar9331_sw_port_mdb_del(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+
+	return ar9331_sw_port_fdb_rmw(priv, mdb->addr, 0, port_mask);
+}
+
+static void ar9331_sw_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int ret;
+	u32 f0;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		goto error;
+
+	/* Flush all non static unicast address on a given port */
+	f0 = FIELD_PREP(AR9331_SW_AT_PORT_NUM, port) |
+		FIELD_PREP(AR9331_SW_AT_FUNC, AR9331_SW_AT_FUNC_FLUSH_PORT) |
+		AR9331_SW_AT_BUSY;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, f0);
+	if (ret)
+		goto error;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		goto error;
+
+	return;
+error:
+	dev_err(priv->dev, "%s: error: %i\n", __func__, ret);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -740,6 +1083,12 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.phylink_mac_link_down	= ar9331_sw_phylink_mac_link_down,
 	.phylink_mac_link_up	= ar9331_sw_phylink_mac_link_up,
 	.get_stats64		= ar9331_get_stats64,
+	.port_fast_age          = ar9331_sw_port_fast_age,
+	.port_fdb_del		= ar9331_sw_port_fdb_del,
+	.port_fdb_add		= ar9331_sw_port_fdb_add,
+	.port_fdb_dump		= ar9331_sw_port_fdb_dump,
+	.port_mdb_add           = ar9331_sw_port_mdb_add,
+	.port_mdb_del           = ar9331_sw_port_mdb_del,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.30.2

Re: [PATCH net-next v3 3/6] net: dsa: qca: ar9331: add forwarding database support

[Vladimir Oltean]

On Mon, Aug 02, 2021 at 03:10:34PM +0200, Oleksij Rempel wrote:
> +static int ar9331_sw_port_fdb_add(struct dsa_switch *ds, int port,
> +				  const unsigned char *mac, u16 vid)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	u16 port_mask = BIT(port);
> +
> +	if (vid)
> +		return -EINVAL;

When did you test this patch last time on net-next?
Asking because when a port joins a VLAN-aware bridge, it will replay the
FDB entries towards the bridge, which include a VLAN-unaware FDB entry
with VID 0, and a VLAN-aware one with the bridge's default_pvid.
If you return -EINVAL, that br_fdb_replay call will fail and you will
fail to join the bridge.

> +
> +	return ar9331_sw_port_fdb_rmw(priv, mac, port_mask, 0);
> +}
> +
> +static int ar9331_sw_port_fdb_del(struct dsa_switch *ds, int port,
> +				  const unsigned char *mac, u16 vid)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	u16 port_mask = BIT(port);
> +
> +	if (vid)
> +		return -EINVAL;
> +
> +	return ar9331_sw_port_fdb_rmw(priv, mac, 0, port_mask);
> +}

[PATCH net-next v3 4/6] net: dsa: qca: ar9331: add ageing time support

[Oleksij Rempel]

This switch provides global ageing time configuration, so let DSA use
it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/qca/ar9331.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index d94c7ea163c4..d726d2f223ea 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -1074,6 +1074,25 @@ static void ar9331_sw_port_fast_age(struct dsa_switch *ds, int port)
 	dev_err(priv->dev, "%s: error: %i\n", __func__, ret);
 }
 
+static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
+				     unsigned int ageing_time)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 time, val;
+
+	time = DIV_ROUND_UP(ageing_time, AR9331_SW_AT_AGE_TIME_COEF);
+	if (!time)
+		time = 1;
+	else if (time > U16_MAX)
+		time = U16_MAX;
+
+	val = FIELD_PREP(AR9331_SW_AT_AGE_TIME, time) | AR9331_SW_AT_AGE_EN;
+	return regmap_update_bits(regmap, AR9331_SW_REG_ADDR_TABLE_CTRL,
+				  AR9331_SW_AT_AGE_EN | AR9331_SW_AT_AGE_TIME,
+				  val);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1089,6 +1108,7 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.port_fdb_dump		= ar9331_sw_port_fdb_dump,
 	.port_mdb_add           = ar9331_sw_port_mdb_add,
 	.port_mdb_del           = ar9331_sw_port_mdb_del,
+	.set_ageing_time	= ar9331_sw_set_ageing_time,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
@@ -1442,6 +1462,8 @@ static int ar9331_sw_probe(struct mdio_device *mdiodev)
 	priv->ops = ar9331_sw_ops;
 	ds->ops = &priv->ops;
 	dev_set_drvdata(&mdiodev->dev, priv);
+	ds->ageing_time_min = AR9331_SW_AT_AGE_TIME_COEF;
+	ds->ageing_time_max = AR9331_SW_AT_AGE_TIME_COEF * U16_MAX;
 
 	for (i = 0; i < ARRAY_SIZE(priv->port); i++) {
 		struct ar9331_sw_port *port = &priv->port[i];
-- 
2.30.2

[PATCH net-next v3 5/6] net: dsa: qca: ar9331: add bridge support

[Oleksij Rempel]

This switch is providing forwarding matrix, with it we can configure
individual bridges. Potentially we can configure more than one not VLAN
based bridge on this HW.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/qca/ar9331.c | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index d726d2f223ea..a0324fed2136 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -40,6 +40,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/if_bridge.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/of_mdio.h>
@@ -1093,6 +1094,56 @@ static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
 				  val);
 }
 
+static int ar9331_sw_port_bridge_mod(struct dsa_switch *ds, int port,
+				     struct net_device *br, bool join)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int port_mask = BIT(dsa_to_port(ds, port)->cpu_dp->index);
+	int i, ret;
+	u32 val;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev != br)
+			continue;
+
+		if (!dsa_is_user_port(ds, port))
+			continue;
+
+		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
+		ret = regmap_set_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
+		if (ret)
+			goto error;
+
+		if (join && i != port)
+			port_mask |= BIT(i);
+	}
+
+	val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask);
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, val);
+	if (ret)
+		goto error;
+
+	return 0;
+error:
+	dev_err(priv->dev, "%s: error: %i\n", __func__, ret);
+
+	return ret;
+}
+
+static int ar9331_sw_port_bridge_join(struct dsa_switch *ds, int port,
+				      struct net_device *br)
+{
+	return ar9331_sw_port_bridge_mod(ds, port, br, true);
+}
+
+static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
+					struct net_device *br)
+{
+	ar9331_sw_port_bridge_mod(ds, port, br, false);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1109,6 +1160,8 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.port_mdb_add           = ar9331_sw_port_mdb_add,
 	.port_mdb_del           = ar9331_sw_port_mdb_del,
 	.set_ageing_time	= ar9331_sw_set_ageing_time,
+	.port_bridge_join	= ar9331_sw_port_bridge_join,
+	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.30.2

Re: [PATCH net-next v3 5/6] net: dsa: qca: ar9331: add bridge support

[Vladimir Oltean]

On Mon, Aug 02, 2021 at 03:10:36PM +0200, Oleksij Rempel wrote:
> This switch is providing forwarding matrix, with it we can configure
> individual bridges. Potentially we can configure more than one not VLAN
> based bridge on this HW.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/dsa/qca/ar9331.c | 53 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index d726d2f223ea..a0324fed2136 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -40,6 +40,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/if_bridge.h>
>  #include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_mdio.h>
> @@ -1093,6 +1094,56 @@ static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
>  				  val);
>  }
>  
> +static int ar9331_sw_port_bridge_mod(struct dsa_switch *ds, int port,
> +				     struct net_device *br, bool join)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;

Reverse Christmas tree notation..

> +	int port_mask = BIT(dsa_to_port(ds, port)->cpu_dp->index);

Could you use dsa_upstream_port(ds, port) instead of raw access to cpu_dp?
Or alternatively, you can add another condition in your "for" loop, for
dsa_is_cpu_port(ds, port) => port_mask |= BIT(i);

> +	int i, ret;
> +	u32 val;
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		if (dsa_to_port(ds, i)->bridge_dev != br)
> +			continue;
> +
> +		if (!dsa_is_user_port(ds, port))
> +			continue;

Only user ports can have a ->bridge_dev pointer populated in the first place.
Also, did you mean dsa_is_user_port(ds, i)? The "port" variable is an
invariant as far as this loop is concerned.

> +
> +		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
> +		ret = regmap_set_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
> +		if (ret)
> +			goto error;
> +
> +		if (join && i != port)
> +			port_mask |= BIT(i);
> +	}
> +
> +	val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask);
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				 AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, val);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err(priv->dev, "%s: error: %i\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int ar9331_sw_port_bridge_join(struct dsa_switch *ds, int port,
> +				      struct net_device *br)
> +{
> +	return ar9331_sw_port_bridge_mod(ds, port, br, true);
> +}
> +
> +static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
> +					struct net_device *br)
> +{
> +	ar9331_sw_port_bridge_mod(ds, port, br, false);
> +}
> +
>  static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
>  	.setup			= ar9331_sw_setup,
> @@ -1109,6 +1160,8 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.port_mdb_add           = ar9331_sw_port_mdb_add,
>  	.port_mdb_del           = ar9331_sw_port_mdb_del,
>  	.set_ageing_time	= ar9331_sw_set_ageing_time,
> +	.port_bridge_join	= ar9331_sw_port_bridge_join,
> +	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
>  };
>  
>  static irqreturn_t ar9331_sw_irq(int irq, void *data)
> -- 
> 2.30.2
> 

Re: [PATCH net-next v3 5/6] net: dsa: qca: ar9331: add bridge support

[Vladimir Oltean]

Hi Oleksij,

On Mon, Aug 02, 2021 at 03:10:36PM +0200, Oleksij Rempel wrote:
> This switch is providing forwarding matrix, with it we can configure
> individual bridges. Potentially we can configure more than one not VLAN
> based bridge on this HW.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

I don't see anywhere in this patch or in this series that the
tag_ar9331.c file is being patched to set skb->offload_fwd_mark to true
for packets sent (flooded) to the CPU that have already been forwarded
by the hardware switch. If the software bridge sees a broadcast packet
coming from your driver and it has offload_fwd_mark = false, it will
forward it a second time and the other nodes in your network will see
duplicates.

Re: [PATCH net-next v3 5/6] net: dsa: qca: ar9331: add bridge support

[Oleksij Rempel]

On Sun, Aug 08, 2021 at 02:08:29AM +0300, Vladimir Oltean wrote:
> Hi Oleksij,
> 
> On Mon, Aug 02, 2021 at 03:10:36PM +0200, Oleksij Rempel wrote:
> > This switch is providing forwarding matrix, with it we can configure
> > individual bridges. Potentially we can configure more than one not VLAN
> > based bridge on this HW.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> 
> I don't see anywhere in this patch or in this series that the
> tag_ar9331.c file is being patched to set skb->offload_fwd_mark to true
> for packets sent (flooded) to the CPU that have already been forwarded
> by the hardware switch. If the software bridge sees a broadcast packet
> coming from your driver and it has offload_fwd_mark = false, it will
> forward it a second time and the other nodes in your network will see
> duplicates.

Ok, thank you, I'll take a look on it.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next v3 6/6] net: dsa: qca: ar9331: add vlan support

[Oleksij Rempel]

This switch provides simple VLAN resolution database for 16 entries.
With this database we can cover typical functionalities as port based
VLANs, untagged and tagged egress. Port based ingress filtering.

The VLAN database is working on top of forwarding database. So,
potentially, we can have multiple VLANs on top of multiple bridges.
Hawing one VLAN on top of multiple bridges will fail on different
levels, most probably DSA framework should warn if some one won't to make
something likes this.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 235 ++++++++++++++++++++++++++++++++++-
 1 file changed, 233 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index a0324fed2136..0865ffbc2c74 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -67,6 +67,27 @@
 #define AR9331_SW_REG_GLOBAL_CTRL		0x30
 #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
 
+#define AR9331_SW_NUM_VLAN_RECORDS		16
+
+#define AR9331_SW_REG_VLAN_TABLE_FUNCTION0	0x40
+#define AR9331_SW_VT0_PRI_EN			BIT(31)
+#define AR9331_SW_VT0_PRI			GENMASK(30, 28)
+#define AR9331_SW_VT0_VID			GENMASK(27, 16)
+#define AR9331_SW_VT0_PORT_NUM			GENMASK(11, 8)
+#define AR9331_SW_VT0_FULL_VIO			BIT(4)
+#define AR9331_SW_VT0_BUSY			BIT(3)
+#define AR9331_SW_VT0_FUNC			GENMASK(2, 0)
+#define AR9331_SW_VT0_FUNC_NOP			0
+#define AR9331_SW_VT0_FUNC_FLUSH_ALL		1
+#define AR9331_SW_VT0_FUNC_LOAD_ENTRY		2
+#define AR9331_SW_VT0_FUNC_PURGE_ENTRY		3
+#define AR9331_SW_VT0_FUNC_DEL_PORT		4
+#define AR9331_SW_VT0_FUNC_GET_NEXT		5
+
+#define AR9331_SW_REG_VLAN_TABLE_FUNCTION1	0x44
+#define AR9331_SW_VT1_VALID			BIT(11)
+#define AR9331_SW_VT1_VID_MEM			GENMASK(9, 0)
+
 /* Size of the address resolution table (ARL) */
 #define AR9331_SW_NUM_ARL_RECORDS		1024
 
@@ -309,6 +330,11 @@ struct ar9331_sw_port {
 	struct spinlock stats_lock;
 };
 
+struct ar9331_sw_vlan_db {
+	u16 vid;
+	u8 port_mask;
+};
+
 struct ar9331_sw_fdb {
 	u8 port_mask;
 	u8 aging;
@@ -327,6 +353,7 @@ struct ar9331_sw_priv {
 	struct regmap *regmap;
 	struct reset_control *sw_reset;
 	struct ar9331_sw_port port[AR9331_SW_PORTS];
+	struct ar9331_sw_vlan_db vdb[AR9331_SW_NUM_VLAN_RECORDS];
 };
 
 static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
@@ -557,8 +584,6 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
 			goto error;
 	}
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	return 0;
 error:
 	dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
@@ -1144,6 +1169,209 @@ static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
 	ar9331_sw_port_bridge_mod(ds, port, br, false);
 }
 
+static int ar9331_port_vlan_filtering(struct dsa_switch *ds, int port,
+				      bool vlan_filtering,
+				      struct netlink_ext_ack *extack)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 mode;
+	int ret;
+
+	if (vlan_filtering)
+		mode = AR9331_SW_8021Q_MODE_SECURE;
+	else
+		mode = AR9331_SW_8021Q_MODE_NONE;
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 AR9331_SW_PORT_VLAN_8021Q_MODE,
+				 FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
+					    mode));
+	if (ret)
+		dev_err(priv->dev, "%s: error: %pe\n", __func__, ERR_PTR(ret));
+
+	return ret;
+}
+
+static int ar9331_sw_vt_wait(struct ar9331_sw_priv *priv, u32 *f0)
+{
+	struct regmap *regmap = priv->regmap;
+
+	return regmap_read_poll_timeout(regmap,
+					AR9331_SW_REG_VLAN_TABLE_FUNCTION0,
+					*f0, !(*f0 & AR9331_SW_VT0_BUSY),
+					100, 2000);
+}
+
+static int ar9331_sw_port_vt_rmw(struct ar9331_sw_priv *priv, u16 vid,
+				 u8 port_mask_set, u8 port_mask_clr)
+{
+	struct regmap *regmap = priv->regmap;
+	u32 f0, f1, port_mask = 0, port_mask_new, func;
+	struct ar9331_sw_vlan_db *vdb = NULL;
+	int ret, i;
+
+	if (!vid)
+		return 0;
+
+	ret = ar9331_sw_vt_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
+	if (ret)
+		goto error;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, 0);
+	if (ret)
+		goto error;
+
+	for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
+		if (priv->vdb[i].vid == vid) {
+			vdb = &priv->vdb[i];
+			break;
+		}
+	}
+
+	ret = regmap_read(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, &f1);
+	if (ret)
+		return ret;
+
+	if (vdb)
+		port_mask = vdb->port_mask;
+
+	port_mask_new = port_mask & ~port_mask_clr;
+	port_mask_new |= port_mask_set;
+
+	if (port_mask_new && port_mask_new == port_mask)
+		return 0;
+
+	if (port_mask_new) {
+		func = AR9331_SW_VT0_FUNC_LOAD_ENTRY;
+	} else {
+		func = AR9331_SW_VT0_FUNC_PURGE_ENTRY;
+		port_mask_new = port_mask;
+	}
+
+	if (vdb) {
+		vdb->port_mask = port_mask_new;
+
+		if (!port_mask_new)
+			vdb->vid = 0;
+	} else {
+		for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
+			if (!priv->vdb[i].vid) {
+				vdb = &priv->vdb[i];
+				break;
+			}
+		}
+
+		if (!vdb)
+			return -ENOMEM;
+
+		vdb->vid = vid;
+		vdb->port_mask = port_mask_new;
+	}
+
+	f0 = FIELD_PREP(AR9331_SW_VT0_VID, vid) |
+	     FIELD_PREP(AR9331_SW_VT0_FUNC, func) |
+	     AR9331_SW_VT0_BUSY;
+	f1 = FIELD_PREP(AR9331_SW_VT1_VID_MEM, port_mask_new) |
+		AR9331_SW_VT1_VALID;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, f1);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, f0);
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_vt_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	if (f0 & AR9331_SW_VT0_FULL_VIO) {
+		/* cleanup error status */
+		regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
+		return -ENOMEM;
+	}
+
+	return 0;
+
+error:
+	dev_err(priv->dev, "%s: error: %pe\n", __func__, ERR_PTR(ret));
+
+	return ret;
+}
+
+static int ar9331_port_vlan_set_pvid(struct ar9331_sw_priv *priv, int port,
+				     u16 pvid)
+{
+	struct regmap *regmap = priv->regmap;
+	int ret;
+	u32 mask, val;
+
+	mask = AR9331_SW_PORT_VLAN_8021Q_MODE |
+		AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
+		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN;
+	val = AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
+		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN |
+		FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
+			   AR9331_SW_8021Q_MODE_FALLBACK);
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 mask, val);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				  AR9331_SW_PORT_VLAN_PORT_VID,
+				  FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
+					     pvid));
+}
+
+static int ar9331_port_vlan_add(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_vlan *vlan,
+				struct netlink_ext_ack *extack)
+{
+	struct ar9331_sw_priv *priv = ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int ret, mode;
+
+	ret = ar9331_sw_port_vt_rmw(priv, vlan->vid, BIT(port), 0);
+	if (ret)
+		goto error;
+
+	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
+		ret = ar9331_port_vlan_set_pvid(priv, port, vlan->vid);
+
+	if (ret)
+		goto error;
+
+	if (vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP;
+	else
+		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD;
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_CTRL(port),
+				 AR9331_SW_PORT_CTRL_EG_VLAN_MODE, mode);
+	if (ret)
+		goto error;
+
+	return 0;
+error:
+	dev_err(priv->dev, "%s: error: %pe\n", __func__, ERR_PTR(ret));
+
+	return ret;
+}
+
+static int ar9331_port_vlan_del(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_vlan *vlan)
+{
+	return ar9331_sw_port_vt_rmw(ds->priv, vlan->vid, 0, BIT(port));
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1162,6 +1390,9 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.set_ageing_time	= ar9331_sw_set_ageing_time,
 	.port_bridge_join	= ar9331_sw_port_bridge_join,
 	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
+	.port_vlan_filtering	= ar9331_port_vlan_filtering,
+	.port_vlan_add		= ar9331_port_vlan_add,
+	.port_vlan_del		= ar9331_port_vlan_del,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.30.2

Re: [PATCH net-next v3 6/6] net: dsa: qca: ar9331: add vlan support

[Vladimir Oltean]

On Mon, Aug 02, 2021 at 03:10:37PM +0200, Oleksij Rempel wrote:
> This switch provides simple VLAN resolution database for 16 entries.
> With this database we can cover typical functionalities as port based
> VLANs, untagged and tagged egress. Port based ingress filtering.
> 
> The VLAN database is working on top of forwarding database. So,
> potentially, we can have multiple VLANs on top of multiple bridges.
> Hawing one VLAN on top of multiple bridges will fail on different
> levels, most probably DSA framework should warn if some one won't to make
> something likes this.

Please define the levels in which it will fail for ar9331.

I opened the AR9331 manual and I see that the only forwarding isolation
mechanism is the PORT_VID_MEMBER, the "port base VLAN", which defines
which other ports can each port forward to. Otherwise, it seems that
when two ports are in 802.1Q mode (VLAN-aware) and in the same VLAN,
they can forward packets in that VLAN regardless of PORT_VID_MEMBER.
If correct, it means you only support a single VLAN-aware bridge, I
recommend you copy what sja1105_prechangeupper does to block a second
one.

The manual also says "The AR9331 only supports shared VLAN learning (SVL)."
So there isn't in fact all that much that the DSA core can save the day.
Even with a single bridge, the standalone ports will still attempt to
look up the FDB, and even though packets from standalone ports should
always reach the CPU port, that might not happen if that MAC DA is in
the bridge FDB of a bridged port. Example: you have a software LAG
interface on top of your standalone ports, and this is in a software
bridge with other ar9331 ports that directly offload the bridge:
https://patchwork.kernel.org/project/netdevbpf/patch/20210731191023.1329446-3-dqfext@gmail.com/

DSA cannot (easily) help you in that case either, but I agree that
tracking all possible constellations of unoffloaded LAG interfaces on
top of ar9331 switches that also have ports in a hardware bridge is
tricky too. I will give this some thought.

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 235 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 233 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index a0324fed2136..0865ffbc2c74 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -67,6 +67,27 @@
>  #define AR9331_SW_REG_GLOBAL_CTRL		0x30
>  #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
>  
> +#define AR9331_SW_NUM_VLAN_RECORDS		16
> +
> +#define AR9331_SW_REG_VLAN_TABLE_FUNCTION0	0x40
> +#define AR9331_SW_VT0_PRI_EN			BIT(31)
> +#define AR9331_SW_VT0_PRI			GENMASK(30, 28)
> +#define AR9331_SW_VT0_VID			GENMASK(27, 16)
> +#define AR9331_SW_VT0_PORT_NUM			GENMASK(11, 8)
> +#define AR9331_SW_VT0_FULL_VIO			BIT(4)
> +#define AR9331_SW_VT0_BUSY			BIT(3)
> +#define AR9331_SW_VT0_FUNC			GENMASK(2, 0)
> +#define AR9331_SW_VT0_FUNC_NOP			0
> +#define AR9331_SW_VT0_FUNC_FLUSH_ALL		1
> +#define AR9331_SW_VT0_FUNC_LOAD_ENTRY		2
> +#define AR9331_SW_VT0_FUNC_PURGE_ENTRY		3
> +#define AR9331_SW_VT0_FUNC_DEL_PORT		4
> +#define AR9331_SW_VT0_FUNC_GET_NEXT		5
> +
> +#define AR9331_SW_REG_VLAN_TABLE_FUNCTION1	0x44
> +#define AR9331_SW_VT1_VALID			BIT(11)
> +#define AR9331_SW_VT1_VID_MEM			GENMASK(9, 0)
> +
>  /* Size of the address resolution table (ARL) */
>  #define AR9331_SW_NUM_ARL_RECORDS		1024
>  
> @@ -309,6 +330,11 @@ struct ar9331_sw_port {
>  	struct spinlock stats_lock;
>  };
>  
> +struct ar9331_sw_vlan_db {
> +	u16 vid;
> +	u8 port_mask;
> +};
> +
>  struct ar9331_sw_fdb {
>  	u8 port_mask;
>  	u8 aging;
> @@ -327,6 +353,7 @@ struct ar9331_sw_priv {
>  	struct regmap *regmap;
>  	struct reset_control *sw_reset;
>  	struct ar9331_sw_port port[AR9331_SW_PORTS];
> +	struct ar9331_sw_vlan_db vdb[AR9331_SW_NUM_VLAN_RECORDS];
>  };
>  
>  static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
> @@ -557,8 +584,6 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>  			goto error;
>  	}
>  
> -	ds->configure_vlan_while_not_filtering = false;
> -
>  	return 0;
>  error:
>  	dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> @@ -1144,6 +1169,209 @@ static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
>  	ar9331_sw_port_bridge_mod(ds, port, br, false);
>  }
>  
> +static int ar9331_port_vlan_filtering(struct dsa_switch *ds, int port,
> +				      bool vlan_filtering,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	u32 mode;
> +	int ret;
> +
> +	if (vlan_filtering)
> +		mode = AR9331_SW_8021Q_MODE_SECURE;
> +	else
> +		mode = AR9331_SW_8021Q_MODE_NONE;
> +
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				 AR9331_SW_PORT_VLAN_8021Q_MODE,
> +				 FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
> +					    mode));
> +	if (ret)
> +		dev_err(priv->dev, "%s: error: %pe\n", __func__, ERR_PTR(ret));
> +
> +	return ret;
> +}
> +
> +static int ar9331_sw_vt_wait(struct ar9331_sw_priv *priv, u32 *f0)
> +{
> +	struct regmap *regmap = priv->regmap;
> +
> +	return regmap_read_poll_timeout(regmap,
> +					AR9331_SW_REG_VLAN_TABLE_FUNCTION0,
> +					*f0, !(*f0 & AR9331_SW_VT0_BUSY),
> +					100, 2000);
> +}
> +
> +static int ar9331_sw_port_vt_rmw(struct ar9331_sw_priv *priv, u16 vid,
> +				 u8 port_mask_set, u8 port_mask_clr)
> +{
> +	struct regmap *regmap = priv->regmap;
> +	u32 f0, f1, port_mask = 0, port_mask_new, func;

Reverse Christmas tree ordering?

> +	struct ar9331_sw_vlan_db *vdb = NULL;
> +	int ret, i;
> +
> +	if (!vid)
> +		return 0;

Explanation?

> +
> +	ret = ar9331_sw_vt_wait(priv, &f0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
> +	if (ret)
> +		goto error;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, 0);
> +	if (ret)
> +		goto error;
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
> +		if (priv->vdb[i].vid == vid) {
> +			vdb = &priv->vdb[i];
> +			break;
> +		}
> +	}
> +
> +	ret = regmap_read(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, &f1);
> +	if (ret)
> +		return ret;
> +
> +	if (vdb)
> +		port_mask = vdb->port_mask;
> +
> +	port_mask_new = port_mask & ~port_mask_clr;
> +	port_mask_new |= port_mask_set;
> +
> +	if (port_mask_new && port_mask_new == port_mask)
> +		return 0;
> +
> +	if (port_mask_new) {
> +		func = AR9331_SW_VT0_FUNC_LOAD_ENTRY;
> +	} else {
> +		func = AR9331_SW_VT0_FUNC_PURGE_ENTRY;
> +		port_mask_new = port_mask;
> +	}
> +
> +	if (vdb) {
> +		vdb->port_mask = port_mask_new;
> +
> +		if (!port_mask_new)
> +			vdb->vid = 0;
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
> +			if (!priv->vdb[i].vid) {
> +				vdb = &priv->vdb[i];
> +				break;
> +			}
> +		}
> +
> +		if (!vdb)
> +			return -ENOMEM;
> +
> +		vdb->vid = vid;
> +		vdb->port_mask = port_mask_new;
> +	}
> +
> +	f0 = FIELD_PREP(AR9331_SW_VT0_VID, vid) |
> +	     FIELD_PREP(AR9331_SW_VT0_FUNC, func) |
> +	     AR9331_SW_VT0_BUSY;
> +	f1 = FIELD_PREP(AR9331_SW_VT1_VID_MEM, port_mask_new) |
> +		AR9331_SW_VT1_VALID;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, f1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, f0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ar9331_sw_vt_wait(priv, &f0);
> +	if (ret)
> +		return ret;
> +
> +	if (f0 & AR9331_SW_VT0_FULL_VIO) {
> +		/* cleanup error status */
> +		regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
> +		return -ENOMEM;

s/ENOMEM/ENOSPC/ since ENOMEM is for kernel memory allocation AFAIK
Also, what about "ret = -ENOSPC; goto error;" to get the print for this
too and be consistent?

> +	}
> +
> +	return 0;
> +
> +error:
> +	dev_err(priv->dev, "%s: error: %pe\n", __func__, ERR_PTR(ret));
> +
> +	return ret;
> +}
> +
> +static int ar9331_port_vlan_set_pvid(struct ar9331_sw_priv *priv, int port,
> +				     u16 pvid)
> +{
> +	struct regmap *regmap = priv->regmap;
> +	int ret;
> +	u32 mask, val;
> +
> +	mask = AR9331_SW_PORT_VLAN_8021Q_MODE |
> +		AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
> +		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN;
> +	val = AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
> +		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN |
> +		FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
> +			   AR9331_SW_8021Q_MODE_FALLBACK);
> +
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				 mask, val);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				  AR9331_SW_PORT_VLAN_PORT_VID,
> +				  FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
> +					     pvid));
> +}
> +
> +static int ar9331_port_vlan_add(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_vlan *vlan,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct ar9331_sw_priv *priv = ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int ret, mode;
> +
> +	ret = ar9331_sw_port_vt_rmw(priv, vlan->vid, BIT(port), 0);
> +	if (ret)
> +		goto error;
> +
> +	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
> +		ret = ar9331_port_vlan_set_pvid(priv, port, vlan->vid);
> +
> +	if (ret)
> +		goto error;
> +
> +	if (vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED)
> +		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP;
> +	else
> +		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD;
> +
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_CTRL(port),
> +				 AR9331_SW_PORT_CTRL_EG_VLAN_MODE, mode);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err(priv->dev, "%s: error: %pe\n", __func__, ERR_PTR(ret));
> +
> +	return ret;
> +}
> +
> +static int ar9331_port_vlan_del(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_vlan *vlan)
> +{
> +	return ar9331_sw_port_vt_rmw(ds->priv, vlan->vid, 0, BIT(port));
> +}
> +
>  static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
>  	.setup			= ar9331_sw_setup,
> @@ -1162,6 +1390,9 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.set_ageing_time	= ar9331_sw_set_ageing_time,
>  	.port_bridge_join	= ar9331_sw_port_bridge_join,
>  	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
> +	.port_vlan_filtering	= ar9331_port_vlan_filtering,
> +	.port_vlan_add		= ar9331_port_vlan_add,
> +	.port_vlan_del		= ar9331_port_vlan_del,
>  };
>  
>  static irqreturn_t ar9331_sw_irq(int irq, void *data)
> -- 
> 2.30.2
>