Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Mon, Jan 24, 2011 at 11:06:09AM +, Russell King - ARM Linux wrote:
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.
I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?
Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode. We really ought to rewrite it whenever we switch back to periodic
mode.
I suspect fixing that means you won't need this save/restore support.
Untested, but should do what's required.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base +
TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
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.
Shadowing ctrl in a percpu variable and rewriting it during every call
to twd_set_next_event does work, but that's not much different, and a
little less efficient, than just saving and restoring the control and
load registers in idle. It does have the advantage that platforms
don't need any extra calls.
The next question is can we teach the generic time infrastructure about
this so we don't have to modify every clock event driver for it? We
really need to get away from having this kind of knowledge buried down
in the lowest levels of every driver.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote: 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. Shadowing ctrl in a percpu variable and rewriting it during every call to twd_set_next_event does work, but that's not much different, and a little less efficient, than just saving and restoring the control and load registers in idle. It does have the advantage that platforms don't need any extra calls. BTW, do you think the patch is, nevertheless, an improvement and something we should do? If so, please can I have your ack for it? Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
-Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 5:00 PM To: Colin Cross Cc: Santosh Shilimkar; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote: 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. Shadowing ctrl in a percpu variable and rewriting it during every call to twd_set_next_event does work, but that's not much different, and a little less efficient, than just saving and restoring the control and load registers in idle. It does have the advantage that platforms don't need any extra calls. BTW, do you think the patch is, nevertheless, an improvement and something we should do? If so, please can I have your ack for it? As I was suspecting the one shot mode wouldn't work it. Collin just confirmed it. To make it fully work it needs control register save restore. And that can be managed within TWD library so that every platform doesn't have to bother of calling it from their idle code. Do you prefer that as a separate patch or I can post a new version in which can add your fix + per cpu control register shadowing ? Regards, Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote: As I was suspecting the one shot mode wouldn't work it. Collin just confirmed it. To make it fully work it needs control register save restore. And that can be managed within TWD library so that every platform doesn't have to bother of calling it from their idle code. Do you prefer that as a separate patch or I can post a new version in which can add your fix + per cpu control register shadowing ? I said earlier: | The next question is can we teach the generic time infrastructure about | this so we don't have to modify every clock event driver for it? We | really need to get away from having this kind of knowledge buried down | in the lowest levels of every driver. IOW, if we go into a PM idle mode, when we come back out we need to call clockevent set_mode to ensure that the control register is properly reset. I don't think its right to have this kind of knowledge buried in each and every clockevent driver. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
Looping Thomas G -Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 5:19 PM To: Santosh Shilimkar Cc: Colin Cross; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote: As I was suspecting the one shot mode wouldn't work it. Collin just confirmed it. To make it fully work it needs control register save restore. And that can be managed within TWD library so that every platform doesn't have to bother of calling it from their idle code. Do you prefer that as a separate patch or I can post a new version in which can add your fix + per cpu control register shadowing ? I said earlier: | The next question is can we teach the generic time infrastructure about | this so we don't have to modify every clock event driver for it? We | really need to get away from having this kind of knowledge buried down | in the lowest levels of every driver. IOW, if we go into a PM idle mode, when we come back out we need to call clockevent set_mode to ensure that the control register is properly reset. I don't think its right to have this kind of knowledge buried in each and every clockevent driver. May be Thomas can comment on this. Regards, Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Tue, Jan 25, 2011 at 05:38:49PM +0530, Santosh Shilimkar wrote: Looping Thomas G -Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 5:19 PM To: Santosh Shilimkar Cc: Colin Cross; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support On Tue, Jan 25, 2011 at 05:10:37PM +0530, Santosh Shilimkar wrote: As I was suspecting the one shot mode wouldn't work it. Collin just confirmed it. To make it fully work it needs control register save restore. And that can be managed within TWD library so that every platform doesn't have to bother of calling it from their idle code. Do you prefer that as a separate patch or I can post a new version in which can add your fix + per cpu control register shadowing ? I said earlier: | The next question is can we teach the generic time infrastructure about | this so we don't have to modify every clock event driver for it? We | really need to get away from having this kind of knowledge buried down | in the lowest levels of every driver. IOW, if we go into a PM idle mode, when we come back out we need to call clockevent set_mode to ensure that the control register is properly reset. I don't think its right to have this kind of knowledge buried in each and every clockevent driver. May be Thomas can comment on this. He was already added on my previous email... we're going round in circles. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
-Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 5:48 PM To: Santosh Shilimkar Cc: Thomas Gleixner; Colin Cross; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support call clockevent set_mode to ensure that the control register is properly reset. I don't think its right to have this kind of knowledge buried in each and every clockevent driver. May be Thomas can comment on this. He was already added on my previous email... we're going round in circles. Sorry about that. Didn't noticed earlier -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Mon, Jan 24, 2011 at 11:06:09AM +, Russell King - ARM Linux wrote:
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.
I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?
Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode. We really ought to rewrite it whenever we switch back to periodic
mode.
I suspect fixing that means you won't need this save/restore support.
Untested, but should do what's required.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE |
TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base +
TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
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.
Shadowing ctrl in a percpu variable and rewriting it during every call
to twd_set_next_event does work, but that's not much different, and a
little less efficient, than just saving and restoring the control and
load registers in idle. It does have the advantage that platforms
don't need any extra calls.
The next question is can we teach the generic time infrastructure about
this so we don't have to modify every clock event driver for it? We
really need to get away from having this kind of knowledge buried down
in the lowest levels of every driver.
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 ?
Thanks,
tglx
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote:
On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:
On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote:
On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Mon, Jan 24, 2011 at 11:06:09AM +, Russell King - ARM Linux
wrote:
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.
I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?
Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode. We really ought to rewrite it whenever we switch back to
periodic
mode.
I suspect fixing that means you won't need this save/restore support.
Untested, but should do what's required.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE |
TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base +
TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
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.
Shadowing ctrl in a percpu variable and rewriting it during every call
to twd_set_next_event does work, but that's not much different, and a
little less efficient, than just saving and restoring the control and
load registers in idle. It does have the advantage that platforms
don't need any extra calls.
The next question is can we teach the generic time infrastructure about
this so we don't have to modify every clock event driver for it? We
really need to get away from having this kind of knowledge buried down
in the lowest levels of every driver.
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.
I suggest we defer this until there's a visible use case available.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
-Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 7:08 PM To: Thomas Gleixner Cc: Colin Cross; Santosh Shilimkar; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support On Tue, Jan 25, 2011 at 02:23:10PM +0100, Thomas Gleixner wrote: On Tue, 25 Jan 2011, Russell King - ARM Linux wrote: [...] The next question is can we teach the generic time infrastructure about this so we don't have to modify every clock event driver for it? We really need to get away from having this kind of knowledge buried down in the lowest levels of every driver. 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. I suggest we defer this until there's a visible use case available. Just for clarification. Without the TWD save restore patch the PM won't work. So what you mean by visible usecase. We need this to be fixed and that's what was done with the my patch. Ofcourse it's a custom hack approach but does the job. Regards, Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 ? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
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.
I quote the code:
static void twd_set_mode(enum clock_event_mode mode,
struct clock_event_device *clk)
{
unsigned long ctrl;
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
ctrl = TWD_TIMER_CONTROL_IT_ENABLE | TWD_TIMER_CONTROL_ONESHOT;
break;
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
default:
ctrl = 0;
}
__raw_writel(ctrl, twd_base + TWD_TIMER_CONTROL);
}
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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: 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
Russell, Thomas
-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Tuesday, January 25, 2011 7:08 PM
To: Thomas Gleixner
Cc: Colin Cross; Santosh Shilimkar; catalin.mari...@arm.com;
linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm-
ker...@lists.infradead.org
Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
[...]
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.
I suggest we defer this until there's a visible use case available.
Got some time to debug this issue. OMAP idle code already has the
broad-cast notifiers working for sometime. So I removed the existing
save restore twd code and just used the below patch which Russell
posted on this thread.
And things just work and I guess we are done with this issue !!
Timer framework is doing all right things with notifiers.
Am going to use this patch for my further work and drop the
save/restore patch. Will update you if there is any other
issue on this. I should have validated this patch on my own
earlier :(
Thanks for the discussion.
-
From 9084dd3e68a8528172d97fb49c42437ae873af43 Mon Sep 17 00:00:00 2001
From: Russell King rmk+ker...@arm.linux.org.uk
Date: Tue, 25 Jan 2011 14:26:41 +0530
Subject: [PATCH] ARM: smp_twd: Always program the loadvalue when switching
to periodic mode.
Always program the load value when switching to periodic mode so that
we don't need to save restore twd load register.
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
arch/arm/kernel/smp_twd.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE |
TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base +
TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
--
1.6.0.4
0001-ARM-smp_twd-Always-program-the-loadvalue-when-swit.patch
Description: Binary data
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Tue, Jan 25, 2011 at 09:34:15PM +0530, Santosh Shilimkar wrote: Russell, Thomas -Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 7:08 PM To: Thomas Gleixner Cc: Colin Cross; Santosh Shilimkar; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support [...] 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. I suggest we defer this until there's a visible use case available. Got some time to debug this issue. OMAP idle code already has the broad-cast notifiers working for sometime. So I removed the existing save restore twd code and just used the below patch which Russell posted on this thread. And things just work and I guess we are done with this issue !! Timer framework is doing all right things with notifiers. Am going to use this patch for my further work and drop the save/restore patch. Will update you if there is any other issue on this. I should have validated this patch on my own earlier :( Thanks for the discussion. Thanks - I guess that's a tested-by then? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
-Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 9:43 PM To: Santosh Shilimkar Cc: Thomas Gleixner; Colin Cross; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support On Tue, Jan 25, 2011 at 09:34:15PM +0530, Santosh Shilimkar wrote: Russell, Thomas -Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Tuesday, January 25, 2011 7:08 PM To: Thomas Gleixner Cc: Colin Cross; Santosh Shilimkar; catalin.mari...@arm.com; linus.ml.wall...@gmail.com; linux-omap@vger.kernel.org; linux- arm- ker...@lists.infradead.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support [...] 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. I suggest we defer this until there's a visible use case available. Got some time to debug this issue. OMAP idle code already has the broad-cast notifiers working for sometime. So I removed the existing save restore twd code and just used the below patch which Russell posted on this thread. And things just work and I guess we are done with this issue !! Timer framework is doing all right things with notifiers. Am going to use this patch for my further work and drop the save/restore patch. Will update you if there is any other issue on this. I should have validated this patch on my own earlier :( Thanks for the discussion. Thanks - I guess that's a tested-by then? Sure. Tested-by: Santosh Shilimkar santosh.shilim...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Tue, Jan 25, 2011 at 3:29 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Mon, Jan 24, 2011 at 11:39:13PM -0800, Colin Cross wrote: 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. Shadowing ctrl in a percpu variable and rewriting it during every call to twd_set_next_event does work, but that's not much different, and a little less efficient, than just saving and restoring the control and load registers in idle. It does have the advantage that platforms don't need any extra calls. BTW, do you think the patch is, nevertheless, an improvement and something we should do? If so, please can I have your ack for it? This patch does work after adding the missing clockevent broadcasts to the Tegra idle driver, and allows me to remove all references to twd registers in the idle and suspend code, so: Acked-by: Colin Cross ccr...@android.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote: In CPU low power state, local timer looses its register context. This patch adds context save restore hooks which can be used by platforms appropriately. I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic timer stuff wouldn't rely on it being kept alive? Hmm, it looks like we bypass the clockevents code by only setting the TWD load value at initialization time, not when we switch to periodic mode. We really ought to rewrite it whenever we switch back to periodic mode. I suspect fixing that means you won't need this save/restore support. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Mon, Jan 24, 2011 at 11:06:09AM +, Russell King - ARM Linux wrote:
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.
I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?
Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode. We really ought to rewrite it whenever we switch back to periodic
mode.
I suspect fixing that means you won't need this save/restore support.
Untested, but should do what's required.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
-Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Monday, January 24, 2011 4:36 PM To: Santosh Shilimkar Cc: linux-arm-ker...@lists.infradead.org; catalin.mari...@arm.com; ccr...@android.com; linus.ml.wall...@gmail.com; linux- o...@vger.kernel.org Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote: In CPU low power state, local timer looses its register context. This patch adds context save restore hooks which can be used by platforms appropriately. I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic timer stuff wouldn't rely on it being kept alive? Hmm, it looks like we bypass the clockevents code by only setting the TWD load value at initialization time, not when we switch to periodic mode. We really ought to rewrite it whenever we switch back to periodic mode. Idea looks good though am not sure the one shot mode. I suspect fixing that means you won't need this save/restore support. Will try it out. Regards, Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] ARM: twd: Add context save restore support
-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Monday, January 24, 2011 4:41 PM
To: Santosh Shilimkar
Cc: linux-arm-ker...@lists.infradead.org; catalin.mari...@arm.com;
ccr...@android.com; linus.ml.wall...@gmail.com; linux-
o...@vger.kernel.org
Subject: Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Mon, Jan 24, 2011 at 11:06:09AM +, Russell King - ARM Linux
wrote:
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
In CPU low power state, local timer looses its register context.
This
patch adds context save restore hooks which can be used by
platforms
appropriately.
I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the
generic
timer stuff wouldn't rely on it being kept alive?
Hmm, it looks like we bypass the clockevents code by only setting
the
TWD load value at initialization time, not when we switch to
periodic
mode. We really ought to rewrite it whenever we switch back to
periodic
mode.
I suspect fixing that means you won't need this save/restore
support.
Untested, but should do what's required.
:) I was just typing an email and you sent a patch. Will test this
and update you.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode
mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE |
TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base +
TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] ARM: twd: Add context save restore support
On Mon, Jan 24, 2011 at 3:11 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Mon, Jan 24, 2011 at 11:06:09AM +, Russell King - ARM Linux wrote:
On Mon, Jan 24, 2011 at 02:21:17PM +0530, Santosh Shilimkar wrote:
In CPU low power state, local timer looses its register context. This
patch adds context save restore hooks which can be used by platforms
appropriately.
I thought the whole point of CLOCK_EVT_FEAT_C3STOP was that the generic
timer stuff wouldn't rely on it being kept alive?
Hmm, it looks like we bypass the clockevents code by only setting the
TWD load value at initialization time, not when we switch to periodic
mode. We really ought to rewrite it whenever we switch back to periodic
mode.
I suspect fixing that means you won't need this save/restore support.
Untested, but should do what's required.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fd91566..60636f4 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -36,6 +36,7 @@ static void twd_set_mode(enum clock_event_mode mode,
/* timer load already set up */
ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
| TWD_TIMER_CONTROL_PERIODIC;
+ __raw_writel(twd_timer_rate / HZ, twd_base + TWD_TIMER_LOAD);
break;
case CLOCK_EVT_MODE_ONESHOT:
/* period set, and timer enabled in 'next_event' hook */
@@ -81,7 +82,7 @@ int twd_timer_ack(void)
static void __cpuinit twd_calibrate_rate(void)
{
- unsigned long load, count;
+ unsigned long count;
u64 waitjiffies;
/*
@@ -116,10 +117,6 @@ static void __cpuinit twd_calibrate_rate(void)
printk(%lu.%02luMHz.\n, twd_timer_rate / 100,
(twd_timer_rate / 100) % 100);
}
-
- load = twd_timer_rate / HZ;
-
- __raw_writel(load, twd_base + TWD_TIMER_LOAD);
}
/*
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.
Shadowing ctrl in a percpu variable and rewriting it during every call
to twd_set_next_event does work, but that's not much different, and a
little less efficient, than just saving and restoring the control and
load registers in idle. It does have the advantage that platforms
don't need any extra calls.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
