[PATCH 0/3] Patches focus on clocksource_calc_mult_shift()

[PATCH 0/3] Patches focus on clocksource_calc_mult_shift()

[Yong Zhang]

Hi Thomas/John,

When I was tring to apply commit d65670a7 to old kernel, I found
it's hard to apply, the reason is we only provide
clockevents_calc_mult_shift() for a period of time. And going
through the latest kernel, there is still the user of that
function. So I think we still need the ->maxadj tweak in
clockevents_calc_mult_shift(), this is the aim of patch#1.

And to loose the burden of you, I convert the only user of
clockevents_calc_mult_shift()/clocksource_register() to
clocksour_register_hz() (patch#2), and then get rid of
clockevents_calc_mult_shift() (patch#3).

IMHO, patch#1 will be more conservative and safe, and patch#3
is a little aggressive. I will let you decide to pick which one ;-)

Thanks,
Yong


Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>

Yong Zhang (3):
  clocksource: Also tweak ->maxadj in clocksource_calc_mult_shift()
  clocksource: dbx500: convert to clocksource_register_hz()
  clocksource: get rid of clocksource_calc_mult_shift()

 drivers/clocksource/clksrc-dbx500-prcmu.c |    5 +----
 include/linux/clocksource.h               |    7 -------
 kernel/time/clocksource.c                 |    1 -
 3 files changed, 1 insertions(+), 12 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] clocksource: Also tweak ->maxadj in clocksource_calc_mult_shift()

[Yong Zhang]

There is still user of clocksource_calc_mult_shift(), and it's
complement to commit d65670a7 [clocksource: Avoid selecting mult
values that might overflow when adjusted]

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/clocksource.h |    8 ++------
 kernel/time/clocksource.c   |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c86c940..a02a8d7 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -314,12 +314,8 @@ static inline void __clocksource_updatefreq_khz(struct clocksource *cs, u32 khz)
 	__clocksource_updatefreq_scale(cs, 1000, khz);
 }
 
-static inline void
-clocksource_calc_mult_shift(struct clocksource *cs, u32 freq, u32 minsec)
-{
-	return clocks_calc_mult_shift(&cs->mult, &cs->shift, freq,
-				      NSEC_PER_SEC, minsec);
-}
+extern void
+clocksource_calc_mult_shift(struct clocksource *cs, u32 freq, u32 minsec);
 
 #ifdef CONFIG_GENERIC_TIME_VSYSCALL
 extern void
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index cfc65e1..5119042 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -724,6 +724,24 @@ int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32 freq)
 }
 EXPORT_SYMBOL_GPL(__clocksource_register_scale);
 
