Re: [Qemu-devel] [PATCH v4 1/4] hw/timer: add sunxi timer device

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

Peter Crosthwaite wrote:
> On Tue, Nov 26, 2013 at 6:59 PM, Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
>    
>> Peter Crosthwaite wrote:
>>      
>>> On Tue, Nov 26, 2013 at 5:22 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            |  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<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"
>>>> +
>>>> +
>>>> +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
>>>>
>>>>
>>>>
>>>>          
>>>
>>>        
>>
>>
>>      
>
>