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

[Feng Tang <feng.tang@intel.com>]

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