[PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK

[PATCH net-next 1/6] 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

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 179 ++++++++++++++-----------
 1 file changed, 99 insertions(+), 80 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index ae55167ce0a6..e853b57b0bc8 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -39,6 +39,7 @@
 #define VSC73XX_BLOCK_SYSTEM	0x7 /* Only subblock 0 */
 
 #define CPU_PORT	6 /* CPU port */
+#define VSC73XX_TABLE_ATTEMPTS	10
 
 /* MAC Block registers */
 #define VSC73XX_MAC_CFG		0x00
@@ -715,8 +716,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 }
 
 static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
-				       int port, struct phy_device *phydev,
-				       u32 initval)
+				       int port, u32 initval)
 {
 	u32 val = initval;
 	u8 seed;
@@ -754,12 +754,40 @@ static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
 			    VSC73XX_MAC_CFG_TX_EN | VSC73XX_MAC_CFG_RX_EN);
 }
 
-static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
-				struct phy_device *phydev)
+static void vsc73xx_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
 {
-	struct vsc73xx *vsc = ds->priv;
-	u32 val;
+	/* This switch only supports full-duplex at 1Gbps */
+	config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000FD |
+				   MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
 
+	if (port == CPU_PORT) {
+		__set_bit(PHY_INTERFACE_MODE_RGMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	} else {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		/* Compatibility for phylib's default interface type when the
+		 * phy-mode property is absent
+		 */
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	}
+
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
+}
+
+static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       const struct phylink_link_state *state)
+{
+	struct vsc73xx *vsc = ds->priv;
 	/* Special handling of the CPU-facing port */
 	if (port == CPU_PORT) {
 		/* Other ports are already initialized but not this one */
@@ -775,104 +803,92 @@ 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 maxloop = 10;
+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;
+	u32 val;
 
-		dev_dbg(vsc->dev, "port %d: went down\n",
-			port);
+	int maxloop = VSC73XX_TABLE_ATTEMPTS;
 
-		/* Disable RX on this port */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
-				    VSC73XX_MAC_CFG,
-				    VSC73XX_MAC_CFG_RX_EN, 0);
+	dev_dbg(vsc->dev, "port %d: went down\n",
+		port);
 
-		/* Discard packets */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				    VSC73XX_ARBDISC, BIT(port), BIT(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 */
+	/* 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);
-		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;
-			}
+		if (--maxloop == 0) {
+			dev_err(vsc->dev,
+				"timeout waiting for block arbiter\n");
+			/* Continue anyway */
+			break;
 		}
+	}
 
-		/* 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);
+	/* Put this port into reset */
+	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+		      VSC73XX_MAC_CFG_RESET);
 
-		/* Allow backward dropping of frames from this port */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
-				    VSC73XX_SBACKWDROP, BIT(port), BIT(port));
+	/* Accept packets again */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			    VSC73XX_ARBDISC, BIT(port), 0);
 
-		/* Receive mask (disable forwarding) */
-		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
-				    VSC73XX_RECVMASK, 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));
 
-		return;
-	}
+	/* Receive mask (disable forwarding) */
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+			    VSC73XX_RECVMASK, BIT(port), 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);
+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;
 
+	switch (speed) {
+	case SPEED_1000:
 		/* Set up default for internal port or external RGMII */
-		if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
+		if (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) {
+		break;
+	case SPEED_100:
+	case SPEED_10:
+		if (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 {
+		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 {
-		dev_err(vsc->dev,
-			"could not adjust link: unknown speed\n");
+		break;
 	}
 
 	/* 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,
@@ -1043,7 +1059,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 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Pawel Dembicki]

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

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

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 51 +++++++++++++++++++++++---
 drivers/net/dsa/vitesse-vsc73xx.h      |  1 +
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index e853b57b0bc8..ce22bd5fa8df 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -165,6 +165,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)
@@ -621,15 +625,17 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
 		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
 		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
-	/* Enable reception of frames on all ports */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
-		      0x5f);
 	/* IP multicast flood mask (table 144) */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
 		      0xff);
 
 	mdelay(50);
 
+	/*configure forward map to CPU <-> port only*/
+	for (i = 0; i < vsc->ds->num_ports; i++)
+		vsc->forward_map[i] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
+	vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(CPU_PORT);
+
 	/* Release reset from the internal PHYs */
 	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_PHY_RESET);
@@ -885,9 +891,6 @@ static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
 		break;
 	}
 
-	/* 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);
 }
 
@@ -1054,6 +1057,41 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
 	return 9600;
 }
 
+static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
+				       u8 state)
+{
+	struct vsc73xx *vsc = ds->priv;
+	/* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
+	 * forwarded only from to PI/SI interface. For more info see chapter
+	 * 2.7.1 (CPU Forwarding) in datasheet.
+	 * This function is required for tag8021q operations.
+	 */
+
+	if (state == BR_STATE_BLOCKING)
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_RECVMASK, BIT(port), 0);
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_RECVMASK, BIT(port), BIT(port));
+
+	if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_LEARNMASK, BIT(port), BIT(port));
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_LEARNMASK, BIT(port), 0);
+
+	if (state == BR_STATE_FORWARDING)
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_SRCMASKS + port,
+				    VSC73XX_SRCMASKS_PORTS_MASK,
+				    vsc->forward_map[port]);
+	else
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_SRCMASKS + port,
+				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
+}
+
 static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.get_tag_protocol = vsc73xx_get_tag_protocol,
 	.setup = vsc73xx_setup,
@@ -1070,6 +1108,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
 	.port_disable = vsc73xx_port_disable,
 	.port_change_mtu = vsc73xx_change_mtu,
 	.port_max_mtu = vsc73xx_get_max_mtu,
+	.port_stp_state_set = vsc73xx_port_stp_state_set,
 };
 
 static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
index 30b1f0a36566..1552a9ca06ff 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.h
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -15,6 +15,7 @@ struct vsc73xx {
 	u8				addr[ETH_ALEN];
 	const struct vsc73xx_ops	*ops;
 	void				*priv;
+	u8				forward_map[8];
 };
 
 struct vsc73xx_ops {
-- 
2.34.1

[PATCH net-next 3/6] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Pawel Dembicki]

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

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

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

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

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/Kconfig                |   2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c | 531 +++++++++++++++++++++----
 include/net/dsa.h                      |   2 +
 net/dsa/Kconfig                        |   6 +
 net/dsa/Makefile                       |   1 +
 net/dsa/tag_vsc73xx_8021q.c            |  87 ++++
 6 files changed, 543 insertions(+), 86 deletions(-)
 create mode 100644 net/dsa/tag_vsc73xx_8021q.c

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

[PATCH net-next 4/6] 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>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 69 ++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

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

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

[Pawel Dembicki]

This patch implement vlan filtering for vsc73xx driver.

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

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

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

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

[Pawel Dembicki]

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

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

Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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

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

[Pawel Dembicki]

This patch series is focused on getting vsc73xx usable.

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

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

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

Last patch fix wrong MTU configuration.

Pawel Dembicki (6):
  net: dsa: vsc73xx: convert to PHYLINK
  net: dsa: vsc73xx: add port_stp_state_set function
  net: dsa: vsc73xx: Add dsa tagging based on 8021q
  net: dsa: vsc73xx: Add bridge support
  net: dsa: vsc73xx: Add vlan filtering
  net: dsa: vsc73xx: fix MTU configuration

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

-- 
2.34.1

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Andrew Lunn]

