[PATCH net-next 0/4 v4] RTL8366RB enhancements

[PATCH net-next 0/4 v4] RTL8366RB enhancements

[Linus Walleij]

This patch set is a set of reasonably mature improvements
for the RTL8366RB switch, implemented after Vladimir
challenged me to dig deeper into the switch functions.

ChangeLog -> v4:
- Rebase earlier circulated patches on the now merged
  VLAN set-up cleanups.

Linus Walleij (4):
  net: dsa: rtl8366rb: Support disabling learning
  net: dsa: rtl8366rb: Support flood control
  net: dsa: rtl8366rb: Support fast aging
  net: dsa: rtl8366rb: Support setting STP state

 drivers/net/dsa/rtl8366rb.c | 162 ++++++++++++++++++++++++++++++++++--
 1 file changed, 156 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Linus Walleij]

The RTL8366RB hardware supports disabling learning per-port
so let's make use of this feature. Rename some unfortunately
named registers in the process.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- No changes, rebased on other patches.
ChangeLog v2->v3:
- Disable learning by default, learning will be turned
  on selectively using the callback.
ChangeLog v1->v2:
- New patch suggested by Vladimir.
---
 drivers/net/dsa/rtl8366rb.c | 50 ++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index bb9d017c2f9f..b3056064b937 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -14,6 +14,7 @@
 
 #include <linux/bitops.h>
 #include <linux/etherdevice.h>
+#include <linux/if_bridge.h>
 #include <linux/interrupt.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
@@ -42,9 +43,12 @@
 /* Port Enable Control register */
 #define RTL8366RB_PECR				0x0001
 
-/* Switch Security Control registers */
-#define RTL8366RB_SSCR0				0x0002
-#define RTL8366RB_SSCR1				0x0003
+/* Switch per-port learning disablement register */
+#define RTL8366RB_PORT_LEARNDIS_CTRL		0x0002
+
+/* Security control, actually aging register */
+#define RTL8366RB_SECURITY_CTRL			0x0003
+
 #define RTL8366RB_SSCR2				0x0004
 #define RTL8366RB_SSCR2_DROP_UNKNOWN_DA		BIT(0)
 
@@ -927,13 +931,14 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		/* layer 2 size, see rtl8366rb_change_mtu() */
 		rb->max_mtu[i] = 1532;
 
-	/* Enable learning for all ports */
-	ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0);
+	/* Disable learning for all ports */
+	ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL,
+			   RTL8366RB_PORT_ALL);
 	if (ret)
 		return ret;
 
 	/* Enable auto ageing for all ports */
-	ret = regmap_write(smi->map, RTL8366RB_SSCR1, 0);
+	ret = regmap_write(smi->map, RTL8366RB_SECURITY_CTRL, 0);
 	if (ret)
 		return ret;
 
@@ -1272,6 +1277,37 @@ static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port,
 	return ret;
 }
 
+static int
+rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+				struct switchdev_brport_flags flags,
+				struct netlink_ext_ack *extack)
+{
+	/* We support enabling/disabling learning */
+	if (flags.mask & ~(BR_LEARNING))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int
+rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
+			    struct switchdev_brport_flags flags,
+			    struct netlink_ext_ack *extack)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (flags.mask & BR_LEARNING) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL,
+					 BIT(port),
+					 (flags.val & BR_LEARNING) ? 0 : BIT(port));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct realtek_smi *smi = ds->priv;
@@ -1682,6 +1718,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.port_vlan_del = rtl8366_vlan_del,
 	.port_enable = rtl8366rb_port_enable,
 	.port_disable = rtl8366rb_port_disable,
+	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
+	.port_bridge_flags = rtl8366rb_port_bridge_flags,
 	.port_change_mtu = rtl8366rb_change_mtu,
 	.port_max_mtu = rtl8366rb_max_mtu,
 };
-- 
2.31.1

Re: [PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Vladimir Oltean]

On Wed, Sep 29, 2021 at 11:03:46PM +0200, Linus Walleij wrote:
> The RTL8366RB hardware supports disabling learning per-port
> so let's make use of this feature. Rename some unfortunately
> named registers in the process.
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Alvin Šipraga]

Hi Linus,

On 9/29/21 11:03 PM, Linus Walleij wrote:
> The RTL8366RB hardware supports disabling learning per-port
> so let's make use of this feature. Rename some unfortunately
> named registers in the process.

Since you have implemented bridge offloading and you are now disabling 
learning on the CPU port by default, will this mean that all ingress 
frames on a user port with DA behind the CPU port will be flooded by the 
switch to all ports in the bridge, as well as the CPU port? It seems 
that will be the case if now the switch can't learn the SA of frames 
coming from the CPU.

Following your discussion with Vladimir [1], did you come to a 
conclusion on how you will handle this?

	Alvin

[1] https://lore.kernel.org/netdev/20210908210939.cwwnwgj3p67qvsrh@skbuf/

> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v3->v4:
> - No changes, rebased on other patches.
> ChangeLog v2->v3:
> - Disable learning by default, learning will be turned
>    on selectively using the callback.
> ChangeLog v1->v2:
> - New patch suggested by Vladimir.
> ---
>   drivers/net/dsa/rtl8366rb.c | 50 ++++++++++++++++++++++++++++++++-----
>   1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index bb9d017c2f9f..b3056064b937 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -14,6 +14,7 @@
>   
>   #include <linux/bitops.h>
>   #include <linux/etherdevice.h>
> +#include <linux/if_bridge.h>
>   #include <linux/interrupt.h>
>   #include <linux/irqdomain.h>
>   #include <linux/irqchip/chained_irq.h>
> @@ -42,9 +43,12 @@
>   /* Port Enable Control register */
>   #define RTL8366RB_PECR				0x0001
>   
> -/* Switch Security Control registers */
> -#define RTL8366RB_SSCR0				0x0002
> -#define RTL8366RB_SSCR1				0x0003
> +/* Switch per-port learning disablement register */
> +#define RTL8366RB_PORT_LEARNDIS_CTRL		0x0002
> +
> +/* Security control, actually aging register */
> +#define RTL8366RB_SECURITY_CTRL			0x0003
> +
>   #define RTL8366RB_SSCR2				0x0004
>   #define RTL8366RB_SSCR2_DROP_UNKNOWN_DA		BIT(0)
>   
> @@ -927,13 +931,14 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>   		/* layer 2 size, see rtl8366rb_change_mtu() */
>   		rb->max_mtu[i] = 1532;
>   
> -	/* Enable learning for all ports */
> -	ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0);
> +	/* Disable learning for all ports */
> +	ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL,
> +			   RTL8366RB_PORT_ALL);
>   	if (ret)
>   		return ret;
>   
>   	/* Enable auto ageing for all ports */
> -	ret = regmap_write(smi->map, RTL8366RB_SSCR1, 0);
> +	ret = regmap_write(smi->map, RTL8366RB_SECURITY_CTRL, 0);
>   	if (ret)
>   		return ret;
>   
> @@ -1272,6 +1277,37 @@ static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port,
>   	return ret;
>   }
>   
> +static int
> +rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> +				struct switchdev_brport_flags flags,
> +				struct netlink_ext_ack *extack)
> +{
> +	/* We support enabling/disabling learning */
> +	if (flags.mask & ~(BR_LEARNING))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
> +			    struct switchdev_brport_flags flags,
> +			    struct netlink_ext_ack *extack)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	int ret;
> +
> +	if (flags.mask & BR_LEARNING) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_LEARNING) ? 0 : BIT(port));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>   {
>   	struct realtek_smi *smi = ds->priv;
> @@ -1682,6 +1718,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>   	.port_vlan_del = rtl8366_vlan_del,
>   	.port_enable = rtl8366rb_port_enable,
>   	.port_disable = rtl8366rb_port_disable,
> +	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
> +	.port_bridge_flags = rtl8366rb_port_bridge_flags,
>   	.port_change_mtu = rtl8366rb_change_mtu,
>   	.port_max_mtu = rtl8366rb_max_mtu,
>   };
> 

Re: [PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Linus Walleij]

On Thu, Sep 30, 2021 at 12:45 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:

> Following your discussion with Vladimir [1], did you come to a
> conclusion on how you will handle this?

I haven't gotten around to running the experiments (short on
time), so I intended to play it safe for now. Unless I feel I have
to.

BTW: all the patches i have left are extensions to RTL8366RB
specifically so I think it should be fine for you to submit patches
for your switch on top of net-next, maybe we can test this
on you chip too, I suspect it works the same on all Realtek
switches?

Yours,
Linus Walleij

Re: [PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Alvin Šipraga]

Hi Linus,

On 10/4/21 10:57 PM, Linus Walleij wrote:
> On Thu, Sep 30, 2021 at 12:45 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> 
>> Following your discussion with Vladimir [1], did you come to a
>> conclusion on how you will handle this?
> 
> I haven't gotten around to running the experiments (short on
> time), so I intended to play it safe for now. Unless I feel I have
> to.

Yeah I understand, it takes some time to figure out how these switches 
really behave... :-)

You have Vladimir's Reviewed-by: tag so I guess this change is OK from 
the maintainer's perspective.

> 
> BTW: all the patches i have left are extensions to RTL8366RB
> specifically so I think it should be fine for you to submit patches
> for your switch on top of net-next, maybe we can test this
> on you chip too, I suspect it works the same on all Realtek
> switches?

Generally speaking I don't think that the patches you have sent for 66RB 
are particularly relevant for the 65MB because the register layout and 
some chip semantics are totally different. Regarding CPU port learning 
for the RTL8365MB chip: right now I am playing around with the "third 
way" Vladimir suggested, by enabling learning selectively only for 
bridge-layer packets (skb->offload_fwd_mark == true). To begin with I'm 
not even sure if you have this capability with the RTL8366RB.

	Alvin

> 
> Yours,
> Linus Walleij
> 

Re: [PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Linus Walleij]

On Tue, Oct 5, 2021 at 9:59 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> On 10/4/21 10:57 PM, Linus Walleij wrote:

> > BTW: all the patches i have left are extensions to RTL8366RB
> > specifically so I think it should be fine for you to submit patches
> > for your switch on top of net-next, maybe we can test this
> > on you chip too, I suspect it works the same on all Realtek
> > switches?
>
> Generally speaking I don't think that the patches you have sent for 66RB
> are particularly relevant for the 65MB because the register layout and
> some chip semantics are totally different.

I was mainly thinking about the crazy VLAN set-up that didn't use
port isolation and which is now deleted. But  maybe you were not
using the rtl8366.c file either? Just realtek-smi.c?

> Regarding CPU port learning
> for the RTL8365MB chip: right now I am playing around with the "third
> way" Vladimir suggested, by enabling learning selectively only for
> bridge-layer packets (skb->offload_fwd_mark == true). To begin with I'm
> not even sure if you have this capability with the RTL8366RB.

This will be interesting to see!

Yours,
Linus Walleij

Re: [PATCH net-next 1/4 v4] net: dsa: rtl8366rb: Support disabling learning

[Alvin Šipraga]

On 10/5/21 4:07 PM, Linus Walleij wrote:
> On Tue, Oct 5, 2021 at 9:59 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
>> On 10/4/21 10:57 PM, Linus Walleij wrote:
> 
>>> BTW: all the patches i have left are extensions to RTL8366RB
>>> specifically so I think it should be fine for you to submit patches
>>> for your switch on top of net-next, maybe we can test this
>>> on you chip too, I suspect it works the same on all Realtek
>>> switches?
>>
>> Generally speaking I don't think that the patches you have sent for 66RB
>> are particularly relevant for the 65MB because the register layout and
>> some chip semantics are totally different.
> 
> I was mainly thinking about the crazy VLAN set-up that didn't use
> port isolation and which is now deleted. But  maybe you were not
> using the rtl8366.c file either? Just realtek-smi.c?

Ah, I was just not using those particularly freaky VLAN functions from 
the rtl8366 library. I still use vlan_{add,del} and the MIB counter 
helpers though, as these seem to be OK. I have been keeping up-to-date 
with your changes to rtl8366.c and tested them locally with my subdriver 
and they are working like a charm. So we should still benefit from some 
level of code reuse.

	Alvin

> 
>> Regarding CPU port learning
>> for the RTL8365MB chip: right now I am playing around with the "third
>> way" Vladimir suggested, by enabling learning selectively only for
>> bridge-layer packets (skb->offload_fwd_mark == true). To begin with I'm
>> not even sure if you have this capability with the RTL8366RB.
> 
> This will be interesting to see!
> 
> Yours,
> Linus Walleij
> 

[PATCH net-next 2/4 v4] net: dsa: rtl8366rb: Support flood control

[Linus Walleij]

Now that we have implemented bridge flag handling we can easily
support flood control as well so let's do it.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- No changes, rebased on the other patches.
ChangeLog v2->v3:
- Move the UNMC under the multicast setting as it is related to
  multicast to unknown address.
- Add some more registers from the API, unfortunately we don't
  know how to make use of them.
- Use tabs for indentation in copypaste bug.
- Since we don't know how to make the elaborate storm control
  work just mention flood control in the message.
ChangeLog v1->v2:
- New patch
---
 drivers/net/dsa/rtl8366rb.c | 55 +++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index b3056064b937..52e750ea790e 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -164,6 +164,26 @@
  */
 #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
 
+/* Storm registers are for flood control
+ *
+ * 02e2 and 02e3 are defined in the header for the RTL8366RB API
+ * but there are no usage examples. The implementation only activates
+ * the filter per port in the CTRL registers.
+ */
+#define RTL8366RB_STORM_FILTERING_1_REG		0x02e2
+#define RTL8366RB_STORM_FILTERING_PERIOD_BIT	BIT(0)
+#define RTL8366RB_STORM_FILTERING_PERIOD_MSK	GENMASK(1, 0)
+#define RTL8366RB_STORM_FILTERING_COUNT_BIT	BIT(1)
+#define RTL8366RB_STORM_FILTERING_COUNT_MSK	GENMASK(3, 2)
+#define RTL8366RB_STORM_FILTERING_BC_BIT	BIT(5)
+#define RTL8366RB_STORM_FILTERING_2_REG		0x02e3
+#define RTL8366RB_STORM_FILTERING_MC_BIT	BIT(0)
+#define RTL8366RB_STORM_FILTERING_UNDA_BIT	BIT(5)
+#define RTL8366RB_STORM_BC_CTRL			0x03e0
+#define RTL8366RB_STORM_MC_CTRL			0x03e1
+#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
+#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
+
 /* LED control registers */
 #define RTL8366RB_LED_BLINKRATE_REG		0x0430
 #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
@@ -1282,8 +1302,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 				struct switchdev_brport_flags flags,
 				struct netlink_ext_ack *extack)
 {
-	/* We support enabling/disabling learning */
-	if (flags.mask & ~(BR_LEARNING))
+	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
+			   BR_MCAST_FLOOD | BR_FLOOD))
 		return -EINVAL;
 
 	return 0;
@@ -1305,6 +1325,37 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
 			return ret;
 	}
 
+	if (flags.mask & BR_BCAST_FLOOD) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
+					 BIT(port),
+					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+	}
+
+	if (flags.mask & BR_MCAST_FLOOD) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
+					 BIT(port),
+					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+		/* UNMC = Unknown multicast address */
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
+					 BIT(port),
+					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+	}
+
+	if (flags.mask & BR_FLOOD) {
+		/* UNDA = Unknown destination address */
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
+					 BIT(port),
+					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH net-next 2/4 v4] net: dsa: rtl8366rb: Support flood control

[Vladimir Oltean]

On Wed, Sep 29, 2021 at 11:03:47PM +0200, Linus Walleij wrote:
> Now that we have implemented bridge flag handling we can easily
> support flood control as well so let's do it.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v3->v4:
> - No changes, rebased on the other patches.
> ChangeLog v2->v3:
> - Move the UNMC under the multicast setting as it is related to
>   multicast to unknown address.
> - Add some more registers from the API, unfortunately we don't
>   know how to make use of them.
> - Use tabs for indentation in copypaste bug.
> - Since we don't know how to make the elaborate storm control
>   work just mention flood control in the message.
> ChangeLog v1->v2:
> - New patch
> ---
>  drivers/net/dsa/rtl8366rb.c | 55 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index b3056064b937..52e750ea790e 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -164,6 +164,26 @@
>   */
>  #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
>  
> +/* Storm registers are for flood control
> + *
> + * 02e2 and 02e3 are defined in the header for the RTL8366RB API
> + * but there are no usage examples. The implementation only activates
> + * the filter per port in the CTRL registers.

The "filter" word bothers me a bit.
Are these settings applied on ingress or on egress? If you have
RTL8366RB_STORM_BC_CTRL == BIT(0) | BIT(1), and a broadcast packet is
received on port 2, then

(a) is it received or dropped?
(b) is it forwarded to port 0 and 1?
(c) is it forwarded to port 3?

> + */
> +#define RTL8366RB_STORM_FILTERING_1_REG		0x02e2
> +#define RTL8366RB_STORM_FILTERING_PERIOD_BIT	BIT(0)
> +#define RTL8366RB_STORM_FILTERING_PERIOD_MSK	GENMASK(1, 0)
> +#define RTL8366RB_STORM_FILTERING_COUNT_BIT	BIT(1)
> +#define RTL8366RB_STORM_FILTERING_COUNT_MSK	GENMASK(3, 2)
> +#define RTL8366RB_STORM_FILTERING_BC_BIT	BIT(5)
> +#define RTL8366RB_STORM_FILTERING_2_REG		0x02e3
> +#define RTL8366RB_STORM_FILTERING_MC_BIT	BIT(0)
> +#define RTL8366RB_STORM_FILTERING_UNDA_BIT	BIT(5)
> +#define RTL8366RB_STORM_BC_CTRL			0x03e0
> +#define RTL8366RB_STORM_MC_CTRL			0x03e1
> +#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
> +#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
> +
>  /* LED control registers */
>  #define RTL8366RB_LED_BLINKRATE_REG		0x0430
>  #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
> @@ -1282,8 +1302,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  				struct switchdev_brport_flags flags,
>  				struct netlink_ext_ack *extack)
>  {
> -	/* We support enabling/disabling learning */
> -	if (flags.mask & ~(BR_LEARNING))
> +	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
> +			   BR_MCAST_FLOOD | BR_FLOOD))
>  		return -EINVAL;
>  
>  	return 0;
> @@ -1305,6 +1325,37 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>  			return ret;
>  	}
>  
> +	if (flags.mask & BR_BCAST_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (flags.mask & BR_MCAST_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +		/* UNMC = Unknown multicast address */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (flags.mask & BR_FLOOD) {
> +		/* UNDA = Unknown destination address */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.31.1
> 

Re: [PATCH net-next 2/4 v4] net: dsa: rtl8366rb: Support flood control

[Alvin Šipraga]

On 9/29/21 11:57 PM, Vladimir Oltean wrote:
> On Wed, Sep 29, 2021 at 11:03:47PM +0200, Linus Walleij wrote:
>> Now that we have implemented bridge flag handling we can easily
>> support flood control as well so let's do it.
>>
>> Cc: Vladimir Oltean <olteanv@gmail.com>
>> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
>> Cc: Mauri Sandberg <sandberg@mailfence.com>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: DENG Qingfang <dqfext@gmail.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> ChangeLog v3->v4:
>> - No changes, rebased on the other patches.
>> ChangeLog v2->v3:
>> - Move the UNMC under the multicast setting as it is related to
>>    multicast to unknown address.
>> - Add some more registers from the API, unfortunately we don't
>>    know how to make use of them.
>> - Use tabs for indentation in copypaste bug.
>> - Since we don't know how to make the elaborate storm control
>>    work just mention flood control in the message.
>> ChangeLog v1->v2:
>> - New patch
>> ---
>>   drivers/net/dsa/rtl8366rb.c | 55 +++++++++++++++++++++++++++++++++++--
>>   1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
>> index b3056064b937..52e750ea790e 100644
>> --- a/drivers/net/dsa/rtl8366rb.c
>> +++ b/drivers/net/dsa/rtl8366rb.c
>> @@ -164,6 +164,26 @@
>>    */
>>   #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
>>   
>> +/* Storm registers are for flood control
>> + *
>> + * 02e2 and 02e3 are defined in the header for the RTL8366RB API
>> + * but there are no usage examples. The implementation only activates
>> + * the filter per port in the CTRL registers.
> 
> The "filter" word bothers me a bit.
> Are these settings applied on ingress or on egress? If you have
> RTL8366RB_STORM_BC_CTRL == BIT(0) | BIT(1), and a broadcast packet is
> received on port 2, then
> 
> (a) is it received or dropped?
> (b) is it forwarded to port 0 and 1?
> (c) is it forwarded to port 3?

Linus, are you sure these STORM_... registers are the right ones to 
control flooding? The doc from Realtek[1] talks briefly about this storm 
control feature, but it seems to be related to rate limiting, not actual 
flooding behaviour.

Just FYI, on the RTL8365MB there are similar storm control registers, 
but the vendor driver doesn't use them to control flooding. Flooding is 
controlled by a set of different registers which allow you to (1) flood, 
(2) flood to a specified portmask, (3) drop, or (4) trap. In the vendor 
driver those registers take names like 
RTL8367C_REG_UNKNOWN_UNICAST_DA_PORT_BEHAVE to control unicast flooding 
on a per-port basis. So there might be something similar for the '66RB.

Originally I thought "storm" was Realtek slang for "flood", but it seems 
that is not the case.

[1] 
https://cdn.jsdelivr.net/gh/libc0607/Realtek_switch_hacking@files/Realtek_Unmanaged_Switch_ProgrammingGuide.pdf

> 
>> + */
>> +#define RTL8366RB_STORM_FILTERING_1_REG		0x02e2
>> +#define RTL8366RB_STORM_FILTERING_PERIOD_BIT	BIT(0)
>> +#define RTL8366RB_STORM_FILTERING_PERIOD_MSK	GENMASK(1, 0)
>> +#define RTL8366RB_STORM_FILTERING_COUNT_BIT	BIT(1)
>> +#define RTL8366RB_STORM_FILTERING_COUNT_MSK	GENMASK(3, 2)
>> +#define RTL8366RB_STORM_FILTERING_BC_BIT	BIT(5)
>> +#define RTL8366RB_STORM_FILTERING_2_REG		0x02e3
>> +#define RTL8366RB_STORM_FILTERING_MC_BIT	BIT(0)
>> +#define RTL8366RB_STORM_FILTERING_UNDA_BIT	BIT(5)
>> +#define RTL8366RB_STORM_BC_CTRL			0x03e0
>> +#define RTL8366RB_STORM_MC_CTRL			0x03e1
>> +#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
>> +#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
>> +
>>   /* LED control registers */
>>   #define RTL8366RB_LED_BLINKRATE_REG		0x0430
>>   #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
>> @@ -1282,8 +1302,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>>   				struct switchdev_brport_flags flags,
>>   				struct netlink_ext_ack *extack)
>>   {
>> -	/* We support enabling/disabling learning */
>> -	if (flags.mask & ~(BR_LEARNING))
>> +	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
>> +			   BR_MCAST_FLOOD | BR_FLOOD))
>>   		return -EINVAL;
>>   
>>   	return 0;
>> @@ -1305,6 +1325,37 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>>   			return ret;
>>   	}
>>   
>> +	if (flags.mask & BR_BCAST_FLOOD) {
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (flags.mask & BR_MCAST_FLOOD) {
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +		/* UNMC = Unknown multicast address */
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (flags.mask & BR_FLOOD) {
>> +		/* UNDA = Unknown destination address */
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.31.1
>>
> 

Re: [PATCH net-next 2/4 v4] net: dsa: rtl8366rb: Support flood control

[Linus Walleij]

On Thu, Sep 30, 2021 at 11:09 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> On 9/29/21 11:57 PM, Vladimir Oltean wrote:
> > On Wed, Sep 29, 2021 at 11:03:47PM +0200, Linus Walleij wrote:

> >> Now that we have implemented bridge flag handling we can easily
> >> support flood control as well so let's do it.

> >> +/* Storm registers are for flood control
> >> + *
> >> + * 02e2 and 02e3 are defined in the header for the RTL8366RB API
> >> + * but there are no usage examples. The implementation only activates
> >> + * the filter per port in the CTRL registers.
> >
> > The "filter" word bothers me a bit.
> > Are these settings applied on ingress or on egress? If you have
> > RTL8366RB_STORM_BC_CTRL == BIT(0) | BIT(1), and a broadcast packet is
> > received on port 2, then
> >
> > (a) is it received or dropped?
> > (b) is it forwarded to port 0 and 1?
> > (c) is it forwarded to port 3?
>
> Linus, are you sure these STORM_... registers are the right ones to
> control flooding? The doc from Realtek[1] talks briefly about this storm
> control feature, but it seems to be related to rate limiting, not actual
> flooding behaviour.

You're probably right. I'll just drop this patch for now.

Yours,
Linus Walleij

[PATCH net-next 3/4 v4] net: dsa: rtl8366rb: Support fast aging

[Linus Walleij]

This implements fast aging per-port using the special "security"
register, which will flush any learned L2 LUT entries on a port.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- No changes, rebased on the other patches.
ChangeLog v2->v3:
- Underscore that this only affects learned L2 entries, not
  static ones.
ChangeLog v1->v2:
- New patch suggested by Vladimir.
---
 drivers/net/dsa/rtl8366rb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 52e750ea790e..748f22ab9130 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -1359,6 +1359,19 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void
+rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct realtek_smi *smi = ds->priv;
+
+	/* This will age out any learned L2 entries */
+	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
+			   BIT(port), BIT(port));
+	/* Restore the normal state of things */
+	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
+			   BIT(port), 0);
+}
+
 static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct realtek_smi *smi = ds->priv;
