Re: [PATCH net-next v7 09/10] net: dsa: realtek: rtl8365mb: make L2/LUT access work on family D

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

On Mon, Nov 02, 2026 at 03:44:36PM +0100, Mieczyslaw Nalewaj wrote:
> The L2 lookup table on family D switches (e.g. RTL8367S-VB) differs
> from family C in ways that make the current FDB/MDB code misbehave:
> 
>  - An L2 entry is 5 words instead of 6, with a different field layout
>    in words 3 and 4 (port, age, static and multicast member bits).
>  - There is no EFID and no per-bridge FDB isolation. The register at
>    0x0A32, used as the per-port EFID register on family C, is the LUT
>    aging timer on family D, so writing an EFID there corrupts it.
>  - In the table access interface, the SPA field is 3 bits wide and the
>    address register is 12 bits wide, with no address extension bit.
>  - Only ports 0..7 are present in the LUT and there is no
>    FLUSH_PORT_EXT register.
> 
> Teach the table and L2 code about these differences, selecting the
> layout at runtime with rtl8365mb_get_family(). Family C behaviour is
> unchanged.
> 
> Specifically:
> 
>  - rtl8365mb_table: use the family D port mask and address width, and
>    read the hit address from status bits [11:0].
>  - rtl8365mb_l2: add family D unicast/multicast entry (de)serialisers
>    behind *_any() wrappers, pass the proper entry size to the table
>    layer, reject ports above 7, report SVL (FID) entries with VID 0.
>    Use age 6 for static unicast entries as the vendor code does, and
>    look an entry up before deleting it so that a missing entry is not
>    allocated by the delete.
>  - rtl8365mb_main: make port_set_efid() a no-op on family D, add a LUT
>    init step that clears IPMC_HASH and BCAM_DISABLE and programs the
>    aging timer, and do not advertise fdb_isolation or
>    max_num_bridges. Traffic between bridges is then isolated by the
>    port isolation masks only.
> 
> Register layouts were taken from the family D register description;
> the code itself is an independent implementation.
> 
> No chip uses family D before the last patch of the series, so this code
> is unreachable until then.
> 
> Signed-off-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
> ---
>  drivers/net/dsa/realtek/rtl8365mb_l2.c    | 216 ++++++++++++++++++++--
>  drivers/net/dsa/realtek/rtl8365mb_main.c  |  46 ++++-
>  drivers/net/dsa/realtek/rtl8365mb_table.c |  46 ++++-
>  3 files changed, 276 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb_l2.c b/drivers/net/dsa/realtek/rtl8365mb_l2.c
> index 0494d8a..a7613e2 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_l2.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_l2.c
> @@ -8,9 +8,12 @@
>  
>  #include "rtl8365mb_l2.h"
>  #include "rtl8365mb_table.h"
> +#include "rtl8365mb.h"
>  #include <linux/regmap.h>
>  
>  #define RTL8365MB_L2_ENTRY_SIZE			6
> +#define RTL8365MB_D_L2_ENTRY_SIZE		5
> +#define RTL8365MB_D_L2_MAX_PORT			7
>  
>  #define RTL8365MB_L2_UC_D0_MAC5_MSK		GENMASK(7, 0)
>  #define RTL8365MB_L2_UC_D0_MAC4_MSK		GENMASK(15, 8)
> @@ -58,6 +61,21 @@
>  #define RTL8365MB_L2_MC_D5_MBR_HI2_MSK		GENMASK(7, 7)
>  #define   RTL8365MB_L2_MC_MBR_HI2_MSK		GENMASK(10, 10)
>  
> +/* Family D unicast (auto-learn / static) entry */
> +#define RTL8365MB_D_L2_UC_D3_VID_MSK		GENMASK(11, 0)
> +#define RTL8365MB_D_L2_UC_D3_IVL_MSK		BIT(13)
> +#define RTL8365MB_D_L2_UC_D3_PORT_LO_MSK	GENMASK(15, 14)
> +#define RTL8365MB_D_L2_UC_D4_PORT_HI_MSK	BIT(0)
> +#define RTL8365MB_D_L2_UC_D4_AGE_MSK		GENMASK(3, 1)
> +#define RTL8365MB_D_L2_UC_D4_STATIC_MSK		BIT(6)
> +
> +/* Family D multicast entry */
> +#define RTL8365MB_D_L2_MC_D3_VID_MSK		GENMASK(11, 0)
> +#define RTL8365MB_D_L2_MC_D3_IVL_MSK		BIT(13)
> +#define RTL8365MB_D_L2_MC_D3_MBR_LO_MSK		GENMASK(15, 14)
> +#define RTL8365MB_D_L2_MC_D4_MBR_HI_MSK		GENMASK(5, 0)
> +#define RTL8365MB_D_L2_MC_D4_STATIC_MSK		BIT(6)
> +
>  /* Port flush command registers - writing a 1 to the port's MASK bit will
>   * initiate the flush procedure. Completion is signalled when the corresponding
>   * BUSY bit is 0.
> @@ -258,6 +276,130 @@ static void rtl8365mb_l2_mc_to_data(const struct rtl8365mb_l2_mc *mc, u16 *data)
>  	data[5] |= FIELD_PREP(RTL8365MB_L2_MC_D5_MBR_HI2_MSK, val);
>  }
>  
> +static bool rtl8365mb_l2_is_d(struct realtek_priv *priv)
> +{
> +	return rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D;
> +}

Shouldn't this be a top-level function, like rtl8365mb_is_d()?

> +
> +static size_t rtl8365mb_l2_entry_size(struct realtek_priv *priv)
> +{
> +	return rtl8365mb_l2_is_d(priv) ? RTL8365MB_D_L2_ENTRY_SIZE :
> +					 RTL8365MB_L2_ENTRY_SIZE;
> +}
> +
> +static void rtl8365mb_l2_mac_to_data(const u8 *m, u16 *data)
> +{
> +	data[0] = (m[4] << 8) | m[5];
> +	data[1] = (m[2] << 8) | m[3];
> +	data[2] = (m[0] << 8) | m[1];
> +}
> +
> +static void rtl8365mb_l2_data_to_mac(const u16 *data, u8 *m)
> +{
> +	m[5] = data[0] & 0xff;
> +	m[4] = data[0] >> 8;
> +	m[3] = data[1] & 0xff;
> +	m[2] = data[1] >> 8;
> +	m[1] = data[2] & 0xff;
> +	m[0] = data[2] >> 8;
> +}

From the diff it would seem that this is specific to the D family, since
you only use it in those functions. But looking over my crappy original
code for the C family, it's quite the same is it not? Can't you update
the C family functions to use these helpers too?

> +
> +static void rtl8365mb_d_l2_data_to_uc(const u16 *data,
> +				      struct rtl8365mb_l2_uc *uc)
> +{
> +	rtl8365mb_l2_data_to_mac(data, uc->key.mac_addr);
> +	uc->key.vid = FIELD_GET(RTL8365MB_D_L2_UC_D3_VID_MSK, data[3]);
> +	uc->key.ivl = FIELD_GET(RTL8365MB_D_L2_UC_D3_IVL_MSK, data[3]);
> +	uc->key.fid = 0;
> +	uc->key.efid = 0;
> +	uc->port = FIELD_GET(RTL8365MB_D_L2_UC_D3_PORT_LO_MSK, data[3]) |
> +		   (FIELD_GET(RTL8365MB_D_L2_UC_D4_PORT_HI_MSK, data[4]) << 2);
> +	uc->age = FIELD_GET(RTL8365MB_D_L2_UC_D4_AGE_MSK, data[4]);
> +	uc->is_static = FIELD_GET(RTL8365MB_D_L2_UC_D4_STATIC_MSK, data[4]);
> +}
> +
> +static void rtl8365mb_d_l2_uc_to_data(const struct rtl8365mb_l2_uc *uc,
> +				      u16 *data)
> +{
> +	memset(data, 0, RTL8365MB_L2_ENTRY_SIZE * 2);
> +	rtl8365mb_l2_mac_to_data(uc->key.mac_addr, data);
> +	data[3] = FIELD_PREP(RTL8365MB_D_L2_UC_D3_VID_MSK, uc->key.vid) |
> +		  FIELD_PREP(RTL8365MB_D_L2_UC_D3_IVL_MSK, uc->key.ivl) |
> +		  FIELD_PREP(RTL8365MB_D_L2_UC_D3_PORT_LO_MSK, uc->port & 0x3);
> +	data[4] = FIELD_PREP(RTL8365MB_D_L2_UC_D4_PORT_HI_MSK,
> +			     (uc->port >> 2) & 0x1) |
> +		  FIELD_PREP(RTL8365MB_D_L2_UC_D4_AGE_MSK, uc->age) |
> +		  FIELD_PREP(RTL8365MB_D_L2_UC_D4_STATIC_MSK, uc->is_static);
> +}
> +
> +static void rtl8365mb_d_l2_data_to_mc(const u16 *data,
> +				      struct rtl8365mb_l2_mc *mc)
> +{
> +	rtl8365mb_l2_data_to_mac(data, mc->key.mac_addr);
> +	mc->key.vid = FIELD_GET(RTL8365MB_D_L2_MC_D3_VID_MSK, data[3]);
> +	mc->key.ivl = FIELD_GET(RTL8365MB_D_L2_MC_D3_IVL_MSK, data[3]);
> +	mc->member = FIELD_GET(RTL8365MB_D_L2_MC_D3_MBR_LO_MSK, data[3]) |
> +		     (FIELD_GET(RTL8365MB_D_L2_MC_D4_MBR_HI_MSK, data[4]) << 2);
> +	mc->is_static = FIELD_GET(RTL8365MB_D_L2_MC_D4_STATIC_MSK, data[4]);
> +	mc->igmpidx = 0;
> +	mc->priority = 0;
> +	mc->fwd_pri = 0;
> +	mc->igmp_asic = 0;
> +}
> +
> +static void rtl8365mb_d_l2_mc_to_data(const struct rtl8365mb_l2_mc *mc,
> +				      u16 *data)
> +{
> +	memset(data, 0, RTL8365MB_L2_ENTRY_SIZE * 2);
> +	rtl8365mb_l2_mac_to_data(mc->key.mac_addr, data);
> +	data[3] = FIELD_PREP(RTL8365MB_D_L2_MC_D3_VID_MSK, mc->key.vid) |
> +		  FIELD_PREP(RTL8365MB_D_L2_MC_D3_IVL_MSK, mc->key.ivl) |
> +		  FIELD_PREP(RTL8365MB_D_L2_MC_D3_MBR_LO_MSK, mc->member & 0x3);
> +	data[4] = FIELD_PREP(RTL8365MB_D_L2_MC_D4_MBR_HI_MSK,
> +			     (mc->member >> 2) & 0x3f) |
> +		  FIELD_PREP(RTL8365MB_D_L2_MC_D4_STATIC_MSK, mc->is_static);
> +}
> +
> +static void rtl8365mb_l2_data_to_uc_any(struct realtek_priv *priv,
> +					const u16 *data,
> +					struct rtl8365mb_l2_uc *uc)
> +{
> +	if (rtl8365mb_l2_is_d(priv))
> +		rtl8365mb_d_l2_data_to_uc(data, uc);
> +	else
> +		rtl8365mb_l2_data_to_uc(data, uc);
> +}

Can't we have:

rtl8365mb_l2_data_to_uc() # same as before at all callsites

call into either:

rtl8365mb_c_l2_data_to_uc()
rtl8365mb_d_l2_data_to_uc()

instead?

Right now it looks quite asymmetric. And the _any() is confusing when
reading the code downstairs. (One wonders, what does "any" mean? Of
course, it's obvious when you read this function.)

Even though family C is the originally supported family, I guess D is an
equal citizen, so it's good to keep things in this driver symmetric.

> +
> +static void rtl8365mb_l2_uc_to_data_any(struct realtek_priv *priv,
> +					const struct rtl8365mb_l2_uc *uc,
> +					u16 *data)
> +{
> +	if (rtl8365mb_l2_is_d(priv))
> +		rtl8365mb_d_l2_uc_to_data(uc, data);
> +	else
> +		rtl8365mb_l2_uc_to_data(uc, data);
> +}
> +
> +static void rtl8365mb_l2_data_to_mc_any(struct realtek_priv *priv,
> +					const u16 *data,
> +					struct rtl8365mb_l2_mc *mc)
> +{
> +	if (rtl8365mb_l2_is_d(priv))
> +		rtl8365mb_d_l2_data_to_mc(data, mc);
> +	else
> +		rtl8365mb_l2_data_to_mc(data, mc);
> +}
> +
> +static void rtl8365mb_l2_mc_to_data_any(struct realtek_priv *priv,
> +					const struct rtl8365mb_l2_mc *mc,
> +					u16 *data)
> +{
> +	if (rtl8365mb_l2_is_d(priv))
> +		rtl8365mb_d_l2_mc_to_data(mc, data);
> +	else
> +		rtl8365mb_l2_mc_to_data(mc, data);
> +}
> +

The above comment applies to all these _any() functions.

Kind regards,
Alvin