Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

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

Hi Grant,

On Mar 19, 2013, at 7:18 PM, Grant Likely wrote:

> On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> Hi Grant,
>> 
>> On Mar 16, 2013, at 11:24 AM, Grant Likely wrote:
>> 
>>> On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>>>> Hi David,
>>>> 
>>>> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
>>>>> Ok.  Nonetheless it's not hard to avoid a recursive approach here.
>>>> 
>>>> How can I find the maximum phandle value of a subtree without using recursion.
>>>> Note that the whole function is just 6 lines long.
>>> 
>>> It's a failure in the existing kernel DT data structures. We need a hash
>>> lookup for the phandles to eliminate the search entirely. Then you'd be
>>> able to allocated new phandles on the fly easily and resolve phandles
>>> without searching the whole tree (which has always been horrible).
>>> 
>> 
>> Yes, it is pretty obvious that the in-kernel data structures are sub-optimal.
>> But I was not after modifying them, since that's a different kind of problem.
> 
> Think about it this way; fixing up that aspect of the data structure
> makes the job you're trying to do a lot easier. I don't feel bad about
> asking you to add a radix tree for phandle lookups when it makes your
> patches a whole lot better.  :-)
> 

Oh that's going to be fun... maybe.

>> Since we're having a 'sub-optimal' data structures, I'd like to point out that
>> the usage of of_find_by_name(), mostly by drivers trying to find a child
>> of their own node, works by a lucky accident of how the device nodes are instantiated
>> by the flat tree loader. Most of the use cases should be replaced by a call
>> to of_get_child_by_name() which does the right thing.
> 
> It is true. In fact, calling of_find_node_by_name() when using .dtb is
> most likely a bug since using node name to determine behaviour is
> strongly discouraged.

Maybe mark the function as deprecated then? Easier to get people to fix it.

> 
>> Fair enough, but be warned that phandle resolution the overlay feature is mostly useless.
>> 
>> In actual practice the amount of driver nodes that can be overlaid without a single case
>> of referencing phandles outside (or within) their own blob is close to zero.
> 
> That's not what I'm saying. I'm saying that (at least for now) we should
> require the overlay to already know the phandles from the parent and to
> refuse to load an overlay that defines phandles already in use in the
> base. Overlays do become usable at that point. A mechanism for phandle
> resolution so that conflicts can be found and resolved can be added
> as a feature enhancement. By splitting it out you'll be able to get the
> overlay feature merged even if we don't have agreement on the resolution
> mechanism yet.
> 

As long as we don't come up with something that's too difficult to use for non
kernel developers. I would hate to punt and push all the complexity of phandle
resolution to the driver/cape developer.

FWIW, the resolution mechanism we use on the beaglebone seems to work just fine,
and people seem to handle it just fine. The part that trips them is mostly how
to install the updated dtc compiler that's required.

> g.
> 

Regards

-- Pantelis