Re: [Qemu-devel] [RFC v3 3/5] hw/arm/digic: add timer support

[Peter Maydell]

On 4 September 2013 06:21, Antony Pavlov <address@hidden> wrote:
> Signed-off-by: Antony Pavlov <address@hidden>
> ---
>  hw/arm/digic.c         |  25 ++++++++++
>  hw/timer/Makefile.objs |   1 +
>  hw/timer/digic-timer.c | 122 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/timer/digic-timer.h |  19 ++++++++
>  include/hw/arm/digic.h |   7 +++
>  5 files changed, 174 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 95a9fcd..a71364b 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -30,21 +30,46 @@
>  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++) {
> +        char name[9];
> +
> +        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, 9, "timer[%d]", i);

Just use g_strdup_printf() and g_free() it afterwards;
that avoids having to care about array sizes at all,
and this is runs-once code so performance isn't
critical.

> +        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, "realized", &err);
>      if (err != NULL) {
>          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 eca5905..5479aee 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -25,5 +25,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..c6cf7ee
> --- /dev/null
> +++ b/hw/timer/digic-timer.c
> @@ -0,0 +1,122 @@
> +/*
> + * 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 library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/ptimer.h"
> +#include "qemu/main-loop.h"
> +
> +#include "hw/timer/digic-timer.h"
> +
> +#ifdef DEBUG_DIGIC_TIMER
> +#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +# define DIGIC_TIMER_CONTROL 0x00
> +# define DIGIC_TIMER_VALUE 0x0c
> +
> +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 = ret & 0xffff;
> +        break;
> +    default:
> +        DPRINTF("Bad offset %x\n", (int)offset);
> +    }
> +
> +    DPRINTF("offset 0x%x, value 0x%x\n", offset, ret);
> +    return ret;
> +}
> +
> +static void digic_timer_write(void *opaque, hwaddr offset,
> +        uint64_t value, unsigned size)
> +{
> +    DigicTimerState *s = opaque;
> +
> +    /* FIXME: just now we ignore timer enable bit */
> +    ptimer_set_limit(s->ptimer, 0x0000ffff, 1);
> +    ptimer_run(s->ptimer, 1);
> +}
> +
> +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);

This isn't doing anything interesting. Surely it
should trigger an interrupt or something?

> +}
> +
> +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's it always run on 1 MHz
> +     * */

this line should be "*/", not "* */".

> +    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 const TypeInfo digic_timer_info = {
> +    .name = TYPE_DIGIC_TIMER,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(DigicTimerState),
> +    .instance_init = digic_timer_init,
> +};

You need a DeviceClass::reset function (which
among other things should reset the ptimer
you're using); this will require a class init
function so you have somewhere to set it up.

You also need a VMStateDescription so migration
works.

> +
> +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..6483516
> --- /dev/null
> +++ b/hw/timer/digic-timer.h
> @@ -0,0 +1,19 @@
> +#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 0ef4723..472d7d7 100644
> --- a/include/hw/arm/digic.h
> +++ b/include/hw/arm/digic.h
> @@ -10,6 +10,11 @@
>
>  #include "cpu-qom.h"
>
> +#include "hw/timer/digic-timer.h"
> +
> +#define DIGIC4_NB_TIMERS 3
> +#define DIGIC4_TIMER_BASE(n)    (0xc0210000 + n * 0x100)

Brackets round the (n) on right hand side of a macro
definition, please.
> +
>  #define TYPE_DIGIC "digic"
>
>  #define DIGIC(obj) OBJECT_CHECK(DigicState, (obj), TYPE_DIGIC)
> @@ -18,6 +23,8 @@ typedef struct DigicState {
>      Object parent_obj;
>
>      ARMCPU cpu;
> +
> +    DigicTimerState timer[DIGIC4_NB_TIMERS];
>  } DigicState;
>
>  #endif /* __DIGIC_H__ */
> --
> 1.8.4.rc3


thanks
-- PMM