Re: [RFC v8 net-next 08/16] mfd: ocelot: add support for the vsc7512 chip via spi

Vladimir Oltean
2022-05-09T10:52:55Z

On Sun, May 08, 2022 at 11:53:05AM -0700, 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, and serial GPIO.
>
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> drivers/mfd/Kconfig | 18 +++
> drivers/mfd/Makefile | 2 +
> drivers/mfd/ocelot-core.c | 135 +++++++++++++++++
> drivers/mfd/ocelot-spi.c | 311 ++++++++++++++++++++++++++++++++++++++
> drivers/mfd/ocelot.h | 34 +++++
> include/soc/mscc/ocelot.h | 5 +
> 6 files changed, 505 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 3b59456f5545..ff177173ca11 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -962,6 +962,24 @@ 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"
> + depends on SPI_MASTER
> + select MFD_CORE
> + select REGMAP_SPI
> + 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 EZX_PCAP
> bool "Motorola EZXPCAP Support"
> depends on SPI_MASTER
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..bc517632ba5f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -120,6 +120,8 @@ obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o
>
> obj-$(CONFIG_MFD_CORE) += mfd-core.o
>
> +obj-$(CONFIG_MFD_OCELOT) += ocelot-core.o 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..117028f7d845
> --- /dev/null
> +++ b/drivers/mfd/ocelot-core.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Core driver for the Ocelot chip family.
> + *
> + * 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 child devices.
> + *
> + * Copyright 2021, 2022 Innovative Advantage Inc.
> + *
> + * 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
> +
> +#define SOFT_CHIP_RST 0x1
> +
> +#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_GPIO_RES_START 0x71070034
> +#define VSC7512_GPIO_RES_SIZE 0x6c
> +
> +#define VSC7512_SIO_RES_START 0x710700f8
> +#define VSC7512_SIO_RES_SIZE 0x100
> +
> +int ocelot_chip_reset(struct device *dev)
> +{
> + struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> + 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(ddata->gcb_regmap, GCB_SOFT_RST, SOFT_CHIP_RST);
> + if (ret)
> + return ret;
> +
> + msleep(100);

Isn't this a bit too long?

> +
> + return ret;

return 0

> +}
> +EXPORT_SYMBOL(ocelot_chip_reset);
> +
> +struct regmap *ocelot_init_regmap_from_resource(struct device *child,
> + const struct resource *res)
> +{
> + struct device *dev = child->parent;
> +
> + return ocelot_spi_devm_init_regmap(dev, child, res);

So much for being bus-agnostic :-/
Maybe get the struct ocelot_ddata and call ocelot_spi_devm_init_regmap()
via a function pointer which is populated by ocelot-spi.c? If you do
that don't forget to clean up drivers/mfd/ocelot.h of SPI specific stuff.

> +}
> +EXPORT_SYMBOL(ocelot_init_regmap_from_resource);
> +
> +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_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,
> + },
> +};
> +
> +int ocelot_core_init(struct device *dev)
> +{
> + int 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");
> diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> new file mode 100644
> index 000000000000..95754deb0b57
> --- /dev/null
> +++ b/drivers/mfd/ocelot-spi.c
> @@ -0,0 +1,311 @@
> +// 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
> + * 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)
> +
> +#define VSC7512_CPUORG_RES_START 0x71000000
> +#define VSC7512_CPUORG_RES_SIZE 0x2ff
> +
> +#define VSC7512_GCB_RES_START 0x71070000
> +#define VSC7512_GCB_RES_SIZE 0x14
> +
> +static const struct resource vsc7512_dev_cpuorg_resource =
> + DEFINE_RES_REG_NAMED(VSC7512_CPUORG_RES_START, VSC7512_CPUORG_RES_SIZE,
> + "devcpu_org");
> +
> +static const struct resource vsc7512_gcb_resource =
> + DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE,
> + "devcpu_gcb_chip_regs");
> +
> +int ocelot_spi_initialize(struct device *dev)

Should be static and unexported.

> +{
> + struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> + u32 val, check;
> + int err;
> +
> + val = OCELOT_SPI_BYTE_ORDER;
> +
> + /*
> + * 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 |
> + */
> + err = regmap_write(ddata->cpuorg_regmap, DEV_CPUORG_IF_CTRL, val);
> + if (err)
> + return err;
> +
> + /*
> + * 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.
> + */
> + val = ddata->spi_padding_bytes;
> + err = regmap_write(ddata->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, val);
> + if (err)
> + return err;
> +
> + /*
> + * 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(ddata->cpuorg_regmap, DEV_CPUORG_IF_CFGSTAT, &val);
> + if (err)
> + return err;
> +
> + if (check != val)
> + return -ENODEV;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(ocelot_spi_initialize);
> +
> +static const struct regmap_config ocelot_spi_regmap_config = {
> + .reg_bits = 24,
> + .reg_stride = 4,
> + .reg_downshift = 2,
> + .val_bits = 32,
> +
> + .write_flag_mask = 0x80,
> +
> + .max_register = 0xffffffff,
> + .use_single_write = true,
> + .can_multi_write = false,
> +
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .val_format_endian = REGMAP_ENDIAN_NATIVE,
> +};
> +
> +static int ocelot_spi_regmap_bus_read(void *context,
> + const void *reg, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + static const u8 dummy_buf[16] = {0};
> + struct spi_transfer tx, padding, rx;
> + struct ocelot_ddata *ddata = context;
> + struct spi_device *spi = ddata->spi;
> + struct spi_message msg;
> +
> + spi = ddata->spi;
> +
> + spi_message_init(&msg);
> +
> + memset(&tx, 0, sizeof(tx));
> +
> + tx.tx_buf = reg;
> + tx.len = reg_size;
> +
> + spi_message_add_tail(&tx, &msg);
> +
> + if (ddata->spi_padding_bytes > 0) {
> + memset(&padding, 0, sizeof(padding));
> +
> + padding.len = ddata->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 = val_size;
> +
> + spi_message_add_tail(&rx, &msg);
> +
> + return spi_sync(spi, &msg);
> +}
> +
> +static int ocelot_spi_regmap_bus_write(void *context, const void *data,
> + size_t count)
> +{
> + struct ocelot_ddata *ddata = context;
> + struct spi_device *spi = ddata->spi;
> +
> + return spi_write(spi, data, count);
> +}
> +
> +static const struct regmap_bus ocelot_spi_regmap_bus = {
> + .write = ocelot_spi_regmap_bus_write,
> + .read = ocelot_spi_regmap_bus_read,
> +};
> +
> +struct regmap *
> +ocelot_spi_devm_init_regmap(struct device *dev, struct device *child,
> + const struct resource *res)
> +{
> + struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> + struct regmap_config regmap_config;
> +
> + 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;
> + regmap_config.reg_base = res->start;
> +
> + return devm_regmap_init(child, &ocelot_spi_regmap_bus, ddata,
> + &regmap_config);
> +}
> +
> +static int ocelot_spi_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct ocelot_ddata *ddata;
> + int err;
> +
> + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata)
> + return -ENOMEM;
> +
> + ddata->dev = dev;
> + dev_set_drvdata(dev, ddata);
> +
> + if (spi->max_speed_hz <= 500000) {
> + ddata->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.
> + */
> + ddata->spi_padding_bytes = 1 +
> + (spi->max_speed_hz / 1000000 + 2) / 8;
> + }
> +
> + ddata->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;
> + }
> +

Personally I'd prefer:

struct regmap *r;

r = ocelot_spi_devm_init_regmap(dev, dev,
&vsc7512_dev_cpuorg_resource);
if (IS_ERR(r))
return ERR_PTR(r);

ddata->cpuorg_regmap = r;

and so on.

> + ddata->cpuorg_regmap =
> + ocelot_spi_devm_init_regmap(dev, dev,
> + &vsc7512_dev_cpuorg_resource);
> + if (IS_ERR(ddata->cpuorg_regmap))
> + return -ENOMEM;
> +
> + ddata->gcb_regmap = ocelot_spi_devm_init_regmap(dev, dev,
> + &vsc7512_gcb_resource);
> + if (IS_ERR(ddata->gcb_regmap))
> + return -ENOMEM;
> +
> + /*
> + * 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(dev);
> + if (err) {
> + dev_err(dev, "Error %d initializing Ocelot SPI bus\n", err);

Maybe showing the symbolic value behind the error number would be
helpful?

dev_err(dev, "Initializing SPI bus returned %pe\n", ERR_PTR(err));

Similar for other places where you print the error using %d, I won't
repeat this comment.

> + return err;
> + }
> +
> + err = ocelot_chip_reset(dev);
> + if (err) {
> + dev_err(dev, "Failed to reset device: %d\n", err);
> + return err;
> + }
> +
> + /*
> + * A chip reset will clear the SPI configuration, so it needs to be done
> + * again before we can access any registers
> + */
> + err = ocelot_spi_initialize(dev);
> + if (err) {
> + dev_err(dev,
> + "Failed to initialize Ocelot SPI bus after reset: %d\n",
> + err);
> + return err;
> + }
> +
> + err = ocelot_core_init(dev);
> + if (err < 0) {
> + dev_err(dev, "Error %d initializing Ocelot core\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +const struct of_device_id ocelot_spi_of_match[] = {

static

> + { .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..b68e6343caca
> --- /dev/null
> +++ b/drivers/mfd/ocelot.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright 2021 Innovative Advantage Inc.
> + */
> +
> +#include <linux/regmap.h>
> +
> +#include <asm/byteorder.h>
> +
> +struct ocelot_ddata {
> + struct device *dev;
> + struct regmap *gcb_regmap;
> + struct regmap *cpuorg_regmap;
> + int spi_padding_bytes;
> + struct spi_device *spi;
> +};
> +
> +int ocelot_chip_reset(struct device *dev);
> +int ocelot_core_init(struct device *dev);
> +
> +/* SPI-specific routines that won't be necessary for other interfaces */
> +struct regmap *ocelot_spi_devm_init_regmap(struct device *dev,
> + struct device *child,
> + const struct resource *res);
> +int ocelot_spi_initialize(struct device *dev);
> +
> +#define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
> +#define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
> +
> +#ifdef __LITTLE_ENDIAN
> +#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_LE
> +#else
> +#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_BE
> +#endif
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 1897119ebb9a..f9124a66e386 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -1039,11 +1039,16 @@ ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
> }
> #endif
>
> +#if IS_ENABLED(CONFIG_MFD_OCELOT)
> +struct regmap *ocelot_init_regmap_from_resource(struct device *child,
> + const struct resource *res);
> +#else
> static inline struct regmap *
> ocelot_init_regmap_from_resource(struct device *child,
> const struct resource *res)
> {
> return ERR_PTR(-EOPNOTSUPP);
> }
> +#endif
>
> #endif
> --
> 2.25.1
>