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

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

[Peter Maydell]

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 :-)
---
 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

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 <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

Thanks for the details!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   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 :-)
> ---
>   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)) {
> 

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

[Guenter Roeck]

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
>