Re: [PATCH net-next v7 10/10] net: dsa: realtek: rtl8365mb: add support for RTL8367S-VB

["Alvin Šipraga" <alvin.sipraga@analog.com>]

Hi Mieczyslaw,

First of all, apologies for such a late review! I hope you appreciate
the feedback anyways.

On Mon, Nov 02, 2026 at 03:45:37PM +0100, Mieczyslaw Nalewaj wrote:
> Add the RTL8367S-VB (chip ID 0x6642, version 0x0010) to the chip table.
> It is the first chip of family D, whose handling was added in the
> preceding patches, so this is the patch that makes it reachable.
> 
> The SGMII/HSGMII SerDes of this chip sits on port 6 and the
> MII/TMII/RMII/RGMII interface on port 7, as on family C, but the external
> interface indices are one lower (EXT0 and EXT1 instead of EXT1 and EXT2).
> The chip reuses the existing initialization jam table.
> 
> The chip has 8 ports, not 11. Cap priv->num_ports at
> RTL8365MB_D_MAX_NUM_PORTS for family D. This is a functional change: it
> sets ds->num_ports, the IRQ domain size and the bound of every loop over
> priv->num_ports.

Something about this below...

> 
> The L2 learning limit is the same as on family C (2112 entries), as
> given by the Realtek rtl8367 common API (rtl8367_common V1.4.2), so
> RTL8365MB_LEARN_LIMIT_MAX needs no per-family handling; only its
> comment is updated.
> 
> Tested on a TP-Link Archer AX55 v1 (RTL8367S-VB, CPU port over HSGMII
> to an IPQ5018) with bridge VLAN filtering and per-port PVIDs. SGMII has
> also been seen working, but I no longer have the exact link partner and
> speed it was tested with, so treat it as lightly tested. RGMII mode has
> not yet been tested.
> 
> Signed-off-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/net/dsa/realtek/Kconfig          |  3 ++-
>  drivers/net/dsa/realtek/rtl8365mb_main.c | 32 +++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index d6eb671..98b1727 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -34,7 +34,8 @@ config NET_DSA_REALTEK_RTL8365MB
>  	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
>  	select NET_DSA_TAG_RTL8_4
>  	help
> -	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
> +	  Select to enable support for Realtek RTL8365MB-VC, RTL8367S and
> +	  RTL8367S-VB.
>  
>  config NET_DSA_REALTEK_RTL8366RB
>  	tristate "Realtek RTL8366RB switch driver"
> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 1c5f139..415468e 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -89,6 +89,12 @@
>   * things will work out-of-the-box for other chips, and a careful review of the
>   * vendor driver may be needed to expand support. The RTL8365MB-VC seems to be
>   * one of the simpler chips.
> + *
> + * The RTL8367S-VB (chip ID 0x6642) is not part of the family listed above. It
> + * belongs to a newer generation, which this driver calls family D. Its
> + * registers differ in the VLAN table, the external interface speed and RGMII
> + * configuration and the SerDes, so the code paths specific to it are selected
> + * with rtl8365mb_get_family().

More below, but I hope we can sort of avoid too much branching of this
kind.

>   */
>  
>  #include <linux/bitfield.h>
> @@ -120,9 +126,10 @@
>  #define RTL8365MB_NUM_PHYREGS		32
>  #define RTL8365MB_PHYREGMAX		(RTL8365MB_NUM_PHYREGS - 1)
>  #define RTL8365MB_MAX_NUM_PORTS		11
> -/* Valid for the whole family except RTL8370B, which has 4160 entries.
> - * RTL8370B is mentioned in vendor code but it might not even belong
> - * to the same RTL8367C family.
> +#define RTL8365MB_D_MAX_NUM_PORTS	8

This is OK, but in the same spirit as my comment to your other patch, I
think we want to make the C and D families equal. So why not:

-#define RTL8365MB_MAX_NUM_PORTS	11
+#define RTL8365MB_C_MAX_NUM_PORTS	11
+#define RTL8365MB_D_MAX_NUM_PORTS	8

and add a num_ports member to struct rtl8365mb_chip_info. Then below:

[...]

>  enum rtl8365mb_stp_state {
> @@ -3996,7 +4016,11 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  
>  	dev_info(priv->dev, "found an %s switch\n", mb->chip_info->name);
>  
> -	priv->num_ports = RTL8365MB_MAX_NUM_PORTS;
> +	if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D)
> +		priv->num_ports = RTL8365MB_D_MAX_NUM_PORTS;
> +	else
> +		priv->num_ports = RTL8365MB_MAX_NUM_PORTS;
> +

The above becomes something like

priv->num_ports = mb->chip_info->num_ports;

The reason I'm saying this is that I saw quite a few places where you
have a construct of the form:

if (family is d)
  // use family d constant
else
  // use family c constant

Which can become a bit hard to follow/maintain.

I haven't yet had the time to look closely at all the patches (I picked
the other one first because it was missing a Reviewed-by). But I'll try
and take a closer look tomorrow and point out where else we could
improve in this direction. I hope it's possible to remove most of the
if (family is x) { do something } constructs and keep things flat.

Another thing I noticed in the L2 patch was that you added a macro:

#define RTL8365MB_D_L2_MAX_PORT 7

followed by:

@@ -286,17 +428,23 @@ int rtl8365mb_l2_get_next_uc(struct realtek_priv *priv, u16 *addr, int port,
        struct rtl8365mb_l2_uc uc;
        int ret;

+       if (rtl8365mb_l2_is_d(priv) && port > RTL8365MB_D_L2_MAX_PORT)
+               return -EINVAL;
+

Which I wonder, can you even get there if the num_ports is 8? We didn't
need that protection for family C. I think you can drop the macro and
this check.

Kind regards,
Alvin