[U-Boot] [PATCH] fdt: Fix handling of paths with options in them

[Simon Glass]

Hi Hans,

On 20 April 2015 at 12:10, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
> On 20-04-15 17:39, Simon Glass wrote:
>>
>> Hi Hans,
>>
>> On 20 April 2015 at 03:13, Hans de Goede <hdegoede at redhat.com> wrote:
>>>
>>> After syncing the sunxi dts files with the upstream kernel dm/fdt sunxi
>>> builds would no longer boot.
>>>
>>> The problem is that stdout-path is now set like this in the upstream dts
>>> files: stdout-path = "serial0:115200n8". The use of options in of-paths,
>>> either after an alias name, or after a full path, e.g. stdout-path =
>>> "/soc at 01c00000/serial at 01c28000:115200", is standard of usage, but
>>> something
>>> which the u-boot dts code so far did not handle.
>>>
>>> This commit fixes this, adding support for both path formats.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>   arch/arm/dts/sun7i-a20-pcduino3.dts |  2 +-
>>>   lib/libfdt/fdt_ro.c                 | 25 ++++++++++++++++++++++---
>>
>>
>> I haven't looked. but is this change in dtc upstream or just in the
>> kernel?
>
>
> This is just a change in the dts files shipped with the kernel not in dtc,
> the dts files for sunxi used to not set stdout-path, and you patched in
> a stdout-path setting for u-boot:

In that case, can we change this in the fdt support /fdtdec code,
instead of making a change to libfdt that will never go upstream?

If that doesn't work or is too painful, then we should take this patch.

>
> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=1a81cf8399675056beef5e76be8a9380d88c4ebf
>
> +       chosen {
> +               stdout-path = &uart0;
> +       };
>
> But now the upstream dts contains a stdout-path itself, but like this;
>
>         alias {
>                 serial0 = &uart0;
>         };
>
>         chosen {
>                 stdout-path = "serial0:115200n8";
>         };
>
> And the current u-boot dts parsing does not grok this due to it not
> recognizing
> the : in there.
>
> Where as the kernel has:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/of/base.c#n713
>
> static struct device_node *__of_find_node_by_path(struct device_node
> *parent,
>                                                 const char *path)
> {
>         struct device_node *child;
>         int len;
>
>         len = strcspn(path, "/:");
>         if (!len)
>                 return NULL;
>
>         __for_each_child_of_node(parent, child) {
>                 const char *name = strrchr(child->full_name, '/');
>                 if (WARN(!name, "malformed device_node %s\n",
> child->full_name))
>                         continue;
>                 name++;
>                 if (strncmp(path, name, len) == 0 && (strlen(name) == len))
>                         return child;
>         }
>         return NULL;
> }
>
> Where the strcspn surves the same purpose as the fdt_path_next_seperator my
> patch
> introduces find the basename to match for stopping at the first occurence of
> either a '/' or a ':' char.
>
> Regards,
>
> Hans
>
>
>
>
>>
>>>   2 files changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/sun7i-a20-pcduino3.dts
>>> b/arch/arm/dts/sun7i-a20-pcduino3.dts
>>> index cd05267..624abf2 100644
>>> --- a/arch/arm/dts/sun7i-a20-pcduino3.dts
>>> +++ b/arch/arm/dts/sun7i-a20-pcduino3.dts
>>> @@ -64,7 +64,7 @@
>>>          };
>>>
>>>          chosen {
>>> -               stdout-path = "serial0:115200n8";
>>> +               stdout-path = "/soc at 01c00000/serial at 01c28000:115200";
>>>          };
>>>
>>>          leds {
>>> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
>>> index 03733e5..44fc0aa 100644
>>> --- a/lib/libfdt/fdt_ro.c
>>> +++ b/lib/libfdt/fdt_ro.c
>>> @@ -113,6 +113,25 @@ int fdt_subnode_offset(const void *fdt, int
>>> parentoffset,
>>>          return fdt_subnode_offset_namelen(fdt, parentoffset, name,
>>> strlen(name));
>>>   }
>>>
>>> +/*
>>> + * Find the next of path seperator, note we need to search for both '/'
>>> and ':'
>>> + * and then take the first one so that we do the rigth thing for e.g.
>>> + * "foo/bar:option" and "bar:option/otheroption", both of which happen,
>>> so
>>> + * first searching for either ':' or '/' does not work.
>>> + */
>>> +static const char *fdt_path_next_seperator(const char *path)
>>> +{
>>> +       const char *sep1 = strchr(path, '/');
>>> +       const char *sep2 = strchr(path, ':');
>>> +
>>> +       if (sep1 && sep2)
>>> +               return (sep1 < sep2) ? sep1 : sep2;
>>> +       else if (sep1)
>>> +               return sep1;
>>> +       else
>>> +               return sep2;
>>> +}
>>> +
>>>   int fdt_path_offset(const void *fdt, const char *path)
>>>   {
>>>          const char *end = path + strlen(path);
>>> @@ -123,7 +142,7 @@ int fdt_path_offset(const void *fdt, const char
>>> *path)
>>>
>>>          /* see if we have an alias */
>>>          if (*path != '/') {
>>> -               const char *q = strchr(path, '/');
>>> +               const char *q = fdt_path_next_seperator(path);
>>>
>>>                  if (!q)
>>>                          q = end;
>>> @@ -141,9 +160,9 @@ int fdt_path_offset(const void *fdt, const char
>>> *path)
>>>
>>>                  while (*p == '/')
>>>                          p++;
>>> -               if (! *p)
>>> +               if (*p == '\0' || *p == ':')
>>>                          return offset;
>>> -               q = strchr(p, '/');
>>> +               q = fdt_path_next_seperator(p);
>>>                  if (! q)
>>>                          q = end;
>>>
>>> --
>>> 2.3.5
>>>
>>
>> Regards,
>> Simon
>>
>

Regards,
Simon