Support specifying console options (like with console=ttyXN,<options>)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
This patch also modifies of_find_node_by_path() to match only the
portion of the path before a ':'.
Signed-off-by: Leif Lindholm <[email protected]>
---
drivers/of/base.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..89c6b33 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
struct device_node *of_chosen;
struct device_node *of_aliases;
struct device_node *of_stdout;
+static char *of_stdout_options;
struct kset *of_kset;
@@ -703,6 +704,8 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
if (!len)
return NULL;
+ len = strchrnul(path, ':') - path;
+
__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))
@@ -747,6 +750,8 @@ struct device_node *of_find_node_by_path(const char *path)
if (!of_aliases)
return NULL;
+ len = strchrnul(path, ':') - path;
+
for_each_property_of_node(of_aliases, pp) {
if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
np = of_find_node_by_path(pp->value);
@@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
name = of_get_property(of_chosen, "linux,stdout-path", NULL);
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
- if (name)
+ if (name) {
of_stdout = of_find_node_by_path(name);
+ of_stdout_options = strchr(name, ':');
+ if (of_stdout_options != NULL)
+ of_stdout_options++;
+ }
}
if (!of_aliases)
@@ -1957,7 +1966,7 @@ bool of_console_check(struct device_node *dn, char *name, int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index, NULL);
+ return !add_preferred_console(name, index, of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
1.9.1
On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
>
> Example:
> stdout-path = "uart0:115200";
>
> This patch also modifies of_find_node_by_path() to match only the
> portion of the path before a ':'.
Hi Leif
These appears to somewhat conform to ePAPR, which says:
A string that specifies the full path to the node representing
the device to be used for boot console output. If the character
":" is present in the value it terminates the path.
So you can put any random junk after the :. However, are we going to
have backward/forward compatibility problems, and problems with
bootloaders? The current kernel code does not look for the :. So a new
DT blob on an old kernel will not work so well.
More worrying, barebox does not support the : either. So there is a
danger your bootloader suddenly goes silent after a dt blob update.
Would it be safer to add a new property in chosen?
Andrew
On Tue, Nov 25, 2014 at 12:00:16AM +0100, Andrew Lunn wrote:
> On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
> >
> > This patch also modifies of_find_node_by_path() to match only the
> > portion of the path before a ':'.
>
> Hi Leif
>
> These appears to somewhat conform to ePAPR, which says:
>
> A string that specifies the full path to the node representing
> the device to be used for boot console output. If the character
> ":" is present in the value it terminates the path.
>
> So you can put any random junk after the :. However, are we going to
> have backward/forward compatibility problems, and problems with
> bootloaders? The current kernel code does not look for the :. So a new
> DT blob on an old kernel will not work so well.
I _think_ this will be less of a problem in practice than it could be
in theory.
The reason this is needed is that at least several platforms have
different baudrate settings in firmware than the default provided by
the kernel for their UART. As a result, stdout-path becomes
semi-useless; the only thing it gives you is the ability to get
earlycon output without specifying a specific device (and then the
console turns to garbage once non-earlycon is enabled).
Hence, a platform that gets along happily today without the ability to
specify console options in stdout-path would have no pressing need to
add it to its dt. This should permit at least a very long, soft,
transition path.
(console= on kernel command line continues to override stdout-path.)
> More worrying, barebox does not support the : either. So there is a
> danger your bootloader suddenly goes silent after a dt blob update.
_That_ would be most unfortunate.
> Would it be safer to add a new property in chosen?
My preference would be not, given the above, but the important thing
is to get the functionality in so we get rid of mandatory console=.
/
Leif
On Tue, Nov 25, 2014 at 12:00:16AM +0100, Andrew Lunn wrote:
> On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
> >
> > This patch also modifies of_find_node_by_path() to match only the
> > portion of the path before a ':'.
>
> Hi Leif
>
> These appears to somewhat conform to ePAPR, which says:
>
> A string that specifies the full path to the node representing
> the device to be used for boot console output. If the character
> ":" is present in the value it terminates the path.
>
> So you can put any random junk after the :. However, are we going to
> have backward/forward compatibility problems, and problems with
> bootloaders? The current kernel code does not look for the :. So a new
> DT blob on an old kernel will not work so well.
>
> More worrying, barebox does not support the : either. So there is a
> danger your bootloader suddenly goes silent after a dt blob update.
Since in barebox we use the upstream Kernel devicetrees it will be under
our control when they arrive, so we can add support for that ':'
before the new devicetrees arrive.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Leif,
On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
>
> Example:
> stdout-path = "uart0:115200";
I would very much like to be able to use this -- it will allow
distributions to boot on a board without having to know _anything_ about
the console UART until userspace is up, which would make it possible to
have a completely generic installer image, without requiring the
platform to provide bootargs.
My only concern is that this conflates the set of kernel command line
options for the UART wit the DT binding for it. I don't know how good
people are at keeping those options stable, and I know they are
currently not documented -- we would need to add a stdout-path options
section to relevant UART bindings.
Thanks,
Mark.
>
> This patch also modifies of_find_node_by_path() to match only the
> portion of the path before a ':'.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> drivers/of/base.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 3823edf..89c6b33 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
> struct device_node *of_chosen;
> struct device_node *of_aliases;
> struct device_node *of_stdout;
> +static char *of_stdout_options;
>
> struct kset *of_kset;
>
> @@ -703,6 +704,8 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> if (!len)
> return NULL;
>
> + len = strchrnul(path, ':') - path;
> +
> __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))
> @@ -747,6 +750,8 @@ struct device_node *of_find_node_by_path(const char *path)
> if (!of_aliases)
> return NULL;
>
> + len = strchrnul(path, ':') - path;
> +
> for_each_property_of_node(of_aliases, pp) {
> if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> np = of_find_node_by_path(pp->value);
> @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> name = of_get_property(of_chosen, "linux,stdout-path", NULL);
> if (IS_ENABLED(CONFIG_PPC) && !name)
> name = of_get_property(of_aliases, "stdout", NULL);
> - if (name)
> + if (name) {
> of_stdout = of_find_node_by_path(name);
> + of_stdout_options = strchr(name, ':');
> + if (of_stdout_options != NULL)
> + of_stdout_options++;
> + }
> }
>
> if (!of_aliases)
> @@ -1957,7 +1966,7 @@ bool of_console_check(struct device_node *dn, char *name, int index)
> {
> if (!dn || dn != of_stdout || console_set_on_cmdline)
> return false;
> - return !add_preferred_console(name, index, NULL);
> + return !add_preferred_console(name, index, of_stdout_options);
> }
> EXPORT_SYMBOL_GPL(of_console_check);
>
> --
> 1.9.1
>
>
On Tue, Nov 25, 2014 at 10:35:04AM +0000, Mark Rutland wrote:
> On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
>
> I would very much like to be able to use this -- it will allow
> distributions to boot on a board without having to know _anything_ about
> the console UART until userspace is up, which would make it possible to
> have a completely generic installer image, without requiring the
> platform to provide bootargs.
>
> My only concern is that this conflates the set of kernel command line
> options for the UART wit the DT binding for it. I don't know how good
> people are at keeping those options stable, and I know they are
> currently not documented -- we would need to add a stdout-path options
> section to relevant UART bindings.
I don't disagree.
Current options are fairly well defined and stable, at least for any
driver that uses uart_parse_options() (documented in
Documentation/serial/driver).
>From drivers/tty/serial/serial_core.c:
* uart_parse_options decodes a string containing the serial console
* options. The format of the string is <baud><parity><bits><flow>,
* eg: 115200n8r
/
Leif
>
> >
> > This patch also modifies of_find_node_by_path() to match only the
> > portion of the path before a ':'.
> >
> > Signed-off-by: Leif Lindholm <[email protected]>
> > ---
> > drivers/of/base.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 3823edf..89c6b33 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
> > struct device_node *of_chosen;
> > struct device_node *of_aliases;
> > struct device_node *of_stdout;
> > +static char *of_stdout_options;
> >
> > struct kset *of_kset;
> >
> > @@ -703,6 +704,8 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> > if (!len)
> > return NULL;
> >
> > + len = strchrnul(path, ':') - path;
> > +
> > __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))
> > @@ -747,6 +750,8 @@ struct device_node *of_find_node_by_path(const char *path)
> > if (!of_aliases)
> > return NULL;
> >
> > + len = strchrnul(path, ':') - path;
> > +
> > for_each_property_of_node(of_aliases, pp) {
> > if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> > np = of_find_node_by_path(pp->value);
> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> > name = of_get_property(of_chosen, "linux,stdout-path", NULL);
> > if (IS_ENABLED(CONFIG_PPC) && !name)
> > name = of_get_property(of_aliases, "stdout", NULL);
> > - if (name)
> > + if (name) {
> > of_stdout = of_find_node_by_path(name);
> > + of_stdout_options = strchr(name, ':');
> > + if (of_stdout_options != NULL)
> > + of_stdout_options++;
> > + }
> > }
> >
> > if (!of_aliases)
> > @@ -1957,7 +1966,7 @@ bool of_console_check(struct device_node *dn, char *name, int index)
> > {
> > if (!dn || dn != of_stdout || console_set_on_cmdline)
> > return false;
> > - return !add_preferred_console(name, index, NULL);
> > + return !add_preferred_console(name, index, of_stdout_options);
> > }
> > EXPORT_SYMBOL_GPL(of_console_check);
> >
> > --
> > 1.9.1
> >
> >
On Tue, 2014-11-25 at 11:17 +0000, Leif Lindholm wrote:
> On Tue, Nov 25, 2014 at 10:35:04AM +0000, Mark Rutland wrote:
> > On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > > Support specifying console options (like with console=ttyXN,<options>)
> > > by appending them to the stdout-path property after a separating ':'.
> > >
> > > Example:
> > > stdout-path = "uart0:115200";
> >
> > I would very much like to be able to use this -- it will allow
> > distributions to boot on a board without having to know _anything_ about
> > the console UART until userspace is up, which would make it possible to
> > have a completely generic installer image, without requiring the
> > platform to provide bootargs.
> >
> > My only concern is that this conflates the set of kernel command line
> > options for the UART wit the DT binding for it. I don't know how good
> > people are at keeping those options stable, and I know they are
> > currently not documented -- we would need to add a stdout-path options
> > section to relevant UART bindings.
>
> I don't disagree.
>
> Current options are fairly well defined and stable, at least for any
> driver that uses uart_parse_options() (documented in
> Documentation/serial/driver).
My concern is that this is Linux specific, other OSes may have different
ideas about how stdout options should be formatted within this property.
(At least I don't know of any standardisation of the 115200n8 thing --
I'd love to be corrected!)
If I were a firmware author I'd be wary of specifying a stdout-path with
a Linux specific suffix.
Search ePAPR for baud it seems that the generic serial binding includes
a current-speed property in 6.2.1.2. It then goes on a bit ambiguously
to talk about the NS16550 in 6.2.2 but I think 6.2.1.2 was intended to
be generic. No mention of stop-bits/parity etc though, they are assumed
to be set already I think
One thought I had was to define a dt-stdout "pseudo-console" so that
console=dt-stdout,115200n8 or something could be used.
Anyway I applied your patch to v3.18-rc5 and ran it on a Mustang and it
didn't work for some reason. I'm using:
fdt set /chosen stdout-path "/soc/serial@1c020000:115200"
setenv bootargs "earlycon=uart8250,mmio32,0x1c020000 root=/dev/sda3 rw debug"
So I get earlycon but then no proper console. Removing earlycon just
makes that stop working.
With:
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 89c6b33..5dc1718 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1840,6 +1840,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
of_stdout_options = strchr(name, ':');
if (of_stdout_options != NULL)
of_stdout_options++;
+ printk(KERN_CRIT "%s: name=%s of_stdout=%p options=%s\n",
+ __func__, name, of_stdout, of_stdout_options);
}
}
I can see in dmesg:
[ 0.000000] of_alias_scan: name=/soc/serial@1c020000:115200 of_stdout= (null) options=115200
So it seems like of_find_node_by_path() is confused by the ":".
I've not tried it but I'd have expected something more like:
if (name) {
of_stdout_options = strchr(name, ':');
if (of_stdout_options != NULL) {
*of_stdout_options = '0';
of_stdout_options++;
}
of_stdout = of_find_node_by_path(name);
}
i.e. strip the options then do the patch lookup. name is const so this
won't actually work, but something like it...
Ian.
On Tue, Nov 25, 2014 at 10:35:04AM +0000, Mark Rutland wrote:
> Hi Leif,
>
> On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
>
> I would very much like to be able to use this -- it will allow
> distributions to boot on a board without having to know _anything_ about
> the console UART until userspace is up, which would make it possible to
> have a completely generic installer image, without requiring the
> platform to provide bootargs.
Note there are several serial drivers that read back the console options
from the hardware assuming it has been initialized correctly by the
bootloader already. So when your bootloader already made some output
on the console (which is likely) you can do that already. Of course
this doesn't solve all problems, like if you want to use a different
console for the kernel than for the bootloader.
Anyway, this behaviour is very nice since the kernel always uses the
same speed for the console as the bootloader does.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Nov 25, 2014 at 12:07:29PM +0000, Ian Campbell wrote:
> My concern is that this is Linux specific, other OSes may have different
> ideas about how stdout options should be formatted within this property.
> (At least I don't know of any standardisation of the 115200n8 thing --
> I'd love to be corrected!)
>
> If I were a firmware author I'd be wary of specifying a stdout-path with
> a Linux specific suffix.
> Search ePAPR for baud it seems that the generic serial binding includes
> a current-speed property in 6.2.1.2. It then goes on a bit ambiguously
> to talk about the NS16550 in 6.2.2 but I think 6.2.1.2 was intended to
> be generic. No mention of stop-bits/parity etc though, they are assumed
> to be set already I think
>
> One thought I had was to define a dt-stdout "pseudo-console" so that
> console=dt-stdout,115200n8 or something could be used.
I'll wait for others to comment on the above.
> Anyway I applied your patch to v3.18-rc5 and ran it on a Mustang and it
> didn't work for some reason. I'm using:
>
> fdt set /chosen stdout-path "/soc/serial@1c020000:115200"
> setenv bootargs "earlycon=uart8250,mmio32,0x1c020000 root=/dev/sda3 rw debug"
>
> So I get earlycon but then no proper console. Removing earlycon just
> makes that stop working.
Right, so having debugged this a bit offline, I'm amazed I booted
anything at all, given how badly I broke path scanning...
Please ignore previous version - a fixed one follows:
/
Leif
>From aef87fd958902afe881720286d525e10997462b8 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <[email protected]>
Date: Mon, 24 Nov 2014 22:23:58 +0000
Subject: [PATCH] of: support passing console options with stdout-path
Support specifying console options (like with console=ttyXN,<options>)
by appending them to the stdout-path property after a separating ':'.
Example:
stdout-path = "uart0:115200";
This patch also modifies of_find_node_by_path() to match only the
portion of the path before a ':'.
Signed-off-by: Leif Lindholm <[email protected]>
---
drivers/of/base.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..ecd6290 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
struct device_node *of_chosen;
struct device_node *of_aliases;
struct device_node *of_stdout;
+static char *of_stdout_options;
struct kset *of_kset;
@@ -699,10 +700,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
{
struct device_node *child;
int len = strchrnul(path, '/') - path;
+ int term;
if (!len)
return NULL;
+ term = strchrnul(path, ':') - path;
+ if (term < len)
+ len = term;
+
__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))
@@ -742,11 +748,16 @@ struct device_node *of_find_node_by_path(const char *path)
if (*path != '/') {
char *p = strchrnul(path, '/');
int len = p - path;
+ int term;
/* of_aliases must not be NULL */
if (!of_aliases)
return NULL;
+ term = strchrnul(path, ':') - path;
+ if (term < len)
+ len = term;
+
for_each_property_of_node(of_aliases, pp) {
if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
np = of_find_node_by_path(pp->value);
@@ -1830,8 +1841,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
name = of_get_property(of_chosen, "linux,stdout-path", NULL);
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
- if (name)
+ if (name) {
of_stdout = of_find_node_by_path(name);
+ of_stdout_options = strchr(name, ':');
+ if (of_stdout_options != NULL)
+ of_stdout_options++;
+ }
}
if (!of_aliases)
@@ -1957,7 +1972,7 @@ bool of_console_check(struct device_node *dn, char *name, int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index, NULL);
+ return !add_preferred_console(name, index, of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
1.7.10.4
On Tue, 25 Nov 2014 00:00:16 +0100
, Andrew Lunn <[email protected]>
wrote:
> On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > Support specifying console options (like with console=ttyXN,<options>)
> > by appending them to the stdout-path property after a separating ':'.
> >
> > Example:
> > stdout-path = "uart0:115200";
> >
> > This patch also modifies of_find_node_by_path() to match only the
> > portion of the path before a ':'.
>
> Hi Leif
>
> These appears to somewhat conform to ePAPR, which says:
>
> A string that specifies the full path to the node representing
> the device to be used for boot console output. If the character
> ":" is present in the value it terminates the path.
>
> So you can put any random junk after the :. However, are we going to
> have backward/forward compatibility problems, and problems with
> bootloaders? The current kernel code does not look for the :. So a new
> DT blob on an old kernel will not work so well.
For DT, we've generally not been worrying about compatibility in that
direction. If we've got a new firmware, we should be able to get a new
kernel. Nobody has really screamed bout it yet.
> More worrying, barebox does not support the : either. So there is a
> danger your bootloader suddenly goes silent after a dt blob update.
Is barebox parsing the property? Or updating it? Or both? I've got no
problem with this patch, but we can also support things like the
'current-speed' property in the UART to set a default when the stdout
path doesn't have any arguments.
g.
On Tue, 25 Nov 2014 12:07:29 +0000
, Ian Campbell <[email protected]>
wrote:
> On Tue, 2014-11-25 at 11:17 +0000, Leif Lindholm wrote:
> > On Tue, Nov 25, 2014 at 10:35:04AM +0000, Mark Rutland wrote:
> > > On Mon, Nov 24, 2014 at 10:23:58PM +0000, Leif Lindholm wrote:
> > > > Support specifying console options (like with console=ttyXN,<options>)
> > > > by appending them to the stdout-path property after a separating ':'.
> > > >
> > > > Example:
> > > > stdout-path = "uart0:115200";
> > >
> > > I would very much like to be able to use this -- it will allow
> > > distributions to boot on a board without having to know _anything_ about
> > > the console UART until userspace is up, which would make it possible to
> > > have a completely generic installer image, without requiring the
> > > platform to provide bootargs.
> > >
> > > My only concern is that this conflates the set of kernel command line
> > > options for the UART wit the DT binding for it. I don't know how good
> > > people are at keeping those options stable, and I know they are
> > > currently not documented -- we would need to add a stdout-path options
> > > section to relevant UART bindings.
> >
> > I don't disagree.
> >
> > Current options are fairly well defined and stable, at least for any
> > driver that uses uart_parse_options() (documented in
> > Documentation/serial/driver).
>
> My concern is that this is Linux specific, other OSes may have different
> ideas about how stdout options should be formatted within this property.
> (At least I don't know of any standardisation of the 115200n8 thing --
> I'd love to be corrected!)
>
> If I were a firmware author I'd be wary of specifying a stdout-path with
> a Linux specific suffix.
>
> Search ePAPR for baud it seems that the generic serial binding includes
> a current-speed property in 6.2.1.2. It then goes on a bit ambiguously
> to talk about the NS16550 in 6.2.2 but I think 6.2.1.2 was intended to
> be generic. No mention of stop-bits/parity etc though, they are assumed
> to be set already I think
>
> One thought I had was to define a dt-stdout "pseudo-console" so that
> console=dt-stdout,115200n8 or something could be used.
>
> Anyway I applied your patch to v3.18-rc5 and ran it on a Mustang and it
> didn't work for some reason. I'm using:
>
> fdt set /chosen stdout-path "/soc/serial@1c020000:115200"
> setenv bootargs "earlycon=uart8250,mmio32,0x1c020000 root=/dev/sda3 rw debug"
>
> So I get earlycon but then no proper console. Removing earlycon just
> makes that stop working.
>
> With:
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 89c6b33..5dc1718 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1840,6 +1840,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> of_stdout_options = strchr(name, ':');
> if (of_stdout_options != NULL)
> of_stdout_options++;
> + printk(KERN_CRIT "%s: name=%s of_stdout=%p options=%s\n",
> + __func__, name, of_stdout, of_stdout_options);
> }
> }
>
>
> I can see in dmesg:
> [ 0.000000] of_alias_scan: name=/soc/serial@1c020000:115200 of_stdout= (null) options=115200
>
> So it seems like of_find_node_by_path() is confused by the ":".
That would be a bug in of_find_node_by_path(). Should add a test case
for that because ':' is supposed to terminate path parsing.
> I've not tried it but I'd have expected something more like:
> if (name) {
> of_stdout_options = strchr(name, ':');
> if (of_stdout_options != NULL) {
> *of_stdout_options = '0';
> of_stdout_options++;
> }
> of_stdout = of_find_node_by_path(name);
> }
This isn't really right because it modifies the property data which is
supposed to be const. of_find_node_by_path() already does the search
piecewise, so it should be fairly straightforward to add a test for the
':'. Anyone care to take a look? If I need to do it then it will take a couple
of weeks before I've got some free time. :-/
>
> i.e. strip the options then do the patch lookup. name is const so this
> won't actually work, but something like it...
>
> Ian.
>
On Mon, 24 Nov 2014 22:23:58 +0000
, Leif Lindholm <[email protected]>
wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
>
> Example:
> stdout-path = "uart0:115200";
>
> This patch also modifies of_find_node_by_path() to match only the
> portion of the path before a ':'.
>
> Signed-off-by: Leif Lindholm <[email protected]>
> ---
> drivers/of/base.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 3823edf..89c6b33 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
> struct device_node *of_chosen;
> struct device_node *of_aliases;
> struct device_node *of_stdout;
> +static char *of_stdout_options;
>
> struct kset *of_kset;
>
> @@ -703,6 +704,8 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> if (!len)
> return NULL;
>
> + len = strchrnul(path, ':') - path;
> +
> __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))
> @@ -747,6 +750,8 @@ struct device_node *of_find_node_by_path(const char *path)
> if (!of_aliases)
> return NULL;
>
> + len = strchrnul(path, ':') - path;
> +
> for_each_property_of_node(of_aliases, pp) {
> if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> np = of_find_node_by_path(pp->value);
Can you add a testcase to drivers/of/unittests.c for this new path
parsing? Then we know it's correct!
> @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> name = of_get_property(of_chosen, "linux,stdout-path", NULL);
> if (IS_ENABLED(CONFIG_PPC) && !name)
> name = of_get_property(of_aliases, "stdout", NULL);
> - if (name)
> + if (name) {
> of_stdout = of_find_node_by_path(name);
> + of_stdout_options = strchr(name, ':');
> + if (of_stdout_options != NULL)
> + of_stdout_options++;
> + }
Not so good. The ':' should actually be a generic part of
of_find_node_by_path(), it doesn't have to be limited to stdout parsing.
There are other places that would use it. I would add a second,
optional, argument to of_find_node_by_path() that will update a pointer
to the arguments after the ':'.
g.
> }
>
> if (!of_aliases)
> @@ -1957,7 +1966,7 @@ bool of_console_check(struct device_node *dn, char *name, int index)
> {
> if (!dn || dn != of_stdout || console_set_on_cmdline)
> return false;
> - return !add_preferred_console(name, index, NULL);
> + return !add_preferred_console(name, index, of_stdout_options);
> }
> EXPORT_SYMBOL_GPL(of_console_check);
>
> --
> 1.9.1
>
On Tue, Nov 25, 2014 at 02:58:54PM +0000, Grant Likely wrote:
> > + len = strchrnul(path, ':') - path;
> > +
> > for_each_property_of_node(of_aliases, pp) {
> > if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> > np = of_find_node_by_path(pp->value);
>
> Can you add a testcase to drivers/of/unittests.c for this new path
> parsing? Then we know it's correct!
Will do.
> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> > name = of_get_property(of_chosen, "linux,stdout-path", NULL);
> > if (IS_ENABLED(CONFIG_PPC) && !name)
> > name = of_get_property(of_aliases, "stdout", NULL);
> > - if (name)
> > + if (name) {
> > of_stdout = of_find_node_by_path(name);
> > + of_stdout_options = strchr(name, ':');
> > + if (of_stdout_options != NULL)
> > + of_stdout_options++;
> > + }
>
> Not so good. The ':' should actually be a generic part of
> of_find_node_by_path(), it doesn't have to be limited to stdout parsing.
> There are other places that would use it. I would add a second,
> optional, argument to of_find_node_by_path() that will update a pointer
> to the arguments after the ':'.
So, I agree this would be nicer...
However, I'm counting 157 callers of this function outside of
drivers/of and 43 inside. Any chance you'd let me get away with a
separate of_find_extra_options_by_path()?
/
Leif
On Tue, Nov 25, 2014 at 3:15 PM, Leif Lindholm <[email protected]> wrote:
> On Tue, Nov 25, 2014 at 02:58:54PM +0000, Grant Likely wrote:
>> > + len = strchrnul(path, ':') - path;
>> > +
>> > for_each_property_of_node(of_aliases, pp) {
>> > if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
>> > np = of_find_node_by_path(pp->value);
>>
>> Can you add a testcase to drivers/of/unittests.c for this new path
>> parsing? Then we know it's correct!
>
> Will do.
>
>> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>> > name = of_get_property(of_chosen, "linux,stdout-path", NULL);
>> > if (IS_ENABLED(CONFIG_PPC) && !name)
>> > name = of_get_property(of_aliases, "stdout", NULL);
>> > - if (name)
>> > + if (name) {
>> > of_stdout = of_find_node_by_path(name);
>> > + of_stdout_options = strchr(name, ':');
>> > + if (of_stdout_options != NULL)
>> > + of_stdout_options++;
>> > + }
>>
>> Not so good. The ':' should actually be a generic part of
>> of_find_node_by_path(), it doesn't have to be limited to stdout parsing.
>> There are other places that would use it. I would add a second,
>> optional, argument to of_find_node_by_path() that will update a pointer
>> to the arguments after the ':'.
>
> So, I agree this would be nicer...
>
> However, I'm counting 157 callers of this function outside of
> drivers/of and 43 inside. Any chance you'd let me get away with a
> separate of_find_extra_options_by_path()?
I'd rather not. It is a trivial change and can be pretty much done
mechanically. Something like (I think; I always have to look up the
sed syntax):
sed s/of_find_node_by_path\((.*)\)/of_find_node_by_path\(\1, NULL\)
If it turns out to be really painful, then I'll relent, but I'd like
you to at least try! :-)
g.
On Tue, 2014-11-25 at 15:20 +0000, Grant Likely wrote:
> On Tue, Nov 25, 2014 at 3:15 PM, Leif Lindholm <[email protected]> wrote:
> > On Tue, Nov 25, 2014 at 02:58:54PM +0000, Grant Likely wrote:
> >> > + len = strchrnul(path, ':') - path;
> >> > +
> >> > for_each_property_of_node(of_aliases, pp) {
> >> > if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> >> > np = of_find_node_by_path(pp->value);
> >>
> >> Can you add a testcase to drivers/of/unittests.c for this new path
> >> parsing? Then we know it's correct!
> >
> > Will do.
> >
> >> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
> >> > name = of_get_property(of_chosen, "linux,stdout-path", NULL);
> >> > if (IS_ENABLED(CONFIG_PPC) && !name)
> >> > name = of_get_property(of_aliases, "stdout", NULL);
> >> > - if (name)
> >> > + if (name) {
> >> > of_stdout = of_find_node_by_path(name);
> >> > + of_stdout_options = strchr(name, ':');
> >> > + if (of_stdout_options != NULL)
> >> > + of_stdout_options++;
> >> > + }
> >>
> >> Not so good. The ':' should actually be a generic part of
> >> of_find_node_by_path(), it doesn't have to be limited to stdout parsing.
> >> There are other places that would use it. I would add a second,
> >> optional, argument to of_find_node_by_path() that will update a pointer
> >> to the arguments after the ':'.
> >
> > So, I agree this would be nicer...
> >
> > However, I'm counting 157 callers of this function outside of
> > drivers/of and 43 inside. Any chance you'd let me get away with a
> > separate of_find_extra_options_by_path()?
>
> I'd rather not. It is a trivial change and can be pretty much done
> mechanically. Something like (I think; I always have to look up the
> sed syntax):
>
> sed s/of_find_node_by_path\((.*)\)/of_find_node_by_path\(\1, NULL\)
Coccinelle rules for this sort of transformation...
Ian.
On Tue, Nov 25, 2014 at 3:24 PM, Ian Campbell <[email protected]> wrote:
> On Tue, 2014-11-25 at 15:20 +0000, Grant Likely wrote:
>> On Tue, Nov 25, 2014 at 3:15 PM, Leif Lindholm <[email protected]> wrote:
>> > On Tue, Nov 25, 2014 at 02:58:54PM +0000, Grant Likely wrote:
>> >> > + len = strchrnul(path, ':') - path;
>> >> > +
>> >> > for_each_property_of_node(of_aliases, pp) {
>> >> > if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
>> >> > np = of_find_node_by_path(pp->value);
>> >>
>> >> Can you add a testcase to drivers/of/unittests.c for this new path
>> >> parsing? Then we know it's correct!
>> >
>> > Will do.
>> >
>> >> > @@ -1830,8 +1835,12 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
>> >> > name = of_get_property(of_chosen, "linux,stdout-path", NULL);
>> >> > if (IS_ENABLED(CONFIG_PPC) && !name)
>> >> > name = of_get_property(of_aliases, "stdout", NULL);
>> >> > - if (name)
>> >> > + if (name) {
>> >> > of_stdout = of_find_node_by_path(name);
>> >> > + of_stdout_options = strchr(name, ':');
>> >> > + if (of_stdout_options != NULL)
>> >> > + of_stdout_options++;
>> >> > + }
>> >>
>> >> Not so good. The ':' should actually be a generic part of
>> >> of_find_node_by_path(), it doesn't have to be limited to stdout parsing.
>> >> There are other places that would use it. I would add a second,
>> >> optional, argument to of_find_node_by_path() that will update a pointer
>> >> to the arguments after the ':'.
>> >
>> > So, I agree this would be nicer...
>> >
>> > However, I'm counting 157 callers of this function outside of
>> > drivers/of and 43 inside. Any chance you'd let me get away with a
>> > separate of_find_extra_options_by_path()?
>>
>> I'd rather not. It is a trivial change and can be pretty much done
>> mechanically. Something like (I think; I always have to look up the
>> sed syntax):
>>
>> sed s/of_find_node_by_path\((.*)\)/of_find_node_by_path\(\1, NULL\)
>
> Coccinelle rules for this sort of transformation...
/me still hasn't gotten his head around how to use Coccinelle properly. :-(
g.
On Tue, 2014-11-25 at 15:39 +0000, Grant Likely wrote:
> On Tue, Nov 25, 2014 at 3:24 PM, Ian Campbell <[email protected]> wrote:
> >> sed s/of_find_node_by_path\((.*)\)/of_find_node_by_path\(\1, NULL\)
> >
> > Coccinelle rules for this sort of transformation...
>
> /me still hasn't gotten his head around how to use Coccinelle properly. :-(
Me neither, but for the "add an extra NULL parameter" case I can
generally find something to cargo cult from ;-) (it's been ages since I
had a need though)
Ian.
> > More worrying, barebox does not support the : either. So there is a
> > danger your bootloader suddenly goes silent after a dt blob update.
>
> Is barebox parsing the property? Or updating it? Or both? I've got no
> problem with this patch, but we can also support things like the
> 'current-speed' property in the UART to set a default when the stdout
> path doesn't have any arguments.
Hi Grant
I'm no export with barebox. So i just grep'ed the code.
It parses the property. Also as with linux, of_find_node_by_path()
does not stop at : as you say it should.
There is one case of the property being updated, but that is only for
the PPC mpc85xx.
Barebox does not appear to use 'current-speed' at all.
Andrew