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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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!
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
+ * Based on tag_sja1105.c:
+ * Copyright (c) 2019, Vladimir Oltean <[email protected]>
+ */
+#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
Hi Vladimir,
wt., 12 wrz 2023 o 18:17 Vladimir Oltean <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
>
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.
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/[email protected]/
śr., 27 wrz 2023 o 01:03 Vladimir Oltean <[email protected]> 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/[email protected]/
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?
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.
śr., 27 wrz 2023 o 01:58 Vladimir Oltean <[email protected]> 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.
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.
> 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
wt., 3 paź 2023 o 23:32 Andrew Lunn <[email protected]> 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.