[PATCH v2 1/4] Implement {read,update}_persistent_clock.

[PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Tony Breeds]

With these functions implemented we cooperate better with the generic
timekeeping code.  This obsoletes the need for the timer sysdev as a bonus.

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

---

Patch set updated to powerpc/for-2.6.24

* Compile tested for arch/powerpc/configs/*_defconfig
* Booted on pSeries, iSeries, Cell and PS3

 arch/powerpc/Kconfig         |    3 +
 arch/powerpc/kernel/time.c   |   85 ++++++++++-------------------------
 arch/powerpc/sysdev/Makefile |    5 --
 arch/powerpc/sysdev/timer.c  |   81 ---------------------------------
 4 files changed, 29 insertions(+), 145 deletions(-)

Index: working/arch/powerpc/Kconfig
===================================================================
--- working.orig/arch/powerpc/Kconfig
+++ working/arch/powerpc/Kconfig
@@ -21,6 +21,9 @@ config MMU
 	bool
 	default y
 
+config GENERIC_CMOS_UPDATE
+	def_bool y
+
 config GENERIC_HARDIRQS
 	bool
 	default y
Index: working/arch/powerpc/kernel/time.c
===================================================================
--- working.orig/arch/powerpc/kernel/time.c
+++ working/arch/powerpc/kernel/time.c
@@ -73,16 +73,11 @@
 #include <asm/iseries/hv_call_xm.h>
 #endif
 
-/* keep track of when we need to update the rtc */
-time_t last_rtc_update;
 #ifdef CONFIG_PPC_ISERIES
 static unsigned long __initdata iSeries_recal_titan;
 static signed long __initdata iSeries_recal_tb;
 #endif
 
-/* The decrementer counts down by 128 every 128ns on a 601. */
-#define DECREMENTER_COUNT_601	(1000000000 / HZ)
-
 #define XSEC_PER_SEC (1024*1024)
 
 #ifdef CONFIG_PPC64
@@ -348,39 +343,6 @@ void udelay(unsigned long usecs)
 }
 EXPORT_SYMBOL(udelay);
 
-static __inline__ void timer_check_rtc(void)
-{
-        /*
-         * update the rtc when needed, this should be performed on the
-         * right fraction of a second. Half or full second ?
-         * Full second works on mk48t59 clocks, others need testing.
-         * Note that this update is basically only used through 
-         * the adjtimex system calls. Setting the HW clock in
-         * any other way is a /dev/rtc and userland business.
-         * This is still wrong by -0.5/+1.5 jiffies because of the
-         * timer interrupt resolution and possible delay, but here we 
-         * hit a quantization limit which can only be solved by higher
-         * resolution timers and decoupling time management from timer
-         * interrupts. This is also wrong on the clocks
-         * which require being written at the half second boundary.
-         * We should have an rtc call that only sets the minutes and
-         * seconds like on Intel to avoid problems with non UTC clocks.
-         */
-        if (ppc_md.set_rtc_time && ntp_synced() &&
-	    xtime.tv_sec - last_rtc_update >= 659 &&
-	    abs((xtime.tv_nsec/1000) - (1000000-1000000/HZ)) < 500000/HZ) {
-		struct rtc_time tm;
-		to_tm(xtime.tv_sec + 1 + timezone_offset, &tm);
-		tm.tm_year -= 1900;
-		tm.tm_mon -= 1;
-		if (ppc_md.set_rtc_time(&tm) == 0)
-			last_rtc_update = xtime.tv_sec + 1;
-		else
-			/* Try again one minute later */
-			last_rtc_update += 60;
-        }
-}
-
 /*
  * This version of gettimeofday has microsecond resolution.
  */
@@ -689,7 +651,6 @@ void timer_interrupt(struct pt_regs * re
 			tb_last_jiffy = tb_next_jiffy;
 			do_timer(1);
 			timer_recalc_offset(tb_last_jiffy);
-			timer_check_rtc();
 		}
 		write_sequnlock(&xtime_lock);
 	}
@@ -801,11 +762,6 @@ int do_settimeofday(struct timespec *tv)
  	set_normalized_timespec(&xtime, new_sec, new_nsec);
 	set_normalized_timespec(&wall_to_monotonic, wtm_sec, wtm_nsec);
 
-	/* In case of a large backwards jump in time with NTP, we want the 
-	 * clock to be updated as soon as the PLL is again in lock.
-	 */
-	last_rtc_update = new_sec - 658;
-
 	ntp_clear();
 
 	new_xsec = xtime.tv_nsec;
@@ -881,12 +837,35 @@ void __init generic_calibrate_decr(void)
 #endif
 }
 
-unsigned long get_boot_time(void)
+int update_persistent_clock(struct timespec now)
 {
 	struct rtc_time tm;
 
-	if (ppc_md.get_boot_time)
-		return ppc_md.get_boot_time();
+	if (!ppc_md.set_rtc_time)
+		return 0;
+
+	to_tm(now.tv_sec + 1 + timezone_offset, &tm);
+	tm.tm_year -= 1900;
+	tm.tm_mon -= 1;
+
+	return ppc_md.set_rtc_time(&tm);
+}
+
+unsigned long read_persistent_clock(void)
+{
+	struct rtc_time tm;
+	static int first = 1;
+
+	/* XXX this is a litle fragile but will work okay in the short term */
+	if (first) {
+		first = 0;
+		if (ppc_md.time_init)
+			timezone_offset = ppc_md.time_init();
+
+		/* get_boot_time() isn't guaranteed to be safe to call late */
+		if (ppc_md.get_boot_time)
+			return ppc_md.get_boot_time() -timezone_offset;
+	}
 	if (!ppc_md.get_rtc_time)
 		return 0;
 	ppc_md.get_rtc_time(&tm);
@@ -898,14 +877,10 @@ unsigned long get_boot_time(void)
 void __init time_init(void)
 {
 	unsigned long flags;
-	unsigned long tm = 0;
 	struct div_result res;
 	u64 scale, x;
 	unsigned shift;
 
-        if (ppc_md.time_init != NULL)
-                timezone_offset = ppc_md.time_init();
-
 	if (__USE_RTC()) {
 		/* 601 processor: dec counts down by 128 every 128ns */
 		ppc_tb_freq = 1000000000;
@@ -980,19 +955,14 @@ void __init time_init(void)
 	/* Save the current timebase to pretty up CONFIG_PRINTK_TIME */
 	boot_tb = get_tb_or_rtc();
 
-	tm = get_boot_time();
-
 	write_seqlock_irqsave(&xtime_lock, flags);
 
 	/* If platform provided a timezone (pmac), we correct the time */
         if (timezone_offset) {
 		sys_tz.tz_minuteswest = -timezone_offset / 60;
 		sys_tz.tz_dsttime = 0;
-		tm -= timezone_offset;
         }
 
-	xtime.tv_sec = tm;
-	xtime.tv_nsec = 0;
 	do_gtod.varp = &do_gtod.vars[0];
 	do_gtod.var_idx = 0;
 	do_gtod.varp->tb_orig_stamp = tb_last_jiffy;
@@ -1010,9 +980,6 @@ void __init time_init(void)
 
 	time_freq = 0;
 
-	last_rtc_update = xtime.tv_sec;
-	set_normalized_timespec(&wall_to_monotonic,
-	                        -xtime.tv_sec, -xtime.tv_nsec);
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
 	/* Not exact, but the timer interrupt takes care of this */
Index: working/arch/powerpc/sysdev/Makefile
===================================================================
--- working.orig/arch/powerpc/sysdev/Makefile
+++ working/arch/powerpc/sysdev/Makefile
@@ -20,11 +20,6 @@ obj-$(CONFIG_MV64X60)		+= $(mv64x60-y) m
 obj-$(CONFIG_RTC_DRV_CMOS)	+= rtc_cmos_setup.o
 obj-$(CONFIG_AXON_RAM)		+= axonram.o
 
-# contains only the suspend handler for time
-ifeq ($(CONFIG_RTC_CLASS),)
-obj-$(CONFIG_PM)		+= timer.o
-endif
-
 ifeq ($(CONFIG_PPC_MERGE),y)
 obj-$(CONFIG_PPC_INDIRECT_PCI)	+= indirect_pci.o
 obj-$(CONFIG_PPC_I8259)		+= i8259.o
Index: working/arch/powerpc/sysdev/timer.c
===================================================================
--- working.orig/arch/powerpc/sysdev/timer.c
+++ /dev/null
@@ -1,81 +0,0 @@
-/*
- * Common code to keep time when machine suspends.
- *
- * Copyright 2007	Johannes Berg <johannes@sipsolutions.net>
- *
- * GPLv2
- */
-
-#include <linux/time.h>
-#include <linux/sysdev.h>
-#include <asm/rtc.h>
-
-static unsigned long suspend_rtc_time;
-
-/*
- * Reset the time after a sleep.
- */
-static int timer_resume(struct sys_device *dev)
-{
-	struct timeval tv;
-	struct timespec ts;
-	struct rtc_time cur_rtc_tm;
-	unsigned long cur_rtc_time, diff;
-
-	/* get current RTC time and convert to seconds */
-	get_rtc_time(&cur_rtc_tm);
-	cur_rtc_time = mktime(cur_rtc_tm.tm_year + 1900,
-			      cur_rtc_tm.tm_mon + 1,
-			      cur_rtc_tm.tm_mday,
-			      cur_rtc_tm.tm_hour,
-			      cur_rtc_tm.tm_min,
-			      cur_rtc_tm.tm_sec);
-
-	diff = cur_rtc_time - suspend_rtc_time;
-
-	/* adjust time of day by seconds that elapsed while
-	 * we were suspended */
-	do_gettimeofday(&tv);
-	ts.tv_sec = tv.tv_sec + diff;
-	ts.tv_nsec = tv.tv_usec * NSEC_PER_USEC;
-	do_settimeofday(&ts);
-
-	return 0;
-}
-
-static int timer_suspend(struct sys_device *dev, pm_message_t state)
-{
-	struct rtc_time suspend_rtc_tm;
-	WARN_ON(!ppc_md.get_rtc_time);
-
-	get_rtc_time(&suspend_rtc_tm);
-	suspend_rtc_time = mktime(suspend_rtc_tm.tm_year + 1900,
-				  suspend_rtc_tm.tm_mon + 1,
-				  suspend_rtc_tm.tm_mday,
-				  suspend_rtc_tm.tm_hour,
-				  suspend_rtc_tm.tm_min,
-				  suspend_rtc_tm.tm_sec);
-
-	return 0;
-}
-
-static struct sysdev_class timer_sysclass = {
-	.resume = timer_resume,
-	.suspend = timer_suspend,
-	set_kset_name("timer"),
-};
-
-static struct sys_device device_timer = {
-	.id = 0,
-	.cls = &timer_sysclass,
-};
-
-static int time_init_device(void)
-{
-	int error = sysdev_class_register(&timer_sysclass);
-	if (!error)
-		error = sysdev_register(&device_timer);
-	return error;
-}
-
-device_initcall(time_init_device);

[PATCH v2 2/4] Implement generic time of day clocksource for powerpc machines.

[Tony Breeds]

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

---

Updated to create a clocksource driver specific to the 601

 arch/powerpc/Kconfig       |    6 
 arch/powerpc/kernel/time.c |  255 ++++++++++++-------------------------
 2 files changed, 91 insertions(+), 170 deletions(-)

Index: working/arch/powerpc/Kconfig
===================================================================
--- working.orig/arch/powerpc/Kconfig
+++ working/arch/powerpc/Kconfig
@@ -24,6 +24,12 @@ config MMU
 config GENERIC_CMOS_UPDATE
 	def_bool y
 
+config GENERIC_TIME
+	def_bool y
+
+config GENERIC_TIME_VSYSCALL
+	def_bool y
+
 config GENERIC_HARDIRQS
 	bool
 	default y
Index: working/arch/powerpc/kernel/time.c
===================================================================
--- working.orig/arch/powerpc/kernel/time.c
+++ working/arch/powerpc/kernel/time.c
@@ -73,9 +73,38 @@
 #include <asm/iseries/hv_call_xm.h>
 #endif
 
+/* powerpc clocksource/clockevent code */
+
+#include <linux/clocksource.h>
+
+static cycle_t rtc_read(void);
+static struct clocksource clocksource_rtc = {
+	.name         = "rtc",
+	.rating       = 400,
+	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
+	.mask         = CLOCKSOURCE_MASK(64),
+	.shift        = 22,
+	.mult         = 0,	/* To be filled in */
+	.read         = rtc_read,
+};
+
+static cycle_t timebase_read(void);
+static struct clocksource clocksource_timebase = {
+	.name         = "timebase",
+	.rating       = 400,
+	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
+	.mask         = CLOCKSOURCE_MASK(64),
+	.shift        = 22,
+	.mult         = 0,	/* To be filled in */
+	.read         = timebase_read,
+};
+
 #ifdef CONFIG_PPC_ISERIES
 static unsigned long __initdata iSeries_recal_titan;
 static signed long __initdata iSeries_recal_tb;
+
+/* Forward declaration is only needed for iSereis compiles */
+void __init clocksource_init(void);
 #endif
 
 #define XSEC_PER_SEC (1024*1024)
@@ -343,65 +372,6 @@ void udelay(unsigned long usecs)
 }
 EXPORT_SYMBOL(udelay);
 
-/*
- * This version of gettimeofday has microsecond resolution.
- */
-static inline void __do_gettimeofday(struct timeval *tv)
-{
-	unsigned long sec, usec;
-	u64 tb_ticks, xsec;
-	struct gettimeofday_vars *temp_varp;
-	u64 temp_tb_to_xs, temp_stamp_xsec;
-
-	/*
-	 * These calculations are faster (gets rid of divides)
-	 * if done in units of 1/2^20 rather than microseconds.
-	 * The conversion to microseconds at the end is done
-	 * without a divide (and in fact, without a multiply)
-	 */
-	temp_varp = do_gtod.varp;
-
-	/* Sampling the time base must be done after loading
-	 * do_gtod.varp in order to avoid racing with update_gtod.
-	 */
-	data_barrier(temp_varp);
-	tb_ticks = get_tb() - temp_varp->tb_orig_stamp;
-	temp_tb_to_xs = temp_varp->tb_to_xs;
-	temp_stamp_xsec = temp_varp->stamp_xsec;
-	xsec = temp_stamp_xsec + mulhdu(tb_ticks, temp_tb_to_xs);
-	sec = xsec / XSEC_PER_SEC;
-	usec = (unsigned long)xsec & (XSEC_PER_SEC - 1);
-	usec = SCALE_XSEC(usec, 1000000);
-
-	tv->tv_sec = sec;
-	tv->tv_usec = usec;
-}
-
-void do_gettimeofday(struct timeval *tv)
-{
-	if (__USE_RTC()) {
-		/* do this the old way */
-		unsigned long flags, seq;
-		unsigned int sec, nsec, usec;
-
-		do {
-			seq = read_seqbegin_irqsave(&xtime_lock, flags);
-			sec = xtime.tv_sec;
-			nsec = xtime.tv_nsec + tb_ticks_since(tb_last_jiffy);
-		} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
-		usec = nsec / 1000;
-		while (usec >= 1000000) {
-			usec -= 1000000;
-			++sec;
-		}
-		tv->tv_sec = sec;
-		tv->tv_usec = usec;
-		return;
-	}
-	__do_gettimeofday(tv);
-}
-
-EXPORT_SYMBOL(do_gettimeofday);
 
 /*
  * There are two copies of tb_to_xs and stamp_xsec so that no
@@ -447,56 +417,6 @@ static inline void update_gtod(u64 new_t
 	++(vdso_data->tb_update_count);
 }
 
-/*
- * When the timebase - tb_orig_stamp gets too big, we do a manipulation
- * between tb_orig_stamp and stamp_xsec. The goal here is to keep the
- * difference tb - tb_orig_stamp small enough to always fit inside a
- * 32 bits number. This is a requirement of our fast 32 bits userland
- * implementation in the vdso. If we "miss" a call to this function
- * (interrupt latency, CPU locked in a spinlock, ...) and we end up
- * with a too big difference, then the vdso will fallback to calling
- * the syscall
- */
-static __inline__ void timer_recalc_offset(u64 cur_tb)
-{
-	unsigned long offset;
-	u64 new_stamp_xsec;
-	u64 tlen, t2x;
-	u64 tb, xsec_old, xsec_new;
-	struct gettimeofday_vars *varp;
-
-	if (__USE_RTC())
-		return;
-	tlen = current_tick_length();
-	offset = cur_tb - do_gtod.varp->tb_orig_stamp;
-	if (tlen == last_tick_len && offset < 0x80000000u)
-		return;
-	if (tlen != last_tick_len) {
-		t2x = mulhdu(tlen << TICKLEN_SHIFT, ticklen_to_xs);
-		last_tick_len = tlen;
-	} else
-		t2x = do_gtod.varp->tb_to_xs;
-	new_stamp_xsec = (u64) xtime.tv_nsec * XSEC_PER_SEC;
-	do_div(new_stamp_xsec, 1000000000);
-	new_stamp_xsec += (u64) xtime.tv_sec * XSEC_PER_SEC;
-
-	++vdso_data->tb_update_count;
-	smp_mb();
-
-	/*
-	 * Make sure time doesn't go backwards for userspace gettimeofday.
-	 */
-	tb = get_tb();
-	varp = do_gtod.varp;
-	xsec_old = mulhdu(tb - varp->tb_orig_stamp, varp->tb_to_xs)
-		+ varp->stamp_xsec;
-	xsec_new = mulhdu(tb - cur_tb, t2x) + new_stamp_xsec;
-	if (xsec_new < xsec_old)
-		new_stamp_xsec += xsec_old - xsec_new;
-
-	update_gtod(cur_tb, new_stamp_xsec, t2x);
-}
-
 #ifdef CONFIG_SMP
 unsigned long profile_pc(struct pt_regs *regs)
 {
@@ -568,6 +488,8 @@ static int __init iSeries_tb_recal(void)
 	iSeries_recal_titan = titan;
 	iSeries_recal_tb = tb;
 
+	/* Called here as now we know accurate values for the timebase */
+	clocksource_init();
 	return 0;
 }
 late_initcall(iSeries_tb_recal);
@@ -650,7 +572,6 @@ void timer_interrupt(struct pt_regs * re
 		if (per_cpu(last_jiffy, cpu) >= tb_next_jiffy) {
 			tb_last_jiffy = tb_next_jiffy;
 			do_timer(1);
-			timer_recalc_offset(tb_last_jiffy);
 		}
 		write_sequnlock(&xtime_lock);
 	}
@@ -722,66 +643,6 @@ unsigned long long sched_clock(void)
 	return mulhdu(get_tb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
 }
 
-int do_settimeofday(struct timespec *tv)
-{
-	time_t wtm_sec, new_sec = tv->tv_sec;
-	long wtm_nsec, new_nsec = tv->tv_nsec;
-	unsigned long flags;
-	u64 new_xsec;
-	unsigned long tb_delta;
-
-	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
-		return -EINVAL;
-
-	write_seqlock_irqsave(&xtime_lock, flags);
-
-	/*
-	 * Updating the RTC is not the job of this code. If the time is
-	 * stepped under NTP, the RTC will be updated after STA_UNSYNC
-	 * is cleared.  Tools like clock/hwclock either copy the RTC
-	 * to the system time, in which case there is no point in writing
-	 * to the RTC again, or write to the RTC but then they don't call
-	 * settimeofday to perform this operation.
-	 */
-
-	/* Make userspace gettimeofday spin until we're done. */
-	++vdso_data->tb_update_count;
-	smp_mb();
-
-	/*
-	 * Subtract off the number of nanoseconds since the
-	 * beginning of the last tick.
-	 */
-	tb_delta = tb_ticks_since(tb_last_jiffy);
-	tb_delta = mulhdu(tb_delta, do_gtod.varp->tb_to_xs); /* in xsec */
-	new_nsec -= SCALE_XSEC(tb_delta, 1000000000);
-
-	wtm_sec  = wall_to_monotonic.tv_sec + (xtime.tv_sec - new_sec);
-	wtm_nsec = wall_to_monotonic.tv_nsec + (xtime.tv_nsec - new_nsec);
-
- 	set_normalized_timespec(&xtime, new_sec, new_nsec);
-	set_normalized_timespec(&wall_to_monotonic, wtm_sec, wtm_nsec);
-
-	ntp_clear();
-
-	new_xsec = xtime.tv_nsec;
-	if (new_xsec != 0) {
-		new_xsec *= XSEC_PER_SEC;
-		do_div(new_xsec, NSEC_PER_SEC);
-	}
-	new_xsec += (u64)xtime.tv_sec * XSEC_PER_SEC;
-	update_gtod(tb_last_jiffy, new_xsec, do_gtod.varp->tb_to_xs);
-
-	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
-	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
-
-	write_sequnlock_irqrestore(&xtime_lock, flags);
-	clock_was_set();
-	return 0;
-}
-
-EXPORT_SYMBOL(do_settimeofday);
-
 static int __init get_freq(char *name, int cells, unsigned long *val)
 {
 	struct device_node *cpu;
@@ -873,6 +734,69 @@ unsigned long read_persistent_clock(void
 		      tm.tm_hour, tm.tm_min, tm.tm_sec);
 }
 
+/* clocksource code */
+static cycle_t rtc_read(void)
+{
+	return (cycle_t)get_rtc();
+}
+
+static cycle_t timebase_read(void)
+{
+	return (cycle_t)get_tb();
+}
+
+void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
+{
+	u64 t2x, stamp_xsec;
+
+	if (__USE_RTC() || clock != &clocksource_timebase)
+		return;
+
+	/* Make userspace gettimeofday spin until we're done. */
+	++vdso_data->tb_update_count;
+	smp_mb();
+
+	/* XXX this assumes clock->shift == 22 */
+	/* 4611686018 ~= 2^(20+64-22) / 1e9 */
+	t2x = (u64) clock->mult * 4611686018ULL;
+	stamp_xsec = (u64) xtime.tv_nsec * XSEC_PER_SEC;
+	do_div(stamp_xsec, 1000000000);
+	stamp_xsec += (u64) xtime.tv_sec * XSEC_PER_SEC;
+	update_gtod(clock->cycle_last, stamp_xsec, t2x);
+}
+
+void update_vsyscall_tz(void)
+{
+	/* Make userspace gettimeofday spin until we're done. */
+	++vdso_data->tb_update_count;
+	smp_mb();
+	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
+	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
+	smp_mb();
+	++vdso_data->tb_update_count;
+}
+
+void __init clocksource_init(void)
+{
+	struct clocksource *clock;
+
+	if (__USE_RTC())
+		clock = &clocksource_rtc;
+	else
+		clock = &clocksource_timebase;
+
+	clock->mult = clocksource_hz2mult(tb_ticks_per_sec, clock->shift);
+
+	if (clocksource_register(clock)) {
+		printk(KERN_ERR "clocksource: %s is already registered\n",
+		       clock->name);
+		return;
+	}
+
+	printk(KERN_INFO "clocksource: %s mult[%x] shift[%d] registered\n",
+	       clock->name, clock->mult, clock->shift);
+}
+
 /* This function is only called on the boot processor */
 void __init time_init(void)
 {
@@ -982,6 +906,12 @@ void __init time_init(void)
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
+	/* Register the clocksource, if we're not running on iSeries */
+#ifdef CONFIG_ISERIES
+	if (!firmware_has_feature(FW_FEATURE_ISERIES))
+#endif
+		clocksource_init();
+
 	/* Not exact, but the timer interrupt takes care of this */
 	set_dec(tb_ticks_per_jiffy);
 }

Re: [PATCH v2 2/4] Implement generic time of day clocksource for powerpc machines.

[Daniel Walker]

On Fri, 2007-09-21 at 13:26 +1000, Tony Breeds wrote:
> +
> +       if (__USE_RTC())
> +               clock = &clocksource_rtc;
> +       else
> +               clock = &clocksource_timebase;
> +
> +       clock->mult = clocksource_hz2mult(tb_ticks_per_sec,
> clock->shift);

I don't think the RTC frequency isn't the same as the timebase? Seems
like the RTC only case about seconds at the lowest level. If that's the
case then the jiffies clock might be better to use .. The other thing I
wonder is if the __USE_RTC boards might have lower level clocks that
could be used instead ..

Daniel

Re: [PATCH v2 2/4] Implement generic time of day clocksource for powerpc machines.

[Paul Mackerras]

Daniel Walker writes:

> I don't think the RTC frequency isn't the same as the timebase? Seems
> like the RTC only case about seconds at the lowest level. If that's the
> case then the jiffies clock might be better to use .. The other thing I
> wonder is if the __USE_RTC boards might have lower level clocks that
> could be used instead ..

It's OK, the RTC isn't what you think it is, it's a pair of
CPU-internal registers which count seconds and nanoseconds.  On
processors with the RTC, tb_ticks_per_sec is initialized to
1000000000.  Trust me, this code is OK. :)

Paul.

Re: [PATCH v2 2/4] Implement generic time of day clocksource for powerpc machines.

[David Gibson]

On Fri, Sep 21, 2007 at 02:59:31PM +1000, Paul Mackerras wrote:
> Daniel Walker writes:
> 
> > I don't think the RTC frequency isn't the same as the timebase? Seems
> > like the RTC only case about seconds at the lowest level. If that's the
> > case then the jiffies clock might be better to use .. The other thing I
> > wonder is if the __USE_RTC boards might have lower level clocks that
> > could be used instead ..
> 
> It's OK, the RTC isn't what you think it is, it's a pair of
> CPU-internal registers which count seconds and nanoseconds.  On
> processors with the RTC, tb_ticks_per_sec is initialized to
> 1000000000.  Trust me, this code is OK. :)

Indeed..  I'm wondering if we should do a s/rtc/ppc601rtc/ or
something.  Otherwise people will keep making this mistake and be
confused.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH v2 2/4] Implement generic time of day clocksource for powerpc machines.

[Stephen Rothwell]

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

Small comments.

On Fri, 21 Sep 2007 13:26:02 +1000 Tony Breeds <tony@bakeyournoodle.com> wrote:
>
> +void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
> +{
> +	u64 t2x, stamp_xsec;
> +
> +	if (__USE_RTC() || clock != &clocksource_timebase)
            ^^^^^^^^^^^
I think this is redundant as if __USE_RTC() is true, you register
clocksource_rtc below.

> +#ifdef CONFIG_ISERIES
> +	if (!firmware_has_feature(FW_FEATURE_ISERIES))
> +#endif

The #ifdef is redundant since if CONFIG_ISERIES is not set,
firmware_has_feature(FW_FEATURE_ISERIES) is constant 0.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH v2 2/4] Implement generic time of day clocksource for powerpc machines.

[Tony Breeds]

On Fri, Sep 21, 2007 at 02:52:12PM +1000, Stephen Rothwell wrote:
> Small comments.

Thanks.

> I think this is redundant as if __USE_RTC() is true, you register
> clocksource_rtc below.

Yup you're right.

> The #ifdef is redundant since if CONFIG_ISERIES is not set,
> firmware_has_feature(FW_FEATURE_ISERIES) is constant 0.

Ahh okay.  I got too fancy for my own good.

patch comming.

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH v3 2/4] Implement generic time of day clocksource for powerpc machines.

[Tony Breeds]

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

---

Updated to create a clocksource driver specific to the 601
Updated to address feedback from sfr
Only compile tested.

 arch/powerpc/Kconfig       |    6 
 arch/powerpc/kernel/time.c |  255 ++++++++++++-------------------------
 2 files changed, 91 insertions(+), 170 deletions(-)

Index: working/arch/powerpc/Kconfig
===================================================================
--- working.orig/arch/powerpc/Kconfig
+++ working/arch/powerpc/Kconfig
@@ -24,6 +24,12 @@ config MMU
 config GENERIC_CMOS_UPDATE
 	def_bool y
 
+config GENERIC_TIME
+	def_bool y
+
+config GENERIC_TIME_VSYSCALL
+	def_bool y
+
 config GENERIC_HARDIRQS
 	bool
 	default y
Index: working/arch/powerpc/kernel/time.c
===================================================================
--- working.orig/arch/powerpc/kernel/time.c
+++ working/arch/powerpc/kernel/time.c
@@ -73,9 +73,38 @@
 #include <asm/iseries/hv_call_xm.h>
 #endif
 
+/* powerpc clocksource/clockevent code */
+
+#include <linux/clocksource.h>
+
+static cycle_t rtc_read(void);
+static struct clocksource clocksource_rtc = {
+	.name         = "rtc",
+	.rating       = 400,
+	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
+	.mask         = CLOCKSOURCE_MASK(64),
+	.shift        = 22,
+	.mult         = 0,	/* To be filled in */
+	.read         = rtc_read,
+};
+
+static cycle_t timebase_read(void);
+static struct clocksource clocksource_timebase = {
+	.name         = "timebase",
+	.rating       = 400,
+	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
+	.mask         = CLOCKSOURCE_MASK(64),
+	.shift        = 22,
+	.mult         = 0,	/* To be filled in */
+	.read         = timebase_read,
+};
+
 #ifdef CONFIG_PPC_ISERIES
 static unsigned long __initdata iSeries_recal_titan;
 static signed long __initdata iSeries_recal_tb;
+
+/* Forward declaration is only needed for iSereis compiles */
+void __init clocksource_init(void);
 #endif
 
 #define XSEC_PER_SEC (1024*1024)
@@ -343,65 +372,6 @@ void udelay(unsigned long usecs)
 }
 EXPORT_SYMBOL(udelay);
 
-/*
- * This version of gettimeofday has microsecond resolution.
- */
-static inline void __do_gettimeofday(struct timeval *tv)
-{
-	unsigned long sec, usec;
-	u64 tb_ticks, xsec;
-	struct gettimeofday_vars *temp_varp;
-	u64 temp_tb_to_xs, temp_stamp_xsec;
-
-	/*
-	 * These calculations are faster (gets rid of divides)
-	 * if done in units of 1/2^20 rather than microseconds.
-	 * The conversion to microseconds at the end is done
-	 * without a divide (and in fact, without a multiply)
-	 */
-	temp_varp = do_gtod.varp;
-
-	/* Sampling the time base must be done after loading
-	 * do_gtod.varp in order to avoid racing with update_gtod.
-	 */
-	data_barrier(temp_varp);
-	tb_ticks = get_tb() - temp_varp->tb_orig_stamp;
-	temp_tb_to_xs = temp_varp->tb_to_xs;
-	temp_stamp_xsec = temp_varp->stamp_xsec;
-	xsec = temp_stamp_xsec + mulhdu(tb_ticks, temp_tb_to_xs);
-	sec = xsec / XSEC_PER_SEC;
-	usec = (unsigned long)xsec & (XSEC_PER_SEC - 1);
-	usec = SCALE_XSEC(usec, 1000000);
-
-	tv->tv_sec = sec;
-	tv->tv_usec = usec;
-}
-
-void do_gettimeofday(struct timeval *tv)
-{
-	if (__USE_RTC()) {
-		/* do this the old way */
-		unsigned long flags, seq;
-		unsigned int sec, nsec, usec;
-
-		do {
-			seq = read_seqbegin_irqsave(&xtime_lock, flags);
-			sec = xtime.tv_sec;
-			nsec = xtime.tv_nsec + tb_ticks_since(tb_last_jiffy);
-		} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
-		usec = nsec / 1000;
-		while (usec >= 1000000) {
-			usec -= 1000000;
-			++sec;
-		}
-		tv->tv_sec = sec;
-		tv->tv_usec = usec;
-		return;
-	}
-	__do_gettimeofday(tv);
-}
-
-EXPORT_SYMBOL(do_gettimeofday);
 
 /*
  * There are two copies of tb_to_xs and stamp_xsec so that no
@@ -447,56 +417,6 @@ static inline void update_gtod(u64 new_t
 	++(vdso_data->tb_update_count);
 }
 
-/*
- * When the timebase - tb_orig_stamp gets too big, we do a manipulation
- * between tb_orig_stamp and stamp_xsec. The goal here is to keep the
- * difference tb - tb_orig_stamp small enough to always fit inside a
- * 32 bits number. This is a requirement of our fast 32 bits userland
- * implementation in the vdso. If we "miss" a call to this function
- * (interrupt latency, CPU locked in a spinlock, ...) and we end up
- * with a too big difference, then the vdso will fallback to calling
- * the syscall
- */
-static __inline__ void timer_recalc_offset(u64 cur_tb)
-{
-	unsigned long offset;
-	u64 new_stamp_xsec;
-	u64 tlen, t2x;
-	u64 tb, xsec_old, xsec_new;
-	struct gettimeofday_vars *varp;
-
-	if (__USE_RTC())
-		return;
-	tlen = current_tick_length();
-	offset = cur_tb - do_gtod.varp->tb_orig_stamp;
-	if (tlen == last_tick_len && offset < 0x80000000u)
-		return;
-	if (tlen != last_tick_len) {
-		t2x = mulhdu(tlen << TICKLEN_SHIFT, ticklen_to_xs);
-		last_tick_len = tlen;
-	} else
-		t2x = do_gtod.varp->tb_to_xs;
-	new_stamp_xsec = (u64) xtime.tv_nsec * XSEC_PER_SEC;
-	do_div(new_stamp_xsec, 1000000000);
-	new_stamp_xsec += (u64) xtime.tv_sec * XSEC_PER_SEC;
-
-	++vdso_data->tb_update_count;
-	smp_mb();
-
-	/*
-	 * Make sure time doesn't go backwards for userspace gettimeofday.
-	 */
-	tb = get_tb();
-	varp = do_gtod.varp;
-	xsec_old = mulhdu(tb - varp->tb_orig_stamp, varp->tb_to_xs)
-		+ varp->stamp_xsec;
-	xsec_new = mulhdu(tb - cur_tb, t2x) + new_stamp_xsec;
-	if (xsec_new < xsec_old)
-		new_stamp_xsec += xsec_old - xsec_new;
-
-	update_gtod(cur_tb, new_stamp_xsec, t2x);
-}
-
 #ifdef CONFIG_SMP
 unsigned long profile_pc(struct pt_regs *regs)
 {
@@ -568,6 +488,8 @@ static int __init iSeries_tb_recal(void)
 	iSeries_recal_titan = titan;
 	iSeries_recal_tb = tb;
 
+	/* Called here as now we know accurate values for the timebase */
+	clocksource_init();
 	return 0;
 }
 late_initcall(iSeries_tb_recal);
@@ -650,7 +572,6 @@ void timer_interrupt(struct pt_regs * re
 		if (per_cpu(last_jiffy, cpu) >= tb_next_jiffy) {
 			tb_last_jiffy = tb_next_jiffy;
 			do_timer(1);
-			timer_recalc_offset(tb_last_jiffy);
 		}
 		write_sequnlock(&xtime_lock);
 	}
@@ -722,66 +643,6 @@ unsigned long long sched_clock(void)
 	return mulhdu(get_tb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
 }
 
-int do_settimeofday(struct timespec *tv)
-{
-	time_t wtm_sec, new_sec = tv->tv_sec;
-	long wtm_nsec, new_nsec = tv->tv_nsec;
-	unsigned long flags;
-	u64 new_xsec;
-	unsigned long tb_delta;
-
-	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
-		return -EINVAL;
-
-	write_seqlock_irqsave(&xtime_lock, flags);
-
-	/*
-	 * Updating the RTC is not the job of this code. If the time is
-	 * stepped under NTP, the RTC will be updated after STA_UNSYNC
-	 * is cleared.  Tools like clock/hwclock either copy the RTC
-	 * to the system time, in which case there is no point in writing
-	 * to the RTC again, or write to the RTC but then they don't call
-	 * settimeofday to perform this operation.
-	 */
-
-	/* Make userspace gettimeofday spin until we're done. */
-	++vdso_data->tb_update_count;
-	smp_mb();
-
-	/*
-	 * Subtract off the number of nanoseconds since the
-	 * beginning of the last tick.
-	 */
-	tb_delta = tb_ticks_since(tb_last_jiffy);
-	tb_delta = mulhdu(tb_delta, do_gtod.varp->tb_to_xs); /* in xsec */
-	new_nsec -= SCALE_XSEC(tb_delta, 1000000000);
-
-	wtm_sec  = wall_to_monotonic.tv_sec + (xtime.tv_sec - new_sec);
-	wtm_nsec = wall_to_monotonic.tv_nsec + (xtime.tv_nsec - new_nsec);
-
- 	set_normalized_timespec(&xtime, new_sec, new_nsec);
-	set_normalized_timespec(&wall_to_monotonic, wtm_sec, wtm_nsec);
-
-	ntp_clear();
-
-	new_xsec = xtime.tv_nsec;
-	if (new_xsec != 0) {
-		new_xsec *= XSEC_PER_SEC;
-		do_div(new_xsec, NSEC_PER_SEC);
-	}
-	new_xsec += (u64)xtime.tv_sec * XSEC_PER_SEC;
-	update_gtod(tb_last_jiffy, new_xsec, do_gtod.varp->tb_to_xs);
-
-	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
-	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
-
-	write_sequnlock_irqrestore(&xtime_lock, flags);
-	clock_was_set();
-	return 0;
-}
-
-EXPORT_SYMBOL(do_settimeofday);
-
 static int __init get_freq(char *name, int cells, unsigned long *val)
 {
 	struct device_node *cpu;
@@ -873,6 +734,69 @@ unsigned long read_persistent_clock(void
 		      tm.tm_hour, tm.tm_min, tm.tm_sec);
 }
 
+/* clocksource code */
+static cycle_t rtc_read(void)
+{
+	return (cycle_t)get_rtc();
+}
+
+static cycle_t timebase_read(void)
+{
+	return (cycle_t)get_tb();
+}
+
+void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
+{
+	u64 t2x, stamp_xsec;
+
+	if (clock != &clocksource_timebase)
+		return;
+
+	/* Make userspace gettimeofday spin until we're done. */
+	++vdso_data->tb_update_count;
+	smp_mb();
+
+	/* XXX this assumes clock->shift == 22 */
+	/* 4611686018 ~= 2^(20+64-22) / 1e9 */
+	t2x = (u64) clock->mult * 4611686018ULL;
+	stamp_xsec = (u64) xtime.tv_nsec * XSEC_PER_SEC;
+	do_div(stamp_xsec, 1000000000);
+	stamp_xsec += (u64) xtime.tv_sec * XSEC_PER_SEC;
+	update_gtod(clock->cycle_last, stamp_xsec, t2x);
+}
+
+void update_vsyscall_tz(void)
+{
+	/* Make userspace gettimeofday spin until we're done. */
+	++vdso_data->tb_update_count;
+	smp_mb();
+	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
+	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
+	smp_mb();
+	++vdso_data->tb_update_count;
+}
+
+void __init clocksource_init(void)
+{
+	struct clocksource *clock;
+
+	if (__USE_RTC())
+		clock = &clocksource_rtc;
+	else
+		clock = &clocksource_timebase;
+
+	clock->mult = clocksource_hz2mult(tb_ticks_per_sec, clock->shift);
+
+	if (clocksource_register(clock)) {
+		printk(KERN_ERR "clocksource: %s is already registered\n",
+		       clock->name);
+		return;
+	}
+
+	printk(KERN_INFO "clocksource: %s mult[%x] shift[%d] registered\n",
+	       clock->name, clock->mult, clock->shift);
+}
+
 /* This function is only called on the boot processor */
 void __init time_init(void)
 {
@@ -982,6 +906,10 @@ void __init time_init(void)
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
+	/* Register the clocksource, if we're not running on iSeries */
+	if (!firmware_has_feature(FW_FEATURE_ISERIES))
+		clocksource_init();
+
 	/* Not exact, but the timer interrupt takes care of this */
 	set_dec(tb_ticks_per_jiffy);
 }

Re: [PATCH v3 2/4] Implement generic time of day clocksource for powerpc machines.

[Paul Mackerras]

Tony Breeds writes:

> @@ -982,6 +906,10 @@ void __init time_init(void)
>  
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
>  
> +	/* Register the clocksource, if we're not running on iSeries */
> +	if (!firmware_has_feature(FW_FEATURE_ISERIES))
> +		clocksource_init();

This breaks the 32-bit compile.

Is it possible to adjust the frequency of a clocksource after it has
been registered?  Or could the timebase clocksource be unregistered
and reregistered in iSeries_tb_recal?

Paul.

Re: [PATCH v3 2/4] Implement generic time of day clocksource for powerpc machines.

[Thomas Gleixner]

On Wed, 3 Oct 2007, Paul Mackerras wrote:

> Tony Breeds writes:
> 
> > @@ -982,6 +906,10 @@ void __init time_init(void)
> >  
> >  	write_sequnlock_irqrestore(&xtime_lock, flags);
> >  
> > +	/* Register the clocksource, if we're not running on iSeries */
> > +	if (!firmware_has_feature(FW_FEATURE_ISERIES))
> > +		clocksource_init();
> 
> This breaks the 32-bit compile.
> 
> Is it possible to adjust the frequency of a clocksource after it has
> been registered?  Or could the timebase clocksource be unregistered
> and reregistered in iSeries_tb_recal?

There is no unregister interface right now. But we need some mechanism to 
tell the core code that you changed it. I have a look if John does not 
beat me.

	tglx

[PATCH v2 4/4] Enable tickless idle and high res timers for powerpc

[Tony Breeds]

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

---

Updated to remove the need for patching generic code to include hrtimer.h

 arch/powerpc/Kconfig                   |    1 +
 arch/powerpc/kernel/idle.c             |    3 +++
 arch/powerpc/platforms/iseries/setup.c |    5 +++++
 3 files changed, 9 insertions(+)

Index: working/arch/powerpc/Kconfig
===================================================================
--- working.orig/arch/powerpc/Kconfig
+++ working/arch/powerpc/Kconfig
@@ -168,6 +168,7 @@ config HIGHMEM
 	bool "High memory support"
 	depends on PPC32
 
+source kernel/time/Kconfig
 source kernel/Kconfig.hz
 source kernel/Kconfig.preempt
 source "fs/Kconfig.binfmt"
Index: working/arch/powerpc/kernel/idle.c
===================================================================
--- working.orig/arch/powerpc/kernel/idle.c
+++ working/arch/powerpc/kernel/idle.c
@@ -24,6 +24,7 @@
 #include <linux/smp.h>
 #include <linux/cpu.h>
 #include <linux/sysctl.h>
+#include <linux/tick.h>
 
 #include <asm/system.h>
 #include <asm/processor.h>
