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 <[email protected]>
---
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
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
<[email protected]> 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 <[email protected]>
> ---
> 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
>
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
Hi
On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
<[email protected]> 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.
On Mon, 10 Nov 2014, Anatol Pomozov wrote:
> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
> <[email protected]> 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
On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <[email protected]> wrote:
> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> <[email protected]> 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
On Thu, 13 Nov 2014, John Stultz wrote:
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <[email protected]> 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
Hi
On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <[email protected]> wrote:
> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> <[email protected]> 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
>
On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <[email protected]> wrote:
> > On Mon, 10 Nov 2014, Anatol Pomozov wrote:
> >> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
> >> <[email protected]> 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
Hi
On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <[email protected]> wrote:
> On Fri, 14 Nov 2014, Anatol Pomozov wrote:
>> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <[email protected]> wrote:
>> > On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>> >> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>> >> <[email protected]> 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.
On 11/14/2014 03:03 PM, Anatol Pomozov wrote:
> Hi
>
> On Thu, Nov 13, 2014 at 2:46 PM, Thomas Gleixner <[email protected]> wrote:
>> On Mon, 10 Nov 2014, Anatol Pomozov wrote:
>>> On Mon, Nov 10, 2014 at 1:53 AM, Thierry Reding
>>> <[email protected]> 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.
On Fri, 14 Nov 2014, Anatol Pomozov wrote:
> On Fri, Nov 14, 2014 at 4:18 PM, Thomas Gleixner <[email protected]> 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
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 <[email protected]> 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