[v5,1/7] stm32f205_timer: Add the stm32f205 Timer

This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
to QEMU.

Signed-off-by: Alistair Francis <alistair23@gmail.com>
---
V4:
 - Update timer units again
    - Thanks to Peter C
V3:
 - Update debug statements
 - Correct the units for timer_mod
 - Correctly set timer_offset from resets
V2:
 - Reorder the Makefile config
 - Fix up the debug printing
 - Correct the timer event trigger
Changes from RFC:
 - Small changes to functionality and style. Thanks to Peter C
 - Rename to make the timer more generic
 - Split the config settings to device level

 default-configs/arm-softmmu.mak    |   1 +
 hw/timer/Makefile.objs             |   2 +
 hw/timer/stm32f205_timer.c         | 318 +++++++++++++++++++++++++++++++++++++
 include/hw/timer/stm32f205_timer.h | 101 ++++++++++++
 4 files changed, 422 insertions(+)
 create mode 100644 hw/timer/stm32f205_timer.c
 create mode 100644 include/hw/timer/stm32f205_timer.h

Sorry about the review delay...

On Thu, Oct 16, 2014 at 10:53 PM, Alistair Francis <alistair23@gmail.com> wrote:
> This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
> to QEMU.
>
> Signed-off-by: Alistair Francis <alistair23@gmail.com>
> ---
> V4:
>  - Update timer units again
>     - Thanks to Peter C
> V3:
>  - Update debug statements
>  - Correct the units for timer_mod
>  - Correctly set timer_offset from resets
> V2:
>  - Reorder the Makefile config
>  - Fix up the debug printing
>  - Correct the timer event trigger
> Changes from RFC:
>  - Small changes to functionality and style. Thanks to Peter C
>  - Rename to make the timer more generic
>  - Split the config settings to device level
>
>  default-configs/arm-softmmu.mak    |   1 +
>  hw/timer/Makefile.objs             |   2 +
>  hw/timer/stm32f205_timer.c         | 318 +++++++++++++++++++++++++++++++++++++
>  include/hw/timer/stm32f205_timer.h | 101 ++++++++++++
>  4 files changed, 422 insertions(+)
>  create mode 100644 hw/timer/stm32f205_timer.c
>  create mode 100644 include/hw/timer/stm32f205_timer.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index f3513fa..cf23b24 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>  CONFIG_REALVIEW=y
>  CONFIG_ZAURUS=y
>  CONFIG_ZYNQ=y
> +CONFIG_STM32F205_TIMER=y
>
>  CONFIG_VERSATILE_PCI=y
>  CONFIG_VERSATILE_I2C=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 2c86c3d..4bd9617 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -31,3 +31,5 @@ obj-$(CONFIG_DIGIC) += digic-timer.o
>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>
>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
> +
> +common-obj-$(CONFIG_STM32F205_TIMER) += stm32f205_timer.o
> diff --git a/hw/timer/stm32f205_timer.c b/hw/timer/stm32f205_timer.c
> new file mode 100644
> index 0000000..aace8df
> --- /dev/null
> +++ b/hw/timer/stm32f205_timer.c
> @@ -0,0 +1,318 @@
> +/*
> + * STM32F205 Timer

ST doc RM0033 which docs this timer refers to a larger family of SOCs.
I think you can change this from 205 to 2XX probably globally for the
series.

> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/timer/stm32f205_timer.h"
> +
> +#ifndef STM_TIMER_ERR_DEBUG
> +#define STM_TIMER_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(lvl, fmt, args...) do { \
> +    if (STM_TIMER_ERR_DEBUG >= lvl) { \
> +        qemu_log("%s: " fmt, __func__, ## args); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> +
> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s);
> +
> +static void stm32f205_timer_interrupt(void *opaque)
> +{
> +    STM32f205TimerState *s = opaque;
> +
> +    DB_PRINT("Interrupt\n");
> +
> +    if (s->tim_dier & TIM_DIER_UIE && s->tim_cr1 & TIM_CR1_CEN) {
> +        s->tim_sr |= 1;
> +        qemu_irq_pulse(s->irq);
> +        stm32f205_timer_set_alarm(s);
> +    }
> +}
> +
> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s)
> +{
> +    uint32_t ticks;
> +    int64_t now;
> +
> +    DB_PRINT("Alarm set at: 0x%x\n", s->tim_cr1);
> +
> +    now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);

So now is in terms of ms.

> +    ticks = s->tim_arr - ((s->tick_offset + (now * (s->freq_hz / 1000))) /

tick_offset is terms of clock-cycles-before-prescalar. ticks and
tim_arr must be in terms of clock-cycles-post-pre-scalar.

I'm slightly hazy on definition of tick_offset but i'm guessing its
the time offset of when the timer started expressed in
before-prescalar cycles? I would then expect this to be:

ticks = tim_arr - (now * (scale) - tick_offset).

with (now * scale - tick_offset) / tim_psc corresponding to the
current value of the running timer (tim_cnt?).

> +            (s->tim_psc + 1));

So in total this expression is calculating a number of clock cycles
until a hit as "ticks".

> +
> +    DB_PRINT("Alarm set in %d ticks\n", ticks);
> +
> +    if (ticks == 0) {

What if ticks is -ve due to a late callback of set_alarm? It will
probably work, but it seems inconsistent that you rely on the callback
path for -ve and +ve tick balances but have a fast path for when ticks
happens to balance to exactly 0. This fast path should probably handle
-ve's or you could just ditch it entirely.

> +        timer_del(s->timer);
> +        stm32f205_timer_interrupt(s);
> +    } else {
> +        timer_mod(s->timer, ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +

Common sub-expression  (now * (s->freq_hz / 1000)) / (s->tim_psc + 1)
with calculation of "ticks" above can be cached in a variable. but ...

> +                             (int64_t) ticks);

this calculation has me confused. My understanding is timer_mode
should be given an absolute value as time. s->timer is defined as a ns
timer whereas this calculation is a clock cycles value.

> +        DB_PRINT("Wait Time: %" PRId64 " ticks\n",
> +                 ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
> +                 (int64_t) ticks);
> +    }
> +}
> +
> +static void stm32f205_timer_reset(DeviceState *dev)
> +{
> +    STM32f205TimerState *s = STM32F205TIMER(dev);
> +
> +    s->tim_cr1 = 0;
> +    s->tim_cr2 = 0;
> +    s->tim_smcr = 0;
> +    s->tim_dier = 0;
> +    s->tim_sr = 0;
> +    s->tim_egr = 0;
> +    s->tim_ccmr1 = 0;
> +    s->tim_ccmr2 = 0;
> +    s->tim_ccer = 0;
> +    s->tim_cnt = 0;
> +    s->tim_psc = 0;
> +    s->tim_arr = 0;
> +    s->tim_ccr1 = 0;
> +    s->tim_ccr2 = 0;
> +    s->tim_ccr3 = 0;
> +    s->tim_ccr4 = 0;
> +    s->tim_dcr = 0;
> +    s->tim_dmar = 0;
> +    s->tim_or = 0;
> +
> +    s->tick_offset = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
> +                     (s->freq_hz / 1000);
> +}
> +
> +static uint64_t stm32f205_timer_read(void *opaque, hwaddr offset,
> +                           unsigned size)
> +{
> +    STM32f205TimerState *s = opaque;
> +
> +    DB_PRINT("Read 0x%"HWADDR_PRIx"\n", offset);
> +
> +    switch (offset) {
> +    case TIM_CR1:
> +        return s->tim_cr1;
> +    case TIM_CR2:
> +        return s->tim_cr2;
> +    case TIM_SMCR:
> +        return s->tim_smcr;
> +    case TIM_DIER:
> +        return s->tim_dier;
> +    case TIM_SR:
> +        return s->tim_sr;
> +    case TIM_EGR:
> +        return s->tim_egr;
> +    case TIM_CCMR1:
> +        return s->tim_ccmr1;
> +    case TIM_CCMR2:
> +        return s->tim_ccmr2;
> +    case TIM_CCER:
> +        return s->tim_ccer;
> +    case TIM_CNT:
> +        s->tim_cnt = s->tick_offset + (qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
> +                                       (s->freq_hz / 1000));

Same comment above about subbing tick_offset rather than adding.

> +        return s->tim_cnt;
> +    case TIM_PSC:
> +        return s->tim_psc;
> +    case TIM_ARR:
> +        return s->tim_arr;
> +    case TIM_CCR1:
> +        return s->tim_ccr1;
> +    case TIM_CCR2:
> +        return s->tim_ccr2;
> +    case TIM_CCR3:
> +        return s->tim_ccr3;
> +    case TIM_CCR4:
> +        return s->tim_ccr4;
> +    case TIM_DCR:
> +        return s->tim_dcr;
> +    case TIM_DMAR:
> +        return s->tim_dmar;
> +    case TIM_OR:
> +        return s->tim_or;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
> +    }
> +
> +    return 0;
> +}
> +
> +static void stm32f205_timer_write(void *opaque, hwaddr offset,
> +                        uint64_t val64, unsigned size)
> +{
> +    STM32f205TimerState *s = opaque;
> +    uint32_t value = val64;
> +
> +    DB_PRINT("Write 0x%x, 0x%"HWADDR_PRIx"\n", value, offset);
> +
> +    switch (offset) {
> +    case TIM_CR1:
> +        s->tim_cr1 = value;
> +        return;
> +    case TIM_CR2:
> +        s->tim_cr2 = value;
> +        return;
> +    case TIM_SMCR:
> +        s->tim_smcr = value;
> +        return;
> +    case TIM_DIER:
> +        s->tim_dier = value;
> +        return;
> +    case TIM_SR:
> +        /* This is set by hardware and cleared by software */
> +        s->tim_sr &= value;
> +        return;
> +    case TIM_EGR:
> +        s->tim_egr = value;
> +        if (s->tim_egr & TIM_EGR_UG) {
> +            /* Re-init the counter */
> +            stm32f205_timer_reset(DEVICE(s));
> +        }
> +        return;
> +    case TIM_CCMR1:
> +        s->tim_ccmr1 = value;
> +        return;
> +    case TIM_CCMR2:
> +        s->tim_ccmr2 = value;
> +        return;
> +    case TIM_CCER:
> +        s->tim_ccer = value;
> +        return;
> +    case TIM_CNT:
> +        s->tim_cnt = value;

