Skip to contentSkip to conversation

dt-uart: cleanups, bugfixes and /chosen/stdout-path support

PATCH In review Ian Campbell 2 participants 13 messages Jan 8, 2015 – Jan 12, 2015

lore
Share & tools
lore

Ian Campbell

author
#0 lore raw

v2 cover dt-uart: cleanups, bugfixes and /chosen/stdout-path support

The two main changes here are:

* a bugfix to deal with DT paths which contain a common, which are perfectly valid and quite common.
* support for the /chosen/stdout-path device tree property, which allows Xen to find a boot console without manual configuration if presented with a suitable device tree. Juno and Seattle both contain the node in the dt supplied with the upstream Linux kernel (not sure about their factory kernel), as do a bunch of non-virt capable arm devices, but with stdout-path support being added to Linux in v3.19-rc1 I expect that number will soon grow.

I think "dt-uart: use ':' as separator between path and options" should be a candidate for stable backport.

Since last time: removed back compat support for ',', constify, warn if stdout-path is truncated.

author
#1 lore raw

PATCH v2 1/4 dt-uart: add an emacs magic block

Signed-off-by Ian Campbell <[email protected]>
Reviewed-by Julien Grall <[email protected]>
---
 xen/drivers/char/dt-uart.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
xen/drivers/char/dt-uart.c +10 -0
index fa92b5c..45a87a6 100644
@@ -70,3 +70,13 @@ void __init dt_uart_init(void)
     if ( ret )
         printk("Unable to initialize serial: %d\n", ret);
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
Signature · 1 line
1.7.10.4
author
#2 lore raw

PATCH v2 2/4 dt-uart: Clarify log messages at init time.

In reply to Ian Campbell

- Don't log at all if console=dtuart (the default) was not present, in that case the user has asked for something else, no need for every other driver to tell them this.
- Use "dtuart" in all other messages, rather than just "console" or "uart".
- Be more explicit if we are exiting because dtuart= wasn't given.
- Log the options which we've parsed.

Signed-off-by Ian Campbell <[email protected]>
Reviewed-by Julien Grall <[email protected]>
---
 xen/drivers/char/dt-uart.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
xen/drivers/char/dt-uart.c +7 -4
index 45a87a6..04dbb97 100644
@@ -41,9 +41,12 @@ void __init dt_uart_init(void)
     const char *devpath = opt_dtuart;
     char *options;
 
-    if ( !console_has("dtuart") || !strcmp(opt_dtuart, "") )
+    if ( !console_has("dtuart") )
+        return; /* Not for us */
+
+    if ( !strcmp(opt_dtuart, "") )
     {
-        printk("No console\n");
+        printk("No dtuart path configured\n");
         return;
     }
 

@@ -53,7 +56,7 @@ void __init dt_uart_init(void)
     else
         options = "";
 
-    printk("Looking for UART console %s\n", devpath);
+    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, options);
     if ( *devpath == '/' )
         dev = dt_find_node_by_path(devpath);
     else

@@ -68,7 +71,7 @@ void __init dt_uart_init(void)
     ret = device_init(dev, DEVICE_SERIAL, options);
 
     if ( ret )
