[PATCH 3/3] mach-u300: cleanup clockevent code

[PATCH 3/3] mach-u300: cleanup clockevent code

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

Use the new clockevents_config_and_register() function to register
the U300 clockevent, since that code requires ->cpumask to be set
we set this even on this UP system to please the framework.

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/mach-u300/timer.c |   32 ++++++++++++--------------------
 1 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-u300/timer.c b/arch/arm/mach-u300/timer.c
index 18d7fa0..bd32dda 100644
--- a/arch/arm/mach-u300/timer.c
+++ b/arch/arm/mach-u300/timer.c
@@ -27,9 +27,6 @@
 #include <asm/mach/time.h>
 #include <asm/mach/irq.h>
 
-/* Be able to sleep for atleast 4 seconds (usually more) */
-#define APPTIMER_MIN_RANGE 4
-
 /*
  * APP side special timer registers
  * This timer contains four timers which can fire an interrupt each.
@@ -309,11 +306,11 @@ static int u300_set_next_event(unsigned long cycles,
 
 /* Use general purpose timer 1 as clock event */
 static struct clock_event_device clockevent_u300_1mhz = {
-	.name           = "GPT1",
-	.rating         = 300, /* Reasonably fast and accurate clock event */
-	.features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
-	.set_next_event = u300_set_next_event,
-	.set_mode       = u300_set_mode,
+	.name		= "GPT1",
+	.rating		= 300, /* Reasonably fast and accurate clock event */
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_next_event	= u300_set_next_event,
+	.set_mode	= u300_set_mode,
 };
 
 /* Clock event timer interrupt handler */
@@ -328,9 +325,9 @@ static irqreturn_t u300_timer_interrupt(int irq, void *dev_id)
 }
 
 static struct irqaction u300_timer_irq = {
-	.name           = "U300 Timer Tick",
-	.flags          = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
-	.handler        = u300_timer_interrupt,
+	.name		= "U300 Timer Tick",
+	.flags		= IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler	= u300_timer_interrupt,
 };
 
 /*
@@ -413,16 +410,11 @@ static void __init u300_timer_init(void)
 			"GPT2", rate, 300, 32, clocksource_mmio_readl_up))
 		pr_err("timer: failed to initialize U300 clock source\n");
 
-	clockevents_calc_mult_shift(&clockevent_u300_1mhz,
-				    rate, APPTIMER_MIN_RANGE);
-	/* 32bit counter, so 32bits delta is max */
-	clockevent_u300_1mhz.max_delta_ns =
-		clockevent_delta2ns(0xffffffff, &clockevent_u300_1mhz);
-	/* This timer is slow enough to set for 1 cycle == 1 MHz */
-	clockevent_u300_1mhz.min_delta_ns =
-		clockevent_delta2ns(1, &clockevent_u300_1mhz);
+	/* Configure and register the clockevent */
 	clockevent_u300_1mhz.cpumask = cpumask_of(0);
