Re: [RFC v5 net-next 01/13] mfd: ocelot: add support for external mfd control over SPI for the VSC7512
From: Lee Jones
Date: Wed Dec 29 2021 - 10:23:09 EST
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.
We're going to need Mark Brown to have a look at this Regmap implementation.
> Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> ---
> 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.
> 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.
> + 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.
> +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?
> + */
> +
> +#include <asm/byteorder.h>
These, if required, usually go at the bottom.
> +#include <linux/spi/spi.h>
> +#include <linux/kconfig.h>
What's this for?
> +#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?
> +enum ocelot_mfd_gcb_regs {
Please remove the term 'mfd\|MFD' from everywhere.
> + 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?
> +static const struct resource vsc7512_gcb_resource = {
> + .start = 0x71070000,
> + .end = 0x7107022b,
No magic numbers please.
> + .name = "devcpu_gcb",
What is a 'devcpu_gcb'?
> +};
> +
> +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.
> + 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.
> + 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.
> + 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.
> +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.
> + 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.
> + return 0;
> +}
> +EXPORT_SYMBOL(ocelot_mfd_remove);
> +
> +MODULE_DESCRIPTION("Ocelot Chip MFD driver");
> +MODULE_AUTHOR("Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>");
> +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.
> +};
> +
> +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.
> +#include "ocelot-mfd.h"
> +
> +#define REG(reg, offset) [reg] = offset
> +
> +struct ocelot_spi {
> + int spi_padding_bytes;
> + struct spi_device *spi;
> + struct ocelot_mfd_config config;
> + struct regmap *cpuorg_regmap;
> + const u32 *map;
> +};
> +
> +enum ocelot_dev_cpuorg_regs {
> + DEV_CPUORG_IF_CTRL,
> + DEV_CPUORG_IF_CFGSTAT,
> + DEV_CPUORG_REG_MAX,
> +};
> +
> +static const u32 vsc7512_dev_cpuorg_regmap[] = {
> + REG(DEV_CPUORG_IF_CTRL, 0x0000),
> + REG(DEV_CPUORG_IF_CFGSTAT, 0x0004),
> +};
> +
> +static const struct resource vsc7512_dev_cpuorg_resource = {
> + .start = 0x71000000,
> + .end = 0x710002ff,
> + .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
> +
> +static struct ocelot_spi *
> +config_to_ocelot_spi(struct ocelot_mfd_config *config)
> +{
> + return container_of(config, struct ocelot_spi, config);
> +}
> +
> +static int ocelot_spi_init_bus(struct ocelot_spi *ocelot_spi)
> +{
> + struct spi_device *spi;
> + struct device *dev;
> + u32 val, check;
> + int err;
> +
> + spi = ocelot_spi->spi;
> + dev = &spi->dev;
> +
> + dev_info(dev, "initializing SPI interface for chip\n");
> +
> + val = 0;
> +
> +#ifdef __LITTLE_ENDIAN
> + val |= VSC7512_BYTE_ORDER_LE;
> +#else
> + val |= VSC7512_BYTE_ORDER_BE;
> +#endif
> +
> + err = regmap_write(ocelot_spi->cpuorg_regmap,
> + ocelot_spi->map[DEV_CPUORG_IF_CTRL], val);
> + if (err)
> + return err;
> +
> + val = ocelot_spi->spi_padding_bytes;
> + err = regmap_write(ocelot_spi->cpuorg_regmap,
> + ocelot_spi->map[DEV_CPUORG_IF_CFGSTAT], val);
> + if (err)
> + return err;
> +
> + check = val | 0x02000000;
> +
> + err = regmap_read(ocelot_spi->cpuorg_regmap,
> + ocelot_spi->map[DEV_CPUORG_IF_CFGSTAT], &val);
> + if (err)
> + return err;
> +
> + if (check != val) {
> + dev_err(dev, "Error configuring SPI bus. V: 0x%08x != 0x%08x\n",
> + val, check);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int ocelot_spi_init_bus_from_config(struct ocelot_mfd_config *config)
> +{
> + struct ocelot_spi *ocelot_spi = config_to_ocelot_spi(config);
> +
> + return ocelot_spi_init_bus(ocelot_spi);
> +}
> +
> +static unsigned int ocelot_spi_translate_address(unsigned int reg)
> +{
> + return cpu_to_be32((reg & 0xffffff) >> 2);
> +}
> +
> +struct ocelot_spi_regmap_context {
> + struct spi_device *spi;
> + u32 base;
> + int padding_bytes;
> +};
> +
> +static int ocelot_spi_reg_read(void *context, unsigned int reg,
> + unsigned int *val)
> +{
> + struct ocelot_spi_regmap_context *regmap_context = context;
> + struct spi_transfer tx, padding, rx;
> + struct ocelot_spi *ocelot_spi;
> + struct spi_message msg;
> + struct spi_device *spi;
> + unsigned int addr;
> + u8 *tx_buf;
> +
> + WARN_ON(!val);
> +
> + spi = regmap_context->spi;
> +
> + ocelot_spi = spi_get_drvdata(spi);
> +
> + addr = ocelot_spi_translate_address(reg + regmap_context->base);
> + tx_buf = (u8 *)&addr;
> +
> + spi_message_init(&msg);
> +
> + memset(&tx, 0, sizeof(struct spi_transfer));
> +
> + /* 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 (regmap_context->padding_bytes > 0) {
> + u8 dummy_buf[16] = {0};
> +
> + memset(&padding, 0, sizeof(struct spi_transfer));
> +
> + /* Just toggle the clock for padding bytes */
> + padding.len = regmap_context->padding_bytes;
> + padding.tx_buf = dummy_buf;
> + padding.dummy_data = 1;
> +
> + spi_message_add_tail(&padding, &msg);
> + }
> +
> + memset(&rx, 0, sizeof(struct spi_transfer));
> + 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 spi_transfer tx[2] = {0};
> + struct spi_message msg;
> + struct spi_device *spi;
> + unsigned int addr;
> + u8 *tx_buf;
> +
> + spi = regmap_context->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,
> +};
> +
> +static struct regmap *
> +ocelot_spi_get_regmap(struct ocelot_mfd_config *config,
> + const struct resource *res, const char *name)
> +{
> + struct ocelot_spi *ocelot_spi = config_to_ocelot_spi(config);
> + struct ocelot_spi_regmap_context *context;
> + struct regmap_config regmap_config;
> + struct regmap *regmap;
> + struct device *dev;
> +
> +
> + dev = &ocelot_spi->spi->dev;
> +
> + /* Don't re-allocate another regmap if we have one */
> + regmap = dev_get_regmap(dev, name);
> + if (regmap)
> + return regmap;
> +
> + context = devm_kzalloc(dev, sizeof(struct ocelot_spi_regmap_context),
> + GFP_KERNEL);
> +
> + if (IS_ERR(context))
> + return ERR_CAST(context);
> +
> + context->base = res->start;
> + context->spi = ocelot_spi->spi;
> + context->padding_bytes = ocelot_spi->spi_padding_bytes;
> +
> + memcpy(®map_config, &ocelot_spi_regmap_config,
> + sizeof(ocelot_spi_regmap_config));
> +
> + regmap_config.name = name;
> + regmap_config.max_register = res->end - res->start;
> +
> + regmap = devm_regmap_init(dev, NULL, context, ®map_config);
> + if (IS_ERR(regmap))
> + return ERR_CAST(regmap);
> +
> + return regmap;
> +}
> +
> +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'.
> + 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).
> + }
> +
> + ocelot_spi->spi = spi;
Why are you saving this?
> + ocelot_spi->map = vsc7512_dev_cpuorg_regmap;
Why not just set up the regmap here?
> + 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?
> + /* Ensure we have devcpu_org regmap before we call ocelot_mfd_init */
because ...
> + ocelot_mfd_get_resource_name(name, &vsc7512_dev_cpuorg_resource,
> + sizeof(name) - 1);
This is an ugly interface. I think it needs to go.
> + /*
> + * Since we created dev, we know there isn't a regmap, so create one
> + * here directly.
> + */
Sorry, what 'dev'? When did we create that?
> + 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.
> + 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?
> + 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.
> + 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?
> + 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@xxxxxxxxxxxxxxxx>");
> +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