[U-Boot] [PATCH] fdt: Try to read #address-cells/size-cells from parent

[Simon Glass]

Hi Michal,

On 16 February 2016 at 09:10, Michal Simek <michal.simek at xilinx.com> wrote:
> Hi Simon,
>
> On 16.2.2016 17:00, Simon Glass wrote:
>> Hi Michal,
>>
>> On 15 February 2016 at 02:58, Michal Simek <michal.simek at xilinx.com> wrote:
>>> Hi Simon,
>>>
>>> On 10.2.2016 13:04, Michal Simek wrote:
>>>> Read #address-cells and #size-cells from parent if they are not present in
>>>> current node.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>> ---
>>>>
>>>> I have code which read information about memory for zynqmp but memory
>>>> node most of the time doesn't contain #address/size-cells which are
>>>> present in parent node.
>>>> That's why let's try to read it from parent.
>>>>
>>>> Also I think that we shouldn't return 2 if property is not found because
>>>> it has side effect on 32bit systems with #address/size-cells = <1>;
>>>>
>>>> ---
>>>>  lib/libfdt/fdt_addresses.c | 18 ++++++++++++++----
>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/libfdt/fdt_addresses.c b/lib/libfdt/fdt_addresses.c
>>>> index 76054d98e5fd..b164d0988079 100644
>>>> --- a/lib/libfdt/fdt_addresses.c
>>>> +++ b/lib/libfdt/fdt_addresses.c
>>>> @@ -19,10 +19,15 @@ int fdt_address_cells(const void *fdt, int nodeoffset)
>>>>       const fdt32_t *ac;
>>>>       int val;
>>>>       int len;
>>>> +     int parent;
>>>>
>>>>       ac = fdt_getprop(fdt, nodeoffset, "#address-cells", &len);
>>>> -     if (!ac)
>>>> -             return 2;
>>>> +     if (!ac) {
>>>> +             parent = fdt_parent_offset(fdt, nodeoffset);
>>>> +             ac = fdt_getprop(fdt, parent, "#address-cells", &len);
>>>> +             if (!ac)
>>>> +                     return 2;
>>>> +     }
>>>>
>>>>       if (len != sizeof(*ac))
>>>>               return -FDT_ERR_BADNCELLS;
>>>> @@ -39,10 +44,15 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
>>>>       const fdt32_t *sc;
>>>>       int val;
>>>>       int len;
>>>> +     int parent;
>>>>
>>>>       sc = fdt_getprop(fdt, nodeoffset, "#size-cells", &len);
>>>> -     if (!sc)
>>>> -             return 2;
>>>> +     if (!sc) {
>>>> +             parent = fdt_parent_offset(fdt, nodeoffset);
>>>> +             sc = fdt_getprop(fdt, parent, "#size-cells", &len);
>>>> +             if (!sc)
>>>> +                     return 2;
>>>> +     }
>>>>
>>>>       if (len != sizeof(*sc))
>>>>               return -FDT_ERR_BADNCELLS;
>>>>
>>>
>>> Simon: Any comment?
>>
>> It seems risky to change the behaviour here. Also fdt_parent_offset() is slow.
>>
>> Can you point me to the binding / example DT that you are trying to parse?
>
> Look at dram_init(), etc.
> https://github.com/Xilinx/u-boot-xlnx/blob/master/board/xilinx/zynqmp/zynqmp.c
>
> fdt_get_reg() is calling fdt_size_cells()
>
>
> And this is DTS fragment.
>         #address-cells = <2>;
>         #size-cells = <1>;
>
>         memory {
>                 device_type = "memory";
>                 reg = <0x0 0x0 0x80000000>, <0x8 0x00000000 0x80000000>;
>         };
>
> Code is in memory node I need to work with and asking for size-cells.
> Current code returns 2 instead of error and the rest of code just works
> with size = 2 which is incorrect for this setup.
>
> I have already changed size-cells = 2 in our repo because I need to
> support for more than 4GB memory anyway but this should point to the
> problem in that generic functions.

I think this should go in a higher-level function. I very much doubt
that this patch would be accepted upstream.

Can you find the caller and make it call this function again (for the
parent) when no nothing is found on the first call? Hopefully this
caller will have access to the parent node and will not need to call
fdt_parent_offset().

>
> Thanks,
> Michal
>

Regards,
Simon