You set tim_cnt here, presumably setting the value of the timer. But
set_alarm doesn't do anything with it. You need to warp tick_offset
here to account for the new 0-base of the timer. When I run into this
situation though, i generally use a two-variable approach where I have
both the value of the timer and the VM timer corresponding to it e.g.

uint32_t timer_val; /* value of timer in clock cycles */
uint64_t sync_time; /* VM time for when timer_val last updated */

And not bother trying to maintain an absolute (0-based) tick offset.
Then every time someone touches a prescalar, timer-value,
enable-switch, etc etc you just sync these two numbers first:

now = qemu_get_clock
timer_val += (now - sync_time) * scale(); /* scale is 0 for a disabled timer */
sync_time = now;

Then do your thing. Then delete or re-arm the callback if needed.

Otherwise you need a sync() pair much like the ones we used for the
ARM PMCCNTR around some of these ops.

> +        stm32f205_timer_set_alarm(s);
> +        return;
> +    case TIM_PSC:
> +        s->tim_psc = value;

Change the prescaler requires a rearming of the callback as it can
have an affect on the calculation of "ticks".

> +        return;
> +    case TIM_ARR:
> +        s->tim_arr = value;
> +        stm32f205_timer_set_alarm(s);
> +        return;
> +    case TIM_CCR1:
> +        s->tim_ccr1 = value;
> +        return;
> +    case TIM_CCR2:
> +        s->tim_ccr2 = value;
> +        return;
> +    case TIM_CCR3:
> +        s->tim_ccr3 = value;
> +        return;
> +    case TIM_CCR4:
> +        s->tim_ccr4 = value;
> +        return;
> +    case TIM_DCR:
> +        s->tim_dcr = value;
> +        return;
> +    case TIM_DMAR:
> +        s->tim_dmar = value;
> +        return;
> +    case TIM_OR:
> +        s->tim_or = value;
> +        return;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
> +    }
> +}
> +
> +static const MemoryRegionOps stm32f205_timer_ops = {
> +    .read = stm32f205_timer_read,
> +    .write = stm32f205_timer_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const VMStateDescription vmstate_stm32f205_timer = {
> +    .name = TYPE_STM32F205_TIMER,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(tick_offset, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_cr1, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_cr2, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_smcr, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_dier, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_sr, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_egr, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccmr1, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccmr2, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccer, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_cnt, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_psc, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_arr, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccr1, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccr2, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccr3, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_ccr4, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_dcr, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_dmar, STM32f205TimerState),
> +        VMSTATE_UINT32(tim_or, STM32f205TimerState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property stm32f205_timer_properties[] = {
> +    DEFINE_PROP_UINT64("clock-frequency", struct STM32f205TimerState,
> +                       freq_hz, 1000000000),

With 1GHz precision should you be using ns timing throughout instead
of ms? You may need to add some muldivs to account for the bigger
numbers.

Regards,
Peter

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void stm32f205_timer_init(Object *obj)
> +{
> +    STM32f205TimerState *s = STM32F205TIMER(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
> +
> +    memory_region_init_io(&s->iomem, obj, &stm32f205_timer_ops, s,
> +                          "stm32f205_timer", 0x2000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +
> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f205_timer_interrupt, s);
> +}
> +
> +static void stm32f205_timer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = stm32f205_timer_reset;
> +    dc->props = stm32f205_timer_properties;
> +    dc->vmsd = &vmstate_stm32f205_timer;
> +}
> +
> +static const TypeInfo stm32f205_timer_info = {
> +    .name          = TYPE_STM32F205_TIMER,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(STM32f205TimerState),
> +    .instance_init = stm32f205_timer_init,
> +    .class_init    = stm32f205_timer_class_init,
> +};
> +
> +static void stm32f205_timer_register_types(void)
> +{
> +    type_register_static(&stm32f205_timer_info);
> +}
> +
> +type_init(stm32f205_timer_register_types)
> diff --git a/include/hw/timer/stm32f205_timer.h b/include/hw/timer/stm32f205_timer.h
> new file mode 100644
> index 0000000..9425cb1
> --- /dev/null
> +++ b/include/hw/timer/stm32f205_timer.h
> @@ -0,0 +1,101 @@
> +/*
> + * STM32F205 Timer
> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_STM_TIMER_H
> +#define HW_STM_TIMER_H
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +
> +#define TIM_CR1      0x00
> +#define TIM_CR2      0x04
> +#define TIM_SMCR     0x08
> +#define TIM_DIER     0x0C
> +#define TIM_SR       0x10
> +#define TIM_EGR      0x14
> +#define TIM_CCMR1    0x18
> +#define TIM_CCMR2    0x1C
> +#define TIM_CCER     0x20
> +#define TIM_CNT      0x24
> +#define TIM_PSC      0x28
> +#define TIM_ARR      0x2C
> +#define TIM_CCR1     0x34
> +#define TIM_CCR2     0x38
> +#define TIM_CCR3     0x3C
> +#define TIM_CCR4     0x40
> +#define TIM_DCR      0x48
> +#define TIM_DMAR     0x4C
> +#define TIM_OR       0x50
> +
> +#define TIM_CR1_CEN   1
> +
> +#define TIM_EGR_UG 1
> +
> +#define TIM_CCER_CC2E   (1 << 4)
> +#define TIM_CCMR1_OC2M2 (1 << 14)
> +#define TIM_CCMR1_OC2M1 (1 << 13)
> +#define TIM_CCMR1_OC2M0 (1 << 12)
> +#define TIM_CCMR1_OC2PE (1 << 11)
> +
> +#define TIM_DIER_UIE  1
> +
> +#define TYPE_STM32F205_TIMER "stm32f205-timer"
> +#define STM32F205TIMER(obj) OBJECT_CHECK(STM32f205TimerState, \
> +                            (obj), TYPE_STM32F205_TIMER)
> +
> +typedef struct STM32f205TimerState {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion iomem;
> +    QEMUTimer *timer;
> +    qemu_irq irq;
> +
> +    uint32_t tick_offset;
> +    uint64_t freq_hz;
> +
> +    uint32_t tim_cr1;
> +    uint32_t tim_cr2;
> +    uint32_t tim_smcr;
> +    uint32_t tim_dier;
> +    uint32_t tim_sr;
> +    uint32_t tim_egr;
> +    uint32_t tim_ccmr1;
> +    uint32_t tim_ccmr2;
> +    uint32_t tim_ccer;
> +    uint32_t tim_cnt;
> +    uint32_t tim_psc;
> +    uint32_t tim_arr;
> +    uint32_t tim_ccr1;
> +    uint32_t tim_ccr2;
> +    uint32_t tim_ccr3;
> +    uint32_t tim_ccr4;
> +    uint32_t tim_dcr;
> +    uint32_t tim_dmar;
> +    uint32_t tim_or;
> +} STM32f205TimerState;
> +
> +#endif
> --
> 1.9.1
>
>

On Mon, Oct 20, 2014 at 5:18 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Sorry about the review delay...
>
> On Thu, Oct 16, 2014 at 10:53 PM, Alistair Francis <alistair23@gmail.com> wrote:
>> This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
>> to QEMU.
>>
>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>> ---
>> V4:
>>  - Update timer units again
>>     - Thanks to Peter C
>> V3:
>>  - Update debug statements
>>  - Correct the units for timer_mod
>>  - Correctly set timer_offset from resets
>> V2:
>>  - Reorder the Makefile config
>>  - Fix up the debug printing
>>  - Correct the timer event trigger
>> Changes from RFC:
>>  - Small changes to functionality and style. Thanks to Peter C
>>  - Rename to make the timer more generic
>>  - Split the config settings to device level
>>
>>  default-configs/arm-softmmu.mak    |   1 +
>>  hw/timer/Makefile.objs             |   2 +
>>  hw/timer/stm32f205_timer.c         | 318 +++++++++++++++++++++++++++++++++++++
>>  include/hw/timer/stm32f205_timer.h | 101 ++++++++++++
>>  4 files changed, 422 insertions(+)
>>  create mode 100644 hw/timer/stm32f205_timer.c
>>  create mode 100644 include/hw/timer/stm32f205_timer.h
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index f3513fa..cf23b24 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>>  CONFIG_REALVIEW=y
>>  CONFIG_ZAURUS=y
>>  CONFIG_ZYNQ=y
>> +CONFIG_STM32F205_TIMER=y
>>
>>  CONFIG_VERSATILE_PCI=y
>>  CONFIG_VERSATILE_I2C=y
>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>> index 2c86c3d..4bd9617 100644
>> --- a/hw/timer/Makefile.objs
>> +++ b/hw/timer/Makefile.objs
>> @@ -31,3 +31,5 @@ obj-$(CONFIG_DIGIC) += digic-timer.o
>>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>>
>>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
>> +
>> +common-obj-$(CONFIG_STM32F205_TIMER) += stm32f205_timer.o
>> diff --git a/hw/timer/stm32f205_timer.c b/hw/timer/stm32f205_timer.c
>> new file mode 100644
>> index 0000000..aace8df
>> --- /dev/null
>> +++ b/hw/timer/stm32f205_timer.c
>> @@ -0,0 +1,318 @@
>> +/*
>> + * STM32F205 Timer
>
> ST doc RM0033 which docs this timer refers to a larger family of SOCs.
> I think you can change this from 205 to 2XX probably globally for the
> series.
>

Ok, I will change all three devices

>> + *
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/timer/stm32f205_timer.h"
>> +
>> +#ifndef STM_TIMER_ERR_DEBUG
>> +#define STM_TIMER_ERR_DEBUG 0
>> +#endif
>> +
>> +#define DB_PRINT_L(lvl, fmt, args...) do { \
>> +    if (STM_TIMER_ERR_DEBUG >= lvl) { \
>> +        qemu_log("%s: " fmt, __func__, ## args); \
>> +    } \
>> +} while (0);
>> +
>> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>> +
>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s);
>> +
>> +static void stm32f205_timer_interrupt(void *opaque)
>> +{
>> +    STM32f205TimerState *s = opaque;
>> +
>> +    DB_PRINT("Interrupt\n");
>> +
>> +    if (s->tim_dier & TIM_DIER_UIE && s->tim_cr1 & TIM_CR1_CEN) {
>> +        s->tim_sr |= 1;
>> +        qemu_irq_pulse(s->irq);
>> +        stm32f205_timer_set_alarm(s);
>> +    }
>> +}
>> +
>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s)
>> +{
>> +    uint32_t ticks;
>> +    int64_t now;
>> +
>> +    DB_PRINT("Alarm set at: 0x%x\n", s->tim_cr1);
>> +
>> +    now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>
> So now is in terms of ms.
>
>> +    ticks = s->tim_arr - ((s->tick_offset + (now * (s->freq_hz / 1000))) /
>
> tick_offset is terms of clock-cycles-before-prescalar. ticks and
> tim_arr must be in terms of clock-cycles-post-pre-scalar.

Yes, that's correct

>
> I'm slightly hazy on definition of tick_offset but i'm guessing its
> the time offset of when the timer started expressed in
> before-prescalar cycles? I would then expect this to be:
>
> ticks = tim_arr - (now * (scale) - tick_offset).

I think tick_offset should be added. That is what the PL031 timer does and
the timer interrupt events don't trigger is tick_offset is subtracted.
I'm not sure
why though

>
> with (now * scale - tick_offset) / tim_psc corresponding to the
> current value of the running timer (tim_cnt?).

That's correct, that is the CNT value. ARR can be set by the guest as an offset

>
>> +            (s->tim_psc + 1));
>
> So in total this expression is calculating a number of clock cycles
> until a hit as "ticks".
>
>> +
>> +    DB_PRINT("Alarm set in %d ticks\n", ticks);
>> +
>> +    if (ticks == 0) {
>
> What if ticks is -ve due to a late callback of set_alarm? It will
> probably work, but it seems inconsistent that you rely on the callback
> path for -ve and +ve tick balances but have a fast path for when ticks
> happens to balance to exactly 0. This fast path should probably handle
> -ve's or you could just ditch it entirely.

Ticks is an unsigned int, it will never be negative. The 'ticks == 0' basically
covers the negative values.

>
>> +        timer_del(s->timer);
>> +        stm32f205_timer_interrupt(s);
>> +    } else {
>> +        timer_mod(s->timer, ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
>
> Common sub-expression  (now * (s->freq_hz / 1000)) / (s->tim_psc + 1)
> with calculation of "ticks" above can be cached in a variable. but ...
>
>> +                             (int64_t) ticks);
>
> this calculation has me confused. My understanding is timer_mode
> should be given an absolute value as time. s->timer is defined as a ns
> timer whereas this calculation is a clock cycles value.

Yes, that's correct. I will fix fix that

>
>> +        DB_PRINT("Wait Time: %" PRId64 " ticks\n",
>> +                 ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
>> +                 (int64_t) ticks);
>> +    }
>> +}
>> +
>> +static void stm32f205_timer_reset(DeviceState *dev)
>> +{
>> +    STM32f205TimerState *s = STM32F205TIMER(dev);
>> +
>> +    s->tim_cr1 = 0;
>> +    s->tim_cr2 = 0;
>> +    s->tim_smcr = 0;
>> +    s->tim_dier = 0;
>> +    s->tim_sr = 0;
>> +    s->tim_egr = 0;
>> +    s->tim_ccmr1 = 0;
>> +    s->tim_ccmr2 = 0;
>> +    s->tim_ccer = 0;
>> +    s->tim_cnt = 0;
>> +    s->tim_psc = 0;
>> +    s->tim_arr = 0;
>> +    s->tim_ccr1 = 0;
>> +    s->tim_ccr2 = 0;
>> +    s->tim_ccr3 = 0;
>> +    s->tim_ccr4 = 0;
>> +    s->tim_dcr = 0;
>> +    s->tim_dmar = 0;
>> +    s->tim_or = 0;
>> +
>> +    s->tick_offset = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
>> +                     (s->freq_hz / 1000);
>> +}
>> +
>> +static uint64_t stm32f205_timer_read(void *opaque, hwaddr offset,
>> +                           unsigned size)
>> +{
>> +    STM32f205TimerState *s = opaque;
>> +
>> +    DB_PRINT("Read 0x%"HWADDR_PRIx"\n", offset);
>> +
>> +    switch (offset) {
>> +    case TIM_CR1:
>> +        return s->tim_cr1;
>> +    case TIM_CR2:
>> +        return s->tim_cr2;
>> +    case TIM_SMCR:
>> +        return s->tim_smcr;
>> +    case TIM_DIER:
>> +        return s->tim_dier;
>> +    case TIM_SR:
>> +        return s->tim_sr;
>> +    case TIM_EGR:
>> +        return s->tim_egr;
>> +    case TIM_CCMR1:
>> +        return s->tim_ccmr1;
>> +    case TIM_CCMR2:
>> +        return s->tim_ccmr2;
>> +    case TIM_CCER:
>> +        return s->tim_ccer;
>> +    case TIM_CNT:
>> +        s->tim_cnt = s->tick_offset + (qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
>> +                                       (s->freq_hz / 1000));
>
> Same comment above about subbing tick_offset rather than adding.
>
>> +        return s->tim_cnt;
>> +    case TIM_PSC:
>> +        return s->tim_psc;
>> +    case TIM_ARR:
>> +        return s->tim_arr;
>> +    case TIM_CCR1:
>> +        return s->tim_ccr1;
>> +    case TIM_CCR2:
>> +        return s->tim_ccr2;
>> +    case TIM_CCR3:
>> +        return s->tim_ccr3;
>> +    case TIM_CCR4:
>> +        return s->tim_ccr4;
>> +    case TIM_DCR:
>> +        return s->tim_dcr;
>> +    case TIM_DMAR:
>> +        return s->tim_dmar;
>> +    case TIM_OR:
>> +        return s->tim_or;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void stm32f205_timer_write(void *opaque, hwaddr offset,
>> +                        uint64_t val64, unsigned size)
>> +{
>> +    STM32f205TimerState *s = opaque;
>> +    uint32_t value = val64;
>> +
>> +    DB_PRINT("Write 0x%x, 0x%"HWADDR_PRIx"\n", value, offset);
>> +
>> +    switch (offset) {
>> +    case TIM_CR1:
>> +        s->tim_cr1 = value;
>> +        return;
>> +    case TIM_CR2:
>> +        s->tim_cr2 = value;
>> +        return;
>> +    case TIM_SMCR:
>> +        s->tim_smcr = value;
>> +        return;
>> +    case TIM_DIER:
>> +        s->tim_dier = value;
>> +        return;
>> +    case TIM_SR:
>> +        /* This is set by hardware and cleared by software */
>> +        s->tim_sr &= value;
>> +        return;
>> +    case TIM_EGR:
>> +        s->tim_egr = value;
>> +        if (s->tim_egr & TIM_EGR_UG) {
>> +            /* Re-init the counter */
>> +            stm32f205_timer_reset(DEVICE(s));
>> +        }
>> +        return;
>> +    case TIM_CCMR1:
>> +        s->tim_ccmr1 = value;
>> +        return;
>> +    case TIM_CCMR2:
>> +        s->tim_ccmr2 = value;
>> +        return;
>> +    case TIM_CCER:
>> +        s->tim_ccer = value;
>> +        return;
>> +    case TIM_CNT:
>> +        s->tim_cnt = value;
>
> You set tim_cnt here, presumably setting the value of the timer. But
> set_alarm doesn't do anything with it. You need to warp tick_offset
> here to account for the new 0-base of the timer. When I run into this
> situation though, i generally use a two-variable approach where I have
> both the value of the timer and the VM timer corresponding to it e.g.
>
> uint32_t timer_val; /* value of timer in clock cycles */
> uint64_t sync_time; /* VM time for when timer_val last updated */
>
> And not bother trying to maintain an absolute (0-based) tick offset.
> Then every time someone touches a prescalar, timer-value,
> enable-switch, etc etc you just sync these two numbers first:
>
> now = qemu_get_clock
> timer_val += (now - sync_time) * scale(); /* scale is 0 for a disabled timer */
> sync_time = now;
>
> Then do your thing. Then delete or re-arm the callback if needed.
>
> Otherwise you need a sync() pair much like the ones we used for the
> ARM PMCCNTR around some of these ops.
>

Can I not just update tick_offset, by adding the difference between what
the current clock value is and what the guest is setting it to?

For example if an event is scheduled for 100 ticks, the clock is at 10 and the
guest writes 90 to the counter. Can't the tick_offset value just be
incremented by
80? Which would push everything forward 80 ticks.

I might be missing something, but that should work shouldn't it?

That way everything will also be updated if the pre-scalar is changed.

>> +        stm32f205_timer_set_alarm(s);
>> +        return;
>> +    case TIM_PSC:
>> +        s->tim_psc = value;
>
> Change the prescaler requires a rearming of the callback as it can
> have an affect on the calculation of "ticks".
>

Yep, will add

>> +        return;
>> +    case TIM_ARR:
>> +        s->tim_arr = value;
>> +        stm32f205_timer_set_alarm(s);
>> +        return;
>> +    case TIM_CCR1:
>> +        s->tim_ccr1 = value;
>> +        return;
>> +    case TIM_CCR2:
>> +        s->tim_ccr2 = value;
>> +        return;
>> +    case TIM_CCR3:
>> +        s->tim_ccr3 = value;
>> +        return;
>> +    case TIM_CCR4:
>> +        s->tim_ccr4 = value;
>> +        return;
>> +    case TIM_DCR:
>> +        s->tim_dcr = value;
>> +        return;
>> +    case TIM_DMAR:
>> +        s->tim_dmar = value;
>> +        return;
>> +    case TIM_OR:
>> +        s->tim_or = value;
>> +        return;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps stm32f205_timer_ops = {
>> +    .read = stm32f205_timer_read,
>> +    .write = stm32f205_timer_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static const VMStateDescription vmstate_stm32f205_timer = {
>> +    .name = TYPE_STM32F205_TIMER,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(tick_offset, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_cr1, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_cr2, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_smcr, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_dier, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_sr, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_egr, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccmr1, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccmr2, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccer, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_cnt, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_psc, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_arr, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccr1, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccr2, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccr3, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_ccr4, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_dcr, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_dmar, STM32f205TimerState),
>> +        VMSTATE_UINT32(tim_or, STM32f205TimerState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static Property stm32f205_timer_properties[] = {
>> +    DEFINE_PROP_UINT64("clock-frequency", struct STM32f205TimerState,
>> +                       freq_hz, 1000000000),
>
> With 1GHz precision should you be using ns timing throughout instead
> of ms? You may need to add some muldivs to account for the bigger
> numbers.

Yeah, I agree. Will fix

Thanks,

Alistair

>
> Regards,
> Peter
>
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void stm32f205_timer_init(Object *obj)
>> +{
>> +    STM32f205TimerState *s = STM32F205TIMER(obj);
>> +
>> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>> +
>> +    memory_region_init_io(&s->iomem, obj, &stm32f205_timer_ops, s,
>> +                          "stm32f205_timer", 0x2000);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +
>> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f205_timer_interrupt, s);
>> +}
>> +
>> +static void stm32f205_timer_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->reset = stm32f205_timer_reset;
>> +    dc->props = stm32f205_timer_properties;
>> +    dc->vmsd = &vmstate_stm32f205_timer;
>> +}
>> +
>> +static const TypeInfo stm32f205_timer_info = {
>> +    .name          = TYPE_STM32F205_TIMER,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(STM32f205TimerState),
>> +    .instance_init = stm32f205_timer_init,
>> +    .class_init    = stm32f205_timer_class_init,
>> +};
>> +
>> +static void stm32f205_timer_register_types(void)
>> +{
>> +    type_register_static(&stm32f205_timer_info);
>> +}
>> +
>> +type_init(stm32f205_timer_register_types)
>> diff --git a/include/hw/timer/stm32f205_timer.h b/include/hw/timer/stm32f205_timer.h
>> new file mode 100644
>> index 0000000..9425cb1
>> --- /dev/null
>> +++ b/include/hw/timer/stm32f205_timer.h
>> @@ -0,0 +1,101 @@
>> +/*
>> + * STM32F205 Timer
>> + *
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#ifndef HW_STM_TIMER_H
>> +#define HW_STM_TIMER_H
>> +
>> +#include "hw/sysbus.h"
>> +#include "qemu/timer.h"
>> +#include "sysemu/sysemu.h"
>> +
>> +#define TIM_CR1      0x00
>> +#define TIM_CR2      0x04
>> +#define TIM_SMCR     0x08
>> +#define TIM_DIER     0x0C
>> +#define TIM_SR       0x10
>> +#define TIM_EGR      0x14
>> +#define TIM_CCMR1    0x18
>> +#define TIM_CCMR2    0x1C
>> +#define TIM_CCER     0x20
>> +#define TIM_CNT      0x24
>> +#define TIM_PSC      0x28
>> +#define TIM_ARR      0x2C
>> +#define TIM_CCR1     0x34
>> +#define TIM_CCR2     0x38
>> +#define TIM_CCR3     0x3C
>> +#define TIM_CCR4     0x40
>> +#define TIM_DCR      0x48
>> +#define TIM_DMAR     0x4C
>> +#define TIM_OR       0x50
>> +
>> +#define TIM_CR1_CEN   1
>> +
>> +#define TIM_EGR_UG 1
>> +
>> +#define TIM_CCER_CC2E   (1 << 4)
>> +#define TIM_CCMR1_OC2M2 (1 << 14)
>> +#define TIM_CCMR1_OC2M1 (1 << 13)
>> +#define TIM_CCMR1_OC2M0 (1 << 12)
>> +#define TIM_CCMR1_OC2PE (1 << 11)
>> +
>> +#define TIM_DIER_UIE  1
>> +
>> +#define TYPE_STM32F205_TIMER "stm32f205-timer"
>> +#define STM32F205TIMER(obj) OBJECT_CHECK(STM32f205TimerState, \
>> +                            (obj), TYPE_STM32F205_TIMER)
>> +
>> +typedef struct STM32f205TimerState {
>> +    /* <private> */
>> +    SysBusDevice parent_obj;
>> +
>> +    /* <public> */
>> +    MemoryRegion iomem;
>> +    QEMUTimer *timer;
>> +    qemu_irq irq;
>> +
>> +    uint32_t tick_offset;
>> +    uint64_t freq_hz;
>> +
>> +    uint32_t tim_cr1;
>> +    uint32_t tim_cr2;
>> +    uint32_t tim_smcr;
>> +    uint32_t tim_dier;
>> +    uint32_t tim_sr;
>> +    uint32_t tim_egr;
>> +    uint32_t tim_ccmr1;
>> +    uint32_t tim_ccmr2;
>> +    uint32_t tim_ccer;
>> +    uint32_t tim_cnt;
>> +    uint32_t tim_psc;
>> +    uint32_t tim_arr;
>> +    uint32_t tim_ccr1;
>> +    uint32_t tim_ccr2;
>> +    uint32_t tim_ccr3;
>> +    uint32_t tim_ccr4;
>> +    uint32_t tim_dcr;
>> +    uint32_t tim_dmar;
>> +    uint32_t tim_or;
>> +} STM32f205TimerState;
>> +
>> +#endif
>> --
>> 1.9.1
>>
>>

On Tue, Oct 21, 2014 at 5:05 PM, Alistair Francis <alistair23@gmail.com> wrote:
> On Mon, Oct 20, 2014 at 5:18 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Sorry about the review delay...
>>
>> On Thu, Oct 16, 2014 at 10:53 PM, Alistair Francis <alistair23@gmail.com> wrote:
>>> This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
>>> to QEMU.
>>>
>>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>>> ---
>>> V4:
>>>  - Update timer units again
>>>     - Thanks to Peter C
>>> V3:
>>>  - Update debug statements
>>>  - Correct the units for timer_mod
>>>  - Correctly set timer_offset from resets
>>> V2:
>>>  - Reorder the Makefile config
>>>  - Fix up the debug printing
>>>  - Correct the timer event trigger
>>> Changes from RFC:
>>>  - Small changes to functionality and style. Thanks to Peter C
>>>  - Rename to make the timer more generic
>>>  - Split the config settings to device level
>>>
>>>  default-configs/arm-softmmu.mak    |   1 +
>>>  hw/timer/Makefile.objs             |   2 +
>>>  hw/timer/stm32f205_timer.c         | 318 +++++++++++++++++++++++++++++++++++++
>>>  include/hw/timer/stm32f205_timer.h | 101 ++++++++++++
>>>  4 files changed, 422 insertions(+)
>>>  create mode 100644 hw/timer/stm32f205_timer.c
>>>  create mode 100644 include/hw/timer/stm32f205_timer.h
>>>
>>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>>> index f3513fa..cf23b24 100644
>>> --- a/default-configs/arm-softmmu.mak
>>> +++ b/default-configs/arm-softmmu.mak
>>> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>>>  CONFIG_REALVIEW=y
>>>  CONFIG_ZAURUS=y
>>>  CONFIG_ZYNQ=y
>>> +CONFIG_STM32F205_TIMER=y
>>>
>>>  CONFIG_VERSATILE_PCI=y
>>>  CONFIG_VERSATILE_I2C=y
>>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>>> index 2c86c3d..4bd9617 100644
>>> --- a/hw/timer/Makefile.objs
>>> +++ b/hw/timer/Makefile.objs
>>> @@ -31,3 +31,5 @@ obj-$(CONFIG_DIGIC) += digic-timer.o
>>>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>>>
>>>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
>>> +
>>> +common-obj-$(CONFIG_STM32F205_TIMER) += stm32f205_timer.o
>>> diff --git a/hw/timer/stm32f205_timer.c b/hw/timer/stm32f205_timer.c
>>> new file mode 100644
>>> index 0000000..aace8df
>>> --- /dev/null
>>> +++ b/hw/timer/stm32f205_timer.c
>>> @@ -0,0 +1,318 @@
>>> +/*
>>> + * STM32F205 Timer
>>
>> ST doc RM0033 which docs this timer refers to a larger family of SOCs.
>> I think you can change this from 205 to 2XX probably globally for the
>> series.
>>
>
> Ok, I will change all three devices
>
>>> + *
>>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>> + */
>>> +
>>> +#include "hw/timer/stm32f205_timer.h"
>>> +
>>> +#ifndef STM_TIMER_ERR_DEBUG
>>> +#define STM_TIMER_ERR_DEBUG 0
>>> +#endif
>>> +
>>> +#define DB_PRINT_L(lvl, fmt, args...) do { \
>>> +    if (STM_TIMER_ERR_DEBUG >= lvl) { \
>>> +        qemu_log("%s: " fmt, __func__, ## args); \
>>> +    } \
>>> +} while (0);
>>> +
>>> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>>> +
>>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s);
>>> +
>>> +static void stm32f205_timer_interrupt(void *opaque)
>>> +{
>>> +    STM32f205TimerState *s = opaque;
>>> +
>>> +    DB_PRINT("Interrupt\n");
>>> +
>>> +    if (s->tim_dier & TIM_DIER_UIE && s->tim_cr1 & TIM_CR1_CEN) {
>>> +        s->tim_sr |= 1;
>>> +        qemu_irq_pulse(s->irq);
>>> +        stm32f205_timer_set_alarm(s);
>>> +    }
>>> +}
>>> +
>>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s)
>>> +{
>>> +    uint32_t ticks;
>>> +    int64_t now;
>>> +
>>> +    DB_PRINT("Alarm set at: 0x%x\n", s->tim_cr1);
>>> +
>>> +    now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>>
>> So now is in terms of ms.
>>
>>> +    ticks = s->tim_arr - ((s->tick_offset + (now * (s->freq_hz / 1000))) /
>>
>> tick_offset is terms of clock-cycles-before-prescalar. ticks and
>> tim_arr must be in terms of clock-cycles-post-pre-scalar.
>
> Yes, that's correct
>
>>
>> I'm slightly hazy on definition of tick_offset but i'm guessing its
>> the time offset of when the timer started expressed in
>> before-prescalar cycles? I would then expect this to be:
>>
>> ticks = tim_arr - (now * (scale) - tick_offset).
>
> I think tick_offset should be added. That is what the PL031 timer does and
> the timer interrupt events don't trigger is tick_offset is subtracted.
> I'm not sure
> why though
>

Ok i'm understanding better now. tick_offset from pl031 is the
absolute real time (seconds since epoch) of when the VM started. now /
scale + tick_offset in that case is the expression for the current
real time:

 79 static uint32_t pl031_get_count(PL031State *s)
 80 {
 81     int64_t now = qemu_clock_get_ns(rtc_clock);
 82     return s->tick_offset + now / get_ticks_per_sec();
 83 }

This is needed to generate real-world RTC times (in seconds) as PL031
is an RTC. This core is not and RTC so you need something different to
pl031 tick_offset concept to account for the reloadable/resettable
timer.

>>
>> with (now * scale - tick_offset) / tim_psc corresponding to the
>> current value of the running timer (tim_cnt?).
>
> That's correct, that is the CNT value. ARR can be set by the guest as an offset
>
>>
>>> +            (s->tim_psc + 1));
>>
>> So in total this expression is calculating a number of clock cycles
>> until a hit as "ticks".
>>
>>> +
>>> +    DB_PRINT("Alarm set in %d ticks\n", ticks);
>>> +
>>> +    if (ticks == 0) {
>>
>> What if ticks is -ve due to a late callback of set_alarm? It will
>> probably work, but it seems inconsistent that you rely on the callback
>> path for -ve and +ve tick balances but have a fast path for when ticks
>> happens to balance to exactly 0. This fast path should probably handle
>> -ve's or you could just ditch it entirely.
>
> Ticks is an unsigned int, it will never be negative. The 'ticks == 0' basically
> covers the negative values.
>

Assuming saturation arithmetic though right? Wont the -ve cases just
overflow into super big numbers that never trigger? Give that ticks
can validly go negative in intermediate calculation maybe it should be
signed.

>>
>>> +        timer_del(s->timer);
>>> +        stm32f205_timer_interrupt(s);
>>> +    } else {
>>> +        timer_mod(s->timer, ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
>>
>> Common sub-expression  (now * (s->freq_hz / 1000)) / (s->tim_psc + 1)
>> with calculation of "ticks" above can be cached in a variable. but ...
>>
>>> +                             (int64_t) ticks);
>>
>> this calculation has me confused. My understanding is timer_mode
>> should be given an absolute value as time. s->timer is defined as a ns
>> timer whereas this calculation is a clock cycles value.
>
> Yes, that's correct. I will fix fix that
>
>>
>>> +        DB_PRINT("Wait Time: %" PRId64 " ticks\n",
>>> +                 ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
>>> +                 (int64_t) ticks);
>>> +    }
>>> +}
>>> +
>>> +static void stm32f205_timer_reset(DeviceState *dev)
>>> +{
>>> +    STM32f205TimerState *s = STM32F205TIMER(dev);
>>> +
>>> +    s->tim_cr1 = 0;
>>> +    s->tim_cr2 = 0;
>>> +    s->tim_smcr = 0;
>>> +    s->tim_dier = 0;
>>> +    s->tim_sr = 0;
>>> +    s->tim_egr = 0;
>>> +    s->tim_ccmr1 = 0;
>>> +    s->tim_ccmr2 = 0;
>>> +    s->tim_ccer = 0;
>>> +    s->tim_cnt = 0;
>>> +    s->tim_psc = 0;
>>> +    s->tim_arr = 0;
>>> +    s->tim_ccr1 = 0;
>>> +    s->tim_ccr2 = 0;
>>> +    s->tim_ccr3 = 0;
>>> +    s->tim_ccr4 = 0;
>>> +    s->tim_dcr = 0;
>>> +    s->tim_dmar = 0;
>>> +    s->tim_or = 0;
>>> +
>>> +    s->tick_offset = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
>>> +                     (s->freq_hz / 1000);
>>> +}
>>> +
>>> +static uint64_t stm32f205_timer_read(void *opaque, hwaddr offset,
>>> +                           unsigned size)
>>> +{
>>> +    STM32f205TimerState *s = opaque;
>>> +
>>> +    DB_PRINT("Read 0x%"HWADDR_PRIx"\n", offset);
>>> +
>>> +    switch (offset) {
>>> +    case TIM_CR1:
>>> +        return s->tim_cr1;
>>> +    case TIM_CR2:
>>> +        return s->tim_cr2;
>>> +    case TIM_SMCR:
>>> +        return s->tim_smcr;
>>> +    case TIM_DIER:
>>> +        return s->tim_dier;
>>> +    case TIM_SR:
>>> +        return s->tim_sr;
>>> +    case TIM_EGR:
>>> +        return s->tim_egr;
>>> +    case TIM_CCMR1:
>>> +        return s->tim_ccmr1;
>>> +    case TIM_CCMR2:
>>> +        return s->tim_ccmr2;
>>> +    case TIM_CCER:
>>> +        return s->tim_ccer;
>>> +    case TIM_CNT:
>>> +        s->tim_cnt = s->tick_offset + (qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
>>> +                                       (s->freq_hz / 1000));
>>
>> Same comment above about subbing tick_offset rather than adding.
>>
>>> +        return s->tim_cnt;
>>> +    case TIM_PSC:
>>> +        return s->tim_psc;
>>> +    case TIM_ARR:
>>> +        return s->tim_arr;
>>> +    case TIM_CCR1:
>>> +        return s->tim_ccr1;
>>> +    case TIM_CCR2:
>>> +        return s->tim_ccr2;
>>> +    case TIM_CCR3:
>>> +        return s->tim_ccr3;
>>> +    case TIM_CCR4:
>>> +        return s->tim_ccr4;
>>> +    case TIM_DCR:
>>> +        return s->tim_dcr;
>>> +    case TIM_DMAR:
>>> +        return s->tim_dmar;
>>> +    case TIM_OR:
>>> +        return s->tim_or;
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void stm32f205_timer_write(void *opaque, hwaddr offset,
>>> +                        uint64_t val64, unsigned size)
>>> +{
>>> +    STM32f205TimerState *s = opaque;
>>> +    uint32_t value = val64;
>>> +
>>> +    DB_PRINT("Write 0x%x, 0x%"HWADDR_PRIx"\n", value, offset);
>>> +
>>> +    switch (offset) {
>>> +    case TIM_CR1:
>>> +        s->tim_cr1 = value;
>>> +        return;
>>> +    case TIM_CR2:
>>> +        s->tim_cr2 = value;
>>> +        return;
>>> +    case TIM_SMCR:
>>> +        s->tim_smcr = value;
>>> +        return;
>>> +    case TIM_DIER:
>>> +        s->tim_dier = value;
>>> +        return;
>>> +    case TIM_SR:
>>> +        /* This is set by hardware and cleared by software */
>>> +        s->tim_sr &= value;
>>> +        return;
>>> +    case TIM_EGR:
>>> +        s->tim_egr = value;
>>> +        if (s->tim_egr & TIM_EGR_UG) {
>>> +            /* Re-init the counter */
>>> +            stm32f205_timer_reset(DEVICE(s));
>>> +        }
>>> +        return;
>>> +    case TIM_CCMR1:
>>> +        s->tim_ccmr1 = value;
>>> +        return;
>>> +    case TIM_CCMR2:
>>> +        s->tim_ccmr2 = value;
>>> +        return;
>>> +    case TIM_CCER:
>>> +        s->tim_ccer = value;
>>> +        return;
>>> +    case TIM_CNT:
>>> +        s->tim_cnt = value;
>>
>> You set tim_cnt here, presumably setting the value of the timer. But
>> set_alarm doesn't do anything with it. You need to warp tick_offset
>> here to account for the new 0-base of the timer. When I run into this
>> situation though, i generally use a two-variable approach where I have
>> both the value of the timer and the VM timer corresponding to it e.g.
>>
>> uint32_t timer_val; /* value of timer in clock cycles */
>> uint64_t sync_time; /* VM time for when timer_val last updated */
>>
>> And not bother trying to maintain an absolute (0-based) tick offset.
>> Then every time someone touches a prescalar, timer-value,
>> enable-switch, etc etc you just sync these two numbers first:
>>
>> now = qemu_get_clock
>> timer_val += (now - sync_time) * scale(); /* scale is 0 for a disabled timer */
>> sync_time = now;
>>
>> Then do your thing. Then delete or re-arm the callback if needed.
>>
>> Otherwise you need a sync() pair much like the ones we used for the
>> ARM PMCCNTR around some of these ops.
>>
>
> Can I not just update tick_offset, by adding the difference between what
> the current clock value is and what the guest is setting it to?
>

I think this will change with the redefinition of tick_offset.

Regards,
Peter

> For example if an event is scheduled for 100 ticks, the clock is at 10 and the
> guest writes 90 to the counter. Can't the tick_offset value just be
> incremented by
> 80? Which would push everything forward 80 ticks.
>
> I might be missing something, but that should work shouldn't it?
>
> That way everything will also be updated if the pre-scalar is changed.
>
>>> +        stm32f205_timer_set_alarm(s);
>>> +        return;
>>> +    case TIM_PSC:
>>> +        s->tim_psc = value;
>>
>> Change the prescaler requires a rearming of the callback as it can
>> have an affect on the calculation of "ticks".
>>
>
> Yep, will add
>
>>> +        return;
>>> +    case TIM_ARR:
>>> +        s->tim_arr = value;
>>> +        stm32f205_timer_set_alarm(s);
>>> +        return;
>>> +    case TIM_CCR1:
>>> +        s->tim_ccr1 = value;
>>> +        return;
>>> +    case TIM_CCR2:
>>> +        s->tim_ccr2 = value;
>>> +        return;
>>> +    case TIM_CCR3:
>>> +        s->tim_ccr3 = value;
>>> +        return;
>>> +    case TIM_CCR4:
>>> +        s->tim_ccr4 = value;
>>> +        return;
>>> +    case TIM_DCR:
>>> +        s->tim_dcr = value;
>>> +        return;
>>> +    case TIM_DMAR:
>>> +        s->tim_dmar = value;
>>> +        return;
>>> +    case TIM_OR:
>>> +        s->tim_or = value;
>>> +        return;
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps stm32f205_timer_ops = {
>>> +    .read = stm32f205_timer_read,
>>> +    .write = stm32f205_timer_write,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>> +};
>>> +
>>> +static const VMStateDescription vmstate_stm32f205_timer = {
>>> +    .name = TYPE_STM32F205_TIMER,
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT32(tick_offset, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_cr1, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_cr2, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_smcr, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_dier, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_sr, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_egr, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccmr1, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccmr2, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccer, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_cnt, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_psc, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_arr, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccr1, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccr2, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccr3, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_ccr4, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_dcr, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_dmar, STM32f205TimerState),
>>> +        VMSTATE_UINT32(tim_or, STM32f205TimerState),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +static Property stm32f205_timer_properties[] = {
>>> +    DEFINE_PROP_UINT64("clock-frequency", struct STM32f205TimerState,
>>> +                       freq_hz, 1000000000),
>>
>> With 1GHz precision should you be using ns timing throughout instead
>> of ms? You may need to add some muldivs to account for the bigger
>> numbers.
>
> Yeah, I agree. Will fix
>
> Thanks,
>
> Alistair
>
>>
>> Regards,
>> Peter
>>
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void stm32f205_timer_init(Object *obj)
>>> +{
>>> +    STM32f205TimerState *s = STM32F205TIMER(obj);
>>> +
>>> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>>> +
>>> +    memory_region_init_io(&s->iomem, obj, &stm32f205_timer_ops, s,
>>> +                          "stm32f205_timer", 0x2000);
>>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>>> +
>>> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f205_timer_interrupt, s);
>>> +}
>>> +
>>> +static void stm32f205_timer_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    dc->reset = stm32f205_timer_reset;
>>> +    dc->props = stm32f205_timer_properties;
>>> +    dc->vmsd = &vmstate_stm32f205_timer;
>>> +}
>>> +
>>> +static const TypeInfo stm32f205_timer_info = {
>>> +    .name          = TYPE_STM32F205_TIMER,
>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>> +    .instance_size = sizeof(STM32f205TimerState),
>>> +    .instance_init = stm32f205_timer_init,
>>> +    .class_init    = stm32f205_timer_class_init,
>>> +};
>>> +
>>> +static void stm32f205_timer_register_types(void)
>>> +{
>>> +    type_register_static(&stm32f205_timer_info);
>>> +}
>>> +
>>> +type_init(stm32f205_timer_register_types)
>>> diff --git a/include/hw/timer/stm32f205_timer.h b/include/hw/timer/stm32f205_timer.h
>>> new file mode 100644
>>> index 0000000..9425cb1
>>> --- /dev/null
>>> +++ b/include/hw/timer/stm32f205_timer.h
>>> @@ -0,0 +1,101 @@
>>> +/*
>>> + * STM32F205 Timer
>>> + *
>>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef HW_STM_TIMER_H
>>> +#define HW_STM_TIMER_H
>>> +
>>> +#include "hw/sysbus.h"
>>> +#include "qemu/timer.h"
>>> +#include "sysemu/sysemu.h"
>>> +
>>> +#define TIM_CR1      0x00
>>> +#define TIM_CR2      0x04
>>> +#define TIM_SMCR     0x08
>>> +#define TIM_DIER     0x0C
>>> +#define TIM_SR       0x10
>>> +#define TIM_EGR      0x14
>>> +#define TIM_CCMR1    0x18
>>> +#define TIM_CCMR2    0x1C
>>> +#define TIM_CCER     0x20
>>> +#define TIM_CNT      0x24
>>> +#define TIM_PSC      0x28
>>> +#define TIM_ARR      0x2C
>>> +#define TIM_CCR1     0x34
>>> +#define TIM_CCR2     0x38
>>> +#define TIM_CCR3     0x3C
>>> +#define TIM_CCR4     0x40
>>> +#define TIM_DCR      0x48
>>> +#define TIM_DMAR     0x4C
>>> +#define TIM_OR       0x50
>>> +
>>> +#define TIM_CR1_CEN   1
>>> +
>>> +#define TIM_EGR_UG 1
>>> +
>>> +#define TIM_CCER_CC2E   (1 << 4)
>>> +#define TIM_CCMR1_OC2M2 (1 << 14)
>>> +#define TIM_CCMR1_OC2M1 (1 << 13)
>>> +#define TIM_CCMR1_OC2M0 (1 << 12)
>>> +#define TIM_CCMR1_OC2PE (1 << 11)
>>> +
>>> +#define TIM_DIER_UIE  1
>>> +
>>> +#define TYPE_STM32F205_TIMER "stm32f205-timer"
>>> +#define STM32F205TIMER(obj) OBJECT_CHECK(STM32f205TimerState, \
>>> +                            (obj), TYPE_STM32F205_TIMER)
>>> +
>>> +typedef struct STM32f205TimerState {
>>> +    /* <private> */
>>> +    SysBusDevice parent_obj;
>>> +
>>> +    /* <public> */
>>> +    MemoryRegion iomem;
>>> +    QEMUTimer *timer;
>>> +    qemu_irq irq;
>>> +
>>> +    uint32_t tick_offset;
>>> +    uint64_t freq_hz;
>>> +
>>> +    uint32_t tim_cr1;
>>> +    uint32_t tim_cr2;
>>> +    uint32_t tim_smcr;
>>> +    uint32_t tim_dier;
>>> +    uint32_t tim_sr;
>>> +    uint32_t tim_egr;
>>> +    uint32_t tim_ccmr1;
>>> +    uint32_t tim_ccmr2;
>>> +    uint32_t tim_ccer;
>>> +    uint32_t tim_cnt;
>>> +    uint32_t tim_psc;
>>> +    uint32_t tim_arr;
>>> +    uint32_t tim_ccr1;
>>> +    uint32_t tim_ccr2;
>>> +    uint32_t tim_ccr3;
>>> +    uint32_t tim_ccr4;
>>> +    uint32_t tim_dcr;
>>> +    uint32_t tim_dmar;
>>> +    uint32_t tim_or;
>>> +} STM32f205TimerState;
>>> +
>>> +#endif
>>> --
>>> 1.9.1
>>>
>>>
>

On Tue, Oct 21, 2014 at 5:40 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Oct 21, 2014 at 5:05 PM, Alistair Francis <alistair23@gmail.com> wrote:
>> On Mon, Oct 20, 2014 at 5:18 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> Sorry about the review delay...
>>>
>>> On Thu, Oct 16, 2014 at 10:53 PM, Alistair Francis <alistair23@gmail.com> wrote:
>>>> This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
>>>> to QEMU.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>>>> ---
>>>> V4:
>>>>  - Update timer units again
>>>>     - Thanks to Peter C
>>>> V3:
>>>>  - Update debug statements
>>>>  - Correct the units for timer_mod
>>>>  - Correctly set timer_offset from resets
>>>> V2:
>>>>  - Reorder the Makefile config
>>>>  - Fix up the debug printing
>>>>  - Correct the timer event trigger
>>>> Changes from RFC:
>>>>  - Small changes to functionality and style. Thanks to Peter C
>>>>  - Rename to make the timer more generic
>>>>  - Split the config settings to device level
>>>>
>>>>  default-configs/arm-softmmu.mak    |   1 +
>>>>  hw/timer/Makefile.objs             |   2 +
>>>>  hw/timer/stm32f205_timer.c         | 318 +++++++++++++++++++++++++++++++++++++
>>>>  include/hw/timer/stm32f205_timer.h | 101 ++++++++++++
>>>>  4 files changed, 422 insertions(+)
>>>>  create mode 100644 hw/timer/stm32f205_timer.c
>>>>  create mode 100644 include/hw/timer/stm32f205_timer.h
>>>>
>>>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>>>> index f3513fa..cf23b24 100644
>>>> --- a/default-configs/arm-softmmu.mak
>>>> +++ b/default-configs/arm-softmmu.mak
>>>> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>>>>  CONFIG_REALVIEW=y
>>>>  CONFIG_ZAURUS=y
>>>>  CONFIG_ZYNQ=y
>>>> +CONFIG_STM32F205_TIMER=y
>>>>
>>>>  CONFIG_VERSATILE_PCI=y
>>>>  CONFIG_VERSATILE_I2C=y
>>>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>>>> index 2c86c3d..4bd9617 100644
>>>> --- a/hw/timer/Makefile.objs
>>>> +++ b/hw/timer/Makefile.objs
>>>> @@ -31,3 +31,5 @@ obj-$(CONFIG_DIGIC) += digic-timer.o
>>>>  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
>>>>
>>>>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
>>>> +
>>>> +common-obj-$(CONFIG_STM32F205_TIMER) += stm32f205_timer.o
>>>> diff --git a/hw/timer/stm32f205_timer.c b/hw/timer/stm32f205_timer.c
>>>> new file mode 100644
>>>> index 0000000..aace8df
>>>> --- /dev/null
>>>> +++ b/hw/timer/stm32f205_timer.c
>>>> @@ -0,0 +1,318 @@
>>>> +/*
>>>> + * STM32F205 Timer
>>>
>>> ST doc RM0033 which docs this timer refers to a larger family of SOCs.
>>> I think you can change this from 205 to 2XX probably globally for the
>>> series.
>>>
>>
>> Ok, I will change all three devices
>>
>>>> + *
>>>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>> + */
>>>> +
>>>> +#include "hw/timer/stm32f205_timer.h"
>>>> +
>>>> +#ifndef STM_TIMER_ERR_DEBUG
>>>> +#define STM_TIMER_ERR_DEBUG 0
>>>> +#endif
>>>> +
>>>> +#define DB_PRINT_L(lvl, fmt, args...) do { \
>>>> +    if (STM_TIMER_ERR_DEBUG >= lvl) { \
>>>> +        qemu_log("%s: " fmt, __func__, ## args); \
>>>> +    } \
>>>> +} while (0);
>>>> +
>>>> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>>>> +
>>>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s);
>>>> +
>>>> +static void stm32f205_timer_interrupt(void *opaque)
>>>> +{
>>>> +    STM32f205TimerState *s = opaque;
>>>> +
>>>> +    DB_PRINT("Interrupt\n");
>>>> +
>>>> +    if (s->tim_dier & TIM_DIER_UIE && s->tim_cr1 & TIM_CR1_CEN) {
>>>> +        s->tim_sr |= 1;
>>>> +        qemu_irq_pulse(s->irq);
>>>> +        stm32f205_timer_set_alarm(s);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void stm32f205_timer_set_alarm(STM32f205TimerState *s)
>>>> +{
>>>> +    uint32_t ticks;
>>>> +    int64_t now;
>>>> +
>>>> +    DB_PRINT("Alarm set at: 0x%x\n", s->tim_cr1);
>>>> +
>>>> +    now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>>>
>>> So now is in terms of ms.
>>>
>>>> +    ticks = s->tim_arr - ((s->tick_offset + (now * (s->freq_hz / 1000))) /
>>>
>>> tick_offset is terms of clock-cycles-before-prescalar. ticks and
>>> tim_arr must be in terms of clock-cycles-post-pre-scalar.
>>
>> Yes, that's correct
>>
>>>
>>> I'm slightly hazy on definition of tick_offset but i'm guessing its
>>> the time offset of when the timer started expressed in
>>> before-prescalar cycles? I would then expect this to be:
>>>
>>> ticks = tim_arr - (now * (scale) - tick_offset).
>>
>> I think tick_offset should be added. That is what the PL031 timer does and
>> the timer interrupt events don't trigger is tick_offset is subtracted.
>> I'm not sure
>> why though
>>
>
> Ok i'm understanding better now. tick_offset from pl031 is the
> absolute real time (seconds since epoch) of when the VM started. now /
> scale + tick_offset in that case is the expression for the current
> real time:
>
>  79 static uint32_t pl031_get_count(PL031State *s)
>  80 {
>  81     int64_t now = qemu_clock_get_ns(rtc_clock);
>  82     return s->tick_offset + now / get_ticks_per_sec();
>  83 }
>
> This is needed to generate real-world RTC times (in seconds) as PL031
> is an RTC. This core is not and RTC so you need something different to
> pl031 tick_offset concept to account for the reloadable/resettable
> timer.

Ok, I will look into fixing up the timer_offset.

>
>>>
>>> with (now * scale - tick_offset) / tim_psc corresponding to the
>>> current value of the running timer (tim_cnt?).
>>
>> That's correct, that is the CNT value. ARR can be set by the guest as an offset
>>
>>>
>>>> +            (s->tim_psc + 1));
>>>
>>> So in total this expression is calculating a number of clock cycles
>>> until a hit as "ticks".
>>>
>>>> +
>>>> +    DB_PRINT("Alarm set in %d ticks\n", ticks);
>>>> +
>>>> +    if (ticks == 0) {
>>>
>>> What if ticks is -ve due to a late callback of set_alarm? It will
>>> probably work, but it seems inconsistent that you rely on the callback
>>> path for -ve and +ve tick balances but have a fast path for when ticks
>>> happens to balance to exactly 0. This fast path should probably handle
>>> -ve's or you could just ditch it entirely.
>>
>> Ticks is an unsigned int, it will never be negative. The 'ticks == 0' basically
>> covers the negative values.
>>
>
> Assuming saturation arithmetic though right? Wont the -ve cases just
> overflow into super big numbers that never trigger? Give that ticks
> can validly go negative in intermediate calculation maybe it should be
> signed.

Values that would be negative for an integer do become big numbers, but they
will eventually wrap around back to zero. I might have to make it an
integer though
when I fix up the timer_offset variable.

Thanks,

Alistair

>
>>>
>>>> +        timer_del(s->timer);
>>>> +        stm32f205_timer_interrupt(s);
>>>> +    } else {
>>>> +        timer_mod(s->timer, ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
>>>
>>> Common sub-expression  (now * (s->freq_hz / 1000)) / (s->tim_psc + 1)
>>> with calculation of "ticks" above can be cached in a variable. but ...
>>>
>>>> +                             (int64_t) ticks);
>>>
>>> this calculation has me confused. My understanding is timer_mode
>>> should be given an absolute value as time. s->timer is defined as a ns
>>> timer whereas this calculation is a clock cycles value.
>>
>> Yes, that's correct. I will fix fix that
>>
>>>
>>>> +        DB_PRINT("Wait Time: %" PRId64 " ticks\n",
>>>> +                 ((now * (s->freq_hz / 1000)) / (s->tim_psc + 1)) +
>>>> +                 (int64_t) ticks);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void stm32f205_timer_reset(DeviceState *dev)
>>>> +{
>>>> +    STM32f205TimerState *s = STM32F205TIMER(dev);
>>>> +
>>>> +    s->tim_cr1 = 0;
>>>> +    s->tim_cr2 = 0;
>>>> +    s->tim_smcr = 0;
>>>> +    s->tim_dier = 0;
>>>> +    s->tim_sr = 0;
>>>> +    s->tim_egr = 0;
>>>> +    s->tim_ccmr1 = 0;
>>>> +    s->tim_ccmr2 = 0;
>>>> +    s->tim_ccer = 0;
>>>> +    s->tim_cnt = 0;
>>>> +    s->tim_psc = 0;
>>>> +    s->tim_arr = 0;
>>>> +    s->tim_ccr1 = 0;
>>>> +    s->tim_ccr2 = 0;
>>>> +    s->tim_ccr3 = 0;
>>>> +    s->tim_ccr4 = 0;
>>>> +    s->tim_dcr = 0;
>>>> +    s->tim_dmar = 0;
>>>> +    s->tim_or = 0;
>>>> +
>>>> +    s->tick_offset = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
>>>> +                     (s->freq_hz / 1000);
>>>> +}
>>>> +
>>>> +static uint64_t stm32f205_timer_read(void *opaque, hwaddr offset,
>>>> +                           unsigned size)
>>>> +{
>>>> +    STM32f205TimerState *s = opaque;
>>>> +
>>>> +    DB_PRINT("Read 0x%"HWADDR_PRIx"\n", offset);
>>>> +
>>>> +    switch (offset) {
>>>> +    case TIM_CR1:
>>>> +        return s->tim_cr1;
>>>> +    case TIM_CR2:
>>>> +        return s->tim_cr2;
>>>> +    case TIM_SMCR:
>>>> +        return s->tim_smcr;
>>>> +    case TIM_DIER:
>>>> +        return s->tim_dier;
>>>> +    case TIM_SR:
>>>> +        return s->tim_sr;
>>>> +    case TIM_EGR:
>>>> +        return s->tim_egr;
>>>> +    case TIM_CCMR1:
>>>> +        return s->tim_ccmr1;
>>>> +    case TIM_CCMR2:
>>>> +        return s->tim_ccmr2;
>>>> +    case TIM_CCER:
>>>> +        return s->tim_ccer;
>>>> +    case TIM_CNT:
>>>> +        s->tim_cnt = s->tick_offset + (qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) *
>>>> +                                       (s->freq_hz / 1000));
>>>
>>> Same comment above about subbing tick_offset rather than adding.
>>>
>>>> +        return s->tim_cnt;
>>>> +    case TIM_PSC:
>>>> +        return s->tim_psc;
>>>> +    case TIM_ARR:
>>>> +        return s->tim_arr;
>>>> +    case TIM_CCR1:
>>>> +        return s->tim_ccr1;
>>>> +    case TIM_CCR2:
>>>> +        return s->tim_ccr2;
>>>> +    case TIM_CCR3:
>>>> +        return s->tim_ccr3;
>>>> +    case TIM_CCR4:
>>>> +        return s->tim_ccr4;
>>>> +    case TIM_DCR:
>>>> +        return s->tim_dcr;
>>>> +    case TIM_DMAR:
>>>> +        return s->tim_dmar;
>>>> +    case TIM_OR:
>>>> +        return s->tim_or;
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void stm32f205_timer_write(void *opaque, hwaddr offset,
>>>> +                        uint64_t val64, unsigned size)
>>>> +{
>>>> +    STM32f205TimerState *s = opaque;
>>>> +    uint32_t value = val64;
>>>> +
>>>> +    DB_PRINT("Write 0x%x, 0x%"HWADDR_PRIx"\n", value, offset);
>>>> +
>>>> +    switch (offset) {
>>>> +    case TIM_CR1:
>>>> +        s->tim_cr1 = value;
>>>> +        return;
>>>> +    case TIM_CR2:
>>>> +        s->tim_cr2 = value;
>>>> +        return;
>>>> +    case TIM_SMCR:
>>>> +        s->tim_smcr = value;
>>>> +        return;
>>>> +    case TIM_DIER:
>>>> +        s->tim_dier = value;
>>>> +        return;
>>>> +    case TIM_SR:
>>>> +        /* This is set by hardware and cleared by software */
>>>> +        s->tim_sr &= value;
>>>> +        return;
>>>> +    case TIM_EGR:
>>>> +        s->tim_egr = value;
>>>> +        if (s->tim_egr & TIM_EGR_UG) {
>>>> +            /* Re-init the counter */
>>>> +            stm32f205_timer_reset(DEVICE(s));
>>>> +        }
>>>> +        return;
>>>> +    case TIM_CCMR1:
>>>> +        s->tim_ccmr1 = value;
>>>> +        return;
>>>> +    case TIM_CCMR2:
>>>> +        s->tim_ccmr2 = value;
>>>> +        return;
>>>> +    case TIM_CCER:
>>>> +        s->tim_ccer = value;
>>>> +        return;
>>>> +    case TIM_CNT:
>>>> +        s->tim_cnt = value;
>>>
>>> You set tim_cnt here, presumably setting the value of the timer. But
>>> set_alarm doesn't do anything with it. You need to warp tick_offset
>>> here to account for the new 0-base of the timer. When I run into this
>>> situation though, i generally use a two-variable approach where I have
>>> both the value of the timer and the VM timer corresponding to it e.g.
>>>
>>> uint32_t timer_val; /* value of timer in clock cycles */
>>> uint64_t sync_time; /* VM time for when timer_val last updated */
>>>
>>> And not bother trying to maintain an absolute (0-based) tick offset.
>>> Then every time someone touches a prescalar, timer-value,
>>> enable-switch, etc etc you just sync these two numbers first:
>>>
>>> now = qemu_get_clock
>>> timer_val += (now - sync_time) * scale(); /* scale is 0 for a disabled timer */
>>> sync_time = now;
>>>
>>> Then do your thing. Then delete or re-arm the callback if needed.
>>>
>>> Otherwise you need a sync() pair much like the ones we used for the
>>> ARM PMCCNTR around some of these ops.
>>>
>>
>> Can I not just update tick_offset, by adding the difference between what
>> the current clock value is and what the guest is setting it to?
>>
>
> I think this will change with the redefinition of tick_offset.
>
> Regards,
> Peter
>
>> For example if an event is scheduled for 100 ticks, the clock is at 10 and the
>> guest writes 90 to the counter. Can't the tick_offset value just be
>> incremented by
>> 80? Which would push everything forward 80 ticks.
>>
>> I might be missing something, but that should work shouldn't it?
>>
>> That way everything will also be updated if the pre-scalar is changed.
>>
>>>> +        stm32f205_timer_set_alarm(s);
>>>> +        return;
>>>> +    case TIM_PSC:
>>>> +        s->tim_psc = value;
>>>
>>> Change the prescaler requires a rearming of the callback as it can
>>> have an affect on the calculation of "ticks".
>>>
>>
>> Yep, will add
>>
>>>> +        return;
>>>> +    case TIM_ARR:
>>>> +        s->tim_arr = value;
>>>> +        stm32f205_timer_set_alarm(s);
>>>> +        return;
>>>> +    case TIM_CCR1:
>>>> +        s->tim_ccr1 = value;
>>>> +        return;
>>>> +    case TIM_CCR2:
>>>> +        s->tim_ccr2 = value;
>>>> +        return;
>>>> +    case TIM_CCR3:
>>>> +        s->tim_ccr3 = value;
>>>> +        return;
>>>> +    case TIM_CCR4:
>>>> +        s->tim_ccr4 = value;
>>>> +        return;
>>>> +    case TIM_DCR:
>>>> +        s->tim_dcr = value;
>>>> +        return;
>>>> +    case TIM_DMAR:
>>>> +        s->tim_dmar = value;
>>>> +        return;
>>>> +    case TIM_OR:
>>>> +        s->tim_or = value;
>>>> +        return;
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, offset);
>>>> +    }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps stm32f205_timer_ops = {
>>>> +    .read = stm32f205_timer_read,
>>>> +    .write = stm32f205_timer_write,
>>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>>> +};
>>>> +
>>>> +static const VMStateDescription vmstate_stm32f205_timer = {
>>>> +    .name = TYPE_STM32F205_TIMER,
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .fields = (VMStateField[]) {
>>>> +        VMSTATE_UINT32(tick_offset, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_cr1, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_cr2, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_smcr, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_dier, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_sr, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_egr, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccmr1, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccmr2, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccer, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_cnt, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_psc, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_arr, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccr1, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccr2, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccr3, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_ccr4, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_dcr, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_dmar, STM32f205TimerState),
>>>> +        VMSTATE_UINT32(tim_or, STM32f205TimerState),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>> +static Property stm32f205_timer_properties[] = {
>>>> +    DEFINE_PROP_UINT64("clock-frequency", struct STM32f205TimerState,
>>>> +                       freq_hz, 1000000000),
>>>
>>> With 1GHz precision should you be using ns timing throughout instead
>>> of ms? You may need to add some muldivs to account for the bigger
>>> numbers.
>>
>> Yeah, I agree. Will fix
>>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>> +static void stm32f205_timer_init(Object *obj)
>>>> +{
>>>> +    STM32f205TimerState *s = STM32F205TIMER(obj);
>>>> +
>>>> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>>>> +
>>>> +    memory_region_init_io(&s->iomem, obj, &stm32f205_timer_ops, s,
>>>> +                          "stm32f205_timer", 0x2000);
>>>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>>>> +
>>>> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f205_timer_interrupt, s);
>>>> +}
>>>> +
>>>> +static void stm32f205_timer_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    dc->reset = stm32f205_timer_reset;
>>>> +    dc->props = stm32f205_timer_properties;
>>>> +    dc->vmsd = &vmstate_stm32f205_timer;
>>>> +}
>>>> +
>>>> +static const TypeInfo stm32f205_timer_info = {
>>>> +    .name          = TYPE_STM32F205_TIMER,
>>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>>> +    .instance_size = sizeof(STM32f205TimerState),
>>>> +    .instance_init = stm32f205_timer_init,
>>>> +    .class_init    = stm32f205_timer_class_init,
>>>> +};
>>>> +
>>>> +static void stm32f205_timer_register_types(void)
>>>> +{
>>>> +    type_register_static(&stm32f205_timer_info);
>>>> +}
>>>> +
>>>> +type_init(stm32f205_timer_register_types)
>>>> diff --git a/include/hw/timer/stm32f205_timer.h b/include/hw/timer/stm32f205_timer.h
>>>> new file mode 100644
>>>> index 0000000..9425cb1
>>>> --- /dev/null
>>>> +++ b/include/hw/timer/stm32f205_timer.h
>>>> @@ -0,0 +1,101 @@
>>>> +/*
>>>> + * STM32F205 Timer
>>>> + *
>>>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>> + */
>>>> +
>>>> +#ifndef HW_STM_TIMER_H
>>>> +#define HW_STM_TIMER_H
>>>> +
>>>> +#include "hw/sysbus.h"
>>>> +#include "qemu/timer.h"
>>>> +#include "sysemu/sysemu.h"
>>>> +
>>>> +#define TIM_CR1      0x00
>>>> +#define TIM_CR2      0x04
>>>> +#define TIM_SMCR     0x08
>>>> +#define TIM_DIER     0x0C
>>>> +#define TIM_SR       0x10
>>>> +#define TIM_EGR      0x14
>>>> +#define TIM_CCMR1    0x18
>>>> +#define TIM_CCMR2    0x1C
>>>> +#define TIM_CCER     0x20
>>>> +#define TIM_CNT      0x24
>>>> +#define TIM_PSC      0x28
>>>> +#define TIM_ARR      0x2C
>>>> +#define TIM_CCR1     0x34
>>>> +#define TIM_CCR2     0x38
>>>> +#define TIM_CCR3     0x3C
>>>> +#define TIM_CCR4     0x40
>>>> +#define TIM_DCR      0x48
>>>> +#define TIM_DMAR     0x4C
>>>> +#define TIM_OR       0x50
>>>> +
>>>> +#define TIM_CR1_CEN   1
>>>> +
>>>> +#define TIM_EGR_UG 1
>>>> +
>>>> +#define TIM_CCER_CC2E   (1 << 4)
>>>> +#define TIM_CCMR1_OC2M2 (1 << 14)
>>>> +#define TIM_CCMR1_OC2M1 (1 << 13)
>>>> +#define TIM_CCMR1_OC2M0 (1 << 12)
>>>> +#define TIM_CCMR1_OC2PE (1 << 11)
>>>> +
>>>> +#define TIM_DIER_UIE  1
>>>> +
>>>> +#define TYPE_STM32F205_TIMER "stm32f205-timer"
>>>> +#define STM32F205TIMER(obj) OBJECT_CHECK(STM32f205TimerState, \
>>>> +                            (obj), TYPE_STM32F205_TIMER)
>>>> +
>>>> +typedef struct STM32f205TimerState {
>>>> +    /* <private> */
>>>> +    SysBusDevice parent_obj;
>>>> +
>>>> +    /* <public> */
>>>> +    MemoryRegion iomem;
>>>> +    QEMUTimer *timer;
>>>> +    qemu_irq irq;
>>>> +
>>>> +    uint32_t tick_offset;
>>>> +    uint64_t freq_hz;
>>>> +
>>>> +    uint32_t tim_cr1;
>>>> +    uint32_t tim_cr2;
>>>> +    uint32_t tim_smcr;
>>>> +    uint32_t tim_dier;
>>>> +    uint32_t tim_sr;
>>>> +    uint32_t tim_egr;
>>>> +    uint32_t tim_ccmr1;
>>>> +    uint32_t tim_ccmr2;
>>>> +    uint32_t tim_ccer;
>>>> +    uint32_t tim_cnt;
>>>> +    uint32_t tim_psc;
>>>> +    uint32_t tim_arr;
>>>> +    uint32_t tim_ccr1;
>>>> +    uint32_t tim_ccr2;
>>>> +    uint32_t tim_ccr3;
>>>> +    uint32_t tim_ccr4;
>>>> +    uint32_t tim_dcr;
>>>> +    uint32_t tim_dmar;
>>>> +    uint32_t tim_or;
>>>> +} STM32f205TimerState;
>>>> +
>>>> +#endif
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>