[PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable

[PATCH net-next v3 0/8] net: dsa: vsc73xx: Make vsc73xx usable

[Pawel Dembicki]

This patch series focuses on making vsc73xx usable.

The first patch was added in v2; it switches from a poll loop to
read_poll_timeout.

The second patch is a simple conversion to phylink because adjust_link won't
work anymore.

The third patch introduces a definition with the maximum number of ports to
avoid using magic numbers.

The fourth patch implements port state configuration, which is required for
bridge functionality. STP frames are not forwarded at this moment. BPDU frames
are only forwarded from/to the PI/SI interface. For more information, see chapter
2.7.1 (CPU Forwarding) in the datasheet.

Patches 5-8 provide a basic implementation of tag8021q functionality with QinQ
support, without VLAN filtering in the bridge and simple VLAN awareness in VLAN
filtering mode.

Pawel Dembicki (8):
  net: dsa: vsc73xx: use read_poll_timeout instead delay loop
  net: dsa: vsc73xx: convert to PHYLINK
  net: dsa: vsc73xx: Add define for max num of ports
  net: dsa: vsc73xx: add port_stp_state_set function
  net: dsa: vsc73xx: Add vlan filtering
  net: dsa: vsc73xx: introduce tag 8021q for vsc73xx
  net: dsa: vsc73xx: Implement vsc73xx 8021q tagger
  net: dsa: vsc73xx: Add bridge support

 drivers/net/dsa/Kconfig                |   2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c | 800 +++++++++++++++++++++----
 drivers/net/dsa/vitesse-vsc73xx.h      |  17 +
 include/net/dsa.h                      |   2 +
 net/dsa/Kconfig                        |   6 +
 net/dsa/Makefile                       |   1 +
 net/dsa/tag_vsc73xx_8021q.c            |  91 +++
 7 files changed, 806 insertions(+), 113 deletions(-)
 create mode 100644 net/dsa/tag_vsc73xx_8021q.c

-- 
2.34.1

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

[Pawel Dembicki]

This commit switches delay loop to read_poll_timeout macro during
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>
---
v3:
  - Add "Reviewed-by" to commit message only
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 4f09e7438f3b..b117c0c18e36 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -779,7 +779,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);
@@ -794,19 +794,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 v3 2/8] 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>
---
v3:
  - remove legacy_pre_march2020 after rebase
v2:
  - replace switch to if and get rid of macros in
    vsc73xx_phylink_mac_link_up function

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

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index b117c0c18e36..39d3d78f4bc3 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -714,8 +714,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;
@@ -753,12 +752,34 @@ 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);
+	}
+}
+
+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 */
@@ -774,101 +795,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,
@@ -1039,7 +1038,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 v3 3/8] net: dsa: vsc73xx: Add define for max num of ports

[Pawel Dembicki]

This patch introduces a new define: VSC73XX_MAX_NUM_PORTS, which can be
used in the future instead of a hardcoded value.

Currently, the only hardcoded value is vsc->ds->num_ports. It is being
replaced with the new define.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

---
v3:
  - Introduce patch

 drivers/net/dsa/vitesse-vsc73xx-core.c | 13 +------------
 drivers/net/dsa/vitesse-vsc73xx.h      | 13 +++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 39d3d78f4bc3..8f2285a03e82 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1175,23 +1175,12 @@ int vsc73xx_probe(struct vsc73xx *vsc)
 		 vsc->addr[0], vsc->addr[1], vsc->addr[2],
 		 vsc->addr[3], vsc->addr[4], vsc->addr[5]);
 
-	/* The VSC7395 switch chips have 5+1 ports which means 5
-	 * ordinary ports and a sixth CPU port facing the processor
-	 * with an RGMII interface. These ports are numbered 0..4
-	 * and 6, so they leave a "hole" in the port map for port 5,
-	 * which is invalid.
-	 *
-	 * The VSC7398 has 8 ports, port 7 is again the CPU port.
-	 *
-	 * We allocate 8 ports and avoid access to the nonexistant
-	 * ports.
-	 */
 	vsc->ds = devm_kzalloc(dev, sizeof(*vsc->ds), GFP_KERNEL);
 	if (!vsc->ds)
 		return -ENOMEM;
 
 	vsc->ds->dev = dev;
-	vsc->ds->num_ports = 8;
+	vsc->ds->num_ports = VSC73XX_MAX_NUM_PORTS;
 	vsc->ds->priv = vsc;
 
 	vsc->ds->ops = &vsc73xx_ds_ops;
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index 30b1f0a36566..f79d81ef24fb 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -3,6 +3,19 @@
 #include <linux/etherdevice.h>
 #include <linux/gpio/driver.h>
 
+/* The VSC7395 switch chips have 5+1 ports which means 5
+ * ordinary ports and a sixth CPU port facing the processor
+ * with an RGMII interface. These ports are numbered 0..4
+ * and 6, so they leave a "hole" in the port map for port 5,
+ * which is invalid.
+ *
+ * The VSC7398 has 8 ports, port 7 is again the CPU port.
+ *
+ * We allocate 8 ports and avoid access to the nonexistant
+ * ports.
+ */
+#define VSC73XX_MAX_NUM_PORTS	8
+
 /**
  * struct vsc73xx - VSC73xx state container
  */
-- 
2.34.1

[PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function

[Pawel Dembicki]

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

This implementation handles properly all states, but vsc73xx doesn't
forward STP packets.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
  - use 'VSC73XX_MAX_NUM_PORTS' define
  - add 'state == BR_STATE_DISABLED' condition
  - fix style issues
v2:
  - fix kdoc

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

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 8f2285a03e82..541fbc195df1 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -163,6 +163,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)
@@ -619,9 +623,6 @@ 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);
@@ -864,9 +865,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);
 }
 
@@ -1033,9 +1031,59 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
 	return 9600 - ETH_HLEN - ETH_FCS_LEN;
 }
 
+static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
+{
+	/* Configure forward map to CPU <-> port only */
+	if (port == CPU_PORT)
+		vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
+					     ~BIT(CPU_PORT);
+	else
+		vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
+					 BIT(CPU_PORT);
+
+	return 0;
+}
+
+/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
+ * forwarded only from and to PI/SI interface. For more info see chapter
+ * 2.7.1 (CPU Forwarding) in datasheet.
+ * This function is required for tag8021q operations.
+ */
+
+static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
+				       u8 state)
+{
+	struct vsc73xx *vsc = ds->priv;
+
+	if (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,
+	.port_setup = vsc73xx_port_setup,
 	.phy_read = vsc73xx_phy_read,
 	.phy_write = vsc73xx_phy_write,
 	.phylink_get_caps = vsc73xx_phylink_get_caps,
@@ -1049,6 +1097,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 f79d81ef24fb..224e284a5573 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -18,6 +18,7 @@
 
 /**
  * struct vsc73xx - VSC73xx state container
+ * @forward_map: Forward table cache
  */
 struct vsc73xx {
 	struct device			*dev;
@@ -28,6 +29,7 @@ struct vsc73xx {
 	u8				addr[ETH_ALEN];
 	const struct vsc73xx_ops	*ops;
 	void				*priv;
+	u8				forward_map[VSC73XX_MAX_NUM_PORTS];
 };
 
 struct vsc73xx_ops {
-- 
2.34.1

[PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Pawel Dembicki]

This patch implements VLAN filtering for the vsc73xx driver.

After starting VLAN filtering, the switch is reconfigured from QinQ to
a simple VLAN aware mode. This is required because VSC73XX chips do not
support inner VLAN tag filtering.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
  - reworked all vlan commits
  - added storage variables for pvid and untagged vlans
  - move length extender settings to port setup
  - remove vlan table cleaning in wrong places
  - note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged'
    and 'vsc73xx_vlan_set_pvid' are used later in tag implementation
v2:
  - no changes done

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

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 541fbc195df1..d9a6eac1fcce 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -21,14 +21,18 @@
 #include <linux/of_mdio.h>
 #include <linux/bitops.h>
 #include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
 #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>
 
 #include "vitesse-vsc73xx.h"
 
+#define VSC73XX_IS_CONFIGURED	0x1
+
 #define VSC73XX_BLOCK_MAC	0x1 /* Subblocks 0-4, 6 (CPU port) */
 #define VSC73XX_BLOCK_ANALYZER	0x2 /* Only subblock 0 */
 #define VSC73XX_BLOCK_MII	0x3 /* Subblocks 0 and 1 */
@@ -61,6 +65,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 */
@@ -121,6 +127,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)
@@ -134,6 +151,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
@@ -188,7 +214,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
@@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
 	{ 29, "TxQoSClass3" }, /* non-standard counter */
 };
 
+enum vsc73xx_port_vlan_conf {
+	VSC73XX_VLAN_FILTER,
+	VSC73XX_VLAN_IGNORE,
+};
+
 int vsc73xx_is_addr_valid(u8 block, u8 subblock)
 {
 	switch (block) {
@@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
 static int vsc73xx_setup(struct dsa_switch *ds)
 {
 	struct vsc73xx *vsc = ds->priv;
-	int i;
+	int i, ret;
 
 	dev_info(vsc->dev, "set up the switch\n");
 
@@ -623,6 +655,9 @@ 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);
+	/* 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);
@@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
 	return 9600 - ETH_HLEN - ETH_FCS_LEN;
 }
 
+static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
+{
+	int ret, err;
+	u32 val;
+
+	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_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
+{
+	u32 val;
+	int ret;
+
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+	ret = vsc73xx_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_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_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
+{
+	int ret;
+
+	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
+	ret = vsc73xx_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));
+
+	return vsc73xx_wait_for_vlan_table_cmd(vsc);
+}
+
+static int
+vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+	u8 portmap;
+	int ret;
+
+	ret = vsc73xx_read_vlan_table_entry(vsc, 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;
+
+	return  vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
+}
+
+static void
+vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
+		      enum vsc73xx_port_vlan_conf port_vlan_conf)
+{
+	if (port_vlan_conf == VSC73XX_VLAN_IGNORE) {
+		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);
+	}
+}
+
+static int
+vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+	u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
+
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
+	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_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)
+{
+	u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
+
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
+			    VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
+
+	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_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid)
+{
+	u32 val;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
+	if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
+		return 0;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+	*vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+
+	return VSC73XX_IS_CONFIGURED;
+}
+
+static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid)
+{
+	u32 val;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+	if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
+		return 0;
+
+	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
+		     &val);
+	*vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+		VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+
+	return VSC73XX_IS_CONFIGURED;
+}
+
+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 = 0;
+
+	if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) {
+		if (vsc->untagged_storage[port] < VLAN_N_VID &&
+		    !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
+		    !vid_is_dsa_8021q(vid) &&
+		    vsc->untagged_storage[port] != vid) {
+			dev_warn(vsc->dev,
+				 "Port %d can have only one untagged vid! Now is: %d.\n",
+				 port, vsc->untagged_storage[port]);
+			return -EOPNOTSUPP;
+		}
+		vsc->untagged_storage[port] = vid;
+	} else {
+		if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+		    !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 -EOPNOTSUPP;
+		}
+		return vsc73xx_vlan_change_untagged(vsc, port, vid, 1);
+	}
+
+	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;
+
+	if (!dsa_port)
+		return -EINVAL;
+
+	if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) {
+		if (vsc->pvid_storage[port] < VLAN_N_VID &&
+		    !vid_is_dsa_8021q(vsc->pvid_storage[port]) &&
+		    !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) {
+			dev_warn(vsc->dev,
+				 "Port %d can have only one pvid! Now is: %d.\n",
+				 port, vsc->pvid_storage[port]);
+			return -EOPNOTSUPP;
+		}
+		vsc->pvid_storage[port] = vid;
+	} else {
+		if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+		    !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 -EOPNOTSUPP;
+		}
+		return vsc73xx_vlan_change_pvid(vsc, port, vid, 1);
+	}
+
+	return 0;
+}
+
+static int
+vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
+			    bool vlan_filtering, struct netlink_ext_ack *extack)
+{
+	struct vsc73xx *vsc = ds->priv;
+	bool store_untagged  = false;
+	bool store_pvid  = false;
+	u16 vlan_no;
+
+	if (vlan_filtering)
+		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
+	else
+		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
+
+	if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
+		store_pvid = true;
+
+	if (vsc->pvid_storage[port] < VLAN_N_VID)
+		vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true);
+	else
+		vsc73xx_vlan_change_pvid(vsc, port, 0, false);
+
+	vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID;
+
+	if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
+		store_untagged  = true;
+
+	if (vsc->untagged_storage[port] < VLAN_N_VID)
+		vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true);
+	else
+		vsc73xx_vlan_change_untagged(vsc, port, 0, false);
+
+	vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_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;
+	struct vsc73xx *vsc = ds->priv;
+	u16 vlan_no;
+	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 (port != CPU_PORT) {
+		if (untagged) {
+			ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
+			if (ret)
+				return ret;
+		} else {
+			if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no)
+			    == VSC73XX_IS_CONFIGURED &&
+			    vlan_no == vlan->vid)
+				vsc73xx_vlan_change_untagged(vsc, port, 0, false);
+			else if (vsc->untagged_storage[port] == vlan->vid)
+				vsc->untagged_storage[port] = VLAN_N_VID;
+		}
+		if (pvid) {
+			ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
+			if (ret)
+				return ret;
+		} else {
+			if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no)
+			    == VSC73XX_IS_CONFIGURED &&
+			    vlan_no == vlan->vid)
+				vsc73xx_vlan_change_pvid(vsc, port, 0, false);
+			else if (vsc->pvid_storage[port] == vlan->vid)
+				vsc->pvid_storage[port] = VLAN_N_VID;
+		}
+	}
+
+	return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1);
+}
+
+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;
+
+	ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0);
+	if (ret)
+		return ret;
+
+	if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+	    vlan_no == vlan->vid)
+		vsc73xx_vlan_change_untagged(vsc, port, 0, false);
+	else if (vsc->untagged_storage[port] == vlan->vid)
+		vsc->untagged_storage[port] = VLAN_N_VID;
+
+	if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
+	    vlan_no == vlan->vid)
+		vsc73xx_vlan_change_pvid(vsc, port, 0, false);
+	else if (vsc->pvid_storage[port] == vlan->vid)
+		vsc->pvid_storage[port] = VLAN_N_VID;
+
+	return 0;
+}
+
 static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
 {
+	struct vsc73xx *vsc = ds->priv;
+	int ret, i;
+
+	/* Those bits are responsible for MTU only. Kernel take care about MTU,
+	 * let's enable +8 bytes frame length unconditionally.
+	 */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+			    VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR,
+			    VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR);
+
+	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);
+	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);
+
+	if (port == CPU_PORT)
+		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
+	else
+		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
+
+	for (i = 0; i <= VLAN_N_VID; i++) {
+		ret = vsc73xx_update_vlan_table(vsc, port, i, 0);
+		if (ret)
+			return ret;
+	}
+
 	/* Configure forward map to CPU <-> port only */
 	if (port == CPU_PORT)
 		vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
@@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
 		vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
 					 BIT(CPU_PORT);
 
+	/* Set storage values out of range*/
+	vsc->pvid_storage[port] = VLAN_N_VID;
+	vsc->untagged_storage[port] = VLAN_N_VID;
+
 	return 0;
 }
 
@@ -1098,6 +1516,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,
 };
 
 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 224e284a5573..b7614dd7d0eb 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -30,6 +30,8 @@ struct vsc73xx {
 	const struct vsc73xx_ops	*ops;
 	void				*priv;
 	u8				forward_map[VSC73XX_MAX_NUM_PORTS];
+	u16				pvid_storage[VSC73XX_MAX_NUM_PORTS];
+	u16				untagged_storage[VSC73XX_MAX_NUM_PORTS];
 };
 
 struct vsc73xx_ops {
-- 
2.34.1

[PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx

[Pawel Dembicki]

This commit introduces a new tagger based on 802.1q tagging.
It's designed for the vsc73xx driver. The VSC73xx family doesn't have
any tag support for the RGMII port, but it could be based on VLANs.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
  - Introduce a patch after the tagging patch split

 include/net/dsa.h           |  2 +
 net/dsa/Kconfig             |  6 +++
 net/dsa/Makefile            |  1 +
 net/dsa/tag_vsc73xx_8021q.c | 91 +++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+)
 create mode 100644 net/dsa/tag_vsc73xx_8021q.c

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 0b9c6aa27047..f83454d4ad02 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..9093a71e6eb0
--- /dev/null
+++ b/net/dsa/tag_vsc73xx_8021q.c
@@ -0,0 +1,91 @@
+// 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);
+	u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
+	u8 pcp;
+
+	if (skb->offload_fwd_mark) {
+		unsigned int bridge_num = dsa_port_bridge_num_get(dp);
+		struct net_device *br = dsa_port_bridge_dev_get(dp);
+
+		if (br_vlan_enabled(br))
+			return skb;
+
+		tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
+	}
+
+	pcp = netdev_txq_to_tc(netdev, queue_mapping);
+
+	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);
+	} else {
+		netdev_warn(netdev, "Couldn't decode source port\n");
+		return NULL;
+	}
+
+	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

[PATCH net-next v3 7/8] net: dsa: vsc73xx: Implement vsc73xx 8021q tagger

[Pawel Dembicki]

This patch is a simple implementation of 802.1q tagging in the vsc73xx
driver. Currently, devices with DSA_TAG_PROTO_NONE are not functional.
The VSC73XX family doesn't provide any tag support for external Ethernet
ports.

The only option available is VLAN-based tagging, which requires constant
hardware VLAN filtering. While the VSC73XX family supports provider
bridging, it only supports QinQ without full implementation of 802.1AD.
This means it only allows the doubled 0x8100 TPID.

In the simple port mode, QinQ is enabled to preserve forwarding of
VLAN-tagged frames.

The tag driver introduces the most basic functionality required for
proper tagging support.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
  - Split tagger and tag implementation into separate commits

 drivers/net/dsa/Kconfig                |  2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c | 48 +++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index f8c1d73b251d..120bc3ded9f2 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -126,7 +126,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
 
 config NET_DSA_VITESSE_VSC73XX
 	tristate
-	select NET_DSA_TAG_NONE
+	select NET_DSA_TAG_VSC73XX_8021Q
 	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 d9a6eac1fcce..bf903502bac1 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_mdio.h>
 #include <linux/bitops.h>
@@ -589,7 +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;
+	return DSA_TAG_PROTO_VSC73XX_8021Q;
 }
 
 static int vsc73xx_setup(struct dsa_switch *ds)
@@ -664,6 +665,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 
 	mdelay(50);
 
+	rtnl_lock();
+	ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
+	rtnl_unlock();
+	if (ret)
+		return ret;
+
 	/* Release reset from the internal PHYs */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_PHY_RESET);
@@ -1412,6 +1419,43 @@ static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
 	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;
+	struct vsc73xx *vsc = ds->priv;
+	int ret;
+
+	if (untagged) {
+		if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) {
+			ret = vsc73xx_vlan_set_untagged(ds, port, vid, false);
+			if (ret)
+				return ret;
+		} else {
+			vsc->untagged_storage[port] = vid;
+		}
+	}
+	if (pvid) {
+		if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) {
+			ret = vsc73xx_vlan_set_pvid(ds, port, vid, false);
+			if (ret)
+				return ret;
+		} else {
+			vsc->pvid_storage[port] = vid;
+		}
+	}
+
+	return vsc73xx_update_vlan_table(vsc, port, vid, 1);
+}
+
+static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
+{
+	struct vsc73xx *vsc = ds->priv;
+
+	return vsc73xx_update_vlan_table(vsc, port, vid, 0);
+}
+
 static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
 {
 	struct vsc73xx *vsc = ds->priv;
@@ -1519,6 +1563,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.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,
 };
 
 static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
-- 
2.34.1

[PATCH net-next v3 8/8] 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.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
v3:
  - All vlan commits was reworked
  - move VLAN_AWR and VLAN_DBLAWR to port setup in other commit
  - drop vlan table upgrade
v2:
  - no changes done

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

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index bf903502bac1..9d0367c2c52c 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -600,6 +600,9 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 
 	dev_info(vsc->dev, "set up the switch\n");
 
+	ds->untag_bridge_pvid = true;
+	ds->max_num_bridges = 7;
+
 	/* Issue RESET */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_MASTER_RESET);
@@ -1456,6 +1459,73 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 	return vsc73xx_update_vlan_table(vsc, port, vid, 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_port_setup(struct dsa_switch *ds, int port)
 {
 	struct vsc73xx *vsc = ds->priv;
@@ -1557,6 +1627,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

Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function

[Vladimir Oltean]

Hi Pawel,

On Tue, Sep 12, 2023 at 02:21:58PM +0200, Pawel Dembicki wrote:
> This isn't a fully functional implementation of 802.1D, but
> port_stp_state_set is required for a future tag8021q operations.
> 
> This implementation handles properly all states, but vsc73xx doesn't
> forward STP packets.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 8f2285a03e82..541fbc195df1 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1033,9 +1031,59 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
>  	return 9600 - ETH_HLEN - ETH_FCS_LEN;
>  }
>  
> +static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> +{

For bisectability, the series must build patch by patch.
Here, you are missing:

	struct vsc73xx *vsc = ds->priv;

../drivers/net/dsa/vitesse-vsc73xx-core.c:1038:3: error: use of undeclared identifier 'vsc'
                vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
                ^
../drivers/net/dsa/vitesse-vsc73xx-core.c:1041:3: error: use of undeclared identifier 'vsc'
                vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
                ^
2 errors generated.

> +	/* Configure forward map to CPU <-> port only */
> +	if (port == CPU_PORT)
> +		vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> +					     ~BIT(CPU_PORT);

		vsc->forward_map[CPU_PORT] = dsa_user_ports(ds);

> +	else
> +		vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> +					 BIT(CPU_PORT);

		vsc->forward_map[port] = BIT(CPU_PORT);

> +
> +	return 0;
> +}
> +
> +/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
> + * forwarded only from and to PI/SI interface. For more info see chapter
> + * 2.7.1 (CPU Forwarding) in datasheet.
> + * This function is required for tag8021q operations.
> + */
> +
> +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	if (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]);

To forward a packet between port A and port B, both of them must be in
BR_STATE_FORWARDING, not just A.

> +	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,
> +	.port_setup = vsc73xx_port_setup,
>  	.phy_read = vsc73xx_phy_read,
>  	.phy_write = vsc73xx_phy_write,
>  	.phylink_get_caps = vsc73xx_phylink_get_caps,
> @@ -1049,6 +1097,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 f79d81ef24fb..224e284a5573 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -18,6 +18,7 @@
>  
>  /**
>   * struct vsc73xx - VSC73xx state container
> + * @forward_map: Forward table cache

If you start describing the member fields, shouldn't all be described?
I think there will be kdoc warnings otherwise.

>   */
>  struct vsc73xx {
>  	struct device			*dev;
> @@ -28,6 +29,7 @@ struct vsc73xx {
>  	u8				addr[ETH_ALEN];
>  	const struct vsc73xx_ops	*ops;
>  	void				*priv;
> +	u8				forward_map[VSC73XX_MAX_NUM_PORTS];
>  };
>  
>  struct vsc73xx_ops {
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function

[Paweł Dembicki]

wt., 12 wrz 2023 o 16:48 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> Hi Pawel,
>
Hi Vladimir,

Thank you for Your time.

> On Tue, Sep 12, 2023 at 02:21:58PM +0200, Pawel Dembicki wrote:
> > This isn't a fully functional implementation of 802.1D, but
> > port_stp_state_set is required for a future tag8021q operations.
> >
> > This implementation handles properly all states, but vsc73xx doesn't
> > forward STP packets.
> >
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 8f2285a03e82..541fbc195df1 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -1033,9 +1031,59 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> >       return 9600 - ETH_HLEN - ETH_FCS_LEN;
> >  }
> >
> > +static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> > +{
>
> For bisectability, the series must build patch by patch.
> Here, you are missing:
>
>         struct vsc73xx *vsc = ds->priv;
>
> ../drivers/net/dsa/vitesse-vsc73xx-core.c:1038:3: error: use of undeclared identifier 'vsc'
>                 vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
>                 ^
> ../drivers/net/dsa/vitesse-vsc73xx-core.c:1041:3: error: use of undeclared identifier 'vsc'
>                 vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
>                 ^
> 2 errors generated.
>
> > +     /* Configure forward map to CPU <-> port only */
> > +     if (port == CPU_PORT)
> > +             vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> > +                                          ~BIT(CPU_PORT);
>
>                 vsc->forward_map[CPU_PORT] = dsa_user_ports(ds);
>
> > +     else
> > +             vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> > +                                      BIT(CPU_PORT);
>
>                 vsc->forward_map[port] = BIT(CPU_PORT);
>
> > +
> > +     return 0;
> > +}
> > +
> > +/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
> > + * forwarded only from and to PI/SI interface. For more info see chapter
> > + * 2.7.1 (CPU Forwarding) in datasheet.
> > + * This function is required for tag8021q operations.
> > + */
> > +
> > +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> > +                                    u8 state)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +
> > +     if (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]);
>
> To forward a packet between port A and port B, both of them must be in
> BR_STATE_FORWARDING, not just A.
>

In this patch bridges are unimplemented. Please look at 8/8 patch [0].

> > +     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,
> > +     .port_setup = vsc73xx_port_setup,
> >       .phy_read = vsc73xx_phy_read,
> >       .phy_write = vsc73xx_phy_write,
> >       .phylink_get_caps = vsc73xx_phylink_get_caps,
> > @@ -1049,6 +1097,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 f79d81ef24fb..224e284a5573 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > @@ -18,6 +18,7 @@
> >
> >  /**
> >   * struct vsc73xx - VSC73xx state container
> > + * @forward_map: Forward table cache
>
> If you start describing the member fields, shouldn't all be described?
> I think there will be kdoc warnings otherwise.
>

Jakub in v1 series points kdoc warn in this case. I added a
description to the field added by me. Should I prepare in the v4
series a separate commit for other descriptions in this struct?

> >   */
> >  struct vsc73xx {
> >       struct device                   *dev;
> > @@ -28,6 +29,7 @@ struct vsc73xx {
> >       u8                              addr[ETH_ALEN];
> >       const struct vsc73xx_ops        *ops;
> >       void                            *priv;
> > +     u8                              forward_map[VSC73XX_MAX_NUM_PORTS];
> >  };
> >
> >  struct vsc73xx_ops {
> > --
> > 2.34.1
> >

[0] https://lore.kernel.org/netdev/20230912122201.3752918-9-paweldembicki@gmail.com/T/#u

Re: [PATCH net-next v3 4/8] net: dsa: vsc73xx: add port_stp_state_set function

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 05:27:45PM +0200, Paweł Dembicki wrote:
> > To forward a packet between port A and port B, both of them must be in
> > BR_STATE_FORWARDING, not just A.
> >
> 
> In this patch bridges are unimplemented. Please look at 8/8 patch [0].
> 
> [0] https://lore.kernel.org/netdev/20230912122201.3752918-9-paweldembicki@gmail.com/T/#u

Yes, but vsc73xx_port_stp_state_set() remains unchanged until the end.
What am I missing? In your implementation, nothing prevents port i
(which is in BR_STATE_FORWARDING) from forwarding packets towards a port j,
present in vsc->forward_map[i] & BIT(j), which is *not* in BR_STATE_FORWARDING.
If you don't have access to the STP protocol yet, you can put port j
down and it will go to the DISABLED state and you can confirm that other
ports in the bridge will still remain configured to forward to it.

> > > diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> > > index f79d81ef24fb..224e284a5573 100644
> > > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > > @@ -18,6 +18,7 @@
> > >
> > >  /**
> > >   * struct vsc73xx - VSC73xx state container
> > > + * @forward_map: Forward table cache
> >
> > If you start describing the member fields, shouldn't all be described?
> > I think there will be kdoc warnings otherwise.
> >
> 
> Jakub in v1 series points kdoc warn in this case. I added a
> description to the field added by me. Should I prepare in the v4
> series a separate commit for other descriptions in this struct?

Yes, but please hold off posting it until I'm done reviewing this version.

Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 02:21:59PM +0200, Pawel Dembicki wrote:
> This patch implements VLAN filtering for the vsc73xx driver.
> 
> After starting VLAN filtering, the switch is reconfigured from QinQ to
> a simple VLAN aware mode. This is required because VSC73XX chips do not
> support inner VLAN tag filtering.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v3:
>   - reworked all vlan commits
>   - added storage variables for pvid and untagged vlans
>   - move length extender settings to port setup
>   - remove vlan table cleaning in wrong places
>   - note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged'
>     and 'vsc73xx_vlan_set_pvid' are used later in tag implementation
> v2:
>   - no changes done
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 425 ++++++++++++++++++++++++-
>  drivers/net/dsa/vitesse-vsc73xx.h      |   2 +
>  2 files changed, 425 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 541fbc195df1..d9a6eac1fcce 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -21,14 +21,18 @@
>  #include <linux/of_mdio.h>
>  #include <linux/bitops.h>
>  #include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
>  #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>
>  
>  #include "vitesse-vsc73xx.h"
>  
> +#define VSC73XX_IS_CONFIGURED	0x1
> +
>  #define VSC73XX_BLOCK_MAC	0x1 /* Subblocks 0-4, 6 (CPU port) */
>  #define VSC73XX_BLOCK_ANALYZER	0x2 /* Only subblock 0 */
>  #define VSC73XX_BLOCK_MII	0x3 /* Subblocks 0 and 1 */
> @@ -61,6 +65,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 */
> @@ -121,6 +127,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)
> @@ -134,6 +151,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
> @@ -188,7 +214,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
> @@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
>  	{ 29, "TxQoSClass3" }, /* non-standard counter */
>  };
>  
> +enum vsc73xx_port_vlan_conf {
> +	VSC73XX_VLAN_FILTER,
> +	VSC73XX_VLAN_IGNORE,
> +};
> +
>  int vsc73xx_is_addr_valid(u8 block, u8 subblock)
>  {
>  	switch (block) {
> @@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
>  static int vsc73xx_setup(struct dsa_switch *ds)
>  {
>  	struct vsc73xx *vsc = ds->priv;
> -	int i;
> +	int i, ret;

../drivers/net/dsa/vitesse-vsc73xx-core.c:598:9: warning: unused variable 'ret' [-Wunused-variable]
        int i, ret;
               ^

>  
>  	dev_info(vsc->dev, "set up the switch\n");
>  
> @@ -623,6 +655,9 @@ 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);
> +	/* 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);
> @@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
>  	return 9600 - ETH_HLEN - ETH_FCS_LEN;
>  }
>  
> +static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
> +{
> +	int ret, err;
> +	u32 val;
> +
> +	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_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
> +{
> +	u32 val;
> +	int ret;
> +
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> +	ret = vsc73xx_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_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_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
> +{
> +	int ret;
> +
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> +	ret = vsc73xx_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));
> +
> +	return vsc73xx_wait_for_vlan_table_cmd(vsc);
> +}
> +
> +static int
> +vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
> +{
> +	u8 portmap;
> +	int ret;
> +
> +	ret = vsc73xx_read_vlan_table_entry(vsc, 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;

Why did you need to do this? To my knowledge, the DSA framework code
will decide when to remove VLANs from the CPU port.

> +
> +	return  vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
> +}
> +
> +static void
> +vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
> +		      enum vsc73xx_port_vlan_conf port_vlan_conf)
> +{
> +	if (port_vlan_conf == VSC73XX_VLAN_IGNORE) {
> +		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);

IMO this, and all the other places that write the same registers from 2
branches, would look less clutered like this:

	u32 val;

	val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
	      VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;

	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
			    VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);

	val = ...

Otherwise, the larger the branches become, the harder it gets to see
what's common and what's different.

> +		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);
> +	}
> +}
> +
> +static int
> +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
> +{
> +	u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
> +
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> +			    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
> +	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_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)

Would it simplify callers to add a "bool operate_on_storage" argument here,
and absorb that logic? The callers could look like this, notice how there is
less code duplication between the "if" and "else" branches.

static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
				 bool tag_8021q_vlan)
{
	struct dsa_port *dp = dsa_to_port(ds, port);
	struct vsc73xx *vsc = ds->priv;
	bool operate_on_storage;
	u16 existing_pvid;
	bool had_pvid;

	operate_on_storage = vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan;

	had_pvid = vsc73xx_port_get_pvid(vsc, port, &existing_pvid,
					 &operate_on_storage);
	if (had_pvid && existing_pvid != vid) {
		dev_warn(vsc->dev,
			 "Port %d can have only one pvid! Now is: %d.\n",
			 port, existing_pvid);
		return -EOPNOTSUPP;
	}

	return vsc73xx_vlan_change_pvid(vsc, port, vid, true, operate_on_storage);
}

> +{
> +	u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
> +
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> +			    VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> +
> +	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_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid)

I believe it would look better if this would return bool, and if it had
a shorter name (vsc73xx_port_get_pvid).

> +{
> +	u32 val;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
> +	if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
> +		return 0;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> +	*vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> +
> +	return VSC73XX_IS_CONFIGURED;
> +}
> +
> +static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid)
> +{
> +	u32 val;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> +	if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
> +		return 0;
> +
> +	vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> +		     &val);

Don't need to read the same register twice.

> +	*vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> +		VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> +
> +	return VSC73XX_IS_CONFIGURED;
> +}
> +
> +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 = 0;
> +
> +	if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) {

I think it would be valuable for readability to create a helper function named:

static bool vsc73xx_tag_8021q_active(struct dsa_port *dp)
{
	return !dsa_port_is_vlan_filtering(dp);
}

and then, to rename "port_vlan" to "!tag_8021q_vlan". It would make it
clearer as to what the checks are about.

	if (vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan) {
		// update cached storage
	} else {
		// update hardware
	}

> +		if (vsc->untagged_storage[port] < VLAN_N_VID &&
> +		    !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> +		    !vid_is_dsa_8021q(vid) &&

The problem here which led to these vid_is_dsa_8021q() checks is that
dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
those, correct? But when the port is VSC73XX_VLAN_IGNORE mode (and
tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
*all* VLANs are egress-untagged VLANs, correct?

If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
first place, for tag_8021q VLANs, if you don't rely on the port's native
VLAN for egress untagging?

> +		    vsc->untagged_storage[port] != vid) {
> +			dev_warn(vsc->dev,
> +				 "Port %d can have only one untagged vid! Now is: %d.\n",
> +				 port, vsc->untagged_storage[port]);
> +			return -EOPNOTSUPP;
> +		}
> +		vsc->untagged_storage[port] = vid;
> +	} else {
> +		if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> +		    !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 -EOPNOTSUPP;
> +		}
> +		return vsc73xx_vlan_change_untagged(vsc, port, vid, 1);
> +	}
> +
> +	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;
> +
> +	if (!dsa_port)
> +		return -EINVAL;

Why is this check here?

> +
> +	if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) {
> +		if (vsc->pvid_storage[port] < VLAN_N_VID &&
> +		    !vid_is_dsa_8021q(vsc->pvid_storage[port]) &&
> +		    !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) {

What are the vid_is_dsa_8021q() checks for?

> +			dev_warn(vsc->dev,
> +				 "Port %d can have only one pvid! Now is: %d.\n",
> +				 port, vsc->pvid_storage[port]);
> +			return -EOPNOTSUPP;
> +		}
> +		vsc->pvid_storage[port] = vid;
> +	} else {
> +		if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> +		    !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 -EOPNOTSUPP;
> +		}
> +		return vsc73xx_vlan_change_pvid(vsc, port, vid, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> +			    bool vlan_filtering, struct netlink_ext_ack *extack)

A comment would be good which states that the flipping between the
hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
only gets called on actual changes to vlan_filtering, and thus, to
vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
needs to go to hardware, and what is in hardware needs to go to storage.

It's an interesting implementation for sure.

> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	bool store_untagged  = false;
> +	bool store_pvid  = false;

nit: replace the 2 spaces before the "=" with a single one.

> +	u16 vlan_no;
> +
> +	if (vlan_filtering)
> +		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> +	else
> +		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> +
> +	if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> +		store_pvid = true;
> +
> +	if (vsc->pvid_storage[port] < VLAN_N_VID)
> +		vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true);
> +	else
> +		vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> +
> +	vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID;
> +
> +	if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> +		store_untagged  = true;
> +
> +	if (vsc->untagged_storage[port] < VLAN_N_VID)
> +		vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true);
> +	else
> +		vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> +
> +	vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_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;
> +	struct vsc73xx *vsc = ds->priv;
> +	u16 vlan_no;
> +	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 (port != CPU_PORT) {
> +		if (untagged) {
> +			ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> +			if (ret)
> +				return ret;
> +		} else {
> +			if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no)
> +			    == VSC73XX_IS_CONFIGURED &&
> +			    vlan_no == vlan->vid)
> +				vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> +			else if (vsc->untagged_storage[port] == vlan->vid)
> +				vsc->untagged_storage[port] = VLAN_N_VID;
> +		}
> +		if (pvid) {
> +			ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> +			if (ret)
> +				return ret;
> +		} else {
> +			if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no)
> +			    == VSC73XX_IS_CONFIGURED &&
> +			    vlan_no == vlan->vid)
> +				vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> +			else if (vsc->pvid_storage[port] == vlan->vid)
> +				vsc->pvid_storage[port] = VLAN_N_VID;
> +		}
> +	}
> +
> +	return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1);

last argument is bool, so pass true or false (here and everywhere else)

> +}
> +
> +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;
> +
> +	ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> +	    vlan_no == vlan->vid)
> +		vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> +	else if (vsc->untagged_storage[port] == vlan->vid)
> +		vsc->untagged_storage[port] = VLAN_N_VID;
> +
> +	if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> +	    vlan_no == vlan->vid)
> +		vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> +	else if (vsc->pvid_storage[port] == vlan->vid)
> +		vsc->pvid_storage[port] = VLAN_N_VID;
> +
> +	return 0;
> +}
> +
>  static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
>  {
> +	struct vsc73xx *vsc = ds->priv;
> +	int ret, i;
> +
> +	/* Those bits are responsible for MTU only. Kernel take care about MTU,
> +	 * let's enable +8 bytes frame length unconditionally.
> +	 */
> +	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> +			    VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR,
> +			    VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR);
> +
> +	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);
> +	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);
> +
> +	if (port == CPU_PORT)
> +		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> +	else
> +		vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> +
> +	for (i = 0; i <= VLAN_N_VID; i++) {

VLAN_N_VID is 4096, so i <= VLAN_N_VID must be a mistake?

> +		ret = vsc73xx_update_vlan_table(vsc, port, i, 0);
> +		if (ret)
> +			return ret;
> +	}

