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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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.

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.

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.

I see Colin uses some regmap accesses in order to set up the SPI bus
interface:

felix_spi_probe
-> felix_spi_init_bus

before the ocelot hardware library is up and running, which is at this
point:

felix_spi_probe
-> dsa_register_switch
   -> felix_setup
      -> ocelot_init

I suspect that if Colin could defer his felix_spi_init_bus work until
some later point, such as until vsc7512_reset, then he could preserve
the existing code structure, with one regmap per register region as
opposed to a single regmap.

By the way, I am not opposed to more refactoring being done to the felix
driver itself, for example if ocelot_init needs to be done before
dsa_register_switch, I'm in favor of that if it's helpful. There is a
comment on top of felix_setup() which is no longer true, it reads:

/* Hardware initialization done here so that we can allocate structures with
 * devm without fear of dsa_register_switch returning -EPROBE_DEFER and causing
 * us to allocate structures twice (leak memory) and map PCI memory twice
 * (which will not work).
 */

What was actually wrong there has been solved in the meantime by commit
b4024c9e5c57 ("felix: Fix initialization of ioremap resources"). So now
there isn't any problem with EPROBE_DEFER, we can initialize the regmap
anywhere.