On Tue, Jan 05, 2016 at 05:33:29AM +0300, Dmitry Osipenko wrote:
> Current ARM MPTimer implementation uses QEMUTimer for the actual timer,
> this implementation isn't complete and mostly tries to duplicate of what
> generic ptimer is already doing fine.
>
> Conversion to ptimer brings the following benefits and fixes:
> - Simple timer pausing implementation
> - Fixes counter value preservation after stopping the timer
> - Code simplification and reduction
>
> Bump VMSD to version 3, since VMState is changed and is not compatible
> with the previous implementation.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
> hw/timer/arm_mptimer.c | 110
> ++++++++++++++++++-----------------------
> include/hw/timer/arm_mptimer.h | 4 +-
> 2 files changed, 49 insertions(+), 65 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 3e59c2a..c06da5e 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -19,8 +19,9 @@
> * with this program; if not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include "hw/ptimer.h"
> #include "hw/timer/arm_mptimer.h"
> -#include "qemu/timer.h"
> +#include "qemu/main-loop.h"
> #include "qom/cpu.h"
>
> /* This device implements the per-cpu private timer and watchdog block
> @@ -47,28 +48,10 @@ static inline uint32_t timerblock_scale(TimerBlock *tb)
> return (((tb->control >> 8) & 0xff) + 1) * 10;
> }
>
> -static void timerblock_reload(TimerBlock *tb, int restart)
> -{
> - if (tb->count == 0) {
> - return;
> - }
> - if (restart) {
> - tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - }
> - tb->tick += (int64_t)tb->count * timerblock_scale(tb);
> - timer_mod(tb->timer, tb->tick);
> -}
> -
> static void timerblock_tick(void *opaque)
> {
> TimerBlock *tb = (TimerBlock *)opaque;
> tb->status = 1;
> - if (tb->control & 2) {
> - tb->count = tb->load;
> - timerblock_reload(tb, 0);
> - } else {
> - tb->count = 0;
> - }
> timerblock_update_irq(tb);
> }
>
> @@ -76,21 +59,11 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
> unsigned size)
> {
> TimerBlock *tb = (TimerBlock *)opaque;
> - int64_t val;
> switch (addr) {
> case 0: /* Load */
> return tb->load;
> case 4: /* Counter. */
> - if (((tb->control & 1) == 0) || (tb->count == 0)) {
> - return 0;
> - }
> - /* Slow and ugly, but hopefully won't happen too often. */
> - val = tb->tick - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - val /= timerblock_scale(tb);
> - if (val < 0) {
> - val = 0;
> - }
> - return val;
> + return ptimer_get_count(tb->timer);
> case 8: /* Control. */
> return tb->control;
> case 12: /* Interrupt status. */
> @@ -100,6 +73,19 @@ static uint64_t timerblock_read(void *opaque, hwaddr addr,
> }
> }
>
> +static void timerblock_run(TimerBlock *tb, uint64_t count, int set_count)
> +{
> + if (set_count) {
> + if (((tb->control & 3) == 3) && (count == 0)) {
Parentheses around == expressions should not be needed.
> + count = tb->load;
> + }
> + ptimer_set_count(tb->timer, count);
> + }
> + if ((tb->control & 1) && (count != 0)) {
This can check against tb->load instead of count to avoid dummy
pass of tb->load to this fn ...
> + ptimer_run(tb->timer, !(tb->control & 2));
> + }
> +}
> +
> static void timerblock_write(void *opaque, hwaddr addr,
> uint64_t value, unsigned size)
> {
> @@ -108,32 +94,34 @@ static void timerblock_write(void *opaque, hwaddr addr,
> switch (addr) {
> case 0: /* Load */
> tb->load = value;
> - /* Fall through. */
> - case 4: /* Counter. */
> - if ((tb->control & 1) && tb->count) {
> - /* Cancel the previous timer. */
> - timer_del(tb->timer);
> + /* Setting load to 0 stops the timer. */
> + if (tb->load == 0) {
> + ptimer_stop(tb->timer);
> }
> - tb->count = value;
> - if (tb->control & 1) {
> - timerblock_reload(tb, 1);
> + ptimer_set_limit(tb->timer, tb->load, 1);
> + timerblock_run(tb, tb->load, 0);
> + break;
> + case 4: /* Counter. */
> + /* Setting counter to 0 stops the one-shot timer. */
> + if (!(tb->control & 2) && (value == 0)) {
> + ptimer_stop(tb->timer);
> }
> + timerblock_run(tb, value, 1);
... then this would just need to be elsed.
> break;
> case 8: /* Control. */
> old = tb->control;
> tb->control = value;
> - if (value & 1) {
> - if ((old & 1) && (tb->count != 0)) {
> - /* Do nothing if timer is ticking right now. */
> - break;
> - }
> - if (tb->control & 2) {
> - tb->count = tb->load;
> - }
> - timerblock_reload(tb, 1);
> - } else if (old & 1) {
> - /* Shutdown the timer. */
> - timer_del(tb->timer);
> + /* Timer mode switch requires ptimer to be stopped. */
Is it worth adding this to ptimer? It seems ptimer can now handle
every other case of running configuration change except this one
case.
> + if ((old & 3) != (tb->control & 3)) {
> + ptimer_stop(tb->timer);
> + }
> + if (!(tb->control & 1)) {
> + break;
> + }
> + ptimer_set_period(tb->timer, timerblock_scale(tb));
> + if ((old & 3) != (tb->control & 3)) {
> + value = ptimer_get_count(tb->timer);
> + timerblock_run(tb, value, 1);
Is this reachable when the load value is still 0?
Following on from the suggested refactor before, I think timerblock_run
should split off the count set to a new helper. Then this is
timerblock_setcount(tb, value);
timerblock_run(tb);
and the boolean arg and dummy pass of tb->load as count are unneeded.
Regards,
Peter
> }
> break;
> case 12: /* Interrupt status. */
> @@ -184,13 +172,12 @@ static const MemoryRegionOps timerblock_ops = {
>
> static void timerblock_reset(TimerBlock *tb)
> {
> - tb->count = 0;
> tb->load = 0;
> tb->control = 0;
> tb->status = 0;
> - tb->tick = 0;
> if (tb->timer) {
> - timer_del(tb->timer);
> + ptimer_stop(tb->timer);
> + ptimer_set_limit(tb->timer, 0, 1);
> }
> }
>
> @@ -235,7 +222,8 @@ static void arm_mptimer_realize(DeviceState *dev, Error
> **errp)
> */
> for (i = 0; i < s->num_cpu; i++) {
> TimerBlock *tb = &s->timerblock[i];
> - tb->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, timerblock_tick, tb);
> + QEMUBH *bh = qemu_bh_new(timerblock_tick, tb);
> + tb->timer = ptimer_init(bh);
> sysbus_init_irq(sbd, &tb->irq);
> memory_region_init_io(&tb->iomem, OBJECT(s), &timerblock_ops, tb,
> "arm_mptimer_timerblock", 0x20);
> @@ -245,26 +233,24 @@ static void arm_mptimer_realize(DeviceState *dev, Error
> **errp)
>
> static const VMStateDescription vmstate_timerblock = {
> .name = "arm_mptimer_timerblock",
> - .version_id = 2,
> - .minimum_version_id = 2,
> + .version_id = 3,
> + .minimum_version_id = 3,
> .fields = (VMStateField[]) {
> - VMSTATE_UINT32(count, TimerBlock),
> VMSTATE_UINT32(load, TimerBlock),
> VMSTATE_UINT32(control, TimerBlock),
> VMSTATE_UINT32(status, TimerBlock),
> - VMSTATE_INT64(tick, TimerBlock),
> - VMSTATE_TIMER_PTR(timer, TimerBlock),
> + VMSTATE_PTIMER(timer, TimerBlock),
> VMSTATE_END_OF_LIST()
> }
> };
>
> static const VMStateDescription vmstate_arm_mptimer = {
> .name = "arm_mptimer",
> - .version_id = 2,
> - .minimum_version_id = 2,
> + .version_id = 3,
> + .minimum_version_id = 3,
> .fields = (VMStateField[]) {
> VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
> - 2, vmstate_timerblock, TimerBlock),
> + 3, vmstate_timerblock, TimerBlock),
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
> index b34cba0..93db61b 100644
> --- a/include/hw/timer/arm_mptimer.h
> +++ b/include/hw/timer/arm_mptimer.h
> @@ -27,12 +27,10 @@
>
> /* State of a single timer or watchdog block */
> typedef struct {
> - uint32_t count;
> uint32_t load;
> uint32_t control;
> uint32_t status;
> - int64_t tick;
> - QEMUTimer *timer;
> + struct ptimer_state *timer;
> qemu_irq irq;
> MemoryRegion iomem;
> } TimerBlock;
> --
> 2.6.4
>
