[RFC PATCH] rtc: add rtc_systohc for ntp use

[RFC PATCH] rtc: add rtc_systohc for ntp use

[Alessandro Zummo]

Adds in-kernel hctosys functionality that can
be used by ntp sync code.

This is an RFC and has not been tested, I just want
to check if something similar could solve the problems
of those who want the NTP sync mode.

Signed-off-by: Alessandro Zummo <a.zummo@towertech.it>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: David Brownell <david-b@pacbell.net>
Cc: David Woodhouse <dwmw2@infradead.org>
---

 drivers/rtc/Kconfig   |   19 ++++++++++++++++++-
 drivers/rtc/Makefile  |    1 +
 drivers/rtc/systohc.c |   35 +++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletions(-)
 create mode 100644 drivers/rtc/systohc.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 8abbb20..1ff9427 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -30,7 +30,7 @@ config RTC_HCTOSYS
 	  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 to set the system time on startup and resume"
 	depends on RTC_HCTOSYS = y
 	default "rtc0"
 	help
@@ -52,6 +52,23 @@ config RTC_HCTOSYS_DEVICE
 	  sleep states. Do not specify an RTC here unless it stays powered
 	  during all this system's supported sleep states.
 
+config RTC_SYSTOHC
+	bool "Set RTC from system time in NTP sync mode"
+	depends on RTC_CLASS = y
+	default y
+	help
+	  If you say yes here, the system time (wall clock) will be written
+	  to the hardware clock every 11 minutes, if the kernel is in NTP
+	  mode and your platforms supports it.
+
+config RTC_SYSTOHC_DEVICE
+	string "RTC used to save the system time in NTP sync mode"
+	depends on RTC_SYSTOHC = y
+	default "rtc0"
+	help
+	  The RTC device that will get written with the system time
+	  in NTP mode.
+
 config RTC_DEBUG
 	bool "RTC debug support"
 	depends on RTC_CLASS = y
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index e9e8474..6d1c519 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -8,6 +8,7 @@ endif
 
 obj-$(CONFIG_RTC_LIB)		+= rtc-lib.o
 obj-$(CONFIG_RTC_HCTOSYS)	+= hctosys.o
+obj-$(CONFIG_RTC_SYSTOHC)	+= systohc.o
 obj-$(CONFIG_RTC_CLASS)		+= rtc-core.o
 rtc-core-y			:= class.o interface.o
 
