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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 22-04-15 19:20, Simon Glass wrote:
> Hi Hans,
>
> On 20 April 2015 at 12:10, Hans de Goede <hdegoede@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@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@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.

Actually I started with fixing this the fdtdev level, but then I noticed
that the kernel does this at the of_find_node_by_path level, so if we
fix this for stdout-path only (which a fdtdec patch would do) then we may
get bit by this again later.

Also fixing it at the fdtdec level means adding a strdup + error checking
since then we need to pass a truncated (options removed) copy of the path
to fdt_path_offset(), while with the current patch we can keep using
read only access to the fdt.

So I've a slight preference for going this way. Why would libfdt upstream not
take this patch ?

Regards,

Hans

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