[RFC,v5,net-next,01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512

Create a single SPI MFD ocelot device that manages the SPI bus on the
external chip and can handle requests for regmaps. This should allow any
ocelot driver (pinctrl, miim, etc.) to be used externally, provided they
utilize regmaps.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/Kconfig       |  15 ++
 drivers/mfd/Makefile      |   3 +
 drivers/mfd/ocelot-core.c | 149 +++++++++++++++
 drivers/mfd/ocelot-mfd.h  |  19 ++
 drivers/mfd/ocelot-spi.c  | 374 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 560 insertions(+)
 create mode 100644 drivers/mfd/ocelot-core.c
 create mode 100644 drivers/mfd/ocelot-mfd.h
 create mode 100644 drivers/mfd/ocelot-spi.c

On Wed, Dec 29, 2021 at 03:22:24PM +0000, Lee Jones wrote:
> On Sat, 18 Dec 2021, Colin Foster wrote:
> 
> > Create a single SPI MFD ocelot device that manages the SPI bus on the
> > external chip and can handle requests for regmaps. This should allow any
> > ocelot driver (pinctrl, miim, etc.) to be used externally, provided they
> > utilize regmaps.

Hi Lee,

Thanks for your feedback.

> 
> We're going to need Mark Brown to have a look at this Regmap implementation.
> 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/mfd/Kconfig       |  15 ++
> >  drivers/mfd/Makefile      |   3 +
> >  drivers/mfd/ocelot-core.c | 149 +++++++++++++++
> >  drivers/mfd/ocelot-mfd.h  |  19 ++
> 
> Drop the '-mfd' part please.

Done. 

> 
> >  drivers/mfd/ocelot-spi.c  | 374 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 560 insertions(+)
> >  create mode 100644 drivers/mfd/ocelot-core.c
> >  create mode 100644 drivers/mfd/ocelot-mfd.h
> >  create mode 100644 drivers/mfd/ocelot-spi.c
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 3fb480818599..af76c9780a10 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -954,6 +954,21 @@ config MFD_MENF21BMC
> >  	  This driver can also be built as a module. If so the module
> >  	  will be called menf21bmc.
> >  
> > +config MFD_OCELOT_CORE
> 
> You can drop the "_CORE" part.

Done. The idea here though was taken after the madera driver, since it
seemed fairly recent and does what I expect this Ocelot driver to end up
doing - allow control over either SPI or I2C.

> 
> > +	tristate "Microsemi Ocelot External Control Support"
> > +	select MFD_CORE
> > +	help
> > +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> > +	  VSC7513, VSC7514) controlled externally.
> 
> Please describe the device in more detail here.
> 
> I'm not sure what an "External Control Support" is.

A second paragraph "All four of these chips can be controlled internally
(MMIO) or externally via SPI, I2C, PCIe. This enables control of these
chips over one or more of these buses"

> 
> > +config MFD_OCELOT_SPI
> > +	tristate "Microsemi Ocelot SPI interface"
> > +	depends on MFD_OCELOT_CORE
> > +	depends on SPI_MASTER
> > +	select REGMAP_SPI
> > +	help
> > +	  Say yes here to add control to the MFD_OCELOT chips via SPI.
> > +
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 0b1b629aef3e..dff83f474fb5 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -120,6 +120,9 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
> >  
> >  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
> >  
> > +obj-$(CONFIG_MFD_OCELOT_CORE)	+= ocelot-core.o
> > +obj-$(CONFIG_MFD_OCELOT_SPI)	+= ocelot-spi.o
> > +
> >  obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
> >  obj-$(CONFIG_MFD_CPCAP)		+= motorola-cpcap.o
> >  
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > new file mode 100644
> > index 000000000000..a65619a8190b
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> 
> Author?
> 
> Short device description?

Added. 

> 
> > + */
> > +
> > +#include <asm/byteorder.h>
> 
> These, if required, usually go at the bottom.

Moved between regmap and ocelot.h

> 
> > +#include <linux/spi/spi.h>
> > +#include <linux/kconfig.h>
> 
> What's this for?

Both were left over from my initial "DSA" implementation that had all
this lumped together. They only belong in ocelot-spi.c, and I didn't
remove them here. Thanks for catching these!

> 
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> 
> These should be alphabetical.
> 
> > +#include "ocelot-mfd.h"
> > +
> > +#define REG(reg, offset)	[reg] = offset
> 
> What does this save, really?

