From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 23 Apr 2015 08:55:56 +0200 Subject: [U-Boot] [PATCH] fdt: Fix handling of paths with options in them In-Reply-To: References: <1429521217-27859-1-git-send-email-hdegoede@redhat.com> <1429521217-27859-2-git-send-email-hdegoede@redhat.com> <5535410A.3080901@redhat.com> Message-ID: <5538977C.3050400@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 22-04-15 19:20, Simon Glass wrote: > Hi Hans, > > On 20 April 2015 at 12:10, Hans de Goede wrote: >> Hi, >> >> On 20-04-15 17:39, Simon Glass wrote: >>> >>> Hi Hans, >>> >>> On 20 April 2015 at 03:13, Hans de Goede 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 >>>> --- >>>> 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 >