Re: [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer.

[Andreas Färber]

Am 10.07.2013 07:08, schrieb address@hidden:
> From: Peter Crosthwaite <address@hidden>
> 
> The ARM A9 MPCore has a timer that is global to all CPUs in the mpcore.
> The timer is shared but each CPU has a private independent comparator
> and interrupt.
> 
> Original version contributed by Francois LEGAL.
> 
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> Francois, do you want to re-add your SOB? I have changed the device
> a lot since your V2.
> 
>  hw/timer/Makefile.objs |   2 +-
>  hw/timer/a9gtimer.c    | 437 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 438 insertions(+), 1 deletion(-)
>  create mode 100644 hw/timer/a9gtimer.c
> 
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 32b5c1a..8ed379e 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -25,5 +25,5 @@ obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o
>  obj-$(CONFIG_SH4) += sh_timer.o
>  obj-$(CONFIG_TUSB6010) += tusb6010.o
>  
> -obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
> +obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o a9gtimer.o
>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> new file mode 100644
> index 0000000..f996169
> --- /dev/null
> +++ b/hw/timer/a9gtimer.c
> @@ -0,0 +1,437 @@
> +/*
> + * Global peripheral timer block for ARM A9MP
> + *
> + * (C) 2013 Xilinx Inc.
> + *
> + * Written by François LEGAL
> + * Written by Peter Crosthwaite <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +
> +#ifndef ARM_GTIMER_ERR_DEBUG
> +#define ARM_GTIMER_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(level, ...) do { \
> +    if (ARM_GTIMER_ERR_DEBUG > (level)) { \
> +        fprintf(stderr,  ": %s: ", __func__); \
> +        fprintf(stderr, ## __VA_ARGS__); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)

I only spot DB_PRINT() being used below. Would be easier to grasp if it
were just #ifndef ARM_GTIMER_ERR_DEBUG #define
ARM_GTIMER_ERR_DEBUG_ENABLED 0 and if (ARM_GTIMER_ERR_DEBUG_ENABLED).

Either something is missing before ": %s: " or that's a duplicate.

> +
> +/* This device implements the Cortex-A9MP global timer. */
> +
> +#define MAX_CPUS 4
> +#define TYPE_ARM_GTIMER "arm.cortex-a9-global-timer"
> +#define ARM_GTIMER(obj) OBJECT_CHECK(A9GlobalTimerState, (obj), 
> TYPE_ARM_GTIMER)
> +
> +#define R_COUNTER_LO                0x00
> +#define R_COUNTER_HI                0x04
> +
> +#define R_CONTROL                   0x08
> +#define R_CONTROL_TIMER_ENABLE      (1 << 0)
> +#define R_CONTROL_COMP_ENABLE       (1 << 1)
> +#define R_CONTROL_IRQ_ENABLE        (1 << 2)
> +#define R_CONTROL_AUTO_INCREMENT    (1 << 2)
> +#define R_CONTROL_PRESCALER_SHIFT   8
> +#define R_CONTROL_PRESCALER_LEN     8
> +#define R_CONTROL_PRESCALER_MASK    (((1 << R_CONTROL_PRESCALER_LEN) - 1) << 
> \
> +                                    R_CONTROL_PRESCALER_SHIFT)
> +
> +#define R_CONTROL_BANKED            (R_CONTROL_COMP_ENABLE | \
> +                                     R_CONTROL_IRQ_ENABLE | \
> +                                     R_CONTROL_AUTO_INCREMENT)
> +#define R_CONTROL_NEEDS_SYNC        (R_CONTROL_TIMER_ENABLE | \
> +                                     R_CONTROL_PRESCALER_MASK)
> +
> +#define R_INTERRUPT_STATUS          0x0C
> +#define R_COMPARATOR_LO             0x10
> +#define R_COMPARATOR_HI             0x14
> +#define R_AUTO_INCREMENT            0x18
> +
> +typedef struct A9GlobalTimerPerCPU A9GlobalTimerPerCPU;
> +typedef struct A9GlobalTimerState A9GlobalTimerState;
> +
> +struct A9GlobalTimerPerCPU {
> +    A9GlobalTimerState *parent;
> +
> +    uint32_t control; /* only per cpu banked bits valid */
> +    uint64_t compare;
> +    uint32_t status;
> +    uint32_t inc;
> +
> +    MemoryRegion iomem;
> +    qemu_irq irq; /* PPI interrupts */
> +};
> +
> +struct A9GlobalTimerState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    /* static props */
> +    uint32_t num_cpu;
> +    QEMUTimer *timer;
> +
> +    uint64_t counter; /* current timer value */
> +
> +    uint64_t ref_counter;
> +    uint64_t cpu_ref_time; /* the cpu time as of last update of ref_counter 
> */
> +    uint32_t control; /* only non per cpu banked bits valid */
> +
> +    A9GlobalTimerPerCPU per_cpu[MAX_CPUS];
> +};

Can we move these to a new header from the start, while not using them
elsewhere yet? In that case please add the gtk-doc private/public markers.

> +
> +typedef struct A9GTimerUpdate {
> +    uint64_t now;
> +    uint64_t new;
> +} A9GTimerUpdate;
> +
> +static inline int a9_gtimer_get_current_cpu(A9GlobalTimerState *s)
> +{
> +    CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env);
> +
> +    if (cpu_single_cpu->cpu_index >= s->num_cpu) {
> +        hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
> +                 s->num_cpu, cpu_single_cpu->cpu_index);
> +    }
> +    return cpu_single_cpu->cpu_index;
> +}
[snip]

Seeing this, I have sent out my qom-cpu PULL, which replaces
cpu_single_env with current_cpu for your convenience.

Otherwise looks okay from a QOM/style viewpoint.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg