[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer |
Date: |
Sat, 23 Jan 2016 21:25:30 -0800 |
On Thu, Jan 21, 2016 at 11:03 AM, Dmitry Osipenko <address@hidden> 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
> - Correctly handles prescaler != 0 cases
> - 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 <address@hidden>
> ---
> hw/timer/arm_mptimer.c | 131
> +++++++++++++++++++++--------------------
> include/hw/timer/arm_mptimer.h | 5 +-
> 2 files changed, 67 insertions(+), 69 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 3e59c2a..a5f46df 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
> @@ -42,33 +43,34 @@ static inline void timerblock_update_irq(TimerBlock *tb)
> }
>
> /* Return conversion factor from mpcore timer ticks to qemu timer ticks. */
> -static inline uint32_t timerblock_scale(TimerBlock *tb)
> +static inline uint32_t timerblock_scale(uint32_t control)
> {
> - return (((tb->control >> 8) & 0xff) + 1) * 10;
> + return (((control >> 8) & 0xff) + 1) * 10;
> }
>
> -static void timerblock_reload(TimerBlock *tb, int restart)
> +static inline void timerblock_set_count(struct ptimer_state *timer,
> + uint32_t control, uint64_t *count)
> {
> - if (tb->count == 0) {
> - return;
> + /* PTimer would immediately trigger interrupt for periodic timer
> + * when counter set to 0, MPtimer under certain condition only. */
newline before */
> + if ((control & 3) == 3 && (*count == 0) && (control & 0xff00) == 0) {
> + *count = ptimer_get_limit(timer);
> }
> - if (restart) {
> - tb->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + ptimer_set_count(timer, *count);
> +}
> +
> +static inline void timerblock_run(struct ptimer_state *timer, uint32_t
> control,
> + bool cond)
drop cond ...
> +{
> + if (cond) {
... control & 1 ...
> + ptimer_run(timer, !(control & 2));
> }
> - 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 +78,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;
> + return ptimer_get_limit(tb->timer);
> 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. */
> @@ -104,39 +96,51 @@ static void timerblock_write(void *opaque, hwaddr addr,
> uint64_t value, unsigned size)
> {
> TimerBlock *tb = (TimerBlock *)opaque;
> - int64_t old;
> + uint32_t control = tb->control;
> 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 prescaler == 0. */
> + if ((control & 1) && (value == 0) && (control & 0xff00) == 0) {
> + ptimer_stop(tb->timer);
> + control &= ~1;
> }
> - tb->count = value;
> - if (tb->control & 1) {
> - timerblock_reload(tb, 1);
> + ptimer_set_limit(tb->timer, value, 1);
> + timerblock_run(tb->timer, control, (control & 1));
> + break;
> + case 4: /* Counter. */
> + /* Setting counter to 0 stops the one-shot timer if prescaler == 0
> + * and periodic if load = 0. */
newline before */
> + if ((control & 1) && (value == 0) && (control & 0xff00) == 0) {
> + if (!(control & 2) || ptimer_get_limit(tb->timer) == 0) {
comment doesn't look to match code. The code looks like:
"setting counter to 0 while prescaler == 0 will disable the timer if
it is either oneshot, or periodic with load == 0",
whearas your comment read as if it only applies to one-shot timers.
ifs can be merged.
> + ptimer_stop(tb->timer);
> + control &= ~1;
> + }
> }
> + timerblock_set_count(tb->timer, control, &value);
... and just add in the extra if in here with if (value != 0). The
boolean cond argument that just conditionalizes the whole thing is a
bit un-usual and the three call sites all pass in (control & 1) in
addition to the full raw control.
> + timerblock_run(tb->timer, control, (value != 0) && (control & 1));
> 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 ((value & 1) && (control & 3) != (value & 3)) {
> + uint64_t count = (value & 0xff00) ? 1 :
> ptimer_get_count(tb->timer);
> + if ((count == 0) && (value & 2)) {
> + timerblock_set_count(tb->timer, value, &count);
This looks like a weird corner-case, what does it do exactly? I can't
follow it so it needs a comment :)
Regards,
Peter
> }
> - if (tb->control & 2) {
> - tb->count = tb->load;
> - }
> - timerblock_reload(tb, 1);
> - } else if (old & 1) {
> - /* Shutdown the timer. */
> - timer_del(tb->timer);
> + timerblock_run(tb->timer, value, (count != 0));
> + } else if ((control & 1) && !(value & 1)) {
> + ptimer_stop(tb->timer);
> + }
> + if ((control & 0xff00) != (value & 0xff00)) {
> + ptimer_set_period(tb->timer, timerblock_scale(value));
> }
> + tb->control = value;
> break;
> case 12: /* Interrupt status. */
> + /* Simulate continuous IRQ gen. */
> + if ((control & 3) == 3 && (control & 0xff00) != 0) {
> + if (ptimer_get_limit(tb->timer) == 0) {
> + break;
> + }
> + }
> tb->status &= ~value;
> timerblock_update_irq(tb);
> break;
> @@ -184,13 +188,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);
> + ptimer_set_period(tb->timer, timerblock_scale(0));
> }
> }
>
> @@ -235,7 +238,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 +249,23 @@ 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..c46d8d2 100644
> --- a/include/hw/timer/arm_mptimer.h
> +++ b/include/hw/timer/arm_mptimer.h
> @@ -27,12 +27,9 @@
>
> /* 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.7.0
>
- [Qemu-devel] [PATCH v11 3/7] hw/ptimer: Update .delta on period/freq change, (continued)
- [Qemu-devel] [PATCH v11 3/7] hw/ptimer: Update .delta on period/freq change, Dmitry Osipenko, 2016/01/21
- [Qemu-devel] [PATCH v11 1/7] hw/ptimer: Fix issues caused by the adjusted timer limit value, Dmitry Osipenko, 2016/01/21
- [Qemu-devel] [PATCH v11 4/7] hw/ptimer: Support "on the fly" timer mode switch, Dmitry Osipenko, 2016/01/21
- [Qemu-devel] [PATCH v11 2/7] hw/ptimer: Perform counter wrap around if timer already expired, Dmitry Osipenko, 2016/01/21
- [Qemu-devel] [PATCH v11 6/7] hw/ptimer: Legalize running with delta = load = 0 and abort on period = 0, Dmitry Osipenko, 2016/01/21
- [Qemu-devel] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer, Dmitry Osipenko, 2016/01/21
- Re: [Qemu-devel] [PATCH v11 7/7] arm_mptimer: Convert to use ptimer,
Peter Crosthwaite <=
- [Qemu-devel] [PATCH v11 5/7] hw/ptimer: Introduce ptimer_get_limit, Dmitry Osipenko, 2016/01/21