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

[Luiz Angelo Daros de Luca]

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

...

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

Regards,

Luiz