Messages in this thread | | | Date | Thu, 7 Feb 2013 17:10:24 +0530 | From | Santosh Shilimkar <> | Subject | Re: [PATCHv3 4/4] arm: Add generic timer broadcast support |
| |
Mark,
On Thursday 07 February 2013 03:34 PM, Mark Rutland wrote: > Hi Stephen, > > Sorry about this; I'm to blame for the bug. > > On Wed, Feb 06, 2013 at 08:51:43PM +0000, Stephen Warren wrote: >> On 01/14/2013 10:05 AM, Mark Rutland wrote: >>> Implement timer_broadcast for the arm architecture, allowing for the use >>> of clock_event_device_drivers decoupled from the timer tick broadcast >>> mechanism. >> >> Mark, this patch is now in next-20130206 and causes a crash during boot >> on Tegra. The reason appears to be because of: >> >>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c >> >>> @@ -524,7 +524,6 @@ static void __cpuinit percpu_timer_setup(void) >>> struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu); >>> >>> evt->cpumask = cpumask_of(cpu); >>> - evt->broadcast = smp_timer_broadcast; >> >> After that change, evt->broadcast is never assigned, and hence is NULL. >> Yet elsewhere in kernel/time/tick-broadcast.c it's used unconditionally: >> >> static void tick_do_broadcast(struct cpumask *mask) >> ... >> if (!cpumask_empty(mask)) { >> ... >> td = &per_cpu(tick_cpu_device, cpumask_first(mask)); >> td->evtdev->broadcast(mask); >> >> Now perhaps the Tegra timer driver simply isn't being set up correctly, >> so the bug is there... But the only other place I can find where >> ->broadcast is assigned is in tick_device_uses_broadcast() which only >> does it for "non-functional" timers, which doesn't apply to Tegra's timer. >> > > The intent of 12ad100046: "clockevents: Add generic timer broadcast function" > was to setup the broadcast function both for non-functional/dummy timers and > those that stop in low-power states (CLOCK_EVT_FEAT_C3STOP). I missed the > CLOCK_EVT_FEAT_C3STOP case. > I am not sure this exactly the case. Because in my testing, the C3STOP path was exercised already. And if the C3STOP is used then notifiers calls are expected for switching of clock-events to broadcast mode.
And dummy broad-cast hook should come into picture only if the per CPU local timer clock-event are not registered.
I just tried "next-20130207" on OMAP and I see the broad-cast mechanism works and didn't observe any crash.
------------------------------ Tick Device: mode: 1 Broadcast device Clock Event Device: gp_timer max_delta_ns: 131071523464981 min_delta_ns: 91552 mult: 70369 shift: 31 mode: 3 next_event: 89984375000 nsecs set_next_event: omap2_gp_timer_set_next_event set_mode: omap2_gp_timer_set_mode event_handler: tick_handle_oneshot_broadcast retries: 0
tick_broadcast_mask: 00000003 tick_broadcast_oneshot_mask: 00000000
Tick Device: mode: 1 Per CPU device: 0 Clock Event Device: local_timer max_delta_ns: 10737418240 min_delta_ns: 1000 mult: 858993459 shift: 31 mode: 3 next_event: 125250000000 nsecs set_next_event: twd_set_next_event set_mode: twd_set_mode event_handler: hrtimer_interrupt retries: 346
Tick Device: mode: 1 Per CPU device: 1 Clock Event Device: local_timer max_delta_ns: 10737418240 min_delta_ns: 1000 mult: 858993459 shift: 31 mode: 3 next_event: 89921875000 nsecs set_next_event: twd_set_next_event set_mode: twd_set_mode event_handler: hrtimer_interrupt retries: 258
#
------------------------------
> I believe the patch below will fix this for Tegra and any other platforms where > broadcast is required in low power states. > Am not sure you really need that patch unless and until am missing a scenario in my test.
Stephan,
Do we have crash log seen on Tegra ? Which tegra CPUIDLE driver is being used when crash is seen ?
Regards Santosh
| |