[RFC,v7,net-next,10/13] mfd: ocelot: add support for the vsc7512 chip via spi

The VSC7512 is a networking chip that contains several peripherals. Many of
these peripherals are currently supported by the VSC7513 and VSC7514 chips,
but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
controlled externally.

Utilize the existing drivers by referencing the chip as an MFD. Add support
for the two MDIO buses, the internal phys, pinctrl, serial GPIO, and HSIO.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/mfd/Kconfig       |  24 +++
 drivers/mfd/Makefile      |   3 +
 drivers/mfd/ocelot-core.c | 189 +++++++++++++++++++++++
 drivers/mfd/ocelot-spi.c  | 313 ++++++++++++++++++++++++++++++++++++++
 drivers/mfd/ocelot.h      |  42 +++++
 include/soc/mscc/ocelot.h |   5 +
 6 files changed, 576 insertions(+)
 create mode 100644 drivers/mfd/ocelot-core.c
 create mode 100644 drivers/mfd/ocelot-spi.c
 create mode 100644 drivers/mfd/ocelot.h

On Sun, 06 Mar 2022, Colin Foster wrote:

> The VSC7512 is a networking chip that contains several peripherals. Many of
> these peripherals are currently supported by the VSC7513 and VSC7514 chips,
> but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
> controlled externally.
> 
> Utilize the existing drivers by referencing the chip as an MFD. Add support
> for the two MDIO buses, the internal phys, pinctrl, serial GPIO, and HSIO.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/mfd/Kconfig       |  24 +++
>  drivers/mfd/Makefile      |   3 +
>  drivers/mfd/ocelot-core.c | 189 +++++++++++++++++++++++
>  drivers/mfd/ocelot-spi.c  | 313 ++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/ocelot.h      |  42 +++++
>  include/soc/mscc/ocelot.h |   5 +
>  6 files changed, 576 insertions(+)
>  create mode 100644 drivers/mfd/ocelot-core.c
>  create mode 100644 drivers/mfd/ocelot-spi.c
>  create mode 100644 drivers/mfd/ocelot.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ba0b3eb131f1..d4312a5252d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -948,6 +948,30 @@ config MFD_MENF21BMC
>  	  This driver can also be built as a module. If so the module
>  	  will be called menf21bmc.
>  
> +config MFD_OCELOT
> +	tristate "Microsemi Ocelot External Control Support"
> +	select MFD_CORE
> +	help
> +	  Ocelot is a family of networking chips that support multiple ethernet
> +	  and fibre interfaces. In addition to networking, they contain several
> +	  other functions, including pictrl, MDIO, and communication with
> +	  external chips. While some chips have an internal processor capable of
> +	  running an OS, others don't. All chips can be controlled externally
> +	  through different interfaces, including SPI, I2C, and PCIe.
> +
> +	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
> +	  VSC7513, VSC7514) controlled externally.
> +
> +	  If unsure, say N
> +
> +config MFD_OCELOT_SPI
> +	tristate "Microsemi Ocelot SPI interface"
> +	depends on MFD_OCELOT
> +	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 df1ecc4a4c95..12513843067a 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)	+= 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..36e4326a853a
> --- /dev/null
> +++ b/drivers/mfd/ocelot-core.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * MFD core driver for the Ocelot chip family.

Please drop all references to 'MFD'.

'Core' is fine, as is 'Parent'.

> + * The VSC7511, 7512, 7513, and 7514 can be controlled internally via an
> + * on-chip MIPS processor, or externally via SPI, I2C, PCIe. This core driver is
> + * intended to be the bus-agnostic glue between, for example, the SPI bus and
> + * the MFD children.
> + *
> + * Copyright 2021 Innovative Advantage Inc.

Out of date?

> + * Author: Colin Foster <colin.foster@in-advantage.com>
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <soc/mscc/ocelot.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include "ocelot.h"
> +
> +#define GCB_SOFT_RST 0x0008

Tab these out to match the others?

> +#define SOFT_CHIP_RST 0x1
> +
> +#define VSC7512_GCB_RES_START	0x71070000
> +#define VSC7512_GCB_RES_SIZE	0x14
> +
> +#define VSC7512_MIIM0_RES_START	0x7107009c
> +#define VSC7512_MIIM0_RES_SIZE	0x24
> +
> +#define VSC7512_MIIM1_RES_START	0x710700c0
> +#define VSC7512_MIIM1_RES_SIZE	0x24
> +
> +#define VSC7512_PHY_RES_START	0x710700f0
> +#define VSC7512_PHY_RES_SIZE	0x4
> +
> +#define VSC7512_HSIO_RES_START	0x710d0000
> +#define VSC7512_HSIO_RES_SIZE	0x10000
> +
> +#define VSC7512_GPIO_RES_START	0x71070034
> +#define VSC7512_GPIO_RES_SIZE	0x6c
> +
> +#define VSC7512_SIO_RES_START	0x710700f8
> +#define VSC7512_SIO_RES_SIZE	0x100
> +
> +static const struct resource vsc7512_gcb_resource =
> +	DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE,
> +			     "devcpu_gcb_chip_regs");
> +
> +static int ocelot_reset(struct ocelot_core *core)
> +{
> +	int ret;
> +
> +	/*
> +	 * Reset the entire chip here to put it into a completely known state.
> +	 * Other drivers may want to reset their own subsystems. The register
> +	 * self-clears, so one write is all that is needed
> +	 */
> +	ret = regmap_write(core->gcb_regmap, GCB_SOFT_RST, SOFT_CHIP_RST);
> +	if (ret)
> +		return ret;
> +
> +	msleep(100);
> +
> +	return ret;
> +}
> +
> +static struct regmap *ocelot_devm_regmap_init(struct ocelot_core *core,
> +					      struct device *child,
> +					      const struct resource *res)
> +{
> +	/*
> +	 * Call directly into ocelot_spi to get a new regmap. This will need to
> +	 * be expanded if additional bus types are to be supported in the
> +	 * future.
> +	 */
> +	return ocelot_spi_devm_get_regmap(core, child, res);
> +}
> +
> +struct regmap *ocelot_get_regmap_from_resource(struct device *child,
> +					       const struct resource *res)
> +{
> +	struct ocelot_core *core = dev_get_drvdata(child->parent);
> +
> +	return ocelot_devm_regmap_init(core, child, res);
> +}
> +EXPORT_SYMBOL(ocelot_get_regmap_from_resource);

What's the reason for having an additional function when one would do?

> +static const struct resource vsc7512_miim0_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_MIIM0_RES_START, VSC7512_MIIM0_RES_SIZE,
> +			     "gcb_miim0"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PHY_RES_START, VSC7512_PHY_RES_SIZE,
> +			     "gcb_phy"),
> +};
> +
> +static const struct resource vsc7512_miim1_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_MIIM1_RES_START, VSC7512_MIIM1_RES_SIZE,
> +			     "gcb_miim1"),
> +};
> +
> +static const struct resource vsc7512_hsio_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE,
> +			     "hsio"),
> +};
> +
> +static const struct resource vsc7512_pinctrl_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_GPIO_RES_START, VSC7512_GPIO_RES_SIZE,
> +			     "gcb_gpio"),
> +};
> +
> +static const struct resource vsc7512_sgpio_resources[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_SIO_RES_START, VSC7512_SIO_RES_SIZE,
> +			     "gcb_sio"),
> +};
> +
> +static const struct mfd_cell vsc7512_devs[] = {
> +	{
> +		.name = "ocelot-pinctrl",
> +		.of_compatible = "mscc,ocelot-pinctrl",
> +		.num_resources = ARRAY_SIZE(vsc7512_pinctrl_resources),
> +		.resources = vsc7512_pinctrl_resources,
> +	}, {
> +		.name = "ocelot-sgpio",
> +		.of_compatible = "mscc,ocelot-sgpio",
> +		.num_resources = ARRAY_SIZE(vsc7512_sgpio_resources),
> +		.resources = vsc7512_sgpio_resources,
> +	}, {
> +		.name = "ocelot-miim0",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim0_resources),
> +		.resources = vsc7512_miim0_resources,
> +	}, {
> +		.name = "ocelot-miim1",
> +		.of_compatible = "mscc,ocelot-miim",
> +		.num_resources = ARRAY_SIZE(vsc7512_miim1_resources),
> +		.resources = vsc7512_miim1_resources,
> +	}, {
> +		.name = "ocelot-serdes",
> +		.of_compatible = "mscc,vsc7514-serdes",
> +		.num_resources = ARRAY_SIZE(vsc7512_hsio_resources),
> +		.resources = vsc7512_hsio_resources,
> +	},
> +};
> +
> +int ocelot_core_init(struct ocelot_core *core)
> +{
> +	struct device *dev = core->dev;
> +	int ret;
> +
> +	dev_set_drvdata(dev, core);
> +
> +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> +						   &vsc7512_gcb_resource);

This just ends up calling ocelot_spi_devm_get_regmap() right?

Why not call that from inside ocelot-spi.c where 'core' was allocated?

> +	if (IS_ERR(core->gcb_regmap))
> +		return -ENOMEM;
> +
> +	ret = ocelot_reset(core);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * A chip reset will clear the SPI configuration, so it needs to be done
> +	 * again before we can access any registers
> +	 */
> +	ret = ocelot_spi_initialize(core);

Not a fan of calling back into the file which called us.

And what happens if SPI isn't controlling us?

Doesn't the documentation above say this device can also be
communicated with via I2C and PCIe?

> +	if (ret) {
> +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_core_init);
> +
> +MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> new file mode 100644
> index 000000000000..c788e239c9a7
> --- /dev/null
> +++ b/drivers/mfd/ocelot-spi.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * SPI core driver for the Ocelot chip family.
> + *
> + * This driver will handle everything necessary to allow for communication over
> + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> + * processor's endianness. This will create and distribute regmaps for any MFD

As above, please drop references to MFD.

> + * children.
> + *
> + * Copyright 2021 Innovative Advantage Inc.
> + *
> + * Author: Colin Foster <colin.foster@in-advantage.com>
> + */
> +
> +#include <linux/iopoll.h>
> +#include <linux/kconfig.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include "ocelot.h"
> +
> +#define DEV_CPUORG_IF_CTRL	0x0000
> +#define DEV_CPUORG_IF_CFGSTAT	0x0004
> +
> +#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
> +#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
> +#define CFGSTAT_IF_NUM_SI	(2 << 24)
> +#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
> +
> +
> +static const struct resource vsc7512_dev_cpuorg_resource = {
> +	.start	= 0x71000000,
> +	.end	= 0x710002ff,

No magic numbers.  Please define these addresses.

> +	.name	= "devcpu_org",
> +};
> +
> +#define VSC7512_BYTE_ORDER_LE 0x00000000
> +#define VSC7512_BYTE_ORDER_BE 0x81818181
> +#define VSC7512_BIT_ORDER_MSB 0x00000000
> +#define VSC7512_BIT_ORDER_LSB 0x42424242
> +
> +int ocelot_spi_initialize(struct ocelot_core *core)
> +{
> +	u32 val, check;
> +	int err;
> +
> +#ifdef __LITTLE_ENDIAN
> +	val = VSC7512_BYTE_ORDER_LE;
> +#else
> +	val = VSC7512_BYTE_ORDER_BE;
> +#endif

Not a fan of ifdefery in the middle of C files.

Please create a macro or define somewhere.

> +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> +	if (err)
> +		return err;

Comment.

> +	val = core->spi_padding_bytes;
> +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> +	if (err)
> +		return err;

Comment.

> +	/*
> +	 * After we write the interface configuration, read it back here. This
> +	 * will verify several different things. The first is that the number of
> +	 * padding bytes actually got written correctly. These are found in bits
> +	 * 0:3.
> +	 *
> +	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
> +	 * and will be set if the register access is too fast. This would be in
> +	 * the condition that the number of padding bytes is insufficient for
> +	 * the SPI bus frequency.
> +	 *
> +	 * The last check is for bits 31:24, which define the interface by which
> +	 * the registers are being accessed. Since we're accessing them via the
> +	 * serial interface, it must return IF_NUM_SI.
> +	 */
> +	check = val | CFGSTAT_IF_NUM_SI;
> +
> +	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> +	if (err)
> +		return err;
> +
> +	if (check != val)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_spi_initialize);
> +
> +/*
> + * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
> + * the register locations are defined as 32-bit. The least significant two bits
> + * get shifted out, as register accesses must always be word-aligned, leaving
> + * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
> + * payload can be optimized for bit / byte order to match whatever architecture
> + * the controlling CPU has.
> + */
> +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> +{
> +	return cpu_to_be32((reg & 0xffffff) >> 2);
> +}
> +
> +struct ocelot_spi_regmap_context {
> +	u32 base;
> +	struct ocelot_core *core;
> +};
> +
> +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> +			       unsigned int *val)
> +{
> +	struct ocelot_spi_regmap_context *regmap_context = context;
> +	struct ocelot_core *core = regmap_context->core;
> +	struct spi_transfer tx, padding, rx;
> +	struct spi_message msg;

How big are all of these?

We will receive warnings if they occupy too much stack space.

> +	struct spi_device *spi;
> +	unsigned int addr;
> +	u8 *tx_buf;
> +
> +	spi = core->spi;
> +
> +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> +	tx_buf = (u8 *)&addr;
> +
> +	spi_message_init(&msg);
> +
> +	memset(&tx, 0, sizeof(tx));
> +
> +	/* Ignore the first byte for the 24-bit address */
> +	tx.tx_buf = &tx_buf[1];
> +	tx.len = 3;
> +
> +	spi_message_add_tail(&tx, &msg);
> +
> +	if (core->spi_padding_bytes > 0) {
> +		u8 dummy_buf[16] = {0};
> +
> +		memset(&padding, 0, sizeof(padding));
> +
> +		/* Just toggle the clock for padding bytes */
> +		padding.len = core->spi_padding_bytes;
> +		padding.tx_buf = dummy_buf;
> +		padding.dummy_data = 1;
> +
> +		spi_message_add_tail(&padding, &msg);
> +	}
> +
> +	memset(&rx, 0, sizeof(rx));
> +	rx.rx_buf = val;
> +	rx.len = 4;
> +
> +	spi_message_add_tail(&rx, &msg);
> +
> +	return spi_sync(spi, &msg);
> +}
> +
> +static int ocelot_spi_reg_write(void *context, unsigned int reg,
> +				unsigned int val)
> +{
> +	struct ocelot_spi_regmap_context *regmap_context = context;
> +	struct ocelot_core *core = regmap_context->core;
> +	struct spi_transfer tx[2] = {0};
> +	struct spi_message msg;
> +	struct spi_device *spi;
> +	unsigned int addr;
> +	u8 *tx_buf;
> +
> +	spi = core->spi;
> +
> +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> +	tx_buf = (u8 *)&addr;
> +
> +	spi_message_init(&msg);
> +
> +	/* Ignore the first byte for the 24-bit address and set the write bit */
> +	tx_buf[1] |= BIT(7);
> +	tx[0].tx_buf = &tx_buf[1];
> +	tx[0].len = 3;
> +
> +	spi_message_add_tail(&tx[0], &msg);
> +
> +	memset(&tx[1], 0, sizeof(struct spi_transfer));
> +	tx[1].tx_buf = &val;
> +	tx[1].len = 4;
> +
> +	spi_message_add_tail(&tx[1], &msg);
> +
> +	return spi_sync(spi, &msg);
> +}
> +
> +static const struct regmap_config ocelot_spi_regmap_config = {
> +	.reg_bits = 24,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +
> +	.reg_read = ocelot_spi_reg_read,
> +	.reg_write = ocelot_spi_reg_write,
> +
> +	.max_register = 0xffffffff,
> +	.use_single_write = true,
> +	.use_single_read = true,
> +	.can_multi_write = false,
> +
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> +};
> +
> +struct regmap *
> +ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
> +			   const struct resource *res)

This seems to always initialise a new Regmap.

To me 'get' implies that it could fetch an already existing one.

... and *perhaps* init a new one if none exists..

> +{
> +	struct ocelot_spi_regmap_context *context;
> +	struct regmap_config regmap_config;
> +
> +	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
> +	if (IS_ERR(context))
> +		return ERR_CAST(context);
> +
> +	context->base = res->start;
> +	context->core = core;
> +
> +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> +	       sizeof(ocelot_spi_regmap_config));
> +
> +	regmap_config.name = res->name;
> +	regmap_config.max_register = res->end - res->start;
> +
> +	return devm_regmap_init(child, NULL, context, &regmap_config);
> +}
> +
> +static int ocelot_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ocelot_core *core;

This would be more in keeping with current drivers if you dropped the
'_core' part of the struct name and called the variable ddata.

> +	int err;
> +
> +	core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
> +	if (!core)
> +		return -ENOMEM;

If you save 'core' (or preferably ddata) here and place it in the
device's driver_data slot via dev_set_drvdata(), you can just pass
around the 'struct device' which is more in keeping with current
implementations.

> +	if (spi->max_speed_hz <= 500000) {
> +		core->spi_padding_bytes = 0;
> +	} else {
> +		/*
> +		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG.
> +		 * Register access time is 1us, so we need to configure and send
> +		 * out enough padding bytes between the read request and data
> +		 * transmission that lasts at least 1 microsecond.
> +		 */
> +		core->spi_padding_bytes = 1 +
> +			(spi->max_speed_hz / 1000000 + 2) / 8;
> +	}
> +
> +	core->spi = spi;
> +
> +	spi->bits_per_word = 8;
> +
> +	err = spi_setup(spi);
> +	if (err < 0) {
> +		dev_err(&spi->dev, "Error %d initializing SPI\n", err);
> +		return err;
> +	}
> +
> +	core->cpuorg_regmap =
> +		ocelot_spi_devm_get_regmap(core, dev,
> +					   &vsc7512_dev_cpuorg_resource);
> +	if (IS_ERR(core->cpuorg_regmap))
> +		return -ENOMEM;
> +
> +	core->dev = dev;
> +
> +	/*
> +	 * The chip must be set up for SPI before it gets initialized and reset.
> +	 * This must be done before calling init, and after a chip reset is
> +	 * performed.
> +	 */
> +	err = ocelot_spi_initialize(core);
> +	if (err) {
> +		dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);
> +		return err;
> +	}
> +
> +	err = ocelot_core_init(core);
> +	if (err < 0) {
> +		dev_err(dev, "Error %d initializing Ocelot MFD\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +const struct of_device_id ocelot_spi_of_match[] = {
> +	{ .compatible = "mscc,vsc7512_mfd_spi" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ocelot_spi_of_match);
> +
> +static struct spi_driver ocelot_spi_driver = {
> +	.driver = {
> +		.name = "ocelot_mfd_spi",
> +		.of_match_table = of_match_ptr(ocelot_spi_of_match),
> +	},
> +	.probe = ocelot_spi_probe,
> +};
> +module_spi_driver(ocelot_spi_driver);
> +
> +MODULE_DESCRIPTION("SPI Controlled Ocelot Chip Driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> +MODULE_LICENSE("Dual MIT/GPL");
> diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
> new file mode 100644
> index 000000000000..20d3853dd6d2
> --- /dev/null
> +++ b/drivers/mfd/ocelot.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright 2021 Innovative Advantage Inc.
> + */
> +
> +#include <linux/kconfig.h>
> +#include <linux/regmap.h>
> +
> +struct ocelot_core {
> +	struct device *dev;
> +	struct regmap *gcb_regmap;
> +	struct regmap *cpuorg_regmap;
> +
> +#if IS_ENABLED(CONFIG_MFD_OCELOT_SPI)

I'd drop this personally.

> +	int spi_padding_bytes;
> +	struct spi_device *spi;
> +#endif
> +};
> +
> +void ocelot_get_resource_name(char *name, const struct resource *res,
> +			      int size);
> +int ocelot_core_init(struct ocelot_core *core);
> +int ocelot_remove(struct ocelot_core *core);

This doesn't appear to be relevant.

> +#if IS_ENABLED(CONFIG_MFD_OCELOT_SPI)
> +struct regmap *ocelot_spi_devm_get_regmap(struct ocelot_core *core,
> +					  struct device *child,
> +					  const struct resource *res);
> +int ocelot_spi_initialize(struct ocelot_core *core);
> +#else
> +static inline struct regmap *ocelot_spi_devm_get_regmap(
> +		struct ocelot_core *core, struct device *child,
> +		const struct resource *res)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline int ocelot_spi_initialize(struct ocelot_core *core)
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 998616511ffb..d9e2710d5646 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -1018,11 +1018,16 @@ ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_MFD_OCELOT)
> +struct regmap *ocelot_get_regmap_from_resource(struct device *child,
> +					       const struct resource *res);
> +#else
>  static inline struct regmap *
>  ocelot_get_regmap_from_resource(struct device *child,
>  				const struct resource *res)
>  {
>  	return ERR_PTR(-EOPNOTSUPP);
>  }
> +#endif
>  
>  #endif

[Adding everyone/lists back on Cc]

On Thu, 14 Apr 2022, Colin Foster wrote:

> Hi Lee,
> 
> Thanks for the feedback. I agree with (and have made) your suggestions.
> Additional comments below.

I'm swamped right now, so I cannot do a full re-review, but please see
in-line for a couple of (most likely flippant i.e. not fully
thought out comments).

Please submit the changes you end up making off the back of this
review and I'll conduct another on the next version you send.

> On Wed, Apr 13, 2022 at 09:32:22AM +0100, Lee Jones wrote:
> > On Sun, 06 Mar 2022, Colin Foster wrote:
> > 
> [...]
> > > +
> > > +int ocelot_core_init(struct ocelot_core *core)
> > > +{
> > > +	struct device *dev = core->dev;
> > > +	int ret;
> > > +
> > > +	dev_set_drvdata(dev, core);
> > > +
> > > +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> > > +						   &vsc7512_gcb_resource);
> > 
> > This just ends up calling ocelot_spi_devm_get_regmap() right?
> > 
> > Why not call that from inside ocelot-spi.c where 'core' was allocated?
> 
> core->gcb_regmap doesn't handle anything more than chip reset. This will
> have to happen regardless of the interface.
> 
> The "spi" part uses the core->cpuorg_regmap, which is needed for
> configuring the SPI bus. In the case of I2C, this cpu_org regmap
> (likely?) wouldn't be necessary, but the gcb_regmap absolutely would.
> That's why gcb is allocated in core and cpuorg is allocated in SPI.
> 
> The previous RFC had cpuorg_regmap hidden inside a private struct to
> emphasize this separation. As you pointed out, there was a lot of
> bouncing between "core" structs and "spi" structs that got ugly.
> 
> (Looking at this more now... the value of cpuorg_regmap should have been
> in the CONFIG_MFD_OCELOT_SPI ifdef, which might have made this
> distinction more clear)

The TL;DR of my review point would be to make this as simple as
possible.  If you can call a single function, instead of needlessly
sending the thread of execution through three, please do.

> > > +	if (IS_ERR(core->gcb_regmap))
> > > +		return -ENOMEM;
> > > +
> > > +	ret = ocelot_reset(core);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to reset device: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * A chip reset will clear the SPI configuration, so it needs to be done
> > > +	 * again before we can access any registers
> > > +	 */
> > > +	ret = ocelot_spi_initialize(core);
> > 
> > Not a fan of calling back into the file which called us.
> > 
> > And what happens if SPI isn't controlling us?
> > 
> > Doesn't the documentation above say this device can also be
> > communicated with via I2C and PCIe?
> 
> During the last RFC this was done through a callback. You had requested
> I not use callbacks.
> 
> From that exchange:
> """"
> > > > +	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.
> """"

Yes, I remember.

This is an improvement on that, but it doesn't mean it's ideal.

> The idea is that we set up the SPI bus so we can read registers. The
> protocol changes based on bus speed, so this is necessary.
> 
> This initial setup is done in ocelot-spi.c, before ocelot_core_init is
> called.
> 
> We then reset the chip by writing some registers. This chip reset also
> clears the SPI configuration, so we need to reconfigure the SPI bus
> before we can read any additional registers.
> 
> Short of using function pointers, I imagine this will have to be
> something akin to:
> 
> if (core->is_spi) {
>     ocelot_spi_initalize();
> }

What about if, instead of calling from SPI into Core, which calls back
into SPI again, you do this from SPI instead:

[flippant - I haven't fully assessed the viability of this suggestion]

foo_type spi_probe(bar_type baz)
{
  setup_spi();

  core_init();

  more_spi_stuff();
}

> I feel if the additional buses are added, they'll have to implement this
> type of change. But as I don't (and don't plan to) have hardware to
> build those interfaces out, right now ocelot_core assumes the bus is
> SPI.

What are the chances of someone else coming along and implementing the
other interfaces?  You might very well be over-complicating this
implementation for support that may never be required.

> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> > > +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(ocelot_core_init);
> > > +
> > > +MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
> > > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > > new file mode 100644
> > > index 000000000000..c788e239c9a7
> > > --- /dev/null
> > > +++ b/drivers/mfd/ocelot-spi.c
> > > @@ -0,0 +1,313 @@
> > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > > +/*
> > > + * SPI core driver for the Ocelot chip family.
> > > + *
> > > + * This driver will handle everything necessary to allow for communication over
> > > + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> > > + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> > > + * processor's endianness. This will create and distribute regmaps for any MFD
> > 
> > As above, please drop references to MFD.
> > 
> > > + * children.
> > > + *
> > > + * Copyright 2021 Innovative Advantage Inc.
> > > + *
> > > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > > + */
> > > +
> > > +#include <linux/iopoll.h>
> > > +#include <linux/kconfig.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> > > +
> > > +#include <asm/byteorder.h>
> > > +
> > > +#include "ocelot.h"
> > > +
> > > +#define DEV_CPUORG_IF_CTRL	0x0000
> > > +#define DEV_CPUORG_IF_CFGSTAT	0x0004
> > > +
> > > +#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
> > > +#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
> > > +#define CFGSTAT_IF_NUM_SI	(2 << 24)
> > > +#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
> > > +
> > > +
> > > +static const struct resource vsc7512_dev_cpuorg_resource = {
> > > +	.start	= 0x71000000,
> > > +	.end	= 0x710002ff,
> > 
> > No magic numbers.  Please define these addresses.
> 
> I missed these. Thanks.
> 
> > 
> > > +	.name	= "devcpu_org",
> > > +};
> > > +
> > > +#define VSC7512_BYTE_ORDER_LE 0x00000000
> > > +#define VSC7512_BYTE_ORDER_BE 0x81818181
> > > +#define VSC7512_BIT_ORDER_MSB 0x00000000
> > > +#define VSC7512_BIT_ORDER_LSB 0x42424242
> > > +
> > > +int ocelot_spi_initialize(struct ocelot_core *core)
> > > +{
> > > +	u32 val, check;
> > > +	int err;
> > > +
> > > +#ifdef __LITTLE_ENDIAN
> > > +	val = VSC7512_BYTE_ORDER_LE;
> > > +#else
> > > +	val = VSC7512_BYTE_ORDER_BE;
> > > +#endif
> > 
> > Not a fan of ifdefery in the middle of C files.
> > 
> > Please create a macro or define somewhere.
> 
> I'll clear this up in comments and move things around. This macro
> specifically tends to lend itself to this type of ifdef dropping:
> 
> https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/__LITTLE_ENDIAN

I see that the majority of implementations exist in header files as I
would expect.  With respect to the others, past acceptance and what is
acceptable in other subsystems has little bearing on what will be
accepted here and now.

> The comment I'm adding is:
>         /*
>          * The SPI address must be big-endian, but we want the payload to match
>          * our CPU. These are two bits (0 and 1) but they're repeated such that
>          * the write from any configuration will be valid. The four
>          * configurations are:
>          *
>          * 0b00: little-endian, MSB first
>          * |            111111   | 22221111 | 33222222 |
>          * | 76543210 | 54321098 | 32109876 | 10987654 |
>          *
>          * 0b01: big-endian, MSB first
>          * | 33222222 | 22221111 | 111111   |          |
>          * | 10987654 | 32109876 | 54321098 | 76543210 |
>          *
>          * 0b10: little-endian, LSB first
>          * |              111111 | 11112222 | 22222233 |
>          * | 01234567 | 89012345 | 67890123 | 45678901 |
>          *
>          * 0b11: big-endian, LSB first
>          * | 22222233 | 11112222 |   111111 |          |
>          * | 45678901 | 67890123 | 89012345 | 01234567 |
>          */
> 
> With this info, would you recommend:
> 1. A file-scope static const at the top of this file
> 2. A macro assigned to one of those sequences
> 3. A function to "detect" which architecture we're running

I do not have a strong opinion.

Just tuck the #iferry away somewhere in a header file.

> > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> > > +	if (err)
> > > +		return err;
> > 
> > Comment.
> > 
> > > +	val = core->spi_padding_bytes;
> > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> > > +	if (err)
> > > +		return err;
> > 
> > Comment.
> 
> Adding:
> 
> /*
>  * Apply the number of padding bytes between a read request and the data
>  * payload. Some registers have access times of up to 1us, so if the
>  * first payload bit is shifted out too quickly, the read will fail.
>  */
> 
> > 
> > > +	/*
> > > +	 * After we write the interface configuration, read it back here. This
> > > +	 * will verify several different things. The first is that the number of
> > > +	 * padding bytes actually got written correctly. These are found in bits
> > > +	 * 0:3.
> > > +	 *
> > > +	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
> > > +	 * and will be set if the register access is too fast. This would be in
> > > +	 * the condition that the number of padding bytes is insufficient for
> > > +	 * the SPI bus frequency.
> > > +	 *
> > > +	 * The last check is for bits 31:24, which define the interface by which
> > > +	 * the registers are being accessed. Since we're accessing them via the
> > > +	 * serial interface, it must return IF_NUM_SI.
> > > +	 */
> > > +	check = val | CFGSTAT_IF_NUM_SI;
> > > +
> > > +	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (check != val)
> > > +		return -ENODEV;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(ocelot_spi_initialize);
> > > +
> > > +/*
> > > + * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
> > > + * the register locations are defined as 32-bit. The least significant two bits
> > > + * get shifted out, as register accesses must always be word-aligned, leaving
> > > + * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
> > > + * payload can be optimized for bit / byte order to match whatever architecture
> > > + * the controlling CPU has.
> > > + */
> > > +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> > > +{
> > > +	return cpu_to_be32((reg & 0xffffff) >> 2);
> > > +}
> > > +
> > > +struct ocelot_spi_regmap_context {
> > > +	u32 base;
> > > +	struct ocelot_core *core;
> > > +};
> > > +
> > > +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> > > +			       unsigned int *val)
> > > +{
> > > +	struct ocelot_spi_regmap_context *regmap_context = context;
> > > +	struct ocelot_core *core = regmap_context->core;
> > > +	struct spi_transfer tx, padding, rx;
> > > +	struct spi_message msg;
> > 
> > How big are all of these?
> > 
> > We will receive warnings if they occupy too much stack space.
> 
> Looking at the structs they're on the order of 10s of bytes. Maybe 70
> bytes per instance (back of napkin calculation)
> 
> But it seems very common to stack-allocate spi_transfers:
> 
> https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/spi/spi.c#L4097
> https://elixir.bootlin.com/linux/v5.18-rc2/source/include/linux/spi/spi.h#L1244
> 
> Do you have a feel for at what point that becomes a concern?

That's fine.  I just want you to bear this in mind.

I wish to prevent adding yet more W=1 level warnings into the kernel.

> > > +	struct spi_device *spi;
> > > +	unsigned int addr;
> > > +	u8 *tx_buf;
> > > +
> > > +	spi = core->spi;
> > > +
> > > +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> > > +	tx_buf = (u8 *)&addr;
> > > +
> > > +	spi_message_init(&msg);
> > > +
> > > +	memset(&tx, 0, sizeof(tx));
> > > +
> > > +	/* Ignore the first byte for the 24-bit address */
> > > +	tx.tx_buf = &tx_buf[1];
> > > +	tx.len = 3;
> > > +
> > > +	spi_message_add_tail(&tx, &msg);
> > > +
> > > +	if (core->spi_padding_bytes > 0) {
> > > +		u8 dummy_buf[16] = {0};
> > > +
> > > +		memset(&padding, 0, sizeof(padding));
> > > +
> > > +		/* Just toggle the clock for padding bytes */
> > > +		padding.len = core->spi_padding_bytes;
> > > +		padding.tx_buf = dummy_buf;
> > > +		padding.dummy_data = 1;
> > > +
> > > +		spi_message_add_tail(&padding, &msg);
> > > +	}
> > > +
> > > +	memset(&rx, 0, sizeof(rx));
> > > +	rx.rx_buf = val;
> > > +	rx.len = 4;
> > > +
> > > +	spi_message_add_tail(&rx, &msg);
> > > +
> > > +	return spi_sync(spi, &msg);
> > > +}
> > > +
> > > +static int ocelot_spi_reg_write(void *context, unsigned int reg,
> > > +				unsigned int val)
> > > +{
> > > +	struct ocelot_spi_regmap_context *regmap_context = context;
> > > +	struct ocelot_core *core = regmap_context->core;
> > > +	struct spi_transfer tx[2] = {0};
> > > +	struct spi_message msg;
> > > +	struct spi_device *spi;
> > > +	unsigned int addr;
> > > +	u8 *tx_buf;
> > > +
> > > +	spi = core->spi;
> > > +
> > > +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> > > +	tx_buf = (u8 *)&addr;
> > > +
> > > +	spi_message_init(&msg);
> > > +
> > > +	/* Ignore the first byte for the 24-bit address and set the write bit */
> > > +	tx_buf[1] |= BIT(7);
> > > +	tx[0].tx_buf = &tx_buf[1];
> > > +	tx[0].len = 3;
> > > +
> > > +	spi_message_add_tail(&tx[0], &msg);
> > > +
> > > +	memset(&tx[1], 0, sizeof(struct spi_transfer));
> > > +	tx[1].tx_buf = &val;
> > > +	tx[1].len = 4;
> > > +
> > > +	spi_message_add_tail(&tx[1], &msg);
> > > +
> > > +	return spi_sync(spi, &msg);
> > > +}
> > > +
> > > +static const struct regmap_config ocelot_spi_regmap_config = {
> > > +	.reg_bits = 24,
> > > +	.reg_stride = 4,
> > > +	.val_bits = 32,
> > > +
> > > +	.reg_read = ocelot_spi_reg_read,
> > > +	.reg_write = ocelot_spi_reg_write,
> > > +
> > > +	.max_register = 0xffffffff,
> > > +	.use_single_write = true,
> > > +	.use_single_read = true,
> > > +	.can_multi_write = false,
> > > +
> > > +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> > > +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> > > +};
> > > +
> > > +struct regmap *
> > > +ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
> > > +			   const struct resource *res)
> > 
> > This seems to always initialise a new Regmap.
> > 
> > To me 'get' implies that it could fetch an already existing one.
> > 
> > ... and *perhaps* init a new one if none exists..
> 
> That's exactly what my intention was when I started.
> 
> But it seems like *if* that is something that is required, it should be
> done through a syscon / device tree implementation and not be snuck into
> this regmap getter. I was trying to do too much.
> 
> I'm renaming to "init"
> 
> > 
> > > +{
> > > +	struct ocelot_spi_regmap_context *context;
> > > +	struct regmap_config regmap_config;
> > > +
> > > +	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
> > > +	if (IS_ERR(context))
> > > +		return ERR_CAST(context);
> > > +
> > > +	context->base = res->start;
> > > +	context->core = core;
> > > +
> > > +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > > +	       sizeof(ocelot_spi_regmap_config));
> > > +
> > > +	regmap_config.name = res->name;
> > > +	regmap_config.max_register = res->end - res->start;
> > > +
> > > +	return devm_regmap_init(child, NULL, context, &regmap_config);
> > > +}
> > > +
> > > +static int ocelot_spi_probe(struct spi_device *spi)
> > > +{
> > > +	struct device *dev = &spi->dev;
> > > +	struct ocelot_core *core;
> > 
> > This would be more in keeping with current drivers if you dropped the
> > '_core' part of the struct name and called the variable ddata.
> 
> There's already a "struct ocelot" defined in include/soc/mscc/ocelot.h.
> I suppose it could be renamed to align with what it actually is: the
> "switch" component of the ocelot chip.
> 
> Vladimir, Alexandre, Horaitu, others:
> Any opinions about this becoming "struct ocelot" and the current struct
> being "struct ocelot_switch"?
> 
> Or maybe a technical / philosophical question: is "ocelot" the switch
> core that can be implemented in other hardware? Or is it the chip family
> entirely, (pinctrl, sgpio, etc.) who's switch core was brought into
> other products?
> 
> The existing struct change would hit about 30 files.
> https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/ocelot

That's not ideal.

Please consider using 'ocelot_ddata' for now and consider a larger
overhaul at a later date, if it makes sense to do so.

[...]

Thanks for the feedback Lee,

I'll do some final cleanup (hopefully this month...?) and prepare
another patch set.


Something I plan to do, lest anyone object, is send the next patch set
that explicitly states that ports 4-10 currently aren't supported, and 
inform a user as such. One main reason for this is that the additional
ports rely on the drivers/phy/mscc/phy-ocelot-serdes driver, which
utilizes syscon_node_to_regmap. The same issue comes up, where syscon of
course only supports mmio. I can foresee that being worthy of its own
rounds of reviews, and will change the .

But I'm quite new to this process - so if that isn't an acceptable path
forward I understand.


On Tue, Apr 19, 2022 at 10:07:04AM +0100, Lee Jones wrote:
> [Adding everyone/lists back on Cc]

Oops... I'm not sure how I did that. Thanks!

> 
> On Thu, 14 Apr 2022, Colin Foster wrote:
> 
> > Hi Lee,
> > 
> > Thanks for the feedback. I agree with (and have made) your suggestions.
> > Additional comments below.
> 
> I'm swamped right now, so I cannot do a full re-review, but please see
> in-line for a couple of (most likely flippant i.e. not fully
> thought out comments).
> 
> Please submit the changes you end up making off the back of this
> review and I'll conduct another on the next version you send.
> 
> > On Wed, Apr 13, 2022 at 09:32:22AM +0100, Lee Jones wrote:
> > > On Sun, 06 Mar 2022, Colin Foster wrote:
> > > 
> > [...]
> > > > +
> > > > +int ocelot_core_init(struct ocelot_core *core)
> > > > +{
> > > > +	struct device *dev = core->dev;
> > > > +	int ret;
> > > > +
> > > > +	dev_set_drvdata(dev, core);
> > > > +
> > > > +	core->gcb_regmap = ocelot_devm_regmap_init(core, dev,
> > > > +						   &vsc7512_gcb_resource);
> > > 
> > > This just ends up calling ocelot_spi_devm_get_regmap() right?
> > > 
> > > Why not call that from inside ocelot-spi.c where 'core' was allocated?
> > 
> > core->gcb_regmap doesn't handle anything more than chip reset. This will
> > have to happen regardless of the interface.
> > 
> > The "spi" part uses the core->cpuorg_regmap, which is needed for
> > configuring the SPI bus. In the case of I2C, this cpu_org regmap
> > (likely?) wouldn't be necessary, but the gcb_regmap absolutely would.
> > That's why gcb is allocated in core and cpuorg is allocated in SPI.
> > 
> > The previous RFC had cpuorg_regmap hidden inside a private struct to
> > emphasize this separation. As you pointed out, there was a lot of
> > bouncing between "core" structs and "spi" structs that got ugly.
> > 
> > (Looking at this more now... the value of cpuorg_regmap should have been
> > in the CONFIG_MFD_OCELOT_SPI ifdef, which might have made this
> > distinction more clear)
> 
> The TL;DR of my review point would be to make this as simple as
> possible.  If you can call a single function, instead of needlessly
> sending the thread of execution through three, please do.
> 
> > > > +	if (IS_ERR(core->gcb_regmap))
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = ocelot_reset(core);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to reset device: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * A chip reset will clear the SPI configuration, so it needs to be done
> > > > +	 * again before we can access any registers
> > > > +	 */
> > > > +	ret = ocelot_spi_initialize(core);
> > > 
> > > Not a fan of calling back into the file which called us.
> > > 
> > > And what happens if SPI isn't controlling us?
> > > 
> > > Doesn't the documentation above say this device can also be
> > > communicated with via I2C and PCIe?
> > 
> > During the last RFC this was done through a callback. You had requested
> > I not use callbacks.
> > 
> > From that exchange:
> > """"
> > > > > +	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.
> > """"
> 
> Yes, I remember.
> 
> This is an improvement on that, but it doesn't mean it's ideal.
> 
> > The idea is that we set up the SPI bus so we can read registers. The
> > protocol changes based on bus speed, so this is necessary.
> > 
> > This initial setup is done in ocelot-spi.c, before ocelot_core_init is
> > called.
> > 
> > We then reset the chip by writing some registers. This chip reset also
> > clears the SPI configuration, so we need to reconfigure the SPI bus
> > before we can read any additional registers.
> > 
> > Short of using function pointers, I imagine this will have to be
> > something akin to:
> > 
> > if (core->is_spi) {
> >     ocelot_spi_initalize();
> > }
> 
> What about if, instead of calling from SPI into Core, which calls back
> into SPI again, you do this from SPI instead:
> 
> [flippant - I haven't fully assessed the viability of this suggestion]
> 
> foo_type spi_probe(bar_type baz)
> {
>   setup_spi();
> 
>   core_init();
> 
>   more_spi_stuff();
> }
> 
> > I feel if the additional buses are added, they'll have to implement this
> > type of change. But as I don't (and don't plan to) have hardware to
> > build those interfaces out, right now ocelot_core assumes the bus is
> > SPI.
> 
> What are the chances of someone else coming along and implementing the
> other interfaces?  You might very well be over-complicating this
> implementation for support that may never be required.

I had one person email me about this already, though I understand they
went another direction.

But I could see this back-and-forth going away. I'll take another look
at it and try to clean it up a little more.

> 
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to initialize SPI interface: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> > > > +				   ARRAY_SIZE(vsc7512_devs), NULL, 0, NULL);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to add sub-devices: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_core_init);
> > > > +
> > > > +MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
> > > > +MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
> > > > +MODULE_LICENSE("GPL v2");
> > > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > > > new file mode 100644
> > > > index 000000000000..c788e239c9a7
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/ocelot-spi.c
> > > > @@ -0,0 +1,313 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > > > +/*
> > > > + * SPI core driver for the Ocelot chip family.
> > > > + *
> > > > + * This driver will handle everything necessary to allow for communication over
> > > > + * SPI to the VSC7511, VSC7512, VSC7513 and VSC7514 chips. The main functions
> > > > + * are to prepare the chip's SPI interface for a specific bus speed, and a host
> > > > + * processor's endianness. This will create and distribute regmaps for any MFD
> > > 
> > > As above, please drop references to MFD.
> > > 
> > > > + * children.
> > > > + *
> > > > + * Copyright 2021 Innovative Advantage Inc.
> > > > + *
> > > > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > > > + */
> > > > +
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/kconfig.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/spi/spi.h>
> > > > +
> > > > +#include <asm/byteorder.h>
> > > > +
> > > > +#include "ocelot.h"
> > > > +
> > > > +#define DEV_CPUORG_IF_CTRL	0x0000
> > > > +#define DEV_CPUORG_IF_CFGSTAT	0x0004
> > > > +
> > > > +#define CFGSTAT_IF_NUM_VCORE	(0 << 24)
> > > > +#define CFGSTAT_IF_NUM_VRAP	(1 << 24)
> > > > +#define CFGSTAT_IF_NUM_SI	(2 << 24)
> > > > +#define CFGSTAT_IF_NUM_MIIM	(3 << 24)
> > > > +
> > > > +
> > > > +static const struct resource vsc7512_dev_cpuorg_resource = {
> > > > +	.start	= 0x71000000,
> > > > +	.end	= 0x710002ff,
> > > 
> > > No magic numbers.  Please define these addresses.
> > 
> > I missed these. Thanks.
> > 
> > > 
> > > > +	.name	= "devcpu_org",
> > > > +};
> > > > +
> > > > +#define VSC7512_BYTE_ORDER_LE 0x00000000
> > > > +#define VSC7512_BYTE_ORDER_BE 0x81818181
> > > > +#define VSC7512_BIT_ORDER_MSB 0x00000000
> > > > +#define VSC7512_BIT_ORDER_LSB 0x42424242
> > > > +
> > > > +int ocelot_spi_initialize(struct ocelot_core *core)
> > > > +{
> > > > +	u32 val, check;
> > > > +	int err;
> > > > +
> > > > +#ifdef __LITTLE_ENDIAN
> > > > +	val = VSC7512_BYTE_ORDER_LE;
> > > > +#else
> > > > +	val = VSC7512_BYTE_ORDER_BE;
> > > > +#endif
> > > 
> > > Not a fan of ifdefery in the middle of C files.
> > > 
> > > Please create a macro or define somewhere.
> > 
> > I'll clear this up in comments and move things around. This macro
> > specifically tends to lend itself to this type of ifdef dropping:
> > 
> > https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/__LITTLE_ENDIAN
> 
> I see that the majority of implementations exist in header files as I
> would expect.  With respect to the others, past acceptance and what is
> acceptable in other subsystems has little bearing on what will be
> accepted here and now.
> 
> > The comment I'm adding is:
> >         /*
> >          * The SPI address must be big-endian, but we want the payload to match
> >          * our CPU. These are two bits (0 and 1) but they're repeated such that
> >          * the write from any configuration will be valid. The four
> >          * configurations are:
> >          *
> >          * 0b00: little-endian, MSB first
> >          * |            111111   | 22221111 | 33222222 |
> >          * | 76543210 | 54321098 | 32109876 | 10987654 |
> >          *
> >          * 0b01: big-endian, MSB first
> >          * | 33222222 | 22221111 | 111111   |          |
> >          * | 10987654 | 32109876 | 54321098 | 76543210 |
> >          *
> >          * 0b10: little-endian, LSB first
> >          * |              111111 | 11112222 | 22222233 |
> >          * | 01234567 | 89012345 | 67890123 | 45678901 |
> >          *
> >          * 0b11: big-endian, LSB first
> >          * | 22222233 | 11112222 |   111111 |          |
> >          * | 45678901 | 67890123 | 89012345 | 01234567 |
> >          */
> > 
> > With this info, would you recommend:
> > 1. A file-scope static const at the top of this file
> > 2. A macro assigned to one of those sequences
> > 3. A function to "detect" which architecture we're running
> 
> I do not have a strong opinion.
> 
> Just tuck the #iferry away somewhere in a header file.

Will do

> 
> > > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> > > > +	if (err)
> > > > +		return err;
> > > 
> > > Comment.
> > > 
> > > > +	val = core->spi_padding_bytes;
> > > > +	err = regmap_write(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> > > > +	if (err)
> > > > +		return err;
> > > 
> > > Comment.
> > 
> > Adding:
> > 
> > /*
> >  * Apply the number of padding bytes between a read request and the data
> >  * payload. Some registers have access times of up to 1us, so if the
> >  * first payload bit is shifted out too quickly, the read will fail.
> >  */
> > 
> > > 
> > > > +	/*
> > > > +	 * After we write the interface configuration, read it back here. This
> > > > +	 * will verify several different things. The first is that the number of
> > > > +	 * padding bytes actually got written correctly. These are found in bits
> > > > +	 * 0:3.
> > > > +	 *
> > > > +	 * The second is that bit 16 is cleared. Bit 16 is IF_CFGSTAT:IF_STAT,
> > > > +	 * and will be set if the register access is too fast. This would be in
> > > > +	 * the condition that the number of padding bytes is insufficient for
> > > > +	 * the SPI bus frequency.
> > > > +	 *
> > > > +	 * The last check is for bits 31:24, which define the interface by which
> > > > +	 * the registers are being accessed. Since we're accessing them via the
> > > > +	 * serial interface, it must return IF_NUM_SI.
> > > > +	 */
> > > > +	check = val | CFGSTAT_IF_NUM_SI;
> > > > +
> > > > +	err = regmap_read(core->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	if (check != val)
> > > > +		return -ENODEV;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(ocelot_spi_initialize);
> > > > +
> > > > +/*
> > > > + * The SPI protocol for interfacing with the ocelot chips uses 24 bits, while
> > > > + * the register locations are defined as 32-bit. The least significant two bits
> > > > + * get shifted out, as register accesses must always be word-aligned, leaving
> > > > + * bits 21:0 as the 22-bit address. It must always be big-endian, whereas the
> > > > + * payload can be optimized for bit / byte order to match whatever architecture
> > > > + * the controlling CPU has.
> > > > + */
> > > > +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> > > > +{
> > > > +	return cpu_to_be32((reg & 0xffffff) >> 2);
> > > > +}
> > > > +
> > > > +struct ocelot_spi_regmap_context {
> > > > +	u32 base;
> > > > +	struct ocelot_core *core;
> > > > +};
> > > > +
> > > > +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> > > > +			       unsigned int *val)
> > > > +{
> > > > +	struct ocelot_spi_regmap_context *regmap_context = context;
> > > > +	struct ocelot_core *core = regmap_context->core;
> > > > +	struct spi_transfer tx, padding, rx;
> > > > +	struct spi_message msg;
> > > 
> > > How big are all of these?
> > > 
> > > We will receive warnings if they occupy too much stack space.
> > 
> > Looking at the structs they're on the order of 10s of bytes. Maybe 70
> > bytes per instance (back of napkin calculation)
> > 
> > But it seems very common to stack-allocate spi_transfers:
> > 
> > https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/spi/spi.c#L4097
> > https://elixir.bootlin.com/linux/v5.18-rc2/source/include/linux/spi/spi.h#L1244
> > 
> > Do you have a feel for at what point that becomes a concern?
> 
> That's fine.  I just want you to bear this in mind.
> 
> I wish to prevent adding yet more W=1 level warnings into the kernel.
> 
> > > > +	struct spi_device *spi;
> > > > +	unsigned int addr;
> > > > +	u8 *tx_buf;
> > > > +
> > > > +	spi = core->spi;
> > > > +
> > > > +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> > > > +	tx_buf = (u8 *)&addr;
> > > > +
> > > > +	spi_message_init(&msg);
> > > > +
> > > > +	memset(&tx, 0, sizeof(tx));
> > > > +
> > > > +	/* Ignore the first byte for the 24-bit address */
> > > > +	tx.tx_buf = &tx_buf[1];
> > > > +	tx.len = 3;
> > > > +
> > > > +	spi_message_add_tail(&tx, &msg);
> > > > +
> > > > +	if (core->spi_padding_bytes > 0) {
> > > > +		u8 dummy_buf[16] = {0};
> > > > +
> > > > +		memset(&padding, 0, sizeof(padding));
> > > > +
> > > > +		/* Just toggle the clock for padding bytes */
> > > > +		padding.len = core->spi_padding_bytes;
> > > > +		padding.tx_buf = dummy_buf;
> > > > +		padding.dummy_data = 1;
> > > > +
> > > > +		spi_message_add_tail(&padding, &msg);
> > > > +	}
> > > > +
> > > > +	memset(&rx, 0, sizeof(rx));
> > > > +	rx.rx_buf = val;
> > > > +	rx.len = 4;
> > > > +
> > > > +	spi_message_add_tail(&rx, &msg);
> > > > +
> > > > +	return spi_sync(spi, &msg);
> > > > +}
> > > > +
> > > > +static int ocelot_spi_reg_write(void *context, unsigned int reg,
> > > > +				unsigned int val)
> > > > +{
> > > > +	struct ocelot_spi_regmap_context *regmap_context = context;
> > > > +	struct ocelot_core *core = regmap_context->core;
> > > > +	struct spi_transfer tx[2] = {0};
> > > > +	struct spi_message msg;
> > > > +	struct spi_device *spi;
> > > > +	unsigned int addr;
> > > > +	u8 *tx_buf;
> > > > +
> > > > +	spi = core->spi;
> > > > +
> > > > +	addr = ocelot_spi_translate_address(reg + regmap_context->base);
> > > > +	tx_buf = (u8 *)&addr;
> > > > +
> > > > +	spi_message_init(&msg);
> > > > +
> > > > +	/* Ignore the first byte for the 24-bit address and set the write bit */
> > > > +	tx_buf[1] |= BIT(7);
> > > > +	tx[0].tx_buf = &tx_buf[1];
> > > > +	tx[0].len = 3;
> > > > +
> > > > +	spi_message_add_tail(&tx[0], &msg);
> > > > +
> > > > +	memset(&tx[1], 0, sizeof(struct spi_transfer));
> > > > +	tx[1].tx_buf = &val;
> > > > +	tx[1].len = 4;
> > > > +
> > > > +	spi_message_add_tail(&tx[1], &msg);
> > > > +
> > > > +	return spi_sync(spi, &msg);
> > > > +}
> > > > +
> > > > +static const struct regmap_config ocelot_spi_regmap_config = {
> > > > +	.reg_bits = 24,
> > > > +	.reg_stride = 4,
> > > > +	.val_bits = 32,
> > > > +
> > > > +	.reg_read = ocelot_spi_reg_read,
> > > > +	.reg_write = ocelot_spi_reg_write,
> > > > +
> > > > +	.max_register = 0xffffffff,
> > > > +	.use_single_write = true,
> > > > +	.use_single_read = true,
> > > > +	.can_multi_write = false,
> > > > +
> > > > +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> > > > +	.val_format_endian = REGMAP_ENDIAN_NATIVE,
> > > > +};
> > > > +
> > > > +struct regmap *
> > > > +ocelot_spi_devm_get_regmap(struct ocelot_core *core, struct device *child,
> > > > +			   const struct resource *res)
> > > 
> > > This seems to always initialise a new Regmap.
> > > 
> > > To me 'get' implies that it could fetch an already existing one.
> > > 
> > > ... and *perhaps* init a new one if none exists..
> > 
> > That's exactly what my intention was when I started.
> > 
> > But it seems like *if* that is something that is required, it should be
> > done through a syscon / device tree implementation and not be snuck into
> > this regmap getter. I was trying to do too much.
> > 
> > I'm renaming to "init"
> > 
> > > 
> > > > +{
> > > > +	struct ocelot_spi_regmap_context *context;
> > > > +	struct regmap_config regmap_config;
> > > > +
> > > > +	context = devm_kzalloc(child, sizeof(*context), GFP_KERNEL);
> > > > +	if (IS_ERR(context))
> > > > +		return ERR_CAST(context);
> > > > +
> > > > +	context->base = res->start;
> > > > +	context->core = core;
> > > > +
> > > > +	memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > > > +	       sizeof(ocelot_spi_regmap_config));
> > > > +
> > > > +	regmap_config.name = res->name;
> > > > +	regmap_config.max_register = res->end - res->start;
> > > > +
> > > > +	return devm_regmap_init(child, NULL, context, &regmap_config);
> > > > +}
> > > > +
> > > > +static int ocelot_spi_probe(struct spi_device *spi)
> > > > +{
> > > > +	struct device *dev = &spi->dev;
> > > > +	struct ocelot_core *core;
> > > 
> > > This would be more in keeping with current drivers if you dropped the
> > > '_core' part of the struct name and called the variable ddata.
> > 
> > There's already a "struct ocelot" defined in include/soc/mscc/ocelot.h.
> > I suppose it could be renamed to align with what it actually is: the
> > "switch" component of the ocelot chip.
> > 
> > Vladimir, Alexandre, Horaitu, others:
> > Any opinions about this becoming "struct ocelot" and the current struct
> > being "struct ocelot_switch"?
> > 
> > Or maybe a technical / philosophical question: is "ocelot" the switch
> > core that can be implemented in other hardware? Or is it the chip family
> > entirely, (pinctrl, sgpio, etc.) who's switch core was brought into
> > other products?
> > 
> > The existing struct change would hit about 30 files.
> > https://elixir.bootlin.com/linux/v5.18-rc2/C/ident/ocelot
> 
> That's not ideal.
> 
> Please consider using 'ocelot_ddata' for now and consider a larger
> overhaul at a later date, if it makes sense to do so.
> 
> [...]
> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog