[v11,net-next,1/9] mfd: ocelot: add helper to get regmap from a resource

Commit Message

Colin Foster June 28, 2022, 8:17 a.m. UTC

Several ocelot-related modules are designed for MMIO / regmaps. As such,
they often use a combination of devm_platform_get_and_ioremap_resource and
devm_regmap_init_mmio.

Operating in an MFD might be different, in that it could be memory mapped,
or it could be SPI, I2C... In these cases a fallback to use IORESOURCE_REG
instead of IORESOURCE_MEM becomes necessary.

When this happens, there's redundant logic that needs to be implemented in
every driver. In order to avoid this redundancy, utilize a single function
that, if the MFD scenario is enabled, will perform this fallback logic.

Signed-off-by: Colin Foster <[e-mail]>
---
 MAINTAINERS                |  5 +++++
 include/linux/mfd/ocelot.h | 27 +++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 include/linux/mfd/ocelot.h

Comments

Andy Shevchenko June 28, 2022, 12:50 p.m. UTC | #1

On Tue, Jun 28, 2022 at 10:17 AM Colin Foster
<[e-mail]> wrote:
>
> Several ocelot-related modules are designed for MMIO / regmaps. As such,
> they often use a combination of devm_platform_get_and_ioremap_resource and
> devm_regmap_init_mmio.
>
> Operating in an MFD might be different, in that it could be memory mapped,
> or it could be SPI, I2C... In these cases a fallback to use IORESOURCE_REG
> instead of IORESOURCE_MEM becomes necessary.
>
> When this happens, there's redundant logic that needs to be implemented in
> every driver. In order to avoid this redundancy, utilize a single function
> that, if the MFD scenario is enabled, will perform this fallback logic.

> +       regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
> +
> +       if (!res)
> +               return ERR_PTR(-ENOENT);

This needs a comment why the original error code from devm_ call above
is not good here.

> +       else if (IS_ERR(regs))
> +               return ERR_CAST(regs);
> +       else
> +               return devm_regmap_init_mmio(&pdev->dev, regs, config);
> +}

'else' is redundant in all cases above.

Vladimir Oltean June 28, 2022, 3:33 p.m. UTC | #2

On Tue, Jun 28, 2022 at 02:50:36PM +0200, Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 10:17 AM Colin Foster
> <[e-mail]> wrote:
> >
> > Several ocelot-related modules are designed for MMIO / regmaps. As such,
> > they often use a combination of devm_platform_get_and_ioremap_resource and
> > devm_regmap_init_mmio.
> >
> > Operating in an MFD might be different, in that it could be memory mapped,
> > or it could be SPI, I2C... In these cases a fallback to use IORESOURCE_REG
> > instead of IORESOURCE_MEM becomes necessary.
> >
> > When this happens, there's redundant logic that needs to be implemented in
> > every driver. In order to avoid this redundancy, utilize a single function
> > that, if the MFD scenario is enabled, will perform this fallback logic.
> 
> > +       regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
> > +
> > +       if (!res)
> > +               return ERR_PTR(-ENOENT);
> 
> This needs a comment why the original error code from devm_ call above
> is not good here.

I think what is really needed is an _optional() variant of
ocelot_platform_init_regmap_from_resource(), which just returns NULL on
missing resource and doesn't ioremap anything. It can be easily open
coded, i.e. instead of creating devm_platform_get_and_ioremap_resource_optional(),
we could just call platform_get_resource() and devm_ioremap_resource()
individually.

Vladimir Oltean June 28, 2022, 4:08 p.m. UTC | #3

On Tue, Jun 28, 2022 at 01:17:01AM -0700, Colin Foster wrote:
> diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
> new file mode 100644
> index 000000000000..5c95e4ee38a6
> --- /dev/null
> +++ b/include/linux/mfd/ocelot.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/* Copyright 2022 Innovative Advantage Inc. */
> +
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +struct resource;
> +
> +static inline struct regmap *
> +ocelot_platform_init_regmap_from_resource(struct platform_device *pdev,
> +					  unsigned int index,
> +					  const struct regmap_config *config)

I think this function name is too long (especially if you're going to
also introduce ocelot_platform_init_regmap_from_resource_optional),
and I have the impression that the "platform_init_" part of the name
doesn't bring too much value. How about ocelot_regmap_from_resource()?

> +{
> +	struct resource *res;
> +	u32 __iomem *regs;
> +
> +	regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
> +
> +	if (!res)
> +		return ERR_PTR(-ENOENT);
> +	else if (IS_ERR(regs))
> +		return ERR_CAST(regs);
> +	else
> +		return devm_regmap_init_mmio(&pdev->dev, regs, config);
> +}
> -- 
> 2.25.1
>

To illustrate what I'm trying to say, these would be the shim
definitions:

static inline struct regmap *
ocelot_regmap_from_resource(struct platform_device *pdev,
			    unsigned int index,
			    const struct regmap_config *config)
{
	struct resource *res;
	void __iomem *regs;

	regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
	if (IS_ERR(regs))
		return regs;

	return devm_regmap_init_mmio(&pdev->dev, regs, config);
}

static inline struct regmap *
ocelot_regmap_from_resource_optional(struct platform_device *pdev,
				     unsigned int index,
				     const struct regmap_config *config)
{
	struct resource *res;
	void __iomem *regs;

	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
	if (!res)
		return NULL;

	regs = devm_ioremap_resource(&pdev->dev, r);
	if (IS_ERR(regs))
		return regs;

	return devm_regmap_init_mmio(&pdev->dev, regs, config);
}

and these would be the full versions:

static struct regmap *
ocelot_regmap_from_mem_resource(struct device *dev, struct resource *res,
				const struct regmap_config *config)
{
	void __iomem *regs;

	regs = devm_ioremap_resource(dev, r);
	if (IS_ERR(regs))
		return regs;

	return devm_regmap_init_mmio(dev, regs, config);
}

static struct regmap *
ocelot_regmap_from_reg_resource(struct device *dev, struct resource *res,
				const struct regmap_config *config)
{
	/* Open question: how to differentiate SPI from I2C resources? */
	return ocelot_spi_init_regmap(dev->parent, dev, res);
}

struct regmap *
ocelot_regmap_from_resource_optional(struct platform_device *pdev,
				     unsigned int index,
				     const struct regmap_config *config)
{
	struct device *dev = &pdev->dev;
	struct resource *res;

	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
	if (res)
		return ocelot_regmap_from_mem_resource(dev, res, config);

	/*
	 * Fall back to using IORESOURCE_REG, which is possible in an
	 * MFD configuration
	 */
	res = platform_get_resource(pdev, IORESOURCE_REG, index);
	if (res)
		return ocelot_regmap_from_reg_resource(dev, res, config);

	return NULL;
}

struct regmap *
ocelot_regmap_from_resource(struct platform_device *pdev,
			    unsigned int index,
			    const struct regmap_config *config)
{
	struct regmap *map;

	map = ocelot_regmap_from_resource_optional(pdev, index, config);
	return map ? : ERR_PTR(-ENOENT);
}

I hope I didn't get something wrong, this is all code written within the
email client, so it is obviously not compiled/tested....

Colin Foster June 28, 2022, 5:25 p.m. UTC | #4

Hi Vladimir,

On Tue, Jun 28, 2022 at 04:08:10PM +0000, Vladimir Oltean wrote:
> On Tue, Jun 28, 2022 at 01:17:01AM -0700, Colin Foster wrote:
> > diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
> > new file mode 100644
> > index 000000000000..5c95e4ee38a6
> > --- /dev/null
> > +++ b/include/linux/mfd/ocelot.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/* Copyright 2022 Innovative Advantage Inc. */
> > +
> > +#include <linux/err.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> > +
> > +struct resource;
> > +
> > +static inline struct regmap *
> > +ocelot_platform_init_regmap_from_resource(struct platform_device *pdev,
> > +					  unsigned int index,
> > +					  const struct regmap_config *config)
> 
> I think this function name is too long (especially if you're going to
> also introduce ocelot_platform_init_regmap_from_resource_optional),
> and I have the impression that the "platform_init_" part of the name
> doesn't bring too much value. How about ocelot_regmap_from_resource()?

I thought the same thing after your first email. My thought was "how do
I indent that?" :-)

> 
> > +{
> > +	struct resource *res;
> > +	u32 __iomem *regs;
> > +
> > +	regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
> > +
> > +	if (!res)
> > +		return ERR_PTR(-ENOENT);
> > +	else if (IS_ERR(regs))
> > +		return ERR_CAST(regs);
> > +	else
> > +		return devm_regmap_init_mmio(&pdev->dev, regs, config);
> > +}
> > -- 
> > 2.25.1
> >
> 
> To illustrate what I'm trying to say, these would be the shim
> definitions:
> 
> static inline struct regmap *
> ocelot_regmap_from_resource(struct platform_device *pdev,
> 			    unsigned int index,
> 			    const struct regmap_config *config)
> {
> 	struct resource *res;
> 	void __iomem *regs;
> 
> 	regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
> 	if (IS_ERR(regs))
> 		return regs;
> 
> 	return devm_regmap_init_mmio(&pdev->dev, regs, config);
> }
> 
> static inline struct regmap *
> ocelot_regmap_from_resource_optional(struct platform_device *pdev,
> 				     unsigned int index,
> 				     const struct regmap_config *config)
> {
> 	struct resource *res;
> 	void __iomem *regs;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> 	if (!res)
> 		return NULL;
> 
> 	regs = devm_ioremap_resource(&pdev->dev, r);
> 	if (IS_ERR(regs))
> 		return regs;
> 
> 	return devm_regmap_init_mmio(&pdev->dev, regs, config);
> }
> 
> and these would be the full versions:
> 
> static struct regmap *
> ocelot_regmap_from_mem_resource(struct device *dev, struct resource *res,
> 				const struct regmap_config *config)
> {
> 	void __iomem *regs;
> 
> 	regs = devm_ioremap_resource(dev, r);
> 	if (IS_ERR(regs))
> 		return regs;
> 
> 	return devm_regmap_init_mmio(dev, regs, config);
> }
> 
> static struct regmap *
> ocelot_regmap_from_reg_resource(struct device *dev, struct resource *res,
> 				const struct regmap_config *config)
> {
> 	/* Open question: how to differentiate SPI from I2C resources? */

My expectation is to set something up in drivers/mfd/ocelot-{spi,i2c}.c
and have an if/else / switch. PCIe might actually be our first hardware
spin.

> 	return ocelot_spi_init_regmap(dev->parent, dev, res);
> }
> 
> struct regmap *
> ocelot_regmap_from_resource_optional(struct platform_device *pdev,
> 				     unsigned int index,
> 				     const struct regmap_config *config)
> {
> 	struct device *dev = &pdev->dev;
> 	struct resource *res;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> 	if (res)
> 		return ocelot_regmap_from_mem_resource(dev, res, config);
> 
> 	/*
> 	 * Fall back to using IORESOURCE_REG, which is possible in an
> 	 * MFD configuration
> 	 */
> 	res = platform_get_resource(pdev, IORESOURCE_REG, index);
> 	if (res)
> 		return ocelot_regmap_from_reg_resource(dev, res, config);
> 
> 	return NULL;
> }
> 
> struct regmap *
> ocelot_regmap_from_resource(struct platform_device *pdev,
> 			    unsigned int index,
> 			    const struct regmap_config *config)
> {
> 	struct regmap *map;
> 
> 	map = ocelot_regmap_from_resource_optional(pdev, index, config);
> 	return map ? : ERR_PTR(-ENOENT);
> }
> 
> I hope I didn't get something wrong, this is all code written within the
> email client, so it is obviously not compiled/tested....

Yep - I definitely get the point. And thanks for the review.

The other (bigger?) issue is around how this MFD can be loaded as a
module. Right now it is pretty straightforward to say
#if IS_ENABLED(CONFIG_MFD_OCELOT). Theres a level of nuance if
CONFIG_MFD_OCELOT=m while the child devices are compiled in
(CONFIG_PINCTRL_MICROCHIP_SGPIO=y for example). It still feels like this
code belongs somewhere in platform / resource / device / mfd...?

It might be perfectly valid to have multiple SGPIO controllers - one
local and one remote / SPI. But without the CONFIG_MFD_OCELOT module
loaded, I don't think the SGPIO module would work.

This patch set deals with the issue by setting MFD_OCELOT to a boolean -
but in the long run I think a module makes sense. I admittedly haven't
spent enough time researching (bashing my head against the wall) this,
but this seems like a good opportunity to at least express that I'm
expecting to have to deal with this issue soon. I met with Alexandre at
ELC this past week, and he said Arnd (both added to CC) might be a good
resource - but again I'd like to do a little more searching before
throwing it over the wall.

Vladimir Oltean June 28, 2022, 6:47 p.m. UTC | #5

On Tue, Jun 28, 2022 at 10:25:18AM -0700, Colin Foster wrote:
> > struct regmap *
> > ocelot_regmap_from_resource(struct platform_device *pdev,
> > 			    unsigned int index,
> > 			    const struct regmap_config *config)
> > {
> > 	struct regmap *map;
> > 
> > 	map = ocelot_regmap_from_resource_optional(pdev, index, config);
> > 	return map ? : ERR_PTR(-ENOENT);
> > }
> > 
> > I hope I didn't get something wrong, this is all code written within the
> > email client, so it is obviously not compiled/tested....
> 
> Yep - I definitely get the point. And thanks for the review.
> 
> The other (bigger?) issue is around how this MFD can be loaded as a
> module. Right now it is pretty straightforward to say
> #if IS_ENABLED(CONFIG_MFD_OCELOT). Theres a level of nuance if
> CONFIG_MFD_OCELOT=m while the child devices are compiled in
> (CONFIG_PINCTRL_MICROCHIP_SGPIO=y for example). It still feels like this
> code belongs somewhere in platform / resource / device / mfd...?
> 
> It might be perfectly valid to have multiple SGPIO controllers - one
> local and one remote / SPI. But without the CONFIG_MFD_OCELOT module
> loaded, I don't think the SGPIO module would work.
> 
> This patch set deals with the issue by setting MFD_OCELOT to a boolean -
> but in the long run I think a module makes sense. I admittedly haven't
> spent enough time researching (bashing my head against the wall) this,
> but this seems like a good opportunity to at least express that I'm
> expecting to have to deal with this issue soon. I met with Alexandre at
> ELC this past week, and he said Arnd (both added to CC) might be a good
> resource - but again I'd like to do a little more searching before
> throwing it over the wall.

Well, that's quite a different ball game. It sounds like what you want
is to have a dynamic list of regmap providers for a device, and when a
device probes, to not depend on all the modules of all the possible
providers being inserted at that particular time. Which makes sense,
and I agree it's something that should be handled by the kernel core if
you don't want the sgpio code to directly call symbols exported by the
ocelot mfd core.

I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
dev_get_regmap(), maybe those could be of help? I'm not completely sure
about the mechanics of it all, but what I'm searching for is something
through which the mfd core could attach a regmap to a platform device
through devres before calling platform_device_add(), such that the
driver can use it (or manually fall back to an MMIO regmap) via
dev_get_regmap().

Vladimir Oltean June 28, 2022, 6:56 p.m. UTC | #6

On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote:
> I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
> dev_get_regmap(), maybe those could be of help?

ah, I see I haven't really brought anything of value to the table,
dev_get_regmap() was discussed around v1 or so. I'll read the
discussions again in a couple of hours to remember what was wrong with
it such that you aren't using it anymore.

Andy Shevchenko June 28, 2022, 7:04 p.m. UTC | #7

On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <[e-mail]> wrote:
>
> On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote:
> > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
> > dev_get_regmap(), maybe those could be of help?
>
> ah, I see I haven't really brought anything of value to the table,
> dev_get_regmap() was discussed around v1 or so. I'll read the
> discussions again in a couple of hours to remember what was wrong with
> it such that you aren't using it anymore.

It would be nice if you can comment after here to clarify that.
Because in another series (not related to this anyhow) somebody
insisted to use dev_get_regmap().

Colin Foster June 28, 2022, 7:56 p.m. UTC | #8

