diff mbox series

[v6,2/2] xen/arm: consolidate make_timer_node and make_timer_domU_node

Message ID 20190805114332.15329-3-viktor.mitin.19@gmail.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Consolidate make_timer_node and make_timer_domU_node | expand

Commit Message

Viktor Mitin Aug. 5, 2019, 11:43 a.m. UTC 
Functions make_timer_node and make_timer_domU_node are quite similar.
So it is better to consolidate them to avoid discrepancy.
The main difference between the functions is the timer interrupts used.

Keep the domU version for the compatible as it is simpler:
do not copy platform's 'compatible' property into hwdom device tree,
instead set either arm,armv7-timer or arm,armv8-timer,
depending on the platform type.

Keep the hw version for the clock as it is relevant for the both cases.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
---
v4 updates:
   updated "Keep the domU version for the compatible as it is simpler"

v5 updates:
    - changed 'kept' to 'keep', etc.
    - removed empty line
    - updated indentation of parameters in functions calls
    - fixed NITs
    - updated commit message

v6 updates:
	- move if out of outer "if"
    - add full stop at the end of the last sentence
    - minor rephrase of commit message
---
 xen/arch/arm/domain_build.c | 100 +++++++++++++-----------------------
 1 file changed, 35 insertions(+), 65 deletions(-)

Comments

Volodymyr Babchuk Aug. 6, 2019, 2:12 p.m. UTC | #1 
Hi Viktor,

Viktor Mitin writes:

> Functions make_timer_node and make_timer_domU_node are quite similar.
> So it is better to consolidate them to avoid discrepancy.
> The main difference between the functions is the timer interrupts used.
>
> Keep the domU version for the compatible as it is simpler:
> do not copy platform's 'compatible' property into hwdom device tree,
> instead set either arm,armv7-timer or arm,armv8-timer,
> depending on the platform type.
>
> Keep the hw version for the clock as it is relevant for the both cases.
>
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
> ---
> v4 updates:
>    updated "Keep the domU version for the compatible as it is simpler"
>
> v5 updates:
>     - changed 'kept' to 'keep', etc.
>     - removed empty line
>     - updated indentation of parameters in functions calls
>     - fixed NITs
>     - updated commit message
>
> v6 updates:
> 	- move if out of outer "if"
>     - add full stop at the end of the last sentence
>     - minor rephrase of commit message
> ---
>  xen/arch/arm/domain_build.c | 100 +++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 65 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bc7d17dd2c..b9954d2c3c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -973,10 +973,8 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
>          { /* sentinel */ },
>      };
>      struct dt_device_node *dev;
> -    u32 len;
> -    const void *compatible;
>      int res;
> -    unsigned int irq;
> +    unsigned int irq[MAX_TIMER_PPI];
>      gic_interrupt_t intrs[3];
>      u32 clock_frequency;
>      bool clock_valid;
> @@ -990,35 +988,47 @@ static int __init make_timer_node(const struct kernel_info *kinfo)
>          return -FDT_ERR_XEN(ENOENT);
>      }
>  
> -    compatible = dt_get_property(dev, "compatible", &len);
> -    if ( !compatible )
> -    {
> -        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
> -        return -FDT_ERR_XEN(ENOENT);
> -    }
> -
>      res = fdt_begin_node(fdt, "timer");
>      if ( res )
>          return res;
>  
> -    res = fdt_property(fdt, "compatible", compatible, len);
> +    if ( !is_64bit_domain(kinfo->d) )
> +    {
> +        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> +    }
> +    else
> +    {
> +        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> +    }
This violates coding style:
"
Braces should be omitted for blocks with a single statement. e.g.,

if ( condition )
    single_statement();

"

>      if ( res )
>          return res;
>  
> -    /* The timer IRQ is emulated by Xen. It always exposes an active-low
> -     * level-sensitive interrupt */
> -
> -    irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> -    dt_dprintk("  Secure interrupt %u\n", irq);
> -    set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -
> -    irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> -    dt_dprintk("  Non secure interrupt %u\n", irq);
> -    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    /*
> +     * The timer IRQ is emulated by Xen.
> +     * It always exposes an active-low level-sensitive interrupt.
> +     */
>  
> -    irq = timer_get_irq(TIMER_VIRT_PPI);
> -    dt_dprintk("  Virt interrupt %u\n", irq);
> -    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    if ( is_hardware_domain(kinfo->d) )
> +    {
> +        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> +        irq[TIMER_PHYS_NONSECURE_PPI] =
> +                                    timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> +        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
> +    }
> +    else
> +    {
> +        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
> +        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
> +        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
> +    }
> +    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
> +    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
> +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]);
> +    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
> +                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
> +    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>  
>      res = fdt_property_interrupts(kinfo, intrs, 3);
>      if ( res )
> @@ -1603,46 +1613,6 @@ static int __init make_gic_domU_node(const struct domain *d, void *fdt)
>      }
>  }
>  
> -static int __init make_timer_domU_node(const struct domain *d, void *fdt)
> -{
> -    int res;
> -    gic_interrupt_t intrs[3];
> -
> -    res = fdt_begin_node(fdt, "timer");
> -    if ( res )
> -        return res;
> -
> -    if ( !is_64bit_domain(d) )
> -    {
> -        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
> -        if ( res )
> -            return res;
> -    }
> -    else
> -    {
> -        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
> -        if ( res )
> -            return res;
> -    }
> -
> -    set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -    set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> -
> -    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
> -    if ( res )
> -        return res;
> -
> -    res = fdt_property_cell(fdt, "interrupt-parent",
> -                            GUEST_PHANDLE_GIC);
> -    if (res)
> -        return res;
> -
> -    res = fdt_end_node(fdt);
> -
> -    return res;
> -}
> -
>  #ifdef CONFIG_SBSA_VUART_CONSOLE
>  static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
>  {
> @@ -1748,7 +1718,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
>  
> -    ret = make_timer_domU_node(d, kinfo->fdt);
> +    ret = make_timer_node(kinfo);
>      if ( ret )
>          goto err;
diff mbox series

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bc7d17dd2c..b9954d2c3c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -973,10 +973,8 @@  static int __init make_timer_node(const struct kernel_info *kinfo)
         { /* sentinel */ },
     };
     struct dt_device_node *dev;
