[PATCH 0/4] dt-uart: cleanups, bugfixes and /chosen/stdout-path support

[PATCH 0/4] dt-uart: cleanups, bugfixes and /chosen/stdout-path support

[Ian Campbell]

Before I put on my air tanks and go looking in my QUEUE-4.6 email folder
I wanted to start the year by doing some actual programming, and this
seemed like an afternoons hacking...

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 default separator between path and options"
should be a candidate for stable backport, and if it makes the backport
easier I'd be inclined to take "dt-uart: Clarify log messages at init
time" too.

Ian.

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

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/drivers/char/dt-uart.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
index fa92b5c..45a87a6 100644
--- a/xen/drivers/char/dt-uart.c
+++ b/xen/drivers/char/dt-uart.c
@@ -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:
+ */
-- 
1.7.10.4

Re: [PATCH 1/4] dt-uart: add an emacs magic block

[Julien Grall]

Hi Ian,

On 07/01/15 15:31, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
>  xen/drivers/char/dt-uart.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> index fa92b5c..45a87a6 100644
> --- a/xen/drivers/char/dt-uart.c
> +++ b/xen/drivers/char/dt-uart.c
> @@ -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:
> + */
> 


-- 
Julien Grall

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

[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 <ian.campbell@citrix.com>
---
 xen/drivers/char/dt-uart.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
index 45a87a6..04dbb97 100644
--- a/xen/drivers/char/dt-uart.c
+++ b/xen/drivers/char/dt-uart.c
@@ -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);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH 2/4] dt-uart: Clarify log messages at init time.

[Julien Grall]

Hi Ian,

On 07/01/15 15:31, Ian Campbell wrote:
> - 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 <ian.campbell@citrix.com>

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
>  xen/drivers/char/dt-uart.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> index 45a87a6..04dbb97 100644
> --- a/xen/drivers/char/dt-uart.c
> +++ b/xen/drivers/char/dt-uart.c
> @@ -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);
>  }
>  
>  /*
> 


-- 
Julien Grall

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

[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).

We still handle ',' for backwards compatibility. Note that this
introduces a wrinkle in that in order to specify a dtuart path
containing a ',' with no options you need to append an otherwise
pointless ':' (or use an alias with no ',' in it).

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 <ian.campbell@citrix.com>
---
I've retained the handling of ',' for compatibility, but I'm almost
inclined to just drop it, if not now then in a release or two.
---
 docs/misc/xen-command-line.markdown |    9 ++++++++-
 xen/drivers/char/dt-uart.c          |    6 ++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 152ae03..f7cb6d9 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -550,13 +550,20 @@ 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: `""`
 
 Specify the full path in the device tree for the UART.  If the path doesn't
 start with `/`, it is assumed to be an alias.  The options are device specific.
 
+The path should be separated from the options with a `:`. For
+backwards compatibility `,` is also supported, however this is
+deprecated because `,` is a valid character in a device tree path.
+
+To specify a path containing a `,` with no options simply append a `:`
+to the path.
+
 ### e820-mtrr-clip
 > `= <boolean>`
 
diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
index 04dbb97..54e65fc 100644
--- a/xen/drivers/char/dt-uart.c
+++ b/xen/drivers/char/dt-uart.c
@@ -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,9 @@ void __init dt_uart_init(void)
         return;
     }
 
-    options = strchr(opt_dtuart, ',');
+    options = strchr(opt_dtuart, ':');
+    if ( !options )
+        options = strchr(opt_dtuart, ',');
     if ( options != NULL )
         *(options++) = '\0';
     else
-- 
1.7.10.4

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

[Julien Grall]

On 07/01/15 15:31, Ian Campbell wrote:
> I've retained the handling of ',' for compatibility, but I'm almost
> inclined to just drop it, if not now then in a release or two.

None of the current drivers support options. So I think we could drop
the support of ",".

Backport: I don't think #2 is required to backport this patch.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2015-01-07 at 16:31 +0000, Julien Grall wrote:
> On 07/01/15 15:31, Ian Campbell wrote:
> > I've retained the handling of ',' for compatibility, but I'm almost
> > inclined to just drop it, if not now then in a release or two.
> 
> None of the current drivers support options. So I think we could drop
> the support of ",".

Oh, I expected without checking that ns16550 would since it does for
non-DT but you are right. I'll respin without the compatibility stuff,
makes some stuff a bit cleaner too.

> Backport: I don't think #2 is required to backport this patch.

Right, I didn't actually check, I just mentioned it as a possibility in
case git cherry-pick complained.

Ian.

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

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

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.

Since there is a base of existing device trees with the property, we
do not support the legacy ',' options separator so we remain
compatible.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |    2 ++
 xen/drivers/char/dt-uart.c  |   29 +++++++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de180d8..c33a73c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -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") ||
diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
index 54e65fc..08b0d76 100644
--- a/xen/drivers/char/dt-uart.c
+++ b/xen/drivers/char/dt-uart.c
@@ -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:
@@ -38,7 +39,7 @@ void __init dt_uart_init(void)
 {
     struct dt_device_node *dev;
     int ret;
-    const char *devpath = opt_dtuart;
+    const char *devpath = opt_dtuart, *stdout = NULL;
     char *options;
 
     if ( !console_has("dtuart") )
@@ -46,12 +47,36 @@ void __init dt_uart_init(void)
 
     if ( !strcmp(opt_dtuart, "") )
     {
+        struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+
+        if ( chosen )
+        {
+            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
+            if ( ret >= 0 )
+            {
+                printk("Taking dtuart configuration from /chosen/stdout-path\n");
+                strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart));
+            }
+            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;
     }
 
     options = strchr(opt_dtuart, ':');
-    if ( !options )
+    /*
+     * Support ',' as a legacy separator for path from command line
+     * only, since there is no legacy on the Xen side with stdout-path
+     * and looking for ',' there would render us incompatible with
+     * many of the device tree files out there which already include a
+     * stdout-path.
+     */
+    if ( !options && !stdout )
         options = strchr(opt_dtuart, ',');
     if ( options != NULL )
         *(options++) = '\0';
