From patchwork Tue Apr 16 12:50:41 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Fran=C3=A7ois_Legal?= X-Patchwork-Id: 236950 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1E1CB2C010C for ; Tue, 16 Apr 2013 22:51:50 +1000 (EST) Received: from localhost ([::1]:53275 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1US5MS-0006hr-8C for incoming@patchwork.ozlabs.org; Tue, 16 Apr 2013 08:51:48 -0400 Received: from eggs.gnu.org ([208.118.235.92]:40855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1US5Ly-0006Ss-OX for qemu-devel@nongnu.org; Tue, 16 Apr 2013 08:51:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1US5Lu-0001CY-NC for qemu-devel@nongnu.org; Tue, 16 Apr 2013 08:51:18 -0400 Received: from smtp1-g21.free.fr ([2a01:e0c:1:1599::10]:60875) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1US5Lt-00014b-Vw for qemu-devel@nongnu.org; Tue, 16 Apr 2013 08:51:14 -0400 Received: from mail.thom.fr.eu.org (unknown [IPv6:2a01:e35:2f1f:e7b0:215:17ff:fec0:7e1f]) by smtp1-g21.free.fr (Postfix) with ESMTP id 5B7AA9401E0 for ; Tue, 16 Apr 2013 14:50:43 +0200 (CEST) Received: from mail.thom.fr.eu.org (localhost [127.0.0.1]) by mail.thom.fr.eu.org (8.14.3/8.14.3/Debian-9.4) with ESMTP id r3GCofY0031985 for ; Tue, 16 Apr 2013 14:50:41 +0200 MIME-Version: 1.0 Date: Tue, 16 Apr 2013 14:50:41 +0200 From: =?UTF-8?Q?Fran=C3=A7ois_Legal?= To: In-Reply-To: References: <3e0e268a897603d5ae067f6e01fad68e@thom.fr.eu.org> <0487933dd01712ab479f234975b8ea2c@thom.fr.eu.org> Message-ID: <37e5da1bbc6d72dac52559deac7ef181@thom.fr.eu.org> X-Sender: francois.legal@thom.fr.eu.org User-Agent: Roundcube Webmail/0.8.5 X-Virus-Scanned: clamav-milter 0.97.6 at tls-srv-01 X-Virus-Status: Clean X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a01:e0c:1:1599::10 Subject: Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Le 16-04-2013 14:19, Peter Maydell a écrit : > On 16 April 2013 13:09, François Legal 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. --- +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 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 . + */ + +#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); +} +