Re: [PATCH net-next v7 04/10] net: dsa: realtek: rtl8365mb: set RGMII mode for family D

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

On Thu, Nov 05, 2026 at 08:56:04AM +0100, Mieczyslaw Nalewaj wrote:
> On Wed, Nov 04, 2026 at 01:39:59PM +0100, Alvin Šipraga wrote:
> > On Mon, Nov 02, 2026 at 03:39:47PM +0100, Mieczyslaw Nalewaj wrote:
> > > On family D, RGMII on extension interface 1 needs two things that the
> > > other families do not: the EXT1 RGMII TX clock delay field in
> > > EXT_TXC_DLY (0x13f9, bits [5:3]) must be cleared, and the pin mux in
> > > the top-level configuration register must be set up.
> > >
> > > The vendor RGMII setup for EXT1 clears the RGMII field of EXT_TXC_DLY
> > > (0x13f9, bits [5:3]) in addition to programming TXDELAY in RGMXF.
> > > Do the same. The init jam table writes 0x0090 to this register, which
> > > leaves the field at 2, so without the clear an extra TX delay would be
> > > applied on top of tx-internal-delay-ps. Clearing it makes
> > > tx-internal-delay-ps the only source of TX delay.
> >
> > Should we not just clear it with the jam table then? It would save some
> > code here, which is just undoing what the jam table did anyway. Or am I
> > missing something? If you deviate from the vendor driver's jam table
> > then a comment inline or in its own commit could be appropriate. But
> > either way it seems it would save some code here?
> 
> The jam table rtl8365mb_init_jam_8365mb_vc is shared between family C
> and D. Changing the 0x13f9 entry from 0x0090 to 0x0080 would also
> change family C behaviour, which I cannot test. Duplicating the whole
> table for a single word seemed worse than two lines of code. Also, the
> vendor code clears this field only when configuring RGMII on EXT1, not
> on every init, so doing it in ext_config_rgmii() matches that.
> 
> If you would still prefer a separate table for D, family_info->jam_table
> already allows it and I can do that in v10.

I lifted the jam tables from the vendor driver mostly verbatim, and
without spending much time deciphering them. During your work on the D
family, did you find that the vendor driver is programming the same
values?

One thing I'm wondering is: for the other C family chips, is some extra
RGMII TX delay being silently added by the jam table we already have? Or
does it have some other meaning on the C family? I never got any
complaints from the hardware engineers about the RGMII timing so I am
kind of inclined to say that it might do something else. That was with
RTL8365MB-VC which uses EXT1.

Anyway, if the jam table the vendor driver uses is indeed the same for
both families, maybe it's best to keep it explicit like you have
done.

> 
> > > If the internal MAC4 block occupies extension 1, do not advertise RGMII
> > > for port 7 in phylink_get_caps(), so the configuration is refused before
> > > any EXT1 register is touched. [...]
> > >
> > > [...]
> > > +    if (extint->supported_interfaces &
> RTL8365MB_PHY_INTERFACE_MODE_RGMII) {
> > > +        /* On family D the EXT1 pins are shared with MAC4 (UTP port 4).
> > > +         * If MAC4 owns them, RGMII on this port cannot work.
> > > +         */
> > > +        if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D &&
> > > +            extint->id == 1) {
> > > +            ret = regmap_read(priv->map, RTL8365MB_D_REG_TOP_CON0,
> &data);
> > > [...]
> >
> > I am a little confused about everything that's going on here, but it's
> > probably just me. Could you perhaps explain the two scenarios and the
> > order in which you expect things to happen? I'm sort of wondering when
> > this warning might be emitted. Part of me also wonders if you can't
> > avoid doing a regmap_read() here in phylink_get_caps.
> 
> On family D the EXT1 pins are shared between MAC7 (the port described
> in the DT) and the internal MAC4 (UTP port 4). TOP_CON0 bit 12 tells
> which of them owns the pins. This comes from the vendor RTL8367D API,
> which refuses RGMII on port 7 when UTP port 4 uses the RGMII interface.
> The driver never writes bit 12 and I do not know what sets its value
> at reset (strap, EEPROM or bootloader); I do not have a board where
> it is set, so I could not test that case. That is why get_caps() reads
> the bit instead of assuming a value.
> 
> 1) Bit 12 set (MAC4 owns EXT1):
>    - get_caps() does not advertise RGMII for port 7,
>    - phylink refuses an RGMII phy-mode for that port,
>    - ext_config_rgmii() is never reached, so no EXT1 register is
>      touched,
>    - the warning is printed from get_caps() when phylink is created
>      for the port.
> 
> 2) Bit 12 clear (MAC4 does not own EXT1):
>    - get_caps() advertises RGMII,
>    - ext_config_rgmii() clears the EXT1 TX delay field, binds the pins
>      to MAC7 (MAC7_SEL_EXT1 in TOP_CON0) and disables SerDes 1, all
>      before RGMII is selected, so the shared pins are never driven by
>      RGMII and SerDes 1 at the same time.

Do you have a datasheet I can look at? I couldn't find one.

I'm also a bit confused by what it means for MAC4 to "own" EXT1. Are we
talking about physical pin muxing (i.e. pins on the package can have
different function), or is this internal muxing? And since you say MAC4
is a UTP port, I don't understand how it can also be associated with
EXT1 (an MII/RGMII interface).

> 
> You are right that the regmap_read() in get_caps() is not nice. It also
> means the warning is printed whenever get_caps() runs for that port,
> even if the DT does not use RGMII there.
> 
> Since the driver never modifies the bit, reading it once in
> rtl8365mb_setup() should be sufficient. I could store it in
> struct rtl8365mb (e.g. bool ext1_owned_by_mac4) and have get_caps()
> only test that flag, so it no longer touches the regmap. The warning
> could then be moved to setup and printed only when the DT actually
> requests RGMII on port 7. Would you like me to do that in v10?

Since MAC7_SEL_EXT1 is writable, I would guess that so is
MAC4_SEL_EXT1. Could we not instead keep it configurable, and do some
bookkeeping in the driver to see who has requested these pins? Then,
whenever either is configured, you first check if it's already in use,
warn if so, or otherwise set it up and mark it as in-use.

Hope that makes sense.

Kind regards,
Alvin