Re: [Qemu-devel] [PATCH v7 3/6] hw/arm/digic: add timer support

[Antony Pavlov]

On Fri, 13 Dec 2013 09:20:27 +1000
Peter Crosthwaite <address@hidden> wrote:

> On Fri, Dec 13, 2013 at 8:23 AM, Antony Pavlov <address@hidden> wrote:
> > Signed-off-by: Antony Pavlov <address@hidden>
> > Reviewed-by: Peter Maydell <address@hidden>
> > ---
> >  hw/arm/digic.c         |  28 ++++++++++
> >  hw/timer/Makefile.objs |   1 +
> >  hw/timer/digic-timer.c | 140 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/timer/digic-timer.h |  36 +++++++++++++
> >  include/hw/arm/digic.h |   6 +++
> >  5 files changed, 211 insertions(+)
> >  create mode 100644 hw/timer/digic-timer.c
> >  create mode 100644 hw/timer/digic-timer.h
> >
> > diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> > index 2620262..e8eb0de 100644
> > --- a/hw/arm/digic.c
> > +++ b/hw/arm/digic.c
> > @@ -22,18 +22,35 @@
> >
> >  #include "hw/arm/digic.h"
> >
> > +#define DIGIC4_TIMER_BASE(n)    (0xc0210000 + (n) * 0x100)
> > +
> >  static void digic_init(Object *obj)
> >  {
> >      DigicState *s = DIGIC(obj);
> > +    DeviceState *dev;
> > +    int i;
> >
> >      object_initialize(&s->cpu, sizeof(s->cpu), "arm946-" TYPE_ARM_CPU);
> >      object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> > +
> > +    for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
> > +#define DIGIC_TIMER_NAME_MLEN    11
> > +        char name[DIGIC_TIMER_NAME_MLEN];
> > +
> > +        object_initialize(&s->timer[i], sizeof(s->timer[i]), 
> > TYPE_DIGIC_TIMER);
> > +        dev = DEVICE(&s->timer[i]);
> > +        qdev_set_parent_bus(dev, sysbus_get_default());
> > +        snprintf(name, DIGIC_TIMER_NAME_MLEN, "timer[%d]", i);
> > +        object_property_add_child(obj, name, OBJECT(&s->timer[i]), NULL);
> > +    }
> >  }
> >
> >  static void digic_realize(DeviceState *dev, Error **errp)
> >  {
> >      DigicState *s = DIGIC(dev);
> >      Error *err = NULL;
> > +    SysBusDevice *sbd;
> > +    int i;
> >
> >      object_property_set_bool(OBJECT(&s->cpu), true, "reset-hivecs", &err);
> >      if (err != NULL) {
> > @@ -46,6 +63,17 @@ static void digic_realize(DeviceState *dev, Error **errp)
> >          error_propagate(errp, err);
> >          return;
> >      }
> > +
> > +    for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
> > +        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized", 
> > &err);
> > +        if (err != NULL) {
> > +            error_propagate(errp, err);
> > +            return;
> > +        }
> > +
> > +        sbd = SYS_BUS_DEVICE(&s->timer[i]);
> > +        sysbus_mmio_map(sbd, 0, DIGIC4_TIMER_BASE(i));
> > +    }
> >  }
> >
> >  static void digic_class_init(ObjectClass *oc, void *data)
> > diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> > index 3ae091c..ea9f11f 100644
> > --- a/hw/timer/Makefile.objs
> > +++ b/hw/timer/Makefile.objs
> > @@ -26,5 +26,6 @@ obj-$(CONFIG_OMAP) += omap_synctimer.o
> >  obj-$(CONFIG_PXA2XX) += pxa2xx_timer.o
> >  obj-$(CONFIG_SH4) += sh_timer.o
> >  obj-$(CONFIG_TUSB6010) += tusb6010.o
> > +obj-$(CONFIG_DIGIC) += digic-timer.o
> >
> >  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
> > diff --git a/hw/timer/digic-timer.c b/hw/timer/digic-timer.c
> > new file mode 100644
> > index 0000000..974e588
> > --- /dev/null
> > +++ b/hw/timer/digic-timer.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * QEMU model of the Canon DIGIC timer block.
> > + *
> > + * Copyright (C) 2013 Antony Pavlov <address@hidden>
> > + *
> > + * This model is based on reverse engineering efforts
> > + * made by CHDK (http://chdk.wikia.com) and
> > + * Magic Lantern (http://www.magiclantern.fm) projects
> > + * contributors.
> > + *
> > + * See "Timer/Clock Module" docs here:
> > + *   http://magiclantern.wikia.com/wiki/Register_Map
> > + *
> > + * The QEMU model of the OSTimer in PKUnity SoC by Guan Xuetao
> > + * is used as a template.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include "hw/sysbus.h"
> > +#include "hw/ptimer.h"
> > +#include "qemu/main-loop.h"
> > +
> > +#include "hw/timer/digic-timer.h"
> > +
> > +#define DIGIC_TIMER_CONTROL 0x00
> > +#define DIGIC_TIMER_VALUE 0x0c
> > +
> > +static const VMStateDescription vmstate_digic_timer = {
> > +    .name = "digic.timer",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_PTIMER(ptimer, DigicTimerState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static uint64_t digic_timer_read(void *opaque, hwaddr offset, unsigned 
> > size)
> > +{
> > +    DigicTimerState *s = opaque;
> > +    uint32_t ret = 0;
> > +
> > +    switch (offset) {
> > +    case DIGIC_TIMER_VALUE:
> > +        ret = (uint32_t)ptimer_get_count(s->ptimer);
> > +        ret &= 0xffff;
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "digic-timer: read access to unknown register 0x"
> > +                      TARGET_FMT_plx, offset);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static void digic_timer_write(void *opaque, hwaddr offset,
> > +                              uint64_t value, unsigned size)
> > +{
> > +    DigicTimerState *s = opaque;
> > +
> > +    /* FIXME: without documentation every write just starts timer */
> > +    ptimer_set_limit(s->ptimer, 0x0000ffff, 1);
> > +    ptimer_run(s->ptimer, 1);
> 
> You are chaining one-shot timers which is not the correct way to do
> periodic timing. Can't you just start the timer as periodic here ...

I used hw/timer/lm32_timer.c as a model. Could you point to a better model 
please?


> > +}
> > +
> > +static const MemoryRegionOps digic_timer_ops = {
> > +    .read = digic_timer_read,
> > +    .write = digic_timer_write,
> > +    .impl = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> > +static void digic_timer_tick(void *opaque)
> > +{
> > +    DigicTimerState *s = opaque;
> > +
> > +    ptimer_run(s->ptimer, 1);
> 
> ... and then this is just a nop?
> 
> Regards,
> Peter
> 
> > +}
> > +
> > +static void digic_timer_init(Object *obj)
> > +{
> > +    DigicTimerState *s = DIGIC_TIMER(obj);
> > +
> > +    s->bh = qemu_bh_new(digic_timer_tick, s);
> > +    s->ptimer = ptimer_init(s->bh);
> > +
> > +    /*
> > +     * FIXME: there is no documentation on Digic timer
> > +     * frequency setup so let it always run at 1 MHz
> > +     */
> > +    ptimer_set_freq(s->ptimer, 1 * 1000 * 1000);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &digic_timer_ops, s,
> > +                          TYPE_DIGIC_TIMER, 0x100);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> > +}
> > +
> > +static void digic_timer_reset(DeviceState *dev)
> > +{
> > +    DigicTimerState *s = DIGIC_TIMER(dev);
> > +
> > +    ptimer_stop(s->ptimer);
> > +}
> > +
> > +static void digic_timer_class_init(ObjectClass *klass, void *class_data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    dc->reset = digic_timer_reset;
> > +    dc->vmsd = &vmstate_digic_timer;
> > +}
> > +
> > +static const TypeInfo digic_timer_info = {
> > +    .name = TYPE_DIGIC_TIMER,
> > +    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(DigicTimerState),
> > +    .instance_init = digic_timer_init,
> > +    .class_init = digic_timer_class_init,
> > +};
> > +
> > +static void digic_timer_register_type(void)
> > +{
> > +    type_register_static(&digic_timer_info);
> > +}
> > +
> > +type_init(digic_timer_register_type)
> > diff --git a/hw/timer/digic-timer.h b/hw/timer/digic-timer.h
> > new file mode 100644
> > index 0000000..daf271d
> > --- /dev/null
> > +++ b/hw/timer/digic-timer.h
> > @@ -0,0 +1,36 @@
> > +/*
> > + * Canon DIGIC timer block declarations.
> > + *
> > + * Copyright (C) 2013 Antony Pavlov <address@hidden>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#ifndef HW_TIMER_DIGIC_TIMER_H
> > +#define HW_TIMER_DIGIC_TIMER_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "qemu/typedefs.h"
> > +#include "hw/ptimer.h"
> > +
> > +#define TYPE_DIGIC_TIMER "digic-timer"
> > +#define DIGIC_TIMER(obj) OBJECT_CHECK(DigicTimerState, (obj), 
> > TYPE_DIGIC_TIMER)
> > +
> > +typedef struct DigicTimerState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion iomem;
> > +    QEMUBH *bh;
> > +    ptimer_state *ptimer;
> > +} DigicTimerState;
> > +
> > +#endif /* HW_TIMER_DIGIC_TIMER_H */
> > diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h
> > index b7d16fb..177a06d 100644
> > --- a/include/hw/arm/digic.h
> > +++ b/include/hw/arm/digic.h
> > @@ -20,16 +20,22 @@
> >
> >  #include "cpu.h"
> >
> > +#include "hw/timer/digic-timer.h"
> > +
> >  #define TYPE_DIGIC "digic"
> >
> >  #define DIGIC(obj) OBJECT_CHECK(DigicState, (obj), TYPE_DIGIC)
> >
> > +#define DIGIC4_NB_TIMERS 3
> > +
> >  typedef struct DigicState {
> >      /*< private >*/
> >      DeviceState parent_obj;
> >      /*< public >*/
> >
> >      ARMCPU cpu;
> > +
> > +    DigicTimerState timer[DIGIC4_NB_TIMERS];
> >  } DigicState;
> >
> >  #endif /* HW_ARM_DIGIC_H */
> > --
> > 1.8.5
> >
> >


-- 
-- 
Best regards,
  Antony Pavlov