Message-ID: <20220420021318.GA1220@COLIN-DESKTOP1.localdomain>
Date:   Tue, 19 Apr 2022 19:13:18 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
        linux-phy@...ts.infradead.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Marc Zyngier <maz@...nel.org>,
        Hector Martin <marcan@...can.st>,
        Angela Czubak <acz@...ihalf.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>, UNGLinuxDriver@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        katie.morris@...advantage.com
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 <colin.foster@...advantage.com>");
> > > > +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 <colin.foster@...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)
> > > > +
> > > > +
> > > > +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

Powered by blists - more mailing lists