-	clockevents_register_device(&clockevent_u300_1mhz);
+	clockevents_config_and_register(&clockevent_u300_1mhz, rate,
+					1, 0xffffffff);
+
 	/*
 	 * TODO: init and register the rest of the timers too, they can be
 	 * used by hrtimers!
-- 
1.7.3.2

[PATCH 3/3] mach-u300: cleanup clockevent code

[Thomas Gleixner]

On Tue, 31 May 2011, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> Use the new clockevents_config_and_register() function to register
> the U300 clockevent, since that code requires ->cpumask to be set
> we set this even on this UP system to please the framework.

Hmm, how about whacking the framework maintainer on the head for that
requirement?

On UP this should be simply ignored and on SMP we can optimize that
for all those architectures which come up with CPU0 in the first
place.

Thanks,

	tglx

[PATCH 3/3] mach-u300: cleanup clockevent code

[Linus Walleij]

2011/5/31 Thomas Gleixner <tglx@linutronix.de>:
> On Tue, 31 May 2011, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> Use the new clockevents_config_and_register() function to register
>> the U300 clockevent, since that code requires ->cpumask to be set
>> we set this even on this UP system to please the framework.
>
> Hmm, how about whacking the framework maintainer on the head for that
> requirement?

Yeah hm, I sort of figured it might be desirable to have this warning
on SMP. This:

BUG_ON(!dev->cpumask);

from clockevents.c is the culprit anyway. I dunno if it's best to
#ifdef CONFIG_SMP that thing (technically I guess it shouldn't
even be in the struct on UP but who cares) or if there is some
more clever way to do it runtime, so whatever you prefer, I can
patch it if you know what you want.

Thanks,
Linus Walleij

[PATCH 3/3] mach-u300: cleanup clockevent code

[Thomas Gleixner]

On Wed, 1 Jun 2011, Linus Walleij wrote:

> 2011/5/31 Thomas Gleixner <tglx@linutronix.de>:
> > On Tue, 31 May 2011, Linus Walleij wrote:
> >> From: Linus Walleij <linus.walleij@linaro.org>
> >>
> >> Use the new clockevents_config_and_register() function to register
> >> the U300 clockevent, since that code requires ->cpumask to be set
> >> we set this even on this UP system to please the framework.
> >
> > Hmm, how about whacking the framework maintainer on the head for that
> > requirement?
> 
> Yeah hm, I sort of figured it might be desirable to have this warning
> on SMP. This:
> 
> BUG_ON(!dev->cpumask);
> 
> from clockevents.c is the culprit anyway. I dunno if it's best to
> #ifdef CONFIG_SMP that thing (technically I guess it shouldn't
> even be in the struct on UP but who cares) or if there is some
> more clever way to do it runtime, so whatever you prefer, I can
> patch it if you know what you want.

We need it even on UP for the &!^%$@ broadcast mechanism to avoid a
massive ifdef mess there :(

But yeah, we can make it conditional for SMP and simply set
cpumask_of(0) in the UP case.

Thanks,

	tglx

[PATCH 3/3] mach-u300: cleanup clockevent code

[Linus Walleij]

2011/6/1 Thomas Gleixner <tglx@linutronix.de>:

> We need it even on UP for the &!^%$@ broadcast mechanism to avoid a
> massive ifdef mess there :(
>
> But yeah, we can make it conditional for SMP and simply set
> cpumask_of(0) in the UP case.

Like this?

[PATCH 3/3] mach-u300: cleanup clockevent code

[Russell King - ARM Linux]

On Wed, Jun 01, 2011 at 10:34:46AM +0200, Thomas Gleixner wrote:
> But yeah, we can make it conditional for SMP and simply set
> cpumask_of(0) in the UP case.

Or cpumask_of(smp_processor_id()) which would also cover the non-CPU0 boot
cases (provided its called on the boot CPU.)

[PATCH 3/3] mach-u300: cleanup clockevent code

[Thomas Gleixner]

On Wed, 1 Jun 2011, Russell King - ARM Linux wrote:

> On Wed, Jun 01, 2011 at 10:34:46AM +0200, Thomas Gleixner wrote:
> > But yeah, we can make it conditional for SMP and simply set
> > cpumask_of(0) in the UP case.
> 
> Or cpumask_of(smp_processor_id()) which would also cover the non-CPU0 boot
> cases (provided its called on the boot CPU.)

Something like the below should work for both UP and SMP

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index c027d4f..739801b 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -173,6 +173,12 @@ static void clockevents_notify_released(void)
 	}
 }
 
+#ifdef CONFIG_SMP
+# define CPU_MASK_CHECK(x)		WARN_ON(!x)
+#else
+# define CPU_MASK_CHECK(x)		(!x)
+#endif
+
 /**
  * clockevents_register_device - register a clock event device
  * @dev:	device to register
@@ -182,7 +188,8 @@ void clockevents_register_device(struct clock_event_device *dev)
 	unsigned long flags;
 
 	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
-	BUG_ON(!dev->cpumask);
+	if (CPU_MASK_CHECK(dev->cpumask))
+		dev->cpumask = cpumask_of(smp_processor_id());
 
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 
 

[PATCH 3/3] mach-u300: cleanup clockevent code

[Linus Walleij]

On Wed, Jun 1, 2011 at 12:03 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> Something like the below should work for both UP and SMP

Works like a charm on U300
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij

[PATCH 3/3] mach-u300: cleanup clockevent code

[Stephen Boyd]

On 06/01/2011 03:03 AM, Thomas Gleixner wrote:
> Something like the below should work for both UP and SMP
>
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index c027d4f..739801b 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -173,6 +173,12 @@ static void clockevents_notify_released(void)
>  	}
>  }
>  
> +#ifdef CONFIG_SMP
> +# define CPU_MASK_CHECK(x)		WARN_ON(!x)
> +#else
> +# define CPU_MASK_CHECK(x)		(!x)
> +#endif
> +
>  /**
>   * clockevents_register_device - register a clock event device
>   * @dev:	device to register
> @@ -182,7 +188,8 @@ void clockevents_register_device(struct clock_event_device *dev)
>  	unsigned long flags;
>  
>  	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
> -	BUG_ON(!dev->cpumask);
> +	if (CPU_MASK_CHECK(dev->cpumask))
> +		dev->cpumask = cpumask_of(smp_processor_id());
>  
>  	raw_spin_lock_irqsave(&clockevents_lock, flags);

Won't this print a big WARNING on SMP_ON_UP=y and is_smp() == false
kernels? Is there a generic cross-architecture way to check for SMP at
runtime?

Also, I don't understand the original motivation for this change. The
assignment to cpumask was there in the u300 timer code already so the
commit text from Linus is a bit misleading/confusing.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 3/3] mach-u300: cleanup clockevent code

[Thomas Gleixner]

On Wed, 1 Jun 2011, Stephen Boyd wrote:
> On 06/01/2011 03:03 AM, Thomas Gleixner wrote:
> > Something like the below should work for both UP and SMP
> >
> > diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> > index c027d4f..739801b 100644
> > --- a/kernel/time/clockevents.c
> > +++ b/kernel/time/clockevents.c
> > @@ -173,6 +173,12 @@ static void clockevents_notify_released(void)
> >  	}
> >  }
> >  
> > +#ifdef CONFIG_SMP
> > +# define CPU_MASK_CHECK(x)		WARN_ON(!x)
> > +#else
> > +# define CPU_MASK_CHECK(x)		(!x)
> > +#endif
> > +
> >  /**
> >   * clockevents_register_device - register a clock event device
> >   * @dev:	device to register
> > @@ -182,7 +188,8 @@ void clockevents_register_device(struct clock_event_device *dev)
> >  	unsigned long flags;
> >  
> >  	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
> > -	BUG_ON(!dev->cpumask);
> > +	if (CPU_MASK_CHECK(dev->cpumask))
> > +		dev->cpumask = cpumask_of(smp_processor_id());
> >  
> >  	raw_spin_lock_irqsave(&clockevents_lock, flags);
> 
> Won't this print a big WARNING on SMP_ON_UP=y and is_smp() == false
> kernels? Is there a generic cross-architecture way to check for SMP at

Need to check that.

> runtime?
> 
> Also, I don't understand the original motivation for this change. The
> assignment to cpumask was there in the u300 timer code already so the
> commit text from Linus is a bit misleading/confusing.

The point is that we can remove these assignements for UP all over the
place and just do it in the core code.

[PATCH 3/3] mach-u300: cleanup clockevent code

[Thomas Gleixner]

On Thu, 2 Jun 2011, Thomas Gleixner wrote:
> On Wed, 1 Jun 2011, Stephen Boyd wrote:
> 
> Need to check that.
> 
> > runtime?

It's pretty simple and thanks for pointing it out!

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index c027d4f..e4c699d 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -182,7 +182,10 @@ void clockevents_register_device(struct clock_event_device *dev)
 	unsigned long flags;
 
 	BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
-	BUG_ON(!dev->cpumask);
+	if (!dev->cpumask) {
+		WARN_ON(num_possible_cpus() > 1);
+		dev->cpumask = cpumask_of(smp_processor_id());
+	}
 
 	raw_spin_lock_irqsave(&clockevents_lock, flags);