Also, could you write an expedited version of this, which is not called
from ds->ops->port_setup() but from ds->ops->setup()? It is wasteful to
traverse the VLAN table for each port, when we know from the get go that
we need to clear all ports.

> +
>  	/* Configure forward map to CPU <-> port only */
>  	if (port == CPU_PORT)
>  		vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> @@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
>  		vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
>  					 BIT(CPU_PORT);
>  
> +	/* Set storage values out of range*/

Nit: Space after "*/" (there are a few other places left which have
this style issue)

> +	vsc->pvid_storage[port] = VLAN_N_VID;
> +	vsc->untagged_storage[port] = VLAN_N_VID;
> +
>  	return 0;
>  }
>  
> @@ -1098,6 +1516,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,
>  };
>  
>  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 224e284a5573..b7614dd7d0eb 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -30,6 +30,8 @@ struct vsc73xx {
>  	const struct vsc73xx_ops	*ops;
>  	void				*priv;
>  	u8				forward_map[VSC73XX_MAX_NUM_PORTS];
> +	u16				pvid_storage[VSC73XX_MAX_NUM_PORTS];
> +	u16				untagged_storage[VSC73XX_MAX_NUM_PORTS];
>  };
>  
>  struct vsc73xx_ops {
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK

[Russell King (Oracle)]

On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> +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;

I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
is this correct, or should it be:

	if (phy_interface_is_rgmii(interface))

since the various RGMII* modes are used to determine the delay on the
PHY side.

Even so, I don't think that is a matter for this patch, but a future
(or maybe a preceeding patch) to address.

Other than that, I think it looks okay.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx

[Vladimir Oltean]

Hi Pawel,

On Tue, Sep 12, 2023 at 02:22:00PM +0200, Pawel Dembicki wrote:
> This commit introduces a new tagger based on 802.1q tagging.
> It's designed for the vsc73xx driver. The VSC73xx family doesn't have
> any tag support for the RGMII port, but it could be based on VLANs.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
> new file mode 100644
> index 000000000000..9093a71e6eb0
> --- /dev/null
> +++ b/net/dsa/tag_vsc73xx_8021q.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>

2022-2023 by now, maybe?

> + * 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);
> +	u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
> +	u8 pcp;
> +
> +	if (skb->offload_fwd_mark) {
> +		unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> +		struct net_device *br = dsa_port_bridge_dev_get(dp);
> +
> +		if (br_vlan_enabled(br))
> +			return skb;
> +
> +		tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
> +	}
> +
> +	pcp = netdev_txq_to_tc(netdev, queue_mapping);
> +
> +	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);
> +	} else {
> +		netdev_warn(netdev, "Couldn't decode source port\n");
> +		return NULL;
> +	}
> +
> +	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);

Hmm, this isn't looking too good.

I think the fact that you had to add my copyright on what should be such
a simple thing as a VLAN-based tagger is a bad sign :)

It's time to consolidate some more stuff that currently lives in
tag_sja1105 and move it into tag_8021q so that you can reuse it more
easily.

I've prepared some (only compile-tested) patches on this branch here:
https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3

I need to double-check that they don't introduce regressions, and we
should discuss the merging strategy. Probably you're going to submit
them together with your patch set.

With that, you can drop my part of the copyright :) The remainder should
look like straightforward use of API which can be written in only a
limited number of ways.

> +	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);

Why do you need to do this? We send VLAN-tagged packets to the
VLAN-aware bridge intentionally, so that it knows what VLAN they come
from (in the dsa_find_designated_bridge_port_by_vid() case). So don't
strip it if it's not causing a problem.

> +
> +	return skb;
> +}

Re: [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 02:22:02PM +0200, Pawel Dembicki wrote:
> 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.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v3:
>   - All vlan commits was reworked
>   - move VLAN_AWR and VLAN_DBLAWR to port setup in other commit
>   - drop vlan table upgrade
> v2:
>   - no changes done
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 72 ++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index bf903502bac1..9d0367c2c52c 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -600,6 +600,9 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>  
>  	dev_info(vsc->dev, "set up the switch\n");
>  
> +	ds->untag_bridge_pvid = true;
> +	ds->max_num_bridges = 7;

Can you please refactor this into DSA_TAG_8021Q_MAX_NUM_BRIDGES and use
it in sja1105 too?

> +
>  	/* Issue RESET */
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
>  		      VSC73XX_GLORESET_MASTER_RESET);
> @@ -1456,6 +1459,73 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
>  	return vsc73xx_update_vlan_table(vsc, port, vid, 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]);
> +	}
> +}

I suspect you'll have to rethink this. If you look at del_nbp() and dsa_port_bridge_leave(),
you'll see it goes through a few phases. First the bridge calls br_stp_disable_port(p),
then netdev_upper_dev_unlink(dev, br->dev) which invokes dsa_port_bridge_leave().
So at this stage, the port is in BR_STATE_DISABLED and ds->ops->port_stp_state_set()
duly notifies the driver of that.

Then, ds->ops->port_bridge_leave() gets called, and then, ds->ops->port_stp_state_set()
again, for the standalone port, in BR_STATE_FORWARDING.

So actually, the last to take effect upon the forwarding map is port_stp_state_set(),
not port_bridge_leave().

I suspect you can remove the vsc73xx_update_forwarding_map() and just
rely on the STP implementation, and fix that while you're at it.

On the other switch ports except the one on which the STP state is changing,
you can look at dp->stp_state. There needs to be an "administrative" forwarding
mask (determined by port_bridge_join and port_bridge_leave), and an "operational"
one (determined by STP states).

> +
> +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;

	dsa_switch_for_each_user_port(other_dp, ds) please

it is a lower-complexity iteration over the port list, which should be
preferred over a for loop + any dsa_to_port() wrapper like dsa_is_user_port().

> +
> +		if (i == port)
> +			continue;
> +
> +		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))

and "other_dp" here

> +			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_port_setup(struct dsa_switch *ds, int port)
>  {
>  	struct vsc73xx *vsc = ds->priv;
> @@ -1557,6 +1627,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
> 

Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Paweł Dembicki]

Hi Vladimir,

