[RFC] clocksouce implementation for powerpc

[RFC] clocksouce implementation for powerpc

[Tony Breeds]

On Sat, Jun 16, 2007 at 08:51:23AM -0700, Daniel Walker wrote:
> On Sat, 2007-06-16 at 10:36 +0000, Thomas Gleixner wrote:
> > plain text document attachment
> > (clocksource-add-settimeofday-hook.patch)
> > From: Tony Breeds <tony@bakeyournoodle.com >
> > 
> > I'm working on a clocksource implementation for all powerpc platforms.
> > some of these platforms needs to do a little work as part of the
> > settimeofday() syscall and I can't see a way to do that without adding
> > this hook to clocksource.
> > 
> 
> 
> I'd like to see how this is used? If the code that uses this API change
> isn't ready yet, then this patch should really wait..

This is my current patch to rework arch/powerpc/kernel/time.c to create
a clocksource.  It's not ready for inclusion.

powerpc needs to keep the vdso in sync whenener settimeodfay() is
called.  Adding the hook the to the clocksource structure was my way of
allowing this to happen.  There are other approaches, but this seemed to
best allow for runtime.  Initially I considered using update_vsyscall()
but this is called from do_timer(), and I don't need this code run then
:(

This has been booted on pSeries and iSeries (I'm using glibc 2.5, which
uses the vdso gettimeoday())

All comments appreiated.

Index: working/arch/powerpc/Kconfig
===================================================================
--- working.orig/arch/powerpc/Kconfig
+++ working/arch/powerpc/Kconfig
@@ -31,6 +31,12 @@ config MMU
 	bool
 	default 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
@@ -74,6 +74,30 @@
 #endif
 #include <asm/smp.h>
 
+/* powerpc clocksource/clockevent code */
+
+/* TODO:
+ *  o Code style
+ *    * Variable names ... be consistent.
+ *
+ * TODO: Clocksource
+ *  o Need a _USE_RTC() clocksource impelementation
+ *  o xtime:  Either time.c manages it, or clocksource does, not both
+ */
+
+#include <linux/clocksource.h>
+
+static struct clocksource clocksource_timebase = {
+	.name         = "timebase",
+	.rating       = 200,
+	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
+	.mask         = CLOCKSOURCE_MASK(64),
+	.shift        = 22,
+	.mult         = 0,	/* To be filled in */
+	.read         = NULL,   /* To be filled in */
+	.settimeofday = NULL,   /* To be filled in */
+};
+
 /* keep track of when we need to update the rtc */
 time_t last_rtc_update;
 #ifdef CONFIG_PPC_ISERIES
@@ -376,65 +400,6 @@ static __inline__ void timer_check_rtc(v
         }
 }
 
-/*
- * 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
@@ -666,8 +631,8 @@ 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);
-			timer_check_rtc();
+			/* timer_recalc_offset() && timer_check_rtc()
+			 * are now called from update_vsyscall() */
 		}
 		write_sequnlock(&xtime_lock);
 	}
@@ -739,77 +704,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.
-	 */
-#ifdef CONFIG_PPC_ISERIES
-	if (firmware_has_feature(FW_FEATURE_ISERIES) && first_settimeofday) {
-		iSeries_tb_recal();
-		first_settimeofday = 0;
-	}
-#endif
-
-	/* 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);
-
-	/* 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;
-	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;
@@ -878,6 +772,78 @@ unsigned long get_boot_time(void)
 		      tm.tm_hour, tm.tm_min, tm.tm_sec);
 }
 
+/* clocksource code */
+static cycle_t timebase_read(void)
+{
+	return (cycle_t)get_tb();
+}
+
+static void clocksource_settimeofday(struct clocksource *cs,
+                                     struct timespec *ts)
+{
+	u64 new_xsec;
+
+#ifdef CONFIG_PPC_ISERIES
+	if (firmware_has_feature(FW_FEATURE_ISERIES) && first_settimeofday) {
+		iSeries_tb_recal();
+		first_settimeofday = 0;
+	}
+#endif
+
+	/* Make userspace gettimeofday spin until we're done. */
+	++vdso_data->tb_update_count;
+	smp_mb();
+
+	/* 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 = xtime.tv_sec - 658;
+
+	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;
+
+	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
+	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
+
+	update_gtod(tb_last_jiffy, new_xsec, do_gtod.varp->tb_to_xs);
+}
+
+void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
+{
+	timer_recalc_offset(tb_last_jiffy);
+	timer_check_rtc();
+}
+
+void __init clocksource_init(void)
+{
+	int mult;
+
+	if (__USE_RTC())
+		return;
+
+	mult = clocksource_hz2mult(tb_ticks_per_sec,
+	                           clocksource_timebase.shift);
+	clocksource_timebase.mult = mult;
+
+	clocksource_timebase.read = timebase_read;
+	clocksource_timebase.settimeofday = clocksource_settimeofday;
+
+	if (clocksource_register(&clocksource_timebase)) {
+		printk(KERN_ERR "clocksource: %s is already registered\n",
+		       clocksource_timebase.name);
+		return;
+	}
+
+	printk(KERN_INFO "clocksource: %s mult[%x] shift[%d] registered\n",
+	       clocksource_timebase.name,
+	       clocksource_timebase.mult, clocksource_timebase.shift);
+}
+
 /* This function is only called on the boot processor */
 void __init time_init(void)
 {
@@ -999,6 +965,9 @@ void __init time_init(void)
 	                        -xtime.tv_sec, -xtime.tv_nsec);
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 
+	/* Register the clocksource */
+	clocksource_init();
+
 	/* Not exact, but the timer interrupt takes care of this */
 	set_dec(tb_ticks_per_jiffy);
 }
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: [RFC] clocksouce implementation for powerpc

[Daniel Walker]

On Wed, 2007-06-20 at 16:57 +1000, Tony Breeds wrote:
> On Sat, Jun 16, 2007 at 08:51:23AM -0700, Daniel Walker wrote:
> > On Sat, 2007-06-16 at 10:36 +0000, Thomas Gleixner wrote:
> > > plain text document attachment
> > > (clocksource-add-settimeofday-hook.patch)
> > > From: Tony Breeds <tony@bakeyournoodle.com >
> > > 
> > > I'm working on a clocksource implementation for all powerpc platforms.
> > > some of these platforms needs to do a little work as part of the
> > > settimeofday() syscall and I can't see a way to do that without adding
> > > this hook to clocksource.
> > > 
> > 
> > 
> > I'd like to see how this is used? If the code that uses this API change
> > isn't ready yet, then this patch should really wait..
> 
> This is my current patch to rework arch/powerpc/kernel/time.c to create
> a clocksource.  It's not ready for inclusion.
> 
> powerpc needs to keep the vdso in sync whenener settimeodfay() is
> called.  Adding the hook the to the clocksource structure was my way of
> allowing this to happen.  There are other approaches, but this seemed to
> best allow for runtime.  Initially I considered using update_vsyscall()
> but this is called from do_timer(), and I don't need this code run then
> :(

As I said in our private thread, I do think you should be using
update_vsyscall() .. update_vsyscall() is just called when the time is
set, usually that happens in the timer interrupt and sometimes that
happens in settimeofday() ..

> This has been booted on pSeries and iSeries (I'm using glibc 2.5, which
> uses the vdso gettimeoday())
> 
> All comments appreiated.

At least some of your code is duplications over what is already being
worked on inside the powerpc community.. For instance, I know there is
already a timebase clocksource,

http://people.redhat.com/~mingo/realtime-preempt/patch-2.6.21.5-rt17


> Index: working/arch/powerpc/Kconfig
> ===================================================================
> --- working.orig/arch/powerpc/Kconfig
> +++ working/arch/powerpc/Kconfig
> @@ -31,6 +31,12 @@ config MMU
>  	bool
>  	default 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
> @@ -74,6 +74,30 @@
>  #endif
>  #include <asm/smp.h>
>  
> +/* powerpc clocksource/clockevent code */
> +
> +/* TODO:
> + *  o Code style
> + *    * Variable names ... be consistent.
> + *
> + * TODO: Clocksource
> + *  o Need a _USE_RTC() clocksource impelementation
> + *  o xtime:  Either time.c manages it, or clocksource does, not both
> + */
> +
> +#include <linux/clocksource.h>
> +
> +static struct clocksource clocksource_timebase = {
> +	.name         = "timebase",
> +	.rating       = 200,
> +	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
> +	.mask         = CLOCKSOURCE_MASK(64),
> +	.shift        = 22,
> +	.mult         = 0,	/* To be filled in */
> +	.read         = NULL,   /* To be filled in */
> +	.settimeofday = NULL,   /* To be filled in */
> +};
> +
>  /* keep track of when we need to update the rtc */
>  time_t last_rtc_update;
>  #ifdef CONFIG_PPC_ISERIES
> @@ -376,65 +400,6 @@ static __inline__ void timer_check_rtc(v
>          }
>  }
>  
> -/*
> - * 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
> @@ -666,8 +631,8 @@ 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);
> -			timer_check_rtc();
> +			/* timer_recalc_offset() && timer_check_rtc()
> +			 * are now called from update_vsyscall() */
>  		}
>  		write_sequnlock(&xtime_lock);
>  	}
> @@ -739,77 +704,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.
> -	 */
> -#ifdef CONFIG_PPC_ISERIES
> -	if (firmware_has_feature(FW_FEATURE_ISERIES) && first_settimeofday) {
> -		iSeries_tb_recal();
> -		first_settimeofday = 0;
> -	}
> -#endif
> -
> -	/* 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);
> -
> -	/* 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;
> -	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;
> @@ -878,6 +772,78 @@ unsigned long get_boot_time(void)
>  		      tm.tm_hour, tm.tm_min, tm.tm_sec);
>  }
>  
> +/* clocksource code */
> +static cycle_t timebase_read(void)
> +{
> +	return (cycle_t)get_tb();
> +}
> +
> +static void clocksource_settimeofday(struct clocksource *cs,
> +                                     struct timespec *ts)
> +{
> +	u64 new_xsec;
> +
> +#ifdef CONFIG_PPC_ISERIES
> +	if (firmware_has_feature(FW_FEATURE_ISERIES) && first_settimeofday) {
> +		iSeries_tb_recal();
> +		first_settimeofday = 0;
> +	}
> +#endif
> +
> +	/* Make userspace gettimeofday spin until we're done. */
> +	++vdso_data->tb_update_count;
> +	smp_mb();
> +
> +	/* 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 = xtime.tv_sec - 658;
> +
> +	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;
> +
> +	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
> +	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
> +
> +	update_gtod(tb_last_jiffy, new_xsec, do_gtod.varp->tb_to_xs);
> +}

It does look too large to run from interrupt context, but it also looks
like it could get cleaned out more ..

> +void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
> +{
> +	timer_recalc_offset(tb_last_jiffy);
> +	timer_check_rtc();
> +}

Hmm .. This doesn't look like it's taking into account that the time has
changed .. Your time has effectively incremented by one jiffie .. The
vdso_data doesn't appear to be updated ..

> +void __init clocksource_init(void)
> +{
> +	int mult;
> +
> +	if (__USE_RTC())
> +		return;
> +
> +	mult = clocksource_hz2mult(tb_ticks_per_sec,
> +	                           clocksource_timebase.shift);
> +	clocksource_timebase.mult = mult;
> +
> +	clocksource_timebase.read = timebase_read;
> +	clocksource_timebase.settimeofday = clocksource_settimeofday;
> +
> +	if (clocksource_register(&clocksource_timebase)) {
> +		printk(KERN_ERR "clocksource: %s is already registered\n",
> +		       clocksource_timebase.name);
> +		return;
> +	}
> +
> +	printk(KERN_INFO "clocksource: %s mult[%x] shift[%d] registered\n",
> +	       clocksource_timebase.name,
> +	       clocksource_timebase.mult, clocksource_timebase.shift);
> +}
> +
>  /* This function is only called on the boot processor */
>  void __init time_init(void)
>  {
> @@ -999,6 +965,9 @@ void __init time_init(void)
>  	                        -xtime.tv_sec, -xtime.tv_nsec);
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
>  
> +	/* Register the clocksource */
> +	clocksource_init();
> +
>  	/* Not exact, but the timer interrupt takes care of this */
>  	set_dec(tb_ticks_per_jiffy);
>  }
> 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: [RFC] clocksouce implementation for powerpc

