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

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:

On Tue, Jan 25, 2011 at 03:12:24PM +0100, Thomas Gleixner wrote:

quoted

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:

quoted

On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:

quoted

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