Re: [PATCH 1/2] clocksource: Add persistent timer support

[Arnd Bergmann]

On Tue, Jan 9, 2018 at 10:09 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
> On some platforms (such as Spreadtrum sc9860 platform), they need one
> persistent timer to calculate the suspended time and compensate it
> for the OS time.
>
> But now there are no method to register one persistent timer on some
> architectures (such as arm64), thus this patch adds one common framework
> for timer drivers to register one persistent timer and implements the
> read_persistent_clock64() to compensate the OS time.
>
> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>

Hi Baolin,

I like the idea of having a more generalized way of dealing with
persistent clocks,
but I wonder if we can do a little better than coming up with another
API that is
completely separated from the clocksource API.

The situation on sc9860 appears to be similar to what we have on OMAP:
there is one clockcource that is persistent, and another one that is for
some reason preferred on some systems as the system clock, but that
is not persistent.

On OMAP, we register drivers/clocksource/timer-ti-32k.c as a clocksource
with CLOCK_SOURCE_SUSPEND_NONSTOP set, and then also register
the same thing from arch/arm/plat-omap/counter_32k.c through the arm32
specific register_persistent_clock() interface, and then we also
register another clocksource on some chips that may be preferred.

In timekeeping_resume(), we fall back to read_persistent_clock64()
when the currently active clocksource doesn't have
CLOCK_SOURCE_SUSPEND_NONSTOP set, so that works fine with
both the OMAP requirement and your new code, but we might be
able to do better: If the persistent clock also gets registered as a
normal clocksource, and the primary clocksource doesn't have
CLOCK_SOURCE_SUSPEND_NONSTOP set, maybe we can
switch clock sources in timekeeping_suspend(), and back in
timekeeping_resume()? Alternatively, we might keep the
read_persistent_clock64() interface for your case, but then get
rid of persistent_timer_init_and_register() and make it a hook
inside of __clocksource_register_scale() that gets called for any
clocksource with CLOCK_SOURCE_SUSPEND_NONSTOP
set?

       Arnd