@@ -59,6 +60,7 @@ void cpu_idle(void)
 
 	set_thread_flag(TIF_POLLING_NRFLAG);
 	while (1) {
+		tick_nohz_stop_sched_tick();
 		while (!need_resched() && !cpu_should_die()) {
 			ppc64_runlatch_off();
 
@@ -90,6 +92,7 @@ void cpu_idle(void)
 
 		HMT_medium();
 		ppc64_runlatch_on();
+		tick_nohz_restart_sched_tick();
 		if (cpu_should_die())
 			cpu_die();
 		preempt_enable_no_resched();
Index: working/arch/powerpc/platforms/iseries/setup.c
===================================================================
--- working.orig/arch/powerpc/platforms/iseries/setup.c
+++ working/arch/powerpc/platforms/iseries/setup.c
@@ -26,6 +26,8 @@
 #include <linux/major.h>
 #include <linux/root_dev.h>
 #include <linux/kernel.h>
+#include <linux/hrtimer.h>
+#include <linux/tick.h>
 
 #include <asm/processor.h>
 #include <asm/machdep.h>
@@ -561,6 +563,7 @@ static void yield_shared_processor(void)
 static void iseries_shared_idle(void)
 {
 	while (1) {
+		tick_nohz_stop_sched_tick();
 		while (!need_resched() && !hvlpevent_is_pending()) {
 			local_irq_disable();
 			ppc64_runlatch_off();
@@ -574,6 +577,7 @@ static void iseries_shared_idle(void)
 		}
 
 		ppc64_runlatch_on();
+		tick_nohz_restart_sched_tick();
 
 		if (hvlpevent_is_pending())
 			process_iSeries_events();
@@ -589,6 +593,7 @@ static void iseries_dedicated_idle(void)
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
 	while (1) {
+		tick_nohz_stop_sched_tick();
 		if (!need_resched()) {
 			while (!need_resched()) {
 				ppc64_runlatch_off();
@@ -605,6 +610,7 @@ static void iseries_dedicated_idle(void)
 		}
 
 		ppc64_runlatch_on();
+		tick_nohz_restart_sched_tick();
 		preempt_enable_no_resched();
 		schedule();
 		preempt_disable();

[PATCH v2 3/4] Implement clockevents driver for powerpc

[Tony Breeds]

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

---
Updated to remove a trivial FIXME left behind

 arch/powerpc/Kconfig       |    3 
 arch/powerpc/kernel/smp.c  |    3 
 arch/powerpc/kernel/time.c |  134 +++++++++++++++++++++++++------------
 include/asm-powerpc/time.h |    1 
 4 files changed, 98 insertions(+), 43 deletions(-)

Index: working/arch/powerpc/Kconfig
===================================================================
--- working.orig/arch/powerpc/Kconfig
+++ working/arch/powerpc/Kconfig
@@ -30,6 +30,9 @@ config GENERIC_TIME
 config GENERIC_TIME_VSYSCALL
 	def_bool y
 
+config GENERIC_CLOCKEVENTS
+	def_bool y
+
 config GENERIC_HARDIRQS
 	bool
 	default y
Index: working/arch/powerpc/kernel/smp.c
===================================================================
--- working.orig/arch/powerpc/kernel/smp.c
+++ working/arch/powerpc/kernel/smp.c
@@ -560,6 +560,8 @@ int __devinit start_secondary(void *unus
 	if (system_state > SYSTEM_BOOTING)
 		snapshot_timebase();
 
+	secondary_cpu_time_init();
+
 	spin_lock(&call_lock);
 	cpu_set(cpu, cpu_online_map);
 	spin_unlock(&call_lock);
Index: working/arch/powerpc/kernel/time.c
===================================================================
--- working.orig/arch/powerpc/kernel/time.c
+++ working/arch/powerpc/kernel/time.c
@@ -75,6 +75,7 @@
 
 /* powerpc clocksource/clockevent code */
 
+#include <linux/clockchips.h>
 #include <linux/clocksource.h>
 
 static cycle_t rtc_read(void);
@@ -99,6 +100,27 @@ static struct clocksource clocksource_ti
 	.read         = timebase_read,
 };
 
+#define DECREMENTER_MAX	0x7fffffff
+
+static int decrementer_set_next_event(unsigned long evt,
+				      struct clock_event_device *dev);
+static void decrementer_set_mode(enum clock_event_mode mode,
+				 struct clock_event_device *dev);
+
+static struct clock_event_device decrementer_clockevent = {
+       .name           = "decrementer",
+       .rating         = 200,
+       .shift          = 32,
+       .mult           = 0,	/* To be filled in */
+       .irq            = -1,
+       .set_next_event = decrementer_set_next_event,
+       .set_mode       = decrementer_set_mode,
+       .features       = CLOCK_EVT_FEAT_ONESHOT,
+};
+
+static DEFINE_PER_CPU(struct clock_event_device, decrementers);
+void init_decrementer_clockevent(void);
+
 #ifdef CONFIG_PPC_ISERIES
 static unsigned long __initdata iSeries_recal_titan;
 static signed long __initdata iSeries_recal_tb;
@@ -519,10 +541,12 @@ void __init iSeries_time_init_early(void
 void timer_interrupt(struct pt_regs * regs)
 {
 	struct pt_regs *old_regs;
-	int next_dec;
 	int cpu = smp_processor_id();
-	unsigned long ticks;
-	u64 tb_next_jiffy;
+	struct clock_event_device *evt = &per_cpu(decrementers, cpu);
+
+	/* Ensure a positive value is written to the decrementer, or else
+	 * some CPUs will continuue to take decrementer exceptions */
+	set_dec(DECREMENTER_MAX);
 
 #ifdef CONFIG_PPC32
 	if (atomic_read(&ppc_n_lost_interrupts) != 0)
@@ -532,7 +556,6 @@ void timer_interrupt(struct pt_regs * re
 	old_regs = set_irq_regs(regs);
 	irq_enter();
 
-	profile_tick(CPU_PROFILING);
 	calculate_steal_time();
 
 #ifdef CONFIG_PPC_ISERIES
@@ -540,44 +563,20 @@ void timer_interrupt(struct pt_regs * re
 		get_lppaca()->int_dword.fields.decr_int = 0;
 #endif
 
-	while ((ticks = tb_ticks_since(per_cpu(last_jiffy, cpu)))
-	       >= tb_ticks_per_jiffy) {
-		/* Update last_jiffy */
-		per_cpu(last_jiffy, cpu) += tb_ticks_per_jiffy;
-		/* Handle RTCL overflow on 601 */
-		if (__USE_RTC() && per_cpu(last_jiffy, cpu) >= 1000000000)
-			per_cpu(last_jiffy, cpu) -= 1000000000;
-
-		/*
-		 * We cannot disable the decrementer, so in the period
-		 * between this cpu's being marked offline in cpu_online_map
-		 * and calling stop-self, it is taking timer interrupts.
-		 * Avoid calling into the scheduler rebalancing code if this
-		 * is the case.
-		 */
-		if (!cpu_is_offline(cpu))
-			account_process_time(regs);
-
-		/*
-		 * No need to check whether cpu is offline here; boot_cpuid
-		 * should have been fixed up by now.
-		 */
-		if (cpu != boot_cpuid)
-			continue;
+	/*
+	 * We cannot disable the decrementer, so in the period
+	 * between this cpu's being marked offline in cpu_online_map
+	 * and calling stop-self, it is taking timer interrupts.
+	 * Avoid calling into the scheduler rebalancing code if this
+	 * is the case.
+	 */
+	if (!cpu_is_offline(cpu))
+		account_process_time(regs);
 
-		write_seqlock(&xtime_lock);
-		tb_next_jiffy = tb_last_jiffy + tb_ticks_per_jiffy;
-		if (__USE_RTC() && tb_next_jiffy >= 1000000000)
-			tb_next_jiffy -= 1000000000;
-		if (per_cpu(last_jiffy, cpu) >= tb_next_jiffy) {
-			tb_last_jiffy = tb_next_jiffy;
-			do_timer(1);
-		}
-		write_sequnlock(&xtime_lock);
-	}
-	
-	next_dec = tb_ticks_per_jiffy - ticks;
-	set_dec(next_dec);
+	if (evt->event_handler)
+		evt->event_handler(evt);
+	else
+		evt->set_next_event(DECREMENTER_MAX, evt);
 
 #ifdef CONFIG_PPC_ISERIES
 	if (firmware_has_feature(FW_FEATURE_ISERIES) && hvlpevent_is_pending())
@@ -797,6 +796,53 @@ void __init clocksource_init(void)
 	       clock->name, clock->mult, clock->shift);
 }
 
+static int decrementer_set_next_event(unsigned long evt,
+				      struct clock_event_device *dev)
+{
+	set_dec(evt);
+	return 0;
+}
+
+static void decrementer_set_mode(enum clock_event_mode mode,
+				 struct clock_event_device *dev)
+{
+	if (mode != CLOCK_EVT_MODE_ONESHOT)
+		decrementer_set_next_event(DECREMENTER_MAX, dev);
+}
+
+static void register_decrementer_clockevent(int cpu)
+{
+	struct clock_event_device *dec = &per_cpu(decrementers, cpu);
+
+	*dec = decrementer_clockevent;
+	dec->cpumask = cpumask_of_cpu(cpu);
+
+	printk(KERN_ERR "clockevent: %s mult[%lx] shift[%d] cpu[%d]\n",
+	       dec->name, dec->mult, dec->shift, cpu);
+
+	clockevents_register_device(dec);
+}
+
+void init_decrementer_clockevent(void)
+{
+	int cpu = smp_processor_id();
+
+	decrementer_clockevent.mult = div_sc(ppc_tb_freq, NSEC_PER_SEC,
+					     decrementer_clockevent.shift);
+	decrementer_clockevent.max_delta_ns =
+		clockevent_delta2ns(DECREMENTER_MAX, &decrementer_clockevent);
+	decrementer_clockevent.min_delta_ns = 1000;
+
+	register_decrementer_clockevent(cpu);
+}
+
+void secondary_cpu_time_init(void)
+{
+	/* FIME: Should make unrelatred change to move snapshot_timebase
+	 * call here ! */
+	register_decrementer_clockevent(smp_processor_id());
+}
+
 /* This function is only called on the boot processor */
 void __init time_init(void)
 {
@@ -912,8 +958,7 @@ void __init time_init(void)
 #endif
 		clocksource_init();
 
-	/* Not exact, but the timer interrupt takes care of this */
-	set_dec(tb_ticks_per_jiffy);
+	init_decrementer_clockevent();
 }
 
 
Index: working/include/asm-powerpc/time.h
===================================================================
--- working.orig/include/asm-powerpc/time.h
+++ working/include/asm-powerpc/time.h
@@ -245,6 +245,7 @@ extern void snapshot_timebases(void);
 #define snapshot_timebases()			do { } while (0)
 #endif
 
+extern void secondary_cpu_time_init(void);
 extern void iSeries_time_init_early(void);
 
 #endif /* __KERNEL__ */

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello.

Tony Breeds wrote:

> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

    I don't see my own signoff or at least a reference to my prior work in
this patch or even at -rt patch -- despite this code ha clearly borrowed from
it.  And I'm not sure why this crippled version (lacking 40x/ Book E specific
clockevents implementation) is preferred over mine, unless this implementation
was only aimed at PPC64 machines...

> Index: working/arch/powerpc/Kconfig
> ===================================================================
> --- working.orig/arch/powerpc/Kconfig
> +++ working/arch/powerpc/Kconfig
> @@ -30,6 +30,9 @@ config GENERIC_TIME
>  config GENERIC_TIME_VSYSCALL
>  	def_bool y
>
> +config GENERIC_CLOCKEVENTS
> +	def_bool y
> +
>  config GENERIC_HARDIRQS
>  	bool
>  	default y

    Also, have the deterministic CPU accounting incompatibility with
clockevents been dealt with?

> Index: working/arch/powerpc/kernel/time.c
> ===================================================================
> --- working.orig/arch/powerpc/kernel/time.c
> +++ working/arch/powerpc/kernel/time.c
[...]
> @@ -519,10 +541,12 @@ void __init iSeries_time_init_early(void
>  void timer_interrupt(struct pt_regs * regs)
>  {
>  	struct pt_regs *old_regs;
> -	int next_dec;
>  	int cpu = smp_processor_id();
> -	unsigned long ticks;
> -	u64 tb_next_jiffy;
> +	struct clock_event_device *evt = &per_cpu(decrementers, cpu);
> +
> +	/* Ensure a positive value is written to the decrementer, or else
> +	 * some CPUs will continuue to take decrementer exceptions */
> +	set_dec(DECREMENTER_MAX);

    BookE and 40x CPUs don't need this.

>  #ifdef CONFIG_PPC32
>  	if (atomic_read(&ppc_n_lost_interrupts) != 0)
> @@ -532,7 +556,6 @@ void timer_interrupt(struct pt_regs * re
>  	old_regs = set_irq_regs(regs);
>  	irq_enter();
>  
> -	profile_tick(CPU_PROFILING);
>  	calculate_steal_time();
>  
>  #ifdef CONFIG_PPC_ISERIES
> @@ -540,44 +563,20 @@ void timer_interrupt(struct pt_regs * re
>  		get_lppaca()->int_dword.fields.decr_int = 0;
>  #endif
>  
> -	while ((ticks = tb_ticks_since(per_cpu(last_jiffy, cpu)))
> -	       >= tb_ticks_per_jiffy) {
> -		/* Update last_jiffy */
> -		per_cpu(last_jiffy, cpu) += tb_ticks_per_jiffy;
> -		/* Handle RTCL overflow on 601 */
> -		if (__USE_RTC() && per_cpu(last_jiffy, cpu) >= 1000000000)
> -			per_cpu(last_jiffy, cpu) -= 1000000000;

    I don't see where the patch removes those variables themselves...

[...]

> -		write_seqlock(&xtime_lock);
> -		tb_next_jiffy = tb_last_jiffy + tb_ticks_per_jiffy;
> -		if (__USE_RTC() && tb_next_jiffy >= 1000000000)
> -			tb_next_jiffy -= 1000000000;
> -		if (per_cpu(last_jiffy, cpu) >= tb_next_jiffy) {
> -			tb_last_jiffy = tb_next_jiffy;
> -			do_timer(1);
> -		}
> -		write_sequnlock(&xtime_lock);
> -	}

    Again, where those variables are removed?

> -	
> -	next_dec = tb_ticks_per_jiffy - ticks;
> -	set_dec(next_dec);
> +	if (evt->event_handler)
> +		evt->event_handler(evt);
> +	else
> +		evt->set_next_event(DECREMENTER_MAX, evt);

    We have already set it to DECREMENTER_MAX at the start of the function.
Please remove the 'else' part...

> @@ -797,6 +796,53 @@ void __init clocksource_init(void)
>  	       clock->name, clock->mult, clock->shift);
>  }
>  
> +static int decrementer_set_next_event(unsigned long evt,
> +				      struct clock_event_device *dev)
> +{
> +	set_dec(evt);

    I'd use (evt - 1) since the interrupt gets generated at 0xffffffff count, 
not 0 (on classic CPUs).  With you removing of the code that compensated for 
the errors, they will accumulate.  And no, this wouldn't be enough anyway, 
since on 40x and Book E you'll need to set it for evt anyway, since the 
interrupt happens at 0 count...
    NAK the patch.  And I really don't understand why you're throwing alway 
already tested/working code...

> +	return 0;
> +}
> +
> +static void decrementer_set_mode(enum clock_event_mode mode,
> +				 struct clock_event_device *dev)
> +{
> +	if (mode != CLOCK_EVT_MODE_ONESHOT)
> +		decrementer_set_next_event(DECREMENTER_MAX, dev);
> +}
> +
> +static void register_decrementer_clockevent(int cpu)
> +{
> +	struct clock_event_device *dec = &per_cpu(decrementers, cpu);
> +
> +	*dec = decrementer_clockevent;
> +	dec->cpumask = cpumask_of_cpu(cpu);
> +
> +	printk(KERN_ERR "clockevent: %s mult[%lx] shift[%d] cpu[%d]\n",

    This is a mistake indeed. :-P

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello, I wrote:

>>@@ -797,6 +796,53 @@ void __init clocksource_init(void)
>> 	       clock->name, clock->mult, clock->shift);
>> }

>>+static int decrementer_set_next_event(unsigned long evt,
>>+				      struct clock_event_device *dev)
>>+{
>>+	set_dec(evt);

>     I'd use (evt - 1) since the interrupt gets generated at 0xffffffff count, 
> not 0 (on classic CPUs).  With you removing of the code that compensated for 
> the errors, they will accumulate.  And no, this wouldn't be enough anyway, 
> since on 40x and Book E you'll need to set it for evt anyway, since the 
> interrupt happens at 0 count...

    What that means is that the off-by-one-clock drift is introduced for 
classic CPUs (not 40x or Book E which interrupt at 0).  And this has dealt 
with months ago in the clockevent driver in the -rt patch.  So much for my 
efforts...

>     NAK the patch.

    It's too late for NAKs -- I should've given these patches more attention 
(yet I was in hospital for 1.5 months).

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

>     I don't see my own signoff or at least a reference to my prior work in
> this patch or even at -rt patch -- despite this code ha clearly borrowed from
> it.  And I'm not sure why this crippled version (lacking 40x/ Book E specific
> clockevents implementation) is preferred over mine, unless this implementation
> was only aimed at PPC64 machines...

Tony started from an earlier patch by John Stultz, not from your
patches.

The main reason your patches were rejected were that you completely
broke the VDSO and the deterministic time accounting, and made no
attempt to fix them.

As for 40x/Book E, the main thing there is the auto-reload.  However,
since the generic core can use a oneshot clock event source to
generate periodic ticks, there is no advantage to using the
auto-reload.

>     Also, have the deterministic CPU accounting incompatibility with
> clockevents been dealt with?

The S390 guys looked at that and solved it with Ingo's and Thomas's
help (although I did see something on linux-kernel about some further
problems).

>     I'd use (evt - 1) since the interrupt gets generated at 0xffffffff count, 
> not 0 (on classic CPUs).  With you removing of the code that compensated for 

See commit d968014b7280e2c447b20363e576999040ac72ef; I already fixed
that.

> the errors, they will accumulate.  And no, this wouldn't be enough anyway, 

No, they don't accumulate.  See tick_setup_periodic().  The interval
until the next tick is determined using ktime_get() rather than just
being a constant.

> since on 40x and Book E you'll need to set it for evt anyway, since the 
> interrupt happens at 0 count...
>     NAK the patch.  And I really don't understand why you're throwing alway 
> already tested/working code...

Because you broke important features and because you seem to have some
fundamental misconceptions (e.g. the comment about errors accumulating
you just made).

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

>>    I don't see my own signoff or at least a reference to my prior work in
>>this patch or even at -rt patch -- despite this code ha clearly borrowed from
>>it.  And I'm not sure why this crippled version (lacking 40x/ Book E specific
>>clockevents implementation) is preferred over mine, unless this implementation
>>was only aimed at PPC64 machines...

> Tony started from an earlier patch by John Stultz, not from your
> patches.

    Well, that I can believe, yet the clockevents patch has traces of my
former work, and looking at read_persisitent_time() it looks suspiciously 
close to my version too...

> The main reason your patches were rejected were that you completely
> broke the VDSO and the deterministic time accounting, and made no

    That's just not true!
    They didn't broke vDSO (to be precise it was John's patch that broke it), 
they just removed the vDSO code known to already be broken by -rt patch for 
several months by then.  And they didn't broke determinictic accounting -- 
they just made two things mutually exclusive.  I haven't yet seen how the 
patches that were preferred dealt with it at all.

> attempt to fix them.

    I lacked time to do this, so did what I could.

> As for 40x/Book E, the main thing there is the auto-reload.  However,
> since the generic core can use a oneshot clock event source to
> generate periodic ticks, there is no advantage to using the
> auto-reload.

    Really? IMO, the harware does keep a constant interrupt rate better than 
software.

>>    Also, have the deterministic CPU accounting incompatibility with
>>clockevents been dealt with?

> The S390 guys looked at that and solved it with Ingo's and Thomas's
> help (although I did see something on linux-kernel about some further
> problems).

    Good to hear.  Nobody offered any help to me (except maybe Thomas -- but we
never came to any viable approach).

>>    I'd use (evt - 1) since the interrupt gets generated at 0xffffffff count, 
>>not 0 (on classic CPUs).  With you removing of the code that compensated for 

> See commit d968014b7280e2c447b20363e576999040ac72ef; I already fixed
> that.

    Good to hear this has been dealt with, along with that PowerMAC "IRQ 
simulation" nuisance -- I didn't care about it, sorry. :-)

>>the errors, they will accumulate.  And no, this wouldn't be enough anyway, 

> No, they don't accumulate.  See tick_setup_periodic().  The interval
> until the next tick is determined using ktime_get() rather than just
> being a constant.

    The interrupt always happens 1 clock later depsite what value
have been selected for the decrementer.  Well, OK, that should still get 
compensated by the tick_setup_periodic() by selecting shorter next period.
    Well, I'm taking comfot in that you've finally had to sutract 1. :-)

>>since on 40x and Book E you'll need to set it for evt anyway, since the 
>>interrupt happens at 0 count...
>>    NAK the patch.  And I really don't understand why you're throwing alway 
>>already tested/working code...

> Because you broke important features

    That is *not true*.
    And nobody had interest to fix them for months (quite strange if they're 
so important) while I had neither time nor interest to deal with them anymore 
having written the code that *did work*, and not only for me.

> and because you seem to have some

> fundamental misconceptions  (e.g. the comment about errors accumulating

    Where's the misconception in the patch? ;-)

> you just made).

    I see, I'm a bad guy... but still not convinced. :-)

> Paul.

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

> > Tony started from an earlier patch by John Stultz, not from your
> > patches.
> 
>     Well, that I can believe, yet the clockevents patch has traces of my
> former work, and looking at read_persisitent_time() it looks suspiciously 
> close to my version too...

There is basically only one reasonably way to do a lot of this stuff.

> > The main reason your patches were rejected were that you completely
> > broke the VDSO and the deterministic time accounting, and made no
> 
>     That's just not true!
>     They didn't broke vDSO (to be precise it was John's patch that broke it), 
> they just removed the vDSO code known to already be broken by -rt patch for 
> several months by then.  And they didn't broke determinictic accounting -- 
> they just made two things mutually exclusive.  I haven't yet seen how the 
> patches that were preferred dealt with it at all.

OK.  My requirement was that the clocksource/clockevent stuff and the
VDSO were both functional.  Your patch didn't meet that requirement.

>     Really? IMO, the harware does keep a constant interrupt rate better than 
> software.

Well, if you have actual numbers to back that up, show them to us.
I don't believe you would be able to measure any difference, and so I
prefer the simplicity of only implementing the one-shot mode.

> > Because you broke important features
> 
>     That is *not true*.
>     And nobody had interest to fix them for months (quite strange if they're 
> so important) while I had neither time nor interest to deal with them anymore 
> having written the code that *did work*, and not only for me.

Well, this is the difference between having a hack that works for you,
and having something that can go upstream into mainline.

Anyway, this discussion doesn't seem to be going anywhere.  If there
are changes you want made, or any other specific concrete action you
want anyone to do, say so.  Otherwise stop whinging.

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

>>>Tony started from an earlier patch by John Stultz, not from your
>>>patches.

>>    Well, that I can believe, yet the clockevents patch has traces of my
>>former work, and looking at read_persisitent_time() it looks suspiciously 
>>close to my version too...

> There is basically only one reasonably way to do a lot of this stuff.

>>>The main reason your patches were rejected were that you completely
>>>broke the VDSO and the deterministic time accounting, and made no

>>    That's just not true!
>>    They didn't broke vDSO (to be precise it was John's patch that broke it), 
>>they just removed the vDSO code known to already be broken by -rt patch for 
>>several months by then.  And they didn't broke determinictic accounting -- 
>>they just made two things mutually exclusive.  I haven't yet seen how the 
>>patches that were preferred dealt with it at all.

> OK.  My requirement was that the clocksource/clockevent stuff and the
> VDSO were both functional.  Your patch didn't meet that requirement.

    Which of my patches? There were many, and only one of them dealing with 
vDSO. That's reasonable to drop that patch but it's not reasonable to drop the 
other ones, not directly connected to vDSO issue.  One flaw doesn't make the 
whole patchset bad.
    And now you have incomplete read_persistent_clock() implementation for 
example, god knows why it was preferred to mine -- well, it also implemented 
update_persistent_clock() bit those functions haven't appeared at the same 
time, so read_persistent_clock() was written by me in the .

>>    Really? IMO, the harware does keep a constant interrupt rate better than 
>>software.

> Well, if you have actual numbers to back that up, show them to us.
> I don't believe you would be able to measure any difference, and so I
> prefer the simplicity of only implementing the one-shot mode.

    Well, that's up to you.  I take it you wouldn't accept a patch 
implementing auto-reload mode?

>>>Because you broke important features

>>    That is *not true*.
>>    And nobody had interest to fix them for months (quite strange if they're 
>>so important) while I had neither time nor interest to deal with them anymore 
>>having written the code that *did work*, and not only for me.

> Well, this is the difference between having a hack that works for you,

    Agreed, -rt is a patchset full of hacks. :-)

> and having something that can go upstream into mainline.

    It *went* upstream. Mainline wasn't my aim at that time.

> Anyway, this discussion doesn't seem to be going anywhere.  If there
> are changes you want made, or any other specific concrete action you

    There are. I'll have to send patches (it's not that I have time for this) 
but this is surely the fastest way to get things fixed (if I don't get ignored 
that is).

> want anyone to do, say so.  Otherwise stop whinging.

    I just wanted the reasons clarified and got what I wanted -- as I thought, 
the decision behind preferring patches was somewhat biased, nobody really 
cared about code quality or just wasn't familiar with hrtimers enough to judge 
on the code quality...

> Paul.

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

>     And now you have incomplete read_persistent_clock() implementation for 

I don't see anything incomplete about it.  If you do, feel free to
post a patch.

> example, god knows why it was preferred to mine -- well, it also implemented 

Your most recent post of your patch to implement read_persistent_clock
was in May -- five months ago -- and you said this about it: "This
patch hasn't received a good testing though".

You don't have to be a god to figure out why I preferred a patch that
had been tested, where the author was responding to comments and
posting updated versions of his patch in the period leading up to the
merge window, over that.

>     Well, that's up to you.  I take it you wouldn't accept a patch 
> implementing auto-reload mode?

I already told you.  Show me numbers (real measurements showing that
it's better) and I'll consider it.

>     There are. I'll have to send patches (it's not that I have time for this) 
> but this is surely the fastest way to get things fixed (if I don't get ignored 
> that is).

All of us only get stuff in by spending the time to develop patches
and posting them for comment.  Stop whinging.

>     I just wanted the reasons clarified and got what I wanted -- as I thought, 
> the decision behind preferring patches was somewhat biased, nobody really 
> cared about code quality or just wasn't familiar with hrtimers enough to judge 
> on the code quality...

You really know how to persuade people to cooperate, don't you...  :P

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Paul Mackerras wrote:
> Sergei Shtylyov writes:

>>    And now you have incomplete read_persistent_clock() implementation for 

> I don't see anything incomplete about it.  If you do, feel free to
> post a patch.

    The xtime_lock is still grabbed by time_init()

>>example, god knows why it was preferred to mine -- well, it also implemented 

> Your most recent post of your patch to implement read_persistent_clock
> was in May -- five months ago -- and you said this about it: "This

    Right, the most recent was in May but that was only a recast of the
October version (i.e. year old) -- that patch got somehow dropped from later
the -rt patches IIRC.

> patch hasn't received a good testing though".

    Right, it never has been tested on macines with RTC. That was a fair 
warning. :-)

> You don't have to be a god to figure out why I preferred a patch that
> had been tested, where the author was responding to comments and
> posting updated versions of his patch in the period leading up to the
> merge window, over that.

    Unfortunately, I didn't have time to try pusing it into every -rc1 since 
2.6.18 -- there has been experimental hrtimers patchset at that time with even 
x86 stuff being unmerged to mainline, so the stuff could only be pushed into 
that patchset last autumn. I was going to try addressing vDSO stuff, yet there 
has been too much work aside of that.  Still, I've answered the mails. :-)

>>    I just wanted the reasons clarified and got what I wanted -- as I thought, 
>>the decision behind preferring patches was somewhat biased, nobody really 
>>cared about code quality or just wasn't familiar with hrtimers enough to judge 
>>on the code quality...

> You really know how to persuade people to cooperate, don't you...  :P

    Well, I'm not persuading anybody, sine I don't believe that I can persuade 
somebody to do my work, so had to just vainly complain :-).
    However, I agree that my complaints/ comments might have been somewhat 
