Message-ID: <CACRpkdYoqeThCHxmApmr2Dh6jcfwf2uEJgSeyPHGymcZ4DWo1Q@mail.gmail.com>
Date:   Sat, 30 Jun 2018 13:05:47 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        netdev <netdev@...r.kernel.org>,
        OpenWrt Development List <openwrt-devel@...ts.openwrt.org>,
        LEDE Development List <lede-dev@...ts.infradead.org>,
        Gabor Juhos <juhosg@...nwrt.org>
Subject: Re: [PATCH 3/3] net: dsa: Add Vitesse VSC73xx DSA router driver

Hi Florian,

Thanks for your review!

will send a v2 shortly, just some minor feedbacks:

On Thu, Jun 14, 2018 at 6:51 PM Florian Fainelli <f.fainelli@...il.com> wrote:
> On 06/14/2018 05:35 AM, Linus Walleij wrote:

> > This driver (currently) only takes control of the switch chip over
> > SPI and configures it to route packages around when connected to a
> > CPU port. The chip has embedded PHYs and VLAN support so we model it
> > using DSA as a best fit so we can easily add VLAN support and maybe
> > later also exploit the internal frame header to get more direct
> > control over the switch.
>
> Yes having the internal frame header working would be really great,
> DSA_TAG_PROTO_NONE is really difficult to use without knowing all the
> DSA details which reminds that we should have the following action items:
>
> - document how DSA_TAG_PROTO_NONE behave differently with respect to
> bridges/VLAN configuration and the DSA master device
>
> - possibly introduce DSA_TAG_PROTO_8021Q which would automatically
> partition ports by allocating one VLAN ID per-port (e.g: from top to
> bottom) that would effectively offer the same features/paradigms as what
> a proper header would offer (Port separation, if nothing else) and it
> could be made seemingly automatic from within DSA

To me this makes a lot of sense. I haven't implemented VLAN yet
for this switch chip, but for the Realtek RTL8366RB that I'm also
working with, the vendor driver already does more or less exactly
this by default. It would be great to have the kernel just put this
in place so we have it under control.

The RTL8366 chips does not even seem to have an internal
frame format AFAICT, it seems they don't know what that is.
So they will likely always have to use VLAN for this.

> > The four built-in GPIO lines are exposed using a standard GPIO chip.
>
> What are those typically used for out of curiosity? Is this to connect
> to an EEPROM?

No, there are actually separate address and data lines for connecting
an EEPROM and booting the 8051 CPU from it if need be.

This is likely mostly for the stand-alone usecase. The VSC73xx
and other Vitesse chips are only used with another CPU (like
one running Linux) as an exception, IIUC. The usual usecase is
to put the Vitesse chip with an EEPROM and SW from the vendor
running on the 8051, and box it up as a rack-mounted or small
switch or router.

The vendor provides an SDK for the 8051 CPU including an
SNMP implementation so it can be remotely managed.

If you use this to build a rack-mounted or boxed product,
you likely want to add some "reset" key or a few LEDs to
indicate whatever, and then those 4 GPIO lines come in
handy.

With another OS, like Linux running, these lines can be used
for whatever you want, just an extra GPIO expander
essentially.

> > +     u16                     chipid;
> > +     bool                    is_vsc7385;
> > +     bool                    is_vsc7388;
> > +     bool                    is_vsc7395;
> > +     bool                    is_vsc7398;
>
> How about having an u16/u32 chip_id instead?

OK I already have the u16 chipid there.

So I added some macros like IS_VSC7385() instead.

> > +     /* The switch internally uses a 8 byte header with length,
> > +      * source port, tag, LPA and priority. This is supposedly
> > +      * only accessible when operating the switch using the internal
> > +      * CPU or with an external CPU mapping the device in, but not
> > +      * when operating the switch over SPI and putting frames in/out
> > +      * on port 6 (the CPU port). So far we must assume that we
> > +      * cannot access the tag. (See "Internal frame header" section
> > +      * 3.9.1 in the manual.)
>
> I would be really good if we could get this to work even in SPI with the
> CPU controlling the switch, I cannot really think of why the 8051 would
> have to be involved, because having the 8051 means either the switch is
> entirely standalone and runs off an EEPROM (which is additional cost on
> your BOM), or the host, through SPI can entirely take over.

If you think back on the original usecase in a switch or router it does
make a bit of sense. Another OS is just in the back seat, the 8051
is supposed to do the heavy lifting here.

This is different from devices without an internal CPU where
some "outside OS" (Linux) is supposed to deal with the raw
DSA-tagged frames.

OpenWRT have a bunch of firmware from vendors being uploaded
to the 8051 for different hardware:
http://mirror2.openwrt.org/sources/vsc73x5-ucode.tar.bz2

These look very small, for the internal RAM. I think those are
usually quite silly hacks and unnecessary, the only thing that really
makes sense to load over there is the full SNMP stack that one
can get from the vendor. I haven't disassembled their code though.
That's why I think these devices are better off using this driver
that just take control of the device over SPI.

But it would be neat to figure out if we can get access to the
raw frames. :/

> Is the datasheet public somehow?

Partminer accidentally managed to download and archive it at
one point, but it seems it disappeared from the link:
https://www.buyparts.io/download.php?id=6263858&t=ih

Since Microchips are much more liberal with datasheets I
expect it to become public ASAP, I just got access to all datasheets
when I asked.

> > +static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
> > +                             struct phy_device *phydev)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u32 val;
>
> No is_pseudo_fixed_link() check, you really have to do all of this for
> each front-panel port? That is really bad if that is the case... most
> switches with front-panel built-in PHYs are at the very least capable of
> re-configuring their internal MAC accordingly.

Sadly this is needed. The manual explicitly says that the MAC
has to be set up depending on speed immediately after the PHY
comes up. I guess the ASIC engineer skipped any integration
of the PHY and MAC and they are just connected, not really
talking to each other.

Luckily the .adjust_link() callback is handy for this.

> > +static const struct of_device_id vsc73xx_of_match[] = {
> > +     {
> > +             .compatible = "vitesse,vsc7385",
>
> Would not you want to pass additional data here, like the possible port
> layout/chip id to be reading?

Since the chip is self-identifying (chip ID) it is not necessary.

If I wanted to be overzealous I could check that the internal
chip ID matches the compatible string (I have seen that some
drivers do that.)

Yours,
Linus Walleij

Powered by blists - more mailing lists