Re: [PATCH v13 net-next 9/9] mfd: ocelot: add support for the vsc7512 chip via spi

Colin Foster
2022-07-19T17:49:28Z

On Mon, Jul 18, 2022 at 03:18:28PM +0100, Lee Jones wrote:
> On Tue, 05 Jul 2022, Colin Foster wrote:
>
> > +MODULE_IMPORT_NS(MFD_OCELOT_SPI);
> > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
> > new file mode 100644
> > index 000000000000..0c1c5215c706
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot-spi.c
> > @@ -0,0 +1,317 @@
> > +// 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, 2022 Innovative Advantage Inc.
> > + *
> > + * Author: Colin Foster <colin.foster@in-advantage.com>
> > + */
> > +
> > +#include <linux/ioport.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <asm/byteorder.h>
> > +
> > +#include "ocelot.h"
> > +
> > +#define REG_DEV_CPUORG_IF_CTRL 0x0000
> > +#define REG_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_DEVCPU_ORG_RES_START 0x71000000
> > +#define VSC7512_DEVCPU_ORG_RES_SIZE 0x38
> > +
> > +#define VSC7512_CHIP_REGS_RES_START 0x71070000
> > +#define VSC7512_CHIP_REGS_RES_SIZE 0x14
> > +
> > +struct spi_device;
>
> Why not just #include?

I mis-understood this to mean drivers/mfd/ocelot-spi.c when it meant
drivers/mfd/ocelot.h. Thanks.

https://patchwork.kernel.org/project/netdevbpf/patch/20220701192609.3970317-10-colin.foster@in-advantage.com/#24921057

"""
You missed a lot of forward declarations that are used in this file.

Like

struct spi_device;
"""

> > +static int ocelot_spi_regmap_bus_read(void *context,
> > + const void *reg, size_t reg_size,
> > + void *val, size_t val_size)
> > +{
> > + struct ocelot_ddata *ddata = context;
> > + struct spi_transfer tx, padding, rx;
> > + struct spi_device *spi = ddata->spi;
> > + struct spi_message msg;
> > +
> > + spi = ddata->spi;
>
> Drop this line.

Yes - and actually since I'm removing ddata->spi altogether it'll become
to_spi_device(ddata->dev) (obviously without the double-assignment that
you're pointing out here)

>
> > + 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) {
> > + memset(&padding, 0, sizeof(padding));
> > +
> > + padding.len = ddata->spi_padding_bytes;
> > + padding.tx_buf = ddata->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;

As above, I'm changing to to_spi_device(ddata->dev)

> > +
> > + 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_init_regmap(struct device *dev, const struct resource *res)
>
> One line, along with all the others.
>
> > +{
> > + struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> > + struct regmap_config regmap_config;
> > +
> > + memcpy(&regmap_config, &ocelot_spi_regmap_config,
> > + sizeof(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(dev, &ocelot_spi_regmap_bus, ddata,
> > + &regmap_config);
> > +}
> > +EXPORT_SYMBOL_NS(ocelot_spi_init_regmap, MFD_OCELOT_SPI);
> > +
> > +static int ocelot_spi_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct ocelot_ddata *ddata;
> > + struct regmap *r;
> > + int err;
> > +
> > + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > + if (!ddata)
> > + return -ENOMEM;
> > +
> > + ddata->dev = dev;
>
> How are you fetching ddata if you don't already have 'dev'?

I don't think I fully understand this question...

Are you saying ddata doesn't need a dev instance? So instead of:

devm_regmap_init(dev, &bus, ddata, &regmap_config);

It could be:

devm_regmap_init(dev, &bus, dev, &regmap_config);

In that case, the context into ocelot_spi_regmap_bus_{read,write} would
be dev, instead of ddata.

Then I get ddata from device via:

static int ocelot_spi_regmap_bus_write(void *context,...)
{
struct device *dev = context;
struct ocelot_ddata *ddata = dev_get_drvdata(dev);
struct spi_device *spi = to_spi_device(dev);

/* ddata isn't actually needed for bus_write, just making a point */
...
}

I haven't tested this yet, but I think this is what you're suggesting.
So now I've removed both spi and dev from the ddata struct (as I mention
below). Cool.

>
> > + dev_set_drvdata(dev, ddata);
>
> This should use the spi_* variant.

Agreed.

>
> > + 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->dummy_buf = devm_kzalloc(dev, ddata->spi_padding_bytes,
> > + GFP_KERNEL);
> > + if (!ddata->dummy_buf)
> > + return -ENOMEM;
> > + }
> > +
> > + ddata->spi = spi;
>
> If you have 'spi' you definitely do not need 'dev'.
>
> You can derive one from the other.

Good point. As I implied above, I'm dropping "spi" from the ddata struct
and will recover spi from to_spi_device(dev)

That does some nice things like removes "struct spi_device" from
drivers/mfd/ocelot.h

> > + spi->bits_per_word = 8;
> > +
> > + err = spi_setup(spi);
> > + if (err < 0)
> > + return dev_err_probe(&spi->dev, err,
> > + "Error performing SPI setup\n");
> > +
> > + r = ocelot_spi_init_regmap(dev, &vsc7512_dev_cpuorg_resource);
> > + if (IS_ERR(r))
> > + return PTR_ERR(r);
> > +
> > + ddata->cpuorg_regmap = r;
> > +
> > + r = ocelot_spi_init_regmap(dev, &vsc7512_gcb_resource);
> > + if (IS_ERR(r))
> > + return PTR_ERR(r);
> > +
> > + ddata->gcb_regmap = r;
> > +
> > + /*
> > + * 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)
> > + return dev_err_probe(dev, err, "Error initializing SPI bus\n");
> > +
> > + err = ocelot_chip_reset(dev);
> > + if (err)
> > + return dev_err_probe(dev, err, "Error resetting device\n");
> > +
> > + /*
> > + * 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)
> > + return dev_err_probe(dev, err,
> > + "Error initializing SPI bus after reset\n");
> > +
> > + err = ocelot_core_init(dev);
> > + if (err < 0)
> > + return dev_err_probe(dev, err,
> > + "Error initializing Ocelot core\n");
> > +
> > + return 0;
> > +}
> > +
> > +static const struct spi_device_id ocelot_spi_ids[] = {
> > + { "vsc7512", 0 },
> > + { }
> > +};
> > +
> > +static const struct of_device_id ocelot_spi_of_match[] = {
> > + { .compatible = "mscc,vsc7512" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ocelot_spi_of_match);
> > +
> > +static struct spi_driver ocelot_spi_driver = {
> > + .driver = {
> > + .name = "ocelot-soc",
> > + .of_match_table = ocelot_spi_of_match,
> > + },
> > + .id_table = ocelot_spi_ids,
> > + .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");
> > +MODULE_IMPORT_NS(MFD_OCELOT);
> > diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
> > new file mode 100644
> > index 000000000000..c86bd6990a3c
> > --- /dev/null
> > +++ b/drivers/mfd/ocelot.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/* Copyright 2021, 2022 Innovative Advantage Inc. */
> > +
> > +#include <asm/byteorder.h>
> > +
> > +struct device;
> > +struct spi_device;
> > +struct regmap;
> > +struct resource;
> > +
> > +struct ocelot_ddata {
> > + struct device *dev;
> > + struct regmap *gcb_regmap;
> > + struct regmap *cpuorg_regmap;
> > + int spi_padding_bytes;
> > + struct spi_device *spi;
> > + void *dummy_buf;
> > +};
>
> This looks like it deserves a doc header.

Will do!

>
> > +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_init_regmap(struct device *dev,
> > + const struct resource *res);
> > +
> > +#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
>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog