Message-ID: <50CB98AA.5090600@linaro.org>
Date:	Fri, 14 Dec 2012 13:22:50 -0800
From:	John Stultz <john.stultz@...aro.org>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
CC:	Feng Tang <feng.tang@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	linux-kernel@...r.kernel.org, alek.du@...el.com
Subject: Re: [PATCH 1/3] timekeeping: Add persistent_clock_exist flag

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists