Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

From: Linus Walleij <linus.walleij@linaro.org>
To: "Alvin Šipraga" <ALSI@bang-olufsen.dk>
Cc: "Vladimir Oltean" <olteanv@gmail.com>,
	"Alvin Šipraga" <alvin@pqrs.dk>, "Andrew Lunn" <andrew@lunn.ch>,
	"Vivien Didelot" <vivien.didelot@gmail.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Russell King" <linux@armlinux.org.uk>,
	"Michael Rasmussen" <MIR@bang-olufsen.dk>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC
Date: Sat, 28 Aug 2021 00:24:42 +0200	[thread overview]
Message-ID: <CACRpkdakr9pY0MmM7ZmFnqgHMr5o13kXGVtiBqCLB0aV+6Z=UA@mail.gmail.com> (raw)
In-Reply-To: <dd2947d5-977d-b150-848e-fb9a20c16668@bang-olufsen.dk>

On Mon, Aug 23, 2021 at 1:56 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote:
> On 8/23/21 12:48 AM, Vladimir Oltean wrote:
> > On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:

> >> +static int rtl8365mb_enable_vlan(struct realtek_smi *smi, bool enable)
> >> +{
> >> +    dev_dbg(smi->dev, "%s VLAN\n", enable ? "enable" : "disable");
> >> +    return regmap_update_bits(
> >> +            smi->map, RTL8365MB_VLAN_CTRL_REG, RTL8365MB_VLAN_CTRL_EN_MASK,
> >> +            FIELD_PREP(RTL8365MB_VLAN_CTRL_EN_MASK, enable ? 1 : 0));
> >> +}
> >> +
> >> +static int rtl8365mb_enable_vlan4k(struct realtek_smi *smi, bool enable)
> >> +{
> >> +    return rtl8365mb_enable_vlan(smi, enable);
> >> +}
> >
> > I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
> > cryptic to me. Why do you do the same thing from .enable_vlan4k as from
> > .enable_vlan? What are these supposed to do in the first place?
> > Or to quote from rtl8366_vlan_add: "what's with this 4k business?"
>
> I think realtek-smi was written with rtl8366rb.c in mind, which appears
> to have different control registers for VLAN and VLAN4k modes, whatever
> that's supposed to mean. Since the RTL8365MB doesn't distinguish between
> the two, I just route one to the other. The approach is one of caution,
> since I don't want to break the other driver (I don't have hardware to
> test for regressions). Maybe Linus can chime in?

Sigh, I have zero documentation, I just mimic what the code dump from
Realtek does.

But my interpretation is that the RTL8366RB can operate with either
16 or 4096 VLAN (VID) member entries. (Called "mc", member configs)
The support for 4096 "4k" entries need to be enabled explicitly,
in succession after enabling the 16 entries, and this is what the
code in rtl8366.c does, and we always enable all 4096 "mc:s"
of course.

I guess some older switch only supported 16 members and this
is a hardware compatibility mode.

Yours,
Linus Walleij