Message-ID: <CACxGe6v9cyybV4+5tQJ-DG8qdyEHfsLa7t+-3CTL+97Q9msrFw@mail.gmail.com>
Date:	Mon, 5 Nov 2012 19:10:27 +0000
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Pantelis Antoniou <panto@...oniou-consulting.com>
Cc:	Alan Cox <alan@...rguk.ukuu.org.uk>, Russ Dill <Russ.Dill@...com>,
	Felipe Balbi <balbi@...com>, Benoit Cousson <b-cousson@...com>,
	Tony Lindgren <tony@...mide.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Matt Porter <mporter@...com>, linux-omap@...r.kernel.org,
	Kevin Hilman <khilman@...com>, Paul Walmsley <paul@...an.com>,
	devicetree-discuss@...ts.ozlabs.org,
	Rob Herring <robherring2@...il.com>
Subject: Re: [PATCH 0/3] capebus moving omap_devices to mach-omap2

On Mon, Nov 5, 2012 at 3:37 PM, Pantelis Antoniou
<panto@...oniou-consulting.com> wrote:
> Hi Grant,
>
> On Nov 5, 2012, at 1:37 AM, Grant Likely wrote:
>
>> On Fri, Nov 2, 2012 at 12:32 PM, Pantelis Antoniou
>> <panto@...oniou-consulting.com> wrote:
>>> The i2c2 alias cannot be resolved at compile time; there has to be
>>>
>>> a) A DT object format where unresolved aliases (symbols) are tracked
>>> b) A runtime DT linker that will resolve the alias, and will insert the
>>>   i2c2-devices child nodes as children in the i2c2 node.
>>> c) A method to trigger platform device creation for the child nodes just
>>>   inserted.
>>>
>>> DT core changes aren't bound to be easily accepted, so without having a clear
>>> signal from the DT maintainers that they would consider such a method people
>>> are just hesitant to go down this road.
>>
>> I do agree and will accept such a method.
>>
>> g.
>
> Understood.
>
> I'll think about the issues and come up with a design for the format, but
> let's talk about this in the open for a while.

Agreed. I'm planning to post my first draft/RFC tonight.

> I don't want to modify the DTB format, in order to avoid impacting any other
> DT users.

Ditto. It should be a direct extension.

> So would something like this work for you?
[...]
> The dt-resolution node, can be appended to the dt fragment contents and
> can be generated by the compiler.
>
> Upon injection of the DT fragment the in-kernel loader can perform
> symbol resolution and replace the phandles with the in-kernel values.
>
> I know there's an aliases node, but I don't know if it's wise to modify it.
>
> What do you think?

If I'm reading your intent correctly, your primary worry is the
problem of matching up phandles from the base dt with (potentially
different) phandle values in the overlay. This solution solves it by
encoding all of the phandle locations as offsets from the start of the
file into the dt-resolution node. Correct? My concern with this
approach is that it is fragile. Any changes to the fragment file, such
as to add extra properties or nodes, or even to repack the tree will
break all the offsets; probably silently.

It would be less fragile if each property containing phandles had some
kind of .<prop>-phandle-offsets companion property that listed the
phandles that need to be fixed up as an offset to the beginning of
only that properties' data. Better, in that modifying the tree won't
break the links, but I still worry that it is fragile and possibly too
complex.

However, the problem is based on the assumption that phandles are
effectively random and could change ever time the tree is recompiled.
Well, what if they weren't? What if dtc generated phandles using a
hash of the node full name so that the value changed rarely? Also,
what if the format was oriented around detecting if the phandles don't
match instead of fixing things up? The solution becomes a lot simpler
if the parser only has to verify that the referenced phandles already
exist at the right path in the tree.

Something like this:

/include/ "base-file.dts"  /* include might not be the right syntax here */
&i2c0 {         /* i2c0 resolved by label */
        touchpad@10 {
                compatible = "acme,touchpad";
                reg = <0x10>;
                interrupt-parent = <&intc>;
                interrupts = <100>;
        };
};

And the generated overlay dtb may look like this:

/ {
        .readonly;
        interrupt-controller@...0005000 {
                .readonly;
                phandle = <0x1234>;
        };

        peripheral-bus {
                .readonly;
                i2c@...01000 {
                        touchpad@10 {
                                compatible = "acme,touchpad";
                                reg = <0x10>;
                                interrupt-parent = <0x1234>;
                                interrupts = <100>;
                        };
                };
        };
};

Which is obviously missing a bunch of information for the rest of the
system, but contains enough data to ensure that the path to the touchpad
node is valid and that the phandle has not changed.

This handles many of the use cases, but it assumes that an overlay is
board specific. If it ever is required to support multiple base boards
with a single overlay file then there is a problem. The .dtb overlays
generated in this manor cannot handle different phandles or nodes that
are in a different place. On the other hand, the overlay source files
should have no problem being compiled for multiple targets, so maybe
it isn't an issue. Plus if dtc is installed on the target, then the
live tree from /proc can be used as the reference when compiling the
overlay.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists