[RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Oleksandr Tyshchenko]

Create preinit_xen_time() and move to it minimum required
subset of operations needed to properly initialized
cpu_khz and boot_count vars. This is allow us to use udelay()
immediately after the call.

Changes in v2:
1. Move timer_ids out of the find_timer_node() and frop this func.
2. Use void as a return value for preinit_xen_time().
3. Move platform_init() before calling preinit_xen_time().

Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
CC: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/setup.c   |  6 +++--
 xen/arch/arm/time.c    | 62 ++++++++++++++++++++++++++++++--------------------
 xen/include/xen/time.h |  1 +
 3 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f49569d..a916ca6 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -743,6 +743,10 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     init_IRQ();
 
+    platform_init();
+
+    preinit_xen_time();
+
     dt_uart_init();
     console_init_preirq();
     console_init_ring();
@@ -751,8 +755,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     processor_id();
 
-    platform_init();
-
     smp_init_cpus();
     cpus = smp_get_max_cpus();
 
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 0add494..d442e61 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -61,17 +61,17 @@ unsigned int timer_get_irq(enum timer_ppi ppi)
     return muldiv64(ns, 1000 * cpu_khz, SECONDS(1));
 }
 
-/* Set up the timer on the boot CPU */
-int __init init_xen_time(void)
+static const struct dt_device_match timer_ids[] __initconst =
+{
+	DT_MATCH_TIMER,
+	{ /* sentinel */ },
+};
+
+/* Set up the timer on the boot CPU (early init function) */
+void __init preinit_xen_time(void)
 {
-    static const struct dt_device_match timer_ids[] __initconst =
-    {
-        DT_MATCH_TIMER,
-        { /* sentinel */ },
-    };
     struct dt_device_node *dev;
     int res;
-    unsigned int i;
     u32 rate;
 
     dev = dt_find_matching_node(NULL, timer_ids);
@@ -80,6 +80,30 @@ int __init init_xen_time(void)
 
     dt_device_set_used_by(dev, DOMID_XEN);
 
+    res = platform_init_time();
+    if ( res )
+        panic("Timer: Cannot initialize platform timer");
+
+    res = dt_property_read_u32(dev, "clock-frequency", &rate);
+    if ( res )
+        cpu_khz = rate / 1000;
+    else
+        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
+
+    boot_count = READ_SYSREG64(CNTPCT_EL0);
+}
+
+/* Set up the timer on the boot CPU (late init function) */
+int __init init_xen_time(void)
+{
+    struct dt_device_node *dev;
+    int res;
+    unsigned int i;
+
+    dev = dt_find_matching_node(NULL, timer_ids);
+    if ( !dev )
+        panic("Unable to find a compatible timer in the device tree");
+
     /* Retrieve all IRQs for the timer */
     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
     {
@@ -90,27 +114,15 @@ int __init init_xen_time(void)
         timer_irq[i] = res;
     }
 
-    printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u\n",
-           timer_irq[TIMER_PHYS_NONSECURE_PPI],
-           timer_irq[TIMER_HYP_PPI],
-           timer_irq[TIMER_VIRT_PPI]);
-
-    res = platform_init_time();
-    if ( res )
-        panic("Timer: Cannot initialize platform timer");
-
     /* Check that this CPU supports the Generic Timer interface */
     if ( !cpu_has_gentimer )
         panic("CPU does not support the Generic Timer v1 interface");
 
-    res = dt_property_read_u32(dev, "clock-frequency", &rate);
-    if ( res )
-        cpu_khz = rate / 1000;
-    else
-        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
-
-    boot_count = READ_SYSREG64(CNTPCT_EL0);
-    printk("Using generic timer at %lu KHz\n", cpu_khz);
+    printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u Freq: %lu KHz\n",
+           timer_irq[TIMER_PHYS_NONSECURE_PPI],
+           timer_irq[TIMER_HYP_PPI],
+           timer_irq[TIMER_VIRT_PPI],
+           cpu_khz);
 
     return 0;
 }
diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
index 709501f..bb6259d 100644
--- a/xen/include/xen/time.h
+++ b/xen/include/xen/time.h
@@ -12,6 +12,7 @@
 #include <public/xen.h>
 
 extern int init_xen_time(void);
+void preinit_xen_time(void);
 extern void cstate_restore_tsc(void);
 
 extern unsigned long cpu_khz;
-- 
1.9.1

