Re: [PATCH net-next v7 03/10] net: dsa: realtek: rtl8365mb: set speed for family D

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

On Mon, Nov 02, 2026 at 03:38:53PM +0100, Mieczyslaw Nalewaj wrote:
> On family C hardware, forcing a 2.5 Gbps link speed requires relying
> strictly on the HSGMII SerDes configuration block, while forcing a dummy
> 1 Gbps value inside the standard digital interface register. Family D
> introduces a dedicated hardware link speed value (5) for 2500M. The speed
> bitfield width has been expanded to 3 bits, splitting the most significant
> bit into a new separate register field (FORCE_SPEED2), which maps directly
> to a distinct per-port digital interface force layout.
> 
> On family D the forced-mode configuration (link, duplex, pause, speed)
> is written to a per-port register at 0x12c0 + port instead of the
> per-extint register. The bit positions of TXPAUSE, RXPAUSE, LINK, DUPLEX
> and the low two speed bits are the same as on family C; the upper speed
> bits go to bits 13:12 (FORCE_SPEED2), so FORCE_EN (bit 12 on family C)
> is not set. A second per-port register at 0x12c8 + port enables the
> forced mode: the vendor RTL8367D API writes 0xFFFF there when the MAC
> is forced and 0 otherwise. This driver always forces the MAC, as on
> family C, so it writes 0xFFFF on every call, including link down.
> 
> The family D branch is not reachable until the chip table entry is
> added in the last patch.
> 
> Signed-off-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/net/dsa/realtek/rtl8365mb_main.c | 82 ++++++++++++++++++++----
>  1 file changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 840db5a..2dabc4d 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -344,6 +344,7 @@
>  #define RTL8365MB_PORT_SPEED_10M	0
>  #define RTL8365MB_PORT_SPEED_100M	1
>  #define RTL8365MB_PORT_SPEED_1000M	2
> +#define RTL8365MB_D_PORT_SPEED_2500M	5

I'm starting to doubt the value of these macros now that I see this _D
exception too. Maybe it's more straightforward to inline the values in
the function.

>  
>  /* External interface force configuration registers 0~2 */
>  #define RTL8365MB_DIGITAL_INTERFACE_FORCE_REG0		0x1310 /* EXT0 */
> @@ -361,6 +362,28 @@
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_LINK_MASK		BIT(4)
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK		BIT(2)
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK		GENMASK(1, 0)
> +/* Width of the FORCE_SPEED field, bits[1:0] */
> +#define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_WIDTH		2

We don't have any other _WIDTH macros so my instinct is that you can
dispose of this...

> +
> +/* bits[13:12]; bit 13 reserved, no defined speed value uses it yet */
> +#define   RTL8365MB_D_DIGITAL_INTERFACE_FORCE_SPEED2_MASK	GENMASK(13, 12)
> +
> +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_BASE		0x12c0
> +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG(_port) \
> +		(RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_BASE + (_port))
> +
> +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_BASE		0x12c8
> +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_EN(_port) \
> +		(RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_BASE + (_port))
> +
> +/* Per-port force-mode enable. The vendor RTL8367D API
> + * (dal_rtl8367d_port_macForceLink_set()) treats it as an on/off switch for
> + * the whole forced configuration: all bits set when the MAC is forced, zero
> + * when it is not. The meaning of the individual bits is not documented.
> + * This driver always forces the MAC, as on family C, so all bits are set
> + * on every call, including link down.
> + */
> +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_ALL_MASK	GENMASK(15, 0)

Ahh, at this rate, maybe the ext_config_forcemode() should just be split
into two functions per family. Since you're now writing to completely
different registers depending on the family, which are only somewhat
similar. Can you try it and see how it looks?

I'd then also update the "old" C register macros to have the
RTL8365MB_C_ prefix. That way we have an approach in the driver where
non-common registers are labelled by their family.

And as above, you can then just inline the values for r_speed and
r_speed2. It's not pretty but the global macros do not really help IMO.

Again, try it out, if you don't like it, then I guess this is still OK.

Kind regards,
Alvin