Re: [PATCH net-next 5/5] net: dsa: realtek: rtl8365mb: handle PHY interface modes correctly

[Alvin Šipraga]

On Tue, Jun 07, 2022 at 11:23:40AM -0300, Luiz Angelo Daros de Luca wrote:
> > From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> >
> > Realtek switches in the rtl8365mb family always have at least one port
> > with a so-called external interface, supporting PHY interface modes such
> > as RGMII or SGMII. The purpose of this patch is to improve the driver's
> > handling of these ports.
> >
> > A new struct rtl8365mb_chip_info is introduced. A static instance of
> > this struct is added for each supported switch, which is distinguished
> > by its chip ID and version. Embedded in each chip_info struct is an
> > array of struct rtl8365mb_extint, describing the external interfaces
> > available. This is more specific than the old rtl8365mb_extint_port_map,
> > which was only valid for switches with up to 6 ports.
> >
> > The struct rtl8365mb_extint also contains a bitmask of supported PHY
> > interface modes, which allows the driver to distinguish which ports
> > support RGMII. This corrects a previous mistake in the driver whereby it
> > was assumed that any port with an external interface supports RGMII.
> > This is not actually the case: for example, the RTL8367S has two
> > external interfaces, only the second of which supports RGMII. The first
> > supports only SGMII and HSGMII. This new design will make it easier to
> > add support for other interface modes.
> >
> > Finally, rtl8365mb_phylink_get_caps() is fixed up to return supported
> > capabilities based on the external interface properties described above.
> > This allows for ports with an external interface to be treated as DSA
> > user ports, and for ports with an internal PHY to be treated as DSA CPU
> > ports.
> 
> That's a nice patch. But while dealing with ext interfaces, wouldn't
> it be nice to also add
> a mask for user ports? We could easily validate if a declared dsa port
> really exists.

At best this will be useful to emit a warning for the device tree author.
Can you see any other benefit?

> 
> ...
> 
> > @@ -1997,33 +2122,27 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >         case RTL8365MB_CHIP_ID_8365MB_VC:
> >                 switch (chip_ver) {
> >                 case RTL8365MB_CHIP_VER_8365MB_VC:
> > -                       dev_info(priv->dev,
> > -                                "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> > -                                chip_ver);
> > +                       mb->chip_info = &rtl8365mb_chip_info_8365mb_vc;
> >                         break;
> >                 case RTL8365MB_CHIP_VER_8367RB_VB:
> > -                       dev_info(priv->dev,
> > -                                "found an RTL8367RB-VB switch (ver=0x%04x)\n",
> > -                                chip_ver);
> > +                       mb->chip_info = &rtl8365mb_chip_info_8367rb_vb;
> >                         break;
> >                 case RTL8365MB_CHIP_VER_8367S:
> > -                       dev_info(priv->dev,
> > -                                "found an RTL8367S switch (ver=0x%04x)\n",
> > -                                chip_ver);
> > +                       mb->chip_info = &rtl8365mb_chip_info_8367s;
> >                         break;
> >                 default:
> > -                       dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
> > -                               chip_ver);
> > +                       dev_err(priv->dev,
> > +                               "unrecognized switch (id=0x%04x, ver=0x%04x)",
> > +                               chip_id, chip_ver);
> >                         return -ENODEV;
> >                 }
> 
> With this patch, we now have a nice chip_info for each device. If we
> group all of them in an array, we could iterate over them instead of
> switching over chip_id/ver. In this case, adding a new variant is just
> a matter of creating a new chip_info and adding to the array. When the
> chip id/ver is only used inside a single chip_info, I don't know if we
> should keep a macro declaring each value. For example,
> 
> #define RTL8365MB_CHIP_ID_8367S         0x6367
> #define RTL8365MB_CHIP_VER_8367S        0x00A0
> ...
> +static const struct rtl8365mb_chip_info rtl8365mb_chip_info_8367s = {
> +       .name = "RTL8367S",
> +       .chip_id = RTL8365MB_CHIP_ID_8367S,
> +       .chip_ver = RTL8365MB_CHIP_VER_8367S,
> +       .extints = {
> +               { 6, 1, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },
> +               { 7, 2, PHY_INTF(MII) | PHY_INTF(TMII) | PHY_INTF(RMII) |
> +                       PHY_INTF(RGMII) },
> +               { /* sentinel */ }
> +       },
> +       .jam_table = rtl8365mb_init_jam_8365mb_vc,
> +       .jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +};
> 
> seems to be as clear as:
> 
> +static const struct rtl8365mb_chip_info rtl8365mb_chip_info_8367s = {
> +       .name = "RTL8367S",
> +       .chip_id = 0x6367,
> +       .chip_ver = 0x00A0,
> +       .extints = {
> +               { 6, 1, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },
> +               { 7, 2, PHY_INTF(MII) | PHY_INTF(TMII) | PHY_INTF(RMII) |
> +                       PHY_INTF(RGMII) },
> +               { /* sentinel */ }
> +       },
> +       .jam_table = rtl8365mb_init_jam_8365mb_vc,
> +       .jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +};
> 
> but smaller and not spread over the source.

Good idea! Albeit at the cost of one more level of indentation ;-)

I'll give it a go for v2.

Kind regards,
Alvin