[2/2] ARM: omap: timers: reduce rating of gp_timer clocksource

Commit Message

J, KEERTHY Nov. 24, 2016, 6:19 a.m. UTC

From: Grygorii Strashko <[e-mail]>

Now ARM Global timer (rating 300) will not be selected as clocksource,
because it's initialized after OMAP GP Timer (rating 300) and
Timekeeping core will not allow to replace clocksource with new one if
both of them have the same rating.

Reduce rating of OMAP GP Timer (300->290) when it's used as
clocksource device - this will allow to select ARM Global timer (300)
as clocksource when enabled.

Signed-off-by: Grygorii Strashko <[e-mail]>
Signed-off-by: Dave Gerlach <[e-mail]>
Signed-off-by: Keerthy <[e-mail]>
---
 arch/arm/mach-omap2/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Grygorii Strashko Nov. 29, 2016, 4:43 p.m. UTC | #1

On 11/24/2016 12:19 AM, Keerthy wrote:
> From: Grygorii Strashko <[e-mail]>
>
> Now ARM Global timer (rating 300) will not be selected as clocksource,
> because it's initialized after OMAP GP Timer (rating 300) and
> Timekeeping core will not allow to replace clocksource with new one if
> both of them have the same rating.
>
> Reduce rating of OMAP GP Timer (300->290) when it's used as
> clocksource device - this will allow to select ARM Global timer (300)
> as clocksource when enabled.
>
> Signed-off-by: Grygorii Strashko <[e-mail]>
> Signed-off-by: Dave Gerlach <[e-mail]>
> Signed-off-by: Keerthy <[e-mail]>

Unfortunately, this patch has dependency [1] and can't be used alone as
it will cause ARM Global timer to be selected as clocksource
always on am437x and this will kill cpuidle, because ARM Global timer
is not in always_on domain.

The intention of enabling ARM Global timer is only for non-pm aware use
cases for RT-kernel latency improvement - where deep cpuidle states are 
not enabled.

[1] https://patchwork.kernel.org/patch/8940051/

> ---
>  arch/arm/mach-omap2/timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b2f2448..a0dbb0b 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -376,7 +376,7 @@ static cycle_t clocksource_read_cycles(struct clocksource *cs)
>  }
>
>  static struct clocksource clocksource_gpt = {
> -	.rating		= 300,
> +	.rating		= 290,
>  	.read		= clocksource_read_cycles,
>  	.mask		= CLOCKSOURCE_MASK(32),
>  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
>

Tony Lindgren Dec. 2, 2016, 4:47 p.m. UTC | #2

* Grygorii Strashko <[e-mail]> [161129 08:43]:
> 
> 
> On 11/24/2016 12:19 AM, Keerthy wrote:
> > From: Grygorii Strashko <[e-mail]>
> > 
> > Now ARM Global timer (rating 300) will not be selected as clocksource,
> > because it's initialized after OMAP GP Timer (rating 300) and
> > Timekeeping core will not allow to replace clocksource with new one if
> > both of them have the same rating.
> > 
> > Reduce rating of OMAP GP Timer (300->290) when it's used as
> > clocksource device - this will allow to select ARM Global timer (300)
> > as clocksource when enabled.
> > 
> > Signed-off-by: Grygorii Strashko <[e-mail]>
> > Signed-off-by: Dave Gerlach <[e-mail]>
> > Signed-off-by: Keerthy <[e-mail]>
> 
> Unfortunately, this patch has dependency [1] and can't be used alone as
> it will cause ARM Global timer to be selected as clocksource
> always on am437x and this will kill cpuidle, because ARM Global timer
> is not in always_on domain.
> 
> The intention of enabling ARM Global timer is only for non-pm aware use
> cases for RT-kernel latency improvement - where deep cpuidle states are not
> enabled.

Yeah we need to fix up things to be able to change the clocksource
in addition to clockevent. However, currently only cpuidle_coupled
knows when the whole system is idle, so quite a bit of work is
needed to do that in a sane way.

What about the first patch in this series?

Regards,

Tony


> [1] https://patchwork.kernel.org/patch/8940051/
> 
> > ---
> >  arch/arm/mach-omap2/timer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> > index b2f2448..a0dbb0b 100644
> > --- a/arch/arm/mach-omap2/timer.c
> > +++ b/arch/arm/mach-omap2/timer.c
> > @@ -376,7 +376,7 @@ static cycle_t clocksource_read_cycles(struct clocksource *cs)
> >  }
> > 
> >  static struct clocksource clocksource_gpt = {
> > -	.rating		= 300,
> > +	.rating		= 290,
> >  	.read		= clocksource_read_cycles,
> >  	.mask		= CLOCKSOURCE_MASK(32),
> >  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> > 
> 
> -- 
> regards,
> -grygorii

Grygorii Strashko Dec. 2, 2016, 6:02 p.m. UTC | #3

On 12/02/2016 10:47 AM, Tony Lindgren wrote:
> * Grygorii Strashko <[e-mail]> [161129 08:43]:
>>
>>
>> On 11/24/2016 12:19 AM, Keerthy wrote:
>>> From: Grygorii Strashko <[e-mail]>
>>>
>>> Now ARM Global timer (rating 300) will not be selected as clocksource,
>>> because it's initialized after OMAP GP Timer (rating 300) and
>>> Timekeeping core will not allow to replace clocksource with new one if
>>> both of them have the same rating.
>>>
>>> Reduce rating of OMAP GP Timer (300->290) when it's used as
>>> clocksource device - this will allow to select ARM Global timer (300)
>>> as clocksource when enabled.
>>>
>>> Signed-off-by: Grygorii Strashko <[e-mail]>
>>> Signed-off-by: Dave Gerlach <[e-mail]>
>>> Signed-off-by: Keerthy <[e-mail]>
>>
>> Unfortunately, this patch has dependency [1] and can't be used alone as
>> it will cause ARM Global timer to be selected as clocksource
>> always on am437x and this will kill cpuidle, because ARM Global timer
>> is not in always_on domain.
>>
>> The intention of enabling ARM Global timer is only for non-pm aware use
>> cases for RT-kernel latency improvement - where deep cpuidle states are not
>> enabled.
>
> Yeah we need to fix up things to be able to change the clocksource
> in addition to clockevent. However, currently only cpuidle_coupled
> knows when the whole system is idle, so quite a bit of work is
> needed to do that in a sane way.

Also sched_clock and timer_delay ;)


