Re: [Qemu-devel] [fixed-up][PATCH v6 2/5] hw/timer: add sunxi timer device

[Li Guang <lig.fnst@cn.fujitsu.com>]

Peter Crosthwaite wrote:
> On Wed, Nov 27, 2013 at 11:35 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com>  wrote:
>    
>> On Wed, Nov 27, 2013 at 6:29 PM, liguang<lig.fnst@cn.fujitsu.com>  wrote:
>>      
>>> Signed-off-by: liguang<lig.fnst@cn.fujitsu.com>
>>> ---
>>>   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<lig.fnst@cn.fujitsu.com>
>>> + *
>>> + * 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
>>>
>>>
>>>        
>
>