Message-ID: <Yx+CzUCbNgAjDK5l@colin-ia-desktop>
Date:   Mon, 12 Sep 2022 12:04:45 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....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 Mon, Sep 12, 2022 at 05:08:09PM +0000, Vladimir Oltean wrote:
> 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?

Will do.

> > +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?

Agreed on this and the other symbol. Thanks.

> 
> > +
> >  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.

Yes, it was a way to deal with this struct. I see that I should have at
least added a comment, but the way you suggest below is cleaner (before
try_add_regmap())

> 
> 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"?

I'll reword the comment - but I think it might go away entirely with
what you're suggesting below.

> 
> > +	 */
> > +	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().

I'm due for another re-read of this documentation. That's a very practical
suggestion.

> 
> > +
> > +	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.

That is exactly right. The ocelot_ext version of init_regmap() now uses
dev_get_regmap() which only cares about the name and essentially drops
the rest of the information. Previous versions hooked into the
ocelot-core / ocelot-spi MFD system to request that a new regmap be
created (with start and end being honored.) A benefit of this design is
that any regmaps that are named the same are automatically shared. A
drawback (or maybe a benefit?) is that the users have no control over
ranges / flags.

I think if this goes the way of index-based that'll work. I'm happy to
revert my previous change (sorry it snuck in) but it seems like there'll
still have to be some trickery... For reference:

enum ocelot_target {
	ANA = 1,
	QS,
	QSYS,
	REW,
	SYS,
	S0,
	S1,
	S2,
	HSIO,
	PTP,
	FDMA,
	GCB,
	DEV_GMII,
	TARGET_MAX,
};

mfd_add_devices will probably need to add a zero-size resource for HSIO,
PTP, FDMA, and anything else that might come along in the future. At
this point, regmap_from_mfd(PTP) might have to look like (pseudocode):

struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index)
{
    return ocelot_regmap_from_resource_optional(pdev, index-1, config);

    /* Note this essentially expands to:
     * res = platform_get_resource(pdev, IORESOURCE_REG, index-1);
     * return dev_get_regmap(pdev->dev.parent, res->name);
     *
     * This essentially throws away everything that my current
     * implementation does, except for the IORESOURCE_REG flag
     */
}

Then drivers/net/dsa/felix.c felix_init_structs() would have two loops
(again, just as an example)

for (i = ANA; i < TARGET_MAX; i++) {
    if (felix->info->regmap_from_mfd)
        target = felix->info->regmap_from_mfd(ocelot, i);
    else {
        /* existing logic back to ocelot_regmap_init() */
    }
}

for (port = 0; port < num_phys_ports; port++) {
    ...
    if (felix->info->regmap_from_mfd)
        target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port);
    else {
        /* existing logic back to ocelot_regmap_init() */
    }
}

And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a
mechanism to say "don't add a regmap for cell->resources[PTP], even
though that resource exists" because... well I suppose it is only in
drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of
those regmaps invokes different behavior. For instance:

	if (ocelot->targets[FDMA])
		ocelot_fdma_init(pdev, ocelot);

I'm not sure whether this last point will have an effect on felix.c in
the end. My current patch set of adding the SERDES ports uses the
existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the
ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that
gets rejected outright. That's for another day.



I'm happy to make these changes if you see them valid. I saw the fact
that dev_get_regmap(dev->parent, res->name) could be used directly in
ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib /
ocelot-core, but I recognize that the subtle "throw away the
IORESOURCE_MEM flag and res->{start,end} information" isn't ideal.

> 
> [ 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