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

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

[Ian Campbell]

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.

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

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 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

[PATCH v2 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 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

[PATCH v2 3/4] dt-uart: use ':' as 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).

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 <ian.campbell@citrix.com>
---
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(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 152ae03..17cf563 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -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: `""`
 
diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
index 04dbb97..b9a0956 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,7 @@ void __init dt_uart_init(void)
         return;
     }
 
-    options = strchr(opt_dtuart, ',');
+    options = strchr(opt_dtuart, ':');
     if ( options != NULL )
         *(options++) = '\0';
     else
-- 
1.7.10.4

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

[Julien Grall]

Hi Ian,

On 08/01/15 11:53, Ian Campbell wrote:
> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> index 04dbb97..b9a0956 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)
>   */

You forgot to modify the comment:

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

With this change:

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

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2015-01-08 at 12:26 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 08/01/15 11:53, Ian Campbell wrote:
> > diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> > index 04dbb97..b9a0956 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)
> >   */
> 
> You forgot to modify the comment:
> 
> "Configure UART port with a string:
> path,options"

Oops, I'll fix on commit.

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

Thanks,
Ian.

> 
> Regards,
> 

[PATCH v2 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. 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 <ian.campbell@citrix.com>
---
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(+)

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 b9a0956..07d1388 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:
@@ -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;
     }
-- 
1.7.10.4

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

[Julien Grall]

Hi Ian,

On 08/01/15 11:53, Ian Campbell wrote:
> +            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 ...

> +            }
> +            else if ( ret != -EINVAL /* Not present */ )
> +                printk("Failed to read /chosen/stdout-path (%d)\n", ret);

XENLOG_ERROR here.

Otherwise:

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

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2015-01-08 at 13:15 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 08/01/15 11:53, Ian Campbell wrote:
> > +            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 ...
> 
> > +            }
> > +            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.

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

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

[Julien Grall]

On 08/01/15 13:22, Ian Campbell wrote:
> On Thu, 2015-01-08 at 13:15 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 08/01/15 11:53, Ian Campbell wrote:
>>> +            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 ...
>>
>>> +            }
>>> +            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.

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,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2015-01-08 at 13:30 +0000, Julien Grall wrote:
> On 08/01/15 13:22, Ian Campbell wrote:
> > On Thu, 2015-01-08 at 13:15 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 08/01/15 11:53, Ian Campbell wrote:
> >>> +            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 ...
> >>
> >>> +            }
> >>> +            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.
> 
> 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.

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

[Julien Grall]

Hi Ian,

On 12/01/15 15:22, Ian Campbell wrote:
> On Thu, 2015-01-08 at 13:30 +0000, Julien Grall wrote:
>> On 08/01/15 13:22, Ian Campbell wrote:
>>> On Thu, 2015-01-08 at 13:15 +0000, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 08/01/15 11:53, Ian Campbell wrote:
>>>>> +            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 ...
>>>>
>>>>> +            }
>>>>> +            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.
>>
>> 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.

Ok. I'm fine with it.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2015-01-12 at 15:24 +0000, Julien Grall wrote:
> Ok. I'm fine with it.

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