[U-Boot] [PATCH] fdt: Fix handling of paths with options in them
Hans de Goede
hdegoede at redhat.com
Thu Apr 23 08:55:56 CEST 2015
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 ?
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