[RFC][PATCH 1/4] RTC: Class device support for persistent clock

[RFC][PATCH 1/4] RTC: Class device support for persistent clock

[Maciej W. Rozycki]

 This is a generic implementation of rtc_read_persistent_clock() and
rtc_update_persistent_clock() suitable for platforms to be used for
read_persistent_clock() and update_persistent_clock() calls.  An RTC
device selected by the user with the RTC_HCTOSYS_DEVICE option is used.  

 As rtc_read_persistent_clock() is not available at the time
timekeeping_init() is called, it will now be disabled if the class device
is to be used as a reference.  In this case rtc_hctosys(), already
present, will be used to set up the system time at the late initcall time.  
This call has now been rewritten to make use of
rtc_read_persistent_clock().

 As rtc_set_mmss() used by rtc_update_persistent_clock() may sleep for 
some hardware, the call is now made from a work queue scheduled by the 
timer originally used for the entire function.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
patch-2.6.26-rc1-20080505-rtc-persistent-clock-11
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/drivers/rtc/Kconfig linux-2.6.26-rc1-20080505/drivers/rtc/Kconfig
--- linux-2.6.26-rc1-20080505.macro/drivers/rtc/Kconfig	2008-05-05 02:55:40.000000000 +0000
+++ linux-2.6.26-rc1-20080505/drivers/rtc/Kconfig	2008-05-05 21:41:34.000000000 +0000
@@ -21,24 +21,30 @@ menuconfig RTC_CLASS
 if RTC_CLASS
 
 config RTC_HCTOSYS
-	bool "Set system time from RTC on startup and resume"
+	bool "Use time from RTC as a reference for the system time"
 	depends on RTC_CLASS = y
 	default y
 	help
-	  If you say yes here, the system time (wall clock) will be set using
-	  the value read from a specified RTC device. This is useful to avoid
-	  unnecessary fsck runs at boot time, and to network better.
+	  If you say yes here, the specified RTC device will be used
+	  as a reference to the system time (wall clock).  The device
+	  will be used to set the system time as required during
+	  startup, suspend and, for platforms that support such usage,
+	  for NTP timekeeping.
+
+	  This is useful to avoid unnecessary fsck runs at boot time,
+	  and to network better.
 
 config RTC_HCTOSYS_DEVICE
-	string "RTC used to set the system time"
+	string "RTC used as a reference for the system time"
 	depends on RTC_HCTOSYS = y
 	default "rtc0"
 	help
-	  The RTC device that will be used to (re)initialize the system
+	  The RTC device that will be used as a reference for the system
 	  clock, usually rtc0.  Initialization is done when the system
-	  starts up, and when it resumes from a low power state.  This
-	  device should record time in UTC, since the kernel won't do
-	  timezone correction.
+	  starts up, and when it resumes from a low power state.  Also,
+	  if supported by the platform, NTP timekeeping uses this device
+	  to record the system time periodically.  This device should
+	  record time in UTC, since the kernel won't do timezone correction.
 
 	  The driver for this RTC device must be loaded before late_initcall
 	  functions run, so it must usually be statically linked.
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/drivers/rtc/hctosys.c linux-2.6.26-rc1-20080505/drivers/rtc/hctosys.c
--- linux-2.6.26-rc1-20080505.macro/drivers/rtc/hctosys.c	2008-05-05 02:55:40.000000000 +0000
+++ linux-2.6.26-rc1-20080505/drivers/rtc/hctosys.c	2008-05-05 21:10:50.000000000 +0000
@@ -1,8 +1,9 @@
 /*
- * RTC subsystem, initialize system time on startup
+ * RTC subsystem, persistent clock and startup initialization support
  *
  * Copyright (C) 2005 Tower Technologies
  * Author: Alessandro Zummo <a.zummo@towertech.it>
+ * Copyright (C) 2008  Maciej W. Rozycki
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -22,46 +23,85 @@
  * the best guess is to add 0.5s.
  */
 
