by Colin Foster

[permalink] [raw]

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

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 <[email protected]>");
> > > > +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 <[email protected]>
> > > > + */
> > > > +
> > > > +#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