Messages in this thread | | | From | Alvin Šipraga <> | Subject | Re: [PATCH net-next 5/5] net: dsa: realtek: rtl8365mb: handle PHY interface modes correctly | Date | Tue, 7 Jun 2022 14:29:39 +0000 |
| |
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 | |