Message-ID: <CA+h21hrh4oYs3j3cOz4Afe2GSbU9ME+nzoRaZ4D22mu9_jkO=g@mail.gmail.com>
Date:   Tue, 12 Nov 2019 22:01:25 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Allan W. Nielsen" <allan.nielsen@...rochip.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        netdev <netdev@...r.kernel.org>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next 10/12] net: dsa: vitesse: move vsc73xx driver to
 a separate folder

On Tue, 12 Nov 2019 at 21:48, Allan W. Nielsen
<allan.nielsen@...rochip.com> wrote:
>
> The 11/12/2019 21:26, Vladimir Oltean wrote:
> > External E-Mail
> >
> >
> > On Tue, 12 Nov 2019 at 21:10, Allan W. Nielsen
> > <allan.nielsen@...rochip.com> wrote:
> > >
> > > The 11/12/2019 17:26, Vladimir Oltean wrote:
> > > > External E-Mail
> > > >
> > > >
> > > > On Tue, 12 Nov 2019 at 16:57, Allan W. Nielsen
> > > > <allan.nielsen@...rochip.com> wrote:
> > > > >
> > > > > The 11/12/2019 15:50, Andrew Lunn wrote:
> > > > > > External E-Mail
> > > > > >
> > > > > >
> > > > > > > > > As there are no commonalities between the vsc73xx and felix drivers,
> > > > > > > > > shouldn't you simply leave that one out and have felix in the existing
> > > > > > > > > microchip folder?
> > > > > > > > >
> > > > > > > >
> > > > > > > > I don't have a strong preference, although where I come from, all new
> > > > > > > > NXP networking drivers are still labeled as "freescale" even though
> > > > > > > > there is no code reuse. There are even less commonalities with
> > > > > > > > Microchip (ex-Micrel, if I am not mistaken) KSZ switches than with the
> > > > > > > > old vsc73xx. I'll let the ex-Vitesse people decide.
> > > > > > > I'm on the same page as Alexandre here.
> > > > > >
> > > > > > Leaving them where they are makes maintenance easier. Fixes are easier
> > > > > > to backport if things don't move around.
> > > > > >
> > > > > > > I think we should leave vsc73xx where it is already, and put the felix driver in
> > > > > > > the drivers/net/ethernet/mscc/ folder where ocelot is already.
> > > > > >
> > > > > > Currently, all DSA drivers are in drivers/net/dsa. We do occasionally
> > > > > > make changes over all DSA drivers at once, so it is nice they are all
> > > > > > together. So i would prefer the DSA part of Felix is also there. But
> > > > > > the core can be in drivers/net/ethernet/mscc/.
> > > > > Ahh, my bad.
> > > > >
> > > > > In that case I do not have any strong feelings on this either.
> > > > >
> > > > > I should say that we are discussing to add support for a Ocelot VSC7511 as a DSA
> > > > > driver. This one does not have an internal MIPS CPU.
> > > > >
> > > > > The vsc73xx, felix and the drivers in dsa/microchip does not share any
> > > > > functionallity. Not in SW and not in HW.
> > > > >
> > > > > Maybe felix should just go directly into drivers/net/dsa/, and then if we add
> > > > > support for VSC7511 then they can both live in drivers/net/dsa/ocelot/
> > >
> > >
> > > A bit of background such that people outside NXP/MCHP has a better change to
> > > follow and add to the discussion.
> > >
> > > Ocelot is a family name covering 4 VSC parts (VSC7511-14), and a IP used by NXP
> > > (VSC9959).
> > >
> > > VSC7511-14 are all register compatible, it uses the same serdes etc.
> > >
> > > VSC7511/12 are "unmanaged" meaning that they do not have an embedded CPU.
> > >
> > > VSC7513/14 has an embedded MIPS CPU.
> > >
> > > VSC9959 not the same core as VSC7511-14, it is a newer generation with more
> > > features, it is not register compatible, but all the basic functionallity is
> > > very similar VSC7511-14 which is why it can call into the
> > > drivers/net/ethernet/mscc/ocelot.c file.
> > >
> > > It is likely that NXP want to add more features in felix/VSC9959 which does not
> > > exists in VSC7511-14.
> > >
> > > > When the felix driver is going to support the vsc7511 ocelot switch
> > > > through the ocelot core, it will be naming chaos.
> > > I do not think a VSC7511 will be based on Felix, but it will relay on the
> > > refacturing/restructuring you have done in Ocelot.
> > >
> > > VSC7511 will use the same PCS and serdes settings as Ocelot (VSC7513/VSC7514)
> > >
> > > > Maybe we need to clarify what "felix" means (at the moment it means VSC9959).
> > > Yes.
> > >
> > > > What if we just make it mean "DSA driver for Ocelot", and it supports both the
> > > > VSC751x (Ocelot) and the VSC9959 (Felix) families?
> > > I'm not too keen on using the felix name for that.
> > >
> > > Here is my suggestion:
> > >
> > > Drop the felix name and put it in drivers/net/dsa/ocelot_vsc9959* (this would be
> > > my preference)
> > >
> >
> > This has one big issue: the name is very long! I can't see myself
> > prefixing all function and structure names with ocelot_vsc9959_*.
> > Felix is just 5 letters. And I can't use "ocelot" either, since that
> > is taken :)
> > So the DSA driver needs its own (short) name.
> I certainly agree that ocelot_vsc9959_* is too long a prefix.
>
> If you put it in drivers/net/dsa/ocelot_felix* or drivers/net/dsa/ocelot/felix*
> then you can prefix with 'felix_'.
>
> If you put it in drivers/net/dsa/ocelot_vsc9959* or drivers/net/dsa/ocelot/vsc9959*
> then you can prefix with 'vsc9959_'.
>
> The one thing all of this parts has in common is that they are all based on the
> Ocelot family, which is why I suggest to include this into the path. It will
> provide more information than putting it in the vitesse/microchip folders.
>
> > > Or if you want the felix name put it in drivers/net/dsa/ocelot_felix*
> > >
> > > Or if we want folders put it in drivers/net/dsa/ocelot/vsc9959*
> > >
> >
> > The way I see an Ocelot DSA driver, it would be done a la mv88e6xxx,
> > aka a single struct dsa_switch_ops registered for the entire family,
> > and function pointers where the implementation differs. You're not
> > proposing that here, but rather that each switch driver works in
> > parallel with each other, and they all call into the Ocelot core. That
> > would produce a lot more boilerplate, I think.
> > And if the DSA driver for Ocelot ends up supporting more than 1
> > device, its name should better not contain "vsc9959" since that's
> > rather specific.
> A vsc7511/12 will not share code with felix/vsc9959. I do not expect any other
> IP/chip will be register compatible with vsc9959.
>

I don't exactly understand this comment. Register-incompatible in a
logical sense, or in a layout sense? Judging from the attachment in
chapter 6 of the VSC7511 datasheet [1], at least the basic
functionality appears to be almost the same. And for the rest, there's
regmap magic.

> A vsc7511/12 will use the ocelot DSA tagger, but other from that it will call into the
> ocelot driver (I think).
>
> But to be honest, I do not think we should spend too much energy on vsc7511/12
> now. When/if it comes, we will see how it fit best.
>
> /Allan

Ok. So the driver will still be called "felix", it will instantiate a
struct felix_info_vsc9959 instead of the current felix_info_ls1028a,
but will live in the root drivers/net/dsa folder. Then, when/if you
add support for vsc7511, you'll move both into an "ocelot" folder and
figure out how much of the driver minus the tagger is worth reusing
(aka instantiate a struct felix_info_vsc7511). Agree?

[1]: http://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10488.pdf

-Vladimir

Powered by blists - more mailing lists