On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <[e-mail]> wrote:
> >
> > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote:
> > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
> > > dev_get_regmap(), maybe those could be of help?
> >
> > ah, I see I haven't really brought anything of value to the table,
> > dev_get_regmap() was discussed around v1 or so. I'll read the
> > discussions again in a couple of hours to remember what was wrong with
> > it such that you aren't using it anymore.
> 
> It would be nice if you can comment after here to clarify that.
> Because in another series (not related to this anyhow) somebody
> insisted to use dev_get_regmap().

To add some info: The main issue at that time was "how do I get a spi
regmap instead of an mmio regmap from the device". V1 was very early,
and was before I knew about the pinctrl / mdio drivers that were to
come, so that led to the existing MFD implementation.

I came across the IORESOURCE_REG, which seems to be created for exactly
this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG
doesn't get used much compared to IORESOURCE_MEM.

Though I'll revisit this again. The switch portion of this driver (no
longer included in this patch set) is actually quite different from the
rest of the MFD in how it allocates regmaps, and that might have been
a major contributor at the time. So maybe I dismissed it at the time,
but it actually makes sense for the MFD portion now.

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Vladimir Oltean June 29, 2022, 5:53 p.m. UTC | #9

On Tue, Jun 28, 2022 at 12:56:54PM -0700, Colin Foster wrote:
> On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote:
> > On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <[e-mail]> wrote:
> > >
> > > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote:
> > > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
> > > > dev_get_regmap(), maybe those could be of help?
> > >
> > > ah, I see I haven't really brought anything of value to the table,
> > > dev_get_regmap() was discussed around v1 or so. I'll read the
> > > discussions again in a couple of hours to remember what was wrong with
> > > it such that you aren't using it anymore.
> > 
> > It would be nice if you can comment after here to clarify that.
> > Because in another series (not related to this anyhow) somebody
> > insisted to use dev_get_regmap().
> 
> To add some info: The main issue at that time was "how do I get a spi
> regmap instead of an mmio regmap from the device". V1 was very early,
> and was before I knew about the pinctrl / mdio drivers that were to
> come, so that led to the existing MFD implementation.
> 
> I came across the IORESOURCE_REG, which seems to be created for exactly
> this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG
> doesn't get used much compared to IORESOURCE_MEM.
> 
> Though I'll revisit this again. The switch portion of this driver (no
> longer included in this patch set) is actually quite different from the
> rest of the MFD in how it allocates regmaps, and that might have been
> a major contributor at the time. So maybe I dismissed it at the time,
> but it actually makes sense for the MFD portion now.

I'm sorry, I can't actually understand what went wrong, I'll need some
help from you, Colin.

So during your RFC v1 and then v1 proper (Nov. 19, 2021), you talked
about dev_get_regmap(dev->parent, res->name) yourself and proposed a
relatively simple interface where the mfd child drivers would just
request a regmap by its name:
https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/

In fact you implemented just this (Dec. 6, 2021):
https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/#24637477
it's just that the pattern went something like:

@@ -1368,7 +1369,11 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 	regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;

-	info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
+	if (device_is_mfd(pdev))
+		info->map = dev_get_regmap(dev->parent, "GCB");
+	else
+		info->map = devm_regmap_init_mmio(dev, base, &regmap_config);

where Lee Jones (rightfully) commented asking why can't you just first
check whether dev->parent has any GCB regmap to give you, and only then
resort to call devm_regmap_init_mmio? A small comment with a small
and pretty actionable change to be made.


As best as I can tell, RFC v5 (Dec. 18, 2021) is the first version after
v1 where you came back with proposed mfd patches:
https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/

And while dev_get_regmap() was technically still there, its usage
pattern changed radically. It was now just used as a sort of
optimization in ocelot_mfd_regmap_init() to not create twice a regmap
that already existed.
What you introduced in RFC v5 instead was this "interface for MFD
children to get regmaps":
https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/

to which Lee replied that "This is almost certainly not the right way to
do whatever it is you're trying to do!"

And rightfully so. What happened to dev_get_regmap() as the "interface
for MFD children to get regmaps" I wonder?
dev_get_regmap() just wants a name, not a full blown resource.
When you're a mfd child, you don't have a full resource, you just know
the name of the regmap you want to use. Only the top dog needs to have
access to the resources. And DSA as a MFD child is not top dog.
So of course I expect it to change. Otherwise said, what is currently
done in felix_init_structs() needs to be more or less replicated in its
entirety in drivers/mfd/ocelot-core.c.
All the regmaps of the switch SoC, created at mfd parent probe time, not
on demand, and attached via devres to the mfd parent device, so that
children can get them via dev_get_regmap.

Next thing would be to modify the felix DSA driver so that it only
attempts to create regmaps if it can do so (if it has the resource
structures). If it doesn't have the resource structures, it calls
dev_get_regmap(ocelot->dev->parent, target->name) and tries to get the
regmaps that way. If that fails => so sad, too bad, fail to probe, bye.

The point is that the ocelot-ext driver you're trying to introduce
should have no resources in struct resource *target_io_res, *port_io_res,
*imdio_res, etc. I really don't know why you're so fixated on this
"regmap from resource" thing when the resource shouldn't even be of
concern to the driver when used as a mfd child.

The contract is _not_ "here's the resource, give me the regmap".
The contract is "I want the regmap named XXX". And in order to fulfill
that contract, a mfd child driver should _not_ call a symbol exported by
the ocelot parent driver directly (for the builtin vs module dependency
reasons you've mentioned yourself), but get the regmap from the list of
regmaps managed by devres using devm_regmap_init().

Yes, there would need to exist a split between the target strings and
their start and end offsets, because the ocelot-ext driver would still
call dev_get_regmap() by the name. But that's a fairly minor rework, and
by the way, it turns out that the introduction of felix->info->init_regmap()
was indeed not the right thing to do, so you'll need to change that again.

I am really not sure what went with the plan above. You said a while ago
that you don't like the fact that regmaps exist in the parent
independently of the drivers that use them and continue to do so even
after the children unbind, and that feels "wrong". Yes, because?

Colin Foster June 29, 2022, 8:39 p.m. UTC | #10

Hi Vladimir,

Wow, thanks for the summary! This is jogging my memory.

On Wed, Jun 29, 2022 at 05:53:06PM +0000, Vladimir Oltean wrote:
> On Tue, Jun 28, 2022 at 12:56:54PM -0700, Colin Foster wrote:
> > On Tue, Jun 28, 2022 at 09:04:21PM +0200, Andy Shevchenko wrote:
> > > On Tue, Jun 28, 2022 at 8:56 PM Vladimir Oltean <[e-mail]> wrote:
> > > >
> > > > On Tue, Jun 28, 2022 at 09:46:59PM +0300, Vladimir Oltean wrote:
> > > > > I searched for 5 minutes or so, and I noticed regmap_attach_dev() and
> > > > > dev_get_regmap(), maybe those could be of help?
> > > >
> > > > ah, I see I haven't really brought anything of value to the table,
> > > > dev_get_regmap() was discussed around v1 or so. I'll read the
> > > > discussions again in a couple of hours to remember what was wrong with
> > > > it such that you aren't using it anymore.
> > > 
> > > It would be nice if you can comment after here to clarify that.
> > > Because in another series (not related to this anyhow) somebody
> > > insisted to use dev_get_regmap().
> > 
> > To add some info: The main issue at that time was "how do I get a spi
> > regmap instead of an mmio regmap from the device". V1 was very early,
> > and was before I knew about the pinctrl / mdio drivers that were to
> > come, so that led to the existing MFD implementation.
> > 
> > I came across the IORESOURCE_REG, which seems to be created for exactly
> > this purpose. Seemingly I'm pretty unique though, since IORESOURCE_REG
> > doesn't get used much compared to IORESOURCE_MEM.
> > 
> > Though I'll revisit this again. The switch portion of this driver (no
> > longer included in this patch set) is actually quite different from the
> > rest of the MFD in how it allocates regmaps, and that might have been
> > a major contributor at the time. So maybe I dismissed it at the time,
> > but it actually makes sense for the MFD portion now.
> 
> I'm sorry, I can't actually understand what went wrong, I'll need some
> help from you, Colin.
> 
> So during your RFC v1 and then v1 proper (Nov. 19, 2021), you talked
> about dev_get_regmap(dev->parent, res->name) yourself and proposed a
> relatively simple interface where the mfd child drivers would just
> request a regmap by its name:
> https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
> 
> In fact you implemented just this (Dec. 6, 2021):
> https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/#24637477
> it's just that the pattern went something like:
> 
> @@ -1368,7 +1369,11 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
>  	regmap_config.max_register = OCELOT_GPIO_SD_MAP * info->stride + 15 * 4;
> 
> -	info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
> +	if (device_is_mfd(pdev))
> +		info->map = dev_get_regmap(dev->parent, "GCB");
> +	else
> +		info->map = devm_regmap_init_mmio(dev, base, &regmap_config);
> 
> where Lee Jones (rightfully) commented asking why can't you just first
> check whether dev->parent has any GCB regmap to give you, and only then
> resort to call devm_regmap_init_mmio? A small comment with a small
> and pretty actionable change to be made.
> 
> 
> As best as I can tell, RFC v5 (Dec. 18, 2021) is the first version after
> v1 where you came back with proposed mfd patches:
> https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
> 
> And while dev_get_regmap() was technically still there, its usage
> pattern changed radically. It was now just used as a sort of
> optimization in ocelot_mfd_regmap_init() to not create twice a regmap
> that already existed.
> What you introduced in RFC v5 instead was this "interface for MFD
> children to get regmaps":
> https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
> 
> to which Lee replied that "This is almost certainly not the right way to
> do whatever it is you're trying to do!"
> 
> And rightfully so. What happened to dev_get_regmap() as the "interface
> for MFD children to get regmaps" I wonder?
> dev_get_regmap() just wants a name, not a full blown resource.
> When you're a mfd child, you don't have a full resource, you just know
> the name of the regmap you want to use. Only the top dog needs to have
> access to the resources. And DSA as a MFD child is not top dog.
> So of course I expect it to change. Otherwise said, what is currently
> done in felix_init_structs() needs to be more or less replicated in its
> entirety in drivers/mfd/ocelot-core.c.
> All the regmaps of the switch SoC, created at mfd parent probe time, not
> on demand, and attached via devres to the mfd parent device, so that
> children can get them via dev_get_regmap.
> 

I think you're pointing out exactly where I went wrong. And I think it
might be related to my starting with the felix dsa driver before
migrating to the MFD setup.

My misconception was "the felix dsa driver should be able to create whatever
regmaps from resources it needs" and maybe I was being to optimistic
that I wouldn't need to change felix dsa's implementation. Or naive.

> Next thing would be to modify the felix DSA driver so that it only
> attempts to create regmaps if it can do so (if it has the resource
> structures). If it doesn't have the resource structures, it calls
> dev_get_regmap(ocelot->dev->parent, target->name) and tries to get the
> regmaps that way. If that fails => so sad, too bad, fail to probe, bye.
> 
> The point is that the ocelot-ext driver you're trying to introduce
> should have no resources in struct resource *target_io_res, *port_io_res,
> *imdio_res, etc. I really don't know why you're so fixated on this
> "regmap from resource" thing when the resource shouldn't even be of
> concern to the driver when used as a mfd child.
> 
> The contract is _not_ "here's the resource, give me the regmap".
> The contract is "I want the regmap named XXX". And in order to fulfill
> that contract, a mfd child driver should _not_ call a symbol exported by
> the ocelot parent driver directly (for the builtin vs module dependency
> reasons you've mentioned yourself), but get the regmap from the list of
> regmaps managed by devres using devm_regmap_init().
> 
> Yes, there would need to exist a split between the target strings and
> their start and end offsets, because the ocelot-ext driver would still
> call dev_get_regmap() by the name. But that's a fairly minor rework, and
> by the way, it turns out that the introduction of felix->info->init_regmap()
> was indeed not the right thing to do, so you'll need to change that again.
> 
> I am really not sure what went with the plan above. You said a while ago
> that you don't like the fact that regmaps exist in the parent
> independently of the drivers that use them and continue to do so even
> after the children unbind, and that feels "wrong". Yes, because?

I liked the idea of the MFD being "code complete" so if future regmaps
were needed for the felix dsa driver came about, it wouldn't require
changes to the "parent." But I think that was a bad goal - especially
since MFD requires all the resources anyway.

Also at the time, I was trying a hybrid "create it if it doesn't exist,
return it if was already created" approach. I backed that out after an
RFC.

Focusing only on the non-felix drivers: it seems trivial for the parent
to create _all_ the possible child regmaps, register them to the parent
via by way of regmap_attach_dev().

At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to
initalize like (untested, and apologies for indentation):

regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
if (IS_ERR(regs)) {
    map = dev_get_regmap(dev->parent, name);
} else {
    map = devm_regmap_init_mmio(dev, regs, config);
}

In that case, "name" would either be hard-coded to match what is in
drivers/mfd/ocelot-core.c. The other option is to fall back to
platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
resource->name. I'll be able to deal with that when I try it. (hopefully
this evening)

This seems to be a solid design that I missed! As you mention, it'll
require changes to felix dsa... but not as much as I had feared. And I
think it solves all my fears about modules to boot. This seems too good
to be true - but maybe I was too deep and needed to take this step back.

Vladimir Oltean June 29, 2022, 11:08 p.m. UTC | #11

On Wed, Jun 29, 2022 at 01:39:05PM -0700, Colin Foster wrote:
> I liked the idea of the MFD being "code complete" so if future regmaps
> were needed for the felix dsa driver came about, it wouldn't require
> changes to the "parent." But I think that was a bad goal - especially
> since MFD requires all the resources anyway.
> 
> Also at the time, I was trying a hybrid "create it if it doesn't exist,
> return it if was already created" approach. I backed that out after an
> RFC.
> 
> Focusing only on the non-felix drivers: it seems trivial for the parent
> to create _all_ the possible child regmaps, register them to the parent
> via by way of regmap_attach_dev().
> 
> At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to
> initalize like (untested, and apologies for indentation):
> 
> regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> if (IS_ERR(regs)) {
>     map = dev_get_regmap(dev->parent, name);
> } else {
>     map = devm_regmap_init_mmio(dev, regs, config);
> }

Again, those dev_err(dev, "invalid resource\n"); prints you were
complaining about earlier are self-inflicted IMO, and caused exactly by
this pattern. I get why you prefer to call larger building blocks if
possible, but in this case, devm_platform_get_and_ioremap_resource()
calls exactly 2 sub-functions: platform_get_resource() and
devm_ioremap_resource(). The IS_ERR() that you check for is caused by
devm_ioremap_resource() being passed a NULL pointer, and same goes for
the print. Just call them individually, and put your dev_get_regmap()
hook in case platform_get_resource() returns NULL, rather than passing
NULL to devm_ioremap_resource() and waiting for that to fail.

> In that case, "name" would either be hard-coded to match what is in
> drivers/mfd/ocelot-core.c. The other option is to fall back to
> platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
> resource->name. I'll be able to deal with that when I try it. (hopefully
> this evening)

I'm not exactly clear on what you'd do with the REG resource once you
get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
from the device tree, what next, who's going to set up the SPI regmap
for you?

> This seems to be a solid design that I missed! As you mention, it'll
> require changes to felix dsa... but not as much as I had feared. And I
> think it solves all my fears about modules to boot. This seems too good
> to be true - but maybe I was too deep and needed to take this step back.

Colin Foster June 29, 2022, 11:54 p.m. UTC | #12

