Re: [PATCH RFC 16/35] ARM64 / ACPI: Parse GTDT to initialize timer

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

Hi Parth,

On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> Parse GTDT (Generic Timer Descriptor Table) to initialize timer.
> Using the information presented by GTDT to initialize the arch
> timer (not momery-mapped).

memory-mapped

> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>   xen/arch/arm/setup.c   |  6 +++++
>   xen/arch/arm/time.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   xen/include/xen/time.h |  1 +
>   3 files changed, 73 insertions(+)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 569b2da..af9f429 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -775,7 +775,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>       smp_init_cpus();
>       cpus = smp_get_max_cpus();
>
> +/* Comment for now take it after GIC initialization */
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM_64)
> +   init_xen_acpi_time();
> +#else
>       init_xen_time();
> +#endif

This is clearly wrong. A same Xen binary should run on both ACPI and DT 
when ACPI is enabled.

Also, I would take care of the different between ACPI and DT in 
init_xen_time, rather than ifdefining here. It will make the code more 
cleaner.

>       gic_init();
>
> @@ -789,6 +794,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       xsm_dt_init();
>
>       init_maintenance_interrupt();
> +

Spurious change.

>       init_timer_interrupt();
>
>       timer_init();
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 0add494..0d4c26d 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -29,6 +29,7 @@
>   #include <xen/time.h>
>   #include <xen/sched.h>
>   #include <xen/event.h>
> +#include <xen/acpi.h>
>   #include <asm/system.h>
>   #include <asm/time.h>
>   #include <asm/gic.h>
> @@ -239,6 +240,71 @@ struct tm wallclock_time(uint64_t *ns)
>       return (struct tm) { 0 };
>   }
>
> +#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)

#ifdef CONFIG_ACPI

> +#define ACPI_GTDT_INTR_MASK 0x3
> +
> +static int GTDT_INTRL_TAB[] ={ DT_IRQ_TYPE_LEVEL_HIGH, DT_IRQ_TYPE_EDGE_RISING,
> +                          DT_IRQ_TYPE_LEVEL_LOW ,DT_IRQ_TYPE_EDGE_FALLING};
> +
> +/* Initialize per-processor generic timer */
> +static int __init arch_timer_acpi_init(struct acpi_table_header *table)
> +{
> +    int res;
> +    int type;
> +    struct acpi_table_gtdt *gtdt;
> +
> +    gtdt = (struct acpi_table_gtdt *)table;
> +
> +    /* Initialize all the generic timer IRQ variable from GTDT table */
> +
> +    type = GTDT_INTRL_TAB[gtdt->non_secure_el1_flags & ACPI_GTDT_INTR_MASK];
> +    acpi_set_irq(gtdt->non_secure_el1_interrupt, type);
> +    timer_irq[TIMER_PHYS_NONSECURE_PPI] = gtdt->non_secure_el1_interrupt;
> +
> +    type = GTDT_INTRL_TAB[gtdt->secure_el1_flags & ACPI_GTDT_INTR_MASK];
> +    acpi_set_irq(gtdt->secure_el1_interrupt, type);
> +    timer_irq[TIMER_PHYS_SECURE_PPI] = gtdt->secure_el1_interrupt;
> +
> +    type = GTDT_INTRL_TAB[gtdt->non_secure_el2_flags & ACPI_GTDT_INTR_MASK];
> +    acpi_set_irq(gtdt->non_secure_el2_interrupt, type);
> +    timer_irq[TIMER_HYP_PPI] = gtdt->non_secure_el2_interrupt;
> +
> +    type = GTDT_INTRL_TAB[gtdt->virtual_timer_flags & ACPI_GTDT_INTR_MASK];
> +    acpi_set_irq(gtdt->virtual_timer_interrupt, type);
> +    timer_irq[TIMER_VIRT_PPI] = gtdt->virtual_timer_interrupt;
> +
> +    printk("Generic Timer IRQ from ACPI GTDT: 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();

The platform code is DT-centrict. Although, most of the code below is 
similar to the Timer DT initialization. Please factor it.

> +    if ( res )
> +        printk("Timer: Cannot initialize platform timer");
> +
> +    /* Check that this CPU supports the Generic Timer interface */
> +    if ( !cpu_has_gentimer )
> +        printk("CPU does not support the Generic Timer v1 interface");
> +
> +    cpu_khz = READ_SYSREG32(CNTFRQ_EL0) / 1000;
> +
> +    boot_count = READ_SYSREG64(CNTPCT_EL0);
> +
> +    printk("Using generic timer at %lu KHz\n", cpu_khz);
> +
> +    return 0;
> +}
> +
> +int __init init_xen_acpi_time(void)
> +{
> +   /* Initialize all the generic timers presented in GTDT */
> +   acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
> +   return 0;
> +}
> +
> +#endif
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
> index 709501f..4598a0c 100644
> --- a/xen/include/xen/time.h
> +++ b/xen/include/xen/time.h
> @@ -11,6 +11,7 @@
>   #include <xen/types.h>
>   #include <public/xen.h>
>
> +extern int init_xen_acpi_time(void);

This is ARM specific, please move the prototype in asm-arm/time.h.

>   extern int init_xen_time(void);
>   extern void cstate_restore_tsc(void);
>
>

Regards,

-- 
Julien Grall