[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [fixed-up][PATCH v6 2/5] hw/timer: add sunxi timer devi
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [fixed-up][PATCH v6 2/5] hw/timer: add sunxi timer device |
|
Date: |
Fri, 29 Nov 2013 10:51:05 +1000 |
On Wed, Nov 27, 2013 at 11:35 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Wed, Nov 27, 2013 at 6:29 PM, liguang <address@hidden> wrote:
>> Signed-off-by: liguang <address@hidden>
>> ---
>> 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 <address@hidden>
>> + *
>> + * 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.
>> + }
>> + 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.
Regards,
Peter
> 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
>>
>>