Re: [Qemu-devel] [PATCH v1 4/5] Netduino_Timer: Add the Netduino Plus 2 Timer2 to 5

[Peter Crosthwaite]

On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <address@hidden> wrote:
> This patch adds the Netduino Plus 2 timers: TIM2, TIM3, TIM4 and TIM5
> to QEMU.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>  hw/timer/Makefile.objs    |   1 +
>  hw/timer/netduino_timer.c | 384 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 385 insertions(+)
>  create mode 100644 hw/timer/netduino_timer.c
>
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 2c86c3d..104fc46 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_NETDUINOP2) += netduino_timer.o
>
>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/netduino_timer.c b/hw/timer/netduino_timer.c
> new file mode 100644
> index 0000000..212ba48
> --- /dev/null
> +++ b/hw/timer/netduino_timer.c
> @@ -0,0 +1,384 @@
> +/*
> + * Netduino Plus 2 USART
> + *
> + * 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 DEBUG_NETTIMER
> +
> +#ifdef DEBUG_NETTIMER
> +#define DPRINTF(fmt, ...) \
> +do { printf("netduino_timer: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#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_NETTIMER "netduino_timer"
> +#define NETTIMER(obj) OBJECT_CHECK(NETTIMERState, (obj), TYPE_NETTIMER)
> +
> +typedef struct NETTIMERState {

Why the all Caps? Is it a symbolic name from documentation?

> +    SysBusDevice parent_obj;
> +
> +    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;
> +} NETTIMERState;
> +
> +static void netduino_timer_update(NETTIMERState *s)
> +{
> +    s->tim_sr |= 1;
> +    qemu_set_irq(s->irq, 1);
> +    qemu_set_irq(s->irq, 0);

qemu_irq_pulse

> +}
> +
> +static void netduino_timer_interrupt(void *opaque)
> +{
> +    NETTIMERState *s = (NETTIMERState *)opaque;
> +
> +    DPRINTF("INterrupt\n");

"Interrupt". Probably add a %s __func__ in there too for clarity.

> +
> +    if (s->tim_dier == 0x01 && s->tim_cr1 & TIM_CR1_CEN) {
> +        netduino_timer_update(s);
> +    }
> +}
> +
> +static uint32_t netduino_timer_get_count(NETTIMERState *s)
> +{
> +    int64_t now = qemu_clock_get_ns(rtc_clock);
> +    return s->tick_offset + now / get_ticks_per_sec();
> +}
> +
> +static void netduino_timer_set_alarm(NETTIMERState *s)
> +{
> +    uint32_t ticks;
> +
> +    DPRINTF("Alarm raised: 0x%x\n", s->tim_cr1);
> +

%s __func__ or something to identify this IP as owning this message.

> +    ticks = (uint32_t) (s->tim_arr - netduino_timer_get_count(s)/
> +                                     (s->tim_psc + 1));

cast not needed.

> +    DPRINTF("Alarm set in %u ticks\n", ticks);
> +    if (ticks <= 0) {

ticks is unsigned, so == would be same semantics or simply "if
(!ticks)". Are you going wanting signed logic?

> +        timer_del(s->timer);
> +        netduino_timer_interrupt(s);
> +    } else {
> +        int64_t now = qemu_clock_get_ns(rtc_clock);
> +        timer_mod(s->timer, now + (int64_t)ticks);

now seems to be in ns, while ticks seems to be in clock-cycles - do
you need a translation factor for ticks? netduino_timer_get_count has
a division factor of get_ticks_per_sec that coverts from ns to "ticks"
so do you need a converse here to go back the other way?

> +        DPRINTF("Wait Time: 0x%x\n", (uint32_t) (now + ticks));
> +    }
> +}
> +
> +static void netduino_timer_reset(DeviceState *dev)
> +{
> +    struct NETTIMERState *s = NETTIMER(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;
> +}
> +
> +static uint64_t netduino_timer_read(void *opaque, hwaddr offset,
> +                           unsigned size)
> +{
> +    NETTIMERState *s = (NETTIMERState *)opaque;
> +
> +    DPRINTF("Read 0x%x\n", (uint) 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:
> +        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;

Guest error default needed?

Regards,
Peter