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 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=1a81cf8399675056beef5e76be8a...

•   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...

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=1a81cf8399675056beef5e76be8a...

•   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...

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