qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH V2] ARM Cortex A9 Global Timer
Date: Tue, 16 Apr 2013 23:23:19 +1000

Hi Francois,

On Tue, Apr 16, 2013 at 10:50 PM, François Legal
<address@hidden> wrote:
> Le 16-04-2013 14:19, Peter Maydell a écrit :
>
>
>> On 16 April 2013 13:09, François Legal <address@hidden> 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), &gtb->irq);
>
> +        memory_region_init_io(&gtb->iomem, &gtimerblock_ops, gtb,
> +                              "arm_mptimer_gtimerblock", 0x20);
> +        sysbus_init_mmio(SYS_BUS_DEVICE(dev), &gtb->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 <address@hidden>
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]