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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

On Mon, Jan 26, 2015 at 3:32 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Oleksandr,
Hi Julien
Thank you for your comments.
>
> 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@xxxxxxxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxxxxx>
>> ---
>>  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).
Agree.

>
>> +
>> +/* 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.
ok

>
>>  {
>> -    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.
ok

>
> Regards,
>
> --
> Julien Grall



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel