Message-ID: <CAJq09z5+QdQVgMX1+icdBKW4xGY0fr99C41R-VBRuNc2uSco7w@mail.gmail.com>
Date: Mon, 5 Jun 2023 01:30:58 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc: Christian Lamparter <chunkeey@...il.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "andrew@...n.ch" <andrew@...n.ch>, 
	"olteanv@...il.com" <olteanv@...il.com>, "f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: Re: [PATCH v1] net: dsa: realtek: rtl8365mb: add missing case for
 digital interface 0

> > The patch "net: dsa: realtek: rtl8365mb: rename extport to extint" mentioned
> > removed:
> >
> > -#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(_extport)   \
> > -               (RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 + \
> > -                ((_extport) >> 1) * (0x13C3 - 0x1305))
> >
> > and replaced it with:
> >
> > +#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(_extint) \
> > +               ((_extint) == 1 ? RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 : \
> > +                (_extint) == 2 ? RTL8365MB_DIGITAL_INTERFACE_SELECT_REG1 : \
> > +                0x0)
> >
> > so with the old RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(0) evaluated to
> > (RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0+(0) (which is 0x1305) and
> > since the patch RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(0) evaluates to
> > 0. So the driver writes to somewhere it shouldn't (in my RTL8363SB)
> > case.
>
> Ah yes, I see what you mean now. OK, so 0 is indeed valid. Please include this
> extra info in your v2 message :)

Yes, the vendor rtl8367c driver does use it for both ext0 and ext1.

if(0 == id || 1 == id)
        {
                ...RTL8367C_REG_DIGITAL_INTERFACE_SELECT...
        }
        else
        {
                ...RTL8367C_REG_DIGITAL_INTERFACE_SELECT_1
        }

But being in the vendor driver does not automatically mean there is a
model that supports that interface, as drivers usually evolve from
previous generation drivers, sometimes leaving unused references
behind. As we didn't have a device supported that used ext0, I
deliberately left it out. There are other features that are not
included in the driver until a device requires them.

But being in the vendor driver does not automatically mean there is a
model that supports that interface as drivers normally grow from
previous generation drivers, sometimes leaving references not used
anymore. As we didn't have a device supported that used ext0, I
deliberately left it out. There are other features not included in the
driver until a device does require it.

> Also I suggest marking REG0 as EXT0 and EXT1, something like this:
>
> #define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0         0x1305 /* EXT0/EXT1 */
>
> >
> > so that's why I said it was "by accident" in the commit message.
> > Since the other macros stayed intact.
>
> Yes, agree. Do you agree with me though that this doesn't warrant backporting to
> stable as there is no functional change with just the patch on its own?
> i.e. this should be part of a broader series adding RTL8363SB-CG support
> targetting net-next. (The Fixes: tag is absolutely fine ofc - stable maintainers
> will tell you that this does not necessarily mean it should go in stable, that's
> what cc: stable@...r is for). If so please add [PATCH v2 net-next] so it goes in
> the right place.
>
> >
> > From what I can gleam, Luiz patch mentions at the end:
> > "[...] and ext_id 0 does not seem to be used as well for this family."
>

When I say "family", I mean:

RTL8363NB
RTL8364NB
RTL8365MB-VC
RTL8367RB-VB
RTL8367SB
RTL8367S
RTL8366SC
RTL8363SC
RTL8370MB
RTL8310SR
RTL8363NB-VB
RTL8364NB-VB
RTL8363SC-VB

And your device is:

RTL8363SB

Realtek product names do not help too much to limit which device is
closer to which other. I'm not saying for sure that none of those I
listed does not use ext0 (I was at the time, but not now without
reviewing lots of docs/code), but it looks like your device is from a
previous generation. And if rtl8365mb.c does work, that's great. We
are expanding the family.

> > Looking around in todays OpenWrt's various DTS. There are these devices:
> >
> > extif0:
> > TP-Link WR2543-v1
> > SFR Neufbox 6 (Sercomm)
> > Edimax BR-6475nD
> > Samsung CY-SWR1100
> > (Netgear WNDAP660 + WNDAP620)

I didn't check all devices but I own a TP-Link WR2543-v1. I'm not sure
if RTL8367R in TP-Link WR2543-v1 is compatible with rtl8365mb.c
driver. In OpenWrt, we have all these realtek swconfig drivers:
- rtl8366
- rtl8366s
- rtl8366rb (should support the same device as DSA rtl8366rb.c driver
but not used there yet)
- rtl8367 (compatible with RTL8367R and RTL8367M)
- rtl8367b
- rtk-gsw / rtl8367c / rtl8367s_gsw (this one is actually the vendor
driver for the same rtl8365mb.c family but statically configured for
rtl8367s)

You can compare rtl8367.c with rtl8367b.c to get an idea of what would
be needed to change in the rtl8365mb.c to include support for those
devices supported by rtl8367 (if the CPU tag is compatible). The only
models I'm sure rtl8365mb.c will work out-of-box are those using the
rtk-gsw (compatible string) because they are only using the RTL8367S
chip. You can see that driver in the mediatek target at
target/linux/mediatek/files/drivers/net/phy/rtk/. The ones using
swconfig rtl8367b might work with rtl8365mb.c as your device was
compatible with that driver and it seems to be working with
rtl8365mb.c after some adjustments. BTW, even RTL8367S was working
with swconfig rtl8367b with some minor touches (see
https://github.com/openwrt/openwrt/pull/2174/files).

Regards,

Luiz

Powered by blists - more mailing lists