[PATCH] Add sched_clock to AT91 TCB clocksource driver

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Remy Bohmer]

On AT91 there is no architecture specific sched_clock() implementation,
so the default fallback is used. This fallback uses the jiffie counter
as sched_clock().
There is NO standard clocksource available that is accurate enough,
except the TC-based clocksource implementation. Therefor this
implementation is used as base for the sched_clock(). This clocksource
offers sub-millisecond time-stamping. (< 200 ns)

Signed-off-by: Remy Bohmer <linux@bohmer.net>
---
 arch/arm/Kconfig                 |    1 +
 arch/arm/kernel/sched_clock.c    |    7 +++++++
 drivers/clocksource/tcb_clksrc.c |   23 +++++++++++++++++++++++
 3 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9adc278..e0563a7 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -296,6 +296,7 @@ config ARCH_AT91
 	select HAVE_CLK
 	select CLKDEV_LOOKUP
 	select ARM_PATCH_PHYS_VIRT if MMU
+	select HAVE_SCHED_CLOCK if ATMEL_TCB_CLKSRC
 	help
 	  This enables support for systems based on the Atmel AT91RM9200,
 	  AT91SAM9 and AT91CAP9 processors.
diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index 9a46370..f9028e4 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -20,6 +20,13 @@ static void (*sched_clock_update_fn)(void);
 
 static void sched_clock_poll(unsigned long wrap_ticks)
 {
+	/*
+	 * The sched_clock_update_fn may be initialised AFTER first call to
+	 * time_init()
+	 */
+	if (unlikely(!sched_clock_update_fn))
+		return;
+
 	mod_timer(&sched_clock_timer, round_jiffies(jiffies + wrap_ticks));
 	sched_clock_update_fn();
 }
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 79c47e8..d6069b3 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -11,6 +11,7 @@
 #include <linux/platform_device.h>
 #include <linux/atmel_tc.h>
 
+#include <asm/sched_clock.h>
 
 /*
  * We're configured to use a specific TC block, one that's not hooked
@@ -210,6 +211,23 @@ static void __init setup_clkevents(struct atmel_tc *tc, int clk32k_divisor_idx)
 
 #endif
 
+static DEFINE_CLOCK_DATA(cd);
+
+unsigned long long notrace sched_clock(void)
+{
+	if (likely(tcaddr)) {
+		cycle_t cyc = tc_get_cycles(&clksrc);
+		return cyc_to_sched_clock(&cd, cyc, (u32)~0);
+	}
+	return 0;
+}
+
+static void notrace tcb_update_sched_clock(void)
+{
+	cycle_t cyc = tc_get_cycles(&clksrc);
+	update_sched_clock(&cd, cyc, (u32)~0);
+}
+
 static int __init tcb_clksrc_init(void)
 {
 	static char bootinfo[] __initdata
@@ -297,6 +315,11 @@ static int __init tcb_clksrc_init(void)
 	/* channel 2:  periodic and oneshot timer support */
 	setup_clkevents(tc, clk32k_divisor_idx);
 
+	/* Now prepare the sched_clock support */
+	init_sched_clock(&cd, tcb_update_sched_clock, 32, divided_rate);
+	sched_clock_postinit();
+
 	return 0;
 }
 arch_initcall(tcb_clksrc_init);
+
-- 
1.7.4.1

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Russell King - ARM Linux]

On Sun, Jul 31, 2011 at 05:03:13PM +0200, Remy Bohmer wrote:
> On AT91 there is no architecture specific sched_clock() implementation,
> so the default fallback is used. This fallback uses the jiffie counter
> as sched_clock().
> There is NO standard clocksource available that is accurate enough,
> except the TC-based clocksource implementation. Therefor this
> implementation is used as base for the sched_clock(). This clocksource
> offers sub-millisecond time-stamping. (< 200 ns)
> 
> Signed-off-by: Remy Bohmer <linux@bohmer.net>
> ---
>  arch/arm/Kconfig                 |    1 +
>  arch/arm/kernel/sched_clock.c    |    7 +++++++
>  drivers/clocksource/tcb_clksrc.c |   23 +++++++++++++++++++++++
>  3 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 9adc278..e0563a7 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -296,6 +296,7 @@ config ARCH_AT91
>  	select HAVE_CLK
>  	select CLKDEV_LOOKUP
>  	select ARM_PATCH_PHYS_VIRT if MMU
> +	select HAVE_SCHED_CLOCK if ATMEL_TCB_CLKSRC
>  	help
>  	  This enables support for systems based on the Atmel AT91RM9200,
>  	  AT91SAM9 and AT91CAP9 processors.
> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
> index 9a46370..f9028e4 100644
> --- a/arch/arm/kernel/sched_clock.c
> +++ b/arch/arm/kernel/sched_clock.c
> @@ -20,6 +20,13 @@ static void (*sched_clock_update_fn)(void);
>  
>  static void sched_clock_poll(unsigned long wrap_ticks)
>  {
> +	/*
> +	 * The sched_clock_update_fn may be initialised AFTER first call to
> +	 * time_init()
> +	 */
> +	if (unlikely(!sched_clock_update_fn))
> +		return;

NAK.  I've said this before - sched_clock _must_ be up and running by the
time this function is called - because shortly after this call is when
the scheduler initializes, and the scheduler wants sched_clock() working
at that point.

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Remy Bohmer]

Hi Russell,

2011/7/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> +++ b/arch/arm/Kconfig
>> @@ -296,6 +296,7 @@ config ARCH_AT91
>> ? ? ? select HAVE_CLK
>> ? ? ? select CLKDEV_LOOKUP
>> ? ? ? select ARM_PATCH_PHYS_VIRT if MMU
>> + ? ? select HAVE_SCHED_CLOCK if ATMEL_TCB_CLKSRC
>> ? ? ? help
>> ? ? ? ? This enables support for systems based on the Atmel AT91RM9200,
>> ? ? ? ? AT91SAM9 and AT91CAP9 processors.
>> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
>> index 9a46370..f9028e4 100644
>> --- a/arch/arm/kernel/sched_clock.c
>> +++ b/arch/arm/kernel/sched_clock.c
>> @@ -20,6 +20,13 @@ static void (*sched_clock_update_fn)(void);
>>
>> ?static void sched_clock_poll(unsigned long wrap_ticks)
>> ?{
>> + ? ? /*
>> + ? ? ?* The sched_clock_update_fn may be initialised AFTER first call to
>> + ? ? ?* time_init()
>> + ? ? ?*/
>> + ? ? if (unlikely(!sched_clock_update_fn))
>> + ? ? ? ? ? ? return;
>
> NAK. ?I've said this before - sched_clock _must_ be up and running by the
> time this function is called - because shortly after this call is when
> the scheduler initializes, and the scheduler wants sched_clock() working
> at that point.

OK, I know that too, but the jiffies based fallback does it even
worse, since it does not run at all...  (It only updates in steps of a
couple of 100msecs and stops after a while, and even does not display
a real time...) This sched_clock implementation at least works before
the drivers are being initialised...

Anyway, do you have a better suggestion how to fix this? The tcb
clocksource is loaded during a arch_initcall(), perhaps we need
something before that point.
I do not see an easy way to integrate it in MACHINE_START(.timer).
Would 'late_time_init' be a better solution?

Kind regards,

Remy

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Russell King - ARM Linux]

On Mon, Aug 01, 2011 at 10:42:35AM +0200, Remy Bohmer wrote:
> OK, I know that too, but the jiffies based fallback does it even
> worse, since it does not run at all...  (It only updates in steps of a
> couple of 100msecs and stops after a while, and even does not display
> a real time...) This sched_clock implementation at least works before
> the drivers are being initialised...

You're forgetting that jiffies doesn't jump about.  A late initializing
sched_clock could jump.

> Anyway, do you have a better suggestion how to fix this? The tcb
> clocksource is loaded during a arch_initcall(), perhaps we need
> something before that point.
> I do not see an easy way to integrate it in MACHINE_START(.timer).
> Would 'late_time_init' be a better solution?

late_time_init() is not that much better as that still happens after
sched_init() has been called.  sched_init() initializes various
structures which involves reading sched_clock().

Why can't it initialize itself at the standard point during the boot
sequence, which is time_init() - which in turn is as you say the
.timer callback?

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Remy Bohmer]

Hi,

> You're forgetting that jiffies doesn't jump about. ?A late initializing
> sched_clock could jump.

Agreed. Currently the jiffies based clock does not move at all, thus
it surely does not jump... ;-)

>> Anyway, do you have a better suggestion how to fix this? The tcb
>> clocksource is loaded during a arch_initcall(), perhaps we need
>> something before that point.
>> I do not see an easy way to integrate it in MACHINE_START(.timer).
>> Would 'late_time_init' be a better solution?
>
> late_time_init() is not that much better as that still happens after
> sched_init() has been called. ?sched_init() initializes various
> structures which involves reading sched_clock().

But late_time_init does it before the sched_clock is declared stable.
Until that point sched_clock will always return zero, as such we can
IMHO safely assume that the scheduler is designed such that it can
handle fixed zero timestamps for a while. The new sched_clock also
nicely starts counting from zero once enabled, and does not make a
jump start.

> Why can't it initialize itself at the standard point during the boot
> sequence, which is time_init() - which in turn is as you say the
> .timer callback?

In that case it has to replace the PIT based clocksource completely.
Since the TCB based clocksource and event device are
optional/configurable for this architecture and it is implemented as a
clocksource driver instead of part of the BSP, it would result in a
lot of ifdefs throughout that code.
The arch_initcall() has to be removed and replaced by something else...
I will try to work out a different solution, and post an update, see
how that looks like...

Kind regards,

Remy

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Russell King - ARM Linux]

On Mon, Aug 01, 2011 at 08:08:19PM +0200, Remy Bohmer wrote:
> Hi,
> 
> > You're forgetting that jiffies doesn't jump about. ?A late initializing
> > sched_clock could jump.
> 
> Agreed. Currently the jiffies based clock does not move at all, thus
> it surely does not jump... ;-)
> 
> >> Anyway, do you have a better suggestion how to fix this? The tcb
> >> clocksource is loaded during a arch_initcall(), perhaps we need
> >> something before that point.
> >> I do not see an easy way to integrate it in MACHINE_START(.timer).
> >> Would 'late_time_init' be a better solution?
> >
> > late_time_init() is not that much better as that still happens after
> > sched_init() has been called. ?sched_init() initializes various
> > structures which involves reading sched_clock().
> 
> But late_time_init does it before the sched_clock is declared stable.
> Until that point sched_clock will always return zero, as such we can
> IMHO safely assume that the scheduler is designed such that it can
> handle fixed zero timestamps for a while.

You're confused.

If you provide your own sched_clock(), that's what is used by everyone
who calls sched_clock() both before and after sched_clock_init() has
been called.

sched_clock_cpu() and cpu_clock() both will return zero up until
sched_clock_init() has been called, but sched_clock() itself may not.

And these parts of the scheduler I'm talking about use sched_clock()
itself, not sched_clock_cpu() nor cpu_clock().

[PATCH] Add sched_clock to AT91 TCB clocksource driver

[Remy Bohmer]

Hi,

>> But late_time_init does it before the sched_clock is declared stable.
>> Until that point sched_clock will always return zero, as such we can
>> IMHO safely assume that the scheduler is designed such that it can
>> handle fixed zero timestamps for a while.
>
> You're confused.
>
> If you provide your own sched_clock(), that's what is used by everyone
> who calls sched_clock() both before and after sched_clock_init() has
> been called.
>
> sched_clock_cpu() and cpu_clock() both will return zero up until
> sched_clock_init() has been called, but sched_clock() itself may not.
>
> And these parts of the scheduler I'm talking about use sched_clock()
> itself, not sched_clock_cpu() nor cpu_clock().

Yep. You are right. I was confused... Sorry.

Kind regards,

Remy