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

[Colin Foster]

On Tue, May 04, 2021 at 05:08:52PM +0200, Alexandre Belloni wrote:
> 
> 
> On 04/05/2021 14:36:34+0000, Vladimir Oltean wrote:
> > On Tue, May 04, 2021 at 03:55:27PM +0200, Alexandre Belloni wrote:
> > > On 04/05/2021 12:59:43+0000, Vladimir Oltean wrote:
> > > > > > +static void vsc7512_phylink_validate(struct ocelot *ocelot, int port,
> > > > > > +				     unsigned long *supported,
> > > > > > +				     struct phylink_link_state *state)
> > > > > > +{
> > > > > > +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> > > > > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = {
> > > > > > +		0,
> > > > > > +	};
> > > > > 
> > > > > This function seems out of place. Why would SPI access change what the
> > > > > ports are capable of doing? Please split this up into more
> > > > > patches. Keep the focus of this patch as being adding SPI support.
> > > > 
> > > > What is going on is that this is just the way in which the drivers are
> > > > structured. Colin is not really "adding SPI support" to any of the
> > > > existing DSA switches that are supported (VSC9953, VSC9959) as much as
> > > > "adding support for a new switch which happens to be controlled over
> > > > SPI" (VSC7512).
> > > 
> > > Note that this should not only be about vsc7512 as the whole ocelot
> > > family (vsc7511, vsc7512, vsc7513 and vsc7514) can be connected over
> > > spi. Also, they can all be used in a DSA configuration, over PCIe, just
> > > like Felix.
> > 
> > I see. From the Linux device driver model's perspective, a SPI driver
> > for VSC7512 is still different than an MMIO driver for the same hardware
> > is, and that is working a bit against us. I don't know much about regmap
> > for SPI, specifically how are the protocol buffers constructed, and if
> > it's easy or not to have a driver-specified hook in which the memory
> > address for the SPI reads and writes is divided by 4. If I understand
> > correctly, that's about the only major difference between a VSC7512
> > driver for SPI vs MMIO, and would allow reusing the same regmaps as e.g.
> > the ones in drivers/net/ethernet/ocelot_vsc7514.c. Avoiding duplication
> > for the rest could be handled with a lot of EXPORT_SYMBOL, although
> > right now, I am not sure that is quite mandated yet. I know that the
> > hardware is capable of a lot more flexibility than what the Linux
> > drivers currently make of, but let's not think of overly complex ways of
> > managing that entire complexity space unless somebody actually needs it.
> > 
> 
> I've been thinking about defining the .reg_read and .reg_write functions
> of the regmap_config to properly abstract accesses and leave the current
> ocelot core as it is.

I considered keeping the regmap definitions from the initial ocelot 
(VSC7514) driver for this. Define a .reg_read and .reg_write to do 
address translation, byte-pad reads, etc. I believe that would require
abandoning devm_regmap_init_spi in favor of a custom implementation.
There were good things I wanted to keep from using init_spi though -
endian checking, possible async capabilities, etc.

drivers/net/dsa/qca8k.c has an example of what I'd start with as far as
defining a custom regmap. It doesn't use SPI, but has custom read /
write functions that could do whatever translation is necessary.

> 
> > As to phylink, I had some old patches converting ocelot to phylink in
> > the blind, but given the fact that I don't have any vsc7514 board and I
> > was relying on Horatiu to test them, those patches didn't land anywhere
> > and would be quite obsolete now.
> > I don't know how similar VSC7512 (Colin's chip) and VSC7514 (the chip
> > supported by the switchdev ocelot) are in terms of hardware interfaces.
> > If the answer is "not very", then this is a bit of a moot point, but if
> > they are, then ocelot might first have to be converted to phylink, and
> > then its symbols exported such that DSA can use them too.
> > 
> 
> VSC7512 and VSC7514 are exactly the same chip. VSC7514 has the MIPS
> CPU enabled.
> 
> > What Colin appears to be doing differently to all other Ocelot/Felix
> > drivers is that he has a single devm_regmap_init_spi() in felix_spi_probe.
> > Whereas everyone else uses a separate devm_regmap_init_mmio() per each
> > memory region, tucked away in ocelot_regmap_init(). I still haven't
> > completely understood why that is, but this is the reason why he needs
> > the "offset" passed to all I/O accessors: since he uses a single regmap,
> > the offset is what accesses one memory region or another in his case.
> > 
> 
> Yes, this is the main pain point. You only have one chip select so from
> the regmap point of view, there is only one region. I'm wondering
> whether we could actually register multiple regmap for a single SPI
> device (and then do the offsetting in .reg_read/.reg_write) which would
> help.

Exactly, this was the main difference. The SPI regmap has no concept of
__iomem, which was a main feature of the underlying ocelot core of
having multiple regmaps. 

So instead of having "offset" in all ocelot accesses, allocate each
regmap in felix_vsc7512_spi.c as part of a struct { u32; regmap; }
during each felix->info->init_regmap call. Use this u32 (or resource, or
whatever it may be) to do the offset in the reg_read / reg_write. That
seems like it should work. This would again require abandoning
devm_regmap_init_spi, which I'm considering more and more...

> 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com