lkml.org 
[lkml]   [2012]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2012-12-12 22:41    [W:0.070 / U:1.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site