-- 
1.7.10.4

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

[Julien Grall]

Hi Ian,

On 07/01/15 15:31, Ian Campbell wrote:
> 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.
> 
> 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.
> 
> Since there is a base of existing device trees with the property, we
> do not support the legacy ',' options separator so we remain
> compatible.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |    2 ++
>  xen/drivers/char/dt-uart.c  |   29 +++++++++++++++++++++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index de180d8..c33a73c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -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") ||
> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> index 54e65fc..08b0d76 100644
> --- a/xen/drivers/char/dt-uart.c
> +++ b/xen/drivers/char/dt-uart.c
> @@ -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:
> @@ -38,7 +39,7 @@ void __init dt_uart_init(void)
>  {
>      struct dt_device_node *dev;
>      int ret;
> -    const char *devpath = opt_dtuart;
> +    const char *devpath = opt_dtuart, *stdout = NULL;
>      char *options;
>  
>      if ( !console_has("dtuart") )
> @@ -46,12 +47,36 @@ void __init dt_uart_init(void)
>  
>      if ( !strcmp(opt_dtuart, "") )
>      {
> +        struct dt_device_node *chosen = dt_find_node_by_path("/chosen");

const struct dt_device_node *chosen

> +
> +        if ( chosen )
> +        {
> +            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
> +            if ( ret >= 0 )
> +            {
> +                printk("Taking dtuart configuration from /chosen/stdout-path\n");
> +                strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart));

The final string in opt_dtuart may be truncated if stdout is bigger than
255 characters.

I would add a check to avoid hours of debugging later.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2015-01-07 at 16:42 +0000, Julien Grall wrote:
> > +
> > +        if ( chosen )
> > +        {
> > +            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
> > +            if ( ret >= 0 )
> > +            {
> > +                printk("Taking dtuart configuration from /chosen/stdout-path\n");
> > +                strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart));
> 
> The final string in opt_dtuart may be truncated if stdout is bigger than
> 255 characters.
> 
> I would add a check to avoid hours of debugging later.

Good point. I suppose it may as well warn and continue: hypothetically
the truncation might only affect some non-critical options so the
console might actually work, so we might as well try.

Ian.

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

[Julien Grall]

On 07/01/15 16:47, Ian Campbell wrote:
> On Wed, 2015-01-07 at 16:42 +0000, Julien Grall wrote:
>>> +
>>> +        if ( chosen )
>>> +        {
>>> +            ret = dt_property_read_string(chosen, "stdout-path", &stdout);
>>> +            if ( ret >= 0 )
>>> +            {
>>> +                printk("Taking dtuart configuration from /chosen/stdout-path\n");
>>> +                strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart));
>>
>> The final string in opt_dtuart may be truncated if stdout is bigger than
>> 255 characters.
>>
>> I would add a check to avoid hours of debugging later.
> 
> Good point. I suppose it may as well warn and continue: hypothetically
> the truncation might only affect some non-critical options so the
> console might actually work, so we might as well try.

Sounds good.

Regards,

-- 
Julien Grall