[RFC] MFD's relationship with Device Tree (OF)

[RFC] MFD's relationship with Device Tree (OF)

[Lee Jones]

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

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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Andy Shevchenko]

On Tue, Jun 9, 2020 at 2:01 PM Lee Jones <lee.jones@linaro.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

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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Lee Jones]

On Tue, 09 Jun 2020, Andy Shevchenko wrote:

> On Tue, Jun 9, 2020 at 2:01 PM Lee Jones <lee.jones@linaro.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.

Is that the properties framework you are trying to replace?

Is that different to placing additional attributes into pdata?

Using my clock example above, how would one allocate a DT based clock
to a child device using properties?

-- 
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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Rob Herring]

On Tue, Jun 9, 2020 at 5:01 AM Lee Jones <lee.jones@linaro.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 {

Just a note, unit-address implies there is a 'reg' property. Why
that's important below.

>                         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.

Presumably anything current already works. If you had the above
example already, requiring updating the DT to make it work seems fine.

>  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.

Multiple instances of something implies you have some way to address
them and 'reg' is what defines the address of something. 0,1,2,etc.
looks suspiciously like just some kernel defined indexes, but if
that's how things are defined in the datasheet I'm okay with them.

The one wrinkle is there's only one address space at one level, so
gpio@0, gpio@1, pwm@0, pwm@1, etc. doesn't really work (well, it
works, but having overlapping addresses is not good practice). Either
we relax that in this case or we can add another level to group nodes.

>
> 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.

Just to pick on ab8500, it should have had 'reg' property IMO. The
'bank' is clearly a h/w property and how you address each sub-device.

>
> 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.

I don't think we should use order.

>
> 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.

Yes, we should use 'reg' whenever possible. If we don't have 'reg',
then you shouldn't have a unit-address either and you can simply match
on the node name (standard DT driver matching is with compatible,
device_type, and node name (w/o unit-address)). We've generally been
doing 'classname-N' when there's no 'reg' to do 'classname@N'.
Matching on 'classname-N' would work with node name matching as only
unit-addresses are stripped.

Rob

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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Lee Jones]

On Tue, 09 Jun 2020, Rob Herring wrote:

Thanks for replying Rob.

> On Tue, Jun 9, 2020 at 5:01 AM Lee Jones <lee.jones@linaro.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 {
> 
> Just a note, unit-address implies there is a 'reg' property. Why
> that's important below.

Right.  This is just an example to express the problem more easily.

> >                         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.
> 
> Presumably anything current already works. If you had the above
> example already, requiring updating the DT to make it work seems fine.

"works" it a matter of opinion.  Some instances "work" out of luck.
Some "work" because they have been worked-around or an alternative
implementation sought.

For instance, 'ab8500-pwm' only has 1 DT node present, yet 3 devices
are registered via MFD.  Since MFD matches devices with DT nodes
containing identical compatible strings using first-found, all PWM
instances are assigned a pointer to the 1 existing DT node.
Fortunately in this case they all share the same clock, so it "works",
but that's clearly not the intended implementation.

> >  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.
> 
> Multiple instances of something implies you have some way to address
> them and 'reg' is what defines the address of something. 0,1,2,etc.
> looks suspiciously like just some kernel defined indexes, but if
> that's how things are defined in the datasheet I'm okay with them.
> 
> The one wrinkle is there's only one address space at one level, so
> gpio@0, gpio@1, pwm@0, pwm@1, etc. doesn't really work (well, it
> works, but having overlapping addresses is not good practice). Either
> we relax that in this case or we can add another level to group nodes.

All agreed.  Sounds promising.

> > 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.
> 
> Just to pick on ab8500, it should have had 'reg' property IMO. The
> 'bank' is clearly a h/w property and how you address each sub-device.
> 
> >
> > 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.
> 
> I don't think we should use order.

If it's always possible to have a 'reg' property then we won't need
to.

> > 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.
> 
> Yes, we should use 'reg' whenever possible. If we don't have 'reg',
> then you shouldn't have a unit-address either and you can simply match
> on the node name (standard DT driver matching is with compatible,
> device_type, and node name (w/o unit-address)). We've generally been
> doing 'classname-N' when there's no 'reg' to do 'classname@N'.
> Matching on 'classname-N' would work with node name matching as only
> unit-addresses are stripped.

Let me try and knock something up.

I'll get back to you when it's done.

-- 
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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Michael Walle]

Hi Rob,

Am 2020-06-10 00:03, schrieb Rob Herring:
[..]
> Yes, we should use 'reg' whenever possible. If we don't have 'reg',
> then you shouldn't have a unit-address either and you can simply match
> on the node name (standard DT driver matching is with compatible,
> device_type, and node name (w/o unit-address)). We've generally been
> doing 'classname-N' when there's no 'reg' to do 'classname@N'.
> Matching on 'classname-N' would work with node name matching as only
> unit-addresses are stripped.

This still keeps me thinking. Shouldn't we allow the (MFD!) device
driver creator to choose between "classname@N" and "classname-N".
In most cases N might not be made up, but it is arbitrarily chosen;
for example you've chosen the bank for the ab8500 reg. It is not
a defined entity, like an I2C address if your parent is an I2C bus,
or a SPI chip select, or the memory address in case of MMIO. Instead
the device driver creator just chooses some "random" property from
the datasheet; another device creator might have chosen another
property. Wouldn't it make more sense, to just say this MFD provides
N pwm devices and the subnodes are matching based on pwm-{0,1..N-1}?
That would also be the logical consequence of the current MFD sub
device to OF node matching code, which just supports N=1.

-michael

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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Frank Rowand]

+Frank (me)

On 2020-06-22 16:03, Michael Walle wrote:
> Am 2020-06-14 12:26, schrieb Michael Walle:
>> Hi Rob,
>>
>> Am 2020-06-10 00:03, schrieb Rob Herring:
>> [..]
>>> Yes, we should use 'reg' whenever possible. If we don't have 'reg',
>>> then you shouldn't have a unit-address either and you can simply match
>>> on the node name (standard DT driver matching is with compatible,
>>> device_type, and node name (w/o unit-address)). We've generally been
>>> doing 'classname-N' when there's no 'reg' to do 'classname@N'.
>>> Matching on 'classname-N' would work with node name matching as only
>>> unit-addresses are stripped.
>>
>> This still keeps me thinking. Shouldn't we allow the (MFD!) device
>> driver creator to choose between "classname@N" and "classname-N".
>> In most cases N might not be made up, but it is arbitrarily chosen;
>> for example you've chosen the bank for the ab8500 reg. It is not
>> a defined entity, like an I2C address if your parent is an I2C bus,
>> or a SPI chip select, or the memory address in case of MMIO. Instead
>> the device driver creator just chooses some "random" property from
>> the datasheet; another device creator might have chosen another
>> property. Wouldn't it make more sense, to just say this MFD provides
>> N pwm devices and the subnodes are matching based on pwm-{0,1..N-1}?
>> That would also be the logical consequence of the current MFD sub
>> device to OF node matching code, which just supports N=1.
>>
> 
> Rob? Lee?
> 
> -michael


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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Lee Jones]

On Wed, 24 Jun 2020, Frank Rowand wrote:
> On 2020-06-22 16:03, Michael Walle wrote:
> > Am 2020-06-14 12:26, schrieb Michael Walle:
> >> Hi Rob,
> >>
> >> Am 2020-06-10 00:03, schrieb Rob Herring:
> >> [..]
> >>> Yes, we should use 'reg' whenever possible. If we don't have 'reg',
> >>> then you shouldn't have a unit-address either and you can simply match
> >>> on the node name (standard DT driver matching is with compatible,
> >>> device_type, and node name (w/o unit-address)). We've generally been
> >>> doing 'classname-N' when there's no 'reg' to do 'classname@N'.
> >>> Matching on 'classname-N' would work with node name matching as only
> >>> unit-addresses are stripped.
> >>
> >> This still keeps me thinking. Shouldn't we allow the (MFD!) device
> >> driver creator to choose between "classname@N" and "classname-N".
> >> In most cases N might not be made up, but it is arbitrarily chosen;
> >> for example you've chosen the bank for the ab8500 reg. It is not
> >> a defined entity, like an I2C address if your parent is an I2C bus,
> >> or a SPI chip select, or the memory address in case of MMIO. Instead
> >> the device driver creator just chooses some "random" property from
> >> the datasheet; another device creator might have chosen another
> >> property. Wouldn't it make more sense, to just say this MFD provides
> >> N pwm devices and the subnodes are matching based on pwm-{0,1..N-1}?
> >> That would also be the logical consequence of the current MFD sub
> >> device to OF node matching code, which just supports N=1.

It's funny.  You reiterate things like "arbitrarily chosen" and
"randomly chosen from the datasheet" but yet your suggestion is just
that.  The only difference is that you wish to place the numerical
differentiator in the node name, rather than the reg property.  Worse
still, you are suggesting that you wish to just enumerate them off
sequentially from some arbitrary base (likely 0).

I don't know of many cases off, the top of my head at least, where
this is a problem.  As you've mentioned, in the case of the AB8500,
the bank is used which is semantically how the devices are actually
addressed.  It's not random, it's physical.

How are the identical devices addressed/identified/differentiated
from each other on your H/W?  You must have a way of saying "I want
PWM X to act in a different way from PWM Y".  What is 'X' and 'Y' in
your datasheet?

-- 
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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Michael Walle]

