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

[Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>]

Hi David,

On 15.3.2016 01:27, David Gibson wrote:
> On Mon, Mar 14, 2016 at 10:10:58PM +0100, Michal Simek wrote:
>> On 13.3.2016 02:54, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On 16 February 2016 at 09:10, Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>>> ---
>>>>>>>
>>>>>>> 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().
>>
>> The funny part is that nothing is found means return 2. If this returns
>> something <0 then there is not a problem to try it with parents.
> 
> I don't have the full context of this thread, so it's a bit hard to be
> sure, but this doesn't look right from what I can see.  Two things to
> remember here:
> 
>   * #address-cells and #size-cells describe the format of addresses
>     for children of this node, not this node itself.  So if you're
>     looking to parse 'reg' for this node, you *always* need to look at
>     the parent, not just as a fallback.

ok that means that I should fix my code to find parent of current node
and then read address and size cells.

fdt - actual memory node
parent = fdt_parent_offset(fdt, nodeoffset);
address_cells = fdt_address_cells(parent, nodeoffset);
size_cells = fdt_size_cells(parent, nodeoffset);

>   * #address-cells and #size-cells are *not* inherited.  If they're
>     missing in a node, then the format for its children's addresses is
>     2 cell addresses and 2 cell sizes, it is *not* correct to look at
>     the next parent up for these properties.
> 

ok. And I expect that this is in spec.

Definitely thank you for your input it was very helpful.

Thanks,
Michal