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

Vladimir Oltean
2021-09-29T21:54:41Z

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
>