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

On Tue, May 04, 2021 at 05:08:52PM +0200, Alexandre Belloni wrote:

On 04/05/2021 14:36:34+0000, Vladimir Oltean wrote:

quoted

On Tue, May 04, 2021 at 03:55:27PM +0200, Alexandre Belloni wrote:

quoted

On 04/05/2021 12:59:43+0000, Vladimir Oltean wrote:

quoted

quoted

quoted

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

quoted

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.

quoted

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