Re: [RFC] FDT fix for 64 bit platforms

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?

-- 
Marcel Moolenaar
marcel_at_xcllnt.net