Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Bamvor Jian Zhang]

Hi Julien

> On some board, there is no alias to the UART. To avoid modification in
> the device tree, dt-uart should also search device by path.
> 
> To distinguish an alias from a path, dt-uart will check the first character.
> If it's a / then it's path, otherwise it's an alias.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> Acked-by: Ian Cambell <ian.campbell@xxxxxxxxxx>
> ---
>  xen/drivers/char/dt-uart.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> index 93bb0f5..d7204fb 100644
> --- a/xen/drivers/char/dt-uart.c
> +++ b/xen/drivers/char/dt-uart.c
> @@ -26,9 +26,10 @@
>  
>  /*
>   * Configure UART port with a string:
> - * alias,options
> + * path,options
>   *
> - * @alias: alias used in the device tree for the UART
> + * @path: full path used in the device tree for the UART. If the path
> + * 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] = "";
> @@ -38,7 +39,7 @@ void __init dt_uart_init(void)
>  {
>      struct dt_device_node *dev;
>      int ret;
> -    const char *devalias = opt_dtuart;
> +    const char *devpath = opt_dtuart;
>      char *options;
>  
>      if ( !console_has("dtuart") || !strcmp(opt_dtuart, "") )
> @@ -53,12 +54,15 @@ void __init dt_uart_init(void)
>      else
>          options = "";
>  
> -    early_printk("Looking for UART console %s\n", devalias);
> -    dev = dt_find_node_by_alias(devalias);
> +    early_printk("Looking for UART console %s\n", devpath);
> +    if ( *devpath == '/' )
> +        dev = dt_find_node_by_path(devpath);
> +    else
> +        dev = dt_find_node_by_alias(devpath);
if '/' start an alias, it should be
+    if ( *devpath == '/' )
+        dev = dt_find_node_by_alias(devpath);
+    else
+        dev = dt_find_node_by_path(devpath);




bamvor
>  
>      if ( !dev )
>      {
> -        early_printk("Unable to find device \"%s\"\n", devalias);
> +        early_printk("Unable to find device \"%s\"\n", devpath);
>          return;
>      }
>  
> -- 
> 1.7.10.4

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Ian Campbell]

On Wed, 2013-08-28 at 19:19 -0600, Bamvor Jian Zhang wrote:

> > -    early_printk("Looking for UART console %s\n", devalias);
> > -    dev = dt_find_node_by_alias(devalias);
> > +    early_printk("Looking for UART console %s\n", devpath);
> > +    if ( *devpath == '/' )
> > +        dev = dt_find_node_by_path(devpath);
> > +    else
> > +        dev = dt_find_node_by_alias(devpath);

> if '/' start an alias, it should be

I think /  is the start of a path not an alias, so the current code is
correct.

> +    if ( *devpath == '/' )
> +        dev = dt_find_node_by_alias(devpath);
> +    else
> +        dev = dt_find_node_by_path(devpath);
> 

Ian.

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Julien Grall]

On 08/29/2013 02:19 AM, Bamvor Jian Zhang wrote:
> Hi Julien

Hi Bamvor,

>> On some board, there is no alias to the UART. To avoid modification in
>> the device tree, dt-uart should also search device by path.
>>
>> To distinguish an alias from a path, dt-uart will check the first character.
>> If it's a / then it's path, otherwise it's an alias.
>>
>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>> Acked-by: Ian Cambell <ian.campbell@xxxxxxxxxx>
>> ---
>>  xen/drivers/char/dt-uart.c |   16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
>> index 93bb0f5..d7204fb 100644
>> --- a/xen/drivers/char/dt-uart.c
>> +++ b/xen/drivers/char/dt-uart.c
>> @@ -26,9 +26,10 @@
>>  
>>  /*
>>   * Configure UART port with a string:
>> - * alias,options
>> + * path,options
>>   *
>> - * @alias: alias used in the device tree for the UART
>> + * @path: full path used in the device tree for the UART. If the path
>> + * 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] = "";
>> @@ -38,7 +39,7 @@ void __init dt_uart_init(void)
>>  {
>>      struct dt_device_node *dev;
>>      int ret;
>> -    const char *devalias = opt_dtuart;
>> +    const char *devpath = opt_dtuart;
>>      char *options;
>>  
>>      if ( !console_has("dtuart") || !strcmp(opt_dtuart, "") )
>> @@ -53,12 +54,15 @@ void __init dt_uart_init(void)
>>      else
>>          options = "";
>>  
>> -    early_printk("Looking for UART console %s\n", devalias);
>> -    dev = dt_find_node_by_alias(devalias);
>> +    early_printk("Looking for UART console %s\n", devpath);
>> +    if ( *devpath == '/' )
>> +        dev = dt_find_node_by_path(devpath);
>> +    else
>> +        dev = dt_find_node_by_alias(devpath);
> if '/' start an alias, it should be

>From the ePAR
(http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf)
section 3.3: an alias name must match the following pattern: [0-9a-z-]+
So we can't find '/' in the alias.

> +    if ( *devpath == '/' )
> +        dev = dt_find_node_by_alias(devpath);
> +    else
> +        dev = dt_find_node_by_path(devpath);

A path always start by '/'. With your solution, it won't be possible to
get the UART by path.

-- 
Julien Grall

[PATCH V1 00/29] Allow Xen to boot with a raw Device Tree

[Julien Grall]

Hi,

This is the second version of this patch series. It allow Xen to boot with
a raw device tree, ie without any specific modification for Xen.

Few months ago, the patch series "Support multiple ARM platform in XEN", has
added a tree structure to handle easier the device tree. Each node can carry
metadata to specify if the node can be pass-through to Dom0. Therefore, this
patch series take advantage of the "new" Device Tree API and get a rid of
the FDT API that is currently used to build Dom0 dts and in some drivers.

Major changes since v1:
    - Add multiple commandlines patch from Andre and rebase it (mostly
      use the new helpers): http://patches.linaro.org/19356/
    - Remove and add fake node for cpus, gic and timer

All the changes can be found in each patch.

TODO list:
    - Add support for memreserve. This is needed for HDLCD framebuffer on the
    versatile express.

Cheers,

Andre Przywara (1):
  ARM: parse separate DT properties for different commandlines

Julien Grall (28):
  xen/char: dt-uart: Allow the user to give a path to the node
  xen: Introduce __initconst to store initial const data
  xen/dts: Don't check the number of address and size cells in
    process_cpu_node
  xen/dts: Constify device_tree_flattened
  xen/arm: Move __PSCI* from traps.c to the header
  xen: Add new string function
  xen: Use the right string comparison function in device tree
  xen/dts: Don't add a fake property "name" in the device tree
  xen/dts: Add new helpers to use the device tree
  xen/dts: Remove device_get_reg call in process_cpu_node
  xen/dts: Check "reg" property length in process_multiboot_node
  xen/dts: Check the CPU ID is not greater than NR_CPUS
  xen/video: hdlcd: Convert the driver to the new device tree API
  xen/video: hdlcd: Use early_printk instead of printk
  xen/arm: Use dt_device_match to avoid multiple if conditions
  xen/arm: Build DOM0 FDT by browsing the device tree structure
  xen/arm: Mark each device used by Xen as disabled in DOM0 FDT
  xen/arm: Don't map disabled device in DOM0
  xen/arm: Create a fake PSCI node in dom0 device tree
  xen/arm: Create a fake cpus node in dom0 device tree
  xen/arm: Create a fake GIC node in dom0 device tree
  xen/arm: Create a fake timer node in dom0 device tree
  xen/arm: Add new platform specific callback device_is_blacklist
  xen/arm: vexpress: Blacklist a list of board specific devices
  xen/arm: exynos5: Blacklist MCT device
  xen/dts: Clean up the exported API for device tree
  xen/dts: device_get_reg: cells are 32 bits big endian value
  xen/arm: Check if the device is available before using it

 docs/misc/arm/device-tree/booting.txt |   28 +-
 xen/arch/arm/device.c                 |    3 +
 xen/arch/arm/domain_build.c           |  582 ++++++++++++++++++++++++---------
 xen/arch/arm/platform.c               |   10 +
 xen/arch/arm/platforms/exynos5.c      |   11 +
 xen/arch/arm/platforms/vexpress.c     |   17 +
 xen/arch/arm/setup.c                  |    6 +-
 xen/arch/arm/time.c                   |   10 +-
 xen/arch/arm/traps.c                  |    5 -
 xen/common/device_tree.c              |  281 +++++++++-------
 xen/common/string.c                   |   15 +
 xen/drivers/char/dt-uart.c            |   16 +-
 xen/drivers/video/arm_hdlcd.c         |   69 ++--
 xen/include/asm-arm/platform.h        |    7 +
 xen/include/asm-arm/psci.h            |    5 +
 xen/include/xen/device_tree.h         |  172 ++++++++--
 xen/include/xen/init.h                |    1 +
 xen/include/xen/string.h              |    3 +
 18 files changed, 894 insertions(+), 347 deletions(-)

-- 
1.7.10.4

[PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Julien Grall]

When a device has a property status with disabled inside, Linux will not use
the device.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 76decf4..d56bc70a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
             return res;
     }
 
+    /* Disable all devices used by Xen */
+    if ( dt_device_used_by(np) == DOMID_XEN )
+    {
+        res = fdt_property(kinfo->fdt, "status", "disabled", 8 + 1);
+        if ( res )
+            return res;
+    }
+
     /*
      * XXX should populate /chosen/linux,initrd-{start,end} here if we
      * have module[2]
-- 
1.7.10.4

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> When a device has a property status with disabled inside, Linux will not use
> the device.

Do we not filter such devices out already?

Or did the previous patch remove that functionality? In which case is
there a bisection hazard here?

Why is it preferred to leave the device with disabled in it instead of
removing it? (Can this go in the changelog please)

I think I've asked before, but is the handling of status = disabled
generic and therefore reliable?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 76decf4..d56bc70a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>              return res;
>      }
>  
> +    /* Disable all devices used by Xen */
> +    if ( dt_device_used_by(np) == DOMID_XEN )
> +    {
> +        res = fdt_property(kinfo->fdt, "status", "disabled", 8 + 1);
> +        if ( res )
> +            return res;
> +    }
> +
>      /*
>       * XXX should populate /chosen/linux,initrd-{start,end} here if we
>       * have module[2]

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Julien Grall]

On 09/09/2013 12:37 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> When a device has a property status with disabled inside, Linux will not use
>> the device.
>
> Do we not filter such devices out already?

We only filter gic/timer node. All the others nodes used by Xen, for 
instance the serial device, will be marked by status = "disabled".

> Or did the previous patch remove that functionality? In which case is
> there a bisection hazard here?

AFAIK, there is no bisection hazard with the current order of this patch 
series.

>
> Why is it preferred to leave the device with disabled in it instead of
> removing it? (Can this go in the changelog please)
> I think I've asked before, but is the handling of status = disabled
> generic and therefore reliable?

I let this patch because I didn't implement all the fake nodes in the 
previous patch series. In another hand, I'm not sure how Linux will 
react if there is a missing node and it should cope with status = 
"disabled" with most of nodes.

I will give a try and see if I can remove completely nodes used by Xen. 
Except, of course, gic and timer nodes which are recreated later.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/domain_build.c |    8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 76decf4..d56bc70a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>               return res;
>>       }
>>
>> +    /* Disable all devices used by Xen */
>> +    if ( dt_device_used_by(np) == DOMID_XEN )
>> +    {
>> +        res = fdt_property(kinfo->fdt, "status", "disabled", 8 + 1);
>> +        if ( res )
>> +            return res;
>> +    }
>> +
>>       /*
>>        * XXX should populate /chosen/linux,initrd-{start,end} here if we
>>        * have module[2]
>
>


-- 
Julien Grall

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Ian Campbell]

On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote:
> On 09/09/2013 12:37 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >> When a device has a property status with disabled inside, Linux will not use
> >> the device.
> >
> > Do we not filter such devices out already?
> 
> We only filter gic/timer node. All the others nodes used by Xen, for 
> instance the serial device, will be marked by status = "disabled".

Ah, the current code omits devices from the mapping but not the fdt, I
was confusing the two. In the current way of doing things
fdt_next_dom0_node would need a check for the owner, but that's not
relevant with this series.

> > Or did the previous patch remove that functionality? In which case is
> > there a bisection hazard here?
> 
> AFAIK, there is no bisection hazard with the current order of this patch 
> series.

Ack, thanks.

> 
> >
> > Why is it preferred to leave the device with disabled in it instead of
> > removing it? (Can this go in the changelog please)
> > I think I've asked before, but is the handling of status = disabled
> > generic and therefore reliable?
> 
> I let this patch because I didn't implement all the fake nodes in the 
> previous patch series. In another hand, I'm not sure how Linux will 
> react if there is a missing node and it should cope with status = 
> "disabled" with most of nodes.
> 
> I will give a try and see if I can remove completely nodes used by Xen. 
> Except, of course, gic and timer nodes which are recreated later.

If you want you can add that to the end of the series/future work rather
than reworking this patch in the middle of a large series.