Patchwork NTP: Add a CONFIG_RTC_SYSTOHC configuration

login
register
mail settings
Submitter Jason Gunthorpe
Date Dec. 12, 2012, 5:56 a.m.
Message ID <20121212055643.GA18078@obsidianresearch.com>
Download mbox | patch
Permalink /patch/205434/
State New
Headers show

Comments

Jason Gunthorpe - Dec. 12, 2012, 5:56 a.m.
The purpose of this option is to allow ARM/etc systems that rely on the
class RTC subsystem to have the same kind of automatic NTP based
synchronization that we have on PC platforms. Today ARM does not
implement update_persistent_clock and makes extensive use of the
class RTC system.

When enabled CONFIG_RTC_SYSTOHC will provide a generic
rtc_update_persistent_clock that stores the current time in the RTC
and is intended complement the existing CONFIG_RTC_HCTOSYS option that
loads the RTC at boot.

The option also overrides the call to any platform update_persistent_clock
so that the RTC subsystem completely and generically replaces this
functionality.

Tested on ARM kirkwood and PPC405

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/rtc/Kconfig   |    8 ++++++++
 drivers/rtc/Makefile  |    1 +
 drivers/rtc/systohc.c |   30 ++++++++++++++++++++++++++++++
 include/linux/time.h  |    1 +
 kernel/time/ntp.c     |   12 ++++++++++--
 5 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 drivers/rtc/systohc.c

I saw on Google an older version of a similar patch to this, but I
couldn't find any indication what happened to it. So here is a
slightly different take, tested on 3.7.

I've been running basically this idea buried in PPC platform specific
code since 2.6.16..
John Stultz - Dec. 12, 2012, 7:40 p.m.
On 12/11/2012 09:56 PM, Jason Gunthorpe wrote:
> The purpose of this option is to allow ARM/etc systems that rely on the
> class RTC subsystem to have the same kind of automatic NTP based
> synchronization that we have on PC platforms. Today ARM does not
> implement update_persistent_clock and makes extensive use of the
> class RTC system.
>
> When enabled CONFIG_RTC_SYSTOHC will provide a generic
> rtc_update_persistent_clock that stores the current time in the RTC
> and is intended complement the existing CONFIG_RTC_HCTOSYS option that
> loads the RTC at boot.
>
> The option also overrides the call to any platform update_persistent_clock
> so that the RTC subsystem completely and generically replaces this
> functionality.
>
> Tested on ARM kirkwood and PPC405
So I'm initially mixed on this, as it feels a little redundant (esp 
given it may override a higher precision arch-specific function). But 
from the perspective of this being a generic RTC function, instead of an 
per-arch implementation, it does seem reasonable.

Some further notes below.


> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>   drivers/rtc/Kconfig   |    8 ++++++++
>   drivers/rtc/Makefile  |    1 +
>   drivers/rtc/systohc.c |   30 ++++++++++++++++++++++++++++++
>   include/linux/time.h  |    1 +
>   kernel/time/ntp.c     |   12 ++++++++++--
>   5 files changed, 50 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/rtc/systohc.c
>
> I saw on Google an older version of a similar patch to this, but I
> couldn't find any indication what happened to it. So here is a
> slightly different take, tested on 3.7.
>
> I've been running basically this idea buried in PPC platform specific
> code since 2.6.16..
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 19c03ab..30a866a 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -48,6 +48,14 @@ 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 the RTC time based on NTP synchronization"
> +	default y
> +	help
> +	  If you say yes here, the system time (wall clock) will be stored
> +          in the RTC specified by RTC_HCTOSYS_DEVICE approximately every 11
> +  	  minutes if the NTP status is synchronized.
> +
Nit: Does this need to depend on RTC_HCTOSYS_DEVICE being set?

Hrm. So on architectures that support GENERIC_CMOS_UPDATE already, this 
creates a duplicated code path that is slightly different. I'd like to 
avoid this if possible.  Since you're plugging 
rtc_update_persistent_clock into the GENERIC_CMOS_UPDATE code, we can't 
just have this option depend on !GENERIC_CMOS_UPDATE.

