[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reloa
From: |
Andrew Jeffery |
Subject: |
Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set |
Date: |
Tue, 13 Jun 2017 14:07:56 +0930 |
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.
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.
signature.asc
Description: This is a digitally signed message part