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

[Philippe Mathieu-Daudé]

On 10/18/19 4:31 PM, Peter Maydell wrote:
> From: Guenter Roeck <address@hidden>
>
> 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 <address@hidden>
> [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

Thanks for the details!

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>

>   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 <address@hidden>
> ---
> 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 :-)
> ---
>   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)) {