Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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 jguntho...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
