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

[Antony Pavlov]

On Sat, 14 Dec 2013 16:41:44 +1000
Peter Crosthwaite <address@hidden> wrote:

> On Sat, Dec 14, 2013 at 7:42 AM, Antony Pavlov <address@hidden> wrote:
> > Signed-off-by: Antony Pavlov <address@hidden>
> > ---
> >  hw/arm/digic.c         |  28 +++++++++
> >  hw/timer/Makefile.objs |   1 +
> >  hw/timer/digic-timer.c | 168 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/timer/digic-timer.h |  38 +++++++++++
> >  include/hw/arm/digic.h |   6 ++
> >  5 files changed, 241 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..14cd352
> > --- /dev/null
> > +++ b/hw/timer/digic-timer.c
> > @@ -0,0 +1,168 @@
> > +/*
> > + * 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_CONTROL_RST 0x80000000
> > +#define DIGIC_TIMER_CONTROL_EN 0x00000001
> > +#define DIGIC_TIMER_RELVALUE 0x08
> > +#define DIGIC_TIMER_VALUE 0x0c
> > +
> 
> Programmer model #defines should go in the timer/digic-timer.h header
> to allow for easy generation of test stimulus - other code can
> exercise your device using your already written full macro suite.
> 
> > +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_UINT32(control, DigicTimerState),
> > +        VMSTATE_UINT32(relvalue, DigicTimerState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void digic_timer_reset(DeviceState *dev)
> > +{
> > +    DigicTimerState *s = DIGIC_TIMER(dev);
> > +
> > +    ptimer_stop(s->ptimer);
> > +    s->control = 0;
> > +    s->relvalue = 0;
> > +}
> > +
> > +static uint64_t digic_timer_read(void *opaque, hwaddr offset, unsigned 
> > size)
> > +{
> > +    DigicTimerState *s = opaque;
> > +    uint64_t ret = 0;
> > +
> > +    switch (offset) {
> > +    case DIGIC_TIMER_CONTROL:
> > +        ret = s->control;
> > +        break;
> > +    case DIGIC_TIMER_RELVALUE:
> > +        ret = s->relvalue;
> > +        break;
> > +    case DIGIC_TIMER_VALUE:
> > +        ret = ptimer_get_count(s->ptimer) & 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;
> > +
> > +    switch (offset) {
> > +    case DIGIC_TIMER_CONTROL:
> > +        if (value & DIGIC_TIMER_CONTROL_RST) {
> > +            digic_timer_reset((DeviceState *)s);
> > +            break;
> > +        }
> > +
> > +        if (value & DIGIC_TIMER_CONTROL_EN) {
> > +            ptimer_run(s->ptimer, 0);
> > +        }
> 
> So is there definately no stop function on de-assertion of this bit? I
> couldnt find it in your documentation but it seem quite natural to me
> that,
> 
> } else {
>     ptimer_stop(s->ptimer);
> }
> 
> Would probably be the implementation of an enable bit, rather than the
> only way to stop the timer being soft reset. I wont block on it
> however.
> 
> > +
> > +        s->control = (uint32_t)value;
> > +        break;
> > +
> > +    case DIGIC_TIMER_RELVALUE:
> > +        s->relvalue = (uint32_t)(value & 0xffff);
> 
> extract32 is preferable over & >> logic.
> 
> > +        ptimer_set_limit(s->ptimer, s->relvalue, 1);
> > +        break;
> > +
> > +    case DIGIC_TIMER_VALUE:
> > +        break;
> > +
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "digic-timer: read access to unknown register 0x"
> > +                      TARGET_FMT_plx, offset);
> > +    }
> > +}
> > +
> > +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_init(Object *obj)
> > +{
> > +    DigicTimerState *s = DIGIC_TIMER(obj);
> > +
> > +    s->ptimer = ptimer_init(NULL);
> > +
> > +    /*
> > +     * 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_class_init(ObjectClass *klass, void *class_data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> 
> Add a blank line here.
> 
> > +    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..a1e526e
> > --- /dev/null
> > +++ b/hw/timer/digic-timer.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * 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 {
> 
> /*< private >*/
> 
> > +    SysBusDevice parent_obj;
> 
> /*< public >*/
> 
> This is all trivial stuff. So if you make the changes (with the
> stop-on-no-en being optional),
> 
> Reviewed-by: Peter Crosthwaite <address@hidden>

I fixed all trivial stuff but the stop-on-no-en and reposted the v9 patchseries.

There are some reasons to not fix stop-on-no-en just now:
1. there is no good documentation on DIGIC. So if I want to realize 
stop-on-no-en
timer feature then I have to check it in hardware. But just now my only 
DIGIC4-based
camera with UART is mothballed.
2. this patchseries is intended only for initial DIGIC support; the only 
software for
this initial support is barebox bootloader that run correctly on real hardware.
After adding initial DIGIC support I'm planning to make advanched DIGIC support
for Magic Lantern and CHDK. So I can carry over realization of more accurate 
timer model.

> > +
> > +    MemoryRegion iomem;
> > +    ptimer_state *ptimer;
> > +
> > +    uint32_t control;
> > +    uint32_t relvalue;
> > +} 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