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

[Mieczyslaw Nalewaj <namiltd@yahoo.com>]

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.

 > > 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.

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?

Kind regards,
Mieczyslaw