> +	struct vsc73xx *vsc = ds->priv;
> +	/* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> +	 * forwarded only from to PI/SI interface. For more info see chapter
> +	 * 2.7.1 (CPU Forwarding) in datasheet.

Do you mean the CPU never gets to see the BPDU frames?

Does the hardware have any sort of packet matching to trap frames to
the CPU? Can you match on the destination MAC address
01:80:C2:00:00:00 ?

	Andrew

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Paweł Dembicki]

śr., 21 cze 2023 o 21:33 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> > +     struct vsc73xx *vsc = ds->priv;
> > +     /* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> > +      * forwarded only from to PI/SI interface. For more info see chapter
> > +      * 2.7.1 (CPU Forwarding) in datasheet.
>
> Do you mean the CPU never gets to see the BPDU frames?
>
> Does the hardware have any sort of packet matching to trap frames to
> the CPU? Can you match on the destination MAC address
> 01:80:C2:00:00:00 ?
>

Analyzer in VSC73XX switches can send some kind of packages to (and
from) processor via registers available from SPI/Platform BUS (for
some external analysis).  In some cases it's possible to configure: if
packet will be copied or forwarded to this special CPU queue.  But
BPDU frames could be sent to processor via CPU queue only. So It's
impossible to forward bridge control data via rgmii interface.

--
Pawel Dembicki

Re: [PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:13 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> 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
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

Thanks for doing this!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:33 PM Andrew Lunn <andrew@lunn.ch> wrote:

> > +     struct vsc73xx *vsc = ds->priv;
> > +     /* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> > +      * forwarded only from to PI/SI interface. For more info see chapter
> > +      * 2.7.1 (CPU Forwarding) in datasheet.
>
> Do you mean the CPU never gets to see the BPDU frames?
>
> Does the hardware have any sort of packet matching to trap frames to
> the CPU? Can you match on the destination MAC address
> 01:80:C2:00:00:00 ?

The hardware contains an embedded Intel 8054 CPU that can
execute programs to do pretty much anything.

The bad news: it requires a custom SDK thingy that we do not
have access to.

So far we used the chips in a bit of vanilla mode, which is all I
have ever seen in the systems we have and it can't do much,
not even add a helpful frame tag, but as can be seen from the
patches it can do VLAN...

Yours,
Linus Walleij

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:13 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> This isn't fully functional implementation of 802.1D, but
> port_stp_state_set is required for future tag8021q operations.
>
> This implementation handle properly all states, but vsc 73xx don't
> forward STP packets.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

I think it is a best effort and should be merged.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next 3/6] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:14 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:

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

From my point of view it is the best we can do.
Admittedly I do not understand the implications of using the 802.1
tagging for this, so I leave that to the experts to review.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next 4/6] net: dsa: vsc73xx: Add bridge support

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:14 PM Pawel Dembicki <paweldembicki@gmail.com> 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>

Given that we use the approach from the other patches, this
makes perfect sense.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:14 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> This patch implement vlan filtering for vsc73xx driver.
>
> After vlan filtering start, switch is reconfigured from QinQ to simple
> vlan aware mode. It's required, because VSC73XX chips haven't support
> for inner vlan tag filter.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Jun 21, 2023 at 9:14 PM Pawel Dembicki <paweldembicki@gmail.com> wrote:

> Switch in MAXLEN register store maximum size of data frame.
> MTU size is 18 bytes smaller than frame size.
>
> Current settings causes problems with packet forwarding.
> This patch fix MTU settings to proper values.
>
> Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

Ooops my bad. Thanks for finding this.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK

[Simon Horman]

On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> 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
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

...

> +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;
>  
> +	switch (speed) {
> +	case SPEED_1000:
>  		/* Set up default for internal port or external RGMII */
> -		if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> +		if (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) {
> +		break;
> +	case SPEED_100:
> +	case SPEED_10:
> +		if (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 {
> +		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 {
> -		dev_err(vsc->dev,
> -			"could not adjust link: unknown speed\n");
> +		break;
>  	}
>  
>  	/* 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);

Hi Pawel,

GCC 12.3.0 [1] reports that val may now be uninitialised at this point,
and in turn used uninitialised in vsc73xx_adjust_enable_port.

 In function 'vsc73xx_adjust_enable_port',
     inlined from 'vsc73xx_phylink_mac_link_up' at drivers/net/dsa/vitesse-vsc73xx-core.c:891:2:
 drivers/net/dsa/vitesse-vsc73xx-core.c:725:13: warning: 'val' may be used uninitialized [-Wmaybe-uninitialized]
   725 |         val |= VSC73XX_MAC_CFG_RESET;
       |             ^
 drivers/net/dsa/vitesse-vsc73xx-core.c: In function 'vsc73xx_phylink_mac_link_up':
 drivers/net/dsa/vitesse-vsc73xx-core.c:869:13: note: 'val' was declared here
   869 |         u32 val;
       |             ^~~

...

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

[Simon Horman]

On Wed, Jun 21, 2023 at 09:13:02PM +0200, Pawel Dembicki wrote:
> Switch in MAXLEN register store maximum size of data frame.
> MTU size is 18 bytes smaller than frame size.
> 
> Current settings causes problems with packet forwarding.
> This patch fix MTU settings to proper values.
> 
> Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

Hi Pawel,

one minor nit on this one: there should not be a blank line between the Fixes
and Signed-off-by lines.

Re: [PATCH net-next 3/6] net: dsa: vsc73xx: Add dsa tagging based on 8021q

[Simon Horman]

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

Hi Pawel,

a few minor nits to consider if you end up respining this series.

> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 3ed5391bb18d..4cf0166fef7b 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -125,7 +125,7 @@ config NET_DSA_SMSC_LAN9303_MDIO
>  
>  config NET_DSA_VITESSE_VSC73XX
>  	tristate
> -	select NET_DSA_TAG_NONE
> +	select NET_DSA_TAG_VSC73XX
>  	select FIXED_PHY
>  	select VITESSE_PHY
>  	select GPIOLIB
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c

...

> +static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> +				 bool port_vlan)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	struct dsa_port *dsa_port =  dsa_to_port(ds, port);
> +	u16 vlan_no;
> +	u32 val;

nit: Please use reverse xmas tree - longest line to shortest,
     for local variable declarations in new Networking code.

     Also, once space is enough after '='.

...

> diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c
> new file mode 100644
> index 000000000000..3d83dfecde31
> --- /dev/null
> +++ b/net/dsa/tag_vsc73xx_8021q.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com>
> + * Based on tag_sja1105.c:
> + * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
> + */
> +#include <linux/dsa/8021q.h>
> +
> +#include "tag.h"
> +#include "tag_8021q.h"
> +
> +#define VSC73XX_8021Q_NAME "vsc73xx-8021q"
> +
> +static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb,
> +				    struct net_device *netdev)
> +{
> +	struct dsa_port *dp = dsa_slave_to_port(netdev);
> +	u16 queue_mapping = skb_get_queue_mapping(skb);
> +	u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
> +	u16 tx_vid = dsa_tag_8021q_standalone_vid(dp);
> +	struct net_device *br = dsa_port_bridge_dev_get(dp);

nit: reverse xmas tree here too.

...

Re: [PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK

[Russell King (Oracle)]

On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> +	/* This driver does not make use of the speed, duplex, pause or the
> +	 * advertisement in its mac_config, so it is safe to mark this driver
> +	 * as non-legacy.
> +	 */
> +	config->legacy_pre_march2020 = false;

Great stuff, thanks!

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

Nit: normally have a blank line between function variable declarations
and the rest of the function body.

>  	/* Special handling of the CPU-facing port */
>  	if (port == CPU_PORT) {
>  		/* Other ports are already initialized but not this one */
> @@ -775,104 +803,92 @@ 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 maxloop = 10;
> +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;
> +	u32 val;
>  
> -		dev_dbg(vsc->dev, "port %d: went down\n",
> -			port);
> +	int maxloop = VSC73XX_TABLE_ATTEMPTS;

Reverse christmas-tree for variable declarations, and there shouldn't
be a blank line between them. In any case, I don't think you need
"maxloop" if you adopt my suggestion below.

>  
> -		/* Disable RX on this port */
> -		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> -				    VSC73XX_MAC_CFG,
> -				    VSC73XX_MAC_CFG_RX_EN, 0);
> +	dev_dbg(vsc->dev, "port %d: went down\n",
> +		port);
>  
> -		/* Discard packets */
> -		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> -				    VSC73XX_ARBDISC, BIT(port), BIT(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 */
> +	/* 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);
> -		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;
> -			}
> +		if (--maxloop == 0) {
> +			dev_err(vsc->dev,
> +				"timeout waiting for block arbiter\n");
> +			/* Continue anyway */
> +			break;
>  		}
> +	}

I know you're only moving this code, but I think it would be good to
_first_ have a patch that fixes this polling function:

	int ret, err;
...
		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 != 0)
			dev_err(vsc->dev,
				"timeout waiting for block arbiter\n");
		else if (err < 0)
			dev_err(vsc->dev,
				"error reading arbiter\n");

This 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.

> +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;
>  
> +	switch (speed) {
> +	case SPEED_1000:
>  		/* Set up default for internal port or external RGMII */
> -		if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> +		if (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) {
> +		break;
> +	case SPEED_100:
> +	case SPEED_10:
> +		if (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 {
> +		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 {
> -		dev_err(vsc->dev,
> -			"could not adjust link: unknown speed\n");
> +		break;
>  	}

Do the dev_dbg() add anything useful over what phylink prints when the
link comes up?

I don't think moving to a switch() statement for this is a good idea.
Given that "val" may be uninitialised, I suspect the following may be
a better solution:

	if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) {
		if (speed == SPEED_1000) {
			...
		} else {
			...
		}

		... set VSC73XX_BLOCK_ANALYZER and call
		vsc73xx_adjust_enable_port ...
	}

However, looking at the definitions of the various macros, it seems we
can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_*
definitions:

		if (speed == SPEED_1000) {
			val = VSC73XX_MAC_CFG_GIGA_MODE |
			      VSC73XX_MAC_CFG_TX_IPG_1000M;

			if (interface == PHY_INTERFACE_MODE_RGMII)
				val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
			else
				val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
		} else {
			val = VSC73XX_MAC_CFG_TX_IPG_100_10M |
			      VSC73XX_MAC_CFG_CLK_SEL_EXT;
		}

		if (duplex == DUPLEX_FULL)
			val |= VSC73XX_MAC_CFG_FDX;

Now, this reveals a question: when operating in RGMII mode, why do we
need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and
VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always
uses CLK_SEL_EXT ?

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 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Andrew Lunn]

On Wed, Jun 21, 2023 at 10:38:22PM +0200, Paweł Dembicki wrote:
> śr., 21 cze 2023 o 21:33 Andrew Lunn <andrew@lunn.ch> napisał(a):
> >
> > > +     struct vsc73xx *vsc = ds->priv;
> > > +     /* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> > > +      * forwarded only from to PI/SI interface. For more info see chapter
> > > +      * 2.7.1 (CPU Forwarding) in datasheet.
> >
> > Do you mean the CPU never gets to see the BPDU frames?
> >
> > Does the hardware have any sort of packet matching to trap frames to
> > the CPU? Can you match on the destination MAC address
> > 01:80:C2:00:00:00 ?
> >
> 
> Analyzer in VSC73XX switches can send some kind of packages to (and
> from) processor via registers available from SPI/Platform BUS (for
> some external analysis).  In some cases it's possible to configure: if
> packet will be copied or forwarded to this special CPU queue.  But
> BPDU frames could be sent to processor via CPU queue only. So It's
> impossible to forward bridge control data via rgmii interface.

So am i correct in saying, if you actually enable STP, and it decides
to block a port, the BPDUs are also blocked. After a while it will
decide the peer has gone, and unblock the port. A broadcast storm will
then happen for a while, until a BPDU is received, at which point it
will block the port again.

     Andrew

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Vladimir Oltean]

On Thu, Jun 22, 2023 at 03:01:02PM +0200, Andrew Lunn wrote:
> On Wed, Jun 21, 2023 at 10:38:22PM +0200, Paweł Dembicki wrote:
> > śr., 21 cze 2023 o 21:33 Andrew Lunn <andrew@lunn.ch> napisał(a):
> > >
> > > > +     struct vsc73xx *vsc = ds->priv;
> > > > +     /* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> > > > +      * forwarded only from to PI/SI interface. For more info see chapter
> > > > +      * 2.7.1 (CPU Forwarding) in datasheet.
> > >
> > > Do you mean the CPU never gets to see the BPDU frames?
> > >
> > > Does the hardware have any sort of packet matching to trap frames to
> > > the CPU? Can you match on the destination MAC address
> > > 01:80:C2:00:00:00 ?
> > >
> > 
> > Analyzer in VSC73XX switches can send some kind of packages to (and
> > from) processor via registers available from SPI/Platform BUS (for
> > some external analysis).  In some cases it's possible to configure: if
> > packet will be copied or forwarded to this special CPU queue.  But
> > BPDU frames could be sent to processor via CPU queue only. So It's
> > impossible to forward bridge control data via rgmii interface.
> 
> So am i correct in saying, if you actually enable STP, and it decides
> to block a port, the BPDUs are also blocked. After a while it will
> decide the peer has gone, and unblock the port. A broadcast storm will
> then happen for a while, until a BPDU is received, at which point it
> will block the port again.
> 
>      Andrew

This is pretty much the expected behavior from a tag_8021q based
implementation with no hardware assist for control packets. tag_8021q
can provide port identification, but it cannot transform a data packet
into a control packet and it cannot force the switch to accept packets
from ports whose data plane is disabled by STP.

I am also going to review this series in the following days, but I don't
have the required amount of time right now. Perhaps during the weekend.

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Jakub Kicinski]

On Wed, 21 Jun 2023 21:12:58 +0200 Pawel Dembicki wrote:
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index 30b1f0a36566..1552a9ca06ff 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -15,6 +15,7 @@ struct vsc73xx {
>  	u8				addr[ETH_ALEN];
>  	const struct vsc73xx_ops	*ops;
>  	void				*priv;
> +	u8				forward_map[8];
>  };

kdoc missing here:

> drivers/net/dsa/vitesse-vsc73xx.h:20: warning: Function parameter or member 'forward_map' not described in 'vsc73xx'
-- 
pw-bot: cr

Re: [PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK

[Paweł Dembicki]

czw., 22 cze 2023 o 13:55 Russell King (Oracle)
<linux@armlinux.org.uk> napisał(a):
>
> On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> > +     /* This driver does not make use of the speed, duplex, pause or the
> > +      * advertisement in its mac_config, so it is safe to mark this driver
> > +      * as non-legacy.
> > +      */
> > +     config->legacy_pre_march2020 = false;
>
> Great stuff, thanks!
>
> > +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;
>
> Nit: normally have a blank line between function variable declarations
> and the rest of the function body.
>
> >       /* Special handling of the CPU-facing port */
> >       if (port == CPU_PORT) {
> >               /* Other ports are already initialized but not this one */
> > @@ -775,104 +803,92 @@ 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 maxloop = 10;
> > +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;
> > +     u32 val;
> >
> > -             dev_dbg(vsc->dev, "port %d: went down\n",
> > -                     port);
> > +     int maxloop = VSC73XX_TABLE_ATTEMPTS;
>
> Reverse christmas-tree for variable declarations, and there shouldn't
> be a blank line between them. In any case, I don't think you need
> "maxloop" if you adopt my suggestion below.
>
> >
> > -             /* Disable RX on this port */
> > -             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > -                                 VSC73XX_MAC_CFG,
> > -                                 VSC73XX_MAC_CFG_RX_EN, 0);
> > +     dev_dbg(vsc->dev, "port %d: went down\n",
> > +             port);
> >
> > -             /* Discard packets */
> > -             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> > -                                 VSC73XX_ARBDISC, BIT(port), BIT(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 */
> > +     /* 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);
> > -             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;
> > -                     }
> > +             if (--maxloop == 0) {
> > +                     dev_err(vsc->dev,
> > +                             "timeout waiting for block arbiter\n");
> > +                     /* Continue anyway */
> > +                     break;
> >               }
> > +     }
>
> I know you're only moving this code, but I think it would be good to
> _first_ have a patch that fixes this polling function:
>
>         int ret, err;
> ...
>                 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 != 0)
>                         dev_err(vsc->dev,
>                                 "timeout waiting for block arbiter\n");
>                 else if (err < 0)
>                         dev_err(vsc->dev,
>                                 "error reading arbiter\n");
>
> This 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.
>

Thank you for the tips. I will prepare additional commit in v2 series.

> > +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;
> >
> > +     switch (speed) {
> > +     case SPEED_1000:
> >               /* Set up default for internal port or external RGMII */
> > -             if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> > +             if (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) {
> > +             break;
> > +     case SPEED_100:
> > +     case SPEED_10:
> > +             if (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 {
> > +             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 {
> > -             dev_err(vsc->dev,
> > -                     "could not adjust link: unknown speed\n");
> > +             break;
> >       }
>
> Do the dev_dbg() add anything useful over what phylink prints when the
> link comes up?
>
> I don't think moving to a switch() statement for this is a good idea.
> Given that "val" may be uninitialised, I suspect the following may be
> a better solution:
>
>         if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) {
>                 if (speed == SPEED_1000) {
>                         ...
>                 } else {
>                         ...
>                 }
>
>                 ... set VSC73XX_BLOCK_ANALYZER and call
>                 vsc73xx_adjust_enable_port ...
>         }
>
> However, looking at the definitions of the various macros, it seems we
> can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_*
> definitions:
>
>                 if (speed == SPEED_1000) {
>                         val = VSC73XX_MAC_CFG_GIGA_MODE |
>                               VSC73XX_MAC_CFG_TX_IPG_1000M;
>
>                         if (interface == PHY_INTERFACE_MODE_RGMII)
>                                 val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
>                         else
>                                 val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
>                 } else {
>                         val = VSC73XX_MAC_CFG_TX_IPG_100_10M |
>                               VSC73XX_MAC_CFG_CLK_SEL_EXT;
>                 }
>
>                 if (duplex == DUPLEX_FULL)
>                         val |= VSC73XX_MAC_CFG_FDX;
>
> Now, this reveals a question: when operating in RGMII mode, why do we
> need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and
> VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always
> uses CLK_SEL_EXT ?
>

VSC73XX_MAC_CFG_CLK_SEL_EXT should be used always when phy is used, no
matter what speed is. VSC73XX_MAC_CFG_CLK_SEL_1000M in RGMII mode. It
can be even more simplified:

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;

--
Pawel Dembicki

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Vladimir Oltean]

On Wed, Jun 21, 2023 at 11:27:14PM +0200, Linus Walleij wrote:
> On Wed, Jun 21, 2023 at 9:33 PM Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > > +     struct vsc73xx *vsc = ds->priv;
> > > +     /* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> > > +      * forwarded only from to PI/SI interface. For more info see chapter
> > > +      * 2.7.1 (CPU Forwarding) in datasheet.
> >
> > Do you mean the CPU never gets to see the BPDU frames?
> >
> > Does the hardware have any sort of packet matching to trap frames to
> > the CPU? Can you match on the destination MAC address
> > 01:80:C2:00:00:00 ?
> 
> The hardware contains an embedded Intel 8054 CPU that can
> execute programs to do pretty much anything.
> 
> The bad news: it requires a custom SDK thingy that we do not
> have access to.
> 
> So far we used the chips in a bit of vanilla mode, which is all I
> have ever seen in the systems we have and it can't do much,
> not even add a helpful frame tag, but as can be seen from the
> patches it can do VLAN...
> 
> Yours,
> Linus Walleij

But even without involving the iCPU, it should be possible to inject/extract
control packets over the SI interface, using the CPU_CAPT and CPUTXDAT block
registers, correct?

IIUC, ocelot with tag_8021q does just that for STP and PTP, see
ocelot_port_inject_frame() and ocelot_xtr_poll_frame().

Re: [PATCH net-next 2/6] net: dsa: vsc73xx: add port_stp_state_set function

[Paweł Dembicki]

niedz., 25 cze 2023 o 13:21 Vladimir Oltean <olteanv@gmail.com> napisał(a):
>
> On Wed, Jun 21, 2023 at 11:27:14PM +0200, Linus Walleij wrote:
> > On Wed, Jun 21, 2023 at 9:33 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > > +     struct vsc73xx *vsc = ds->priv;
> > > > +     /* FIXME: STP frames isn't forwarded at this moment. BPDU frames are
> > > > +      * forwarded only from to PI/SI interface. For more info see chapter
> > > > +      * 2.7.1 (CPU Forwarding) in datasheet.
> > >
> > > Do you mean the CPU never gets to see the BPDU frames?
> > >
> > > Does the hardware have any sort of packet matching to trap frames to
> > > the CPU? Can you match on the destination MAC address
> > > 01:80:C2:00:00:00 ?
> >
> > The hardware contains an embedded Intel 8054 CPU that can
> > execute programs to do pretty much anything.
> >
> > The bad news: it requires a custom SDK thingy that we do not
> > have access to.
> >
> > So far we used the chips in a bit of vanilla mode, which is all I
> > have ever seen in the systems we have and it can't do much,
> > not even add a helpful frame tag, but as can be seen from the
> > patches it can do VLAN...
> >
> > Yours,
> > Linus Walleij
>
> But even without involving the iCPU, it should be possible to inject/extract
> control packets over the SI interface, using the CPU_CAPT and CPUTXDAT block
> registers, correct?

Yes , It should work with CPU_CAPT and CPUTXDAT.

>
> IIUC, ocelot with tag_8021q does just that for STP and PTP, see
> ocelot_port_inject_frame() and ocelot_xtr_poll_frame().

I was planning to do it in the next step after making this driver
however usable.

--
Pawel Dembicki

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

[Paweł Dembicki]

śr., 21 cze 2023 o 21:14 Pawel Dembicki <paweldembicki@gmail.com> napisał(a):
>
> This patch series is focused on getting vsc73xx usable.
>
> First patch is simple convert to phylink, because adjust_link won't work
> anymore.
>
> Patches 2-5 are basic implementation of tag8021q funcionality with QinQ
> support without vlan filtering in bridge and simple vlan aware in vlan
> filtering mode.
>
> STP frames isn't forwarded at this moment. BPDU frames are forwarded
> only from to PI/SI interface. For more info see chapter
> 2.7.1 (CPU Forwarding) in datasheet.
>
> Last patch fix wrong MTU configuration.
>
> Pawel Dembicki (6):
>   net: dsa: vsc73xx: convert to PHYLINK
>   net: dsa: vsc73xx: add port_stp_state_set function
>   net: dsa: vsc73xx: Add dsa tagging based on 8021q
>   net: dsa: vsc73xx: Add bridge support
>   net: dsa: vsc73xx: Add vlan filtering
>   net: dsa: vsc73xx: fix MTU configuration
>
>  drivers/net/dsa/Kconfig                |   2 +-
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 928 ++++++++++++++++++++-----
>  drivers/net/dsa/vitesse-vsc73xx.h      |   1 +
>  include/net/dsa.h                      |   2 +
>  net/dsa/Kconfig                        |   6 +
>  net/dsa/Makefile                       |   1 +
>  net/dsa/tag_vsc73xx_8021q.c            |  87 +++
>  7 files changed, 857 insertions(+), 170 deletions(-)
>  create mode 100644 net/dsa/tag_vsc73xx_8021q.c
>
> --
> 2.34.1
>

I pushed v2. Thank Russell, Jakub, Simon and other people for helping me.

--
Pawel Dembicki