Re: [SPAM] [PATCH 1/1] mfd: Make a best effort attempt to match devices with the correct of_nodes

[Lee Jones <lee.jones@linaro.org>]

On Thu, 11 Jun 2020, Michael Walle wrote:

> Am 2020-06-11 12:57, schrieb Lee Jones:
> > On Thu, 11 Jun 2020, Michael Walle wrote:
> > 
> > > Am 2020-06-11 10:56, schrieb Lee Jones:
> > > > Currently, when a child platform device (sometimes referred to as a
> > > > sub-device) is registered via the Multi-Functional Device (MFD) API,
> > > > the framework attempts to match the newly registered platform device
> > > > with its associated Device Tree (OF) node.  Until now, the device has
> > > > been allocated the first node found with an identical OF compatible
> > > > string.  Unfortunately, if there are, say for example '3' devices
> > > > which are to be handled by the same driver and therefore have the same
> > > > compatible string, each of them will be allocated a pointer to the
> > > > *first* node.
> > > >
> > > > An example Device Tree entry might look like this:
> > > >
> > > >   mfd_of_test {
> > > >           compatible = "mfd,of-test-parent";
> > > >           #address-cells = <0x02>;
> > > >           #size-cells = <0x02>;
> > > >
> > > >           child@aaaaaaaaaaaaaaaa {
> > > >                   compatible = "mfd,of-test-child";
> > > >                   reg = <0xaaaaaaaa 0xaaaaaaaa 0 0x11>,
> > > >                         <0xbbbbbbbb 0xbbbbbbbb 0 0x22>;
> > > >           };
> > > >
> > > >           child@cccccccc {
> > > >                   compatible = "mfd,of-test-child";
> > > >                   reg = <0x00000000 0xcccccccc 0 0x33>;
> > > >           };
> > > >
> > > >           child@dddddddd00000000 {
> > > >                   compatible = "mfd,of-test-child";
> > > >                   reg = <0xdddddddd 0x00000000 0 0x44>;
> > > >           };
> > > >   };
> > > >
> > > > When used with example sub-device registration like this:
> > > >
> > > >   static const struct mfd_cell mfd_of_test_cell[] = {
> > > >         OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 0,
> > > > "mfd,of-test-child"),
> > > >         OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1,
> > > > "mfd,of-test-child"),
> > > >         OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 2,
> > > > "mfd,of-test-child")
> > > >   };
> > > >
> > > > ... the current implementation will result in all devices being
> > > > allocated
> > > > the first OF node found containing a matching compatible string:
> > > >
> > > >   [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform
> > > > device: 0
> > > >   [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node:
> > > > child@aaaaaaaaaaaaaaaa
> > > >   [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform
> > > > device: 1
> > > >   [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node:
> > > > child@aaaaaaaaaaaaaaaa
> > > >   [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform
> > > > device: 2
> > > >   [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node:
> > > > child@aaaaaaaaaaaaaaaa
> > > >
> > > > After this patch each device will be allocated a unique OF node:
> > > >
> > > >   [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform
> > > > device: 0
> > > >   [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node:
> > > > child@aaaaaaaaaaaaaaaa
> > > >   [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform
> > > > device: 1
> > > >   [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node:
> > > > child@cccccccc
> > > >   [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform
> > > > device: 2
> > > >   [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node:
> > > > child@dddddddd00000000
> > > >
> > > > Which is fine if all OF nodes are identical.  However if we wish to
> > > > apply an attribute to particular device, we really need to ensure the
> > > > correct OF node will be associated with the device containing the
> > > > correct address.  We accomplish this by matching the device's address
> > > > expressed in DT with one provided during sub-device registration.
> > > > Like this:
> > > >
> > > >   static const struct mfd_cell mfd_of_test_cell[] = {
> > > >         OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 1,
> > > > "mfd,of-test-child", 0xdddddddd00000000),
> > > >         OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> > > > "mfd,of-test-child", 0xaaaaaaaaaaaaaaaa),
> > > >         OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> > > > "mfd,of-test-child", 0x00000000cccccccc)
> > > >   };
> > > >
> > > > This will ensure a specific device (designated here using the
> > > > platform_ids; 1, 2 and 3) is matched with a particular OF node:
> > > >
> > > >   [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform
> > > > device: 0
> > > >   [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node:
> > > > child@dddddddd00000000
> > > >   [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform
> > > > device: 1
> > > >   [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node:
> > > > child@aaaaaaaaaaaaaaaa
> > > >   [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform
> > > > device: 2
> > > >   [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node:
> > > > child@cccccccc
> > > >
> > > > This implementation is still not infallible, hence the mention of
> > > > "best effort" in the commit subject.  Since we have not *insisted* on
> > > > the existence of 'reg' properties (in some scenarios they just do not
> > > > make sense) and no device currently uses the new 'of_reg' attribute,
> > > > we have to make an on-the-fly judgement call whether to associate the
> > > > OF node anyway.  Which we do in cases where parent drivers haven't
> > > > specified a particular OF node to match to.  So there is a *slight*
> > > > possibility of the following result (note: the implementation here is
> > > > convoluted, but it shows you one means by which this process can
> > > > still break):
> > > >
> > > >   /*
> > > >    * First entry will match to the first OF node with matching
> > > > compatible
> > > >    * Second will fail, since the first took its OF node and is no
> > > > longer available
> > > >    * Third will succeed
> > > >    */
> > > >   static const struct mfd_cell mfd_of_test_cell[] = {
> > > >         OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1,
> > > > "mfd,of-test-child"),
> > > > 	OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> > > > "mfd,of-test-child", 0xaaaaaaaaaaaaaaaa),
> > > >         OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> > > > "mfd,of-test-child", 0x00000000cccccccc)
> > > 
> > > Why would anyone do that? Mix and match OF_MFD_CELL() and
> > > OF_MFD_CELL_REG()
> > > on the same compatible string?
> > 
> > See above:
> > 
> >   "(note: the implementation here is convoluted, but it shows you one
> >    means by which this process can still break)"
> 
> Yes, but my point was, do we really need to to something about it - or
> like Andy would put it, do we expect to shoot ourselves in the foot or not.
> If I understand it correctly, this list handling is just there to make
> sure we are using the mfd_cells correctly.

Yes, I want to ensure devices aren't sharing OF nodes.

No, I won't want to report every failure use-case.

> > > Wouldn't it be easier to check that there is
> > > no OF_MFD_CELL() when OF_MFD_CELL_REG() is used, instead of keeping
> > > a global
> > > list?
> > 
> > See below:
> > 
> >   "We could code around this with some pre-parsing semantics, but the
> >    added complexity required to cover each and every corner-case is not
> >    justified.  Merely patching the current failing (via this patch) is
> >    already working with some pretty small corner-cases.  Other issues
> >    should be patched in the parent drivers which can be achieved simply
> >    by implementing OF_MFD_CELL_REG()."
> > 
> > If the API isn't used properly, things will break.
> 
> So we could also just skip the list handling, can't we?

Why would we though?  It's lightweight and trivial.

In an ideal world, I'd also code-up protection against all the other
possible misdemeanours, but that's complex and heavyweight, thus not
worth the time, energy and RAM.

> > We can't prevent every erroneous corner-case.

[...]

> > > > +
> > > > +	/* We only care about each node's first defined address */
> > > > +	reg = of_get_address(np, 0, NULL, NULL);
> > > 
> > > Does of_get_address() work with any (internal) addresses?
> > 
> > What is an internal address?
> 
> I meant an address (offset) local to a I2C/SPI device for example.

Yes, it returns anything which is in the 'reg' property.

> If you look at of_get_address(), it does more than just reading the "reg"
> property. I'm not that deep into that, and unfortunately, there is no
> documentation. But I guess its fine, because it should get the "defaul".

No, that's pretty much all it does.  It can also return the 'size' and
'flag' attributes, but we don't need those for what we're doing.

<OFF_TOPIC>

> I actually have a more conceptual question. If a device just consists of
> OF_MFD_CELL_REG(), where do you draw the line between being a real MFD and
> some kind of simple-mfd.

Now we're talking cross-discussion, however I will answer.

If a device uses the MFD API, it's a legitimate MFD.

> Having this patch in place, I'd rewrite my driver to be an actual MFD using
> mfd_cells, instead of of_platform_populate. Reasons for this:
>  - IORESOURCE_REG is possible, which is the correct resource type for the
>    sub device drivers in my case, IMHO.
>  - There is just one interrupt line in hardware, which is connected to
>    the device. So this should get into the parent device node and should
>    be distributed by the MFD driver to the sub device drivers. In my
>    latest patch this was worked around by having an "interrupts"
>    property in each child device which needed this one interrupt.
>  - there is only one instance, namely the i2c driver for the parent node,
>    which does the regmap_init() and provide it to its children.
> Although that would mean duplicating the base register offset in the
> mfd_cell and in the reg property of the sub nodes (but that should be
> the case for most OF_MFD_CELL_REG() users).

It's up to you.  I will review what's in front of me.

</OFF_TOPIC>
-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel