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

[Peter Crosthwaite]

Hi,

On Mon, Nov 25, 2013 at 5:41 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            |  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 <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"
> +
> +
> +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.

> +            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?

> +        }
> +        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?

> +}
> +
> +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.

> +     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.

Regards,
Peter

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