Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration

[Jason Gunthorpe]

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/