Re: [PATCH 14/23] clocksource: increase initcall priority

Re: [PATCH 14/23] clocksource: increase initcall priority

[David Brownell]

> As a note, arm and mips both register their clocksources during
> time_init() instead of using initcalls.

That's actually platform-specific.  If there's only one possible
clocksource, that'd be the normal answer.

Otherwise, there can be multiple clocksources just like any other
architecture.  Maybe there's an always-present 32 KHz counter, and
a bank of timers that can run at higher frequences ... but those
may not be available for timekeeping on all boards, maybe because
they're used for something else (motor control, etc) or because the
system has prioritized battery life over timestamp precision.

So don't assume any platform doesn't use clocksource initcalls.

- Dave

Re: [PATCH 14/23] clocksource: increase initcall priority

[Daniel Walker]

On Wed, 2007-01-31 at 11:43 -0800, David Brownell wrote:
> > As a note, arm and mips both register their clocksources during
> > time_init() instead of using initcalls.
> 
> That's actually platform-specific.  If there's only one possible
> clocksource, that'd be the normal answer.
> 
> Otherwise, there can be multiple clocksources just like any other
> architecture.  Maybe there's an always-present 32 KHz counter, and
> a bank of timers that can run at higher frequences ... but those
> may not be available for timekeeping on all boards, maybe because
> they're used for something else (motor control, etc) or because the
> system has prioritized battery life over timestamp precision.
> 
> So don't assume any platform doesn't use clocksource initcalls.

What does your OMAP clocksource do now ? I thought one of the changes
that you made was to have both 32k and mpu both registered ..

Daniel

Re: [PATCH 14/23] clocksource: increase initcall priority

[Thomas Gleixner]

Daniel,

On Wed, 2007-01-31 at 14:47 -0800, Daniel Walker wrote:
> > So don't assume any platform doesn't use clocksource initcalls.
> 
> What does your OMAP clocksource do now ? I thought one of the changes
> that you made was to have both 32k and mpu both registered ..

It is up to the clocksource driver, when the clocksource_register() call
is done. This may happen in early boot as well as after initializing
some other things first.

Johns clocksource code works with ARM which does the register call in
timer_init() as well as with some other hardware which gets initialized
late in the boot process.

clocksource_initcall is simply superfluid. 

Adjusting the initcalls of a particular device to the point where it
becomes available is a seperate topic. I'm not in the way of that and I
think John's patch draft to solve the sound issue is heading into
exactly this direction.

We do not need another initcall level, especially not one which just
aliases an existing one.

The only thing we really want to control explicitely is when we actually
switch away from jiffies_clocksource. But this does not need an
artificial initcall at all.

	tglx

Re: [PATCH 14/23] clocksource: increase initcall priority

[Daniel Walker]

On Thu, 2007-02-01 at 00:23 +0100, Thomas Gleixner wrote:
> Daniel,
> 
> On Wed, 2007-01-31 at 14:47 -0800, Daniel Walker wrote:
> > > So don't assume any platform doesn't use clocksource initcalls.
> > 
> > What does your OMAP clocksource do now ? I thought one of the changes
> > that you made was to have both 32k and mpu both registered ..
> 
> It is up to the clocksource driver, when the clocksource_register() call
> is done. This may happen in early boot as well as after initializing
> some other things first.
> 
> Johns clocksource code works with ARM which does the register call in
> timer_init() as well as with some other hardware which gets initialized
> late in the boot process.
> 
> clocksource_initcall is simply superfluid. 
> 


My position has always been that clocksources should be registered as
early as possible .. The fs_initcall() usage is a compromise stemming
from early resistance that John, and you gave to moving the clocks up in
the initcall sequence. the clocksource_initcall() exists only to allow
the clocks easily be raised if it was needed ..

I'm glad that you, John, and myself have come to a consensus on the
issue offline ..

Daniel 

Re: [PATCH 14/23] clocksource: increase initcall priority

[David Brownell]

On Wednesday 31 January 2007 2:47 pm, Daniel Walker wrote:
> On Wed, 2007-01-31 at 11:43 -0800, David Brownell wrote:
> > > As a note, arm and mips both register their clocksources during
> > > time_init() instead of using initcalls.
> > 
> > That's actually platform-specific.  ...
> > 
> > So don't assume any platform doesn't use clocksource initcalls.
> 
> What does your OMAP clocksource do now ? 

OMAP being one family of ARMs ... well, maybe a few families
under the same branding umbrella.


> 	I thought one of the changes 
> that you made was to have both 32k and mpu both registered ..

Some OMAP1 chips will do that, if the MPU timer is configured *AND*
they have that 32K counter register.  Not OMAP2 (which doesn't have
MPU timers), and not the older OMAP1 chips (no 32K counter).

It turned out to be simpler to do it that way.  Otherwise the
configuration options get too confusing.  There are three types
of timer (MPU timers on most OMAP1 systems, 32K timer on some, dual
mode timers on newer chips including OMAP2) that can generate ticks.

In some cases, another of those timers wil also be used in free
run mode as a clocksource.  That 32K counter isn't a timer, but
it's a fine clocksource (if present).  But until clockevents go
upstream, it's kind of pointless to factor it all differently.

- Dave

Re: [PATCH 14/23] clocksource: increase initcall priority

[Thomas Gleixner]

On Wed, 2007-01-31 at 16:15 -0800, Daniel Walker wrote:
> > clocksource_initcall is simply superfluid. 
>
> My position has always been that clocksources should be registered as
> early as possible .. The fs_initcall() usage is a compromise stemming
> from early resistance that John, and you gave to moving the clocks up in
> the initcall sequence.

No. I never objected against the registering of clocks at any given
time. Why would I have otherwise accepted ARM patches, which register
their clocksources in the early timer init ?

The only concern I had and still have is when we decide to use something
else than the "safe" heaven of jiffies.

	tglx

Re: [PATCH 14/23] clocksource: increase initcall priority

[Thomas Gleixner]

On Wed, 2007-01-31 at 09:20 -0800, Daniel Walker wrote:
> The purpose of clocksource_initcall is so the initcall can be increased
> without changing all the individual clocksources .. So I have to
> disconnect that from the acpi_pm because it can't tolerate it.

Each clocksource has to decide on its own, when the registration is
done. ARM clocksources register in the timer init, pm timer needs to
wait. 

So what exactly is solved by an extra initcall ?

	tglx

Re: [PATCH 14/23] clocksource: increase initcall priority

[Daniel Walker]

On Wed, 2007-01-31 at 18:10 +0100, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > On Wed, 2007-01-31 at 12:50 +0100, Ingo Molnar wrote:
> > > * Daniel Walker <dwalker@mvista.com> wrote:
> > > 
> > > > -module_init(init_acpi_pm_clocksource);
> > > > +/*
> > > > + * This clocksource is removed from the clocksource_initcall
> > > > + * macro since it's mandatory for it to be in fs_initcall as the
> > > > + * highest initcall level, or else it doesn't work properly with
> > > > + * it's PCI fix ups.
> > > > + */
> > > > +fs_initcall(init_acpi_pm_clocksource);
> > > 
> > > ugh - this bit looks quite interesting.
> > > 
> > > what's the purpose of this patch? Switching to high-res timers 
> > > should be done late in the bootup - in case there's a problem it's 
> > > more debuggable, etc.
> > 
> > The timekeeping code, and hrt/dynamic tick both wait till after boot 
> > up to switch to high res mode ..
> 
> again, let me repeat the question: what's the purpose of this (whole!) 
> patch. Not just of this chunk - the whole patch. You change around 
> initcall levels - why? It makes no sense to me. The explanation in the 
> patch header makes no sense to me.
> 
> Please if possible try to build clear arguments: first outlining "this 
> is how it worked before: <X>", then outlining "this is how it will work 
> after my change: <Y>", and then maybe a small blurb about "Y is better 
> than X, because: <Z>". That X, Y, Z analysis is what is needed to accept 
> a patch.

I don't know that I could explain it better than what's in the patch
header .. I'll think about it.

> > The specific code block you commented on above was added cause acpi_pm 
> > has special pci fixups that don't function properly earlier in boot 
> > up. So it can't be arbitrarily raised to order in the initcall 
> > sequence ..
> 
> i think you misunderstood my comment. In the chunk above you used 
> fs_initcall() initcall instead of clocksource_initcall(). (Which is 
> funny even ignoring the bad hack that clocksource_initcall is defined to 
> fs_initcall.)

The purpose of clocksource_initcall is so the initcall can be increased
without changing all the individual clocksources .. So I have to
disconnect that from the acpi_pm because it can't tolerate it.

Daniel

Re: [PATCH 14/23] clocksource: increase initcall priority

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> On Wed, 2007-01-31 at 12:50 +0100, Ingo Molnar wrote:
> > * Daniel Walker <dwalker@mvista.com> wrote:
> > 
> > > -module_init(init_acpi_pm_clocksource);
> > > +/*
> > > + * This clocksource is removed from the clocksource_initcall
> > > + * macro since it's mandatory for it to be in fs_initcall as the
> > > + * highest initcall level, or else it doesn't work properly with
> > > + * it's PCI fix ups.
> > > + */
> > > +fs_initcall(init_acpi_pm_clocksource);
> > 
> > ugh - this bit looks quite interesting.
> > 
> > what's the purpose of this patch? Switching to high-res timers 
> > should be done late in the bootup - in case there's a problem it's 
> > more debuggable, etc.
> 
> The timekeeping code, and hrt/dynamic tick both wait till after boot 
> up to switch to high res mode ..

again, let me repeat the question: what's the purpose of this (whole!) 
patch. Not just of this chunk - the whole patch. You change around 
initcall levels - why? It makes no sense to me. The explanation in the 
patch header makes no sense to me.

Please if possible try to build clear arguments: first outlining "this 
is how it worked before: <X>", then outlining "this is how it will work 
after my change: <Y>", and then maybe a small blurb about "Y is better 
than X, because: <Z>". That X, Y, Z analysis is what is needed to accept 
a patch.

> The specific code block you commented on above was added cause acpi_pm 
> has special pci fixups that don't function properly earlier in boot 
> up. So it can't be arbitrarily raised to order in the initcall 
> sequence ..

i think you misunderstood my comment. In the chunk above you used 
fs_initcall() initcall instead of clocksource_initcall(). (Which is 
funny even ignoring the bad hack that clocksource_initcall is defined to 
fs_initcall.)

	Ingo

Re: [PATCH 14/23] clocksource: increase initcall priority

[Daniel Walker]

On Wed, 2007-01-31 at 12:50 +0100, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > -module_init(init_acpi_pm_clocksource);
> > +/*
> > + * This clocksource is removed from the clocksource_initcall
> > + * macro since it's mandatory for it to be in fs_initcall as the
> > + * highest initcall level, or else it doesn't work properly with
> > + * it's PCI fix ups.
> > + */
> > +fs_initcall(init_acpi_pm_clocksource);
> 
> ugh - this bit looks quite interesting.
> 
> what's the purpose of this patch? Switching to high-res timers should be 
> done late in the bootup - in case there's a problem it's more 
> debuggable, etc.

The timekeeping code, and hrt/dynamic tick both wait till after boot up
to switch to high res mode ..

The specific code block you commented on above was added cause acpi_pm
has special pci fixups that don't function properly earlier in boot up.
So it can't be arbitrarily raised to order in the initcall sequence ..

Daniel

Re: [PATCH 14/23] clocksource: increase initcall priority

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> -module_init(init_acpi_pm_clocksource);
> +/*
> + * This clocksource is removed from the clocksource_initcall
> + * macro since it's mandatory for it to be in fs_initcall as the
> + * highest initcall level, or else it doesn't work properly with
> + * it's PCI fix ups.
> + */
> +fs_initcall(init_acpi_pm_clocksource);

ugh - this bit looks quite interesting.

what's the purpose of this patch? Switching to high-res timers should be 
done late in the bootup - in case there's a problem it's more 
debuggable, etc.

	Ingo

[PATCH 14/23] clocksource: increase initcall priority

[Daniel Walker]

[-- Attachment #1: clocksource_init_call.patch --]
[-- Type: text/plain, Size: 9439 bytes --]


Normal systems often have almost everything registered in
device_initcall() . Most drivers are registered there, and usually if
people add code that needs an initcall they will either use
device_initcall() or module_init() which both result in the same
initcall..

When John created the clocksource interface he did what most people
would do , and he made the clocksource registration happen in  
device_initcall with most everything else .. The effect of doing this
was the addition of the following code,

/* clocksource_done_booting - Called near the end of bootup
 *
 * Hack to avoid lots of clocksource churn at boot time
 */
static int __init clocksource_done_booting(void)
{
        finished_booting = 1;
        return 0;
}

late_initcall(clocksource_done_booting);

This is one of two initcalls in the clocksource interface , the other
one is device_initcall(init_clocksource_sysfs); ..

If I leave the clocksource initcall alone then anything that uses a
clocksource in the future would need at least one late_initcall().
Since the clocksources aren't all fully register until after 
device_initcall. 

The reason behind changing that is because it doesn't fit the usually
development flow of initialization functions which , as I said earlier,
almost always end up into device_initcall .

This change certainly isn't mandatory . I feel it would reduce the
likely hood of developers that use the clocksource interface from adding
multiple initcalls (one late_initcall, and one device_initcall). It also
better fits developers tendencies to put almost everything into
device_initcall() ..

In addition,

This patch removes a small amount of code in time keeping which existed to
detect the end of the initcall sequence then selected a clock.

As a note, arm and mips both register their clocksources during time_init()
instead of using initcalls.

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

---
 arch/i386/kernel/hpet.c          |    2 +-
 arch/i386/kernel/i8253.c         |    2 +-
 arch/i386/kernel/tsc.c           |    2 +-
 arch/x86_64/kernel/hpet.c        |    2 +-
 arch/x86_64/kernel/tsc.c         |    3 +--
 drivers/clocksource/acpi_pm.c    |    8 +++++++-
 drivers/clocksource/cyclone.c    |    2 +-
 drivers/clocksource/scx200_hrt.c |    2 +-
 include/linux/clocksource.h      |    6 ++++++
 kernel/time/clocksource.c        |   13 -------------
 kernel/time/jiffies.c            |    2 +-
 kernel/time/tick-sched.c         |    8 ++++++++
 kernel/time/timekeeping.c        |   15 +++++++--------
 13 files changed, 36 insertions(+), 31 deletions(-)

Index: linux-2.6.19/arch/i386/kernel/hpet.c
===================================================================
--- linux-2.6.19.orig/arch/i386/kernel/hpet.c
+++ linux-2.6.19/arch/i386/kernel/hpet.c
@@ -315,7 +315,7 @@ static int __init init_hpet_clocksource(
 	return clocksource_register(&clocksource_hpet);
 }
 
-module_init(init_hpet_clocksource);
+clocksource_initcall(init_hpet_clocksource);
 
 #ifdef CONFIG_HPET_EMULATE_RTC
 
Index: linux-2.6.19/arch/i386/kernel/i8253.c
===================================================================
--- linux-2.6.19.orig/arch/i386/kernel/i8253.c
+++ linux-2.6.19/arch/i386/kernel/i8253.c
@@ -195,4 +195,4 @@ static int __init init_pit_clocksource(v
 	clocksource_pit.mult = clocksource_hz2mult(CLOCK_TICK_RATE, 20);
 	return clocksource_register(&clocksource_pit);
 }
-module_init(init_pit_clocksource);
+clocksource_initcall(init_pit_clocksource);
Index: linux-2.6.19/arch/i386/kernel/tsc.c
===================================================================
--- linux-2.6.19.orig/arch/i386/kernel/tsc.c
+++ linux-2.6.19/arch/i386/kernel/tsc.c
@@ -458,4 +458,4 @@ static int __init init_tsc_clocksource(v
 	return 0;
 }
 
-module_init(init_tsc_clocksource);
+clocksource_initcall(init_tsc_clocksource);
Index: linux-2.6.19/arch/x86_64/kernel/hpet.c
===================================================================
--- linux-2.6.19.orig/arch/x86_64/kernel/hpet.c
+++ linux-2.6.19/arch/x86_64/kernel/hpet.c
@@ -508,4 +508,4 @@ static int __init init_hpet_clocksource(
 	return clocksource_register(&clocksource_hpet);
 }
 
-module_init(init_hpet_clocksource);
+clocksource_initcall(init_hpet_clocksource);
Index: linux-2.6.19/arch/x86_64/kernel/tsc.c
===================================================================
--- linux-2.6.19.orig/arch/x86_64/kernel/tsc.c
+++ linux-2.6.19/arch/x86_64/kernel/tsc.c
@@ -224,5 +224,4 @@ static int __init init_tsc_clocksource(v
 	}
 	return 0;
 }
-
-module_init(init_tsc_clocksource);
+clocksource_initcall(init_tsc_clocksource);
Index: linux-2.6.19/drivers/clocksource/acpi_pm.c
===================================================================
--- linux-2.6.19.orig/drivers/clocksource/acpi_pm.c
+++ linux-2.6.19/drivers/clocksource/acpi_pm.c
@@ -214,4 +214,10 @@ pm_good:
 	return clocksource_register(&clocksource_acpi_pm);
 }
 
-module_init(init_acpi_pm_clocksource);
+/*
+ * This clocksource is removed from the clocksource_initcall
+ * macro since it's mandatory for it to be in fs_initcall as the
+ * highest initcall level, or else it doesn't work properly with
+ * it's PCI fix ups.
+ */
+fs_initcall(init_acpi_pm_clocksource);
Index: linux-2.6.19/drivers/clocksource/cyclone.c
===================================================================
--- linux-2.6.19.orig/drivers/clocksource/cyclone.c
+++ linux-2.6.19/drivers/clocksource/cyclone.c
@@ -116,4 +116,4 @@ static int __init init_cyclone_clocksour
 	return clocksource_register(&clocksource_cyclone);
 }
 
-module_init(init_cyclone_clocksource);
+clocksource_initcall(init_cyclone_clocksource);
Index: linux-2.6.19/drivers/clocksource/scx200_hrt.c
===================================================================
--- linux-2.6.19.orig/drivers/clocksource/scx200_hrt.c
+++ linux-2.6.19/drivers/clocksource/scx200_hrt.c
@@ -94,7 +94,7 @@ static int __init init_hrt_clocksource(v
 	return clocksource_register(&cs_hrt);
 }
 
-module_init(init_hrt_clocksource);
+clocksource_initcall(init_hrt_clocksource);
 
 MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
 MODULE_DESCRIPTION("clocksource on SCx200 HiRes Timer");
Index: linux-2.6.19/include/linux/clocksource.h
===================================================================
--- linux-2.6.19.orig/include/linux/clocksource.h
+++ linux-2.6.19/include/linux/clocksource.h
@@ -41,6 +41,12 @@ extern struct atomic_notifier_head clock
 #define CLOCKSOURCE_NOTIFY_RATING	2
 #define CLOCKSOURCE_NOTIFY_FREQ		4
 
+/*
+ * Defined so the initcall can be changes without touching
+ * every clocksource in the system.
+ */
+#define clocksource_initcall(x)		fs_initcall(x)
+
 /**
  * clocksource_notifier_register - Registers a list change notifier
  * @nb:		pointer to a notifier block
Index: linux-2.6.19/kernel/time/clocksource.c
===================================================================
--- linux-2.6.19.orig/kernel/time/clocksource.c
+++ linux-2.6.19/kernel/time/clocksource.c
@@ -38,22 +38,9 @@
  */
 static LIST_HEAD(clocksource_list);
 static DEFINE_SPINLOCK(clocksource_lock);
-static int finished_booting;
 
 ATOMIC_NOTIFIER_HEAD(clocksource_list_notifier);
 
-/* clocksource_done_booting - Called near the end of bootup
- *
- * Hack to avoid lots of clocksource churn at boot time
- */
-static int __init clocksource_done_booting(void)
-{
-	finished_booting = 1;
-	return 0;
-}
-
-late_initcall(clocksource_done_booting);
-
 /**
  * __is_registered - Returns a clocksource if it's registered
  * @name:		name of the clocksource to return
Index: linux-2.6.19/kernel/time/jiffies.c
===================================================================
--- linux-2.6.19.orig/kernel/time/jiffies.c
+++ linux-2.6.19/kernel/time/jiffies.c
@@ -69,4 +69,4 @@ static int __init init_jiffies_clocksour
 	return clocksource_register(&clocksource_jiffies);
 }
 
-module_init(init_jiffies_clocksource);
+clocksource_initcall(init_jiffies_clocksource);
Index: linux-2.6.19/kernel/time/tick-sched.c
===================================================================
--- linux-2.6.19.orig/kernel/time/tick-sched.c
+++ linux-2.6.19/kernel/time/tick-sched.c
@@ -540,6 +540,14 @@ int tick_check_oneshot_change(int allow_
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
 
+	/*
+	 * HRT or Dynamic tick can't be started during init, but it's possible
+	 * for generic time to switch to a good clock during init. So
+	 * explicitly check that we are't still booting here.
+	 */
+	if (system_state == SYSTEM_BOOTING)
+		return 0;
+
 	if (!test_and_clear_bit(0, &ts->check_clocks))
 		return 0;
 
Index: linux-2.6.19/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.19.orig/kernel/time/timekeeping.c
+++ linux-2.6.19/kernel/time/timekeeping.c
@@ -318,14 +318,6 @@ static __init int timekeeping_init_sysfs
 device_initcall(timekeeping_init_sysfs);
 #endif /* CONFIG_SYSFS */
 
-static int __init timekeeping_early_clockswitch(void)
-{
-	atomic_inc(&clock_check);
-	return 0;
-}
-
-late_initcall(timekeeping_early_clockswitch);
-
 /**
  * boot_override_clocksource - boot clock override
  * @str:	override name
@@ -471,6 +463,13 @@ static int __init timekeeping_init_devic
 	if (!error)
 		error = sysdev_register(&device_timer);
 
+#ifdef CONFIG_GENERIC_TIME
+	/*
+	 * Now we signal to switch to a high-res clock.
+	 */
+	atomic_inc(&clock_check);
+#endif
+
 	return error;
 }
 

--