From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo1yM-0006Iz-N4 for qemu-devel@nongnu.org; Tue, 03 Dec 2013 21:13:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo1yH-0001XT-SB for qemu-devel@nongnu.org; Tue, 03 Dec 2013 21:13:54 -0500 Received: from [222.73.24.84] (port=32022 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo1yG-0001XE-Qn for qemu-devel@nongnu.org; Tue, 03 Dec 2013 21:13:49 -0500 Message-ID: <529E8F7C.50700@cn.fujitsu.com> Date: Wed, 04 Dec 2013 10:12:12 +0800 From: Li Guang MIME-Version: 1.0 References: <1386061881-12720-1-git-send-email-lig.fnst@cn.fujitsu.com> <1386061881-12720-3-git-send-email-lig.fnst@cn.fujitsu.com> In-Reply-To: Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=ISO-8859-1; format=flowed Subject: Re: [Qemu-devel] [PATCH 2/5] hw/timer: add allwinner a10 timer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" , =?ISO-8859-1?Q?Andreas_F=E4rber?= Peter Crosthwaite wrote: > On Tue, Dec 3, 2013 at 7:11 PM, liguang wrote: > >> Signed-off-by: liguang >> --- >> default-configs/arm-softmmu.mak | 2 + >> hw/timer/Makefile.objs | 2 + >> hw/timer/allwinner-a10_pit.c | 253 ++++++++++++++++++++++++++++++++++ >> > Mix of _ and - in filename is ugly. I think all - in filename is best. > > OK , will fix, though I think '_' and '-' have different meanings here. '_' is for separation, '-' is for concatenation. >> include/hw/timer/allwinner-a10_pit.h | 57 ++++++++ >> 4 files changed, 314 insertions(+), 0 deletions(-) >> create mode 100644 hw/timer/allwinner-a10_pit.c >> create mode 100644 include/hw/timer/allwinner-a10_pit.h >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >> index a555eef..0029596 100644 >> --- a/default-configs/arm-softmmu.mak >> +++ b/default-configs/arm-softmmu.mak >> @@ -81,3 +81,5 @@ CONFIG_VERSATILE_I2C=y >> >> CONFIG_SDHCI=y >> CONFIG_INTEGRATOR_DEBUG=y >> + >> +CONFIG_ALLWINNER_A10=y >> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs >> index eca5905..4ff2e1f 100644 >> --- a/hw/timer/Makefile.objs >> +++ b/hw/timer/Makefile.objs >> @@ -27,3 +27,5 @@ obj-$(CONFIG_SH4) += sh_timer.o >> obj-$(CONFIG_TUSB6010) += tusb6010.o >> >> obj-$(CONFIG_MC146818RTC) += mc146818rtc.o >> + >> +obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10_pit.o >> diff --git a/hw/timer/allwinner-a10_pit.c b/hw/timer/allwinner-a10_pit.c >> new file mode 100644 >> index 0000000..2f9b458 >> --- /dev/null >> +++ b/hw/timer/allwinner-a10_pit.c >> @@ -0,0 +1,253 @@ >> +/* >> + * Allwinner A10 timer device emulation >> + * >> + * Copyright (C) 2013 Li Guang >> + * Written by Li Guang >> + * >> + * 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 "sysemu/sysemu.h" >> +#include "hw/timer/allwinner-a10_pit.h" >> + >> + >> +static uint64_t a10_pit_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + A10PITState *s = A10_PIT(opaque); >> + uint8_t index; >> + >> + switch (offset) { >> + case A10PIT_TIMER_IRQ_EN: >> > Its odd to have no _ between A10 and PIT. Does that match your documentation? > > hmm, OK, will fix >> + return s->irq_enable; >> + case A10PIT_TIMER_IRQ_ST: >> + return s->irq_status; >> + case A10PIT_TIMER_BASE ... A10PIT_TIMER_BASE * 6 + A10PIT_TIMER_COUNT: >> + index = offset& 0xf0; >> + index>>= 4; >> + index -= 1; >> + switch (offset& 0x0f) { >> + case A10PIT_TIMER_CONTROL: >> + return s->control[index]; >> + case A10PIT_TIMER_INTERVAL: >> + return s->interval[index]; >> + case A10PIT_TIMER_COUNT: >> + s->count[index] = ptimer_get_count(s->timer[index]); >> + return s->count[index]; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >> + break; >> + } >> + case A10PIT_WDOG_CONTROL: >> + break; >> + case A10PIT_WDOG_MODE: >> + break; >> + case A10PIT_COUNT_LO: >> + return s->count_lo; >> + case A10PIT_COUNT_HI: >> + return s->count_hi; >> + case A10PIT_COUNT_CTL: >> + return s->count_ctl; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >> + break; >> + } >> + >> + return 0; >> +} >> + >> +static void a10_pit_write(void *opaque, hwaddr offset, uint64_t value, >> + unsigned size) >> +{ >> + A10PITState *s = A10_PIT(opaque); >> + uint8_t index; >> + >> + switch (offset) { >> + case A10PIT_TIMER_IRQ_EN: >> + s->irq_enable = value; >> + break; >> + case A10PIT_TIMER_IRQ_ST: >> + s->irq_status&= ~value; >> + break; >> + case A10PIT_TIMER_BASE ... A10PIT_TIMER_BASE * 6 + A10PIT_TIMER_COUNT: >> + index = offset& 0xf0; >> + index>>= 4; >> + index -= 1; >> + switch (offset& 0x0f) { >> + case A10PIT_TIMER_CONTROL: >> + s->control[index] = value; >> + if (s->control[index]& A10PIT_TIMER_RELOAD) { >> + ptimer_set_count(s->timer[index], s->interval[index]); >> + } >> > Why? Doesn't the count just always stay up to date when writing TIMER_INTERVAL? > > just do as datasheet says, "Reload timer Interval value" >> + if (s->control[index]& A10PIT_TIMER_EN) { >> + ptimer_run(s->timer[index], 1); >> + } else { >> + ptimer_stop(s->timer[index]); >> + } >> + break; >> + case A10PIT_TIMER_INTERVAL: >> + s->interval[index] = value; >> + ptimer_set_count(s->timer[index], s->interval[index]); >> + break; >> + case A10PIT_TIMER_COUNT: >> + s->count[index] = value; >> + break; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >> + } >> + break; >> + case A10PIT_WDOG_CONTROL: >> + s->watch_dog_control = value; >> + break; >> + case A10PIT_WDOG_MODE: >> + s->watch_dog_mode = value; >> + break; >> + case A10PIT_COUNT_LO: >> + s->count_lo = value; >> + break; >> + case A10PIT_COUNT_HI: >> + s->count_hi = value; >> + break; >> + case A10PIT_COUNT_CTL: >> + s->count_ctl = value; >> + if (s->count_ctl& A10PIT_COUNT_RL_EN) { >> + s->count_lo = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> + s->count_hi = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)>> 32; >> + s->count_ctl&= ~A10PIT_COUNT_RL_EN; >> + } >> + if (s->count_ctl& A10PIT_COUNT_CLR_EN) { >> + s->count_lo = 0; >> + s->count_hi = 0; >> + s->count_ctl&= ~A10PIT_COUNT_CLR_EN; >> + } >> + break; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >> + break; >> + } >> +} >> + >> +static const MemoryRegionOps a10_pit_ops = { >> + .read = a10_pit_read, >> + .write = a10_pit_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> +}; >> + >> +static const VMStateDescription vmstate_a10_pit = { >> + .name = "a10.pit", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .minimum_version_id_old = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT32(irq_enable, A10PITState), >> + VMSTATE_UINT32(irq_status, A10PITState), >> + VMSTATE_UINT32_ARRAY(control, A10PITState, A10PIT_TIMER_NR), >> + VMSTATE_UINT32_ARRAY(interval, A10PITState, A10PIT_TIMER_NR), >> + VMSTATE_UINT32_ARRAY(count, A10PITState, A10PIT_TIMER_NR), >> + VMSTATE_UINT32(watch_dog_mode, A10PITState), >> + VMSTATE_UINT32(watch_dog_control, A10PITState), >> + VMSTATE_UINT32(count_lo, A10PITState), >> + VMSTATE_UINT32(count_hi, A10PITState), >> + VMSTATE_UINT32(count_ctl, A10PITState), >> + VMSTATE_PTIMER_ARRAY(timer, A10PITState, A10PIT_TIMER_NR), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static void a10_pit_reset(DeviceState *dev) >> +{ >> + A10PITState *s = A10_PIT(dev); >> + uint8_t i; >> + >> + s->irq_enable = 0; >> + s->irq_status = 0; >> + for (i = 0; i< 6; i++) { >> + s->control[i] = A10PIT_DEFAULT_CLOCK; >> + s->interval[i] = 0; >> + s->count[i] = 0; >> + ptimer_stop(s->timer[i]); >> + } >> + s->watch_dog_mode = 0; >> + s->watch_dog_control = 0; >> + s->count_lo = 0; >> + s->count_hi = 0; >> + s->count_ctl = 0; >> +} >> + >> +static void a10_pit_timer_cb(void *opaque) >> +{ >> + A10PITState *s = A10_PIT(opaque); >> + uint8_t i; >> + >> + for (i = 0; i< A10PIT_TIMER_NR; i++) { >> + if (s->control[i]& A10PIT_TIMER_EN&& >> + ptimer_get_count(s->timer[i]) == 0) { >> + s->irq_status |= 1<< i; >> + if (!(s->control[i]& A10PIT_TIMER_MODE)) { >> + ptimer_set_count(s->timer[i], s->interval[i]); >> + ptimer_run(s->timer[i], 1); >> > You are still chaining oneshot timers together here to form a periodic > timer. Just run the timer as periodic in the first place when > !A10PIT_TIMER_MODE. > > OK, will use continuous timer. >> + s->control[i] |= A10PIT_TIMER_EN; >> > Dead assignment. TIMER_EN is already set. > > >> + } else { >> + s->control[i]&= ~A10PIT_TIMER_EN; >> > So the spurious interrupts are gone now, which is good news > > >> + } >> + qemu_irq_pulse(s->irq[i]); >> + } >> + } >> +} >> + >> +static void a10_pit_realize(DeviceState *dev, Error **errp) >> +{ >> + A10PITState *s = A10_PIT(dev); >> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> + QEMUBH * bh[A10PIT_TIMER_NR]; >> + uint8_t i; >> + >> + for (i = 0; i< A10PIT_TIMER_NR; i++) { >> + sysbus_init_irq(sbd,&s->irq[i]); >> + } >> + memory_region_init_io(&s->iomem, OBJECT(s),&a10_pit_ops, s, >> + TYPE_A10_PIT, 0x400); >> + sysbus_init_mmio(sbd,&s->iomem); >> + >> + for (i = 0; i< A10PIT_TIMER_NR; i++) { >> + bh[i] = qemu_bh_new(a10_pit_timer_cb, s); >> + s->timer[i] = ptimer_init(bh[i]); >> + ptimer_set_freq(s->timer[i], 240000); >> + } >> +} >> + >> +static void a10_pit_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->realize = a10_pit_realize; >> + dc->reset = a10_pit_reset; >> + dc->desc = "allwinner a10 timer"; >> + dc->vmsd =&vmstate_a10_pit; >> +} >> + >> +static const TypeInfo a10_pit_info = { >> + .name = TYPE_A10_PIT, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(A10PITState), >> + .class_init = a10_pit_class_init, >> +}; >> + >> +static void a10_register_types(void) >> +{ >> + type_register_static(&a10_pit_info); >> +} >> + >> +type_init(a10_register_types); >> diff --git a/include/hw/timer/allwinner-a10_pit.h b/include/hw/timer/allwinner-a10_pit.h >> new file mode 100644 >> index 0000000..e061cc2 >> --- /dev/null >> +++ b/include/hw/timer/allwinner-a10_pit.h >> @@ -0,0 +1,57 @@ >> +#ifndef A10_PIT_H >> +#define A10_PIT_H >> + >> +#include "hw/ptimer.h" >> + >> +#define TYPE_A10_PIT "a10-timer" >> +#define A10_PIT(obj) OBJECT_CHECK(A10PITState, (obj), TYPE_A10_PIT) >> + >> +#define A10PIT_TIMER_NR 6 >> +#define A10PIT_TIMER_IRQ 0x1 >> +#define A10PIT_WDOG_IRQ 0x100 >> + >> > So "A10" as the macro prefix is a little vague. We could easily have > confusion with ARMs own usage of Axx. Is there a contraction of > "allwinner" we could prefix in there? > If you plan to use this timer in the future for devices other than > A10, you could just drop the "A10" labelling here now. That suggestion > to use the specific SoC name is more for the SoC container object, not > the peripheral devices. > > I prefer A10 prefix, for we can't assure all allwinner chips implement the same timer like A10. and any chips implemented this timer can realize this timer. Thanks! Li Guang >> +#define A10PIT_TIMER_IRQ_EN 0 >> +#define A10PIT_TIMER_IRQ_ST 0x4 >> + >> +#define A10PIT_TIMER_CONTROL 0x0 >> +#define A10PIT_TIMER_EN 0x1 >> +#define A10PIT_TIMER_RELOAD 0x2 >> +#define A10PIT_TIMER_MODE 0x80 >> + >> +#define A10PIT_TIMER_INTERVAL 0x4 >> +#define A10PIT_TIMER_COUNT 0x8 >> +#define A10PIT_WDOG_CONTROL 0x90 >> +#define A10PIT_WDOG_MODE 0x94 >> + >> +#define A10PIT_COUNT_CTL 0xa0 >> +#define A10PIT_COUNT_RL_EN 0x2 >> +#define A10PIT_COUNT_CLR_EN 0x1 >> +#define A10PIT_COUNT_LO 0xa4 >> +#define A10PIT_COUNT_HI 0xa8 >> + >> +#define A10PIT_TIMER_BASE 0x10 >> + >> +#define A10PIT_DEFAULT_CLOCK 0x4 >> + >> +typedef struct A10PITState { >> + /*< private>*/ >> + SysBusDevice parent_obj; >> + /*< public>*/ >> + qemu_irq irq[A10PIT_TIMER_NR]; >> + ptimer_state *timer[A10PIT_TIMER_NR]; >> + MemoryRegion iomem; >> + >> + uint32_t irq_enable; >> + uint32_t irq_status; >> + uint32_t control[A10PIT_TIMER_NR]; >> + uint32_t interval[A10PIT_TIMER_NR]; >> + uint32_t count[A10PIT_TIMER_NR]; >> + uint32_t watch_dog_mode; >> + uint32_t watch_dog_control; >> + uint32_t count_lo; >> + uint32_t count_hi; >> + uint32_t count_ctl; >> +} A10PITState; >> + >> +#endif >> + >> -- >> 1.7.2.5 >> >> >> > >