@@ -1771,6 +1784,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_fast_age = rtl8366rb_port_fast_age,
 	.port_change_mtu = rtl8366rb_change_mtu,
 	.port_max_mtu = rtl8366rb_max_mtu,
 };
-- 
2.31.1

Re: [PATCH net-next 3/4 v4] net: dsa: rtl8366rb: Support fast aging

[Vladimir Oltean]

On Wed, Sep 29, 2021 at 11:03:48PM +0200, Linus Walleij wrote:
> This implements fast aging per-port using the special "security"
> register, which will flush any learned L2 LUT entries on a port.
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v3->v4:
> - No changes, rebased on the other patches.
> ChangeLog v2->v3:
> - Underscore that this only affects learned L2 entries, not
>   static ones.
> ChangeLog v1->v2:
> - New patch suggested by Vladimir.
> ---
>  drivers/net/dsa/rtl8366rb.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 52e750ea790e..748f22ab9130 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -1359,6 +1359,19 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static void
> +rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +
> +	/* This will age out any learned L2 entries */
> +	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> +			   BIT(port), BIT(port));

Is there any delay that needs to be added between these two operations?

> +	/* Restore the normal state of things */
> +	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> +			   BIT(port), 0);
> +}
> +
>  static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
>  	struct realtek_smi *smi = ds->priv;
> @@ -1771,6 +1784,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_fast_age = rtl8366rb_port_fast_age,
>  	.port_change_mtu = rtl8366rb_change_mtu,
>  	.port_max_mtu = rtl8366rb_max_mtu,
>  };
> -- 
> 2.31.1
> 

Re: [PATCH net-next 3/4 v4] net: dsa: rtl8366rb: Support fast aging

[Linus Walleij]

On Wed, Sep 29, 2021 at 11:45 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Sep 29, 2021 at 11:03:48PM +0200, Linus Walleij wrote:

> > +     /* This will age out any learned L2 entries */
> > +     regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> > +                        BIT(port), BIT(port));
>
> Is there any delay that needs to be added between these two operations?
>
> > +     /* Restore the normal state of things */
> > +     regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> > +                        BIT(port), 0);

Absolutely no idea. The API from the vendor essentially just set/clear
this bit with no comments on use.

Yours,
Linus Walleij

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

[Linus Walleij]

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

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

[Vladimir Oltean]

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
> 

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

[Linus Walleij]

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