Re: [PATCH] hw/timer/exynos4210_mct: Initialize ptimer before starting it

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Oct 18, 2019 at 03:31:49PM +0100, Peter Maydell wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> 
> When booting a recent Linux kernel, the qemu message "Timer with delta
> zero, disabling" is seen, apparently because a ptimer is started before
> being initialized.  Fix the problem by initializing the offending ptimer
> before starting it.
> 
> The bug is effectively harmless in the old QEMUBH setup
> because the sequence of events is:
>  * the delta zero means the timer expires immediately
>  * ptimer_reload() arranges for exynos4210_gfrc_event() to be called
>  * ptimer_reload() notices the zero delta and disables the timer
>  * later, the QEMUBH runs, and exynos4210_gfrc_event() correctly
>    configures the timer and restarts it
> 
> In the new transaction based API the bug is still harmless,
> but differences of when the callback function runs mean the
> message is not printed any more:
>  * ptimer_run() does nothing as it's inside a transaction block
>  * ptimer_transaction_commit() sees it has work to do and
>    calls ptimer_reload()
>  * the zero delta means the timer expires immediately
>  * ptimer_reload() calls exynos4210_gfrc_event() directly
>  * exynos4210_gfrc_event() configures the timer
>  * the delta is no longer zero so ptimer_reload() doesn't complain
>    (the zero-delta test is after the trigger-callback in
>    the ptimer_reload() function)
> 
> Regardless, the behaviour here was not intentional, and we should
> just program the ptimer correctly to start with.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> [PMM: Expansion/clarification of the commit message:
>  the message is about a zero delta, not a zero period;
>  added detail to the commit message of the analysis of what
>  is happening and why the kernel boots even with the message;
>  added note that the message goes away with the new ptimer API]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Philippe pointed me at this bugfix from Guenter. At one point
> in my working on the ptimer API changes I thought this bugfix
> would be necessary as a prerequisite, but in fact the issue
> was in my ptimer changes, and it just happened that fixing
> the MCT bug was a workaround for my bug. Even though the
> ptimer API changes actually coincidentally now suppress the
> annoying message about a zero delta, the behaviour is definitely
> not intentional, and since I spent the time working through the
> analysis of what was actually going on here I don't want
> to waste it :-)
> ---

Thanks a lot for picking this up, and for the great analysis!

Guenter

>  hw/timer/exynos4210_mct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 72257584145..944120aea59 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -1254,7 +1254,7 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
>          /* Start FRC if transition from disabled to enabled */
>          if ((value & G_TCON_TIMER_ENABLE) > (old_val &
>                  G_TCON_TIMER_ENABLE)) {
> -            exynos4210_gfrc_start(&s->g_timer);
> +            exynos4210_gfrc_restart(s);
>          }
>          if ((value & G_TCON_TIMER_ENABLE) < (old_val &
>                  G_TCON_TIMER_ENABLE)) {
> -- 
> 2.20.1
>