-    u32 len;
-    const void *compatible;
     int res;
-    unsigned int irq;
+    unsigned int irq[MAX_TIMER_PPI];
     gic_interrupt_t intrs[3];
     u32 clock_frequency;
     bool clock_valid;
@@ -990,35 +988,47 @@  static int __init make_timer_node(const struct kernel_info *kinfo)
         return -FDT_ERR_XEN(ENOENT);
     }
 
-    compatible = dt_get_property(dev, "compatible", &len);
-    if ( !compatible )
-    {
-        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
-        return -FDT_ERR_XEN(ENOENT);
-    }
-
     res = fdt_begin_node(fdt, "timer");
     if ( res )
         return res;
 
-    res = fdt_property(fdt, "compatible", compatible, len);
+    if ( !is_64bit_domain(kinfo->d) )
+    {
+        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
+    }
+    else
+    {
+        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
+    }
     if ( res )
         return res;
 
-    /* The timer IRQ is emulated by Xen. It always exposes an active-low
-     * level-sensitive interrupt */
-
-    irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
-    dt_dprintk("  Secure interrupt %u\n", irq);
-    set_interrupt(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-
-    irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
-    dt_dprintk("  Non secure interrupt %u\n", irq);
-    set_interrupt(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    /*
+     * The timer IRQ is emulated by Xen.
+     * It always exposes an active-low level-sensitive interrupt.
+     */
 
-    irq = timer_get_irq(TIMER_VIRT_PPI);
-    dt_dprintk("  Virt interrupt %u\n", irq);
-    set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    if ( is_hardware_domain(kinfo->d) )
+    {
+        irq[TIMER_PHYS_SECURE_PPI] = timer_get_irq(TIMER_PHYS_SECURE_PPI);
+        irq[TIMER_PHYS_NONSECURE_PPI] =
+                                    timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
+        irq[TIMER_VIRT_PPI] = timer_get_irq(TIMER_VIRT_PPI);
+    }
+    else
+    {
+        irq[TIMER_PHYS_SECURE_PPI] = GUEST_TIMER_PHYS_S_PPI;
+        irq[TIMER_PHYS_NONSECURE_PPI] = GUEST_TIMER_PHYS_NS_PPI;
+        irq[TIMER_VIRT_PPI] = GUEST_TIMER_VIRT_PPI;
+    }
+    dt_dprintk("  Secure interrupt %u\n", irq[TIMER_PHYS_SECURE_PPI]);
+    set_interrupt(intrs[0], irq[TIMER_PHYS_SECURE_PPI],
+                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    dt_dprintk("  Non secure interrupt %u\n", irq[TIMER_PHYS_NONSECURE_PPI]);
+    set_interrupt(intrs[1], irq[TIMER_PHYS_NONSECURE_PPI],
+                  0xf, DT_IRQ_TYPE_LEVEL_LOW);
+    dt_dprintk("  Virt interrupt %u\n", irq[TIMER_VIRT_PPI]);
+    set_interrupt(intrs[2], irq[TIMER_VIRT_PPI], 0xf, DT_IRQ_TYPE_LEVEL_LOW);
 
     res = fdt_property_interrupts(kinfo, intrs, 3);
     if ( res )
@@ -1603,46 +1613,6 @@  static int __init make_gic_domU_node(const struct domain *d, void *fdt)
     }
 }
 
-static int __init make_timer_domU_node(const struct domain *d, void *fdt)
-{
-    int res;
-    gic_interrupt_t intrs[3];
-
-    res = fdt_begin_node(fdt, "timer");
-    if ( res )
-        return res;
-
-    if ( !is_64bit_domain(d) )
-    {
-        res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
-        if ( res )
-            return res;
-    }
-    else
-    {
-        res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
-        if ( res )
-            return res;
-    }
-
-    set_interrupt(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    set_interrupt(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    set_interrupt(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-
-    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
-    if ( res )
-        return res;
-
-    res = fdt_property_cell(fdt, "interrupt-parent",
-                            GUEST_PHANDLE_GIC);
-    if (res)
-        return res;
-
-    res = fdt_end_node(fdt);
-
-    return res;
-}
-
 #ifdef CONFIG_SBSA_VUART_CONSOLE
 static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
 {
@@ -1748,7 +1718,7 @@  static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         goto err;
 
-    ret = make_timer_domU_node(d, kinfo->fdt);
+    ret = make_timer_node(kinfo);
     if ( ret )
         goto err;