So this might need a slightly deeper rework?
I'd suggest the following:
1) Convert GENERIC_CMOS_UPDATE into SUPPORTS_UPDATE_PERSISTENT_CLOCK.
2) Add RTC_SYSTOHC but make it depend on !SUPPORTS_UPDATE_PERSISTENT_CLOCK


> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 24174b4..f79ab16 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -483,7 +483,15 @@ out:
>   	return leap;
>   }
>
> -#ifdef CONFIG_GENERIC_CMOS_UPDATE
> +#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
> +
> +/* Only do one, if using CONFIG_RTC_SYSTOHC then the platform function
> + * might be mapped to the RTC code already. */
> +#ifdef CONFIG_RTC_SYSTOHC
> +#define __update_persistent_clock rtc_update_persistent_clock
> +#else
> +#define __update_persistent_clock update_persistent_clock
> +#endif
Also, maybe this could be simplified, using a weak function that calls 
rtc_update_persistent_clock by default?
That way if there is an arch specific implementation, we will prioritize 
that one with less ifdef logic.

thanks
-john
Jason Gunthorpe - Dec. 12, 2012, 9:04 p.m.
On Wed, Dec 12, 2012 at 11:40:26AM -0800, John Stultz wrote:

> >The option also overrides the call to any platform update_persistent_clock
> >so that the RTC subsystem completely and generically replaces this
> >functionality.
> >
> >Tested on ARM kirkwood and PPC405

> So I'm initially mixed on this, as it feels a little redundant (esp
> given it may override a higher precision arch-specific function).
> But from the perspective of this being a generic RTC function,
> instead of an per-arch implementation, it does seem reasonable.

It isn't really redundant. The kernel still has two RTC subsystems,
'class RTC' and various platform specific
things. update_persisent_clock is the entry for the platform specific
RTC, while rtc_update_persistent_clock is the entry for class RTC.

The issue is on platforms like PPC where both co-exist. On PPC
update_persisent_clock just calls through a function pointer
(set_rtc_time) to the machine description to do whatever that mach
needs. Currently all the PPC mach's that use class RTC drivers via DTS
set set_rtc_time to null. Only the machs that implement a non class
RTC driver provide an implementation.

So it is an either/or case - either rtc_update_persistent_clock works
or update_persistent_clock does, never both.

> >diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> >index 19c03ab..30a866a 100644
> >+++ b/drivers/rtc/Kconfig
> >@@ -48,6 +48,14 @@ 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 the RTC time based on NTP synchronization"
> >+	default y
> >+	help
> >+	  If you say yes here, the system time (wall clock) will be stored
> >+          in the RTC specified by RTC_HCTOSYS_DEVICE approximately every 11
> >+  	  minutes if the NTP status is synchronized.
> >+
> Nit: Does this need to depend on RTC_HCTOSYS_DEVICE being set?

Yes, it looks like RTC_HCTOSYS_DEVICE should have:
 depends on RTC_SYSTOHC = y

I will correct that

> Hrm. So on architectures that support GENERIC_CMOS_UPDATE already,
> this creates a duplicated code path that is slightly different. I'd
> like to avoid this if possible.  Since you're plugging
> rtc_update_persistent_clock into the GENERIC_CMOS_UPDATE code, we
> can't just have this option depend on !GENERIC_CMOS_UPDATE.

It isn't duplicate, we need to keep update_persistent_clock to support
non-class RTC machines.

I thought about this:

        if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
	        fail = -1;
#ifdef CONFIG_RTC_SYSTOHC
	        fail = rtc_update_persistent_clock(now);
#endif
#ifdef CONFIG_GENERIC_CMOS_UPDATE
                if (fail)
                         fail = update_persistent_clock(now);
#endif
       }

Try the RTC function first, then fall back to the legacy platform
function if it didn't work.

That seems better to me, do you agree?

> So this might need a slightly deeper rework?
> I'd suggest the following:
> 1) Convert GENERIC_CMOS_UPDATE into SUPPORTS_UPDATE_PERSISTENT_CLOCK.
> 2) Add RTC_SYSTOHC but make it depend on !SUPPORTS_UPDATE_PERSISTENT_CLOCK

This would only work for ARM, not PPC..

Ultimately I suspect the clean up needed is to convert more things to
class rtc drivers and remove update_persistent_clock.
ppc is pretty close already, and I think ARM is already there.

Jason
John Stultz - Dec. 13, 2012, 12:18 a.m.
On 12/12/2012 01:04 PM, Jason Gunthorpe wrote:
> On Wed, Dec 12, 2012 at 11:40:26AM -0800, John Stultz wrote:
>
>>> The option also overrides the call to any platform update_persistent_clock
>>> so that the RTC subsystem completely and generically replaces this
>>> functionality.
>>>
>>> Tested on ARM kirkwood and PPC405
>> So I'm initially mixed on this, as it feels a little redundant (esp
>> given it may override a higher precision arch-specific function).
>> But from the perspective of this being a generic RTC function,
>> instead of an per-arch implementation, it does seem reasonable.
> It isn't really redundant. The kernel still has two RTC subsystems,
> 'class RTC' and various platform specific
> things. update_persisent_clock is the entry for the platform specific
> RTC, while rtc_update_persistent_clock is the entry for class RTC.
>
> The issue is on platforms like PPC where both co-exist. On PPC
> update_persisent_clock just calls through a function pointer
> (set_rtc_time) to the machine description to do whatever that mach
> needs. Currently all the PPC mach's that use class RTC drivers via DTS
> set set_rtc_time to null. Only the machs that implement a non class
> RTC driver provide an implementation.
>
> So it is an either/or case - either rtc_update_persistent_clock works
> or update_persistent_clock does, never both.
Right.  I just want to make sure that we reduce the duplication between 
the two paths (and ensure sane defaults, so users don't have to go 
hunting for the right config for things to work properly). The other 
complication is that in some cases, the arch specific path is much finer 
grained then the RTC layer's second granularity, so we need to ensure we 
prefer the arch specific path in those cases.

>> Hrm. So on architectures that support GENERIC_CMOS_UPDATE already,
>> this creates a duplicated code path that is slightly different. I'd
>> like to avoid this if possible.  Since you're plugging
>> rtc_update_persistent_clock into the GENERIC_CMOS_UPDATE code, we
>> can't just have this option depend on !GENERIC_CMOS_UPDATE.
> It isn't duplicate, we need to keep update_persistent_clock to support
> non-class RTC machines.
>
> I thought about this:
>
>          if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
> 	        fail = -1;
> #ifdef CONFIG_RTC_SYSTOHC
> 	        fail = rtc_update_persistent_clock(now);
> #endif
> #ifdef CONFIG_GENERIC_CMOS_UPDATE
>                  if (fail)
>                           fail = update_persistent_clock(now);
> #endif
>         }
>
> Try the RTC function first, then fall back to the legacy platform
> function if it didn't work.
>
> That seems better to me, do you agree?

I do, although again, in the case where the arch specific implementation 
is "better", we end up losing granularity (s390 is the specific example 
I'm thinking of), since this prefers the RTC implementation over the 
arch specific one.  So maybe I'd suggest switching it so we prefer the 
arch specific one, and then remove the arch specific implementations 
where they are inferior to the RTC one.

>
>> So this might need a slightly deeper rework?
>> I'd suggest the following:
>> 1) Convert GENERIC_CMOS_UPDATE into SUPPORTS_UPDATE_PERSISTENT_CLOCK.
>> 2) Add RTC_SYSTOHC but make it depend on !SUPPORTS_UPDATE_PERSISTENT_CLOCK
> This would only work for ARM, not PPC..
>
> Ultimately I suspect the clean up needed is to convert more things to
> class rtc drivers and remove update_persistent_clock.
> ppc is pretty close already, and I think ARM is already there.
As long as we have a clear iterative path forward, with a solution for 
the cases where the arch method is actually preferred, I think it sounds 
like a reasonable approach.

thanks
-john
Jason Gunthorpe - Dec. 13, 2012, 5:21 a.m.
On Wed, Dec 12, 2012 at 04:18:31PM -0800, John Stultz wrote:

> I do, although again, in the case where the arch specific
> implementation is "better", we end up losing granularity (s390 is
> the specific example I'm thinking of), since this prefers the RTC
> implementation over the arch specific one.  So maybe I'd suggest
> switching it so we prefer the arch specific one, and then remove the
> arch specific implementations where they are inferior to the RTC
> one.

Unfortunately I put rtc_update_persistent_clock first because it can
have sensible error reporting. update_persistent_clock will return 0
if there is no RTC device available, or if the RTC was successfully
updated.

I can make rtc_update_persistent_clock return -ENODEV.

> As long as we have a clear iterative path forward, with a solution
> for the cases where the arch method is actually preferred, I think
> it sounds like a reasonable approach.

I think it is fine to leave it as a configure option, archs can
default it to yes when it is appropriate for them.

A quick scan through the 3.7 tree for update_persistent_clock::
 alpha - single non class RTC clock scheme
 cris - printk's and does nothing
 mips - weak function rtc_mips_set_time, all implementations are
        non class rtc
 mn10300 - single non class RTC clock scheme
 powerpc - indirects through ppc_md.set_rtc_time, all implementations
           do not use class RTC
 sh - indirects through rtc_sh_set_time, two implementations, neither
      use class RTC
 sparc - Seems to be class rtc converted. Already implements this
         patch's functionality in an arch specific file
 x86 - All the functions under the set_wallclock indirection seem
       to be non class RTC cases

No other arches seem to have update_persistent_clock in them.

I think the s390 functionality you are refering to is in its
read_persistant_clock, which looks like it has ns resolution.

That is also fine because s390 does not use class rtc so there is no
duplicate path to the 'tod' code either through
rtc_update_persistent_clock or through rtc_hctosys.

Basically, as far as I can tell, if rtc_update_persistent_clock
succeeds then update_persistent_clock is a nop. I can't find any case
where *both* could succeed. Thus trying rtc_update_persistent_clock
first is OK.

Jason
John Stultz - Dec. 14, 2012, 1:29 a.m.
On 12/12/2012 09:21 PM, Jason Gunthorpe wrote:
> On Wed, Dec 12, 2012 at 04:18:31PM -0800, John Stultz wrote:
>
>> I do, although again, in the case where the arch specific
>> implementation is "better", we end up losing granularity (s390 is
>> the specific example I'm thinking of), since this prefers the RTC
>> implementation over the arch specific one.  So maybe I'd suggest
>> switching it so we prefer the arch specific one, and then remove the
>> arch specific implementations where they are inferior to the RTC
>> one.
> Unfortunately I put rtc_update_persistent_clock first because it can
> have sensible error reporting. update_persistent_clock will return 0
> if there is no RTC device available, or if the RTC was successfully
> updated.

Hrm.. Maybe update_persistent_clock() should be changed to return an error?

> I can make rtc_update_persistent_clock return -ENODEV.
>
>> As long as we have a clear iterative path forward, with a solution
>> for the cases where the arch method is actually preferred, I think
>> it sounds like a reasonable approach.
> I think it is fine to leave it as a configure option, archs can
> default it to yes when it is appropriate for them.
>
> A quick scan through the 3.7 tree for update_persistent_clock::
>   alpha - single non class RTC clock scheme
>   cris - printk's and does nothing
>   mips - weak function rtc_mips_set_time, all implementations are
>          non class rtc
>   mn10300 - single non class RTC clock scheme
>   powerpc - indirects through ppc_md.set_rtc_time, all implementations
>             do not use class RTC
>   sh - indirects through rtc_sh_set_time, two implementations, neither
>        use class RTC
>   sparc - Seems to be class rtc converted. Already implements this
>           patch's functionality in an arch specific file
>   x86 - All the functions under the set_wallclock indirection seem
>         to be non class RTC cases
>
> No other arches seem to have update_persistent_clock in them.
>
> I think the s390 functionality you are refering to is in its
> read_persistant_clock, which looks like it has ns resolution.
>
> That is also fine because s390 does not use class rtc so there is no
> duplicate path to the 'tod' code either through
> rtc_update_persistent_clock or through rtc_hctosys.
>
> Basically, as far as I can tell, if rtc_update_persistent_clock
> succeeds then update_persistent_clock is a nop. I can't find any case
> where *both* could succeed. Thus trying rtc_update_persistent_clock
> first is OK.
Ok then. I think part of my confusion is that on the 
read_persistent_clock/rtc_hctosys side of things, we are careful to 
prioritize the read_persistent_clock implementation (since it has the 
additional requirement to be safe to call in irq context, it allows us 
to update the system time at resume earlier, avoiding time error).  So I 
guess I just naturally expect the same prioritization on the write side.

So yea, I guess your approach will work. Although I get the suspicion it 
will require further cleanups down the road (just to help make the 
various arches more consistent).

Want to resend with the changes you suggested, and I'll take another 
look at it?

thanks
-john

Patch

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 19c03ab..30a866a 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -48,6 +48,14 @@  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 the RTC time based on NTP synchronization"
+	default y
+	help
+	  If you say yes here, the system time (wall clock) will be stored
+          in the RTC specified by RTC_HCTOSYS_DEVICE approximately every 11
+  	  minutes if the NTP status is synchronized.
+
 config RTC_DEBUG
 	bool "RTC debug support"
 	help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 56297f0..69d11f1 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -6,6 +6,7 @@  ccflags-$(CONFIG_RTC_DEBUG)	:= -DDEBUG
 
 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..0536cae
--- /dev/null
+++ b/drivers/rtc/systohc.c
@@ -0,0 +1,30 @@ 
+/*
+ * 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>
+#include <linux/time.h>
+
+/* Replacement for the NTP platform function 'update_persistent_clock'
+ * that does the opposite of rtc_hctosys.c */
+int rtc_update_persistent_clock(struct timespec now)
+{
+	struct rtc_device *rtc;
+	struct rtc_time tm;
+	int err = -ENODEV;
+
+	if (now.tv_nsec < (NSEC_PER_SEC >> 1))
+		rtc_time_to_tm(now.tv_sec, &tm);
+	else
+		rtc_time_to_tm(now.tv_sec + 1, &tm);
+
+	rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
+	if (rtc) {
+		err = rtc_set_mmss(rtc, now.tv_sec);
+		rtc_class_close(rtc);
+	}
+
+	return err;
+}
diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..d668f9c 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -118,6 +118,7 @@  static inline bool timespec_valid_strict(const struct timespec *ts)
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
 extern int update_persistent_clock(struct timespec now);
+extern int rtc_update_persistent_clock(struct timespec now);
 void timekeeping_init(void);
 extern int timekeeping_suspended;
 
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 24174b4..f79ab16 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -483,7 +483,15 @@  out:
 	return leap;
 }
 
-#ifdef CONFIG_GENERIC_CMOS_UPDATE
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
+
+/* Only do one, if using CONFIG_RTC_SYSTOHC then the platform function
+ * might be mapped to the RTC code already. */
+#ifdef CONFIG_RTC_SYSTOHC
+#define __update_persistent_clock rtc_update_persistent_clock
+#else
+#define __update_persistent_clock update_persistent_clock
+#endif
 
 static void sync_cmos_clock(struct work_struct *work);
 
@@ -511,7 +519,7 @@  static void sync_cmos_clock(struct work_struct *work)
 
 	getnstimeofday(&now);
 	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
-		fail = update_persistent_clock(now);
+		fail = __update_persistent_clock(now);
 
 	next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec - (TICK_NSEC / 2);
 	if (next.tv_nsec <= 0)