schedule_timeout sleeps too long after dividing CPU frequency

schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

Hello,

I'm working on a Cortex A9 based platform.

I have a basic clock tree, and a very basic cpufreq driver using
mostly generic driver glue:

static struct cpufreq_driver tangox_cpufreq_driver = {
	.name		= "tangox-cpufreq",
	.init		= tangox_cpu_init,
	.verify		= cpufreq_generic_frequency_table_verify,
	.target_index	= tangox_target,
	.get		= cpufreq_generic_get,
	.exit		= cpufreq_generic_exit,
	.attr		= cpufreq_generic_attr,
};

My target_index function is trivial:

static int tangox_target(struct cpufreq_policy *policy, unsigned int idx)
{
	return clk_set_rate(policy->clk, freq_table[idx].frequency * 1000);
}

I was testing an unrelated driver at low frequencies, with the nominal
frequency (999 MHz) divided by 54 (i.e. freq = 18.5 MHz) and I noticed
that when the driver calls

    schedule_timeout(HZ);

the thread sleeps 54 seconds instead of 1.

[ 1107.827279] Sleep for 300 jiffies.
[ 1161.838513] Done sleeping.

(I have HZ set to 300)

I enabled DEBUG in the generic cpufreq driver to see what happens
when I request a frequency change:

# cd /sys/devices/system/cpu/cpu0/cpufreq
# cat scaling_governor
performance
# cat scaling_available_frequencies
999000 499500 333000 199800 111000 18500
# echo 18500 > scaling_max_freq
[   19.597257] cpufreq: setting new policy for CPU 0: 18500 - 18500 kHz
[   19.604017] cpufreq: new min and max freqs are 18500 - 18500 kHz
[   19.610345] cpufreq: governor: change or update limits
[   19.615596] cpufreq: __cpufreq_governor for CPU 0, event 3
[   19.621186] cpufreq: target for CPU 0: 18500 kHz, relation 1, requested 18500 kHz
[   19.628783] cpufreq: __cpufreq_driver_target: cpu: 0, oldfreq: 999000, new freq: 18500
[   19.636818] cpufreq: notification 0 of frequency transition to 18500 kHz
[   19.643625] cpufreq: notification 0 of frequency transition to 18500 kHz
[   19.650454] NEW RATE=9250000
[   19.653644] NEW RATE=9250000
[   19.657091] cpufreq: notification 1 of frequency transition to 18500 kHz
[   19.664176] cpufreq: FREQ: 18500 - CPU: 0
[   19.668412] cpufreq: notification 1 of frequency transition to 18500 kHz
[   19.675648] cpufreq: FREQ: 18500 - CPU: 1

The "NEW RATE" traces are ones I added in smp_twd.c:twd_update_frequency()
(my clockevent source) to check whether the CPU frequency change propagated
to smp_twd.

I must have made an obvious mistake. Could someone point it out to me?
(I have attached the output of /proc/timer_list to this message)

Looking more closely at schedule_timeout(), I suppose the work happens
in __mod_timer()... Hmm, that one is over my head.

What am I missing?

Regards.

[-- Attachment #2: timer_list --]
[-- Type: text/plain, Size: 3403 bytes --]

Timer List Version: v0.7
HRTIMER_MAX_CLOCK_BASES: 4
now at 1996236245543 nsecs

cpu: 0
 clock 0:
  .base:       e7ae51e8
  .index:      0
  .resolution: 3333333 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
 #0: sched_clock_timer, sched_clock_poll, S:01
 # expires at 2087831315985-2087831315985 nsecs [in 91595070442 to 91595070442 nsecs]
 clock 1:
  .base:       e7ae5220
  .index:      1
  .resolution: 3333333 nsecs
  .get_time:   ktime_get_real
  .offset:     0 nsecs
active timers:
 clock 2:
  .base:       e7ae5258
  .index:      2
  .resolution: 3333333 nsecs
  .get_time:   ktime_get_boottime
  .offset:     0 nsecs
active timers:
 clock 3:
  .base:       e7ae5290
  .index:      3
  .resolution: 3333333 nsecs
  .get_time:   ktime_get_clocktai
  .offset:     0 nsecs
active timers:
  .expires_next   : 9223372036854775807 nsecs
  .hres_active    : 0
  .nr_events      : 0
  .nr_retries     : 0
  .nr_hangs       : 0
  .max_hang_time  : 0 nsecs
  .nohz_mode      : 0
  .last_tick      : 0 nsecs
  .tick_stopped   : 0
  .idle_jiffies   : 0
  .idle_calls     : 0
  .idle_sleeps    : 0
  .idle_entrytime : 1996229420769 nsecs
  .idle_waketime  : 0 nsecs
  .idle_exittime  : 0 nsecs
  .idle_sleeptime : 1985388003747 nsecs
  .iowait_sleeptime: 10995516 nsecs
  .last_jiffies   : 0
  .next_jiffies   : 0
  .idle_expires   : 0 nsecs
jiffies: 4294894041

cpu: 1
 clock 0:
  .base:       e7aed1e8
  .index:      0
  .resolution: 3333333 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
 clock 1:
  .base:       e7aed220
  .index:      1
  .resolution: 3333333 nsecs
  .get_time:   ktime_get_real
  .offset:     0 nsecs
active timers:
 clock 2:
  .base:       e7aed258
  .index:      2
  .resolution: 3333333 nsecs
  .get_time:   ktime_get_boottime
  .offset:     0 nsecs
active timers:
 clock 3:
  .base:       e7aed290
  .index:      3
  .resolution: 3333333 nsecs
  .get_time:   ktime_get_clocktai
  .offset:     0 nsecs
active timers:
  .expires_next   : 9223372036854775807 nsecs
  .hres_active    : 0
  .nr_events      : 0
  .nr_retries     : 0
  .nr_hangs       : 0
  .max_hang_time  : 0 nsecs
  .nohz_mode      : 0
  .last_tick      : 0 nsecs
  .tick_stopped   : 0
  .idle_jiffies   : 0
  .idle_calls     : 0
  .idle_sleeps    : 0
  .idle_entrytime : 1996231929568 nsecs
  .idle_waketime  : 0 nsecs
  .idle_exittime  : 0 nsecs
  .idle_sleeptime : 1992784761637 nsecs
  .iowait_sleeptime: 40375788 nsecs
  .last_jiffies   : 0
  .next_jiffies   : 0
  .idle_expires   : 0 nsecs
jiffies: 4294894041

Tick Device: mode:     0
Broadcast device
Clock Event Device: <NULL>
tick_broadcast_mask: 00000000
tick_broadcast_oneshot_mask: 00000000

Tick Device: mode:     0
Per CPU device: 0
Clock Event Device: local_timer
 max_delta_ns:   464320782665
 min_delta_ns:   1622
 mult:           19864224
 shift:          31
 mode:           2
 next_event:     9223372036854775807 nsecs
 set_next_event: twd_set_next_event
 set_mode:       twd_set_mode
 event_handler:  tick_handle_periodic
 retries:        0

Tick Device: mode:     0
Per CPU device: 1
Clock Event Device: local_timer
 max_delta_ns:   464320782665
 min_delta_ns:   1622
 mult:           19864224
 shift:          31
 mode:           2
 next_event:     9223372036854775807 nsecs
 set_next_event: twd_set_next_event
 set_mode:       twd_set_mode
 event_handler:  tick_handle_periodic
 retries:        0

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

CC'ing Daniel..

On 12 May 2015 at 20:02, Mason <slash.tmp@free.fr> wrote:
> Hello,
>
> I'm working on a Cortex A9 based platform.
>
> I have a basic clock tree, and a very basic cpufreq driver using
> mostly generic driver glue:
>
> static struct cpufreq_driver tangox_cpufreq_driver = {
>         .name           = "tangox-cpufreq",
>         .init           = tangox_cpu_init,
>         .verify         = cpufreq_generic_frequency_table_verify,
>         .target_index   = tangox_target,
>         .get            = cpufreq_generic_get,
>         .exit           = cpufreq_generic_exit,
>         .attr           = cpufreq_generic_attr,
> };
>
> My target_index function is trivial:
>
> static int tangox_target(struct cpufreq_policy *policy, unsigned int idx)
> {
>         return clk_set_rate(policy->clk, freq_table[idx].frequency * 1000);
> }
>
> I was testing an unrelated driver at low frequencies, with the nominal
> frequency (999 MHz) divided by 54 (i.e. freq = 18.5 MHz) and I noticed
> that when the driver calls
>
>     schedule_timeout(HZ);
>
> the thread sleeps 54 seconds instead of 1.
>
> [ 1107.827279] Sleep for 300 jiffies.
> [ 1161.838513] Done sleeping.
>
> (I have HZ set to 300)
>
> I enabled DEBUG in the generic cpufreq driver to see what happens
> when I request a frequency change:
>
> # cd /sys/devices/system/cpu/cpu0/cpufreq
> # cat scaling_governor
> performance
> # cat scaling_available_frequencies
> 999000 499500 333000 199800 111000 18500
> # echo 18500 > scaling_max_freq
> [   19.597257] cpufreq: setting new policy for CPU 0: 18500 - 18500 kHz
> [   19.604017] cpufreq: new min and max freqs are 18500 - 18500 kHz
> [   19.610345] cpufreq: governor: change or update limits
> [   19.615596] cpufreq: __cpufreq_governor for CPU 0, event 3
> [   19.621186] cpufreq: target for CPU 0: 18500 kHz, relation 1, requested 18500 kHz
> [   19.628783] cpufreq: __cpufreq_driver_target: cpu: 0, oldfreq: 999000, new freq: 18500
> [   19.636818] cpufreq: notification 0 of frequency transition to 18500 kHz
> [   19.643625] cpufreq: notification 0 of frequency transition to 18500 kHz
> [   19.650454] NEW RATE=9250000
> [   19.653644] NEW RATE=9250000
> [   19.657091] cpufreq: notification 1 of frequency transition to 18500 kHz
> [   19.664176] cpufreq: FREQ: 18500 - CPU: 0
> [   19.668412] cpufreq: notification 1 of frequency transition to 18500 kHz
> [   19.675648] cpufreq: FREQ: 18500 - CPU: 1
>
> The "NEW RATE" traces are ones I added in smp_twd.c:twd_update_frequency()
> (my clockevent source) to check whether the CPU frequency change propagated
> to smp_twd.
>
> I must have made an obvious mistake. Could someone point it out to me?
> (I have attached the output of /proc/timer_list to this message)
>
> Looking more closely at schedule_timeout(), I suppose the work happens
> in __mod_timer()... Hmm, that one is over my head.
>
> What am I missing?

A Broadcast device ? So you have two CPUs with a local timer each, whose
expires-next is set to KTIME_MAX (infinite).... So you will get interrupted
once the counter has overflown, probably that's what 54 seconds is all about..

Probably your CPUs are going into idle and no one is waking them up and that's
why you need a broadcast device, i.e. another global timer on your SoC, outside
of the CPU subsystem.

It doesn't have anything to do with cpufreq AFAICT ..

--
viresh

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 12/05/2015 16:46, Viresh Kumar wrote:

> On 12 May 2015 at 20:02, Mason wrote:
>
>> I'm working on a Cortex A9 based platform.
>>
>> I have a basic clock tree, and a very basic cpufreq driver using
>> mostly generic driver glue:
>>
>> static struct cpufreq_driver tangox_cpufreq_driver = {
>>         .name           = "tangox-cpufreq",
>>         .init           = tangox_cpu_init,
>>         .verify         = cpufreq_generic_frequency_table_verify,
>>         .target_index   = tangox_target,
>>         .get            = cpufreq_generic_get,
>>         .exit           = cpufreq_generic_exit,
>>         .attr           = cpufreq_generic_attr,
>> };
>>
>> My target_index function is trivial:
>>
>> static int tangox_target(struct cpufreq_policy *policy, unsigned int idx)
>> {
>>         return clk_set_rate(policy->clk, freq_table[idx].frequency * 1000);
>> }
>>
>> I was testing an unrelated driver at low frequencies, with the nominal
>> frequency (999 MHz) divided by 54 (i.e. freq = 18.5 MHz) and I noticed
>> that when the driver calls
>>
>>     schedule_timeout(HZ);
>>
>> the thread sleeps 54 seconds instead of 1.
>>
>> [ 1107.827279] Sleep for 300 jiffies.
>> [ 1161.838513] Done sleeping.
>>
>> (I have HZ set to 300)
>>
>> I enabled DEBUG in the generic cpufreq driver to see what happens
>> when I request a frequency change:
>>
>> # cd /sys/devices/system/cpu/cpu0/cpufreq
>> # cat scaling_governor
>> performance
>> # cat scaling_available_frequencies
>> 999000 499500 333000 199800 111000 18500
>> # echo 18500 > scaling_max_freq
>> [   19.597257] cpufreq: setting new policy for CPU 0: 18500 - 18500 kHz
>> [   19.604017] cpufreq: new min and max freqs are 18500 - 18500 kHz
>> [   19.610345] cpufreq: governor: change or update limits
>> [   19.615596] cpufreq: __cpufreq_governor for CPU 0, event 3
>> [   19.621186] cpufreq: target for CPU 0: 18500 kHz, relation 1, requested 18500 kHz
>> [   19.628783] cpufreq: __cpufreq_driver_target: cpu: 0, oldfreq: 999000, new freq: 18500
>> [   19.636818] cpufreq: notification 0 of frequency transition to 18500 kHz
>> [   19.643625] cpufreq: notification 0 of frequency transition to 18500 kHz
>> [   19.650454] NEW RATE=9250000
>> [   19.653644] NEW RATE=9250000
>> [   19.657091] cpufreq: notification 1 of frequency transition to 18500 kHz
>> [   19.664176] cpufreq: FREQ: 18500 - CPU: 0
>> [   19.668412] cpufreq: notification 1 of frequency transition to 18500 kHz
>> [   19.675648] cpufreq: FREQ: 18500 - CPU: 1
>>
>> The "NEW RATE" traces are ones I added in smp_twd.c:twd_update_frequency()
>> (my clockevent source) to check whether the CPU frequency change propagated
>> to smp_twd.
>>
>> I must have made an obvious mistake. Could someone point it out to me?
>> (I have attached the output of /proc/timer_list to this message)
>>
>> Looking more closely at schedule_timeout(), I suppose the work happens
>> in __mod_timer()... Hmm, that one is over my head.
>>
>> What am I missing?
> 
> A Broadcast device ? So you have two CPUs with a local timer each, whose
> expires-next is set to KTIME_MAX (infinite).... So you will get interrupted
> once the counter has overflown, probably that's what 54 seconds is all about..

If I divide the CPU clock by 54, then
schedule_timeout(HZ) sleeps 54 seconds (instead of 1)
schedule_timeout(HZ/10) sleeps 5.4 seconds (instead of 0.1)

If I divide the CPU clock by 9, then
schedule_timeout(HZ) sleeps 9 seconds (instead of 1)
schedule_timeout(HZ/10) sleeps 0.9 seconds (instead of 0.1)

I didn't test other dividers, but I am convinced that if I divide
the CPU clock by N, schedule_timeout(HZ) will sleep N seconds.

It looks like my system is calculating the number of cycles to wait
using the nominal (higher) frequency, and so the system waits N times
too long, because the actual clock is N times slower than nominal.

> Probably your CPUs are going into idle and no one is waking them up and that's
> why you need a broadcast device, i.e. another global timer on your SoC, outside
> of the CPU subsystem.

This ties in to another thread I started in LAKML:
("High-resolution timers not supported when using smp_twd on Cortex A9")

$ git show 5388a6b2 arch/arm/kernel/smp_twd.c
commit 5388a6b266e9c3357353332ba0cd5549082887f1
Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Mon Jul 26 13:19:43 2010 +0100

    ARM: SMP: Always enable clock event broadcast support
    
    The TWD local timers are unable to wake up the CPU when it is placed
    into a low power mode, eg. C3.  Therefore, we need to adapt things
    such that the TWD code can cope with this.
    
    We do this by always providing a broadcast tick function, and marking
    the fact that the TWD local timer will stop in low power modes.  This
    means that when the CPU is placed into a low power mode, the core
    timer code marks this fact, and allows an IPI to be given to the core.

This mentions a "broadcast tick function" (of which I know nothing).
Is this what you're referring to?

> It doesn't have anything to do with cpufreq AFAICT ..

I'm sure it's not in the cpufreq driver, if that's what you mean.
It's probably something I did, or didn't do. But it happens when
I change the frequency of the CPU, so I hoped someone on cpufreq
would spot my blunder.

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Tue, May 12, 2015 at 04:32:47PM +0200, Mason wrote:
> Hello,
> 
> I'm working on a Cortex A9 based platform.
> 
> I have a basic clock tree, and a very basic cpufreq driver using
> mostly generic driver glue:
> 
> static struct cpufreq_driver tangox_cpufreq_driver = {
> 	.name		= "tangox-cpufreq",
> 	.init		= tangox_cpu_init,
> 	.verify		= cpufreq_generic_frequency_table_verify,
> 	.target_index	= tangox_target,
> 	.get		= cpufreq_generic_get,
> 	.exit		= cpufreq_generic_exit,
> 	.attr		= cpufreq_generic_attr,
> };
> 
> My target_index function is trivial:
> 
> static int tangox_target(struct cpufreq_policy *policy, unsigned int idx)
> {
> 	return clk_set_rate(policy->clk, freq_table[idx].frequency * 1000);
> }
> 
> I was testing an unrelated driver at low frequencies, with the nominal
> frequency (999 MHz) divided by 54 (i.e. freq = 18.5 MHz) and I noticed
> that when the driver calls
> 
>     schedule_timeout(HZ);
> 
> the thread sleeps 54 seconds instead of 1.

I'm guessing that this will be because your local timer changes frequency
with the CPU, which means that the clockevent which was set for one second
ends up timing out after 54 seconds.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Tue, May 12, 2015 at 05:14:15PM +0200, Mason wrote:
> This ties in to another thread I started in LAKML:
> ("High-resolution timers not supported when using smp_twd on Cortex A9")
> 
> $ git show 5388a6b2 arch/arm/kernel/smp_twd.c
> commit 5388a6b266e9c3357353332ba0cd5549082887f1
> Author: Russell King <rmk+kernel@arm.linux.org.uk>
> Date:   Mon Jul 26 13:19:43 2010 +0100
> 
>     ARM: SMP: Always enable clock event broadcast support
>     
>     The TWD local timers are unable to wake up the CPU when it is placed
>     into a low power mode, eg. C3.  Therefore, we need to adapt things
>     such that the TWD code can cope with this.
>     
>     We do this by always providing a broadcast tick function, and marking
>     the fact that the TWD local timer will stop in low power modes.  This
>     means that when the CPU is placed into a low power mode, the core
>     timer code marks this fact, and allows an IPI to be given to the core.
> 
> This mentions a "broadcast tick function" (of which I know nothing).
> Is this what you're referring to?

No.  This has nothing to do with low power modes.

How this works depends on how your kernel is configured, but essentially
it's something like this:

* The CPU which will be idling sets its local timer to wake up after N
  counter cycles, where N is calculated from the timer frequency.

* When the local timer fires, the CPU is kicked out of the idle loop, and
  it reads the current system time.  If the current system time indicates
  that the software timer set in schedule_timeout() has fired, that
  software timer fires.

If the local timer changes frequency without the idling CPU being woken
up, then the problem you're referring to can happen.

As you're not giving much information about your system (including
indicating where we might see some source code) we're not able to help
more than providing above descriptions.  Maybe if you posted your
patches so far to support the project you're working on, we could
provide better answers.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 12/05/2015 17:23, Russell King - ARM Linux wrote:

> On Tue, May 12, 2015 at 04:32:47PM +0200, Mason wrote:
> 
>> I'm working on a Cortex A9 based platform.
>>
>> I have a basic clock tree, and a very basic cpufreq driver using
>> mostly generic driver glue:
>>
>> static struct cpufreq_driver tangox_cpufreq_driver = {
>> 	.name		= "tangox-cpufreq",
>> 	.init		= tangox_cpu_init,
>> 	.verify		= cpufreq_generic_frequency_table_verify,
>> 	.target_index	= tangox_target,
>> 	.get		= cpufreq_generic_get,
>> 	.exit		= cpufreq_generic_exit,
>> 	.attr		= cpufreq_generic_attr,
>> };
>>
>> My target_index function is trivial:
>>
>> static int tangox_target(struct cpufreq_policy *policy, unsigned int idx)
>> {
>> 	return clk_set_rate(policy->clk, freq_table[idx].frequency * 1000);
>> }
>>
>> I was testing an unrelated driver at low frequencies, with the nominal
>> frequency (999 MHz) divided by 54 (i.e. freq = 18.5 MHz) and I noticed
>> that when the driver calls
>>
>>     schedule_timeout(HZ);
>>
>> the thread sleeps 54 seconds instead of 1.
> 
> I'm guessing that this will be because your local timer changes frequency
> with the CPU, which means that the clockevent which was set for one second
> ends up timing out after 54 seconds.

That's the first thing I suspected, but smp_twd.c registers a clk_notifier
to be notified of CPU frequency changes:

static struct notifier_block twd_clk_nb = {
	.notifier_call = twd_rate_change,
};

static int twd_clk_init(void)
{
	if (twd_evt && __this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
		return clk_notifier_register(twd_clk, &twd_clk_nb);

	return 0;
}

And I instrumented twd_update_frequency() to check it was being called.

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

On 12/05/2015 17:50, Russell King - ARM Linux wrote:
> On Tue, May 12, 2015 at 05:14:15PM +0200, Mason wrote:
>> This ties in to another thread I started in LAKML:
>> ("High-resolution timers not supported when using smp_twd on Cortex A9")
>>
>> $ git show 5388a6b2 arch/arm/kernel/smp_twd.c
>> commit 5388a6b266e9c3357353332ba0cd5549082887f1
>> Author: Russell King <rmk+kernel@arm.linux.org.uk>
>> Date:   Mon Jul 26 13:19:43 2010 +0100
>>
>>     ARM: SMP: Always enable clock event broadcast support
>>     
>>     The TWD local timers are unable to wake up the CPU when it is placed
>>     into a low power mode, eg. C3.  Therefore, we need to adapt things
>>     such that the TWD code can cope with this.
>>     
>>     We do this by always providing a broadcast tick function, and marking
>>     the fact that the TWD local timer will stop in low power modes.  This
>>     means that when the CPU is placed into a low power mode, the core
>>     timer code marks this fact, and allows an IPI to be given to the core.
>>
>> This mentions a "broadcast tick function" (of which I know nothing).
>> Is this what you're referring to?
> 
> No.  This has nothing to do with low power modes.
> 
> How this works depends on how your kernel is configured, but essentially
> it's something like this:
> 
> * The CPU which will be idling sets its local timer to wake up after N
>   counter cycles, where N is calculated from the timer frequency.
> 
> * When the local timer fires, the CPU is kicked out of the idle loop, and
>   it reads the current system time.  If the current system time indicates
>   that the software timer set in schedule_timeout() has fired, that
>   software timer fires.
> 
> If the local timer changes frequency without the idling CPU being woken
> up, then the problem you're referring to can happen.
> 
> As you're not giving much information about your system (including
> indicating where we might see some source code) we're not able to help
> more than providing above descriptions.  Maybe if you posted your
> patches so far to support the project you're working on, we could
> provide better answers.

Hello Russell,

I am using as much generic code as I can.

I've attached my clock registration code and cpufreq platform
driver to this message.

I plan to set up a github repo in order to share my progress
while I try to mainline the port.

Regards.


[-- Attachment #2: clock-tangox.c --]
[-- Type: text/x-csrc, Size: 4167 bytes --]

#include <linux/clocksource.h>	/* clocksource_register_khz	*/
#include <linux/sched_clock.h>	/* sched_clock_register		*/
#include <linux/clk-provider.h>	/* clk_register_fixed_rate	*/
#include <linux/clkdev.h>	/* clk_register_clkdev		*/
#include <linux/delay.h>	/* register_current_timer_delay	*/
#include <asm/smp_twd.h>	/* twd_local_timer_register	*/
#include <asm/smp_scu.h>	/* scu_a9_get_base		*/

#define FAST_RAMP		1
#define XTAL_FREQ		27000000 /* in Hz */
#define CLKGEN_BASE		0x10000
#define SYS_clkgen0_pll		(clkgen_base + 0x00)
#define SYS_cpuclk_div_ctrl	(clkgen_base + 0x24)
#define SYS_xtal_in_cnt		(clkgen_base + 0x48)

static void __iomem *clkgen_base;

static unsigned long read_xtal_counter(void)
{
	return readl_relaxed(SYS_xtal_in_cnt);
}

static u64 read_sched_clock(void)
{
	return read_xtal_counter();
}

static cycle_t read_clocksource(struct clocksource *cs)
{
	return read_xtal_counter();
}

static struct clocksource tangox_xtal = {
	.name	= "tangox_xtal",
	.rating	= 300,
	.read	= read_clocksource,
	.mask	= CLOCKSOURCE_MASK(32),
	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
};

static struct delay_timer delay_timer = { read_xtal_counter, XTAL_FREQ };

#define pi_alert(format, ...) do {				\
	static char fmt[] __initdata = KERN_ALERT format;	\
	printk(fmt, ## __VA_ARGS__);				\
} while (0)

static int __init wrap_local_timer_register(void)
{
	unsigned long twd_base = scu_a9_get_base() + 0x600;
	struct twd_local_timer tangox_twd = {{
		DEFINE_RES_MEM(twd_base, 16), DEFINE_RES_IRQ(29)
	}};
	return twd_local_timer_register(&tangox_twd);
}

#define REG(name, ...) union name { struct { u32 __VA_ARGS__; }; u32 val; }

REG(SYS_clkgen_pll, N:7, :6, K:3, M:3, :5, Isel:3, :3, T:1, B:1);
/*
 * CG0, CG1, CG2, CG3 PLL Control:
 * -------------------------------
 *
 * |    Byte 3     |    Byte 2     |    Byte 1     |    Byte 0     |
 * |3 3 2 2 2 2 2 2|2 2 2 2 1 1 1 1|1 1 1 1 1 1    |               |
 * |1 0 9 8 7 6 5 4|3 2 1 0 9 8 7 6|5 4 3 2 1 0 9 8|7 6 5 4 3 2 1 0|
 * |-|-|-----|-----|---------|-----|-----|---------|-|-------------|
 * |B|T|xxxxx|Isel |xxxxxxxxx|  M  |  K  |xxxxxxxxx|x|      N      |
 * |-|-|-----|-----|---------|-----|-----|---------|-|-------------|
 *
 * These registers are used to configure the PLL parameters:
 *
 * Bits  6 to  0: N[6:0]. Default = 29
 * Bits 15 to 13: K[2:0]. Default = 1
 * Bit  18 to 16: M[2:0]. Default = 0
 * Bits 26 to 24: Isel[2:0] (PLL Input Select). Default = 1
 * Bits 30      : T (PLL Test). Default = 0
 * Bits 31      : B (PLL Bypass). Default = 0
 *
 * PLL0 : Out = In * (N+1) / (M+1) / 2^K
 * PLL1 : Same as PLL0
 * PLL2 : Same as PLL0
 * Default values : All PLLs configured to output 405MHz.
 */
static void __init tangox_clock_tree_register(void)
{
	struct clk *clk;
	unsigned int mul, div;
	union SYS_clkgen_pll pll;

	pll.val = readl_relaxed(SYS_clkgen0_pll);
	mul = pll.N + 1; div = (pll.M + 1) << pll.K;
	if (pll.Isel != 1) pi_alert("PLL0 source is not XTAL_IN!\n");

	clk = clk_register_fixed_rate(0, "XTAL", 0, CLK_IS_ROOT, XTAL_FREQ);
	if (!clk) pi_alert("Failed to register %s clk!\n", "XTAL");

	clk = clk_register_fixed_factor(0, "PLL0", "XTAL", 0, mul, div);
	if (!clk) pi_alert("Failed to register %s clk!\n", "PLL0");

	clk = clk_register_divider(0, "CPU_CLK", "PLL0", 0, SYS_cpuclk_div_ctrl, 8, 8, CLK_DIVIDER_ONE_BASED, 0);
	if (!clk) pi_alert("Failed to register %s clk!\n", "CPU_CLK");
	clk_register_clkdev(clk, NULL, "cpu_clk");

	clk = clk_register_fixed_factor(0, "PERIPHCLK", "CPU_CLK", 0, 1, 2);
	if (!clk) pi_alert("Failed to register %s clk!\n", "PERIPHCLK");
	clk_register_clkdev(clk, NULL, "smp_twd");

	writel_relaxed(FAST_RAMP << 21 | 1 << 8, SYS_cpuclk_div_ctrl);
}

void __init tangox_timer_init(void)
{
	int err;

	clkgen_base = ioremap(CLKGEN_BASE, 0x100);
	if (clkgen_base == NULL) return;

	register_current_timer_delay(&delay_timer);
	sched_clock_register(read_sched_clock, 32, XTAL_FREQ);

	err = clocksource_register_hz(&tangox_xtal, XTAL_FREQ);
	if (err) pi_alert("Failed to register tangox_xtal clocksource!\n");

	tangox_clock_tree_register();

	err = wrap_local_timer_register();
	if (err) pi_alert("Failed to register local timer!\n");
}

[-- Attachment #3: cpufreq.c --]
[-- Type: text/x-csrc, Size: 1906 bytes --]

/*
 * Copyright 2015 Sigma Designs
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 */
#include <linux/module.h>
#include <linux/cpufreq.h>

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Sigma Designs");
MODULE_DESCRIPTION("cpufreq driver for Tangox");

static struct cpufreq_frequency_table freq_table[] = {
	{ .driver_data = 1 },
	{ .driver_data = 2 },
	{ .driver_data = 3 },
	{ .driver_data = 5 },
	{ .driver_data = 9 },
	{ .driver_data = 54 },
	{ .frequency = CPUFREQ_TABLE_END }
};

static int tangox_target(struct cpufreq_policy *policy, unsigned int idx)
{
	// TODO: MUST CHECK FOR IDLE BEFORE CALLING clk_set_rate()
	return clk_set_rate(policy->clk, freq_table[idx].frequency * 1000);
}

#define FAST_RAMP_SPEED 15 /* in kHz per nanosecond */

static int tangox_cpu_init(struct cpufreq_policy *policy)
{
	struct cpufreq_frequency_table *p;
	unsigned int freq, transition_latency;

	policy->clk = clk_get_sys("cpu_clk", NULL);
	freq = clk_get_rate(policy->clk) / 1000;
	transition_latency = freq / FAST_RAMP_SPEED;

	for (p = freq_table; p->frequency != CPUFREQ_TABLE_END; ++p) {
		p->frequency = freq / p->driver_data;
	}

	return cpufreq_generic_init(policy, freq_table, transition_latency);
}

static struct cpufreq_driver tangox_cpufreq_driver = {
	.name		= "tangox-cpufreq",
	.init		= tangox_cpu_init,
	.verify		= cpufreq_generic_frequency_table_verify,
	.target_index	= tangox_target,
	.get		= cpufreq_generic_get,
	.exit		= cpufreq_generic_exit,
	.attr		= cpufreq_generic_attr,
};

static int __init tangox_cpufreq_init(void)
{
	return cpufreq_register_driver(&tangox_cpufreq_driver);
}

static void __exit tangox_cpufreq_exit(void)
{
	cpufreq_unregister_driver(&tangox_cpufreq_driver);
}

module_init(tangox_cpufreq_init);
module_exit(tangox_cpufreq_exit);

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

On 12/05/2015 17:50, Russell King - ARM Linux wrote:
> On Tue, May 12, 2015 at 05:14:15PM +0200, Mason wrote:
>> This ties in to another thread I started in LAKML:
>> ("High-resolution timers not supported when using smp_twd on Cortex A9")
>>
>> $ git show 5388a6b2 arch/arm/kernel/smp_twd.c
>> commit 5388a6b266e9c3357353332ba0cd5549082887f1
>> Author: Russell King <rmk+kernel@arm.linux.org.uk>
>> Date:   Mon Jul 26 13:19:43 2010 +0100
>>
>>     ARM: SMP: Always enable clock event broadcast support
>>     
>>     The TWD local timers are unable to wake up the CPU when it is placed
>>     into a low power mode, eg. C3.  Therefore, we need to adapt things
>>     such that the TWD code can cope with this.
>>     
>>     We do this by always providing a broadcast tick function, and marking
>>     the fact that the TWD local timer will stop in low power modes.  This
>>     means that when the CPU is placed into a low power mode, the core
>>     timer code marks this fact, and allows an IPI to be given to the core.
>>
>> This mentions a "broadcast tick function" (of which I know nothing).
>> Is this what you're referring to?
> 
> No.  This has nothing to do with low power modes.
> 
> How this works depends on how your kernel is configured, but essentially
> it's something like this:
> 
> * The CPU which will be idling sets its local timer to wake up after N
>   counter cycles, where N is calculated from the timer frequency.
> 
> * When the local timer fires, the CPU is kicked out of the idle loop, and
>   it reads the current system time.  If the current system time indicates
>   that the software timer set in schedule_timeout() has fired, that
>   software timer fires.
> 
> If the local timer changes frequency without the idling CPU being woken
> up, then the problem you're referring to can happen.
> 
> As you're not giving much information about your system (including
> indicating where we might see some source code) we're not able to help
> more than providing above descriptions.  Maybe if you posted your
> patches so far to support the project you're working on, we could
> provide better answers.

$ git diff v3.14.41 HEAD >tango.patch && xz tango.patch

I don't understand the IRQ-related part yet
( arch/arm/mach-tangox/irq.c and drivers/irqchip/irq-gic.c )

If anyone spots the problem, that would make my day.

I tested with a loadable module whose init function is

static int __init ts_init(void)
{
	long res;
	printk("ABOUT TO SLEEP\n");
	set_current_state(TASK_INTERRUPTIBLE);
	res = schedule_timeout(HZ);
	printk("WAKE UP res=%ld\n", res);
	return 0;
}

Loading the module, with cpufreq divided by 9, prints:
[ 1738.962982] ABOUT TO SLEEP
[ 1747.956191] WAKE UP res=0

Regards.


[-- Attachment #2: tango.patch.xz --]
[-- Type: application/force-download, Size: 30112 bytes --]

[-- Attachment #3: dot.config.xz --]
[-- Type: application/force-download, Size: 7924 bytes --]

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On Wed, May 13, 2015 at 10:21 PM, Mason <slash.tmp@free.fr> wrote:
> $ git diff v3.14.41 HEAD >tango.patch && xz tango.patch
>
> I don't understand the IRQ-related part yet
> ( arch/arm/mach-tangox/irq.c and drivers/irqchip/irq-gic.c )
>
> If anyone spots the problem, that would make my day.
>
> I tested with a loadable module whose init function is
>
> static int __init ts_init(void)
> {
>         long res;
>         printk("ABOUT TO SLEEP\n");
>         set_current_state(TASK_INTERRUPTIBLE);
>         res = schedule_timeout(HZ);
>         printk("WAKE UP res=%ld\n", res);
>         return 0;
> }
>
> Loading the module, with cpufreq divided by 9, prints:
> [ 1738.962982] ABOUT TO SLEEP
> [ 1747.956191] WAKE UP res=0

I hope you are also matching this time with a physical watch,
to make sure 38->47 is really 9 seconds :)

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 14/05/2015 04:13, Viresh Kumar wrote:
> On Wed, May 13, 2015 at 10:21 PM, Mason <slash.tmp@free.fr> wrote:
>> $ git diff v3.14.41 HEAD >tango.patch && xz tango.patch
>>
>> I don't understand the IRQ-related part yet
>> ( arch/arm/mach-tangox/irq.c and drivers/irqchip/irq-gic.c )
>>
>> If anyone spots the problem, that would make my day.
>>
>> I tested with a loadable module whose init function is
>>
>> static int __init ts_init(void)
>> {
>>         long res;
>>         printk("ABOUT TO SLEEP\n");
>>         set_current_state(TASK_INTERRUPTIBLE);
>>         res = schedule_timeout(HZ);
>>         printk("WAKE UP res=%ld\n", res);
>>         return 0;
>> }
>>
>> Loading the module, with cpufreq divided by 9, prints:
>> [ 1738.962982] ABOUT TO SLEEP
>> [ 1747.956191] WAKE UP res=0
> 
> I hope you are also matching this time with a physical watch,
> to make sure 38->47 is really 9 seconds :)

Hello Viresh,

I didn't /literally/ take a stop-watch to verify it was EXACTLY
9 seconds, but I was staring at the prompt, and it /felt/ like
9 seconds. And the 54 second case felt like a minute.

I'm using a 27 MHz crystal as clocksource. This is independent
of the CPU frequency. However, I'm using the ARM TWD as the
system's clockevent source, and the TWD's clock is tied to
the CPU clock (PERIPHCLK = CPUCLK / 2 on this SoC).

I'm wondering if there's another standard clockevent source
I could try (it would be great if it supported high-resolution
timers).

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On 14-05-15, 13:22, Mason wrote:
> I didn't /literally/ take a stop-watch to verify it was EXACTLY
> 9 seconds, but I was staring at the prompt, and it /felt/ like
> 9 seconds. And the 54 second case felt like a minute.

:)

> I'm using a 27 MHz crystal as clocksource. This is independent
> of the CPU frequency. However, I'm using the ARM TWD as the
> system's clockevent source, and the TWD's clock is tied to
> the CPU clock (PERIPHCLK = CPUCLK / 2 on this SoC).

The only (very straight forward) problem is that we aren't propagating
the freq update to clockevents core and you need to debug a bit there.

Also I wanted to see the source of your print message:
[   19.650454] NEW RATE=9250000
[   19.653644] NEW RATE=9250000

What's this rate ? Old/new ? Because you are atleast printing the old
rate here, and the function by default gets the new rate.

> I'm wondering if there's another standard clockevent source
> I could try (it would be great if it supported high-resolution
> timers).

I hope you have some platform general-purpose-timers.

-- 
viresh

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 14/05/2015 13:54, Viresh Kumar wrote:

> Mason wrote:
> 
>> I'm using a 27 MHz crystal as clocksource. This is independent
>> of the CPU frequency. However, I'm using the ARM TWD as the
>> system's clockevent source, and the TWD's clock is tied to
>> the CPU clock (PERIPHCLK = CPUCLK / 2 on this SoC).
> 
> The only (very straight forward) problem is that we aren't propagating
> the freq update to clockevents core and you need to debug a bit there.

I had the same thought, which is why I added the "NEW RATE" trace.

> Also I wanted to see the source of your print message:
> [   19.650454] NEW RATE=9250000
> [   19.653644] NEW RATE=9250000
> 
> What's this rate ? Old/new ? Because you are at least printing the old
> rate here, and the function by default gets the new rate.

I added a printk inside twd_update_frequency().

http://lxr.free-electrons.com/source/arch/arm/kernel/smp_twd.c?v=3.14#L107

I inserted printk("NEW RATE=%lu\n", twd_timer_rate);
right before the call to clockevents_update_freq().

When I execute "echo 18500 > scaling_max_freq"
the system is supposed to change the CPU frequency to 18.5 MHz
(I might have a bug lurking there) and PERIPHCLK is 1/2 of that,
i.e 9.25 MHz.

twd_update_frequency() is called twice: once for each CPU.
(The timers are local to each CPU.)

>> I'm wondering if there's another standard clockevent source
>> I could try (it would be great if it supported high-resolution
>> timers).
> 
> I hope you have some platform general-purpose-timers.

Yes, I do, but I was trying to use as much generic code as
possible to limit the chances of introducing bugs.

I'll take a fresh look at the ARM GLOBAL TIMER, but last I
checked, it didn't seem to handle frequency scaling.

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Thu, May 14, 2015 at 03:06:54PM +0200, Mason wrote:
> I added a printk inside twd_update_frequency().
> 
> http://lxr.free-electrons.com/source/arch/arm/kernel/smp_twd.c?v=3.14#L107
> 
> I inserted printk("NEW RATE=%lu\n", twd_timer_rate);
> right before the call to clockevents_update_freq().
> 
> When I execute "echo 18500 > scaling_max_freq"
> the system is supposed to change the CPU frequency to 18.5 MHz
> (I might have a bug lurking there) and PERIPHCLK is 1/2 of that,
> i.e 9.25 MHz.
> 
> twd_update_frequency() is called twice: once for each CPU.
> (The timers are local to each CPU.)

That's expected.

I wonder if clockevents_update_freq() is returning an error - which we
don't check, and don't print a warning.  It would probably be a very
good idea to print a warning as a failure to reconfigure the clock
events code for a different frequency will lead to this kind of issue.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On 14 May 2015 at 18:36, Mason <slash.tmp@free.fr> wrote:
> When I execute "echo 18500 > scaling_max_freq"
> the system is supposed to change the CPU frequency to 18.5 MHz
> (I might have a bug lurking there) and PERIPHCLK is 1/2 of that,
> i.e 9.25 MHz.

So at least we are on the right path. But it looks to me that this
call is not getting propagated well.

>From the attachment you gave initially, the event handler for
twd-timers is: tick_handle_periodic(). i.e. you are running in
periodic mode and not onshot...

why ?

> Yes, I do, but I was trying to use as much generic code as
> possible to limit the chances of introducing bugs.

Hmm..

> I'll take a fresh look at the ARM GLOBAL TIMER, but last I
> checked, it didn't seem to handle frequency scaling.

why is that required? Why will you change its freq ?
The same timer is probably used for SPEAr (the platform
I used to work on):

http://lxr.free-electrons.com/source/arch/arm/mach-spear/time.c?v=3.14

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On 14 May 2015 at 19:29, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 14 May 2015 at 18:36, Mason <slash.tmp@free.fr> wrote:
>> When I execute "echo 18500 > scaling_max_freq"
>> the system is supposed to change the CPU frequency to 18.5 MHz
>> (I might have a bug lurking there) and PERIPHCLK is 1/2 of that,
>> i.e 9.25 MHz.
>
> So at least we are on the right path. But it looks to me that this
> call is not getting propagated well.
>
> From the attachment you gave initially, the event handler for
> twd-timers is: tick_handle_periodic(). i.e. you are running in
> periodic mode and not onshot...
>
> why ?
>
>> Yes, I do, but I was trying to use as much generic code as
>> possible to limit the chances of introducing bugs.
>
> Hmm..
>
>> I'll take a fresh look at the ARM GLOBAL TIMER, but last I
>> checked, it didn't seem to handle frequency scaling.
>
> why is that required? Why will you change its freq ?
> The same timer is probably used for SPEAr (the platform
> I used to work on):
>
> http://lxr.free-electrons.com/source/arch/arm/mach-spear/time.c?v=3.14

And this might be the problem you are facing:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-May/098564.html

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Thu, May 14, 2015 at 07:29:38PM +0530, Viresh Kumar wrote:
> On 14 May 2015 at 18:36, Mason <slash.tmp@free.fr> wrote:
> > When I execute "echo 18500 > scaling_max_freq"
> > the system is supposed to change the CPU frequency to 18.5 MHz
> > (I might have a bug lurking there) and PERIPHCLK is 1/2 of that,
> > i.e 9.25 MHz.
> 
> So at least we are on the right path. But it looks to me that this
> call is not getting propagated well.
> 
> >From the attachment you gave initially, the event handler for
> twd-timers is: tick_handle_periodic(). i.e. you are running in
> periodic mode and not onshot...
> 
> why ?

If it's in periodic mode, the update should still be propagated to the
hardware, assuming the generic time keeping code doesn't produce an
error.

twd_update_frequency
`-clockevents_update_freq
  `-__clockevents_update_freq
    `-__clockevents_set_state(, CLOCK_EVT_STATE_PERIODIC)
      `-dev->set_mode (twd_set_mode)

That re-writes the TWD_TIMER_LOAD register based on twd_timer_rate,
which would have been updated by twd_update_frequency().

The question I posed earlier remains: is clockevents_update_freq()
failing?  We don't know, because we never check its return value.

Another thing to look at is whether we reach twd_set_mode().

Lastly, printing the values of the TWD_TIMER_LOAD and TWD_TIMER_COUNTER
after twd_set_mode() has written TWD_TIMER_LOAD might provide some
hints as to what's going on.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 14/05/2015 15:59, Viresh Kumar wrote:
> On 14 May 2015 at 18:36, Mason wrote:
>> When I execute "echo 18500 > scaling_max_freq"
>> the system is supposed to change the CPU frequency to 18.5 MHz
>> (I might have a bug lurking there) and PERIPHCLK is 1/2 of that,
>> i.e 9.25 MHz.
> 
> So at least we are on the right path. But it looks to me that this
> call is not getting propagated well.
> 
> From the attachment you gave initially, the event handler for
> twd-timers is: tick_handle_periodic(). i.e. you are running in
> periodic mode and not one-shot... why ?

I don't know. Is it not obvious (to someone who knows what to
look for) from reading the smp_twd.c source?

How do I run the TWD in one-shot mode?

>> Yes, I do, but I was trying to use as much generic code as
>> possible to limit the chances of introducing bugs.
> 
> Hmm..
> 
>> I'll take a fresh look at the ARM GLOBAL TIMER, but last I
>> checked, it didn't seem to handle frequency scaling.
> 
> why is that required? Why will you change its freq ?

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407g/CIHGECHJ.html

"The global timer is clocked by PERIPHCLK."

PERIPHCLK = CPUCLK/2

Change the CPUCLK, change the PERIPHCLK.

> The same timer is probably used for SPEAr (the platform
> I used to work on):
> 
> http://lxr.free-electrons.com/source/arch/arm/mach-spear/time.c?v=3.14

Did that platform use cpufreq DFS?

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 14/05/2015 15:53, Russell King - ARM Linux wrote:

> I wonder if clockevents_update_freq() is returning an error - which we
> don't check, and don't print a warning.  It would probably be a very
> good idea to print a warning as a failure to reconfigure the clock
> events code for a different frequency will lead to this kind of issue.

I'll add code to test the return value, and report back here.

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On 14-05-15, 16:48, Mason wrote:
> I don't know. Is it not obvious (to someone who knows what to
> look for) from reading the smp_twd.c source?
> 
> How do I run the TWD in one-shot mode?

I haven't refreshed my memory for the earlier reply, but I have tried
to go through the code again.

The logic is this:
- Oneshot mode is only useful if we are going to support NO_HZ mode.
  i.e. we can disable the clkevt device during CPU idle ..
- For that to work, or in other words for the CPU to wake up from the
  idle state, we need a broadcast timer. Which will wake up the idle
  cpu and its clkevt device.

  You don't have this 'broadcast' timer in your case and so we aren't
  able to switch to oneshot mode.

  Even if you do that, you will be stuck in LOW resolution mode. And
  so you probably leave this timer as is and use a out-of-CPU-domain
  timer for this work, which can support high resolution mode.

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407g/CIHGECHJ.html

What core does your platform has? Cortex A9? Doesn't that uses this
timer instead: drivers/clocksource/arm_arch_timer.c ?

> "The global timer is clocked by PERIPHCLK."
> 
> PERIPHCLK = CPUCLK/2
> 
> Change the CPUCLK, change the PERIPHCLK.

I am not sure if GPT on SPEAr was an ARM provided timer. But it
definitely doesn't depend on cpu-clk and so no cpufreq support in
that.

> > The same timer is probably used for SPEAr (the platform
> > I used to work on):
> > 
> > http://lxr.free-electrons.com/source/arch/arm/mach-spear/time.c?v=3.14
> 
> Did that platform use cpufreq DFS?

Yes, but the clock to that timer was independent of CPU's clock.

-- 
viresh

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On 15-05-15, 09:46, Viresh Kumar wrote:
> The logic is this:
> - Oneshot mode is only useful if we are going to support NO_HZ mode.
>   i.e. we can disable the clkevt device during CPU idle ..

Minor correction. The last line talks about NO_HZ_IDLE instead. NO_HZ
is about disabling to tick when not required.

-- 
viresh

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Fri, May 15, 2015 at 09:46:32AM +0530, Viresh Kumar wrote:
> What core does your platform has? Cortex A9? Doesn't that uses this
> timer instead: drivers/clocksource/arm_arch_timer.c ?

Cortex A9 doesn't always have the architected timer.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 15/05/2015 06:16, Viresh Kumar wrote:

> What core does your platform have? Cortex A9? Doesn't that uses this
> timer instead: drivers/clocksource/arm_arch_timer.c ?

Cortex-A9 MPCore r3p0

Generic Timer (aka Architected system timer) is not available
in Cortex-A9.

https://community.arm.com/message/8775

"Generic Timer is not an option on the Cortex-A9 (or A8 or A5)
processors. The Generic Timer is an optional part of the ARMv7
architecture, and is not implemented by those three processors."

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 14/05/2015 16:42, Russell King - ARM Linux wrote:

> If it's in periodic mode, the update should still be propagated to the
> hardware, assuming the generic time keeping code doesn't produce an
> error.
> 
> twd_update_frequency
> `-clockevents_update_freq
>   `-__clockevents_update_freq
>     `-__clockevents_set_state(, CLOCK_EVT_STATE_PERIODIC)
>       `-dev->set_mode (twd_set_mode)
> 
> That re-writes the TWD_TIMER_LOAD register based on twd_timer_rate,
> which would have been updated by twd_update_frequency().
> 
> The question I posed earlier remains: is clockevents_update_freq()
> failing?  We don't know, because we never check its return value.
> 
> Another thing to look at is whether we reach twd_set_mode().
> 
> Lastly, printing the values of the TWD_TIMER_LOAD and TWD_TIMER_COUNTER
> after twd_set_mode() has written TWD_TIMER_LOAD might provide some
> hints as to what's going on.

I'll have access to the board on Monday.

I'll add printk in strategic places, and report back ASAP.

(I'm considering dropping TWD, and using platform timers.)

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Fri, May 15, 2015 at 11:29:34AM +0200, Mason wrote:
> On 14/05/2015 16:42, Russell King - ARM Linux wrote:
> 
> > If it's in periodic mode, the update should still be propagated to the
> > hardware, assuming the generic time keeping code doesn't produce an
> > error.
> > 
> > twd_update_frequency
> > `-clockevents_update_freq
> >   `-__clockevents_update_freq
> >     `-__clockevents_set_state(, CLOCK_EVT_STATE_PERIODIC)
> >       `-dev->set_mode (twd_set_mode)
> > 
> > That re-writes the TWD_TIMER_LOAD register based on twd_timer_rate,
> > which would have been updated by twd_update_frequency().
> > 
> > The question I posed earlier remains: is clockevents_update_freq()
> > failing?  We don't know, because we never check its return value.
> > 
> > Another thing to look at is whether we reach twd_set_mode().
> > 
> > Lastly, printing the values of the TWD_TIMER_LOAD and TWD_TIMER_COUNTER
> > after twd_set_mode() has written TWD_TIMER_LOAD might provide some
> > hints as to what's going on.
> 
> I'll have access to the board on Monday.
> 
> I'll add printk in strategic places, and report back ASAP.

That would be good.

> (I'm considering dropping TWD, and using platform timers.)

As you don't say which kernel version you're using, for all we know, you
might be using a version which omits some fixes in this area, such as
this one which you really must have if your timer is operating in
period mode:

fe79a9ba1196 clockevents: Adjust timer interval when frequency changes



-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Viresh Kumar]

On 15-05-15, 10:51, Russell King - ARM Linux wrote:
> As you don't say which kernel version you're using, for all we know, you
> might be using a version which omits some fixes in this area, such as
> this one which you really must have if your timer is operating in
> period mode:
> 
> fe79a9ba1196 clockevents: Adjust timer interval when frequency changes

Atleast the link he gave was for 3.14 and this patch got commited in
3.15 :)

If that's the Mason, this patch is all you need.

-- 
viresh

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 15/05/2015 06:16, Viresh Kumar wrote:

> On 14-05-15, 16:48, Mason wrote:
>
>> How do I run the TWD in one-shot mode?
> 
> I haven't refreshed my memory for the earlier reply, but I have tried
> to go through the code again.
> 
> The logic is this:
> - Oneshot mode is only useful if we are going to support NO_HZ mode.
>   i.e. we can disable the clkevt device during CPU idle ..
> - For that to work, or in other words for the CPU to wake up from the
>   idle state, we need a broadcast timer. Which will wake up the idle
>   cpu and its clkevt device.
> 
>   You don't have this 'broadcast' timer in your case and so we aren't
>   able to switch to oneshot mode.
> 
>   Even if you do that, you will be stuck in LOW resolution mode. And
>   so you probably leave this timer as is and use a out-of-CPU-domain
>   timer for this work, which can support high resolution mode.

For the record, my kernel config contains:
(There are some BROADCAST-related options enabled.)

#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_HARDIRQS_SW_RESEND=y
CONFIG_IRQ_DOMAIN=y
# CONFIG_IRQ_DOMAIN_DEBUG is not set
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_KTIME_SCALAR=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BUILD=y
CONFIG_ARCH_HAS_TICK_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y

#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
CONFIG_NO_HZ_IDLE=y
# CONFIG_NO_HZ_FULL is not set
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y

#
# CPU/Task time and stats accounting
#
CONFIG_TICK_CPU_ACCOUNTING=y
# CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set
# CONFIG_IRQ_TIME_ACCOUNTING is not set
# CONFIG_BSD_PROCESS_ACCT is not set
# CONFIG_TASKSTATS is not set


Full config (and platform patch) provided in <555380F8.5050306@free.fr>
http://article.gmane.org/gmane.linux.power-management.general/60164

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 15/05/2015 11:51, Russell King - ARM Linux wrote:

> As you don't say which kernel version you're using,

Sorry about that, I thought the patch-generating command
I posted conveyed the information.

$ git diff v3.14.41 HEAD >tango.patch && xz tango.patch

I'm using the 3.14.y branch from linux-stable.

> for all we know, you might be using a version which omits some fixes
> in this area, such as this one which you really must have if your
> timer is operating in period mode:
> 
> fe79a9ba1196 clockevents: Adjust timer interval when frequency changes

Hmmm, according to git log, this patch was accepted during
the 3.14-rc2 merge window. Why didn't it make it for 3.14?

I'll cherry-pick that commit, and report back along with
the other tests.

By any chance, is there a patch that would allow the platform
to get high-resolution timers when using smp_twd?

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Fri, May 15, 2015 at 12:36:10PM +0200, Mason wrote:
> On 15/05/2015 11:51, Russell King - ARM Linux wrote:
> 
> > As you don't say which kernel version you're using,
> 
> Sorry about that, I thought the patch-generating command
> I posted conveyed the information.
> 
> $ git diff v3.14.41 HEAD >tango.patch && xz tango.patch
> 
> I'm using the 3.14.y branch from linux-stable.

So you're using a kernel over a year old...

> > for all we know, you might be using a version which omits some fixes
> > in this area, such as this one which you really must have if your
> > timer is operating in period mode:
> > 
> > fe79a9ba1196 clockevents: Adjust timer interval when frequency changes
> 
> Hmmm, according to git log, this patch was accepted during
> the 3.14-rc2 merge window. Why didn't it make it for 3.14?

No it wasn't:

$ git describe --contains fe79a9ba1196
v3.15-rc1~123^2~12

It was merged during the merge window immediately preceding v3.15-rc1.

> By any chance, is there a patch that would allow the platform
> to get high-resolution timers when using smp_twd?

I think you'll have to do the research for that yourself... what
I can say is that TWD is capable of one-shot mode, and is capable
of running as a high-res timer:

Per CPU device: 0
Clock Event Device: local_timer
 max_delta_ns:   21691754031
 min_delta_ns:   1000
 mult:           425201762
 shift:          31
 mode:           3			<== CLOCK_EVT_MODE_ONESHOT
 next_event:     595603944000000 nsecs
 set_next_event: twd_set_next_event
 set_mode:       twd_set_mode
 event_handler:  hrtimer_interrupt	<== indicates that it's switched
					to high-res mode
 retries:        0

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Mason]

On 15/05/2015 13:58, Russell King - ARM Linux wrote:
> On Fri, May 15, 2015 at 12:36:10PM +0200, Mason wrote:
>> On 15/05/2015 11:51, Russell King - ARM Linux wrote:
>>
>>> As you don't say which kernel version you're using,
>>
>> Sorry about that, I thought the patch-generating command
>> I posted conveyed the information.
>>
>> $ git diff v3.14.41 HEAD >tango.patch && xz tango.patch
>>
>> I'm using the 3.14.y branch from linux-stable.
> 
> So you're using a kernel over a year old...

The sad part is: I'm working on the "new" kernel.
The current "official" kernel is 3.4 (and I'm afraid
there's a 2.6.32 still kicking around somewhere).

I'm trying to convince management that one of the advantages
of mainlining the port is that it is easier to switch to newer
kernels. Do you agree with this assertion?

>>> for all we know, you might be using a version which omits some fixes
>>> in this area, such as this one which you really must have if your
>>> timer is operating in period mode:
>>>
>>> fe79a9ba1196 clockevents: Adjust timer interval when frequency changes
>>
>> Hmmm, according to git log, this patch was accepted during
>> the 3.14-rc2 merge window. Why didn't it make it for 3.14?
> 
> No it wasn't:
> 
> $ git describe --contains fe79a9ba1196
> v3.15-rc1~123^2~12
> 
> It was merged during the merge window immediately preceding v3.15-rc1.
> 
>> By any chance, is there a patch that would allow the platform
>> to get high-resolution timers when using smp_twd?
> 
> I think you'll have to do the research for that yourself... what
> I can say is that TWD is capable of one-shot mode,

Is one-shot mode preferred over periodic mode?
(Seems counter-intuitive, as one-shot mode needs to be
periodically reprogrammed, whereas periodic mode just
keeps firing.)

> and is capable of running as a high-res timer:
> 
> Per CPU device: 0
> Clock Event Device: local_timer
>  max_delta_ns:   21691754031
>  min_delta_ns:   1000
>  mult:           425201762
>  shift:          31
>  mode:           3			<== CLOCK_EVT_MODE_ONESHOT
>  next_event:     595603944000000 nsecs
>  set_next_event: twd_set_next_event
>  set_mode:       twd_set_mode
>  event_handler:  hrtimer_interrupt	<== indicates that it's switched
> 					to high-res mode
>  retries:        0

I'm confused.

In the "High-resolution timers not supported when using
smp_twd on Cortex A9" thread, you seemed to agree that
TWD could not be used as a hrtimer...

> Yes - because the TWD stops in low power modes, which makes it
> unsuitable as a high-resolution timer.  These kinds of issues
> are annoying, but it's the way things are.

So it looks like TWD can be used as a hrtimer, but something
on my platform is making it choose otherwise, and it is not
the CLOCK_EVT_FEAT_C3STOP flag?

Regards.

Re: schedule_timeout sleeps too long after dividing CPU frequency

[Russell King - ARM Linux]

On Fri, May 15, 2015 at 02:45:28PM +0200, Mason wrote:
> The sad part is: I'm working on the "new" kernel.
> The current "official" kernel is 3.4 (and I'm afraid
> there's a 2.6.32 still kicking around somewhere).
> 
> I'm trying to convince management that one of the advantages
> of mainlining the port is that it is easier to switch to newer
> kernels. Do you agree with this assertion?

Yes and no.  If you can get stuff upstream, it should make things
easier.

The difficult bit - and time consuming bit - is getting code upstream.
Even in my position, I'd suggest allowing about five years for that
activity, and even then don't have expectations of getting everything
upstream.

Frankly now, my advice to people would be to just not bother, it's
*way* too much effort to get everything to an acceptable state,
especially if the SoC has no support what so ever.

For example, we're still banging on over the iMX6 SoCs - just getting
the display stuff sorted took more than a year.  Ethernet keeps becoming
unstable (people keep reporting problems with it) and it became far too
difficult for me to get my patch set for that upstream, so I dumped it.

> > I think you'll have to do the research for that yourself... what
> > I can say is that TWD is capable of one-shot mode,
> 
> Is one-shot mode preferred over periodic mode?
> (Seems counter-intuitive, as one-shot mode needs to be
> periodically reprogrammed, whereas periodic mode just
> keeps firing.)

Think about it in terms of high-resolution timers.  How can something
that's programmed to fire HZ times a second provide you with a high
resolution timer?  Something that you can program for an arbitary
period depending on your needs obviously allows higher resolution
intervals, up to the minimum timer count interval.  It can also provide
a longer interval, up to the maximum timer count interval.

> > and is capable of running as a high-res timer:
> > 
> > Per CPU device: 0
> > Clock Event Device: local_timer
> >  max_delta_ns:   21691754031
> >  min_delta_ns:   1000
> >  mult:           425201762
> >  shift:          31
> >  mode:           3			<== CLOCK_EVT_MODE_ONESHOT
> >  next_event:     595603944000000 nsecs
> >  set_next_event: twd_set_next_event
> >  set_mode:       twd_set_mode
> >  event_handler:  hrtimer_interrupt	<== indicates that it's switched
> > 					to high-res mode
> >  retries:        0
> 
> I'm confused.
> 
> In the "High-resolution timers not supported when using
> smp_twd on Cortex A9" thread, you seemed to agree that
> TWD could not be used as a hrtimer...

I guess I was wrong.  I'm no expert on the kernel's time code.  That's
why I've suggested to you several times that you do the research
yourself, rather than me having to do that research for you and then
write an email about it.  It's kind of wasting my time, and you're not
paying me for this kind of support service...

> > Yes - because the TWD stops in low power modes, which makes it
> > unsuitable as a high-resolution timer.  These kinds of issues
> > are annoying, but it's the way things are.
> 
> So it looks like TWD can be used as a hrtimer, but something
> on my platform is making it choose otherwise, and it is not
> the CLOCK_EVT_FEAT_C3STOP flag?

I've no idea.  I don't have the 3.14 code in front of me to research.
I _could_ wind my tree back to 3.14, and read that code, but I don't
see why I should go to all that effort... especially as I have other
things I need to be getting on with, and other problems to be taking
care of.

What I'm saying is that I have a platform here running a modern kernel
which _does_ use the TWD, and it _does_ appear to be running in high-res
mode.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.