From: Magnus Damm <[email protected]>
Modify the ARM architected timer driver to not set C3STOP
in case CPU_IDLE is disabled. This is a short term fix that
allows use of high resolution timers even though no additional
clock event is registered.
Not-really-Signed-off-by: Magnus Damm <[email protected]>
---
If someone cares about this case then perhaps it should be
moved up to the clock event main code. The same issue should
in theory trigger on all architectures, perhaps x86 people
hunting for low latency may try to disable CPU_IDLE?
I propose carrying this patch locally to enable high resolution
timers until CPU_IDLE and an additional clock event is supported.
Observed on r8a73a4 and APE6EVM.
drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
--- 0001/drivers/clocksource/arm_arch_timer.c
+++ work/drivers/clocksource/arm_arch_timer.c 2013-06-17 09:03:44.000000000 +0900
@@ -125,7 +125,23 @@ static int arch_timer_set_next_event_phy
static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
{
- clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
+ clk->features = CLOCK_EVT_FEAT_ONESHOT;
+#ifdef CONFIG_CPU_IDLE
+ /* By not setting the C3STOP flag it is possible to let the
+ * ARM architected timer to be the only clock event installed
+ * on the system and have working high resolution timers.
+ *
+ * If the C3STOP flag is set unconditionally then the kernel
+ * will always prevent using the high resoultion timer feature
+ * unless an additional clock event is registered.
+ *
+ * In the case where CPU_IDLE is enabled then there is a chance
+ * that deeper sleep states will be handled by software, but
+ * if CPU_IDLE is disabled then deep sleep states cannot be
+ * entered and the feature flagged by C3STOP is not needed.
+ */
+ clk->features |= CLOCK_EVT_FEAT_C3STOP;
+#endif
clk->name = "arch_sys_timer";
clk->rating = 450;
if (arch_timer_use_virtual) {
On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
>
> Modify the ARM architected timer driver to not set C3STOP
> in case CPU_IDLE is disabled. This is a short term fix that
> allows use of high resolution timers even though no additional
> clock event is registered.
>
> Not-really-Signed-off-by: Magnus Damm <[email protected]>
> ---
>
> If someone cares about this case then perhaps it should be
> moved up to the clock event main code. The same issue should
> in theory trigger on all architectures, perhaps x86 people
> hunting for low latency may try to disable CPU_IDLE?
>
> I propose carrying this patch locally to enable high resolution
> timers until CPU_IDLE and an additional clock event is supported.
>
> Observed on r8a73a4 and APE6EVM.
Hi Magnus,
Is this patch intended to be picked up by me for the LTSI-3.4.25 based
backports that live in my renesas-backports tree?
If so, could you clearly state this (below the '---' is fine) and
include a proper Sob line to indicate that it is fit to be merged
even if that merge is not into mainline.
Thanks
>
> drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> --- 0001/drivers/clocksource/arm_arch_timer.c
> +++ work/drivers/clocksource/arm_arch_timer.c 2013-06-17 09:03:44.000000000 +0900
> @@ -125,7 +125,23 @@ static int arch_timer_set_next_event_phy
>
> static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
> {
> - clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
> + clk->features = CLOCK_EVT_FEAT_ONESHOT;
> +#ifdef CONFIG_CPU_IDLE
> + /* By not setting the C3STOP flag it is possible to let the
> + * ARM architected timer to be the only clock event installed
> + * on the system and have working high resolution timers.
> + *
> + * If the C3STOP flag is set unconditionally then the kernel
> + * will always prevent using the high resoultion timer feature
> + * unless an additional clock event is registered.
> + *
> + * In the case where CPU_IDLE is enabled then there is a chance
> + * that deeper sleep states will be handled by software, but
> + * if CPU_IDLE is disabled then deep sleep states cannot be
> + * entered and the feature flagged by C3STOP is not needed.
> + */
> + clk->features |= CLOCK_EVT_FEAT_C3STOP;
> +#endif
> clk->name = "arch_sys_timer";
> clk->rating = 450;
> if (arch_timer_use_virtual) {
>
Hi Simon,
On Mon, Jun 17, 2013 at 11:13 AM, Simon Horman <[email protected]> wrote:
> On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote:
>> From: Magnus Damm <[email protected]>
>>
>> Modify the ARM architected timer driver to not set C3STOP
>> in case CPU_IDLE is disabled. This is a short term fix that
>> allows use of high resolution timers even though no additional
>> clock event is registered.
>>
>> Not-really-Signed-off-by: Magnus Damm <[email protected]>
>> ---
>>
>> If someone cares about this case then perhaps it should be
>> moved up to the clock event main code. The same issue should
>> in theory trigger on all architectures, perhaps x86 people
>> hunting for low latency may try to disable CPU_IDLE?
>>
>> I propose carrying this patch locally to enable high resolution
>> timers until CPU_IDLE and an additional clock event is supported.
>>
>> Observed on r8a73a4 and APE6EVM.
>
> Hi Magnus,
>
> Is this patch intended to be picked up by me for the LTSI-3.4.25 based
> backports that live in my renesas-backports tree?
Yes, correct.
The patch was mainly written to satisfy a feature request for your
backports, but I noticed that the same issue exists in upstream as
well.
Ideally I'd like to use the same code for the backport and upstream,
but I am not sure if anyone in upstream really cares. The more long
term solution is obviously to install a second clock event, perhaps
that's good enough.
> If so, could you clearly state this (below the '---' is fine) and
> include a proper Sob line to indicate that it is fit to be merged
> even if that merge is not into mainline.
Sure, but I'd like to hear opinions from other people before
resending. I will follow your recommendation in next version.
Thanks,
/ magnus
On Mon, Jun 17, 2013 at 11:47:11AM +0900, Magnus Damm wrote:
> Hi Simon,
>
> On Mon, Jun 17, 2013 at 11:13 AM, Simon Horman <[email protected]> wrote:
> > On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <[email protected]>
> >>
> >> Modify the ARM architected timer driver to not set C3STOP
> >> in case CPU_IDLE is disabled. This is a short term fix that
> >> allows use of high resolution timers even though no additional
> >> clock event is registered.
> >>
> >> Not-really-Signed-off-by: Magnus Damm <[email protected]>
> >> ---
> >>
> >> If someone cares about this case then perhaps it should be
> >> moved up to the clock event main code. The same issue should
> >> in theory trigger on all architectures, perhaps x86 people
> >> hunting for low latency may try to disable CPU_IDLE?
> >>
> >> I propose carrying this patch locally to enable high resolution
> >> timers until CPU_IDLE and an additional clock event is supported.
> >>
> >> Observed on r8a73a4 and APE6EVM.
> >
> > Hi Magnus,
> >
> > Is this patch intended to be picked up by me for the LTSI-3.4.25 based
> > backports that live in my renesas-backports tree?
>
> Yes, correct.
>
> The patch was mainly written to satisfy a feature request for your
> backports, but I noticed that the same issue exists in upstream as
> well.
>
> Ideally I'd like to use the same code for the backport and upstream,
> but I am not sure if anyone in upstream really cares. The more long
> term solution is obviously to install a second clock event, perhaps
> that's good enough.
>
> > If so, could you clearly state this (below the '---' is fine) and
> > include a proper Sob line to indicate that it is fit to be merged
> > even if that merge is not into mainline.
>
> Sure, but I'd like to hear opinions from other people before
> resending. I will follow your recommendation in next version.
Thanks, I understand. I'll wait for discussion and a new version.
On Mon, Jun 17, 2013 at 01:20:56AM +0100, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
>
> Modify the ARM architected timer driver to not set C3STOP
> in case CPU_IDLE is disabled. This is a short term fix that
> allows use of high resolution timers even though no additional
> clock event is registered.
>
> Not-really-Signed-off-by: Magnus Damm <[email protected]>
> ---
>
> If someone cares about this case then perhaps it should be
> moved up to the clock event main code. The same issue should
> in theory trigger on all architectures, perhaps x86 people
> hunting for low latency may try to disable CPU_IDLE?
I think that changing tick_is_oneshot_capable and friends to only worry about
C3STOP when CPU_IDLE is enabled would be a nicer solution. That way you enable
all clock_event_devices with C3STOP to function as high resolution timers when
CPU_IDLE's selected. Presenting the hardware differently depending on CPU_IDLE
feels wrong.
Having some other clock_event_device would be a nicer solution still.
Thanks,
Mark.
>
> I propose carrying this patch locally to enable high resolution
> timers until CPU_IDLE and an additional clock event is supported.
>
> Observed on r8a73a4 and APE6EVM.
>
> drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> --- 0001/drivers/clocksource/arm_arch_timer.c
> +++ work/drivers/clocksource/arm_arch_timer.c 2013-06-17 09:03:44.000000000 +0900
> @@ -125,7 +125,23 @@ static int arch_timer_set_next_event_phy
>
> static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
> {
> - clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
> + clk->features = CLOCK_EVT_FEAT_ONESHOT;
> +#ifdef CONFIG_CPU_IDLE
> + /* By not setting the C3STOP flag it is possible to let the
> + * ARM architected timer to be the only clock event installed
> + * on the system and have working high resolution timers.
> + *
> + * If the C3STOP flag is set unconditionally then the kernel
> + * will always prevent using the high resoultion timer feature
> + * unless an additional clock event is registered.
> + *
> + * In the case where CPU_IDLE is enabled then there is a chance
> + * that deeper sleep states will be handled by software, but
> + * if CPU_IDLE is disabled then deep sleep states cannot be
> + * entered and the feature flagged by C3STOP is not needed.
> + */
> + clk->features |= CLOCK_EVT_FEAT_C3STOP;
> +#endif
> clk->name = "arch_sys_timer";
> clk->rating = 450;
> if (arch_timer_use_virtual) {
>
Hi Mark,
On Mon, Jun 17, 2013 at 11:53 PM, Mark Rutland <[email protected]> wrote:
> On Mon, Jun 17, 2013 at 01:20:56AM +0100, Magnus Damm wrote:
>> From: Magnus Damm <[email protected]>
>>
>> Modify the ARM architected timer driver to not set C3STOP
>> in case CPU_IDLE is disabled. This is a short term fix that
>> allows use of high resolution timers even though no additional
>> clock event is registered.
>>
>> Not-really-Signed-off-by: Magnus Damm <[email protected]>
>> ---
>>
>> If someone cares about this case then perhaps it should be
>> moved up to the clock event main code. The same issue should
>> in theory trigger on all architectures, perhaps x86 people
>> hunting for low latency may try to disable CPU_IDLE?
>
> I think that changing tick_is_oneshot_capable and friends to only worry about
> C3STOP when CPU_IDLE is enabled would be a nicer solution. That way you enable
> all clock_event_devices with C3STOP to function as high resolution timers when
> CPU_IDLE's selected. Presenting the hardware differently depending on CPU_IDLE
> feels wrong.
I agree that doing this in the clock event driver is a bit odd. So
because of that I just posted this patch:
[PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled
> Having some other clock_event_device would be a nicer solution still.
No doubt that another clock event device helps, but mainly together
with CPU Idle IMO.
Thanks for your comments!
/ magnus