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

[Simon Glass]

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(-)
>
> 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