| Message ID | 1312124593-6088-1-git-send-email-linux@bohmer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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?
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
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().
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
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); +
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(-)