mach-u300: cleanup clockevent code

PATCH New Linus Walleij 5 participants 11 messages May 31, 2011 – Jun 2, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures High-resolution timers, timer wheel, clockevents

lore

Share & tools

lore

Linus Walleij

author

2011-05-31 21:09 UTC #0 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

From: Linus Walleij <[email protected]>

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 <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

---
 arch/arm/mach-u300/timer.c |   32 ++++++++++++--------------------
 1 files changed, 12 insertions(+), 20 deletions(-)

arch/arm/mach-u300/timer.c +12 ~~-20~~

index 18d7fa0..bd32dda 100644
@@ -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!

Signature · 1 line

1.7.3.2

Thomas Gleixner

2011-05-31 21:23 UTC #1 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

Quoted 5 lines · Linus Walleij

From: Linus Walleij <[email protected]>
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

Linus Walleij

2011-06-01 07:37 UTC #2 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

2011/5/31 Thomas Gleixner <[email protected]>:

Quoted 9 lines · Linus Walleij

Quoted 5 lines
From: Linus Walleij <[email protected]>
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

Thomas Gleixner

2011-06-01 08:34 UTC #3 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

Quoted 14 lines · Linus Walleij

2011/5/31 Thomas Gleixner <[email protected]>:
Quoted 9 lines · Linus Walleij
Quoted 5 lines
From: Linus Walleij <[email protected]>
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:

Quoted 7 lines

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

Linus Walleij

2011-06-01 08:53 UTC #4 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

2011/6/1 Thomas Gleixner <[email protected]>:

Quoted 5 lines

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?

Russell King - ARM Linux

2011-06-01 09:22 UTC #5 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

In reply to Thomas Gleixner

Quoted 2 lines · Thomas Gleixner

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.)

Thomas Gleixner

2011-06-01 10:03 UTC #6 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

Quoted 6 lines · Russell King - ARM Linux

Quoted 2 lines · Thomas Gleixner
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

kernel/time/clockevents.c +8 -1

index c027d4f..739801b 100644
@@ -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);

Linus Walleij

2011-06-01 11:23 UTC #7 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

Quoted 1 line · Thomas Gleixner

Something like the below should work for both UP and SMP

Works like a charm on U300

Tested-by Linus Walleij <[email protected]>

Thanks,
Linus Walleij

Stephen Boyd

2011-06-02 01:51 UTC #8 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

In reply to Thomas Gleixner

Quoted 20 lines · Thomas Gleixner

Something like the below should work for both UP and SMP

kernel/time/clockevents.c +6 -0

index c027d4f..739801b 100644
@@ -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

kernel/time/clockevents.c void clockevents_register_device(struct clock_event_device *dev) · line 194

Quoted 8 lines

kernel/time/clockevents.c +2 -1

@@ -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.

Signature · 2 lines

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

Thomas Gleixner

2011-06-02 09:35 UTC #9 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

Quoted 28 lines · Stephen Boyd

Quoted 27 lines · Thomas Gleixner

Something like the below should work for both UP and SMP

kernel/time/clockevents.c +8 -1

index c027d4f..739801b 100644
@@ -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());

Quoted 4 lines

kernel/time/clockevents.c void clockevents_register_device(struct clock_event_device *dev) · line 194
Quoted 1 line
kernel/time/clockevents.c +0 -0
 	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.

Quoted 5 lines

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.

Thomas Gleixner

2011-06-02 10:29 UTC #10 lore raw

PATCH 3/3 mach-u300: cleanup clockevent code

Quoted 5 lines · Thomas Gleixner

On Wed, 1 Jun 2011, Stephen Boyd wrote:
Need to check that.
Quoted 1 line
runtime?

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

kernel/time/clockevents.c +4 -1

index c027d4f..e4c699d 100644
@@ -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);