Date:   Wed, 29 Jun 2022 13:39:05 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Terry Bowman <terry.bowman@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Arnd Bergmann <arnd@...nel.org>
Subject: Re: [PATCH v11 net-next 1/9] mfd: ocelot: add helper to get regmap
 from a resource

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 <vladimir.oltean@....com> 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/20211119224313.2803941-4-colin.foster@in-advantage.com/
> 
> In fact you implemented just this (Dec. 6, 2021):
> https://patchwork.kernel.org/project/netdevbpf/patch/20211203211611.946658-1-colin.foster@in-advantage.com/#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/20211218214954.109755-2-colin.foster@in-advantage.com/
> 
> 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/20211218214954.109755-3-colin.foster@in-advantage.com/
> 
> 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.

Powered by blists - more mailing lists