Re: [PATCH net-next v2 1/2] net: dsa: realtek: rtl8365mb: add SGMII support for RTL8367S

[Luiz Angelo Daros de Luca]

> The RTL8367S can mux its embedded SerDes to external interface 1,
> which is typically used to connect the switch to a CPU port. The chip
> info table already declares SGMII as a supported interface mode for
> this chip, but the driver only implements RGMII so far.
>
> Implement SGMII support, with the configuration sequence derived from
> the GPL-licensed Realtek rtl8367c vendor driver as distributed in the
> Mercusys MR80X GPL code drop:
>
>  - Add accessors for the SerDes indirect access registers (SDS_INDACS),
>    through which the SerDes internal registers are reached.
>
>  - Clear the line rate bypass bit for the external interface, tune the
>    SerDes with the vendor-prescribed parameters, mux the SerDes to MAC8
>    in SGMII mode and only then take the SerDes out of reset, as the
>    vendor driver does.
>
>  - Load firmware into the embedded DW8051 microcontroller, which
>    performs the remaining SerDes initialization. The firmware is
>    requested via the firmware API under the name
>    realtek/rtl8367s-sgmii.bin.
>
>  - Force the SGMII link parameters (link, speed, duplex, flow control)
>    in the SDS_MISC register from the phylink mac_link_up/down
>    operations, in addition to the usual external interface force
>    configuration. SGMII in-band autonegotiation is disabled, so only
>    fixed-link and conventional PHY setups are supported, just like
>    RGMII.
>
> Tested on a Mercusys MR80X v2.20, where the RTL8367S is connected to
> the SoC over SGMII.
>
> Signed-off-by: Johan Alvarado <contact@xxxxxxxx>

This code is based on code from other developers. Please ensure proper
credits/Co-developed-by tags are added if applicable.

> +
> +static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port,
> +                                     phy_interface_t interface)
> +{
> +       const struct rtl8365mb_extint *extint =
> +               rtl8365mb_get_port_extint(priv, port);
> +       u16 val;
> +       int ret;
> +       int i;
> +
> +       if (!extint)
> +               return -ENODEV;
> +
> +       /* The SerDes can only be muxed to external interface 1 */
> +       if (extint->id != 1)
> +               return -EOPNOTSUPP;
> +
> +       /* The vendor driver clears the line rate bypass for all interface
> +        * modes except TMII.
> +        */
> +       ret = regmap_update_bits(priv->map, RTL8365MB_BYPASS_LINE_RATE_REG,
> +                                BIT(extint->id), 0);
> +       if (ret)
> +               return ret;
> +
> +       /* Tune the SerDes with vendor-prescribed parameters */
> +       for (i = 0; i < ARRAY_SIZE(rtl8365mb_sds_jam_sgmii); i++) {
> +               ret = rtl8365mb_sds_write(priv, 0,
> +                                         rtl8365mb_sds_jam_sgmii[i].reg,
> +                                         rtl8365mb_sds_jam_sgmii[i].val);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* Mux the SerDes to MAC8 in SGMII mode */
> +       ret = regmap_update_bits(priv->map, RTL8365MB_SDS_MISC_REG,
> +                                RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK |
> +                                        RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK,
> +                                RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_update_bits(
> +               priv->map, RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(extint->id),
> +               RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(extint->id),
> +               RTL8365MB_EXT_PORT_MODE_SGMII
> +                       << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(
> +                                  extint->id));
> +       if (ret)
> +               return ret;
> +
> +       /* Take the SerDes out of reset. The vendor driver does this only
> +        * after the SerDes mux and the interface mode are configured.
> +        */
> +       ret = rtl8365mb_sds_write(priv, 0, RTL8365MB_SDS_REG_RESET,
> +                                 RTL8365MB_SDS_RESET_DEASSERT);
> +       if (ret)
> +               return ret;
> +
> +       /* The embedded microcontroller performs the remaining SerDes
> +        * initialization. Load its firmware and give it some time to bring
> +        * the SerDes up.
> +        */
> +       ret = rtl8365mb_dw8051_load_firmware(priv);

I checked the vendor firmware behavior and it essentially initializes
the interface and runs an infinite loop polling for link state
changes. Crucially, it writes to the exact same registers we already
manage in this driver via phylink:

- RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0
- RTL8365MB_DIGITAL_INTERFACE_SELECT_REG1
- RTL8365MB_DIGITAL_INTERFACE_FORCE_REG0
- RTL8365MB_DIGITAL_INTERFACE_FORCE_REG1
- RTL8365MB_DIGITAL_INTERFACE_FORCE_REG2
- RTL8365MB_EXT_RGMXF_REG0

By allowing the DW8051 to run, we introduce a "two heads controlling
the switch" scenario. The firmware loops polling for port status and
sets REG_EXT_FORCE_{0,1,2} behind our backs. While this might go
unnoticed in a strictly fixed-link topology, it introduces race
conditions and unexpected behaviors if phylink is also manipulating
these registers.

Furthermore, analyzing the firmware reveals that right after
deasserting the SerDes Reset, it applies the following sequence:

rtl8365mb_sds_write(priv, 0, 0x0000, 0x1401);
rtl8365mb_sds_write(priv, 0, 0x0000, 0x1403);

These are writes to the SerDes Basic Mode Control Register (BMCR,
Register 0). 0x1400 asserts BMCR_ANENABLE | BMCR_ISOLATE. Toggling the
lower vendor-specific bits (0x01 to 0x03) triggers a data-path reset /
PLL resync. This clears out FIFOs and ensures a clean cold boot for
the SGMII link, preventing silent drops or CRC errors.

I think we don't need the firmware at all. Requesting a proprietary
binary that we don't control to do things phylink already does is
unnecessary and adds a burden to linux-firmware.
I strongly suggest dropping rtl8365mb_dw8051_load_firmware() entirely,
porting the BMCR reset sequence above directly into the driver, and
keeping the 8051 disabled.

> +       /* Pause the microcontroller while accessing the SerDes directly */
> +       ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0_REG,
> +                                RTL8365MB_MISC_CFG0_DW8051_EN_MASK, 0);
> +       if (ret)
> +               return ret;
> +
> +       /* Disable SGMII in-band autonegotiation: the link parameters are
> +        * forced in rtl8365mb_phylink_mac_link_up.
> +        */
> +       ret = rtl8365mb_sds_read(priv, 0, RTL8365MB_SDS_REG_NWAY, &val);
> +       if (ret)
> +               return ret;
> +

If we intend to support non-fixed links in the future (where in-band
AN is required), we would need to implement phylink_pcs and
pcs_get_state instead of just forcing the MAC. For fixed-link it is
fine, but we should plan accordingly.

> +       val &= ~RTL8365MB_SDS_NWAY_EN_MASK;
> +       val |= RTL8365MB_SDS_NWAY_RESTART_MASK;
> +
> +       ret = rtl8365mb_sds_write(priv, 0, RTL8365MB_SDS_REG_NWAY, val);
> +       if (ret)
> +               return ret;
> +
> +       return regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0_REG,
> +                                 RTL8365MB_MISC_CFG0_DW8051_EN_MASK,
> +                                 RTL8365MB_MISC_CFG0_DW8051_EN_MASK);
> +}
> +

Regards,

Luiz