Re: [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Ian Campbell]

On Tue, 2015-01-27 at 15:39 +0200, Oleksandr Tyshchenko wrote:
> Create preinit_xen_time() and move to it minimum required
> subset of operations needed to properly initialized
> cpu_khz and boot_count vars. This is allow us to use udelay()
> immediately after the call.
> 
> Changes in v2:
> 1. Move timer_ids out of the find_timer_node() and frop this func.
> 2. Use void as a return value for preinit_xen_time().
> 3. Move platform_init() before calling preinit_xen_time().

Please put these after the first --- so they get cut from the eventual
commit log.

Apart from this the patch looks good to me, is there a reason I'm not
aware of that it is still RFC?

> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> CC: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/setup.c   |  6 +++--
>  xen/arch/arm/time.c    | 62 ++++++++++++++++++++++++++++++--------------------
>  xen/include/xen/time.h |  1 +
>  3 files changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index f49569d..a916ca6 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -743,6 +743,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      init_IRQ();
>  
> +    platform_init();
> +
> +    preinit_xen_time();
> +
>      dt_uart_init();
>      console_init_preirq();
>      console_init_ring();
> @@ -751,8 +755,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      processor_id();
>  
> -    platform_init();
> -
>      smp_init_cpus();
>      cpus = smp_get_max_cpus();
>  
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 0add494..d442e61 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -61,17 +61,17 @@ unsigned int timer_get_irq(enum timer_ppi ppi)
>      return muldiv64(ns, 1000 * cpu_khz, SECONDS(1));
>  }
>  
> -/* Set up the timer on the boot CPU */
> -int __init init_xen_time(void)
> +static const struct dt_device_match timer_ids[] __initconst =
> +{
> +	DT_MATCH_TIMER,
> +	{ /* sentinel */ },
> +};
> +
> +/* Set up the timer on the boot CPU (early init function) */
> +void __init preinit_xen_time(void)
>  {
> -    static const struct dt_device_match timer_ids[] __initconst =
> -    {
> -        DT_MATCH_TIMER,
> -        { /* sentinel */ },
> -    };
>      struct dt_device_node *dev;
>      int res;
> -    unsigned int i;
>      u32 rate;
>  
>      dev = dt_find_matching_node(NULL, timer_ids);
> @@ -80,6 +80,30 @@ int __init init_xen_time(void)
>  
>      dt_device_set_used_by(dev, DOMID_XEN);
>  
> +    res = platform_init_time();
> +    if ( res )
> +        panic("Timer: Cannot initialize platform timer");
> +
> +    res = dt_property_read_u32(dev, "clock-frequency", &rate);
> +    if ( res )
> +        cpu_khz = rate / 1000;
> +    else
> +        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
> +
> +    boot_count = READ_SYSREG64(CNTPCT_EL0);
> +}
> +
> +/* Set up the timer on the boot CPU (late init function) */
> +int __init init_xen_time(void)
> +{
> +    struct dt_device_node *dev;
> +    int res;
> +    unsigned int i;
> +
> +    dev = dt_find_matching_node(NULL, timer_ids);
> +    if ( !dev )
> +        panic("Unable to find a compatible timer in the device tree");
> +
>      /* Retrieve all IRQs for the timer */
>      for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>      {
> @@ -90,27 +114,15 @@ int __init init_xen_time(void)
>          timer_irq[i] = res;
>      }
>  
> -    printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u\n",
> -           timer_irq[TIMER_PHYS_NONSECURE_PPI],
> -           timer_irq[TIMER_HYP_PPI],
> -           timer_irq[TIMER_VIRT_PPI]);
> -
> -    res = platform_init_time();
> -    if ( res )
> -        panic("Timer: Cannot initialize platform timer");
> -
>      /* Check that this CPU supports the Generic Timer interface */
>      if ( !cpu_has_gentimer )
>          panic("CPU does not support the Generic Timer v1 interface");
>  
> -    res = dt_property_read_u32(dev, "clock-frequency", &rate);
> -    if ( res )
> -        cpu_khz = rate / 1000;
> -    else
> -        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
> -
> -    boot_count = READ_SYSREG64(CNTPCT_EL0);
> -    printk("Using generic timer at %lu KHz\n", cpu_khz);
> +    printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u Freq: %lu KHz\n",
> +           timer_irq[TIMER_PHYS_NONSECURE_PPI],
> +           timer_irq[TIMER_HYP_PPI],
> +           timer_irq[TIMER_VIRT_PPI],
> +           cpu_khz);
>  
>      return 0;
>  }
> diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
> index 709501f..bb6259d 100644
> --- a/xen/include/xen/time.h
> +++ b/xen/include/xen/time.h
> @@ -12,6 +12,7 @@
>  #include <public/xen.h>
>  
>  extern int init_xen_time(void);
> +void preinit_xen_time(void);
>  extern void cstate_restore_tsc(void);
>  
>  extern unsigned long cpu_khz;

