diff mbox

[RFC,v3,3/5] hw/arm/digic: add timer support

Message ID 1378272100-26672-4-git-send-email-antonynpavlov@gmail.com
State New
Headers show

Commit Message

Antony Pavlov Sept. 4, 2013, 5:21 a.m. UTC
Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 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

Comments

Peter Crosthwaite Sept. 4, 2013, 6:18 a.m. UTC | #1
On Wed, Sep 4, 2013 at 3:21 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  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];
> +

Bit of a trap if theres every more than 10 timers as the name string
will run off the end if %d is below with 10. Its ok to round up a
little just for defensiveness.

> +        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);
> +        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 <antonynpavlov@gmail.com>
> + *
> + * 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

I think we were encouraging unconditional compilation of error printfs
rather than stripping them. This means bugs in maybe change patterns
involving types which affect debug printfs can be caught in developer
compile testing.

Something like this would work, although Andreas played with this
recently and may have more convincing ideas.

#ifndef XILINX_SPIPS_ERR_DEBUG
#define XILINX_SPIPS_ERR_DEBUG 0
#endif

#define DB_PRINT_L(level, ...) do { \
    if (XILINX_SPIPS_ERR_DEBUG > (level)) { \
        fprintf(stderr,  ": %s: ", __func__); \
        fprintf(stderr, ## __VA_ARGS__); \
    } \
} while (0);

> +
> +# 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);

Guest error or Unimp?

> +    }
> +
> +    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);
> +}
> +
> +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

"let" (no "'s"). s/on/at

> +     * */

Extra * unneeded.

> +    ptimer_set_freq(s->ptimer, 1 * 1000 * 1000);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &digic_timer_ops, s,
> +            TYPE_DIGIC_TIMER, 0x100);

Line continued function patameters should line up just past the
opening ( on the next line:

function(foo,
         bar

> +    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,
> +};
> +
> +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)

(n) in its usage to guard against order of operations confusion incase
macro is ever used with an arithmetic expression.

> +
>  #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
>
>
Antony Pavlov Sept. 5, 2013, 5:57 a.m. UTC | #2
On Wed, 4 Sep 2013 16:18:37 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Wed, Sep 4, 2013 at 3:21 PM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > ---
> >  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];
> > +
> 
> Bit of a trap if theres every more than 10 timers as the name string
> will run off the end if %d is below with 10. Its ok to round up a
> little just for defensiveness.
> 
> > +        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);
> > +        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 <antonynpavlov@gmail.com>
> > + *
> > + * 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
> 
> I think we were encouraging unconditional compilation of error printfs
> rather than stripping them. This means bugs in maybe change patterns
> involving types which affect debug printfs can be caught in developer
> compile testing.

  Yes, barebox project have same the same tendency. 

> Something like this would work, although Andreas played with this
> recently and may have more convincing ideas.
> 
> #ifndef XILINX_SPIPS_ERR_DEBUG
> #define XILINX_SPIPS_ERR_DEBUG 0
> #endif
> 
> #define DB_PRINT_L(level, ...) do { \
>     if (XILINX_SPIPS_ERR_DEBUG > (level)) { \
>         fprintf(stderr,  ": %s: ", __func__); \
>         fprintf(stderr, ## __VA_ARGS__); \
>     } \
> } while (0);

  Do we really need a preprocessor macro here?
  IMHO we can use a static inline function.

> > +
> > +# 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);
> 
> Guest error or Unimp?
> 
> > +    }
> > +
> > +    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);
> > +}
> > +
> > +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
> 
> "let" (no "'s"). s/on/at
> 
> > +     * */
> 
> Extra * unneeded.
> 
> > +    ptimer_set_freq(s->ptimer, 1 * 1000 * 1000);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &digic_timer_ops, s,
> > +            TYPE_DIGIC_TIMER, 0x100);
> 
> Line continued function patameters should line up just past the
> opening ( on the next line:
> 
> function(foo,
>          bar
> 
> > +    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,
> > +};
> > +
> > +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)
> 
> (n) in its usage to guard against order of operations confusion incase
> macro is ever used with an arithmetic expression.
> 
> > +
> >  #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
> >
> >
Peter Maydell Sept. 5, 2013, 6:05 p.m. UTC | #3
On 4 September 2013 06:21, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  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 <antonynpavlov@gmail.com>
> + *
> + * 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
diff mbox

Patch

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);
+        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 <antonynpavlov@gmail.com>
+ *
+ * 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);
+}
+
+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
+     * */
+    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,
+};
+
+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)
+
 #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__ */