Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext
From: Colin Foster
Date: Mon Sep 12 2022 - 15:05:02 EST
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@xxxxxxxxxxxxxxxx>
> > ---
> >
> > 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 ]