Re: [Qemu-devel] [PATCH] hw/char/cmsdk-apb-timer: Correctly identify and set one-shot mode

[Guenter Roeck]

On Tue, Jun 26, 2018 at 06:17:44PM +0100, Peter Maydell wrote:
> On 19 June 2018 at 03:08, Guenter Roeck <address@hidden> wrote:
> > The CMSDK APB timer is currently always configured as periodic timer.
> > This results in the following messages when trying to boot Linux.
> >
> > Timer with delta zero, disabling
> >
> > If the timer limit set with the RELOAD command is 0, the timer
> > needs to be enabled as one-shot timer.
> >
> > Signed-off-by: Guenter Roeck <address@hidden>
> > ---
> >  hw/timer/cmsdk-apb-timer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/timer/cmsdk-apb-timer.c b/hw/timer/cmsdk-apb-timer.c
> > index 9878746..79c1b49 100644
> > --- a/hw/timer/cmsdk-apb-timer.c
> > +++ b/hw/timer/cmsdk-apb-timer.c
> > @@ -119,7 +119,7 @@ static void cmsdk_apb_timer_write(void *opaque, hwaddr 
> > offset, uint64_t value,
> >          }
> >          s->ctrl = value & 0xf;
> >          if (s->ctrl & R_CTRL_EN_MASK) {
> > -            ptimer_run(s->timer, 0);
> > +            ptimer_run(s->timer, ptimer_get_limit(s->timer) == 0);
> >          } else {
> >              ptimer_stop(s->timer);
> >          }
> > --
> > 2.7.4
> 
> Thanks for this patch. I was wondering whether it would be better
> just to remove the fprintf message instead. I'll either apply
> this or send a patch to do that before 3.0, anyway.
> 

If I recall correctly, I tried that, and it did not help.
The messages don't happen too often, and the message itself
does not cause a problem. Issue is that the interrupts happen
at the wrong time or not at all (after a while, ie after the
configured one-shot time expires), and the kernel really doesn't
like that.

I think the underlying problem was that the periodic timer counts
the period down (based on the time set for the one-shot timer),
stops with the "Timer with delta zero, disabling" message once
the period reaches 0, and does not fire anymore afterwards.
As a result, the kernel fails to boot maybe 90% of the time.
I should probably have mentioned that in more detail in the
commit log.

> I think we also want to make sure we convert back to a
> periodic timer if the reload register is written with a nonzero
> value before the timer expires (and that if that happens after
> the timer expired that we restart the timer). Thinking about
> this is also on my todo list.
> 

I thought that was handled in the Linux driver by disabling
the timer, updating the period, and then re-enabling it in
periodic mode, but I may have misinterpreted the code.

Guenter