[PATCH v2 1/3] timekeeping: Add persistent_clock_exist flag

[PATCH v2 1/3] timekeeping: Add persistent_clock_exist flag

[Feng Tang]

In current kernel, there are several places which need to check
whether there is a persistent clock for the platform. Current check
is done by calling the read_persistent_clock() and validating its
return value.

So one optimization is to do the check only once in timekeeping_init(),
and use a flag persistent_clock_exist to record it.

v2: Add a has_persistent_clock() helper function, as suggested by John.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 include/linux/time.h      |    6 ++++++
 kernel/time/timekeeping.c |   16 +++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..2f58603 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -115,6 +115,12 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 	return true;
 }
 
+extern bool persistent_clock_exist;
+static inline bool has_persistent_clock(void)
+{
+	return persistent_clock_exist;
+}
+
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
 extern int update_persistent_clock(struct timespec now);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cbc6acb..2fd3aed 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -29,6 +29,9 @@ static struct timekeeper timekeeper;
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
 
+/* Flag for if there is a persistent clock on this platform */
+bool __read_mostly persistent_clock_exist = false;
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -640,12 +643,14 @@ void __init timekeeping_init(void)
 	struct timespec now, boot, tmp;
 
 	read_persistent_clock(&now);
+
 	if (!timespec_valid_strict(&now)) {
 		pr_warn("WARNING: Persistent clock returned invalid value!\n"
 			"         Check your CMOS/BIOS settings.\n");
 		now.tv_sec = 0;
 		now.tv_nsec = 0;
-	}
+	} else if (now.tv_sec || now.tv_nsec)
+		persistent_clock_exist = true;
 
 	read_boot_clock(&boot);
 	if (!timespec_valid_strict(&boot)) {
@@ -718,11 +723,12 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 {
 	struct timekeeper *tk = &timekeeper;
 	unsigned long flags;
-	struct timespec ts;
 
-	/* Make sure we don't set the clock twice */
-	read_persistent_clock(&ts);
-	if (!(ts.tv_sec == 0 && ts.tv_nsec == 0))
+	/*
+	 * Make sure we don't set the clock twice, as timekeeping_resume()
+	 * already did it
+	 */
+	if (has_persistent_clock())
 		return;
 
 	write_seqlock_irqsave(&tk->lock, flags);
-- 
1.7.9.5

[PATCH v2 2/3] rtc: Skip the suspend/resume handling if persistent clock exist

[Feng Tang]

All the RTC suspend and resume functions are to compensate the
sleep time, but this is already done in timekeeping.c if persistent
clock exist.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Arve Hjønnevåg <arve@android.com>
Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 drivers/rtc/class.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 5143629..26388f1 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -50,6 +50,10 @@ static int rtc_suspend(struct device *dev, pm_message_t mesg)
 	struct rtc_device	*rtc = to_rtc_device(dev);
 	struct rtc_time		tm;
 	struct timespec		delta, delta_delta;
+
+	if (has_persistent_clock())
+		return 0;
+
 	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
 		return 0;
 
@@ -88,6 +92,9 @@ static int rtc_resume(struct device *dev)
 	struct timespec		new_system, new_rtc;
 	struct timespec		sleep_time;
 
+	if (has_persistent_clock())
+		return 0;
+
 	rtc_hctosys_ret = -ENODEV;
 	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
 		return 0;
-- 
1.7.9.5

Re: [PATCH v2 2/3] rtc: Skip the suspend/resume handling if persistent clock exist

[John Stultz]

On 01/15/2013 08:09 AM, Feng Tang wrote:
> All the RTC suspend and resume functions are to compensate the
> sleep time, but this is already done in timekeeping.c if persistent
> clock exist.
>

Applied. Thanks!
-john

[PATCH v2 3/3] timekeeping: Add CONFIG_HAS_PERSISTENT_CLOCK option

[Feng Tang]

Make the persistent clock check a kernel config option, so that some
platform can explicitely select it, also make CONFIG_RTC_HCTOSYS depends
on its non-existence, which could prevent the persistent clock and RTC
code from doing similar thing twice during system's init/suspend/resume
phases.

If the CONFIG_HAS_PERSISTENT_CLOCK=n, then no change happens for kernel
which still does the persistent clock check in timekeeping_init().

Cc: Thomas Gleixner <tglx@linutronix.de>
Suggested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 drivers/rtc/Kconfig  |    1 +
 include/linux/time.h |    5 +++++
 kernel/time/Kconfig  |    5 +++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 923a9da..5b963c6 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -20,6 +20,7 @@ if RTC_CLASS
 config RTC_HCTOSYS
 	bool "Set system time from RTC on startup and resume"
 	default y
+	depends on !HAS_PERSISTENT_CLOCK
 	help
 	  If you say yes here, the system time (wall clock) will be set using
 	  the value read from a specified RTC device. This is useful to avoid
diff --git a/include/linux/time.h b/include/linux/time.h
index 2f58603..e08051c 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -116,10 +116,15 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 }
 
 extern bool persistent_clock_exist;
+
+#ifdef CONFIG_HAS_PERSISTENT_CLOCK
+#define has_persistent_clock()	true
+#else
 static inline bool has_persistent_clock(void)
 {
 	return persistent_clock_exist;
 }
+#endif
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 8601f0d..f7e45b9 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -12,6 +12,11 @@ config CLOCKSOURCE_WATCHDOG
 config ARCH_CLOCKSOURCE_DATA
 	bool
 
+# Platforms has a persistent clock
+config HAS_PERSISTENT_CLOCK
+	bool
+	default n
+
 # Timekeeping vsyscall support
 config GENERIC_TIME_VSYSCALL
 	bool
-- 
1.7.9.5

Re: [PATCH v2 3/3] timekeeping: Add CONFIG_HAS_PERSISTENT_CLOCK option

[John Stultz]

On 01/15/2013 08:09 AM, Feng Tang wrote:
> Make the persistent clock check a kernel config option, so that some
> platform can explicitely select it, also make CONFIG_RTC_HCTOSYS depends
> on its non-existence, which could prevent the persistent clock and RTC
> code from doing similar thing twice during system's init/suspend/resume
> phases.
>
> If the CONFIG_HAS_PERSISTENT_CLOCK=n, then no change happens for kernel
> which still does the persistent clock check in timekeeping_init().
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Suggested-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Feng Tang <feng.tang@intel.com>

Applied. I also added a dependency for Jason's CONFIG_RTC_SYSTOHC.

thanks
-john

Re: [PATCH v2 3/3] timekeeping: Add CONFIG_HAS_PERSISTENT_CLOCK option

[Jason Gunthorpe]

On Tue, Jan 15, 2013 at 11:50:18AM -0800, John Stultz wrote:
> On 01/15/2013 08:09 AM, Feng Tang wrote:
> >Make the persistent clock check a kernel config option, so that some
> >platform can explicitely select it, also make CONFIG_RTC_HCTOSYS depends
> >on its non-existence, which could prevent the persistent clock and RTC
> >code from doing similar thing twice during system's init/suspend/resume
> >phases.
> >
> >If the CONFIG_HAS_PERSISTENT_CLOCK=n, then no change happens for kernel
> >which still does the persistent clock check in timekeeping_init().
> >
> >Cc: Thomas Gleixner <tglx@linutronix.de>
> >Suggested-by: John Stultz <john.stultz@linaro.org>
> >Signed-off-by: Feng Tang <feng.tang@intel.com>
> 
> Applied. I also added a dependency for Jason's CONFIG_RTC_SYSTOHC.

Sort of an ugly config name, since I gather ARM should always set this
to 'n'...

CONFIG_USE_ONLY_PERSISTENT_CLOCK ?

Cheers,
Jason

Re: [PATCH v2 3/3] timekeeping: Add CONFIG_HAS_PERSISTENT_CLOCK option

[John Stultz]

On 01/22/2013 11:44 AM, Jason Gunthorpe wrote:
> On Tue, Jan 15, 2013 at 11:50:18AM -0800, John Stultz wrote:
>> On 01/15/2013 08:09 AM, Feng Tang wrote:
>>> Make the persistent clock check a kernel config option, so that some
>>> platform can explicitely select it, also make CONFIG_RTC_HCTOSYS depends
>>> on its non-existence, which could prevent the persistent clock and RTC
>>> code from doing similar thing twice during system's init/suspend/resume
>>> phases.
>>>
>>> If the CONFIG_HAS_PERSISTENT_CLOCK=n, then no change happens for kernel
>>> which still does the persistent clock check in timekeeping_init().
>>>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Suggested-by: John Stultz <john.stultz@linaro.org>
>>> Signed-off-by: Feng Tang <feng.tang@intel.com>
>> Applied. I also added a dependency for Jason's CONFIG_RTC_SYSTOHC.
> Sort of an ugly config name, since I gather ARM should always set this
> to 'n'...
>
> CONFIG_USE_ONLY_PERSISTENT_CLOCK ?
(Sigh. I got this seemingly microseconds after I sent the pull request :)

So yea, fair point, there could be some confusion. But 
ONLY_PERSISTENT_CLOCK isn't quite right either,  more like 
CONFIG_HAS_PERSISTENT_CLOCK_ALWAYS or something.

Hrm. Let me think on it for a bit, and feel free to suggest further 
improvements.

thanks
-john

Re: [PATCH v2 3/3] timekeeping: Add CONFIG_HAS_PERSISTENT_CLOCK option

[John Stultz]

On 01/22/2013 11:49 AM, John Stultz wrote:
> On 01/22/2013 11:44 AM, Jason Gunthorpe wrote:
>> On Tue, Jan 15, 2013 at 11:50:18AM -0800, John Stultz wrote:
>>> On 01/15/2013 08:09 AM, Feng Tang wrote:
>>>> Make the persistent clock check a kernel config option, so that some
>>>> platform can explicitely select it, also make CONFIG_RTC_HCTOSYS 
>>>> depends
>>>> on its non-existence, which could prevent the persistent clock and RTC
>>>> code from doing similar thing twice during system's 
>>>> init/suspend/resume
>>>> phases.
>>>>
>>>> If the CONFIG_HAS_PERSISTENT_CLOCK=n, then no change happens for 
>>>> kernel
>>>> which still does the persistent clock check in timekeeping_init().
>>>>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Suggested-by: John Stultz <john.stultz@linaro.org>
>>>> Signed-off-by: Feng Tang <feng.tang@intel.com>
>>> Applied. I also added a dependency for Jason's CONFIG_RTC_SYSTOHC.
>> Sort of an ugly config name, since I gather ARM should always set this
>> to 'n'...
>>
>> CONFIG_USE_ONLY_PERSISTENT_CLOCK ?
> (Sigh. I got this seemingly microseconds after I sent the pull request :)
>
> So yea, fair point, there could be some confusion. But 
> ONLY_PERSISTENT_CLOCK isn't quite right either,  more like 
> CONFIG_HAS_PERSISTENT_CLOCK_ALWAYS or something.
>

Decided upon CONFIG_ALWAYS_USE_PERSISTENT_CLOCK which I think is clear 
enough.

Let me know if you object or have a better idea.

thanks
-john

Re: [PATCH v2 1/3] timekeeping: Add persistent_clock_exist flag

[John Stultz]

On 01/15/2013 08:09 AM, Feng Tang wrote:
> In current kernel, there are several places which need to check
> whether there is a persistent clock for the platform. Current check
> is done by calling the read_persistent_clock() and validating its
> return value.
>
> So one optimization is to do the check only once in timekeeping_init(),
> and use a flag persistent_clock_exist to record it.
>
> v2: Add a has_persistent_clock() helper function, as suggested by John.

Applied! Thanks
-john