| Message ID | 20121212055643.GA18078@obsidianresearch.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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)
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..