Re: [PATCH 0/3] capebus moving omap_devices to mach-omap2

[Pantelis Antoniou <panto@antoniou-consulting.com>]

Hi Grant,

On Nov 5, 2012, at 8:10 PM, Grant Likely wrote:

> On Mon, Nov 5, 2012 at 3:37 PM, Pantelis Antoniou
> <panto@antoniou-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@antoniou-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.
> 

Avoiding mobs of angry users with broken bootloader/kernel combos is a good
thing...

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

Yes, this will not survive modification of the fragment file.
For the use case I'm targeting the DT fragment is going to be quite
minimal, a few tens of nodes at the most. Modification of a single
fragment file is not expected.

Modifications to the base tree, those would work just fine. The base
tree by definition will not have external references.

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

I believe this is considerably more complex, without fixing the 
fragility problem.

> 
> 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@0x10005000 {
>                .readonly;
>                phandle = <0x1234>;
>        };
> 
>        peripheral-bus {
>                .readonly;
>                i2c@20001000 {
>                        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.

I see what you mean. It will work, provided you 'link' (I don't know what
the proper terminology is) against the correct board dts file.
> 
> 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.

My worry is that this format is dependent on linking against the board
DTS file. One of the ideas thrown around here was that it might make
sense to store the DTB fragment in the EEPROM of the device. 

In that case you have a OS independent hardware description, which can 
be even used even by the bootloader to access devices it knows not about
at compile time.

Other than that, I have no other objections.

> 
> g.

Regards

-- Pantelis