Message-ID: <CAJq09z6mCrg58_RwygetF2hnWV0Kq5YMYWOg4sF3eLAASzqJTg@mail.gmail.com>
Date:   Thu, 30 Dec 2021 21:18:23 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v2 12/13] net: dsa: realtek: rtl8365mb: add
 RTL8367S support

> > @@ -31,7 +31,10 @@ config NET_DSA_REALTEK_RTL8365MB
> >       depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
> >       select NET_DSA_TAG_RTL8_4
> >       help
> > -       Select to enable support for Realtek RTL8365MB
> > +       Select to enable support for Realtek RTL8365MB-VC and RTL8367S. This subdriver
> > +       might also support RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB, RTL8364NB,
> > +       RTL8364NB-VB, RTL8366SC, RTL8367RB-VB, RTL8367SB, RTL8370MB, RTL8310SR
> > +       in the future.
>
> Not sure how useful this marketing is when I am configuring my kernel.
>

I'll clean it, mentioning only the supported drivers.

> >   /* Chip-specific data and limits */
> >   #define RTL8365MB_CHIP_ID_8365MB_VC         0x6367
> > -#define RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC  2112
>
> The learn limit actually seems to be chip-specific and not family
> specific, that's why I placed it here to begin with. For example,
> something called RTL8370B has a limit of 4160...
>

>From Realtek rtl8367c driver:

typedef enum switch_chip_e
{
    CHIP_RTL8367C = 0,
    CHIP_RTL8370B,
    CHIP_RTL8364B,
    CHIP_RTL8363SC_VB,
    CHIP_END
}switch_chip_t;

and

    switch (data)
    {
        case 0x0276:
        case 0x0597:
        case 0x6367:
            *pSwitchChip = CHIP_RTL8367C;
            halCtrl = &rtl8367c_hal_Ctrl;
            break;
        case 0x0652:
        case 0x6368:
            *pSwitchChip = CHIP_RTL8370B;
            halCtrl = &rtl8370b_hal_Ctrl;
            break;
        case 0x0801:
        case 0x6511:
            if( (regValue & 0x00F0) == 0x0080)
            {
                *pSwitchChip = CHIP_RTL8363SC_VB;
                halCtrl = &rtl8363sc_vb_hal_Ctrl;
            }
            else
            {
                *pSwitchChip = CHIP_RTL8364B;
                halCtrl = &rtl8364b_hal_Ctrl;
            }
            break;
        default:
            return RT_ERR_FAILED;
    }

RTL8370B does not seem to be a real device, but another "(sub)family",
like RTL8367C. I can leave it as chip_version specific but the
RTL8365MB is, for now, about RTL8367C chips. I think it is better to
leave it as a family limit.

It would make sense to have it specific for each chip family if all
Realtek DSA drivers get merged into a single one.

> > +#define RTL8365MB_CHIP_VER_8365MB_VC         0x0040
> > +
> > +#define RTL8365MB_CHIP_ID_8367S                      0x6367
> > +#define RTL8365MB_CHIP_VER_8367S             0x00A0
> > +
> > +#define RTL8365MB_LEARN_LIMIT_MAX            2112
>
> But anyways, if you are going to make it family-specific rather than
> chip-specific, place it ...

OK

>
> >
> >   /* Family-specific data and limits */
>
> ... somewhere under here.
>
> >   #define RTL8365MB_PHYADDRMAX        7
> >   #define RTL8365MB_NUM_PHYREGS       32
> >   #define RTL8365MB_PHYREGMAX (RTL8365MB_NUM_PHYREGS - 1)
> > -#define RTL8365MB_MAX_NUM_PORTS  7
> > +// RTL8370MB and RTL8310SR, possibly suportable by this driver, have 10 ports
>
> C style comments :-)
>
> > +#define RTL8365MB_MAX_NUM_PORTS              10
>
> Did you mess up the indentation here? Also seems unrelated to RTL8367S
> support...

It looks like...

>
> >
> >   /* Chip identification registers */
> >   #define RTL8365MB_CHIP_ID_REG               0x1300
> > @@ -1964,9 +1970,22 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >
> >       switch (chip_id) {
> >       case RTL8365MB_CHIP_ID_8365MB_VC:
> > -             dev_info(priv->dev,
> > -                      "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> > -                      chip_ver);
> > +             switch (chip_ver) {
> > +             case RTL8365MB_CHIP_VER_8365MB_VC:
> > +                     dev_info(priv->dev,
> > +                              "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> > +                              chip_ver);
> > +                     break;
> > +             case RTL8365MB_CHIP_VER_8367S:
> > +                     dev_info(priv->dev,
> > +                              "found an RTL8367S switch (ver=0x%04x)\n",
> > +                              chip_ver);
> > +                     break;
> > +             default:
> > +                     dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
> > +                             chip_ver);
> > +                     return -ENODEV;
> > +             }
> >
> >               priv->num_ports = RTL8365MB_MAX_NUM_PORTS;
> >
> > @@ -1974,7 +1993,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >               mb->chip_id = chip_id;
> >               mb->chip_ver = chip_ver;
> >               mb->port_mask = GENMASK(priv->num_ports - 1, 0);
> > -             mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC;
> > +             mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
> >               mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> >               mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
> >
>

Powered by blists - more mailing lists