diff --git a/drivers/rtc/systohc.c b/drivers/rtc/systohc.c
new file mode 100644
index 0000000..41ec77b
--- /dev/null
+++ b/drivers/rtc/systohc.c
@@ -0,0 +1,35 @@
+/*
+ * RTC subsystem, systohc for ntp use
+ *
+ * Copyright (C) 2008 Tower Technologies
+ * Author: Alessandro Zummo <a.zummo@towertech.it>
+ *
+ * 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
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/rtc.h>
+
+static int rtc_systohc(struct rtc_time *tm)
+{
+	int err;
+	struct rtc_time tm;
+	struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE);
+
+	if (rtc == NULL) {
+		printk("%s: unable to open rtc device (%s)\n",
+			__FILE__, CONFIG_RTC_SYSTOHC_DEVICE);
+		return -ENODEV;
+	}
+
+	err = rtc_set_time(rtc, tm);
+	if (err != 0)
+		dev_err(rtc->dev.parent,
+			"systohc: unable to set the hardware clock\n");
+
+	rtc_class_close(rtc);
+
+	return err;
+}
+EXPORT_SYMBOL(rtc_systohc);

Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[David Brownell]

Yeah, we should have one of these.  :)


On Monday 10 November 2008, Alessandro Zummo wrote:
> @@ -30,7 +30,7 @@ config RTC_HCTOSYS
>  	  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 to set the system time on startup and resume"
>  	depends on RTC_HCTOSYS = y
>  	default "rtc0"
>  	help
> @@ -52,6 +52,23 @@ config RTC_HCTOSYS_DEVICE
>  	  sleep states. Do not specify an RTC here unless it stays powered
>  	  during all this system's supported sleep states.
>  
> +config RTC_SYSTOHC
> +	bool "Set RTC from system time in NTP sync mode"
> +	depends on RTC_CLASS = y
> +	default y
> +	help
> +	  If you say yes here, the system time (wall clock) will be written
> +	  to the hardware clock every 11 minutes, if the kernel is in NTP
> +	  mode and your platforms supports it.
> +
> +config RTC_SYSTOHC_DEVICE
> +	string "RTC used to save the system time in NTP sync mode"

Why not just use RTC_HCTOSYS_DEVICE for this?  I have a hard time
sseeing any other value make sense ... assuming there's only one
RTC involved.

A better default might be to update all the RTCs on the system.

I'm thinking of my trusty test-case here:  rtc0 is highly functional
(including wake from RTC alarm) but not battery backed, while rtc1
is battery backed but only tracks time.  NTP really needs to update
both of them ... rtc0 since that's what's used most of the time,
and also rtc1 since that's what actually *stores* the time during
power off cycles.


> +static int rtc_systohc(struct rtc_time *tm)

I think "static" will lose, especially since ...


> +EXPORT_SYMBOL(rtc_systohc);

... you want other code to call it. 

Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[Alessandro Zummo]

On Mon, 10 Nov 2008 14:57:28 -0800
David Brownell <david-b@pacbell.net> wrote:

> Yeah, we should have one of these.  :)

 :) great! now let's see if we can get others
 to agree!
 

> A better default might be to update all the RTCs on the system.
> 
> I'm thinking of my trusty test-case here:  rtc0 is highly functional
> (including wake from RTC alarm) but not battery backed, while rtc1
> is battery backed but only tracks time.  NTP really needs to update
> both of them ... rtc0 since that's what's used most of the time,
> and also rtc1 since that's what actually *stores* the time during
> power off cycles.

 well, let's start with one... we all lived with one rtc until
 a couple of year ago :)
 
 
> > +static int rtc_systohc(struct rtc_time *tm)
> 
> I think "static" will lose, especially since ...

 wooops!

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[David Woodhouse]

On Mon, 2008-11-10 at 16:40 +0100, Alessandro Zummo wrote:
> Adds in-kernel hctosys functionality that can
> be used by ntp sync code.
> 
> This is an RFC and has not been tested, I just want
> to check if something similar could solve the problems
> of those who want the NTP sync mode.

You might do better to open the device once and keep it open, rather
than taking the mutex and opening it again _during_ each call? You're
going to be perturbing the timing by doing that.

I believe you were also concerned that some device wouldn't want the
behaviour given by the existing sync_cmos_clock() function and workqueue
stuff in kernel/ntp.c, where we update the clock precisely half-way
through the second?

We should probably rip that code out of ntp.c (or just disable it ifdef
CONFIG_RTC_CLASS), and provide our own version of notify_cmos_timer().

The workqueue stuff to trigger at half-past the second could be kept as
a library function which most RTC devices would use, but they'd have the
option to use their own instead.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[Gabriel Paubert]

On Tue, Nov 11, 2008 at 02:10:39PM +0000, David Woodhouse wrote:
> On Mon, 2008-11-10 at 16:40 +0100, Alessandro Zummo wrote:
> > Adds in-kernel hctosys functionality that can
> > be used by ntp sync code.
> > 
> > This is an RFC and has not been tested, I just want
> > to check if something similar could solve the problems
> > of those who want the NTP sync mode.
> 
> You might do better to open the device once and keep it open, rather
> than taking the mutex and opening it again _during_ each call? You're
> going to be perturbing the timing by doing that.
> 
> I believe you were also concerned that some device wouldn't want the
> behaviour given by the existing sync_cmos_clock() function and workqueue
> stuff in kernel/ntp.c, where we update the clock precisely half-way
> through the second?

FYI, the RTC that are in my PPC machines definitely want an update
on the whole second, within a few hundred microseconds that is, since
it seems that not all LSB bits are reset by a write of the time: the 
jitter is much higher than the 31µs peak-peak you'd expect from a 32768Hz
crystal but I can't remember how much I found.

	Regards,
	Gabriel

Re: [rtc-linux] Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[Alessandro Zummo]

On Tue, 11 Nov 2008 14:10:39 +0000
David Woodhouse <dwmw2@infradead.org> wrote:

> You might do better to open the device once and keep it open, rather
> than taking the mutex and opening it again _during_ each call? You're
> going to be perturbing the timing by doing that.

 If you keep it open no other in-kernel user would be able to
 use it.

> I believe you were also concerned that some device wouldn't want the
> behaviour given by the existing sync_cmos_clock() function and workqueue
> stuff in kernel/ntp.c, where we update the clock precisely half-way
> through the second?
> 
> We should probably rip that code out of ntp.c (or just disable it ifdef
> CONFIG_RTC_CLASS), and provide our own version of notify_cmos_timer().
> 
> The workqueue stuff to trigger at half-past the second could be kept as
> a library function which most RTC devices would use, but they'd have the
> option to use their own instead.

 I'll give it a look.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[David Brownell]

On Tuesday 11 November 2008, David Woodhouse wrote:
> I believe you were also concerned that some device wouldn't want the
> behaviour given by the existing sync_cmos_clock() function and workqueue
> stuff in kernel/ntp.c, where we update the clock precisely half-way
> through the second?

That's a problem, yes.  I've never heard of any RTC that wants
such delays ... except for the PC's "CMOS" RTC.

There was some discussion about how to expose that knowledge to
userspace too.  The "hwclock" utility always imposes that delay,
and it shouldn't (except when talking to a PC RTC).

> We should probably rip that code out of ntp.c (or just disable it ifdef
> CONFIG_RTC_CLASS), and provide our own version of notify_cmos_timer().

My suggestion for in-kernel code is that "struct rtc_device" just
grow a field like "unsigned settime_delay_msec" which would never
be initialized except by rtc-cmos (which uses 500) ... and the NTP
sync code would use that value.

For out-of-kernel use, that value could be extracted with an ioctl(),
and used similarly.


> The workqueue stuff to trigger at half-past the second could be kept as
> a library function which most RTC devices would use, but they'd have the
> option to use their own instead.

I thought the workqueue stuff was primarily there to make sure
that RTC was always updated in task context -- so it can grab the
relevant mutex -- and the half-second delay was legacy PC code ...

- Dave

Re: [RFC PATCH] rtc: add rtc_systohc for ntp use

[Gabriel Paubert]

On Tue, Nov 11, 2008 at 12:58:59PM -0800, David Brownell wrote:
> On Tuesday 11 November 2008, David Woodhouse wrote:
> > I believe you were also concerned that some device wouldn't want the
> > behaviour given by the existing sync_cmos_clock() function and workqueue
> > stuff in kernel/ntp.c, where we update the clock precisely half-way
> > through the second?
> 
> That's a problem, yes.  I've never heard of any RTC that wants
> such delays ... except for the PC's "CMOS" RTC.

Indeed, they are the oddball, although very frequent. Could it be
made a constant (500msec on x86, 0 on all other architectures) ?

> 
> There was some discussion about how to expose that knowledge to
> userspace too.  The "hwclock" utility always imposes that delay,
> and it shouldn't (except when talking to a PC RTC).
> 
> > We should probably rip that code out of ntp.c (or just disable it ifdef
> > CONFIG_RTC_CLASS), and provide our own version of notify_cmos_timer().
> 
> My suggestion for in-kernel code is that "struct rtc_device" just
> grow a field like "unsigned settime_delay_msec" which would never
> be initialized except by rtc-cmos (which uses 500) ... and the NTP
> sync code would use that value.

Hmm, I believed that most internal interfaces are now using nanoseconds
for subsecond fields and values; 500000000 also fits in 32 bits and may 
avoid some unnecessary arithmetic. It is obviously overkill with 32768Hz 
crystals but consistency is nice. 

> 
> For out-of-kernel use, that value could be extracted with an ioctl(),
> and used similarly.
> 
> 
> > The workqueue stuff to trigger at half-past the second could be kept as
> > a library function which most RTC devices would use, but they'd have the
> > option to use their own instead.
> 
> I thought the workqueue stuff was primarily there to make sure
> that RTC was always updated in task context -- so it can grab the
> relevant mutex -- and the half-second delay was legacy PC code ...

Please allow configs that only use RTC internally without exposing
the driver to userspace. 

At least that's how I use the RTC, I have some messages on shutdown telling
me that the clock could not be accessed but I don't care since all these 
machines have ntp and the RTC is completely useless for anything else than 
setting the time quite precisely on reboot (on some of the boards I have, 
the RTC interrupt is not even wired, so no alarm, no periodic interrupt).

	Gabriel