* [U-Boot] [PATCH 0/1] fdt: Fix handling of paths with options in them
@ 2015-04-20 9:13 Hans de Goede
2015-04-20 9:13 ` [U-Boot] [PATCH] " Hans de Goede
0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2015-04-20 9:13 UTC (permalink / raw)
To: u-boot
Hi all,
Here is a small but important fdt parsing fix. See the patch / commit
message for details.
I'm wondering how to best take this upstream. This is a dependency for the
sunxi dm work I'm doing so I will happily take this upstream (once reviewed)
through the sunxi tree.
Regards,
Hans
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-20 9:13 [U-Boot] [PATCH 0/1] fdt: Fix handling of paths with options in them Hans de Goede
@ 2015-04-20 9:13 ` Hans de Goede
2015-04-20 14:58 ` Hans de Goede
2015-04-20 15:39 ` Simon Glass
0 siblings, 2 replies; 12+ messages in thread
From: Hans de Goede @ 2015-04-20 9:13 UTC (permalink / raw)
To: u-boot
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@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 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-20 9:13 ` [U-Boot] [PATCH] " Hans de Goede
@ 2015-04-20 14:58 ` Hans de Goede
2015-04-20 15:39 ` Simon Glass
1 sibling, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2015-04-20 14:58 UTC (permalink / raw)
To: u-boot
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 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@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 at 01c00000/serial at 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;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-20 9:13 ` [U-Boot] [PATCH] " Hans de Goede
2015-04-20 14:58 ` Hans de Goede
@ 2015-04-20 15:39 ` Simon Glass
2015-04-20 18:10 ` Hans de Goede
1 sibling, 1 reply; 12+ messages in thread
From: Simon Glass @ 2015-04-20 15:39 UTC (permalink / raw)
To: u-boot
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 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@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 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-20 15:39 ` Simon Glass
@ 2015-04-20 18:10 ` Hans de Goede
2015-04-22 17:20 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2015-04-20 18:10 UTC (permalink / raw)
To: u-boot
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 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@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 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-20 18:10 ` Hans de Goede
@ 2015-04-22 17:20 ` Simon Glass
2015-04-23 6:55 ` Hans de Goede
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2015-04-22 17:20 UTC (permalink / raw)
To: u-boot
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 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@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 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-22 17:20 ` Simon Glass
@ 2015-04-23 6:55 ` Hans de Goede
2015-04-23 16:15 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2015-04-23 6:55 UTC (permalink / raw)
To: u-boot
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 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@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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-23 6:55 ` Hans de Goede
@ 2015-04-23 16:15 ` Simon Glass
2015-04-24 12:42 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2015-04-23 16:15 UTC (permalink / raw)
To: u-boot
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 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@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 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
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-23 16:15 ` Simon Glass
@ 2015-04-24 12:42 ` Simon Glass
2015-04-24 13:34 ` Hans de Goede
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2015-04-24 12:42 UTC (permalink / raw)
To: u-boot
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 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@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-24 12:42 ` Simon Glass
@ 2015-04-24 13:34 ` Hans de Goede
2015-04-28 23:29 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Hans de Goede @ 2015-04-24 13:34 UTC (permalink / raw)
To: u-boot
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 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@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-24 13:34 ` Hans de Goede
@ 2015-04-28 23:29 ` Simon Glass
2015-07-14 19:49 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2015-04-28 23:29 UTC (permalink / raw)
To: u-boot
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 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@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] fdt: Fix handling of paths with options in them
2015-04-28 23:29 ` Simon Glass
@ 2015-07-14 19:49 ` Simon Glass
0 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2015-07-14 19:49 UTC (permalink / raw)
To: u-boot
+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 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@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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-07-14 19:49 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-20 9:13 [U-Boot] [PATCH 0/1] fdt: Fix handling of paths with options in them Hans de Goede
2015-04-20 9:13 ` [U-Boot] [PATCH] " Hans de Goede
2015-04-20 14:58 ` Hans de Goede
2015-04-20 15:39 ` Simon Glass
2015-04-20 18:10 ` Hans de Goede
2015-04-22 17:20 ` Simon Glass
2015-04-23 6:55 ` Hans de Goede
2015-04-23 16:15 ` Simon Glass
2015-04-24 12:42 ` Simon Glass
2015-04-24 13:34 ` Hans de Goede
2015-04-28 23:29 ` Simon Glass
2015-07-14 19:49 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.