[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n

Commit Message

Magnus Damm June 17, 2013, 12:20 a.m. UTC

From: Magnus Damm <[e-mail]>

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 <[e-mail]>
---

 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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Simon Horman June 17, 2013, 2:13 a.m. UTC | #1

On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote:
> From: Magnus Damm <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 
>  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) {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm June 17, 2013, 2:47 a.m. UTC | #2

Hi Simon,

On Mon, Jun 17, 2013 at 11:13 AM, Simon Horman <[e-mail]> wrote:
> On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote:
>> From: Magnus Damm <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Simon Horman June 17, 2013, 2:57 a.m. UTC | #3

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 <[e-mail]> wrote:
> > On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> ---
> >>
> >>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland June 17, 2013, 2:53 p.m. UTC | #4

On Mon, Jun 17, 2013 at 01:20:56AM +0100, Magnus Damm wrote:
> From: Magnus Damm <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 
>  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) {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Magnus Damm June 18, 2013, 7:32 a.m. UTC | #5

Hi Mark,

On Mon, Jun 17, 2013 at 11:53 PM, Mark Rutland <[e-mail]> wrote:
> On Mon, Jun 17, 2013 at 01:20:56AM +0100, Magnus Damm wrote:
>> From: Magnus Damm <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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) {