Re: [Qemu-devel] [RFC v1 1/6] stm32f205_timer: Add the stm32f205 SoC Timer2 to 5

[Peter Crosthwaite]

On Tue, Sep 9, 2014 at 6:23 PM, Alistair Francis <address@hidden> wrote:
> This patch adds the stm32f205 timers: TIM2, TIM3, TIM4 and TIM5
> to QEMU.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V2:
>  - Small changes to functionality and style. Thanks to Peter C
>  - Rename for the Netduino 2 and it's SoC
>
>  default-configs/arm-softmmu.mak    |   1 +
>  hw/timer/Makefile.objs             |   1 +
>  hw/timer/stm32f205_timer.c         | 334 
> +++++++++++++++++++++++++++++++++++++
>  include/hw/timer/stm32f205_timer.h |  84 ++++++++++
>  4 files changed, 420 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..8550084 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_NETDUINO2=y
>

I think we were moving aware from SoC level shared configs, i.e. it's
ok to have a confing per-dev in the arm-softmmu defconfig. You also
want to avoid tying the device (and the SoC) to netduino as much as
possible so even if CONFIG_NETDUINO existed it would simply set
another config for STM etc. Easiest to just go flat with one config
per dev (the ZYNQ example right above does do it wrong too :( ).

>  CONFIG_VERSATILE_PCI=y
>  CONFIG_VERSATILE_I2C=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 2c86c3d..ce68c4a 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
>  common-obj-$(CONFIG_IMX) += imx_gpt.o
>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> +common-obj-$(CONFIG_NETDUINO2) += stm32f205_timer.o
>
>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/stm32f205_timer.c b/hw/timer/stm32f205_timer.c
> new file mode 100644
> index 0000000..bbeaf6b
> --- /dev/null
> +++ b/hw/timer/stm32f205_timer.c
> @@ -0,0 +1,334 @@
> +/*
> + * STM32F205xx Timer 2 to 5
> + *

Looking at the TRM i notice some of the other timers may be similar.
Are TIM9-14 potentially modellable with a slight parameterisation of
this?

I think its ok in first series to limit functionality to TIM2-5 but
you should soften the naming, probably just to stm32f205_timer
throughout (removing all ref to TIM2-5) and leave this 2-5 1/8 9-13
configury to the SoC level.

> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * 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 ST_TIM2_5_ERR_DEBUG
> +#define ST_TIM2_5_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(lvl, fmt, args...) do { \
> +    if (ST_TIM2_5_ERR_DEBUG >= lvl) { \
> +        fprintf(stderr, "stm32f205xx_timer: %s:" fmt, __func__, ## args); \

qemu_log()

> +    } \
> +} while (0);
> +
> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> +
> +static void stm32f205xx_timer_update(Stm32f205TimerState *s)
> +{
> +    s->tim_sr |= 1;
> +    qemu_irq_pulse(s->irq);
> +}

This only serves the purpose of interrupt assertion on the timer
callback below and is single use. Just inline it.

> +
> +static void stm32f205xx_timer_interrupt(void *opaque)
> +{
> +    Stm32f205TimerState *s = (Stm32f205TimerState *)opaque;
> +

No explicit cast needed.

> +    DB_PRINT("Interrupt in: %s\n", __func__);
> +
> +    if (s->tim_dier == 0x01 && s->tim_cr1 & TIM_CR1_CEN) {

The UIE bit of dier is only a single bit so you should use a mask and
macro (just like with & TIM_CR1_CEN).

> +        stm32f205xx_timer_update(s);
> +    }
> +}
> +
> +static uint32_t stm32f205xx_timer_get_count(Stm32f205TimerState *s)
> +{
> +    int64_t now = qemu_clock_get_ns(rtc_clock);

Why use the RTC rather than the virtual clock? Most timer use the
virtual unless they are explicitly an RTC device (does TIMx have RTC
capability?)

> +    return s->tick_offset + now / get_ticks_per_sec();
> +}
> +
> +static void stm32f205xx_timer_set_alarm(Stm32f205TimerState *s)
> +{
> +    uint32_t ticks;
> +
> +    DB_PRINT("Alarm raised in: %s at 0x%x\n", __func__, s->tim_cr1);
> +
> +    ticks = s->tim_arr - stm32f205xx_timer_get_count(s)/
> +                         (s->tim_psc + 1);
> +    DB_PRINT("Alarm set in %u ticks\n", ticks);
> +
> +    if (ticks == 0) {
> +        timer_del(s->timer);
> +        stm32f205xx_timer_interrupt(s);
> +    } else {
> +        int64_t now = qemu_clock_get_ns(rtc_clock) / get_ticks_per_sec();

You should avoid two now calculations in the one callback. You have
already crunched a now in timer_get_count() above and you do it again
here. As timer_get_count is low (single?) use you can just inline to
get now as a calculate-once local var.

> +        timer_mod(s->timer, now + (int64_t)ticks);
> +        DB_PRINT("Wait Time: 0x%x\n", (uint32_t) (now + ticks));
> +    }
> +}
> +
> +static void stm32f205xx_timer_reset(DeviceState *dev)
> +{
> +    struct Stm32f205TimerState *s = STM32F205xxTIMER(dev);
> +

No struct needed.

> +    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;
> +}
> +
> +static uint64_t stm32f205xx_timer_read(void *opaque, hwaddr offset,
> +                           unsigned size)
> +{
> +    Stm32f205TimerState *s = (Stm32f205TimerState *)opaque;
> +

No explicit cast.

> +    DB_PRINT("Read 0x%x\n", (uint) offset);

HWADDR_PRIx instead of cast.

> +
> +    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:
> +        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,
> +                      "STM32F205xx_timer2_5_write: Bad offset %x\n",
> +                      (int) offset);

same.

> +    }
> +
> +    return 0;
> +}
> +
> +static void stm32f205xx_timer_write(void *opaque, hwaddr offset,
> +                        uint64_t val64, unsigned size)
> +{
> +    Stm32f205TimerState *s = (Stm32f205TimerState *)opaque;
> +    uint32_t value = (uint32_t) val64;

Casts not needed.

> +
> +    DB_PRINT("Write 0x%x, 0x%x\n", value, (uint) 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:
> +        s->tim_sr &= value;
> +        stm32f205xx_timer_set_alarm(s);
> +        return;
> +    case TIM_EGR:
> +        s->tim_egr = value;
> +        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;
> +        stm32f205xx_timer_set_alarm(s);
> +        return;
> +    case TIM_PSC:
> +        s->tim_psc = value;
> +        return;
> +    case TIM_ARR:
> +        s->tim_arr = value;
> +        stm32f205xx_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,
> +                      "STM32F205xx_timer2_5_write: Bad offset %x\n",
> +                      (int) offset);
> +    }
> +}
> +
> +static const MemoryRegionOps stm32f205xx_timer_ops = {
> +    .read = stm32f205xx_timer_read,
> +    .write = stm32f205xx_timer_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void stm32f205xx_timer_init(Object *obj)
> +{
> +    Stm32f205TimerState *s = STM32F205xxTIMER(obj);
> +    struct tm tm;
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
> +
> +    memory_region_init_io(&s->iomem, obj, &stm32f205xx_timer_ops, s,
> +                          "stm32f205xx_timer", 0x2000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +
> +    qemu_get_timedate(&tm, 0);
> +    s->tick_offset = mktimegm(&tm) -
> +        qemu_clock_get_ns(rtc_clock) / get_ticks_per_sec();
> +
> +    s->timer = timer_new_ns(rtc_clock, stm32f205xx_timer_interrupt, s);
> +}
> +
> +static void stm32f205xx_timer_pre_save(void *opaque)
> +{
> +    Stm32f205TimerState *s = (Stm32f205TimerState *)opaque;
> +
> +    int64_t delta = qemu_clock_get_ns(rtc_clock) -
> +                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

Blank line here.

> +    s->tick_offset_vmstate = s->tick_offset + delta / get_ticks_per_sec();
> +}
> +
> +static int stm32f205xx_timer_post_load(void *opaque, int version_id)
> +{
> +    Stm32f205TimerState *s = (Stm32f205TimerState *)opaque;
> +
> +    int64_t delta = qemu_clock_get_ns(rtc_clock) -
> +                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

Blank line here.

> +    s->tick_offset = s->tick_offset_vmstate - delta / get_ticks_per_sec();
> +    stm32f205xx_timer_set_alarm(s);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_stm32f205xx_timer = {
> +    .name = "stm32f205xx_timer",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .pre_save = stm32f205xx_timer_pre_save,
> +    .post_load = stm32f205xx_timer_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(tick_offset_vmstate, 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_ccmr1, 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 void stm32f205xx_timer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_stm32f205xx_timer;
> +    dc->reset = stm32f205xx_timer_reset;
> +}
> +
> +static const TypeInfo stm32f205xx_timer_info = {
> +    .name          = TYPE_STM32F205_TIMER,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Stm32f205TimerState),
> +    .instance_init = stm32f205xx_timer_init,
> +    .class_init    = stm32f205xx_timer_class_init,
> +};
> +
> +static void stm32f205xx_timer_register_types(void)
> +{
> +    type_register_static(&stm32f205xx_timer_info);
> +}
> +
> +type_init(stm32f205xx_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..b8ba1ad
> --- /dev/null
> +++ b/include/hw/timer/stm32f205_timer.h
> @@ -0,0 +1,84 @@
> +/*
> + * STM32F205xx Timer 2 to 5
> + *
> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * 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/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 TYPE_STM32F205_TIMER "stm32f205xx-timer"
> +#define STM32F205xxTIMER(obj) OBJECT_CHECK(Stm32f205TimerState, (obj), \
> +                              TYPE_STM32F205_TIMER)
> +
> +typedef struct Stm32f205TimerState {

STM

/*< private >*/

> +    SysBusDevice parent_obj;
> +

/*< public >*/

Regards,
Peter

> +    MemoryRegion iomem;
> +    QEMUTimer *timer;
> +    qemu_irq irq;
> +
> +    uint32_t tick_offset_vmstate;
> +    uint32_t tick_offset;
> +
> +    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;
> --
> 1.9.1
>
>