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

Simon Glass sjg at chromium.org
Thu Apr 23 18:15:35 CEST 2015


Hi Hans,

On 23 April 2015 at 00:55, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 22-04-15 19:20, Simon Glass wrote:
>>
>> 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.
>
>
> 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 ?

I'm not sure, but please go ahead and send it upstream. Thanks for the
explanation, I'll pick this up.

Acked-by: Simon Glass <sjg at chromium.org>

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


More information about the U-Boot mailing list