Message-ID: <aec7e5c30811252121i23a719fcl19b4db4af785dd46@mail.gmail.com>
Date:	Wed, 26 Nov 2008 14:21:14 +0900
From:	"Magnus Damm" <magnus.damm@...il.com>
To:	"Ingo Molnar" <mingo@...e.hu>
Cc:	"john stultz" <johnstul@...ibm.com>,
	"Thomas Gleixner" <tglx@...utronix.de>,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	lethal@...ux-sh.org, mingo@...hat.com
Subject: Re: [RFC] Reentrant clock sources

Hi guys,

Thanks for all the feedback so far.

On Wed, Nov 26, 2008 at 12:52 PM, Ingo Molnar <mingo@...e.hu> wrote:
>
> * john stultz <johnstul@...ibm.com> wrote:
>
>> On Wed, 2008-11-26 at 04:07 +0100, Ingo Molnar wrote:
>> > * Thomas Gleixner <tglx@...utronix.de> wrote:
>> >
>> > > > +       cycle_t (*vread)(struct clocksource *cs);
>> > >
>> > > This is crap. vread can not access the clocksource.
>> >
>> > i think 'reentrant' in the sense of creating self-sufficient driver
>> > entities. vread wont (and shouldnt) call ->vread() recursively - but
>> > it might want to access fields on the clocksource.

Uhm, I should have used that wording instead. =)

>> I think Thomas' issue is that vread() *cannot* access fields on the
>> clocksource (since vread has to be careful not to access any
>> non-vsyscall mapped memory).
>
> ah, yeah - i was thinking about ->read().

My main concern is ->read(). Initially I thought that I could pass the
clock source as argument to all callbacks, but I now understand that
passing cs to ->vread() is difficult.

> in a more dynamic driver model the clocksource driver could store
> dynamic data like target port/memory address next to the clocksource
> driver, and access that - if the driver pointer is passed in.

Exactly.

>> However not all clocksources use vread(), but really I'm not quite
>> clear on what one would want to access in the clocksource structure
>> when making a ->read() call.

Below is a link to some helper functions to manage incrementing
timers. The code is a bit rough but the idea is simple - create one
simple interface for timers that can be used as clock source and/or
clock event.

http://comments.gmane.org/gmane.linux.ports.sh.devel/4850

If you like this idea then similar helper code for decrementing timers
may be useful as well.

>> So maybe a further description of what specific need motivates this
>> change would be helpful? The brief description of power management
>> doesn't quite click in my head yet.
>
> yeah, that would be nice.

The reason for passing the clock source as argument to the read() and
resume() callbacks is that I want to use the same function for
multiple timer instances and use container_of() to get access to
per-instance private data. Without any argument I have to rely on
global data or compile time constants which makes it difficult to
reuse functions for multiple instances. Right now the timer_inc helper
code only exports a single clock source because of this limitation.

Regarding power management, the clock event code allows us to put the
hardware block for unused clock events in some kind of sleep mode when
CLOCK_EVT_MODE_SHUTDOWN or CLOCK_EVT_MODE_UNUSED is passed to
->set_mode(). I'd like to have something similar for clock sources
where unused clock sources can be powered down. For instance, we may
have two clock sources, one high resolution and one with not so good
accuracy. If the system is running with performance in mind then the
high resolution clock source should be used, but when we need to save
energy we want to switch over to the more primitive clock source and
disable the hight resolution clock source to be able to enter deeper
sleep states.

It is unfortunately a bit difficult to fully tie the clock framework
to the clock event drivers today. The timer hardware that generates
clock events for us is driven by some clock managed by the clock
framework. We use clk_enable() and clk_disable() to dynamically gate
off the clock for the timer hardware. This gives us basic run time
power management - disabling the clock when the device is unused is a
must to be able to enter deep sleep states.

Anyway, the current clock event interface wants details such as clock
rate and delta values at registration time. The problem is that the
clock framework doesn't guarantee the clock rate to be stable until
the clock is enabled, and at the time the clock is registered the
clock is disabled. So we don't have that information at clock event
registration time. And even if we did - after resuming from SHUTDOWN
or UNUSED the clock rate and deltas may have changed so we need some
way to update these parameters if we want to integrate well with the
clock framework.

I hope this makes things a bit more clear. =)

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists