[PATCH net-next v2 1/7] net: dsa: vsc73xx: use read_poll_timeout instead delay loop

[PATCH net-next v2 1/7] net: dsa: vsc73xx: use read_poll_timeout instead delay loop

[Pawel Dembicki]

This commit switches delay loop to read_poll_timeout macro durring
Arbiter empty check in adjust link function.

As Russel King suggested:

"This [change] avoids the issue that on the last iteration, the code reads
the register, test it, find the condition that's being waiting for is
false, _then_ waits and end up printing the error message - that last
wait is rather useless, and as the arbiter state isn't checked after
waiting, it could be that we had success during the last wait."

It also remove one short msleep delay.

Suggested-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - introduced patch

 drivers/net/dsa/vitesse-vsc73xx-core.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index ae55167ce0a6..bea5ec7a89fd 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -780,7 +780,7 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
 	 * after a PHY or the CPU port comes up or down.
 	 */
 	if (!phydev->link) {
-		int maxloop = 10;
+		int ret, err;
 
 		dev_dbg(vsc->dev, "port %d: went down\n",
 			port);
@@ -795,19 +795,16 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
 				    VSC73XX_ARBDISC, BIT(port), BIT(port));
 
 		/* Wait until queue is empty */
-		vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
-			     VSC73XX_ARBEMPTY, &val);
-		while (!(val & BIT(port))) {
-			msleep(1);
-			vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				     VSC73XX_ARBEMPTY, &val);
-			if (--maxloop == 0) {
-				dev_err(vsc->dev,
-					"timeout waiting for block arbiter\n");
-				/* Continue anyway */
-				break;
-			}
-		}
+		ret = read_poll_timeout(vsc73xx_read, err,
+					err < 0 || (val & BIT(port)),
+					1000, 10000, false,
+					vsc, VSC73XX_BLOCK_ARBITER, 0,
+					VSC73XX_ARBEMPTY, &val);
+		if (ret)
+			dev_err(vsc->dev,
+				"timeout waiting for block arbiter\n");
+		else if (err < 0)
+			dev_err(vsc->dev, "error reading arbiter\n");
 
 		/* Put this port into reset */
 		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
-- 
2.34.1

[PATCH net-next v2 2/7] net: dsa: vsc73xx: convert to PHYLINK

[Pawel Dembicki]

This patch replaces the adjust_link api with the phylink apis that provide
equivalent functionality.

The remaining functionality from the adjust_link is now covered in the
phylink_mac_link_* and phylink_mac_config.

Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_down
.phylink_mac_link_up
.phylink_mac_link_down

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - replace switch to if and get rid of macros in
    vsc73xx_phylink_mac_link_up function

 drivers/net/dsa/vitesse-vsc73xx-core.c | 196 +++++++++++++------------
 1 file changed, 102 insertions(+), 94 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index bea5ec7a89fd..221672b9e17f 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -715,8 +715,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 }
 
 static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
-				       int port, struct phy_device *phydev,
-				       u32 initval)
+				       int port, u32 initval)
 {
 	u32 val = initval;
 	u8 seed;
@@ -754,12 +753,40 @@ static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
 			    VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN);
 }
 
-static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
-				struct phy_device *phydev)
+static void vsc73xx_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	struct vsc73xx *vsc = ds->priv;
-	u32 val;
+	/* This switch only supports full-duplex at 1Gbps */
+	config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000FD |
+				   MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
+
+	if (port == CPU_PORT) {
+		__set_bit(PHY_INTERFACE_MODE_RGMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	} else {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		/* Compatibility for phylib's default interface type when the
+		 * phy-mode property is absent
+		 */
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	}
+
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
+}
 
+static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       const struct phylink_link_state *state)
+{
+	struct vsc73xx *vsc = ds->priv;
 	/* Special handling of the CPU-facing port */
 	if (port == CPU_PORT) {
 		/* Other ports are already initialized but not this one */
@@ -775,101 +802,79 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
 			      VSC73XX_ADVPORTM_ENA_GTX |
 			      VSC73XX_ADVPORTM_DDR_MODE);
 	}
+}
 
-	/* This is the MAC confiuration that always need to happen
-	 * after a PHY or the CPU port comes up or down.
-	 */
-	if (!phydev->link) {
-		int ret, err;
-
-		dev_dbg(vsc->dev, "port %d: went down\n",
-			port);
-
-		/* Disable RX on this port */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
-				    VSC73XX_MAC_CFG,
-				    VSC73XX_MAC_CFG_RX_EN, 0);
-
-		/* Discard packets */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				    VSC73XX_ARBDISC, BIT(port), BIT(port));
-
-		/* Wait until queue is empty */
-		ret = read_poll_timeout(vsc73xx_read, err,
-					err < 0 || (val & BIT(port)),
-					1000, 10000, false,
-					vsc, VSC73XX_BLOCK_ARBITER, 0,
-					VSC73XX_ARBEMPTY, &val);
-		if (ret)
-			dev_err(vsc->dev,
-				"timeout waiting for block arbiter\n");
-		else if (err < 0)
-			dev_err(vsc->dev, "error reading arbiter\n");
-
-		/* Put this port into reset */
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
-			      VSC73XX_MAC_CFG_RESET);
-
-		/* Accept packets again */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				    VSC73XX_ARBDISC, BIT(port), 0);
-
-		/* Allow backward dropping of frames from this port */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				    VSC73XX_SBACKWDROP, BIT(port), BIT(port));
-
-		/* Receive mask (disable forwarding) */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
-				    VSC73XX_RECVMASK, BIT(port), 0);
+static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					  unsigned int mode,
+					  phy_interface_t interface)
+{
+	struct vsc73xx *vsc = ds->priv;
+	int ret, err;
+	u32 val;
 
-		return;
-	}
+	/* Disable RX on this port */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+			    VSC73XX_MAC_CFG,
+			    VSC73XX_MAC_CFG_RX_EN, 0);
 
-	/* Figure out what speed was negotiated */
-	if (phydev->speed == SPEED_1000) {
-		dev_dbg(vsc->dev, "port %d: 1000 Mbit mode full duplex\n",
-			port);
-
-		/* Set up default for internal port or external RGMII */
-		if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
-			val = VSC73XX_MAC_CFG_1000M_F_RGMII;
-		else
-			val = VSC73XX_MAC_CFG_1000M_F_PHY;
-		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
-	} else if (phydev->speed == SPEED_100) {
-		if (phydev->duplex == DUPLEX_FULL) {
-			val = VSC73XX_MAC_CFG_100_10M_F_PHY;
-			dev_dbg(vsc->dev,
-				"port %d: 100 Mbit full duplex mode\n",
-				port);
-		} else {
-			val = VSC73XX_MAC_CFG_100_10M_H_PHY;
-			dev_dbg(vsc->dev,
-				"port %d: 100 Mbit half duplex mode\n",
-				port);
-		}
-		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
-	} else if (phydev->speed == SPEED_10) {
-		if (phydev->duplex == DUPLEX_FULL) {
-			val = VSC73XX_MAC_CFG_100_10M_F_PHY;
-			dev_dbg(vsc->dev,
-				"port %d: 10 Mbit full duplex mode\n",
-				port);
-		} else {
-			val = VSC73XX_MAC_CFG_100_10M_H_PHY;
-			dev_dbg(vsc->dev,
-				"port %d: 10 Mbit half duplex mode\n",
-				port);
-		}
-		vsc73xx_adjust_enable_port(vsc, port, phydev, val);
-	} else {
+	/* Discard packets */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			    VSC73XX_ARBDISC, BIT(port), BIT(port));
+
+	/* Wait until queue is empty */
+	ret = read_poll_timeout(vsc73xx_read, err, err < 0 || (val & BIT(port)),
+				1000, 10000, false, vsc, VSC73XX_BLOCK_ARBITER,
+				0, VSC73XX_ARBEMPTY, &val);
+	if (ret)
 		dev_err(vsc->dev,
-			"could not adjust link: unknown speed\n");
-	}
+			"timeout waiting for block arbiter\n");
+	else if (err < 0)
+		dev_err(vsc->dev, "error reading arbiter\n");
+
+	/* Put this port into reset */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+		      VSC73XX_MAC_CFG_RESET);
+
+	/* Accept packets again */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			    VSC73XX_ARBDISC, BIT(port), 0);
+
+	/* Allow backward dropping of frames from this port */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			    VSC73XX_SBACKWDROP, BIT(port), BIT(port));
+
+	/* Receive mask (disable forwarding) */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+			    VSC73XX_RECVMASK, BIT(port), 0);
+}
+
+static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
+					unsigned int mode,
+					phy_interface_t interface,
+					struct phy_device *phydev,
+					int speed, int duplex,
+					bool tx_pause, bool rx_pause)
+{
+	struct vsc73xx *vsc = ds->priv;
+	u32 val;
+
+	if (speed == SPEED_1000)
+		val = VSC73XX_MAC_CFG_GIGA_MODE | VSC73XX_MAC_CFG_TX_IPG_1000M;
+	else
+		val = VSC73XX_MAC_CFG_TX_IPG_100_10M;
+
+	if (interface == PHY_INTERFACE_MODE_RGMII)
+		val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
+	else
+		val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
+
+	if (duplex == DUPLEX_FULL)
+		val |= VSC73XX_MAC_CFG_FDX;
 
 	/* Enable port (forwarding) in the receieve mask */
 	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
 			    VSC73XX_RECVMASK, BIT(port), BIT(port));
+	vsc73xx_adjust_enable_port(vsc, port, val);
 }
 
 static int vsc73xx_port_enable(struct dsa_switch *ds, int port,
@@ -1040,7 +1045,10 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.setup = vsc73xx_setup,
 	.phy_read = vsc73xx_phy_read,
 	.phy_write = vsc73xx_phy_write,
-	.adjust_link = vsc73xx_adjust_link,
+	.phylink_get_caps = vsc73xx_phylink_get_caps,
+	.phylink_mac_config = vsc73xx_phylink_mac_config,
+	.phylink_mac_link_down = vsc73xx_phylink_mac_link_down,
+	.phylink_mac_link_up = vsc73xx_phylink_mac_link_up,
 	.get_strings = vsc73xx_get_strings,
 	.get_ethtool_stats = vsc73xx_get_ethtool_stats,
 	.get_sset_count = vsc73xx_get_sset_count,
-- 
2.34.1

[PATCH net-next v2 3/7] net: dsa: vsc73xx: add port_stp_state_set function

[Pawel Dembicki]

This isn't fully functional implementation of 802.1D, but
port_stp_state_set is required for future tag8021q operations.

This implementation handle properly all states, but vsc 73xx don't
forward STP packets.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - fix kdoc

 drivers/net/dsa/vitesse-vsc73xx-core.c | 51 +++++++++++++++++++++++---
 drivers/net/dsa/vitesse-vsc73xx.h      |  2 +
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 221672b9e17f..f123ce2ed244 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -164,6 +164,10 @@
 #define VSC73XX_AGENCTRL	0xf0
 #define VSC73XX_CAPRST		0xff
 
+#define VSC73XX_SRCMASKS_CPU_COPY		BIT(27)
+#define VSC73XX_SRCMASKS_MIRROR			BIT(26)
+#define VSC73XX_SRCMASKS_PORTS_MASK		GENMASK(7, 0)
+
 #define VSC73XX_MACACCESS_CPU_COPY		BIT(14)
 #define VSC73XX_MACACCESS_FWD_KILL		BIT(13)
 #define VSC73XX_MACACCESS_IGNORE_VLAN		BIT(12)
@@ -620,15 +624,17 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
 		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
 		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
-	/* Enable reception of frames on all ports */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
-		      0x5f);
 	/* IP multicast flood mask (table 144) */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
 		      0xff);
 
 	mdelay(50);
 
+	/*configure forward map to CPU <-> port only*/
+	for (i = 0; i < vsc->ds->num_ports; i++)
+		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
+	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(CPU_PORT);
+
 	/* Release reset from the internal PHYs */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_PHY_RESET);
@@ -871,9 +877,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
 	if (duplex == DUPLEX_FULL)
 		val |= VSC73XX_MAC_CFG_FDX;
 
-	/* Enable port (forwarding) in the receieve mask */
-	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
-			    VSC73XX_RECVMASK, BIT(port), BIT(port));
 	vsc73xx_adjust_enable_port(vsc, port, val);
 }
 
@@ -1040,6 +1043,41 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
 	return 9600;
 }
 
+static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
+				       u8 state)
+{
+	struct vsc73xx *vsc = ds->priv;
+	/* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
+	 * forwarded only from to PI/SI interface. For more info see chapter
+	 * 2.7.1 (CPU Forwarding) in datasheet.
+	 * This function is required for tag8021q operations.
+	 */
+
+	if (state == BR_STATE_BLOCKING)
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_RECVMASK, BIT(port), 0);
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_RECVMASK, BIT(port), BIT(port));
+
+	if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_LEARNMASK, BIT(port), BIT(port));
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_LEARNMASK, BIT(port), 0);
+
+	if (state == BR_STATE_FORWARDING)
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_SRCMASKS + port,
+				    VSC73XX_SRCMASKS_PORTS_MASK,
+				    vsc->forward_map[port]);
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_SRCMASKS + port,
+				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
+}
+
 static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.get_tag_protocol = vsc73xx_get_tag_protocol,
 	.setup = vsc73xx_setup,
@@ -1056,6 +1094,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.port_disable = vsc73xx_port_disable,
 	.port_change_mtu = vsc73xx_change_mtu,
 	.port_max_mtu = vsc73xx_get_max_mtu,
+	.port_stp_state_set = vsc73xx_port_stp_state_set,
 };
 
 static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index 30b1f0a36566..c4d5398edeeb 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -5,6 +5,7 @@
 
 /**
  * struct vsc73xx - VSC73xx state container
+ * @forward_map: Forward table cache
  */
 struct vsc73xx {
 	struct device			*dev;
@@ -15,6 +16,7 @@ struct vsc73xx {
 	u8				addr[ETH_ALEN];
 	const struct vsc73xx_ops	*ops;
 	void				*priv;
+	u8				forward_map[8];
 };
 
 struct vsc73xx_ops {
-- 
2.34.1

Re: [PATCH net-next v2 3/7] net: dsa: vsc73xx: add port_stp_state_set function

[Vladimir Oltean]

On Sun, Jun 25, 2023 at 01:53:38PM +0200, Pawel Dembicki wrote:
> This isn't fully functional implementation of 802.1D, but
> port_stp_state_set is required for future tag8021q operations.
> 
> This implementation handle properly all states, but vsc 73xx don't

handles

vsc73xx doesn't

> forward STP packets.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v2:
>   - fix kdoc
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 51 +++++++++++++++++++++++---
>  drivers/net/dsa/vitesse-vsc73xx.h      |  2 +
>  2 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 221672b9e17f..f123ce2ed244 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -164,6 +164,10 @@
>  #define VSC73XX_AGENCTRL	0xf0
>  #define VSC73XX_CAPRST		0xff
>  
> +#define VSC73XX_SRCMASKS_CPU_COPY		BIT(27)
> +#define VSC73XX_SRCMASKS_MIRROR			BIT(26)
> +#define VSC73XX_SRCMASKS_PORTS_MASK		GENMASK(7, 0)
> +
>  #define VSC73XX_MACACCESS_CPU_COPY		BIT(14)
>  #define VSC73XX_MACACCESS_FWD_KILL		BIT(13)
>  #define VSC73XX_MACACCESS_IGNORE_VLAN		BIT(12)
> @@ -620,15 +624,17 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
>  		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
>  		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> -	/* Enable reception of frames on all ports */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
> -		      0x5f);
>  	/* IP multicast flood mask (table 144) */
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
>  		      0xff);
>  
>  	mdelay(50);
>  
> +	/*configure forward map to CPU <-> port only*/
> +	for (i = 0; i < vsc->ds->num_ports; i++)
> +		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
> +	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(CPU_PORT);
> +
>  	/* Release reset from the internal PHYs */
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
>  		      VSC73XX_GLORESET_PHY_RESET);
> @@ -871,9 +877,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  	if (duplex == DUPLEX_FULL)
>  		val |= VSC73XX_MAC_CFG_FDX;
>  
> -	/* Enable port (forwarding) in the receieve mask */
> -	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> -			    VSC73XX_RECVMASK, BIT(port), BIT(port));
>  	vsc73xx_adjust_enable_port(vsc, port, val);
>  }
>  
> @@ -1040,6 +1043,41 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
>  	return 9600;
>  }
>  
> +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct vsc73xx *vsc = ds->priv;

Blank link after variable declarations; this affects comments too.
You can either put that comment above the function, or right above the
first "if" block (to me it makes more sense above the function).

> +	/* FIXME: STP frames isn't forwarded at this moment. BPDU frames are

s/isn't/aren't/

> +	 * forwarded only from to PI/SI interface. For more info see chapter

s/from to/from and to/

> +	 * 2.7.1 (CPU Forwarding) in datasheet.
> +	 * This function is required for tag8021q operations.
> +	 */
> +
> +	if (state == BR_STATE_BLOCKING)

state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED

> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), 0);
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), BIT(port));
> +
> +	if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_LEARNMASK, BIT(port), BIT(port));
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_LEARNMASK, BIT(port), 0);
> +
> +	if (state == BR_STATE_FORWARDING)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK,
> +				    vsc->forward_map[port]);
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +}
> +
>  static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_tag_protocol = vsc73xx_get_tag_protocol,
>  	.setup = vsc73xx_setup,
> @@ -1056,6 +1094,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.port_disable = vsc73xx_port_disable,
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
> +	.port_stp_state_set = vsc73xx_port_stp_state_set,
>  };
>  
>  static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index 30b1f0a36566..c4d5398edeeb 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -5,6 +5,7 @@
>  
>  /**
>   * struct vsc73xx - VSC73xx state container
> + * @forward_map: Forward table cache
>   */
>  struct vsc73xx {
>  	struct device			*dev;
> @@ -15,6 +16,7 @@ struct vsc73xx {
>  	u8				addr[ETH_ALEN];
>  	const struct vsc73xx_ops	*ops;
>  	void				*priv;
> +	u8				forward_map[8];

Can we have this 8 and the one from the vsc->ds->num_ports assignment
consolidated into a single macro, VSC73XX_MAX_NUM_PORTS?

>  };
>  
>  struct vsc73xx_ops {
> -- 
> 2.34.1
> 

[PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Pawel Dembicki]

This patch is simple implementation of 8021q tagging in vsc73xx driver.
At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
family doesn't provide any tag support for external ethernet ports.

The only way is vlan-based tagging. It require constant hardware vlan
filtering. VSC73XX family support provider bridging but QinQ only without
fully implemented 802.1AD. It allow only doubled 0x8100 TPID.

In simple port mode QinQ is enabled to preserve forwarding vlan tagged
frames.

Tag driver introduce most simple funcionality required for proper taging
support.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - change poll loop into dedicated macro
  - fix style issues

 drivers/net/dsa/Kconfig                |   2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c | 532 +++++++++++++++++++++----
 include/net/dsa.h                      |   2 +
 net/dsa/Kconfig                        |   6 +
 net/dsa/Makefile                       |   1 +
 net/dsa/tag_vsc73xx_8021q.c            |  87 ++++
 6 files changed, 544 insertions(+), 86 deletions(-)
 create mode 100644 net/dsa/tag_vsc73xx_8021q.c

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 3ed5391bb18d..4cf0166fef7b 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -125,7 +125,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
 
 config NET_DSA_VITESSE_VSC73XX
 	tristate
-	select NET_DSA_TAG_NONE
+	select NET_DSA_TAG_VSC73XX
 	select FIXED_PHY
 	select VITESSE_PHY
 	select GPIOLIB
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index f123ce2ed244..f7c38f9a81a8 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
@@ -25,6 +26,7 @@
 #include <linux/etherdevice.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/dsa/8021q.h>
 #include <linux/random.h>
 #include <net/dsa.h>
 
@@ -62,6 +64,8 @@
 #define VSC73XX_CAT_DROP	0x6e
 #define VSC73XX_CAT_PR_MISC_L2	0x6f
 #define VSC73XX_CAT_PR_USR_PRIO	0x75
+#define VSC73XX_CAT_VLAN_MISC	0x79
+#define VSC73XX_CAT_PORT_VLAN	0x7a
 #define VSC73XX_Q_MISC_CONF	0xdf
 
 /* MAC_CFG register bits */
@@ -122,6 +126,17 @@
 #define VSC73XX_ADVPORTM_IO_LOOPBACK	BIT(1)
 #define VSC73XX_ADVPORTM_HOST_LOOPBACK	BIT(0)
 
+/*  TXUPDCFG transmit modify setup bits */
+#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE	GENMASK(20, 19)
+#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA	BIT(18)
+#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA	BIT(17)
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID	GENMASK(15, 4)
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA	BIT(3)
+#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA	BIT(1)
+#define VSC73XX_TXUPDCFG_TX_INSERT_TAG	BIT(0)
+
+#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
+
 /* CAT_DROP categorizer frame dropping register bits */
 #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA	BIT(6)
 #define VSC73XX_CAT_DROP_FWD_CTRL_ENA		BIT(4)
@@ -135,6 +150,15 @@
 #define VSC73XX_Q_MISC_CONF_EARLY_TX_512	(1 << 1)
 #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE	BIT(0)
 
+/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
+#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
+#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
+
+/* CAT_PORT_VLAN categorizer port VLAN*/
+#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
+#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
+#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
+
 /* Frame analyzer block 2 registers */
 #define VSC73XX_STORMLIMIT	0x02
 #define VSC73XX_ADVLEARN	0x03
@@ -189,7 +213,8 @@
 #define VSC73XX_VLANACCESS_VLAN_MIRROR		BIT(29)
 #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK	BIT(28)
 #define VSC73XX_VLANACCESS_VLAN_PORT_MASK	GENMASK(9, 2)
-#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK	GENMASK(2, 0)
+#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT	2
+#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK	GENMASK(1, 0)
 #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE	0
 #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY	1
 #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY	2
@@ -344,6 +369,13 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
 	{ 29, "TxQoSClass3" }, /* non-standard counter */
 };
 
+enum vsc73xx_port_vlan_conf {
+	VSC73XX_VLAN_UNAWARE,
+	VSC73XX_VLAN_AWARE,
+	VSC73XX_DOUBLE_VLAN_AWARE,
+	VSC73XX_DOUBLE_VLAN_CPU_AWARE,
+};
+
 int vsc73xx_is_addr_valid(u8 block, u8 subblock)
 {
 	switch (block) {
@@ -558,90 +590,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
 	 * cannot access the tag. (See "Internal frame header" section
 	 * 3.9.1 in the manual.)
 	 */
-	return DSA_TAG_PROTO_NONE;
-}
-
-static int vsc73xx_setup(struct dsa_switch *ds)
-{
-	struct vsc73xx *vsc = ds->priv;
-	int i;
-
-	dev_info(vsc->dev, "set up the switch\n");
-
-	/* Issue RESET */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
-		      VSC73XX_GLORESET_MASTER_RESET);
-	usleep_range(125, 200);
-
-	/* Initialize memory, initialize RAM bank 0..15 except 6 and 7
-	 * This sequence appears in the
-	 * VSC7385 SparX-G5 datasheet section 6.6.1
-	 * VSC7395 SparX-G5e datasheet section 6.6.1
-	 * "initialization sequence".
-	 * No explanation is given to the 0x1010400 magic number.
-	 */
-	for (i = 0; i <= 15; i++) {
-		if (i != 6 && i != 7) {
-			vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
-				      2,
-				      0, 0x1010400 + i);
-			mdelay(1);
-		}
-	}
-	mdelay(30);
-
-	/* Clear MAC table */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
-		      VSC73XX_MACACCESS,
-		      VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
-
-	/* Clear VLAN table */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
-		      VSC73XX_VLANACCESS,
-		      VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
-
-	msleep(40);
-
-	/* Use 20KiB buffers on all ports on VSC7395
-	 * The VSC7385 has 16KiB buffers and that is the
-	 * default if we don't set this up explicitly.
-	 * Port "31" is "all ports".
-	 */
-	if (IS_739X(vsc))
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
-			      VSC73XX_Q_MISC_CONF,
-			      VSC73XX_Q_MISC_CONF_EXTENT_MEM);
-
-	/* Put all ports into reset until enabled */
-	for (i = 0; i < 7; i++) {
-		if (i == 5)
-			continue;
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
-			      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
-	}
-
-	/* MII delay, set both GTX and RX delay to 2 ns */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
-		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
-		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
-	/* IP multicast flood mask (table 144) */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
-		      0xff);
-
-	mdelay(50);
-
-	/*configure forward map to CPU <-> port only*/
-	for (i = 0; i < vsc->ds->num_ports; i++)
-		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
-	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(CPU_PORT);
-
-	/* Release reset from the internal PHYs */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
-		      VSC73XX_GLORESET_PHY_RESET);
-
-	udelay(4);
-
-	return 0;
+	return DSA_TAG_PROTO_VSC73XX_8021Q;
 }
 
 static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
@@ -1078,6 +1027,417 @@ static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
 				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
 }
 
+static int
+vsc73xx_port_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
+{
+	u32 val;
+	int ret, err;
+
+	ret = read_poll_timeout(vsc73xx_read, err,
+				err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) ==
+					    VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE),
+				1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
+				0, VSC73XX_VLANACCESS, &val);
+	if (ret)
+		return ret;
+	return err;
+}
+
+static int
+vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap)
+{
+	struct vsc73xx *vsc = ds->priv;
+	u32 val;
+	int ret;
+
+	if (vid > 4095)
+		return -EPERM;
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
+	if (ret)
+		return ret;
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
+			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
+			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
+	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
+	if (ret)
+		return ret;
+	vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
+	*portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
+		   VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
+	return 0;
+}
+
+static int
+vsc73xx_port_write_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 portmap)
+{
+	struct vsc73xx *vsc = ds->priv;
+	int ret;
+
+	if (vid > 4095)
+		return -EPERM;
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
+	if (ret)
+		return ret;
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
+			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
+			    VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
+			    VSC73XX_VLANACCESS_VLAN_PORT_MASK,
+			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
+			    VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
+			    (portmap <<
+			     VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
+	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+static int
+vsc73xx_port_update_vlan_table(struct dsa_switch *ds, int port, u16 vid,
+			       bool set)
+{
+	u8 portmap;
+	int ret;
+
+	if (vid > 4095)
+		return -EPERM;
+
+	ret = vsc73xx_port_read_vlan_table_entry(ds, vid, &portmap);
+	if (ret)
+		return ret;
+
+	if (set)
+		portmap |= BIT(port) | BIT(CPU_PORT);
+	else
+		portmap &= ~BIT(port);
+
+	if (portmap == BIT(CPU_PORT))
+		portmap = 0;
+
+	ret = vsc73xx_port_write_vlan_table_entry(ds, vid, portmap);
+
+	return ret;
+}
+
+static void
+vsc73xx_port_set_vlan_conf(struct dsa_switch *ds, int port,
+			   enum vsc73xx_port_vlan_conf port_vlan_conf)
+{
+	struct vsc73xx *vsc = ds->priv;
+
+	if (port_vlan_conf == VSC73XX_VLAN_UNAWARE) {
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_MAC_CFG,
+				    VSC73XX_MAC_CFG_VLAN_AWR, 0);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_MAC_CFG,
+				    VSC73XX_MAC_CFG_VLAN_DBLAWR, 0);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_VLAN_MISC,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_VLAN_MISC,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_DROP,
+				    VSC73XX_CAT_DROP_TAGGED_ENA, 0);
+	} else {
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_MAC_CFG,
+				    VSC73XX_MAC_CFG_VLAN_AWR,
+				    VSC73XX_MAC_CFG_VLAN_AWR);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_DROP,
+				    VSC73XX_CAT_DROP_TAGGED_ENA, 0);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_DROP,
+				    VSC73XX_CAT_DROP_UNTAGGED_ENA,
+				    VSC73XX_CAT_DROP_UNTAGGED_ENA);
+
+		if (port_vlan_conf == VSC73XX_DOUBLE_VLAN_CPU_AWARE)
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_MAC_CFG,
+					    VSC73XX_MAC_CFG_VLAN_DBLAWR,
+					    VSC73XX_MAC_CFG_VLAN_DBLAWR);
+		else
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_MAC_CFG,
+					    VSC73XX_MAC_CFG_VLAN_DBLAWR, 0);
+
+		if (port_vlan_conf == VSC73XX_DOUBLE_VLAN_AWARE) {
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_CAT_VLAN_MISC,
+					    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+					    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_CAT_VLAN_MISC,
+					    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
+					    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_TXUPDCFG,
+					    VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0);
+		} else {
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_CAT_VLAN_MISC,
+					    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+					    0);
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_CAT_VLAN_MISC,
+					    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
+					    0);
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_TXUPDCFG,
+					    VSC73XX_TXUPDCFG_TX_INSERT_TAG,
+					    VSC73XX_TXUPDCFG_TX_INSERT_TAG);
+		}
+
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_TXUPDCFG,
+				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_TXUPDCFG,
+				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_PORT_VLAN,
+				    VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
+	}
+}
+
+static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
+{
+	int i, ret;
+
+	if (port == CPU_PORT)
+		vsc73xx_port_set_vlan_conf(ds, port,
+					   VSC73XX_DOUBLE_VLAN_CPU_AWARE);
+	else
+		vsc73xx_port_set_vlan_conf(ds, port,
+					   VSC73XX_DOUBLE_VLAN_AWARE);
+
+	for (i = 0; i <= 4095; i++) {
+		ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
+		if (ret)
+			return ret;
+	}
+	return ret;
+}
+
+static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
+				     bool port_vlan)
+{
+	struct vsc73xx *vsc = ds->priv;
+	u16 vlan_no;
+	u32 val;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+
+	if (port_vlan && (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)) {
+		vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+			     &val);
+		vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+				VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+		if (!vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) &&
+		    vlan_no != vid) {
+			dev_warn(vsc->dev,
+				 "Port %d can have only one untagged vid! Now is: %d.\n",
+				 port, vlan_no);
+				return -EPERM;
+		}
+	}
+
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+			    VSC73XX_CAT_DROP_UNTAGGED_ENA, 0);
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
+			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA);
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
+			    (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
+			     VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
+	return 0;
+}
+
+static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
+				 bool port_vlan)
+{
+	struct dsa_port *dsa_port = dsa_to_port(ds, port);
+	struct vsc73xx *vsc = ds->priv;
+	u16 vlan_no;
+	u32 val;
+
+	if (!dsa_port)
+		return -EINVAL;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+	vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+
+	if (port_vlan && vlan_no && !vid_is_dsa_8021q(vlan_no) &&
+	    !vid_is_dsa_8021q(vid) && vlan_no != vid) {
+		dev_warn(vsc->dev,
+			 "Port %d can have only one pvid! Now is: %d.\n",
+			 port, vlan_no);
+		return -EPERM;
+	}
+
+	if (dsa_port_is_vlan_filtering(dsa_port))
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_VLAN_MISC,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+				    0);
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_VLAN_MISC,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
+				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
+
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
+			    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
+			    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
+
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
+			    VSC73XX_CAT_PORT_VLAN_VLAN_VID,
+			    vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
+	return 0;
+}
+
+static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
+				      u16 flags)
+{
+	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = flags & BRIDGE_VLAN_INFO_PVID;
+	int ret;
+
+	if (untagged) {
+		ret = vsc73xx_vlan_set_untagged(ds, port, vid, false);
+		if (ret)
+			return ret;
+	}
+	if (pvid) {
+		ret = vsc73xx_vlan_set_pvid(ds, port, vid, false);
+		if (ret)
+			return ret;
+	}
+	ret = vsc73xx_port_update_vlan_table(ds, port, vid, 1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
+{
+	return vsc73xx_port_update_vlan_table(ds, port, vid, 0);
+}
+
+static int vsc73xx_setup(struct dsa_switch *ds)
+{
+	struct vsc73xx *vsc = ds->priv;
+	int i, ret;
+
+	dev_info(vsc->dev, "set up the switch\n");
+
+	ds->vlan_filtering_is_global = false;
+	ds->configure_vlan_while_not_filtering = false;
+
+	/* Issue RESET */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
+		      VSC73XX_GLORESET_MASTER_RESET);
+	usleep_range(125, 200);
+
+	/* Initialize memory, initialize RAM bank 0..15 except 6 and 7
+	 * This sequence appears in the
+	 * VSC7385 SparX-G5 datasheet section 6.6.1
+	 * VSC7395 SparX-G5e datasheet section 6.6.1
+	 * "initialization sequence".
+	 * No explanation is given to the 0x1010400 magic number.
+	 */
+	for (i = 0; i <= 15; i++) {
+		if (i != 6 && i != 7) {
+			vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
+				      2,
+				      0, 0x1010400 + i);
+			mdelay(1);
+		}
+	}
+	mdelay(30);
+
+	/* Clear MAC table */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+		      VSC73XX_MACACCESS,
+		      VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
+
+	/* Clear VLAN table */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+		      VSC73XX_VLANACCESS,
+		      VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
+
+	msleep(40);
+
+	/* Use 20KiB buffers on all ports on VSC7395
+	 * The VSC7385 has 16KiB buffers and that is the
+	 * default if we don't set this up explicitly.
+	 * Port "31" is "all ports".
+	 */
+	if (IS_739X(vsc))
+		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
+			      VSC73XX_Q_MISC_CONF,
+			      VSC73XX_Q_MISC_CONF_EXTENT_MEM);
+
+	/* Put all ports into reset until enabled */
+	for (i = 0; i < 7; i++) {
+		if (i == 5)
+			continue;
+		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
+			      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
+	}
+
+	/* MII delay, set both GTX and RX delay to 2 ns */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
+		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
+		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
+	/* Ingess VLAN reception mask (table 145) */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK,
+		      0x5f);
+	/* IP multicast flood mask (table 144) */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
+		      0xff);
+
+	mdelay(50);
+
+	for (i = 0; i < vsc->ds->num_ports; i++) {
+		if (i == 5)
+			continue;
+		ret = vsc73xx_port_set_double_vlan_aware(ds, i);
+		if (ret)
+			return ret;
+	}
+
+	rtnl_lock();
+	ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
+	rtnl_unlock();
+	if (ret)
+		return ret;
+
+	/*configure forward map to CPU <-> port only*/
+	for (i = 0; i < vsc->ds->num_ports; i++)
+		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK &
+				      BIT(CPU_PORT);
+	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
+				     ~BIT(CPU_PORT);
+
+	/* Release reset from the internal PHYs */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
+		      VSC73XX_GLORESET_PHY_RESET);
+
+	udelay(4);
+
+	return 0;
+}
+
 static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.get_tag_protocol = vsc73xx_get_tag_protocol,
 	.setup = vsc73xx_setup,
@@ -1095,6 +1455,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.port_change_mtu = vsc73xx_change_mtu,
 	.port_max_mtu = vsc73xx_get_max_mtu,
 	.port_stp_state_set = vsc73xx_port_stp_state_set,
+	.tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
+	.tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
 };
 
 static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 75022cf771cf..2440df7ea6c9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -56,6 +56,7 @@ struct phylink_link_state;
 #define DSA_TAG_PROTO_RTL8_4T_VALUE		25
 #define DSA_TAG_PROTO_RZN1_A5PSW_VALUE		26
 #define DSA_TAG_PROTO_LAN937X_VALUE		27
+#define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE	28
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -86,6 +87,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_RTL8_4T		= DSA_TAG_PROTO_RTL8_4T_VALUE,
 	DSA_TAG_PROTO_RZN1_A5PSW	= DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
 	DSA_TAG_PROTO_LAN937X		= DSA_TAG_PROTO_LAN937X_VALUE,
+	DSA_TAG_PROTO_VSC73XX_8021Q	= DSA_TAG_PROTO_VSC73XX_8021Q_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 8e698bea99a3..e59360071c67 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -166,6 +166,12 @@ config NET_DSA_TAG_TRAILER
 	  Say Y or M if you want to enable support for tagging frames at
 	  with a trailed. e.g. Marvell 88E6060.
 
+config NET_DSA_TAG_VSC73XX_8021Q
+	tristate "Tag driver for Microchip/Vitesse VSC73xx family of switches, using VLAN"
+	help
+	  Say Y or M if you want to enable support for tagging frames with a
+	  custom VLAN-based header.
+
 config NET_DSA_TAG_XRS700X
 	tristate "Tag driver for XRS700x switches"
 	help
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 12e305824a96..bab8a933c514 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
 obj-$(CONFIG_NET_DSA_TAG_RZN1_A5PSW) += tag_rzn1_a5psw.o
 obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
 obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
+obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
 obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
 
 # for tracing framework to find trace.h
diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
new file mode 100644
index 000000000000..3d83dfecde31
--- /dev/null
+++ b/net/dsa/tag_vsc73xx_8021q.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>
+ * Based on tag_sja1105.c:
+ * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
+ */
+#include <linux/dsa/8021q.h>
+
+#include "tag.h"
+#include "tag_8021q.h"
+
+#define VSC73XX_8021Q_NAME "vsc73xx-8021q"
+
+static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb,
+				    struct net_device *netdev)
+{
+	struct dsa_port *dp = dsa_slave_to_port(netdev);
+	u16 queue_mapping = skb_get_queue_mapping(skb);
+	u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
+	u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
+	struct net_device *br = dsa_port_bridge_dev_get(dp);
+
+	if (skb->offload_fwd_mark) {
+		unsigned int bridge_num = dsa_port_bridge_num_get(dp);
+
+		if (br_vlan_enabled(br))
+			return skb;
+		else
+			tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
+	}
+
+	return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,
+			      ((pcp << VLAN_PRIO_SHIFT) | tx_vid));
+}
+
+static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
+			     int *switch_id, int *vbid, u16 *vid)
+{
+	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
+		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
+
+	/* Try our best with imprecise RX */
+	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
+}
+
+static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
+				   struct net_device *netdev)
+{
+	int src_port = -1, switch_id = -1, vbid = -1;
+	u16 vid;
+
+	if (skb_vlan_tag_present(skb))
+		/* Normal traffic path. */
+		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
+
+	if (vbid >= 1)
+		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
+	else if (src_port == -1 || switch_id == -1)
+		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
+	else
+		skb->dev = dsa_master_find_slave(netdev, switch_id, src_port);
+	if (!skb->dev) {
+		netdev_warn(netdev, "Couldn't decode source port\n");
+		return NULL;
+	}
+
+	dsa_default_offload_fwd_mark(skb);
+
+	if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) &&
+	    eth_hdr(skb)->h_proto == htons(ETH_P_8021Q))
+		__vlan_hwaccel_clear_tag(skb);
+
+	return skb;
+}
+
+static const struct dsa_device_ops vsc73xx_8021q_netdev_ops = {
+	.name			= VSC73XX_8021Q_NAME,
+	.proto			= DSA_TAG_PROTO_VSC73XX_8021Q,
+	.xmit			= vsc73xx_xmit,
+	.rcv			= vsc73xx_rcv,
+	.needed_headroom	= VLAN_HLEN,
+	.promisc_on_master	= true,
+};
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_VSC73XX_8021Q, VSC73XX_8021Q_NAME);
+
+module_dsa_tag_driver(vsc73xx_8021q_netdev_ops);
-- 
2.34.1

Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Simon Horman]

+ Dan Carpenter

On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> This patch is simple implementation of 8021q tagging in vsc73xx driver.
> At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> family doesn't provide any tag support for external ethernet ports.
> 
> The only way is vlan-based tagging. It require constant hardware vlan
> filtering. VSC73XX family support provider bridging but QinQ only without
> fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> 
> In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> frames.
> 
> Tag driver introduce most simple funcionality required for proper taging
> support.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

...

> +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> +			     int *switch_id, int *vbid, u16 *vid)
> +{
> +	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> +		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> +
> +	/* Try our best with imprecise RX */
> +	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> +}
> +
> +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
> +				   struct net_device *netdev)
> +{
> +	int src_port = -1, switch_id = -1, vbid = -1;
> +	u16 vid;
> +
> +	if (skb_vlan_tag_present(skb))
> +		/* Normal traffic path. */
> +		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> +
> +	if (vbid >= 1)
> +		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> +	else if (src_port == -1 || switch_id == -1)
> +		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);

Hi Pawel,

Smatch warns that vid may be used uninitialised here.
And it's not clear to me why that cannot be the case.

> +	else
> +		skb->dev = dsa_master_find_slave(netdev, switch_id, src_port);
> +	if (!skb->dev) {
> +		netdev_warn(netdev, "Couldn't decode source port\n");
> +		return NULL;
> +	}
> +
> +	dsa_default_offload_fwd_mark(skb);
> +
> +	if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) &&
> +	    eth_hdr(skb)->h_proto == htons(ETH_P_8021Q))
> +		__vlan_hwaccel_clear_tag(skb);
> +
> +	return skb;
> +}

...

Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Dan Carpenter]

On Sun, Jun 25, 2023 at 02:42:27PM +0200, Simon Horman wrote:
> + Dan Carpenter
> 
> On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> > This patch is simple implementation of 8021q tagging in vsc73xx driver.
> > At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> > family doesn't provide any tag support for external ethernet ports.
> > 
> > The only way is vlan-based tagging. It require constant hardware vlan
> > filtering. VSC73XX family support provider bridging but QinQ only without
> > fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> > 
> > In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> > frames.
> > 
> > Tag driver introduce most simple funcionality required for proper taging
> > support.
> > 
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> 
> ...
> 
> > +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
> > +			     int *switch_id, int *vbid, u16 *vid)
> > +{
> > +	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
> > +		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
> > +
> > +	/* Try our best with imprecise RX */
> > +	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
> > +}
> > +
> > +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
> > +				   struct net_device *netdev)
> > +{
> > +	int src_port = -1, switch_id = -1, vbid = -1;
> > +	u16 vid;
> > +
> > +	if (skb_vlan_tag_present(skb))
> > +		/* Normal traffic path. */
> > +		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
> > +
> > +	if (vbid >= 1)
> > +		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
> > +	else if (src_port == -1 || switch_id == -1)
> > +		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);
> 
> Hi Pawel,
> 
> Smatch warns that vid may be used uninitialised here.
> And it's not clear to me why that cannot be the case.
> 

The problem is only when skb_vlan_tag_present() is false.

If vsc73xx_vlan_rcv() is called then actually it's fine.  Either vbid,
src_port and switch_id will be set or vid will be initialized.  Smatch
thinks the vbid is set to 0-7, src_port is set to 0-15 and
switch_id is set to 0-7.  Smatch kind of ignores the switch_id == -1
condition because it's known to be false given that we already checked
src_port == -1.

But the concern again is the skb_vlan_tag_present() is false and then
vbid, src_port and switch_id would all be set to -1.

regards,
dan carpenter

Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Vladimir Oltean]

On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> This patch is simple implementation of 8021q tagging in vsc73xx driver.
> At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> family doesn't provide any tag support for external ethernet ports.
> 
> The only way is vlan-based tagging. It require constant hardware vlan
> filtering. VSC73XX family support provider bridging but QinQ only without
> fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> 
> In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> frames.
> 
> Tag driver introduce most simple funcionality required for proper taging
> support.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v2:
>   - change poll loop into dedicated macro
>   - fix style issues
> 
>  drivers/net/dsa/Kconfig                |   2 +-
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 532 +++++++++++++++++++++----
>  include/net/dsa.h                      |   2 +
>  net/dsa/Kconfig                        |   6 +
>  net/dsa/Makefile                       |   1 +
>  net/dsa/tag_vsc73xx_8021q.c            |  87 ++++
>  6 files changed, 544 insertions(+), 86 deletions(-)
>  create mode 100644 net/dsa/tag_vsc73xx_8021q.c
> 
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 3ed5391bb18d..4cf0166fef7b 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -125,7 +125,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
>  
>  config NET_DSA_VITESSE_VSC73XX
>  	tristate
> -	select NET_DSA_TAG_NONE
> +	select NET_DSA_TAG_VSC73XX
>  	select FIXED_PHY
>  	select VITESSE_PHY
>  	select GPIOLIB
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index f123ce2ed244..f7c38f9a81a8 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -17,6 +17,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/device.h>
> +#include <linux/iopoll.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_mdio.h>
> @@ -25,6 +26,7 @@
>  #include <linux/etherdevice.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/dsa/8021q.h>
>  #include <linux/random.h>
>  #include <net/dsa.h>
>  
> @@ -62,6 +64,8 @@
>  #define VSC73XX_CAT_DROP	0x6e
>  #define VSC73XX_CAT_PR_MISC_L2	0x6f
>  #define VSC73XX_CAT_PR_USR_PRIO	0x75
> +#define VSC73XX_CAT_VLAN_MISC	0x79
> +#define VSC73XX_CAT_PORT_VLAN	0x7a
>  #define VSC73XX_Q_MISC_CONF	0xdf
>  
>  /* MAC_CFG register bits */
> @@ -122,6 +126,17 @@
>  #define VSC73XX_ADVPORTM_IO_LOOPBACK	BIT(1)
>  #define VSC73XX_ADVPORTM_HOST_LOOPBACK	BIT(0)
>  
> +/*  TXUPDCFG transmit modify setup bits */
> +#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE	GENMASK(20, 19)
> +#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA	BIT(18)
> +#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA	BIT(17)
> +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID	GENMASK(15, 4)
> +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA	BIT(3)
> +#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA	BIT(1)
> +#define VSC73XX_TXUPDCFG_TX_INSERT_TAG	BIT(0)
> +
> +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
> +
>  /* CAT_DROP categorizer frame dropping register bits */
>  #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA	BIT(6)
>  #define VSC73XX_CAT_DROP_FWD_CTRL_ENA		BIT(4)
> @@ -135,6 +150,15 @@
>  #define VSC73XX_Q_MISC_CONF_EARLY_TX_512	(1 << 1)
>  #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE	BIT(0)
>  
> +/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
> +#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
> +#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
> +
> +/* CAT_PORT_VLAN categorizer port VLAN*/
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
> +
>  /* Frame analyzer block 2 registers */
>  #define VSC73XX_STORMLIMIT	0x02
>  #define VSC73XX_ADVLEARN	0x03
> @@ -189,7 +213,8 @@
>  #define VSC73XX_VLANACCESS_VLAN_MIRROR		BIT(29)
>  #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK	BIT(28)
>  #define VSC73XX_VLANACCESS_VLAN_PORT_MASK	GENMASK(9, 2)
> -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK	GENMASK(2, 0)
> +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT	2
> +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK	GENMASK(1, 0)
>  #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE	0
>  #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY	1
>  #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY	2
> @@ -344,6 +369,13 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
>  	{ 29, "TxQoSClass3" }, /* non-standard counter */
>  };
>  
> +enum vsc73xx_port_vlan_conf {
> +	VSC73XX_VLAN_UNAWARE,
> +	VSC73XX_VLAN_AWARE,
> +	VSC73XX_DOUBLE_VLAN_AWARE,
> +	VSC73XX_DOUBLE_VLAN_CPU_AWARE,
> +};
> +
>  int vsc73xx_is_addr_valid(u8 block, u8 subblock)
>  {
>  	switch (block) {
> @@ -558,90 +590,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
>  	 * cannot access the tag. (See "Internal frame header" section
>  	 * 3.9.1 in the manual.)
>  	 */
> -	return DSA_TAG_PROTO_NONE;
> -}
> -
> -static int vsc73xx_setup(struct dsa_switch *ds)
> -{
> -	struct vsc73xx *vsc = ds->priv;
> -	int i;
> -
> -	dev_info(vsc->dev, "set up the switch\n");
> -
> -	/* Issue RESET */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> -		      VSC73XX_GLORESET_MASTER_RESET);
> -	usleep_range(125, 200);
> -
> -	/* Initialize memory, initialize RAM bank 0..15 except 6 and 7
> -	 * This sequence appears in the
> -	 * VSC7385 SparX-G5 datasheet section 6.6.1
> -	 * VSC7395 SparX-G5e datasheet section 6.6.1
> -	 * "initialization sequence".
> -	 * No explanation is given to the 0x1010400 magic number.
> -	 */
> -	for (i = 0; i <= 15; i++) {
> -		if (i != 6 && i != 7) {
> -			vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
> -				      2,
> -				      0, 0x1010400 + i);
> -			mdelay(1);
> -		}
> -	}
> -	mdelay(30);
> -
> -	/* Clear MAC table */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> -		      VSC73XX_MACACCESS,
> -		      VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
> -
> -	/* Clear VLAN table */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> -		      VSC73XX_VLANACCESS,
> -		      VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
> -
> -	msleep(40);
> -
> -	/* Use 20KiB buffers on all ports on VSC7395
> -	 * The VSC7385 has 16KiB buffers and that is the
> -	 * default if we don't set this up explicitly.
> -	 * Port "31" is "all ports".
> -	 */
> -	if (IS_739X(vsc))
> -		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
> -			      VSC73XX_Q_MISC_CONF,
> -			      VSC73XX_Q_MISC_CONF_EXTENT_MEM);
> -
> -	/* Put all ports into reset until enabled */
> -	for (i = 0; i < 7; i++) {
> -		if (i == 5)
> -			continue;
> -		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
> -			      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
> -	}
> -
> -	/* MII delay, set both GTX and RX delay to 2 ns */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
> -		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
> -		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> -	/* IP multicast flood mask (table 144) */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> -		      0xff);
> -
> -	mdelay(50);
> -
> -	/*configure forward map to CPU <-> port only*/
> -	for (i = 0; i < vsc->ds->num_ports; i++)
> -		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
> -	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(CPU_PORT);
> -
> -	/* Release reset from the internal PHYs */
> -	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> -		      VSC73XX_GLORESET_PHY_RESET);
> -
> -	udelay(4);
> -
> -	return 0;

Try to make this code movement part of a separate patch. It's
distracting and makes it hard to see what really changed.

> +	return DSA_TAG_PROTO_VSC73XX_8021Q;
>  }
>  
>  static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
> @@ -1078,6 +1027,417 @@ static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
>  				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
>  }
>  
> +static int
> +vsc73xx_port_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
> +{
> +	u32 val;
> +	int ret, err;
> +
> +	ret = read_poll_timeout(vsc73xx_read, err,
> +				err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) ==
> +					    VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE),
> +				1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
> +				0, VSC73XX_VLANACCESS, &val);
> +	if (ret)
> +		return ret;
> +	return err;
> +}
> +
> +static int
> +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u32 val;
> +	int ret;
> +
> +	if (vid > 4095)
> +		return -EPERM;

Unnecessary defensive programming.

> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> +	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> +	if (ret)
> +		return ret;
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> +			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
> +			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
> +	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> +	if (ret)
> +		return ret;
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
> +	*portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
> +		   VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
> +	return 0;
> +}
> +
> +static int
> +vsc73xx_port_write_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 portmap)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	int ret;
> +
> +	if (vid > 4095)
> +		return -EPERM;

Unnecessary defensive programming.

> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> +	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> +	if (ret)
> +		return ret;
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> +			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
> +			    VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> +			    VSC73XX_VLANACCESS_VLAN_PORT_MASK,
> +			    VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
> +			    VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> +			    (portmap <<
> +			     VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
> +	ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
> +static int
> +vsc73xx_port_update_vlan_table(struct dsa_switch *ds, int port, u16 vid,
> +			       bool set)
> +{
> +	u8 portmap;
> +	int ret;
> +
> +	if (vid > 4095)
> +		return -EPERM;

Unnecessary defensive programming.

> +
> +	ret = vsc73xx_port_read_vlan_table_entry(ds, vid, &portmap);
> +	if (ret)
> +		return ret;
> +
> +	if (set)
> +		portmap |= BIT(port) | BIT(CPU_PORT);
> +	else
> +		portmap &= ~BIT(port);
> +
> +	if (portmap == BIT(CPU_PORT))
> +		portmap = 0;
> +
> +	ret = vsc73xx_port_write_vlan_table_entry(ds, vid, portmap);
> +
> +	return ret;

return vsc73xx_port_write_vlan_table_entry(...)

> +}
> +
> +static void
> +vsc73xx_port_set_vlan_conf(struct dsa_switch *ds, int port,
> +			   enum vsc73xx_port_vlan_conf port_vlan_conf)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	if (port_vlan_conf == VSC73XX_VLAN_UNAWARE) {
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_MAC_CFG,
> +				    VSC73XX_MAC_CFG_VLAN_AWR, 0);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_MAC_CFG,
> +				    VSC73XX_MAC_CFG_VLAN_DBLAWR, 0);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_VLAN_MISC,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_VLAN_MISC,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_DROP,
> +				    VSC73XX_CAT_DROP_TAGGED_ENA, 0);
> +	} else {
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_MAC_CFG,
> +				    VSC73XX_MAC_CFG_VLAN_AWR,
> +				    VSC73XX_MAC_CFG_VLAN_AWR);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_DROP,
> +				    VSC73XX_CAT_DROP_TAGGED_ENA, 0);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_DROP,
> +				    VSC73XX_CAT_DROP_UNTAGGED_ENA,
> +				    VSC73XX_CAT_DROP_UNTAGGED_ENA);
> +
> +		if (port_vlan_conf == VSC73XX_DOUBLE_VLAN_CPU_AWARE)
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_MAC_CFG,
> +					    VSC73XX_MAC_CFG_VLAN_DBLAWR,
> +					    VSC73XX_MAC_CFG_VLAN_DBLAWR);
> +		else
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_MAC_CFG,
> +					    VSC73XX_MAC_CFG_VLAN_DBLAWR, 0);
> +
> +		if (port_vlan_conf == VSC73XX_DOUBLE_VLAN_AWARE) {
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_CAT_VLAN_MISC,
> +					    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> +					    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_CAT_VLAN_MISC,
> +					    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> +					    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_TXUPDCFG,
> +					    VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0);
> +		} else {
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_CAT_VLAN_MISC,
> +					    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> +					    0);
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_CAT_VLAN_MISC,
> +					    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> +					    0);
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_TXUPDCFG,
> +					    VSC73XX_TXUPDCFG_TX_INSERT_TAG,
> +					    VSC73XX_TXUPDCFG_TX_INSERT_TAG);
> +		}
> +
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_TXUPDCFG,
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_TXUPDCFG,
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_PORT_VLAN,
> +				    VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
> +	}
> +}
> +

I need some time to understand what is being done here.

> +static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
> +{
> +	int i, ret;
> +
> +	if (port == CPU_PORT)
> +		vsc73xx_port_set_vlan_conf(ds, port,
> +					   VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> +	else
> +		vsc73xx_port_set_vlan_conf(ds, port,
> +					   VSC73XX_DOUBLE_VLAN_AWARE);
> +
> +	for (i = 0; i <= 4095; i++) {

i < VLAN_N_VID

> +		ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> +		if (ret)
> +			return ret;

Clearing the VLAN table absolutely does not belong in a function that
makes a port aware of double VLANs. And neither does it belong in a
function that changes VLAN awareness, but that's not part of this patch,
but 6/7. The bridge expects the VLAN table to be able to be populated
asynchronously relative to enabling/disabling it. The same for the
VLAN-aware FDB and MDB entries.

> +	}
> +	return ret;
> +}
> +
> +static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> +				     bool port_vlan)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u16 vlan_no;
> +	u32 val;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> +
> +	if (port_vlan && (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)) {
> +		vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> +			     &val);
> +		vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> +				VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> +		if (!vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) &&
> +		    vlan_no != vid) {
> +			dev_warn(vsc->dev,
> +				 "Port %d can have only one untagged vid! Now is: %d.\n",
> +				 port, vlan_no);
> +				return -EPERM;

drop indentation by 1 level

don't return -EPERM, as that indicates a permissions-related issue to
the user process. Perhaps -EOPNOTSUPP.

use the extack for the message.. we now also have NL_SET_ERR_MSG_FMT()

> +		}
> +	}
> +
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> +			    VSC73XX_CAT_DROP_UNTAGGED_ENA, 0);
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> +			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> +			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA);
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> +			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> +			    (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> +			     VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> +	return 0;
> +}
> +
> +static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> +				 bool port_vlan)
> +{
> +	struct dsa_port *dsa_port = dsa_to_port(ds, port);
> +	struct vsc73xx *vsc = ds->priv;
> +	u16 vlan_no;
> +	u32 val;
> +
> +	if (!dsa_port)
> +		return -EINVAL;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> +	vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> +
> +	if (port_vlan && vlan_no && !vid_is_dsa_8021q(vlan_no) &&
> +	    !vid_is_dsa_8021q(vid) && vlan_no != vid) {
> +		dev_warn(vsc->dev,
> +			 "Port %d can have only one pvid! Now is: %d.\n",
> +			 port, vlan_no);
> +		return -EPERM;
> +	}
> +
> +	if (dsa_port_is_vlan_filtering(dsa_port))
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_VLAN_MISC,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> +				    0);
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_VLAN_MISC,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> +				    VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
> +
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> +			    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> +			    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> +
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> +			    VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> +			    vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);

Why would changing the PVID (what this function does) alter VLAN_TCI_IGNORE_ENA
and VLAN_KEEP_TAG_ENA?

> +	return 0;
> +}
> +
> +static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
> +				      u16 flags)
> +{
> +	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = flags & BRIDGE_VLAN_INFO_PVID;
> +	int ret;
> +
> +	if (untagged) {
> +		ret = vsc73xx_vlan_set_untagged(ds, port, vid, false);
> +		if (ret)
> +			return ret;
> +	}
> +	if (pvid) {
> +		ret = vsc73xx_vlan_set_pvid(ds, port, vid, false);
> +		if (ret)
> +			return ret;
> +	}
> +	ret = vsc73xx_port_update_vlan_table(ds, port, vid, 1);

return vsc73xx_port_update_vlan_table(...)

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
> +{
> +	return vsc73xx_port_update_vlan_table(ds, port, vid, 0);
> +}
> +
> +static int vsc73xx_setup(struct dsa_switch *ds)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	int i, ret;
> +
> +	dev_info(vsc->dev, "set up the switch\n");
> +
> +	ds->vlan_filtering_is_global = false;

No reason to set this, it's false by default.

> +	ds->configure_vlan_while_not_filtering = false;

This is a legacy flag that should be set to true (default value) for all
drivers and then eliminated.

To check whether your implementation is doing the right thing, you must
remove this line, and make sure that after this configuration:

ip link add br0 type bridge vlan_filtering 0
ip link set swp0 master br0
bridge vlan add dev swp0 vid 100 pvid untagged

the VLAN-unaware forwarding of the switch is not affected. This means
that the PVID of the port that is committed to hardware (and thus, the
allowed destination port mask) must remain what the driver set it to,
while the bridge vlan_filtering option is not enabled. The hardware PVID
must only follow the bridge port PVID while in vlan_filtering=true mode.

> +
> +	/* Issue RESET */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> +		      VSC73XX_GLORESET_MASTER_RESET);
> +	usleep_range(125, 200);
> +
> +	/* Initialize memory, initialize RAM bank 0..15 except 6 and 7
> +	 * This sequence appears in the
> +	 * VSC7385 SparX-G5 datasheet section 6.6.1
> +	 * VSC7395 SparX-G5e datasheet section 6.6.1
> +	 * "initialization sequence".
> +	 * No explanation is given to the 0x1010400 magic number.
> +	 */
> +	for (i = 0; i <= 15; i++) {
> +		if (i != 6 && i != 7) {
> +			vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
> +				      2,
> +				      0, 0x1010400 + i);
> +			mdelay(1);
> +		}
> +	}
> +	mdelay(30);
> +
> +	/* Clear MAC table */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +		      VSC73XX_MACACCESS,
> +		      VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
> +
> +	/* Clear VLAN table */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +		      VSC73XX_VLANACCESS,
> +		      VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
> +
> +	msleep(40);
> +
> +	/* Use 20KiB buffers on all ports on VSC7395
> +	 * The VSC7385 has 16KiB buffers and that is the
> +	 * default if we don't set this up explicitly.
> +	 * Port "31" is "all ports".
> +	 */
> +	if (IS_739X(vsc))
> +		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
> +			      VSC73XX_Q_MISC_CONF,
> +			      VSC73XX_Q_MISC_CONF_EXTENT_MEM);
> +
> +	/* Put all ports into reset until enabled */
> +	for (i = 0; i < 7; i++) {
> +		if (i == 5)
> +			continue;
> +		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
> +			      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
> +	}
> +
> +	/* MII delay, set both GTX and RX delay to 2 ns */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
> +		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
> +		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> +	/* Ingess VLAN reception mask (table 145) */

s/ingess/ingress/

> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK,
> +		      0x5f);
> +	/* IP multicast flood mask (table 144) */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> +		      0xff);
> +
> +	mdelay(50);
> +
> +	for (i = 0; i < vsc->ds->num_ports; i++) {
> +		if (i == 5)
> +			continue;

dsa_switch_for_each_available_port()

> +		ret = vsc73xx_port_set_double_vlan_aware(ds, i);
> +		if (ret)
> +			return ret;

If ports are made VLAN-aware by default (including in standalone mode),
shouldn't this be reflected in ds->needs_standalone_vlan_filtering?

> +	}
> +
> +	rtnl_lock();
> +	ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
> +	rtnl_unlock();
> +	if (ret)
> +		return ret;
> +
> +	/*configure forward map to CPU <-> port only*/

style: space after /* and before */ (also present in a few other places)

> +	for (i = 0; i < vsc->ds->num_ports; i++)
> +		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK &
> +				      BIT(CPU_PORT);
> +	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> +				     ~BIT(CPU_PORT);
> +
> +	/* Release reset from the internal PHYs */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> +		      VSC73XX_GLORESET_PHY_RESET);
> +
> +	udelay(4);
> +
> +	return 0;
> +}
> +
>  static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_tag_protocol = vsc73xx_get_tag_protocol,
>  	.setup = vsc73xx_setup,
> @@ -1095,6 +1455,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
>  	.port_stp_state_set = vsc73xx_port_stp_state_set,
> +	.tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
> +	.tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
>  };
>  
>  static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 75022cf771cf..2440df7ea6c9 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -56,6 +56,7 @@ struct phylink_link_state;
>  #define DSA_TAG_PROTO_RTL8_4T_VALUE		25
>  #define DSA_TAG_PROTO_RZN1_A5PSW_VALUE		26
>  #define DSA_TAG_PROTO_LAN937X_VALUE		27
> +#define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE	28
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -86,6 +87,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_RTL8_4T		= DSA_TAG_PROTO_RTL8_4T_VALUE,
>  	DSA_TAG_PROTO_RZN1_A5PSW	= DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
>  	DSA_TAG_PROTO_LAN937X		= DSA_TAG_PROTO_LAN937X_VALUE,
> +	DSA_TAG_PROTO_VSC73XX_8021Q	= DSA_TAG_PROTO_VSC73XX_8021Q_VALUE,
>  };
>  
>  struct dsa_switch;
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 8e698bea99a3..e59360071c67 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -166,6 +166,12 @@ config NET_DSA_TAG_TRAILER
>  	  Say Y or M if you want to enable support for tagging frames at
>  	  with a trailed. e.g. Marvell 88E6060.
>  
> +config NET_DSA_TAG_VSC73XX_8021Q
> +	tristate "Tag driver for Microchip/Vitesse VSC73xx family of switches, using VLAN"
> +	help
> +	  Say Y or M if you want to enable support for tagging frames with a
> +	  custom VLAN-based header.
> +

Separate patches for the tagger and for the driver infrastructure please.
It's hard to review.

Also, there's some very strange splitting between this patch and 6/7
("net: dsa: vsc73xx: Add vlan filtering"). I would have expected that to
contain the basic infrastructure (thus appearing first), then the
driver-side tag_8021q infra, then the tagger. It's impossible for me to
comment on things that have to do with VLAN infrastructure when it's
split between 2 patches.

Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Paweł Dembicki]

