| Message ID | 20240213220331.239031-7-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: vsc73xx: Make vsc73xx usable | expand |
On Tue, Feb 13, 2024 at 11:03:19PM +0100, Pawel Dembicki wrote: > This isn't a fully functional implementation of 802.1D, but > port_stp_state_set is required for a future tag8021q operations. > > This implementation handles properly all states, but vsc73xx doesn't > forward STP packets. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > --- > v4: > - fully reworked port_stp_state_set A "fully reworked" patch isn't exactly compatible with keeping attached Reviewed-by tags attached to the patch. It implies Linus didn't actually review the code you are submitting now. > 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 | 83 ++++++++++++++++++++++++-- > 1 file changed, 77 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c > index 75597daaad17..1dca3c476fac 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); > @@ -841,9 +842,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); > } > > @@ -1026,6 +1024,78 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port, > config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000; > } > > +static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, bool configure) Why have you chosen the name "bool configure" exactly? It is not intuitive. Could you just pass the STP state of the port as an argument? > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct vsc73xx *vsc = ds->priv; > + int i; > + > + if (configure) { > + u16 mask = BIT(CPU_PORT); > + > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > + VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port)); > + > + if (dp->bridge) { > + struct dsa_port *other_dp; > + > + dsa_switch_for_each_user_port(other_dp, ds) { > + if (other_dp->bridge == dp->bridge && > + other_dp->index != port && > + other_dp->stp_state == BR_STATE_FORWARDING) { > + int other_port = other_dp->index; > + > + mask |= BIT(other_port); > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > + VSC73XX_SRCMASKS + other_port, > + BIT(port), BIT(port)); > + } > + } > + } > + > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port, > + VSC73XX_SRCMASKS_PORTS_MASK, mask); > + } else { > + for (i = 0; i < vsc->ds->num_ports; i++) { Try to refrain from doing plain integer iterations through ports when there are holes in the port indices. Use dsa_switch_for_each_available_port() which skips the unused/absent ports, and consequently you're not touching unwanted registers. > + if (i == port) > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > + VSC73XX_SRCMASKS + i, > + VSC73XX_SRCMASKS_PORTS_MASK, 0); > + else > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > + VSC73XX_SRCMASKS + i, BIT(port), 0); > + } > + } To save on indentation, I would simplify the big if () else () block like this. if (state != BR_STATE_FORWARDING) { /* Ports that aren't in the forwarding state must not * forward packets anywhere. */ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port, VSC73XX_SRCMASKS_PORTS_MASK, 0); dsa_switch_for_each_available_port(other_dp, ds) { if (dp == other_dp) continue; vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + other_dp->index, BIT(port), 0); } return; } /* Forwarding ports must forward to the CPU and to other ports * in the same bridge */ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port)); mask = BIT(CPU_PORT); if (dp->bridge) { struct dsa_port *other_dp; dsa_switch_for_each_user_port(other_dp, ds) { if (other_dp->bridge == dp->bridge && other_dp->index != port && other_dp->stp_state == BR_STATE_FORWARDING) { int other_port = other_dp->index; mask |= BIT(other_port); vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + other_port, BIT(port), BIT(port)); } } } vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port, VSC73XX_SRCMASKS_PORTS_MASK, mask); > +} > + > +/* 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. Please keep the name consistent at "tag_8021q". > + */ > + nitpick: remove the blank line. > +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > +{ > + struct vsc73xx *vsc = ds->priv; > + u32 val; > + > + val = (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED) ? > + 0 : BIT(port); > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > + VSC73XX_RECVMASK, BIT(port), val); > + > + val = (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) ? > + BIT(port) : 0; > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, > + VSC73XX_LEARNMASK, BIT(port), val); I am extremely particular about this. Please also implement .port_pre_bridge_flags() and .port_bridge_flags(), and treat the BR_LEARNING flag. The idea is that standalone ports are in the BR_STATE_FORWARDING state (it's a DSA API thing), but learning should _not_ be enabled (it breaks things when one port sees traffic originated from the other). So you need to set the bit in VSC73XX_LEARNMASK only if the port is in the BR_STATE_LEARNING || BR_STATE_FORWARDING, _and_ the BR_LEARNING flag is set. This should be relatively easy to do. If you're not going to do it now because of patch set size constraints, at least leave a big FIXME in the code or in the commit message. > + > + /* CPU Port should always forward packets when user ports are forwarding > + * so let's configure it from other ports only. > + */ > + if (port != CPU_PORT) > + vsc73xx_refresh_fwd_map(ds, port, state == BR_STATE_FORWARDING); > +} > + > static const struct dsa_switch_ops vsc73xx_ds_ops = { > .get_tag_protocol = vsc73xx_get_tag_protocol, > .setup = vsc73xx_setup, > @@ -1041,6 +1111,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, > .phylink_get_caps = vsc73xx_phylink_get_caps, > }; > > -- > 2.34.1 >
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c index 75597daaad17..1dca3c476fac 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); @@ -841,9 +842,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); } @@ -1026,6 +1024,78 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port, config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000; } +static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, bool configure) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct vsc73xx *vsc = ds->priv; + int i; + + if (configure) { + u16 mask = BIT(CPU_PORT); + + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port)); + + if (dp->bridge) { + struct dsa_port *other_dp; + + dsa_switch_for_each_user_port(other_dp, ds) { + if (other_dp->bridge == dp->bridge && + other_dp->index != port && + other_dp->stp_state == BR_STATE_FORWARDING) { + int other_port = other_dp->index; + + mask |= BIT(other_port); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_SRCMASKS + other_port, + BIT(port), BIT(port)); + } + } + } + + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port, + VSC73XX_SRCMASKS_PORTS_MASK, mask); + } else { + for (i = 0; i < vsc->ds->num_ports; i++) { + if (i == port) + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_SRCMASKS + i, + VSC73XX_SRCMASKS_PORTS_MASK, 0); + else + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_SRCMASKS + i, BIT(port), 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; + u32 val; + + val = (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED) ? + 0 : BIT(port); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_RECVMASK, BIT(port), val); + + val = (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) ? + BIT(port) : 0; + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, + VSC73XX_LEARNMASK, BIT(port), val); + + /* CPU Port should always forward packets when user ports are forwarding + * so let's configure it from other ports only. + */ + if (port != CPU_PORT) + vsc73xx_refresh_fwd_map(ds, port, state == BR_STATE_FORWARDING); +} + static const struct dsa_switch_ops vsc73xx_ds_ops = { .get_tag_protocol = vsc73xx_get_tag_protocol, .setup = vsc73xx_setup, @@ -1041,6 +1111,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, .phylink_get_caps = vsc73xx_phylink_get_caps, };