rash and unjust -- Tony's patches are *not* that bad after all. :-)
    The only thing I'm still unusre about is that deterministic accounting. 
Could you point me at the patch which deals with this (at least for System 390 
:-)?

> Paul.

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

>     The xtime_lock is still grabbed by time_init()

That was left in there because we are setting sys_tz and do_gtod, and
do_gtod at least is only updated with the xtime_lock held.  Of course,
at that early stage in the boot process, no lock is really needed, but
xtime_lock was taken for consistency with other code.

In fact there's quite a lot of stuff in there that could be removed
now.  I also want to make the vdso use an algorithm more like what
getnstimeofday does.

>     The only thing I'm still unusre about is that deterministic accounting. 
> Could you point me at the patch which deals with this (at least for System 390 

See efe567fc8281661524ffa75477a7c4ca9b466c63 in Linus' tree, and look
for posts to lkml by Christian Borntraeger, who has been pursuing the
issue (subject "Re: [stable] 2.6.23 regression: top displaying 9999%
CPU usage").

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

>>    The xtime_lock is still grabbed by time_init()

    Oops, I got distracted and hadn't finish the passage.
    My patch got rid of this xtime_lock stuff -- but this was in a different
context, with all vDSO initialization code in between being killed by John's
patch.  I'm not sure it still has sense to hold this lock in time_init()
around that initialization...

> That was left in there because we are setting sys_tz

    My patch moved that to read_persistent_clock()...

> and do_gtod, and do_gtod at least is only updated with the xtime_lock held.

> Of course, at that early stage in the boot process, no lock is really needed, but
> xtime_lock was taken for consistency with other code.

    Thanks for claryfing this.

>>    The only thing I'm still unusre about is that deterministic accounting. 
>>Could you point me at the patch which deals with this (at least for System 390 

> See efe567fc8281661524ffa75477a7c4ca9b466c63 in Linus' tree, and look

    Wait, how this is related to the hrtimer's event handlers not being able 
to call account_process_time() from arch/powerpc/kernel/time.c instead of 
update_process_timess()?

> for posts to lkml by Christian Borntraeger, who has been pursuing the
> issue (subject "Re: [stable] 2.6.23 regression: top displaying 9999%
> CPU usage").

    Fun. :-)

> Paul.

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello, I wrote:

>>>   The only thing I'm still unusre about is that deterministic accounting. 
>>>Could you point me at the patch which deals with this (at least for System 390 

>>See efe567fc8281661524ffa75477a7c4ca9b466c63 in Linus' tree, and look

>     Wait, how this is related to the hrtimer's event handlers not being able 
> to call account_process_time() from arch/powerpc/kernel/time.c instead of 
> update_process_timess()?

    I've just realized that I've missed the call to account_process_time() in 
the new timer_interrupt(). :-<
    Anyway, this leads to each tick being accounted twice if the deterministic 
accounting is not enabled -- first by timer_interrupt() and then by the 
hrtimers core, doesn't it?

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

>     I've just realized that I've missed the call to account_process_time() in 
> the new timer_interrupt(). :-<

Which is bogus.  I had removed it in the version of the patch that I
posted in early September, but apparently it crept back in.

>     Anyway, this leads to each tick being accounted twice if the deterministic 
> accounting is not enabled -- first by timer_interrupt() and then by the 
> hrtimers core, doesn't it?

Yep.

Actually, I thought I was told that with CFS, the total process time
was accounted accurately using sched_clock(), then apportioned between
utime and stime (using counts of ticks in user/system state, which are
somewhat inaccurate).  At the time I thought that would be OK, but now
I'm not so sure.

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

>>    I'd use (evt - 1) since the interrupt gets generated at 0xffffffff count, 
>>not 0 (on classic CPUs).  With you removing of the code that compensated for 

> See commit d968014b7280e2c447b20363e576999040ac72ef; I already fixed
> that.

    BTW, while fixing that for classic PPC, but you've broke it for 40x / book 
E CPU which interrupt at 0.  Congratulations. :-)

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

>     BTW, while fixing that for classic PPC, but you've broke it for 40x / book 
> E CPU which interrupt at 0.  Congratulations. :-)

What problem do you see arising from this?

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Paul Mackerras wrote:

>>    BTW, while fixing that for classic PPC, but you've broke it for 40x / book 
>>E CPU which interrupt at 0.  Congratulations. :-)

> What problem do you see arising from this?

    Timers firing too early.

> Paul.

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

> > What problem do you see arising from this?
> 
>     Timers firing too early.

Only if the minimum interrupt latency is less than 1 decrementer
tick.  That seems pretty unlikely to me unless you have a very slow
timebase frequency.

In fact what we should program the decrementer to is:

	timeout - (is_booke? 0: 1) - min_interrupt_latency

I was assuming that min_interrupt_latency (measured in timebase ticks)
would be at least 1, but apparently some systems can have a timebase
frequency as low as 1kHz, so we'll have to have an ifdef or something.

Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Gabriel Paubert]

