Message-ID: <20220912170808.y4l4u2el7dozpx4j@skbuf>
Date:   Mon, 12 Sep 2022 17:08:09 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Russell King <linux@...linux.org.uk>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "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" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Lee Jones <lee@...nel.org>
Subject: Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

On Sun, Sep 11, 2022 at 01:02:43PM -0700, Colin Foster wrote:
> The Ocelot switch core driver relies heavily on a fixed array of resources
> for both ports and peripherals. This is in contrast to existing peripherals
> - pinctrl for example - which have a one-to-one mapping of driver <>
> resource. As such, these regmaps must be created differently so that
> enumeration-based offsets are preserved.
> 
> Register the regmaps to the core MFD device unconditionally so they can be
> referenced by the Ocelot switch / Felix DSA systems.
> 
> Signed-off-by: Colin Foster <colin.foster@...advantage.com>
> ---
> 
> v1 from previous RFC:
>     * New patch
> 
> ---
>  drivers/mfd/ocelot-core.c  | 88 +++++++++++++++++++++++++++++++++++---
>  include/linux/mfd/ocelot.h |  5 +++
>  2 files changed, 88 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 1816d52c65c5..aa7fa21b354c 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -34,16 +34,55 @@
>  
>  #define VSC7512_MIIM0_RES_START		0x7107009c
>  #define VSC7512_MIIM1_RES_START		0x710700c0
> -#define VSC7512_MIIM_RES_SIZE		0x024
> +#define VSC7512_MIIM_RES_SIZE		0x00000024
>  
>  #define VSC7512_PHY_RES_START		0x710700f0
> -#define VSC7512_PHY_RES_SIZE		0x004
> +#define VSC7512_PHY_RES_SIZE		0x00000004
>  
>  #define VSC7512_GPIO_RES_START		0x71070034
> -#define VSC7512_GPIO_RES_SIZE		0x06c
> +#define VSC7512_GPIO_RES_SIZE		0x0000006c
>  
>  #define VSC7512_SIO_CTRL_RES_START	0x710700f8
> -#define VSC7512_SIO_CTRL_RES_SIZE	0x100
> +#define VSC7512_SIO_CTRL_RES_SIZE	0x00000100

Split the gratuitous changes to _RES_SIZE to a separate patch please, as
they're just noise here?

> +
> +#define VSC7512_HSIO_RES_START		0x710d0000
> +#define VSC7512_HSIO_RES_SIZE		0x00000128
> +
> +#define VSC7512_ANA_RES_START		0x71880000
> +#define VSC7512_ANA_RES_SIZE		0x00010000
> +
> +#define VSC7512_QS_RES_START		0x71080000
> +#define VSC7512_QS_RES_SIZE		0x00000100
> +
> +#define VSC7512_QSYS_RES_START		0x71800000
> +#define VSC7512_QSYS_RES_SIZE		0x00200000
> +
> +#define VSC7512_REW_RES_START		0x71030000
> +#define VSC7512_REW_RES_SIZE		0x00010000
> +
> +#define VSC7512_SYS_RES_START		0x71010000
> +#define VSC7512_SYS_RES_SIZE		0x00010000
> +
> +#define VSC7512_S0_RES_START		0x71040000
> +#define VSC7512_S1_RES_START		0x71050000
> +#define VSC7512_S2_RES_START		0x71060000
> +#define VSC7512_S_RES_SIZE		0x00000400
> +
> +#define VSC7512_GCB_RES_START		0x71070000
> +#define VSC7512_GCB_RES_SIZE		0x0000022c
> +
> +#define VSC7512_PORT_0_RES_START	0x711e0000
> +#define VSC7512_PORT_1_RES_START	0x711f0000
> +#define VSC7512_PORT_2_RES_START	0x71200000
> +#define VSC7512_PORT_3_RES_START	0x71210000
> +#define VSC7512_PORT_4_RES_START	0x71220000
> +#define VSC7512_PORT_5_RES_START	0x71230000
> +#define VSC7512_PORT_6_RES_START	0x71240000
> +#define VSC7512_PORT_7_RES_START	0x71250000
> +#define VSC7512_PORT_8_RES_START	0x71260000
> +#define VSC7512_PORT_9_RES_START	0x71270000
> +#define VSC7512_PORT_10_RES_START	0x71280000
> +#define VSC7512_PORT_RES_SIZE		0x00010000
>  
>  #define VSC7512_GCB_RST_SLEEP_US	100
>  #define VSC7512_GCB_RST_TIMEOUT_US	100000
> @@ -96,6 +135,34 @@ static const struct resource vsc7512_sgpio_resources[] = {
>  	DEFINE_RES_REG_NAMED(VSC7512_SIO_CTRL_RES_START, VSC7512_SIO_CTRL_RES_SIZE, "gcb_sio"),
>  };
>  
> +const struct resource vsc7512_target_io_res[TARGET_MAX] = {
> +	[ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"),
> +	[QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"),
> +	[QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"),
> +	[REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"),
> +	[SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"),
> +	[S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"),
> +	[S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"),
> +	[S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"),
> +	[GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"),
> +	[HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"),
> +};

EXPORT_SYMBOL is required, I believe, for when ocelot_ext is built as
module?

> +
> +const struct resource vsc7512_port_io_res[] = {
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_0_RES_START, VSC7512_PORT_RES_SIZE, "port0"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_1_RES_START, VSC7512_PORT_RES_SIZE, "port1"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_2_RES_START, VSC7512_PORT_RES_SIZE, "port2"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_3_RES_START, VSC7512_PORT_RES_SIZE, "port3"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_4_RES_START, VSC7512_PORT_RES_SIZE, "port4"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_5_RES_START, VSC7512_PORT_RES_SIZE, "port5"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_6_RES_START, VSC7512_PORT_RES_SIZE, "port6"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_7_RES_START, VSC7512_PORT_RES_SIZE, "port7"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_8_RES_START, VSC7512_PORT_RES_SIZE, "port8"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_9_RES_START, VSC7512_PORT_RES_SIZE, "port9"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_10_RES_START, VSC7512_PORT_RES_SIZE, "port10"),
> +	{}
> +};

Here too.

> +
>  static const struct mfd_cell vsc7512_devs[] = {
>  	{
>  		.name = "ocelot-pinctrl",
> @@ -127,7 +194,7 @@ static const struct mfd_cell vsc7512_devs[] = {
>  static void ocelot_core_try_add_regmap(struct device *dev,
>  				       const struct resource *res)
>  {
> -	if (dev_get_regmap(dev, res->name))
> +	if (!res->start || dev_get_regmap(dev, res->name))

I didn't understand at first what this extra condition here is for.
I don't think that adding this extra condition here is the clearest
way to deal with the sparsity of the vsc7512_target_io_res[] array, plus
it seems to indicate the masking of a more unclean code design.

I would propose an alternative below, at the caller site....

>  		return;
>  
>  	ocelot_spi_init_regmap(dev, res);
> @@ -144,6 +211,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev,
>  
>  int ocelot_core_init(struct device *dev)
>  {
> +	const struct resource *port_res;
>  	int i, ndevs;
>  
>  	ndevs = ARRAY_SIZE(vsc7512_devs);
> @@ -151,6 +219,16 @@ int ocelot_core_init(struct device *dev)
>  	for (i = 0; i < ndevs; i++)
>  		ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
>  
> +	/*
> +	 * Both the target_io_res and tbe port_io_res structs need to be referenced directly by

s/tbe/the

> +	 * the ocelot_ext driver, so they can't be attached to the dev directly

I don't exactly understand the meaning of "they can't be attached to the
dev *directly*". You mean that the "struct mfd_cell vsc7512_devs[]" entry
for "mscc,vsc7512-ext-switch" will not have a "resources" property, right?
Better to say "using mfd_add_devices()" rather than "directly"?

> +	 */
> +	for (i = 0; i < TARGET_MAX; i++)
> +		ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]);

	/*
	 * vsc7512_target_io_res[] is a sparse array, skip the missing
	 * elements
	 */
	for (i = 0; i < TARGET_MAX; i++) {
		res = &vsc7512_target_io_res[i];
		if (!res->start)
			continue;

		ocelot_core_try_add_regmap(dev, res);
	}

Something interesting that I stumbled upon in Documentation/process/6.Followthrough.rst
was:

| Andrew Morton has suggested that every review comment which does not result
| in a code change should result in an additional code comment instead; that
| can help future reviewers avoid the questions which came up the first time
| around.

so if you don't like my alternative, please at least do add a comment in
ocelot_core_try_add_regmap().

> +
> +	for (port_res = vsc7512_port_io_res; port_res->start; port_res++)
> +		ocelot_core_try_add_regmap(dev, port_res);
> +
>  	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL);
>  }
>  EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
> diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
> index dd72073d2d4f..439ff5256cf0 100644
> --- a/include/linux/mfd/ocelot.h
> +++ b/include/linux/mfd/ocelot.h
> @@ -11,8 +11,13 @@
>  #include <linux/regmap.h>
>  #include <linux/types.h>
>  
> +#include <soc/mscc/ocelot.h>
> +
>  struct resource;
>  
> +extern const struct resource vsc7512_target_io_res[TARGET_MAX];
> +extern const struct resource vsc7512_port_io_res[];
> +
>  static inline struct regmap *
>  ocelot_regmap_from_resource_optional(struct platform_device *pdev,
>  				     unsigned int index,
> -- 
> 2.25.1
>

Actually I don't like this mechanism too much, if at all. I have 4 mutt
windows open right now, plus the previous mfd patch at:
https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=ib-mfd-net-pinctrl-6.0&id=f3e893626abeac3cdd9ba41d3395dc6c1b7d5ad6
to follow what is going on. So I'll copy some code from other places
here, to concentrate the discussion in a single place:

>From patch 8/8:

> +static struct regmap *ocelot_ext_regmap_init(struct ocelot *ocelot,
> +					     struct resource *res)
> +{
> +	return dev_get_regmap(ocelot->dev->parent, res->name);
> +}

> +static const struct felix_info vsc7512_info = {
> +	.target_io_res			= vsc7512_target_io_res, // exported by drivers/mfd/ocelot-core.c
> +	.port_io_res			= vsc7512_port_io_res, // exported by drivers/mfd/ocelot-core.c
> +	.init_regmap			= ocelot_ext_regmap_init,
> +};

>From drivers/net/dsa/felix.c:

static int felix_init_structs(struct felix *felix, int num_phys_ports)
{
	for (i = 0; i < TARGET_MAX; i++) {
		struct regmap *target;

		if (!felix->info->target_io_res[i].name)
			continue;

		memcpy(&res, &felix->info->target_io_res[i], sizeof(res));
		res.flags = IORESOURCE_MEM;
		res.start += felix->switch_base;
		res.end += felix->switch_base;

		target = felix->info->init_regmap(ocelot, &res);
		if (IS_ERR(target)) {
			dev_err(ocelot->dev,
				"Failed to map device memory space\n");
			kfree(port_phy_modes);
			return PTR_ERR(target);
		}

		ocelot->targets[i] = target;
	}
}

So here's what I don't like. You export the resources from ocelot-mfd to
DSA, to get just their *string* names. Then you let the common code
create some bogus res.start and res.end in felix_init_structs(), which
you discard in felix->info->init_regmap() - ocelot_ext_regmap_init(),
and use just the name. You even discard the IORESOURCE_MEM flag, because
what you get back are IORESOURCE_REG resources. This is all very confusing.

So you need to retrieve a regmap for each ocelot target that you can.
Why don't you make it, via mfd_add_devices() and the "resources" array
of struct mfd_cell (i.e. the same mechanism as for every other peripheral),
such that the resources used by the DSA device have an index determined
by i = 0; i < TARGET_MAX; i++; platform_get_resource(dev, i, IORESOURCE_REG)?
This way, DSA needs to know no more than the index of the resource it
asks for.

[ yes, you'll need to revert your own commit 242bd0c10bbd ("net: dsa:
  ocelot: felix: add interface for custom regmaps"), which I asked you
  about if you're sure if this is the final way in which DSA will get
  its regmaps. Then you'll need to provide a different felix->info
  operation, such as felix->info->regmap_from_mfd() or something, where
  just the index is provided. If that isn't provided by the switch, we
  "fall back" to the code that exists right now, which, when reverted,
  does create an actual resource, and directly calls ocelot_regmap_init()
  on it, to create an MMIO regmap from it ]

Powered by blists - more mailing lists