[PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Clément Léger]

Add support for vlan operation (add, del, filtering) on the RZN1
driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
tagged/untagged VLANs and PVID for each ports.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/dsa/rzn1_a5psw.c | 182 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/rzn1_a5psw.h |  10 +-
 2 files changed, 189 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index ed413d555bec..8ecb9214b5e6 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -540,6 +540,161 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port,
 	return ret;
 }
 
+static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port,
+				     bool vlan_filtering,
+				     struct netlink_ext_ack *extack)
+{
+	u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT)
+		   | BIT(port + A5PSW_VLAN_DISC_SHIFT);
+	struct a5psw *a5psw = ds->priv;
+	u32 val = 0;
+
+	if (vlan_filtering)
+		val = BIT(port + A5PSW_VLAN_VERI_SHIFT)
+		      | BIT(port + A5PSW_VLAN_DISC_SHIFT);
+
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val);
+
+	return 0;
+}
+
+static int a5psw_find_vlan_entry(struct a5psw *a5psw, u16 vid)
+{
+	u32 vlan_res;
+	int i;
+
+	/* Find vlan for this port */
+	for (i = 0; i < A5PSW_VLAN_COUNT; i++) {
+		vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i));
+		if (FIELD_GET(A5PSW_VLAN_RES_VLANID, vlan_res) == vid)
+			return i;
+	}
+
+	return -1;
+}
+
+static int a5psw_get_vlan_res_entry(struct a5psw *a5psw, u16 newvid)
+{
+	u32 vlan_res;
+	int i;
+
+	/* Find a free VLAN entry */
+	for (i = 0; i < A5PSW_VLAN_COUNT; i++) {
+		vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i));
+		if (!(FIELD_GET(A5PSW_VLAN_RES_PORTMASK, vlan_res))) {
+			vlan_res = FIELD_PREP(A5PSW_VLAN_RES_VLANID, newvid);
+			a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(i), vlan_res);
+			return i;
+		}
+	}
+
+	return -1;
+}
+
+static void a5psw_port_vlan_tagged_cfg(struct a5psw *a5psw, int vlan_res_id,
+				       int port, bool set)
+{
+	u32 mask = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_RD_TAGMASK |
+		   BIT(port);
+	u32 vlan_res_off = A5PSW_VLAN_RES(vlan_res_id);
+	u32 val = A5PSW_VLAN_RES_WR_TAGMASK, reg;
+
+	if (set)
+		val |= BIT(port);
+
+	/* Toggle tag mask read */
+	a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK);
+	reg = a5psw_reg_readl(a5psw, vlan_res_off);
+	a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK);
+
+	reg &= ~mask;
+	reg |= val;
+	a5psw_reg_writel(a5psw, vlan_res_off, reg);
+}
+
+static void a5psw_port_vlan_cfg(struct a5psw *a5psw, int vlan_res_id, int port,
+				bool set)
+{
+	u32 mask = A5PSW_VLAN_RES_WR_TAGMASK | BIT(port);
+	u32 reg = A5PSW_VLAN_RES_WR_PORTMASK;
+
+	if (set)
+		reg |= BIT(port);
+
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_RES(vlan_res_id), mask, reg);
+}
+
+static int a5psw_port_vlan_add(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_vlan *vlan,
+			       struct netlink_ext_ack *extack)
+{
+	bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	struct a5psw *a5psw = ds->priv;
+	u16 vid = vlan->vid;
+	int ret = -EINVAL;
+	int vlan_res_id;
+
+	dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n",
+		vid, port, tagged ? "tagged" : "untagged",
+		pvid ? "PVID" : "no PVID");
+
+	mutex_lock(&a5psw->vlan_lock);
+
+	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
+	if (vlan_res_id < 0) {
+		vlan_res_id = a5psw_get_vlan_res_entry(a5psw, vid);
+		if (vlan_res_id < 0)
+			goto out;
+	}
+
+	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true);
+	if (tagged)
+		a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, true);
+
+	if (pvid) {
+		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
+			      BIT(port));
+		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port), vid);
+	}
+
+	ret = 0;
+out:
+	mutex_unlock(&a5psw->vlan_lock);
+
+	return ret;
+}
+
+static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_vlan *vlan)
+{
+	struct a5psw *a5psw = ds->priv;
+	u16 vid = vlan->vid;
+	int ret = -EINVAL;
+	int vlan_res_id;
+
+	dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port);
+
+	mutex_lock(&a5psw->vlan_lock);
+
+	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
+	if (vlan_res_id < 0)
+		goto out;
+
+	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false);
+	a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false);
+
+	/* Disable PVID if the vid is matching the port one */
+	if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port)))
+		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0);
+
+	ret = 0;
+out:
+	mutex_unlock(&a5psw->vlan_lock);
+
+	return ret;
+}
+
 static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port)
 {
 	u32 reg_lo, reg_hi;
@@ -657,6 +812,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port,
 	ctrl_stats->MACControlFramesReceived = stat;
 }
 
+static void a5psw_vlan_setup(struct a5psw *a5psw, int port)
+{
+	u32 reg;
+
+	/* Enable TAG always mode for the port, this is actually controlled
+	 * by VLAN_IN_MODE_ENA field which will be used for PVID insertion
+	 */
+	reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS;
+	reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port);
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, A5PSW_VLAN_IN_MODE_PORT(port),
+		      reg);
+
+	/* Set transparent mode for output frame manipulation, this will depend
+	 * on the VLAN_RES configuration mode
+	 */
+	reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT;
+	reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port);
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE,
+		      A5PSW_VLAN_OUT_MODE_PORT(port), reg);
+}
+
 static int a5psw_setup(struct dsa_switch *ds)
 {
 	struct a5psw *a5psw = ds->priv;
@@ -729,6 +905,8 @@ static int a5psw_setup(struct dsa_switch *ds)
 		/* Enable management forward only for user ports */
 		if (dsa_port_is_user(dp))
 			a5psw_port_mgmtfwd_set(a5psw, port, true);
+
+		a5psw_vlan_setup(a5psw, port);
 	}
 
 	return 0;
@@ -756,6 +934,9 @@ static const struct dsa_switch_ops a5psw_switch_ops = {
 	.port_bridge_leave = a5psw_port_bridge_leave,
 	.port_stp_state_set = a5psw_port_stp_state_set,
 	.port_fast_age = a5psw_port_fast_age,
+	.port_vlan_filtering = a5psw_port_vlan_filtering,
+	.port_vlan_add = a5psw_port_vlan_add,
+	.port_vlan_del = a5psw_port_vlan_del,
 	.port_fdb_add = a5psw_port_fdb_add,
 	.port_fdb_del = a5psw_port_fdb_del,
 	.port_fdb_dump = a5psw_port_fdb_dump,
@@ -945,6 +1126,7 @@ static int a5psw_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	a5psw->dev = dev;
+	mutex_init(&a5psw->vlan_lock);
 	mutex_init(&a5psw->lk_lock);
 	spin_lock_init(&a5psw->reg_lock);
 	a5psw->base = devm_platform_ioremap_resource(pdev, 0);
diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
index c67abd49c013..614453d9a59e 100644
--- a/drivers/net/dsa/rzn1_a5psw.h
+++ b/drivers/net/dsa/rzn1_a5psw.h
@@ -50,7 +50,9 @@
 #define A5PSW_VLAN_IN_MODE_TAG_ALWAYS		0x2
 
 #define A5PSW_VLAN_OUT_MODE		0x2C
-#define A5PSW_VLAN_OUT_MODE_PORT(port)	(GENMASK(1, 0) << ((port) * 2))
+#define A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port)	((port) * 2)
+#define A5PSW_VLAN_OUT_MODE_PORT(port)	(GENMASK(1, 0) << \
+					A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port))
 #define A5PSW_VLAN_OUT_MODE_DIS		0x0
 #define A5PSW_VLAN_OUT_MODE_STRIP	0x1
 #define A5PSW_VLAN_OUT_MODE_TAG_THROUGH	0x2
@@ -59,7 +61,7 @@
 #define A5PSW_VLAN_IN_MODE_ENA		0x30
 #define A5PSW_VLAN_TAG_ID		0x34
 
-#define A5PSW_SYSTEM_TAGINFO(port)	(0x200 + A5PSW_PORT_OFFSET(port))
+#define A5PSW_SYSTEM_TAGINFO(port)	(0x200 + 4 * (port))
 
 #define A5PSW_AUTH_PORT(port)		(0x240 + 4 * (port))
 #define A5PSW_AUTH_PORT_AUTHORIZED	BIT(0)
@@ -68,7 +70,7 @@
 #define A5PSW_VLAN_RES_WR_PORTMASK	BIT(30)
 #define A5PSW_VLAN_RES_WR_TAGMASK	BIT(29)
 #define A5PSW_VLAN_RES_RD_TAGMASK	BIT(28)
-#define A5PSW_VLAN_RES_ID		GENMASK(16, 5)
+#define A5PSW_VLAN_RES_VLANID		GENMASK(16, 5)
 #define A5PSW_VLAN_RES_PORTMASK		GENMASK(4, 0)
 
 #define A5PSW_RXMATCH_CONFIG(port)	(0x3e80 + 4 * (port))
@@ -240,6 +242,7 @@ union lk_data {
  * @ds: DSA switch struct
  * @stats_lock: lock to access statistics (shared HI counter)
  * @lk_lock: Lock for the lookup table
+ * @vlan_lock: Lock for the vlan operation
  * @reg_lock: Lock for register read-modify-write operation
  * @bridged_ports: Mask of ports that are bridged and should be flooded
  * @br_dev: Bridge net device
@@ -253,6 +256,7 @@ struct a5psw {
 	struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
 	struct dsa_switch ds;
 	struct mutex lk_lock;
+	struct mutex vlan_lock;
 	spinlock_t reg_lock;
 	u32 bridged_ports;
 	struct net_device *br_dev;
-- 
2.39.0

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Jakub Kicinski]

On Wed, 11 Jan 2023 12:56:07 +0100 Clément Léger wrote:
> Add support for vlan operation (add, del, filtering) on the RZN1
> driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> tagged/untagged VLANs and PVID for each ports.

noob question - do you need that mutex? 
aren't those ops all under rtnl_lock?

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Andrew Lunn]

On Thu, Jan 12, 2023 at 09:37:55PM -0800, Jakub Kicinski wrote:
> On Wed, 11 Jan 2023 12:56:07 +0100 Clément Léger wrote:
> > Add support for vlan operation (add, del, filtering) on the RZN1
> > driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> > tagged/untagged VLANs and PVID for each ports.
> 
> noob question - do you need that mutex? 
> aren't those ops all under rtnl_lock?

Hi Jakub

Not commenting about this specific patch, but not everything in DSA is
done under RTNL. So you need to deal with some parallel API calls. But
they tend to be in different areas. I would not expect to see two VLAN
changes as the same time, but maybe VLAN and polling in a workqueue to
update the statistics for example could happen. Depending on the
switch, some protect might be needed to stop these operations
interfering with each other. And DSA drivers in general tend to KISS
and over lock. Nothing here is particularly hot path, the switch
itself is on the end of a slow bus, so the overhead of locks are
minimum.

	Andrew

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Vladimir Oltean]

On Fri, Jan 13, 2023 at 03:12:40PM +0100, Andrew Lunn wrote:
> On Thu, Jan 12, 2023 at 09:37:55PM -0800, Jakub Kicinski wrote:
> > On Wed, 11 Jan 2023 12:56:07 +0100 Clément Léger wrote:
> > > Add support for vlan operation (add, del, filtering) on the RZN1
> > > driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> > > tagged/untagged VLANs and PVID for each ports.
> > 
> > noob question - do you need that mutex? 
> > aren't those ops all under rtnl_lock?
> 
> Hi Jakub
> 
> Not commenting about this specific patch, but not everything in DSA is
> done under RTNL. So you need to deal with some parallel API calls. But
> they tend to be in different areas. I would not expect to see two VLAN
> changes as the same time, but maybe VLAN and polling in a workqueue to
> update the statistics for example could happen. Depending on the
> switch, some protect might be needed to stop these operations
> interfering with each other. And DSA drivers in general tend to KISS
> and over lock. Nothing here is particularly hot path, the switch
> itself is on the end of a slow bus, so the overhead of locks are
> minimum.

That being said, port_vlan_add(), port_vlan_del() and port_vlan_filtering()
are all serialized by the rtnl_lock().

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Clément Léger]

Le Fri, 13 Jan 2023 17:37:30 +0200,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> On Fri, Jan 13, 2023 at 03:12:40PM +0100, Andrew Lunn wrote:
> > On Thu, Jan 12, 2023 at 09:37:55PM -0800, Jakub Kicinski wrote:  
> > > On Wed, 11 Jan 2023 12:56:07 +0100 Clément Léger wrote:  
> > > > Add support for vlan operation (add, del, filtering) on the RZN1
> > > > driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> > > > tagged/untagged VLANs and PVID for each ports.  
> > > 
> > > noob question - do you need that mutex? 
> > > aren't those ops all under rtnl_lock?  
> > 
> > Hi Jakub
> > 
> > Not commenting about this specific patch, but not everything in DSA is
> > done under RTNL. So you need to deal with some parallel API calls. But
> > they tend to be in different areas. I would not expect to see two VLAN
> > changes as the same time, but maybe VLAN and polling in a workqueue to
> > update the statistics for example could happen. Depending on the
> > switch, some protect might be needed to stop these operations
> > interfering with each other. And DSA drivers in general tend to KISS
> > and over lock. Nothing here is particularly hot path, the switch
> > itself is on the end of a slow bus, so the overhead of locks are
> > minimum.  
> 
> That being said, port_vlan_add(), port_vlan_del() and port_vlan_filtering()
> are all serialized by the rtnl_lock().

Ok then, I'll remove this lock and rely on the RTNL lock.

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Arun.Ramadoss]

Hi Clement,
On Wed, 2023-01-11 at 12:56 +0100, Clément Léger wrote:
> Add support for vlan operation (add, del, filtering) on the RZN1
> driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> tagged/untagged VLANs and PVID for each ports.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/net/dsa/rzn1_a5psw.c | 182
> +++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/rzn1_a5psw.h |  10 +-
>  2 files changed, 189 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/rzn1_a5psw.c
> b/drivers/net/dsa/rzn1_a5psw.c
> index ed413d555bec..8ecb9214b5e6 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -540,6 +540,161 @@ static int a5psw_port_fdb_dump(struct
> dsa_switch *ds, int port,
>  	return ret;
>  }
>  
> +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int
> port,
> +				     bool vlan_filtering,
> +				     struct netlink_ext_ack *extack)
> +{
> +	u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT)
> +		   | BIT(port + A5PSW_VLAN_DISC_SHIFT);

Operator | at the end of line

> +	struct a5psw *a5psw = ds->priv;
> +	u32 val = 0;
> +
> +	if (vlan_filtering)
> +		val = BIT(port + A5PSW_VLAN_VERI_SHIFT)
> +		      | BIT(port + A5PSW_VLAN_DISC_SHIFT);

Operator | at the end of line

> +
> +	a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val);
> +
> +	return 0;
> +}
> +
> +static int a5psw_port_vlan_add(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan
> *vlan,
> +			       struct netlink_ext_ack *extack)
> +{
> +	bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct a5psw *a5psw = ds->priv;
> +	u16 vid = vlan->vid;
> +	int ret = -EINVAL;
> +	int vlan_res_id;
> +
> +	dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n",
> +		vid, port, tagged ? "tagged" : "untagged",
> +		pvid ? "PVID" : "no PVID");
> +
> +	mutex_lock(&a5psw->vlan_lock);
> +
> +	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
> +	if (vlan_res_id < 0) {
> +		vlan_res_id = a5psw_get_vlan_res_entry(a5psw, vid);
> +		if (vlan_res_id < 0)

nit: We can initialize ret = 0 initially, and assign ret = -EINVAL here
& remove ret = 0 at end of function.

> +			goto out;
> +	}
> +
> +	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true);
> +	if (tagged)
> +		a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port,
> true);
> +
> +	if (pvid) {
> +		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> +			      BIT(port));
> +		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port),
> vid);
> +	}
> +
> +	ret = 0;
> +out:
> +	mutex_unlock(&a5psw->vlan_lock);
> +
> +	return ret;
> +}
> +
> +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan
> *vlan)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	u16 vid = vlan->vid;
> +	int ret = -EINVAL;

Simillarly here.

> +	int vlan_res_id;
> +
> +	dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid,
> port);
> +
> +	mutex_lock(&a5psw->vlan_lock);
> +
> +	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
> +	if (vlan_res_id < 0)
> +		goto out;
> +
> +	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false);
> +	a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false);
> +
> +	/* Disable PVID if the vid is matching the port one */
> +	if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port)))
> +		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> 0);
> +
> +	ret = 0;
> +out:
> +	mutex_unlock(&a5psw->vlan_lock);
> +
> +	return ret;
> +}
> +
> 

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Clément Léger]

Le Fri, 13 Jan 2023 14:40:26 +0000,
<Arun.Ramadoss@microchip.com> a écrit :

> Hi Clement,
> On Wed, 2023-01-11 at 12:56 +0100, Clément Léger wrote:
> > Add support for vlan operation (add, del, filtering) on the RZN1
> > driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> > tagged/untagged VLANs and PVID for each ports.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/net/dsa/rzn1_a5psw.c | 182
> > +++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/rzn1_a5psw.h |  10 +-
> >  2 files changed, 189 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/rzn1_a5psw.c
> > b/drivers/net/dsa/rzn1_a5psw.c
> > index ed413d555bec..8ecb9214b5e6 100644
> > --- a/drivers/net/dsa/rzn1_a5psw.c
> > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > @@ -540,6 +540,161 @@ static int a5psw_port_fdb_dump(struct
> > dsa_switch *ds, int port,
> >  	return ret;
> >  }
> >  
> > +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int
> > port,
> > +				     bool vlan_filtering,
> > +				     struct netlink_ext_ack *extack)
> > +{
> > +	u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT)
> > +		   | BIT(port + A5PSW_VLAN_DISC_SHIFT);  
> 
> Operator | at the end of line
> 
> > +	struct a5psw *a5psw = ds->priv;
> > +	u32 val = 0;
> > +
> > +	if (vlan_filtering)
> > +		val = BIT(port + A5PSW_VLAN_VERI_SHIFT)
> > +		      | BIT(port + A5PSW_VLAN_DISC_SHIFT);  
> 
> Operator | at the end of line

Hi Arun,

I'll fix that.

> 
> > +
> > +	a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int a5psw_port_vlan_add(struct dsa_switch *ds, int port,
> > +			       const struct switchdev_obj_port_vlan
> > *vlan,
> > +			       struct netlink_ext_ack *extack)
> > +{
> > +	bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> > +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > +	struct a5psw *a5psw = ds->priv;
> > +	u16 vid = vlan->vid;
> > +	int ret = -EINVAL;
> > +	int vlan_res_id;
> > +
> > +	dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n",
> > +		vid, port, tagged ? "tagged" : "untagged",
> > +		pvid ? "PVID" : "no PVID");
> > +
> > +	mutex_lock(&a5psw->vlan_lock);
> > +
> > +	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
> > +	if (vlan_res_id < 0) {
> > +		vlan_res_id = a5psw_get_vlan_res_entry(a5psw, vid);
> > +		if (vlan_res_id < 0)  
> 
> nit: We can initialize ret = 0 initially, and assign ret = -EINVAL here
> & remove ret = 0 at end of function.
> 
> > +			goto out;
> > +	}
> > +
> > +	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true);
> > +	if (tagged)
> > +		a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port,
> > true);
> > +
> > +	if (pvid) {
> > +		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> > +			      BIT(port));
> > +		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port),
> > vid);
> > +	}
> > +
> > +	ret = 0;
> > +out:
> > +	mutex_unlock(&a5psw->vlan_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
> > +			       const struct switchdev_obj_port_vlan
> > *vlan)
> > +{
> > +	struct a5psw *a5psw = ds->priv;
> > +	u16 vid = vlan->vid;
> > +	int ret = -EINVAL;  
> 
> Simillarly here.

Since I removed the mutex thanks to the previous comments, I have
removed all the "ret" usage.

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Vladimir Oltean]

On Wed, Jan 11, 2023 at 12:56:07PM +0100, Clément Léger wrote:
> Add support for vlan operation (add, del, filtering) on the RZN1
> driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> tagged/untagged VLANs and PVID for each ports.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---

Have you run the bridge_vlan_aware.sh and bridge_vlan_unaware.sh from
tools/testing/selftests/drivers/net/dsa/?

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Clément Léger]

Le Fri, 13 Jan 2023 17:12:48 +0200,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> On Wed, Jan 11, 2023 at 12:56:07PM +0100, Clément Léger wrote:
> > Add support for vlan operation (add, del, filtering) on the RZN1
> > driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> > tagged/untagged VLANs and PVID for each ports.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---  
> 
> Have you run the bridge_vlan_aware.sh and bridge_vlan_unaware.sh from
> tools/testing/selftests/drivers/net/dsa/?

Nope, I will do that.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[Clément Léger]

Le Mon, 16 Jan 2023 10:19:14 +0100,
Clément Léger <clement.leger@bootlin.com> a écrit :

> Le Fri, 13 Jan 2023 17:12:48 +0200,
> Vladimir Oltean <olteanv@gmail.com> a écrit :
> 
> > On Wed, Jan 11, 2023 at 12:56:07PM +0100, Clément Léger wrote:  
> > > Add support for vlan operation (add, del, filtering) on the RZN1
> > > driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> > > tagged/untagged VLANs and PVID for each ports.
> > > 
> > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > > ---    
> > 
> > Have you run the bridge_vlan_aware.sh and bridge_vlan_unaware.sh from
> > tools/testing/selftests/drivers/net/dsa/?  
> 
> Nope, I will do that.
> 

Finally found the time to run them and both bridge_vlan_aware.sh and
bridge_vlan_unaware.sh all tests yields a [ OK ] status.

I'll resubmit a V2 with Arun Ramadoss comments fixed.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com