[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