On Wed, Jun 29, 2022 at 11:08:05PM +0000, Vladimir Oltean wrote:
> On Wed, Jun 29, 2022 at 01:39:05PM -0700, Colin Foster wrote:
> > I liked the idea of the MFD being "code complete" so if future regmaps
> > were needed for the felix dsa driver came about, it wouldn't require
> > changes to the "parent." But I think that was a bad goal - especially
> > since MFD requires all the resources anyway.
> > 
> > Also at the time, I was trying a hybrid "create it if it doesn't exist,
> > return it if was already created" approach. I backed that out after an
> > RFC.
> > 
> > Focusing only on the non-felix drivers: it seems trivial for the parent
> > to create _all_ the possible child regmaps, register them to the parent
> > via by way of regmap_attach_dev().
> > 
> > At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to
> > initalize like (untested, and apologies for indentation):
> > 
> > regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > if (IS_ERR(regs)) {
> >     map = dev_get_regmap(dev->parent, name);
> > } else {
> >     map = devm_regmap_init_mmio(dev, regs, config);
> > }
> 
> Again, those dev_err(dev, "invalid resource\n"); prints you were
> complaining about earlier are self-inflicted IMO, and caused exactly by
> this pattern. I get why you prefer to call larger building blocks if
> possible, but in this case, devm_platform_get_and_ioremap_resource()
> calls exactly 2 sub-functions: platform_get_resource() and
> devm_ioremap_resource(). The IS_ERR() that you check for is caused by
> devm_ioremap_resource() being passed a NULL pointer, and same goes for
> the print. Just call them individually, and put your dev_get_regmap()
> hook in case platform_get_resource() returns NULL, rather than passing
> NULL to devm_ioremap_resource() and waiting for that to fail.

I see that now. Hoping this next version removes a lot of this
unnecessary complexity.

> 
> > In that case, "name" would either be hard-coded to match what is in
> > drivers/mfd/ocelot-core.c. The other option is to fall back to
> > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
> > resource->name. I'll be able to deal with that when I try it. (hopefully
> > this evening)
> 
> I'm not exactly clear on what you'd do with the REG resource once you
> get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
> from the device tree, what next, who's going to set up the SPI regmap
> for you?

The REG resource would only get the resource name, while the MFD core
driver would set up the regmaps.

e.g. drivers/mfd/ocelot-core.c has (annotated):
static const struct resource vsc7512_sgpio_resources[] = {
    DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") };

Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the
gpio resource, and gets the resource by index.

So for this there seem to be two options:
Option 1:
drivers/pinctrl/pinctrl-ocelot.c:
res = platform_get_resource(pdev, IORESOURCE_REG, 0);
map = dev_get_regmap(dev->parent, res->name);


OR Option 2:
include/linux/mfd/ocelot.h has something like:
#define GCB_GPIO_REGMAP_NAME "gcb_gpio"

and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to:
map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME);

(With error checking, macro reuse, etc.)


I like option 1, since it then makes ocelot-pinctrl.c have no reliance
on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are
set up in advance during the start of ocelot_core_init, just before
devm_mfd_add_devices is called.


I should be able to test this all tonight.

> 
> > This seems to be a solid design that I missed! As you mention, it'll
> > require changes to felix dsa... but not as much as I had feared. And I
> > think it solves all my fears about modules to boot. This seems too good
> > to be true - but maybe I was too deep and needed to take this step back.

Lee Jones June 30, 2022, 7:54 a.m. UTC | #13

On Wed, 29 Jun 2022, Colin Foster wrote:

> On Wed, Jun 29, 2022 at 11:08:05PM +0000, Vladimir Oltean wrote:
> > On Wed, Jun 29, 2022 at 01:39:05PM -0700, Colin Foster wrote:
> > > I liked the idea of the MFD being "code complete" so if future regmaps
> > > were needed for the felix dsa driver came about, it wouldn't require
> > > changes to the "parent." But I think that was a bad goal - especially
> > > since MFD requires all the resources anyway.
> > > 
> > > Also at the time, I was trying a hybrid "create it if it doesn't exist,
> > > return it if was already created" approach. I backed that out after an
> > > RFC.
> > > 
> > > Focusing only on the non-felix drivers: it seems trivial for the parent
> > > to create _all_ the possible child regmaps, register them to the parent
> > > via by way of regmap_attach_dev().
> > > 
> > > At that point, changing things like drivers/pinctrl/pinctrl-ocelot.c to
> > > initalize like (untested, and apologies for indentation):
> > > 
> > > regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > > if (IS_ERR(regs)) {
> > >     map = dev_get_regmap(dev->parent, name);
> > > } else {
> > >     map = devm_regmap_init_mmio(dev, regs, config);
> > > }
> > 
> > Again, those dev_err(dev, "invalid resource\n"); prints you were
> > complaining about earlier are self-inflicted IMO, and caused exactly by
> > this pattern. I get why you prefer to call larger building blocks if
> > possible, but in this case, devm_platform_get_and_ioremap_resource()
> > calls exactly 2 sub-functions: platform_get_resource() and
> > devm_ioremap_resource(). The IS_ERR() that you check for is caused by
> > devm_ioremap_resource() being passed a NULL pointer, and same goes for
> > the print. Just call them individually, and put your dev_get_regmap()
> > hook in case platform_get_resource() returns NULL, rather than passing
> > NULL to devm_ioremap_resource() and waiting for that to fail.
> 
> I see that now. Hoping this next version removes a lot of this
> unnecessary complexity.
> 
> > 
> > > In that case, "name" would either be hard-coded to match what is in
> > > drivers/mfd/ocelot-core.c. The other option is to fall back to
> > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
> > > resource->name. I'll be able to deal with that when I try it. (hopefully
> > > this evening)
> > 
> > I'm not exactly clear on what you'd do with the REG resource once you
> > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
> > from the device tree, what next, who's going to set up the SPI regmap
> > for you?
> 
> The REG resource would only get the resource name, while the MFD core
> driver would set up the regmaps.
> 
> e.g. drivers/mfd/ocelot-core.c has (annotated):
> static const struct resource vsc7512_sgpio_resources[] = {
>     DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") };
> 
> Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the
> gpio resource, and gets the resource by index.
> 
> So for this there seem to be two options:
> Option 1:
> drivers/pinctrl/pinctrl-ocelot.c:
> res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> map = dev_get_regmap(dev->parent, res->name);
> 
> 
> OR Option 2:
> include/linux/mfd/ocelot.h has something like:
> #define GCB_GPIO_REGMAP_NAME "gcb_gpio"
> 
> and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to:
> map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME);
> 
> (With error checking, macro reuse, etc.)
> 
> 
> I like option 1, since it then makes ocelot-pinctrl.c have no reliance
> on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are
> set up in advance during the start of ocelot_core_init, just before
> devm_mfd_add_devices is called.
> 
> 
> I should be able to test this all tonight.

Thank you Vladimir for stepping in to clarify previous points.

Well done both of you for this collaborative effort.  Great to see!

Vladimir Oltean June 30, 2022, 1:11 p.m. UTC | #14

On Wed, Jun 29, 2022 at 04:54:35PM -0700, Colin Foster wrote:
> > > In that case, "name" would either be hard-coded to match what is in
> > > drivers/mfd/ocelot-core.c. The other option is to fall back to
> > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
> > > resource->name. I'll be able to deal with that when I try it. (hopefully
> > > this evening)
> > 
> > I'm not exactly clear on what you'd do with the REG resource once you
> > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
> > from the device tree, what next, who's going to set up the SPI regmap
> > for you?
> 
> The REG resource would only get the resource name, while the MFD core
> driver would set up the regmaps.
> 
> e.g. drivers/mfd/ocelot-core.c has (annotated):
> static const struct resource vsc7512_sgpio_resources[] = {
>     DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") };
> 
> Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the
> gpio resource, and gets the resource by index.
> 
> So for this there seem to be two options:
> Option 1:
> drivers/pinctrl/pinctrl-ocelot.c:
> res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> map = dev_get_regmap(dev->parent, res->name);
> 
> 
> OR Option 2:
> include/linux/mfd/ocelot.h has something like:
> #define GCB_GPIO_REGMAP_NAME "gcb_gpio"
> 
> and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to:
> map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME);
> 
> (With error checking, macro reuse, etc.)
> 
> 
> I like option 1, since it then makes ocelot-pinctrl.c have no reliance
> on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are
> set up in advance during the start of ocelot_core_init, just before
> devm_mfd_add_devices is called.
> 
> 
> I should be able to test this all tonight.

I see what you mean now with the named resources from drivers/mfd/ocelot-core.c.
I don't particularly like the platform_get_resource(0) option, because
it's not as obvious/searchable what resource the pinctrl driver is
asking for.

I suppose a compromise variant might be to combine the 2 options.
Put enum ocelot_target in a header included by both drivers/mfd/ocelot-core.c,
then create a _single_ resource table in the MFD driver, indexed by enum
ocelot_target:

static const struct resource vsc7512_resources[TARGET_MAX] = {
	[ANA] = DEFINE_RES_REG_NAMED(start, end, "ana"),
	...
};

then provide the exact same resource table to all children.

In the pinctrl driver you can then do:
	res = platform_get_resource(pdev, IORESOURCE_REG, GPIO);
	map = dev_get_regmap(dev->parent, res->name);

and you get both the benefit of not hardcoding the string twice, and the
benefit of having some obvious keyword which can be used to link the mfd
driver to the child driver via grep, for those trying to understand what
goes on.

In addition, if there's a single resource table used for all peripherals,
theoretically you need to modify less code in mfd/ocelot-core.c in case
one driver or another needs access to one more regmap, if that regmap
happened to be needed by some other driver already. Plus fewer tables to
lug around, in general.

Colin Foster June 30, 2022, 8:09 p.m. UTC | #15

On Thu, Jun 30, 2022 at 01:11:56PM +0000, Vladimir Oltean wrote:
> On Wed, Jun 29, 2022 at 04:54:35PM -0700, Colin Foster wrote:
> > > > In that case, "name" would either be hard-coded to match what is in
> > > > drivers/mfd/ocelot-core.c. The other option is to fall back to
> > > > platform_get_resource(pdev, IORESOURCE_REG, 0), and pass in
> > > > resource->name. I'll be able to deal with that when I try it. (hopefully
> > > > this evening)
> > > 
> > > I'm not exactly clear on what you'd do with the REG resource once you
> > > get it. Assuming you'd get access to the "reg = <0x71070034 0x6c>;"
> > > from the device tree, what next, who's going to set up the SPI regmap
> > > for you?
> > 
> > The REG resource would only get the resource name, while the MFD core
> > driver would set up the regmaps.
> > 
> > e.g. drivers/mfd/ocelot-core.c has (annotated):
> > static const struct resource vsc7512_sgpio_resources[] = {
> >     DEFINE_RES_REG_NAMED(start, size, "gcb_gpio") };
> > 
> > Now, the drivers/pinctrl/pinctrl-ocelot.c expects resource 0 to be the
> > gpio resource, and gets the resource by index.
> > 
> > So for this there seem to be two options:
> > Option 1:
> > drivers/pinctrl/pinctrl-ocelot.c:
> > res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> > map = dev_get_regmap(dev->parent, res->name);
> > 
> > 
> > OR Option 2:
> > include/linux/mfd/ocelot.h has something like:
> > #define GCB_GPIO_REGMAP_NAME "gcb_gpio"
> > 
> > and drivers/pinctrl/pinctrl-ocelot.c skips get_resource and jumps to:
> > map = dev_get_regmap(dev->parent, GCB_GPIO_REGMAP_NAME);
> > 
> > (With error checking, macro reuse, etc.)
> > 
> > 
> > I like option 1, since it then makes ocelot-pinctrl.c have no reliance
> > on include/linux/mfd/ocelot.h. But in both cases, all the regmaps are
> > set up in advance during the start of ocelot_core_init, just before
> > devm_mfd_add_devices is called.
> > 
> > 
> > I should be able to test this all tonight.
> 
> I see what you mean now with the named resources from drivers/mfd/ocelot-core.c.
> I don't particularly like the platform_get_resource(0) option, because
> it's not as obvious/searchable what resource the pinctrl driver is
> asking for.
> 
> I suppose a compromise variant might be to combine the 2 options.
> Put enum ocelot_target in a header included by both drivers/mfd/ocelot-core.c,
> then create a _single_ resource table in the MFD driver, indexed by enum
> ocelot_target:
> 
> static const struct resource vsc7512_resources[TARGET_MAX] = {
> 	[ANA] = DEFINE_RES_REG_NAMED(start, end, "ana"),
> 	...
> };
> 
> then provide the exact same resource table to all children.
> 
> In the pinctrl driver you can then do:
> 	res = platform_get_resource(pdev, IORESOURCE_REG, GPIO);
> 	map = dev_get_regmap(dev->parent, res->name);
> 
> and you get both the benefit of not hardcoding the string twice, and the
> benefit of having some obvious keyword which can be used to link the mfd
> driver to the child driver via grep, for those trying to understand what
> goes on.
> 
> In addition, if there's a single resource table used for all peripherals,
> theoretically you need to modify less code in mfd/ocelot-core.c in case
> one driver or another needs access to one more regmap, if that regmap
> happened to be needed by some other driver already. Plus fewer tables to
> lug around, in general.

Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet,
but I'm liking this:

static inline struct regmap *
ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index,
                            const struct regmap_config *config)
{
        struct device *dev = &pdev->dev;
        struct resource *res;
        u32 __iomem *regs;

        res = platform_get_resource(pdev, IORESOURCE_MEM, index);
        if (res) {
                regs = devm_ioremap_resource(dev, res);
                if (IS_ERR(regs))
                        return ERR_CAST(regs);
                return devm_regmap_init_mmio(dev, regs, config);
        }

        /*
         * Fall back to using REG and getting the resource from the parent
         * device, which is possible in an MFD configuration
         */
        res = platform_get_resource(pdev, IORESOURCE_REG, index);
        if (!res)
                return ERR_PTR(-ENOENT);

        return (dev_get_regmap(dev->parent, res->name));
}

So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
an inline helper function. And so long as ocelot_core_init does this:

static void ocelot_core_try_add_regmap(struct device *dev,
                                       const struct resource *res)
{
        if (!dev_get_regmap(dev, res->name)) {
                ocelot_spi_init_regmap(dev, res);
        }
}

static void ocelot_core_try_add_regmaps(struct device *dev,
                                        const struct mfd_cell *cell)
{
        int i;

        for (i = 0; i < cell->num_resources; i++) {
                ocelot_core_try_add_regmap(dev, &cell->resources[i]);
        }
}

int ocelot_core_init(struct device *dev)
{
        int i, ndevs;

        ndevs = ARRAY_SIZE(vsc7512_devs);

        for (i = 0; i < ndevs; i++)
                ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);

        return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
                                    ndevs, NULL, 0, NULL);
}
EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);

we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux
game still)


I like the enum / macro idea for cleanup, but I think that's a different
problem I can address. The main question I have now is this:

The ocelot_regmap_from_resource now has nothing to do with the ocelot
MFD system. It is generic. (If you listen carefully, you might hear me
cheering)

I can keep this in linux/mfd/ocelot.h, but is this actually something
that belongs elsewhere? platform? device? mfd-core?


And yes, I like the idea of changing the driver to
"ocelot_regmap_from_resource(pdev, GPIO, config);" from
"ocelot_regmap_from_resource(pdev, 0, config);"

Vladimir Oltean July 1, 2022, 4:21 p.m. UTC | #16

On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote:
> Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet,
> but I'm liking this:
> 
> static inline struct regmap *
> ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index,
>                             const struct regmap_config *config)
> {
>         struct device *dev = &pdev->dev;
>         struct resource *res;
>         u32 __iomem *regs;
> 
>         res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>         if (res) {
>                 regs = devm_ioremap_resource(dev, res);
>                 if (IS_ERR(regs))
>                         return ERR_CAST(regs);
>                 return devm_regmap_init_mmio(dev, regs, config);
>         }
> 
>         /*
>          * Fall back to using REG and getting the resource from the parent
>          * device, which is possible in an MFD configuration
>          */
>         res = platform_get_resource(pdev, IORESOURCE_REG, index);
>         if (!res)
>                 return ERR_PTR(-ENOENT);
> 
>         return (dev_get_regmap(dev->parent, res->name));

parentheses not needed around dev_get_regmap.

> }
> 
> So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> an inline helper function. And so long as ocelot_core_init does this:
> 
> static void ocelot_core_try_add_regmap(struct device *dev,
>                                        const struct resource *res)
> {
>         if (!dev_get_regmap(dev, res->name)) {
>                 ocelot_spi_init_regmap(dev, res);
>         }
> }
> 
> static void ocelot_core_try_add_regmaps(struct device *dev,
>                                         const struct mfd_cell *cell)
> {
>         int i;
> 
>         for (i = 0; i < cell->num_resources; i++) {
>                 ocelot_core_try_add_regmap(dev, &cell->resources[i]);
>         }
> }
> 
> int ocelot_core_init(struct device *dev)
> {
>         int i, ndevs;
> 
>         ndevs = ARRAY_SIZE(vsc7512_devs);
> 
>         for (i = 0; i < ndevs; i++)
>                 ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);

Dumb question, why just "try"?

> 
>         return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
>                                     ndevs, NULL, 0, NULL);
> }
> EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
> 
> we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux
> game still)
> 
> 
> I like the enum / macro idea for cleanup, but I think that's a different
> problem I can address. The main question I have now is this:
> 
> The ocelot_regmap_from_resource now has nothing to do with the ocelot
> MFD system. It is generic. (If you listen carefully, you might hear me
> cheering)
> 
> I can keep this in linux/mfd/ocelot.h, but is this actually something
> that belongs elsewhere? platform? device? mfd-core?

Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(),
but I'm not 100% clear where it should sit. Platform devices are independent
of regmap, regmap is independent of platform devices, device core of both.

FWIW platform devices are always built-in and have no config option;
regmap is bool and is selected by others.

Logically, the melting pot of regmaps and platform devices is mfd.
However, it seems that include/linux/mfd/core.h only provides API for
mfd parent drivers, not children. So a new header would be needed?

Alternatively, you could just duplicate this logic in the drivers
(by the way, only spelling out the function name takes up half of the
implementation). How many times would it be needed? Felix DSA would roll
its own thing, as mentioned. I'm thinking, let it be open coded for now,
let's agree on the entire solution in terms of operations that are
actually being done, and we can revisit proper placement for this later.

> And yes, I like the idea of changing the driver to
> "ocelot_regmap_from_resource(pdev, GPIO, config);" from
> "ocelot_regmap_from_resource(pdev, 0, config);"

Sorry, I just realized we need to junk this idea with GPIO instead of 0.
Presenting the entire resource table to all peripherals implies that
there is no more than one single peripheral of each kind. This is not
true for the MDIO controllers, where the driver would need to know it
has to request the region corresponding to MIIM1 or MIIM2 according to
some crystal ball.

Colin Foster July 1, 2022, 5:18 p.m. UTC | #17

On Fri, Jul 01, 2022 at 04:21:28PM +0000, Vladimir Oltean wrote:
> On Thu, Jun 30, 2022 at 01:09:51PM -0700, Colin Foster wrote:
> > Ok... so I haven't yet changed any of the pinctrl / mdio drivers yet,
> > but I'm liking this:
> > 
> > static inline struct regmap *
> > ocelot_regmap_from_resource(struct platform_device *pdev, unsigned int index,
> >                             const struct regmap_config *config)
> > {
> >         struct device *dev = &pdev->dev;
> >         struct resource *res;
> >         u32 __iomem *regs;
> > 
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> >         if (res) {
> >                 regs = devm_ioremap_resource(dev, res);
> >                 if (IS_ERR(regs))
> >                         return ERR_CAST(regs);
> >                 return devm_regmap_init_mmio(dev, regs, config);
> >         }
> > 
> >         /*
> >          * Fall back to using REG and getting the resource from the parent
> >          * device, which is possible in an MFD configuration
> >          */
> >         res = platform_get_resource(pdev, IORESOURCE_REG, index);
> >         if (!res)
> >                 return ERR_PTR(-ENOENT);
> > 
> >         return (dev_get_regmap(dev->parent, res->name));
> 
> parentheses not needed around dev_get_regmap.

Oops.

While I have your ear: do I need to check for dev->parent == NULL before
calling dev_get_regmap? I see find_dr will call
(dev->parent)->devres_head... but specifically "does every device have a
valid parent?"

> 
> > }
> > 
> > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> > an inline helper function. And so long as ocelot_core_init does this:
> > 
> > static void ocelot_core_try_add_regmap(struct device *dev,
> >                                        const struct resource *res)
> > {
> >         if (!dev_get_regmap(dev, res->name)) {
> >                 ocelot_spi_init_regmap(dev, res);
> >         }
> > }
> > 
> > static void ocelot_core_try_add_regmaps(struct device *dev,
> >                                         const struct mfd_cell *cell)
> > {
> >         int i;
> > 
> >         for (i = 0; i < cell->num_resources; i++) {
> >                 ocelot_core_try_add_regmap(dev, &cell->resources[i]);
> >         }
> > }
> > 
> > int ocelot_core_init(struct device *dev)
> > {
> >         int i, ndevs;
> > 
> >         ndevs = ARRAY_SIZE(vsc7512_devs);
> > 
> >         for (i = 0; i < ndevs; i++)
> >                 ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> 
> Dumb question, why just "try"?

Because of this conditional:
> >         if (!dev_get_regmap(dev, res->name)) {
Don't add it if it is already there.


This might get interesting... The soc uses the HSIO regmap by way of
syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If
dev->parent has all the regmaps, what role does syscon play?

But that's a problem for another day...

> 
> > 
> >         return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs,
> >                                     ndevs, NULL, 0, NULL);
> > }
> > EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
> > 
> > we're good! (sorry about spaces / tabs... I have to up my mutt/vim/tmux
> > game still)
> > 
> > 
> > I like the enum / macro idea for cleanup, but I think that's a different
> > problem I can address. The main question I have now is this:
> > 
> > The ocelot_regmap_from_resource now has nothing to do with the ocelot
> > MFD system. It is generic. (If you listen carefully, you might hear me
> > cheering)
> > 
> > I can keep this in linux/mfd/ocelot.h, but is this actually something
> > that belongs elsewhere? platform? device? mfd-core?
> 
> Sounds like something which could be named devm_platform_get_regmap_from_resource_or_parent(),
> but I'm not 100% clear where it should sit. Platform devices are independent
> of regmap, regmap is independent of platform devices, device core of both.
> 
> FWIW platform devices are always built-in and have no config option;
> regmap is bool and is selected by others.
> 
> Logically, the melting pot of regmaps and platform devices is mfd.
> However, it seems that include/linux/mfd/core.h only provides API for
> mfd parent drivers, not children. So a new header would be needed?
> 
> Alternatively, you could just duplicate this logic in the drivers
> (by the way, only spelling out the function name takes up half of the
> implementation). How many times would it be needed? Felix DSA would roll
> its own thing, as mentioned. I'm thinking, let it be open coded for now,
> let's agree on the entire solution in terms of operations that are
> actually being done, and we can revisit proper placement for this later.

I came to the same conclusion. Hopefully I'll button up v12 today.

> 
> > And yes, I like the idea of changing the driver to
> > "ocelot_regmap_from_resource(pdev, GPIO, config);" from
> > "ocelot_regmap_from_resource(pdev, 0, config);"
> 
> Sorry, I just realized we need to junk this idea with GPIO instead of 0.
> Presenting the entire resource table to all peripherals implies that
> there is no more than one single peripheral of each kind. This is not
> true for the MDIO controllers, where the driver would need to know it
> has to request the region corresponding to MIIM1 or MIIM2 according to
> some crystal ball.

Vladimir Oltean July 2, 2022, 12:42 p.m. UTC | #18

On Fri, Jul 01, 2022 at 10:18:31AM -0700, Colin Foster wrote:
> While I have your ear: do I need to check for dev->parent == NULL before
> calling dev_get_regmap? I see find_dr will call
> (dev->parent)->devres_head... but specifically "does every device have a
> valid parent?"

While the technical answer is "no", the practical answer is "pretty much".
Platform devices sit at least on the "platform" bus created in drivers/base/platform.c,
and they are reparented to the "platform_bus" struct device named "platform"
within platform_device_add(), if they don't have a parent.

Additionally, for MMIO-controlled platform devices in Ocelot, these have
as parent a platform device probed by the drivers/bus/simple-pm-bus.c
driver on the "ahb@70000000" simple-bus OF node. That simple-bus
platform device has as parent the "platform_bus" device mentioned above.

So it's a pretty long way to the top in the device hierarchy, I wouldn't
concern myself too much with checking for NULL, unless you intend to
call dev_get_regmap() on a parent's parent's parent, or things like that.

> > > }
> > > 
> > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> > > an inline helper function. And so long as ocelot_core_init does this:
> > > 
> > > static void ocelot_core_try_add_regmap(struct device *dev,
> > >                                        const struct resource *res)
> > > {
> > >         if (!dev_get_regmap(dev, res->name)) {
> > >                 ocelot_spi_init_regmap(dev, res);
> > >         }
> > > }
> > > 
> > > static void ocelot_core_try_add_regmaps(struct device *dev,
> > >                                         const struct mfd_cell *cell)
> > > {
> > >         int i;
> > > 
> > >         for (i = 0; i < cell->num_resources; i++) {
> > >                 ocelot_core_try_add_regmap(dev, &cell->resources[i]);
> > >         }
> > > }
> > > 
> > > int ocelot_core_init(struct device *dev)
> > > {
> > >         int i, ndevs;
> > > 
> > >         ndevs = ARRAY_SIZE(vsc7512_devs);
> > > 
> > >         for (i = 0; i < ndevs; i++)
> > >                 ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> > 
> > Dumb question, why just "try"?
> 
> Because of this conditional:
> > >         if (!dev_get_regmap(dev, res->name)) {
> Don't add it if it is already there.

Hmm. So that's because you add regmaps iterating by the resource table
of each device. What if you keep a single resource table for regmap
creation purposes, and the device resource tables as separate?

> This might get interesting... The soc uses the HSIO regmap by way of
> syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If
> dev->parent has all the regmaps, what role does syscon play?
> 
> But that's a problem for another day...

Interesting question. I think part of the reason why syscon exists is to
not have OF nodes with overlapping address regions. In that sense, its
need does not go away here - I expect the layout of OF nodes beneath the
ocelot SPI device to be the same as their AHB variants. But in terms of
driver implementation, I don't know. Even if the OF nodes for your MFD
functions will contain all the regs that their AHB variants do, I'd
personally still be inclined to also hardcode those as resources in the
ocelot mfd parent driver and use those - case in which the OF regs will
more or less exist just as a formality. Maybe because the HSIO syscon is
already compatible with "simple-mfd", devices beneath it should just
probe. I haven't studied how syscon_node_to_regmap() behaves when the
syscon itself is probed as a MFD function. If that "just works", then
the phy-ocelot-serdes.c driver might not need to be modified.

Colin Foster July 2, 2022, 4:17 p.m. UTC | #19

On Sat, Jul 02, 2022 at 12:42:06PM +0000, Vladimir Oltean wrote:
> On Fri, Jul 01, 2022 at 10:18:31AM -0700, Colin Foster wrote:
> > While I have your ear: do I need to check for dev->parent == NULL before
> > calling dev_get_regmap? I see find_dr will call
> > (dev->parent)->devres_head... but specifically "does every device have a
> > valid parent?"
> 
> While the technical answer is "no", the practical answer is "pretty much".
> Platform devices sit at least on the "platform" bus created in drivers/base/platform.c,
> and they are reparented to the "platform_bus" struct device named "platform"
> within platform_device_add(), if they don't have a parent.
> 
> Additionally, for MMIO-controlled platform devices in Ocelot, these have
> as parent a platform device probed by the drivers/bus/simple-pm-bus.c
> driver on the "ahb@70000000" simple-bus OF node. That simple-bus
> platform device has as parent the "platform_bus" device mentioned above.
> 
> So it's a pretty long way to the top in the device hierarchy, I wouldn't
> concern myself too much with checking for NULL, unless you intend to
> call dev_get_regmap() on a parent's parent's parent, or things like that.

Thanks for the info. I have the NULL check in there, since I followed
the code and didn't see anything in device initialization that always
initializes parent. Maybe a default initializer would be
dev->parent = dev;

> 
> > > > }
> > > > 
> > > > So now there's no need for #if (CONFIG_MFD_OCELOT) - it can just remain
> > > > an inline helper function. And so long as ocelot_core_init does this:
> > > > 
> > > > static void ocelot_core_try_add_regmap(struct device *dev,
> > > >                                        const struct resource *res)
> > > > {
> > > >         if (!dev_get_regmap(dev, res->name)) {
> > > >                 ocelot_spi_init_regmap(dev, res);
> > > >         }
> > > > }
> > > > 
> > > > static void ocelot_core_try_add_regmaps(struct device *dev,
> > > >                                         const struct mfd_cell *cell)
> > > > {
> > > >         int i;
> > > > 
> > > >         for (i = 0; i < cell->num_resources; i++) {
> > > >                 ocelot_core_try_add_regmap(dev, &cell->resources[i]);
> > > >         }
> > > > }
> > > > 
> > > > int ocelot_core_init(struct device *dev)
> > > > {
> > > >         int i, ndevs;
> > > > 
> > > >         ndevs = ARRAY_SIZE(vsc7512_devs);
> > > > 
> > > >         for (i = 0; i < ndevs; i++)
> > > >                 ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
> > > 
> > > Dumb question, why just "try"?
> > 
> > Because of this conditional:
> > > >         if (!dev_get_regmap(dev, res->name)) {
> > Don't add it if it is already there.
> 
> Hmm. So that's because you add regmaps iterating by the resource table
> of each device. What if you keep a single resource table for regmap
> creation purposes, and the device resource tables as separate?

That would work - though it seems like it might be adding extra info
that isn't necessary. I'll take a look.

> 
> > This might get interesting... The soc uses the HSIO regmap by way of
> > syscon. Among other things, drivers/phy/mscc/phy-ocelot-serdes.c. If
> > dev->parent has all the regmaps, what role does syscon play?
> > 
> > But that's a problem for another day...
> 
> Interesting question. I think part of the reason why syscon exists is to
> not have OF nodes with overlapping address regions. In that sense, its
> need does not go away here - I expect the layout of OF nodes beneath the
> ocelot SPI device to be the same as their AHB variants. But in terms of
> driver implementation, I don't know. Even if the OF nodes for your MFD
> functions will contain all the regs that their AHB variants do, I'd
> personally still be inclined to also hardcode those as resources in the
> ocelot mfd parent driver and use those - case in which the OF regs will
> more or less exist just as a formality. Maybe because the HSIO syscon is
> already compatible with "simple-mfd", devices beneath it should just
> probe. I haven't studied how syscon_node_to_regmap() behaves when the
> syscon itself is probed as a MFD function. If that "just works", then
> the phy-ocelot-serdes.c driver might not need to be modified.

That'd be nice! When I looked into it a few months ago I came to the
conclusion that I'd need to implement "mscc,ocelot-hsio" but maybe
there's something I missed.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 36f0a205c54a..4d9ccec78f18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14413,6 +14413,11 @@  F:	net/dsa/tag_ocelot.c
 F:	net/dsa/tag_ocelot_8021q.c
 F:	tools/testing/selftests/drivers/net/ocelot/*
 
+OCELOT EXTERNAL SWITCH CONTROL
+M:	Colin Foster <[e-mail]>
+S:	Supported
+F:	include/linux/mfd/ocelot.h
+
 OCXL (Open Coherent Accelerator Processor Interface OpenCAPI) DRIVER
 M:	Frederic Barrat <[e-mail]>
 M:	Andrew Donnellan <[e-mail]>
diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
new file mode 100644
index 000000000000..5c95e4ee38a6
--- /dev/null
+++ b/include/linux/mfd/ocelot.h
@@ -0,0 +1,27 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/* Copyright 2022 Innovative Advantage Inc. */
+
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct resource;
+
+static inline struct regmap *
+ocelot_platform_init_regmap_from_resource(struct platform_device *pdev,
+					  unsigned int index,
+					  const struct regmap_config *config)
+{
+	struct resource *res;
+	u32 __iomem *regs;
+
+	regs = devm_platform_get_and_ioremap_resource(pdev, index, &res);
+
+	if (!res)
+		return ERR_PTR(-ENOENT);
+	else if (IS_ERR(regs))
+		return ERR_CAST(regs);
+	else
+		return devm_regmap_init_mmio(&pdev->dev, regs, config);
+}