-        printk("Unable to initialize serial: %d\n", ret);
+        printk("Unable to initialize dtuart: %d\n", ret);
 }
 
 /*
Signature · 1 line
1.7.10.4
author
#3 lore raw

PATCH v2 3/4 dt-uart: use ':' as separator between path and options

In reply to Ian Campbell

',' is a valid character in a device-tree path (see ePAPR v1.1 Table 2-1), in fact ',' is actually pretty common in node names.

Using ',' as a separator breaks for example on fast models. If you use the full path (/smb/motherboard/iofpga@3,00000000/uart@090000) rather than the alias then earlyprintk gives:

(XEN) Looking for UART console /smb/motherboard/iofpga@3 (XEN) Unable to find device "/smb/motherboard/iofpga@3" (XEN) Bad console= option 'dtuart'

I actually noticed this on Jetson where the uart is "/serial@0,70006300" and there happened to be no alias defined.

Instead use ':' as the separator, it is defined to terminate the path in the context of /chosen/stdout-path (Table 3-4) which is pretty closely analogous to the dtuart= option and so makes a pretty good choice (especially since the next patch adds support for stdout-path).

Since no DT aware driver current supports any options there is no point in retaining support for ',' for backwards compatibility.

Additionally, expand the buffer for the dtuart option, a path can be far longer than 30 characters (in fact the maximum size of a single node name is 31, so it's not even necessarily enough for an alias). 128 is completely arbitrary and allows for paths at least 8 deep even with worst case node names.

Signed-off-by Ian Campbell <[email protected]>

---
v2:
- none of our drivers handle any options, so there is no point in retaining support for backwards compat reasons.

---
 docs/misc/xen-command-line.markdown |    2 +-
 xen/drivers/char/dt-uart.c          |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
docs/misc/xen-command-line.markdown +1 -1
index 152ae03..17cf563 100644
@@ -550,7 +550,7 @@ Pin dom0 vcpus to their respective pcpus
 Flag that makes a 64bit dom0 boot in PVH mode. No 32bit support at present.
 
 ### dtuart (ARM)
-> `= path [,options]`
+> `= path [:options]`
 
 > Default: `""`
xen/drivers/char/dt-uart.c +2 -2
index 04dbb97..b9a0956 100644
@@ -31,7 +31,7 @@
  * doesn't start with '/', we assuming that it's an alias.
  * @options: UART speficic options (see in each UART driver)
  */
-static char __initdata opt_dtuart[30] = "";
+static char __initdata opt_dtuart[256] = "";
 string_param("dtuart", opt_dtuart);
 
 void __init dt_uart_init(void)

@@ -50,7 +50,7 @@ void __init dt_uart_init(void)
         return;
     }
 
-    options = strchr(opt_dtuart, ',');
+    options = strchr(opt_dtuart, ':');
     if ( options != NULL )
         *(options++) = '\0';
     else
Signature · 1 line
1.7.10.4
Reviewed-by on v2 3/4
#4 lore raw

Re: [PATCH v2 3/4] dt-uart: use ':' as separator between path and options

Hi Ian,

xen/drivers/char/dt-uart.c line 33

Quoted 8 lines · Ian Campbell
xen/drivers/char/dt-uart.c +0 -0
index 04dbb97..b9a0956 100644
@@ -31,7 +31,7 @@
  * doesn't start with '/', we assuming that it's an alias.
  * @options: UART speficic options (see in each UART driver)
  */

You forgot to modify the comment:

"Configure UART port with a string:
path,options"

With this change:

Reviewed-by Julien Grall <[email protected]>

Regards,

Signature · 1 line
Julien Grall
author
#5 lore raw

Re: [PATCH v2 3/4] dt-uart: use ':' as separator between path and options

Quoted 14 lines · Julien Grall

Hi Ian,

xen/drivers/char/dt-uart.c line 33

Quoted 8 lines · Ian Campbell
xen/drivers/char/dt-uart.c +0 -0
index 04dbb97..b9a0956 100644
@@ -31,7 +31,7 @@
  * doesn't start with '/', we assuming that it's an alias.
  * @options: UART speficic options (see in each UART driver)
  */

You forgot to modify the comment:

Quoted 2 lines

"Configure UART port with a string:
path,options"

Oops, I'll fix on commit.

Quoted 3 lines

With this change:

Reviewed-by Julien Grall <[email protected]>

Thanks,
Ian.

Quoted 3 lines

Regards,

author
#6 lore raw

PATCH v2 4/4 dt-uart: support /chosen/stdout-path property.

In reply to Ian Campbell

ePAPR v1.1 section 3.5 defines the /chosen/stdout-path property to refer to the device to be used for boot console output, so if no dtuart property is given try to use that instead. This will make Xen find a suitable console by default on DT platforms which include this property.

As it happens the dtuart option has the exact same syntax as stdout-path, so we can just copy the value into that buffer if it is empty. If the string is too large for the buffer we truncate and warn but continue in the hopes that enough of the path survived (i.e. only the options part was dropped) to get something out.

FWIW support for this was added to Linux in v3.19-rc1 (7914a7c5651a "of: support passing console options with stdout-path") and a fairly large number of the dts files shipped with Linux have already included a stdout-path property for quite a while now.

Signed-off-by Ian Campbell <[email protected]>

---
v2:
- simplified after removing unnecessary compat support for ',' in previous patch, declared stdout in the now smaller scope.
- constify struct dt_device_node pointer
- warn if stdout-path is truncated.

---
 xen/arch/arm/domain_build.c |    2 ++
 xen/drivers/char/dt-uart.c  |   22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)
xen/arch/arm/domain_build.c +2 -0
index de180d8..c33a73c 100644
@@ -424,6 +424,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
          *   bootargs (from module #1, above).
          * * remove bootargs,  xen,dom0-bootargs, xen,xen-bootargs,
          *   linux,initrd-start and linux,initrd-end.
+         * * remove stdout-path.
          * * remove bootargs, linux,uefi-system-table,
          *   linux,uefi-mmap-start, linux,uefi-mmap-size,
          *   linux,uefi-mmap-desc-size, and linux,uefi-mmap-desc-ver

@@ -434,6 +435,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
             if ( dt_property_name_is_equal(prop, "xen,xen-bootargs") ||
                  dt_property_name_is_equal(prop, "linux,initrd-start") ||
                  dt_property_name_is_equal(prop, "linux,initrd-end") ||
+                 dt_property_name_is_equal(prop, "stdout-path") ||
                  dt_property_name_is_equal(prop, "linux,uefi-system-table") ||
                  dt_property_name_is_equal(prop, "linux,uefi-mmap-start") ||
                  dt_property_name_is_equal(prop, "linux,uefi-mmap-size") ||
xen/drivers/char/dt-uart.c +22 -0
index b9a0956..07d1388 100644
@@ -22,6 +22,7 @@
 #include <xen/console.h>
 #include <xen/device_tree.h>
 #include <xen/serial.h>
+#include <xen/errno.h>
 
 /*
  * Configure UART port with a string:

@@ -46,6 +47,27 @@ void __init dt_uart_init(void)
 
     if ( !strcmp(opt_dtuart, "") )
     {
+        const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+
+        if ( chosen )
+        {
+            const char *stdout;
+
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
+        }
+    }
+
+    if ( !strcmp(opt_dtuart, "") )
+    {
         printk("No dtuart path configured\n");
         return;
     }
Signature · 1 line
1.7.10.4
Reviewed-by on v2 4/4
#7 lore raw

Re: [PATCH v2 4/4] dt-uart: support /chosen/stdout-path property.

Hi Ian,

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 62

Quoted 7 lines · Ian Campbell
xen/drivers/char/dt-uart.c +7 -0
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");

I would add XENLOG_WARNING here and ...

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 65

Quoted 3 lines
xen/drivers/char/dt-uart.c +3 -0
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);

XENLOG_ERROR here.

Otherwise:

Reviewed-by Julien Grall <[email protected]>

Regards,

Signature · 1 line
Julien Grall
author
#8 lore raw

Re: [PATCH v2 4/4] dt-uart: support /chosen/stdout-path property.

Quoted 11 lines · Julien Grall

Hi Ian,

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 62

Quoted 7 lines · Ian Campbell
xen/drivers/char/dt-uart.c +7 -0
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");
Quoted 7 lines

I would add XENLOG_WARNING here and ...

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 65

Quoted 3 lines
xen/drivers/char/dt-uart.c +3 -0
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);

XENLOG_ERROR here.

In practice these only go via the earlyprintk mechanism, since the console can't be setup yet. I'm not sure it's worthwhile tagging such messages.

Quoted 7 lines

Otherwise:

Reviewed-by Julien Grall <[email protected]>

Regards,

#9 lore raw

Re: [PATCH v2 4/4] dt-uart: support /chosen/stdout-path property.

Quoted 18 lines · Ian Campbell
Quoted 17 lines · Julien Grall

Hi Ian,

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 62

Quoted 7 lines · Ian Campbell
xen/drivers/char/dt-uart.c +7 -0
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");

I would add XENLOG_WARNING here and ...

Quoted 3 lines
xen/drivers/char/dt-uart.c +3 -0
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
Quoted 5 lines
Quoted 1 line

XENLOG_ERROR here.

In practice these only go via the earlyprintk mechanism, since the console can't be setup yet. I'm not sure it's worthwhile tagging such messages.

earlyprintk is transparent for the console code. Tagging may help if we decide to implement other kind of console later (VGA, PCI UART...).

Anyway, I doesn't change much things here as the message is tagged as WARNING by default. So it will be always printing.

Regards,

Signature · 1 line
Julien Grall
author
#10 lore raw

Re: [PATCH v2 4/4] dt-uart: support /chosen/stdout-path property.

Quoted 25 lines · Julien Grall
Quoted 18 lines · Ian Campbell
Quoted 17 lines · Julien Grall

Hi Ian,

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 62

Quoted 7 lines · Ian Campbell
xen/drivers/char/dt-uart.c +7 -0
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");

I would add XENLOG_WARNING here and ...

Quoted 3 lines
xen/drivers/char/dt-uart.c +3 -0
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
Quoted 5 lines
Quoted 1 line

XENLOG_ERROR here.

In practice these only go via the earlyprintk mechanism, since the console can't be setup yet. I'm not sure it's worthwhile tagging such messages.

Quoted 5 lines

earlyprintk is transparent for the console code. Tagging may help if we decide to implement other kind of console later (VGA, PCI UART...).

Anyway, I doesn't change much things here as the message is tagged as WARNING by default. So it will be always printing.

It turns out that none of the existing prints in this function use the tags, and I think its of marginal use in this context so I don't think it is necessary to go changing them all, or to only use the tags for these two messages.

Ian.

#11 lore raw

Re: [PATCH v2 4/4] dt-uart: support /chosen/stdout-path property.

Hi Ian,

Quoted 29 lines · Ian Campbell
Quoted 28 lines · Julien Grall
Quoted 18 lines · Ian Campbell
Quoted 17 lines · Julien Grall

Hi Ian,

xen/drivers/char/dt-uart.c void __init dt_uart_init(void) · line 62

Quoted 7 lines · Ian Campbell
xen/drivers/char/dt-uart.c +7 -0
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart))
+                     >= sizeof(opt_dtuart) )
+                    printk("WARNING: /chosen/stdout-path too long, truncated\n");

I would add XENLOG_WARNING here and ...

Quoted 3 lines
xen/drivers/char/dt-uart.c +3 -0
+            }
+            else if ( ret != -EINVAL /* Not present */ )
+                printk("Failed to read /chosen/stdout-path (%d)\n", ret);
Quoted 5 lines
Quoted 1 line

XENLOG_ERROR here.

In practice these only go via the earlyprintk mechanism, since the console can't be setup yet. I'm not sure it's worthwhile tagging such messages.

earlyprintk is transparent for the console code. Tagging may help if we decide to implement other kind of console later (VGA, PCI UART...).

Quoted 7 lines
Quoted 2 lines

Anyway, I doesn't change much things here as the message is tagged as WARNING by default. So it will be always printing.

It turns out that none of the existing prints in this function use the tags, and I think its of marginal use in this context so I don't think it is necessary to go changing them all, or to only use the tags for these two messages.

Ok. I'm fine with it.

Regards,

Signature · 1 line
Julien Grall
author
#12 lore raw

Re: [PATCH v2 4/4] dt-uart: support /chosen/stdout-path property.

Quoted 1 line · Julien Grall

Ok. I'm fine with it.

Thanks, I've applied the whole series, fixing the comment you noted in #3 as I went.