Re: [Qemu-devel] [PATCH v8 4/4] arm_mptimer: Convert to use ptimer

[Dmitry Osipenko <digetx@gmail.com>]

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 <digetx@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.
>

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