clocksource: add enable() and disable() callbacks
Subsystems (2)
PATCH 9/10 clocksource: add enable() and disable() callbacks
From: Magnus Damm <[email protected]>
Add enable() and disable() callbacks for clocksources. This allows us to put unused clocksources in power save mode. The functions clocksource_enable() and clocksource_disable() wrap the callbacks and are inserted in the timekeeping code to enable before use and disable after switching to a new clocksource.
---
include/linux/clocksource.h | 31 +++++++++++++++++++++++++++++++ kernel/time/timekeeping.c | 12 +++++++++--- 2 files changed, 40 insertions(+), 3 deletions(-)
include/linux/clocksource.h +31 -0
@@ -44,6 +44,8 @@ struct clocksource;* available. * @read: returns a cycle value * @read2: returns a cycle value, passes clocksource as argument +* @enable: optional function to enable the clocksource +* @disable: optional function to disable the clocksource * @mask: bitmask for two's complement * subtraction of non 64 bit counters * @mult: cycle to nanosecond multiplier (adjusted by NTP)
@@ -64,6 +66,8 @@ struct clocksource {int rating;cycle_t (*read)(void);cycle_t (*read2)(struct clocksource *cs); +int (*enable)(struct clocksource *cs); +void (*disable)(struct clocksource *cs);cycle_t mask;u32 mult;u32 mult_orig;
@@ -176,6 +180,33 @@ static inline cycle_t clocksource_read(s }/** +* clocksource_enable: - enable clocksource +* @cs: pointer to clocksource +* +* Enables the specified clocksource. The clocksource callback +* function should start up the hardware and setup mult and field +* members of struct clocksource to reflect hardware capabilities. +*/ +static inline int clocksource_enable (struct clocksource *cs) +{ +return cs->enable ? cs->enable (cs) :0 ; +} + +/** +* clocksource_disable: - disable clocksource +* @cs: pointer to clocksource +* +* Disables the specified clocksource. The clocksource callback +* function should power down the now unused hardware block to +* save power. +*/ +static inline void clocksource_disable (struct clocksource *cs) +{ +if (cs->disable) + cs->disable (cs); +} + +/** * cyc2ns - converts clocksource cycles to nanoseconds * @cs: Pointer to clocksource * @cycles: Cycles
kernel/time/timekeeping.c +9 -3
@@ -177,7 +177,7 @@ EXPORT_SYMBOL(do_settimeofday);*/ static void change_clocksource (void ) { -struct clocksource *new; +struct clocksource *new, *old; new =clocksource_get_next ();
@@ -186,11 +186,16 @@ static void change_clocksource(void)clocksource_forward_now (); - new->raw_time = clock->raw_time; +if (clocksource_enable (new)) +return ; + new->raw_time = clock->raw_time; + old = clock; clock = new; +clocksource_disable (old); + clock->cycle_last =0 ; - clock->cycle_last =clocksource_read (new); + clock->cycle_last =clocksource_read (clock); clock->error =0 ; clock->xtime_nsec =0 ;clocksource_calculate_interval (clock,NTP_INTERVAL_LENGTH );
@@ -287,6 +292,7 @@ void __init timekeeping_init(void)ntp_init (); clock =clocksource_get_next (); +clocksource_enable (clock);clocksource_calculate_interval (clock,NTP_INTERVAL_LENGTH ); clock->cycle_last =clocksource_read (clock);
Quoted 9 lines · Magnus Damm
From: Magnus Damm <[email protected]>
Add
enable()anddisable()callbacks for clocksources. This allows us to put unused clocksources in power save mode. The functionsclocksource_enable()andclocksource_disable()wrap the callbacks and are inserted in the timekeeping code to enable before use and disable after switching to a new clocksource.Signed-off-by Magnus Damm <[email protected]>
I like this! Its a nice addition to the clocksource structure to allow for improved power management.
I've got some worries that if clocksource_enable() is allowed to fail it might give us some grief in early boot (you don't check for errors there, but understandably, as there isn't much we can do).
However, maybe if a clocksource fails to be enabled, we need to eject it from the list of usable clocksources, as if clocksource with a high rating value, were to fail to be enabled, every tick we'd try to swap the bad clocksource in and fail, never to move to a good clocksource with slightly lower rating.
But other then that nit, I think this is a good change.
thanks
-john
Quoted 98 lines
---
include/linux/clocksource.h | 31 +++++++++++++++++++++++++++++++ kernel/time/timekeeping.c | 12 +++++++++--- 2 files changed, 40 insertions(+), 3 deletions(-)include/linux/clocksource.h +31
-0@@ -44,6 +44,8 @@ struct clocksource;* available. * @read: returns a cycle value * @read2: returns a cycle value, passes clocksource as argument +* @enable: optional function to enable the clocksource +* @disable: optional function to disable the clocksource * @mask: bitmask for two's complement * subtraction of non 64 bit counters * @mult: cycle to nanosecond multiplier (adjusted by NTP) @@ -64,6 +66,8 @@ struct clocksource {int rating;cycle_t (*read)(void);cycle_t (*read2)(struct clocksource *cs); +int (*enable)(struct clocksource *cs); +void (*disable)(struct clocksource *cs);cycle_t mask;u32 mult;u32 mult_orig;@@ -176,6 +180,33 @@ static inline cycle_t clocksource_read(s }/** +* clocksource_enable: - enable clocksource +* @cs: pointer to clocksource +* +* Enables the specified clocksource. The clocksource callback +* function should start up the hardware and setup mult and field +* members of struct clocksource to reflect hardware capabilities. +*/ +static inline int clocksource_enable (struct clocksource *cs) +{ +return cs->enable ? cs->enable (cs) :0 ; +} + +/** +* clocksource_disable: - disable clocksource +* @cs: pointer to clocksource +* +* Disables the specified clocksource. The clocksource callback +* function should power down the now unused hardware block to +* save power. +*/ +static inline void clocksource_disable (struct clocksource *cs) +{ +if (cs->disable) + cs->disable (cs); +} + +/** * cyc2ns - converts clocksource cycles to nanoseconds * @cs: Pointer to clocksource * @cycles: Cycles kernel/time/timekeeping.c +9
-3@@ -177,7 +177,7 @@ EXPORT_SYMBOL(do_settimeofday);*/ static void change_clocksource (void ) { -struct clocksource *new; +struct clocksource *new, *old; new =clocksource_get_next ();@@ -186,11 +186,16 @@ static void change_clocksource(void)clocksource_forward_now (); - new->raw_time = clock->raw_time; +if (clocksource_enable (new)) +return ; + new->raw_time = clock->raw_time; + old = clock; clock = new; +clocksource_disable (old); + clock->cycle_last =0 ; - clock->cycle_last =clocksource_read (new); + clock->cycle_last =clocksource_read (clock); clock->error =0 ; clock->xtime_nsec =0 ;clocksource_calculate_interval (clock,NTP_INTERVAL_LENGTH );@@ -287,6 +292,7 @@ void __init timekeeping_init(void)ntp_init (); clock =clocksource_get_next (); +clocksource_enable (clock);clocksource_calculate_interval (clock,NTP_INTERVAL_LENGTH ); clock->cycle_last =clocksource_read (clock);
Quoted 10 lines · john stultz
Quoted 9 lines · Magnus Damm
From: Magnus Damm <[email protected]>
Add
enable()anddisable()callbacks for clocksources. This allows us to put unused clocksources in power save mode. The functionsclocksource_enable()andclocksource_disable()wrap the callbacks and are inserted in the timekeeping code to enable before use and disable after switching to a new clocksource.Signed-off-by Magnus Damm <[email protected]>
Hi John,
Quoted 2 lines
I like this! Its a nice addition to the clocksource structure to allow for improved power management.
Great, thank you!
Quoted 3 lines
I've got some worries that if
clocksource_enable()is allowed to fail it might give us some grief in early boot (you don't check for errors there, but understandably, as there isn't much we can do).
I've been changing the code back and forth between returning void or int, but I settled on int since the clock framework returns an int from clk_enable(). So apparently that can fail. The clock framework is the main reason why I want to be add enable() and disable(), and if we can't enable the clock then it's very likely that we can't use the clocksource hardware either.
At boot, maybe being a bit more verbose about what is failing would be good.
Quoted 5 lines
However, maybe if a clocksource fails to be enabled, we need to eject it from the list of usable clocksources, as if clocksource with a high rating value, were to fail to be enabled, every tick we'd try to swap the bad clocksource in and fail, never to move to a good clocksource with slightly lower rating.
Yeah, moving it out from the list that sounds like a good idea. I wonder if this problem overlaps with unregistration of clocksources? My feeling is that we need to extend the clocksource state to include a broken-state somehow. But that raises all sorts of other questions.
Also while at the topic, it would be nice if we could use clocksources for other purposes such as trace clocks. Or maybe that is being done already?
Quoted 1 line
But other then that nit, I think this is a good change.
Excellent! Do you want to extend this patch with smarter logic for failing clocksources? I'd prefer to keep this as-is and try to get unregistration and improved handling of failing clocksources solved together.
Cheers,
/ magnus