From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40997) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwvoz-0007qn-7d for qemu-devel@nongnu.org; Wed, 10 Jul 2013 10:56:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uwvow-0003H5-M9 for qemu-devel@nongnu.org; Wed, 10 Jul 2013 10:56:45 -0400 Received: from cantor2.suse.de ([195.135.220.15]:33872 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uwvow-0003G9-7f for qemu-devel@nongnu.org; Wed, 10 Jul 2013 10:56:42 -0400 Message-ID: <51DD7626.607@suse.de> Date: Wed, 10 Jul 2013 16:56:38 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH arm-devs v1 1/2] hw/timer: Introduce ARM A9 Global Timer. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: peter.crosthwaite@xilinx.com Cc: devel@thom.fr.eu.org, peter.maydell@linaro.org, qemu-devel@nongnu.org Am 10.07.2013 07:08, schrieb peter.crosthwaite@xilinx.com: > From: Peter Crosthwaite >=20 > 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. >=20 > Original version contributed by Francois LEGAL. >=20 > Signed-off-by: Peter Crosthwaite > --- > Francois, do you want to re-add your SOB? I have changed the device > a lot since your V2. >=20 > hw/timer/Makefile.objs | 2 +- > hw/timer/a9gtimer.c | 437 +++++++++++++++++++++++++++++++++++++++++= ++++++++ > 2 files changed, 438 insertions(+), 1 deletion(-) > create mode 100644 hw/timer/a9gtimer.c >=20 > 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) +=3D pxa2xx_timer.o > obj-$(CONFIG_SH4) +=3D sh_timer.o > obj-$(CONFIG_TUSB6010) +=3D tusb6010.o > =20 > -obj-$(CONFIG_ARM_MPTIMER) +=3D arm_mptimer.o > +obj-$(CONFIG_ARM_MPTIMER) +=3D arm_mptimer.o a9gtimer.o > obj-$(CONFIG_MC146818RTC) +=3D 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=C3=A7ois LEGAL > + * Written by Peter Crosthwaite > + * > + * 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 a= long > + * with this program; if not, see . > + */ > + > +#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_A= RM_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_co= unter */ > + 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 =3D ENV_GET_CPU(cpu_single_env); > + > + if (cpu_single_cpu->cpu_index >=3D 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg