From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49720) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHBkf-0001UY-L3 for qemu-devel@nongnu.org; Thu, 07 Jan 2016 09:41:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHBke-0002c3-AW for qemu-devel@nongnu.org; Thu, 07 Jan 2016 09:41:21 -0500 References: <20160106131744.GE4227@pcrost-box> From: Dmitry Osipenko Message-ID: <568E78F2.4010200@gmail.com> Date: Thu, 7 Jan 2016 17:40:50 +0300 MIME-Version: 1.0 In-Reply-To: <20160106131744.GE4227@pcrost-box> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v8 4/4] arm_mptimer: Convert to use ptimer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , qemu-arm@nongnu.org, QEMU Developers 06.01.2016 16:17, Peter Crosthwaite пишет: > 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 >> --- >> 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 . >> */ >> >> +#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. > Yeah, should make code cleaner as well. >> + 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. > What about to just inline timerblock_run? It would allow compiler to eliminate dead code. -- Dmitry