On Fri, Oct 19, 2007 at 10:14:54AM +1000, Paul Mackerras wrote:
> Sergei Shtylyov writes:
> 
> > > What problem do you see arising from this?
> > 
> >     Timers firing too early.
> 
> Only if the minimum interrupt latency is less than 1 decrementer
> tick.  That seems pretty unlikely to me unless you have a very slow
> timebase frequency.
> 
> In fact what we should program the decrementer to is:
> 
> 	timeout - (is_booke? 0: 1) - min_interrupt_latency
> 
> I was assuming that min_interrupt_latency (measured in timebase ticks)
> would be at least 1, but apparently some systems can have a timebase
> frequency as low as 1kHz, so we'll have to have an ifdef or something.

If it is the case (the slowest I've seen was in the 8 MHz range), then
it is better not to subtract the one: think of what happens when you
have say, a 10kHz timebase/decrementer frequency and you want to
interrupt after 1 ms. You actually want to interrupt after 11
transitions of the decrementer, not 10 since the result could be
as low as 900us if you happen to read the timebase just before
a transition.

I'd really wish there were a guarantee of a minimum timebase 
frequency, for anything above ~10MHz, we are splitting hairs
about an off-by-one error that never accumulates, but if systems
with very low TB freq exist, you have to be very careful. In any case
timer interrupts should never came earlier than asked for.

	Gabriel

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Gabriel Paubert writes:

> I'd really wish there were a guarantee of a minimum timebase 
> frequency, for anything above ~10MHz, we are splitting hairs
> about an off-by-one error that never accumulates, but if systems
> with very low TB freq exist, you have to be very careful.

Exactly.  In looking through the generic clockevents code, there are a
few places where counts are rounded down.  For example,
clockevents_program_event does this to convert from nanoseconds (in
"delta") to the count value for the clock event device:

	clc = delta * dev->mult;
	clc >>= dev->shift;

Here the right shift can lose up to a whole count.  If the clock
frequency of the device is very slow, say 1kHz, then this could lose
up to 999999 nanoseconds.  Then, on Book E, putting 1 into the
decrementer could result in an interrupt anywhere from straight away to
1 millisecond later (assuming 1kHz timebase frequency, again).

The net result is that delta = 1999999 nanoseconds could result in the
interrupt coming in immediately, i.e. almost 2ms early.

I believe the clockevents framework has not been designed for use with
very slow one-shot clock event devices.  If it is to be used with very
low clock rates, then there are several points where I think the
rounding/truncation issues need to be carefully thought through.

In any case, the code we have at the moment won't work with timebase
clock rates below 15.258kHz, because decrementer_clockevent.mult will
become zero.

Regards,
Paul.

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Sergei Shtylyov]

Hello.

Paul Mackerras wrote:

>>>What problem do you see arising from this?

>>    Timers firing too early.

> Only if the minimum interrupt latency is less than 1 decrementer
> tick.  That seems pretty unlikely to me unless you have a very slow
> timebase frequency.

    Well, MPC8540 has 825 MHz CPU clock yet decrementor/timebase are clocked 
with 25 MHz clock if I don't mistake. That gives us 33 CPU clocks of available 
interrupt latency...

> In fact what we should program the decrementer to is:

> 	timeout - (is_booke? 0: 1) - min_interrupt_latency

    BTW, why not handle the decrementer difference right in set_dec() where we 
already have #ifdef'ed code?

> I was assuming that min_interrupt_latency (measured in timebase ticks)
> would be at least 1, but apparently some systems can have a timebase
> frequency as low as 1kHz, so we'll have to have an ifdef or something.

    IMHO it's better to have #ifdef based on the decremeter model and forget 
about the whole issue, rather than to #ifdef based on some bizarre system with 
slowish decremeter, isn't it?

> Paul.

WBR, Sergei

Re: [PATCH v2 3/4] Implement clockevents driver for powerpc

[Paul Mackerras]

Sergei Shtylyov writes:

>     BTW, why not handle the decrementer difference right in set_dec() where we 
> already have #ifdef'ed code?

Excellent idea.  Let's do that.

Paul.

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Steven Rostedt]

Tony,

I'm about to release a new -rt patch based on -rc8. This patch series
blew up totally in trying to get it applied. I'm leaving it out, so after
I release the next series, could you update these patches.

Thanks,

