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

[Baolin Wang]

On 9 January 2018 at 19:16, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> 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

If we register one clocksource, the clocksource can only convert
maximum 10 minutes time considering a good conversion precision. But
10 minutes is not enough for suspend, we need one larger time can be
suspended. That is why I did not register the persistent clock as one
clocksource.

We can saw in arch/arm/plat-omap/counter_32k.c, the counter
implemented its mult and shift with the maximum time 120000 seconds,
instead of using the clocksource to read persistent clock.

> CLOCK_SOURCE_SUSPEND_NONSTOP set, maybe we can
> switch clock sources in timekeeping_suspend(), and back in
> timekeeping_resume()? Alternatively, we might keep the

I checked there are no APIs to switch one SUSPEND_NONSTOP clocksource
now, maybe we can implement them but we should solve the 10 minutes
conversion limitation for clocksource firstly.

> 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?

I think this is simple and good to solve our issue, but like I said
the conversion limitation of one clocksource is not enough for
persistent clock. Thanks for your suggestion.

-- 
Baolin.wang
Best Regards