Re: [Qemu-arm] [PATCH v16 05/16] hw/ptimer: Add "wraparound after one period" policy

[Dmitry Osipenko <digetx@gmail.com>]

On 20.09.2016 20:20, Peter Maydell wrote:
> On 7 September 2016 at 14:22, Dmitry Osipenko <digetx@gmail.com> wrote:
>> Currently, periodic counter wraps around immediately once counter reaches
>> "0", this is wrong behaviour for some of the timers, resulting in one period
>> being lost. Add new ptimer policy that provides correct behaviour for such
>> timers, so that counter stays with "0" for a one period before wrapping
>> around.
> 
> This says it's just adding a new policy...
> 
>> @@ -91,7 +96,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>  {
>>      uint64_t counter;
>>
>> -    if (s->enabled) {
>> +    if (s->enabled && s->delta != 0) {
>>          int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>          int64_t next = s->next_event;
>>          bool expired = (now - next >= 0);
>> @@ -145,6 +150,14 @@ uint64_t ptimer_get_count(ptimer_state *s)
>>                      div += 1;
>>              }
>>              counter = rem / div;
>> +
>> +            if (!oneshot && s->delta == s->limit) {
>> +                /* Before wrapping around, timer should stay with counter = 0
>> +                   for a one period.  The delta has been adjusted by +1 for
>> +                   the wrapped around counter, so taking a modulo of limit + 1
>> +                   gives that period.  */
>> +                counter %= s->limit + 1;
>> +            }
>>          }
>>      } else {
>>          counter = s->delta;
> 
> ...but the changes in this function look like they affect
> behaviour even if that policy flag isn't set.
> 

No, the delta value is adjusted only when PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD is
set.

+    if (s->policy_mask & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) {
+        delta += delta_adjust;
+    }
+

That adjustment is applied only on reload of the periodic timer.

@@ -83,7 +88,7 @@ static void ptimer_tick(void *opaque)
     if (s->enabled == 2) {
         s->enabled = 0;
     } else {
-        ptimer_reload(s);
+        ptimer_reload(s, 1);
     }
 }

All other ptimer_reload's are ptimer_reload(s, 0).

The periodic timer is reloaded with the limit value. When s->delta == s->limit,
we can assume that timer is free running. When delta is adjusted by +1 on
reload, the counter = limit + 1 (counter value is calculated based on elapsed
QEMU time) and that "counter %= s->limit + 1" modulo gives counter = 0 while in
fact the counter = adjusted delta (limit + 1).

When delta is *not* adjusted, the modulo returns the actual counter value, since
counter value is always less than s->limit + 1.

So, this patch doesn't affect old behaviour at all.

Looking more at it now, I think "counter %= s->limit + 1" should be changed to
"counter %= s->limit" in this patch and +1 added in the "no counter round down"
patch, since the counter value is always rounded down here. Instead of the
"staying with counter = 0 for a one period" it would wraparound to the limit
value if "wraparound after one period" policy is used. I'll change it in V17.

-- 
Dmitry