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