Date:   Tue, 9 Jun 2020 12:01:36 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        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: [RFC] MFD's relationship with Device Tree (OF)

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?

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

Powered by blists - more mailing lists