[U-Boot] [PATCH] fdt: Fix fdtdec_get_addr_size() for 64-bit

[Simon Glass]

Hi,

On 27 July 2015 at 11:13, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 23 July 2015 at 10:51, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> From: Thierry Reding <treding at nvidia.com>
>>
>> Signed-off-by: Thierry Reding <treding at nvidia.com>
>> Signed-off-by: Tom Warren <twarren at nvidia.com>
>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>> ---
>> Simon,
>>
>> When Thierry first posted this patch, you responded:
>>
>>> > +       parent = fdt_parent_offset(blob, node);
>>>
>>> This function is very slow as it must scan the whole tree. Can we
>>> instead pass in the parent node?
>>
>> I don't think that's possible in general. This function is called from
>> fdtdec_get_addr(), and it's easy to find call sites of that function that
>> don't have the parent node available. IIRC, the first couple of example I
>> found scan the DT for a node with a certain compatible value, or look up
>> nodes via aliases, rather than being called while parsing the DT in a
>> top-down tree-like fashion, where the parent node is easily available. I
>> didn't do an exhaustive search after I found a few problematic cases.
>>
>>> Also, how about (in addition) a
>>> version of this function that works for devices? Like:
>>>
>>> device_get_addr_size(struct udevice *dev, ...)
>>>
>>> so that it can handle this for you.
>>
>> That sounds like a separate patch?
>
> Yes, but I think we should get it in there so that people don't start
> using this (wildly inefficient) function when they don't need to. I
> think by passing in the parent node we force people to think about the
> cost.
>
> Yes the driver model function can be a separate patch, but let's get
> it in at about the same time. We have dev_get_addr() so could have
> dev_get_addr_size().
>
>>
>> Equally, I see that struct udevice contains an of_offset field, but no
>> parent_of_offset or similar. There is a struct udevice *parent though;
>> is the struct udevice hierarchy guaranteed to 100% match the DT
>> hierarchy? I know this isn't necessarily guaranteed in Linux's device
>> model for example.
>
> Yes it is 100% guaranteed, so dev->parent->of_offset will do the right thing.
>
>>
>> As such, this patch seems OK to me as-is.
>> ---
>>  lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 36 insertions(+), 20 deletions(-)
>>

This patch has been applied. I'm going to post a revert of this patch.
Please can you take a look at the comments above? In particular this
function is called from dev_get_addr() which is a core driver model
function. It needs to be fast - and in fact dev_get_addr() already has
access to the parent node.

Also looking a bit closer this patch does a lot more than 'fix it for
64-bit'. A commit message would be useful to explain what problems it
is fixing, etc.

Another point is that fdt_addr_t and fdt_size_t are supposed to match
the address size used in the device tree. Is that not the case with
Tegra210?

>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index 9c6b3619da24..56e72eafaade 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -88,29 +88,45 @@ const char *fdtdec_get_compatible(enum fdt_compat_id id)
>>  fdt_addr_t fdtdec_get_addr_size(const void *blob, int node,
>>                 const char *prop_name, fdt_size_t *sizep)
>>  {
>> -       const fdt_addr_t *cell;
>> -       int len;
>> +       const fdt32_t *ptr, *end;
>> +       int parent, na, ns, len;
>> +       fdt_addr_t addr;
>>
>>         debug("%s: %s: ", __func__, prop_name);
>> -       cell = fdt_getprop(blob, node, prop_name, &len);
>> -       if (cell && ((!sizep && len == sizeof(fdt_addr_t)) ||
>> -                    len == sizeof(fdt_addr_t) * 2)) {
>> -               fdt_addr_t addr = fdt_addr_to_cpu(*cell);
>> -               if (sizep) {
>> -                       const fdt_size_t *size;
>> -
>> -                       size = (fdt_size_t *)((char *)cell +
>> -                                       sizeof(fdt_addr_t));
>> -                       *sizep = fdt_size_to_cpu(*size);
>> -                       debug("addr=%08lx, size=%llx\n",
>> -                             (ulong)addr, (u64)*sizep);
>> -               } else {
>> -                       debug("%08lx\n", (ulong)addr);
>> -               }
>> -               return addr;
>> +
>> +       parent = fdt_parent_offset(blob, node);
>> +       if (parent < 0) {
>> +               debug("(no parent found)\n");
>> +               return FDT_ADDR_T_NONE;
>>         }
>> -       debug("(not found)\n");
>> -       return FDT_ADDR_T_NONE;
>> +
>> +       na = fdt_address_cells(blob, parent);
>> +       ns = fdt_size_cells(blob, parent);
>> +
>> +       ptr = fdt_getprop(blob, node, prop_name, &len);
>> +       if (!ptr) {
>> +               debug("(not found)\n");
>> +               return FDT_ADDR_T_NONE;
>> +       }
>> +
>> +       end = ptr + len / sizeof(*ptr);
>> +
>> +       if (ptr + na + ns > end) {
>> +               debug("(not enough data: expected %d bytes, got %d bytes)\n",
>> +                     (na + ns) * 4, len);
>> +               return FDT_ADDR_T_NONE;
>> +       }
>> +
>> +       addr = fdtdec_get_number(ptr, na);
>> +
>> +       if (sizep) {
>> +               *sizep = fdtdec_get_number(ptr + na, ns);
>> +               debug("addr=%pa, size=%pa\n", &addr, sizep);
>> +       } else {
>> +               debug("%pa\n", &addr);
>> +       }
>> +
>> +       return addr;
>>  }
>>
>>  fdt_addr_t fdtdec_get_addr(const void *blob, int node,
>> --
>> 1.9.1
>>

Regards,
Simon