niedz., 25 cze 2023 o 14:47 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> > This patch is simple implementation of 8021q tagging in vsc73xx driver.
> > At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> > family doesn't provide any tag support for external ethernet ports.
> >
> > The only way is vlan-based tagging. It require constant hardware vlan
> > filtering. VSC73XX family support provider bridging but QinQ only without
> > fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> >
> > In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> > frames.
> >
> > Tag driver introduce most simple funcionality required for proper taging
> > support.
> >
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > v2:
> >   - change poll loop into dedicated macro
> >   - fix style issues
> >
> >  drivers/net/dsa/Kconfig                |   2 +-
> >  drivers/net/dsa/vitesse-vsc73xx-core.c | 532 +++++++++++++++++++++----
> >  include/net/dsa.h                      |   2 +
> >  net/dsa/Kconfig                        |   6 +
> >  net/dsa/Makefile                       |   1 +
> >  net/dsa/tag_vsc73xx_8021q.c            |  87 ++++
> >  6 files changed, 544 insertions(+), 86 deletions(-)
> >  create mode 100644 net/dsa/tag_vsc73xx_8021q.c
> >
> > diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> > index 3ed5391bb18d..4cf0166fef7b 100644
> > --- a/drivers/net/dsa/Kconfig
> > +++ b/drivers/net/dsa/Kconfig
> > @@ -125,7 +125,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
> >
> >  config NET_DSA_VITESSE_VSC73XX
> >       tristate
> > -     select NET_DSA_TAG_NONE
> > +     select NET_DSA_TAG_VSC73XX
> >       select FIXED_PHY
> >       select VITESSE_PHY
> >       select GPIOLIB
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index f123ce2ed244..f7c38f9a81a8 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/device.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_mdio.h>
> > @@ -25,6 +26,7 @@
> >  #include <linux/etherdevice.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/gpio/driver.h>
> > +#include <linux/dsa/8021q.h>
> >  #include <linux/random.h>
> >  #include <net/dsa.h>
> >
> > @@ -62,6 +64,8 @@
> >  #define VSC73XX_CAT_DROP     0x6e
> >  #define VSC73XX_CAT_PR_MISC_L2       0x6f
> >  #define VSC73XX_CAT_PR_USR_PRIO      0x75
> > +#define VSC73XX_CAT_VLAN_MISC        0x79
> > +#define VSC73XX_CAT_PORT_VLAN        0x7a
> >  #define VSC73XX_Q_MISC_CONF  0xdf
> >
> >  /* MAC_CFG register bits */
> > @@ -122,6 +126,17 @@
> >  #define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1)
> >  #define VSC73XX_ADVPORTM_HOST_LOOPBACK       BIT(0)
> >
> > +/*  TXUPDCFG transmit modify setup bits */
> > +#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE      GENMASK(20, 19)
> > +#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA       BIT(18)
> > +#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA       BIT(17)
> > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID     GENMASK(15, 4)
> > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3)
> > +#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA       BIT(1)
> > +#define VSC73XX_TXUPDCFG_TX_INSERT_TAG       BIT(0)
> > +
> > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4
> > +
> >  /* CAT_DROP categorizer frame dropping register bits */
> >  #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA    BIT(6)
> >  #define VSC73XX_CAT_DROP_FWD_CTRL_ENA                BIT(4)
> > @@ -135,6 +150,15 @@
> >  #define VSC73XX_Q_MISC_CONF_EARLY_TX_512     (1 << 1)
> >  #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE   BIT(0)
> >
> > +/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/
> > +#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8)
> > +#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7)
> > +
> > +/* CAT_PORT_VLAN categorizer port VLAN*/
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15)
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> > +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
> > +
> >  /* Frame analyzer block 2 registers */
> >  #define VSC73XX_STORMLIMIT   0x02
> >  #define VSC73XX_ADVLEARN     0x03
> > @@ -189,7 +213,8 @@
> >  #define VSC73XX_VLANACCESS_VLAN_MIRROR               BIT(29)
> >  #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK    BIT(28)
> >  #define VSC73XX_VLANACCESS_VLAN_PORT_MASK    GENMASK(9, 2)
> > -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0)
> > +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT      2
> > +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0)
> >  #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0
> >  #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY   1
> >  #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY  2
> > @@ -344,6 +369,13 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
> >       { 29, "TxQoSClass3" }, /* non-standard counter */
> >  };
> >
> > +enum vsc73xx_port_vlan_conf {
> > +     VSC73XX_VLAN_UNAWARE,
> > +     VSC73XX_VLAN_AWARE,
> > +     VSC73XX_DOUBLE_VLAN_AWARE,
> > +     VSC73XX_DOUBLE_VLAN_CPU_AWARE,
> > +};
> > +
> >  int vsc73xx_is_addr_valid(u8 block, u8 subblock)
> >  {
> >       switch (block) {
> > @@ -558,90 +590,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
> >        * cannot access the tag. (See "Internal frame header" section
> >        * 3.9.1 in the manual.)
> >        */
> > -     return DSA_TAG_PROTO_NONE;
> > -}
> > -
> > -static int vsc73xx_setup(struct dsa_switch *ds)
> > -{
> > -     struct vsc73xx *vsc = ds->priv;
> > -     int i;
> > -
> > -     dev_info(vsc->dev, "set up the switch\n");
> > -
> > -     /* Issue RESET */
> > -     vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> > -                   VSC73XX_GLORESET_MASTER_RESET);
> > -     usleep_range(125, 200);
> > -
> > -     /* Initialize memory, initialize RAM bank 0..15 except 6 and 7
> > -      * This sequence appears in the
> > -      * VSC7385 SparX-G5 datasheet section 6.6.1
> > -      * VSC7395 SparX-G5e datasheet section 6.6.1
> > -      * "initialization sequence".
> > -      * No explanation is given to the 0x1010400 magic number.
> > -      */
> > -     for (i = 0; i <= 15; i++) {
> > -             if (i != 6 && i != 7) {
> > -                     vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
> > -                                   2,
> > -                                   0, 0x1010400 + i);
> > -                     mdelay(1);
> > -             }
> > -     }
> > -     mdelay(30);
> > -
> > -     /* Clear MAC table */
> > -     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > -                   VSC73XX_MACACCESS,
> > -                   VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
> > -
> > -     /* Clear VLAN table */
> > -     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > -                   VSC73XX_VLANACCESS,
> > -                   VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
> > -
> > -     msleep(40);
> > -
> > -     /* Use 20KiB buffers on all ports on VSC7395
> > -      * The VSC7385 has 16KiB buffers and that is the
> > -      * default if we don't set this up explicitly.
> > -      * Port "31" is "all ports".
> > -      */
> > -     if (IS_739X(vsc))
> > -             vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
> > -                           VSC73XX_Q_MISC_CONF,
> > -                           VSC73XX_Q_MISC_CONF_EXTENT_MEM);
> > -
> > -     /* Put all ports into reset until enabled */
> > -     for (i = 0; i < 7; i++) {
> > -             if (i == 5)
> > -                     continue;
> > -             vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
> > -                           VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
> > -     }
> > -
> > -     /* MII delay, set both GTX and RX delay to 2 ns */
> > -     vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
> > -                   VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
> > -                   VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> > -     /* IP multicast flood mask (table 144) */
> > -     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> > -                   0xff);
> > -
> > -     mdelay(50);
> > -
> > -     /*configure forward map to CPU <-> port only*/
> > -     for (i = 0; i < vsc->ds->num_ports; i++)
> > -             vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
> > -     vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(CPU_PORT);
> > -
> > -     /* Release reset from the internal PHYs */
> > -     vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> > -                   VSC73XX_GLORESET_PHY_RESET);
> > -
> > -     udelay(4);
> > -
> > -     return 0;
>
> Try to make this code movement part of a separate patch. It's
> distracting and makes it hard to see what really changed.
>
> > +     return DSA_TAG_PROTO_VSC73XX_8021Q;
> >  }
> >
> >  static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
> > @@ -1078,6 +1027,417 @@ static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> >                                   VSC73XX_SRCMASKS_PORTS_MASK, 0);
> >  }
> >
> > +static int
> > +vsc73xx_port_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
> > +{
> > +     u32 val;
> > +     int ret, err;
> > +
> > +     ret = read_poll_timeout(vsc73xx_read, err,
> > +                             err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) ==
> > +                                         VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE),
> > +                             1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER,
> > +                             0, VSC73XX_VLANACCESS, &val);
> > +     if (ret)
> > +             return ret;
> > +     return err;
> > +}
> > +
> > +static int
> > +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u32 val;
> > +     int ret;
> > +
> > +     if (vid > 4095)
> > +             return -EPERM;
>
> Unnecessary defensive programming.
>
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> > +     ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> > +     if (ret)
> > +             return ret;
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> > +                         VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK,
> > +                         VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY);
> > +     ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> > +     if (ret)
> > +             return ret;
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val);
> > +     *portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >>
> > +                VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT;
> > +     return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_port_write_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 portmap)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     int ret;
> > +
> > +     if (vid > 4095)
> > +             return -EPERM;
>
> Unnecessary defensive programming.
>
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> > +     ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> > +     if (ret)
> > +             return ret;
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS,
> > +                         VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK |
> > +                         VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> > +                         VSC73XX_VLANACCESS_VLAN_PORT_MASK,
> > +                         VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY |
> > +                         VSC73XX_VLANACCESS_VLAN_SRC_CHECK |
> > +                         (portmap <<
> > +                          VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT));
> > +     ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc);
> > +     if (ret)
> > +             return ret;
> > +     return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_port_update_vlan_table(struct dsa_switch *ds, int port, u16 vid,
> > +                            bool set)
> > +{
> > +     u8 portmap;
> > +     int ret;
> > +
> > +     if (vid > 4095)
> > +             return -EPERM;
>
> Unnecessary defensive programming.
>
> > +
> > +     ret = vsc73xx_port_read_vlan_table_entry(ds, vid, &portmap);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (set)
> > +             portmap |= BIT(port) | BIT(CPU_PORT);
> > +     else
> > +             portmap &= ~BIT(port);
> > +
> > +     if (portmap == BIT(CPU_PORT))
> > +             portmap = 0;
> > +
> > +     ret = vsc73xx_port_write_vlan_table_entry(ds, vid, portmap);
> > +
> > +     return ret;
>
> return vsc73xx_port_write_vlan_table_entry(...)
>
> > +}
> > +
> > +static void
> > +vsc73xx_port_set_vlan_conf(struct dsa_switch *ds, int port,
> > +                        enum vsc73xx_port_vlan_conf port_vlan_conf)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +
> > +     if (port_vlan_conf == VSC73XX_VLAN_UNAWARE) {
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_MAC_CFG,
> > +                                 VSC73XX_MAC_CFG_VLAN_AWR, 0);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_MAC_CFG,
> > +                                 VSC73XX_MAC_CFG_VLAN_DBLAWR, 0);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_VLAN_MISC,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_VLAN_MISC,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_DROP,
> > +                                 VSC73XX_CAT_DROP_TAGGED_ENA, 0);
> > +     } else {
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_MAC_CFG,
> > +                                 VSC73XX_MAC_CFG_VLAN_AWR,
> > +                                 VSC73XX_MAC_CFG_VLAN_AWR);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_DROP,
> > +                                 VSC73XX_CAT_DROP_TAGGED_ENA, 0);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_DROP,
> > +                                 VSC73XX_CAT_DROP_UNTAGGED_ENA,
> > +                                 VSC73XX_CAT_DROP_UNTAGGED_ENA);
> > +
> > +             if (port_vlan_conf == VSC73XX_DOUBLE_VLAN_CPU_AWARE)
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_MAC_CFG,
> > +                                         VSC73XX_MAC_CFG_VLAN_DBLAWR,
> > +                                         VSC73XX_MAC_CFG_VLAN_DBLAWR);
> > +             else
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_MAC_CFG,
> > +                                         VSC73XX_MAC_CFG_VLAN_DBLAWR, 0);
> > +
> > +             if (port_vlan_conf == VSC73XX_DOUBLE_VLAN_AWARE) {
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_CAT_VLAN_MISC,
> > +                                         VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > +                                         VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_CAT_VLAN_MISC,
> > +                                         VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> > +                                         VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_TXUPDCFG,
> > +                                         VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0);
> > +             } else {
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_CAT_VLAN_MISC,
> > +                                         VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > +                                         0);
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_CAT_VLAN_MISC,
> > +                                         VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> > +                                         0);
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_TXUPDCFG,
> > +                                         VSC73XX_TXUPDCFG_TX_INSERT_TAG,
> > +                                         VSC73XX_TXUPDCFG_TX_INSERT_TAG);
> > +             }
> > +
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_TXUPDCFG,
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_TXUPDCFG,
> > +                                 VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_PORT_VLAN,
> > +                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
> > +     }
> > +}
> > +
>
> I need some time to understand what is being done here.
>
> > +static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
> > +{
> > +     int i, ret;
> > +
> > +     if (port == CPU_PORT)
> > +             vsc73xx_port_set_vlan_conf(ds, port,
> > +                                        VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> > +     else
> > +             vsc73xx_port_set_vlan_conf(ds, port,
> > +                                        VSC73XX_DOUBLE_VLAN_AWARE);
> > +
> > +     for (i = 0; i <= 4095; i++) {
>
> i < VLAN_N_VID
>
> > +             ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > +             if (ret)
> > +                     return ret;
>
> Clearing the VLAN table absolutely does not belong in a function that
> makes a port aware of double VLANs. And neither does it belong in a
> function that changes VLAN awareness, but that's not part of this patch,
> but 6/7. The bridge expects the VLAN table to be able to be populated
> asynchronously relative to enabling/disabling it. The same for the
> VLAN-aware FDB and MDB entries.
>
> > +     }
> > +     return ret;
> > +}
> > +
> > +static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> > +                                  bool port_vlan)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u16 vlan_no;
> > +     u32 val;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> > +
> > +     if (port_vlan && (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)) {
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > +                          &val);
> > +             vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> > +                             VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> > +             if (!vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) &&
> > +                 vlan_no != vid) {
> > +                     dev_warn(vsc->dev,
> > +                              "Port %d can have only one untagged vid! Now is: %d.\n",
> > +                              port, vlan_no);
> > +                             return -EPERM;
>
> drop indentation by 1 level
>
> don't return -EPERM, as that indicates a permissions-related issue to
> the user process. Perhaps -EOPNOTSUPP.
>
> use the extack for the message.. we now also have NL_SET_ERR_MSG_FMT()
>
> > +             }
> > +     }
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > +                         VSC73XX_CAT_DROP_UNTAGGED_ENA, 0);
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > +                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> > +                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA);
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > +                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> > +                         (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> > +                          VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> > +     return 0;
> > +}
> > +
> > +static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> > +                              bool port_vlan)
> > +{
> > +     struct dsa_port *dsa_port = dsa_to_port(ds, port);
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u16 vlan_no;
> > +     u32 val;
> > +
> > +     if (!dsa_port)
> > +             return -EINVAL;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> > +     vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> > +
> > +     if (port_vlan && vlan_no && !vid_is_dsa_8021q(vlan_no) &&
> > +         !vid_is_dsa_8021q(vid) && vlan_no != vid) {
> > +             dev_warn(vsc->dev,
> > +                      "Port %d can have only one pvid! Now is: %d.\n",
> > +                      port, vlan_no);
> > +             return -EPERM;
> > +     }
> > +
> > +     if (dsa_port_is_vlan_filtering(dsa_port))
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_VLAN_MISC,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > +                                 0);
> > +     else
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_VLAN_MISC,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA,
> > +                                 VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA);
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
> > +                         VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA,
> > +                         VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA);
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN,
> > +                         VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> > +                         vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
>
> Why would changing the PVID (what this function does) alter VLAN_TCI_IGNORE_ENA
> and VLAN_KEEP_TAG_ENA?
>
> > +     return 0;
> > +}
> > +
> > +static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
> > +                                   u16 flags)
> > +{
> > +     bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     bool pvid = flags & BRIDGE_VLAN_INFO_PVID;
> > +     int ret;
> > +
> > +     if (untagged) {
> > +             ret = vsc73xx_vlan_set_untagged(ds, port, vid, false);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     if (pvid) {
> > +             ret = vsc73xx_vlan_set_pvid(ds, port, vid, false);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     ret = vsc73xx_port_update_vlan_table(ds, port, vid, 1);
>
> return vsc73xx_port_update_vlan_table(...)
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
> > +{
> > +     return vsc73xx_port_update_vlan_table(ds, port, vid, 0);
> > +}
> > +
> > +static int vsc73xx_setup(struct dsa_switch *ds)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     int i, ret;
> > +
> > +     dev_info(vsc->dev, "set up the switch\n");
> > +
> > +     ds->vlan_filtering_is_global = false;
>
> No reason to set this, it's false by default.
>
> > +     ds->configure_vlan_while_not_filtering = false;
>
> This is a legacy flag that should be set to true (default value) for all
> drivers and then eliminated.
>
> To check whether your implementation is doing the right thing, you must
> remove this line, and make sure that after this configuration:
>
> ip link add br0 type bridge vlan_filtering 0
> ip link set swp0 master br0
> bridge vlan add dev swp0 vid 100 pvid untagged
>
> the VLAN-unaware forwarding of the switch is not affected. This means
> that the PVID of the port that is committed to hardware (and thus, the
> allowed destination port mask) must remain what the driver set it to,
> while the bridge vlan_filtering option is not enabled. The hardware PVID
> must only follow the bridge port PVID while in vlan_filtering=true mode.
>
> > +
> > +     /* Issue RESET */
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> > +                   VSC73XX_GLORESET_MASTER_RESET);
> > +     usleep_range(125, 200);
> > +
> > +     /* Initialize memory, initialize RAM bank 0..15 except 6 and 7
> > +      * This sequence appears in the
> > +      * VSC7385 SparX-G5 datasheet section 6.6.1
> > +      * VSC7395 SparX-G5e datasheet section 6.6.1
> > +      * "initialization sequence".
> > +      * No explanation is given to the 0x1010400 magic number.
> > +      */
> > +     for (i = 0; i <= 15; i++) {
> > +             if (i != 6 && i != 7) {
> > +                     vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
> > +                                   2,
> > +                                   0, 0x1010400 + i);
> > +                     mdelay(1);
> > +             }
> > +     }
> > +     mdelay(30);
> > +
> > +     /* Clear MAC table */
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > +                   VSC73XX_MACACCESS,
> > +                   VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
> > +
> > +     /* Clear VLAN table */
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > +                   VSC73XX_VLANACCESS,
> > +                   VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
> > +
> > +     msleep(40);
> > +
> > +     /* Use 20KiB buffers on all ports on VSC7395
> > +      * The VSC7385 has 16KiB buffers and that is the
> > +      * default if we don't set this up explicitly.
> > +      * Port "31" is "all ports".
> > +      */
> > +     if (IS_739X(vsc))
> > +             vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
> > +                           VSC73XX_Q_MISC_CONF,
> > +                           VSC73XX_Q_MISC_CONF_EXTENT_MEM);
> > +
> > +     /* Put all ports into reset until enabled */
> > +     for (i = 0; i < 7; i++) {
> > +             if (i == 5)
> > +                     continue;
> > +             vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
> > +                           VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
> > +     }
> > +
> > +     /* MII delay, set both GTX and RX delay to 2 ns */
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
> > +                   VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
> > +                   VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
> > +     /* Ingess VLAN reception mask (table 145) */
>
> s/ingess/ingress/
>
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK,
> > +                   0x5f);
> > +     /* IP multicast flood mask (table 144) */
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
> > +                   0xff);
> > +
> > +     mdelay(50);
> > +
> > +     for (i = 0; i < vsc->ds->num_ports; i++) {
> > +             if (i == 5)
> > +                     continue;
>
> dsa_switch_for_each_available_port()
>
> > +             ret = vsc73xx_port_set_double_vlan_aware(ds, i);
> > +             if (ret)
> > +                     return ret;
>
> If ports are made VLAN-aware by default (including in standalone mode),
> shouldn't this be reflected in ds->needs_standalone_vlan_filtering?
>

Not for QinQ. Hardware filtering without vlan_filtering=1 is used only
for separation of outer vlans.

> > +     }
> > +
> > +     rtnl_lock();
> > +     ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
> > +     rtnl_unlock();
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*configure forward map to CPU <-> port only*/
>
> style: space after /* and before */ (also present in a few other places)
>
> > +     for (i = 0; i < vsc->ds->num_ports; i++)
> > +             vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK &
> > +                                   BIT(CPU_PORT);
> > +     vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> > +                                  ~BIT(CPU_PORT);
> > +
> > +     /* Release reset from the internal PHYs */
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
> > +                   VSC73XX_GLORESET_PHY_RESET);
> > +
> > +     udelay(4);
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .get_tag_protocol = vsc73xx_get_tag_protocol,
> >       .setup = vsc73xx_setup,
> > @@ -1095,6 +1455,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .port_change_mtu = vsc73xx_change_mtu,
> >       .port_max_mtu = vsc73xx_get_max_mtu,
> >       .port_stp_state_set = vsc73xx_port_stp_state_set,
> > +     .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
> > +     .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
> >  };
> >
> >  static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 75022cf771cf..2440df7ea6c9 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -56,6 +56,7 @@ struct phylink_link_state;
> >  #define DSA_TAG_PROTO_RTL8_4T_VALUE          25
> >  #define DSA_TAG_PROTO_RZN1_A5PSW_VALUE               26
> >  #define DSA_TAG_PROTO_LAN937X_VALUE          27
> > +#define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE    28
> >
> >  enum dsa_tag_protocol {
> >       DSA_TAG_PROTO_NONE              = DSA_TAG_PROTO_NONE_VALUE,
> > @@ -86,6 +87,7 @@ enum dsa_tag_protocol {
> >       DSA_TAG_PROTO_RTL8_4T           = DSA_TAG_PROTO_RTL8_4T_VALUE,
> >       DSA_TAG_PROTO_RZN1_A5PSW        = DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
> >       DSA_TAG_PROTO_LAN937X           = DSA_TAG_PROTO_LAN937X_VALUE,
> > +     DSA_TAG_PROTO_VSC73XX_8021Q     = DSA_TAG_PROTO_VSC73XX_8021Q_VALUE,
> >  };
> >
> >  struct dsa_switch;
> > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> > index 8e698bea99a3..e59360071c67 100644
> > --- a/net/dsa/Kconfig
> > +++ b/net/dsa/Kconfig
> > @@ -166,6 +166,12 @@ config NET_DSA_TAG_TRAILER
> >         Say Y or M if you want to enable support for tagging frames at
> >         with a trailed. e.g. Marvell 88E6060.
> >
> > +config NET_DSA_TAG_VSC73XX_8021Q
> > +     tristate "Tag driver for Microchip/Vitesse VSC73xx family of switches, using VLAN"
> > +     help
> > +       Say Y or M if you want to enable support for tagging frames with a
> > +       custom VLAN-based header.
> > +
>
> Separate patches for the tagger and for the driver infrastructure please.
> It's hard to review.
>
> Also, there's some very strange splitting between this patch and 6/7
> ("net: dsa: vsc73xx: Add vlan filtering"). I would have expected that to
> contain the basic infrastructure (thus appearing first), then the
> driver-side tag_8021q infra, then the tagger. It's impossible for me to
> comment on things that have to do with VLAN infrastructure when it's
> split between 2 patches.

I will rework it: move vsc73xx_setup movement to a different commit,
the same with tagger code and squash all vlan stuff. In this version,
I decided to make tag 8021q first, because it's impossible/very hard
to use and test the driver with TAG_NONE at this moment.

--
Pawel Dembicki

Re: [PATCH net-next v2 4/7] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Vladimir Oltean]

