[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v3 3/5] hw/arm/digic: add timer support
From: |
Antony Pavlov |
Subject: |
Re: [Qemu-devel] [RFC v3 3/5] hw/arm/digic: add timer support |
Date: |
Thu, 5 Sep 2013 09:57:23 +0400 |
On Wed, 4 Sep 2013 16:18:37 +1000
Peter Crosthwaite <address@hidden> wrote:
> On Wed, Sep 4, 2013 at 3:21 PM, 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];
> > +
>
> 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 <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
>
> 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
> >
> >
--
--
Best regards,
Antony Pavlov
- [Qemu-devel] [RFC v3 0/5] hw/arm: add initial support for Canon DIGIC SoC, Antony Pavlov, 2013/09/04
- [Qemu-devel] [RFC v3 1/5] hw/arm: add very initial support for Canon DIGIC SoC, Antony Pavlov, 2013/09/04
- [Qemu-devel] [RFC v3 3/5] hw/arm/digic: add timer support, Antony Pavlov, 2013/09/04
- [Qemu-devel] [RFC v3 4/5] hw/arm/digic: add UART support, Antony Pavlov, 2013/09/04
- [Qemu-devel] [RFC v3 2/5] hw/arm/digic: prepare DIGIC-based boards support, Antony Pavlov, 2013/09/04
- [Qemu-devel] [RFC v3 5/5] hw/arm/digic: add NOR ROM support, Antony Pavlov, 2013/09/04