Re: [PATCH 3/5] ARM: twd: Add context save restore support

Thomas Gleixner
2011-01-25T14:25:08Z

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 03:12:24PM +0100, Thomas Gleixner wrote:
> > On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
> > > On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
> > > > In which way? I mean the generic code issues a call to the set_mode
> > > > function when we leave the broadcast mode. So what should the generic
> > > > code do more ?
> > >
> > > I can't say because these patches only add the hooks, there's no
> > > implementation yet which uses the hooks.
> > >
> > > Given the description about _why_ those hooks are necessary, it seems
> > > that something is required. Either we start adding custom hacks to
> > > each clockevent driver as is done with this patch, or we get some
> > > generic help in place.
> > >
> > > I'm not thrilled by the custom hack approach - and I thought the
> > > clockevent stuff was created to stop this kind of thing happening.
> >
> > Yes, and it does the right thing:
> >
> > idle enter (where the cpu local tick device stops)
> >
> > tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_ENTER)
> >
> > clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
> > tick_broadcast_set_event(dev->next_event, 1)
> >
> > idle exit
> >
> > tick_broadcast_oneshot_control(CLOCK_EVT_NOTIFY_BROADCAST_EXIT)
> >
> > clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)
> > tick_program_event(dev->next_event, 1)
> >
> > So the generic code has all the calls in place. If a clock chip
> > implementation misses to set control registers in the
> > CLOCK_EVT_MODE_ONESHOT case, then it's not a short coming of the
> > generic code which needs magic hooks in the arch code.
> >
> > The same applies for the periodic mode switch, which is handled via
> > tick_broadcast_on_off().
> >
> > Am I missing something ?
>
> I quote Colin:
>
> | This doesn't work for oneshot timers if the TWD_TIMER_CONTROL register
> | gets reset by cpu idle between twd_set_mode and twd_set_next_event.

And I fear that he means the twd_set_mode(ONESHOT) call which happens
during boot/cpuonline.

The point is that the above function needs to be called from the cpu
idle code to notify the clock events layer. The correct mechanism is
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT). The
existing callers are in:

arch/x86/kernel/process.c
drivers/acpi/acpi_pad.c
drivers/acpi/processor_idle.c
drivers/idle/intel_idle.c

So i'm not surprised, that this wont work on ARM :)

Thanks,

tglx