-- Steve

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Sergei Shtylyov]

Hello.

Steven Rostedt wrote:
> Tony,

> I'm about to release a new -rt patch based on -rc8. This patch series
> blew up totally in trying to get it applied. I'm leaving it out, so after
> I release the next series, could you update these patches.

    No wonder here: the -rt patch already has much of this code since around 
2.6.21.  They have been submitted by me, mostly... and this patchset is 
against the Linus' tree.

WBR, Sergei

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Steven Rostedt]

--
On Wed, 26 Sep 2007, Sergei Shtylyov wrote:

> Hello.
>
> Steven Rostedt wrote:
> > Tony,
>
> > I'm about to release a new -rt patch based on -rc8. This patch series
> > blew up totally in trying to get it applied. I'm leaving it out, so after
> > I release the next series, could you update these patches.
>
>     No wonder here: the -rt patch already has much of this code since around
> 2.6.21.  They have been submitted by me, mostly... and this patchset is
> against the Linus' tree.

That would explain it ;-)

I was searching the linux-rt-users mailing list for any patches, and
pulled in any that looked legit. These happen to in that mailing list too.

-- Steve

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Thomas Gleixner]

On Wed, 2007-09-26 at 15:44 -0400, Steven Rostedt wrote:
> >     No wonder here: the -rt patch already has much of this code since around
> > 2.6.21.  They have been submitted by me, mostly... and this patchset is
> > against the Linus' tree.
> 
> That would explain it ;-)
> 
> I was searching the linux-rt-users mailing list for any patches, and
> pulled in any that looked legit. These happen to in that mailing list too.

I'll sort that out along with the other hrt updates.

	tglx

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Sergei Shtylyov]

Hello.

Thomas Gleixner wrote:

>>>    No wonder here: the -rt patch already has much of this code since around
>>>2.6.21.  They have been submitted by me, mostly... and this patchset is
>>>against the Linus' tree.

>>That would explain it ;-)

>>I was searching the linux-rt-users mailing list for any patches, and
>>pulled in any that looked legit. These happen to in that mailing list too.

> I'll sort that out along with the other hrt updates.

   Eh... poor you. Tony got clockevent driver reengineered for no apparent 
reason.  And he's introduced the jiffy drift by deleting the main loop from 
timer_interrupt(). Yet this borken version was preferred to what was known 
working since about 2.6.18 and included into 2.6.21-rt patchset.  I don't like 
that policy. Will you be pushing fixes from -rt to PowerPC, or will it fall on 
my shoulders now?

> 	tglx

WBR, Sergei

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Paul Mackerras]

Sergei Shtylyov writes:

>    Eh... poor you. Tony got clockevent driver reengineered for no apparent 
> reason.  And he's introduced the jiffy drift by deleting the main loop from 
> timer_interrupt().

The main loop in timer_interrupt() became unnecessary, because
update_wall_time contains an equivalent loop.

Paul.

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Benjamin Herrenschmidt]

>    Eh... poor you. Tony got clockevent driver reengineered for no apparent 
> reason.  And he's introduced the jiffy drift by deleting the main loop from 
> timer_interrupt(). Yet this borken version was preferred to what was known 
> working since about 2.6.18 and included into 2.6.21-rt patchset.  I don't like 
> that policy. Will you be pushing fixes from -rt to PowerPC, or will it fall on 
> my shoulders now?

Possibly because whatever implementation existed before was never
provided in a mergeable form and pushed to -rt bypassing the powerpc
architecture maintainer ?

Ben.

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>   Eh... poor you. Tony got clockevent driver reengineered for no apparent 
>>reason.  And he's introduced the jiffy drift by deleting the main loop from 
>>timer_interrupt(). Yet this borken version was preferred to what was known 
>>working since about 2.6.18 and included into 2.6.21-rt patchset.  I don't like 
>>that policy. Will you be pushing fixes from -rt to PowerPC, or will it fall on 
>>my shoulders now?

> Possibly because whatever implementation existed before was never
> provided in a mergeable form

    The fact that vDSO calls were removed out of the necessity (because having 
the broken TOD stuff removed was better than to leave it in place producing 
inexact results) doesn't mean that everything in the patches provided was 
broken and of no use.  Yet you want everything removed in favor of somewhat 
dubious implementation.

> and pushed to -rt bypassing the powerpc architecture maintainer ?

    There was no bypassing, everything was submitted publicly via this list. 
The -rt patch (via the hrtimers patchset) was the place to merge the hrtimers 
code at thiat time, and nobody showed any interest in making the code better, 
i.e. amending vDSO stuff, for months... (The question also is why Tony was 
submitting his patches also to linux-rt-users while they were known not to 
apply to the -rt patch.)

> Ben.

WBR, Sergei

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Benjamin Herrenschmidt]

On Wed, 2007-09-26 at 23:39 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> Steven Rostedt wrote:
> > Tony,
> 
> > I'm about to release a new -rt patch based on -rc8. This patch series
> > blew up totally in trying to get it applied. I'm leaving it out, so after
> > I release the next series, could you update these patches.
> 
>     No wonder here: the -rt patch already has much of this code since around 
> 2.6.21.  They have been submitted by me, mostly... and this patchset is 
> against the Linus' tree.

Proper approach is to rip off what is altready in -rt there and replace
it with Tony patch set.

Ben.

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Sergei Shtylyov]

Benjamin Herrenschmidt wrote:

>>>I'm about to release a new -rt patch based on -rc8. This patch series
>>>blew up totally in trying to get it applied. I'm leaving it out, so after
>>>I release the next series, could you update these patches.

>>    No wonder here: the -rt patch already has much of this code since around 
>>2.6.21.  They have been submitted by me, mostly... and this patchset is 
>>against the Linus' tree.

> Proper approach is to rip off what is altready in -rt there and replace
> it with Tony patch set.

    Tony's patchset is broken at places compared to what is in -rt. So that 
would be proper *double standard* approach. :-/

> Ben.

WBR, Sergei

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Benjamin Herrenschmidt]

On Mon, 2007-10-15 at 22:07 +0400, Sergei Shtylyov wrote:
> > Proper approach is to rip off what is altready in -rt there and
> replace
> > it with Tony patch set.
> 
>     Tony's patchset is broken at places compared to what is in -rt. So
> that 
> would be proper *double standard* approach. :-/

Rather than tony patchset, can you look at what paulus has been
submitting, which should fix a few issues in tony initial patch and is
working, and tell us if you think something is still broken ?

Cheers,
Ben.

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Sergei Shtylyov]

Hello.

Tony Breeds wrote:

> With these functions implemented we cooperate better with the generic
> timekeeping code.  This obsoletes the need for the timer sysdev as a bonus.

    Aha, I'm seeing it's not merged to mainline yet!  And this can't be merged 
to -rt patch either, beucase -rt alsready has read_persistent_clock() 
implemented since around 2.6.18-rt...

> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

> ---

> Patch set updated to powerpc/for-2.6.24

> * Compile tested for arch/powerpc/configs/*_defconfig
> * Booted on pSeries, iSeries, Cell and PS3

>  arch/powerpc/Kconfig         |    3 +
>  arch/powerpc/kernel/time.c   |   85 ++++++++++-------------------------
>  arch/powerpc/sysdev/Makefile |    5 --
>  arch/powerpc/sysdev/timer.c  |   81 ---------------------------------
>  4 files changed, 29 insertions(+), 145 deletions(-)
> 
> Index: working/arch/powerpc/Kconfig
> ===================================================================
> --- working.orig/arch/powerpc/Kconfig
> +++ working/arch/powerpc/Kconfig
> @@ -21,6 +21,9 @@ config MMU
>  	bool
>  	default y
>  
> +config GENERIC_CMOS_UPDATE
> +	def_bool y
> +
>  config GENERIC_HARDIRQS
>  	bool
>  	default y
> Index: working/arch/powerpc/kernel/time.c
> ===================================================================
> --- working.orig/arch/powerpc/kernel/time.c
> +++ working/arch/powerpc/kernel/time.c
> @@ -881,12 +837,35 @@ void __init generic_calibrate_decr(void)
>  #endif
>  }
>  
> -unsigned long get_boot_time(void)
> +int update_persistent_clock(struct timespec now)
>  {
>  	struct rtc_time tm;
>  
> -	if (ppc_md.get_boot_time)
> -		return ppc_md.get_boot_time();
> +	if (!ppc_md.set_rtc_time)
> +		return 0;
> +
> +	to_tm(now.tv_sec + 1 + timezone_offset, &tm);
> +	tm.tm_year -= 1900;
> +	tm.tm_mon -= 1;
> +
> +	return ppc_md.set_rtc_time(&tm);
> +}
> +
> +unsigned long read_persistent_clock(void)
> +{
> +	struct rtc_time tm;
> +	static int first = 1;
> +
> +	/* XXX this is a litle fragile but will work okay in the short term */
> +	if (first) {
> +		first = 0;
> +		if (ppc_md.time_init)
> +			timezone_offset = ppc_md.time_init();
> +
> +		/* get_boot_time() isn't guaranteed to be safe to call late */
> +		if (ppc_md.get_boot_time)
> +			return ppc_md.get_boot_time() -timezone_offset;
> +	}

    This looks incomplete.

> @@ -898,14 +877,10 @@ unsigned long get_boot_time(void)
>  void __init time_init(void)
>  {
>  	unsigned long flags;
> -	unsigned long tm = 0;
>  	struct div_result res;
>  	u64 scale, x;
>  	unsigned shift;
>  
> -        if (ppc_md.time_init != NULL)
> -                timezone_offset = ppc_md.time_init();
> -
>  	if (__USE_RTC()) {
>  		/* 601 processor: dec counts down by 128 every 128ns */
>  		ppc_tb_freq = 1000000000;
> @@ -980,19 +955,14 @@ void __init time_init(void)
>  	/* Save the current timebase to pretty up CONFIG_PRINTK_TIME */
>  	boot_tb = get_tb_or_rtc();
>  
> -	tm = get_boot_time();
> -
>  	write_seqlock_irqsave(&xtime_lock, flags);

    Is there any sense of grabbing xtime_lock in time_init() when you've 
implemented read_persistent_clock()?

>  
>  	/* If platform provided a timezone (pmac), we correct the time */
>          if (timezone_offset) {
>  		sys_tz.tz_minuteswest = -timezone_offset / 60;
>  		sys_tz.tz_dsttime = 0;
> -		tm -= timezone_offset;
>          }
>  
> -	xtime.tv_sec = tm;
> -	xtime.tv_nsec = 0;

    Huh? The 'xtime' should now be set by the *generic* timekeeping code using 
read_persistent_clock() -- or else what's the point to implement the function? :-/

> @@ -1010,9 +980,6 @@ void __init time_init(void)
>  
>  	time_freq = 0;
>  
> -	last_rtc_update = xtime.tv_sec;
> -	set_normalized_timespec(&wall_to_monotonic,
> -	                        -xtime.tv_sec, -xtime.tv_nsec);
>  	write_sequnlock_irqrestore(&xtime_lock, flags);

    That 'xtime_lock' grabbing should be gone with this patch, and is not. NAK.

WBR, Sergei

Re: [PATCH v2 1/4] Implement {read,update}_persistent_clock.

[Sergei Shtylyov]

Hello, I wrote:

>>With these functions implemented we cooperate better with the generic
>>timekeeping code.  This obsoletes the need for the timer sysdev as a bonus.

>     Aha, I'm seeing it's not merged to mainline yet!

    Contrarywise, it's been merged first -- looks like this all happened 
because of me not commenting on the patches in time... :-(
    Now one can only send incremental patches... sigh...

WBR, Sergei