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

[Julien Grall <julien.grall@linaro.org>]

Hi Oleksandr,

Thank you for the patch. See few comments below.

On 23/01/15 15:49, 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.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> CC: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/setup.c   |  2 ++
>  xen/arch/arm/time.c    | 71 ++++++++++++++++++++++++++++++++------------------
>  xen/include/xen/time.h |  1 +
>  3 files changed, 48 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index f49569d..dbd7d5a 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -743,6 +743,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      init_IRQ();
>  
> +    preinit_xen_time();
> +
>      dt_uart_init();
>      console_init_preirq();
>      console_init_ring();
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 455f217..b2560db 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -61,25 +61,56 @@ 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 __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);
> +}

If you store the device tree pointer into a static variable, you won't
able to look at twice the node (in preinit_xen_time and init_xen_time).

> +
> +/* Set up the timer on the boot CPU (early init function) */
> +int __init preinit_xen_time(void)

You always return 0 and never check the return. Please use 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);
> +    dev = find_timer_node();
>      if ( !dev )
>          panic("Unable to find a compatible timer in the device tree");
>  
>      dt_device_set_used_by(dev, DOMID_XEN);
>  
> +    res = platform_init_time();

If you move platform_init_time in preinit_xen_time, you also have to
move platform_init before calling preinit_xen_time.

Regards,

-- 
Julien Grall