Re: [RFC PATCH vN net-next 2/2] net: mscc: ocelot: add support for VSC75XX SPI control

[Vladimir Oltean]

On Thu, May 06, 2021 at 11:34:18AM -0700, Colin Foster wrote:
> Vladimir, Thank you very much for the feedback. I appreciate your time
> and your patience when dealing with my many blunders. This is a large
> learning experience for me.

You haven't seen mine...

> On Thu, May 06, 2021 at 01:22:04PM +0300, Vladimir Oltean wrote:
> > On Mon, May 03, 2021 at 10:11:27PM -0700, Colin Foster wrote:
> > > Add support for control for VSC75XX chips over SPI control. Starting with the
> > > VSC9959 code, this will utilize a spi bus instead of PCIe or memory-mapped IO to
> > > control the chip.
> > > 
> > > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/rpi-vsc7512-spi-overlay.dts |  124 ++
> > >  drivers/net/dsa/ocelot/Kconfig                |   11 +
> > >  drivers/net/dsa/ocelot/Makefile               |    5 +
> > >  drivers/net/dsa/ocelot/felix_vsc7512_spi.c    | 1214 +++++++++++++++++
> > >  include/soc/mscc/ocelot.h                     |   15 +
> > >  5 files changed, 1369 insertions(+)
> > >  create mode 100644 arch/arm/boot/dts/rpi-vsc7512-spi-overlay.dts
> > >  create mode 100644 drivers/net/dsa/ocelot/felix_vsc7512_spi.c
> > > 
> > > diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
> > > index 932b6b6fe817..2db147ce9fe7 100644
> > > --- a/drivers/net/dsa/ocelot/Kconfig
> > > +++ b/drivers/net/dsa/ocelot/Kconfig
> > > @@ -14,6 +14,17 @@ config NET_DSA_MSCC_FELIX
> > >  	  This driver supports the VSC9959 (Felix) switch, which is embedded as
> > >  	  a PCIe function of the NXP LS1028A ENETC RCiEP.
> > >  
> > > +config NET_DSA_MSCC_FELIX_SPI
> > > +	tristate "Ocelot / Felix Ethernet SPI switch support"
> > > +	depends on NET_DSA && SPI
> > > +	depends on NET_VENDOR_MICROSEMI
> > > +	select MSCC_OCELOT_SWITCH_LIB
> > > +	select NET_DSA_TAG_OCELOT_8021Q
> > > +	select NET_DSA_TAG_OCELOT
> > > +	select PCS_LYNX
> > 
> > You most probably don't need to select PCS_LYNX (that's an NXP thing).
> > For that reason, you might want to call your module NET_DSA_MSCC_OCELOT_SPI.
> > The "felix" name is just the name of the common DSA driver and of the
> > VSC9959 chip. VSC7512 is probably called Ocelot-1 according to Microchip
> > marketing.
> 
> Thank you. I couldn't find where "Felix" and "Seville" actually came
> from. My next RFC submission will include these changes.

felix = code name for the VSC9959 from NXP LS1028A, derivative of Ocelot-1
seville = code name for the VSC9953 from NXP T1040, derivative of Serval-1

The common DSA driver layer is called felix because felix was the first
switch it supported. But since probing is now handled individually and
the PCIe probing of felix is done in a different driver compared to the
platform device probing of seville, you can name your VSC7512 driver in
any way you feel appropriate.

> > I'm surprised that the reset procedure for VSC7512 is different than
> > what is implemented in ocelot_reset() for VSC7514.
> 
> I will look more into this and repair as needed. I'm also considering
> the ability to use a GPIO as an external reset, which might negate any
> reset procedure the MIPS would be doing.

I think some memories need to be initialized too, not sure if you can
achieve that with a reset pin only.

> > You must be working on an old kernel or something. There's also a
> > watermark decode function which you can reuse from ocelot (ocelot_wm_dec).
> 
> Yes, my proof-of-concept was from a 5.10 kernel. I brought it up to
> mainline before submitting the patch and must have missed this. Per the
> documentation, I'll base my next submission entirely on the latest 5.12
> release.

What you actually mean when you set the patch subject prefix to
"PATCH net-next" is that your patches can be applied to the current
master branch of this tree, not on any release tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/

The official kernel tag v5.12 has just been released, and kernel v5.13
entered release candidate status (starting with rc1 and ending with rc7
or rc8, depending on how many fixes it gets). The development process
happens simultaneously with the release candidates for the official
v5.13 release, but new development is done for v5.14 only (that's why it
is called "next"). When Linus decides that the v5.13 release candidates
are mature enough to print the v5.13 final release tag, development for
v5.14 stops too, the maintainers stop accepting development patches for
a little while, everybody prepares a pull request, there is a "memory
barrier" until the official v5.14 rc1 is released which contains all
pull requests with new features from the maintainers' development trees,
and then these "next" trees reopen for development that will be included
in v5.15.

This is a bit oversimplified, but hopefully it should give you an idea
why sometimes you are told to rebase on a different tree, or why you
can't send patches right now, etc.

> > > +static int felix_spi_remove(struct spi_device *pdev)
> > > +{
> > > +	struct felix *felix;
> > > +
> > > +	felix = spi_get_drvdata(pdev);
> > > +
> > > +	return 0;
> > > +}
> > 
> > You got bored of writing code here? :)
> 
> A bad habit of mine. I'm trying to test the code as I'm writing it.
> Communication doesn't work, so I can't get past probe. I'm excited for
> the day I can see this memory leak in action.
> 
> I'll repair this for RFC V2, regardless of whether I can test it.

By testing, you mean this?

echo spiN.M > /sys/bus/spi/drivers/vsc7512/unbind
echo spiN.M > /sys/bus/spi/drivers/vsc7512/bind

(where of course, N is the bus number and M is the chip select)