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

  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
  

Hi Pawel,

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

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

	struct vsc73xx *vsc = ds->priv;

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

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

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

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

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

> +
> +	return 0;
> +}
> +
> +/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
> + * forwarded only from and to PI/SI interface. For more info see chapter
> + * 2.7.1 (CPU Forwarding) in datasheet.
> + * This function is required for tag8021q operations.
> + */
> +
> +static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	if (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), 0);
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_RECVMASK, BIT(port), BIT(port));
> +
> +	if (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_LEARNMASK, BIT(port), BIT(port));
> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_LEARNMASK, BIT(port), 0);
> +
> +	if (state == BR_STATE_FORWARDING)
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK,
> +				    vsc->forward_map[port]);

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

> +	else
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_SRCMASKS + port,
> +				    VSC73XX_SRCMASKS_PORTS_MASK, 0);
> +}
> +
>  static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_tag_protocol = vsc73xx_get_tag_protocol,
>  	.setup = vsc73xx_setup,
> +	.port_setup = vsc73xx_port_setup,
>  	.phy_read = vsc73xx_phy_read,
>  	.phy_write = vsc73xx_phy_write,
>  	.phylink_get_caps = vsc73xx_phylink_get_caps,
> @@ -1049,6 +1097,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.port_disable = vsc73xx_port_disable,
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
> +	.port_stp_state_set = vsc73xx_port_stp_state_set,
>  };
>  
>  static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index f79d81ef24fb..224e284a5573 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -18,6 +18,7 @@
>  
>  /**
>   * struct vsc73xx - VSC73xx state container
> + * @forward_map: Forward table cache

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

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

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

Thank you for Your time.

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

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

> > +     else
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > +                                 VSC73XX_SRCMASKS + port,
> > +                                 VSC73XX_SRCMASKS_PORTS_MASK, 0);
> > +}
> > +
> >  static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .get_tag_protocol = vsc73xx_get_tag_protocol,
> >       .setup = vsc73xx_setup,
> > +     .port_setup = vsc73xx_port_setup,
> >       .phy_read = vsc73xx_phy_read,
> >       .phy_write = vsc73xx_phy_write,
> >       .phylink_get_caps = vsc73xx_phylink_get_caps,
> > @@ -1049,6 +1097,7 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .port_disable = vsc73xx_port_disable,
> >       .port_change_mtu = vsc73xx_change_mtu,
> >       .port_max_mtu = vsc73xx_get_max_mtu,
> > +     .port_stp_state_set = vsc73xx_port_stp_state_set,
> >  };
> >
> >  static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> > index f79d81ef24fb..224e284a5573 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > @@ -18,6 +18,7 @@
> >
> >  /**
> >   * struct vsc73xx - VSC73xx state container
> > + * @forward_map: Forward table cache
>
> If you start describing the member fields, shouldn't all be described?
> I think there will be kdoc warnings otherwise.
>

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

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

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

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

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

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

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

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.
  

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

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

> 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