wt., 12 wrz 2023 o 18:17 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Tue, Sep 12, 2023 at 02:21:59PM +0200, Pawel Dembicki wrote:
> > This patch implements VLAN filtering for the vsc73xx driver.
> >
> > After starting VLAN filtering, the switch is reconfigured from QinQ to
> > a simple VLAN aware mode. This is required because VSC73XX chips do not
> > support inner VLAN tag filtering.
> >
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > v3:
> >   - reworked all vlan commits
> >   - added storage variables for pvid and untagged vlans
> >   - move length extender settings to port setup
> >   - remove vlan table cleaning in wrong places
> >   - note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged'
> >     and 'vsc73xx_vlan_set_pvid' are used later in tag implementation
> > v2:
> >   - no changes done
> >
> >  drivers/net/dsa/vitesse-vsc73xx-core.c | 425 ++++++++++++++++++++++++-
> >  drivers/net/dsa/vitesse-vsc73xx.h      |   2 +
> >  2 files changed, 425 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 541fbc195df1..d9a6eac1fcce 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -21,14 +21,18 @@
> >  #include <linux/of_mdio.h>
> >  #include <linux/bitops.h>
> >  #include <linux/if_bridge.h>
> > +#include <linux/if_vlan.h>
> >  #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>
> >
> >  #include "vitesse-vsc73xx.h"
> >
> > +#define VSC73XX_IS_CONFIGURED        0x1
> > +
> >  #define VSC73XX_BLOCK_MAC    0x1 /* Subblocks 0-4, 6 (CPU port) */
> >  #define VSC73XX_BLOCK_ANALYZER       0x2 /* Only subblock 0 */
> >  #define VSC73XX_BLOCK_MII    0x3 /* Subblocks 0 and 1 */
> > @@ -61,6 +65,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 */
> > @@ -121,6 +127,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)
> > @@ -134,6 +151,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
> > @@ -188,7 +214,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
> > @@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
> >       { 29, "TxQoSClass3" }, /* non-standard counter */
> >  };
> >
> > +enum vsc73xx_port_vlan_conf {
> > +     VSC73XX_VLAN_FILTER,
> > +     VSC73XX_VLAN_IGNORE,
> > +};
> > +
> >  int vsc73xx_is_addr_valid(u8 block, u8 subblock)
> >  {
> >       switch (block) {
> > @@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds,
> >  static int vsc73xx_setup(struct dsa_switch *ds)
> >  {
> >       struct vsc73xx *vsc = ds->priv;
> > -     int i;
> > +     int i, ret;
>
> ../drivers/net/dsa/vitesse-vsc73xx-core.c:598:9: warning: unused variable 'ret' [-Wunused-variable]
>         int i, ret;
>                ^
>
> >
> >       dev_info(vsc->dev, "set up the switch\n");
> >
> > @@ -623,6 +655,9 @@ 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);
> > +     /* 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);
> > @@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> >       return 9600 - ETH_HLEN - ETH_FCS_LEN;
> >  }
> >
> > +static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc)
> > +{
> > +     int ret, err;
> > +     u32 val;
> > +
> > +     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_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap)
> > +{
> > +     u32 val;
> > +     int ret;
> > +
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> > +     ret = vsc73xx_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_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_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap)
> > +{
> > +     int ret;
> > +
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid);
> > +     ret = vsc73xx_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));
> > +
> > +     return vsc73xx_wait_for_vlan_table_cmd(vsc);
> > +}
> > +
> > +static int
> > +vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set)
> > +{
> > +     u8 portmap;
> > +     int ret;
> > +
> > +     ret = vsc73xx_read_vlan_table_entry(vsc, 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;
>
> Why did you need to do this? To my knowledge, the DSA framework code
> will decide when to remove VLANs from the CPU port.
>

I will fix it.

> > +
> > +     return  vsc73xx_write_vlan_table_entry(vsc, vid, portmap);
> > +}
> > +
> > +static void
> > +vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
> > +                   enum vsc73xx_port_vlan_conf port_vlan_conf)
> > +{
> > +     if (port_vlan_conf == VSC73XX_VLAN_IGNORE) {
> > +             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);
>
> IMO this, and all the other places that write the same registers from 2
> branches, would look less clutered like this:
>
>         u32 val;
>
>         val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ?
>               VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0;
>
>         vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC,
>                             VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
>
>         val = ...
>
> Otherwise, the larger the branches become, the harder it gets to see
> what's common and what's different.
>
> > +             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);
> > +     }
> > +}
> > +
> > +static int
> > +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
> > +{
> > +     u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0;
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > +                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val);
> > +     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_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)
>
> Would it simplify callers to add a "bool operate_on_storage" argument here,
> and absorb that logic? The callers could look like this, notice how there is
> less code duplication between the "if" and "else" branches.
>

I will rework it.

> static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
>                                  bool tag_8021q_vlan)
> {
>         struct dsa_port *dp = dsa_to_port(ds, port);
>         struct vsc73xx *vsc = ds->priv;
>         bool operate_on_storage;
>         u16 existing_pvid;
>         bool had_pvid;
>
>         operate_on_storage = vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan;
>
>         had_pvid = vsc73xx_port_get_pvid(vsc, port, &existing_pvid,
>                                          &operate_on_storage);
>         if (had_pvid && existing_pvid != vid) {
>                 dev_warn(vsc->dev,
>                          "Port %d can have only one pvid! Now is: %d.\n",
>                          port, existing_pvid);
>                 return -EOPNOTSUPP;
>         }
>
>         return vsc73xx_vlan_change_pvid(vsc, port, vid, true, operate_on_storage);
> }
>
> > +{
> > +     u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP,
> > +                         VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> > +
> > +     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_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid)
>
> I believe it would look better if this would return bool, and if it had
> a shorter name (vsc73xx_port_get_pvid).
>
> > +{
> > +     u32 val;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val);
> > +     if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA)
> > +             return 0;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> > +     *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> > +
> > +     return VSC73XX_IS_CONFIGURED;
> > +}
> > +
> > +static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid)
> > +{
> > +     u32 val;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> > +     if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA))
> > +             return 0;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG,
> > +                  &val);
>
> Don't need to read the same register twice.
>
> > +     *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> > +             VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> > +
> > +     return VSC73XX_IS_CONFIGURED;
> > +}
> > +
> > +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 = 0;
> > +
> > +     if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) {
>
> I think it would be valuable for readability to create a helper function named:
>
> static bool vsc73xx_tag_8021q_active(struct dsa_port *dp)
> {
>         return !dsa_port_is_vlan_filtering(dp);
> }
>
> and then, to rename "port_vlan" to "!tag_8021q_vlan". It would make it
> clearer as to what the checks are about.
>
>         if (vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan) {
>                 // update cached storage
>         } else {
>                 // update hardware
>         }
>
> > +             if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > +                 !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > +                 !vid_is_dsa_8021q(vid) &&
>
> The problem here which led to these vid_is_dsa_8021q() checks is that
> dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> those, correct?

In my case, the major problem with tag8021q vlans is
"dsa_tag_8021q_bridge_join" function:
"dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
I must disable pvid/untagged checking, because it will always fail. I
let kernel do the job,
it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".

> But when the port is VSC73XX_VLAN_IGNORE mode (and
> tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> *all* VLANs are egress-untagged VLANs, correct?
>
> If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> first place, for tag_8021q VLANs, if you don't rely on the port's native
> VLAN for egress untagging?
>
> > +                 vsc->untagged_storage[port] != vid) {
> > +                     dev_warn(vsc->dev,
> > +                              "Port %d can have only one untagged vid! Now is: %d.\n",
> > +                              port, vsc->untagged_storage[port]);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +             vsc->untagged_storage[port] = vid;
> > +     } else {
> > +             if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > +                 !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 -EOPNOTSUPP;
> > +             }
> > +             return vsc73xx_vlan_change_untagged(vsc, port, vid, 1);
> > +     }
> > +
> > +     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;
> > +
> > +     if (!dsa_port)
> > +             return -EINVAL;
>
> Why is this check here?
>

Unnecessary defensive programming.

> > +
> > +     if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) {
> > +             if (vsc->pvid_storage[port] < VLAN_N_VID &&
> > +                 !vid_is_dsa_8021q(vsc->pvid_storage[port]) &&
> > +                 !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) {
>
> What are the vid_is_dsa_8021q() checks for?

The same reason as the "untagged" case.

>
> > +                     dev_warn(vsc->dev,
> > +                              "Port %d can have only one pvid! Now is: %d.\n",
> > +                              port, vsc->pvid_storage[port]);
> > +                     return -EOPNOTSUPP;
> > +             }
> > +             vsc->pvid_storage[port] = vid;
> > +     } else {
> > +             if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > +                 !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 -EOPNOTSUPP;
> > +             }
> > +             return vsc73xx_vlan_change_pvid(vsc, port, vid, 1);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> > +                         bool vlan_filtering, struct netlink_ext_ack *extack)
>
> A comment would be good which states that the flipping between the
> hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> only gets called on actual changes to vlan_filtering, and thus, to
> vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> needs to go to hardware, and what is in hardware needs to go to storage.
>
> It's an interesting implementation for sure.
>

Thank you.

> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     bool store_untagged  = false;
> > +     bool store_pvid  = false;
>
> nit: replace the 2 spaces before the "=" with a single one.
>
> > +     u16 vlan_no;
> > +
> > +     if (vlan_filtering)
> > +             vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> > +     else
> > +             vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> > +
> > +     if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> > +             store_pvid = true;
> > +
> > +     if (vsc->pvid_storage[port] < VLAN_N_VID)
> > +             vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true);
> > +     else
> > +             vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> > +
> > +     vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID;
> > +
> > +     if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED)
> > +             store_untagged  = true;
> > +
> > +     if (vsc->untagged_storage[port] < VLAN_N_VID)
> > +             vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true);
> > +     else
> > +             vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> > +
> > +     vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_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;
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u16 vlan_no;
> > +     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 (port != CPU_PORT) {
> > +             if (untagged) {
> > +                     ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> > +                     if (ret)
> > +                             return ret;
> > +             } else {
> > +                     if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no)
> > +                         == VSC73XX_IS_CONFIGURED &&
> > +                         vlan_no == vlan->vid)
> > +                             vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> > +                     else if (vsc->untagged_storage[port] == vlan->vid)
> > +                             vsc->untagged_storage[port] = VLAN_N_VID;
> > +             }
> > +             if (pvid) {
> > +                     ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> > +                     if (ret)
> > +                             return ret;
> > +             } else {
> > +                     if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no)
> > +                         == VSC73XX_IS_CONFIGURED &&
> > +                         vlan_no == vlan->vid)
> > +                             vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> > +                     else if (vsc->pvid_storage[port] == vlan->vid)
> > +                             vsc->pvid_storage[port] = VLAN_N_VID;
> > +             }
> > +     }
> > +
> > +     return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1);
>
> last argument is bool, so pass true or false (here and everywhere else)
>
> > +}
> > +
> > +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;
> > +
> > +     ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > +         vlan_no == vlan->vid)
> > +             vsc73xx_vlan_change_untagged(vsc, port, 0, false);
> > +     else if (vsc->untagged_storage[port] == vlan->vid)
> > +             vsc->untagged_storage[port] = VLAN_N_VID;
> > +
> > +     if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED &&
> > +         vlan_no == vlan->vid)
> > +             vsc73xx_vlan_change_pvid(vsc, port, 0, false);
> > +     else if (vsc->pvid_storage[port] == vlan->vid)
> > +             vsc->pvid_storage[port] = VLAN_N_VID;
> > +
> > +     return 0;
> > +}
> > +
> >  static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> >  {
> > +     struct vsc73xx *vsc = ds->priv;
> > +     int ret, i;
> > +
> > +     /* Those bits are responsible for MTU only. Kernel take care about MTU,
> > +      * let's enable +8 bytes frame length unconditionally.
> > +      */
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
> > +                         VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR,
> > +                         VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR);
> > +
> > +     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);
> > +     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);
> > +
> > +     if (port == CPU_PORT)
> > +             vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER);
> > +     else
> > +             vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE);
> > +
> > +     for (i = 0; i <= VLAN_N_VID; i++) {
>
> VLAN_N_VID is 4096, so i <= VLAN_N_VID must be a mistake?
>

Yeep.

> > +             ret = vsc73xx_update_vlan_table(vsc, port, i, 0);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> Also, could you write an expedited version of this, which is not called
> from ds->ops->port_setup() but from ds->ops->setup()? It is wasteful to
> traverse the VLAN table for each port, when we know from the get go that
> we need to clear all ports.
>

I'll rework it in setup.

> > +
> >       /* Configure forward map to CPU <-> port only */
> >       if (port == CPU_PORT)
> >               vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK &
> > @@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> >               vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK &
> >                                        BIT(CPU_PORT);
> >
> > +     /* Set storage values out of range*/
>
> Nit: Space after "*/" (there are a few other places left which have
> this style issue)
>
> > +     vsc->pvid_storage[port] = VLAN_N_VID;
> > +     vsc->untagged_storage[port] = VLAN_N_VID;
> > +
> >       return 0;
> >  }
> >
> > @@ -1098,6 +1516,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,
> >  };
> >
> >  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 224e284a5573..b7614dd7d0eb 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > @@ -30,6 +30,8 @@ struct vsc73xx {
> >       const struct vsc73xx_ops        *ops;
> >       void                            *priv;
> >       u8                              forward_map[VSC73XX_MAX_NUM_PORTS];
> > +     u16                             pvid_storage[VSC73XX_MAX_NUM_PORTS];
> > +     u16                             untagged_storage[VSC73XX_MAX_NUM_PORTS];
> >  };
> >
> >  struct vsc73xx_ops {
> > --
> > 2.34.1
> >
>

Re: [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx

[Paweł Dembicki]

wt., 12 wrz 2023 o 23:39 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> Hi Pawel,

Hi Vladimir,

>
> On Tue, Sep 12, 2023 at 02:22:00PM +0200, Pawel Dembicki wrote:
> > This commit introduces a new tagger based on 802.1q tagging.
> > It's designed for the vsc73xx driver. The VSC73xx family doesn't have
> > any tag support for the RGMII port, but it could be based on VLANs.
> >
> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> > ---
> > diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
> > new file mode 100644
> > index 000000000000..9093a71e6eb0
> > --- /dev/null
> > +++ b/net/dsa/tag_vsc73xx_8021q.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>
>
> 2022-2023 by now, maybe?
>
> > + * 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);
> > +     u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
> > +     u8 pcp;
> > +
> > +     if (skb->offload_fwd_mark) {
> > +             unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> > +             struct net_device *br = dsa_port_bridge_dev_get(dp);
> > +
> > +             if (br_vlan_enabled(br))
> > +                     return skb;
> > +
> > +             tx_vid = dsa_tag_8021q_bridge_vid(bridge_num);
> > +     }
> > +
> > +     pcp = netdev_txq_to_tc(netdev, queue_mapping);
> > +
> > +     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);
> > +     } else {
> > +             netdev_warn(netdev, "Couldn't decode source port\n");
> > +             return NULL;
> > +     }
> > +
> > +     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);
>
> Hmm, this isn't looking too good.
>
> I think the fact that you had to add my copyright on what should be such
> a simple thing as a VLAN-based tagger is a bad sign :)
>
> It's time to consolidate some more stuff that currently lives in
> tag_sja1105 and move it into tag_8021q so that you can reuse it more
> easily.
>
> I've prepared some (only compile-tested) patches on this branch here:
> https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3
>
> I need to double-check that they don't introduce regressions,