On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:
> This patch is simple implementation of 8021q tagging in vsc73xx driver.
> At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
> family doesn't provide any tag support for external ethernet ports.
> 
> The only way is vlan-based tagging. It require constant hardware vlan
> filtering. VSC73XX family support provider bridging but QinQ only without
> fully implemented 802.1AD. It allow only doubled 0x8100 TPID.
> 
> In simple port mode QinQ is enabled to preserve forwarding vlan tagged
> frames.
> 
> Tag driver introduce most simple funcionality required for proper taging
> support.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 3ed5391bb18d..4cf0166fef7b 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -125,7 +125,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
>  
>  config NET_DSA_VITESSE_VSC73XX
>  	tristate
> -	select NET_DSA_TAG_NONE
> +	select NET_DSA_TAG_VSC73XX

typo: "select NET_DSA_TAG_VSC73XX_8021Q". This does not do anything, and
Kconfig still asks for a prompt for the config option that does exist.

>  	select FIXED_PHY
>  	select VITESSE_PHY
>  	select GPIOLIB

[PATCH net-next v2 5/7] net: dsa: vsc73xx: Add bridge support

[Pawel Dembicki]

This patch adds bridge support for vsc73xx driver.
It introduce two functions for port_bridge_join and
vsc73xx_port_bridge_leave handling.

Those functions implement forwarding adjust and use
dsa_tag_8021q_bridge_* api for adjust VLAN configuration.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - no changes done

 drivers/net/dsa/vitesse-vsc73xx-core.c | 69 ++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index f7c38f9a81a8..457eb7fddf4c 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1304,6 +1304,72 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
 	return 0;
 }
 
+static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
+{
+	int i;
+
+	for (i = 0; i < vsc->ds->num_ports; i++) {
+		u32 val;
+
+		vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+			     VSC73XX_SRCMASKS + i, &val);
+		/* update only if port is in forwarding state*/
+		if (val & VSC73XX_SRCMASKS_PORTS_MASK)
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+					    VSC73XX_SRCMASKS + i,
+					    VSC73XX_SRCMASKS_PORTS_MASK,
+					    vsc->forward_map[i]);
+	}
+}
+
+static int vsc73xx_port_bridge_join(struct dsa_switch *ds, int port,
+				    struct dsa_bridge bridge,
+				    bool *tx_fwd_offload,
+				    struct netlink_ext_ack *extack)
+{
+	struct vsc73xx *vsc = ds->priv;
+	int i;
+
+	*tx_fwd_offload = true;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		/* Add this port to the forwarding matrix of the
+		 * other ports in the same bridge, and viceversa.
+		 */
+		if (!dsa_is_user_port(ds, i))
+			continue;
+
+		if (i == port)
+			continue;
+
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			continue;
+
+		vsc->forward_map[port] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(i);
+		vsc->forward_map[i] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(port);
+	}
+	vsc73xx_update_forwarding_map(vsc);
+
+	return dsa_tag_8021q_bridge_join(ds, port, bridge);
+}
+
+static void vsc73xx_port_bridge_leave(struct dsa_switch *ds, int port,
+				      struct dsa_bridge bridge)
+{
+	struct vsc73xx *vsc = ds->priv;
+	int i;
+	/*configure forward map to CPU <-> port only*/
+	for (i = 0; i < vsc->ds->num_ports; i++) {
+		if (i == CPU_PORT)
+			continue;
+		vsc->forward_map[i] &= VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(port);
+	}
+	vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
+
+	vsc73xx_update_forwarding_map(vsc);
+	dsa_tag_8021q_bridge_leave(ds, port, bridge);
+}
+
 static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
 				      u16 flags)
 {
@@ -1342,6 +1408,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 
 	ds->vlan_filtering_is_global = false;
 	ds->configure_vlan_while_not_filtering = false;
+	ds->max_num_bridges = 7;
 
 	/* Issue RESET */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
@@ -1452,6 +1519,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.get_sset_count = vsc73xx_get_sset_count,
 	.port_enable = vsc73xx_port_enable,
 	.port_disable = vsc73xx_port_disable,
+	.port_bridge_join = vsc73xx_port_bridge_join,
+	.port_bridge_leave = vsc73xx_port_bridge_leave,
 	.port_change_mtu = vsc73xx_change_mtu,
 	.port_max_mtu = vsc73xx_get_max_mtu,
 	.port_stp_state_set = vsc73xx_port_stp_state_set,
-- 
2.34.1

[PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering

[Pawel Dembicki]

This patch implement vlan filtering for vsc73xx driver.

After vlan filtering start, switch is reconfigured from QinQ to simple
vlan aware mode. It's required, because VSC73XX chips haven't support
for inner vlan tag filter.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - no changes done

 drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 457eb7fddf4c..c946464489ab 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
 	return ret;
 }
 
+static int
+vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
+			    bool vlan_filtering, struct netlink_ext_ack *extack)
+{
+	int ret, i;
+
+	if (vlan_filtering) {
+		vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
+	} else {
+		if (port == CPU_PORT)
+			vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
+		else
+			vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
+	}
+
+	for (i = 0; i <= 3072; i++) {
+		ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
 static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
 				     bool port_vlan)
 {
@@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
 	return 0;
 }
 
+static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
+				 const struct switchdev_obj_port_vlan *vlan,
+				 struct netlink_ext_ack *extack)
+{
+	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	int ret;
+
+	/* Be sure to deny alterations to the configuration done by tag_8021q.
+	 */
+	if (vid_is_dsa_8021q(vlan->vid)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Range 3072-4095 reserved for dsa_8021q operation");
+		return -EBUSY;
+	}
+
+	if (untagged && port != CPU_PORT) {
+		ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
+		if (ret)
+			return ret;
+	}
+	if (pvid && port != CPU_PORT) {
+		ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
+		if (ret)
+			return ret;
+	}
+
+	ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
+
+	return ret;
+}
+
+static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
+				 const struct switchdev_obj_port_vlan *vlan)
+{
+	struct vsc73xx *vsc = ds->priv;
+	u16 vlan_no;
+	int ret;
+	u32 val;
+
+	ret =
+	    vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
+	if (ret)
+		return ret;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+
+	if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
+		vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
+			     VSC73XX_TXUPDCFG, &val);
+		vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+			  VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+		if (vlan_no == vlan->vid) {
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_TXUPDCFG,
+					    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
+					    0);
+			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+					    VSC73XX_TXUPDCFG,
+					    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
+		}
+	}
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+	vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+	if (vlan_no && vlan_no == vlan->vid) {
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+				    VSC73XX_CAT_PORT_VLAN,
+				    VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
+	}
+
+	return 0;
+}
+
 static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
 {
 	int i;
@@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.port_change_mtu = vsc73xx_change_mtu,
 	.port_max_mtu = vsc73xx_get_max_mtu,
 	.port_stp_state_set = vsc73xx_port_stp_state_set,
+	.port_vlan_filtering = vsc73xx_port_vlan_filtering,
+	.port_vlan_add = vsc73xx_port_vlan_add,
+	.port_vlan_del = vsc73xx_port_vlan_del,
 	.tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
 	.tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
 };
-- 
2.34.1

Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Sun, Jun 25, 2023 at 01:53:41PM +0200, Pawel Dembicki wrote:
> This patch implement vlan filtering for vsc73xx driver.
> 
> After vlan filtering start, switch is reconfigured from QinQ to simple
> vlan aware mode. It's required, because VSC73XX chips haven't support
> for inner vlan tag filter.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v2:
>   - no changes done
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 457eb7fddf4c..c946464489ab 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
>  	return ret;
>  }
>  
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> +			    bool vlan_filtering, struct netlink_ext_ack *extack)
> +{
> +	int ret, i;
> +
> +	if (vlan_filtering) {
> +		vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
> +	} else {
> +		if (port == CPU_PORT)
> +			vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> +		else
> +			vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
> +	}

Why do you need ports to be double VLAN aware when vlan_filtering=0?
Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
incoming packets, and set up the PVIDs of user ports as egress-tagged on
the CPU port?

> +
> +	for (i = 0; i <= 3072; i++) {
> +		ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> +		if (ret)
> +			return ret;
> +	}

What is the purpose of this?

> +
> +	return ret;
> +}
> +
>  static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
>  				     bool port_vlan)
>  {
> @@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
>  	return 0;
>  }
>  
> +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan,
> +				 struct netlink_ext_ack *extack)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	int ret;
> +
> +	/* Be sure to deny alterations to the configuration done by tag_8021q.
> +	 */
> +	if (vid_is_dsa_8021q(vlan->vid)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Range 3072-4095 reserved for dsa_8021q operation");
> +		return -EBUSY;
> +	}
> +
> +	if (untagged && port != CPU_PORT) {
> +		ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> +		if (ret)
> +			return ret;
> +	}
> +	if (pvid && port != CPU_PORT) {

Missing logic to change hardware PVID only while VLAN-aware, and to
restore the tag_8021q PVID when the bridge VLAN awareness gets disabled.
DSA does not resolve the conflicts on resources between .port_vlan_add()
and .tag_8021q_vlan_add(), the driver must do that.

> +		ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
> +
> +	return ret;

Style: return vsc73xx_port_update_vlan_table(...)

> +}
> +
> +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u16 vlan_no;
> +	int ret;
> +	u32 val;
> +
> +	ret =
> +	    vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);

Style: single line

> +	if (ret)
> +		return ret;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> +
> +	if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
> +		vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> +			     VSC73XX_TXUPDCFG, &val);
> +		vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> +			  VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> +		if (vlan_no == vlan->vid) {
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_TXUPDCFG,
> +					    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> +					    0);
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +					    VSC73XX_TXUPDCFG,
> +					    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> +		}
> +	}
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> +	vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> +	if (vlan_no && vlan_no == vlan->vid) {
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_PORT_VLAN,
> +				    VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);

As documented in Documentation/networking/switchdev.rst:

When the bridge has VLAN filtering enabled and a PVID is not configured on the
ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
priority-tagged packets must be accepted and forwarded according to the
bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
disabled, the presence/lack of a PVID should not influence the packet
forwarding decision.

Setting the hardware PVID to 0 when the bridge PVID is deleted sounds
like it accomplishes none of those.

> +	}
> +
> +	return 0;
> +}
> +
>  static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
>  {
>  	int i;
> @@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
>  	.port_stp_state_set = vsc73xx_port_stp_state_set,
> +	.port_vlan_filtering = vsc73xx_port_vlan_filtering,
> +	.port_vlan_add = vsc73xx_port_vlan_add,
> +	.port_vlan_del = vsc73xx_port_vlan_del,
>  	.tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
>  	.tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
>  };
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering

[Paweł Dembicki]

niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Sun, Jun 25, 2023 at 01:53:41PM +0200, Pawel Dembicki wrote:
> > This patch implement vlan filtering for vsc73xx driver.
> >
> > After vlan filtering start, switch is reconfigured from QinQ to simple
> > vlan aware mode. It's required, because VSC73XX chips haven't support
> > for inner vlan tag filter.
> >
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > v2:
> >   - no changes done
> >
> >  drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 457eb7fddf4c..c946464489ab 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
> >       return ret;
> >  }
> >
> > +static int
> > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> > +                         bool vlan_filtering, struct netlink_ext_ack *extack)
> > +{
> > +     int ret, i;
> > +
> > +     if (vlan_filtering) {
> > +             vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
> > +     } else {
> > +             if (port == CPU_PORT)
> > +                     vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> > +             else
> > +                     vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
> > +     }
>
> Why do you need ports to be double VLAN aware when vlan_filtering=0?
> Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
> incoming packets, and set up the PVIDs of user ports as egress-tagged on
> the CPU port?
>

Because I want to forward tagged and untagged frames when
vlan_filtering is off.  If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put
all (tagged and untagged) traffic into the outer vlan, called by the
datasheet as "MAN space". In QinQ mode, it is possible to ignore what
goes from a particular port but it is possible to separate traffic
from different ports.

> > +
> > +     for (i = 0; i <= 3072; i++) {
> > +             ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> What is the purpose of this?

I want to be sure that the table is cleared when vlan awareness is changed.

>
> > +
> > +     return ret;
> > +}
> > +
> >  static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> >                                    bool port_vlan)
> >  {
> > @@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> >       return 0;
> >  }
> >
> > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan,
> > +                              struct netlink_ext_ack *extack)
> > +{
> > +     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > +     int ret;
> > +
> > +     /* Be sure to deny alterations to the configuration done by tag_8021q.
> > +      */
> > +     if (vid_is_dsa_8021q(vlan->vid)) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "Range 3072-4095 reserved for dsa_8021q operation");
> > +             return -EBUSY;
> > +     }
> > +
> > +     if (untagged && port != CPU_PORT) {
> > +             ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     if (pvid && port != CPU_PORT) {
>
> Missing logic to change hardware PVID only while VLAN-aware, and to
> restore the tag_8021q PVID when the bridge VLAN awareness gets disabled.
> DSA does not resolve the conflicts on resources between .port_vlan_add()
> and .tag_8021q_vlan_add(), the driver must do that.
>
> > +             ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
> > +
> > +     return ret;
>
> Style: return vsc73xx_port_update_vlan_table(...)
>
> > +}
> > +
> > +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u16 vlan_no;
> > +     int ret;
> > +     u32 val;
> > +
> > +     ret =
> > +         vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
>
> Style: single line
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> > +
> > +     if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> > +                          VSC73XX_TXUPDCFG, &val);
> > +             vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> > +                       VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> > +             if (vlan_no == vlan->vid) {
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_TXUPDCFG,
> > +                                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> > +                                         0);
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_TXUPDCFG,
> > +                                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> > +             }
> > +     }
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> > +     vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> > +     if (vlan_no && vlan_no == vlan->vid) {
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_PORT_VLAN,
> > +                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
>
> As documented in Documentation/networking/switchdev.rst:
>
> When the bridge has VLAN filtering enabled and a PVID is not configured on the
> ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
> has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
> priority-tagged packets must be accepted and forwarded according to the
> bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
> disabled, the presence/lack of a PVID should not influence the packet
> forwarding decision.
>
> Setting the hardware PVID to 0 when the bridge PVID is deleted sounds
> like it accomplishes none of those.
>

My bad. I should just set VSC73XX_CAT_DROP_UNTAGGED_ENA here.

> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
> >  {
> >       int i;
> > @@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .port_change_mtu = vsc73xx_change_mtu,
> >       .port_max_mtu = vsc73xx_get_max_mtu,
> >       .port_stp_state_set = vsc73xx_port_stp_state_set,
> > +     .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> > +     .port_vlan_add = vsc73xx_port_vlan_add,
> > +     .port_vlan_del = vsc73xx_port_vlan_del,
> >       .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
> >       .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
> >  };
> > --
> > 2.34.1
> >
>

Thank you for such detailed responses and clarifying for me many issues.

--
Pawel Dembicki

Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Thu, Jun 29, 2023 at 10:18:08PM +0200, Paweł Dembicki wrote:
> niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@gmail.com> napisał(a):
> > Why do you need ports to be double VLAN aware when vlan_filtering=0?
> > Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
> > incoming packets, and set up the PVIDs of user ports as egress-tagged on
> > the CPU port?
> 
> Because I want to forward tagged and untagged frames when
> vlan_filtering is off.  If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put
> all (tagged and untagged) traffic into the outer vlan, called by the
> datasheet as "MAN space". In QinQ mode, it is possible to ignore what
> goes from a particular port but it is possible to separate traffic
> from different ports.

I think we may have some problem in finding common terminology.

Opening the manual and seeing the table "Customer Port Sample Configuration",
I think it's indeed what you need. But I wouldn't call it "double VLAN aware".
The port is actually configured to be VLAN *unaware* from the perspective of
classification, and always encapsulate all packets in one more VLAN (the
port PVID).

This switch's analyzer is always aware only of the outer VLAN header, and
that's not "double VLAN aware" (it can perform no action based on the
inner VLAN, if that exists), but it's fine for what is needed of it.

You might be mixing these with MAC_CFG::VLAN_AWR and MAC_CFG::VLAN_DBLAWR,
which essentially are only there to allow single- and double-VLAN-tagged
frames to be longer by 4 and 8 bytes, respectively, than the max frame
size. I don't think that these 2 fields have any reason to depend upon
the bridge VLAN awareness state of the port. They can be unconditionally
enabled. After all, Linux only cares about MTU, and that is the size of
the L2 payload, excluding any VLAN headers, if present.

I would suggest that if you exclude the MAC_CFG registers from
vsc73xx_port_set_vlan_conf(), you end up with not as many VLAN awareness
modes as you think. 2, to be precise: on or off. So you don't need the
enum.

Also, AFAIU, I don't see a reason to modify CAT_VLAN_MISC::VLAN_KEEP_TAG_ENA
from the value of 1 at all. You could always keep frames in the queue
system with the VID attached, and strip that VID on egress, if necessary,
via TXUPDCFG.

Not sure if you're noticed this, but drivers/net/ethernet/mscc/ and
drivers/net/dsa/ocelot/ contain a driver for a newer generation of
hardware than the VSC73xx, but many of the concepts apply. Maybe you
can take a look at how some things were done there.

> > > +
> > > +     for (i = 0; i <= 3072; i++) {
> > > +             ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> >
> > What is the purpose of this?
> 
> I want to be sure that the table is cleared when vlan awareness is changed.

Yes, but why? That should specifically not be done, since there is no
code in the kernel to replay the port_vlan_add() and tag_8021q_vlan_add()
calls for you when the VLAN awareness state changes. If you delete the
VLANs, they're gone, even though in software they're still there.

[PATCH net-next v2 7/7] net: dsa: vsc73xx: fix MTU configuration

[Pawel Dembicki]

Switch in MAXLEN register store maximum size of data frame.
MTU size is 18 bytes smaller than frame size.

Current settings causes problems with packet forwarding.
This patch fix MTU settings to proper values.

Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v2:
  - fix commit message style issue

 drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index c946464489ab..59bb3dbe780a 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -979,17 +979,18 @@ static int vsc73xx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 	struct vsc73xx *vsc = ds->priv;
 
 	return vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
-			     VSC73XX_MAXLEN, new_mtu);
+			     VSC73XX_MAXLEN, new_mtu + ETH_HLEN + ETH_FCS_LEN);
 }
 
 /* According to application not "VSC7398 Jumbo Frames" setting
- * up the MTU to 9.6 KB does not affect the performance on standard
+ * up the frame size to 9.6 KB does not affect the performance on standard
  * frames. It is clear from the application note that
  * "9.6 kilobytes" == 9600 bytes.
  */
 static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
 {
-	return 9600;
+	/* max mtu = 9600 - ETH_HLEN - ETH_FCS_LEN */
+	return 9582;
 }
 
 static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
-- 
2.34.1

Re: [PATCH net-next v2 7/7] net: dsa: vsc73xx: fix MTU configuration

[Vladimir Oltean]

On Sun, Jun 25, 2023 at 01:53:42PM +0200, Pawel Dembicki wrote:
> Switch in MAXLEN register store maximum size of data frame.
> MTU size is 18 bytes smaller than frame size.
> 
> Current settings causes problems with packet forwarding.
> This patch fix MTU settings to proper values.
> 
> Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---

Please split this patch from the rest of the series and re-target it
towards net.git.

> v2:
>   - fix commit message style issue
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index c946464489ab..59bb3dbe780a 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -979,17 +979,18 @@ static int vsc73xx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	struct vsc73xx *vsc = ds->priv;
>  
>  	return vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
> -			     VSC73XX_MAXLEN, new_mtu);
> +			     VSC73XX_MAXLEN, new_mtu + ETH_HLEN + ETH_FCS_LEN);
>  }
>  
>  /* According to application not "VSC7398 Jumbo Frames" setting
> - * up the MTU to 9.6 KB does not affect the performance on standard
> + * up the frame size to 9.6 KB does not affect the performance on standard
>   * frames. It is clear from the application note that
>   * "9.6 kilobytes" == 9600 bytes.
>   */
>  static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
>  {
> -	return 9600;
> +	/* max mtu = 9600 - ETH_HLEN - ETH_FCS_LEN */
> +	return 9582;

This can also be:

	return 9600 - ETH_HLEN - ETH_FCS_LEN;

since the arithmetic is on constants, it can be evaluated at compile
time and it results in the same generated code, but the comment is no
longer necessary.

>  }
>  
>  static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 7/7] net: dsa: vsc73xx: fix MTU configuration

[Paweł Dembicki]

niedz., 25 cze 2023 o 16:54 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Sun, Jun 25, 2023 at 01:53:42PM +0200, Pawel Dembicki wrote:
> > Switch in MAXLEN register store maximum size of data frame.
> > MTU size is 18 bytes smaller than frame size.
> >
> > Current settings causes problems with packet forwarding.
> > This patch fix MTU settings to proper values.
> >
> > Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
>
> Please split this patch from the rest of the series and re-target it
> towards net.git.
>

I resend it.
https://lore.kernel.org/netdev/20230628194327.1765644-1-paweldembicki@gmail.com/

--
Pawel Dembicki

> > v2:
> >   - fix commit message style issue
> >
> >  drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index c946464489ab..59bb3dbe780a 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -979,17 +979,18 @@ static int vsc73xx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> >       struct vsc73xx *vsc = ds->priv;
> >
> >       return vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
> > -                          VSC73XX_MAXLEN, new_mtu);
> > +                          VSC73XX_MAXLEN, new_mtu + ETH_HLEN + ETH_FCS_LEN);
> >  }
> >
> >  /* According to application not "VSC7398 Jumbo Frames" setting
> > - * up the MTU to 9.6 KB does not affect the performance on standard
> > + * up the frame size to 9.6 KB does not affect the performance on standard
> >   * frames. It is clear from the application note that
> >   * "9.6 kilobytes" == 9600 bytes.
> >   */
> >  static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> >  {
> > -     return 9600;
> > +     /* max mtu = 9600 - ETH_HLEN - ETH_FCS_LEN */
> > +     return 9582;
>
> This can also be:
>
>         return 9600 - ETH_HLEN - ETH_FCS_LEN;
>
> since the arithmetic is on constants, it can be evaluated at compile
> time and it results in the same generated code, but the comment is no
> longer necessary.
>
> >  }
> >
> >  static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> > --
> > 2.34.1
> >
>

[PATCH net-next v2 0/7] net: dsa: vsc73xx: Make vsc73xx usable

[Pawel Dembicki]

This patch series is focused on getting vsc73xx usable.

First patch was added in v2, it's switch from poll loop to
read_poll_timeout.

Second patch is simple convert to phylink, because adjust_link won't work
anymore.

Patches 3-6 are basic implementation of tag8021q funcionality with QinQ
support without vlan filtering in bridge and simple vlan aware in vlan
filtering mode.

STP frames isn't forwarded at this moment. BPDU frames are forwarded 
only from to PI/SI interface. For more info see chapter 
2.7.1 (CPU Forwarding) in datasheet.

Last patch fix wrong MTU configuration.
Pawel Dembicki (7):
  net: dsa: vsc73xx: use read_poll_timeout instead delay loop
  net: dsa: vsc73xx: convert to PHYLINK
  net: dsa: vsc73xx: add port_stp_state_set function
  net: dsa: vsc73xx: Add dsa tagging based on 8021q
  net: dsa: vsc73xx: Add bridge support
  net: dsa: vsc73xx: Add vlan filtering
  net: dsa: vsc73xx: fix MTU configuration

 drivers/net/dsa/Kconfig                |   2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c | 937 ++++++++++++++++++++-----
 drivers/net/dsa/vitesse-vsc73xx.h      |   2 +
 include/net/dsa.h                      |   2 +
 net/dsa/Kconfig                        |   6 +
 net/dsa/Makefile                       |   1 +
 net/dsa/tag_vsc73xx_8021q.c            |  87 +++
 7 files changed, 856 insertions(+), 181 deletions(-)
 create mode 100644 net/dsa/tag_vsc73xx_8021q.c

-- 
2.34.1

Re: [PATCH net-next v2 0/7] net: dsa: vsc73xx: Make vsc73xx usable

[Vladimir Oltean]

Hi Pawel,

On Sun, Jun 25, 2023 at 01:53:43PM +0200, Pawel Dembicki wrote:
> This patch series is focused on getting vsc73xx usable.
> 
> First patch was added in v2, it's switch from poll loop to
> read_poll_timeout.
> 
> Second patch is simple convert to phylink, because adjust_link won't work
> anymore.
> 
> Patches 3-6 are basic implementation of tag8021q funcionality with QinQ
> support without vlan filtering in bridge and simple vlan aware in vlan
> filtering mode.
> 
> STP frames isn't forwarded at this moment. BPDU frames are forwarded 
> only from to PI/SI interface. For more info see chapter 
> 2.7.1 (CPU Forwarding) in datasheet.
> 
> Last patch fix wrong MTU configuration.

It is considered good practice for the people whom you've selectively
copied to the individual patches to also be copied to the cover letter.

Re: [PATCH net-next v2 1/7] net: dsa: vsc73xx: use read_poll_timeout instead delay loop

[Andrew Lunn]

On Sun, Jun 25, 2023 at 01:53:36PM +0200, Pawel Dembicki wrote:
> This commit switches delay loop to read_poll_timeout macro durring
> Arbiter empty check in adjust link function.
> 
> As Russel King suggested:
> 
> "This [change] avoids the issue that on the last iteration, the code reads
> the register, test it, find the condition that's being waiting for is
> false, _then_ waits and end up printing the error message - that last
> wait is rather useless, and as the arbiter state isn't checked after
> waiting, it could be that we had success during the last wait."
> 
> It also remove one short msleep delay.
> 
> Suggested-by: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

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

    Andrew