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

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

Peter Crosthwaite wrote:
> Hi,
>
> On Mon, Nov 25, 2013 at 5:41 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            |  260 +++++++++++++++++++++++++++++++++++++++
>>   include/hw/timer/sunxi-pit.h    |   26 ++++
>>   4 files changed, 289 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..6220b60
>> --- /dev/null
>> +++ b/hw/timer/sunxi-pit.c
>> @@ -0,0 +1,260 @@
>> +/*
>> + * 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 SunxiPITState {
>> +    SysBusDevice busdev;
>>      
> parent_obj
>
>    
>> +    qemu_irq irq[SUNXI_TIMER_NR];
>> +    ptimer_state *timer[6];
>> +    ptimer_state *counter;
>> +    MemoryRegion iomem;
>>      
> blank line here (to demarcate start of registers) would improve readability.
>
>    
>> +    uint32_t  irq_enable;
>> +    uint32_t irq_status;
>> +    uint32_t control[6];
>> +    uint32_t interval[6];
>> +    uint32_t count[6];
>> +    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;
>> +
>> +    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:
>> +            s->count[index] = ptimer_get_count(s->timer[index]);
>> +            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;
>> +    }
>> +
>> +    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:
>> +        for (index = 0; index<  32; index++) {
>> +            if (test_bit(index, (void *)&value)) {
>> +                clear_bit(index, (void *)&s->irq_status);
>> +            }
>> +        }
>>      
> s->irq_status&= ~value;
>
> Is probably a simpler and more common way to implement write-to-clear semantic.
>
>    
>> +        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]&  0x2) {
>> +                ptimer_set_count(s->timer[index], s->interval[index]);
>> +            }
>> +            if (s->control[index]&  0x1) {
>> +                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;
>> +        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&  0x2) {
>>      
> best to define macros for the bitmasks.
>
>    

all above are reasonable, will fix
>> +            s->count_lo = ptimer_get_count(s->counter) ;
>> +            s->count_hi = ptimer_get_count(s->counter)>>  32;
>> +            s->count_ctl&= ~0x2;
>>      
> But this does seem strange. The counter register value only updates to
> the free running counter value when you write this magic bit?
>    

Yes, SPEC says,
" to latch the 64-bit Counter to the Low/Hi registers and it
will change to zero after the registers are latched"
>    
>> +        }
>> +        if (s->count_ctl&  0x1) {
>> +            s->count_lo =0;
>> +            s->count_hi =0;
>> +            s->count_ctl&= ~0x1;
>> +        }
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps sunxi_pit_ops = {
>> +    .read = sunxi_pit_read,
>> +    .write = sunxi_pit_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +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++) {
>> +        s->control[i] = 0x4;
>>      
> define magic number 0x4.
>
>    
>> +        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 = 0;
>> +
>> +    for (i = 0; i<  SUNXI_TIMER_NR; i++) {
>> +        if (!(s->control[i]&  0x80)&&  s->control[i]&  0x1) {
>> +            s->irq_status |= 1<<  i;
>> +            ptimer_set_count(s->timer[i], s->interval[i]);
>> +            ptimer_run(s->timer[i], 1);
>> +        }
>> +        if (s->irq_status&  s->irq_enable&  (1<<  i)) {
>> +            qemu_irq_raise(s->irq[i]);
>> +        } else {
>> +            qemu_irq_lower(s->irq[i]);
>> +        }
>>      
> qemu_irq_set can make shorter work of this.
>
>    
>> +    }
>>      
> but this loop seems strange. It doesnt seem to identify which timer
> actually hit, yet its potenitally raising interrupts for all 6 in a
> loop. Will this cause spurious interrupts when multiple timers are
> running?
>
>    

my poor implementation
>> +}
>> +
>> +static void sunxi_pit_counter_cb(void *opaque)
>> +{
>> +}
>> +
>> +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];
>> +    QEMUBH *cbh;
>> +    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++) {
>> +         bh[i] = qemu_bh_new(sunxi_pit_timer_cb, s);
>> +         s->timer[i] = ptimer_init(bh[i]);
>> +         ptimer_set_freq(s->timer[i], 240000);
>> +     }
>> +     cbh = qemu_bh_new(sunxi_pit_counter_cb, s);
>> +     s->counter = ptimer_init(cbh);
>>      
> use of a ptimer for a free running counter seems strange to me.
> Ptimers are intend to wrap the lower level QEMU timer for periodic
> timing applications. This doesn't really fit that. Its perhaps just
> simpler to use qemu_get_clock_ns.

good suggestion!
>
>> +     ptimer_set_freq(s->counter, 100);
>> +     ptimer_set_count(s->counter, 0xffffffff);
>> +     ptimer_run(s->counter, 0);
>>      
> This is strange for a realize function. You are starting a timer as a
> machine creation step. Shouldn't this be in reset (along with a reset
> of the timer value itself)?
>
>    
>> +}
>> +
>> +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";
>>      
> You should add VMSD support.
>
>    
>> +}
>> +
>> +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..38f1b4d
>> --- /dev/null
>> +++ b/include/hw/timer/sunxi-pit.h
>> @@ -0,0 +1,26 @@
>> +#ifndef SUNXI_PIT_H
>> +#define SUNXI_PIT_H
>> +
>> +
>> +#define TYPE_SUNXI_PIT "sunix-timer"
>>      
> "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_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_LO 0xa4
>> +#define SUNXI_COUNT_HI 0xa8
>> +#define SUNXI_TIMER_BASE 0x10
>> +
>>      
> You could tab out the numbers to a consistent tab-stop for easier reading.
>
>    

reasonable, will fix.

Thanks a lot!

>> +
>> +#endif
>> +
>> --
>> 1.7.2.5
>>
>>
>>      
>