Re: [PATCH net-next 4/4 v4] net: dsa: rtl8366rb: Support setting STP state

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Sep 29, 2021 at 11:03:49PM +0200, Linus Walleij wrote:
> This adds support for setting the STP state to the RTL8366RB
> DSA switch. This rids the following message from the kernel on
> e.g. OpenWrt:
> 
> DSA: failed to set STP state 3 (-95)
> 
> Since the RTL8366RB has one STP state register per FID with
> two bit per port in each, we simply loop over all the FIDs
> and set the state on all of them.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v4:
> - New patch after discovering that we can do really nice
>   bridge offloading with these bits.
> ---
>  drivers/net/dsa/rtl8366rb.c | 47 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 748f22ab9130..c143fdab4802 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -110,6 +110,14 @@
>  
>  #define RTL8366RB_POWER_SAVING_REG	0x0021
>  
> +/* Spanning tree status (STP) control, two bits per port per FID */
> +#define RTL8368S_SPT_STATE_BASE		0x0050 /* 0x0050..0x0057 */

What does "SPT" stand for?

Also, is there any particular reason why these are named after RTL8368S,
when the entire driver has a naming scheme which follows RTL8366RB?

> +#define RTL8368S_SPT_STATE_MSK		0x3
> +#define RTL8368S_SPT_STATE_DISABLED	0x0
> +#define RTL8368S_SPT_STATE_BLOCKING	0x1
> +#define RTL8368S_SPT_STATE_LEARNING	0x2
> +#define RTL8368S_SPT_STATE_FORWARDING	0x3
> +
>  /* CPU port control reg */
>  #define RTL8368RB_CPU_CTRL_REG		0x0061
>  #define RTL8368RB_CPU_PORTS_MSK		0x00FF
> @@ -254,6 +262,7 @@
>  #define RTL8366RB_NUM_LEDGROUPS		4
>  #define RTL8366RB_NUM_VIDS		4096
>  #define RTL8366RB_PRIORITYMAX		7
> +#define RTL8366RB_NUM_FIDS		8
>  #define RTL8366RB_FIDMAX		7
>  
>  #define RTL8366RB_PORT_1		BIT(0) /* In userspace port 0 */
> @@ -1359,6 +1368,43 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static void
> +rtl8366rb_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	u16 mask;
> +	u32 val;
> +	int i;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		val = RTL8368S_SPT_STATE_DISABLED;
> +		break;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		val = RTL8368S_SPT_STATE_BLOCKING;
> +		break;
> +	case BR_STATE_LEARNING:
> +		val = RTL8368S_SPT_STATE_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		val = RTL8368S_SPT_STATE_FORWARDING;
> +		break;
> +	default:
> +		dev_err(smi->dev, "unknown bridge state requested\n");
> +		return;
> +	};
> +
> +	mask = (RTL8368S_SPT_STATE_MSK << (port * 2));

Could you not add a port argument:

#define RTL8366RB_STP_MASK			GENMASK(1, 0)
#define RTL8366RB_STP_STATE(port, state)	(((state) << ((port) * 2))
#define RTL8366RB_STP_STATE_MASK(port)		RTL8366RB_STP_STATE(RTL8366RB_STP_MASK, (port))

	regmap_update_bits(smi->map, RTL8366RB_STP_STATE_BASE + i,
			   RTL8366RB_STP_STATE_MASK(port),
			   RTL8366RB_STP_STATE(port, val));

> +	val <<= (port * 2);
> +
> +	/* Set the same status for the port on all the FIDs */
> +	for (i = 0; i < RTL8366RB_NUM_FIDS; i++) {
> +		regmap_update_bits(smi->map, RTL8368S_SPT_STATE_BASE + i,
> +				   mask, val);
> +	}
> +}
> +
>  static void
>  rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
>  {
> @@ -1784,6 +1830,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>  	.port_disable = rtl8366rb_port_disable,
>  	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
>  	.port_bridge_flags = rtl8366rb_port_bridge_flags,
> +	.port_stp_state_set = rtl8366rb_port_stp_state_set,
>  	.port_fast_age = rtl8366rb_port_fast_age,
>  	.port_change_mtu = rtl8366rb_change_mtu,
>  	.port_max_mtu = rtl8366rb_max_mtu,
> -- 
> 2.31.1
>