I tested it on my device and I couldn't find any regressions. vlan
filtering and tagging work as expected.

> and we
> should discuss the merging strategy. Probably you're going to submit
> them together with your patch set.

I prepared the v4 patch series. Please look if that format is ok with you.
https://github.com/CHKDSK88/linux/commits/vsc73xx-vlan-net-next

>
> With that, you can drop my part of the copyright :) The remainder should
> look like straightforward use of API which can be written in only a
> limited number of ways.

Now it is much simpler.

>
> > +     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);
>
> Why do you need to do this? We send VLAN-tagged packets to the
> VLAN-aware bridge intentionally, so that it knows what VLAN they come
> from (in the dsa_find_designated_bridge_port_by_vid() case). So don't
> strip it if it's not causing a problem.
>

I dropped it in v4. I needed it when I started with this patch series,
because bridge in vlan filtering causes double tagged frames (one from
hardware and one from bridge). But after recent changes it is useless
and it could be dropped because vlan works as expected without it.

> > +
> > +     return skb;
> > +}

Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 05:49:36PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> > +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;
> 
> I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
> is this correct, or should it be:
> 
> 	if (phy_interface_is_rgmii(interface))
> 
> since the various RGMII* modes are used to determine the delay on the
> PHY side.
> 
> Even so, I don't think that is a matter for this patch, but a future
> (or maybe a preceeding patch) to address.
> 
> Other than that, I think it looks okay.
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

I also agree with adding one more patch to this which converts to
phy_interface_is_rgmii(). Paweł: there was a recent discussion about
the (ir)relevance of the specific rgmii phy-mode in fixed-link here.
https://lore.kernel.org/netdev/ZNpEaMJjmDqhK1dW@shell.armlinux.org.uk/

Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote:
> > > +             if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > > +                 !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > > +                 !vid_is_dsa_8021q(vid) &&
> >
> > The problem here which led to these vid_is_dsa_8021q() checks is that
> > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> > those, correct?
> 
> In my case, the major problem with tag8021q vlans is
> "dsa_tag_8021q_bridge_join" function:
> "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
> I must disable pvid/untagged checking, because it will always fail. I
> let kernel do the job,
> it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".

I'm not sure that you described the problem in a way that I can understand, here.

After dsa_tag_8021q_bridge_join():
-> dsa_port_tag_8021q_vlan_add(dp, bridge_vid)
-> dsa_port_tag_8021q_vlan_del(dp, standalone_vid)

it's *expected* that there should be only one untagged/pvid per port: the bridge_vid.

For context, consider the fact that you can run the following commands:

bridge vlan add dev eth0 vid 10 pvid
bridge vlan add dev eth0 vid 11 pvid

and after the second command, vid 10 stops being a pvid.

So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior
is not correct. You need to implement the pvid overwriting behavior, since
there's always only 1 pvid.

So that leaves the "untagged" flag as being problematic, correct? Could
you comment...

> 
> > But when the port is VSC73XX_VLAN_IGNORE mode (and
> > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> > *all* VLANs are egress-untagged VLANs, correct?
> >
> > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> > first place, for tag_8021q VLANs, if you don't rely on the port's native
> > VLAN for egress untagging?

... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag"
is a configuration that can actually be supported by vsc73xx. You just
need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q
basically requests exactly that configuration on user ports (both the
bridge_vid and the standalone_vid are egress-untagged). So your check is
too restrictive, you are denying a configuration that would work.
The problem only appears when you mix egress-tagged with egress-untagged
VLANs on a port. Only then there can be at most 1 egress-untagged VID,
because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the
egress-tagged VIDs to work.

> > A comment would be good which states that the flipping between the
> > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> > only gets called on actual changes to vlan_filtering, and thus, to
> > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> > needs to go to hardware, and what is in hardware needs to go to storage.
> >
> > It's an interesting implementation for sure.
> >
> 
> Thank you.

I'm not sure if that was a compliment :) At least in this form, it's
certainly non-trivial to determine by looking at the code if it is
correct or not, and it uses different patterns than the other VLAN
implementations in DSA drivers. Generally, boring and obvious is
preferable. But after I took the time to understand, it seems plausible
that the approach might work.

Let's see how the same idea looks, cleaned up a bit but not redesigned,
in v4.

Re: [PATCH net-next v3 6/8] net: dsa: vsc73xx: introduce tag 8021q for vsc73xx

[Vladimir Oltean]

On Mon, Sep 25, 2023 at 10:55:29PM +0200, Paweł Dembicki wrote:
> > I've prepared some (only compile-tested) patches on this branch here:
> > https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3
> >
> > I need to double-check that they don't introduce regressions,
> 
> I tested it on my device and I couldn't find any regressions. vlan
> filtering and tagging work as expected.

Thanks for testing. Obviously I still haven't :-/ Please feel free to
post the next version and I'll try to find some time to test.

> > and we
> > should discuss the merging strategy. Probably you're going to submit
> > them together with your patch set.
> 
> I prepared the v4 patch series. Please look if that format is ok with you.
> https://github.com/CHKDSK88/linux/commits/vsc73xx-vlan-net-next

I still have some style nitpicks similar to the ones expressed already,
but it isn't trivial for me to leave them through Github, so you can
post your best attempt at v4. You should also try to add a comment
explaining what's the reason for the special case for "if (port !=
CPU_PORT)" in vsc73xx_port_vlan_add().

Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK

[Paweł Dembicki]

śr., 27 wrz 2023 o 01:03 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Tue, Sep 12, 2023 at 05:49:36PM +0100, Russell King (Oracle) wrote:
> > On Tue, Sep 12, 2023 at 02:21:56PM +0200, Pawel Dembicki wrote:
> > > +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;
> >
> > I know the original code tested against PHY_INTERFACE_MODE_RGMII, but
> > is this correct, or should it be:
> >
> >       if (phy_interface_is_rgmii(interface))
> >
> > since the various RGMII* modes are used to determine the delay on the
> > PHY side.
> >
> > Even so, I don't think that is a matter for this patch, but a future
> > (or maybe a preceeding patch) to address.
> >
> > Other than that, I think it looks okay.
> >
> > Thanks.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>
> I also agree with adding one more patch to this which converts to
> phy_interface_is_rgmii(). Paweł: there was a recent discussion about
> the (ir)relevance of the specific rgmii phy-mode in fixed-link here.
> https://lore.kernel.org/netdev/ZNpEaMJjmDqhK1dW@shell.armlinux.org.uk/

I plan to make rgmii delays configurable from the device tree. Should I?
a. switch to phy_interface_is_rgmii in the current patch?
b. add another patch in this series?
c. wait with change to phy_interface_is_rgmii for patch with rgmii
delays configuration?

Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK

[Vladimir Oltean]

Hi Paweł,

On Tue, Oct 03, 2023 at 10:45:45PM +0200, Paweł Dembicki wrote:
> I plan to make rgmii delays configurable from the device tree. Should I?
> a. switch to phy_interface_is_rgmii in the current patch?
> b. add another patch in this series?
> c. wait with change to phy_interface_is_rgmii for patch with rgmii
> delays configuration?

If you want to configure the RGMII delays in the vsc73xx MAC, you should
look at the "rx-internal-delay-ps" and "tx-internal-delay-ps" properties
in the MAC OF node, rather than at the phy-mode. The phy-mode is for the
internal delays from the PHY.

In any case, you should accept any phy_interface_is_rgmii() regardless
of whether internal delays are configurable in the MAC. And yes, parsing
those MAC OF properties should be a separate change.

If the series exceeds 15 patches, I would consider splitting it per
topic and submitting separate series (link management would be one,
tag_8021q/bridging/VLAN would be another). If they don't conflict and
can be applied independently, you could also send the 2 series
simultaneously.

Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Paweł Dembicki]

śr., 27 wrz 2023 o 01:58 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote:
> > > > +             if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > > > +                 !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > > > +                 !vid_is_dsa_8021q(vid) &&
> > >
> > > The problem here which led to these vid_is_dsa_8021q() checks is that
> > > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> > > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> > > those, correct?
> >
> > In my case, the major problem with tag8021q vlans is
> > "dsa_tag_8021q_bridge_join" function:
> > "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
> > I must disable pvid/untagged checking, because it will always fail. I
> > let kernel do the job,
> > it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".
>
> I'm not sure that you described the problem in a way that I can understand, here.
>
> After dsa_tag_8021q_bridge_join():
> -> dsa_port_tag_8021q_vlan_add(dp, bridge_vid)
> -> dsa_port_tag_8021q_vlan_del(dp, standalone_vid)
>
> it's *expected* that there should be only one untagged/pvid per port: the bridge_vid.
>
> For context, consider the fact that you can run the following commands:
>
> bridge vlan add dev eth0 vid 10 pvid
> bridge vlan add dev eth0 vid 11 pvid
>
> and after the second command, vid 10 stops being a pvid.
>
> So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior
> is not correct. You need to implement the pvid overwriting behavior, since
> there's always only 1 pvid.
>

Yes, overwriting pvid is the only proper way. Kernel mechanism will
take care about the number of pvids. I will fixit in v4.

> So that leaves the "untagged" flag as being problematic, correct? Could
> you comment...
>
> >
> > > But when the port is VSC73XX_VLAN_IGNORE mode (and
> > > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> > > *all* VLANs are egress-untagged VLANs, correct?
> > >
> > > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> > > first place, for tag_8021q VLANs, if you don't rely on the port's native
> > > VLAN for egress untagging?
>
> ... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag"
> is a configuration that can actually be supported by vsc73xx. You just
> need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q
> basically requests exactly that configuration on user ports (both the
> bridge_vid and the standalone_vid are egress-untagged). So your check is
> too restrictive, you are denying a configuration that would work.
> The problem only appears when you mix egress-tagged with egress-untagged
> VLANs on a port. Only then there can be at most 1 egress-untagged VID,
> because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the
> egress-tagged VIDs to work.

Should I make a local copy of the quantity of egress untagged and
tagged vlans per port to resolve this issue, shouldn't I?
And then I check how many vlans are egress tagged or untagged for a
properly restricted solution?

I see another problem. Even if I return an error value, the untagged
will be marked in 'bridge vlan' listing. I'm not sure how it should
work in this case.

>
> > > A comment would be good which states that the flipping between the
> > > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> > > only gets called on actual changes to vlan_filtering, and thus, to
> > > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> > > needs to go to hardware, and what is in hardware needs to go to storage.
> > >
> > > It's an interesting implementation for sure.
> > >
> >
> > Thank you.
>
> I'm not sure if that was a compliment :)

Touché. :)

>At least in this form, it's
> certainly non-trivial to determine by looking at the code if it is
> correct or not, and it uses different patterns than the other VLAN
> implementations in DSA drivers. Generally, boring and obvious is
> preferable. But after I took the time to understand, it seems plausible
> that the approach might work.
>
> Let's see how the same idea looks, cleaned up a bit but not redesigned,
> in v4.

I try to at least clean pvid and untagged issues before v4.

Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Tue, Oct 03, 2023 at 11:14:55PM +0200, Paweł Dembicki wrote:
> Should I make a local copy of the quantity of egress untagged and
> tagged vlans per port to resolve this issue, shouldn't I?
> And then I check how many vlans are egress tagged or untagged for a
> properly restricted solution?

Yeah, I guess so. It is the same problem that ocelot_vlan_prepare()
tries to solve, and that counts ocelot_port_num_untagged_vlans() and
ocelot_port_num_tagged_vlans() indeed.

> I see another problem. Even if I return an error value, the untagged
> will be marked in 'bridge vlan' listing. I'm not sure how it should
> work in this case.

What error code are you returning, -EOPNOTSUPP I guess? That's
deliberately ignored by callers of br_switchdev_port_vlan_add(), because
it means "no one responded to the switchdev notifier, so the VLAN is not
offloaded". If you want to deny the configuration you need to return
something else, like -EBUSY.

Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK

[Andrew Lunn]

> I plan to make rgmii delays configurable from the device tree. Should I?
> a. switch to phy_interface_is_rgmii in the current patch?
> b. add another patch in this series?
> c. wait with change to phy_interface_is_rgmii for patch with rgmii
> delays configuration?

Do you actually need this feature? Does the PHY you are using already
support fine tuning of the delays?

	Andrew

Re: [PATCH net-next v3 2/8] net: dsa: vsc73xx: convert to PHYLINK

[Paweł Dembicki]

wt., 3 paź 2023 o 23:32 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> > I plan to make rgmii delays configurable from the device tree. Should I?
> > a. switch to phy_interface_is_rgmii in the current patch?
> > b. add another patch in this series?
> > c. wait with change to phy_interface_is_rgmii for patch with rgmii
> > delays configuration?
>
> Do you actually need this feature? Does the PHY you are using already
> support fine tuning of the delays?
>

After Vladimir answer I know that it should be a separate change.
I need it for MAC <-> switch connection, the rgmii port connected to
the cpu. At this moment, rgmii delays are hardcoded in vsc73xx_setup
and it should be tunable for the p2020rdb board. I plan to work on it
after the driver becomes usable.