+void clocksource_calc_mult_shift(struct clocksource *cs, u32 freq, u32 minsec)
+{
+	clocks_calc_mult_shift(&cs->mult, &cs->shift, freq,
+				NSEC_PER_SEC, minsec);
+
+	/*
+	 * for clocksources that have large mults, to avoid overflow.
+	 * Since mult may be adjusted by ntp, add an safety extra margin
+	 *
+	 */
+	cs->maxadj = clocksource_max_adjustment(cs);
+	while ((cs->mult + cs->maxadj < cs->mult)
+		|| (cs->mult - cs->maxadj > cs->mult)) {
+		cs->mult >>= 1;
+		cs->shift--;
+		cs->maxadj = clocksource_max_adjustment(cs);
+	}
+}
 
 /**
  * clocksource_register - Used to install new clocksources
-- 
1.7.5.4

[PATCH 2/3] clocksource: dbx500: convert to clocksource_register_hz()

[Yong Zhang]

Convert clocksource_dbx500_prcmu to use clocksource_register_hz.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Mattias Wallin <mattias.wallin@stericsson.com>
---
 drivers/clocksource/clksrc-dbx500-prcmu.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/clksrc-dbx500-prcmu.c b/drivers/clocksource/clksrc-dbx500-prcmu.c
index 59feefe..74e8901 100644
--- a/drivers/clocksource/clksrc-dbx500-prcmu.c
+++ b/drivers/clocksource/clksrc-dbx500-prcmu.c
@@ -52,7 +52,6 @@ static struct clocksource clocksource_dbx500_prcmu = {
 	.name		= "dbx500-prcmu-timer",
 	.rating		= 300,
 	.read		= clksrc_dbx500_prcmu_read,
-	.shift		= 10,
 	.mask		= CLOCKSOURCE_MASK(32),
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 };
@@ -100,7 +99,5 @@ void __init clksrc_dbx500_prcmu_init(void __iomem *base)
 	init_sched_clock(&cd, clksrc_dbx500_prcmu_update_sched_clock,
 			 32, RATE_32K);
 #endif
-	clocksource_calc_mult_shift(&clocksource_dbx500_prcmu,
-				    RATE_32K, SCHED_CLOCK_MIN_WRAP);
-	clocksource_register(&clocksource_dbx500_prcmu);
+	clocksource_register_hz(&clocksource_dbx500_prcmu, RATE_32K);
 }
-- 
1.7.5.4

[PATCH 3/3] clocksource: get rid of clocksource_calc_mult_shift()

[Yong Zhang]

Encourage the user to use clocksource_register_hz/khz(), and
lose the burden of maintainer.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/clocksource.h |    3 ---
 kernel/time/clocksource.c   |   19 -------------------
 2 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index a02a8d7..f018278 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -314,9 +314,6 @@ static inline void __clocksource_updatefreq_khz(struct clocksource *cs, u32 khz)
 	__clocksource_updatefreq_scale(cs, 1000, khz);
 }
 
-extern void
-clocksource_calc_mult_shift(struct clocksource *cs, u32 freq, u32 minsec);
-
 #ifdef CONFIG_GENERIC_TIME_VSYSCALL
 extern void
 update_vsyscall(struct timespec *ts, struct timespec *wtm,
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 5119042..a75bd7b 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -724,25 +724,6 @@ int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32 freq)
 }
 EXPORT_SYMBOL_GPL(__clocksource_register_scale);
 
-void clocksource_calc_mult_shift(struct clocksource *cs, u32 freq, u32 minsec)
-{
-	clocks_calc_mult_shift(&cs->mult, &cs->shift, freq,
-				NSEC_PER_SEC, minsec);
-
-	/*
-	 * for clocksources that have large mults, to avoid overflow.
-	 * Since mult may be adjusted by ntp, add an safety extra margin
-	 *
-	 */
-	cs->maxadj = clocksource_max_adjustment(cs);
-	while ((cs->mult + cs->maxadj < cs->mult)
-		|| (cs->mult - cs->maxadj > cs->mult)) {
-		cs->mult >>= 1;
-		cs->shift--;
-		cs->maxadj = clocksource_max_adjustment(cs);
-	}
-}
-
 /**
  * clocksource_register - Used to install new clocksources
  * @t:		clocksource to be registered
-- 
1.7.5.4

Re: [PATCH 2/3] clocksource: dbx500: convert to clocksource_register_hz()

[Linus Walleij]

On Thu, Dec 1, 2011 at 8:20 AM, Yong Zhang <yong.zhang0@gmail.com> wrote:

> Convert clocksource_dbx500_prcmu to use clocksource_register_hz.
>
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Mattias Wallin <mattias.wallin@stericsson.com>
> ---
>  drivers/clocksource/clksrc-dbx500-prcmu.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/clksrc-dbx500-prcmu.c b/drivers/clocksource/clksrc-dbx500-prcmu.c
> index 59feefe..74e8901 100644
> --- a/drivers/clocksource/clksrc-dbx500-prcmu.c
> +++ b/drivers/clocksource/clksrc-dbx500-prcmu.c
> @@ -52,7 +52,6 @@ static struct clocksource clocksource_dbx500_prcmu = {
>        .name           = "dbx500-prcmu-timer",
>        .rating         = 300,
>        .read           = clksrc_dbx500_prcmu_read,
> -       .shift          = 10,
>        .mask           = CLOCKSOURCE_MASK(32),
>        .flags          = CLOCK_SOURCE_IS_CONTINUOUS,
>  };
> @@ -100,7 +99,5 @@ void __init clksrc_dbx500_prcmu_init(void __iomem *base)
>        init_sched_clock(&cd, clksrc_dbx500_prcmu_update_sched_clock,
>                         32, RATE_32K);
>  #endif
> -       clocksource_calc_mult_shift(&clocksource_dbx500_prcmu,
> -                                   RATE_32K, SCHED_CLOCK_MIN_WRAP);
> -       clocksource_register(&clocksource_dbx500_prcmu);
> +       clocksource_register_hz(&clocksource_dbx500_prcmu, RATE_32K);
>  }
> --
> 1.7.5.4

Looks good,
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

Re: [PATCH 2/3] clocksource: dbx500: convert to clocksource_register_hz()

[John Stultz]

On Thu, 2011-12-01 at 15:20 +0800, Yong Zhang wrote:
> Convert clocksource_dbx500_prcmu to use clocksource_register_hz.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Mattias Wallin <mattias.wallin@stericsson.com>
> ---

Acked-by: John Stultz <john.stultz@linaro.org>

I've queued this myself, but I'd prefer it to go through the appropriate
maintainer. That said, if I don't see it in -next in a week or so, I'll
go ahead and add it to my 3.3 list for Thomas.

thanks
-john

Re: [PATCH 1/3] clocksource: Also tweak ->maxadj in clocksource_calc_mult_shift()

[john stultz]

On Thu, 2011-12-01 at 15:20 +0800, Yong Zhang wrote:
> There is still user of clocksource_calc_mult_shift(), and it's
> complement to commit d65670a7 [clocksource: Avoid selecting mult
> values that might overflow when adjusted]

I'd probably drop this patch, as its just extra churn.

Again, I've queued 2/3 to resolve the issue and queued a simplified 3/3
to just remove clocksource_calc_mult_shift. 

thanks
-john

Re: [PATCH 1/3] clocksource: Also tweak ->maxadj in clocksource_calc_mult_shift()

[Yong Zhang]

On Thu, Dec 01, 2011 at 03:58:27PM -0800, john stultz wrote:
> On Thu, 2011-12-01 at 15:20 +0800, Yong Zhang wrote:
> > There is still user of clocksource_calc_mult_shift(), and it's
> > complement to commit d65670a7 [clocksource: Avoid selecting mult
> > values that might overflow when adjusted]
> 
> I'd probably drop this patch, as its just extra churn.

Hmm, so you drop option#1,

> 
> Again, I've queued 2/3 to resolve the issue and queued a simplified 3/3
> to just remove clocksource_calc_mult_shift. 

and take option#2 instead (and 3/3 will become more simple).
Both work to me ;-)

Thank you John.

-Yong

Re: [PATCH 2/3] clocksource: dbx500: convert to clocksource_register_hz()

[Linus Walleij]

On Fri, Dec 2, 2011 at 12:55 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, 2011-12-01 at 15:20 +0800, Yong Zhang wrote:
>> Convert clocksource_dbx500_prcmu to use clocksource_register_hz.

> I've queued this myself, but I'd prefer it to go through the appropriate
> maintainer. That said, if I don't see it in -next in a week or so, I'll
> go ahead and add it to my 3.3 list for Thomas.

Given that the driver is in drivers/clocksource/* aren't You+Thomas
the maintainers?

Actually this is one of the instances where we try to push ARM stuff
down to a subsystem where it belongs for proper handling by the
subsystem maintainers.

This was a fortunate case since it happened to be a "clean"
clocksource driver, I don't know what to do with all the combined
clocksource+clock event drivers we have in ARM, part of me
wants to push that to drivers/clocksource too, but mainly the
cross-dependency of needing to be called during early machine
init get me to want to keep it in arch/arm/* FTM...

Yours,
Linus Walleij

Re: [PATCH 2/3] clocksource: dbx500: convert to clocksource_register_hz()

[John Stultz]

On Fri, 2011-12-02 at 09:29 +0100, Linus Walleij wrote:
> On Fri, Dec 2, 2011 at 12:55 AM, John Stultz <john.stultz@linaro.org> wrote:
> > On Thu, 2011-12-01 at 15:20 +0800, Yong Zhang wrote:
> >> Convert clocksource_dbx500_prcmu to use clocksource_register_hz.
> 
> > I've queued this myself, but I'd prefer it to go through the appropriate
> > maintainer. That said, if I don't see it in -next in a week or so, I'll
> > go ahead and add it to my 3.3 list for Thomas.
> 
> Given that the driver is in drivers/clocksource/* aren't You+Thomas
> the maintainers?

Fair enough. :) 

I just want to make sure I'm not breaking anything, or surprising
anyone, since I've got limited ability to test all the various hardware
drivers.

> Actually this is one of the instances where we try to push ARM stuff
> down to a subsystem where it belongs for proper handling by the
> subsystem maintainers.

Alrighty, as long as I get an ack or tested-by from someone who has
hardware this sounds reasonable. 

thanks!
-john