[PATCH 1/3] Add clocksource_register_hz/khz interface

[PATCH 1/3] Add clocksource_register_hz/khz interface

[John Stultz]

Hey Thomas,

It would be great to get this into 2.6.35, so I can push on getting the per-arch
clocksource conversions queued and tested for 2.6.36-37.

thanks
-john

How to pick good mult/shif pairs has always been difficult to describe
to folks writing clocksource drivers, since it requires careful tradeoffs
in adjustment accuracy vs overflow limits.

Now, with the clocks_calc_mult_shift function, its much easier. However,
not many clocksources have converted to using that function, and there is
still the issue of the max interval length assumption being made by
each clocksource driver independently.

So this patch simplifies the registration process by having clocksources
be registered with a hz/khz value and the the registration function taking
care of setting mult/shift.

This should take most of the confusion out of writing a clocksource driver.

Additionally it also keeps the shift size tradeoff (more accuracy vs longer
possible nohz times) centralized so the timekeeping core can keep track of the
assumptions being made.

CC: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Stultz <johnstul@us.ibm.com>
---
 include/linux/clocksource.h |    5 +++
 kernel/time/clocksource.c   |   71 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 4bca8b6..1ef1c36 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -274,6 +274,9 @@ static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 shift)
 
 
 /* used to install a new clocksource */
+
+extern int clocksource_register_hz(struct clocksource*, u32);
+extern int clocksource_register_khz(struct clocksource*, u32);
 extern int clocksource_register(struct clocksource*);
 extern void clocksource_unregister(struct clocksource*);
 extern void clocksource_touch_watchdog(void);
@@ -294,6 +297,8 @@ clocksource_calc_mult_shift(struct clocksource *cs, u32 freq, u32 minsec)
 				      NSEC_PER_SEC, minsec);
 }
 
+
+
 #ifdef CONFIG_GENERIC_TIME_VSYSCALL
 extern void
 update_vsyscall(struct timespec *ts, struct clocksource *c, u32 mult);
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 1f5dde6..73b9aeb 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -625,6 +625,77 @@ static void clocksource_enqueue(struct clocksource *cs)
 	list_add(&cs->list, entry);
 }
 
