[PATCH 1/3] timekeeping: Add persistent_clock_exist flag

[PATCH 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 the
return value.

Add such a flag to make code more readable and call read_persistent_clock()
only once for all the checks.

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      |    1 +
 kernel/time/timekeeping.c |   15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..19388ad 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -115,6 +115,7 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 	return true;
 }
 
+extern int 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 4c7de02..b82f218 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -28,6 +28,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 */
+int __read_mostly persistent_clock_exist;
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -590,12 +593,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 = 1;
 
 	read_boot_clock(&boot);
 	if (!timespec_valid_strict(&boot)) {
@@ -670,9 +675,11 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	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 (persistent_clock_exist)
 		return;
 
 	write_seqlock_irqsave(&tk->lock, flags);
-- 
1.7.9.5

[PATCH 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 f8a0aab..bf4c6d3 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 (persistent_clock_exist)
+		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 (persistent_clock_exist)
+		return 0;
+
 	rtc_hctosys_ret = -ENODEV;
 	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
 		return 0;
-- 
1.7.9.5

[PATCH 3/3] rtc: Skip setting xtime if persisent clock exist

[Feng Tang]

As the timekeeping_init() already initialize the xtime with
read_persistent_clock().

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/hctosys.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
index 4aa60d7..2b539cd 100644
--- a/drivers/rtc/hctosys.c
+++ b/drivers/rtc/hctosys.c
@@ -52,6 +52,10 @@ static int __init rtc_hctosys(void)
 		goto err_invalid;
 	}
 
+	/* Skip setting xtime again if persistent clock exist */
+	if (persistent_clock_exist)
+		goto skip_timeset;
+
 	rtc_tm_to_time(&tm, &tv.tv_sec);
 
 	err = do_settimeofday(&tv);
@@ -65,6 +69,7 @@ static int __init rtc_hctosys(void)
 
 err_invalid:
 err_read:
+skip_timeset:
 	rtc_class_close(rtc);
 
 err_open:
-- 
1.7.9.5

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

[John Stultz]

On 12/12/2012 06:05 PM, 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 the
> return value.
>
> Add such a flag to make code more readable and call read_persistent_clock()
> only once for all the checks.
Sorry.. What  the actual benefit of this patch set?   (Usually with 
changelogs its better to explain why you're doing something, rather then 
just what you're doing.)

Maybe I'm missing something, but it seems this doesn't change the 
resulting logic of the code, does it?  As I thought we already check 
read_persistent_clocks() output (and make sure its null) before using 
the rtc HCTOSYS_DEVICE.

thanks
-john

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

[Feng Tang]

Hi John,

Thanks for the review.

On Thu, Dec 13, 2012 at 05:20:36PM -0800, John Stultz wrote:
> On 12/12/2012 06:05 PM, 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 the
> >return value.
> >
> >Add such a flag to make code more readable and call read_persistent_clock()
> >only once for all the checks.
> Sorry.. What  the actual benefit of this patch set?   (Usually with
> changelogs its better to explain why you're doing something, rather
> then just what you're doing.)

The main benefits is not bother to do the rtc_resume and rtc_suspend work
if persistent clock exists. Current RTC suspend/resume code will do many
time calculation and compensation work at first, and then call
timekeeping_inject_sleeptime() which will just return for platform with
persistent clock, what I did in this patchset is to put the check at
the start, also I save the persistent_clock_exist flag for all possible
check after timekeeping_init().

> 
> Maybe I'm missing something, but it seems this doesn't change the
> resulting logic of the code, does it?  As I thought we already check
> read_persistent_clocks() output (and make sure its null) before
> using the rtc HCTOSYS_DEVICE.

No, it doesn't change the code logic. 

Thanks,
Feng

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

[John Stultz]

On 12/13/2012 05:37 PM, Feng Tang wrote:
> On Thu, Dec 13, 2012 at 05:20:36PM -0800, John Stultz wrote:
>> On 12/12/2012 06:05 PM, 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 the
>>> return value.
>>>
>>> Add such a flag to make code more readable and call read_persistent_clock()
>>> only once for all the checks.
>> Sorry.. What  the actual benefit of this patch set?   (Usually with
>> changelogs its better to explain why you're doing something, rather
>> then just what you're doing.)
> The main benefits is not bother to do the rtc_resume and rtc_suspend work
> if persistent clock exists. Current RTC suspend/resume code will do many
> time calculation and compensation work at first, and then call
> timekeeping_inject_sleeptime() which will just return for platform with
> persistent clock, what I did in this patchset is to put the check at
> the start, also I save the persistent_clock_exist flag for all possible
> check after timekeeping_init().

CC'ing Jason as his recent patch is conceptually connected here.

Ok, Feng, so your patch set is a suspend/resume optimization for the 
case where the architecture has a read_persistent_clock() 
implementation, but the kernel config has also the rtc HCTOSYS_DEVICE 
set, right?

So we basically short-cut the rtc's HCTOSYS_DEVICE suspend/resume logic, 
likely to speed up suspend/resume.

So per Jason's related patch, he's made the point that the 
persistent_clock and RTC class functionality are basically exclusive 
(well, in his case, he said this with respect to updating the RTC, not 
reading it - I don't mean to put words in his mouth - Please do correct 
me here Jason. :).  In other words, we probably should avoid 
configurations where both the rtc hctosys and persistent_clock 
interfaces are both active.

So my thought here is that this same behavioral change could be made via 
Kconfig constraints rather then extra run-time conditionals. Basically 
we add a HAS_PERSISTENT_CLOCK, that architectures select if they want to 
use the read/update_persistent_clock calls. Then we make the HCTOSYS 
option be dependent on !HAS_PERSISTENT_CLOCK. This way we avoid having 
configs where there are conflicting paths that we chose from.

thanks
-john

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

[Feng Tang]

On Thu, Dec 13, 2012 at 06:00:23PM -0800, John Stultz wrote:
> On 12/13/2012 05:37 PM, Feng Tang wrote:
> >On Thu, Dec 13, 2012 at 05:20:36PM -0800, John Stultz wrote:
> >>On 12/12/2012 06:05 PM, 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 the
> >>>return value.
> >>>
> >>>Add such a flag to make code more readable and call read_persistent_clock()
> >>>only once for all the checks.
> >>Sorry.. What  the actual benefit of this patch set?   (Usually with
> >>changelogs its better to explain why you're doing something, rather
> >>then just what you're doing.)
> >The main benefits is not bother to do the rtc_resume and rtc_suspend work
> >if persistent clock exists. Current RTC suspend/resume code will do many
> >time calculation and compensation work at first, and then call
> >timekeeping_inject_sleeptime() which will just return for platform with
> >persistent clock, what I did in this patchset is to put the check at
> >the start, also I save the persistent_clock_exist flag for all possible
> >check after timekeeping_init().
> 
> CC'ing Jason as his recent patch is conceptually connected here.
> 
> Ok, Feng, so your patch set is a suspend/resume optimization for the
> case where the architecture has a read_persistent_clock()
> implementation, but the kernel config has also the rtc
> HCTOSYS_DEVICE set, right?

Exactly! Sorry for I didn't make it clear

> 
> So we basically short-cut the rtc's HCTOSYS_DEVICE suspend/resume
> logic, likely to speed up suspend/resume.
> 
> So per Jason's related patch, he's made the point that the
> persistent_clock and RTC class functionality are basically exclusive
> (well, in his case, he said this with respect to updating the RTC,
> not reading it - I don't mean to put words in his mouth - Please do
> correct me here Jason. :).  In other words, we probably should avoid
> configurations where both the rtc hctosys and persistent_clock
> interfaces are both active.

Yes, I agree these 2 should be exclusive.

> 
> So my thought here is that this same behavioral change could be made
> via Kconfig constraints rather then extra run-time conditionals.
> Basically we add a HAS_PERSISTENT_CLOCK, that architectures select
> if they want to use the read/update_persistent_clock calls. Then we
> make the HCTOSYS option be dependent on !HAS_PERSISTENT_CLOCK. This
> way we avoid having configs where there are conflicting paths that
> we chose from.

Sounds good to me, and we may need to add the HAS_PERSISTENT_CLOCK
to the default config of platforms who implemente the read_persistent_clock().

One concern is if a platform has read_persistent_clock capability, will
it also has the write_persistent_clock? The answer may be no  

Thanks,
Feng

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

[Jason Gunthorpe]

On Thu, Dec 13, 2012 at 06:00:23PM -0800, John Stultz wrote:

> So per Jason's related patch, he's made the point that the
> persistent_clock and RTC class functionality are basically exclusive
> (well, in his case, he said this with respect to updating the RTC,
> not reading it - I don't mean to put words in his mouth - Please do
> correct me here Jason. :).  In other words, we probably should avoid
> configurations where both the rtc hctosys and persistent_clock
> interfaces are both active.

I only studied update_persistent_clock, read_persistent_clock is
very much different.

Looking at it, I don't think that update_persistent_clock is in any
way related to read_persistent_clock..  update_persistent_clock is
*only* called by NTP, and its *only* purpose is to update the RTC with
NTP synchronized time. In many configurations it will never even be
called.

I think update_persistent_clock is badly named, it should be called
platform_save_ntp_time_to_rtc(), keep it divorced from
read_presistent_clock :)

> make the HCTOSYS option be dependent on !HAS_PERSISTENT_CLOCK. This
> way we avoid having configs where there are conflicting paths that
> we chose from.

On ARM the read_presistent_clock is used to access a true monotonic
counter that is divorced from the system RTC - look at
arch/arm/plat-omap/counter_32k.c for instance.

This seems like a great use of that hardware resource, and no doubt
those mach's also have a class RTC driver available talking to
different hardware.

For mach's without that functionality ARM returns a fixed 0 value
from read_persistent_clock, persumably the kernel detects this and
falls back to using class rtc functions?

Maybe Feng would be better off adjusting read_persistent_clock to
return ENODEV in such cases??

So, I think you have to keep your test as a run time test. To support
the single image ARM boot you can't make the distinction with kconfig.

Regards,
Jason

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

[Feng Tang]

On Thu, Dec 13, 2012 at 07:38:26PM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 13, 2012 at 06:00:23PM -0800, John Stultz wrote:
> 
> > So per Jason's related patch, he's made the point that the
> > persistent_clock and RTC class functionality are basically exclusive
> > (well, in his case, he said this with respect to updating the RTC,
> > not reading it - I don't mean to put words in his mouth - Please do
> > correct me here Jason. :).  In other words, we probably should avoid
> > configurations where both the rtc hctosys and persistent_clock
> > interfaces are both active.
> 
> I only studied update_persistent_clock, read_persistent_clock is
> very much different.
> 
> Looking at it, I don't think that update_persistent_clock is in any
> way related to read_persistent_clock..  update_persistent_clock is
> *only* called by NTP, and its *only* purpose is to update the RTC with
> NTP synchronized time. In many configurations it will never even be
> called.
> 
> I think update_persistent_clock is badly named, it should be called
> platform_save_ntp_time_to_rtc(), keep it divorced from
> read_presistent_clock :)
> 
> > make the HCTOSYS option be dependent on !HAS_PERSISTENT_CLOCK. This
> > way we avoid having configs where there are conflicting paths that
> > we chose from.
> 
> On ARM the read_presistent_clock is used to access a true monotonic
> counter that is divorced from the system RTC - look at
> arch/arm/plat-omap/counter_32k.c for instance.
> 
> This seems like a great use of that hardware resource, and no doubt
> those mach's also have a class RTC driver available talking to
> different hardware.

Interesting to know this, thanks for the info. For the x86 desktop
and mobile processors I've used, the read_persistent_clock and rtc
are the same on-board device (always power on), so I see many time
related code are execuated twice, like init/suspend/resume if
HCTOSYS config is enabled, that's why I came up with the patches.

> 
> For mach's without that functionality ARM returns a fixed 0 value
> from read_persistent_clock, persumably the kernel detects this and
> falls back to using class rtc functions?
> 
> Maybe Feng would be better off adjusting read_persistent_clock to
> return ENODEV in such cases??

For mach's without read_persistent_clock capability, there is already
a weakly defined 

	void __attribute__((weak)) read_persistent_clock(struct timespec *ts)
	{
		ts->tv_sec = 0;
		ts->tv_nsec = 0;
	}
so those machs can simply do nothing, and let time core code to judge it.

> 
> So, I think you have to keep your test as a run time test. To support
> the single image ARM boot you can't make the distinction with kconfig.

Good point. Figuring out the kconfig for all arm platforms is very
challenging.

Thanks,
Feng

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

[Jason Gunthorpe]

On Fri, Dec 14, 2012 at 11:13:30AM +0800, Feng Tang wrote:

> > This seems like a great use of that hardware resource, and no doubt
> > those mach's also have a class RTC driver available talking to
> > different hardware.
> 
> Interesting to know this, thanks for the info. For the x86 desktop
> and mobile processors I've used, the read_persistent_clock and rtc
> are the same on-board device (always power on), so I see many time
> related code are execuated twice, like init/suspend/resume if
> HCTOSYS config is enabled, that's why I came up with the patches.

Ah, I see, there is some duplication here, my earlier comments about
update_persistent_clock are not quite right, some places like PCs
stick a RTC driver and then continue to access the same hardware
directly outside the rtc driver context! That seems ugly :|

I see the PC CMOS rtc driver does not implement the set_mmss
operation, instead running that code through update_persistent_clock..
That seems like a cleanup waiting to happen.

Regarding your problem - IMHO, it would be fantastic if the class RTC
driver could be used instead of read_persistent_clock on PC.

John mentioned that read_persistent_clock had a requirement to work
with IRQs off - that seems like it would be easy to incorporate into
class rtc - for hardware that supports it (and PC is not the only RTC
HW that can do this) Is that the only reason it still exists on pc?

I have to feel the long term direction should be to remove
*_persistent_clock in favor of class RTC?

> > Maybe Feng would be better off adjusting read_persistent_clock to
> > return ENODEV in such cases??
> 
> For mach's without read_persistent_clock capability, there is already
> a weakly defined 

This is used for arch's without the functionality, mach's are arch
specific things. ARM provides a function pointer indirection for it's
read_persistent_clock implementation.

Jason

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

[John Stultz]

On 12/13/2012 08:10 PM, Jason Gunthorpe wrote:
>
> Ah, I see, there is some duplication here, my earlier comments about
> update_persistent_clock are not quite right, some places like PCs
> stick a RTC driver and then continue to access the same hardware
> directly outside the rtc driver context! That seems ugly :|
>
> I see the PC CMOS rtc driver does not implement the set_mmss
> operation, instead running that code through update_persistent_clock..
> That seems like a cleanup waiting to happen.
>
> Regarding your problem - IMHO, it would be fantastic if the class RTC
> driver could be used instead of read_persistent_clock on PC.
>
> John mentioned that read_persistent_clock had a requirement to work
> with IRQs off - that seems like it would be easy to incorporate into
> class rtc - for hardware that supports it (and PC is not the only RTC
> HW that can do this) Is that the only reason it still exists on pc?
>
> I have to feel the long term direction should be to remove
> *_persistent_clock in favor of class RTC?
So yes, I'd love to see a cleanup here.

Although from a timekeeping perspective, the read_persistent_clock() 
interface is actually *much* preferred over the rtc HCTOSYS device.

Since read_persistent_clock() has the requirement that its safe to call 
with IRQs disabled, we can use it in the timekeeping suspend/resume 
code, which allows for better time accuracy.

That said, many systems don't have a persistent clock that they can 
access with irqs off, and so the timekeeping_inject_sleeptime code was 
added to work with the rtc class drivers,  but it is more prone to problems.

That's why I've been suggesting we add a HAS_PERSISTENT_CLOCK and make 
the HCTOSYS_DEVICE depend on !HAS_PERSISTENT_CLOCK.  As it avoids these 
weird cases we're we have possibly duplicated code paths.

But there is the risk that some architectures may require both for some 
system configs (ie: read_persistent_clock works on all but one SoC, 
which has an rtc over a usb bus or something).  Although its not clear 
yet that this is a situation that actually exists.  (And we could work 
around it with something like HAS_EXCLUSIVE_PERSISTENT_CLOCK or something).

While we're suggesting cleanups, the RTC Kconfig choices probably need a 
cleanup too, as  the list of all possible drivers can be confusing, when 
usually each architecture has only a few that they exclusively support 
(I know there are exceptions to this).  It makes it hard to even figure 
out for a specific rtc driver what architecture one should look for in 
order to test with (I usually have to look at the commit log, and then 
try to associate email address with arch maintainers). A good number of 
drivers are fine, and already depend on the SoC platform support being 
there, but I suspect we could narrow a number of the drivers down to one 
or two arches or even platforms that acutally make use of it.

thanks
-john

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

[Jason Gunthorpe]

On Fri, Dec 14, 2012 at 01:22:50PM -0800, John Stultz wrote:

> Although from a timekeeping perspective, the read_persistent_clock()
> interface is actually *much* preferred over the rtc HCTOSYS device.
> 
> Since read_persistent_clock() has the requirement that its safe to
> call with IRQs disabled, we can use it in the timekeeping
> suspend/resume code, which allows for better time accuracy.

Sure, but my view on this is that it has nothing to do with
read_persistent_clock. If the RTC driver can run with IRQs off is a
property of the RTC driver and RTC hardware - it has nothing to do
with the platform. ARM platforms will vary on a machine by machine
basis. The rtc-mv driver used on my ARM system is perfectly
re-entrant, lots of rtc on SOC drivers will be the same.

If this is the only thing keeping you on read_persistent_clock, for
real RTCs, then how about a RTC_DEV_SAFE_READ flag (or whatever) in
rtc_device.flags?

Reserve read_persistent_clock for things like that very specialized
non-RTC ARM counter.

> 
> While we're suggesting cleanups, the RTC Kconfig choices probably
> need a cleanup too, as  the list of all possible drivers can be
> confusing, when usually each architecture has only a few that they

That is a general pain with the new 'everything is a driver'..
However device tree enabled stuff is much nicer, just grep the tree
for the compatible string and you'll find a big clue about what
physical boards are using that driver.

For instance:
                rtc@10300 {
                        compatible = "marvell,kirkwood-rtc", "marvell,orion-rtc";
                        reg = <0x10300 0x20>;
                        interrupts = <53>;
                };
is matched to:

drivers/rtc/rtc-mv.c:

static struct of_device_id rtc_mv_of_match_table[] = {
        { .compatible = "marvell,orion-rtc", },


> exclusively support (I know there are exceptions to this).  It makes
> it hard to even figure out for a specific rtc driver what
> architecture one should look for in order to test with (I usually

With anything embedded the answer is 'anything and everything'
unfortunately. For example, our board has two bits of RTC capable
hardware - one inside the CPU SOC and one outside. We went through a
big analysis to pick the best one, based on battery selection and
battery life time. Even the presence of a RTC block in a SOC is not a
guarentee it will be used universally.

Regards,
Jason

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

[John Stultz]

On 12/14/2012 01:56 PM, Jason Gunthorpe wrote:
> On Fri, Dec 14, 2012 at 01:22:50PM -0800, John Stultz wrote:
>
>> Although from a timekeeping perspective, the read_persistent_clock()
>> interface is actually *much* preferred over the rtc HCTOSYS device.
>>
>> Since read_persistent_clock() has the requirement that its safe to
>> call with IRQs disabled, we can use it in the timekeeping
>> suspend/resume code, which allows for better time accuracy.
> Sure, but my view on this is that it has nothing to do with
> read_persistent_clock. If the RTC driver can run with IRQs off is a
> property of the RTC driver and RTC hardware - it has nothing to do
> with the platform. ARM platforms will vary on a machine by machine
> basis. The rtc-mv driver used on my ARM system is perfectly
> re-entrant, lots of rtc on SOC drivers will be the same.
>
> If this is the only thing keeping you on read_persistent_clock, for
> real RTCs, then how about a RTC_DEV_SAFE_READ flag (or whatever) in
> rtc_device.flags?
>
> Reserve read_persistent_clock for things like that very specialized
> non-RTC ARM counter.
Something like this could work, although I worry it only causes even 
more code paths:
1) read_persistent_clock for non-RTC counters, called from 
timekeeping_suspend/resume
2) IRQ safe RTC called from timekeeping_suspend/resume
3) Non-IRQ safe RTC suspend/resume logic


>> While we're suggesting cleanups, the RTC Kconfig choices probably
>> need a cleanup too, as  the list of all possible drivers can be
>> confusing, when usually each architecture has only a few that they
> That is a general pain with the new 'everything is a driver'..
True, and maybe something I just have to live with.  But I can still 
make holiday wish lists :)

thanks
-john

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

[Feng Tang]

On Fri, Dec 14, 2012 at 02:56:51PM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 14, 2012 at 01:22:50PM -0800, John Stultz wrote:
> 
> > Although from a timekeeping perspective, the read_persistent_clock()
> > interface is actually *much* preferred over the rtc HCTOSYS device.
> > 
> > Since read_persistent_clock() has the requirement that its safe to
> > call with IRQs disabled, we can use it in the timekeeping
> > suspend/resume code, which allows for better time accuracy.
> 
> Sure, but my view on this is that it has nothing to do with
> read_persistent_clock. If the RTC driver can run with IRQs off is a
> property of the RTC driver and RTC hardware - it has nothing to do
> with the platform. ARM platforms will vary on a machine by machine
> basis. The rtc-mv driver used on my ARM system is perfectly
> re-entrant, lots of rtc on SOC drivers will be the same.
> 
> If this is the only thing keeping you on read_persistent_clock, for
> real RTCs, then how about a RTC_DEV_SAFE_READ flag (or whatever) in
> rtc_device.flags?
> 
> Reserve read_persistent_clock for things like that very specialized
> non-RTC ARM counter.

Yes, these non-RTC device is one reason for keeping read_persistent_clock,
one other reason I can think of is the CONFIG_RTC_LIB is not always on by
default for all Archs, and some platforms may chose to disable it on purpose. 
When CONFIG_RTC_LIB is not set, we need the read_persistent_clock for
time init/suspend/resume.

Thanks,
Feng

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

[Jason Gunthorpe]

On Tue, Dec 18, 2012 at 12:14:33AM +0800, Feng Tang wrote:

> > Sure, but my view on this is that it has nothing to do with
> > read_persistent_clock. If the RTC driver can run with IRQs off is a
> > property of the RTC driver and RTC hardware - it has nothing to do
> > with the platform. ARM platforms will vary on a machine by machine
> > basis. The rtc-mv driver used on my ARM system is perfectly
> > re-entrant, lots of rtc on SOC drivers will be the same.
> > 
> > If this is the only thing keeping you on read_persistent_clock, for
> > real RTCs, then how about a RTC_DEV_SAFE_READ flag (or whatever) in
> > rtc_device.flags?
> > 
> > Reserve read_persistent_clock for things like that very specialized
> > non-RTC ARM counter.
> 
> Yes, these non-RTC device is one reason for keeping read_persistent_clock,
> one other reason I can think of is the CONFIG_RTC_LIB is not always on by
> default for all Archs, and some platforms may chose to disable it on purpose. 
> When CONFIG_RTC_LIB is not set, we need the read_persistent_clock for
> time init/suspend/resume.

I thought your concern was the case where the RTC was turned on and
read_persistent_clock was also turned on. Having a flag in the RTC and
disabling read_persistent_clock for that situation would help you
avoid the double code path to the same hardware.

What is motivating having a RTC but not using RTC lib? Embedded
doesn't seem to the be the case, nearly all the interesting rtcs are
under class rtc....

Jason

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

[Feng Tang]

On Mon, Dec 17, 2012 at 11:22:02AM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 18, 2012 at 12:14:33AM +0800, Feng Tang wrote:
> 
> > > Sure, but my view on this is that it has nothing to do with
> > > read_persistent_clock. If the RTC driver can run with IRQs off is a
> > > property of the RTC driver and RTC hardware - it has nothing to do
> > > with the platform. ARM platforms will vary on a machine by machine
> > > basis. The rtc-mv driver used on my ARM system is perfectly
> > > re-entrant, lots of rtc on SOC drivers will be the same.
> > > 
> > > If this is the only thing keeping you on read_persistent_clock, for
> > > real RTCs, then how about a RTC_DEV_SAFE_READ flag (or whatever) in
> > > rtc_device.flags?
> > > 
> > > Reserve read_persistent_clock for things like that very specialized
> > > non-RTC ARM counter.
> > 
> > Yes, these non-RTC device is one reason for keeping read_persistent_clock,
> > one other reason I can think of is the CONFIG_RTC_LIB is not always on by
> > default for all Archs, and some platforms may chose to disable it on purpose. 
> > When CONFIG_RTC_LIB is not set, we need the read_persistent_clock for
> > time init/suspend/resume.
> 
> I thought your concern was the case where the RTC was turned on and
> read_persistent_clock was also turned on. Having a flag in the RTC and
> disabling read_persistent_clock for that situation would help you
> avoid the double code path to the same hardware.

No, it's not about my concern (actually I don't have concerns :), my 3
patches are just some optimization).

