Date:   Tue, 12 Nov 2019 21:26:20 +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: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.

> 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.

> When we get to it, we can add vsc7511/12 in drivers/net/dsa/ocelot_vsc7512*
>
> To be consisten and clean up (my) earlier mistake we should rename the
> drivers/net/ethernet/mscc/ocelot_board.c to
> drivers/net/ethernet/mscc/ocelot_vsc7514.c (as it is really not board stuff, but
> ocelot internal cpu stuff).
>
> Andrew also pointed out that the stuff put into this file did not seem very
> board related.
>
> > Is anybody else instantiating the VSC9959 core, or close derivatives, except
> > NXP LS1028A? If the answer is yes, are those other instantiations PCI devices,
> > or something else?
> Not what I'm aware of.
>

Ok, this is good to know.

> > I would appreciate if you could take a
> > look through the probing part of patch 11/12 (the "felix_instance_tbl"
> > part and felix-regs.c) and see if there are any naming changes I can
> > make that would make it easier for you to fit in one more device.
> Will do.
>
> > Of course, I don't expect to make radical changes, you'd still need to do some
> > refactoring if you decide to add your vsc7511, I just care that the
> > refactoring doesn't change any current semantics.
> The majority of the needed changes to add vsc7511 has been done by now ;-)
>
> /Allan
>

Powered by blists - more mailing lists