-static int __init rtc_hctosys(void)
+/*
+ * Note the unusual API:
+ * zero returned means a failure, anything else is seconds from epoch.
+ */
+unsigned long rtc_read_persistent_clock(void)
 {
-	int err;
-	struct rtc_time tm;
 	struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
+	struct rtc_time tm;
+	unsigned long time = 0;
 
 	if (rtc == NULL) {
-		printk("%s: unable to open rtc device (%s)\n",
-			__FILE__, CONFIG_RTC_HCTOSYS_DEVICE);
-		return -ENODEV;
+		printk(KERN_ERR "hctosys: unable to open rtc device (%s)\n",
+		       CONFIG_RTC_HCTOSYS_DEVICE);
+		goto out;
+	}
+	if (rtc_read_time(rtc, &tm) < 0) {
+		dev_err(rtc->dev.parent,
+			"hctosys: unable to read the hardware clock\n");
+		goto out_close;
 	}
+	if (rtc_valid_tm(&tm) < 0) {
+		dev_err(rtc->dev.parent, "hctosys: invalid date/time\n");
+		goto out_close;
+	}
+
+	rtc_tm_to_time(&tm, &time);
 
-	err = rtc_read_time(rtc, &tm);
-	if (err == 0) {
-		err = rtc_valid_tm(&tm);
-		if (err == 0) {
-			struct timespec tv;
-
-			tv.tv_nsec = NSEC_PER_SEC >> 1;
-
-			rtc_tm_to_time(&tm, &tv.tv_sec);
-
-			do_settimeofday(&tv);
-
-			dev_info(rtc->dev.parent,
-				"setting system clock to "
-				"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
-				tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
-				tm.tm_hour, tm.tm_min, tm.tm_sec,
-				(unsigned int) tv.tv_sec);
-		}
-		else
-			dev_err(rtc->dev.parent,
-				"hctosys: invalid date/time\n");
+out_close:
+	rtc_class_close(rtc);
+out:
+	return time;
+}
+
+int rtc_update_persistent_clock(struct timespec now)
+{
+	struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
+	int err;
+
+	if (rtc == NULL) {
+		printk(KERN_ERR "hctosys: unable to open rtc device (%s)\n",
+		       CONFIG_RTC_HCTOSYS_DEVICE);
+		err = -ENXIO;
+		goto out;
 	}
-	else
+	err = rtc_set_mmss(rtc, now.tv_sec);
+	if (err < 0) {
 		dev_err(rtc->dev.parent,
-			"hctosys: unable to read the hardware clock\n");
+			"hctosys: unable to set the hardware clock\n");
+		goto out_close;
+	}
 
+	err = 0;
+
+out_close:
 	rtc_class_close(rtc);
+out:
+	return err;
+}
+
+static int __init rtc_hctosys(void)
+{
+	struct rtc_time tm;
+	struct timespec tv;
+	unsigned long time;
+
+	time = rtc_read_persistent_clock();
+	if (!time)
+		return -ENODEV;
+
+	tv.tv_nsec = NSEC_PER_SEC >> 1;
+	tv.tv_sec = time;
+	do_settimeofday(&tv);
+
+	rtc_time_to_tm(time, &tm);
+	pr_info("%s: setting system clock to "
+		"%d-%02d-%02d %02d:%02d:%02d UTC (%lu)\n",
+		CONFIG_RTC_HCTOSYS_DEVICE,
+		tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		tm.tm_hour, tm.tm_min, tm.tm_sec, (unsigned long)tv.tv_sec);
 
 	return 0;
 }
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/include/linux/rtc.h linux-2.6.26-rc1-20080505/include/linux/rtc.h
--- linux-2.6.26-rc1-20080505.macro/include/linux/rtc.h	2008-05-05 02:55:59.000000000 +0000
+++ linux-2.6.26-rc1-20080505/include/linux/rtc.h	2008-05-05 21:10:50.000000000 +0000
@@ -200,6 +200,9 @@ extern int rtc_irq_set_state(struct rtc_
 extern int rtc_irq_set_freq(struct rtc_device *rtc,
 				struct rtc_task *task, int freq);
 
+extern unsigned long rtc_read_persistent_clock(void);
+extern int rtc_update_persistent_clock(struct timespec now);
+
 typedef struct rtc_task {
 	void (*func)(void *private_data);
 	void *private_data;
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/kernel/time/ntp.c linux-2.6.26-rc1-20080505/kernel/time/ntp.c
--- linux-2.6.26-rc1-20080505.macro/kernel/time/ntp.c	2008-05-05 02:56:03.000000000 +0000
+++ linux-2.6.26-rc1-20080505/kernel/time/ntp.c	2008-05-05 21:10:50.000000000 +0000
@@ -3,6 +3,8 @@
  *
  * NTP state machine interfaces and logic.
  *
+ * Copyright (c) 2008  Maciej W. Rozycki
+ *
  * This code was mainly moved from kernel/timer.c and kernel/time.c
  * Please see those files for relevant copyright info and historical
  * changelogs.
@@ -17,6 +19,7 @@
 #include <linux/capability.h>
 #include <linux/math64.h>
 #include <linux/clocksource.h>
+#include <linux/workqueue.h>
 #include <asm/timex.h>
 
 /*
@@ -218,11 +221,13 @@ void second_overflow(void)
 /* Disable the cmos update - used by virtualization and embedded */
 int no_sync_cmos_clock  __read_mostly;
 
-static void sync_cmos_clock(unsigned long dummy);
+static void sync_cmos_clock(unsigned long data);
+static void do_sync_cmos_clock(struct work_struct *work);
 
 static DEFINE_TIMER(sync_cmos_timer, sync_cmos_clock, 0, 0);
+static DECLARE_WORK(sync_cmos_work, do_sync_cmos_clock);
 
-static void sync_cmos_clock(unsigned long dummy)
+static void do_sync_cmos_clock(struct work_struct *work)
 {
 	struct timespec now, next;
 	int fail = 1;
@@ -261,6 +266,12 @@ static void sync_cmos_clock(unsigned lon
 	mod_timer(&sync_cmos_timer, jiffies + timespec_to_jiffies(&next));
 }
 
+static void sync_cmos_clock(unsigned long data)
+{
+	/* Some implementations of update_persistent_clock() may sleep.  */
+	schedule_work(&sync_cmos_work);
+}
+
 static void notify_cmos_timer(void)
 {
 	if (!no_sync_cmos_clock)
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/kernel/time/timekeeping.c linux-2.6.26-rc1-20080505/kernel/time/timekeeping.c
--- linux-2.6.26-rc1-20080505.macro/kernel/time/timekeeping.c	2008-05-05 02:56:03.000000000 +0000
+++ linux-2.6.26-rc1-20080505/kernel/time/timekeeping.c	2008-05-05 21:10:50.000000000 +0000
@@ -242,7 +242,11 @@ unsigned long __attribute__((weak)) read
 void __init timekeeping_init(void)
 {
 	unsigned long flags;
-	unsigned long sec = read_persistent_clock();
+	unsigned long sec = 0;
+
+#ifndef CONFIG_RTC_HCTOSYS
+	sec = read_persistent_clock();
+#endif
 
 	write_seqlock_irqsave(&xtime_lock, flags);
 

Re: [rtc-linux] [RFC][PATCH 1/4] RTC: Class device support for persistent clock

[David Woodhouse]

On Wed, 2008-05-07 at 01:40 +0100, Maciej W. Rozycki wrote:
> 
> +int rtc_update_persistent_clock(struct timespec now)
> +{
> +       struct rtc_device *rtc =
> rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
> +       int err;
> +
> +       if (rtc == NULL) {
> +               printk(KERN_ERR "hctosys: unable to open rtc device (%
> s)\n",
> +                      CONFIG_RTC_HCTOSYS_DEVICE);
> +               err = -ENXIO;
> +               goto out;
>         }
> -       else
> +       err = rtc_set_mmss(rtc, now.tv_sec);
> +       if (err < 0) {
>                 dev_err(rtc->dev.parent,
> -                       "hctosys: unable to read the hardware clock
> \n");
> +                       "hctosys: unable to set the hardware clock
> \n");
> +               goto out_close;
> +       }
>  
> +       err = 0;
> +
> +out_close:
>         rtc_class_close(rtc);
> +out:
> +       return err;
> +}

Ooh, shiny -- you saved me the trouble of doing this (and hopefully also
the trouble of looking through it to check whether all the callers of
read_persistent_clock() can sleep, etc.?)

One thing I was going to do in rtc_update_persistent_clock() was make it
use mutex_trylock() for grabbing rtc->lock. We go to great lengths to
make sure we're updating the clock at the correct time -- we don't want
to be doing things which delay the update. So we should probably just
use mutex_trylock() and abort the update (this time) if it fails.

I was also thinking of holding the RTC_HCTOSYS device open all the time,
too. If it's a problem that you then couldn't unload the module, perhaps
a sysfs interface to set/change/clear which device is used for this?

When we discussed it last week, Alessandro was concerned that the
'update at precisely 500ms past the second' rule was not universal to
all RTC devices, although I'm not entirely sure. It might be worth
moving that logic into a 'default' NTP-sync routine provided by the RTC
class, so that if any strange devices exist which require different
treatment, they can override that.

I wouldn't worry too much about leaving the old
update_persistent_clock() and read_persistent_clock() -- I hope we can
plan to remove those entirely in favour of the RTC class methods.

-- 
dwmw2

Re: [rtc-linux] Re: [RFC][PATCH 1/4] RTC: Class device support for persistent clock

[Alessandro Zummo]

On Wed, 07 May 2008 09:24:15 +0100
David Woodhouse <dwmw2@infradead.org> wrote:


> Ooh, shiny -- you saved me the trouble of doing this (and hopefully also
> the trouble of looking through it to check whether all the callers of
> read_persistent_clock() can sleep, etc.?)

 I knew you would have liked that patch :)
 
> One thing I was going to do in rtc_update_persistent_clock() was make it
> use mutex_trylock() for grabbing rtc->lock. We go to great lengths to
> make sure we're updating the clock at the correct time -- we don't want
> to be doing things which delay the update. So we should probably just
> use mutex_trylock() and abort the update (this time) if it fails.

 agreable. 
 
> I was also thinking of holding the RTC_HCTOSYS device open all the time,
> too. If it's a problem that you then couldn't unload the module, perhaps
> a sysfs interface to set/change/clear which device is used for this?

 mm.. let's keep it easy. the chances the rtc is in use
 are usually real low.
 

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [rtc-linux] [RFC][PATCH 1/4] RTC: Class device support for persistent clock

[Maciej W. Rozycki]

On Wed, 7 May 2008, David Woodhouse wrote:

> Ooh, shiny -- you saved me the trouble of doing this (and hopefully also
> the trouble of looking through it to check whether all the callers of
> read_persistent_clock() can sleep, etc.?)

 :-)

 Yes, the generic callers of read_persistent_clock() seem to be fine.  I
have not dug into various pieces of platform code to check there though.  
I do hope platform maintainers enable these debugging checks from time to
time -- I mean especially those taking care of all these more or less
exotic systems and boards residing one level below generic architecture
support... ;-)

> One thing I was going to do in rtc_update_persistent_clock() was make it
> use mutex_trylock() for grabbing rtc->lock. We go to great lengths to
> make sure we're updating the clock at the correct time -- we don't want
> to be doing things which delay the update. So we should probably just
> use mutex_trylock() and abort the update (this time) if it fails.

 Good idea.

> I was also thinking of holding the RTC_HCTOSYS device open all the time,
> too. If it's a problem that you then couldn't unload the module, perhaps
> a sysfs interface to set/change/clear which device is used for this?

 I have thought of this and recognised the concern about modules.  I think
another possibility that could work with modules might be opening and
closing the device on demand.  But still it is just an optimisation, which
can certainly be done gradually on top of these changes without even
changing the interface.

 From the code structure's point of view it is certainly cleaner to open
and close the device each time it is used and I think as such it is a good
starting point -- let's spoil it later. ;-)

> When we discussed it last week, Alessandro was concerned that the
> 'update at precisely 500ms past the second' rule was not universal to
> all RTC devices, although I'm not entirely sure. It might be worth
> moving that logic into a 'default' NTP-sync routine provided by the RTC
> class, so that if any strange devices exist which require different
> treatment, they can override that.

 Even better than that -- some devices may have better precision and 
require no strange handling.  For example this very M41T81 chip I am 
working with supports resolution up to .01s -- which means there is little 
point in trying to work out delays, etc. to get the clock written back at 
the right point, where we can easily obtain two levels of magnitude better 
a resolution by simply not discarding the sub-second part of a timestamp.  

 Of course for sub-second resolution of the RTC the interface of
read_persistent_clock() would have to be modified to return a struct
timespec; contrarily update_persistent_clock() is ready for that, but then
struct rtc_time plus rtc_read_time(), rtc_set_mmss() and rtc_set_time()  
will still have to be updated accordingly.

 BTW, this chip is even better than that, because it can be disciplined --
there is a five-bit calibration register that lets one add or remove
oscillator ticks to/from the input at a certain stage of the divider chain
-- that could be used by the NTP daemon or tools like `hwclock --adjust'
somehow.  It's just an idea -- I have not investigated it further.

> I wouldn't worry too much about leaving the old
> update_persistent_clock() and read_persistent_clock() -- I hope we can
> plan to remove those entirely in favour of the RTC class methods.

 Yes, that I would consider a reasonable long-term plan, but it will take
at least a short while during which it might not be a good idea to break
all the platforms that have not got converted yet.  Especially as, however
unbelievable it may sound, unlike myself I think most people do not
consider the Broadcom SWARM the most obvious platform to use and/or
support.

  Maciej

Re: [RFC][PATCH 1/4] RTC: Class device support for persistent clock

[john stultz]

On Tue, May 6, 2008 at 5:40 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote:
>  This is a generic implementation of rtc_read_persistent_clock() and
>  rtc_update_persistent_clock() suitable for platforms to be used for
>  read_persistent_clock() and update_persistent_clock() calls.  An RTC
>  device selected by the user with the RTC_HCTOSYS_DEVICE option is used.
>
>   As rtc_read_persistent_clock() is not available at the time
>  timekeeping_init() is called, it will now be disabled if the class device
>  is to be used as a reference.  In this case rtc_hctosys(), already
>  present, will be used to set up the system time at the late initcall time.
>  This call has now been rewritten to make use of
>  rtc_read_persistent_clock().

Hrmm. So how is this going to work with suspend and resume?

Ideally, on resume we want to update the clock before interrupts are
reenabled so we don't get stale time values post-resume.  For systems
that sleep on reading the persistent clock, I'm open to having them
fix it up as best they can later (partly why the code can handle
read_persistent_clock() not returning anything), but unless I'm
misreading this, it seems you're proposing to make systems that do
have a safe persistent clock have to have the window where code may
see the pre-suspend time after resume.

Am I missing something here?

thanks
-john

Maciej: Sorry for the dup, I forgot to reply to all on my first mail.

Re: [RFC][PATCH 1/4] RTC: Class device support for persistent clock

[Maciej W. Rozycki]

Hi John,

 Sorry about the delay -- I have missed your comment in the flood.

On Wed, 7 May 2008, john stultz wrote:

> >   As rtc_read_persistent_clock() is not available at the time
> >  timekeeping_init() is called, it will now be disabled if the class device
> >  is to be used as a reference.  In this case rtc_hctosys(), already
> >  present, will be used to set up the system time at the late initcall time.
> >  This call has now been rewritten to make use of
> >  rtc_read_persistent_clock().
> 
> Hrmm. So how is this going to work with suspend and resume?

 Hmm, I have never used suspend/resume, so I cannot really comment.  Here
is what I gathered by glancing over the code and some bits of
documentation.

> Ideally, on resume we want to update the clock before interrupts are
> reenabled so we don't get stale time values post-resume.  For systems
> that sleep on reading the persistent clock, I'm open to having them
> fix it up as best they can later (partly why the code can handle
> read_persistent_clock() not returning anything), but unless I'm
> misreading this, it seems you're proposing to make systems that do
> have a safe persistent clock have to have the window where code may
> see the pre-suspend time after resume.

 Right now it looks the time is restored in two places, 
timekeeping_resume() and rtc_resume().  Of course once the transition to 
the new RTC infrastructure has been done, one is going to be redundant.  
For the time being I think it is harmless to have them both.

 That written, both are called from the relevant driver's ->resume()  
method.  My set of patches does not change it and as far as I can tell if
it worked before, it will work afterwards.  As I understand ->resume()  
methods may sleep and are called with interrupts already enabled.
 
> Am I missing something here?

 No idea -- anyone?

  Maciej