| Message ID | 1416867838-18652-1-git-send-email-leif.lindholm@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <leif.lindholm@linaro.org> > > --- > > 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 > > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 25 Nov 2014 00:00:16 +0100 , Andrew Lunn <andrew@lunn.ch> 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 25 Nov 2014 12:07:29 +0000 , Ian Campbell <ijc@debian.org> 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. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, 24 Nov 2014 22:23:58 +0000 , Leif Lindholm <leif.lindholm@linaro.org> 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 <leif.lindholm@linaro.org> > --- > 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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Nov 25, 2014 at 3:15 PM, Leif Lindholm <leif.lindholm@linaro.org> 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 25, 2014 at 3:24 PM, Ian Campbell <ijc@debian.org> wrote: > On Tue, 2014-11-25 at 15:20 +0000, Grant Likely wrote: >> On Tue, Nov 25, 2014 at 3:15 PM, Leif Lindholm <leif.lindholm@linaro.org> 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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 <leif.lindholm@linaro.org> --- drivers/of/base.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)