My point is the "driver/rtc" or "drivers/rtc/class.c" can't be ganrateeded
to be built for all kernels, myself saw several cases, under which
the read_persistent_clock is still needed.

If you want drivers/rtc code to take the priority, first thing you have
to do is to make those codes always "obj-y=xxx.o"

Thanks,
Feng

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

[John Stultz]

On 12/13/2012 06:38 PM, Jason Gunthorpe wrote:
> On Thu, Dec 13, 2012 at 06:00:23PM -0800, John Stultz wrote:
>
>> So per Jason's related patch, he's made the point that the
>> persistent_clock and RTC class functionality are basically exclusive
>> (well, in his case, he said this with respect to updating the RTC,
>> not reading it - I don't mean to put words in his mouth - Please do
>> correct me here Jason. :).  In other words, we probably should avoid
>> configurations where both the rtc hctosys and persistent_clock
>> interfaces are both active.
> I only studied update_persistent_clock, read_persistent_clock is
> very much different.
>
> Looking at it, I don't think that update_persistent_clock is in any
> way related to read_persistent_clock..  update_persistent_clock is
> *only* called by NTP, and its *only* purpose is to update the RTC with
> NTP synchronized time. In many configurations it will never even be
> called.
>
> I think update_persistent_clock is badly named, it should be called
> platform_save_ntp_time_to_rtc(), keep it divorced from
> read_presistent_clock :)
Fair enough.  I think you've done a decent job convincing me that for 
the update path converting everything to using the RTC class exclusively 
is the better approach. That way we avoid the duplication on the write 
at least.

