* [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer
@ 2013-04-15 15:41 François Legal
2013-04-16 9:50 ` Peter Maydell
2013-04-16 13:29 ` [Qemu-devel] [PATCH] " Peter Crosthwaite
0 siblings, 2 replies; 10+ messages in thread
From: François Legal @ 2013-04-15 15:41 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 14385 bytes --]
Hello,
I made up this patch to implement the Cortex A9 global timer in
Qemu.
My patch is based on the Qemu branch maintained by Xilinx for the Zynq.
diff -urN qemu-master/hw/cpu/a9mpcore.c qemu-master.new/hw/cpu/a9mpcore.c
---
qemu-master/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200
+++
qemu-master.new/hw/cpu/a9mpcore.c 2013-04-15 12:54:06.000000000 +0200
@@ -15,6
+15,7 @@
uint32_t num_cpu;
MemoryRegion container;
DeviceState *mptimer;
+
DeviceState *mpgtimer;
DeviceState *wdt;
DeviceState *gic;
DeviceState
*scu;
@@ -31,6 +32,7 @@
{
A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
+ SysBusDevice
*gtimerbusdev;
int i;
s->gic = qdev_create(NULL, "arm_gic");
@@ -50,6 +52,11
@@
qdev_init_nofail(s->scu);
scubusdev = SYS_BUS_DEVICE(s->scu);
+
s->mpgtimer = qdev_create(NULL, "arm_mp_globaltimer");
+
qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
+
qdev_init_nofail(s->mpgtimer);
+ gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
+
s->mptimer = qdev_create(NULL, "arm_mptimer");
qdev_prop_set_uint32(s->mptimer,
"num-cpu", s->num_cpu);
qdev_init_nofail(s->mptimer);
@@ -68,8 +75,6 @@
*
0x0600-0x06ff -- private timers and watchdogs
* 0x0700-0x0fff -- nothing
*
0x1000-0x1fff -- GIC Distributor
- *
- * We should implement the global timer
but don't currently do so.
*/
memory_region_init(&s->container,
"a9mp-priv-container", 0x2000);
memory_region_add_subregion(&s->container,
0,
@@ -80,6 +85,8 @@
/* Note that the A9 exposes only the "timer/watchdog for
this core"
* memory region, not the "timer/watchdog for core X" ones 11MPcore
has.
*/
+ memory_region_add_subregion(&s->container, 0x200,
+
sysbus_mmio_get_region(gtimerbusdev, 0));
memory_region_add_subregion(&s->container, 0x600,
sysbus_mmio_get_region(timerbusdev, 0));
memory_region_add_subregion(&s->container, 0x620,
@@ -90,10 +97,13 @@
sysbus_init_mmio(dev, &s->container);
/* Wire up the interrupt from each
watchdog and timer.
- * For each core the timer is PPI 29 and the watchdog PPI
30.
+ * For each core the global timer is PPI 27, the private
+ * timer is PPI
29 and the watchdog PPI 30.
*/
for (i = 0; i < s->num_cpu; i++) {
int ppibase
= (s->num_irq - 32) + i * 32;
+ sysbus_connect_irq(gtimerbusdev, i,
+
qdev_get_gpio_in(s->gic, ppibase + 27));
sysbus_connect_irq(timerbusdev, i,
qdev_get_gpio_in(s->gic, ppibase + 29));
sysbus_connect_irq(wdtbusdev, i,
diff
-urN qemu-master/hw/timer/arm_mpgtimer.c
qemu-master.new/hw/timer/arm_mpgtimer.c
--- qemu-master/hw/timer/arm_mpgtimer.c
1970-01-01 01:00:00.000000000 +0100
+++ qemu-master.new/hw/timer/arm_mpgtimer.c
2013-04-15 13:56:23.000000000 +0200
@@ -0,0 +1,359 @@
+/*
+ * Global peripheral
timer block for ARM 11MPCore and A9MP
+ *
+ * Written by François LEGAL
+ *
+ *
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"
+
+/* This device implements the per-cpu private timer and
watchdog block
+ * which is used in both the ARM11MPCore and Cortex-A9MP.
+
*/
+
+#define MAX_CPUS 4
+
+/* State of a single gtimer or block */
+typedef
struct {
+ uint32_t control;
+ uint64_t compare;
+ uint32_t inc;
+ uint32_t
status;
+ int64_t tick;
+
+ int64_t delta;
+ uint64_t *gtimer_counter;
+
uint32_t *gtimer_control;
+
+ QEMUTimer *timer;
+ MemoryRegion iomem;
+ qemu_irq
irq;
+} gTimerBlock;
+
+typedef struct {
+ SysBusDevice busdev;
+ uint32_t
num_cpu;
+ uint64_t gtimer_counter;
+ uint32_t gtimer_control;
+ gTimerBlock
gtimer[MAX_CPUS];
+ MemoryRegion iomem;
+} ARMMPGTimerState;
+
+static inline
int get_current_cpu(ARMMPGTimerState *s)
+{
+ CPUState *cpu_single_cpu;
+
+ if
(cpu_single_env != NULL) {
+ 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;
+ } else {
+ return 0;
+ }
+}
+
+static inline void
gtimerblock_update_irq(gTimerBlock *gtb)
+{
+ qemu_set_irq(gtb->irq,
gtb->status);
+}
+
+/* Return conversion factor from mpcore timer ticks to qemu
timer ticks. */
+static inline uint32_t gtimerblock_scale(gTimerBlock *gtb)
+{
+
return ((((*gtb->gtimer_control) >> 8) & 0xff) + 1) * 10;
+}
+
+static void
gtimerblock_reload(gTimerBlock *gtb, int restart)
+{
+ if (restart) {
+
gtb->tick = qemu_get_clock_ns(vm_clock);
+ gtb->tick += (int64_t)(((gtb->compare
- *gtb->gtimer_counter) +
+ gtb->delta) * gtimerblock_scale(gtb));
+ } else {
+
gtb->tick += (int64_t)(gtb->inc * gtimerblock_scale(gtb));
+ }
+
qemu_mod_timer(gtb->timer, gtb->tick);
+}
+
+static void gtimerblock_tick(void
*opaque)
+{
+ gTimerBlock *gtb = (gTimerBlock *)opaque;
+ *gtb->gtimer_counter =
gtb->compare;
+ if ((gtb->control | *gtb->gtimer_control) & 0x9) {
+
gtb->compare += gtb->inc;
+ gtimerblock_reload(gtb, 0);
+ }
+ if (((gtb->control
| *gtb->gtimer_control) & 0x7) &&
+ ((gtb->status & 1) == 0)) {
+ gtb->status =
1;
+ gtimerblock_update_irq(gtb);
+ }
+}
+
+static uint64_t gtimer_read(void
*opaque, hwaddr addr,
+ unsigned size)
+{
+ gTimerBlock *gtb = (gTimerBlock
*)opaque;
+ int64_t val = 0;
+ switch (addr) {
+ case 0: /* Counter LSB */
+ if
(*gtb->gtimer_control & 1) {
+ val = gtb->tick - qemu_get_clock_ns(vm_clock);
+
val /= gtimerblock_scale(gtb);
+ }
+ return val < 0 ? val : 0 & 0xFFFFFFFF;
+
case 4: /* Counter MSB. */
+ if (*gtb->gtimer_control & 1) {
+ val = gtb->tick -
qemu_get_clock_ns(vm_clock);
+ val /= gtimerblock_scale(gtb);
+ }
+ return val <
0 ? (val >> 32) : 0;
+ case 8: /* Control. */
+ return (gtb->control &
0x0000000E) |
+ ((*gtb->gtimer_control) & 0x0000FF01);
+ case 12: /* Interrupt
status. */
+ return gtb->status;
+ case 16: /* Compare LSB */
+ return
gtb->compare & 0xFFFFFFFF;
+ case 20: /* Counter MSB */
+ return gtb->compare >>
32;
+ case 24: /* Autoincrement */
+ return gtb->inc;
+ default:
+ return 0;
+
}
+}
+
+static void gtimer_write(void *opaque, hwaddr addr,
+ uint64_t value,
unsigned size)
+{
+ gTimerBlock *gtb = (gTimerBlock *)opaque;
+ int64_t old;
+
switch (addr) {
+ case 0: /* Counter LSB */
+ old = (*gtb->gtimer_counter);
+
old &= 0xFFFFFFFF;
+ gtb->delta = old - (value & 0xFFFFFFFF);
+ old |= value &
0xFFFFFFFF;
+ *gtb->gtimer_counter = old;
+ /* Cancel the previous timer. */
+
if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
+
gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 4: /* Counter MSB. */
+ old =
(*gtb->gtimer_counter);
+ old &= 0xFFFFFFFF00000000;
+ gtb->delta = old - (value
<< 32);
+ old |= value << 32;
+ *gtb->gtimer_counter = old;
+ if (((gtb->control
| *gtb->gtimer_control) & 7) == 7) {
+ gtimerblock_reload(gtb, 1);
+ }
+
break;
+ case 8: /* Control. */
+ old = *gtb->gtimer_control;
+ gtb->control =
value & 0x0000000E;
+ *gtb->gtimer_control = value & 0x0000FF01;
+ if (((old &
1) == 0) && ((value & 7) == 7)) {
+ gtb->delta = 0;
+ gtimerblock_reload(gtb,
1);
+ }
+ break;
+ case 12: /* Interrupt status. */
+ gtb->status ^= value &
1;
+ gtimerblock_update_irq(gtb);
+ break;
+ case 16: /* Compare LSB */
+ old =
gtb->compare;
+ old &= 0xFFFFFFFF;
+ gtb->delta = (value & 0xFFFFFFFF) - old;
+
old |= value & 0xFFFFFFFF;
+ gtb->compare = old;
+ if (((gtb->control |
*gtb->gtimer_control) & 7) == 7) {
+ gtimerblock_reload(gtb, 1);
+ }
+ break;
+
case 20: /* Compare MSB. */
+ old = gtb->compare;
+ old &= 0xFFFFFFFF00000000;
+
gtb->delta = (value << 32) - old;
+ old |= value << 32;
+ gtb->compare = old;
+
if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
+
gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 24: /* Autoincrement */
+
gtb->inc = value;
+ break;
+ default:
+ break;
+ }
+}
+
+/* Wrapper functions to
implement the "read timer/watchdog for
+ * the current CPU" memory regions.
+
*/
+static uint64_t arm_thisgtimer_read(void *opaque, hwaddr addr,
+ unsigned
size)
+{
+ ARMMPGTimerState *s = (ARMMPGTimerState *)opaque;
+ int id =
get_current_cpu(s);
+ return gtimer_read(&s->gtimer[id], addr,
size);
+}
+
+static void arm_thisgtimer_write(void *opaque, hwaddr addr,
+
uint64_t value, unsigned size)
+{
+ ARMMPGTimerState *s = (ARMMPGTimerState
*)opaque;
+ int id = get_current_cpu(s);
+ gtimer_write(&s->gtimer[id], addr,
value, size);
+}
+
+static const MemoryRegionOps arm_thisgtimer_ops = {
+ .read
= arm_thisgtimer_read,
+ .write = arm_thisgtimer_write,
+ .valid = {
+
.min_access_size = 4,
+ .max_access_size = 4,
+ },
+ .endianness =
DEVICE_NATIVE_ENDIAN,
+};
+
+static const MemoryRegionOps gtimerblock_ops = {
+
.read = gtimer_read,
+ .write = gtimer_write,
+ .valid = {
+ .min_access_size =
4,
+ .max_access_size = 4,
+ },
+ .endianness =
DEVICE_NATIVE_ENDIAN,
+};
+
+static void gtimer_reset(gTimerBlock *gtb)
+{
+
gtb->control = 0;
+ gtb->status = 0;
+ gtb->compare = 0;
+ gtb->inc = 0;
+
gtb->tick = 0;
+}
+
+static void arm_mpgtimer_reset(DeviceState *dev)
+{
+
ARMMPGTimerState *s =
+ FROM_SYSBUS(ARMMPGTimerState, SYS_BUS_DEVICE(dev));
+
int i;
+ for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) {
+
gtimer_reset(&s->gtimer[i]);
+ }
+}
+
+static int arm_mpgtimer_init(SysBusDevice
*dev)
+{
+ ARMMPGTimerState *s = FROM_SYSBUS(ARMMPGTimerState, dev);
+ int i;
+
if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
+ hw_error("%s: num-cpu must be
between 1 and %dn", __func__, MAX_CPUS);
+ }
+
+ /* We implement one timer block
per CPU, and expose multiple MMIO regions:
+ * * region 0 is "timer for this
core"
+ * * region 1 is "timer for core 0"
+ * * region 2 is "timer for core
1"
+ * and so on.
+ * The outgoing interrupt lines are
+ * * timer for core 0
+
* * timer for core 1
+ * and so on.
+ */
+ memory_region_init_io(&s->iomem,
&arm_thisgtimer_ops, s,
+ "arm_mptimer_gtimer", 0x20);
+ sysbus_init_mmio(dev,
&s->iomem);
+ for (i = 0; i < s->num_cpu; i++) {
+ gTimerBlock *gtb =
&s->gtimer[i];
+ gtb->gtimer_counter = &s->gtimer_counter;
+ gtb->gtimer_control
= &s->gtimer_control;
+ gtb->timer = qemu_new_timer_ns(vm_clock,
gtimerblock_tick, gtb);
+ sysbus_init_irq(dev, >b->irq);
+
memory_region_init_io(>b->iomem, >imerblock_ops, gtb,
+
"arm_mptimer_gtimerblock", 0x20);
+ sysbus_init_mmio(dev, >b->iomem);
+ }
+
+
return 0;
+}
+
+static const VMStateDescription vmstate_gtimerblock = {
+ .name
= "arm_mptimer_gtimerblock",
+ .version_id = 2,
+ .minimum_version_id = 2,
+
.fields = (VMStateField[]) {
+ VMSTATE_UINT32(control, gTimerBlock),
+
VMSTATE_UINT32(status, gTimerBlock),
+ VMSTATE_UINT64(compare, gTimerBlock),
+
VMSTATE_INT64(tick, gTimerBlock),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static
const VMStateDescription vmstate_arm_mpgtimer = {
+ .name =
"arm_mp_globaltimer",
+ .version_id = 2,
+ .minimum_version_id = 2,
+ .fields =
(VMStateField[]) {
+ VMSTATE_STRUCT_VARRAY_UINT32(gtimer, ARMMPGTimerState,
num_cpu,
+ 1, vmstate_gtimerblock, gTimerBlock),
+ VMSTATE_END_OF_LIST()
+
}
+};
+
+static Property arm_mpgtimer_properties[] = {
+
DEFINE_PROP_UINT32("num-cpu", ARMMPGTimerState, num_cpu, 0),
+
DEFINE_PROP_END_OF_LIST()
+};
+
+static void arm_mpgtimer_class_init(ObjectClass
*klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
+
+ sbc->init =
arm_mpgtimer_init;
+ dc->vmsd = &vmstate_arm_mpgtimer;
+ dc->reset =
arm_mpgtimer_reset;
+ dc->no_user = 1;
+ dc->props =
arm_mpgtimer_properties;
+}
+
+static const TypeInfo arm_mpgtimer_info = {
+
.name = "arm_mp_globaltimer",
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size
= sizeof(ARMMPGTimerState),
+ .class_init =
arm_mpgtimer_class_init,
+};
+
+static void
arm_mpgtimer_register_types(void)
+{
+
type_register_static(&arm_mpgtimer_info);
+}
+
+type_init(arm_mpgtimer_register_types)
+
diff
-urN qemu-master/hw/timer/arm_mptimer.c
qemu-master.new/hw/timer/arm_mptimer.c
--- qemu-master/hw/timer/arm_mptimer.c
2013-04-08 20:12:33.000000000 +0200
+++ qemu-master.new/hw/timer/arm_mptimer.c
2013-04-15 13:44:33.000000000 +0200
@@ -49,13 +49,19 @@
static inline int
get_current_cpu(ARMMPTimerState *s)
{
- CPUState *cpu_single_cpu =
ENV_GET_CPU(cpu_single_env);
+ CPUState *cpu_single_cpu;
- 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);
+ if
(cpu_single_env != NULL) {
+ 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;
+ } else {
+ return 0;
}
- return
cpu_single_cpu->cpu_index;
}
static inline void
timerblock_update_irq(TimerBlock *tb)
diff -urN
qemu-master/hw/timer/Makefile.objs qemu-master.new/hw/timer/Makefile.objs
---
qemu-master/hw/timer/Makefile.objs 2013-04-08 20:12:33.000000000 +0200
+++
qemu-master.new/hw/timer/Makefile.objs 2013-04-15 12:54:06.000000000 +0200
@@
-24,5 +24,5 @@
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 arm_mpgtimer.o
obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
Signed-off-by: François LEGAL
<devel@thom.fr.eu.org>
[-- Attachment #2: Type: text/html, Size: 28576 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer
2013-04-15 15:41 [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer François Legal
@ 2013-04-16 9:50 ` Peter Maydell
2013-04-16 12:09 ` François Legal
2013-04-16 13:29 ` [Qemu-devel] [PATCH] " Peter Crosthwaite
1 sibling, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2013-04-16 9:50 UTC (permalink / raw)
To: François Legal; +Cc: Peter Crosthwaite, qemu-devel
On 15 April 2013 16:41, François Legal <francois.legal@thom.fr.eu.org> wrote:
> I made up this patch to implement the Cortex A9 global timer in Qemu.
>
> My patch is based on the Qemu branch maintained by Xilinx for the Zynq.
Hi François; thanks for this patch. Some comments on the code below.
Firstly, if you could send future versions of this patch in the
standard QEMU format that would be helpful:
* text/plain mail, not multipart with an HTML part
* commit message at the top describing the patch, with comments
below a '---' line
* Signed-off-by: line in the commit message itself
* please submit patches based on git.qemu.org's master branch,
not on other trees
http://wiki.qemu.org/Contribute/SubmitAPatch has some other helpful
suggestions.
Onto the code:
> diff -urN qemu-master/hw/cpu/a9mpcore.c qemu-master.new/hw/cpu/a9mpcore.c
> --- qemu-master/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200
> +++ qemu-master.new/hw/cpu/a9mpcore.c 2013-04-15 12:54:06.000000000 +0200
> @@ -15,6 +15,7 @@
> uint32_t num_cpu;
> MemoryRegion container;
> DeviceState *mptimer;
> + DeviceState *mpgtimer;
> DeviceState *wdt;
> DeviceState *gic;
> DeviceState *scu;
> @@ -31,6 +32,7 @@
> {
> A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
> SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
> + SysBusDevice *gtimerbusdev;
> int i;
>
> s->gic = qdev_create(NULL, "arm_gic");
> @@ -50,6 +52,11 @@
> qdev_init_nofail(s->scu);
> scubusdev = SYS_BUS_DEVICE(s->scu);
>
> + s->mpgtimer = qdev_create(NULL, "arm_mp_globaltimer");
I think a better name for the device would be "a9-globaltimer".
This fits our convention of preferring '-' rather than '_'
in new device names, and makes it clear that the global
timer is only used for the A9. (The private timers are used
also by the 11MPCore.)
> + qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
> + qdev_init_nofail(s->mpgtimer);
> + gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
> +
> s->mptimer = qdev_create(NULL, "arm_mptimer");
> qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
> qdev_init_nofail(s->mptimer);
> @@ -68,8 +75,6 @@
> * 0x0600-0x06ff -- private timers and watchdogs
> * 0x0700-0x0fff -- nothing
> * 0x1000-0x1fff -- GIC Distributor
> - *
> - * We should implement the global timer but don't currently do so.
> */
> memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
> memory_region_add_subregion(&s->container, 0,
> @@ -80,6 +85,8 @@
> /* Note that the A9 exposes only the "timer/watchdog for this core"
> * memory region, not the "timer/watchdog for core X" ones 11MPcore
> has.
> */
> + memory_region_add_subregion(&s->container, 0x200,
> + sysbus_mmio_get_region(gtimerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x600,
> sysbus_mmio_get_region(timerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x620,
> @@ -90,10 +97,13 @@
> sysbus_init_mmio(dev, &s->container);
>
> /* Wire up the interrupt from each watchdog and timer.
> - * For each core the timer is PPI 29 and the watchdog PPI 30.
> + * For each core the global timer is PPI 27, the private
> + * timer is PPI 29 and the watchdog PPI 30.
> */
> for (i = 0; i < s->num_cpu; i++) {
> int ppibase = (s->num_irq - 32) + i * 32;
> + sysbus_connect_irq(gtimerbusdev, i,
> + qdev_get_gpio_in(s->gic, ppibase + 27));
> sysbus_connect_irq(timerbusdev, i,
> qdev_get_gpio_in(s->gic, ppibase + 29));
> sysbus_connect_irq(wdtbusdev, i,
> diff -urN qemu-master/hw/timer/arm_mpgtimer.c
> qemu-master.new/hw/timer/arm_mpgtimer.c
> --- qemu-master/hw/timer/arm_mpgtimer.c 1970-01-01 01:00:00.000000000
> +0100
> +++ qemu-master.new/hw/timer/arm_mpgtimer.c 2013-04-15 13:56:23.000000000
> +0200
The file should also be renamed: 'hw/timer/a9gtimer.c'
> @@ -0,0 +1,359 @@
> +/*
> + * Global peripheral timer block for ARM 11MPCore and A9MP
This isn't used in 11MPCore.
> + *
> + * Written by François LEGAL
> + *
> + * 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"
> +
> +/* This device implements the per-cpu private timer and watchdog block
> + * which is used in both the ARM11MPCore and Cortex-A9MP.
> + */
This comment looks like an incorrect cut-n-paste from the private
timer implementation.
> +
> +#define MAX_CPUS 4
> +
> +/* State of a single gtimer or block */
> +typedef struct {
> + uint32_t control;
> + uint64_t compare;
> + uint32_t inc;
> + uint32_t status;
> + int64_t tick;
> +
> + int64_t delta;
> + uint64_t *gtimer_counter;
> + uint32_t *gtimer_control;
> +
> + QEMUTimer *timer;
> + MemoryRegion iomem;
> + qemu_irq irq;
> +} gTimerBlock;
This isn't right -- the global timer should have a single
QEMUTimer shared between all CPUs, not one per gTimerBlock.
You might just want to roll all the banked registers into
the top level device struct, so
uint64_t comparator[MAX_CPUS];
and so on (the global timer is more of a single block
of logic with some banked registers, unlike the private
timers which really are separate blocks of logic per
core).
> +
> +typedef struct {
> + SysBusDevice busdev;
> + uint32_t num_cpu;
> + uint64_t gtimer_counter;
> + uint32_t gtimer_control;
> + gTimerBlock gtimer[MAX_CPUS];
> + MemoryRegion iomem;
> +} ARMMPGTimerState;
> +
> +static inline int get_current_cpu(ARMMPGTimerState *s)
> +{
> + CPUState *cpu_single_cpu;
> +
> + if (cpu_single_env != NULL) {
> + 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;
> + } else {
> + return 0;
> + }
> +}
This is more complicated than get_current_cpu() in arm_mptimer.c:
why does it need to be different? I don't think this function
should ever be called with cpu_single_env NULL.
> +
> +static inline void gtimerblock_update_irq(gTimerBlock *gtb)
> +{
> + qemu_set_irq(gtb->irq, gtb->status);
> +}
> +
> +/* Return conversion factor from mpcore timer ticks to qemu timer ticks.
> */
> +static inline uint32_t gtimerblock_scale(gTimerBlock *gtb)
> +{
> + return ((((*gtb->gtimer_control) >> 8) & 0xff) + 1) * 10;
> +}
> +
> +static void gtimerblock_reload(gTimerBlock *gtb, int restart)
> +{
> + if (restart) {
> + gtb->tick = qemu_get_clock_ns(vm_clock);
> + gtb->tick += (int64_t)(((gtb->compare - *gtb->gtimer_counter) +
> + gtb->delta) * gtimerblock_scale(gtb));
> + } else {
> + gtb->tick += (int64_t)(gtb->inc * gtimerblock_scale(gtb));
> + }
> + qemu_mod_timer(gtb->timer, gtb->tick);
> +}
> +
> +static void gtimerblock_tick(void *opaque)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + *gtb->gtimer_counter = gtb->compare;
> + if ((gtb->control | *gtb->gtimer_control) & 0x9) {
Please abstract out 'read the control register value as this
core sees it' into a function, rather than doing
"(gtb->control | *gtb->gtimer_control)" everywhere.
> + gtb->compare += gtb->inc;
> + gtimerblock_reload(gtb, 0);
> + }
> + if (((gtb->control | *gtb->gtimer_control) & 0x7) &&
> + ((gtb->status & 1) == 0)) {
> + gtb->status = 1;
> + gtimerblock_update_irq(gtb);
> + }
> +}
> +
> +static uint64_t gtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t val = 0;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + if (*gtb->gtimer_control & 1) {
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? val : 0 & 0xFFFFFFFF;
> + case 4: /* Counter MSB. */
> + if (*gtb->gtimer_control & 1) {
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? (val >> 32) : 0;
> + case 8: /* Control. */
> + return (gtb->control & 0x0000000E) |
> + ((*gtb->gtimer_control) & 0x0000FF01);
> + case 12: /* Interrupt status. */
> + return gtb->status;
> + case 16: /* Compare LSB */
> + return gtb->compare & 0xFFFFFFFF;
> + case 20: /* Counter MSB */
> + return gtb->compare >> 32;
> + case 24: /* Autoincrement */
> + return gtb->inc;
> + default:
> + return 0;
> + }
> +}
> +
> +static void gtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t old;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + old = (*gtb->gtimer_counter);
> + old &= 0xFFFFFFFF;
> + gtb->delta = old - (value & 0xFFFFFFFF);
> + old |= value & 0xFFFFFFFF;
> + *gtb->gtimer_counter = old;
> + /* Cancel the previous timer. */
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 4: /* Counter MSB. */
> + old = (*gtb->gtimer_counter);
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = old - (value << 32);
> + old |= value << 32;
> + *gtb->gtimer_counter = old;
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 8: /* Control. */
> + old = *gtb->gtimer_control;
> + gtb->control = value & 0x0000000E;
> + *gtb->gtimer_control = value & 0x0000FF01;
> + if (((old & 1) == 0) && ((value & 7) == 7)) {
> + gtb->delta = 0;
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 12: /* Interrupt status. */
> + gtb->status ^= value & 1;
This doesn't look right -- this bit is write-one-to-clear,
not write-one-to-invert.
> + gtimerblock_update_irq(gtb);
> + break;
> + case 16: /* Compare LSB */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF;
> + gtb->delta = (value & 0xFFFFFFFF) - old;
> + old |= value & 0xFFFFFFFF;
> + gtb->compare = old;
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 20: /* Compare MSB. */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = (value << 32) - old;
> + old |= value << 32;
> + gtb->compare = old;
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 24: /* Autoincrement */
> + gtb->inc = value;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/* Wrapper functions to implement the "read timer/watchdog for
"watchdog" doesn't apply here.
> + * the current CPU" memory regions.
> + */
> +static uint64_t arm_thisgtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + ARMMPGTimerState *s = (ARMMPGTimerState *)opaque;
> + int id = get_current_cpu(s);
> + return gtimer_read(&s->gtimer[id], addr, size);
> +}
> +
> +static void arm_thisgtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + ARMMPGTimerState *s = (ARMMPGTimerState *)opaque;
> + int id = get_current_cpu(s);
> + gtimer_write(&s->gtimer[id], addr, value, size);
> +}
> +
> +static const MemoryRegionOps arm_thisgtimer_ops = {
> + .read = arm_thisgtimer_read,
> + .write = arm_thisgtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const MemoryRegionOps gtimerblock_ops = {
> + .read = gtimer_read,
> + .write = gtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void gtimer_reset(gTimerBlock *gtb)
> +{
> + gtb->control = 0;
> + gtb->status = 0;
> + gtb->compare = 0;
> + gtb->inc = 0;
> + gtb->tick = 0;
Reset should delete the qemu timer too.
> +}
> +
> +static void arm_mpgtimer_reset(DeviceState *dev)
> +{
> + ARMMPGTimerState *s =
> + FROM_SYSBUS(ARMMPGTimerState, SYS_BUS_DEVICE(dev));
Please don't use FROM_SYSBUS in new code.
> + int i;
> + for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) {
> + gtimer_reset(&s->gtimer[i]);
> + }
> +}
> +
> +static int arm_mpgtimer_init(SysBusDevice *dev)
> +{
> + ARMMPGTimerState *s = FROM_SYSBUS(ARMMPGTimerState, dev);
> + int i;
> + if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
> + hw_error("%s: num-cpu must be between 1 and %d\n", __func__,
> MAX_CPUS);
If you make this a DeviceClass realize method (see below) then
you can use error_setg rather than hw_error here. (pl330.c has
an example.)
> + }
> +
> + /* We implement one timer block per CPU, and expose multiple MMIO
> regions:
> + * * region 0 is "timer for this core"
> + * * region 1 is "timer for core 0"
> + * * region 2 is "timer for core 1"
> + * and so on.
Why implement multiple memory regions? There is only one
for the global timer.
> + * The outgoing interrupt lines are
> + * * timer for core 0
> + * * timer for core 1
> + * and so on.
> + */
> + memory_region_init_io(&s->iomem, &arm_thisgtimer_ops, s,
> + "arm_mptimer_gtimer", 0x20);
> + sysbus_init_mmio(dev, &s->iomem);
> + for (i = 0; i < s->num_cpu; i++) {
> + gTimerBlock *gtb = &s->gtimer[i];
> + gtb->gtimer_counter = &s->gtimer_counter;
> + gtb->gtimer_control = &s->gtimer_control;
> + gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick, gtb);
> + sysbus_init_irq(dev, >b->irq);
> + memory_region_init_io(>b->iomem, >imerblock_ops, gtb,
> + "arm_mptimer_gtimerblock", 0x20);
> + sysbus_init_mmio(dev, >b->iomem);
> + }
> +
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_gtimerblock = {
> + .name = "arm_mptimer_gtimerblock",
> + .version_id = 2,
> + .minimum_version_id = 2,
Why version 2?
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(control, gTimerBlock),
> + VMSTATE_UINT32(status, gTimerBlock),
> + VMSTATE_UINT64(compare, gTimerBlock),
> + VMSTATE_INT64(tick, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_arm_mpgtimer = {
> + .name = "arm_mp_globaltimer",
> + .version_id = 2,
> + .minimum_version_id = 2,
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT_VARRAY_UINT32(gtimer, ARMMPGTimerState, num_cpu,
> + 1, vmstate_gtimerblock, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static Property arm_mpgtimer_properties[] = {
> + DEFINE_PROP_UINT32("num-cpu", ARMMPGTimerState, num_cpu, 0),
> + DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void arm_mpgtimer_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
> +
> + sbc->init = arm_mpgtimer_init;
Can you use the DeviceClass realize method rather than
SysBusDeviceClass init, please? (see eg a9scu.c for an
example.)
> + dc->vmsd = &vmstate_arm_mpgtimer;
> + dc->reset = arm_mpgtimer_reset;
> + dc->no_user = 1;
> + dc->props = arm_mpgtimer_properties;
> +}
> +
> +static const TypeInfo arm_mpgtimer_info = {
> + .name = "arm_mp_globaltimer",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(ARMMPGTimerState),
> + .class_init = arm_mpgtimer_class_init,
> +};
> +
> +static void arm_mpgtimer_register_types(void)
> +{
> + type_register_static(&arm_mpgtimer_info);
> +}
> +
> +type_init(arm_mpgtimer_register_types)
> +
> diff -urN qemu-master/hw/timer/arm_mptimer.c
> qemu-master.new/hw/timer/arm_mptimer.c
> --- qemu-master/hw/timer/arm_mptimer.c 2013-04-08 20:12:33.000000000
> +0200
> +++ qemu-master.new/hw/timer/arm_mptimer.c 2013-04-15 13:44:33.000000000
> +0200
> @@ -49,13 +49,19 @@
>
> static inline int get_current_cpu(ARMMPTimerState *s)
> {
> - CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env);
> + CPUState *cpu_single_cpu;
>
> - 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);
> + if (cpu_single_env != NULL) {
> + 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;
> + } else {
> + return 0;
> }
> - return cpu_single_cpu->cpu_index;
> }
Why is this patch changing this code in an unrelated device?
>
> static inline void timerblock_update_irq(TimerBlock *tb)
> diff -urN qemu-master/hw/timer/Makefile.objs
> qemu-master.new/hw/timer/Makefile.objs
> --- qemu-master/hw/timer/Makefile.objs 2013-04-08 20:12:33.000000000
> +0200
> +++ qemu-master.new/hw/timer/Makefile.objs 2013-04-15 12:54:06.000000000
> +0200
> @@ -24,5 +24,5 @@
> 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 arm_mpgtimer.o
> obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>
>
> Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer
2013-04-16 9:50 ` Peter Maydell
@ 2013-04-16 12:09 ` François Legal
2013-04-16 12:19 ` Peter Maydell
0 siblings, 1 reply; 10+ messages in thread
From: François Legal @ 2013-04-16 12:09 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 9112 bytes --]
Le 16-04-2013 11:50, Peter Maydell a écrit :
> On 15 April 2013 16:41,
François Legal <francois.legal@thom.fr.eu.org> wrote:
>
>> I made up this patch
to implement the Cortex A9 global timer in Qemu. My patch is based on the Qemu
branch maintained by Xilinx for the Zynq.
>
> Hi François; thanks for this
patch. Some comments on the code below.
>
> Firstly, if you could send future
versions of this patch in the
> standard QEMU format that would be helpful:
> *
text/plain mail, not multipart with an HTML part
> * commit message at the top
describing the patch, with comments
> below a '---' line
> * Signed-off-by: line
in the commit message itself
> * please submit patches based on git.qemu.org's
master branch,
> not on other trees
>
>
http://wiki.qemu.org/Contribute/SubmitAPatch [1]has some other helpful
>
suggestions.
This is correct. I made my best to follow the guidelines, but
failed for some of them. My point here was that globaltimer was required for my
application based on the Xilinx tree, and the comments on that tree tend to let
me believe they were quite close to the official tree.
Sorry about the format I
did not pay attention to my mail reader. This is my very first post to this
list.
> Onto the code:
>
>> diff -urN qemu-master/hw/cpu/a9mpcore.c
qemu-master.new/hw/cpu/a9mpcore.c --- qemu-master/hw/cpu/a9mpcore.c 2013-04-08
20:12:33.000000000 +0200 +++ qemu-master.new/hw/cpu/a9mpcore.c 2013-04-15
12:54:06.000000000 +0200 @@ -15,6 +15,7 @@ uint32_t num_cpu; MemoryRegion
container; DeviceState *mptimer; + DeviceState *mpgtimer; DeviceState *wdt;
DeviceState *gic; DeviceState *scu; @@ -31,6 +32,7 @@ { A9MPPrivState *s =
FROM_SYSBUS(A9MPPrivState, dev); SysBusDevice *timerbusdev, *wdtbusdev,
*gicbusdev, *scubusdev; + SysBusDevice *gtimerbusdev; int i; s->gic =
qdev_create(NULL, "arm_gic"); @@ -50,6 +52,11 @@ qdev_init_nofail(s->scu);
scubusdev = SYS_BUS_DEVICE(s->scu); + s->mpgtimer = qdev_create(NULL,
"arm_mp_globaltimer");
>
> I think a better name for the device would be
"a9-globaltimer".
> This fits our convention of preferring '-' rather than '_'
>
in new device names, and makes it clear that the global
> timer is only used for
the A9. (The private timers are used
> also by the 11MPCore.)
Alright. I'll fix
this is the next version of patch
+ qdev_prop_set_uint32(s->mpgtimer,
"num-cpu", s->num_cpu); + qdev_init_nofail(s->mpgtimer); + gtimerbusdev =
SYS_BUS_DEVICE(s->mpgtimer
> um_cpu); qdev_init_nofail(s->mptimer); @@ -68,8
+75,6 @@ * 0x0600-0x06ff -- private timers and watchdogs * 0x0700-0x0fff --
nothing * 0x1000-0x1fff -- GIC Distributor - * - * We should implement the
global timer but don't currently do so. */ memory_region_init(&s->container,
"a9mp-priv-container", 0x2000); memory_region_add_subregion(&s->container, 0, @@
-80,6 +85,8 @@ /* Note that the A9 exposes only the "timer/watchdog for this
core" * memory region, not the "timer/watchdog for core X" ones 11MPcore has. */
+ memory_region_add_subregion(&s->container, 0x200, +
sysbus_mmio_get_region(gtimerbusdev, 0));
memory_region_add_subregion(&s->container, 0x600,
sysbus_mmio_get_region(timerbusdev, 0));
memory_region_add_subregion(&s->container, 0x620, @@ -90,10 +97,13 @@
sysbus_init_mmio(dev, &s->container); /* Wire up the interrupt from each
watchdog and timer. - * For each core the timer is PPI 29 and the watchdog PPI
30. + * For each core the global timer is PPI 27, the private + * timer is PPI
29 and the watchdog PPI 30. */ for (i = 0; i < s->num_cpu; i++) { int ppibase =
(s->num_irq - 32) + i * 32; + sysbus_connect_irq(gtimerbusdev, i, +
qdev_get_gpio_in(s->gic, ppibase + 27)); sysbus_connect_irq(timerbusdev, i,
qdev_get_gpio_in(s->gic, ppibase + 29)); sysbus_connect_irq(wdtbusdev, i, diff
-urN qemu-master/hw/timer/arm_mpgtimer.c qemu-master.new/hw/timer/arm_mpgtimer.c
--- qemu-master/hw/timer/arm_mpgtimer.c 1970-01-01 01:00:00.000000000 +0100 +++
qemu-master.new/hw/timer/arm_mpgtimer.c 2013-04-15 13:56:23.000000000 +0200
>
> The file should also be renamed: 'hw/timer/a9gtimer.c'
>
> Alright. I'll fix
this is the next version of patch
>
> @@ -0,0 +1,359 @@ +/* + * Global
peripheral timer block
Core and A9MP
This isn't used in 11MPCore.
Alright.
I'll fix this is the next version of the patch
+ int i; + for (i = 0; i <
ARRAY_SIZE(s->gtimer); i++) { + gtimer_reset(&s->gtimer[i]); + } +} + +static
int arm_mpgtimer_init(SysBusDevice *dev) +{ + ARMMPGTimerState *s =
FROM_SYSBUS(ARMMPGTimerState, dev); + int i; + if (s->num_cpu < 1 || s->num_cpu
> MAX_CPUS) { + hw_error("%s: num-cpu must be between 1 and %dn", __func__,
MAX_CPUS);
If you make this a DeviceClass realize method (see below) > br
/>Alright. I'll fix this is the next version of patch
>
>>> + } + + /* We
implement one timer block per CPU, and expose multiple MMIO regions: + * *
region 0 is "timer for this core" + * * region 1 is "timer for core 0" + * *
region 2 is "timer for core 1" + * and so on.
>>
>> Why implement multiple
memory regions? There is only one
>> for the global timer.
>
> Sorry.
Copy/Paste...
>
> + * The outgoing interrupt lines are + * * timer for core 0 +
* * timer for core 1 + * and so on. + */ + memory_region_init_io(&s->iomem,
&arm_thisgtimer_ops, s, + "arm_mptimer_gtimer", 0x20); + sysbus_init_mmio(dev,
&s->iomem); + for (i = 0; i < s->num_cpu; i++) { + gTimerBlock *gtb =
&s->gtimer[i]; + gtb->gtimer_counter = &s->gtimer_counter;
imer_control =
&s->gtimer_control; + gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick,
gtb); + sysbus_init_irq(dev, >b->irq); + memory_region_init_io(>b->iomem,
>imerblock_ops, gtb, + "arm_mptimer_gtimerblock", 0x20); +
sysbus_init_mmio(dev, >b->iomem); + } + + return 0; +} + +static const
VMStateDescription vmstate_gtimerblock = { + .name = "arm_mptimer_gtimerblock",
+ .version_id = 2, + .minimum_version_id = 2,
Why vers> dding-left:5px;
border-left:#1010ff 2px solid; margin-left:5px; width:100%"> + .fields =
(VMStateField[]) { + VMSTATE_UINT32(control, gTimerBlock), +
VMSTATE_UINT32(status, gTimerBlock), + VMSTATE_UINT64(c
rBlock), +
VMSTATE_INT64(tick, gTimerBlock), + VMSTATE_END_OF_LIST() + } +}; + +static
const VMStateDescription vmstate_arm_mpgtimer = { + .name =
"arm_mp_globaltimer", + .version_id = 2, + .minimum_version_id = 2, + .fields =
(VMStateField[]) { + VMSTATE_STRUCT_VARRAY_UINT32(gtimer, ARMMPGTimerState,
num_cpu, + 1, vmstate_gtimerblock, gTimerBlock), + VMSTATE_END_OF_LIST() + } +};
+ +static Property arm_mpgtimer_properties[] = { + DEFINE_PROP_UINT32("num-cpu",
ARMMPGTimerState, num_cpu, 0), + DEFINE_PROP_END_OF_LIST() +}; + +static void
arm_mpgtimer_class_init(ObjectClass *klass, void *d
> sbc->init =
arm_mpgtimer_init;
>
> Can you use the DeviceClass realize method rather
than
> SysBusDeviceClass init, please? (see eg a9scu.c for an
> example.)
>
>
Alright. I'll fix this is the next version of patch
>
> + dc->vmsd =
&vmstate_arm_mpgtimer; + dc->reset = arm_mpgtimer_reset; + dc->no_user = 1; +
dc->props = arm_mpgtimer_properties; +} +
t TypeInfo arm_mpgtimer_info = { +
.name = "arm_mp_globaltimer", + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size
= sizeof(ARMMPGTimerState), + .class_init = arm_mpgtimer_class_init, +}; +
+static void arm_mpgtimer_register_types(void) +{ +
type_register_static(&arm_mpgtimer_info);
> w/timer/arm_mptimer.c ---
qemu-master/hw/timer/arm_mptimer.c 2013-04-08 20:12:33.000000000 +0200 +++
qemu-master.new/hw/timer/arm_mptimer.c 2013-04-15 13:44:33.000000000 +0200 @@
-49,13 +49,19 @@ static inline int get_current_cpu(ARMMPTimerState *s) { -
CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env); + CPUState
*cpu_single_cpu; - if (cpu_single_cpu->cpu_index >= s->num_cpu) { -
hw_error("arm_mptimer: num-cpu
pu is %d!n", - s->num_cpu,
cpu_single_cpu->cpu_index); + if (cpu_single_env != NULL) { + 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; + } else { +
return 0; } - return cpu_single_cpu
> ockquote>
>
> This is for the same
reason I modified the get_current_cpu for global timer.
>
> static inline void
timerblock_update_irq(TimerBlock *tb) diff -urN
qemu-master/hw/timer/Makefile.objs qemu-master.new/hw/timer/Makefile.objs ---
qemu-master/hw/timer/Makefile.objs 2013-04-08 20:12:33.000000000 +0200 +++
qemu-master.new/hw/timer/Makefile.objs 2013-04-15 12:54:06.000000000 +0200 @@
-24,5 +24,5 @@ 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 arm_mpgtimer.o
obj-$(CONFIG_MC146818RTC) += mc146818rtc.o Signed-off-by: François LEGAL
<devel@thom.fr.eu.org>
>
> thanks
> -- PMM
>
>
>
Links:
------
[1]
http://wiki.qemu.org/Contribute/SubmitAPatch
[-- Attachment #2: Type: text/html, Size: 24163 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer
2013-04-16 12:09 ` François Legal
@ 2013-04-16 12:19 ` Peter Maydell
2013-04-16 12:50 ` [Qemu-devel] [PATCH V2] " François Legal
0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2013-04-16 12:19 UTC (permalink / raw)
To: François Legal; +Cc: qemu-devel
On 16 April 2013 13:09, François Legal <devel@thom.fr.eu.org> wrote:
Ugh. Your mail client has completely mangled things (it's
run all the lines of code into each other and it's still
posting as multipart text+HTML). Please could you look at
fixing its configuration -- it makes it hard to reply in
response to individual things you've said.
Anyway, you may be right about needing multiple qemutimers,
I think I misunderstood that part. We set up one timer
for each comparator, right? (ie it fires when the comparator
would fire). In that case I think that is correct.
You might like to try having each gTimerBlock have an
ARMMPGTimerState* field, so you get at the non-banked
parts of the control register with 'gtb->gtimer.gtimer_control'
rather than via an anonymous uint32_t*. I think that would
be clearer.
Regarding gdb access to memory mapped registers causing a crash
due to NULL cpu_single_env -- I think this is a general issue with
the gdb support code. I suggest you drop those parts of your
patch for now; we should look at fixing it in a coherent way
separately and later (eg by having gdb memory accesses always look
as if they are from CPU 0, or something).
PS: I didn't mention it, but the struct names and so on
should also change in line with my suggested new device
name/filename.
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer
2013-04-16 12:19 ` Peter Maydell
@ 2013-04-16 12:50 ` François Legal
2013-04-16 13:23 ` Peter Crosthwaite
2013-06-24 5:40 ` Peter Crosthwaite
0 siblings, 2 replies; 10+ messages in thread
From: François Legal @ 2013-04-16 12:50 UTC (permalink / raw)
To: qemu-devel
Le 16-04-2013 14:19, Peter Maydell a écrit :
> On 16 April 2013 13:09, François Legal <devel@thom.fr.eu.org> wrote:
> Ugh. Your mail client has completely mangled things (it's
> run all the lines of code into each other and it's still
> posting as multipart text+HTML). Please could you look at
> fixing its configuration -- it makes it hard to reply in
> response to individual things you've said.
>
Sorry !
> Anyway, you may be right about needing multiple qemutimers,
> I think I misunderstood that part. We set up one timer
> for each comparator, right? (ie it fires when the comparator
> would fire). In that case I think that is correct.
>
At least, that's how I understood the stuff.
> You might like to try having each gTimerBlock have an
> ARMMPGTimerState* field, so you get at the non-banked
> parts of the control register with 'gtb->gtimer.gtimer_control'
> rather than via an anonymous uint32_t*. I think that would
> be clearer.
>
Ok.
> Regarding gdb access to memory mapped registers causing a crash
> due to NULL cpu_single_env -- I think this is a general issue with
> the gdb support code. I suggest you drop those parts of your
> patch for now; we should look at fixing it in a coherent way
> separately and later (eg by having gdb memory accesses always look
> as if they are from CPU 0, or something).
>
Alright. I'll drop that from the patch.
> PS: I didn't mention it, but the struct names and so on
> should also change in line with my suggested new device
> name/filename.
>
Done.
> thanks
> -- PMM
New patch follows.
---
diff -urN qemu-master.old/hw/cpu/a9mpcore.c qemu-master/hw/cpu/a9mpcore.c
--- qemu-master.old/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200
+++ qemu-master/hw/cpu/a9mpcore.c 2013-04-16 13:18:39.000000000 +0200
@@ -15,6 +15,7 @@
uint32_t num_cpu;
MemoryRegion container;
DeviceState *mptimer;
+ DeviceState *mpgtimer;
DeviceState *wdt;
DeviceState *gic;
DeviceState *scu;
@@ -31,6 +32,7 @@
{
A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
+ SysBusDevice *gtimerbusdev;
int i;
s->gic = qdev_create(NULL, "arm_gic");
@@ -50,6 +52,11 @@
qdev_init_nofail(s->scu);
scubusdev = SYS_BUS_DEVICE(s->scu);
+ s->mpgtimer = qdev_create(NULL, "a9_globaltimer");
+ qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
+ qdev_init_nofail(s->mpgtimer);
+ gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
+
s->mptimer = qdev_create(NULL, "arm_mptimer");
qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
qdev_init_nofail(s->mptimer);
@@ -68,8 +75,6 @@
* 0x0600-0x06ff -- private timers and watchdogs
* 0x0700-0x0fff -- nothing
* 0x1000-0x1fff -- GIC Distributor
- *
- * We should implement the global timer but don't currently do so.
*/
memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
memory_region_add_subregion(&s->container, 0,
@@ -80,6 +85,8 @@
/* Note that the A9 exposes only the "timer/watchdog for this core"
* memory region, not the "timer/watchdog for core X" ones 11MPcore
has.
*/
+ memory_region_add_subregion(&s->container, 0x200,
+ sysbus_mmio_get_region(gtimerbusdev, 0));
memory_region_add_subregion(&s->container, 0x600,
sysbus_mmio_get_region(timerbusdev, 0));
memory_region_add_subregion(&s->container, 0x620,
@@ -90,10 +97,13 @@
sysbus_init_mmio(dev, &s->container);
/* Wire up the interrupt from each watchdog and timer.
- * For each core the timer is PPI 29 and the watchdog PPI 30.
+ * For each core the global timer is PPI 27, the private
+ * timer is PPI 29 and the watchdog PPI 30.
*/
for (i = 0; i < s->num_cpu; i++) {
int ppibase = (s->num_irq - 32) + i * 32;
+ sysbus_connect_irq(gtimerbusdev, i,
+ qdev_get_gpio_in(s->gic, ppibase + 27));
sysbus_connect_irq(timerbusdev, i,
qdev_get_gpio_in(s->gic, ppibase + 29));
sysbus_connect_irq(wdtbusdev, i,
diff -urN qemu-master.old/hw/timer/a9gtimer.c
qemu-master/hw/timer/a9gtimer.c
--- qemu-master.old/hw/timer/a9gtimer.c 1970-01-01 01:00:00.000000000 +0100
+++ qemu-master/hw/timer/a9gtimer.c 2013-04-16 14:35:48.000000000 +0200
@@ -0,0 +1,348 @@
+/*
+ * Global peripheral timer block for ARM A9MP
+ *
+ * Written by François LEGAL
+ *
+ * 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"
+
+/* This device implements the per-cpu private timer and watchdog block
+ * which is used in the Cortex-A9MP.
+ */
+
+#define MAX_CPUS 4
+#define TYPE_GTIMER "a9_globaltimer"
+#define GTIMER(obj) OBJECT_CHECK(a9globaltimerState, (obj), TYPE_GTIMER)
+
+/* State of a single gtimer or block */
+typedef struct {
+ uint32_t control;
+ uint64_t compare;
+ uint32_t inc;
+ uint32_t status;
+ int64_t tick;
+
+ int64_t delta;
+
+ struct a9globaltimerState *gtimer_state;
+ QEMUTimer *timer;
+ MemoryRegion iomem;
+ qemu_irq irq;
+} gTimerBlock;
+
+typedef struct a9globaltimerState {
+ SysBusDevice busdev;
+ uint32_t num_cpu;
+ uint64_t gtimer_counter;
+ uint32_t gtimer_control;
+ gTimerBlock gtimer[MAX_CPUS];
+ MemoryRegion iomem;
+} a9globaltimerState;
+
+static inline int 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;
+}
+
+static inline uint32_t gtimerblock_get_control_reg(gTimerBlock *gtb)
+{
+ return gtb->control | gtb->gtimer_state->gtimer_control;
+}
+
+static inline void gtimerblock_update_irq(gTimerBlock *gtb)
+{
+ qemu_set_irq(gtb->irq, gtb->status);
+}
+
+/* Return conversion factor from mpcore timer ticks to qemu timer ticks.
*/
+static inline uint32_t gtimerblock_scale(gTimerBlock *gtb)
+{
+ return (((gtb->gtimer_state->gtimer_control >> 8) & 0xff) + 1) * 10;
+}
+
+static void gtimerblock_reload(gTimerBlock *gtb, int restart)
+{
+ if (restart) {
+ gtb->tick = qemu_get_clock_ns(vm_clock);
+ gtb->tick += (int64_t)(((gtb->compare -
+ gtb->gtimer_state->gtimer_counter) +
+ gtb->delta) * gtimerblock_scale(gtb));
+ } else {
+ gtb->tick += (int64_t)(gtb->inc * gtimerblock_scale(gtb));
+ }
+ qemu_mod_timer(gtb->timer, gtb->tick);
+}
+
+static void gtimerblock_tick(void *opaque)
+{
+ gTimerBlock *gtb = (gTimerBlock *)opaque;
+ gtb->gtimer_state->gtimer_counter = gtb->compare;
+ if (gtimerblock_get_control_reg(gtb) & 0x9) {
+ gtb->compare += gtb->inc;
+ gtimerblock_reload(gtb, 0);
+ }
+ if ((gtimerblock_get_control_reg(gtb) & 0x7) &&
+ ((gtb->status & 1) == 0)) {
+ gtb->status = 1;
+ gtimerblock_update_irq(gtb);
+ }
+}
+
+static uint64_t gtimer_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ gTimerBlock *gtb = (gTimerBlock *)opaque;
+ int64_t val = 0;
+ switch (addr) {
+ case 0: /* Counter LSB */
+ if (gtimerblock_get_control_reg(gtb) & 1) {
+ val = gtb->tick - qemu_get_clock_ns(vm_clock);
+ val /= gtimerblock_scale(gtb);
+ }
+ return val < 0 ? val : 0 & 0xFFFFFFFF;
+ case 4: /* Counter MSB. */
+ if (gtimerblock_get_control_reg(gtb) & 1) {
+ val = gtb->tick - qemu_get_clock_ns(vm_clock);
+ val /= gtimerblock_scale(gtb);
+ }
+ return val < 0 ? (val >> 32) : 0;
+ case 8: /* Control. */
+ return gtimerblock_get_control_reg(gtb) & 0x0000FF0F;
+ case 12: /* Interrupt status. */
+ return gtb->status;
+ case 16: /* Compare LSB */
+ return gtb->compare & 0xFFFFFFFF;
+ case 20: /* Counter MSB */
+ return gtb->compare >> 32;
+ case 24: /* Autoincrement */
+ return gtb->inc;
+ default:
+ return 0;
+ }
+}
+
+static void gtimer_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size)
+{
+ gTimerBlock *gtb = (gTimerBlock *)opaque;
+ int64_t old;
+ switch (addr) {
+ case 0: /* Counter LSB */
+ old = gtb->gtimer_state->gtimer_counter;
+ old &= 0xFFFFFFFF;
+ gtb->delta = old - (value & 0xFFFFFFFF);
+ old |= value & 0xFFFFFFFF;
+ gtb->gtimer_state->gtimer_counter = old;
+ /* Cancel the previous timer. */
+ if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
+ gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 4: /* Counter MSB. */
+ old = gtb->gtimer_state->gtimer_counter;
+ old &= 0xFFFFFFFF00000000;
+ gtb->delta = old - (value << 32);
+ old |= value << 32;
+ gtb->gtimer_state->gtimer_counter = old;
+ if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
+ gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 8: /* Control. */
+ old = gtb->gtimer_state->gtimer_control;
+ gtb->control = value & 0x0000000E;
+ gtb->gtimer_state->gtimer_control = value & 0x0000FF01;
+ if (((old & 1) == 0) && ((value & 7) == 7)) {
+ gtb->delta = 0;
+ gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 12: /* Interrupt status. */
+ gtb->status &= ~ value;
+ gtimerblock_update_irq(gtb);
+ break;
+ case 16: /* Compare LSB */
+ old = gtb->compare;
+ old &= 0xFFFFFFFF;
+ gtb->delta = (value & 0xFFFFFFFF) - old;
+ old |= value & 0xFFFFFFFF;
+ gtb->compare = old;
+ if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
+ gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 20: /* Compare MSB. */
+ old = gtb->compare;
+ old &= 0xFFFFFFFF00000000;
+ gtb->delta = (value << 32) - old;
+ old |= value << 32;
+ gtb->compare = old;
+ if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
+ gtimerblock_reload(gtb, 1);
+ }
+ break;
+ case 24: /* Autoincrement */
+ gtb->inc = value;
+ break;
+ default:
+ break;
+ }
+}
+
+/* Wrapper functions to implement the "read global timer for
+ * the current CPU" memory regions.
+ */
+static uint64_t arm_thisgtimer_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ a9globaltimerState *s = (a9globaltimerState *)opaque;
+ int id = get_current_cpu(s);
+ return gtimer_read(&s->gtimer[id], addr, size);
+}
+
+static void arm_thisgtimer_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size)
+{
+ a9globaltimerState *s = (a9globaltimerState *)opaque;
+ int id = get_current_cpu(s);
+ gtimer_write(&s->gtimer[id], addr, value, size);
+}
+
+static const MemoryRegionOps arm_thisgtimer_ops = {
+ .read = arm_thisgtimer_read,
+ .write = arm_thisgtimer_write,
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ },
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const MemoryRegionOps gtimerblock_ops = {
+ .read = gtimer_read,
+ .write = gtimer_write,
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ },
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void gtimer_reset(gTimerBlock *gtb)
+{
+ if (gtb->timer) {
+ qemu_del_timer(gtb->timer);
+ }
+ gtb->control = 0;
+ gtb->status = 0;
+ gtb->compare = 0;
+ gtb->inc = 0;
+ gtb->tick = 0;
+}
+
+static void a9mp_globaltimer_reset(DeviceState *dev)
+{
+ a9globaltimerState *s = GTIMER(dev);
+ int i;
+ for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) {
+ gtimer_reset(&s->gtimer[i]);
+ }
+}
+
+static void a9mp_globaltimer_realize(DeviceState *dev, Error **errp)
+{
+ a9globaltimerState *s = GTIMER(dev);
+ int i;
+
+ if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
+ error_setg(errp, "%s: num-cpu must be between 1 and %d\n",
+ __func__, MAX_CPUS);
+ }
+
+ memory_region_init_io(&s->iomem, &arm_thisgtimer_ops, s,
+ "arm_mptimer_gtimer", 0x20);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
+ for (i = 0; i < s->num_cpu; i++) {
+ gTimerBlock *gtb = &s->gtimer[i];
+ gtb->gtimer_state = s;
+ gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick, gtb);
+ sysbus_init_irq(SYS_BUS_DEVICE(dev), >b->irq);
+ memory_region_init_io(>b->iomem, >imerblock_ops, gtb,
+ "arm_mptimer_gtimerblock", 0x20);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), >b->iomem);
+ }
+}
+
+static const VMStateDescription vmstate_gtimerblock = {
+ .name = "a9mp_gtimerblock",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(control, gTimerBlock),
+ VMSTATE_UINT32(status, gTimerBlock),
+ VMSTATE_UINT64(compare, gTimerBlock),
+ VMSTATE_INT64(tick, gTimerBlock),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_a9mp_globaltimer = {
+ .name = "a9mp_globaltimer",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT_VARRAY_UINT32(gtimer, a9globaltimerState, num_cpu,
+ 1, vmstate_gtimerblock, gTimerBlock),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static Property a9mp_globaltimer_properties[] = {
+ DEFINE_PROP_UINT32("num-cpu", a9globaltimerState, num_cpu, 0),
+ DEFINE_PROP_END_OF_LIST()
+};
+
+static void a9mp_globaltimer_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = a9mp_globaltimer_realize;
+ dc->vmsd = &vmstate_a9mp_globaltimer;
+ dc->reset = a9mp_globaltimer_reset;
+ dc->no_user = 1;
+ dc->props = a9mp_globaltimer_properties;
+}
+
+static const TypeInfo a9mp_globaltimer_info = {
+ .name = "a9_globaltimer",
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(a9globaltimerState),
+ .class_init = a9mp_globaltimer_class_init,
+};
+
+static void a9mp_globaltimer_register_types(void)
+{
+ type_register_static(&a9mp_globaltimer_info);
+}
+
+type_init(a9mp_globaltimer_register_types)
diff -urN qemu-master.old/hw/timer/Makefile.objs
qemu-master/hw/timer/Makefile.objs
--- qemu-master.old/hw/timer/Makefile.objs 2013-04-08 20:12:33.000000000
+0200
+++ qemu-master/hw/timer/Makefile.objs 2013-04-16 13:53:09.000000000 +0200
@@ -24,5 +24,5 @@
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
Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer
2013-04-16 12:50 ` [Qemu-devel] [PATCH V2] " François Legal
@ 2013-04-16 13:23 ` Peter Crosthwaite
2013-04-16 14:06 ` François Legal
2013-06-24 5:40 ` Peter Crosthwaite
1 sibling, 1 reply; 10+ messages in thread
From: Peter Crosthwaite @ 2013-04-16 13:23 UTC (permalink / raw)
To: François Legal; +Cc: qemu-devel
Hi Francois,
On Tue, Apr 16, 2013 at 10:50 PM, François Legal
<francois.legal@thom.fr.eu.org> wrote:
> Le 16-04-2013 14:19, Peter Maydell a écrit :
>
>
>> On 16 April 2013 13:09, François Legal <devel@thom.fr.eu.org> wrote:
>> Ugh. Your mail client has completely mangled things (it's
>> run all the lines of code into each other and it's still
>> posting as multipart text+HTML). Please could you look at
>> fixing its configuration -- it makes it hard to reply in
>> response to individual things you've said.
>>
>
> Sorry !
>
>
>> Anyway, you may be right about needing multiple qemutimers,
>> I think I misunderstood that part. We set up one timer
>> for each comparator, right? (ie it fires when the comparator
>> would fire). In that case I think that is correct.
>>
>
I had this problem with timer/cadence_ttc.c, which is a single timer
with multiple comparators. Went the other way implementation wise
though, using a single QEMUTimer and each trap of the timer it
computes which event is going to occur next and qemu_mod_timer with
the MIN of the comparators.
> At least, that's how I understood the stuff.
>
>
>> You might like to try having each gTimerBlock have an
>> ARMMPGTimerState* field, so you get at the non-banked
>> parts of the control register with 'gtb->gtimer.gtimer_control'
>> rather than via an anonymous uint32_t*. I think that would
>> be clearer.
>>
>
> Ok.
>
>
>> Regarding gdb access to memory mapped registers causing a crash
>> due to NULL cpu_single_env -- I think this is a general issue with
>> the gdb support code. I suggest you drop those parts of your
>> patch for now; we should look at fixing it in a coherent way
>> separately and later (eg by having gdb memory accesses always look
>> as if they are from CPU 0, or something).
>>
>
> Alright. I'll drop that from the patch.
>
>
>> PS: I didn't mention it, but the struct names and so on
>> should also change in line with my suggested new device
>> name/filename.
>>
>
> Done.
>>
>> thanks
>> -- PMM
>
>
>
> New patch follows.
>
Use git to create and send patches. Check out the commands:
git format-patch
git send-email
This will send you patch as plain text as well.
Regards,
Peter
> ---
>
> diff -urN qemu-master.old/hw/cpu/a9mpcore.c qemu-master/hw/cpu/a9mpcore.c
> --- qemu-master.old/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200
> +++ qemu-master/hw/cpu/a9mpcore.c 2013-04-16 13:18:39.000000000 +0200
>
> @@ -15,6 +15,7 @@
> uint32_t num_cpu;
> MemoryRegion container;
> DeviceState *mptimer;
> + DeviceState *mpgtimer;
> DeviceState *wdt;
> DeviceState *gic;
> DeviceState *scu;
> @@ -31,6 +32,7 @@
> {
> A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
> SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
> + SysBusDevice *gtimerbusdev;
> int i;
>
> s->gic = qdev_create(NULL, "arm_gic");
> @@ -50,6 +52,11 @@
> qdev_init_nofail(s->scu);
> scubusdev = SYS_BUS_DEVICE(s->scu);
>
> + s->mpgtimer = qdev_create(NULL, "a9_globaltimer");
>
> + qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
> + qdev_init_nofail(s->mpgtimer);
> + gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
> +
> s->mptimer = qdev_create(NULL, "arm_mptimer");
> qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
> qdev_init_nofail(s->mptimer);
> @@ -68,8 +75,6 @@
> * 0x0600-0x06ff -- private timers and watchdogs
> * 0x0700-0x0fff -- nothing
> * 0x1000-0x1fff -- GIC Distributor
> - *
> - * We should implement the global timer but don't currently do so.
> */
> memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
> memory_region_add_subregion(&s->container, 0,
> @@ -80,6 +85,8 @@
> /* Note that the A9 exposes only the "timer/watchdog for this core"
> * memory region, not the "timer/watchdog for core X" ones 11MPcore
> has.
> */
> + memory_region_add_subregion(&s->container, 0x200,
> + sysbus_mmio_get_region(gtimerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x600,
> sysbus_mmio_get_region(timerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x620,
> @@ -90,10 +97,13 @@
> sysbus_init_mmio(dev, &s->container);
>
> /* Wire up the interrupt from each watchdog and timer.
> - * For each core the timer is PPI 29 and the watchdog PPI 30.
> + * For each core the global timer is PPI 27, the private
> + * timer is PPI 29 and the watchdog PPI 30.
> */
> for (i = 0; i < s->num_cpu; i++) {
> int ppibase = (s->num_irq - 32) + i * 32;
> + sysbus_connect_irq(gtimerbusdev, i,
> + qdev_get_gpio_in(s->gic, ppibase + 27));
> sysbus_connect_irq(timerbusdev, i,
> qdev_get_gpio_in(s->gic, ppibase + 29));
> sysbus_connect_irq(wdtbusdev, i,
> diff -urN qemu-master.old/hw/timer/a9gtimer.c
> qemu-master/hw/timer/a9gtimer.c
> --- qemu-master.old/hw/timer/a9gtimer.c 1970-01-01 01:00:00.000000000 +0100
> +++ qemu-master/hw/timer/a9gtimer.c 2013-04-16 14:35:48.000000000 +0200
> @@ -0,0 +1,348 @@
> +/*
> + * Global peripheral timer block for ARM A9MP
>
> + *
> + * Written by François LEGAL
> + *
> + * 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"
> +
> +/* This device implements the per-cpu private timer and watchdog block
> + * which is used in the Cortex-A9MP.
>
> + */
> +
> +#define MAX_CPUS 4
> +#define TYPE_GTIMER "a9_globaltimer"
> +#define GTIMER(obj) OBJECT_CHECK(a9globaltimerState, (obj), TYPE_GTIMER)
>
> +
> +/* State of a single gtimer or block */
> +typedef struct {
> + uint32_t control;
> + uint64_t compare;
> + uint32_t inc;
> + uint32_t status;
> + int64_t tick;
> +
> + int64_t delta;
> +
> + struct a9globaltimerState *gtimer_state;
>
> + QEMUTimer *timer;
> + MemoryRegion iomem;
> + qemu_irq irq;
> +} gTimerBlock;
> +
> +typedef struct a9globaltimerState {
>
> + SysBusDevice busdev;
> + uint32_t num_cpu;
> + uint64_t gtimer_counter;
> + uint32_t gtimer_control;
> + gTimerBlock gtimer[MAX_CPUS];
> + MemoryRegion iomem;
> +} a9globaltimerState;
> +
> +static inline int 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;
> +}
> +
> +static inline uint32_t gtimerblock_get_control_reg(gTimerBlock *gtb)
> +{
> + return gtb->control | gtb->gtimer_state->gtimer_control;
> +}
>
> +
> +static inline void gtimerblock_update_irq(gTimerBlock *gtb)
> +{
> + qemu_set_irq(gtb->irq, gtb->status);
> +}
> +
> +/* Return conversion factor from mpcore timer ticks to qemu timer ticks.
> */
> +static inline uint32_t gtimerblock_scale(gTimerBlock *gtb)
> +{
> + return (((gtb->gtimer_state->gtimer_control >> 8) & 0xff) + 1) * 10;
>
> +}
> +
> +static void gtimerblock_reload(gTimerBlock *gtb, int restart)
> +{
> + if (restart) {
> + gtb->tick = qemu_get_clock_ns(vm_clock);
> + gtb->tick += (int64_t)(((gtb->compare -
> + gtb->gtimer_state->gtimer_counter) +
>
> + gtb->delta) * gtimerblock_scale(gtb));
> + } else {
> + gtb->tick += (int64_t)(gtb->inc * gtimerblock_scale(gtb));
> + }
> + qemu_mod_timer(gtb->timer, gtb->tick);
> +}
> +
> +static void gtimerblock_tick(void *opaque)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + gtb->gtimer_state->gtimer_counter = gtb->compare;
> + if (gtimerblock_get_control_reg(gtb) & 0x9) {
>
> + gtb->compare += gtb->inc;
> + gtimerblock_reload(gtb, 0);
> + }
> + if ((gtimerblock_get_control_reg(gtb) & 0x7) &&
>
> + ((gtb->status & 1) == 0)) {
> + gtb->status = 1;
> + gtimerblock_update_irq(gtb);
> + }
> +}
> +
> +static uint64_t gtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t val = 0;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + if (gtimerblock_get_control_reg(gtb) & 1) {
>
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? val : 0 & 0xFFFFFFFF;
> + case 4: /* Counter MSB. */
> + if (gtimerblock_get_control_reg(gtb) & 1) {
>
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? (val >> 32) : 0;
> + case 8: /* Control. */
> + return gtimerblock_get_control_reg(gtb) & 0x0000FF0F;
>
> + case 12: /* Interrupt status. */
> + return gtb->status;
> + case 16: /* Compare LSB */
> + return gtb->compare & 0xFFFFFFFF;
> + case 20: /* Counter MSB */
> + return gtb->compare >> 32;
> + case 24: /* Autoincrement */
> + return gtb->inc;
> + default:
> + return 0;
> + }
> +}
> +
> +static void gtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t old;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + old = gtb->gtimer_state->gtimer_counter;
>
> + old &= 0xFFFFFFFF;
> + gtb->delta = old - (value & 0xFFFFFFFF);
> + old |= value & 0xFFFFFFFF;
> + gtb->gtimer_state->gtimer_counter = old;
>
> + /* Cancel the previous timer. */
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 4: /* Counter MSB. */
> + old = gtb->gtimer_state->gtimer_counter;
>
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = old - (value << 32);
> + old |= value << 32;
> + gtb->gtimer_state->gtimer_counter = old;
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 8: /* Control. */
> + old = gtb->gtimer_state->gtimer_control;
>
> + gtb->control = value & 0x0000000E;
> + gtb->gtimer_state->gtimer_control = value & 0x0000FF01;
>
> + if (((old & 1) == 0) && ((value & 7) == 7)) {
> + gtb->delta = 0;
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 12: /* Interrupt status. */
> + gtb->status &= ~ value;
>
> + gtimerblock_update_irq(gtb);
> + break;
> + case 16: /* Compare LSB */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF;
> + gtb->delta = (value & 0xFFFFFFFF) - old;
> + old |= value & 0xFFFFFFFF;
> + gtb->compare = old;
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 20: /* Compare MSB. */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = (value << 32) - old;
> + old |= value << 32;
> + gtb->compare = old;
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 24: /* Autoincrement */
> + gtb->inc = value;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/* Wrapper functions to implement the "read global timer for
>
> + * the current CPU" memory regions.
> + */
> +static uint64_t arm_thisgtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + a9globaltimerState *s = (a9globaltimerState *)opaque;
>
> + int id = get_current_cpu(s);
> + return gtimer_read(&s->gtimer[id], addr, size);
> +}
> +
> +static void arm_thisgtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + a9globaltimerState *s = (a9globaltimerState *)opaque;
>
> + int id = get_current_cpu(s);
> + gtimer_write(&s->gtimer[id], addr, value, size);
> +}
> +
> +static const MemoryRegionOps arm_thisgtimer_ops = {
> + .read = arm_thisgtimer_read,
> + .write = arm_thisgtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const MemoryRegionOps gtimerblock_ops = {
> + .read = gtimer_read,
> + .write = gtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void gtimer_reset(gTimerBlock *gtb)
> +{
> + if (gtb->timer) {
> + qemu_del_timer(gtb->timer);
>
> + }
> + gtb->control = 0;
> + gtb->status = 0;
> + gtb->compare = 0;
> + gtb->inc = 0;
> + gtb->tick = 0;
> +}
> +
> +static void a9mp_globaltimer_reset(DeviceState *dev)
> +{
> + a9globaltimerState *s = GTIMER(dev);
>
> + int i;
> + for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) {
> + gtimer_reset(&s->gtimer[i]);
> + }
> +}
> +
> +static void a9mp_globaltimer_realize(DeviceState *dev, Error **errp)
> +{
> + a9globaltimerState *s = GTIMER(dev);
> + int i;
> +
>
> + if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
> + error_setg(errp, "%s: num-cpu must be between 1 and %d\n",
> + __func__, MAX_CPUS);
> + }
> +
>
> + memory_region_init_io(&s->iomem, &arm_thisgtimer_ops, s,
> + "arm_mptimer_gtimer", 0x20);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>
> + for (i = 0; i < s->num_cpu; i++) {
> + gTimerBlock *gtb = &s->gtimer[i];
> + gtb->gtimer_state = s;
>
> + gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick, gtb);
> + sysbus_init_irq(SYS_BUS_DEVICE(dev), >b->irq);
>
> + memory_region_init_io(>b->iomem, >imerblock_ops, gtb,
> + "arm_mptimer_gtimerblock", 0x20);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), >b->iomem);
> + }
> +}
> +
>
> +static const VMStateDescription vmstate_gtimerblock = {
> + .name = "a9mp_gtimerblock",
> + .version_id = 1,
> + .minimum_version_id = 1,
>
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(control, gTimerBlock),
> + VMSTATE_UINT32(status, gTimerBlock),
> + VMSTATE_UINT64(compare, gTimerBlock),
> + VMSTATE_INT64(tick, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_a9mp_globaltimer = {
> + .name = "a9mp_globaltimer",
> + .version_id = 1,
> + .minimum_version_id = 1,
>
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT_VARRAY_UINT32(gtimer, a9globaltimerState, num_cpu,
>
> + 1, vmstate_gtimerblock, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static Property a9mp_globaltimer_properties[] = {
> + DEFINE_PROP_UINT32("num-cpu", a9globaltimerState, num_cpu, 0),
> + DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void a9mp_globaltimer_class_init(ObjectClass *klass, void *data)
>
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = a9mp_globaltimer_realize;
> + dc->vmsd = &vmstate_a9mp_globaltimer;
> + dc->reset = a9mp_globaltimer_reset;
>
> + dc->no_user = 1;
> + dc->props = a9mp_globaltimer_properties;
> +}
> +
> +static const TypeInfo a9mp_globaltimer_info = {
> + .name = "a9_globaltimer",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(a9globaltimerState),
> + .class_init = a9mp_globaltimer_class_init,
> +};
> +
> +static void a9mp_globaltimer_register_types(void)
> +{
> + type_register_static(&a9mp_globaltimer_info);
> +}
> +
> +type_init(a9mp_globaltimer_register_types)
> diff -urN qemu-master.old/hw/timer/Makefile.objs
> qemu-master/hw/timer/Makefile.objs
> --- qemu-master.old/hw/timer/Makefile.objs 2013-04-08
> 20:12:33.000000000 +0200
> +++ qemu-master/hw/timer/Makefile.objs 2013-04-16 13:53:09.000000000 +0200
>
> @@ -24,5 +24,5 @@
> 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
>
> Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer
2013-04-15 15:41 [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer François Legal
2013-04-16 9:50 ` Peter Maydell
@ 2013-04-16 13:29 ` Peter Crosthwaite
1 sibling, 0 replies; 10+ messages in thread
From: Peter Crosthwaite @ 2013-04-16 13:29 UTC (permalink / raw)
To: François Legal; +Cc: qemu-devel
Hi Francois,
On Tue, Apr 16, 2013 at 1:41 AM, François Legal
<francois.legal@thom.fr.eu.org> wrote:
> Hello,
>
>
>
> I made up this patch to implement the Cortex A9 global timer in Qemu.
>
> My patch is based on the Qemu branch maintained by Xilinx for the Zynq.
>
Patches against this tree can be submitted to git@xilinx.com. But this
work is generic enough to be appropriate for this forum. There is
nothing in our tree that should affect this patch. So I suggest
rebasing against the qemu.org tree - it should be trivial.
Regards,
Peter
>
>
> diff -urN qemu-master/hw/cpu/a9mpcore.c qemu-master.new/hw/cpu/a9mpcore.c
> --- qemu-master/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200
> +++ qemu-master.new/hw/cpu/a9mpcore.c 2013-04-15 12:54:06.000000000 +0200
> @@ -15,6 +15,7 @@
> uint32_t num_cpu;
> MemoryRegion container;
> DeviceState *mptimer;
> + DeviceState *mpgtimer;
> DeviceState *wdt;
> DeviceState *gic;
> DeviceState *scu;
> @@ -31,6 +32,7 @@
> {
> A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
> SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
> + SysBusDevice *gtimerbusdev;
> int i;
>
> s->gic = qdev_create(NULL, "arm_gic");
> @@ -50,6 +52,11 @@
> qdev_init_nofail(s->scu);
> scubusdev = SYS_BUS_DEVICE(s->scu);
>
> + s->mpgtimer = qdev_create(NULL, "arm_mp_globaltimer");
> + qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
> + qdev_init_nofail(s->mpgtimer);
> + gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
> +
> s->mptimer = qdev_create(NULL, "arm_mptimer");
> qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
> qdev_init_nofail(s->mptimer);
> @@ -68,8 +75,6 @@
> * 0x0600-0x06ff -- private timers and watchdogs
> * 0x0700-0x0fff -- nothing
> * 0x1000-0x1fff -- GIC Distributor
> - *
> - * We should implement the global timer but don't currently do so.
> */
> memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
> memory_region_add_subregion(&s->container, 0,
> @@ -80,6 +85,8 @@
> /* Note that the A9 exposes only the "timer/watchdog for this core"
> * memory region, not the "timer/watchdog for core X" ones 11MPcore
> has.
> */
> + memory_region_add_subregion(&s->container, 0x200,
> + sysbus_mmio_get_region(gtimerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x600,
> sysbus_mmio_get_region(timerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x620,
> @@ -90,10 +97,13 @@
> sysbus_init_mmio(dev, &s->container);
>
> /* Wire up the interrupt from each watchdog and timer.
> - * For each core the timer is PPI 29 and the watchdog PPI 30.
> + * For each core the global timer is PPI 27, the private
> + * timer is PPI 29 and the watchdog PPI 30.
> */
> for (i = 0; i < s->num_cpu; i++) {
> int ppibase = (s->num_irq - 32) + i * 32;
> + sysbus_connect_irq(gtimerbusdev, i,
> + qdev_get_gpio_in(s->gic, ppibase + 27));
> sysbus_connect_irq(timerbusdev, i,
> qdev_get_gpio_in(s->gic, ppibase + 29));
> sysbus_connect_irq(wdtbusdev, i,
> diff -urN qemu-master/hw/timer/arm_mpgtimer.c
> qemu-master.new/hw/timer/arm_mpgtimer.c
> --- qemu-master/hw/timer/arm_mpgtimer.c 1970-01-01 01:00:00.000000000
> +0100
> +++ qemu-master.new/hw/timer/arm_mpgtimer.c 2013-04-15 13:56:23.000000000
> +0200
> @@ -0,0 +1,359 @@
> +/*
> + * Global peripheral timer block for ARM 11MPCore and A9MP
> + *
> + * Written by François LEGAL
> + *
> + * 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"
> +
> +/* This device implements the per-cpu private timer and watchdog block
> + * which is used in both the ARM11MPCore and Cortex-A9MP.
> + */
> +
> +#define MAX_CPUS 4
> +
> +/* State of a single gtimer or block */
> +typedef struct {
> + uint32_t control;
> + uint64_t compare;
> + uint32_t inc;
> + uint32_t status;
> + int64_t tick;
> +
> + int64_t delta;
> + uint64_t *gtimer_counter;
> + uint32_t *gtimer_control;
> +
> + QEMUTimer *timer;
> + MemoryRegion iomem;
> + qemu_irq irq;
> +} gTimerBlock;
> +
> +typedef struct {
> + SysBusDevice busdev;
> + uint32_t num_cpu;
> + uint64_t gtimer_counter;
> + uint32_t gtimer_control;
> + gTimerBlock gtimer[MAX_CPUS];
> + MemoryRegion iomem;
> +} ARMMPGTimerState;
> +
> +static inline int get_current_cpu(ARMMPGTimerState *s)
> +{
> + CPUState *cpu_single_cpu;
> +
> + if (cpu_single_env != NULL) {
> + 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;
> + } else {
> + return 0;
> + }
> +}
> +
> +static inline void gtimerblock_update_irq(gTimerBlock *gtb)
> +{
> + qemu_set_irq(gtb->irq, gtb->status);
> +}
> +
> +/* Return conversion factor from mpcore timer ticks to qemu timer ticks.
> */
> +static inline uint32_t gtimerblock_scale(gTimerBlock *gtb)
> +{
> + return ((((*gtb->gtimer_control) >> 8) & 0xff) + 1) * 10;
> +}
> +
> +static void gtimerblock_reload(gTimerBlock *gtb, int restart)
> +{
> + if (restart) {
> + gtb->tick = qemu_get_clock_ns(vm_clock);
> + gtb->tick += (int64_t)(((gtb->compare - *gtb->gtimer_counter) +
> + gtb->delta) * gtimerblock_scale(gtb));
> + } else {
> + gtb->tick += (int64_t)(gtb->inc * gtimerblock_scale(gtb));
> + }
> + qemu_mod_timer(gtb->timer, gtb->tick);
> +}
> +
> +static void gtimerblock_tick(void *opaque)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + *gtb->gtimer_counter = gtb->compare;
> + if ((gtb->control | *gtb->gtimer_control) & 0x9) {
> + gtb->compare += gtb->inc;
> + gtimerblock_reload(gtb, 0);
> + }
> + if (((gtb->control | *gtb->gtimer_control) & 0x7) &&
> + ((gtb->status & 1) == 0)) {
> + gtb->status = 1;
> + gtimerblock_update_irq(gtb);
> + }
> +}
> +
> +static uint64_t gtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t val = 0;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + if (*gtb->gtimer_control & 1) {
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? val : 0 & 0xFFFFFFFF;
> + case 4: /* Counter MSB. */
> + if (*gtb->gtimer_control & 1) {
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? (val >> 32) : 0;
> + case 8: /* Control. */
> + return (gtb->control & 0x0000000E) |
> + ((*gtb->gtimer_control) & 0x0000FF01);
> + case 12: /* Interrupt status. */
> + return gtb->status;
> + case 16: /* Compare LSB */
> + return gtb->compare & 0xFFFFFFFF;
> + case 20: /* Counter MSB */
> + return gtb->compare >> 32;
> + case 24: /* Autoincrement */
> + return gtb->inc;
> + default:
> + return 0;
> + }
> +}
> +
> +static void gtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t old;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + old = (*gtb->gtimer_counter);
> + old &= 0xFFFFFFFF;
> + gtb->delta = old - (value & 0xFFFFFFFF);
> + old |= value & 0xFFFFFFFF;
> + *gtb->gtimer_counter = old;
> + /* Cancel the previous timer. */
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 4: /* Counter MSB. */
> + old = (*gtb->gtimer_counter);
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = old - (value << 32);
> + old |= value << 32;
> + *gtb->gtimer_counter = old;
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 8: /* Control. */
> + old = *gtb->gtimer_control;
> + gtb->control = value & 0x0000000E;
> + *gtb->gtimer_control = value & 0x0000FF01;
> + if (((old & 1) == 0) && ((value & 7) == 7)) {
> + gtb->delta = 0;
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 12: /* Interrupt status. */
> + gtb->status ^= value & 1;
> + gtimerblock_update_irq(gtb);
> + break;
> + case 16: /* Compare LSB */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF;
> + gtb->delta = (value & 0xFFFFFFFF) - old;
> + old |= value & 0xFFFFFFFF;
> + gtb->compare = old;
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 20: /* Compare MSB. */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = (value << 32) - old;
> + old |= value << 32;
> + gtb->compare = old;
> + if (((gtb->control | *gtb->gtimer_control) & 7) == 7) {
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 24: /* Autoincrement */
> + gtb->inc = value;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/* Wrapper functions to implement the "read timer/watchdog for
> + * the current CPU" memory regions.
> + */
> +static uint64_t arm_thisgtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + ARMMPGTimerState *s = (ARMMPGTimerState *)opaque;
> + int id = get_current_cpu(s);
> + return gtimer_read(&s->gtimer[id], addr, size);
> +}
> +
> +static void arm_thisgtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + ARMMPGTimerState *s = (ARMMPGTimerState *)opaque;
> + int id = get_current_cpu(s);
> + gtimer_write(&s->gtimer[id], addr, value, size);
> +}
> +
> +static const MemoryRegionOps arm_thisgtimer_ops = {
> + .read = arm_thisgtimer_read,
> + .write = arm_thisgtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const MemoryRegionOps gtimerblock_ops = {
> + .read = gtimer_read,
> + .write = gtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void gtimer_reset(gTimerBlock *gtb)
> +{
> + gtb->control = 0;
> + gtb->status = 0;
> + gtb->compare = 0;
> + gtb->inc = 0;
> + gtb->tick = 0;
> +}
> +
> +static void arm_mpgtimer_reset(DeviceState *dev)
> +{
> + ARMMPGTimerState *s =
> + FROM_SYSBUS(ARMMPGTimerState, SYS_BUS_DEVICE(dev));
> + int i;
> + for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) {
> + gtimer_reset(&s->gtimer[i]);
> + }
> +}
> +
> +static int arm_mpgtimer_init(SysBusDevice *dev)
> +{
> + ARMMPGTimerState *s = FROM_SYSBUS(ARMMPGTimerState, dev);
> + int i;
> + if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
> + hw_error("%s: num-cpu must be between 1 and %d\n", __func__,
> MAX_CPUS);
> + }
> +
> + /* We implement one timer block per CPU, and expose multiple MMIO
> regions:
> + * * region 0 is "timer for this core"
> + * * region 1 is "timer for core 0"
> + * * region 2 is "timer for core 1"
> + * and so on.
> + * The outgoing interrupt lines are
> + * * timer for core 0
> + * * timer for core 1
> + * and so on.
> + */
> + memory_region_init_io(&s->iomem, &arm_thisgtimer_ops, s,
> + "arm_mptimer_gtimer", 0x20);
> + sysbus_init_mmio(dev, &s->iomem);
> + for (i = 0; i < s->num_cpu; i++) {
> + gTimerBlock *gtb = &s->gtimer[i];
> + gtb->gtimer_counter = &s->gtimer_counter;
> + gtb->gtimer_control = &s->gtimer_control;
> + gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick, gtb);
> + sysbus_init_irq(dev, >b->irq);
> + memory_region_init_io(>b->iomem, >imerblock_ops, gtb,
> + "arm_mptimer_gtimerblock", 0x20);
> + sysbus_init_mmio(dev, >b->iomem);
> + }
> +
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_gtimerblock = {
> + .name = "arm_mptimer_gtimerblock",
> + .version_id = 2,
> + .minimum_version_id = 2,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(control, gTimerBlock),
> + VMSTATE_UINT32(status, gTimerBlock),
> + VMSTATE_UINT64(compare, gTimerBlock),
> + VMSTATE_INT64(tick, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_arm_mpgtimer = {
> + .name = "arm_mp_globaltimer",
> + .version_id = 2,
> + .minimum_version_id = 2,
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT_VARRAY_UINT32(gtimer, ARMMPGTimerState, num_cpu,
> + 1, vmstate_gtimerblock, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static Property arm_mpgtimer_properties[] = {
> + DEFINE_PROP_UINT32("num-cpu", ARMMPGTimerState, num_cpu, 0),
> + DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void arm_mpgtimer_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
> +
> + sbc->init = arm_mpgtimer_init;
> + dc->vmsd = &vmstate_arm_mpgtimer;
> + dc->reset = arm_mpgtimer_reset;
> + dc->no_user = 1;
> + dc->props = arm_mpgtimer_properties;
> +}
> +
> +static const TypeInfo arm_mpgtimer_info = {
> + .name = "arm_mp_globaltimer",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(ARMMPGTimerState),
> + .class_init = arm_mpgtimer_class_init,
> +};
> +
> +static void arm_mpgtimer_register_types(void)
> +{
> + type_register_static(&arm_mpgtimer_info);
> +}
> +
> +type_init(arm_mpgtimer_register_types)
> +
> diff -urN qemu-master/hw/timer/arm_mptimer.c
> qemu-master.new/hw/timer/arm_mptimer.c
> --- qemu-master/hw/timer/arm_mptimer.c 2013-04-08 20:12:33.000000000
> +0200
> +++ qemu-master.new/hw/timer/arm_mptimer.c 2013-04-15 13:44:33.000000000
> +0200
> @@ -49,13 +49,19 @@
>
> static inline int get_current_cpu(ARMMPTimerState *s)
> {
> - CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env);
> + CPUState *cpu_single_cpu;
>
> - 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);
> + if (cpu_single_env != NULL) {
> + 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;
> + } else {
> + return 0;
> }
> - return cpu_single_cpu->cpu_index;
> }
>
> static inline void timerblock_update_irq(TimerBlock *tb)
> diff -urN qemu-master/hw/timer/Makefile.objs
> qemu-master.new/hw/timer/Makefile.objs
> --- qemu-master/hw/timer/Makefile.objs 2013-04-08 20:12:33.000000000
> +0200
> +++ qemu-master.new/hw/timer/Makefile.objs 2013-04-15 12:54:06.000000000
> +0200
> @@ -24,5 +24,5 @@
> 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 arm_mpgtimer.o
> obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>
>
> Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer
2013-04-16 13:23 ` Peter Crosthwaite
@ 2013-04-16 14:06 ` François Legal
2013-04-16 15:17 ` Peter Crosthwaite
0 siblings, 1 reply; 10+ messages in thread
From: François Legal @ 2013-04-16 14:06 UTC (permalink / raw)
To: qemu-devel
Hi Peter,
Le 16-04-2013 15:23, Peter Crosthwaite a écrit :
> Hi Francois,
>
> On Tue, Apr 16, 2013 at 10:50 PM, François Legal
> <francois.legal@thom.fr.eu.org> wrote:
>
>> Le 16-04-2013 14:19, Peter Maydell a écrit :
>>
>>> On 16 April 2013 13:09, François Legal <devel@thom.fr.eu.org> wrote:
>>> Ugh. Your mail client has completely mangled things (it's run all the
>>> lines of code into each other and it's still posting as multipart
>>> text+HTML). Please could you look at fixing its configuration -- it
>>> makes it hard to reply in response to individual things you've said.
>> Sorry !
>>
>>> Anyway, you may be right about needing multiple qemutimers, I think I
>>> misunderstood that part. We set up one timer for each comparator,
>>> right? (ie it fires when the comparator would fire). In that case I
>>> think that is correct.
>
> I had this problem with timer/cadence_ttc.c, which is a single timer
> with multiple comparators. Went the other way implementation wise
> though, using a single QEMUTimer and each trap of the timer it
> computes which event is going to occur next and qemu_mod_timer with
> the MIN of the comparators.
>
That's another solution, but I found out that, given my poor knowledge on
Qemu, the other way was easier ;-)
>> At least, that's how I understood the stuff.
>>
>>> You might like to try having each gTimerBlock have an
>>> ARMMPGTimerState* field, so you get at the non-banked parts of the
>>> control register with 'gtb->gtimer.gtimer_control' rather than via an
>>> anonymous uint32_t*. I think that would be clearer.
>> Ok.
>>
>>> Regarding gdb access to memory mapped registers causing a crash due
>>> to NULL cpu_single_env -- I think this is a general issue with the
>>> gdb support code. I suggest you drop those parts of your patch for
>>> now; we should look at fixing it in a coherent way separately and
>>> later (eg by having gdb memory accesses always look as if they are
>>> from CPU 0, or something).
>> Alright. I'll drop that from the p
>>
>>> quote>PS: I didn't mention it, but the struct names and so on should
>>> also change in line with my suggested new device name/> te> Done.
>> ks -- PMM New patch follows.
>
> Use git to create and send patches. Check out the commands:
>
> git format-patch
> git send-email
>
> This will send you patch as plain text as well.
I wish I could, unfortunately, my corporate VPN/proxy blocks GIT.
>
> Regards,
> Peter
Links:
------
[1] http://www.gnu.org/licenses/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer
2013-04-16 14:06 ` François Legal
@ 2013-04-16 15:17 ` Peter Crosthwaite
0 siblings, 0 replies; 10+ messages in thread
From: Peter Crosthwaite @ 2013-04-16 15:17 UTC (permalink / raw)
To: François Legal; +Cc: qemu-devel
Hi Francois,
On Wed, Apr 17, 2013 at 12:06 AM, François Legal
<francois.legal@thom.fr.eu.org> wrote:
> Hi Peter,
>
> Le 16-04-2013 15:23, Peter Crosthwaite a écrit :
>
>
>> Hi Francois,
>>
>> On Tue, Apr 16, 2013 at 10:50 PM, François Legal
>> <francois.legal@thom.fr.eu.org> wrote:
>>
>>> Le 16-04-2013 14:19, Peter Maydell a écrit :
>>>
>>>> On 16 April 2013 13:09, François Legal <devel@thom.fr.eu.org> wrote:
>>>> Ugh. Your mail client has completely mangled things (it's run all the
>>>> lines of code into each other and it's still posting as multipart
>>>> text+HTML). Please could you look at fixing its configuration -- it
>>>> makes it hard to reply in response to individual things you've said.
>>>
>>> Sorry !
>>>
>>>> Anyway, you may be right about needing multiple qemutimers, I think I
>>>> misunderstood that part. We set up one timer for each comparator,
>>>> right? (ie it fires when the comparator would fire). In that case I
>>>> think that is correct.
>>
>>
>> I had this problem with timer/cadence_ttc.c, which is a single timer
>> with multiple comparators. Went the other way implementation wise
>> though, using a single QEMUTimer and each trap of the timer it
>> computes which event is going to occur next and qemu_mod_timer with
>> the MIN of the comparators.
>>
>
> That's another solution, but I found out that, given my poor knowledge on
> Qemu, the other way was easier ;-)
>
>>> At least, that's how I understood the stuff.
>>>
>>>> You might like to try having each gTimerBlock have an
>>>> ARMMPGTimerState* field, so you get at the non-banked parts of the
>>>> control register with 'gtb->gtimer.gtimer_control' rather than via an
>>>> anonymous uint32_t*. I think that would be clearer.
>>>
>>> Ok.
>>>
>>>> Regarding gdb access to memory mapped registers causing a crash due
>>>> to NULL cpu_single_env -- I think this is a general issue with the
>>>> gdb support code. I suggest you drop those parts of your patch for
>>>> now; we should look at fixing it in a coherent way separately and
>>>> later (eg by having gdb memory accesses always look as if they are
>>>> from CPU 0, or something).
>>>
>>> Alright. I'll drop that from the p
>>>
>>>> quote>PS: I didn't mention it, but the struct names and so on should
>>>> also change in line with my suggested new device name/> te> Done.
>>>
>>> ks -- PMM New patch follows.
>>
>>
>> Use git to create and send patches. Check out the commands:
>>
>> git format-patch
>> git send-email
>>
>> This will send you patch as plain text as well.
>
>
> I wish I could, unfortunately, my corporate VPN/proxy blocks GIT.
>
It is very difficult to work on this project without git.
But git remotes also work over http, so that could be usable to do
clones and pulls. If QEMU still has an up-to-date github mirror, you
should be able to clone from there.
So once you have your http clone you can use git locally to create and
format your patches. You are most of the way there, you just need a
way to send email. Just need a smtp server and config git send-email
to talk to it.
Ultimately, none of this requires the git network protocol - so git
can be done even with a proxy/firewall that blocks git.
Regards,
Peter
>>
>> Regards,
>> Peter
>
>
>
>
> Links:
> ------
> [1] http://www.gnu.org/licenses/
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer
2013-04-16 12:50 ` [Qemu-devel] [PATCH V2] " François Legal
2013-04-16 13:23 ` Peter Crosthwaite
@ 2013-06-24 5:40 ` Peter Crosthwaite
1 sibling, 0 replies; 10+ messages in thread
From: Peter Crosthwaite @ 2013-06-24 5:40 UTC (permalink / raw)
To: François Legal; +Cc: qemu-devel
I've respun this, but still testing and debugging a few issues.
Some notes FTR.
On Tue, Apr 16, 2013 at 10:50 PM, François Legal
<francois.legal@thom.fr.eu.org> wrote:
> Le 16-04-2013 14:19, Peter Maydell a écrit :
[Snip]
>
>
> New patch follows.
>
> ---
>
> diff -urN qemu-master.old/hw/cpu/a9mpcore.c qemu-master/hw/cpu/a9mpcore.c
> --- qemu-master.old/hw/cpu/a9mpcore.c 2013-04-08 20:12:33.000000000 +0200
> +++ qemu-master/hw/cpu/a9mpcore.c 2013-04-16 13:18:39.000000000 +0200
>
> @@ -15,6 +15,7 @@
> uint32_t num_cpu;
> MemoryRegion container;
> DeviceState *mptimer;
> + DeviceState *mpgtimer;
> DeviceState *wdt;
> DeviceState *gic;
> DeviceState *scu;
> @@ -31,6 +32,7 @@
> {
> A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
> SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
> + SysBusDevice *gtimerbusdev;
> int i;
>
> s->gic = qdev_create(NULL, "arm_gic");
> @@ -50,6 +52,11 @@
> qdev_init_nofail(s->scu);
> scubusdev = SYS_BUS_DEVICE(s->scu);
>
> + s->mpgtimer = qdev_create(NULL, "a9_globaltimer");
>
> + qdev_prop_set_uint32(s->mpgtimer, "num-cpu", s->num_cpu);
> + qdev_init_nofail(s->mpgtimer);
> + gtimerbusdev = SYS_BUS_DEVICE(s->mpgtimer);
> +
> s->mptimer = qdev_create(NULL, "arm_mptimer");
> qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
> qdev_init_nofail(s->mptimer);
> @@ -68,8 +75,6 @@
> * 0x0600-0x06ff -- private timers and watchdogs
> * 0x0700-0x0fff -- nothing
> * 0x1000-0x1fff -- GIC Distributor
> - *
> - * We should implement the global timer but don't currently do so.
> */
> memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
> memory_region_add_subregion(&s->container, 0,
> @@ -80,6 +85,8 @@
> /* Note that the A9 exposes only the "timer/watchdog for this core"
> * memory region, not the "timer/watchdog for core X" ones 11MPcore
> has.
> */
> + memory_region_add_subregion(&s->container, 0x200,
> + sysbus_mmio_get_region(gtimerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x600,
> sysbus_mmio_get_region(timerbusdev, 0));
> memory_region_add_subregion(&s->container, 0x620,
> @@ -90,10 +97,13 @@
> sysbus_init_mmio(dev, &s->container);
>
> /* Wire up the interrupt from each watchdog and timer.
> - * For each core the timer is PPI 29 and the watchdog PPI 30.
> + * For each core the global timer is PPI 27, the private
> + * timer is PPI 29 and the watchdog PPI 30.
> */
> for (i = 0; i < s->num_cpu; i++) {
> int ppibase = (s->num_irq - 32) + i * 32;
> + sysbus_connect_irq(gtimerbusdev, i,
> + qdev_get_gpio_in(s->gic, ppibase + 27));
> sysbus_connect_irq(timerbusdev, i,
> qdev_get_gpio_in(s->gic, ppibase + 29));
> sysbus_connect_irq(wdtbusdev, i,
> diff -urN qemu-master.old/hw/timer/a9gtimer.c
> qemu-master/hw/timer/a9gtimer.c
> --- qemu-master.old/hw/timer/a9gtimer.c 1970-01-01 01:00:00.000000000 +0100
> +++ qemu-master/hw/timer/a9gtimer.c 2013-04-16 14:35:48.000000000 +0200
> @@ -0,0 +1,348 @@
> +/*
> + * Global peripheral timer block for ARM A9MP
>
> + *
> + * Written by François LEGAL
> + *
> + * 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"
> +
> +/* This device implements the per-cpu private timer and watchdog block
> + * which is used in the Cortex-A9MP.
>
> + */
> +
> +#define MAX_CPUS 4
> +#define TYPE_GTIMER "a9_globaltimer"
> +#define GTIMER(obj) OBJECT_CHECK(a9globaltimerState, (obj), TYPE_GTIMER)
>
> +
> +/* State of a single gtimer or block */
> +typedef struct {
> + uint32_t control;
> + uint64_t compare;
> + uint32_t inc;
> + uint32_t status;
> + int64_t tick;
> +
> + int64_t delta;
> +
> + struct a9globaltimerState *gtimer_state;
>
> + QEMUTimer *timer;
Set it up so there's only one timer which will trigger on the
next-to-occur comparator. Stops rounding error from potentially
occuring events out of order.
> + MemoryRegion iomem;
> + qemu_irq irq;
> +} gTimerBlock;
> +
> +typedef struct a9globaltimerState {
>
> + SysBusDevice busdev;
> + uint32_t num_cpu;
> + uint64_t gtimer_counter;
> + uint32_t gtimer_control;
> + gTimerBlock gtimer[MAX_CPUS];
> + MemoryRegion iomem;
> +} a9globaltimerState;
Struct name fixed.
> +
> +static inline int 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;
> +}
> +
> +static inline uint32_t gtimerblock_get_control_reg(gTimerBlock *gtb)
> +{
> + return gtb->control | gtb->gtimer_state->gtimer_control;
> +}
>
> +
> +static inline void gtimerblock_update_irq(gTimerBlock *gtb)
> +{
> + qemu_set_irq(gtb->irq, gtb->status);
> +}
> +
> +/* Return conversion factor from mpcore timer ticks to qemu timer ticks.
> */
> +static inline uint32_t gtimerblock_scale(gTimerBlock *gtb)
> +{
> + return (((gtb->gtimer_state->gtimer_control >> 8) & 0xff) + 1) * 10;
>
> +}
> +
> +static void gtimerblock_reload(gTimerBlock *gtb, int restart)
> +{
> + if (restart) {
> + gtb->tick = qemu_get_clock_ns(vm_clock);
> + gtb->tick += (int64_t)(((gtb->compare -
> + gtb->gtimer_state->gtimer_counter) +
>
> + gtb->delta) * gtimerblock_scale(gtb));
> + } else {
> + gtb->tick += (int64_t)(gtb->inc * gtimerblock_scale(gtb));
> + }
> + qemu_mod_timer(gtb->timer, gtb->tick);
> +}
> +
> +static void gtimerblock_tick(void *opaque)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + gtb->gtimer_state->gtimer_counter = gtb->compare;
> + if (gtimerblock_get_control_reg(gtb) & 0x9) {
Bad condition - auto will always happen when timer enabled.
>
> + gtb->compare += gtb->inc;
> + gtimerblock_reload(gtb, 0);
> + }
> + if ((gtimerblock_get_control_reg(gtb) & 0x7) &&
>
This reads that if any of the three LSB of control are set you get an interrupt.
> + ((gtb->status & 1) == 0)) {
> + gtb->status = 1;
> + gtimerblock_update_irq(gtb);
> + }
> +}
> +
> +static uint64_t gtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t val = 0;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + if (gtimerblock_get_control_reg(gtb) & 1) {
>
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
Strange - the further forward you are in emulation time, the lower the
counter value? I had some strangness here when using the timer in
polled mode - it ran backwards on me and I think its this, and I think
the operation of the timer is dep. on interrupts. I've reworked this
significantly to get polling working.
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? val : 0 & 0xFFFFFFFF;
> + case 4: /* Counter MSB. */
> + if (gtimerblock_get_control_reg(gtb) & 1) {
>
> + val = gtb->tick - qemu_get_clock_ns(vm_clock);
> + val /= gtimerblock_scale(gtb);
> + }
> + return val < 0 ? (val >> 32) : 0;
> + case 8: /* Control. */
> + return gtimerblock_get_control_reg(gtb) & 0x0000FF0F;
>
factored out replicated code. used extract64 rather than mask/shift.
> + case 12: /* Interrupt status. */
> + return gtb->status;
> + case 16: /* Compare LSB */
> + return gtb->compare & 0xFFFFFFFF;
> + case 20: /* Counter MSB */
> + return gtb->compare >> 32;
> + case 24: /* Autoincrement */
> + return gtb->inc;
> + default:
> + return 0;
> + }
> +}
> +
> +static void gtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + gTimerBlock *gtb = (gTimerBlock *)opaque;
> + int64_t old;
> + switch (addr) {
> + case 0: /* Counter LSB */
> + old = gtb->gtimer_state->gtimer_counter;
>
> + old &= 0xFFFFFFFF;
> + gtb->delta = old - (value & 0xFFFFFFFF);
> + old |= value & 0xFFFFFFFF;
> + gtb->gtimer_state->gtimer_counter = old;
>
> + /* Cancel the previous timer. */
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 4: /* Counter MSB. */
> + old = gtb->gtimer_state->gtimer_counter;
>
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = old - (value << 32);
> + old |= value << 32;
> + gtb->gtimer_state->gtimer_counter = old;
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 8: /* Control. */
> + old = gtb->gtimer_state->gtimer_control;
>
> + gtb->control = value & 0x0000000E;
> + gtb->gtimer_state->gtimer_control = value & 0x0000FF01;
>
> + if (((old & 1) == 0) && ((value & 7) == 7)) {
> + gtb->delta = 0;
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 12: /* Interrupt status. */
> + gtb->status &= ~ value;
>
> + gtimerblock_update_irq(gtb);
> + break;
> + case 16: /* Compare LSB */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF;
> + gtb->delta = (value & 0xFFFFFFFF) - old;
> + old |= value & 0xFFFFFFFF;
> + gtb->compare = old;
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 20: /* Compare MSB. */
> + old = gtb->compare;
> + old &= 0xFFFFFFFF00000000;
> + gtb->delta = (value << 32) - old;
> + old |= value << 32;
> + gtb->compare = old;
> + if ((gtimerblock_get_control_reg(gtb) & 7) == 7) {
>
> + gtimerblock_reload(gtb, 1);
> + }
> + break;
> + case 24: /* Autoincrement */
> + gtb->inc = value;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/* Wrapper functions to implement the "read global timer for
>
> + * the current CPU" memory regions.
> + */
> +static uint64_t arm_thisgtimer_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + a9globaltimerState *s = (a9globaltimerState *)opaque;
>
> + int id = get_current_cpu(s);
> + return gtimer_read(&s->gtimer[id], addr, size);
> +}
> +
> +static void arm_thisgtimer_write(void *opaque, hwaddr addr,
> + uint64_t value, unsigned size)
> +{
> + a9globaltimerState *s = (a9globaltimerState *)opaque;
>
> + int id = get_current_cpu(s);
> + gtimer_write(&s->gtimer[id], addr, value, size);
> +}
> +
> +static const MemoryRegionOps arm_thisgtimer_ops = {
> + .read = arm_thisgtimer_read,
> + .write = arm_thisgtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const MemoryRegionOps gtimerblock_ops = {
> + .read = gtimer_read,
> + .write = gtimer_write,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void gtimer_reset(gTimerBlock *gtb)
> +{
> + if (gtb->timer) {
> + qemu_del_timer(gtb->timer);
>
> + }
> + gtb->control = 0;
> + gtb->status = 0;
> + gtb->compare = 0;
> + gtb->inc = 0;
> + gtb->tick = 0;
> +}
> +
> +static void a9mp_globaltimer_reset(DeviceState *dev)
> +{
> + a9globaltimerState *s = GTIMER(dev);
>
> + int i;
> + for (i = 0; i < ARRAY_SIZE(s->gtimer); i++) {
> + gtimer_reset(&s->gtimer[i]);
> + }
> +}
> +
> +static void a9mp_globaltimer_realize(DeviceState *dev, Error **errp)
> +{
> + a9globaltimerState *s = GTIMER(dev);
> + int i;
> +
>
> + if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
> + error_setg(errp, "%s: num-cpu must be between 1 and %d\n",
> + __func__, MAX_CPUS);
> + }
> +
>
> + memory_region_init_io(&s->iomem, &arm_thisgtimer_ops, s,
> + "arm_mptimer_gtimer", 0x20);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>
> + for (i = 0; i < s->num_cpu; i++) {
> + gTimerBlock *gtb = &s->gtimer[i];
> + gtb->gtimer_state = s;
>
> + gtb->timer = qemu_new_timer_ns(vm_clock, gtimerblock_tick, gtb);
> + sysbus_init_irq(SYS_BUS_DEVICE(dev), >b->irq);
>
> + memory_region_init_io(>b->iomem, >imerblock_ops, gtb,
> + "arm_mptimer_gtimerblock", 0x20);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), >b->iomem);
> + }
> +}
> +
>
> +static const VMStateDescription vmstate_gtimerblock = {
> + .name = "a9mp_gtimerblock",
> + .version_id = 1,
> + .minimum_version_id = 1,
>
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(control, gTimerBlock),
> + VMSTATE_UINT32(status, gTimerBlock),
> + VMSTATE_UINT64(compare, gTimerBlock),
> + VMSTATE_INT64(tick, gTimerBlock),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_a9mp_globaltimer = {
> + .name = "a9mp_globaltimer",
> + .version_id = 1,
> + .minimum_version_id = 1,
>
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT_VARRAY_UINT32(gtimer, a9globaltimerState, num_cpu,
>
> + 1, vmstate_gtimerblock, gTimerBlock),
> + VMSTATE_END_OF_LIST()
Dont think theres enough state here. The counter value should be here.
> + }
> +};
> +
> +static Property a9mp_globaltimer_properties[] = {
> + DEFINE_PROP_UINT32("num-cpu", a9globaltimerState, num_cpu, 0),
> + DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void a9mp_globaltimer_class_init(ObjectClass *klass, void *data)
>
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = a9mp_globaltimer_realize;
> + dc->vmsd = &vmstate_a9mp_globaltimer;
> + dc->reset = a9mp_globaltimer_reset;
>
> + dc->no_user = 1;
> + dc->props = a9mp_globaltimer_properties;
> +}
> +
> +static const TypeInfo a9mp_globaltimer_info = {
> + .name = "a9_globaltimer",
Macroified.
Regards,
Peter
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(a9globaltimerState),
> + .class_init = a9mp_globaltimer_class_init,
> +};
> +
> +static void a9mp_globaltimer_register_types(void)
> +{
> + type_register_static(&a9mp_globaltimer_info);
> +}
> +
> +type_init(a9mp_globaltimer_register_types)
> diff -urN qemu-master.old/hw/timer/Makefile.objs
> qemu-master/hw/timer/Makefile.objs
> --- qemu-master.old/hw/timer/Makefile.objs 2013-04-08
> 20:12:33.000000000 +0200
> +++ qemu-master/hw/timer/Makefile.objs 2013-04-16 13:53:09.000000000 +0200
>
> @@ -24,5 +24,5 @@
> 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
>
> Signed-off-by: François LEGAL <devel@thom.fr.eu.org>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-06-24 5:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-15 15:41 [Qemu-devel] [PATCH] ARM Cortex A9 Global Timer François Legal
2013-04-16 9:50 ` Peter Maydell
2013-04-16 12:09 ` François Legal
2013-04-16 12:19 ` Peter Maydell
2013-04-16 12:50 ` [Qemu-devel] [PATCH V2] " François Legal
2013-04-16 13:23 ` Peter Crosthwaite
2013-04-16 14:06 ` François Legal
2013-04-16 15:17 ` Peter Crosthwaite
2013-06-24 5:40 ` Peter Crosthwaite
2013-04-16 13:29 ` [Qemu-devel] [PATCH] " Peter Crosthwaite
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).