[PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Anatol Pomozov]

ARM timekeeping functionality allows to register persistent/boot clock dynamically.
This code is arch-independent and can be useful on other plaforms as well.

As a byproduct of this change, tegra20_timer becomes ARM64 compatible.

Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
board, made sure high-resolution clock works.

Signed-off-by: Anatol Pomozov <anatol.pomozov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/include/asm/mach/time.h    |  5 -----
 arch/arm/kernel/time.c              | 36 -------------------------------
 arch/arm/plat-omap/counter_32k.c    |  9 +++++---
 drivers/clocksource/tegra20_timer.c | 10 +++++----
 include/linux/timekeeping.h         | 11 ++++++++++
 kernel/time/timekeeping.c           | 43 +++++++++++++++++++++++++++++++++----
 6 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/arch/arm/include/asm/mach/time.h b/arch/arm/include/asm/mach/time.h
index 90c12e1..3cbcafc 100644
--- a/arch/arm/include/asm/mach/time.h
+++ b/arch/arm/include/asm/mach/time.h
@@ -12,9 +12,4 @@
 
 extern void timer_tick(void);
 
-struct timespec;
-typedef void (*clock_access_fn)(struct timespec *);
-extern int register_persistent_clock(clock_access_fn read_boot,
-				     clock_access_fn read_persistent);
-
 #endif
diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index 0cc7e58..0aa1dcd 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -76,42 +76,6 @@ void timer_tick(void)
 }
 #endif
 
-static void dummy_clock_access(struct timespec *ts)
-{
-	ts->tv_sec = 0;
-	ts->tv_nsec = 0;
-}
-
-static clock_access_fn __read_persistent_clock = dummy_clock_access;
-static clock_access_fn __read_boot_clock = dummy_clock_access;;
-
-void read_persistent_clock(struct timespec *ts)
-{
-	__read_persistent_clock(ts);
-}
-
-void read_boot_clock(struct timespec *ts)
-{
-	__read_boot_clock(ts);
-}
-
-int __init register_persistent_clock(clock_access_fn read_boot,
-				     clock_access_fn read_persistent)
-{
-	/* Only allow the clockaccess functions to be registered once */
-	if (__read_persistent_clock == dummy_clock_access &&
-	    __read_boot_clock == dummy_clock_access) {
-		if (read_boot)
-			__read_boot_clock = read_boot;
-		if (read_persistent)
-			__read_persistent_clock = read_persistent;
-
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
 void __init time_init(void)
 {
 	if (machine_desc->init_time) {
diff --git a/arch/arm/plat-omap/counter_32k.c b/arch/arm/plat-omap/counter_32k.c
index 61b4d70..0dbfffd 100644
--- a/arch/arm/plat-omap/counter_32k.c
+++ b/arch/arm/plat-omap/counter_32k.c
@@ -18,10 +18,9 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/clocksource.h>
+#include <linux/time.h>
 #include <linux/sched_clock.h>
 
-#include <asm/mach/time.h>
-
 #include <plat/counter-32k.h>
 
 /* OMAP2_32KSYNCNT_CR_OFF: offset of 32ksync counter register */
@@ -76,6 +75,10 @@ static void omap_read_persistent_clock(struct timespec *ts)
 	spin_unlock_irqrestore(&read_persistent_clock_lock, flags);
 }
 
+static const struct persistent_clock_ops omap_persistent_clock = {
+	.read = omap_read_persistent_clock
+};
+
 /**
  * omap_init_clocksource_32k - setup and register counter 32k as a
  * kernel clocksource
@@ -116,7 +119,7 @@ int __init omap_init_clocksource_32k(void __iomem *vbase)
 	}
 
 	sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
-	register_persistent_clock(NULL, omap_read_persistent_clock);
+	register_persistent_clock(&omap_persistent_clock);
 	pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n");
 
 	return 0;
diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index d2616ef..210130e 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -28,9 +28,7 @@
 #include <linux/of_irq.h>
 #include <linux/sched_clock.h>
 #include <linux/delay.h>
-
-#include <asm/mach/time.h>
-#include <asm/smp_twd.h>
+#include <linux/timekeeping.h>
 
 #define RTC_SECONDS            0x08
 #define RTC_SHADOW_SECONDS     0x0c
@@ -142,6 +140,10 @@ static void tegra_read_persistent_clock(struct timespec *ts)
 	*ts = *tsp;
 }
 
+static const struct persistent_clock_ops tegra_persistent_clock = {
+	.read = tegra_read_persistent_clock
+};
+
 static unsigned long tegra_delay_timer_read_counter_long(void)
 {
 	return readl(timer_reg_base + TIMERUS_CNTR_1US);
@@ -252,7 +254,7 @@ static void __init tegra20_init_rtc(struct device_node *np)
 	else
 		clk_prepare_enable(clk);
 
-	register_persistent_clock(NULL, tegra_read_persistent_clock);
+	register_persistent_clock(&tegra_persistent_clock);
 }
 CLOCKSOURCE_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
 
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 1caa6b0..02023f7 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -201,6 +201,17 @@ static inline bool has_persistent_clock(void)
 	return persistent_clock_exist;
 }
 
+struct persistent_clock_ops {
+	void (*read)(struct timespec *ts);
+	int (*update)(const struct timespec ts);
+};
+
+struct boot_clock_ops {
+	void (*read)(struct timespec *ts);
+};
+
+extern void register_persistent_clock(const struct persistent_clock_ops *clock);
+extern void register_boot_clock(const struct boot_clock_ops *clock);
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
 extern int update_persistent_clock(struct timespec now);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index fb4a9c2..414c172 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -66,6 +66,9 @@ int __read_mostly timekeeping_suspended;
 /* Flag for if there is a persistent clock on this platform */
 bool __read_mostly persistent_clock_exist = false;
 
+const struct persistent_clock_ops __read_mostly *persistent_clock = NULL;
+const struct boot_clock_ops __read_mostly *boot_clock = NULL;
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->tkr.xtime_nsec >= ((u64)NSEC_PER_SEC << tk->tkr.shift)) {
@@ -956,6 +959,30 @@ u64 timekeeping_max_deferment(void)
 	return ret;
 }
 
+extern void register_persistent_clock(const struct persistent_clock_ops *clock)
+{
+	BUG_ON(!clock);
+	BUG_ON(!clock->read);
+
+	if (persistent_clock) {
+		pr_warn("Ignore extra persistent clock registration");
+		return;
+	}
+	persistent_clock = clock;
+}
+
+extern void register_boot_clock(const struct boot_clock_ops *clock)
+{
+	BUG_ON(!clock);
+	BUG_ON(!clock->read);
+
+	if (boot_clock) {
+		pr_warn("Ignore extra boot clock registration");
+		return;
+	}
+	boot_clock = clock;
+}
+
 /**
  * read_persistent_clock -  Return time from the persistent clock.
  *
@@ -967,8 +994,12 @@ u64 timekeeping_max_deferment(void)
  */
 void __weak read_persistent_clock(struct timespec *ts)
 {
-	ts->tv_sec = 0;
-	ts->tv_nsec = 0;
+	if (persistent_clock) {
+		persistent_clock->read(ts);
+	} else {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+	}
 }
 
 /**
@@ -982,8 +1013,12 @@ void __weak read_persistent_clock(struct timespec *ts)
  */
 void __weak read_boot_clock(struct timespec *ts)
 {
-	ts->tv_sec = 0;
-	ts->tv_nsec = 0;
+	if (boot_clock) {
+		boot_clock->read(ts);
+	} else {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+	}
 }
 
 /*
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Anatol Pomozov]

Hi

This patch opens possibility for further timekeeping cleanup:
read_persistent_clock() is defined as a weak and expected that
architecture implement it. The users of this function need to check
return value. If it is equal zero then persistent clock is not
provided. It looks hacky. It makes code nicer if architecture called

register_persistent_clock(&my_persistent_clock);

and then users check if persistent_clock variable is non-NULL.

On Fri, Nov 7, 2014 at 11:34 AM, Anatol Pomozov
<anatol.pomozov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
> This code is arch-independent and can be useful on other plaforms as well.
>
> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>
> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
> board, made sure high-resolution clock works.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/include/asm/mach/time.h    |  5 -----
>  arch/arm/kernel/time.c              | 36 -------------------------------
>  arch/arm/plat-omap/counter_32k.c    |  9 +++++---
>  drivers/clocksource/tegra20_timer.c | 10 +++++----
>  include/linux/timekeeping.h         | 11 ++++++++++
>  kernel/time/timekeeping.c           | 43 +++++++++++++++++++++++++++++++++----
>  6 files changed, 62 insertions(+), 52 deletions(-)
>
> diff --git a/arch/arm/include/asm/mach/time.h b/arch/arm/include/asm/mach/time.h
> index 90c12e1..3cbcafc 100644
> --- a/arch/arm/include/asm/mach/time.h
> +++ b/arch/arm/include/asm/mach/time.h
> @@ -12,9 +12,4 @@
>
>  extern void timer_tick(void);
>
> -struct timespec;
> -typedef void (*clock_access_fn)(struct timespec *);
> -extern int register_persistent_clock(clock_access_fn read_boot,
> -                                    clock_access_fn read_persistent);
> -
>  #endif
> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
> index 0cc7e58..0aa1dcd 100644
> --- a/arch/arm/kernel/time.c
> +++ b/arch/arm/kernel/time.c
> @@ -76,42 +76,6 @@ void timer_tick(void)
>  }
>  #endif
>
> -static void dummy_clock_access(struct timespec *ts)
> -{
> -       ts->tv_sec = 0;
> -       ts->tv_nsec = 0;
> -}
> -
> -static clock_access_fn __read_persistent_clock = dummy_clock_access;
> -static clock_access_fn __read_boot_clock = dummy_clock_access;;
> -
> -void read_persistent_clock(struct timespec *ts)
> -{
> -       __read_persistent_clock(ts);
> -}
> -
> -void read_boot_clock(struct timespec *ts)
> -{
> -       __read_boot_clock(ts);
> -}
> -
> -int __init register_persistent_clock(clock_access_fn read_boot,
> -                                    clock_access_fn read_persistent)
> -{
> -       /* Only allow the clockaccess functions to be registered once */
> -       if (__read_persistent_clock == dummy_clock_access &&
> -           __read_boot_clock == dummy_clock_access) {
> -               if (read_boot)
> -                       __read_boot_clock = read_boot;
> -               if (read_persistent)
> -                       __read_persistent_clock = read_persistent;
> -
> -               return 0;
> -       }
> -
> -       return -EINVAL;
> -}
> -
>  void __init time_init(void)
>  {
>         if (machine_desc->init_time) {
> diff --git a/arch/arm/plat-omap/counter_32k.c b/arch/arm/plat-omap/counter_32k.c
> index 61b4d70..0dbfffd 100644
> --- a/arch/arm/plat-omap/counter_32k.c
> +++ b/arch/arm/plat-omap/counter_32k.c
> @@ -18,10 +18,9 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/clocksource.h>
> +#include <linux/time.h>
>  #include <linux/sched_clock.h>
>
> -#include <asm/mach/time.h>
> -
>  #include <plat/counter-32k.h>
>
>  /* OMAP2_32KSYNCNT_CR_OFF: offset of 32ksync counter register */
> @@ -76,6 +75,10 @@ static void omap_read_persistent_clock(struct timespec *ts)
>         spin_unlock_irqrestore(&read_persistent_clock_lock, flags);
>  }
>
> +static const struct persistent_clock_ops omap_persistent_clock = {
> +       .read = omap_read_persistent_clock
> +};
> +
>  /**
>   * omap_init_clocksource_32k - setup and register counter 32k as a
>   * kernel clocksource
> @@ -116,7 +119,7 @@ int __init omap_init_clocksource_32k(void __iomem *vbase)
>         }
>
>         sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
> -       register_persistent_clock(NULL, omap_read_persistent_clock);
> +       register_persistent_clock(&omap_persistent_clock);
>         pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n");
>
>         return 0;
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index d2616ef..210130e 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -28,9 +28,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/sched_clock.h>
>  #include <linux/delay.h>
> -
> -#include <asm/mach/time.h>
> -#include <asm/smp_twd.h>
> +#include <linux/timekeeping.h>
>
>  #define RTC_SECONDS            0x08
>  #define RTC_SHADOW_SECONDS     0x0c
> @@ -142,6 +140,10 @@ static void tegra_read_persistent_clock(struct timespec *ts)
>         *ts = *tsp;
>  }
>
> +static const struct persistent_clock_ops tegra_persistent_clock = {
> +       .read = tegra_read_persistent_clock
> +};
> +
>  static unsigned long tegra_delay_timer_read_counter_long(void)
>  {
>         return readl(timer_reg_base + TIMERUS_CNTR_1US);
> @@ -252,7 +254,7 @@ static void __init tegra20_init_rtc(struct device_node *np)
>         else
>                 clk_prepare_enable(clk);
>
> -       register_persistent_clock(NULL, tegra_read_persistent_clock);
> +       register_persistent_clock(&tegra_persistent_clock);
>  }
>  CLOCKSOURCE_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 1caa6b0..02023f7 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -201,6 +201,17 @@ static inline bool has_persistent_clock(void)
>         return persistent_clock_exist;
>  }
>
> +struct persistent_clock_ops {
> +       void (*read)(struct timespec *ts);
> +       int (*update)(const struct timespec ts);
> +};
> +
> +struct boot_clock_ops {
> +       void (*read)(struct timespec *ts);
> +};
> +
> +extern void register_persistent_clock(const struct persistent_clock_ops *clock);
> +extern void register_boot_clock(const struct boot_clock_ops *clock);
>  extern void read_persistent_clock(struct timespec *ts);
>  extern void read_boot_clock(struct timespec *ts);
>  extern int update_persistent_clock(struct timespec now);
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index fb4a9c2..414c172 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -66,6 +66,9 @@ int __read_mostly timekeeping_suspended;
>  /* Flag for if there is a persistent clock on this platform */
>  bool __read_mostly persistent_clock_exist = false;
>
> +const struct persistent_clock_ops __read_mostly *persistent_clock = NULL;
> +const struct boot_clock_ops __read_mostly *boot_clock = NULL;
> +
>  static inline void tk_normalize_xtime(struct timekeeper *tk)
>  {
>         while (tk->tkr.xtime_nsec >= ((u64)NSEC_PER_SEC << tk->tkr.shift)) {
> @@ -956,6 +959,30 @@ u64 timekeeping_max_deferment(void)
>         return ret;
>  }
>
> +extern void register_persistent_clock(const struct persistent_clock_ops *clock)
> +{
> +       BUG_ON(!clock);
> +       BUG_ON(!clock->read);
> +
> +       if (persistent_clock) {
> +               pr_warn("Ignore extra persistent clock registration");
> +               return;
> +       }
> +       persistent_clock = clock;
> +}
> +
> +extern void register_boot_clock(const struct boot_clock_ops *clock)
> +{
> +       BUG_ON(!clock);
> +       BUG_ON(!clock->read);
> +
> +       if (boot_clock) {
> +               pr_warn("Ignore extra boot clock registration");
> +               return;
> +       }
> +       boot_clock = clock;
> +}
> +
>  /**
>   * read_persistent_clock -  Return time from the persistent clock.
>   *
> @@ -967,8 +994,12 @@ u64 timekeeping_max_deferment(void)
>   */
>  void __weak read_persistent_clock(struct timespec *ts)
>  {
> -       ts->tv_sec = 0;
> -       ts->tv_nsec = 0;
> +       if (persistent_clock) {
> +               persistent_clock->read(ts);
> +       } else {
> +               ts->tv_sec = 0;
> +               ts->tv_nsec = 0;
> +       }
>  }
>
>  /**
> @@ -982,8 +1013,12 @@ void __weak read_persistent_clock(struct timespec *ts)
>   */
>  void __weak read_boot_clock(struct timespec *ts)
>  {
> -       ts->tv_sec = 0;
> -       ts->tv_nsec = 0;
> +       if (boot_clock) {
> +               boot_clock->read(ts);
> +       } else {
> +               ts->tv_sec = 0;
> +               ts->tv_nsec = 0;
> +       }
>  }
>
>  /*
> --
> 2.1.0.rc2.206.gedb03e5
>

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
> This code is arch-independent and can be useful on other plaforms as well.
> 
> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
> 
> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
> board, made sure high-resolution clock works.

Using this on an upstream kernel doesn't work, though, because 64-bit
ARM doesn't implement struct delay_timer which the driver needs since
v3.17.

But I suppose the delay timer infrastructure could be moved into the
core similar to the persistent and boot clock as this patch does.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Anatol Pomozov]

Hi

On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>> This code is arch-independent and can be useful on other plaforms as well.
>>
>> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>>
>> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>> board, made sure high-resolution clock works.
>
> Using this on an upstream kernel doesn't work, though, because 64-bit
> ARM doesn't implement struct delay_timer which the driver needs since
> v3.17.
>
> But I suppose the delay timer infrastructure could be moved into the
> core similar to the persistent and boot clock as this patch does.

Thanks. It makes sense, I will send it in a separate patch, once this
one will be reviewed. On our kernel I haven't seen this issue as we
still use 3.14.

In fact none of arch/arm/lib/delay.c code seems ARM32 related.

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thomas Gleixner]

On Mon, 10 Nov 2014, Anatol Pomozov wrote:
> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
> >> This code is arch-independent and can be useful on other plaforms as well.
> >>
> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
> >>
> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
> >> board, made sure high-resolution clock works.
> >
> > Using this on an upstream kernel doesn't work, though, because 64-bit
> > ARM doesn't implement struct delay_timer which the driver needs since
> > v3.17.
> >
> > But I suppose the delay timer infrastructure could be moved into the
> > core similar to the persistent and boot clock as this patch does.
> 
> Thanks. It makes sense, I will send it in a separate patch, once this
> one will be reviewed. On our kernel I haven't seen this issue as we
> still use 3.14.

That's why you should test/compile your stuff on latest greatest and
not on a year old conglomorate of unknown provenance. :)

Aside of that I really wonder why we need that persistent_clock stuff
at all. We already have mechanisms to register persistent clocks AKA
RTCs after the early boot process and update the wall clock time
before we actually need it. Nothing in early boot depends on correct
wall clock at all.

So instead of adding more extra persistent clock nonsense, can we just
move all of that to the place where it belongs, i.e. RTC?

Thanks,

	tglx

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[John Stultz]

On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>> >> This code is arch-independent and can be useful on other plaforms as well.
>> >>
>> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>> >>
>> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>> >> board, made sure high-resolution clock works.
>> >
>> > Using this on an upstream kernel doesn't work, though, because 64-bit
>> > ARM doesn't implement struct delay_timer which the driver needs since
>> > v3.17.
>> >
>> > But I suppose the delay timer infrastructure could be moved into the
>> > core similar to the persistent and boot clock as this patch does.
>>
>> Thanks. It makes sense, I will send it in a separate patch, once this
>> one will be reviewed. On our kernel I haven't seen this issue as we
>> still use 3.14.
>
> That's why you should test/compile your stuff on latest greatest and
> not on a year old conglomorate of unknown provenance. :)
>
> Aside of that I really wonder why we need that persistent_clock stuff
> at all. We already have mechanisms to register persistent clocks AKA
> RTCs after the early boot process and update the wall clock time
> before we actually need it. Nothing in early boot depends on correct
> wall clock at all.
>
> So instead of adding more extra persistent clock nonsense, can we just
> move all of that to the place where it belongs, i.e. RTC?

Sigh.. I've got this on an eventual todo list.. The big problem though
is that the RTC infrastructure can't be called with irqs off, so its
not as optimal for measuring suspend time. Some of the suspend-time
measurement with clocksources that don't halt is interesting here.

So we need to add to the RTC infrastructure special accessors that are
safe when irqs are off, and we can then deprecate the persistent clock
bits. There's still evaluation quirks with setting the time earlier in
boot or not (possibly some rng effects as well there), but that could
be worked out if we had the suspend timing via safe RTC interfaces
sorted.

thanks
-john

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thomas Gleixner]

On Thu, 13 Nov 2014, John Stultz wrote:
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > Aside of that I really wonder why we need that persistent_clock stuff
> > at all. We already have mechanisms to register persistent clocks AKA
> > RTCs after the early boot process and update the wall clock time
> > before we actually need it. Nothing in early boot depends on correct
> > wall clock at all.
> >
> > So instead of adding more extra persistent clock nonsense, can we just
> > move all of that to the place where it belongs, i.e. RTC?
> 
> Sigh.. I've got this on an eventual todo list.. The big problem though
> is that the RTC infrastructure can't be called with irqs off, so its
> not as optimal for measuring suspend time. Some of the suspend-time
> measurement with clocksources that don't halt is interesting here.
> 
> So we need to add to the RTC infrastructure special accessors that are
> safe when irqs are off, and we can then deprecate the persistent clock
> bits. There's still evaluation quirks with setting the time earlier in
> boot or not (possibly some rng effects as well there), but that could
> be worked out if we had the suspend timing via safe RTC interfaces
> sorted.

So we better work on this instead of creating more legacy enforcement
APIs....

Thanks,

	tglx

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4498 bytes --]

On Thu, Nov 13, 2014 at 03:21:22PM -0800, John Stultz wrote:
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 10 Nov 2014, Anatol Pomozov wrote:
> >> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
> >> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
> >> >> This code is arch-independent and can be useful on other plaforms as well.
> >> >>
> >> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
> >> >>
> >> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
> >> >> board, made sure high-resolution clock works.
> >> >
> >> > Using this on an upstream kernel doesn't work, though, because 64-bit
> >> > ARM doesn't implement struct delay_timer which the driver needs since
> >> > v3.17.
> >> >
> >> > But I suppose the delay timer infrastructure could be moved into the
> >> > core similar to the persistent and boot clock as this patch does.
> >>
> >> Thanks. It makes sense, I will send it in a separate patch, once this
> >> one will be reviewed. On our kernel I haven't seen this issue as we
> >> still use 3.14.
> >
> > That's why you should test/compile your stuff on latest greatest and
> > not on a year old conglomorate of unknown provenance. :)
> >
> > Aside of that I really wonder why we need that persistent_clock stuff
> > at all. We already have mechanisms to register persistent clocks AKA
> > RTCs after the early boot process and update the wall clock time
> > before we actually need it. Nothing in early boot depends on correct
> > wall clock at all.
> >
> > So instead of adding more extra persistent clock nonsense, can we just
> > move all of that to the place where it belongs, i.e. RTC?
> 
> Sigh.. I've got this on an eventual todo list.. The big problem though
> is that the RTC infrastructure can't be called with irqs off, so its
> not as optimal for measuring suspend time.

Is that because many RTC devices are accessed over something like I2C or
SPI where interrupts are needed? Or are there additional reasons?

> Some of the suspend-time measurement with clocksources that don't halt
> is interesting here.
> 
> So we need to add to the RTC infrastructure special accessors that are
> safe when irqs are off, and we can then deprecate the persistent clock
> bits. There's still evaluation quirks with setting the time earlier in
> boot or not (possibly some rng effects as well there), but that could
> be worked out if we had the suspend timing via safe RTC interfaces
> sorted.

If it's only about slow busses, perhaps we could copy what other
subsystems have been doing and add a ->can_sleep flag to RTC devices to
mark those that can't be accessed with IRQs off.

Having extra accessors seems to me like it won't work well. As I
understand it we have two types of RTC devices: those that use slow
busses and hence can't be accessed with interrupts off, and those that
don't use a slow bus and therefore can be used with interrupts disabled.
For the former I don't think it's possible to implement accessors that
are safe when IRQs are disabled and for the latter the accessors don't
need to be special. So I think a simple flag should be enough.

I've been thinking a little about how the implementation could look in
practice. Would we simply add code to the weak implementation of the
read_persistent_clock() function (kernel/time/timekeeping.c) which looks
for an RTC device usable as persistent clock?

So something like this:

	void __weak read_persistent_clock(struct timespec *ts)
	{
		struct rtc_device *rtc;

		rtc = rtc_class_open_persistent();
		if (rtc) {
			struct rtc_time tm;
			int err;

			err = rtc_read_time(rtc, &tm);
			rtc_class_close(rtc);

			if (!err) {
				rtc_tm_to_timespec(&tm, ts);
				return;
			}
		}

		ts->tv_sec = 0;
		tv->tv_nsec = 0;
	}

Where rtc_class_open_persistent() could be like rtc_class_open(), except
that it uses a match function like this:

	static int __rtc_match_persistent(struct device *dev, const void *data)
	{
		struct rtc_device *rtc = to_rtc_device(dev);

		return !rtc->can_sleep;
	}

If you still prefer to do this with accessors I suspect something very
similar could be done.

Adding Paul, who's been looking into this as well, to Cc.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[John Stultz]

On Fri, Jan 9, 2015 at 1:43 AM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Nov 13, 2014 at 03:21:22PM -0800, John Stultz wrote:
>> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>> > On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> >> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> >> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>> >> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>> >> >> This code is arch-independent and can be useful on other plaforms as well.
>> >> >>
>> >> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>> >> >>
>> >> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>> >> >> board, made sure high-resolution clock works.
>> >> >
>> >> > Using this on an upstream kernel doesn't work, though, because 64-bit
>> >> > ARM doesn't implement struct delay_timer which the driver needs since
>> >> > v3.17.
>> >> >
>> >> > But I suppose the delay timer infrastructure could be moved into the
>> >> > core similar to the persistent and boot clock as this patch does.
>> >>
>> >> Thanks. It makes sense, I will send it in a separate patch, once this
>> >> one will be reviewed. On our kernel I haven't seen this issue as we
>> >> still use 3.14.
>> >
>> > That's why you should test/compile your stuff on latest greatest and
>> > not on a year old conglomorate of unknown provenance. :)
>> >
>> > Aside of that I really wonder why we need that persistent_clock stuff
>> > at all. We already have mechanisms to register persistent clocks AKA
>> > RTCs after the early boot process and update the wall clock time
>> > before we actually need it. Nothing in early boot depends on correct
>> > wall clock at all.
>> >
>> > So instead of adding more extra persistent clock nonsense, can we just
>> > move all of that to the place where it belongs, i.e. RTC?
>>
>> Sigh.. I've got this on an eventual todo list.. The big problem though
>> is that the RTC infrastructure can't be called with irqs off, so its
>> not as optimal for measuring suspend time.
>
> Is that because many RTC devices are accessed over something like I2C or
> SPI where interrupts are needed? Or are there additional reasons?

Right. The persistent_clock logic is called when irqs may be off, and
many RTC devices require interrupts.


>> Some of the suspend-time measurement with clocksources that don't halt
>> is interesting here.
>>
>> So we need to add to the RTC infrastructure special accessors that are
>> safe when irqs are off, and we can then deprecate the persistent clock
>> bits. There's still evaluation quirks with setting the time earlier in
>> boot or not (possibly some rng effects as well there), but that could
>> be worked out if we had the suspend timing via safe RTC interfaces
>> sorted.
>
> If it's only about slow busses, perhaps we could copy what other
> subsystems have been doing and add a ->can_sleep flag to RTC devices to
> mark those that can't be accessed with IRQs off.
>
> Having extra accessors seems to me like it won't work well. As I
> understand it we have two types of RTC devices: those that use slow
> busses and hence can't be accessed with interrupts off, and those that
> don't use a slow bus and therefore can be used with interrupts disabled.
> For the former I don't think it's possible to implement accessors that
> are safe when IRQs are disabled and for the latter the accessors don't
> need to be special. So I think a simple flag should be enough.

Yea, it doesn't necessarily need to be done via a new accessor, but I
assumed there would be some corner case hardware and the function
pointer would allow some extra flexibility.


> I've been thinking a little about how the implementation could look in
> practice. Would we simply add code to the weak implementation of the
> read_persistent_clock() function (kernel/time/timekeeping.c) which looks
> for an RTC device usable as persistent clock?
>
> So something like this:
>
>         void __weak read_persistent_clock(struct timespec *ts)
>         {
>                 struct rtc_device *rtc;
>
>                 rtc = rtc_class_open_persistent();
>                 if (rtc) {
>                         struct rtc_time tm;
>                         int err;
>
>                         err = rtc_read_time(rtc, &tm);
>                         rtc_class_close(rtc);
>
>                         if (!err) {
>                                 rtc_tm_to_timespec(&tm, ts);
>                                 return;
>                         }
>                 }
>
>                 ts->tv_sec = 0;
>                 tv->tv_nsec = 0;
>         }
>
> Where rtc_class_open_persistent() could be like rtc_class_open(), except
> that it uses a match function like this:
>
>         static int __rtc_match_persistent(struct device *dev, const void *data)
>         {
>                 struct rtc_device *rtc = to_rtc_device(dev);
>
>                 return !rtc->can_sleep;
>         }
>
> If you still prefer to do this with accessors I suspect something very
> similar could be done.

Something like this could work, but I'd prefer we move it into the rtc
framework and use that directly, rather then stuffing the logic into
the persistent_clock code (so we can eventually get rid of the
persistent clock logic).

thanks
-john

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Anatol Pomozov]

Hi

On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>> >> This code is arch-independent and can be useful on other plaforms as well.
>> >>
>> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>> >>
>> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>> >> board, made sure high-resolution clock works.
>> >
>> > Using this on an upstream kernel doesn't work, though, because 64-bit
>> > ARM doesn't implement struct delay_timer which the driver needs since
>> > v3.17.
>> >
>> > But I suppose the delay timer infrastructure could be moved into the
>> > core similar to the persistent and boot clock as this patch does.
>>
>> Thanks. It makes sense, I will send it in a separate patch, once this
>> one will be reviewed. On our kernel I haven't seen this issue as we
>> still use 3.14.
>
> That's why you should test/compile your stuff on latest greatest and
> not on a year old conglomorate of unknown provenance. :)

Unfortunately it is not possible to test this patch with upstream.
There is no ARM64 bit support for Tegra yet. I am trying to
cleanup/upstream my ChromeOS patches and this clock patch in
particular makes one small step towards this goal. Also Thierry
mentioned that he works on full ARM64 Tegra support and it is really
exciting!


So what I suppose to do with my patch? If it does not work could
anyone provide patch that removes ARM arch dependency from
tegra20_timer.c?

> Aside of that I really wonder why we need that persistent_clock stuff
> at all. We already have mechanisms to register persistent clocks AKA
> RTCs after the early boot process and update the wall clock time
> before we actually need it. Nothing in early boot depends on correct
> wall clock at all.
>
> So instead of adding more extra persistent clock nonsense, can we just
> move all of that to the place where it belongs, i.e. RTC?
>
> Thanks,
>
>         tglx
>

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thomas Gleixner]

On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > On Mon, 10 Nov 2014, Anatol Pomozov wrote:
> >> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
> >> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
> >> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
> >> >> This code is arch-independent and can be useful on other plaforms as well.
> >> >>
> >> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
> >> >>
> >> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
> >> >> board, made sure high-resolution clock works.
> >> >
> >> > Using this on an upstream kernel doesn't work, though, because 64-bit
> >> > ARM doesn't implement struct delay_timer which the driver needs since
> >> > v3.17.
> >> >
> >> > But I suppose the delay timer infrastructure could be moved into the
> >> > core similar to the persistent and boot clock as this patch does.
> >>
> >> Thanks. It makes sense, I will send it in a separate patch, once this
> >> one will be reviewed. On our kernel I haven't seen this issue as we
> >> still use 3.14.
> >
> > That's why you should test/compile your stuff on latest greatest and
> > not on a year old conglomorate of unknown provenance. :)
> 
> Unfortunately it is not possible to test this patch with upstream.
> There is no ARM64 bit support for Tegra yet. I am trying to
> cleanup/upstream my ChromeOS patches and this clock patch in
> particular makes one small step towards this goal. Also Thierry
> mentioned that he works on full ARM64 Tegra support and it is really
> exciting!

Everything is exciting, but it does not change the fact, that this
patch cannot work on current upstream.

> So what I suppose to do with my patch? If it does not work could
> anyone provide patch that removes ARM arch dependency from
> tegra20_timer.c?

Huch? You want other people to solve your problems?

Thanks,

	tglx

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Anatol Pomozov]

Hi

On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On Fri, 14 Nov 2014, Anatol Pomozov wrote:
>> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>> > On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> >> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> >> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> > On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>> >> >> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>> >> >> This code is arch-independent and can be useful on other plaforms as well.
>> >> >>
>> >> >> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>> >> >>
>> >> >> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>> >> >> board, made sure high-resolution clock works.
>> >> >
>> >> > Using this on an upstream kernel doesn't work, though, because 64-bit
>> >> > ARM doesn't implement struct delay_timer which the driver needs since
>> >> > v3.17.
>> >> >
>> >> > But I suppose the delay timer infrastructure could be moved into the
>> >> > core similar to the persistent and boot clock as this patch does.
>> >>
>> >> Thanks. It makes sense, I will send it in a separate patch, once this
>> >> one will be reviewed. On our kernel I haven't seen this issue as we
>> >> still use 3.14.
>> >
>> > That's why you should test/compile your stuff on latest greatest and
>> > not on a year old conglomorate of unknown provenance. :)
>>
>> Unfortunately it is not possible to test this patch with upstream.
>> There is no ARM64 bit support for Tegra yet. I am trying to
>> cleanup/upstream my ChromeOS patches and this clock patch in
>> particular makes one small step towards this goal. Also Thierry
>> mentioned that he works on full ARM64 Tegra support and it is really
>> exciting!
>
> Everything is exciting, but it does not change the fact, that this
> patch cannot work on current upstream.

Could you please be more specific what exactly does not work? Are you
talking about delay timer? But my patch does not touch any delay timer
code. I can compile tegra_timer for ARM. And this code is not usable
on arm64 anyway because whole Tegra is not ported yet. Somebody should
make additional changes to upstream tegra20_timer.c code. I might try
to do it later when Tegra will be ported.

>> So what I suppose to do with my patch? If it does not work could
>> anyone provide patch that removes ARM arch dependency from
>> tegra20_timer.c?
>
> Huch? You want other people to solve your problems?

This is not the point. I provided patch that fixes the issue. Other
people said that they have ideas how to do it different (and better)
way. So I am asking to share these ideas represented as a patch.

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thomas Gleixner]

On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> >> So what I suppose to do with my patch? If it does not work could
> >> anyone provide patch that removes ARM arch dependency from
> >> tegra20_timer.c?
> >
> > Huch? You want other people to solve your problems?
> 
> This is not the point. I provided patch that fixes the issue. Other
> people said that they have ideas how to do it different (and better)
> way. So I am asking to share these ideas represented as a patch.

That's not the way it works.

You sent a patch to solve an problem which you are facing.

Now the people who review the patch think that there is a better
approach than moving code from arm/ to the timekeeping core code.

So it's up to you to come up with a patch which solves the problem in
the right way.

Thanks,

	tglx

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thomas Gleixner]

On Sat, 15 Nov 2014, Thomas Gleixner wrote:
> On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> > On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > >> So what I suppose to do with my patch? If it does not work could
> > >> anyone provide patch that removes ARM arch dependency from
> > >> tegra20_timer.c?
> > >
> > > Huch? You want other people to solve your problems?
> > 
> > This is not the point. I provided patch that fixes the issue. Other
> > people said that they have ideas how to do it different (and better)
> > way. So I am asking to share these ideas represented as a patch.
> 
> That's not the way it works.
> 
> You sent a patch to solve an problem which you are facing.
> 
> Now the people who review the patch think that there is a better
> approach than moving code from arm/ to the timekeeping core code.
> 
> So it's up to you to come up with a patch which solves the problem in
> the right way.

And just for the record this whole thing is just hilarious.

ARM64 selects ARM_ARCH_TIMER which registers the architected timer as
the primary clocksource.

Now that timer has the following flag set:

    CLOCK_SOURCE_SUSPEND_NONSTOP

And that flag causes the core timekeeping code to use the clocksource
to figure out the time which the machine spent in suspend.

So registering that tegra RTC as persistent clock does not have any
value at all. Simply because it is completely irrelevant at boot time
whether the RTC can be accessed right at timekeeping init or
not. Nothing in early boot needs wall clock time. It's good enough to
set wall clock time late in the boot process as the first use case is
when the root file system gets mounted. And the RTC driver which
obviously deals with the same hardware is initialized before that.

So you are trying to move something which is outright pointless to the
core code just because you carry that patch in your ChromeOS support
hackery.

Thanks,

	tglx

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Sat, Nov 15, 2014 at 02:38:00AM +0100, Thomas Gleixner wrote:
> On Sat, 15 Nov 2014, Thomas Gleixner wrote:
> > On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> > > On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > > >> So what I suppose to do with my patch? If it does not work could
> > > >> anyone provide patch that removes ARM arch dependency from
> > > >> tegra20_timer.c?
> > > >
> > > > Huch? You want other people to solve your problems?
> > > 
> > > This is not the point. I provided patch that fixes the issue. Other
> > > people said that they have ideas how to do it different (and better)
> > > way. So I am asking to share these ideas represented as a patch.
> > 
> > That's not the way it works.
> > 
> > You sent a patch to solve an problem which you are facing.
> > 
> > Now the people who review the patch think that there is a better
> > approach than moving code from arm/ to the timekeeping core code.
> > 
> > So it's up to you to come up with a patch which solves the problem in
> > the right way.
> 
> And just for the record this whole thing is just hilarious.
> 
> ARM64 selects ARM_ARCH_TIMER which registers the architected timer as
> the primary clocksource.
> 
> Now that timer has the following flag set:
> 
>     CLOCK_SOURCE_SUSPEND_NONSTOP
> 
> And that flag causes the core timekeeping code to use the clocksource
> to figure out the time which the machine spent in suspend.

As I understand it the architected timer will be turned off along with
the rest of the CPU complex on Tegra. I'm not sure if that's specific to
Tegra or something that other SoCs may do as well.

Cc'ing Paul who's more familiar with the details.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Mark Rutland]

On Fri, Jan 09, 2015 at 09:49:14AM +0000, Thierry Reding wrote:
> On Sat, Nov 15, 2014 at 02:38:00AM +0100, Thomas Gleixner wrote:
> > On Sat, 15 Nov 2014, Thomas Gleixner wrote:
> > > On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> > > > On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > > > >> So what I suppose to do with my patch? If it does not work could
> > > > >> anyone provide patch that removes ARM arch dependency from
> > > > >> tegra20_timer.c?
> > > > >
> > > > > Huch? You want other people to solve your problems?
> > > > 
> > > > This is not the point. I provided patch that fixes the issue. Other
> > > > people said that they have ideas how to do it different (and better)
> > > > way. So I am asking to share these ideas represented as a patch.
> > > 
> > > That's not the way it works.
> > > 
> > > You sent a patch to solve an problem which you are facing.
> > > 
> > > Now the people who review the patch think that there is a better
> > > approach than moving code from arm/ to the timekeeping core code.
> > > 
> > > So it's up to you to come up with a patch which solves the problem in
> > > the right way.
> > 
> > And just for the record this whole thing is just hilarious.
> > 
> > ARM64 selects ARM_ARCH_TIMER which registers the architected timer as
> > the primary clocksource.
> > 
> > Now that timer has the following flag set:
> > 
> >     CLOCK_SOURCE_SUSPEND_NONSTOP
> > 
> > And that flag causes the core timekeeping code to use the clocksource
> > to figure out the time which the machine spent in suspend.
> 
> As I understand it the architected timer will be turned off along with
> the rest of the CPU complex on Tegra. I'm not sure if that's specific to
> Tegra or something that other SoCs may do as well.

That doesn't sound right to me: the architecture specifies that the
system counter must be implemented in an always-on power domain.

Note that that only applies to the counter, not the timers (as the
comparators can be turned off with the CPUs).

Mark.

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2508 bytes --]

On Fri, Jan 09, 2015 at 01:59:07PM +0000, Mark Rutland wrote:
> On Fri, Jan 09, 2015 at 09:49:14AM +0000, Thierry Reding wrote:
> > On Sat, Nov 15, 2014 at 02:38:00AM +0100, Thomas Gleixner wrote:
> > > On Sat, 15 Nov 2014, Thomas Gleixner wrote:
> > > > On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> > > > > On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > > > > >> So what I suppose to do with my patch? If it does not work could
> > > > > >> anyone provide patch that removes ARM arch dependency from
> > > > > >> tegra20_timer.c?
> > > > > >
> > > > > > Huch? You want other people to solve your problems?
> > > > > 
> > > > > This is not the point. I provided patch that fixes the issue. Other
> > > > > people said that they have ideas how to do it different (and better)
> > > > > way. So I am asking to share these ideas represented as a patch.
> > > > 
> > > > That's not the way it works.
> > > > 
> > > > You sent a patch to solve an problem which you are facing.
> > > > 
> > > > Now the people who review the patch think that there is a better
> > > > approach than moving code from arm/ to the timekeeping core code.
> > > > 
> > > > So it's up to you to come up with a patch which solves the problem in
> > > > the right way.
> > > 
> > > And just for the record this whole thing is just hilarious.
> > > 
> > > ARM64 selects ARM_ARCH_TIMER which registers the architected timer as
> > > the primary clocksource.
> > > 
> > > Now that timer has the following flag set:
> > > 
> > >     CLOCK_SOURCE_SUSPEND_NONSTOP
> > > 
> > > And that flag causes the core timekeeping code to use the clocksource
> > > to figure out the time which the machine spent in suspend.
> > 
> > As I understand it the architected timer will be turned off along with
> > the rest of the CPU complex on Tegra. I'm not sure if that's specific to
> > Tegra or something that other SoCs may do as well.
> 
> That doesn't sound right to me: the architecture specifies that the
> system counter must be implemented in an always-on power domain.
> 
> Note that that only applies to the counter, not the timers (as the
> comparators can be turned off with the CPUs).

I'll let Paul comment on this, since I don't know the intimate details.
Of course if the system counter is indeed active across suspend/resume
then it doesn't seem like we'd need the Tegra timer at all on 64-bit.
Unless of course if we use it for something else.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Paul Walmsley]

Hi

On Fri, 9 Jan 2015, Thierry Reding wrote:

> On Fri, Jan 09, 2015 at 01:59:07PM +0000, Mark Rutland wrote:
> > On Fri, Jan 09, 2015 at 09:49:14AM +0000, Thierry Reding wrote:
> >
> > > As I understand it the architected timer will be turned off along with
> > > the rest of the CPU complex on Tegra. I'm not sure if that's specific to
> > > Tegra or something that other SoCs may do as well.
> > 
> > That doesn't sound right to me: the architecture specifies that the
> > system counter must be implemented in an always-on power domain.
> > 
> > Note that that only applies to the counter, not the timers (as the
> > comparators can be turned off with the CPUs).
> 
> I'll let Paul comment on this, since I don't know the intimate details.

I believe Mark's comments are accurate.

In Linux terms, when deep CPU power management states are in use, the ARM 
architected "timer" is useful as a clocksource, but useless as a wakeup 
source (a clockevent device).

Regarding clocksources, my personal view is that the SoC timer IP 
blocks should be exposed as a clocksource option if the timer IP blocks 
are present in the hardware.  That written, I would expect that most 
production implementations will use the ARM architected "timer" as the 
clocksource.


- Paul

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Daniel Lezcano]

On 11/15/2014 01:51 AM, Anatol Pomozov wrote:
> Hi
>
> On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Fri, 14 Nov 2014, Anatol Pomozov wrote:
>>> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>>>>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>>>>> <thierry.reding@gmail.com> wrote:
>>>>>> On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>>>>>>> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>>>>>>> This code is arch-independent and can be useful on other plaforms as well.
>>>>>>>
>>>>>>> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>>>>>>>
>>>>>>> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>>>>>>> board, made sure high-resolution clock works.
>>>>>>
>>>>>> Using this on an upstream kernel doesn't work, though, because 64-bit
>>>>>> ARM doesn't implement struct delay_timer which the driver needs since
>>>>>> v3.17.
>>>>>>
>>>>>> But I suppose the delay timer infrastructure could be moved into the
>>>>>> core similar to the persistent and boot clock as this patch does.
>>>>>
>>>>> Thanks. It makes sense, I will send it in a separate patch, once this
>>>>> one will be reviewed. On our kernel I haven't seen this issue as we
>>>>> still use 3.14.
>>>>
>>>> That's why you should test/compile your stuff on latest greatest and
>>>> not on a year old conglomorate of unknown provenance. :)
>>>
>>> Unfortunately it is not possible to test this patch with upstream.
>>> There is no ARM64 bit support for Tegra yet. I am trying to
>>> cleanup/upstream my ChromeOS patches and this clock patch in
>>> particular makes one small step towards this goal. Also Thierry
>>> mentioned that he works on full ARM64 Tegra support and it is really
>>> exciting!
>>
>> Everything is exciting, but it does not change the fact, that this
>> patch cannot work on current upstream.
>
> Could you please be more specific what exactly does not work? Are you
> talking about delay timer? But my patch does not touch any delay timer
> code. I can compile tegra_timer for ARM. And this code is not usable
> on arm64 anyway because whole Tegra is not ported yet. Somebody should
> make additional changes to upstream tegra20_timer.c code. I might try
> to do it later when Tegra will be ported.
>
>>> So what I suppose to do with my patch? If it does not work could
>>> anyone provide patch that removes ARM arch dependency from
>>> tegra20_timer.c?
>>
>> Huch? You want other people to solve your problems?
>
> This is not the point. I provided patch that fixes the issue. Other
> people said that they have ideas how to do it different (and better)
> way. So I am asking to share these ideas represented as a patch.

I think your issue is solved [1] ... for the moment.

Thanks
   -- Daniel

[1] https://lkml.org/lkml/2015/1/9/264



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] timekeeping: Move persistent clock registration code from ARM to kernel

[Stephen Warren]

On 11/14/2014 03:03 PM, Anatol Pomozov wrote:
> Hi
>
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>>> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On Fri, Nov 07, 2014 at 11:34:15AM -0800, Anatol Pomozov wrote:
>>>>> ARM timekeeping functionality allows to register persistent/boot clock dynamically.
>>>>> This code is arch-independent and can be useful on other plaforms as well.
>>>>>
>>>>> As a byproduct of this change, tegra20_timer becomes ARM64 compatible.
>>>>>
>>>>> Tested: backported the change to chromeos-3.14 kernel ran on tegra 64bit
>>>>> board, made sure high-resolution clock works.
>>>>
>>>> Using this on an upstream kernel doesn't work, though, because 64-bit
>>>> ARM doesn't implement struct delay_timer which the driver needs since
>>>> v3.17.
>>>>
>>>> But I suppose the delay timer infrastructure could be moved into the
>>>> core similar to the persistent and boot clock as this patch does.
>>>
>>> Thanks. It makes sense, I will send it in a separate patch, once this
>>> one will be reviewed. On our kernel I haven't seen this issue as we
>>> still use 3.14.
>>
>> That's why you should test/compile your stuff on latest greatest and
>> not on a year old conglomorate of unknown provenance. :)
>
> Unfortunately it is not possible to test this patch with upstream.
> There is no ARM64 bit support for Tegra yet. I am trying to
> cleanup/upstream my ChromeOS patches and this clock patch in
> particular makes one small step towards this goal. Also Thierry
> mentioned that he works on full ARM64 Tegra support and it is really
> exciting!

What we usually do is send patches in the order the kernel boot process 
needs them. First modify the kernel to know about 64-bit Tegra, add 
earlyprintk support, make sure the early boot process spits out 
something on the UART, then add whatever next item is missing (e.g. 
clock driver, timers, ...). That way, every patch we apply can actually 
be tested in the mainline kernel, since the code actually reaches that 
point in execution.

If we were for example to send in a ton of driver patches for ARM64 
right now, we couldn't test them. Quite possibly those patches wouldn't 
fully work, and we'd just have churn fixing them up later once the base 
CPU/SoC support was added. It's better to only upstream patches that can 
actually be exercised in order to avoid that churn.