Re: [PATCH net-next 4/4 v4] net: dsa: rtl8366rb: Support setting STP state
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-09-29 21:54:41
On Wed, Sep 29, 2021 at 11:03:49PM +0200, Linus Walleij wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Cc: Mauri Sandberg <redacted> Cc: DENG Qingfang <dqfext@gmail.com> Signed-off-by: Linus Walleij <redacted> --- 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?
quoted hunk ↗ jump to hunk
+#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));
quoted hunk ↗ jump to hunk
+ 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