Fair point. This was done for consistency with other ocelot
drivers that use the upper bits in the enumeration as a separate index.
Specifically the enumeration in include/soc/mscc/ocelot.h and
drivers/net/ethernet/mscc/vsc7514_regs.c. It has no other benefit than
just that, and I have no problem removing it if that's desired.

> 
> > +enum ocelot_mfd_gcb_regs {
> 
> Please remove the term 'mfd\|MFD' from everywhere.

"ocelot_init" conflicts with a symbol in
drivers/net/ethernet/mscc/ocelot.o, otherwise I belive I got them all
now.

> 
> > +	GCB_SOFT_RST,
> > +	GCB_REG_MAX,
> > +};
> > +
> > +enum ocelot_mfd_gcb_regfields {
> > +	GCB_SOFT_RST_CHIP_RST,
> > +	GCB_REGFIELD_MAX,
> > +};
> > +
> > +static const u32 vsc7512_gcb_regmap[] = {
> > +	REG(GCB_SOFT_RST,	0x0008),
> > +};
> > +
> > +static const struct reg_field vsc7512_mfd_gcb_regfields[GCB_REGFIELD_MAX] = {
> > +	[GCB_SOFT_RST_CHIP_RST] = REG_FIELD(vsc7512_gcb_regmap[GCB_SOFT_RST], 0, 0),
> > +};
> > +
> > +struct ocelot_mfd_core {
> > +	struct ocelot_mfd_config *config;
> > +	struct regmap *gcb_regmap;
> > +	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
> > +};
> 
> Not sure about this at all.
> 
> Which driver did you take your inspiration from?

Mainly drivers/net/dsa/ocelot/* and drivers/net/ethernet/mscc/*.

> 
> > +static const struct resource vsc7512_gcb_resource = {
> > +	.start	= 0x71070000,
> > +	.end	= 0x7107022b,
> 
> No magic numbers please.

I've gotten conflicting feedback on this. Several of the ocelot drivers
(drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
all passed in through the device tree. 

https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/

> 
> > +	.name	= "devcpu_gcb",
> 
> What is a 'devcpu_gcb'?

It matches the datasheet of the CPU's general configuation block.

> 
> > +};
> > +
> > +static int ocelot_mfd_reset(struct ocelot_mfd_core *core)
> > +{
> > +	int ret;
> > +
> > +	dev_info(core->config->dev, "resetting ocelot chip\n");
> 
> These types of calls are not useful in production code.

Removed.

> 
> > +	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);
> 
> No magic numbers please.  I have no idea what this is doing.

I'm not sure how much more verbose it can be... I suppose a define for
"RESET" and "CLEAR" instead of "1" and "0" on that bit. Maybe I'm just
blinded by having stared at this code for the last several months.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
> > +	 * suggest how to do a reset over SPI, and the register strategy isn't
> > +	 * possible.
> > +	 */
> > +	msleep(100);
> > +
> > +	ret = core->config->init_bus(core->config);
> 
> You're not writing a bus.  I doubt you need ops call-backs.

In the case of SPI, the chip needs to be configured both before and
after reset. It sets up the bus for endianness, padding bytes, etc. This
is currently achieved by running "ocelot_spi_init_bus" once during SPI
probe, and once immediately after chip reset.

For other control mediums I doubt this is necessary. Perhaps "init_bus"
is a misnomer in this scenario...

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > +				  int size)
> > +{
> > +	if (res->name)
> > +		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
> > +	else
> > +		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_get_resource_name);
> 
> What is this used for?
> 
> You should not be hand rolling device resource names like this.
> 
> This sort of code belongs in the bus/class API.
> 
> Please use those instead.

The idea here was to allow shared regmaps across different devices. The
"devcpu_gcb" might be used in two ways - either everyone shares the same
regmap across the "GCB" range, or everyone creates their own. 

This was more useful when the ocelot-core.c had a copy of the 
"devcpu_org" regmap that was shared with ocelot-spi.c. I was able to
remove that, but also feel like the full switch driver (patch 6 of this
set) ocelot-ext should use the same "devcpu_gcb" regmap instance as
ocelot-core does.

Admittedly, there are complications. There should probably be more
checks added to "ocelot_regmap_init" / "get_regmap" to ensure that the
regmap for ocelot_ext exactly matches the existing regmap for
ocelot_core.

There's yet another complexity with these, and I'm not sure what the
answer is. Currently all regmaps are tied to the ocelot_spi device...
ocelot_spi calls devm_regmap_init. So those regmaps hang around if
they're created by a module that has been removed. At least until the
entire MFD module is removed. Maybe there's something I haven't seen yet
where the devres or similar has a reference count. I don't know the best
path forward on this one.

> 
> > +static struct regmap *ocelot_mfd_regmap_init(struct ocelot_mfd_core *core,
> > +					     const struct resource *res)
> > +{
> > +	struct device *dev = core->config->dev;
> > +	struct regmap *regmap;
> > +	char name[32];
> > +
> > +	ocelot_mfd_get_resource_name(name, res, sizeof(name) - 1);
> > +
> > +	regmap = dev_get_regmap(dev, name);
> > +
> > +	if (!regmap)
> > +		regmap = core->config->get_regmap(core->config, res, name);
> > +
> > +	return regmap;
> > +}
> > +
> > +int ocelot_mfd_init(struct ocelot_mfd_config *config)
> > +{
> > +	struct device *dev = config->dev;
> > +	const struct reg_field *regfield;
> > +	struct ocelot_mfd_core *core;
> > +	int i, ret;
> > +
> > +	core = devm_kzalloc(dev, sizeof(struct ocelot_mfd_config), GFP_KERNEL);
> > +	if (!core)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, core);
> > +
> > +	core->config = config;
> > +
> > +	/* Create regmaps and regfields here */
> > +	core->gcb_regmap = ocelot_mfd_regmap_init(core, &vsc7512_gcb_resource);
> > +	if (!core->gcb_regmap)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < GCB_REGFIELD_MAX; i++) {
> > +		regfield = &vsc7512_mfd_gcb_regfields[i];
> > +		core->gcb_regfields[i] =
> > +			devm_regmap_field_alloc(dev, core->gcb_regmap,
> > +						*regfield);
> > +		if (!core->gcb_regfields[i])
> > +			return -ENOMEM;
> > +	}
> > +
> > +	/* Prepare the chip */
> > +	ret = ocelot_mfd_reset(core);
> > +	if (ret) {
> > +		dev_err(dev, "ocelot mfd reset failed with code %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Create and loop over all child devices here */
> 
> These need to all go in now please.

I'll squash them, as I saw you suggested in your other responses. I
tried to keep them separate, especially since adding ocelot_ext to this
commit (which has no functionality until this one) makes it quite a
large single commit. That's why I went the path I did, which was to roll
them in one at a time.

> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_init);
> > +
> > +int ocelot_mfd_remove(struct ocelot_mfd_config *config)
> > +{
> > +	/* Loop over all children and remove them */
> 
> Use devm_* then you won't have to.

Yeah, I was more worried than I needed to be when I wrote that comment.
The only thing called to clean everything up is mfd_remove_devices();

> 
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ocelot_mfd_remove);
> > +
> > +MODULE_DESCRIPTION("Ocelot Chip MFD driver");
> > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/mfd/ocelot-mfd.h b/drivers/mfd/ocelot-mfd.h
> > new file mode 100644
> > index 000000000000..6af8b8c5a316
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-mfd.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> > + */
> > +
> > +#include <linux/regmap.h>
> > +
> > +struct ocelot_mfd_config {
> > +	struct device *dev;
> > +	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
> > +				     const struct resource *res,
> > +				     const char *name);
> > +	int (*init_bus)(struct ocelot_mfd_config *config);
> 
> Please re-work and delete this 'config' concept.
> 
> See other drivers in this sub-directory for reference.

Do you have a specific example? I had focused on madera for no specific
reason. But I really dislike the idea of throwing all of the structure
definition for the MFD inside of something like
"include/linux/mfd/ocelot/core.h", especially since all the child
drivers (madera-pinctrl, madera-gpio, etc) heavily rely on this struct. 

It seemed to me that without the concept of
"mfd_get_regmap_from_resource" this sort of back-and-forth was actually
necessary.

> 
> > +};
> > +
> > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > +				  int size);
> > +int ocelot_mfd_init(struct ocelot_mfd_config *config);
> > +int ocelot_mfd_remove(struct ocelot_mfd_config *config);
> > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > new file mode 100644
> > index 000000000000..65ceb68f27af
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-spi.c
> > @@ -0,0 +1,374 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2021 Innovative Advantage Inc.
> 
> As above.
> 
> > + */
> > +
> > +#include <asm/byteorder.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> 
> As above.

Done.

> > +static int ocelot_spi_probe(struct spi_device *spi)
> > +{
> > +	struct ocelot_spi *ocelot_spi;
> > +	struct device *dev;
> > +	char name[32];
> > +	int err;
> > +
> > +	dev = &spi->dev;
> 
> Put this on the declaration line.
> 
> > +	ocelot_spi = devm_kzalloc(dev, sizeof(struct ocelot_spi),
> 
> sizeof(*ocelot_spi)
> 
> > +				      GFP_KERNEL);
> > +
> 
> No '\n'.

Done. I must've changed a variable name and missed this one. Oops.

> 
> > +	if (!ocelot_spi)
> > +		return -ENOMEM;
> > +
> > +	if (spi->max_speed_hz <= 500000) {
> > +		ocelot_spi->spi_padding_bytes = 0;
> > +	} else {
> > +		/*
> > +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
> > +		 * on the side of more padding bytes, as having too few can be
> > +		 * difficult to detect at runtime.
> > +		 */
> > +		ocelot_spi->spi_padding_bytes = 1 +
> > +			(spi->max_speed_hz / 1000000 + 2) / 8;
> 
> Please explain what this means or define the values (or both).

I can certainly elaborate the comment. Searching the manual for the term
"if_cfgstat" will take you right to the equation, and a description of
what padding bytes are, etc. 

> 
> > +	}
> > +
> > +	ocelot_spi->spi = spi;
> 
> Why are you saving this?

This file keeps the regmap_{read,write} implementations, so is needed
for spi_sync() for any regmap. There might be a better way to infer
this... but it seemed pretty nice to have each regmap only carry along
an instance of "ocelot_spi_regmap_context."

> 
> > +	ocelot_spi->map = vsc7512_dev_cpuorg_regmap;
> 
> Why not just set up the regmap here?

I think this was fairly unclear, and that is my fault. I have updated 
the header of this file to better explain its purpose.

Basically ocelot-spi is not much more than an interface by which others
can get regmaps. It handles the low-level protocols (endianness, padding
bytes, address translation...) and leaves everything else to the
children.

So when a VSC7511/7513 comes on line, it will probably need to dish out 5
regmaps for just net ports. If it is a 7512/7514, it would probably need
to dish out 11. If PTP is desired... another regmap. Pinctrl, SGPIO,
MDIO... All those are either separate regmaps, shared regmaps, or 
sub-regmaps. Likely occupying either overlapping or identical regions.

"ocelot-spi" is designed to not care about those features. And
"ocelot-core" really isn't either. It is mostly a conduit to say
"here's your regmap" and be on it's merry way.

> 
> > +	spi->bits_per_word = 8;
> > +
> > +	err = spi_setup(spi);
> > +	if (err < 0) {
> > +		dev_err(&spi->dev, "Error %d initializing SPI\n", err);
> 
> The error code usually comes at the end.
> 
> > +		return err;
> > +	}
> > +
> > +	dev_info(dev, "configured SPI bus for speed %d, rx padding bytes %d\n",
> > +			spi->max_speed_hz, ocelot_spi->spi_padding_bytes);
> 
> When would this be useful?
> 
> Don't we already have debug interfaces to find this out?

Padding bytes, no. If someone were to put a scope on the SPI lines this
message is likely helpful to see what might be going on. The expectation
would be (from memory) 3 address bytes, followed by spi_padding_bytes of 
0x88888888, then the actual data.

That said, I won't lose any sleep if this one gets cut.

> 
> > +	/* Ensure we have devcpu_org regmap before we call ocelot_mfd_init */
> 
> because ...

That isn't clear. I'm fixing the comment.

Children of ocelot_mfd might require the "dev_cpuorg" regmap region. By
having this region named and registered to the device, child devices
don't need to allocate new regmaps, they can benefit from
dev_get_regmap()

> 
> > +	ocelot_mfd_get_resource_name(name, &vsc7512_dev_cpuorg_resource,
> > +				     sizeof(name) - 1);
> 
> This is an ugly interface.  I think it needs to go.

I agree that it is an ugly interface. It is only exposed so that
ocelot_spi can get the same regmap name as ocelot_mfd->children. It
isn't much better than hard-coding the name. But if I give up the idea
that children can share this regmap, I can take away this interface and
have it not be linked to ocelot_mfd.

> 
> > +	/*
> > +	 * Since we created dev, we know there isn't a regmap, so create one
> > +	 * here directly.
> > +	 */
> 
> Sorry, what 'dev'?  When did we create that?

Unclear comment. 

"Since this is a newly probed device, we know it doesn't have a regmap
so a call to dev_get_regmap isn't necessary - just create it"

> 
> > +	ocelot_spi->cpuorg_regmap =
> > +		ocelot_spi_get_regmap(&ocelot_spi->config,
> > +				      &vsc7512_dev_cpuorg_resource, name);
> > +	if (!ocelot_spi->cpuorg_regmap)
> > +		return -ENOMEM;
> > +
> > +	ocelot_spi->config.init_bus = ocelot_spi_init_bus_from_config;
> > +	ocelot_spi->config.get_regmap = ocelot_spi_get_regmap;
> > +	ocelot_spi->config.dev = dev;
> 
> Please remove this API.

I might need to look at this more. But get_regmap is the main purpose of
ocelot-spi, and init_bus is a necessary call for any SPI communication
after a chip reset. Should I2C, or even MMIO be added, it can be done
without modification to ocelot-core.

> 
> > +	spi_set_drvdata(spi, ocelot_spi);
> > +
> > +	/*
> > +	 * The chip must be set up for SPI before it gets initialized and reset.
> > +	 * Do this once here before calling mfd_init
> > +	 */
> > +	err = ocelot_spi_init_bus(ocelot_spi);
> > +	if (err) {
> > +		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
> 
> Doesn't this already print out an error message?

In some cases ocelot_spi_init_bus does. I don't know about whether a
err return from spi_driver->probe does or not.

> 
> > +		return err;
> > +	}
> > +
> > +	err = ocelot_mfd_init(&ocelot_spi->config);
> > +	if (err < 0) {
> > +		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
> > +		return err;
> > +	}
> > +
> > +	dev_info(&spi->dev, "ocelot spi mfd probed\n");
> 
> Please, remove all of these.

Done

> 
> > +	return 0;
> > +}
> > +
> > +static int ocelot_spi_remove(struct spi_device *spi)
> > +{
> > +	struct ocelot_spi *ocelot_spi;
> > +
> > +	ocelot_spi = spi_get_drvdata(spi);
> > +	devm_kfree(&spi->dev, ocelot_spi);
> 
> Why use devm_* if you're going to free anyway?

Good point. It looks like none of this remove is necessary.

> 
> > +	return 0;
> > +}
> > +
> > +const struct of_device_id ocelot_mfd_of_match[] = {
> > +	{ .compatible = "mscc,vsc7514_mfd_spi" },
> > +	{ .compatible = "mscc,vsc7513_mfd_spi" },
> > +	{ .compatible = "mscc,vsc7512_mfd_spi" },
> > +	{ .compatible = "mscc,vsc7511_mfd_spi" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ocelot_mfd_of_match);
> > +
> > +static struct spi_driver ocelot_mfd_spi_driver = {
> > +	.driver = {
> > +		.name = "ocelot_mfd_spi",
> > +		.of_match_table = of_match_ptr(ocelot_mfd_of_match),
> > +	},
> > +	.probe = ocelot_spi_probe,
> > +	.remove = ocelot_spi_remove,
> > +};
> > +module_spi_driver(ocelot_mfd_spi_driver);
> > +
> > +MODULE_DESCRIPTION("Ocelot Chip MFD SPI driver");
> > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > +MODULE_LICENSE("Dual MIT/GPL");
> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Hi Lee,

Thank you for all your feedback. I expect to create another RFC in the
next week or two with all the changes you suggest.

On Mon, Jan 10, 2022 at 12:16:59PM +0000, Lee Jones wrote:
> On Wed, 29 Dec 2021, Colin Foster wrote:
> > On Wed, Dec 29, 2021 at 03:22:24PM +0000, Lee Jones wrote:
> 
> [...]
> 
> > > > +	tristate "Microsemi Ocelot External Control Support"
> > > > +	select MFD_CORE
> > > > +	help
> > > > +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> > > > +	  VSC7513, VSC7514) controlled externally.
> > > 
> > > Please describe the device in more detail here.
> > > 
> > > I'm not sure what an "External Control Support" is.
> > 
> > A second paragraph "All four of these chips can be controlled internally
> > (MMIO) or externally via SPI, I2C, PCIe. This enables control of these
> > chips over one or more of these buses"
> 
> Where?  Or do you mean that you'll add one?

I meant I added one. Sorry that wasn't clear.

> 
> > > Please remove the term 'mfd\|MFD' from everywhere.
> > 
> > "ocelot_init" conflicts with a symbol in
> > drivers/net/ethernet/mscc/ocelot.o, otherwise I belive I got them all
> > now.
> 
> Then rename the other one.  Or call this one 'core', or something.

I'll add "core" or something to this one.

> 
> > > > +struct ocelot_mfd_core {
> > > > +	struct ocelot_mfd_config *config;
> > > > +	struct regmap *gcb_regmap;
> > > > +	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
> > > > +};
> > > 
> > > Not sure about this at all.
> > > 
> > > Which driver did you take your inspiration from?
> > 
> > Mainly drivers/net/dsa/ocelot/* and drivers/net/ethernet/mscc/*.
> 
> I doubt you need it.  Please try to remove it.

Fair point. I'll remove it here.

> 
> > > > +static const struct resource vsc7512_gcb_resource = {
> > > > +	.start	= 0x71070000,
> > > > +	.end	= 0x7107022b,
> > > 
> > > No magic numbers please.
> > 
> > I've gotten conflicting feedback on this. Several of the ocelot drivers
> > (drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
> > Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
> > all passed in through the device tree. 
> > 
> > https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/
> 
> Ref or quote?
> 
> I'm not brain grepping it searching for what you might be referring to.
> 
> I'm not sure what you're trying to say here.  I'm asking you to define
> this numbers please.

I'll define the numbers as you suggest.


The quote I was referring to is this:

> The last option I haven't put much consideration toward would be to
> move some of the decision making to the device tree. The main ocelot
> driver appears to leave a lot of these addresses out. For instance
> Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt.
> That added DT complexity could remove needs for lines like this:
> > > +              ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
> But that would probably impose DT changes on Seville and Felix, which
> is the last thing I want to do.

The thing with putting the targets in the device tree is that you're
inflicting yourself unnecessary pain. Take a look at
Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
they mark the "ptp" target as optional because it wasn't needed when
they first published the device tree, and now they need to maintain
compatibility with those old blobs.

> 
> > > > +	.name	= "devcpu_gcb",
> > > 
> > > What is a 'devcpu_gcb'?
> > 
> > It matches the datasheet of the CPU's general configuation block.
> 
> Please could you quote that part for me?

Hmm... I'm not sure exactly what you mean by this.

https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf

There are 64 instances of "DEVCPU_GCB" in the main datasheet. 
Chapter 6 of this PDF has an attached PDF around the phrase "Information
about the registers for this product is available in the attached file"

Section 2.3 of that attached PDF is dedicated entirely to DEVCPU_GCB 
registers. Table 1 defines that register block starting at 0x71070000.
The entry from that table is
| DEVCPU_GCB | 0x71070000 | General configuration block. | Page 63 |
This document has 175 references to the phrase "DEVCPU_GCB".

> 
> > > > +	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);
> > > 
> > > No magic numbers please.  I have no idea what this is doing.
> > 
> > I'm not sure how much more verbose it can be... I suppose a define for
> > "RESET" and "CLEAR" instead of "1" and "0" on that bit. Maybe I'm just
> > blinded by having stared at this code for the last several months.
> 
> Yes please.  '1' could mean anything.
> 
> 'CLEAR' is just as opaque.
> 
> What actually happens when you clear that register bit?

Agreed. I'll fix this.

> 
> > > 
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
> > > > +	 * suggest how to do a reset over SPI, and the register strategy isn't
> > > > +	 * possible.
> > > > +	 */
> > > > +	msleep(100);
> > > > +
> > > > +	ret = core->config->init_bus(core->config);
> > > 
> > > You're not writing a bus.  I doubt you need ops call-backs.
> > 
> > In the case of SPI, the chip needs to be configured both before and
> > after reset. It sets up the bus for endianness, padding bytes, etc. This
> > is currently achieved by running "ocelot_spi_init_bus" once during SPI
> > probe, and once immediately after chip reset.
> > 
> > For other control mediums I doubt this is necessary. Perhaps "init_bus"
> > is a misnomer in this scenario...
> 
> Please find a clearer way to do this without function pointers.

