diff mbox series

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

Message ID 20210929210349.130099-5-linus.walleij@linaro.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series RTL8366RB enhancements | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 71 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Linus Walleij Sept. 29, 2021, 9:03 p.m. UTC 
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(+)

Comments

Vladimir Oltean Sept. 29, 2021, 9:54 p.m. UTC | #1 
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
>
Linus Walleij Oct. 4, 2021, 9:07 p.m. UTC | #2 
On Wed, Sep 29, 2021 at 11:54 PM Vladimir Oltean <olteanv@gmail.com> wrote:

> > +/* Spanning tree status (STP) control, two bits per port per FID */
> > +#define RTL8368S_SPT_STATE_BASE              0x0050 /* 0x0050..0x0057 */
>
> What does "SPT" stand for?

No idea but I guess "spanning tree". It's what the register is named
in the vendor code:

/*
@func int32 | rtl8368s_setAsicSpanningTreeStatus | Configure spanning
tree state per each port.
@parm enum PORTID | port | Physical port number (0~5).
@parm uint32 | fid | FID of 8 SVL/IVL in port (0~7).
@parm enum SPTSTATE | state | Spanning tree state for FID of 8 SVL/IVL.
@rvalue SUCCESS | Success.
@rvalue ERRNO_SMI | SMI access error.
@rvalue ERRNO_PORT_NUM | Invalid port number.
@rvalue ERRNO_FID | Invalid FID.
@rvalue ERRNO_STP_STATE | Invalid spanning tree state
@common
    System supports 8 SVL/IVL configuration and each port has
dedicated spanning tree state setting for each FID. There are four
states supported by ASIC.

    Disable state         ASIC did not receive and transmit packets at
port with disable state.
    Blocking state        ASIC will receive BPDUs without L2 auto
learning and does not transmit packet out of port in blocking state.
    Learning state        ASIC will receive packets with L2 auto
learning and transmit out BPDUs only.
    Forwarding state    The port will receive and transmit packets normally.

*/
int32 rtl8368s_setAsicSpanningTreeStatus(enum PORTID port, enum
FIDVALUE fid, enum SPTSTATE state)
{
    uint32 regAddr;
    uint32 regData;
    uint32 regBits;
    int32 retVal;

    if(port >=PORT_MAX)
        return ERRNO_PORT_NUM;

    if(fid > RTL8368S_FIDMAX)
        return ERRNO_FID;

    if(state > FORWARDING)
        return ERRNO_STP_STATE;

    regAddr = RTL8368S_SPT_STATE_BASE + fid;
    regBits = RTL8368S_SPT_STATE_MSK << (port*RTL8368S_SPT_STATE_BITS);
    regData = (state << (port*RTL8368S_SPT_STATE_BITS)) & regBits;


    retVal = rtl8368s_setAsicRegBits(regAddr,regBits,regData);

    return retVal;
}

Maybe it is just the coder mixing up STP and SPT, but the register is indeed
named like this in their code.

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

Ooops, my bad. The RTL8368S == RTL8366RB AFAICT, the product
numbers from Realtek makes no sense.

> > +     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));

Yup that's neat, I'll do this!

Yours,
Linus Walleij
diff mbox series

Patch

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 */
+#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));
+	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,