Re: [Qemu-devel] [PATCH v4 11/13] hw/timer/nrf51_timer: Add nRF51 Timer peripheral

[Peter Maydell]

On 2 November 2018 at 17:07, Steffen Görtz <address@hidden> wrote:
> This patch adds the model for the nRF51 timer peripheral.
> Currently, only the TIMER mode is implemented.
>
> Signed-off-by: Steffen Görtz <address@hidden>
> ---
>  hw/timer/Makefile.objs         |   1 +
>  hw/timer/nrf51_timer.c         | 368 +++++++++++++++++++++++++++++++++
>  hw/timer/trace-events          |   5 +
>  include/hw/timer/nrf51_timer.h |  75 +++++++
>  4 files changed, 449 insertions(+)
>  create mode 100644 hw/timer/nrf51_timer.c
>  create mode 100644 include/hw/timer/nrf51_timer.h

Hi; I have some review comments on this device below; sorry
it's taken me a while to get to it.

> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index b32194d153..0e9a4530f8 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -23,6 +23,7 @@ common-obj-$(CONFIG_IMX) += imx_gpt.o
>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
>  common-obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp-rtc.o
> +common-obj-$(CONFIG_NRF51_SOC) += nrf51_timer.o
>
>  obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
> diff --git a/hw/timer/nrf51_timer.c b/hw/timer/nrf51_timer.c
> new file mode 100644
> index 0000000000..623b5dd18e
> --- /dev/null
> +++ b/hw/timer/nrf51_timer.c
> @@ -0,0 +1,368 @@
> +/*
> + * nRF51 System-on-Chip Timer peripheral
> + *
> + * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
> + * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/arm/nrf51.h"
> +#include "hw/timer/nrf51_timer.h"
> +#include "trace.h"
> +
> +#define TIMER_CLK 16000000ULL
> +
> +static uint8_t const bitwidths[] = {16, 8, 24, 32};
> +#define BWM(x) ((1UL << bitwidths[x]) - 1)
> +
> +typedef enum {
> +    NRF51_TIMER_TIMER = 0,
> +    NRF51_TIMER_COUNTER = 1
> +} Nrf51TimerMode;

We capitalize NRF51 in type names elsewhere; prefer to be consistent.

> +
> +
> +static inline uint64_t ns_to_ticks(NRF51TimerState *s, uint64_t ns)
> +{
> +    uint64_t t = NANOSECONDS_PER_SECOND * (1 << s->prescaler);
> +    return muldiv64(ns, TIMER_CLK, t);

muldiv64's third argument is only a uint32_t, so something is odd here
because we're passing it a uint64_t.

if s->prescaler is 15 then NANOSECONDS_PER_SECOND * (1 << s->prescaler)
will be too large to fit in a 32-bit number, and passing t to
the 3rd argument of muldiv64() will implicitly truncate it.
So we need to do something else here. I think we want:

uint32_t frq = TIMER_CLK >> s->prescaler;
return muldiv64(ns, frq, t);

The calculation of frq will not lose precision for prescaler
values in the datasheet-specified range of 0..9, because
TIMER_CLK is 0xF42400 and we can shift that by up to 10 bits
without losing precision. (The register-write limitation of
the value to 4 bits means we won't accidentally divide by
zero if the guest writes an overlarge value.)

> +}
> +
> +static inline uint64_t ticks_to_ns(NRF51TimerState *s, uint64_t ticks)
> +{
> +    ticks *= (1 << s->prescaler);
> +    return muldiv64(ticks, NANOSECONDS_PER_SECOND, TIMER_CLK);

Here we might overflow when we multiply ticks by 1 >> s->prescaler.
Instead we can do

uint32_t frq = TIMER_CLK >> s->prescaler;
return muldiv64(ticks, NANOSECONDS_PER_SECOND, frq);

(which has the advantage of being more clearly the inverse of
ns_to_ticks()).

> +}
> +
> +static void update_irq(NRF51TimerState *s)
> +{
> +    bool flag = false;
> +    size_t i;
> +
> +    for (i = 0; i < NRF51_TIMER_REG_COUNT; i++) {
> +        flag |= s->events_compare[i] && extract32(s->inten, 16 + i, 1);
> +    }
> +    qemu_set_irq(s->irq, flag);
> +}
> +
> +static void update_events(NRF51TimerState *s, uint64_t now)
> +{
> +    uint64_t strobe;
> +    uint64_t tick;
> +    uint64_t cc;
> +    size_t i;
> +    bool occured;

"occurred" has two 'r's.

> +
> +    strobe = ns_to_ticks(s, now - s->last_visited);
> +    tick = ns_to_ticks(s, s->last_visited - s->time_offset) & 
> BWM(s->bitmode);
> +
> +    for (i = 0; i < NRF51_TIMER_REG_COUNT; i++) {
> +        cc = s->cc[i];
> +
> +        if (tick < cc) {
> +            occured = (cc - tick) <= strobe;
> +        } else {
> +            occured = ((cc + (1UL << bitwidths[s->bitmode])) - tick) <= 
> strobe;
> +        }
> +
> +        s->events_compare[i] |= occured;
> +    }

I find this logic a bit confusing. A comment about what we're doing
might help readers.

> +
> +    s->last_visited = now;
> +}
> +
> +static int cmpfunc(const void *a, const void *b)
> +{
> +   return *(uint32_t *)a - *(uint32_t *)b;
> +}
> +
> +static uint64_t get_next_timeout(NRF51TimerState *s, uint64_t now)
> +{
> +    uint64_t r;
> +    size_t idx;
> +
> +    uint64_t tick = (ns_to_ticks(s, now - s->time_offset)) & BWM(s->bitmode);
> +    int8_t next = -1;
> +
> +    for (idx = 0; idx < NRF51_TIMER_REG_COUNT; idx++) {
> +        if (s->cc_sorted[idx] > tick) {
> +            next = idx;
> +            break;
> +        }
> +    }

There are only 4 timers, so I'm pretty sure the time you save
here by not having to scan the whole array is not significant,
so keeping a cached sorted version of the comparison registers
seems like overkill to me.

> +
> +    if (next == -1) {
> +        r = s->cc_sorted[0] + (1UL << bitwidths[s->bitmode]);
> +    } else {
> +        r = s->cc_sorted[next];
> +    }
> +
> +    return now + ticks_to_ns(s, r - tick);

This is going to overestimate if 'now' is in the middle
between two ticks. Ideally you want to add the time-in-ns
of the time when the last tick was (ie now rounded down to a tick).

> +}
> +
> +static void update_internal_state(NRF51TimerState *s, uint64_t now)
> +{
> +    if (s->running) {
> +        timer_mod(&s->timer, get_next_timeout(s, now));
> +    } else {
> +        timer_del(&s->timer);
> +    }
> +
> +    update_irq(s);
> +}
> +
> +static void timer_expire(void *opaque)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(opaque);
> +    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    update_events(s, now);
> +    update_internal_state(s, now);
> +}
> +
> +static uint64_t nrf51_timer_read(void *opaque, hwaddr offset, unsigned int 
> size)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(opaque);
> +    uint64_t r = 0;
> +
> +    switch (offset) {
> +    case NRF51_TIMER_EVENT_COMPARE_0 ... NRF51_TIMER_EVENT_COMPARE_3:
> +        r = s->events_compare[(offset - NRF51_TIMER_EVENT_COMPARE_0) / 4];
> +        break;
> +    case NRF51_TIMER_REG_SHORTS:
> +        r = s->shorts;
> +        break;
> +    case NRF51_TIMER_REG_INTENSET:
> +        r = s->inten;
> +        break;
> +    case NRF51_TIMER_REG_INTENCLR:
> +        r = s->inten;
> +        break;
> +    case NRF51_TIMER_REG_MODE:
> +        r = s->mode;
> +        break;
> +    case NRF51_TIMER_REG_BITMODE:
> +        r = s->bitmode;
> +        break;
> +    case NRF51_TIMER_REG_PRESCALER:
> +        r = s->prescaler;
> +        break;
> +    case NRF51_TIMER_REG_CC0 ... NRF51_TIMER_REG_CC3:
> +        r = s->cc[(offset - NRF51_TIMER_REG_CC0) / 4];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: bad read offset 0x%" HWADDR_PRIx "\n",
> +                      __func__, offset);
> +    }
> +
> +    trace_nrf51_timer_read(offset, r, size);
> +
> +    return r;
> +}
> +
> +static inline void update_cc_sorted(NRF51TimerState *s)
> +{
> +    memcpy(s->cc_sorted, s->cc, sizeof(s->cc_sorted));
> +    qsort(s->cc_sorted, NRF51_TIMER_REG_COUNT, sizeof(uint32_t), cmpfunc);
> +}
> +
> +static void nrf51_timer_write(void *opaque, hwaddr offset,
> +                       uint64_t value, unsigned int size)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(opaque);
> +    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    size_t idx;
> +
> +    trace_nrf51_timer_write(offset, value, size);
> +
> +    switch (offset) {
> +    case NRF51_TIMER_TASK_START:
> +        if (value == NRF51_TRIGGER_TASK) {
> +            s->running = true;
> +        }
> +        break;
> +    case NRF51_TIMER_TASK_STOP:
> +    case NRF51_TIMER_TASK_SHUTDOWN:
> +        if (value == NRF51_TRIGGER_TASK) {
> +            s->running = false;
> +        }
> +        break;
> +    case NRF51_TIMER_TASK_COUNT:
> +        if (value == NRF51_TRIGGER_TASK) {
> +            qemu_log_mask(LOG_UNIMP, "COUNTER mode not implemented\n");
> +        }
> +        break;
> +    case NRF51_TIMER_TASK_CLEAR:
> +        if (value == NRF51_TRIGGER_TASK) {
> +            s->time_offset = now;
> +            s->last_visited = now;
> +        }
> +        break;
> +    case NRF51_TIMER_TASK_CAPTURE_0 ... NRF51_TIMER_TASK_CAPTURE_3:
> +        if (value == NRF51_TRIGGER_TASK) {
> +            idx = (offset - NRF51_TIMER_TASK_CAPTURE_0) / 4;
> +            s->cc[idx] = ns_to_ticks(s, now - s->time_offset) & 
> BWM(s->bitmode);
> +            update_cc_sorted(s);
> +        }
> +        break;
> +    case NRF51_TIMER_EVENT_COMPARE_0 ... NRF51_TIMER_EVENT_COMPARE_3:
> +        if (value == NRF51_EVENT_CLEAR) {
> +            s->events_compare[(offset - NRF51_TIMER_EVENT_COMPARE_0) / 4] = 
> 0;
> +        }
> +        break;
> +    case NRF51_TIMER_REG_SHORTS:
> +        s->shorts = value & NRF51_TIMER_REG_SHORTS_MASK;
> +        break;
> +    case NRF51_TIMER_REG_INTENSET:
> +        s->inten |= value & NRF51_TIMER_REG_INTEN_MASK;
> +        break;
> +    case NRF51_TIMER_REG_INTENCLR:
> +        s->inten &= ~(value & NRF51_TIMER_REG_INTEN_MASK);
> +        break;
> +    case NRF51_TIMER_REG_MODE:
> +        if (s->mode != NRF51_TIMER_TIMER) {
> +            qemu_log_mask(LOG_UNIMP, "COUNTER mode not implemented\n");
> +            return;
> +        }
> +        s->mode = value;
> +        break;
> +    case NRF51_TIMER_REG_BITMODE:
> +        if (s->mode == NRF51_TIMER_TIMER && s->running) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: erroneous change of BITMODE while timer is running\n",
> +                __func__);
> +        }
> +        s->bitmode = value & NRF51_TIMER_REG_BITMODE_MASK;
> +        s->time_offset = now;
> +        s->last_visited = now;
> +        break;
> +    case NRF51_TIMER_REG_PRESCALER:
> +        if (s->mode == NRF51_TIMER_TIMER && s->running) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: erroneous change of PRESCALER while timer is running\n",
> +                __func__);
> +        }
> +        s->prescaler = value & NRF51_TIMER_REG_PRESCALER_MASK;
> +        s->time_offset = now;
> +        s->last_visited = now;
> +        break;
> +    case NRF51_TIMER_REG_CC0 ... NRF51_TIMER_REG_CC3:
> +        s->cc[(offset - NRF51_TIMER_REG_CC0) / 4] = value & BWM(s->bitmode);
> +        update_cc_sorted(s);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: bad write offset 0x%" HWADDR_PRIx "\n",
> +                      __func__, offset);
> +    }
> +
> +    update_internal_state(s, now);
> +}
> +
> +static const MemoryRegionOps rng_ops = {
> +    .read =  nrf51_timer_read,
> +    .write = nrf51_timer_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4,
> +};
> +
> +static void nrf51_timer_init(Object *obj)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &rng_ops, s,
> +            TYPE_NRF51_TIMER, NRF51_TIMER_SIZE);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    timer_init_ns(&s->timer, QEMU_CLOCK_VIRTUAL, timer_expire, s);
> +}
> +
> +static void nrf51_timer_reset(DeviceState *dev)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(dev);
> +
> +    s->running = false;
> +
> +    memset(s->events_compare, 0x00, sizeof(s->events_compare));
> +    memset(s->cc, 0x00, sizeof(s->cc));
> +    memset(s->cc_sorted, 0x00, sizeof(s->cc_sorted));
> +    s->shorts = 0x00;
> +    s->inten = 0x00;
> +    s->mode = 0x00;
> +    s->bitmode = 0x00;
> +    s->prescaler = 0x00;
> +
> +    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    s->time_offset = now;
> +    s->last_visited = now;
> +
> +    update_internal_state(s, now);
> +}
> +
> +static int nrf51_timer_post_load(void *opaque, int version_id)
> +{
> +    NRF51TimerState *s = NRF51_TIMER(opaque);
> +    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    update_internal_state(s, now);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_nrf51_timer = {
> +    .name = TYPE_NRF51_TIMER,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = nrf51_timer_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_TIMER(timer, NRF51TimerState),
> +        VMSTATE_BOOL(running, NRF51TimerState),
> +        VMSTATE_UINT64(time_offset, NRF51TimerState),
> +        VMSTATE_UINT64(last_visited, NRF51TimerState),
> +        VMSTATE_UINT8_ARRAY(events_compare, NRF51TimerState,
> +                            NRF51_TIMER_REG_COUNT),
> +        VMSTATE_UINT32_ARRAY(cc, NRF51TimerState, NRF51_TIMER_REG_COUNT),
> +        VMSTATE_UINT32_ARRAY(cc_sorted, NRF51TimerState, 
> NRF51_TIMER_REG_COUNT),

Better not to transfer the cached cc in the migration state.
Instead (if you have it at all) recalculate it on load.

> +        VMSTATE_UINT32(shorts, NRF51TimerState),
> +        VMSTATE_UINT32(inten, NRF51TimerState),
> +        VMSTATE_UINT32(mode, NRF51TimerState),
> +        VMSTATE_UINT32(bitmode, NRF51TimerState),
> +        VMSTATE_UINT32(prescaler, NRF51TimerState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static Property nrf51_timer_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),

If you don't have any properties then you don't need to set
dc->props at all.

> +};
> +
> +static void nrf51_timer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = nrf51_timer_properties;
> +    dc->reset = nrf51_timer_reset;
> +    dc->vmsd = &vmstate_nrf51_timer;
> +}
> +
> +static const TypeInfo nrf51_timer_info = {
> +    .name = TYPE_NRF51_TIMER,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(NRF51TimerState),
> +    .instance_init = nrf51_timer_init,
> +    .class_init = nrf51_timer_class_init
> +};
> +
> +static void nrf51_timer_register_types(void)
> +{
> +    type_register_static(&nrf51_timer_info);
> +}
> +
> +type_init(nrf51_timer_register_types)
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index 75bd3b1042..0144a68951 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -72,3 +72,8 @@ sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 
> 0x%" PRIx64 " value
>
>  # hw/timer/xlnx-zynqmp-rtc.c
>  xlnx_zynqmp_rtc_gettime(int year, int month, int day, int hour, int min, int 
> sec) "Get time from host: %d-%d-%d %2d:%02d:%02d"
> +
> +# hw/timer/nrf51_timer.c
> +nrf51_timer_read(uint64_t addr, uint32_t value, unsigned size) "read addr 
> 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
> +nrf51_timer_write(uint64_t addr, uint32_t value, unsigned size) "write addr 
> 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
> +
> diff --git a/include/hw/timer/nrf51_timer.h b/include/hw/timer/nrf51_timer.h
> new file mode 100644
> index 0000000000..e113bd7e96
> --- /dev/null
> +++ b/include/hw/timer/nrf51_timer.h
> @@ -0,0 +1,75 @@
> +/*
> + * nRF51 System-on-Chip Timer peripheral
> + *
> + * QEMU interface:
> + * + sysbus MMIO regions 0: GPIO registers
> + * + sysbus irq
> + *
> + * Accuracy of the peripheral model:
> + * + Only TIMER mode is implemented, COUNTER mode is not implemented.
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef NRF51_TIMER_H
> +#define NRF51_TIMER_H
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#define TYPE_NRF51_TIMER "nrf51_soc.timer"
> +#define NRF51_TIMER(obj) OBJECT_CHECK(NRF51TimerState, (obj), 
> TYPE_NRF51_TIMER)
> +
> +#define NRF51_TIMER_REG_COUNT 4
> +
> +#define NRF51_TIMER_TASK_START 0x000
> +#define NRF51_TIMER_TASK_STOP 0x004
> +#define NRF51_TIMER_TASK_COUNT 0x008
> +#define NRF51_TIMER_TASK_CLEAR 0x00C
> +#define NRF51_TIMER_TASK_SHUTDOWN 0x010
> +#define NRF51_TIMER_TASK_CAPTURE_0 0x040
> +#define NRF51_TIMER_TASK_CAPTURE_3 0x04C
> +
> +#define NRF51_TIMER_EVENT_COMPARE_0 0x140
> +#define NRF51_TIMER_EVENT_COMPARE_3 0x14C
> +
> +#define NRF51_TIMER_REG_SHORTS 0x200
> +#define NRF51_TIMER_REG_SHORTS_MASK 0xf0f
> +#define NRF51_TIMER_REG_INTENSET 0x304
> +#define NRF51_TIMER_REG_INTENCLR 0x308
> +#define NRF51_TIMER_REG_INTEN_MASK 0xf0000
> +#define NRF51_TIMER_REG_MODE 0x504
> +#define NRF51_TIMER_REG_MODE_MASK 0x01
> +#define NRF51_TIMER_REG_BITMODE 0x508
> +#define NRF51_TIMER_REG_BITMODE_MASK 0x03
> +#define NRF51_TIMER_REG_PRESCALER 0x510
> +#define NRF51_TIMER_REG_PRESCALER_MASK 0x0F
> +#define NRF51_TIMER_REG_CC0 0x540
> +#define NRF51_TIMER_REG_CC3 0x54C
> +
> +typedef struct NRF51TimerState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +
> +    QEMUTimer timer;
> +
> +    bool running;
> +
> +    uint64_t time_offset;
> +    uint64_t last_visited;
> +
> +    uint8_t events_compare[NRF51_TIMER_REG_COUNT];
> +    uint32_t cc[NRF51_TIMER_REG_COUNT];
> +    uint32_t cc_sorted[NRF51_TIMER_REG_COUNT];
> +    uint32_t shorts;
> +    uint32_t inten;
> +    uint32_t mode;
> +    uint32_t bitmode;
> +    uint32_t prescaler;
> +} NRF51TimerState;
> +
> +
> +#endif
> --
> 2.19.1

thanks
-- PMM