Re: [RFC] FDT fix for 64 bit platforms

On 2011-10-15, at 18:48, Marcel Moolenaar wrote:

> 
> On Oct 15, 2011, at 9:33 AM, Jayachandran C. wrote:
> 
>> On Sat, Oct 15, 2011 at 8:43 PM, Nathan Whitehorn
>> <nwhitehorn_at_freebsd.org> wrote:
>>> On 10/15/11 01:12, Jayachandran C. wrote:
>>>> 
>>>> On Sat, Oct 15, 2011 at 2:01 AM, Nathan Whitehorn
>>>> <nwhitehorn_at_freebsd.org>  wrote:
>>>>> 
>>>>> On 10/14/11 14:10, Jayachandran C. wrote:
>>>>>> 
>>>>>> I'm planning commit this -CURRENT if there an no objections.
>>>>>> 
>>>>>> In the current implementation, phandle is used to store a pointer to
>>>>>> the location inside the device tree.  Since phandle_t is u32, this
>>>>>> will not work on 64 bit platforms. With this fix, the phandle is the
>>>>>> offset from the start of device tree pointer 'fdtp', which will be 32
>>>>>> bit.
>>>>>> 
>>>>>> Review or testing from device tree users will be welcome.
>>>>>> 
>>>>>> JC.
>>>>> 
>>>>> Why not use offsets into the FDT rather than full pointers? I believe
>>>>> having
>>>>> phandles greater than 32 bits violates the FDT spec, and declaring that
>>>>> the
>>>>> FDT can't itself be larger than 4 GB seems reasonable.
>>>> 
>>>> I am actually using the offset from the beginning of FDT (fdtp) as
>>>> phandle.  I cannot use the usual fdt offset (after off_dt_struct) as
>>>> phandle, because in that case offset of 0 is valid, but phandle 0
>>>> should not be valid.
>>> 
>>> Why shouldn't phandle 0 be valid? The invalid phandle is -1. This is one of
>>> the problems with our existing FDT code -- it makes all kinds of wrong
>>> assumptions like this about IEEE 1275.
>> 
>> Well, the existing FDT code returns 0 as the invalid handle and I do
>> not want to change that in this commit.
>> 
>> If the return value is really wrong, we will need a bigger exercise to
>> change the return value and fix any callers which are affected by that
>> change.
> 
> It should be fairly easy to change the base from fdtp to the "usual"
> fdt offset, so let me propose the following:
> 
> 1.  JC commits what he has and based on the current code.
> 2.  We get all the facts on the table. I say this because I
>    read different and contradictory things (0 being an
>    invalid phandle in OF, negative phandles exist, etc).
> 3.  We change the implementation, if such is warranted, in
>    a separate effort.
> 
> The point really is that 0 is an invalid phandle right now,
> right or wrong, and JCs changes are based on that. I see no
> problem proceeding on the path we're on, while we discuss
> what's the correct implementation and whether or not we
> should have a course change...
> 
> Thoughts?

The patch looks fine to me, but we didn't have a chance yet to test it on any PPC/ARM system, have you, Marcel? Regarding the phandle validity I need to recall the context as this was a while back and I don't quite remember all constraints and motivations.

Rafal