[PATCH] clocksource: register persistent clock for arm arch_timer

[PATCH] clocksource: register persistent clock for arm arch_timer

[Lei Wen]

Since arm's arch_timer's counter would keep accumulated even in the
low power mode, including suspend state, it is very suitable to be
the persistent clock instead of RTC.

While read_persistent_clock calling place shall be rare, like only
suspend/resume place? So we shall don't care for its performance
very much, so use direclty divided by frequency should be accepted
for this reason. Actually archtimer's counter read performance already
be very good, since it is directly access from core's bus, not from
soc, so this is another reason why we choose use divide here.

Final reason for why we don't use multi+shift way is for we may not
call read_persistent_clock for long time, like system long time
not enter into suspend, so that the accumulated cycle difference value
may larger than we used for calculate the multi+shift, thus precise
would be highly affected in such corner case.

Signed-off-by: Lei Wen <leiwen@marvell.com>
---

I am not sure whether it is good to add something like
generic_persistent_clock_read in the new added kernel/time/sched_clock.c?
Since from arch timer's perspective, all it need to do is to pick
the suspend period from the place where sched_clock being stopped/restarted.

Any idea for make the persistent clock reading as one generic function,
like current sched_clock do?


 drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 57e823c..5eaa34a 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -23,6 +23,7 @@
 #include <linux/sched_clock.h>
 
 #include <asm/arch_timer.h>
+#include <asm/mach/time.h>
 #include <asm/virt.h>
 
 #include <clocksource/arm_arch_timer.h>
@@ -52,6 +53,8 @@ struct arch_timer {
 #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
 
 static u32 arch_timer_rate;
+static u32 persistent_multi;
+static u32 persistent_div;
 
 enum ppi_nr {
 	PHYS_SECURE_PPI,
@@ -68,6 +71,31 @@ static struct clock_event_device __percpu *arch_timer_evt;
 static bool arch_timer_use_virtual = true;
 static bool arch_timer_mem_use_virtual;
 
+static void get_persistent_clock_multi_div(void)
+{
+	u32 i, tmp;
+
+	tmp = arch_timer_rate;
+	persistent_multi = NSEC_PER_SEC;
+	for (i = 0; i < 9; i ++) {
+		if (tmp % 10)
+			break;
+		tmp /= 10;
+		persistent_multi /= 10;
+	}
+
+	persistent_div = tmp;
+}
+
+static void arch_timer_persistent_clock_read(struct timespec *ts)
+{
+	u64 ns;
+
+	ns = arch_timer_read_counter() * persistent_multi;
+	do_div(ns, persistent_div);
+	*ts = ns_to_timespec(ns);
+}
+
 /*
  * Architected system timer support.
  */
@@ -631,6 +659,9 @@ static void __init arch_timer_common_init(void)
 	arch_timer_banner(arch_timers_present);
 	arch_counter_register(arch_timers_present);
 	arch_timer_arch_init();
+
+	get_persistent_clock_multi_div();
+	register_persistent_clock(NULL, arch_timer_persistent_clock_read);
 }
 
 static void __init arch_timer_init(struct device_node *np)
-- 
1.8.3.2

Re: [PATCH] clocksource: register persistent clock for arm arch_timer

[Stephen Boyd]

On 04/02/14 04:02, Lei Wen wrote:
> Since arm's arch_timer's counter would keep accumulated even in the
> low power mode, including suspend state, it is very suitable to be
> the persistent clock instead of RTC.
>
> While read_persistent_clock calling place shall be rare, like only
> suspend/resume place? So we shall don't care for its performance
> very much, so use direclty divided by frequency should be accepted
> for this reason. Actually archtimer's counter read performance already
> be very good, since it is directly access from core's bus, not from
> soc, so this is another reason why we choose use divide here.
>
> Final reason for why we don't use multi+shift way is for we may not
> call read_persistent_clock for long time, like system long time
> not enter into suspend, so that the accumulated cycle difference value
> may larger than we used for calculate the multi+shift, thus precise
> would be highly affected in such corner case.
>
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
>
> I am not sure whether it is good to add something like
> generic_persistent_clock_read in the new added kernel/time/sched_clock.c?
> Since from arch timer's perspective, all it need to do is to pick
> the suspend period from the place where sched_clock being stopped/restarted.
>
> Any idea for make the persistent clock reading as one generic function,
> like current sched_clock do?

Why do we need this? Don't we put the CLOCK_SOURCE_SUSPEND_NONSTOP flag
on the arm_arch_timer clocksource to handle this? The only reason I can
think of would be that you're calling read_persistent_clock() from
somewhere else besides the timekeeping core. If that's why, please use
the time functionality like ktime_get_boottime() or
get_monotonic_boottime().

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] clocksource: register persistent clock for arm arch_timer

[Lei Wen]

Hi Stephen,

On Thu, Apr 3, 2014 at 2:09 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 04/02/14 04:02, Lei Wen wrote:
>> Since arm's arch_timer's counter would keep accumulated even in the
>> low power mode, including suspend state, it is very suitable to be
>> the persistent clock instead of RTC.
>>
>> While read_persistent_clock calling place shall be rare, like only
>> suspend/resume place? So we shall don't care for its performance
>> very much, so use direclty divided by frequency should be accepted
>> for this reason. Actually archtimer's counter read performance already
>> be very good, since it is directly access from core's bus, not from
>> soc, so this is another reason why we choose use divide here.
>>
>> Final reason for why we don't use multi+shift way is for we may not
>> call read_persistent_clock for long time, like system long time
>> not enter into suspend, so that the accumulated cycle difference value
>> may larger than we used for calculate the multi+shift, thus precise
>> would be highly affected in such corner case.
>>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> ---
>>
>> I am not sure whether it is good to add something like
>> generic_persistent_clock_read in the new added kernel/time/sched_clock.c?
>> Since from arch timer's perspective, all it need to do is to pick
>> the suspend period from the place where sched_clock being stopped/restarted.
>>
>> Any idea for make the persistent clock reading as one generic function,
>> like current sched_clock do?
>
> Why do we need this? Don't we put the CLOCK_SOURCE_SUSPEND_NONSTOP flag
> on the arm_arch_timer clocksource to handle this? The only reason I can
> think of would be that you're calling read_persistent_clock() from
> somewhere else besides the timekeeping core. If that's why, please use
> the time functionality like ktime_get_boottime() or
> get_monotonic_boottime().

You are right for the CLOCK_SOURCE_SUSPEND_NONSTOP flag.
I haven't noticed that latest kernel already has such patch for arch_timer,
so provide additional such implementation shall no need then.

Another question may not related with this patch:
After 3.10, seem sched_clock also would get suspend by default during
suspend, and thus printk message's timestamp would get freezed
during the suspend, and later resume print message appears suspend
never happen, for the sched_clock already deduce the suspend period.

It makes the kernel log hard to align with other cores' generated message.
For other cores, I mean like communication processor.
So it make the debug process painful...
Any idea to solve it? Replace the local_clock to ktime_get_boottime in printk.c?

Thanks,
Lei