+
+
+
+/* Maximum time we expect to go between ticks.
+ * This is includes idle tickless time. It provides
+ * the trade off between selecting a mult/shift pair 
+ * that is very precise but can only handle a short period 
+ * of time, vs. a mult/shift pair that can handle long 
+ * periods of time but isn't as precise. 
+ *
+ * This is a subsystem constant, and actual hardware limitations
+ * may override it (ie: clocksources that wrap every 3 seconds).
+ */
+#define MAX_UPDATE_LENGTH 5 /* Seconds */
+
+
+/**
+ * clocksource_register - Used to install new clocksources
+ * @t:		clocksource to be registered
+ *
+ * Returns -EBUSY if registration fails, zero otherwise.
+ */
+int clocksource_register_hz(struct clocksource *cs, u32 hz)
+{
+	
+	/* Ideally we want to use  some of the limits used in 
+	 * clocksource_max_deferment, to provide a more informed
+	 * MAX_UPDATE_LENGTH. But for now this just gets the
+	 * regiseter interface working properly.
+	 */
+	clocks_calc_mult_shift(&cs->mult, &cs->shift, hz,
+				      NSEC_PER_SEC, MAX_UPDATE_LENGTH);
+	cs->max_idle_ns = clocksource_max_deferment(cs);
+
+	mutex_lock(&clocksource_mutex);
+	clocksource_enqueue(cs);
+	clocksource_select();
+	clocksource_enqueue_watchdog(cs);
+	mutex_unlock(&clocksource_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(clocksource_register_hz);
+
+/**
+ * clocksource_register - Used to install new clocksources
+ * @t:		clocksource to be registered
+ *
+ * Returns -EBUSY if registration fails, zero otherwise.
+ */
+int clocksource_register_khz(struct clocksource *cs, u32 khz)
+{
+	
+	/* Ideally we want to use  some of the limits used in 
+	 * clocksource_max_deferment, to provide a more informed
+	 * MAX_UPDATE_LENGTH. But for now this just gets the
+	 * regiseter interface working properly.
+	 */
+	clocks_calc_mult_shift(&cs->mult, &cs->shift, khz,
+				      NSEC_PER_SEC/1000,
+				      MAX_UPDATE_LENGTH*1000);
+	cs->max_idle_ns = clocksource_max_deferment(cs);
+
+	mutex_lock(&clocksource_mutex);
+	clocksource_enqueue(cs);
+	clocksource_select();
+	clocksource_enqueue_watchdog(cs);
+	mutex_unlock(&clocksource_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(clocksource_register_hz);
+
 /**
  * clocksource_register - Used to install new clocksources
  * @t:		clocksource to be registered
-- 
1.6.0.4

[PATCH 2/3] Convert common x86 clocksources to use clocksource_register_hz/khz

[John Stultz]

This converts the most common of the x86 clocksources over to use
clocksource_register_hz/khz.

I've tested this on my system, and would probably be fine for 2.6.35.

CC: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Stultz <johnstul@us.ibm.com>
---
 arch/x86/kernel/hpet.c        |   13 +++++++++----
 arch/x86/kernel/tsc.c         |    6 ++----
 drivers/clocksource/acpi_pm.c |    9 ++-------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 23b4ecd..bfd1e6f 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -16,7 +16,6 @@
 #include <asm/hpet.h>
 
 #define HPET_MASK			CLOCKSOURCE_MASK(32)
-#define HPET_SHIFT			22
 
 /* FSEC = 10^-15
    NSEC = 10^-9 */
@@ -782,7 +781,6 @@ static struct clocksource clocksource_hpet = {
 	.rating		= 250,
 	.read		= read_hpet,
 	.mask		= HPET_MASK,
-	.shift		= HPET_SHIFT,
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 	.resume		= hpet_resume_counter,
 #ifdef CONFIG_X86_64
@@ -793,6 +791,7 @@ static struct clocksource clocksource_hpet = {
 static int hpet_clocksource_register(void)
 {
 	u64 start, now;
+	u64 hpet_freq;
 	cycle_t t1;
 
 	/* Start the counter */
@@ -827,9 +826,15 @@ static int hpet_clocksource_register(void)
 	 *  mult = (hpet_period * 2^shift)/10^6
 	 *  mult = (hpet_period << shift)/FSEC_PER_NSEC
 	 */
-	clocksource_hpet.mult = div_sc(hpet_period, FSEC_PER_NSEC, HPET_SHIFT);
 
-	clocksource_register(&clocksource_hpet);
+	/* Need to convert hpet_period (fsec/cyc) to cyc/sec:
+	 * 
+	 * cyc/sec = FSEC_PER_SEC/hpet_period(fsec/cyc)
+	 * cyc/sec = (FSEC_PER_NSEC * NSEC_PER_SEC)/hpet_period
+	 */
+	hpet_freq = FSEC_PER_NSEC * NSEC_PER_SEC;
+	do_div(hpet_freq, hpet_period);
+	clocksource_register_hz(&clocksource_hpet, (u32)hpet_freq);
 
 	return 0;
 }
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 9faf91a..5ca6370 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -751,7 +751,6 @@ static struct clocksource clocksource_tsc = {
 	.read                   = read_tsc,
 	.resume			= resume_tsc,
 	.mask                   = CLOCKSOURCE_MASK(64),
-	.shift                  = 22,
 	.flags                  = CLOCK_SOURCE_IS_CONTINUOUS |
 				  CLOCK_SOURCE_MUST_VERIFY,
 #ifdef CONFIG_X86_64
@@ -845,8 +844,6 @@ __cpuinit int unsynchronized_tsc(void)
 
 static void __init init_tsc_clocksource(void)
 {
-	clocksource_tsc.mult = clocksource_khz2mult(tsc_khz,
-			clocksource_tsc.shift);
 	if (tsc_clocksource_reliable)
 		clocksource_tsc.flags &= ~CLOCK_SOURCE_MUST_VERIFY;
 	/* lower the rating if we already know its unstable: */
@@ -854,7 +851,8 @@ static void __init init_tsc_clocksource(void)
 		clocksource_tsc.rating = 0;
 		clocksource_tsc.flags &= ~CLOCK_SOURCE_IS_CONTINUOUS;
 	}
-	clocksource_register(&clocksource_tsc);
+	
+	clocksource_register_khz(&clocksource_tsc, tsc_khz);
 }
 
 #ifdef CONFIG_X86_64
diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 72a633a..cfb0f52 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -68,10 +68,7 @@ static struct clocksource clocksource_acpi_pm = {
 	.rating		= 200,
 	.read		= acpi_pm_read,
 	.mask		= (cycle_t)ACPI_PM_MASK,
-	.mult		= 0, /*to be calculated*/
-	.shift		= 22,
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
-
 };
 
 
@@ -190,9 +187,6 @@ static int __init init_acpi_pm_clocksource(void)
 	if (!pmtmr_ioport)
 		return -ENODEV;
 
-	clocksource_acpi_pm.mult = clocksource_hz2mult(PMTMR_TICKS_PER_SEC,
-						clocksource_acpi_pm.shift);
-
 	/* "verify" this timing source: */
 	for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
 		udelay(100 * j);
@@ -220,7 +214,8 @@ static int __init init_acpi_pm_clocksource(void)
 	if (verify_pmtmr_rate() != 0)
 		return -ENODEV;
 
-	return clocksource_register(&clocksource_acpi_pm);
+	return clocksource_register_hz(&clocksource_acpi_pm,
+						PMTMR_TICKS_PER_SEC);
 }
 
 /* We use fs_initcall because we want the PCI fixups to have run
-- 
1.6.0.4

[RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[John Stultz]

NOT FOR INCLUSION!
NOT FOR INCLUSION!

I've already gone through and converted the rest of the clocksources 
to use clocksource_register_hz, and I'll be hopefully pushing those 
to arch maintainers for 2.6.36-2.6.37.

However, in going through all the clocksources, I hit a few
non-trivial conversions and wanted to bring them up on the list
early so they can be handled soon.

The following patch tries to convert the non-trival clocksources
to use clocksource_register_hz/khz. It is likely broken. I will
need arch maintainer help to figure out the best way to resovle 
these clocksources.

This patch requires the previous "Add clocksource_register_hz/khz 
interface" patch to build. 

So any help maintainers can provide in finding solutions to
break the mult/shift assumptions in the arch code would be
greatly appreciated!

thanks
-john

NOT FOR INCLUSION!
NOT FOR INCLUSION!

CC: Ralf Baechle <ralf@linux-mips.org>
CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mundt <lethal@linux-sh.org>
CC: Jim Cromie <jim.cromie@gmail.com>
CC: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Stultz <johnstul@us.ibm.com>
---
 arch/mips/nxp/pnx8550/common/time.c |    7 ++++++-
 arch/powerpc/kernel/time.c          |   14 +++++++-------
 arch/s390/kernel/time.c             |   13 ++++++++++---
 drivers/clocksource/scx200_hrt.c    |   19 ++++++-------------
 drivers/clocksource/sh_cmt.c        |    4 ++--
 drivers/clocksource/sh_tmu.c        |    4 ++--
 kernel/time/jiffies.c               |    3 ++-
 7 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/arch/mips/nxp/pnx8550/common/time.c b/arch/mips/nxp/pnx8550/common/time.c
index 8836c62..a4420f0 100644
--- a/arch/mips/nxp/pnx8550/common/time.c
+++ b/arch/mips/nxp/pnx8550/common/time.c
@@ -104,7 +104,12 @@ __init void plat_time_init(void)
 
 	pnx8xxx_clockevent.cpumask = cpu_none_mask;
 	clockevents_register_device(&pnx8xxx_clockevent);
-	clocksource_register(&pnx_clocksource);
+	
+	/*
+	 * XXX - Nothing seems to set pnx_clocksource mult/shift! 
+	 * So I don't know what freq to use here. Help! -johnstul
+	 */
+	clocksource_register_hz(&pnx_clocksource, 0);
 
 	/* Timer 1 start */
 	configPR = read_c0_config7();
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 1b16b9a..827abe6 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -86,8 +86,6 @@ static struct clocksource clocksource_rtc = {
 	.rating       = 400,
 	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
 	.mask         = CLOCKSOURCE_MASK(64),
-	.shift        = 22,
-	.mult         = 0,	/* To be filled in */
 	.read         = rtc_read,
 };
 
@@ -97,8 +95,6 @@ static struct clocksource clocksource_timebase = {
 	.rating       = 400,
 	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
 	.mask         = CLOCKSOURCE_MASK(64),
-	.shift        = 22,
-	.mult         = 0,	/* To be filled in */
 	.read         = timebase_read,
 };
 
@@ -848,6 +844,12 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock,
 	/* Make userspace gettimeofday spin until we're done. */
 	++vdso_data->tb_update_count;
 	smp_mb();
+	
+	/*
+	 * XXX UGH! the below assumes  clock->shift == 22. 
+	 * This will no longer be the case with clocksource_register_hz
+	 * HELP! -johnstul
+	 */
 
 	/* XXX this assumes clock->shift == 22 */
 	/* 4611686018 ~= 2^(20+64-22) / 1e9 */
@@ -878,9 +880,7 @@ static void __init clocksource_init(void)
 	else
 		clock = &clocksource_timebase;
 
-	clock->mult = clocksource_hz2mult(tb_ticks_per_sec, clock->shift);
-
-	if (clocksource_register(clock)) {
+	if (clocksource_register_hz(clock, tb_ticks_per_sec)) {
 		printk(KERN_ERR "clocksource: %s is already registered\n",
 		       clock->name);
 		return;
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index d906bf1..6412752 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -197,8 +197,6 @@ static struct clocksource clocksource_tod = {
 	.rating		= 400,
 	.read		= read_tod_clock,
 	.mask		= -1ULL,
-	.mult		= 1000,
-	.shift		= 12,
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
@@ -257,7 +255,16 @@ void __init time_init(void)
 	if (register_external_interrupt(0x1406, timing_alert_interrupt))
 		panic("Couldn't request external interrupt 0x1406");
 
-	if (clocksource_register(&clocksource_tod) != 0)
+	/* 
+	 * XXX - Brute forced tod freq from:
+	 *    2^shift/mult * NSEC_PER_SEC	= cyc/sec
+	 *    2^12 / 1000  * 1000000000		= cyc/sec
+	 *    2^12 * 1000000			= cyc/sec
+	 *    2^12 * 1000			= kcyc/sec
+	 *    1000<<12				= kcyc/sec
+	 * Could be totally wrong. -johnstul
+	 */
+	if (clocksource_register_khz(&clocksource_tod, (1000<<12) ) != 0)
 		panic("Could not register TOD clock source");
 
 	/* Enable TOD clock interrupts on the boot cpu. */
diff --git a/drivers/clocksource/scx200_hrt.c b/drivers/clocksource/scx200_hrt.c
index 27f4d96..24f884c 100644
--- a/drivers/clocksource/scx200_hrt.c
+++ b/drivers/clocksource/scx200_hrt.c
@@ -49,9 +49,6 @@ static cycle_t read_hrt(struct clocksource *cs)
 	return (cycle_t) inl(scx200_cb_base + SCx200_TIMER_OFFSET);
 }
 
-#define HRT_SHIFT_1	22
-#define HRT_SHIFT_27	26
-
 static struct clocksource cs_hrt = {
 	.name		= "scx200_hrt",
 	.rating		= 250,
@@ -63,6 +60,7 @@ static struct clocksource cs_hrt = {
 
 static int __init init_hrt_clocksource(void)
 {
+	u32 freq;
 	/* Make sure scx200 has initialized the configuration block */
 	if (!scx200_cb_present())
 		return -ENODEV;
@@ -79,19 +77,14 @@ static int __init init_hrt_clocksource(void)
 	outb(HR_TMEN | (mhz27 ? HR_TMCLKSEL : 0),
 	     scx200_cb_base + SCx200_TMCNFG_OFFSET);
 
-	if (mhz27) {
-		cs_hrt.shift = HRT_SHIFT_27;
-		cs_hrt.mult = clocksource_hz2mult((HRT_FREQ + ppm) * 27,
-						  cs_hrt.shift);
-	} else {
-		cs_hrt.shift = HRT_SHIFT_1;
-		cs_hrt.mult = clocksource_hz2mult(HRT_FREQ + ppm,
-						  cs_hrt.shift);
-	}
+	freq = (HRT_FREQ + ppm);
+	if (mhz27)
+		freq *= 27;
+
 	printk(KERN_INFO "enabling scx200 high-res timer (%s MHz +%d ppm)\n",
 		mhz27 ? "27":"1", ppm);
 
-	return clocksource_register(&cs_hrt);
+	return clocksource_register_hz(&cs_hrt, freq);
 }
 
 module_init(init_hrt_clocksource);
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 744f748..23bb543 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -422,7 +422,6 @@ static int sh_cmt_clocksource_enable(struct clocksource *cs)
 		return ret;
 
 	/* TODO: calculate good shift from rate and counter bit width */
-	cs->shift = 0;
 	cs->mult = clocksource_hz2mult(p->rate, cs->shift);
 	return 0;
 }
@@ -452,7 +451,8 @@ static int sh_cmt_register_clocksource(struct sh_cmt_priv *p,
 	cs->mask = CLOCKSOURCE_MASK(sizeof(unsigned long) * 8);
 	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
 	pr_info("sh_cmt: %s used as clock source\n", cs->name);
-	clocksource_register(cs);
+	/* XXX - What do we do about rate changes? -johnstul */
+	clocksource_register_hz(cs, p->rate);
 	return 0;
 }
 
diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index fc9ff1e..5d7f947 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -207,7 +207,6 @@ static int sh_tmu_clocksource_enable(struct clocksource *cs)
 		return ret;
 
 	/* TODO: calculate good shift from rate and counter bit width */
-	cs->shift = 10;
 	cs->mult = clocksource_hz2mult(p->rate, cs->shift);
 	return 0;
 }
@@ -230,7 +229,8 @@ static int sh_tmu_register_clocksource(struct sh_tmu_priv *p,
 	cs->mask = CLOCKSOURCE_MASK(32);
 	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
 	pr_info("sh_tmu: %s used as clock source\n", cs->name);
-	clocksource_register(cs);
+	/* XXX - What do we do about the rate changes in enable/disable? */
+	clocksource_register_hz(cs, p->rate);
 	return 0;
 }
 
diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
index 5404a84..73b0a86 100644
--- a/kernel/time/jiffies.c
+++ b/kernel/time/jiffies.c
@@ -66,7 +66,8 @@ struct clocksource clocksource_jiffies = {
 
 static int __init init_jiffies_clocksource(void)
 {
-	return clocksource_register(&clocksource_jiffies);
+	/* XXX - HRMMM.. we're losing some precision since ACTHZ>>8 != HZ */
+	return clocksource_register_hz(&clocksource_jiffies, HZ);
 }
 
 core_initcall(init_jiffies_clocksource);
-- 
1.6.0.4

Re: [PATCH 1/3] Add clocksource_register_hz/khz interface

[Thomas Gleixner]

On Fri, 30 Apr 2010, John Stultz wrote:
> +/**
> + * clocksource_register - Used to install new clocksources
> + * @t:		clocksource to be registered

  Can you fix the kernel doc please ?

> + *
> + * Returns -EBUSY if registration fails, zero otherwise.
> + */
> +int clocksource_register_hz(struct clocksource *cs, u32 hz)
> +{
> +	
> +	/* Ideally we want to use  some of the limits used in 
> +	 * clocksource_max_deferment, to provide a more informed
> +	 * MAX_UPDATE_LENGTH. But for now this just gets the
> +	 * regiseter interface working properly.
> +	 */
> +	clocks_calc_mult_shift(&cs->mult, &cs->shift, hz,
> +				      NSEC_PER_SEC, MAX_UPDATE_LENGTH);
> +	cs->max_idle_ns = clocksource_max_deferment(cs);
> +
> +	mutex_lock(&clocksource_mutex);
> +	clocksource_enqueue(cs);
> +	clocksource_select();
> +	clocksource_enqueue_watchdog(cs);
> +	mutex_unlock(&clocksource_mutex);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clocksource_register_hz);

  EXPORT_SYMBOL_GPL please

> +
> +/**
> + * clocksource_register - Used to install new clocksources
> + * @t:		clocksource to be registered
> + *
> + * Returns -EBUSY if registration fails, zero otherwise.
> + */
> +int clocksource_register_khz(struct clocksource *cs, u32 khz)
> +{

  shouldn't that be a simple inline wrapper which does

  return clocksource_register_hz(cs, khz * 1000);

Thanks,

	tglx

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[Martin Schwidefsky]

On Fri, 30 Apr 2010 18:36:25 -0700
John Stultz <johnstul@us.ibm.com> wrote:

> NOT FOR INCLUSION!
> NOT FOR INCLUSION!
> 
> I've already gone through and converted the rest of the clocksources 
> to use clocksource_register_hz, and I'll be hopefully pushing those 
> to arch maintainers for 2.6.36-2.6.37.
> 
> However, in going through all the clocksources, I hit a few
> non-trivial conversions and wanted to bring them up on the list
> early so they can be handled soon.
> 
> The following patch tries to convert the non-trival clocksources
> to use clocksource_register_hz/khz. It is likely broken. I will
> need arch maintainer help to figure out the best way to resovle 
> these clocksources.
> 
> This patch requires the previous "Add clocksource_register_hz/khz 
> interface" patch to build. 
> 
> So any help maintainers can provide in finding solutions to
> break the mult/shift assumptions in the arch code would be
> greatly appreciated!
> 
> thanks
> -john
> 
> NOT FOR INCLUSION!
> NOT FOR INCLUSION!

The s390 conversion to clocksource_register_khz looks good - at least
the kilo hertz value is correct. Bit 2^12 of the TOD value are
microseconds, which makes one millisecond (1000<<12). 

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[john stultz]

On Mon, 2010-05-10 at 17:53 +0200, Martin Schwidefsky wrote:
> On Fri, 30 Apr 2010 18:36:25 -0700
> John Stultz <johnstul@us.ibm.com> wrote:
> 
> > NOT FOR INCLUSION!
> > NOT FOR INCLUSION!
> > 
> > I've already gone through and converted the rest of the clocksources 
> > to use clocksource_register_hz, and I'll be hopefully pushing those 
> > to arch maintainers for 2.6.36-2.6.37.
> > 
> > However, in going through all the clocksources, I hit a few
> > non-trivial conversions and wanted to bring them up on the list
> > early so they can be handled soon.
> > 
> > The following patch tries to convert the non-trival clocksources
> > to use clocksource_register_hz/khz. It is likely broken. I will
> > need arch maintainer help to figure out the best way to resovle 
> > these clocksources.
> > 
> > This patch requires the previous "Add clocksource_register_hz/khz 
> > interface" patch to build. 
> > 
> > So any help maintainers can provide in finding solutions to
> > break the mult/shift assumptions in the arch code would be
> > greatly appreciated!
> > 
> > thanks
> > -john
> > 
> > NOT FOR INCLUSION!
> > NOT FOR INCLUSION!
> 
> The s390 conversion to clocksource_register_khz looks good - at least
> the kilo hertz value is correct. Bit 2^12 of the TOD value are
> microseconds, which makes one millisecond (1000<<12). 

Thanks for the review!

However, is there a more straightforward frequency value that we could
use? Is 4096000000 hz really the correct value or is there something
more precise?

thanks again!
-john

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[Jim Cromie]

On Fri, Apr 30, 2010 at 9:36 PM, John Stultz <johnstul@us.ibm.com> wrote:
> NOT FOR INCLUSION!
> NOT FOR INCLUSION!
>
> I've already gone through and converted the rest of the clocksources
> to use clocksource_register_hz, and I'll be hopefully pushing those
> to arch maintainers for 2.6.36-2.6.37.
>
> However, in going through all the clocksources, I hit a few
> non-trivial conversions and wanted to bring them up on the list
> early so they can be handled soon.
>
> The following patch tries to convert the non-trival clocksources
> to use clocksource_register_hz/khz. It is likely broken. I will
> need arch maintainer help to figure out the best way to resovle
> these clocksources.
>
> This patch requires the previous "Add clocksource_register_hz/khz
> interface" patch to build.
>
> So any help maintainers can provide in finding solutions to
> break the mult/shift assumptions in the arch code would be
> greatly appreciated!
>
> thanks
> -john
>
> NOT FOR INCLUSION!
> NOT FOR INCLUSION!
>  drivers/clocksource/scx200_hrt.c    |   19 ++++++-------------
>  drivers/clocksource/sh_cmt.c        |    4 ++--
>  drivers/clocksource/sh_tmu.c        |    4 ++--
>  kernel/time/jiffies.c               |    3 ++-
>  7 files changed, 35 insertions(+), 29 deletions(-)
>

> diff --git a/drivers/clocksource/scx200_hrt.c b/drivers/clocksource/scx200_hrt.c
> index 27f4d96..24f884c 100644
> --- a/drivers/clocksource/scx200_hrt.c
> +++ b/drivers/clocksource/scx200_hrt.c
> @@ -49,9 +49,6 @@ static cycle_t read_hrt(struct clocksource *cs)
>        return (cycle_t) inl(scx200_cb_base + SCx200_TIMER_OFFSET);
>  }
>
> -#define HRT_SHIFT_1    22
> -#define HRT_SHIFT_27   26
> -
>  static struct clocksource cs_hrt = {
>        .name           = "scx200_hrt",
>        .rating         = 250,
> @@ -63,6 +60,7 @@ static struct clocksource cs_hrt = {
>
>  static int __init init_hrt_clocksource(void)
>  {
> +       u32 freq;
>        /* Make sure scx200 has initialized the configuration block */
>        if (!scx200_cb_present())
>                return -ENODEV;
> @@ -79,19 +77,14 @@ static int __init init_hrt_clocksource(void)
>        outb(HR_TMEN | (mhz27 ? HR_TMCLKSEL : 0),
>             scx200_cb_base + SCx200_TMCNFG_OFFSET);
>
> -       if (mhz27) {
> -               cs_hrt.shift = HRT_SHIFT_27;
> -               cs_hrt.mult = clocksource_hz2mult((HRT_FREQ + ppm) * 27,
> -                                                 cs_hrt.shift);
> -       } else {
> -               cs_hrt.shift = HRT_SHIFT_1;
> -               cs_hrt.mult = clocksource_hz2mult(HRT_FREQ + ppm,
> -                                                 cs_hrt.shift);
> -       }
> +       freq = (HRT_FREQ + ppm);
> +       if (mhz27)
> +               freq *= 27;
> +
>        printk(KERN_INFO "enabling scx200 high-res timer (%s MHz +%d ppm)\n",
>                mhz27 ? "27":"1", ppm);
>
> -       return clocksource_register(&cs_hrt);
> +       return clocksource_register_hz(&cs_hrt, freq);
>  }
>
>  module_init(init_hrt_clocksource);


hi John,

On casual inspection, this looks good, however
I wont be able to try it on the hardware til early June earliest.

I have a few qs, (idle musings really)

- scx200_hrt cannot be rmmod'd, due iirc to the clocksource design.
This is not a problem, but I wonder if it might be added later, and what
it might mean to the drivers (in a new module_exit())

- HRT_SHIFT_1/_27 macros are removed, and now calculated in clocksource code.
I chose a 1:16 shift ratio for the 1:27 input clocks.  I suppose I could have
used 1:32, I dont recall thinking of it or trying it then.
What would/should your new code do ?

- when writing scx200_hrt, I chose to adjust by ppm, not +/- hz
partly cuz it was easier to describe in a 1-line mod-desc, and partly
cuz 1/27ppm vernier-knob is overkill on a $0.25 crystal.
The new API name (with _hz suffix) suggests that I could change
the mod-param-name, and add it in after the 1:27 multiplier.

<OT - out of scope in this thread>

on 5/3, slashdot had a synopsis of this item:
http://queue.acm.org/detail.cfm?id=1773943
It proposes a different split of duty between kernel and NTP daemon,
but oddly doesnt mention PTP.
Does this or PTP make any sense in Linux ?

thanks
-jimc

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[john stultz]

On Mon, 2010-05-10 at 17:33 -0400, Jim Cromie wrote:
> On Fri, Apr 30, 2010 at 9:36 PM, John Stultz <johnstul@us.ibm.com> wrote:
> hi John,
> 
> On casual inspection, this looks good, however
> I wont be able to try it on the hardware til early June earliest.
> 
> I have a few qs, (idle musings really)
> 
> - scx200_hrt cannot be rmmod'd, due iirc to the clocksource design.
> This is not a problem, but I wonder if it might be added later, and what
> it might mean to the drivers (in a new module_exit())


I've not really heard much of a compelling argument for clocksource
driver removal, nor has anyone wanted to put much effort there so far.
That said, I'm not opposed to it right off.  It just not a priority.


> - HRT_SHIFT_1/_27 macros are removed, and now calculated in clocksource code.
> I chose a 1:16 shift ratio for the 1:27 input clocks.  I suppose I could have
> used 1:32, I dont recall thinking of it or trying it then.
> What would/should your new code do ?

I'm not sure I'm following what you mean with the ratios, but the new
code will select the largest shift value possible, where the resulting
mult won't overflow 64bits when multiplied against 5 seconds (that
interval length may be changed in the future) worth of cycles.


> - when writing scx200_hrt, I chose to adjust by ppm, not +/- hz
> partly cuz it was easier to describe in a 1-line mod-desc, and partly
> cuz 1/27ppm vernier-knob is overkill on a $0.25 crystal.
> The new API name (with _hz suffix) suggests that I could change
> the mod-param-name, and add it in after the 1:27 multiplier.

I believe its still mathematically identical to what you had before, but
let me know if I'm mistaken here.

thanks
-john

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[john stultz]

On Mon, 2010-05-10 at 17:33 -0400, Jim Cromie wrote:
> <OT - out of scope in this thread>
> 
> on 5/3, slashdot had a synopsis of this item:
> http://queue.acm.org/detail.cfm?id=1773943
> It proposes a different split of duty between kernel and NTP daemon,
> but oddly doesnt mention PTP.

Yea, I emailed with some the RADclocks folks last year. They have done
quite a bit of very interesting work, but to my knowledge, they haven't
been working with upstream very much to push the patches.

Julien/Darryl: Sorry for dragging you out here, but I was curious if you
had any plans to push your patches to lkml in the near term? From the
patches in the tarball, it looks like you're structurally fairly clean
(and you're using git, so that's good!) so they're probably a good
starting point.

However, I do still have some concerns about the new interfaces that
expose the raw counter values. From your paper (its very nice btw,
congrats!) you mentioned CLOCK_MONOTONIC_RAW as a possible C_c(t) clock,
but from the patches it seems that you found it insufficient? It would
be really interesting to hear more about that, as well as your thoughts
about the dual-system in-kernel and out of kernel NTP adjustment loops. 

I'm curious if the RADclocks calculation of C_a(t) = Cd(t) - E(t) can
actually be done in the kernel, assuming E(t) is provided by userland.
Or is that missing something core to RADclocks design?

> Does this or PTP make any sense in Linux ?

Folks are working on using PTP with Linux. My understanding is that the
interfaces don't really change except for the packet timestamping.
Google up Patrick Ohly's work for some details (although I've not heard
too much on this recently, so I'm not sure how active it is).

thanks
-john

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[Jim Cromie]

On Mon, May 10, 2010 at 8:04 PM, john stultz <johnstul@us.ibm.com> wrote:
> On Mon, 2010-05-10 at 17:33 -0400, Jim Cromie wrote:
>> On Fri, Apr 30, 2010 at 9:36 PM, John Stultz <johnstul@us.ibm.com> wrote:
>> hi John,
>>
>> On casual inspection, this looks good, however
>> I wont be able to try it on the hardware til early June earliest.
>>
>> I have a few qs, (idle musings really)
>>
>> - scx200_hrt cannot be rmmod'd, due iirc to the clocksource design.
>> This is not a problem, but I wonder if it might be added later, and what
>> it might mean to the drivers (in a new module_exit())
>
>
> I've not really heard much of a compelling argument for clocksource
> driver removal, nor has anyone wanted to put much effort there so far.
> That said, I'm not opposed to it right off.  It just not a priority.
>

OK.  Just to be clear, Im not gonna make that argument.
I was imagining dragons there anyway.

>
>> - HRT_SHIFT_1/_27 macros are removed, and now calculated in clocksource code.
>> I chose a 1:16 shift ratio for the 1:27 input clocks.  I suppose I could have
>> used 1:32, I dont recall thinking of it or trying it then.
>> What would/should your new code do ?
>
> I'm not sure I'm following what you mean with the ratios, but the new
> code will select the largest shift value possible, where the resulting
> mult won't overflow 64bits when multiplied against 5 seconds (that
> interval length may be changed in the future) worth of cycles.
>

I hesitate to pursue this (musings), but:
the SHIFT_1, _27 values were 26,22, hence the 16:1 ratio.
the fastclock is 27* the slowclock, which is closer to 32:1 than 16:1,
so one of my SHIFT values is probably suboptimal.
Your new code will fix my error :-)
I'll probably add a printk just to see what shift, mult terms you compute.

>
>> - when writing scx200_hrt, I chose to adjust by ppm, not +/- hz
>> partly cuz it was easier to describe in a 1-line mod-desc, and partly
>> cuz 1/27ppm vernier-knob is overkill on a $0.25 crystal.
>> The new API name (with _hz suffix) suggests that I could change
>> the mod-param-name, and add it in after the 1:27 multiplier.
>
> I believe its still mathematically identical to what you had before, but
> let me know if I'm mistaken here.

Your changes look faithful to the original.
what I meant was changing the meaning of the param, and the computation:

+       freq = HRT_FREQ;
+       if (mhz27)
+               freq *= 27;
+       freq += hz_adjust;

This would give finer (apparent) control when using 27MHz clock,
though temp swings would still move real frequency much more
than the minimum adjustment (hence only an appearance of accuracy).

So, is this proposed parameter-name change an improvement, or needless churn ?
Obviously, I should ask this Q with a patch on top of yours :-}

>
> thanks
> -john
>

no, thank you
-jim

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[Martin Schwidefsky]

On Mon, 10 May 2010 12:19:10 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Mon, 2010-05-10 at 17:53 +0200, Martin Schwidefsky wrote:
> > On Fri, 30 Apr 2010 18:36:25 -0700 John Stultz <johnstul@us.ibm.com> wrote:
> > The s390 conversion to clocksource_register_khz looks good - at least
> > the kilo hertz value is correct. Bit 2^12 of the TOD value are
> > microseconds, which makes one millisecond (1000<<12). 
> 
> Thanks for the review!
> 
> However, is there a more straightforward frequency value that we could
> use? Is 4096000000 hz really the correct value or is there something
> more precise?

>From the principles of operation manual 4-37:
The TOD clock nominally is incremented by adding a one in bit position
51 every microsecond. In models having a higher or lower resolution, a
different bit position is incremented at such a frequency that the rate
of advancing the clock is the same as if a one were added in bit
position 51 every microsecond. The resolution of the TOD clock is such
that the incrementing rate is comparable to the instruction-execution
rate of the model.

Remember that the bit numbering in IBM manuals starts with 0 for the
most significant bit (bit 51 == 2^12 for a 64 bit value). The quality
of the TOD clock is very good, with ETR or STP it will be drifted to an
external time source with a precision in the range of microseconds
(you could think of it as NTP in hardware / microcode).
If there is a clocksource register function that takes a microsecond
value we could improve the precision.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[Julien Ridoux]

Hi John and all!

I put some comments inline.

On 10/05/2010, at 8:37 PM, john stultz wrote:

> On Mon, 2010-05-10 at 17:33 -0400, Jim Cromie wrote:
>> <OT - out of scope in this thread>
>> 
>> on 5/3, slashdot had a synopsis of this item:
>> http://queue.acm.org/detail.cfm?id=1773943
>> It proposes a different split of duty between kernel and NTP daemon,
>> but oddly doesnt mention PTP.

As a quick comment, IEEE 1588 essentially specifies the network protocol and does not say much about the synchronisation algorithm itself. Our work focuses mostly on the synchronisation algorithm. We should be able to adapt to any network protocol without too many problems (hopefully).

> Yea, I emailed with some the RADclocks folks last year. They have done
> quite a bit of very interesting work, but to my knowledge, they haven't
> been working with upstream very much to push the patches.
> 
> Julien/Darryl: Sorry for dragging you out here, but I was curious if you
> had any plans to push your patches to lkml in the near term? From the
> patches in the tarball, it looks like you're structurally fairly clean
> (and you're using git, so that's good!) so they're probably a good
> starting point.

We still really want to clean up our patches and push them upstream. We have been working at a fairly wide range of issues in the past months that have been keeping us really busy.

> However, I do still have some concerns about the new interfaces that
> expose the raw counter values. From your paper (its very nice btw,
> congrats!) you mentioned CLOCK_MONOTONIC_RAW as a possible C_c(t) clock,
> but from the patches it seems that you found it insufficient? It would
> be really interesting to hear more about that, as well as your thoughts
> about the dual-system in-kernel and out of kernel NTP adjustment loops. 

On the particular CLOCK_MONOTONIC_RAW issue, I believe we will be able to find a solution that makes use of it and avoid the need for extra interfaces. I haven't had the time to check all the possible consequences, but we will definitely bring possible issues up in the community.

We have however fairly strong feelings regarding the dual-system (kernel, NTPd adjustment loop) as you know. Some recent experiments with virtual system are giving us strong arguments to push our approach. Again, we hope to share this with the community soon.


> I'm curious if the RADclocks calculation of C_a(t) = Cd(t) - E(t) can
> actually be done in the kernel, assuming E(t) is provided by userland.
> Or is that missing something core to RADclocks design?

It can!  Our kernel patches do exactly that. The current mechanism could be improved for sure, but it is functional.

> 
>> Does this or PTP make any sense in Linux ?
> 
> Folks are working on using PTP with Linux. My understanding is that the
> interfaces don't really change except for the packet timestamping.
> Google up Patrick Ohly's work for some details (although I've not heard
> too much on this recently, so I'm not sure how active it is).

Completely agree and we hope the RADclock will be able to benefit from this low level timestamping.

I am not following the linux-kernel mailing list much at the moment. Please Cc: me on any comments or questions you guys may have.

Cheers,
Julien

> 
> thanks
> -john