Re: [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Oleksandr Tyshchenko]

Hi, Ian

On Tue, Jan 27, 2015 at 6:43 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2015-01-27 at 15:39 +0200, Oleksandr Tyshchenko wrote:
>> Create preinit_xen_time() and move to it minimum required
>> subset of operations needed to properly initialized
>> cpu_khz and boot_count vars. This is allow us to use udelay()
>> immediately after the call.
>>
>> Changes in v2:
>> 1. Move timer_ids out of the find_timer_node() and frop this func.
>> 2. Use void as a return value for preinit_xen_time().
>> 3. Move platform_init() before calling preinit_xen_time().
>
> Please put these after the first --- so they get cut from the eventual
> commit log.
ok

>
> Apart from this the patch looks good to me, is there a reason I'm not
> aware of that it is still RFC?
No, it isn't.
I forgot to drop RFC when I was creating v2.

>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>> CC: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/setup.c   |  6 +++--
>>  xen/arch/arm/time.c    | 62 ++++++++++++++++++++++++++++++--------------------
>>  xen/include/xen/time.h |  1 +
>>  3 files changed, 42 insertions(+), 27 deletions(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index f49569d..a916ca6 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -743,6 +743,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>>      init_IRQ();
>>
>> +    platform_init();
>> +
>> +    preinit_xen_time();
>> +
>>      dt_uart_init();
>>      console_init_preirq();
>>      console_init_ring();
>> @@ -751,8 +755,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>>
>>      processor_id();
>>
>> -    platform_init();
>> -
>>      smp_init_cpus();
>>      cpus = smp_get_max_cpus();
>>
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index 0add494..d442e61 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -61,17 +61,17 @@ unsigned int timer_get_irq(enum timer_ppi ppi)
>>      return muldiv64(ns, 1000 * cpu_khz, SECONDS(1));
>>  }
>>
>> -/* Set up the timer on the boot CPU */
>> -int __init init_xen_time(void)
>> +static const struct dt_device_match timer_ids[] __initconst =
>> +{
>> +     DT_MATCH_TIMER,
>> +     { /* sentinel */ },
>> +};
>> +
>> +/* Set up the timer on the boot CPU (early init function) */
>> +void __init preinit_xen_time(void)
>>  {
>> -    static const struct dt_device_match timer_ids[] __initconst =
>> -    {
>> -        DT_MATCH_TIMER,
>> -        { /* sentinel */ },
>> -    };
>>      struct dt_device_node *dev;
>>      int res;
>> -    unsigned int i;
>>      u32 rate;
>>
>>      dev = dt_find_matching_node(NULL, timer_ids);
>> @@ -80,6 +80,30 @@ int __init init_xen_time(void)
>>
>>      dt_device_set_used_by(dev, DOMID_XEN);
>>
>> +    res = platform_init_time();
>> +    if ( res )
>> +        panic("Timer: Cannot initialize platform timer");
>> +
>> +    res = dt_property_read_u32(dev, "clock-frequency", &rate);
>> +    if ( res )
>> +        cpu_khz = rate / 1000;
>> +    else
>> +        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
>> +
>> +    boot_count = READ_SYSREG64(CNTPCT_EL0);
>> +}
>> +
>> +/* Set up the timer on the boot CPU (late init function) */
>> +int __init init_xen_time(void)
>> +{
>> +    struct dt_device_node *dev;
>> +    int res;
>> +    unsigned int i;
>> +
>> +    dev = dt_find_matching_node(NULL, timer_ids);
>> +    if ( !dev )
>> +        panic("Unable to find a compatible timer in the device tree");
>> +
>>      /* Retrieve all IRQs for the timer */
>>      for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>      {
>> @@ -90,27 +114,15 @@ int __init init_xen_time(void)
>>          timer_irq[i] = res;
>>      }
>>
>> -    printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u\n",
>> -           timer_irq[TIMER_PHYS_NONSECURE_PPI],
>> -           timer_irq[TIMER_HYP_PPI],
>> -           timer_irq[TIMER_VIRT_PPI]);
>> -
>> -    res = platform_init_time();
>> -    if ( res )
>> -        panic("Timer: Cannot initialize platform timer");
>> -
>>      /* Check that this CPU supports the Generic Timer interface */
>>      if ( !cpu_has_gentimer )
>>          panic("CPU does not support the Generic Timer v1 interface");
>>
>> -    res = dt_property_read_u32(dev, "clock-frequency", &rate);
>> -    if ( res )
>> -        cpu_khz = rate / 1000;
>> -    else
>> -        cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
>> -
>> -    boot_count = READ_SYSREG64(CNTPCT_EL0);
>> -    printk("Using generic timer at %lu KHz\n", cpu_khz);
>> +    printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u Freq: %lu KHz\n",
>> +           timer_irq[TIMER_PHYS_NONSECURE_PPI],
>> +           timer_irq[TIMER_HYP_PPI],
>> +           timer_irq[TIMER_VIRT_PPI],
>> +           cpu_khz);
>>
>>      return 0;
>>  }
>> diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
>> index 709501f..bb6259d 100644
>> --- a/xen/include/xen/time.h
>> +++ b/xen/include/xen/time.h
>> @@ -12,6 +12,7 @@
>>  #include <public/xen.h>
>>
>>  extern int init_xen_time(void);
>> +void preinit_xen_time(void);
>>  extern void cstate_restore_tsc(void);
>>
>>  extern unsigned long cpu_khz;
>
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

Re: [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Julien Grall]

Hi Oleksandr,

On 27/01/15 13:39, Oleksandr Tyshchenko wrote:
> -/* Set up the timer on the boot CPU */
> -int __init init_xen_time(void)
> +static const struct dt_device_match timer_ids[] __initconst =
> +{
> +	DT_MATCH_TIMER,
> +	{ /* sentinel */ },
> +};
> +
> +/* Set up the timer on the boot CPU (early init function) */
> +void __init preinit_xen_time(void)
>  {
> -    static const struct dt_device_match timer_ids[] __initconst =
> -    {
> -        DT_MATCH_TIMER,
> -        { /* sentinel */ },
> -    };

I guess this is a left-over from the previous version?
I would keep the definition of the variable here.

Otherwise this patch looks good to me.

Regards,

-- 
Julien Grall

Re: [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Oleksandr Tyshchenko]

On Tue, Jan 27, 2015 at 7:09 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Oleksandr,
Hi Julien

>
> On 27/01/15 13:39, Oleksandr Tyshchenko wrote:
>> -/* Set up the timer on the boot CPU */
>> -int __init init_xen_time(void)
>> +static const struct dt_device_match timer_ids[] __initconst =
>> +{
>> +     DT_MATCH_TIMER,
>> +     { /* sentinel */ },
>> +};
>> +
>> +/* Set up the timer on the boot CPU (early init function) */
>> +void __init preinit_xen_time(void)
>>  {
>> -    static const struct dt_device_match timer_ids[] __initconst =
>> -    {
>> -        DT_MATCH_TIMER,
>> -        { /* sentinel */ },
>> -    };
>
> I guess this is a left-over from the previous version?
> I would keep the definition of the variable here.
Sorry, I am not sure that I fully understand you.

in v1 I created separate find_timer_node() a move to it timer_ids,
where the reason is to not duplicate it in init_xen_time() and
preinit_xen_time().

+static __init struct dt_device_node *find_timer_node(void)
+{
+       static const struct dt_device_match timer_ids[] __initconst =
+       {
+           DT_MATCH_TIMER,
+           { /* sentinel */ },
+       };
+
+       return dt_find_matching_node(NULL, timer_ids);
+}

in v2 taking into account your comment about "static" I decided to not
introduce additional func whose purpose is just to return pointer and
drop it.

>
> Otherwise this patch looks good to me.
>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

Re: [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Julien Grall]

On 27/01/15 17:52, Oleksandr Tyshchenko wrote:
> On Tue, Jan 27, 2015 at 7:09 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hi Oleksandr,
> Hi Julien
> 
>>
>> On 27/01/15 13:39, Oleksandr Tyshchenko wrote:
>>> -/* Set up the timer on the boot CPU */
>>> -int __init init_xen_time(void)
>>> +static const struct dt_device_match timer_ids[] __initconst =
>>> +{
>>> +     DT_MATCH_TIMER,
>>> +     { /* sentinel */ },
>>> +};
>>> +
>>> +/* Set up the timer on the boot CPU (early init function) */
>>> +void __init preinit_xen_time(void)
>>>  {
>>> -    static const struct dt_device_match timer_ids[] __initconst =
>>> -    {
>>> -        DT_MATCH_TIMER,
>>> -        { /* sentinel */ },
>>> -    };
>>
>> I guess this is a left-over from the previous version?
>> I would keep the definition of the variable here.
> Sorry, I am not sure that I fully understand you.
> 
> in v1 I created separate find_timer_node() a move to it timer_ids,
> where the reason is to not duplicate it in init_xen_time() and
> preinit_xen_time().
> 
> +static __init struct dt_device_node *find_timer_node(void)
> +{
> +       static const struct dt_device_match timer_ids[] __initconst =
> +       {
> +           DT_MATCH_TIMER,
> +           { /* sentinel */ },
> +       };
> +
> +       return dt_find_matching_node(NULL, timer_ids);
> +}
> 
> in v2 taking into account your comment about "static" I decided to not
> introduce additional func whose purpose is just to return pointer and
> drop it.

I think you misunderstood my comment on the V1. You don't need to look
twice for the device node. You can store it outside the function.

What I asked was something like:

static __initdata struct dt_device_node *timer;

void __init preinit_xen_time(void)
{
	static const struct dt_device_match timer_ids[] ... =
		...
	
	timer = dt_find_matching_node(NULL, timer_ids);
	if (!timer)
		panic("...");

}

init __init init_xen_time(void)
{

	/* Use timer without checking */
}

Regards,

-- 
Julien Grall

Re: [RFC PATCH v2] xen/arm: split the init_xen_time() in 2 parts

[Oleksandr Tyshchenko]

On Tue, Jan 27, 2015 at 8:00 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 27/01/15 17:52, Oleksandr Tyshchenko wrote:
>> On Tue, Jan 27, 2015 at 7:09 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>> Hi Oleksandr,
>> Hi Julien
>>
>>>
>>> On 27/01/15 13:39, Oleksandr Tyshchenko wrote:
>>>> -/* Set up the timer on the boot CPU */
>>>> -int __init init_xen_time(void)
>>>> +static const struct dt_device_match timer_ids[] __initconst =
>>>> +{
>>>> +     DT_MATCH_TIMER,
>>>> +     { /* sentinel */ },
>>>> +};
>>>> +
>>>> +/* Set up the timer on the boot CPU (early init function) */
>>>> +void __init preinit_xen_time(void)
>>>>  {
>>>> -    static const struct dt_device_match timer_ids[] __initconst =
>>>> -    {
>>>> -        DT_MATCH_TIMER,
>>>> -        { /* sentinel */ },
>>>> -    };
>>>
>>> I guess this is a left-over from the previous version?
>>> I would keep the definition of the variable here.
>> Sorry, I am not sure that I fully understand you.
>>
>> in v1 I created separate find_timer_node() a move to it timer_ids,
>> where the reason is to not duplicate it in init_xen_time() and
>> preinit_xen_time().
>>
>> +static __init struct dt_device_node *find_timer_node(void)
>> +{
>> +       static const struct dt_device_match timer_ids[] __initconst =
>> +       {
>> +           DT_MATCH_TIMER,
>> +           { /* sentinel */ },
>> +       };
>> +
>> +       return dt_find_matching_node(NULL, timer_ids);
>> +}
>>
>> in v2 taking into account your comment about "static" I decided to not
>> introduce additional func whose purpose is just to return pointer and
>> drop it.
>
> I think you misunderstood my comment on the V1.
You are right.

> You don't need to look twice for the device node. You can store it outside the function.
>
> What I asked was something like:
>
> static __initdata struct dt_device_node *timer;
>
> void __init preinit_xen_time(void)
> {
>         static const struct dt_device_match timer_ids[] ... =
>                 ...
>
>         timer = dt_find_matching_node(NULL, timer_ids);
>         if (!timer)
>                 panic("...");
>
> }
>
> init __init init_xen_time(void)
> {
>
>         /* Use timer without checking */
> }
Yes, it is really better.


>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com