Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755063AbeAJCxA (ORCPT + 1 other); Tue, 9 Jan 2018 21:53:00 -0500 Received: from mail-oi0-f66.google.com ([209.85.218.66]:40821 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754973AbeAJCw7 (ORCPT ); Tue, 9 Jan 2018 21:52:59 -0500 X-Google-Smtp-Source: ACJfBosdKPqED64lWxh/a41k9sJkvEcEXHxQffOv9SS6aMIfkTqsWEaVn4xNnTCI3qp0VCcYjSw5VbPZpsPViJfveRw= MIME-Version: 1.0 In-Reply-To: References: From: Baolin Wang Date: Wed, 10 Jan 2018 10:52:58 +0800 Message-ID: Subject: Re: [PATCH 1/2] clocksource: Add persistent timer support To: Arnd Bergmann Cc: Daniel Lezcano , Thomas Gleixner , Mark Brown , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 9 January 2018 at 19:16, Arnd Bergmann wrote: > On Tue, Jan 9, 2018 at 10:09 AM, Baolin Wang 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 > > 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