>
> What about the first patch in this series?
>

Fist one is ok. It was originally posted long time ago.

Tony Lindgren Dec. 2, 2016, 6:31 p.m. UTC | #4

* Grygorii Strashko <[e-mail]> [161202 10:02]:
> 
> 
> On 12/02/2016 10:47 AM, Tony Lindgren wrote:
> > * Grygorii Strashko <[e-mail]> [161129 08:43]:
> > > 
> > > 
> > > On 11/24/2016 12:19 AM, Keerthy wrote:
> > > > From: Grygorii Strashko <[e-mail]>
> > > > 
> > > > Now ARM Global timer (rating 300) will not be selected as clocksource,
> > > > because it's initialized after OMAP GP Timer (rating 300) and
> > > > Timekeeping core will not allow to replace clocksource with new one if
> > > > both of them have the same rating.
> > > > 
> > > > Reduce rating of OMAP GP Timer (300->290) when it's used as
> > > > clocksource device - this will allow to select ARM Global timer (300)
> > > > as clocksource when enabled.
> > > > 
> > > > Signed-off-by: Grygorii Strashko <[e-mail]>
> > > > Signed-off-by: Dave Gerlach <[e-mail]>
> > > > Signed-off-by: Keerthy <[e-mail]>
> > > 
> > > Unfortunately, this patch has dependency [1] and can't be used alone as
> > > it will cause ARM Global timer to be selected as clocksource
> > > always on am437x and this will kill cpuidle, because ARM Global timer
> > > is not in always_on domain.
> > > 
> > > The intention of enabling ARM Global timer is only for non-pm aware use
> > > cases for RT-kernel latency improvement - where deep cpuidle states are not
> > > enabled.
> > 
> > Yeah we need to fix up things to be able to change the clocksource
> > in addition to clockevent. However, currently only cpuidle_coupled
> > knows when the whole system is idle, so quite a bit of work is
> > needed to do that in a sane way.
> 
> Also sched_clock and timer_delay ;)

Yeah sched_clock would need something to save and restore it..
Not nice :)

> > What about the first patch in this series?
> > 
> 
> Fist one is ok. It was originally posted long time ago.

OK so can you please resend that one more time with proper
Fixes tag on it?

Regards,

Tony

J, KEERTHY Dec. 5, 2016, 4:12 a.m. UTC | #5

On Saturday 03 December 2016 12:01 AM, Tony Lindgren wrote:
> * Grygorii Strashko <[e-mail]> [161202 10:02]:
>>
>>
>> On 12/02/2016 10:47 AM, Tony Lindgren wrote:
>>> * Grygorii Strashko <[e-mail]> [161129 08:43]:
>>>>
>>>>
>>>> On 11/24/2016 12:19 AM, Keerthy wrote:
>>>>> From: Grygorii Strashko <[e-mail]>
>>>>>
>>>>> Now ARM Global timer (rating 300) will not be selected as clocksource,
>>>>> because it's initialized after OMAP GP Timer (rating 300) and
>>>>> Timekeeping core will not allow to replace clocksource with new one if
>>>>> both of them have the same rating.
>>>>>
>>>>> Reduce rating of OMAP GP Timer (300->290) when it's used as
>>>>> clocksource device - this will allow to select ARM Global timer (300)
>>>>> as clocksource when enabled.
>>>>>
>>>>> Signed-off-by: Grygorii Strashko <[e-mail]>
>>>>> Signed-off-by: Dave Gerlach <[e-mail]>
>>>>> Signed-off-by: Keerthy <[e-mail]>
>>>>
>>>> Unfortunately, this patch has dependency [1] and can't be used alone as
>>>> it will cause ARM Global timer to be selected as clocksource
>>>> always on am437x and this will kill cpuidle, because ARM Global timer
>>>> is not in always_on domain.
>>>>
>>>> The intention of enabling ARM Global timer is only for non-pm aware use
>>>> cases for RT-kernel latency improvement - where deep cpuidle states are not
>>>> enabled.
>>>
>>> Yeah we need to fix up things to be able to change the clocksource
>>> in addition to clockevent. However, currently only cpuidle_coupled
>>> knows when the whole system is idle, so quite a bit of work is
>>> needed to do that in a sane way.
>>
>> Also sched_clock and timer_delay ;)
>
> Yeah sched_clock would need something to save and restore it..
> Not nice :)
>
>>> What about the first patch in this series?
>>>
>>
>> Fist one is ok. It was originally posted long time ago.
>
> OK so can you please resend that one more time with proper
> Fixes tag on it?

Tony,

I have posted a v2 with the proper Fixes tag.

- Keerthy
>
> Regards,
>
> Tony
>

Patch

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index b2f2448..a0dbb0b 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -376,7 +376,7 @@  static cycle_t clocksource_read_cycles(struct clocksource *cs)
 }
 
 static struct clocksource clocksource_gpt = {
-	.rating		= 300,
+	.rating		= 290,
 	.read		= clocksource_read_cycles,
 	.mask		= CLOCKSOURCE_MASK(32),
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,