lkml.org 
[lkml]   [2022]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH net-next 5/5] net: dsa: realtek: rtl8365mb: handle PHY interface modes correctly
Date
On Tue, Jun 07, 2022 at 11:23:40AM -0300, Luiz Angelo Daros de Luca wrote:
> > From: Alvin Šipraga <alsi@bang-olufsen.dk>
> >
> > 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
\
 
 \ /
  Last update: 2022-06-07 16:31    [W:0.056 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site