qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH] timer/aspeed: fix timer enablement when a reload


From: Cédric Le Goater
Subject: Re: [Qemu-arm] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
Date: Tue, 13 Jun 2017 07:28:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

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 <address@hidden>
>>>>
>>>> ---
>>>>  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 <address@hidden>
>>
>> Thanks,
>>
>> C.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]