Am 2020-06-25 08:13, schrieb Lee Jones:
> On Wed, 24 Jun 2020, Frank Rowand wrote:
>> On 2020-06-22 16:03, Michael Walle wrote:
>> > Am 2020-06-14 12:26, schrieb Michael Walle:
>> >> Hi Rob,
>> >>
>> >> Am 2020-06-10 00:03, schrieb Rob Herring:
>> >> [..]
>> >>> Yes, we should use 'reg' whenever possible. If we don't have 'reg',
>> >>> then you shouldn't have a unit-address either and you can simply match
>> >>> on the node name (standard DT driver matching is with compatible,
>> >>> device_type, and node name (w/o unit-address)). We've generally been
>> >>> doing 'classname-N' when there's no 'reg' to do 'classname@N'.
>> >>> Matching on 'classname-N' would work with node name matching as only
>> >>> unit-addresses are stripped.
>> >>
>> >> This still keeps me thinking. Shouldn't we allow the (MFD!) device
>> >> driver creator to choose between "classname@N" and "classname-N".
>> >> In most cases N might not be made up, but it is arbitrarily chosen;
>> >> for example you've chosen the bank for the ab8500 reg. It is not
>> >> a defined entity, like an I2C address if your parent is an I2C bus,
>> >> or a SPI chip select, or the memory address in case of MMIO. Instead
>> >> the device driver creator just chooses some "random" property from
>> >> the datasheet; another device creator might have chosen another
>> >> property. Wouldn't it make more sense, to just say this MFD provides
>> >> N pwm devices and the subnodes are matching based on pwm-{0,1..N-1}?
>> >> That would also be the logical consequence of the current MFD sub
>> >> device to OF node matching code, which just supports N=1.
> 
> It's funny.  You reiterate things like "arbitrarily chosen" and
> "randomly chosen from the datasheet" but yet your suggestion is just
> that. The only difference is that you wish to place the numerical
> differentiator in the node name, rather than the reg property.

Correct, because from my understand, the N in the nodename-N form is
exactly that: arbitrarily chosen, or sequentially numbered. Doesn't
matter. Using the reg property instead, you would have to have
unique values; which are normally the addresses or some other kind
of property related to the parent bus. See below.

> Worse
> still, you are suggesting that you wish to just enumerate them off
> sequentially from some arbitrary base (likely 0).

Yes I suggested that, but you could choose any other number. I was
under the impression that the nodename-N is somehwhat sequentially
numbered.

> I don't know of many cases off, the top of my head at least, where
> this is a problem.  As you've mentioned, in the case of the AB8500,
> the bank is used which is semantically how the devices are actually
> addressed.  It's not random, it's physical.

You didn't get my point. The choice to use the bank address was random.
Why did you choose that in particular? Why didn't you choose the whole
register offset of the first register which belongs to that PWM, e.g.
0x1060? That is what I mean, it is arbitrary chosen by someone, what
the reg property contains. In the MFD case it doesn't matter, because
it is just there for node matching. Worse, it might not even be unique,
i.e. there might be multiple GPIOs and you choose the also the bank of
that which might overlap with the bank of the PWM and then you have
pwm@60 and gpio@60. Again, Rob suggested to relax that rule, but why
was this rule there in the first place? To my knowlegde, all other
uses of the reg field are precisely defined.

So my reasoning is: if it is up to the developer what to choose to
differentiate between the instances and you cannot guarantee that it
is unique among all the subdevices, why not use the nodename-N form.
Also, what do we gain if we have the reg property if you don't know
how to use it? It is just there to match an OF node to an mfd_cell.

> How are the identical devices addressed/identified/differentiated
> from each other on your H/W?  You must have a way of saying "I want
> PWM X to act in a different way from PWM Y".  What is 'X' and 'Y' in
> your datasheet?

There is no official datasheet, so I could say anything now which
would be in my favor, for example, that there is a table labeling
the PWMs sequentially starting from 0 ;) Would it be accepted then?
I really don't know, but it emphasize my point that the value is
"arbitrary chosen" (sorry to repeat myself..).
Yes there is some kind of internal datasheet or internal design
document and for the current implementation (as I said, this is
a more generic board controller where you can mix and match the
individual components per board, or even per flavor of the board).
But this document states that there are two PWMs, it tells you
the offsets which you have to use to if you want to access it
via I2C and what the PWMs are for, i.e. one is for the LCD
backlight and one is for general purpose, connected to the edge
connector of the processor module.

Sorry if I keep discussing this, but it looks wrong to me to have
the internal offset in both the MFD driver and in the device tree.
And I tried to start a discussion back in March [1] before
submitting my MFD patch series, but nobody replied back then.

-michael

[1] 
https://lore.kernel.org/linux-devicetree/0e3e8204ab992d75aa07fc36af7e4ab2@walle.cc/

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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Frank Rowand]

Hi Lee,

Please add me to the distribution list for future versions of this.

-Frank

On 2020-06-09 06:01, Lee Jones 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?
> 


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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Lee Jones]

On Thu, 11 Jun 2020, Frank Rowand wrote:

> Please add me to the distribution list for future versions of this.

The solution patch is already on v2.

https://lore.kernel.org/lkml/20200611191002.2256570-1-lee.jones@linaro.org/

I'll bounce it to you.

-- 
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

Re: [RFC] MFD's relationship with Device Tree (OF)

[Frank Rowand]

Hi Lee,

On 2020-06-09 06:01, Lee Jones 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

You can not count on order.  There are no guarantees of node ordering in
compilation, modification by bootloader, and within Linux kernel data
structures.

-Frank

> 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?
> 


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