Hi Hans, On 23 April 2015 at 00:55, Hans de Goede <hdego...@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 <hdego...@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 <hdego...@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@01c00000/serial@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 <hdego...@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 <s...@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@01c00000/serial@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 >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
