[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/timer/exynos4210_mct: Initialize ptimer before starting i
From: |
Guenter Roeck |
Subject: |
Re: [PATCH] hw/timer/exynos4210_mct: Initialize ptimer before starting it |
Date: |
Fri, 18 Oct 2019 11:18:04 -0700 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
On Fri, Oct 18, 2019 at 03:31:49PM +0100, 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
> 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 :-)
> ---
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
>