From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53896) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmIxD-0008Dz-03 for qemu-devel@nongnu.org; Fri, 29 Nov 2013 02:57:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VmIx7-0007lA-Rr for qemu-devel@nongnu.org; Fri, 29 Nov 2013 02:57:34 -0500 Received: from [222.73.24.84] (port=21050 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmIx6-0007i0-K5 for qemu-devel@nongnu.org; Fri, 29 Nov 2013 02:57:29 -0500 Message-ID: <52984884.4090200@cn.fujitsu.com> Date: Fri, 29 Nov 2013 15:55:48 +0800 From: Li Guang MIME-Version: 1.0 References: <1385540994-27940-1-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] [fixed-up][PATCH v6 2/5] hw/timer: add sunxi timer device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" Peter Crosthwaite wrote: > On Wed, Nov 27, 2013 at 11:35 PM, Peter Crosthwaite > wrote: > >> On Wed, Nov 27, 2013 at 6:29 PM, liguang wrote: >> >>> Signed-off-by: liguang >>> --- >>> default-configs/arm-softmmu.mak | 2 + >>> hw/timer/Makefile.objs | 1 + >>> hw/timer/sunxi-pit.c | 254 +++++++++++++++++++++++++++++++++++++++ >>> include/hw/timer/sunxi-pit.h | 56 +++++++++ >>> 4 files changed, 313 insertions(+), 0 deletions(-) >>> create mode 100644 hw/timer/sunxi-pit.c >>> create mode 100644 include/hw/timer/sunxi-pit.h >>> >>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >>> index a555eef..7bf5ad0 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_SUNXI_PIT=y >>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs >>> index eca5905..f7888e9 100644 >>> --- a/hw/timer/Makefile.objs >>> +++ b/hw/timer/Makefile.objs >>> @@ -27,3 +27,4 @@ obj-$(CONFIG_SH4) += sh_timer.o >>> obj-$(CONFIG_TUSB6010) += tusb6010.o >>> >>> obj-$(CONFIG_MC146818RTC) += mc146818rtc.o >>> +obj-$(CONFIG_SUNXI_PIT) += sunxi-pit.o >>> diff --git a/hw/timer/sunxi-pit.c b/hw/timer/sunxi-pit.c >>> new file mode 100644 >>> index 0000000..19bc16c >>> --- /dev/null >>> +++ b/hw/timer/sunxi-pit.c >>> @@ -0,0 +1,254 @@ >>> +/* >>> + * Allwinner sunxi timer device emulation >>> >> May need to do global find/replace on sunxi, depending on outcoming of >> the naming discussion. >> >> >>> + * >>> + * 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 "hw/ptimer.h" >>> +#include "sysemu/sysemu.h" >>> +#include "hw/timer/sunxi-pit.h" >>> + >>> + >>> +static uint64_t sunxi_pit_read(void *opaque, hwaddr offset, unsigned size) >>> +{ >>> + SunxiPITState *s = SUNXI_PIT(opaque); >>> + uint8_t index; >>> + >>> + switch (offset) { >>> + case SUNXI_TIMER_IRQ_EN: >>> + return s->irq_enable; >>> + case SUNXI_TIMER_IRQ_ST: >>> + return s->irq_status; >>> + case SUNXI_TIMER_BASE ... SUNXI_TIMER_BASE * 6 + SUNXI_TIMER_COUNT: >>> + index = offset& 0xf0; >>> + index>>= 4; >>> + index -= 1; >>> + switch (offset& 0x0f) { >>> + case SUNXI_TIMER_CONTROL: >>> + return s->control[index]; >>> + case SUNXI_TIMER_INTERVAL: >>> + return s->interval[index]; >>> + case SUNXI_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 SUNXI_WDOG_CONTROL: >>> + break; >>> + case SUNXI_WDOG_MODE: >>> + break; >>> + case SUNXI_COUNT_LO: >>> + return s->count_lo; >>> + break; >>> >> break after return. >> >> git diff | grep "return" -A 1 | grep "break" >> >> It's crude, but will find any you missed. >> >> >>> + case SUNXI_COUNT_HI: >>> + return s->count_hi; >>> + break; >>> + case SUNXI_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 sunxi_pit_write(void *opaque, hwaddr offset, uint64_t value, >>> + unsigned size) >>> +{ >>> + SunxiPITState *s = SUNXI_PIT(opaque); >>> + uint8_t index; >>> + >>> + switch (offset) { >>> + case SUNXI_TIMER_IRQ_EN: >>> + s->irq_enable = value; >>> + break; >>> + case SUNXI_TIMER_IRQ_ST: >>> + s->irq_status&= ~value; >>> + break; >>> + case SUNXI_TIMER_BASE ... SUNXI_TIMER_BASE * 6 + SUNXI_TIMER_COUNT: >>> + index = offset& 0xf0; >>> + index>>= 4; >>> + index -= 1; >>> + switch (offset& 0x0f) { >>> + case SUNXI_TIMER_CONTROL: >>> + s->control[index] = value; >>> + if (s->control[index]& SUNXI_TIMER_RELOAD) { >>> + ptimer_set_count(s->timer[index], s->interval[index]); >>> + } >>> + if (s->control[index]& SUNXI_TIMER_EN) { >>> + ptimer_run(s->timer[index], 1); >>> + } else { >>> + ptimer_stop(s->timer[index]); >>> + } >>> + break; >>> + case SUNXI_TIMER_INTERVAL: >>> + s->interval[index] = value; >>> + ptimer_set_count(s->timer[index], s->interval[index]); >>> + break; >>> + case SUNXI_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 SUNXI_WDOG_CONTROL: >>> + s->watch_dog_control = value; >>> + break; >>> + case SUNXI_WDOG_MODE: >>> + s->watch_dog_mode = value; >>> + break; >>> + case SUNXI_COUNT_LO: >>> + s->count_lo = value; >>> + break; >>> + case SUNXI_COUNT_HI: >>> + s->count_hi = value; >>> + break; >>> + case SUNXI_COUNT_CTL: >>> + s->count_ctl = value; >>> + if (s->count_ctl& SUNXI_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&= ~SUNXI_COUNT_RL_EN; >>> + } >>> + if (s->count_ctl& SUNXI_COUNT_CLR_EN) { >>> + s->count_lo =0; >>> + s->count_hi =0; >>> + s->count_ctl&= ~SUNXI_COUNT_CLR_EN; >>> + } >>> + break; >>> + default: >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >>> + break; >>> + } >>> +} >>> + >>> +static const MemoryRegionOps sunxi_pit_ops = { >>> + .read = sunxi_pit_read, >>> + .write = sunxi_pit_write, >>> + .endianness = DEVICE_NATIVE_ENDIAN, >>> +}; >>> + >>> +static const VMStateDescription vmstate_sunxi_pit = { >>> + .name = "sunxi.pit", >>> + .version_id = 1, >>> + .minimum_version_id = 1, >>> + .minimum_version_id_old = 1, >>> + .fields = (VMStateField[]) { >>> + VMSTATE_UINT32(irq_enable, SunxiPITState), >>> + VMSTATE_UINT32(irq_status, SunxiPITState), >>> + VMSTATE_UINT32_ARRAY(control, SunxiPITState, SUNXI_TIMER_NR), >>> + VMSTATE_UINT32_ARRAY(interval, SunxiPITState, SUNXI_TIMER_NR), >>> + VMSTATE_UINT32_ARRAY(count, SunxiPITState, SUNXI_TIMER_NR), >>> + VMSTATE_UINT32(watch_dog_mode, SunxiPITState), >>> + VMSTATE_UINT32(watch_dog_control, SunxiPITState), >>> + VMSTATE_UINT32(count_lo, SunxiPITState), >>> + VMSTATE_UINT32(count_hi, SunxiPITState), >>> + VMSTATE_UINT32(count_ctl, SunxiPITState), >>> + VMSTATE_PTIMER_ARRAY(timer, SunxiPITState, SUNXI_TIMER_NR), >>> + VMSTATE_END_OF_LIST() >>> + } >>> +}; >>> + >>> +static void sunxi_pit_reset(DeviceState *dev) >>> +{ >>> + SunxiPITState *s = SUNXI_PIT(dev); >>> + uint8_t i; >>> + >>> + s->irq_enable = 0; >>> + s->irq_status = 0; >>> + for (i = 0; i< 6; i++) { >>> + s->control[i] = SUNXI_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 sunxi_pit_timer_cb(void *opaque) >>> +{ >>> + SunxiPITState *s = SUNXI_PIT(opaque); >>> + uint8_t i; >>> + >>> + for (i = 0; i< SUNXI_TIMER_NR; i++) { >>> + if (s->control[i]& SUNXI_TIMER_EN&& >>> + ptimer_get_count(s->timer[i]) == 0) { >>> + s->irq_status |= 1<< i; >>> + if (!(s->control[i]& SUNXI_TIMER_MODE)) { >>> + ptimer_set_count(s->timer[i], s->interval[i]); >>> + ptimer_run(s->timer[i], 1); >>> > So looking at this more closely, I see you are chaining one-shot > ptimers. My understanding is that you shouldn't do this, as it leads > to small slips in time, that over many hits integrate to a noticable > slippage. You should refactor the intial ptimer_run to not one-shot > when !SUNXI_TIMER_MODE. > > OK >>> + } >>> + qemu_irq_raise(s->irq[i]); >>> >> I see that it's an edge triggered interrupt - makes a lot more sense >> now. I think there is qemu_irq_pulse for this very purpose. But I also >> think you are susceptible to spurious edges here however. If timer A >> hits (and doesn't reload), and then timer B hits, I don't see what >> stops this logic being reached for timer A again? >> >> > This issue is orthogonal and still at large even with one-shot fix. > > > sorry, currently can't get it? can you give more details? Thanks! Li Guang >> Regards, >> Peter >> >> >>> + } >>> + qemu_irq_lower(s->irq[i]); >>> + } >>> +} >>> + >>> +static void sunxi_pit_realize(DeviceState *dev, Error **errp) >>> +{ >>> + SunxiPITState *s = SUNXI_PIT(dev); >>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> + QEMUBH *bh[SUNXI_TIMER_NR]; >>> + uint8_t i; >>> + >>> + for (i = 0; i< SUNXI_TIMER_NR; i++) { >>> + sysbus_init_irq(sbd,&s->irq[i]); >>> + } >>> + memory_region_init_io(&s->iomem, OBJECT(s),&sunxi_pit_ops, s, >>> + TYPE_SUNXI_PIT, 0x400); >>> + sysbus_init_mmio(sbd,&s->iomem); >>> + >>> + for (i = 0; i< SUNXI_TIMER_NR; i++) { >>> + bh[i] = qemu_bh_new(sunxi_pit_timer_cb, s); >>> + s->timer[i] = ptimer_init(bh[i]); >>> + ptimer_set_freq(s->timer[i], 240000); >>> + } >>> +} >>> + >>> +static void sunxi_pit_class_init(ObjectClass *klass, void *data) >>> +{ >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> + >>> + dc->realize = sunxi_pit_realize; >>> + dc->reset = sunxi_pit_reset; >>> + dc->desc = "sunxi timer"; >>> + dc->vmsd =&vmstate_sunxi_pit; >>> +} >>> + >>> +static const TypeInfo sunxi_pit_info = { >>> + .name = TYPE_SUNXI_PIT, >>> + .parent = TYPE_SYS_BUS_DEVICE, >>> + .instance_size = sizeof(SunxiPITState), >>> + .class_init = sunxi_pit_class_init, >>> +}; >>> + >>> +static void sunxi_register_types(void) >>> +{ >>> + type_register_static(&sunxi_pit_info); >>> +} >>> + >>> +type_init(sunxi_register_types); >>> diff --git a/include/hw/timer/sunxi-pit.h b/include/hw/timer/sunxi-pit.h >>> new file mode 100644 >>> index 0000000..7c84946 >>> --- /dev/null >>> +++ b/include/hw/timer/sunxi-pit.h >>> @@ -0,0 +1,56 @@ >>> +#ifndef SUNXI_PIT_H >>> +#define SUNXI_PIT_H >>> + >>> + >>> +#define TYPE_SUNXI_PIT "sunxi-timer" >>> +#define SUNXI_PIT(obj) OBJECT_CHECK(SunxiPITState, (obj), TYPE_SUNXI_PIT) >>> + >>> +#define SUNXI_TIMER_NR 6 >>> +#define SUNXI_TIMER_IRQ 0x1 >>> +#define SUNXI_WDOG_IRQ 0x100 >>> + >>> +#define SUNXI_TIMER_IRQ_EN 0 >>> +#define SUNXI_TIMER_IRQ_ST 0x4 >>> + >>> +#define SUNXI_TIMER_CONTROL 0x0 >>> +#define SUNXI_TIMER_EN 0x1 >>> +#define SUNXI_TIMER_RELOAD 0x2 >>> +#define SUNXI_TIMER_MODE 0x80 >>> + >>> +#define SUNXI_TIMER_INTERVAL 0x4 >>> +#define SUNXI_TIMER_COUNT 0x8 >>> +#define SUNXI_WDOG_CONTROL 0x90 >>> +#define SUNXI_WDOG_MODE 0x94 >>> + >>> +#define SUNXI_COUNT_CTL 0xa0 >>> +#define SUNXI_COUNT_RL_EN 0x2 >>> +#define SUNXI_COUNT_CLR_EN 0x1 >>> +#define SUNXI_COUNT_LO 0xa4 >>> +#define SUNXI_COUNT_HI 0xa8 >>> + >>> +#define SUNXI_TIMER_BASE 0x10 >>> + >>> +#define SUNXI_DEFAULT_CLOCK 0x4 >>> + >>> +typedef struct SunxiPITState { >>> + /*< private>*/ >>> + SysBusDevice parent_obj; >>> + /*< public>*/ >>> + qemu_irq irq[SUNXI_TIMER_NR]; >>> + ptimer_state *timer[SUNXI_TIMER_NR]; >>> + MemoryRegion iomem; >>> + >>> + uint32_t irq_enable; >>> + uint32_t irq_status; >>> + uint32_t control[SUNXI_TIMER_NR]; >>> + uint32_t interval[SUNXI_TIMER_NR]; >>> + uint32_t count[SUNXI_TIMER_NR]; >>> + uint32_t watch_dog_mode; >>> + uint32_t watch_dog_control; >>> + uint32_t count_lo; >>> + uint32_t count_hi; >>> + uint32_t count_ctl; >>> +} SunxiPITState; >>> + >>> +#endif >>> + >>> -- >>> 1.7.2.5 >>> >>> >>> > >