Understood.

> 
> > > > +void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
> > > > +				  int size)
> > > > +{
> > > > +	if (res->name)
> > > > +		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
> > > > +	else
> > > > +		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_mfd_get_resource_name);
> > > 
> > > What is this used for?
> > > 
> > > You should not be hand rolling device resource names like this.
> > > 
> > > This sort of code belongs in the bus/class API.
> > > 
> > > Please use those instead.
> > 
> > The idea here was to allow shared regmaps across different devices. The
> > "devcpu_gcb" might be used in two ways - either everyone shares the same
> > regmap across the "GCB" range, or everyone creates their own. 
> > 
> > This was more useful when the ocelot-core.c had a copy of the 
> > "devcpu_org" regmap that was shared with ocelot-spi.c. I was able to
> > remove that, but also feel like the full switch driver (patch 6 of this
> > set) ocelot-ext should use the same "devcpu_gcb" regmap instance as
> > ocelot-core does.
> > 
> > Admittedly, there are complications. There should probably be more
> > checks added to "ocelot_regmap_init" / "get_regmap" to ensure that the
> > regmap for ocelot_ext exactly matches the existing regmap for
> > ocelot_core.
> > 
> > There's yet another complexity with these, and I'm not sure what the
> > answer is. Currently all regmaps are tied to the ocelot_spi device...
> > ocelot_spi calls devm_regmap_init. So those regmaps hang around if
> > they're created by a module that has been removed. At least until the
> > entire MFD module is removed. Maybe there's something I haven't seen yet
> > where the devres or similar has a reference count. I don't know the best
> > path forward on this one.
> 
> Why are you worrying about creating them 2 different ways?
> 
> If it's possible for them to all create and use their own regmaps,
> what's preventing you from just do that all the time?

There isn't really any worry, there just might be efficiencies to be
had if two children share the same regmap. But so long as any regmap is
created with unique names, there's no reason multiple regmaps can't
overlap the same regions. In those cases, maybe syscon would be the best
thing to implement if it becomes needed.

I have nothing against making every child regmap be unique if that's the
desire.

> 
> > > > +	/* Create and loop over all child devices here */
> > > 
> > > These need to all go in now please.
> > 
> > I'll squash them, as I saw you suggested in your other responses. I
> > tried to keep them separate, especially since adding ocelot_ext to this
> > commit (which has no functionality until this one) makes it quite a
> > large single commit. That's why I went the path I did, which was to roll
> > them in one at a time.
> 
> This is not an MFD until they are present.

Sounds good. I'll squash before the next RFC.

> 
> > > > +int ocelot_mfd_remove(struct ocelot_mfd_config *config)
> > > > +{
> > > > +	/* Loop over all children and remove them */
> > > 
> > > Use devm_* then you won't have to.
> > 
> > Yeah, I was more worried than I needed to be when I wrote that comment.
> > The only thing called to clean everything up is mfd_remove_devices();
> 
> Use devm_mfd_add_devices(), then you don't have to.
> 
> [...]

Ooh. Thanks!

> 
> > > > +#include <linux/regmap.h>
> > > > +
> > > > +struct ocelot_mfd_config {
> > > > +	struct device *dev;
> > > > +	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
> > > > +				     const struct resource *res,
> > > > +				     const char *name);
> > > > +	int (*init_bus)(struct ocelot_mfd_config *config);
> > > 
> > > Please re-work and delete this 'config' concept.
> > > 
> > > See other drivers in this sub-directory for reference.
> > 
> > Do you have a specific example? I had focused on madera for no specific
> > reason. But I really dislike the idea of throwing all of the structure
> > definition for the MFD inside of something like
> > "include/linux/mfd/ocelot/core.h", especially since all the child
> > drivers (madera-pinctrl, madera-gpio, etc) heavily rely on this struct. 
> > 
> > It seemed to me that without the concept of
> > "mfd_get_regmap_from_resource" this sort of back-and-forth was actually
> > necessary.
> 
> Things like regmaps are usually passed in via driver_data or
> platform_data.  Almost anything is better than call-backs.
> 
> [...]

I'll work to clean this up for the next RFC.

> 
> > > > +	if (!ocelot_spi)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	if (spi->max_speed_hz <= 500000) {
> > > > +		ocelot_spi->spi_padding_bytes = 0;
> > > > +	} else {
> > > > +		/*
> > > > +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
> > > > +		 * on the side of more padding bytes, as having too few can be
> > > > +		 * difficult to detect at runtime.
> > > > +		 */
> > > > +		ocelot_spi->spi_padding_bytes = 1 +
> > > > +			(spi->max_speed_hz / 1000000 + 2) / 8;
> > > 
> > > Please explain what this means or define the values (or both).
> > 
> > I can certainly elaborate the comment. Searching the manual for the term
> > "if_cfgstat" will take you right to the equation, and a description of
> > what padding bytes are, etc. 
> 
> You shouldn't insist for your readers to RTFM.
> 
> If the code doesn't read well or is overly complicated, change it.
> 
> If the complexity is required, document it in comments.

Understood. I'll elaborate.

> 
> > > > +	ocelot_spi->spi = spi;
> > > 
> > > Why are you saving this?
> > 
> > This file keeps the regmap_{read,write} implementations, so is needed
> > for spi_sync() for any regmap. There might be a better way to infer
> > this... but it seemed pretty nice to have each regmap only carry along
> > an instance of "ocelot_spi_regmap_context."
> 
> I still need Mark to look at your Regmap implementation.

Thank you. And again, I appreciate all the feedback.

> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

On Tue, Jan 11, 2022 at 10:13:43AM +0000, Lee Jones wrote:

> Unless something has changed or my understanding is not correct,
> regmap does not support over-lapping register ranges.

If there's no caches and we're always going direct to hardware it will
work a lot of the time since the buses generally have concurrency
protection at the lowest level, though if the drivers ever do any
read/modify/write operations the underlying hardware bus isn't going to
know about it so you could get data corruption if two drivers decide to
try to operate on the same register.  If there's caches things will
probably go badly since the cache will tend to amplify the
read/modify/write issues.

> However, even if that is required, I still think we can come up with
> something cleaner than creating a whole API based around creating
> and fetching different regmap configurations depending on how the
> system was initialised.

Yeah, I'd expect the usual pattern is to have wrapper drivers that
instantiate a regmap then have the bulk of the driver be a library that
they call into should work.

Hi Mark and Lee,

On Tue, Jan 11, 2022 at 01:44:28PM +0000, Mark Brown wrote:
> On Tue, Jan 11, 2022 at 10:13:43AM +0000, Lee Jones wrote:
> 
> > Unless something has changed or my understanding is not correct,
> > regmap does not support over-lapping register ranges.
> 
> If there's no caches and we're always going direct to hardware it will
> work a lot of the time since the buses generally have concurrency
> protection at the lowest level, though if the drivers ever do any
> read/modify/write operations the underlying hardware bus isn't going to
> know about it so you could get data corruption if two drivers decide to
> try to operate on the same register.  If there's caches things will
> probably go badly since the cache will tend to amplify the
> read/modify/write issues.

Good point about caches. No, nothing in these drivers utilize caches
currently, but that doesn't mean it shouldn't... or won't. Any
concurrency in this specific case would be around the SPI bus.

I think the "overlapping regmaps" already exist in the current drivers... 
but actually I'm not so sure anymore.

Either way, this is helping nudge me in the right direction.

> 
> > However, even if that is required, I still think we can come up with
> > something cleaner than creating a whole API based around creating
> > and fetching different regmap configurations depending on how the
> > system was initialised.
> 
> Yeah, I'd expect the usual pattern is to have wrapper drivers that
> instantiate a regmap then have the bulk of the driver be a library that
> they call into should work.

Understood. And I think this can make sense and clean things up. The
"ocelot_core" mfd will register every regmap range, regardless of
whether any child actually uses them. Every child can then get regmaps
by name, via dev_get_regmap. That'll get rid of the back-and-forth
regmap hooks.

I think I know where to go from here. Thank you both! I'll send along
another RFC soon, once I can get this all cleaned up.

On 11/01/2022 10:28:15-0800, Colin Foster wrote:
> At the same time, mscc-miim can be probed independently, at which point it
> would create a smaller regmap at 0x7107009c
> (Documentation/devicetree/bindings/mscc-miim.txt)
> 
> So the mscc-miim driver supports multiple use-cases. I expect the same
> type of "offset" idea can be reasonably added to the following drivers,
> all of which already exist but need to support the same type of
> use-case:
> 
> mscc,ocelot-pinctrl, mscc,ocelot-sgpio, mscc,ocelot-miim, and
> mscc,vsc7514-serdes. As I'm bringing up different parts of the hardware,
> there might be more components that become necessary.

Indeed, I guess at some point you'll need the irqchip driver too. Until
now, what I did was handling the irq controller inside the mfd driver as
reusing the irqchip driver is not trivial. Ths create a bit of code
duplication but it is not that bad.

> 
> With the exception of vsc7514-serdes, those all exist outside of MFD.
> The vsc7512-serdes driver currently relies on syscon / MFD, which adds a
> different complexity. One that I think probably merits a separate probe
> function.
> 
> > 
> > -- 
> > Lee Jones [李琼斯]
> > Principal Technical Lead - Developer Services
> > Linaro.org │ Open source software for Arm SoCs
> > Follow Linaro: Facebook | Twitter | Blog