The clock to the ARM TWD local timer scales with the cpu
frequency. To allow the cpu frequency to change while
maintaining a constant TWD frequency, pick a lower target
frequency for the TWD and use the prescaler to divide down
to the closest lower frequency.
This patch provides a new initialization function that takes
a target TWD frequency and the ratio between the cpu
clock and the TWD clock, specified as an integer divider >= 2
in the Cortex A9 MPCore TRM, and 2 in the ARM11 MPCore TRM.
It also registers a cpufreq notifier that adjusts the
prescaler when the cpu frequency changes.
Signed-off-by: Colin Cross <[email protected]>
---
arch/arm/include/asm/smp_twd.h | 11 ++++
arch/arm/kernel/smp_twd.c | 106 +++++++++++++++++++++++++++++++++++++---
2 files changed, 109 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index 634f357..5119763 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -17,6 +17,7 @@
#define TWD_TIMER_CONTROL_ONESHOT (0 << 1)
#define TWD_TIMER_CONTROL_PERIODIC (1 << 1)
#define TWD_TIMER_CONTROL_IT_ENABLE (1 << 2)
+#define TWD_TIMER_CONTROL_PRESCALE_MASK (0xFF << 8)
struct clock_event_device;
@@ -26,4 +27,14 @@ void twd_timer_stop(void);
int twd_timer_ack(void);
void twd_timer_setup(struct clock_event_device *);
+/*
+ * Use this setup function on systems that support cpufreq.
+ * periphclk_prescaler is the fixed divider value between the cpu
+ * clock and the PERIPHCLK clock that feeds the TWD. target_rate should be
+ * low enough that the prescaler can accurately reach the target rate from the
+ * lowest cpu frequency, but high enough to give a reasonable timer accuracy.
+ */
+void twd_timer_setup_scalable(struct clock_event_device *,
+ unsigned long target_rate, unsigned int periphclk_prescaler);
+
#endif
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 35882fb..e2bc65c 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -17,6 +17,7 @@
#include <linux/clockchips.h>
#include <linux/irq.h>
#include <linux/io.h>
+#include <linux/cpufreq.h>
#include <asm/smp_twd.h>
#include <asm/hardware/gic.h>
@@ -25,11 +26,17 @@
void __iomem *twd_base;
static unsigned long twd_timer_rate;
+static unsigned long twd_periphclk_prescaler;
+static unsigned long twd_target_rate;
static void twd_set_mode(enum clock_event_mode mode,
struct clock_event_device *clk)
{
unsigned long ctrl;
+ unsigned long prescale;
+
+ prescale = __raw_readl(twd_base + TWD_TIMER_CONTROL) &
+ TWD_TIMER_CONTROL_PRESCALE_MASK;
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
@@ -47,6 +54,8 @@ static void twd_set_mode(enum clock_event_mode mode,
ctrl = 0;
}
+ ctrl |= prescale;
+
__raw_writel(ctrl, twd_base + TWD_TIMER_CONTROL);
}
@@ -79,9 +88,55 @@ int twd_timer_ack(void)
return 0;
}
+/*
+ * must be called with interrupts disabled and on the cpu that is being changed
+ */
+static void twd_update_cpu_frequency(unsigned long new_rate)
+{
+ u32 ctrl;
+ int prescaler;
+
+ BUG_ON(twd_periphclk_prescaler == 0 || twd_target_rate == 0);
+
+ twd_timer_rate = new_rate / twd_periphclk_prescaler;
+
+ prescaler = DIV_ROUND_UP(twd_timer_rate, twd_target_rate);
+ prescaler = clamp(prescaler - 1, 0, 0xFF);
+
+ ctrl = __raw_readl(twd_base + TWD_TIMER_CONTROL);
+ ctrl &= ~TWD_TIMER_CONTROL_PRESCALE_MASK;
+ ctrl |= prescaler << 8;
+ __raw_writel(ctrl, twd_base + TWD_TIMER_CONTROL);
+}
+
+static void twd_update_cpu_frequency_on_cpu(void *data)
+{
+ struct cpufreq_freqs *freq = data;
+
+ twd_update_cpu_frequency(freq->new * 1000);
+}
+
+static int twd_cpufreq_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct cpufreq_freqs *freq = data;
+
+ if (event == CPUFREQ_RESUMECHANGE ||
+ (event == CPUFREQ_PRECHANGE && freq->new > freq->old) ||
+ (event == CPUFREQ_POSTCHANGE && freq->new < freq->old))
+ smp_call_function_single(freq->cpu,
+ twd_update_cpu_frequency_on_cpu, freq, 1);
+
+ return 0;
+}
+
+static struct notifier_block twd_cpufreq_notifier_block = {
+ .notifier_call = twd_cpufreq_notifier,
+};
+
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -114,23 +169,37 @@ static void __cpuinit twd_calibrate_rate(void)
twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);
printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
- (twd_timer_rate / 100000) % 100);
+ (twd_timer_rate / 10000) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
* Setup the local clock events for a CPU.
*/
-void __cpuinit twd_timer_setup(struct clock_event_device *clk)
+static void __cpuinit __twd_timer_setup(struct clock_event_device *clk,
+ unsigned long target_rate, unsigned int periphclk_prescaler)
{
unsigned long flags;
+ unsigned long load;
+ unsigned long cpu_rate;
+ unsigned long twd_tick_rate;
twd_calibrate_rate();
+ if (target_rate && periphclk_prescaler) {
+ cpu_rate = twd_timer_rate * periphclk_prescaler;
+ twd_target_rate = target_rate;
+ twd_periphclk_prescaler = periphclk_prescaler;
+ twd_update_cpu_frequency(cpu_rate);
+ twd_tick_rate = twd_target_rate;
+ } else {
+ twd_tick_rate = twd_timer_rate;
+ }
+
+ load = twd_tick_rate / HZ;
+
+ __raw_writel(load, twd_base + TWD_TIMER_LOAD);
+
clk->name = "local_timer";
clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
CLOCK_EVT_FEAT_C3STOP;
@@ -138,7 +207,7 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
clk->set_mode = twd_set_mode;
clk->set_next_event = twd_set_next_event;
clk->shift = 20;
- clk->mult = div_sc(twd_timer_rate, NSEC_PER_SEC, clk->shift);
+ clk->mult = div_sc(twd_tick_rate, NSEC_PER_SEC, clk->shift);
clk->max_delta_ns = clockevent_delta2ns(0xffffffff, clk);
clk->min_delta_ns = clockevent_delta2ns(0xf, clk);
@@ -151,6 +220,27 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
clockevents_register_device(clk);
}
+void __cpuinit twd_timer_setup_scalable(struct clock_event_device *clk,
+ unsigned long target_rate, unsigned int periphclk_prescaler)
+{
+ __twd_timer_setup(clk, target_rate, periphclk_prescaler);
+}
+
+void __cpuinit twd_timer_setup(struct clock_event_device *clk)
+{
+ __twd_timer_setup(clk, 0, 0);
+}
+
+static int twd_timer_setup_cpufreq(void)
+{
+ if (twd_periphclk_prescaler)
+ cpufreq_register_notifier(&twd_cpufreq_notifier_block,
+ CPUFREQ_TRANSITION_NOTIFIER);
+
+ return 0;
+}
+arch_initcall(twd_timer_setup_cpufreq);
+
#ifdef CONFIG_HOTPLUG_CPU
/*
* take a local timer down
--
1.7.3.1
2010/11/18 Colin Cross <[email protected]>:
> The clock to the ARM TWD local timer scales with the cpu
> frequency. To allow the cpu frequency to change while
> maintaining a constant TWD frequency, pick a lower target
> frequency for the TWD and use the prescaler to divide down
> to the closest lower frequency.
We are using this with some custom hooks for the U8500.
Tested-by: Linus Walleij <[email protected]>
Martin Persson can probably provide an additional Tested-by
from ST-Ericsson if it helps.
Colin are you merging this patch for 2.6.39 through
Russells tracker or pull request? It's an important patch
for us.
Yours,
Linus Walleij
We've seen no problems using this patch and have been grateful for it's existence. So I'm happy to put:
Tested-by: [email protected]
/Martin
On 03/04/2011 11:17 AM, Linus Walleij wrote:
> 2010/11/18 Colin Cross<[email protected]>:
>
>> The clock to the ARM TWD local timer scales with the cpu
>> frequency. To allow the cpu frequency to change while
>> maintaining a constant TWD frequency, pick a lower target
>> frequency for the TWD and use the prescaler to divide down
>> to the closest lower frequency.
> We are using this with some custom hooks for the U8500.
>
> Tested-by: Linus Walleij<[email protected]>
>
> Martin Persson can probably provide an additional Tested-by
> from ST-Ericsson if it helps.
>
> Colin are you merging this patch for 2.6.39 through
> Russells tracker or pull request? It's an important patch
> for us.
>
> Yours,
> Linus Walleij
On Fri, Mar 4, 2011 at 2:27 AM, martin persson
<[email protected]> wrote:
> We've seen no problems using this patch and have been grateful for it's
> existence. So I'm happy to put:
>
> Tested-by: [email protected]
>
> /Martin
>
> On 03/04/2011 11:17 AM, Linus Walleij wrote:
>>
>> 2010/11/18 Colin Cross<[email protected]>:
>>
>>> The clock to the ARM TWD local timer scales with the cpu
>>> frequency. To allow the cpu frequency to change while
>>> maintaining a constant TWD frequency, pick a lower target
>>> frequency for the TWD and use the prescaler to divide down
>>> to the closest lower frequency.
>>
>> We are using this with some custom hooks for the U8500.
>>
>> Tested-by: Linus Walleij<[email protected]>
>>
>> Martin Persson can probably provide an additional Tested-by
>> from ST-Ericsson if it helps.
>>
>> Colin are you merging this patch for 2.6.39 through
>> Russells tracker or pull request? It's an important patch
>> for us.
>>
>> Yours,
>> Linus Walleij
>
I never got any responses, and it conflicts with Rob Herring's patch
6434/1, although that has not been applied. Russell, do you want this
in the patch tracker?
On 03/04/2011 02:11 PM, Colin Cross wrote:
> On Fri, Mar 4, 2011 at 2:27 AM, martin persson
> <[email protected]> wrote:
>> We've seen no problems using this patch and have been grateful for it's
>> existence. So I'm happy to put:
>>
>> Tested-by: [email protected]
>>
>> /Martin
>>
>> On 03/04/2011 11:17 AM, Linus Walleij wrote:
>>>
>>> 2010/11/18 Colin Cross<[email protected]>:
>>>
>>>> The clock to the ARM TWD local timer scales with the cpu
>>>> frequency. To allow the cpu frequency to change while
>>>> maintaining a constant TWD frequency, pick a lower target
>>>> frequency for the TWD and use the prescaler to divide down
>>>> to the closest lower frequency.
>>>
>>> We are using this with some custom hooks for the U8500.
>>>
>>> Tested-by: Linus Walleij<[email protected]>
>>>
>>> Martin Persson can probably provide an additional Tested-by
>>> from ST-Ericsson if it helps.
>>>
>>> Colin are you merging this patch for 2.6.39 through
>>> Russells tracker or pull request? It's an important patch
>>> for us.
>>>
>>> Yours,
>>> Linus Walleij
>>
>
> I never got any responses, and it conflicts with Rob Herring's patch
> 6434/1, although that has not been applied. Russell, do you want this
> in the patch tracker?
Russell wanted to move over completely to using the clock api rather
than making clock api usage optional and this was dependent on his
init_early changes for Realview/Versatile. So I need to update the
patches based on that.
I think a cleaner solution for this is platforms should define a clock
for the local timer and the notifier can just get the clock rate again.
But these clocks have to be implemented first on all platforms using
local timer to make it unconditional, and I don't have the clock tree
knowledge of all those platforms.
Rob
On Fri, Mar 4, 2011 at 12:31 PM, Rob Herring <[email protected]> wrote:
> On 03/04/2011 02:11 PM, Colin Cross wrote:
>>
>> On Fri, Mar 4, 2011 at 2:27 AM, martin persson
>> <[email protected]> ?wrote:
>>>
>>> We've seen no problems using this patch and have been grateful for it's
>>> existence. So I'm happy to put:
>>>
>>> Tested-by: [email protected]
>>>
>>> /Martin
>>>
>>> On 03/04/2011 11:17 AM, Linus Walleij wrote:
>>>>
>>>> 2010/11/18 Colin Cross<[email protected]>:
>>>>
>>>>> The clock to the ARM TWD local timer scales with the cpu
>>>>> frequency. To allow the cpu frequency to change while
>>>>> maintaining a constant TWD frequency, pick a lower target
>>>>> frequency for the TWD and use the prescaler to divide down
>>>>> to the closest lower frequency.
>>>>
>>>> We are using this with some custom hooks for the U8500.
>>>>
>>>> Tested-by: Linus Walleij<[email protected]>
>>>>
>>>> Martin Persson can probably provide an additional Tested-by
>>>> from ST-Ericsson if it helps.
>>>>
>>>> Colin are you merging this patch for 2.6.39 through
>>>> Russells tracker or pull request? It's an important patch
>>>> for us.
>>>>
>>>> Yours,
>>>> Linus Walleij
>>>
>>
>> I never got any responses, and it conflicts with Rob Herring's patch
>> 6434/1, although that has not been applied. ?Russell, do you want this
>> in the patch tracker?
>
> Russell wanted to move over completely to using the clock api rather than
> making clock api usage optional and this was dependent on his init_early
> changes for Realview/Versatile. So I need to update the patches based on
> that.
>
> I think a cleaner solution for this is platforms should define a clock for
> the local timer and the notifier can just get the clock rate again. But
> these clocks have to be implemented first on all platforms using local timer
> to make it unconditional, and I don't have the clock tree knowledge of all
> those platforms.
>
> Rob
>
Using a clock along with the cpufreq notifiers would simplify my patch
some. Maybe we can use the traction this patch has among platforms
that use the twd to get the necessary clocks added. Can you post a
version of your patch that assumes the presence of a twd clock, and
I'll work on getting the necessary clocks added and updating my patch
on top of yours?
Collin, Rob,
> -----Original Message-----
> From: [email protected] [mailto:linux-
> [email protected]] On Behalf Of Colin Cross
> Sent: Saturday, March 05, 2011 3:03 AM
> To: Rob Herring
> Cc: Russell King; Srinidhi KASAGAR; Harald Gustafsson; Linus
> Walleij; [email protected]; Rickard ANDERSSON; Varun
> Swara; Catalin Marinas; martin persson; linux-arm-
> [email protected]
> Subject: Re: [PATCH] ARM: twd: Adjust localtimer frequency with
> cpufreqnotifiers
>
[...]
>
> Using a clock along with the cpufreq notifiers would simplify my
> patch
> some. Maybe we can use the traction this patch has among platforms
> that use the twd to get the necessary clocks added. Can you post a
> version of your patch that assumes the presence of a twd clock, and
> I'll work on getting the necessary clocks added and updating my
> patch on top of yours?
>
Some time back I also fixed the TWD scaling problem based on more
or less similar idea of pre-scaler and CPUFREQ notifiers.
Rob's clock idea was neat and useful to re-calibration
While doing this patch for OMAP I also found that
CPUFREQ notifiers does delays scaling timer frequency
and there is a tick deviation(3-4 ms) around 1st tick and
last tick around twd rescaling.
Another issue was not able to select higher fixed twd rate
and found fix for the same.
Regards,
Santosh
On Sun, Mar 6, 2011 at 6:42 PM, Colin Cross <[email protected]> wrote:
> Is there any way
> for a clockevent to invalidate the current event and ask for it to be
> reprogrammed?
TGLX will know for sure... Thomas?
(This is inside a CPUfreq hook for a localtimer for the record.)
Yours,
Linus Walleij
On Sun, Mar 6, 2011 at 6:20 AM, Santosh Shilimkar
<[email protected]> wrote:
>> -----Original Message-----
>> From: [email protected] [mailto:linux-
>> [email protected]] On Behalf Of Linus Walleij
>> Sent: Sunday, March 06, 2011 5:37 PM
>> To: Santosh Shilimkar
>> Cc: Russell King; Srinidhi KASAGAR; Harald Gustafsson; Linus
>> Walleij; [email protected]; Rickard ANDERSSON; martin
>> persson; Colin Cross; Varun Swara; Catalin Marinas; linux-arm-
>> [email protected]
>> Subject: Re: [PATCH] ARM: twd: Adjust localtimer frequency
>> withcpufreqnotifiers
>>
>> On Sat, Mar 5, 2011 at 9:19 AM, Santosh Shilimkar
>> <[email protected]> wrote:
>>
>> > While doing this patch for OMAP I also found that
>> > CPUFREQ notifiers does delays scaling timer frequency
>> > and there is a tick deviation(3-4 ms) around 1st tick and
>> > last tick around twd rescaling.
>>
>> Is this caused by ticks that have been programmed
>> already (based on the previous frequency) when the scaling
>> takes effect? (That's most likely I think.)
>>
> That's correct Linus. I noticed that Collin's patch reduces that
> problem a bit. In my patch I was always leaving the scaling
> to post notifier which actually aggravates the scenario.
If you always scale in the post notifier, you will end up with delays
that are too short when the cpu frequency increases before the scaling
is decreased.
A large tick deviation is unexpected - the timer value doesn't need to
be reprogrammed, we are adjusting a prescaler between the cpu clock
and the counter so that the counter always increments at the same
frequency. The only inaccuracy occurs between when the prescaler is
reprogrammed and the clock frequency changes, where it counts at the
wrong frequency. This time should hopefully be very short.
On Tegra, the worst case transition is from the lowest frequency,
54MHz, to the highest frequency, 250 MHz. Assuming a 1MHz twd rate,
the prescaler goes from 54 to 250. If the time between changing the
prescaler and clock is 3 ms (1 ms for the pll to lock, 2 ms worst case
waiting for the IPI to the other cpu), the timer will increment 648
times (3 ms * 54 MHz / 250) instead of 3000 times (3 ms * 250 Mhz /
250), delaying the next tick by 2.3 ms.
>> The latter could be fixed by simply calling
>> schedule() for each CPU connected in the same core as
>> the TWD at the end of twd_update_cpu_frequency(),
>> couldn't it?
>>
> I don't think schedule will do any difference here because
> it's the actual tick duration which getting changed. The
> tick does happen as per the timer load value. Now it all
> depends at what point time in tick window the timer scaling
> happens.
>
>> Colin what do you say?
>>
You can't call schedule, interrupts are disabled. You may be able to
reprogram the clockevents based on the clocksource. Is there any way
for a clockevent to invalidate the current event and ask for it to be
reprogrammed?
>> > Another issue was not able to select higher fixed twd rate
>> > and found fix for the same.
>>
>> Can you send out the patch?
>>
> Sure. May be let's get all these twd scaling
> patches on the list. Rob's clock patches, Collin's
> notifier patch. And then I shall post the fix on
> top of that.
>
> Regards,
> Santosh
>
> -----Original Message-----
> From: [email protected] [mailto:linux-
> [email protected]] On Behalf Of Linus Walleij
> Sent: Sunday, March 06, 2011 5:37 PM
> To: Santosh Shilimkar
> Cc: Russell King; Srinidhi KASAGAR; Harald Gustafsson; Linus
> Walleij; [email protected]; Rickard ANDERSSON; martin
> persson; Colin Cross; Varun Swara; Catalin Marinas; linux-arm-
> [email protected]
> Subject: Re: [PATCH] ARM: twd: Adjust localtimer frequency
> withcpufreqnotifiers
>
> On Sat, Mar 5, 2011 at 9:19 AM, Santosh Shilimkar
> <[email protected]> wrote:
>
> > While doing this patch for OMAP I also found that
> > CPUFREQ notifiers does delays scaling timer frequency
> > and there is a tick deviation(3-4 ms) around 1st tick and
> > last tick around twd rescaling.
>
> Is this caused by ticks that have been programmed
> already (based on the previous frequency) when the scaling
> takes effect? (That's most likely I think.)
>
That's correct Linus. I noticed that Collin's patch reduces that
problem a bit. In my patch I was always leaving the scaling
to post notifier which actually aggravates the scenario.
> The latter could be fixed by simply calling
> schedule() for each CPU connected in the same core as
> the TWD at the end of twd_update_cpu_frequency(),
> couldn't it?
>
I don't think schedule will do any difference here because
it's the actual tick duration which getting changed. The
tick does happen as per the timer load value. Now it all
depends at what point time in tick window the timer scaling
happens.
> Colin what do you say?
>
> > Another issue was not able to select higher fixed twd rate
> > and found fix for the same.
>
> Can you send out the patch?
>
Sure. May be let's get all these twd scaling
patches on the list. Rob's clock patches, Collin's
notifier patch. And then I shall post the fix on
top of that.
Regards,
Santosh
On Sat, Mar 5, 2011 at 9:19 AM, Santosh Shilimkar
<[email protected]> wrote:
> While doing this patch for OMAP I also found that
> CPUFREQ notifiers does delays scaling timer frequency
> and there is a tick deviation(3-4 ms) around 1st tick and
> last tick around twd rescaling.
Is this caused by ticks that have been programmed
already (based on the previous frequency) when the scaling
takes effect? (That's most likely I think.)
The latter could be fixed by simply calling
schedule() for each CPU connected in the same core as
the TWD at the end of twd_update_cpu_frequency(),
couldn't it?
Colin what do you say?
> Another issue was not able to select higher fixed twd rate
> and found fix for the same.
Can you send out the patch?
Yours,
Linus Walleij
2011/3/6 Linus Walleij <[email protected]>:
> On Sun, Mar 6, 2011 at 6:42 PM, Colin Cross <[email protected]> wrote:
>
>> Is there any way
>> for a clockevent to invalidate the current event and ask for it to be
>> reprogrammed?
>
> TGLX will know for sure... Thomas?
>
> (This is inside a CPUfreq hook for a localtimer for the record.)
I discussed this briefly with TGLX at the UDS here and he's confident
we could add some nifty feature to reprogram an event. I dare not
try to guess where the code would sit or how it'd look...
Yours,
Linus Walleij
On Thu, 12 May 2011, Linus Walleij wrote:
> 2011/3/6 Linus Walleij <[email protected]>:
> > On Sun, Mar 6, 2011 at 6:42 PM, Colin Cross <[email protected]> wrote:
> >
> >> Is there any way
> >> for a clockevent to invalidate the current event and ask for it to be
> >> reprogrammed?
> >
> > TGLX will know for sure... Thomas?
> >
> > (This is inside a CPUfreq hook for a localtimer for the record.)
>
> I discussed this briefly with TGLX at the UDS here and he's confident
> we could add some nifty feature to reprogram an event. I dare not
> try to guess where the code would sit or how it'd look...
Does the following work for you ?
Thanks,
tglx
---------->
Subject: clock-ev-reconf.patch
From: Thomas Gleixner <[email protected]>
Date: Fri, 13 May 2011 10:53:13 +0200
Signed-off-by: Thomas Gleixner <[email protected]>
---
include/linux/clockchips.h | 2 ++
kernel/time/clockevents.c | 24 ++++++++++++++++++++++++
2 files changed, 26 insertions(+)
Index: linux-2.6/include/linux/clockchips.h
===================================================================
--- linux-2.6.orig/include/linux/clockchips.h
+++ linux-2.6/include/linux/clockchips.h
@@ -132,6 +132,8 @@ extern int clockevents_program_event(str
extern void clockevents_handle_noop(struct clock_event_device *dev);
+extern int clockevents_reconfigure(struct clock_event_device *ce, u32 freq, u32 minsec);
+
static inline void
clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
{
Index: linux-2.6/kernel/time/clockevents.c
===================================================================
--- linux-2.6.orig/kernel/time/clockevents.c
+++ linux-2.6/kernel/time/clockevents.c
@@ -133,6 +133,30 @@ int clockevents_program_event(struct clo
}
/**
+ * clockevents_reconfigure - Reconfigure and reprogram a clock event device.
+ * @dev: device to modify
+ * @freq: new device frequency
+ * @secr: guaranteed runtime conversion range in seconds
+ *
+ * Reconfigure and reprogram a clock event device in oneshot
+ * mode. Must only be called from low level idle code where
+ * interaction with hrtimers/nohz code etc. is not possible and
+ * guaranteed not to conflict. Must be called with interrupts
+ * disabled!
+ * Returns 0 on success, -ETIME when the event is in the past or
+ * -EINVAL when called with invalid parameters.
+ */
+int clockevents_reconfigure(struct clock_event_device *dev, u32 freq, u32 secr)
+{
+ if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
+ return -EINVAL;
+
+ clockevents_calc_mult_shift(dev, freq, secr ? secr : 1);
+
+ return clockevents_program_event(dev, dev->next_event, ktime_get());
+}
+
+/**
* clockevents_register_notifier - register a clock events change listener
*/
int clockevents_register_notifier(struct notifier_block *nb)
2011/5/13 Thomas Gleixner <[email protected]>:
> Does the following work for you ?
This patch is interesting because it uses calc_mult_shift() and the current
localtimer driver for TWD (arch/arm/kernel/smp_twd.c) still uses hardcoded
mult+shift.
So this patch in combination with TGLX add-on in the CPUfreq path would
eliminate the need of using the prescaler at all, which is a more appealing
solution to me for one, since it results in less code.
Colin, do you think it will work? Or was there some specific reason to use
a dynamic prescaler instead of doing things like this?
Yours,
Linus Walleij
On Fri, May 13, 2011 at 12:59:20PM +0200, Linus Walleij wrote:
> This patch is interesting because it uses calc_mult_shift() and the current
> localtimer driver for TWD (arch/arm/kernel/smp_twd.c) still uses hardcoded
> mult+shift.
I don't think there's any reason it uses a hard coded shift, just
that's what was the norm when the code was originally written. It
(and many other clock event code) probably could do with an update.
On Fri, May 13, 2011 at 3:59 AM, Linus Walleij <[email protected]> wrote:
> 2011/5/13 Thomas Gleixner <[email protected]>:
>
>> Does the following work for you ?
>
> This patch is interesting because it uses calc_mult_shift() and the current
> localtimer driver for TWD (arch/arm/kernel/smp_twd.c) still uses hardcoded
> mult+shift.
>
> So this patch in combination with TGLX add-on in the CPUfreq path would
> eliminate the need of using the prescaler at all, which is a more appealing
> solution to me for one, since it results in less code.
>
> Colin, do you think it will work? Or was there some specific reason to use
> a dynamic prescaler instead of doing things like this?
This sounds much better than the dynamic prescaler. Using the
prescaler causes issues with accuracy and resolution that this avoids.
On Fri, May 13, 2011 at 3:02 AM, Thomas Gleixner <[email protected]> wrote:
> Does the following work for you ?
Not if the comments are accurate.
> Thanks,
>
> ? ? ? ?tglx
>
> ---------->
> Subject: clock-ev-reconf.patch
> From: Thomas Gleixner <[email protected]>
> Date: Fri, 13 May 2011 10:53:13 +0200
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> ?include/linux/clockchips.h | ? ?2 ++
> ?kernel/time/clockevents.c ?| ? 24 ++++++++++++++++++++++++
> ?2 files changed, 26 insertions(+)
>
> Index: linux-2.6/include/linux/clockchips.h
> ===================================================================
> --- linux-2.6.orig/include/linux/clockchips.h
> +++ linux-2.6/include/linux/clockchips.h
> @@ -132,6 +132,8 @@ extern int clockevents_program_event(str
>
> ?extern void clockevents_handle_noop(struct clock_event_device *dev);
>
> +extern int clockevents_reconfigure(struct clock_event_device *ce, u32 freq, u32 minsec);
> +
> ?static inline void
> ?clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
> ?{
> Index: linux-2.6/kernel/time/clockevents.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/clockevents.c
> +++ linux-2.6/kernel/time/clockevents.c
> @@ -133,6 +133,30 @@ int clockevents_program_event(struct clo
> ?}
>
> ?/**
> + * clockevents_reconfigure - Reconfigure and reprogram a clock event device.
> + * @dev: ? ? ? device to modify
> + * @freq: ? ? ?new device frequency
> + * @secr: ? ? ?guaranteed runtime conversion range in seconds
> + *
> + * Reconfigure and reprogram a clock event device in oneshot
> + * mode. Must only be called from low level idle code where
> + * interaction with hrtimers/nohz code etc. is not possible and
> + * guaranteed not to conflict. Must be called with interrupts
> + * disabled!
> + * Returns 0 on success, -ETIME when the event is in the past or
> + * -EINVAL when called with invalid parameters.
> + */
We need to call this from a cpufreq notifier with interrupts disabled,
not from idle.
> +int clockevents_reconfigure(struct clock_event_device *dev, u32 freq, u32 secr)
> +{
> + ? ? ? if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> + ? ? ? clockevents_calc_mult_shift(dev, freq, secr ? secr : 1);
> +
> + ? ? ? return clockevents_program_event(dev, dev->next_event, ktime_get());
> +}
> +
> +/**
> ?* clockevents_register_notifier - register a clock events change listener
> ?*/
> ?int clockevents_register_notifier(struct notifier_block *nb)
>
On Fri, 13 May 2011, Colin Cross wrote:
> On Fri, May 13, 2011 at 3:02 AM, Thomas Gleixner <[email protected]> wrote:
> > ?/**
> > + * clockevents_reconfigure - Reconfigure and reprogram a clock event device.
> > + * @dev: ? ? ? device to modify
> > + * @freq: ? ? ?new device frequency
> > + * @secr: ? ? ?guaranteed runtime conversion range in seconds
> > + *
> > + * Reconfigure and reprogram a clock event device in oneshot
> > + * mode. Must only be called from low level idle code where
> > + * interaction with hrtimers/nohz code etc. is not possible and
> > + * guaranteed not to conflict. Must be called with interrupts
> > + * disabled!
> > + * Returns 0 on success, -ETIME when the event is in the past or
> > + * -EINVAL when called with invalid parameters.
> > + */
> We need to call this from a cpufreq notifier with interrupts disabled,
> not from idle.
That works as well. Comments needs update. The important thing is that
neither a timer interrupt nor a hrtimer function should interfere on
that very cpu.
Thanks,
tglx
On 5/14/2011 9:21 PM, Thomas Gleixner wrote:
> On Fri, 13 May 2011, Colin Cross wrote:
>> On Fri, May 13, 2011 at 3:02 AM, Thomas Gleixner<[email protected]> wrote:
>>> /**
>>> + * clockevents_reconfigure - Reconfigure and reprogram a clock event device.
>>> + * @dev: device to modify
>>> + * @freq: new device frequency
>>> + * @secr: guaranteed runtime conversion range in seconds
>>> + *
>>> + * Reconfigure and reprogram a clock event device in oneshot
>>> + * mode. Must only be called from low level idle code where
>>> + * interaction with hrtimers/nohz code etc. is not possible and
>>> + * guaranteed not to conflict. Must be called with interrupts
>>> + * disabled!
>>> + * Returns 0 on success, -ETIME when the event is in the past or
>>> + * -EINVAL when called with invalid parameters.
>>> + */
>> We need to call this from a cpufreq notifier with interrupts disabled,
>> not from idle.
>
> That works as well. Comments needs update. The important thing is that
> neither a timer interrupt nor a hrtimer function should interfere on
> that very cpu.
>
The new interface certainly better than the TWD PRE-SCALER method.
Thanks for this Thomas.
Just for my understanding, the clockevents_reconfigure() needs to
be called with interrupts disabled on that CPU as part of
the CPUFREQ notifiers. I assume the right place is do it
in POST notifier after the CPU clock and hence TWD clock is
updated. Is that right ?
Since there is need to call this API in interrupt
disable context, does it make sense to take care of it
inside the API itself instead of relying on caller fn ?
The arch's where the per CPU TWD's share clock, per-cpu
clock-events should be reconfigured on all CPUs, whenever
the parent(CPU) clock has changed using some thing like
smp_call_function_any() etc. Is that right understanding?
At least this is how I did twd pre-scaler based fn
for OMAP.
Regards
Santosh
On Mon, 16 May 2011, Santosh Shilimkar wrote:
> On 5/14/2011 9:21 PM, Thomas Gleixner wrote:
> Just for my understanding, the clockevents_reconfigure() needs to
> be called with interrupts disabled on that CPU as part of
> the CPUFREQ notifiers. I assume the right place is do it
> in POST notifier after the CPU clock and hence TWD clock is
> updated. Is that right ?
Yes.
> Since there is need to call this API in interrupt
> disable context, does it make sense to take care of it
> inside the API itself instead of relying on caller fn ?
Hmm, no strong opinion
> The arch's where the per CPU TWD's share clock, per-cpu
> clock-events should be reconfigured on all CPUs, whenever
> the parent(CPU) clock has changed using some thing like
> smp_call_function_any() etc. Is that right understanding?
Yes. If that's a common requirement we should move that to core code.
Thanks,
tglx
On Mon, May 16, 2011 at 7:44 AM, Thomas Gleixner <[email protected]> wrote:
> On Mon, 16 May 2011, Santosh Shilimkar wrote:
>> On 5/14/2011 9:21 PM, Thomas Gleixner wrote:
>> Just for my understanding, the clockevents_reconfigure() needs to
>> be called with interrupts disabled on that CPU as part of
>> the CPUFREQ notifiers. I assume the right place is do it
>> in POST notifier after the CPU clock and hence TWD clock is
>> updated. Is that right ?
>
> Yes.
Is it safe to only call it in POST? If the frequency is increasing,
and the TWD is not updated until after the CPU frequency has changed,
it is possible for a clockevent to fire too early. Will that cause
problems, or does the clockevent code check against a clocksource to
ensure the desired time has been reached? If that is OK, it
drastically simplifies the code, because the driver only needs to know
the current TWD frequency, not predict a future TWD frequency.
>> Since there is need to call this API in interrupt
>> disable context, does it make sense to take care of it
>> inside the API itself instead of relying on caller fn ?
>
> Hmm, no strong opinion
For SMP TWD, the caller will always be in interrupt disabled mode,
because the cpufreq notifier will get called on a random cpu, so
smp_call_function_single will be used to transition to the correct
cpu, which disables interrupts.
>> The arch's where the per CPU TWD's share clock, per-cpu
>> clock-events should be reconfigured on all CPUs, whenever
>> the parent(CPU) clock has changed using some thing like
>> smp_call_function_any() etc. Is that right understanding?
>
> Yes. If that's a common requirement we should move that to core code.
Santosh, are you suggesting the TWD be updated from the clock
framework instead of the cpufreq notifier?
I believe ARMv7 requires all CPUs to run at the same frequency, so it
would be possible to do this in the core code somewhere, but A15 has
fixed frequency counters, and all SMP Cortex-A9s I've seen use the SMP
TWD driver, so in practice this may end up being the only user.
It would be possible for the clockevent to have a flag
CLOCKEVENT_EVT_FEAT_SCALES_WITH_CPU, which registers a cpufreq
notifier, if there were any other users.
On Mon, 16 May 2011, Colin Cross wrote:
> On Mon, May 16, 2011 at 7:44 AM, Thomas Gleixner <[email protected]> wrote:
> > On Mon, 16 May 2011, Santosh Shilimkar wrote:
> >> On 5/14/2011 9:21 PM, Thomas Gleixner wrote:
> >> Just for my understanding, the clockevents_reconfigure() needs to
> >> be called with interrupts disabled on that CPU as part of
> >> the CPUFREQ notifiers. I assume the right place is do it
> >> in POST notifier after the CPU clock and hence TWD clock is
> >> updated. Is that right ?
> >
> > Yes.
>
> Is it safe to only call it in POST? If the frequency is increasing,
> and the TWD is not updated until after the CPU frequency has changed,
> it is possible for a clockevent to fire too early. Will that cause
> problems, or does the clockevent code check against a clocksource to
> ensure the desired time has been reached? If that is OK, it
> drastically simplifies the code, because the driver only needs to know
> the current TWD frequency, not predict a future TWD frequency.
Yes, the hrtimer code checks against current time, so if it fires too
early it rearms the timer.
Thanks,
tglx
On 5/16/2011 9:59 PM, Colin Cross wrote:
> On Mon, May 16, 2011 at 7:44 AM, Thomas Gleixner<[email protected]> wrote:
>> On Mon, 16 May 2011, Santosh Shilimkar wrote:
>>> On 5/14/2011 9:21 PM, Thomas Gleixner wrote:
>>> Just for my understanding, the clockevents_reconfigure() needs to
>>> be called with interrupts disabled on that CPU as part of
>>> the CPUFREQ notifiers. I assume the right place is do it
>>> in POST notifier after the CPU clock and hence TWD clock is
>>> updated. Is that right ?
>>
>> Yes.
>
> Is it safe to only call it in POST? If the frequency is increasing,
> and the TWD is not updated until after the CPU frequency has changed,
> it is possible for a clockevent to fire too early. Will that cause
> problems, or does the clockevent code check against a clocksource to
> ensure the desired time has been reached? If that is OK, it
> drastically simplifies the code, because the driver only needs to know
> the current TWD frequency, not predict a future TWD frequency.
>
This was the exact reason I asked this question. As discussed
earlier on this thread, we observed drift in ticks especially
at lowest and highest clock-points. But they way I understood
is clockevents_reconfigure() will block those additional
ticks at least during the reconfiguration of the clock-event.
>>> Since there is need to call this API in interrupt
>>> disable context, does it make sense to take care of it
>>> inside the API itself instead of relying on caller fn ?
>>
>> Hmm, no strong opinion
>
> For SMP TWD, the caller will always be in interrupt disabled mode,
> because the cpufreq notifier will get called on a random cpu, so
> smp_call_function_single will be used to transition to the correct
> cpu, which disables interrupts.
>
Ok. So it's indirectly taken care then.
>>> The arch's where the per CPU TWD's share clock, per-cpu
>>> clock-events should be reconfigured on all CPUs, whenever
>>> the parent(CPU) clock has changed using some thing like
>>> smp_call_function_any() etc. Is that right understanding?
>>
>> Yes. If that's a common requirement we should move that to core code.
>
> Santosh, are you suggesting the TWD be updated from the clock
> framework instead of the cpufreq notifier?
>
That's where I was kind of leaning to. Basically doing this in common
core code at one place and possibly outside the ARM TWD library. You
might get same requirement on other arch's in future.
> I believe ARMv7 requires all CPUs to run at the same frequency, so it
> would be possible to do this in the core code somewhere, but A15 has
> fixed frequency counters, and all SMP Cortex-A9s I've seen use the SMP
> TWD driver, so in practice this may end up being the only user.
>
Yes but the code managing the architectural timer(A15) and TWD(A9) is
different. But I understand your point about the usage and it
might be limited to CA9 at this point of time.
> It would be possible for the clockevent to have a flag
> CLOCKEVENT_EVT_FEAT_SCALES_WITH_CPU, which registers a cpufreq
> notifier, if there were any other users.
>
Something like this is better to get better clarity on the
hardware behavior. O.w we will have piece of code in TWD
library which needs proper documentation about the
usage of likes of smp_call_function_single().
Regards
Santosh
On Mon, May 16, 2011 at 9:43 AM, Santosh Shilimkar
<[email protected]> wrote:
> On 5/16/2011 9:59 PM, Colin Cross wrote:
>>
>> On Mon, May 16, 2011 at 7:44 AM, Thomas Gleixner<[email protected]>
>> ?wrote:
>>>
>>> On Mon, 16 May 2011, Santosh Shilimkar wrote:
>>>>
>>>> On 5/14/2011 9:21 PM, Thomas Gleixner wrote:
>>>> Just for my understanding, the clockevents_reconfigure() needs to
>>>> be called with interrupts disabled on that CPU as part of
>>>> the CPUFREQ notifiers. I assume the right place is do it
>>>> in POST notifier after the CPU clock and hence TWD clock is
>>>> updated. Is that right ?
>>>
>>> Yes.
>>
>> Is it safe to only call it in POST? ?If the frequency is increasing,
>> and the TWD is not updated until after the CPU frequency has changed,
>> it is possible for a clockevent to fire too early. ?Will that cause
>> problems, or does the clockevent code check against a clocksource to
>> ensure the desired time has been reached? ?If that is OK, it
>> drastically simplifies the code, because the driver only needs to know
>> the current TWD frequency, not predict a future TWD frequency.
>>
> This was the exact reason I asked this question. As discussed
> earlier on this thread, we observed drift in ticks especially
> at lowest and highest clock-points. But they way I understood
> is clockevents_reconfigure() will block those additional
> ticks at least during the reconfiguration of the clock-event.
>
>
>>>> Since there is need to call this API in interrupt
>>>> disable context, does it make sense to take care of it
>>>> inside the API itself instead of relying on caller fn ?
>>>
>>> Hmm, no strong opinion
>>
>> For SMP TWD, the caller will always be in interrupt disabled mode,
>> because the cpufreq notifier will get called on a random cpu, so
>> smp_call_function_single will be used to transition to the correct
>> cpu, which disables interrupts.
>>
> Ok. So it's indirectly taken care then.
>
>>>> The arch's where the per CPU TWD's share clock, per-cpu
>>>> clock-events should be reconfigured on all CPUs, whenever
>>>> the parent(CPU) clock has changed using some thing like
>>>> smp_call_function_any() etc. Is that right understanding?
>>>
>>> Yes. If that's a common requirement we should move that to core code.
>>
>> Santosh, are you suggesting the TWD be updated from the clock
>> framework instead of the cpufreq notifier?
>>
> That's where I was kind of leaning to. Basically doing this in common
> core code at one place and possibly outside the ARM TWD library. You
> might get same requirement on other arch's in future.
But right now, there is no common clock place, and even if there was,
there is no plan that I know of for a clock notifier, which is what
would be required here.
>> I believe ARMv7 requires all CPUs to run at the same frequency, so it
>> would be possible to do this in the core code somewhere, but A15 has
>> fixed frequency counters, and all SMP Cortex-A9s I've seen use the SMP
>> TWD driver, so in practice this may end up being the only user.
>>
> Yes but the code managing the architectural timer(A15) and TWD(A9) is
> different. But I understand your point about the usage and it
> might be limited to CA9 at this point of time.
>
>> It would be possible for the clockevent to have a flag
>> CLOCKEVENT_EVT_FEAT_SCALES_WITH_CPU, which registers a cpufreq
>> notifier, if there were any other users.
>
>>
> Something like this is better to get better clarity on the
> hardware behavior. O.w we will have piece of code in TWD
> library which needs proper documentation about the
> usage of likes of smp_call_function_single().
This is harder than I made it sound, unless the clockevent code took a
struct clk as well. Something needs to get the frequency of the
smp_twd clock and pass it in to clockevents_reconfigure.
I will post an RFC patch that uses a clock to call
clockevents_reconfigure from a cpufreq notifier in smp_twd.c.