From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlEpP-0000Ju-GQ for qemu-devel@nongnu.org; Tue, 26 Nov 2013 04:21:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VlEpK-0007ks-Pf for qemu-devel@nongnu.org; Tue, 26 Nov 2013 04:21:07 -0500 Received: from [222.73.24.84] (port=19634 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlEpJ-0007kL-F5 for qemu-devel@nongnu.org; Tue, 26 Nov 2013 04:21:02 -0500 Message-ID: <52946796.1080702@cn.fujitsu.com> Date: Tue, 26 Nov 2013 17:19:18 +0800 From: Li Guang MIME-Version: 1.0 References: <1385450531-3170-1-git-send-email-lig.fnst@cn.fujitsu.com> <1385450531-3170-2-git-send-email-lig.fnst@cn.fujitsu.com> <52946308.4080209@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 v4 1/4] 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 Tue, Nov 26, 2013 at 6:59 PM, Li Guang wrote: > >> Peter Crosthwaite wrote: >> >>> On Tue, Nov 26, 2013 at 5:22 PM, liguang wrote: >>> >>> >>>> Signed-off-by: liguang >>>> --- >>>> default-configs/arm-softmmu.mak | 2 + >>>> hw/timer/Makefile.objs | 1 + >>>> hw/timer/sunxi-pit.c | 295 >>>> +++++++++++++++++++++++++++++++++++++++ >>>> include/hw/timer/sunxi-pit.h | 37 +++++ >>>> 4 files changed, 335 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..39b84ab >>>> --- /dev/null >>>> +++ b/hw/timer/sunxi-pit.c >>>> @@ -0,0 +1,295 @@ >>>> +/* >>>> + * Allwinner sunxi 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 "hw/ptimer.h" >>>> +#include "sysemu/sysemu.h" >>>> +#include "hw/timer/sunxi-pit.h" >>>> + >>>> + >>>> +typedef struct SunxiTimer { >>>> + ptimer_state *timer; >>>> +} SunxiTimer; >>>> + >>>> >>>> >>> I don't understand the need for this struct. What was wrong with the >>> direct array of ptimers you had before? >>> >>> >> >> because I have to pack timer array into VMSD, >> and there's no VMSTATE_PTIMER_ARRAY for ptimer_state. >> >> > Anyway you can just use VMSTATE_STRUCT_ARRAY on ptimers own VMSD definition? > > If you cant, then I think you make a reasonable case for moving the > relevant bits and pieces to headers so they are visible. That or > implement VMSTATE_PTIMER_ARRAY. > > right, but that seems be in a separated patch, I consider to use current way, can I? >> >>> >>>> +typedef struct SunxiPITState { >>>> + /*< private>*/ >>>> + SysBusDevice parent_obj; >>>> + /*< public>*/ >>>> + qemu_irq irq[SUNXI_TIMER_NR]; >>>> + SunxiTimer 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; >>>> + >>>> +static uint64_t sunxi_pit_read(void *opaque, hwaddr offset, unsigned >>>> size) >>>> +{ >>>> + SunxiPITState *s = SUNXI_PIT(opaque); >>>> + uint8_t index = 0; >>>> >>>> >>> initializer to 0 un-needed. >>> >>> >>> >> >> OK. >> >>>> + >>>> + switch (offset) { >>>> + case SUNXI_TIMER_IRQ_EN: >>>> + return s->irq_enable; >>>> + break; >>>> + case SUNXI_TIMER_IRQ_ST: >>>> + return s->irq_status; >>>> + 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: >>>> + return s->control[index]; >>>> + break; >>>> + case SUNXI_TIMER_INTERVAL: >>>> + return s->interval[index]; >>>> + break; >>>> + case SUNXI_TIMER_COUNT: { >>>> + SunxiTimer *t =&s->timer[index]; >>>> >>>> + s->count[index] = ptimer_get_count(t->timer); >>>> + return s->count[index]; >>>> + } >>>> + default: >>>> + break; >>>> + } >>>> + break; >>>> + case SUNXI_WDOG_CONTROL: >>>> + break; >>>> + case SUNXI_WDOG_MODE: >>>> + break; >>>> + case SUNXI_COUNT_LO: >>>> + return s->count_lo; >>>> + break; >>>> + case SUNXI_COUNT_HI: >>>> + return s->count_hi; >>>> + break; >>>> + case SUNXI_COUNT_CTL: >>>> + return s->count_ctl; >>>> + default: >>>> + break; >>>> >>>> >>> Usual to do a log_guest error here. Same for writes below. >>> >>> >> >> OK. >> >>> >>> >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void sunxi_pit_write(void *opaque, hwaddr offset, uint64_t value, >>>> + unsigned size) >>>> +{ >>>> + SunxiPITState *s = SUNXI_PIT(opaque); >>>> + uint8_t index = 0; >>>> + >>>> + switch (offset) { >>>> + case SUNXI_TIMER_IRQ_EN: >>>> + s->irq_enable = value; >>>> + break; >>>> + case SUNXI_TIMER_IRQ_ST: >>>> + s->irq_status&= value; >>>> >>>> >>> Are you missing a ~ ? This is a clear-to-clear semantic rather than >>> write-to-clear. >>> >>> >> >> yes >> >> >>> Also shouldn't this de-assert the relevant interrupt lines? >>> >>> >>> >> >> there's no related tips in sunxi SoC data-sheet, >> and test is fine until now. >> >> > But AFAICT, there is no way for software to lower an interrupt in an > immediate timeframe, which is very un-usual. Usually the WTC of an ISR > bring down the IRQ pin. There is only one call to qemu_set_irq and > thats in the timer callback so the only way to lower interrupts is for > the timer to hit again with the IRQ masked. > > > I will consider how to de-asset irq line correctly in this case, Thanks! >>>> + 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: { >>>> + SunxiTimer *t =&s->timer[index]; >>>> >>>> + s->control[index] = value; >>>> + if (s->control[index]& SUNXI_TIMER_RELOAD) { >>>> + ptimer_set_count(t->timer, s->interval[index]); >>>> + } >>>> + if (s->control[index]& SUNXI_TIMER_EN) { >>>> + ptimer_run(t->timer, 1); >>>> + } else { >>>> + ptimer_stop(t->timer); >>>> + } >>>> + break; >>>> + } >>>> + case SUNXI_TIMER_INTERVAL: { >>>> + SunxiTimer *t =&s->timer[index]; >>>> >>>> + s->interval[index] = value; >>>> + ptimer_set_count(t->timer, s->interval[index]); >>>> + break; >>>> + } >>>> + case SUNXI_TIMER_COUNT: >>>> + s->count[index] = value; >>>> + default: >>>> + break; >>>> + } >>>> + 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; >>>> >>>> + } >>>> + default: >>>> + break; >>>> + } >>>> +} >>>> + >>>> +static const MemoryRegionOps sunxi_pit_ops = { >>>> + .read = sunxi_pit_read, >>>> + .write = sunxi_pit_write, >>>> + .endianness = DEVICE_NATIVE_ENDIAN, >>>> +}; >>>> + >>>> +static const VMStateDescription vmstate_sunxi_timer = { >>>> + .name = "sunxi.timer", >>>> + .version_id = 1, >>>> + .minimum_version_id = 1, >>>> + .minimum_version_id_old = 1, >>>> + .fields = (VMStateField[]) { >>>> + VMSTATE_PTIMER(timer, SunxiTimer), >>>> + VMSTATE_END_OF_LIST() >>>> + } >>>> +}; >>>> + >>>> +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_STRUCT_ARRAY(timer, SunxiPITState, SUNXI_TIMER_NR, 1, >>>> + vmstate_sunxi_timer, SunxiTimer), >>>> + VMSTATE_END_OF_LIST() >>>> + } >>>> +}; >>>> + >>>> +static void sunxi_pit_reset(DeviceState *dev) >>>> +{ >>>> + SunxiPITState *s = SUNXI_PIT(dev); >>>> + uint8_t i = 0; >>>> + >>>> + s->irq_enable = 0; >>>> + s->irq_status = 0; >>>> + for (i = 0; i< 6; i++) { >>>> + SunxiTimer *t =&s->timer[i]; >>>> >>>> + s->control[i] = 0x4; >>>> >>>> >>> Magic number. >>> >>> >>> >> OK, should be SUNXI_DEFAULT_CLOCK, >> >> Thanks! >> >> >>>> + s->interval[i] = 0; >>>> + s->count[i] = 0; >>>> + ptimer_stop(t->timer); >>>> + } >>>> + 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 = 0; >>>> + >>>> + for (i = 0; i< SUNXI_TIMER_NR; i++) { >>>> + SunxiTimer *t =&s->timer[i]; >>>> >>>> + if (s->control[i]& SUNXI_TIMER_EN&& >>>> + ptimer_get_count(t->timer) == 0) { >>>> + s->irq_status |= 1<< i; >>>> + if (!(s->control[i]& SUNXI_TIMER_MODE)) { >>>> + ptimer_set_count(t->timer, s->interval[i]); >>>> + ptimer_run(t->timer, 1); >>>> + } >>>> + qemu_set_irq(s->irq[i], >>>> + (s->irq_status& s->irq_enable& 1<< i) != 0); >>>> >>>> + } >>>> + } >>>> +} >>>> + >>>> +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 = 0; >>>> + >>>> + 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++) { >>>> + SunxiTimer *t =&s->timer[i]; >>>> >>>> + bh[i] = qemu_bh_new(sunxi_pit_timer_cb, s); >>>> + t->timer = ptimer_init(bh[i]); >>>> + ptimer_set_freq(t->timer, 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..260d36f >>>> --- /dev/null >>>> +++ b/include/hw/timer/sunxi-pit.h >>>> @@ -0,0 +1,37 @@ >>>> +#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 >>>> + >>>> + >>>> +#endif >>>> + >>>> -- >>>> 1.7.2.5 >>>> >>>> >>>> >>>> >>> >>> >> >> >> > >