From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47994) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsJzp-0007yF-28 for qemu-devel@nongnu.org; Sun, 15 Dec 2013 17:17:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VsJzk-0001W3-HY for qemu-devel@nongnu.org; Sun, 15 Dec 2013 17:17:08 -0500 Received: from mail-la0-x233.google.com ([2a00:1450:4010:c03::233]:46467) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsJzk-0001U6-38 for qemu-devel@nongnu.org; Sun, 15 Dec 2013 17:17:04 -0500 Received: by mail-la0-f51.google.com with SMTP id ec20so2358647lab.38 for ; Sun, 15 Dec 2013 14:17:03 -0800 (PST) Date: Mon, 16 Dec 2013 02:24:35 +0400 From: Antony Pavlov Message-Id: <20131216022435.96256d99d6e9e325da9629dd@gmail.com> In-Reply-To: References: <1386970932-4886-1-git-send-email-antonynpavlov@gmail.com> <1386970932-4886-4-git-send-email-antonynpavlov@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v8 3/6] hw/arm/digic: add timer support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" On Sat, 14 Dec 2013 16:41:44 +1000 Peter Crosthwaite wrote: > On Sat, Dec 14, 2013 at 7:42 AM, Antony Pavlov = wrote: > > Signed-off-by: Antony Pavlov > > --- > > 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 =3D 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 =3D 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_DIGI= C_TIMER); > > + dev =3D 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]), NUL= L); > > + } > > } > > > > static void digic_realize(DeviceState *dev, Error **errp) > > { > > DigicState *s =3D DIGIC(dev); > > Error *err =3D NULL; > > + SysBusDevice *sbd; > > + int i; > > > > object_property_set_bool(OBJECT(&s->cpu), true, "reset-hivecs", &e= rr); > > if (err !=3D NULL) { > > @@ -46,6 +63,17 @@ static void digic_realize(DeviceState *dev, Error **= errp) > > error_propagate(errp, err); > > return; > > } > > + > > + for (i =3D 0; i < DIGIC4_NB_TIMERS; i++) { > > + object_property_set_bool(OBJECT(&s->timer[i]), true, "realized= ", &err); > > + if (err !=3D NULL) { > > + error_propagate(errp, err); > > + return; > > + } > > + > > + sbd =3D 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) +=3D omap_synctimer.o > > obj-$(CONFIG_PXA2XX) +=3D pxa2xx_timer.o > > obj-$(CONFIG_SH4) +=3D sh_timer.o > > obj-$(CONFIG_TUSB6010) +=3D tusb6010.o > > +obj-$(CONFIG_DIGIC) +=3D digic-timer.o > > > > obj-$(CONFIG_MC146818RTC) +=3D 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 > > + * > > + * 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 > > + >=20 > 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. >=20 > > +static const VMStateDescription vmstate_digic_timer =3D { > > + .name =3D "digic.timer", > > + .version_id =3D 1, > > + .minimum_version_id =3D 1, > > + .minimum_version_id_old =3D 1, > > + .fields =3D (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 =3D DIGIC_TIMER(dev); > > + > > + ptimer_stop(s->ptimer); > > + s->control =3D 0; > > + s->relvalue =3D 0; > > +} > > + > > +static uint64_t digic_timer_read(void *opaque, hwaddr offset, unsigned= size) > > +{ > > + DigicTimerState *s =3D opaque; > > + uint64_t ret =3D 0; > > + > > + switch (offset) { > > + case DIGIC_TIMER_CONTROL: > > + ret =3D s->control; > > + break; > > + case DIGIC_TIMER_RELVALUE: > > + ret =3D s->relvalue; > > + break; > > + case DIGIC_TIMER_VALUE: > > + ret =3D 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 =3D 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); > > + } >=20 > 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, >=20 > } else { > ptimer_stop(s->ptimer); > } >=20 > 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. >=20 > > + > > + s->control =3D (uint32_t)value; > > + break; > > + > > + case DIGIC_TIMER_RELVALUE: > > + s->relvalue =3D (uint32_t)(value & 0xffff); >=20 > extract32 is preferable over & >> logic. >=20 > > + 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 =3D { > > + .read =3D digic_timer_read, > > + .write =3D digic_timer_write, > > + .impl =3D { > > + .min_access_size =3D 4, > > + .max_access_size =3D 4, > > + }, > > + .endianness =3D DEVICE_NATIVE_ENDIAN, > > +}; > > + > > +static void digic_timer_init(Object *obj) > > +{ > > + DigicTimerState *s =3D DIGIC_TIMER(obj); > > + > > + s->ptimer =3D 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_dat= a) > > +{ > > + DeviceClass *dc =3D DEVICE_CLASS(klass); >=20 > Add a blank line here. >=20 > > + dc->reset =3D digic_timer_reset; > > + dc->vmsd =3D &vmstate_digic_timer; > > +} > > + > > +static const TypeInfo digic_timer_info =3D { > > + .name =3D TYPE_DIGIC_TIMER, > > + .parent =3D TYPE_SYS_BUS_DEVICE, > > + .instance_size =3D sizeof(DigicTimerState), > > + .instance_init =3D digic_timer_init, > > + .class_init =3D 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 > > + * > > + * 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_DIG= IC_TIMER) > > + > > +typedef struct DigicTimerState { >=20 > /*< private >*/ >=20 > > + SysBusDevice parent_obj; >=20 > /*< public >*/ >=20 > This is all trivial stuff. So if you make the changes (with the > stop-on-no-en being optional), >=20 > Reviewed-by: Peter Crosthwaite I fixed all trivial stuff but the stop-on-no-en and reposted the v9 patchse= ries. 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 DIG= IC4-based camera with UART is mothballed. 2. this patchseries is intended only for initial DIGIC support; the only so= ftware for this initial support is barebox bootloader that run correctly on real hardw= are. After adding initial DIGIC support I'm planning to make advanched DIGIC sup= port for Magic Lantern and CHDK. So I can carry over realization of more accurat= e 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 > > > > --=20 --=A0 Best regards, =A0 Antony Pavlov