Date:   Tue, 9 Jun 2020 17:19:30 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Michael Walle <michael@...le.cc>, Rob Herring <robh+dt@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Robin Murphy <robin.murphy@....com>,
        GregKroah-Hartmangregkh@...uxfoundation.org
Subject: Re: [RFC] MFD's relationship with Device Tree (OF)

On Tue, Jun 9, 2020 at 2:01 PM Lee Jones <lee.jones@...aro.org> wrote:
>
> Good morning,
>
> After a number of reports/queries surrounding a known long-term issue
> in the MFD core, including the submission of a couple of attempted
> solutions, I've decided to finally tackle this one myself.
>
> 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.
>
> Let me give you an example.
>
> I have knocked up an example 'parent' and 'child' device driver.  The
> parent utilises the MFD API to register 3 identical children, each
> controlled by the same driver.  This happens a lot.  Fortunately, in
> the majority of cases, the OF nodes are also totally identical, but
> what if you wish to configure one of the child devices with different
> attributes or resources supplied via Device Tree, like a clock?  This
> is currently impossible.
>
> Here is the Device Tree representation for the 1 parent and the 3
> child (sub) devices described above:
>
>         parent {
>                 compatible = "mfd,of-test-parent";
>
>                 child@0 {
>                         compatible = "mfd,of-test-child";
>                         clocks = <&clock 0>;
>                 };
>
>                 child@1 {
>                         compatible = "mfd,of-test-child";
>                         clocks = <&clock 1>;
>                 };
>
>                 child@2 {
>                         compatible = "mfd,of-test-child";
>                         clocks = <&clock 2>;
>                 };
>         };
>
> This is how we register those devices from MFD:
>
> 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")
> };
>
> ... which we pass into mfd_add_devices() for processing.
>
> In an ideal world.  The devices with the platform_id; 0, 1 and 2 would
> be matched up to Device Tree nodes; child@0, child@1 and child@2
> respectively.  Instead all 3 devices will be allocated a pointer to
> child@0's OF node, which is obviously not correct.
>
> This is how it looks when each of the child devices are probed:
>
>  [0.708287] mfd-of-test-parent mfd_of_test: Registering 3 devices
>  [...]
>  [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@0
>  [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@0
>  [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@0
>
> "Why is it when I change child 2's clock rate, it also changes 0's?"
>
> Whoops!
>
> So in order to fix this, we need to make MFD more-cleverer!
>
> However, this is not so simple.  There are some rules we should abide
> by (I use "should" intentionally here, as something might just have to
> give):
>
>  a) Since Device Tree is designed to describe hardware, inserting
>     arbitrary properties into DT is forbidden.  This precludes things
>     we would ordinarily be able to match on, like 'id' or 'name'.
>  b) As an extension to a) DTs should also be OS agnostic, so
>     properties like 'mfd-device', 'mfd-order' etc are also not
>     not suitable for inclusion.
>  c) The final solution should ideally be capable of supporting both
>     newly defined and current trees (without retroactive edits)
>     alike.
>  d) Existing properties could be used, but not abused.  For example,
>     one of my suggestions (see below) is to use the 'reg' property.
>     This is fine in principle but loading 'reg' with arbitrary values
>     (such as; 0, 1, 2 ... x) which 1) clearly do not have anything to
>     do with registers and 2) would be meaningless in other OSes/
>     implementations, just to serve our purpose, is to be interpreted
>     as an abuse.
>
> Proposal 1:
>
> As mentioned above, my initial thoughts were to use the 'reg' property
> to match an MFD cell entry with the correct DT node.  However, not
> all Device Tree nodes have 'reg' properties.  Particularly true in the
> case of MFD, where memory resources are usually shared with the parent
> via Regmap, or (as in the case of the ab8500) the MFD handles all
> register transactions via its own API.
>
> Proposal 2:
>
> If we can't guarantee that all DT nodes will have at least one
> property in common to be used for matching and we're prevented from
> supplying additional, potentially bespoke properties, then we must
> seek an alternative procedure.
>
> It should be possible to match based on order.  However, the developer
> would have to guarantee that the order in which the child devices are
> presented to the MFD API are in exactly the same order as they are
> represented in the Device Tree.  The obvious draw-back to this
> strategy is that it's potentially very fragile.
>
> Current Proposal:
>
> How about a collection of Proposal 1 and Proposal 2?  First we could
> attempt a match on the 'reg' property.  Then, if that fails, we would
> use the fragile-but-its-all-we-have Proposal 2 as the fall-back.
>
> Thoughts?

Just a side note, have you considered software nodes on the picture?
You can add properties or additional references to the existing
(firmware) nodes.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists