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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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.

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


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