[RFC] rtc: add rtc_systohc for ntp use

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

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.

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

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.

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

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