The read side, I suspect we can't eliminate the duplication, but we can 
likely configure it out for most cases.

>> make the HCTOSYS option be dependent on !HAS_PERSISTENT_CLOCK. This
>> way we avoid having configs where there are conflicting paths that
>> we chose from.
> On ARM the read_presistent_clock is used to access a true monotonic
> counter that is divorced from the system RTC - look at
> arch/arm/plat-omap/counter_32k.c for instance.
Yea, the 32k counter in that case is an interesting one (and a good 
example of wanting both a read_persistent_clock and a rtc). It doesn't 
exactly implement what read_persistent_clock() intends, but gets away 
with it, since on the suspend/resume we only take a delta from the 
read_persistent_clock(), we don't actually care if its accurate time 
wise.  It works well enough for suspend/resume and is nicely more 
accurate then the RTC.

Although this does cause duplication on the initialization side, where 
we probably set the time initially to the wrong time using the 32k 
counter, and then follow up with the HCTOSYS_DEVICE to get the correct 
time (and there's the extra overhead of the RTC suspend/resume path 
calculating and calling timekeeping_inject_sleeptime, but since 
read_persistent_clock is there, we throw that less accurate data out).


> This seems like a great use of that hardware resource, and no doubt
> those mach's also have a class RTC driver available talking to
> different hardware.
>
> For mach's without that functionality ARM returns a fixed 0 value
> from read_persistent_clock, persumably the kernel detects this and
> falls back to using class rtc functions?
>
> Maybe Feng would be better off adjusting read_persistent_clock to
> return ENODEV in such cases??
Although he wants to avoid even doing any read/calculation in the RTC 
suspend/resume path if read_persistent_clock is there. So probably some 
sort of a flag interface will be needed in that case.

> So, I think you have to keep your test as a run time test. To support
> the single image ARM boot you can't make the distinction with kconfig.
So yea, I agree there are some cases where both are necessary, but I 
also think that in *most* cases we can optimize out the choice at 
compile time, which is even better then having even the improved runtime 
conditional checking.

thanks
-john

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

[Feng Tang]

Hi Jason,

On Thu, Dec 13, 2012 at 07:38:26PM -0700, Jason Gunthorpe wrote:
> 
> > make the HCTOSYS option be dependent on !HAS_PERSISTENT_CLOCK. This
> > way we avoid having configs where there are conflicting paths that
> > we chose from.
> 
> On ARM the read_presistent_clock is used to access a true monotonic
> counter that is divorced from the system RTC - look at
> arch/arm/plat-omap/counter_32k.c for instance.
> 
> This seems like a great use of that hardware resource, and no doubt
> those mach's also have a class RTC driver available talking to
> different hardware.
> 
> For mach's without that functionality ARM returns a fixed 0 value
> from read_persistent_clock, persumably the kernel detects this and
> falls back to using class rtc functions?

I read the counter_32k.c and the read_persistent_clock() magic for
ARM, and got a question: for the omap platforms using the
counter_32k.c's read_persistent_clock, why will they also calles
the time service from RTC device like the rtc_suspend/resume? I 
thought usually we just need to use the time source with the better
accuracy.

Thanks,
Feng

> 
> Maybe Feng would be better off adjusting read_persistent_clock to
> return ENODEV in such cases??
> 
> So, I think you have to keep your test as a run time test. To support
> the single image ARM boot you can't make the distinction with kconfig.
> 
> Regards,
> Jason