[Sergei Shtylyov]

Daniel Walker wrote:
>>+static void clocksource_settimeofday(struct clocksource *cs,
>>+                                     struct timespec *ts)
>>+{
>>+	u64 new_xsec;
>>+
>>+#ifdef CONFIG_PPC_ISERIES
>>+	if (firmware_has_feature(FW_FEATURE_ISERIES) && first_settimeofday) {
>>+		iSeries_tb_recal();
>>+		first_settimeofday = 0;
>>+	}
>>+#endif
>>+
>>+	/* Make userspace gettimeofday spin until we're done. */
>>+	++vdso_data->tb_update_count;
>>+	smp_mb();
>>+
>>+	/* 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 = xtime.tv_sec - 658;
>>+
>>+	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;
>>+
>>+	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
>>+	vdso_data->tz_dsttime = sys_tz.tz_dsttime;

    I'm not sure why these are copied *only* here. Shouldn't they be copied 
only once, at init. time?

>>+
>>+	update_gtod(tb_last_jiffy, new_xsec, do_gtod.varp->tb_to_xs);
>>+}

> It does look too large to run from interrupt context,

    You mean if this would have been "included" into update_vsyscall()?

> but it also looks
> like it could get cleaned out more ..

    Yeah, at least new_xsec calculation is duplicated in timer_recalc_offset()..

>>+void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>>+{
>>+	timer_recalc_offset(tb_last_jiffy);
>>+	timer_check_rtc();
>>+}

> Hmm .. This doesn't look like it's taking into account that the time has
> changed ..

    Why? By the time it gets called (form the timer softirq context) 
tb_last_jiffy should've been incremented. Well, this won't happen wither in or 
right after the timer interrupt... since timer has no IRQ on PowerPC -- it 
signals "exception". Well, HRT works somehow anyway. :-)

> Your time has effectively incremented by one jiffie .. The
> vdso_data doesn't appear to be updated ..

    Moreover, it will get called for settimeofday() as well which would seem 
to double the overhead since your clocksource hook will get called beforehand.

WBR, Sergei

Re: [RFC] clocksouce implementation for powerpc

[Thomas Gleixner]

On Wed, 2007-06-20 at 21:20 +0400, Sergei Shtylyov wrote:
> >>+void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
> >>+{
> >>+	timer_recalc_offset(tb_last_jiffy);
> >>+	timer_check_rtc();
> >>+}
> 
> > Hmm .. This doesn't look like it's taking into account that the time has
> > changed ..
> 
>     Why? By the time it gets called (form the timer softirq context) 

It is called from interrupt context at least in mainline. Only the -rt
patch moves this to the softirq.

	tglx

Re: [RFC] clocksouce implementation for powerpc

[Sergei Shtylyov]

Hello.

Thomas Gleixner wrote:

>>>>+void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>>>>+{
>>>>+	timer_recalc_offset(tb_last_jiffy);
>>>>+	timer_check_rtc();
>>>>+}
>>
>>>Hmm .. This doesn't look like it's taking into account that the time has
>>>changed ..
>>
>>    Why? By the time it gets called (form the timer softirq context) 
> 
> 
> It is called from interrupt context at least in mainline. Only the -rt
> patch moves this to the softirq.

    Anyway, by the time it gets called, tb_last_jiffy gets updated.

> 	tglx

WBR, Sergei

Re: [RFC] clocksouce implementation for powerpc

[Tony Breeds]

On Wed, Jun 20, 2007 at 07:57:19AM -0700, Daniel Walker wrote:

Hi Daniel.

> As I said in our private thread, I do think you should be using
> update_vsyscall() .. update_vsyscall() is just called when the time is
> set, usually that happens in the timer interrupt and sometimes that
> happens in settimeofday() ..

Well I've taken another look at the code and I think I can probably
restructure my code to use update_vsyscall().  I thought I needed a
hook that was called /only/ from settimeofday() (which as you say
doesn't match update_vsyscall()'s usage).

I'll try again and see what problems I hit.
 
> At least some of your code is duplications over what is already being
> worked on inside the powerpc community.. For instance, I know there is
> already a timebase clocksource,
> 
> http://people.redhat.com/~mingo/realtime-preempt/patch-2.6.21.5-rt17

Thanks. The one in -rt doesn't seem to support the VDSO.  however I see
that there is duplication of effort there.
 
> Hmm .. This doesn't look like it's taking into account that the time has
> changed .. Your time has effectively incremented by one jiffie .. The
> vdso_data doesn't appear to be updated ..

Unless I miss your meaning, the vdso is updated in
timer_recalc_offset()/update_gtod() when needed.

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: [RFC] clocksouce implementation for powerpc

[Sergei Shtylyov]

Hello.

Tony Breeds wrote:

>>>plain text document attachment
>>>(clocksource-add-settimeofday-hook.patch)
>>>From: Tony Breeds <tony@bakeyournoodle.com >

>>>I'm working on a clocksource implementation for all powerpc platforms.
>>>some of these platforms needs to do a little work as part of the
>>>settimeofday() syscall and I can't see a way to do that without adding
>>>this hook to clocksource.

>>I'd like to see how this is used? If the code that uses this API change
>>isn't ready yet, then this patch should really wait..

> This is my current patch to rework arch/powerpc/kernel/time.c to create
> a clocksource.  It's not ready for inclusion.

    I guess it's been based on the prior work by John Stultz (and me too :-)?

> powerpc needs to keep the vdso in sync whenener settimeodfay() is
> called.  Adding the hook the to the clocksource structure was my way of
> allowing this to happen.  There are other approaches, but this seemed to
> best allow for runtime.  Initially I considered using update_vsyscall()
> but this is called from do_timer(), and I don't need this code run then
> :(

> This has been booted on pSeries and iSeries (I'm using glibc 2.5, which
> uses the vdso gettimeoday())

[...]
> Index: working/arch/powerpc/kernel/time.c
> ===================================================================
> --- working.orig/arch/powerpc/kernel/time.c
> +++ working/arch/powerpc/kernel/time.c
> @@ -74,6 +74,30 @@
>  #endif
>  #include <asm/smp.h>
>  
> +/* powerpc clocksource/clockevent code */
> +
> +/* TODO:
> + *  o Code style
> + *    * Variable names ... be consistent.
> + *
> + * TODO: Clocksource
> + *  o Need a _USE_RTC() clocksource impelementation
> + *  o xtime:  Either time.c manages it, or clocksource does, not both

    If you mean the init. part, this has been already done by me -- I've 
implemented read_persistent_clock() and got rid of xtime setting. What's left 
is to implemet update_persistent_clock() and get rid of timer_check_rtc()...

> + */
> +
> +#include <linux/clocksource.h>
> +
> +static struct clocksource clocksource_timebase = {
> +	.name         = "timebase",
> +	.rating       = 200,

    Perhaps we even need to raise the rating to 300 or 400 -- according to 
what <linux/clocksource.h> says?

> +	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
> +	.mask         = CLOCKSOURCE_MASK(64),
> +	.shift        = 22,

    PPC64 has issues with the fixed shift value, see:

http://patchwork.ozlabs.org/linuxppc/patch?id=11125

> +	.mult         = 0,	/* To be filled in */
> +	.read         = NULL,   /* To be filled in */
> +	.settimeofday = NULL,   /* To be filled in */

    I don't quite understand why not just init them right away?  The values 
are fixed anyways.

> +};
> +
>  /* keep track of when we need to update the rtc */
>  time_t last_rtc_update;
>  #ifdef CONFIG_PPC_ISERIES
[...]
> @@ -666,8 +631,8 @@ 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);
> -			timer_check_rtc();
> +			/* timer_recalc_offset() && timer_check_rtc()
> +			 * are now called from update_vsyscall() */

    I.e. in the softirq context...

[...]

WBR, Sergei

Re: [RFC] clocksouce implementation for powerpc

[Tony Breeds]

On Wed, Jun 20, 2007 at 08:53:47PM +0400, Sergei Shtylyov wrote:

Hi Sergei,
	Thanks for taking the time to look over my patch.

>    I guess it's been based on the prior work by John Stultz (and me too :-)?

At some level I guess so.  John did send me a patch a while ago.
 
>    If you mean the init. part, this has been already done by me -- I've 
> implemented read_persistent_clock() and got rid of xtime setting. What's 
> left is to implemet update_persistent_clock() and get rid of 
> timer_check_rtc()...

Actually I think that comment is redundant.  and should be removed
sorry.

>    Perhaps we even need to raise the rating to 300 or 400 -- according to 
> what <linux/clocksource.h> says?

Sure.
 
> >+	.flags        = CLOCK_SOURCE_IS_CONTINUOUS,
> >+	.mask         = CLOCKSOURCE_MASK(64),
> >+	.shift        = 22,
> 
>    PPC64 has issues with the fixed shift value, see:
> 
> http://patchwork.ozlabs.org/linuxppc/patch?id=11125

Thanks!
 
> >+	.mult         = 0,	/* To be filled in */
> >+	.read         = NULL,   /* To be filled in */
> >+	.settimeofday = NULL,   /* To be filled in */
> 
>    I don't quite understand why not just init them right away?  The values 
> are fixed anyways.

Well at least mult needs to be calculated at runtime, and I prefer to
have the structure near the top of the file at which stage the
read/settimeofday functions aren't defined.

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: [RFC] clocksouce implementation for powerpc

[Sergei Shtylyov]

Tony Breeds wrote:

> 	Thanks for taking the time to look over my patch.

>>   I guess it's been based on the prior work by John Stultz (and me too :-)?

> At some level I guess so.  John did send me a patch a while ago.

>>   If you mean the init. part, this has been already done by me -- I've 
>>implemented read_persistent_clock() and got rid of xtime setting. What's 
>>left is to implemet update_persistent_clock() and get rid of 
>>timer_check_rtc()...

> Actually I think that comment is redundant.  and should be removed
> sorry.

    I guess you haven't looked thru the -rt patch? There's much more than 
John's initial patch there now, including the clockevents driver.

>>>+	.mult         = 0,	/* To be filled in */
>>>+	.read         = NULL,   /* To be filled in */
>>>+	.settimeofday = NULL,   /* To be filled in */
>>
>>   I don't quite understand why not just init them right away?  The values 
>>are fixed anyways.

> Well at least mult needs to be calculated at runtime, and I prefer to

    I was talking about the method intializers specifically.

> have the structure near the top of the file at which stage the
> read/settimeofday functions aren't defined.

    I don't think it's justified anyway.

> Yours Tony

WBR, Sergei

Re: [RFC] clocksouce implementation for powerpc

[john stultz]

On Wed, 2007-06-20 at 16:57 +1000, Tony Breeds wrote:
> On Sat, Jun 16, 2007 at 08:51:23AM -0700, Daniel Walker wrote:
> > On Sat, 2007-06-16 at 10:36 +0000, Thomas Gleixner wrote:
> > > plain text document attachment
> > > (clocksource-add-settimeofday-hook.patch)
> > > From: Tony Breeds <tony@bakeyournoodle.com >
> > > 
> > > I'm working on a clocksource implementation for all powerpc platforms.
> > > some of these platforms needs to do a little work as part of the
> > > settimeofday() syscall and I can't see a way to do that without adding
> > > this hook to clocksource.
> > > 
> > 
> > 
> > I'd like to see how this is used? If the code that uses this API change
> > isn't ready yet, then this patch should really wait..
> 
> This is my current patch to rework arch/powerpc/kernel/time.c to create
> a clocksource.  It's not ready for inclusion.

Hey Tony,
	Thanks for sending this out! I really appreciate this work, as its been
on my todo forever, and I've just not been able to focus on it.
Currently it seems a bit minimal of a conversion (ideally there should
be very little time code left), but It looks like a great start!

More comments below.

> powerpc needs to keep the vdso in sync whenener settimeodfay() is
> called.  Adding the hook the to the clocksource structure was my way of
> allowing this to happen.  There are other approaches, but this seemed to
> best allow for runtime.  Initially I considered using update_vsyscall()
> but this is called from do_timer(), and I don't need this code run then
> :(

I might be missing a subtlety in the ppc code, but I'm still not sure if
I see the need for the clocksource settimeofday hook.

update_vsyscall() is intended to provide a hook that allows the generic
time code to provide all the needed timekeeping state to the arch
specific vsyscall implementation. It is called any time the base
timekeeping variables are changed.

You're already calling timer_recalc_offset from there which looks almost
as expensive as the settime hook, so I'm not sure I understand the
division.

But to your credit, the patch Sergei and I have been slowly working on
(I believe I've sent that to you already, but if not let me know) never
got the VDSO code working, so good show!

> +static void clocksource_settimeofday(struct clocksource *cs,
> +                                     struct timespec *ts)
> +{
> +	u64 new_xsec;
> +
> +#ifdef CONFIG_PPC_ISERIES
> +	if (firmware_has_feature(FW_FEATURE_ISERIES) && first_settimeofday) {
> +		iSeries_tb_recal();
> +		first_settimeofday = 0;
> +	}
> +#endif
> +
> +	/* Make userspace gettimeofday spin until we're done. */
> +	++vdso_data->tb_update_count;
> +	smp_mb();
> +
> +	/* 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 = xtime.tv_sec - 658;
> +
> +	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;
> +
> +	vdso_data->tz_minuteswest = sys_tz.tz_minuteswest;
> +	vdso_data->tz_dsttime = sys_tz.tz_dsttime;
> +
> +	update_gtod(tb_last_jiffy, new_xsec, do_gtod.varp->tb_to_xs);
> +}
> +
> +void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
> +{
> +	timer_recalc_offset(tb_last_jiffy);
> +	timer_check_rtc();
> +}

I think it would be enlightening to flatten this out a bit. Putting both
the timer_recalc_offset and clocksource_settime code in the same
function. It might illustrate where some optimizations could be done and
where it might make more sense to split things up.

Also I'd leave timer_check_rtc() in the timer_interrupt for now (later
moving it to tglx's generic rtc update).

thanks
-john

Re: [RFC] clocksouce implementation for powerpc

[Tony Breeds]

On Wed, Jun 20, 2007 at 02:06:01PM -0700, john stultz wrote:

Hi John.

> Hey Tony,
> 	Thanks for sending this out! I really appreciate this work, as its been
> on my todo forever, and I've just not been able to focus on it.
> Currently it seems a bit minimal of a conversion (ideally there should
> be very little time code left), but It looks like a great start!

Thanks.

> I might be missing a subtlety in the ppc code, but I'm still not sure if
> I see the need for the clocksource settimeofday hook.
> 
> update_vsyscall() is intended to provide a hook that allows the generic
> time code to provide all the needed timekeeping state to the arch
> specific vsyscall implementation. It is called any time the base
> timekeeping variables are changed.

Well as I just said the Daniel, I was under the impression I needed a
hook that was only called from settimeofday().  The comments I've
recieved from everyone has given me good cause to re-evaluate.

I think I can make it work without the hook, that started this
discussion.  Thomas, I think it's probably best to axe it now.  If I
/really/ need it then I'll start the discussion again :)  Thanks.
 
> I think it would be enlightening to flatten this out a bit. Putting both
> the timer_recalc_offset and clocksource_settime code in the same
> function. It might illustrate where some optimizations could be done and
> where it might make more sense to split things up.
> 
> Also I'd leave timer_check_rtc() in the timer_interrupt for now (later
> moving it to tglx's generic rtc update).

Yes you're rigth I don't need to move the timer_check_rtc() call.

Yours Tony

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