[RFC PATCH 0/5] clockevents: decouple broadcast mechanism from drivers

[RFC PATCH 0/5] clockevents: decouple broadcast mechanism from drivers

[Mark Rutland]

In some SMP systems, cpu-local timers may stop delivering interrupts
when in low power states, or not all CPUs may have local timers. To
support these systems we have a mechanism for broadcasting timer ticks
to other CPUs. This mechanism relies on the struct
clock_event_device::broadcast function pointer, which is a
driver-specific mechanism for broadcasting ticks to other CPUs.

As the broadcast mechanism is architecture-specific, placing the
broadcast function on struct clock_event_device ties each driver to a
single architecture. Additionally the driver or architecture backend
must handle the routing of broadcast ticks to the correct
clock_event_device, leading to duplication of the list of active
clock_event_devices.

These patches introduce a generic mechanism for handling the receipt of
timer broadcasts, and an optional architecture-specific broadcast
function which allows drivers to be decoupled from a particular
architecture will retaining support for timer tick broadcasts. These
mechanisms are wired up for the arm port, and have been boot-tested on a
pandaboard.

Thanks,
Mark.

Mark Rutland (5):
  ARM: remove useless guard in smp.c
  clockevents: Add generic timer broadcast receiver
  ARM: Use generic timer broadcast receive
  clockevents: Add generic timer broadcast function
  ARM: Add generic timer broadcast support

 arch/arm/Kconfig             |    1 +
 arch/arm/kernel/smp.c        |   15 ++-------------
 include/linux/clockchips.h   |    9 +++++++++
 kernel/time/Kconfig          |    4 ++++
 kernel/time/tick-broadcast.c |   30 ++++++++++++++++++++++++++++++
 5 files changed, 46 insertions(+), 13 deletions(-)

[RFC PATCH 1/5] ARM: remove useless guard in smp.c

[Mark Rutland]

Currently we only provide an implementation of smp_timer_broadcast in
smp.c if GENERIC_CLOCKEVENTS_BROADCAST is selected. As
smp_timer_broadcast is only used in smp.c, smp.c depends on SMP, and
GENERIC_CLOCKEVENTS_BROADCAST is selected by SMP, this is unnecessary.

This patch removes the redundant guard.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm/kernel/smp.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index fbc8b26..21954bc 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -473,14 +473,10 @@ static void ipi_timer(void)
 	evt->event_handler(evt);
 }
 
-#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 static void smp_timer_broadcast(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_TIMER);
 }
-#else
-#define smp_timer_broadcast	NULL
-#endif
 
 static void broadcast_timer_set_mode(enum clock_event_mode mode,
 	struct clock_event_device *evt)
-- 
1.7.0.4

[RFC PATCH 1/5] ARM: remove useless guard in smp.c

[Stephen Boyd]

On 12/18/12 04:06, Mark Rutland wrote:
> Currently we only provide an implementation of smp_timer_broadcast in
> smp.c if GENERIC_CLOCKEVENTS_BROADCAST is selected. As
> smp_timer_broadcast is only used in smp.c, smp.c depends on SMP, and
> GENERIC_CLOCKEVENTS_BROADCAST is selected by SMP, this is unnecessary.

You might want to add that GENERIC_CLOCKEVENTS_BROADCAST depends on
GENERIC_CLOCKEVENTS and SMP depends on GENERIC_CLOCKEVENTS too.

> This patch removes the redundant guard.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[RFC PATCH 1/5] ARM: remove useless guard in smp.c

[Mark Rutland]

On Tue, Dec 18, 2012 at 06:47:09PM +0000, Stephen Boyd wrote:
> On 12/18/12 04:06, Mark Rutland wrote:
> > Currently we only provide an implementation of smp_timer_broadcast in
> > smp.c if GENERIC_CLOCKEVENTS_BROADCAST is selected. As
> > smp_timer_broadcast is only used in smp.c, smp.c depends on SMP, and
> > GENERIC_CLOCKEVENTS_BROADCAST is selected by SMP, this is unnecessary.
> 
> You might want to add that GENERIC_CLOCKEVENTS_BROADCAST depends on
> GENERIC_CLOCKEVENTS and SMP depends on GENERIC_CLOCKEVENTS too.

Will do.

> > This patch removes the redundant guard.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> 
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks!

Mark.

[RFC PATCH 1/5] ARM: remove useless guard in smp.c

[Santosh Shilimkar]

On Tuesday 18 December 2012 05:36 PM, Mark Rutland wrote:
> Currently we only provide an implementation of smp_timer_broadcast in
> smp.c if GENERIC_CLOCKEVENTS_BROADCAST is selected. As
> smp_timer_broadcast is only used in smp.c, smp.c depends on SMP, and
> GENERIC_CLOCKEVENTS_BROADCAST is selected by SMP, this is unnecessary.
>
> This patch removes the redundant guard.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST isn't mandatory to
have SMP kernel working so the below select in ARM arch
kconfig not seems to entirely accurate. SMP kernel will
still boot with !GENERIC_CLOCKEVENTS_BROADCAST.

select GENERIC_CLOCKEVENTS_BROADCAST if SMP

The issue comes only for deeper CPU power C-states.
Anyway, you patch is correct from the current code point of
view.

Regards
Santosh

[RFC PATCH 1/5] ARM: remove useless guard in smp.c

[Mark Rutland]

On Fri, Dec 21, 2012 at 10:02:01AM +0000, Santosh Shilimkar wrote:
> On Tuesday 18 December 2012 05:36 PM, Mark Rutland wrote:
> > Currently we only provide an implementation of smp_timer_broadcast in
> > smp.c if GENERIC_CLOCKEVENTS_BROADCAST is selected. As
> > smp_timer_broadcast is only used in smp.c, smp.c depends on SMP, and
> > GENERIC_CLOCKEVENTS_BROADCAST is selected by SMP, this is unnecessary.
> >
> > This patch removes the redundant guard.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> CONFIG_GENERIC_CLOCKEVENTS_BROADCAST isn't mandatory to
> have SMP kernel working so the below select in ARM arch
> kconfig not seems to entirely accurate. SMP kernel will
> still boot with !GENERIC_CLOCKEVENTS_BROADCAST.
> 
> select GENERIC_CLOCKEVENTS_BROADCAST if SMP

Agreed, the selection is not entirely accurate. I'd be happy to see it made
more fine-grained (i.e. selected by platforms) instead.

If people want that I'm happy to drop this patch.

> The issue comes only for deeper CPU power C-states.
> Anyway, you patch is correct from the current code point of
> view.
> 
> Regards
> Santosh

Thanks,
Mark.

[RFC PATCH 2/5] clockevents: Add generic timer broadcast receiver

[Mark Rutland]

Currently the broadcast mechanism used for timers is abstracted by a
function pointer on struct clock_event_device. As the fundamental
mechanism for broadcast is architecture-specific, this ties each
clock_event_device driver to a single architecture, even where the
driver is otherwise generic.

This patch adds a standard path for the receipt of timer broadcasts, so
drivers and/or architecture backends need not manage redundant lists of
timers for the purpose of routing broadcast timer ticks.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 include/linux/clockchips.h   |    4 ++++
 kernel/time/tick-broadcast.c |   15 +++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 8a7096f..e1089aa 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -161,6 +161,10 @@ clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
 extern void clockevents_suspend(void);
 extern void clockevents_resume(void);
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+extern int tick_receive_broadcast(void);
+#endif
+
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 extern void clockevents_notify(unsigned long reason, void *arg);
 #else
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index f113755..c2dd022 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -125,6 +125,21 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 	return ret;
 }
 
+int tick_receive_broadcast(void)
+{
+	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *evt = td->evtdev;
+
+	if (!evt)
+		return -ENODEV;
+
+	if (!evt->event_handler)
+		return -EINVAL;
+
+	evt->event_handler(evt);
+	return 0;
+}
+
 /*
  * Broadcast the event to the cpus, which are set in the mask (mangled).
  */
-- 
1.7.0.4

[RFC PATCH 2/5] clockevents: Add generic timer broadcast receiver

[Stephen Boyd]

On 12/18/12 04:06, Mark Rutland wrote:
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index f113755..c2dd022 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -125,6 +125,21 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
>  	return ret;
>  }
>  
> +int tick_receive_broadcast(void)
> +{
> +	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
> +	struct clock_event_device *evt = td->evtdev;
> +
> +	if (!evt)
> +		return -ENODEV;
> +
> +	if (!evt->event_handler)
> +		return -EINVAL;
> +

Does this actually happen? It's not obvious to me how it does.

> +	evt->event_handler(evt);
> +	return 0;
> +}
> +
>  /*
>   * Broadcast the event to the cpus, which are set in the mask (mangled).
>   */


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[RFC PATCH 2/5] clockevents: Add generic timer broadcast receiver

[Mark Rutland]

On Tue, Dec 18, 2012 at 10:17:11PM +0000, Stephen Boyd wrote:
> On 12/18/12 04:06, Mark Rutland wrote:
> > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> > index f113755..c2dd022 100644
> > --- a/kernel/time/tick-broadcast.c
> > +++ b/kernel/time/tick-broadcast.c
> > @@ -125,6 +125,21 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
> >  	return ret;
> >  }
> >  
> > +int tick_receive_broadcast(void)
> > +{
> > +	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
> > +	struct clock_event_device *evt = td->evtdev;
> > +
> > +	if (!evt)
> > +		return -ENODEV;
> > +
> > +	if (!evt->event_handler)
> > +		return -EINVAL;
> > +
> 
> Does this actually happen? It's not obvious to me how it does.

Taking a look at it, I don't think it does. Any tick_device's
clock_event_device should have an event_handler. I'll drop this check.

A tick device might not have a clock_event_device registered in some cases --
the x86 lapic timer has to deal with interrupts pending across a kexec, where
interrupts are enabled before local timers are registered. I'm not sure how
common this problem might be though, and whether it should be dealt with here
or elsewhere.

> > +	evt->event_handler(evt);
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Broadcast the event to the cpus, which are set in the mask (mangled).
> >   */
> 
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

Thanks,
Mark.

[RFC PATCH 2/5] clockevents: Add generic timer broadcast receiver

[Santosh Shilimkar]

On Tuesday 18 December 2012 05:36 PM, Mark Rutland wrote:
> Currently the broadcast mechanism used for timers is abstracted by a
> function pointer on struct clock_event_device. As the fundamental
> mechanism for broadcast is architecture-specific, this ties each
> clock_event_device driver to a single architecture, even where the
> driver is otherwise generic.
>
> This patch adds a standard path for the receipt of timer broadcasts, so
> drivers and/or architecture backends need not manage redundant lists of
> timers for the purpose of routing broadcast timer ticks.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>   include/linux/clockchips.h   |    4 ++++
>   kernel/time/tick-broadcast.c |   15 +++++++++++++++
>   2 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 8a7096f..e1089aa 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -161,6 +161,10 @@ clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
>   extern void clockevents_suspend(void);
>   extern void clockevents_resume(void);
>
> +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> +extern int tick_receive_broadcast(void);
> +#endif
> +
As mentioned in earlier patch, CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
isn't must for SMP kernel and hence when build with
!GENERIC_CLOCKEVENTS_BROADCAST, $subject patch will break the build.

Below is the fix for the same. Feel free to fold it if you agree.

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..921568b 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -168,6 +168,11 @@ extern void tick_broadcast(const struct cpumask *mask);
  #define tick_broadcast	NULL
  #endif
  extern int tick_receive_broadcast(void);
+#else
+static inline int tick_receive_broadcast(void)
+{
+	return 0;
+}
  #endif

  #ifdef CONFIG_GENERIC_CLOCKEVENTS

[RFC PATCH 2/5] clockevents: Add generic timer broadcast receiver

[Mark Rutland]

On Fri, Dec 21, 2012 at 10:02:18AM +0000, Santosh Shilimkar wrote:
> On Tuesday 18 December 2012 05:36 PM, Mark Rutland wrote:
> > Currently the broadcast mechanism used for timers is abstracted by a
> > function pointer on struct clock_event_device. As the fundamental
> > mechanism for broadcast is architecture-specific, this ties each
> > clock_event_device driver to a single architecture, even where the
> > driver is otherwise generic.
> >
> > This patch adds a standard path for the receipt of timer broadcasts, so
> > drivers and/or architecture backends need not manage redundant lists of
> > timers for the purpose of routing broadcast timer ticks.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >   include/linux/clockchips.h   |    4 ++++
> >   kernel/time/tick-broadcast.c |   15 +++++++++++++++
> >   2 files changed, 19 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> > index 8a7096f..e1089aa 100644
> > --- a/include/linux/clockchips.h
> > +++ b/include/linux/clockchips.h
> > @@ -161,6 +161,10 @@ clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
> >   extern void clockevents_suspend(void);
> >   extern void clockevents_resume(void);
> >
> > +#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> > +extern int tick_receive_broadcast(void);
> > +#endif
> > +
> As mentioned in earlier patch, CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
> isn't must for SMP kernel and hence when build with
> !GENERIC_CLOCKEVENTS_BROADCAST, $subject patch will break the build.

I'd assumed that the broadcast receive path would be ifdef'd on
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST, and thus this wouldn't be a problem.
Though I guess it probably makes arch/platform code a bit nicer if it doesn't
have to ifdef everything.

> Below is the fix for the same. Feel free to fold it if you agree.

Will do.

> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 6634652..921568b 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -168,6 +168,11 @@ extern void tick_broadcast(const struct cpumask *mask);
>   #define tick_broadcast	NULL
>   #endif
>   extern int tick_receive_broadcast(void);
> +#else
> +static inline int tick_receive_broadcast(void)
> +{
> +	return 0;
> +}
>   #endif
> 
>   #ifdef CONFIG_GENERIC_CLOCKEVENTS
> 

Thanks,
Mark.

[RFC PATCH 3/5] ARM: Use generic timer broadcast receive

[Mark Rutland]

Currently, the ARM backend must maintain a redundant list of timers for
the purpose of centralising timer broadcast functionality. This prevents
sharing timer drivers across architectures.

This patch moves the pain of dealing with timer broadcasts to the core
clockevents tick broadcast code, which already maintains its own list
of timers.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm/kernel/smp.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 21954bc..59bf6d5 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -467,12 +467,6 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
  */
 static DEFINE_PER_CPU(struct clock_event_device, percpu_clockevent);
 
-static void ipi_timer(void)
-{
-	struct clock_event_device *evt = &__get_cpu_var(percpu_clockevent);
-	evt->event_handler(evt);
-}
-
 static void smp_timer_broadcast(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_TIMER);
@@ -586,7 +580,7 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 	case IPI_TIMER:
 		irq_enter();
-		ipi_timer();
+		tick_receive_broadcast();
 		irq_exit();
 		break;
 
-- 
1.7.0.4

[RFC PATCH 4/5] clockevents: Add generic timer broadcast function

[Mark Rutland]

Currently, the timer broadcast mechanism is defined by a function
pointer on struct clock_event_device. As the fundamental mechanism for
broadcast is architecture-specific, this means that clock_event_device
drivers cannot be shared across multiple architectures.

This patch adds an (optional) architecture-specific function for timer
tick broadcast, allowing drivers which may require broadcast
functionality to be shared across multiple architectures.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 include/linux/clockchips.h   |    5 +++++
 kernel/time/Kconfig          |    4 ++++
 kernel/time/tick-broadcast.c |   15 +++++++++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index e1089aa..6634652 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -162,6 +162,11 @@ extern void clockevents_suspend(void);
 extern void clockevents_resume(void);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+#ifdef CONFIG_ARCH_HAS_TICK_BROADCAST
+extern void tick_broadcast(const struct cpumask *mask);
+#else
+#define tick_broadcast	NULL
+#endif
 extern int tick_receive_broadcast(void);
 #endif
 
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 8601f0d..b696922 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -38,6 +38,10 @@ config GENERIC_CLOCKEVENTS_BUILD
 	default y
 	depends on GENERIC_CLOCKEVENTS
 
+# Architecture can handle broadcast in a driver-agnostic way
+config ARCH_HAS_TICK_BROADCAST
+	bool
+
 # Clockevents broadcasting infrastructure
 config GENERIC_CLOCKEVENTS_BROADCAST
 	bool
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index c2dd022..ec22a80 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -18,6 +18,7 @@
 #include <linux/percpu.h>
 #include <linux/profile.h>
 #include <linux/sched.h>
+#include <linux/smp.h>
 
 #include "tick-internal.h"
 
@@ -86,6 +87,12 @@ int tick_is_broadcast_device(struct clock_event_device *dev)
 	return (dev && tick_broadcast_device.evtdev == dev);
 }
 
+static void err_broadcast(const struct cpumask *mask)
+{
+	pr_crit_once("Attempted to broadcast tick, but no broadcast mechanism "
+		     "present. Some CPUs may be unresponsive.");
+}
+
 /*
  * Check, if the device is disfunctional and a place holder, which
  * needs to be handled by the broadcast device.
@@ -105,6 +112,14 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 	 */
 	if (!tick_device_is_functional(dev)) {
 		dev->event_handler = tick_handle_periodic;
+		if (!dev->broadcast)
+			dev->broadcast = tick_broadcast;
+		if (!dev->broadcast) {
+			pr_warn_once("%s depends on broadcast, but no "
+				     "broadcast function available\n",
+				     dev->name);
+			dev->broadcast = err_broadcast;
+		}
 		cpumask_set_cpu(cpu, tick_get_broadcast_mask());
 		tick_broadcast_start_periodic(tick_broadcast_device.evtdev);
 		ret = 1;
-- 
1.7.0.4

[RFC PATCH 4/5] clockevents: Add generic timer broadcast function

[Stephen Boyd]

Minor nit

On 12/18/12 04:06, Mark Rutland wrote:
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index c2dd022..ec22a80 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -86,6 +87,12 @@ int tick_is_broadcast_device(struct clock_event_device *dev)
>  	return (dev && tick_broadcast_device.evtdev == dev);
>  }
>  
> +static void err_broadcast(const struct cpumask *mask)
> +{
> +	pr_crit_once("Attempted to broadcast tick, but no broadcast mechanism "
> +		     "present. Some CPUs may be unresponsive.");

This is missing a newline. You may also want to put the string on a
single line so we can easily grep for it in the sources.

> @@ -105,6 +112,14 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
>  	 */
>  	if (!tick_device_is_functional(dev)) {
>  		dev->event_handler = tick_handle_periodic;
> +		if (!dev->broadcast)
> +			dev->broadcast = tick_broadcast;
> +		if (!dev->broadcast) {
> +			pr_warn_once("%s depends on broadcast, but no "
> +				     "broadcast function available\n",

Same one line comment here. I thought checkpatch didn't complain anymore.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[RFC PATCH 4/5] clockevents: Add generic timer broadcast function

[Mark Rutland]

On Tue, Dec 18, 2012 at 10:17:13PM +0000, Stephen Boyd wrote:
> Minor nit
> 
> On 12/18/12 04:06, Mark Rutland wrote:
> > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> > index c2dd022..ec22a80 100644
> > --- a/kernel/time/tick-broadcast.c
> > +++ b/kernel/time/tick-broadcast.c
> > @@ -86,6 +87,12 @@ int tick_is_broadcast_device(struct clock_event_device *dev)
> >  	return (dev && tick_broadcast_device.evtdev == dev);
> >  }
> >  
> > +static void err_broadcast(const struct cpumask *mask)
> > +{
> > +	pr_crit_once("Attempted to broadcast tick, but no broadcast mechanism "
> > +		     "present. Some CPUs may be unresponsive.");
> 
> This is missing a newline. You may also want to put the string on a
> single line so we can easily grep for it in the sources.

Whoops, fixed. I'll change both strings to be single line.

> > @@ -105,6 +112,14 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
> >  	 */
> >  	if (!tick_device_is_functional(dev)) {
> >  		dev->event_handler = tick_handle_periodic;
> > +		if (!dev->broadcast)
> > +			dev->broadcast = tick_broadcast;
> > +		if (!dev->broadcast) {
> > +			pr_warn_once("%s depends on broadcast, but no "
> > +				     "broadcast function available\n",
> 
> Same one line comment here. I thought checkpatch didn't complain anymore.

In fact it actively warns. Not sure how I missed that.

Thanks,
Mark.

[RFC PATCH 5/5] ARM: Add generic timer broadcast support

[Mark Rutland]

Implement timer_broadcast for the arm architecture, allowing for the use
of clock_event_device_drivers decoupled from the timer tick broadcast
mechanism.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm/Kconfig      |    1 +
 arch/arm/kernel/smp.c |    3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9759fec..d28a899 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -4,6 +4,7 @@ config ARM
 	select ARCH_BINFMT_ELF_RANDOMIZE_PIE
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAVE_CUSTOM_GPIO_H
+	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_WANT_IPC_PARSE_VERSION
 	select CPU_PM if (SUSPEND || CPU_IDLE)
 	select DCACHE_WORD_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && !CPU_BIG_ENDIAN
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 59bf6d5..ba1dc4a 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -467,7 +467,7 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
  */
 static DEFINE_PER_CPU(struct clock_event_device, percpu_clockevent);
 
-static void smp_timer_broadcast(const struct cpumask *mask)
+void tick_broadcast(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_TIMER);
 }
@@ -512,7 +512,6 @@ static void __cpuinit percpu_timer_setup(void)
 	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
 
 	evt->cpumask = cpumask_of(cpu);
-	evt->broadcast = smp_timer_broadcast;
 
 	if (!lt_ops || lt_ops->setup(evt))
 		broadcast_timer_setup(evt);
-- 
1.7.0.4

[RFC PATCH 0/5] clockevents: decouple broadcast mechanism from drivers

[Santosh Shilimkar]

Mark,

On Tuesday 18 December 2012 05:36 PM, Mark Rutland wrote:
> In some SMP systems, cpu-local timers may stop delivering interrupts
> when in low power states, or not all CPUs may have local timers. To
> support these systems we have a mechanism for broadcasting timer ticks
> to other CPUs. This mechanism relies on the struct
> clock_event_device::broadcast function pointer, which is a
> driver-specific mechanism for broadcasting ticks to other CPUs.
>
> As the broadcast mechanism is architecture-specific, placing the
> broadcast function on struct clock_event_device ties each driver to a
> single architecture. Additionally the driver or architecture backend
> must handle the routing of broadcast ticks to the correct
> clock_event_device, leading to duplication of the list of active
> clock_event_devices.
>
> These patches introduce a generic mechanism for handling the receipt of
> timer broadcasts, and an optional architecture-specific broadcast
> function which allows drivers to be decoupled from a particular
> architecture will retaining support for timer tick broadcasts. These
> mechanisms are wired up for the arm port, and have been boot-tested on a
> pandaboard.
>
Apart from the relevant comments given against couple of patches and
Stephen's printk string comment, the series looks pretty good to me.

I have tested the series with CPUIdle where the broadcast is actually
used actively.

So feel free to add,
Reviewed-tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[RFC PATCH 0/5] clockevents: decouple broadcast mechanism from drivers

[Mark Rutland]

On Fri, Dec 21, 2012 at 10:08:02AM +0000, Santosh Shilimkar wrote:
> Mark,
> 
> On Tuesday 18 December 2012 05:36 PM, Mark Rutland wrote:
> > In some SMP systems, cpu-local timers may stop delivering interrupts
> > when in low power states, or not all CPUs may have local timers. To
> > support these systems we have a mechanism for broadcasting timer ticks
> > to other CPUs. This mechanism relies on the struct
> > clock_event_device::broadcast function pointer, which is a
> > driver-specific mechanism for broadcasting ticks to other CPUs.
> >
> > As the broadcast mechanism is architecture-specific, placing the
> > broadcast function on struct clock_event_device ties each driver to a
> > single architecture. Additionally the driver or architecture backend
> > must handle the routing of broadcast ticks to the correct
> > clock_event_device, leading to duplication of the list of active
> > clock_event_devices.
> >
> > These patches introduce a generic mechanism for handling the receipt of
> > timer broadcasts, and an optional architecture-specific broadcast
> > function which allows drivers to be decoupled from a particular
> > architecture will retaining support for timer tick broadcasts. These
> > mechanisms are wired up for the arm port, and have been boot-tested on a
> > pandaboard.
> >
> Apart from the relevant comments given against couple of patches and
> Stephen's printk string comment, the series looks pretty good to me.
> 
> I have tested the series with CPUIdle where the broadcast is actually
> used actively.

Great!

> So feel free to add,
> Reviewed-tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Thanks!

Mark.