Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set

["Cédric Le Goater" <clg@kaod.org>]

On 06/13/2017 06:37 AM, Andrew Jeffery wrote:
> On Fri, 2017-06-09 at 07:40 +0200, Cédric Le Goater wrote:
>> On 06/09/2017 04:26 AM, Andrew Jeffery wrote:
>>> On Tue, 2017-06-06 at 10:55 +0200, Cédric Le Goater wrote:
>>>> When a timer is enabled before a reload value is set, the controller
>>>> waits for a reload value to be set before starting decrementing. This
>>>> fix tries to cover that case by changing the timer expiry only when
>>>> a reload value is valid.
>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> ---
>>>>  hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
>>>>  1 file changed, 29 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>>> index 9b70ee09b07f..50acbf530a3a 100644
>>>> --- a/hw/timer/aspeed_timer.c
>>>> +++ b/hw/timer/aspeed_timer.c
>>>> @@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer *t)
>>>>              next = seq[1];
>>>>          } else if (now < seq[2]) {
>>>>              next = seq[2];
>>>> -        } else {
>>>> +        } else if (t->reload) {
>>>>              reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
>>>>              t->start = now - ((now - t->start) % reload_ns);
>>>> +        } else {
>>>> +            /* no reload value, return 0 */
>>>> +            break;
>>>>          }
>>>>      }
>>>>  
>>>>      return next;
>>>>  }
>>>>  
>>>> +static void aspeed_timer_mod(AspeedTimer *t)
>>>> +{
>>>> +    uint64_t next = calculate_next(t);
>>>> +    if (next) {
>>>> +        timer_mod(&t->timer, next);
>>>> +    }
>>>> +}
>>>> +
>>>>  static void aspeed_timer_expire(void *opaque)
>>>>  {
>>>>      AspeedTimer *t = opaque;
>>>> @@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque)
>>>>          qemu_set_irq(t->irq, t->level);
>>>>      }
>>>>  
>>>> -    timer_mod(&t->timer, calculate_next(t));
>>>> +    aspeed_timer_mod(t);
>>>>  }
>>>>  
>>>>  static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
>>>> @@ -227,10 +238,23 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>>>>                                     uint32_t value)
>>>>  {
>>>>      AspeedTimer *t;
>>>> +    uint32_t old_reload;
>>>>  
>>>>      trace_aspeed_timer_set_value(timer, reg, value);
>>>>      t = &s->timers[timer];
>>>>      switch (reg) {
>>>> +    case TIMER_REG_RELOAD:
>>>> +        old_reload = t->reload;
>>>> +        t->reload = value;
>>>> +
>>>> +        /* If the reload value was not previously set, or zero, and
>>>> +         * the current value is valid, try to start the timer if it is
>>>> +         * enabled.
>>>> +         */
>>>> +        if (old_reload || !t->reload) {
>>>> +            break;
>>>> +        }
>>>
>>> Maybe I need more caffeine, but I initially struggled to reconcile the
>>> condition with the comment, as the condition checks the inverse in
>>> order to break while the comment discusses the non-breaking case. 
>>
>> I agree. The reload "value" is used in a hidden way to the activate the 
>> timer.
>>
>>> However, after trying for several minutes, I'm not sure there's an easy
>>> way to improve it.
>>
>> I tried a few things. May be, we could move the following code in 
>> its own routine and call it twice ? 
> 
> I don't think it's necessary. The comment serves as enough warning - it
> should at least make people think before modifying the code.

OK. Let it be.


Peter, 

The Moxa Art timer driver was recently merged into the Faraday 
FTTMR010 driver and the initial setup is slightly different, 
it enables the timer before setting the reload value, which 
breaks the current QEMU model.

Thanks,

C. 


> Cheers,
> 
> Andrew
> 
>>  
>>>> +
>>>>      case TIMER_REG_STATUS:
>>>>          if (timer_enabled(t)) {
>>>>              uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> @@ -238,17 +262,14 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>>>>              uint32_t rate = calculate_rate(t);
>>>>  
>>>>              t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
>>>> -            timer_mod(&t->timer, calculate_next(t));
>>>> +            aspeed_timer_mod(t);
>>>>          }
>>>>          break;
>>>> -    case TIMER_REG_RELOAD:
>>>> -        t->reload = value;
>>>> -        break;
>>>>      case TIMER_REG_MATCH_FIRST:
>>>>      case TIMER_REG_MATCH_SECOND:
>>>>          t->match[reg - 2] = value;
>>>>          if (timer_enabled(t)) {
>>>> -            timer_mod(&t->timer, calculate_next(t));
>>>> +            aspeed_timer_mod(t);
>>>>          }
>>>>          break;
>>>>      default:
>>>> @@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
>>>>      trace_aspeed_timer_ctrl_enable(t->id, enable);
>>>>      if (enable) {
>>>>          t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> -        timer_mod(&t->timer, calculate_next(t));
>>>> +        aspeed_timer_mod(t);
>>>>      } else {
>>>>          timer_del(&t->timer);
>>>>      }
>>>
>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>
>> Thanks,
>>
>> C.