Messages in this thread | | | Date | Wed, 8 Feb 2023 09:38:04 -0800 | Subject | Re: [PATCH net-next v3 3/3] net: dsa: rzn1-a5psw: add vlan support | From | Florian Fainelli <> |
| |
On 2/8/23 08:17, Clément Léger wrote: > Add support for vlan operation (add, del, filtering) on the RZN1 > driver. The a5psw switch supports up to 32 VLAN IDs with filtering, > tagged/untagged VLANs and PVID for each ports. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
This looks good to me, just a few nits/suggestions below.
> --- > drivers/net/dsa/rzn1_a5psw.c | 167 +++++++++++++++++++++++++++++++++++ > drivers/net/dsa/rzn1_a5psw.h | 8 +- > 2 files changed, 172 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 0ce3948952db..de6b18ec647d 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -583,6 +583,147 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port, > return ret; > } > > +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port, > + bool vlan_filtering, > + struct netlink_ext_ack *extack) > +{ > + u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT) | > + BIT(port + A5PSW_VLAN_DISC_SHIFT); > + struct a5psw *a5psw = ds->priv; > + u32 val = 0; > + > + if (vlan_filtering) > + val = BIT(port + A5PSW_VLAN_VERI_SHIFT) | > + BIT(port + A5PSW_VLAN_DISC_SHIFT); > + > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val); > + > + return 0; > +} > + > +static int a5psw_find_vlan_entry(struct a5psw *a5psw, u16 vid) > +{ > + u32 vlan_res; > + int i; > + > + /* Find vlan for this port */ > + for (i = 0; i < A5PSW_VLAN_COUNT; i++) { > + vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i)); > + if (FIELD_GET(A5PSW_VLAN_RES_VLANID, vlan_res) == vid) > + return i; > + } > + > + return -1; > +} > + > +static int a5psw_get_vlan_res_entry(struct a5psw *a5psw, u16 newvid)
The name seems a bit misleading with respect to what it does, maybe a5psw_new_vlan_res_entry()?
> +{ > + u32 vlan_res; > + int i; > + > + /* Find a free VLAN entry */ > + for (i = 0; i < A5PSW_VLAN_COUNT; i++) { > + vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i)); > + if (!(FIELD_GET(A5PSW_VLAN_RES_PORTMASK, vlan_res))) { > + vlan_res = FIELD_PREP(A5PSW_VLAN_RES_VLANID, newvid); > + a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(i), vlan_res); > + return i; > + } > + } > + > + return -1; > +} > + > +static void a5psw_port_vlan_tagged_cfg(struct a5psw *a5psw, int vlan_res_id,
unsigned int vlan_res_id
> + int port, bool set) > +{ > + u32 mask = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_RD_TAGMASK | > + BIT(port); > + u32 vlan_res_off = A5PSW_VLAN_RES(vlan_res_id); > + u32 val = A5PSW_VLAN_RES_WR_TAGMASK, reg; > + > + if (set) > + val |= BIT(port); > + > + /* Toggle tag mask read */ > + a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK); > + reg = a5psw_reg_readl(a5psw, vlan_res_off); > + a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK); > + > + reg &= ~mask; > + reg |= val; > + a5psw_reg_writel(a5psw, vlan_res_off, reg); > +} > + > +static void a5psw_port_vlan_cfg(struct a5psw *a5psw, int vlan_res_id, int port,
Likewise
> + bool set) > +{ > + u32 mask = A5PSW_VLAN_RES_WR_TAGMASK | BIT(port); > + u32 reg = A5PSW_VLAN_RES_WR_PORTMASK; > + > + if (set) > + reg |= BIT(port); > + > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_RES(vlan_res_id), mask, reg); > +} > + > +static int a5psw_port_vlan_add(struct dsa_switch *ds, int port, > + const struct switchdev_obj_port_vlan *vlan, > + struct netlink_ext_ack *extack) > +{ > + bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED); > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; > + struct a5psw *a5psw = ds->priv; > + u16 vid = vlan->vid; > + int vlan_res_id; > + > + dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n", > + vid, port, tagged ? "tagged" : "untagged", > + pvid ? "PVID" : "no PVID"); > + > + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid); > + if (vlan_res_id < 0) { > + vlan_res_id = a5psw_get_vlan_res_entry(a5psw, vid); > + if (vlan_res_id < 0) > + return -EINVAL;
return -ENOSPC?
> + } > + > + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true); > + if (tagged) > + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, true); > + > + if (pvid) { > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), > + BIT(port)); > + a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port), vid); > + } > + > + return 0; > +} > + > +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + struct a5psw *a5psw = ds->priv; > + u16 vid = vlan->vid; > + int vlan_res_id; > + > + dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port); > + > + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid); > + if (vlan_res_id < 0) > + return -EINVAL;
-EINVAL looks legit here
> + > + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false); > + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false); > + > + /* Disable PVID if the vid is matching the port one */ > + if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port))) > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0); > + > + return 0; > +} > + > static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port) > { > u32 reg_lo, reg_hi; > @@ -700,6 +841,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port, > ctrl_stats->MACControlFramesReceived = stat; > } > > +static void a5psw_vlan_setup(struct a5psw *a5psw, int port) > +{ > + u32 reg; > + > + /* Enable TAG always mode for the port, this is actually controlled > + * by VLAN_IN_MODE_ENA field which will be used for PVID insertion > + */ > + reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS; > + reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port); > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, A5PSW_VLAN_IN_MODE_PORT(port), > + reg);
If we always enable VLAN mode, which VLAN ID do switch ports not part of a VLAN aware bridge get classified into?
> + > + /* Set transparent mode for output frame manipulation, this will depend > + * on the VLAN_RES configuration mode > + */ > + reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT; > + reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port); > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE, > + A5PSW_VLAN_OUT_MODE_PORT(port), reg);
Sort of a follow-on to the previous question, what does transparent mean? Does that mean the frames ingressing with a certain VLAN tag will egress with the same VLAN tag in the absence of a VLAN configuration rewriting the tag?
> +} > + > static int a5psw_setup(struct dsa_switch *ds) > { > struct a5psw *a5psw = ds->priv; > @@ -772,6 +934,8 @@ static int a5psw_setup(struct dsa_switch *ds) > /* Enable management forward only for user ports */ > if (dsa_port_is_user(dp)) > a5psw_port_mgmtfwd_set(a5psw, port, true); > + > + a5psw_vlan_setup(a5psw, port); > } > > return 0; > @@ -801,6 +965,9 @@ static const struct dsa_switch_ops a5psw_switch_ops = { > .port_bridge_flags = a5psw_port_bridge_flags, > .port_stp_state_set = a5psw_port_stp_state_set, > .port_fast_age = a5psw_port_fast_age, > + .port_vlan_filtering = a5psw_port_vlan_filtering, > + .port_vlan_add = a5psw_port_vlan_add, > + .port_vlan_del = a5psw_port_vlan_del, > .port_fdb_add = a5psw_port_fdb_add, > .port_fdb_del = a5psw_port_fdb_del, > .port_fdb_dump = a5psw_port_fdb_dump, > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > index c67abd49c013..2bad2e3edc2a 100644 > --- a/drivers/net/dsa/rzn1_a5psw.h > +++ b/drivers/net/dsa/rzn1_a5psw.h > @@ -50,7 +50,9 @@ > #define A5PSW_VLAN_IN_MODE_TAG_ALWAYS 0x2 > > #define A5PSW_VLAN_OUT_MODE 0x2C > -#define A5PSW_VLAN_OUT_MODE_PORT(port) (GENMASK(1, 0) << ((port) * 2)) > +#define A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port) ((port) * 2) > +#define A5PSW_VLAN_OUT_MODE_PORT(port) (GENMASK(1, 0) << \ > + A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port)) > #define A5PSW_VLAN_OUT_MODE_DIS 0x0 > #define A5PSW_VLAN_OUT_MODE_STRIP 0x1 > #define A5PSW_VLAN_OUT_MODE_TAG_THROUGH 0x2 > @@ -59,7 +61,7 @@ > #define A5PSW_VLAN_IN_MODE_ENA 0x30 > #define A5PSW_VLAN_TAG_ID 0x34 > > -#define A5PSW_SYSTEM_TAGINFO(port) (0x200 + A5PSW_PORT_OFFSET(port)) > +#define A5PSW_SYSTEM_TAGINFO(port) (0x200 + 4 * (port)) > > #define A5PSW_AUTH_PORT(port) (0x240 + 4 * (port)) > #define A5PSW_AUTH_PORT_AUTHORIZED BIT(0) > @@ -68,7 +70,7 @@ > #define A5PSW_VLAN_RES_WR_PORTMASK BIT(30) > #define A5PSW_VLAN_RES_WR_TAGMASK BIT(29) > #define A5PSW_VLAN_RES_RD_TAGMASK BIT(28) > -#define A5PSW_VLAN_RES_ID GENMASK(16, 5) > +#define A5PSW_VLAN_RES_VLANID GENMASK(16, 5) > #define A5PSW_VLAN_RES_PORTMASK GENMASK(4, 0) > > #define A5PSW_RXMATCH_CONFIG(port) (0x3e80 + 4 * (port))
-- Florian
| |