| Message ID | 1429521217-27859-2-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
Hi, On 20-04-15 11: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@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 <hdegoede@redhat.com> > --- > arch/arm/dts/sun7i-a20-pcduino3.dts | 2 +- > lib/libfdt/fdt_ro.c | 25 ++++++++++++++++++++++--- > 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 { Ugh, this does not belong in here, this was just here to test the second code path of fdt_path_offset handles paths with options correctly. Will drop from my personal tree. Regards, Hans > 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; > >
Hi Hans, On 20 April 2015 at 03:13, Hans de Goede <hdegoede@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 <hdegoede@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? > 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
Hi, On 20-04-15 17:39, Simon Glass wrote: > Hi Hans, > > On 20 April 2015 at 03:13, Hans de Goede <hdegoede@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 <hdegoede@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: 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 >
Hi Hans, On 20 April 2015 at 12:10, Hans de Goede <hdegoede@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@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 <hdegoede@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. > > 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
Hi, On 22-04-15 19:20, Simon Glass wrote: > Hi Hans, > > On 20 April 2015 at 12:10, Hans de Goede <hdegoede@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@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 <hdegoede@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@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 >
Hi Hans, On 23 April 2015 at 00:55, Hans de Goede <hdegoede@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@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@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 <hdegoede@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@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 >> >
Hi Hans, On 23 April 2015 at 10:15, Simon Glass <sjg@chromium.org> wrote: > Hi Hans, > > On 23 April 2015 at 00:55, Hans de Goede <hdegoede@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@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@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 <hdegoede@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@chromium.org> > Can I just check if you are OK to send this upstream? We don't need to wait for it to be accepted, just want to know it will be sent. Regards, Simon
Hi, On 24-04-15 14:42, Simon Glass wrote: > Hi Hans, > > On 23 April 2015 at 10:15, Simon Glass <sjg@chromium.org> wrote: >> Hi Hans, >> >> On 23 April 2015 at 00:55, Hans de Goede <hdegoede@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@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@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 <hdegoede@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@chromium.org> >> > > Can I just check if you are OK to send this upstream? We don't need to > wait for it to be accepted, just want to know it will be sent. Yes I will submit this upstream. Regards, Hans
On 24 April 2015 at 07:34, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 24-04-15 14:42, Simon Glass wrote: >> >> Hi Hans, >> >> On 23 April 2015 at 10:15, Simon Glass <sjg@chromium.org> wrote: >>> >>> Hi Hans, >>> >>> On 23 April 2015 at 00:55, Hans de Goede <hdegoede@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@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@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 <hdegoede@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@chromium.org> >>> >> >> Can I just check if you are OK to send this upstream? We don't need to >> wait for it to be accepted, just want to know it will be sent. > > > Yes I will submit this upstream. Applied to u-boot-fdt, thanks! - Simon
+Scott Hi Hans, On 28 April 2015 at 17:29, Simon Glass <sjg@chromium.org> wrote: > > On 24 April 2015 at 07:34, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > > > > > On 24-04-15 14:42, Simon Glass wrote: > >> > >> Hi Hans, > >> > >> On 23 April 2015 at 10:15, Simon Glass <sjg@chromium.org> wrote: > >>> > >>> Hi Hans, > >>> > >>> On 23 April 2015 at 00:55, Hans de Goede <hdegoede@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@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@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 <hdegoede@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@chromium.org> > >>> > >> > >> Can I just check if you are OK to send this upstream? We don't need to > >> wait for it to be accepted, just want to know it will be sent. > > > > > > Yes I will submit this upstream. > > Applied to u-boot-fdt, thanks! It looks like this was rejected upstream. If so can you please create a patch for fdtdec.c (I assume) to move your code into there? Regards, Simon
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;
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 <hdegoede@redhat.com> --- arch/arm/dts/sun7i-a20-pcduino3.dts | 2 +- lib/libfdt/fdt_ro.c | 25 ++++++++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-)