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 <c...@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 ?
>> +
>> 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 <and...@aj.id.au>
Thanks,
C.
