Skip to contentSkip to conversation

Consolidating GIC per-cpu interrupts

PATCH Superseded Marc Zyngier 3 participants 10 messages Aug 9, 2011 – Sep 9, 2011

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

v11 cover Consolidating GIC per-cpu interrupts

The current GIC per-cpu interrupts (aka PPIs) suffer from a number of problems:

- They use a completely separate scheme to handle the interrupts, mostly because the PPI concept doesn't really match the kernel view of an interrupt.
- PPIs can only be used by the timer code, unless we add more low-level assembly code.
- The local timer code can only be used by devices generating PPIs, and not SPIs.
- At least one platform (msm) has started implementing its own alternative scheme.
- Some low-level code gets duplicated, as usual...

As the previous solution which tried to map PPIs to normal interrupts has been proved to be buggy, I've opted to a much simpler scheme (based on Russell's input).

The proposed solution is to handle the interrupt using the same path as SPIs, with a common handler for all PPIs. Each PPI can be requested using gic_request_ppi(), similar to request_irq(). The local timer code is updated to reflect these changes.

Patches against v3.1-rc1 + Will Deacon's TWD patch ("ARM: twd:
register clockevents device before enabling PPI"). Tested on PB11MP, VE, OMAP4 (Panda) and Tegra (Harmony). As this patch series is quite different from the previous one, I've dropped all previous acks from platform maintainers.

#1 lore raw

PATCH v11 1/4 ARM: gic: consolidate PPI handling

PPI handling is a bit of an odd beast. It uses its own low level handling code and is hardwired to the local timers (hence lacking a registration interface).

Instead, switch the low handling to the normal SPI handling code.

This also allows the removal of some duplicated code.

Cc Kukjin Kim <[email protected]>
Cc David Brown <[email protected]>
Cc Bryan Huntsman <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Paul Mundt <[email protected]>
Cc Magnus Damm <[email protected]>
---
 arch/arm/common/gic.c                             |   69 +++++++++++++++++++-
 arch/arm/include/asm/entry-macro-multi.S          |    7 --
 arch/arm/include/asm/hardirq.h                    |    3 -
 arch/arm/include/asm/hardware/entry-macro-gic.S   |   19 +-----
 arch/arm/include/asm/localtimer.h                 |    6 +-
 arch/arm/include/asm/smp.h                        |    5 --
 arch/arm/kernel/irq.c                             |    3 -
 arch/arm/kernel/smp.c                             |   27 ++------
 arch/arm/mach-exynos4/include/mach/entry-macro.S  |    6 +--
 arch/arm/mach-msm/board-msm8x60.c                 |   11 ---
 arch/arm/mach-msm/include/mach/entry-macro-qgic.S |   73 +--------------------
 arch/arm/mach-omap2/include/mach/entry-macro.S    |   14 +----
 arch/arm/mach-shmobile/entry-intc.S               |    3 -
 arch/arm/mach-shmobile/include/mach/entry-macro.S |    3 -
 14 files changed, 82 insertions(+), 167 deletions(-)
arch/arm/common/gic.c +68 -1
index 3227ca9..8adc002 100644
@@ -28,10 +28,14 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/percpu.h>
+#include <linux/slab.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
 #include <asm/hardware/gic.h>
+#include <asm/localtimer.h>
 
 static DEFINE_SPINLOCK(irq_controller_lock);
 
@@ -255,12 +259,39 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
+#ifdef CONFIG_LOCAL_TIMERS
+#define gic_ppi_handler		percpu_timer_handler
+#else
+static irqreturn_t gic_ppi_handler(int irq, void *dev_id)
+{
+	return IRQ_NONE;
+}
+#endif
+
+#define PPI_IRQACT(nr)						\
+	{							\
+		.handler	= gic_ppi_handler,		\
+		.flags		= IRQF_PERCPU | IRQF_TIMER,	\
+		.irq		= nr,				\
+		.name		= "PPI-" # nr,			\
+	}
+
+static struct irqaction ppi_irqaction_template[16] __initdata = {
+	PPI_IRQACT(0),  PPI_IRQACT(1),  PPI_IRQACT(2),  PPI_IRQACT(3),
+	PPI_IRQACT(4),  PPI_IRQACT(5),  PPI_IRQACT(6),  PPI_IRQACT(7),
+	PPI_IRQACT(8),  PPI_IRQACT(9),  PPI_IRQACT(10), PPI_IRQACT(11),
+	PPI_IRQACT(12), PPI_IRQACT(13), PPI_IRQACT(14), PPI_IRQACT(15),
+};
+
+static struct irqaction *ppi_irqaction;
+
 static void __init gic_dist_init(struct gic_chip_data *gic,
 	unsigned int irq_start)
 {
 	unsigned int gic_irqs, irq_limit, i;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
+	u32 nrppis = 0, ppi_base = 0;
 
 	cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;
@@ -277,6 +308,28 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 		gic_irqs = 1020;
 
 	/*
+	 * Nobody would be insane enough to use PPIs on a secondary
+	 * GIC, right?
+	 */
+	if (gic == &gic_data[0]) {
+		nrppis = 16 - (irq_start & 15);
+		ppi_base = gic->irq_offset + 32 - nrppis;
+
+		ppi_irqaction = kmemdup(&ppi_irqaction_template[16 - nrppis],
+					sizeof(*ppi_irqaction) * nrppis,
+					GFP_KERNEL);
+
+		if (nrppis && !ppi_irqaction) {
+			pr_err("GIC: Can't allocate PPI memory");
+			nrppis = 0;
+			ppi_base = 0;
+		}
+	}
+
+	pr_info("Configuring GIC with %d sources (%d PPIs)\n",
+		gic_irqs, (gic == &gic_data[0]) ? nrppis : 0);
+
+	/*
 	 * Set all global interrupts to be level triggered, active low.
 	 */
 	for (i = 32; i < gic_irqs; i += 16)
@@ -311,7 +364,21 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
-	for (i = irq_start; i < irq_limit; i++) {
+	for (i = 0; i < nrppis; i++) {
+		int ppi = i + ppi_base;
+		int err;
+
+		irq_set_chip_and_handler(ppi, &gic_chip, handle_percpu_irq);
+		irq_set_chip_data(ppi, gic);
+		irq_set_status_flags(ppi, IRQ_NOAUTOEN | IRQ_NOPROBE);
+
+		err = setup_irq(ppi, &ppi_irqaction[i]);
+		if (err)
+			pr_err("GIC: can't setup IRQ%d PPI%d (%d)\n",
+			       i, ppi, err);
+	}
+
+	for (i = irq_start + nrppis; i < irq_limit; i++) {
 		irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
 		irq_set_chip_data(i, gic);
 		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
arch/arm/include/asm/entry-macro-multi.S +0 -7
index 2f1e209..88d6181 100644
@@ -25,13 +25,6 @@
 	movne	r1, sp
 	adrne	lr, BSYM(1b)
 	bne	do_IPI
-
-#ifdef CONFIG_LOCAL_TIMERS
-	test_for_ltirq r0, r2, r6, lr
-	movne	r0, sp
-	adrne	lr, BSYM(1b)
-	bne	do_local_timer
-#endif
 #endif
 9997:
 	.endm
arch/arm/include/asm/hardirq.h +0 -3
index 89ad180..ddf07a9 100644
@@ -9,9 +9,6 @@
 
 typedef struct {
 	unsigned int __softirq_pending;
-#ifdef CONFIG_LOCAL_TIMERS
-	unsigned int local_timer_irqs;
-#endif
 #ifdef CONFIG_SMP
 	unsigned int ipi_irqs[NR_IPI];
 #endif
arch/arm/include/asm/hardware/entry-macro-gic.S +2 -17
index c115b82..74ebc80 100644
@@ -22,15 +22,11 @@
  * interrupt controller spec.  To wit:
  *
  * Interrupts 0-15 are IPI
- * 16-28 are reserved
- * 29-31 are local.  We allow 30 to be used for the watchdog.
+ * 16-31 are local.  We allow 30 to be used for the watchdog.
  * 32-1020 are global
  * 1021-1022 are reserved
  * 1023 is "spurious" (no interrupt)
  *
- * For now, we ignore all local interrupts so only return an interrupt if it's
- * between 30 and 1020.  The test_for_ipi routine below will pick up on IPIs.
- *
  * A simple read from the controller will tell us the number of the highest
  * priority enabled interrupt.  We then just need to check whether it is in the
  * valid range for an IRQ (30-1020 inclusive).
@@ -43,7 +39,7 @@
 
 	ldr	\tmp, =1021
 	bic     \irqnr, \irqstat, #0x1c00
-	cmp     \irqnr, #29
+	cmp     \irqnr, #15
 	cmpcc	\irqnr, \irqnr
 	cmpne	\irqnr, \tmp
 	cmpcs	\irqnr, \irqnr
@@ -62,14 +58,3 @@
 	strcc	\irqstat, [\base, #GIC_CPU_EOI]
 	cmpcs	\irqnr, \irqnr
 	.endm
-
-/* As above, this assumes that irqstat and base are preserved.. */
-
-	.macro test_for_ltirq, irqnr, irqstat, base, tmp
-	bic	\irqnr, \irqstat, #0x1c00
-	mov 	\tmp, #0
-	cmp	\irqnr, #29
-	moveq	\tmp, #1
-	streq	\irqstat, [\base, #GIC_CPU_EOI]
-	cmp	\tmp, #0
-	.endm
arch/arm/include/asm/localtimer.h +4 -2
index 080d74f..e3663f7 100644
@@ -10,6 +10,8 @@
 #ifndef __ASM_ARM_LOCALTIMER_H
 #define __ASM_ARM_LOCALTIMER_H
 
+#include <linux/interrupt.h>
+
 struct clock_event_device;
 
 /*
@@ -18,9 +20,9 @@ struct clock_event_device;
 void percpu_timer_setup(void);
 
 /*
- * Called from assembly, this is the local timer IRQ handler
+ * Per-cpu timer IRQ handler
  */
-asmlinkage void do_local_timer(struct pt_regs *);
+irqreturn_t percpu_timer_handler(int irq, void *dev_id);
 
 
 #ifdef CONFIG_LOCAL_TIMERS
arch/arm/include/asm/smp.h +0 -5
index e42d96a..73ec155 100644
@@ -88,9 +88,4 @@ extern void platform_cpu_enable(unsigned int cpu);
 extern void arch_send_call_function_single_ipi(int cpu);
 extern void arch_send_call_function_ipi_mask(const struct cpumask *mask);
 
-/*
- * show local interrupt info
- */
-extern void show_local_irqs(struct seq_file *, int);
-
 #endif /* ifndef __ASM_ARM_SMP_H */
arch/arm/kernel/irq.c +0 -3
index de3dcab..8f24dc9 100644
@@ -59,9 +59,6 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 #ifdef CONFIG_SMP
 	show_ipi_list(p, prec);
 #endif
-#ifdef CONFIG_LOCAL_TIMERS
-	show_local_irqs(p, prec);
-#endif
 	seq_printf(p, "%*s: %10lu\n", prec, "Err", irq_err_count);
 	return 0;
 }
arch/arm/kernel/smp.c +5 -22
index 8e60a4f..bf11ffe 100644
@@ -453,10 +453,6 @@ u64 smp_irq_stat_cpu(unsigned int cpu)
 	for (i = 0; i < NR_IPI; i++)
 		sum += __get_irq_stat(cpu, ipi_irqs[i]);
 
-#ifdef CONFIG_LOCAL_TIMERS
-	sum += __get_irq_stat(cpu, local_timer_irqs);
-#endif
-
 	return sum;
 }
 
@@ -474,29 +470,16 @@ static void ipi_timer(void)
 }
 
 #ifdef CONFIG_LOCAL_TIMERS
-asmlinkage void __exception_irq_entry do_local_timer(struct pt_regs *regs)
+irqreturn_t percpu_timer_handler(int irq, void *dev_id)
 {
-	struct pt_regs *old_regs = set_irq_regs(regs);
-	int cpu = smp_processor_id();
+	struct clock_event_device *evt = &__get_cpu_var(percpu_clockevent);
 
 	if (local_timer_ack()) {
-		__inc_irq_stat(cpu, local_timer_irqs);
-		ipi_timer();
+		evt->event_handler(evt);
+		return IRQ_HANDLED;
 	}
 
-	set_irq_regs(old_regs);
-}
-
-void show_local_irqs(struct seq_file *p, int prec)
-{
-	unsigned int cpu;
-
-	seq_printf(p, "%*s: ", prec, "LOC");
-
-	for_each_present_cpu(cpu)
-		seq_printf(p, "%10u ", __get_irq_stat(cpu, local_timer_irqs));
-
-	seq_printf(p, " Local timer interrupts\n");
+	return IRQ_NONE;
 }
 #endif
 
arch/arm/mach-exynos4/include/mach/entry-macro.S +1 -5
index d7a1e28..807d05d 100644
@@ -55,7 +55,7 @@
 
 		bic     \irqnr, \irqstat, #0x1c00
 
-		cmp     \irqnr, #29
+		cmp     \irqnr, #15
 		cmpcc	\irqnr, \irqnr
 		cmpne	\irqnr, \tmp
 		cmpcs	\irqnr, \irqnr
@@ -77,7 +77,3 @@
 		cmpcs	\irqnr, \irqnr
 		.endm
 
-		/* As above, this assumes that irqstat and base are preserved.. */
-
-		.macro test_for_ltirq, irqnr, irqstat, base, tmp
-		.endm
arch/arm/mach-msm/board-msm8x60.c +0 -11
index 1163b6f..d70a2f6 100644
@@ -36,8 +36,6 @@ static void __init msm8x60_map_io(void)
 
 static void __init msm8x60_init_irq(void)
 {
-	unsigned int i;
-
 	gic_init(0, GIC_PPI_START, MSM_QGIC_DIST_BASE,
 		 (void *)MSM_QGIC_CPU_BASE);
 
@@ -49,15 +47,6 @@ static void __init msm8x60_init_irq(void)
 	 */
 	if (!machine_is_msm8x60_sim())
 		writel(0x0000FFFF, MSM_QGIC_DIST_BASE + GIC_DIST_ENABLE_SET);
-
-	/* FIXME: Not installing AVS_SVICINT and AVS_SVICINTSWDONE yet
-	 * as they are configured as level, which does not play nice with
-	 * handle_percpu_irq.
-	 */
-	for (i = GIC_PPI_START; i < GIC_SPI_START; i++) {
-		if (i != AVS_SVICINT && i != AVS_SVICINTSWDONE)
-			irq_set_handler(i, handle_percpu_irq);
-	}
 }
 
 static void __init msm8x60_init(void)
arch/arm/mach-msm/include/mach/entry-macro-qgic.S +1 -72

82 lines not on this page · raw message

arch/arm/mach-omap2/include/mach/entry-macro.S +1 -13
index ceb8b7e..feb90a1 100644
@@ -78,7 +78,7 @@
 4401:		ldr     \irqstat, [\base, #GIC_CPU_INTACK]
 		ldr     \tmp, =1021
 		bic     \irqnr, \irqstat, #0x1c00
-		cmp     \irqnr, #29
+		cmp     \irqnr, #15
 		cmpcc   \irqnr, \irqnr
 		cmpne   \irqnr, \tmp
 		cmpcs   \irqnr, \irqnr
@@ -101,18 +101,6 @@
 		it	cs
 		cmpcs	\irqnr, \irqnr
 		.endm
-
-		/* As above, this assumes that irqstat and base are preserved */
-
-		.macro test_for_ltirq, irqnr, irqstat, base, tmp
-		bic	\irqnr, \irqstat, #0x1c00
-		mov 	\tmp, #0
-		cmp	\irqnr, #29
-		itt	eq
-		moveq	\tmp, #1
-		streq	\irqstat, [\base, #GIC_CPU_EOI]
-		cmp	\tmp, #0
-		.endm
 #endif	/* CONFIG_SMP */
 
 #else	/* MULTI_OMAP2 */
arch/arm/mach-shmobile/entry-intc.S +0 -3
index cac0a7a..1a1c00c 100644
@@ -51,7 +51,4 @@
 	.macro  test_for_ipi, irqnr, irqstat, base, tmp
 	.endm
 
-	.macro  test_for_ltirq, irqnr, irqstat, base, tmp
-	.endm
-
 	arch_irq_handler shmobile_handle_irq_intc
arch/arm/mach-shmobile/include/mach/entry-macro.S +0 -3
index d791f10..8d4a416 100644
@@ -27,8 +27,5 @@
 	.macro  test_for_ipi, irqnr, irqstat, base, tmp
 	.endm
 
-	.macro  test_for_ltirq, irqnr, irqstat, base, tmp
-	.endm
-
 	.macro  arch_ret_to_user, tmp1, tmp2
 	.endm
Signature · 1 line
1.7.0.4
#2 lore raw

PATCH v11 2/4 ARM: gic: Add PPI registration interface

In reply to Marc Zyngier

This patch makes it possible to request (and free) a PPI. It thus makes it useable for more than just the local timers.

Update TWD and MSM timers to use that functionnality.

Based on an earlier patch by Russell King.

---
 arch/arm/common/gic.c               |   94 ++++++++++++++++++++++++++++------
 arch/arm/include/asm/hardware/gic.h |    5 ++-
 arch/arm/include/asm/localtimer.h   |   11 ++++-
 arch/arm/include/asm/smp_twd.h      |    1 +
 arch/arm/kernel/smp.c               |    4 +-
 arch/arm/kernel/smp_twd.c           |   15 +++++-
 arch/arm/mach-msm/timer.c           |   56 +++++++++++----------
 7 files changed, 137 insertions(+), 49 deletions(-)
arch/arm/common/gic.c +77 -17
index 8adc002..9022836 100644
@@ -35,7 +35,6 @@
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
 #include <asm/hardware/gic.h>
-#include <asm/localtimer.h>
 
 static DEFINE_SPINLOCK(irq_controller_lock);
 
@@ -259,14 +258,25 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
-#ifdef CONFIG_LOCAL_TIMERS
-#define gic_ppi_handler		percpu_timer_handler
-#else
+struct gic_action {
+	irq_handler_t handler;
+	void *data;
+};
+
+static DEFINE_PER_CPU(struct gic_action *, gic_ppi_action);
+static unsigned int gic_nr_ppis, gic_ppi_base;
+
 static irqreturn_t gic_ppi_handler(int irq, void *dev_id)
 {
+	unsigned int ppi = irq - gic_ppi_base;
+	struct gic_action *act;
+
+	act = &__get_cpu_var(gic_ppi_action)[ppi];
+	if (likely(act->handler))
+		return act->handler(irq, act->data);
+
 	return IRQ_NONE;
 }
-#endif
 
 #define PPI_IRQACT(nr)						\
 	{							\
@@ -291,6 +301,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	unsigned int gic_irqs, irq_limit, i;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
+	u32 dist_ctr, nrcpus;
 	u32 nrppis = 0, ppi_base = 0;
 
 	cpumask |= cpumask << 8;
@@ -302,11 +313,14 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	 * Find out how many interrupts are supported.
 	 * The GIC only supports up to 1020 interrupt sources.
 	 */
-	gic_irqs = readl_relaxed(base + GIC_DIST_CTR) & 0x1f;
-	gic_irqs = (gic_irqs + 1) * 32;
+	dist_ctr = readl_relaxed(base + GIC_DIST_CTR);
+	gic_irqs = ((dist_ctr & 0x1f) + 1) * 32;
 	if (gic_irqs > 1020)
 		gic_irqs = 1020;
 
+	/* Find out how many CPUs are supported (8 max). */
+	nrcpus = ((dist_ctr >> 5) & 7) + 1;
+
 	/*
 	 * Nobody would be insane enough to use PPIs on a secondary
 	 * GIC, right?
@@ -324,6 +338,19 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 			nrppis = 0;
 			ppi_base = 0;
 		}
+
+		for (i = 0; i < nrcpus; i++) {
+			struct gic_action **ppiacts;
+
+			ppiacts = &per_cpu(gic_ppi_action, i);
+			*ppiacts = kzalloc(sizeof(*gic_ppi_action) * nrppis,
+					   GFP_KERNEL);
+			if (!*ppiacts)
+				pr_err("GIC: Can't allocate PPI CPU%d memory", i);
+		}
+
+		gic_nr_ppis = nrppis;
+		gic_ppi_base = ppi_base;
 	}
 
 	pr_info("Configuring GIC with %d sources (%d PPIs)\n",
@@ -387,6 +414,49 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	writel_relaxed(1, base + GIC_DIST_CTRL);
 }
 
+int gic_request_ppi(unsigned int irq, irq_handler_t handler, void *data)
+{
+	struct gic_action *act;
+	unsigned long flags;
+	unsigned int ppi = irq - gic_ppi_base;
+	int ret = -EBUSY;
+
+	if (ppi >= gic_nr_ppis)
+		return -EINVAL;
+
+	local_irq_save(flags);
+	act = &__get_cpu_var(gic_ppi_action)[ppi];
+	if (!act->handler) {
+		act->handler = handler;
+		act->data = data;
+
+		gic_unmask_irq(irq_get_irq_data(irq));
+		ret = 0;
+	}
+	local_irq_restore(flags);
+
+	return ret;
+}
+
+void gic_free_ppi(unsigned int irq, void *data)
+{
+	struct gic_action *act;
+	unsigned long flags;
+	unsigned int ppi = irq - gic_ppi_base;
+
+	if (ppi >= gic_nr_ppis)
+		return;
+
+	local_irq_save(flags);
+	act = &__get_cpu_var(gic_ppi_action)[ppi];
+	if (act->data == data) {
+		gic_mask_irq(irq_get_irq_data(irq));
+		act->handler = NULL;
+		act->data = NULL;
+	}
+	local_irq_restore(flags);
+}
+
 static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
 {
 	void __iomem *dist_base = gic->dist_base;
@@ -436,16 +506,6 @@ void __cpuinit gic_secondary_init(unsigned int gic_nr)
 	gic_cpu_init(&gic_data[gic_nr]);
 }
 
-void __cpuinit gic_enable_ppi(unsigned int irq)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	irq_set_status_flags(irq, IRQ_NOPROBE);
-	gic_unmask_irq(irq_get_irq_data(irq));
-	local_irq_restore(flags);
-}
-
 #ifdef CONFIG_SMP
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 {
arch/arm/include/asm/hardware/gic.h +4 -1
index 435d3f8..b37780f 100644
@@ -33,6 +33,8 @@
 #define GIC_DIST_SOFTINT		0xf00
 
 #ifndef __ASSEMBLY__
+#include <linux/interrupt.h>
+
 extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
@@ -40,7 +42,8 @@ void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
-void gic_enable_ppi(unsigned int);
+int gic_request_ppi(unsigned int irq, irq_handler_t handler, void *data);
+void gic_free_ppi(unsigned int irq, void *data);
 
 struct gic_chip_data {
 	unsigned int irq_offset;
arch/arm/include/asm/localtimer.h +10 -1
index e3663f7..136a31b 100644
@@ -24,7 +24,6 @@ void percpu_timer_setup(void);
  */
 irqreturn_t percpu_timer_handler(int irq, void *dev_id);
 
-
 #ifdef CONFIG_LOCAL_TIMERS
 
 #ifdef CONFIG_HAVE_ARM_TWD
@@ -32,6 +31,7 @@ irqreturn_t percpu_timer_handler(int irq, void *dev_id);
 #include "smp_twd.h"
 
 #define local_timer_ack()	twd_timer_ack()
+#define local_timer_stop(c)	twd_timer_stop((c))
 
 #else
 
@@ -41,6 +41,11 @@ irqreturn_t percpu_timer_handler(int irq, void *dev_id);
  */
 int local_timer_ack(void);
 
+/*
+ * Stop the local timer
+ */
+void local_timer_stop(struct clock_event_device *);
+
 #endif
 
 /*
@@ -54,6 +59,10 @@ static inline int local_timer_setup(struct clock_event_device *evt)
 {
 	return -ENXIO;
 }
+
+static inline void local_timer_stop(struct clock_event_device *evt)
+{
+}
 #endif
 
 #endif
arch/arm/include/asm/smp_twd.h +1 -0
index fed9981..6923037 100644
@@ -24,5 +24,6 @@ extern void __iomem *twd_base;
 
 int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+void twd_timer_stop(struct clock_event_device *);
 
 #endif
arch/arm/kernel/smp.c +2 -2
index bf11ffe..93b7c9a 100644
@@ -472,7 +472,7 @@ static void ipi_timer(void)
 #ifdef CONFIG_LOCAL_TIMERS
 irqreturn_t percpu_timer_handler(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = &__get_cpu_var(percpu_clockevent);
+	struct clock_event_device *evt = dev_id;
 
 	if (local_timer_ack()) {
 		evt->event_handler(evt);
@@ -533,7 +533,7 @@ static void percpu_timer_stop(void)
 	unsigned int cpu = smp_processor_id();
 	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
 
-	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	local_timer_stop(evt);
 }
 #endif
 
arch/arm/kernel/smp_twd.c +13 -2
index 01c1862..1bf0631 100644
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 
 #include <asm/smp_twd.h>
+#include <asm/localtimer.h>
 #include <asm/hardware/gic.h>
 
 /* set up by the platform code */
@@ -80,6 +81,12 @@ int twd_timer_ack(void)
 	return 0;
 }
 
+void twd_timer_stop(struct clock_event_device *clk)
+{
+	twd_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+	gic_free_ppi(clk->irq, clk);
+}
+
 static void __cpuinit twd_calibrate_rate(void)
 {
 	unsigned long count;
@@ -124,6 +131,8 @@ static void __cpuinit twd_calibrate_rate(void)
  */
 void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 {
+	int err;
+
 	twd_calibrate_rate();
 
 	clk->name = "local_timer";
@@ -139,6 +148,8 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 
 	clockevents_register_device(clk);
 
-	/* Make sure our local interrupt controller has this enabled */
-	gic_enable_ppi(clk->irq);
+	err = gic_request_ppi(clk->irq, percpu_timer_handler, clk);
+	if (err)
+		pr_err("%s: can't register interrupt %d on cpu %d (%d)\n",
+		       clk->name, clk->irq, smp_processor_id(), err);
 }
arch/arm/mach-msm/timer.c +30 -26

119 lines not on this page · raw message

Signature · 1 line
1.7.0.4
#3 lore raw

PATCH v11 3/4 ARM: local timers: drop local_timer_ack()

In reply to Marc Zyngier

Only TWD uses local_timer_ack() to perform something useful. Make it the real (private) interrupt handler, remove the common interrupt handler as well as the useless stubs from MCT and MSM.

Cc Kukjin Kim <[email protected]>
Cc David Brown <[email protected]>
---
 arch/arm/include/asm/localtimer.h |   12 ------------
 arch/arm/include/asm/smp_twd.h    |    1 -
 arch/arm/kernel/smp.c             |   14 --------------
 arch/arm/kernel/smp_twd.c         |   17 +++++++----------
 arch/arm/mach-exynos4/mct.c       |    6 ------
 arch/arm/mach-msm/timer.c         |    6 ------
 6 files changed, 7 insertions(+), 49 deletions(-)
arch/arm/include/asm/localtimer.h +0 -12
index 136a31b..f5e1cec 100644
@@ -19,29 +19,17 @@ struct clock_event_device;
  */
 void percpu_timer_setup(void);
 
-/*
- * Per-cpu timer IRQ handler
- */
-irqreturn_t percpu_timer_handler(int irq, void *dev_id);
-
 #ifdef CONFIG_LOCAL_TIMERS
 
 #ifdef CONFIG_HAVE_ARM_TWD
 
 #include "smp_twd.h"
 
-#define local_timer_ack()	twd_timer_ack()
 #define local_timer_stop(c)	twd_timer_stop((c))
 
 #else
 
 /*
- * Platform provides this to acknowledge a local timer IRQ.
- * Returns true if the local timer IRQ is to be processed.
- */
-int local_timer_ack(void);
-
-/*
  * Stop the local timer
  */
 void local_timer_stop(struct clock_event_device *);
arch/arm/include/asm/smp_twd.h +0 -1
index 6923037..ef9ffba 100644
@@ -22,7 +22,6 @@ struct clock_event_device;
 
 extern void __iomem *twd_base;
 
-int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
 void twd_timer_stop(struct clock_event_device *);
 
arch/arm/kernel/smp.c +0 -14
index 93b7c9a..5d1f2e2 100644
@@ -469,20 +469,6 @@ static void ipi_timer(void)
 	irq_exit();
 }
 
-#ifdef CONFIG_LOCAL_TIMERS
-irqreturn_t percpu_timer_handler(int irq, void *dev_id)
-{
-	struct clock_event_device *evt = dev_id;
-
-	if (local_timer_ack()) {
-		evt->event_handler(evt);
-		return IRQ_HANDLED;
-	}
-
-	return IRQ_NONE;
-}
-#endif
-
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 static void smp_timer_broadcast(const struct cpumask *mask)
 {
arch/arm/kernel/smp_twd.c +7 -10
index 1bf0631..7156f09 100644
@@ -65,20 +65,17 @@ static int twd_set_next_event(unsigned long evt,
 	return 0;
 }
 
-/*
- * local_timer_ack: checks for a local timer interrupt.
- *
- * If a local timer interrupt has occurred, acknowledge and return 1.
- * Otherwise, return 0.
- */
-int twd_timer_ack(void)
+static irqreturn_t twd_timer_handler(int irq, void *dev_id)
 {
+	struct clock_event_device *clk = dev_id;
+
 	if (__raw_readl(twd_base + TWD_TIMER_INTSTAT)) {
 		__raw_writel(1, twd_base + TWD_TIMER_INTSTAT);
-		return 1;
+		clk->event_handler(clk);
+		return IRQ_HANDLED;
 	}
 
-	return 0;
+	return IRQ_NONE;
 }
 
 void twd_timer_stop(struct clock_event_device *clk)
@@ -148,7 +145,7 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 
 	clockevents_register_device(clk);
 
-	err = gic_request_ppi(clk->irq, percpu_timer_handler, clk);
+	err = gic_request_ppi(clk->irq, twd_timer_handler, clk);
 	if (err)
 		pr_err("%s: can't register interrupt %d on cpu %d (%d)\n",
 		       clk->name, clk->irq, smp_processor_id(), err);
arch/arm/mach-exynos4/mct.c +0 -6
index 1ae059b..194fc6d 100644
@@ -393,12 +393,6 @@ void __cpuinit local_timer_setup(struct clock_event_device *evt)
 {
 	exynos4_mct_tick_init(evt);
 }
-
-int local_timer_ack(void)
-{
-	return 0;
-}
-
 #endif /* CONFIG_LOCAL_TIMERS */
 
 static void __init exynos4_timer_resources(void)
arch/arm/mach-msm/timer.c +0 -6
index 1bcdf66..94e6fc5 100644
@@ -309,12 +309,6 @@ void local_timer_stop(struct clock_event_device *evt)
 	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
 	gic_free_ppi(evt->irq, evt);
 }
-
-inline int local_timer_ack(void)
-{
-	return 1;
-}
-
 #endif
 
 struct sys_timer msm_timer = {
Signature · 1 line
1.7.0.4
#4 lore raw

PATCH v11 4/4 ARM: gic: add compute_irqnr macro for exynos4

In reply to Marc Zyngier

exynos4 has a full copy of entry-macro-gic.S, just for the sake of an offset added to the IRQ number read from the GIC.

Add a compute_irqnr macro to entry-macro-gic.S so that any platform can add it's own hook without having to copy the whole file again.

---
 arch/arm/include/asm/hardware/entry-macro-gic.S  |    3 +
 arch/arm/mach-exynos4/include/mach/entry-macro.S |   63 ++-------------------
 2 files changed, 9 insertions(+), 57 deletions(-)
arch/arm/include/asm/hardware/entry-macro-gic.S +3 -0
index 74ebc80..fbb50dc 100644
@@ -43,6 +43,9 @@
 	cmpcc	\irqnr, \irqnr
 	cmpne	\irqnr, \tmp
 	cmpcs	\irqnr, \irqnr
+#ifdef HAVE_GIC_BASE_OFFSET
+	compute_irqnr	\irqnr, \tmp
+#endif
 	.endm
 
 /* We assume that irqstat (the raw value of the IRQ acknowledge
arch/arm/mach-exynos4/include/mach/entry-macro.S +6 -57

77 lines not on this page · raw message

Signature · 1 line
1.7.0.4
#5 lore raw

v11 cover Consolidating GIC per-cpu interrupts

In reply to Marc Zyngier

Thomas,

Could you _please_ look Marcs patch series and give an opinion on it. I've also attached my patch to this reply too, which is an alternative approach to Marcs.

Quoted 66 lines · Marc Zyngier

The current GIC per-cpu interrupts (aka PPIs) suffer from a number of problems:

- They use a completely separate scheme to handle the interrupts, mostly because the PPI concept doesn't really match the kernel view of an interrupt.
- PPIs can only be used by the timer code, unless we add more low-level assembly code.
- The local timer code can only be used by devices generating PPIs, and not SPIs.
- At least one platform (msm) has started implementing its own alternative scheme.
- Some low-level code gets duplicated, as usual...

As the previous solution which tried to map PPIs to normal interrupts has been proved to be buggy, I've opted to a much simpler scheme (based on Russell's input).

The proposed solution is to handle the interrupt using the same path as SPIs, with a common handler for all PPIs. Each PPI can be requested using gic_request_ppi(), similar to request_irq(). The local timer code is updated to reflect these changes.

Patches against v3.1-rc1 + Will Deacon's TWD patch ("ARM: twd:
register clockevents device before enabling PPI"). Tested on PB11MP, VE, OMAP4 (Panda) and Tegra (Harmony). As this patch series is quite different from the previous one, I've dropped all previous acks from platform maintainers.

Quoted 1 line

From v10:

- Fixed exynos4 compilation

Quoted 1 line

From v9:

- Fixed PPI request in the MSM timer code
- Fixed UP/non-local-timer build
- Moved some bits and pieces from patch 1 to patch 2

Marc Zyngier (4):
ARM: gic: consolidate PPI handling
ARM: gic: Add PPI registration interface
ARM: local timers: drop local_timer_ack()
ARM: gic: add compute_irqnr macro for exynos4

 arch/arm/common/gic.c                             |  153 +++++++++++++++++++--
 arch/arm/include/asm/entry-macro-multi.S          |    7 -
 arch/arm/include/asm/hardirq.h                    |    3 -
 arch/arm/include/asm/hardware/entry-macro-gic.S   |   22 +---
 arch/arm/include/asm/hardware/gic.h               |    5 +-
 arch/arm/include/asm/localtimer.h                 |   19 ++--
 arch/arm/include/asm/smp.h                        |    5 -
 arch/arm/include/asm/smp_twd.h                    |    2 +-
 arch/arm/kernel/irq.c                             |    3 -
 arch/arm/kernel/smp.c                             |   33 +-----
 arch/arm/kernel/smp_twd.c                         |   30 +++--
 arch/arm/mach-exynos4/include/mach/entry-macro.S  |   67 +--------
 arch/arm/mach-exynos4/mct.c                       |    6 -
 arch/arm/mach-msm/board-msm8x60.c                 |   11 --
 arch/arm/mach-msm/include/mach/entry-macro-qgic.S |   73 +----------
 arch/arm/mach-msm/timer.c                         |   56 ++++----
 arch/arm/mach-omap2/include/mach/entry-macro.S    |   14 +--
 arch/arm/mach-shmobile/entry-intc.S               |    3 -
 arch/arm/mach-shmobile/include/mach/entry-macro.S |    3 -
 19 files changed, 214 insertions(+), 301 deletions(-)
Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-------------- next part --------------

 arch/arm/common/gic.c                             |   70 ++++++++++++++++++++-
 arch/arm/include/asm/entry-macro-multi.S          |    5 +-
 arch/arm/include/asm/hardware/entry-macro-gic.S   |   17 +++--
 arch/arm/include/asm/hardware/gic.h               |    5 +-
 arch/arm/include/asm/localtimer.h                 |   14 +----
 arch/arm/include/asm/smp_twd.h                    |    2 +-
 arch/arm/kernel/smp.c                             |   18 +-----
 arch/arm/kernel/smp_twd.c                         |   41 +++++++-----
 arch/arm/mach-exynos4/include/mach/entry-macro.S  |    8 +-
 arch/arm/mach-msm/include/mach/entry-macro-qgic.S |   14 ++--
 arch/arm/mach-msm/timer.c                         |   20 +++++-
 arch/arm/mach-omap2/include/mach/entry-macro.S    |    9 +--
 arch/arm/mach-shmobile/entry-intc.S               |    2 +-
 arch/arm/mach-shmobile/include/mach/entry-macro.S |    2 +-
 14 files changed, 144 insertions(+), 83 deletions(-)
arch/arm/common/gic.c +67 -3
index 4ddd0a6..148367d 100644
@@ -27,12 +27,16 @@
 #include <linux/list.h>
 #include <linux/smp.h>
 #include <linux/cpumask.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
 #include <asm/hardware/gic.h>
 
+#define GIC_FIRST_PPI	16
+#define NR_PPI		16
+
 static DEFINE_SPINLOCK(irq_controller_lock);
 
 /* Address of GIC 0 CPU interface */
@@ -376,14 +380,74 @@ void __cpuinit gic_secondary_init(unsigned int gic_nr)
 	gic_cpu_init(&gic_data[gic_nr]);
 }
 
-void __cpuinit gic_enable_ppi(unsigned int irq)
+struct gic_action {
+	irq_handler_t handler;
+	void *data;
+};
+
+static DEFINE_PER_CPU(struct gic_action[NR_PPI], gic_ppi_action);
+
+asmlinkage void __exception_irq_entry gic_call_ppi(unsigned int irq,
+	struct pt_regs *regs)
+{
+	unsigned int ppi = irq - GIC_FIRST_PPI;
+	struct gic_action *act;
+
+	act = &__get_cpu_var(gic_ppi_action)[ppi];
+	if (act->handler) {
+		struct pt_regs *old_regs = set_irq_regs(regs);
+
+		/* FIXME: need to deal with PPI IRQ stats better.. */
+		__inc_irq_stat(smp_processor_id(), local_timer_irqs);
+
+		irq_enter();
+		act->handler(irq, act->data);
+		irq_exit();
+
+		set_irq_regs(old_regs);
+	}
+}
+
+int gic_request_ppi(unsigned int irq, irq_handler_t handler, void *data)
 {
+	struct gic_action *act;
 	unsigned long flags;
+	unsigned int ppi = irq - GIC_FIRST_PPI;
+	int ret = -EBUSY;
+
+	if (ppi >= NR_PPI)
+		return -EINVAL;
 
 	local_irq_save(flags);
-	irq_set_status_flags(irq, IRQ_NOPROBE);
-	gic_unmask_irq(irq_get_irq_data(irq));
+	act = &__get_cpu_var(gic_ppi_action)[ppi];
+	if (!act->handler) {
+		act->handler = handler;
+		act->data = data;
+
+		irq_set_status_flags(irq, IRQ_NOPROBE);
+		gic_unmask_irq(irq_get_irq_data(irq));
+	}
 	local_irq_restore(flags);
+
+	return ret;
+}
+
+void gic_free_ppi(unsigned int irq, void *data)
+{
+	struct gic_action *act;
+	unsigned long flags;
+	unsigned int ppi = irq - GIC_FIRST_PPI;
+
+	if (ppi < NR_PPI) {
+		local_irq_save(flags);
+		act = &__get_cpu_var(gic_ppi_action)[ppi];
+		if (act->data == data) {
+			gic_mask_irq(irq_get_irq_data(irq));
+			act->handler = NULL;
+			act->data = NULL;
+		}
+		local_irq_restore(flags);
+	}
 }
 
 #ifdef CONFIG_SMP
arch/arm/include/asm/entry-macro-multi.S +1 -4
index 2f1e209..f1ee1e6 100644
@@ -27,10 +27,7 @@
 	bne	do_IPI
 
 #ifdef CONFIG_LOCAL_TIMERS
-	test_for_ltirq r0, r2, r6, lr
-	movne	r0, sp
-	adrne	lr, BSYM(1b)
-	bne	do_local_timer
+	test_for_ppi r0, r2, r6, lr, sp, BSYM(1b)
 #endif
 #endif
 9997:
arch/arm/include/asm/hardware/entry-macro-gic.S +10 -7
index c115b82..a74990f 100644
@@ -63,13 +63,16 @@
 	cmpcs	\irqnr, \irqnr
 	.endm
 
-/* As above, this assumes that irqstat and base are preserved.. */
+/*
+ * We will have checked for normal IRQs (32+) and IPIs (0-15) so only
+ * PPIs are left here.
+ */
 
-	.macro test_for_ltirq, irqnr, irqstat, base, tmp
+	.macro test_for_ppi, irqnr, irqstat, base, tmp, regs, sym
 	bic	\irqnr, \irqstat, #0x1c00
-	mov 	\tmp, #0
-	cmp	\irqnr, #29
-	moveq	\tmp, #1
-	streq	\irqstat, [\base, #GIC_CPU_EOI]
-	cmp	\tmp, #0
+	cmp	\irqnr, #32
+	strcc	\irqstat, [\base, #GIC_CPU_EOI]
+	movcc	r1, \regs
+	adrcc	lr, \sym
+	bcc	gic_call_ppi
 	.endm
arch/arm/include/asm/hardware/gic.h +4 -1
index 0691f9d..768521d4 100644
@@ -33,6 +33,8 @@
 #define GIC_DIST_SOFTINT		0xf00
 
 #ifndef __ASSEMBLY__
+#include <linux/interrupt.h>
+
 extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
@@ -40,7 +42,8 @@ void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
-void gic_enable_ppi(unsigned int);
+int gic_request_ppi(unsigned int, irq_handler_t, void *);
+void gic_free_ppi(unsigned int, void *);
 #endif
 
 #endif
arch/arm/include/asm/localtimer.h +2 -12
index 080d74f..42a842f 100644
@@ -17,27 +17,17 @@ struct clock_event_device;
  */
 void percpu_timer_setup(void);
 
-/*
- * Called from assembly, this is the local timer IRQ handler
- */
-asmlinkage void do_local_timer(struct pt_regs *);
-
-
 #ifdef CONFIG_LOCAL_TIMERS
 
 #ifdef CONFIG_HAVE_ARM_TWD
 
 #include "smp_twd.h"
 
-#define local_timer_ack()	twd_timer_ack()
+#define local_timer_stop twd_timer_stop
 
 #else
 
-/*
- * Platform provides this to acknowledge a local timer IRQ.
- * Returns true if the local timer IRQ is to be processed.
- */
-int local_timer_ack(void);
+int local_timer_stop(struct clock_event_device *);
 
 #endif
 
arch/arm/include/asm/smp_twd.h +1 -1
index fed9981..ef9ffba9 100644
@@ -22,7 +22,7 @@ struct clock_event_device;
 
 extern void __iomem *twd_base;
 
-int twd_timer_ack(void);
 void twd_timer_setup(struct clock_event_device *);
+void twd_timer_stop(struct clock_event_device *);
 
 #endif
arch/arm/kernel/smp.c +1 -17
index 167e3cb..f83ef5e 100644
@@ -458,19 +458,6 @@ static void ipi_timer(void)
 }
 
 #ifdef CONFIG_LOCAL_TIMERS
-asmlinkage void __exception_irq_entry do_local_timer(struct pt_regs *regs)
-{
-	struct pt_regs *old_regs = set_irq_regs(regs);
-	int cpu = smp_processor_id();
-
-	if (local_timer_ack()) {
-		__inc_irq_stat(cpu, local_timer_irqs);
-		ipi_timer();
-	}
-
-	set_irq_regs(old_regs);
-}
-
 void show_local_irqs(struct seq_file *p, int prec)
 {
 	unsigned int cpu;
@@ -531,10 +518,7 @@ void __cpuinit percpu_timer_setup(void)
  */
 static void percpu_timer_stop(void)
 {
-	unsigned int cpu = smp_processor_id();
-	struct clock_event_device *evt = &per_cpu(percpu_clockevent, cpu);
-
-	evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
+	local_timer_stop(&per_cpu(percpu_clockevent, smp_processor_id()));
 }
 #endif
 
arch/arm/kernel/smp_twd.c +24 -17

65 lines not on this page · raw message

arch/arm/mach-exynos4/include/mach/entry-macro.S +4 -4
index d8f38c2..fdb24bb 100644
@@ -74,11 +74,11 @@
 
 		/* As above, this assumes that irqstat and base are preserved.. */
 
-		.macro test_for_ltirq, irqnr, irqstat, base, tmp
+		.macro test_for_ppi, irqnr, irqstat, base, tmp, regs, sym
 		bic	\irqnr, \irqstat, #0x1c00
-		mov	\tmp, #0
 		cmp	\irqnr, #29
-		moveq	\tmp, #1
 		streq	\irqstat, [\base, #GIC_CPU_EOI]
-		cmp	\tmp, #0
+		moveq	r1, \regs
+		adreq	lr, \sym
+		bcc	gic_call_ppi
 		.endm
arch/arm/mach-msm/include/mach/entry-macro-qgic.S +7 -7
index 1246715..b3ebb06 100644
@@ -78,11 +78,11 @@
 
 	/* As above, this assumes that irqstat and base are preserved.. */
 
-	.macro test_for_ltirq, irqnr, irqstat, base, tmp
-    bic \irqnr, \irqstat, #0x1c00
-    mov     \tmp, #0
-    cmp \irqnr, #16
-    moveq   \tmp, #1
-    streq   \irqstat, [\base, #GIC_CPU_EOI]
-    cmp \tmp, #0
+	.macro test_for_ppi, irqnr, irqstat, base, tmp, regs, sym
+	bic	\irqnr, \irqstat, #0x1c00
+	cmp	\irqnr, #16
+	streq	\irqstat, [\base, #GIC_CPU_EOI]
+	moveq	r1, \regs
+	adreq	lr, \sym
+	beq	gic_call_ppi
 	.endm
arch/arm/mach-msm/timer.c +17 -3
index 63621f1..b553a14 100644
@@ -271,9 +271,19 @@ static void __init msm_timer_init(void)
 }
 
 #ifdef CONFIG_SMP
+static irqreturn_t local_timer_irq(int irq, void *data)
+{
+	struct clock_event_device *evt = data;
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
 int __cpuinit local_timer_setup(struct clock_event_device *evt)
 {
 	struct msm_clock *clock = &msm_clocks[MSM_GLOBAL_TIMER];
+	int ret;
 
 	/* Use existing clock_event for cpu 0 */
 	if (!smp_processor_id())
@@ -300,15 +310,19 @@ int __cpuinit local_timer_setup(struct clock_event_device *evt)
 
 	local_clock_event = evt;
 
-	gic_enable_ppi(clock->irq.irq);
+	ret = gic_request_ppi(evt->irq, local_timer_irq, evt);
+	if (ret)
+		pr_err("CPU%u: unable to request local timer interrupt\n",
+			smp_processor_id());
 
 	clockevents_register_device(evt);
 	return 0;
 }
 
-inline int local_timer_ack(void)
+void local_timer_stop(struct clock_event_device *evt)
 {
-	return 1;
+	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+	gic_free_ppi(clk->irq, clk);
 }
 
 #endif
arch/arm/mach-omap2/include/mach/entry-macro.S +4 -5
index ceb8b7e..66329f4 100644
@@ -104,14 +104,13 @@
 
 		/* As above, this assumes that irqstat and base are preserved */
 
-		.macro test_for_ltirq, irqnr, irqstat, base, tmp
+		.macro test_for_ppi, irqnr, irqstat, base, tmp, regs, sym
 		bic	\irqnr, \irqstat, #0x1c00
-		mov 	\tmp, #0
 		cmp	\irqnr, #29
-		itt	eq
-		moveq	\tmp, #1
 		streq	\irqstat, [\base, #GIC_CPU_EOI]
-		cmp	\tmp, #0
+		moveq	r1, \regs
+		adreq	lr, \sym
+		beq	gic_call_ppi
 		.endm
 #endif	/* CONFIG_SMP */
 
arch/arm/mach-shmobile/entry-intc.S +1 -1
index cac0a7a..b4ece8e 100644
@@ -51,7 +51,7 @@
 	.macro  test_for_ipi, irqnr, irqstat, base, tmp
 	.endm
 
-	.macro  test_for_ltirq, irqnr, irqstat, base, tmp
+	.macro  test_for_ppi, irqnr, irqstat, base, tmp, regs, sym
 	.endm
 
 	arch_irq_handler shmobile_handle_irq_intc
arch/arm/mach-shmobile/include/mach/entry-macro.S +1 -1
index d791f10..e6dcafd 100644
@@ -27,7 +27,7 @@
 	.macro  test_for_ipi, irqnr, irqstat, base, tmp
 	.endm
 
-	.macro  test_for_ltirq, irqnr, irqstat, base, tmp
+	.macro  test_for_ppi, irqnr, irqstat, base, tmp, regs, sym
 	.endm
 
 	.macro  arch_ret_to_user, tmp1, tmp2
#6 lore raw

v11 cover Consolidating GIC per-cpu interrupts

B1;2601;0cRussell,

Quoted 5 lines · Russell King - ARM Linux

Thomas,

Could you _please_ look Marcs patch series and give an opinion on it. I've also attached my patch to this reply too, which is an alternative approach to Marcs.

I don't have fundamental objections to Marcs or your approach. In fact they are very similar.

The main difference is that Marc sets up regular interrupts with handle_percpu_irq instead of going through a separate entry point. The only downside is that it exposes the PPI interrupts to the generic irq API, so nothing can prevent stupid drivers to call disable/enable_irq & al. I'm not sure how much of an issue that is in reality. If it matters we can add a flag to the core code which excludes such interrupts from being accessed.

Another thing, which sticks out compared to other percpu interrupt users in arch/* is that you provide the ability to assign different handlers on different CPUs to a given PPI interrupt number. Most other percpu implementations setup the interrupt with a unique percpu aware handler and just enable/disable it per core in the low level setup/shutdown code. Is running different handlers on different cores a real requirement or just a nice feature with no usecase?

Thanks,

tglx

#7 lore raw

v11 cover Consolidating GIC per-cpu interrupts

Thomas,

Quoted 7 lines · Thomas Gleixner

Another thing, which sticks out compared to other percpu interrupt users in arch/* is that you provide the ability to assign different handlers on different CPUs to a given PPI interrupt number. Most other percpu implementations setup the interrupt with a unique percpu aware handler and just enable/disable it per core in the low level setup/shutdown code. Is running different handlers on different cores a real requirement or just a nice feature with no usecase?

At the moment, it sort of falls into the second category. MSM has "asymmetric" timers (each core has its private timer on a different PPI), but that doesn't mandate having separate handlers per core, unless someone decides to connect something on another CPU, using the same PPI... The architecture would probably allow it.

But a clear requirement we have is that the handler has to be called with a per-cpu dev_id pointer (we use this to obtain the clock_event_device in the timer handler, for example). Which makes having something similar to request_irq() quite the natural thing.

Cheers,

	M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#8 lore raw

v11 cover Consolidating GIC per-cpu interrupts

Marc,

Quoted 12 lines · Marc Zyngier

Thomas,

Quoted 7 lines · Thomas Gleixner

Another thing, which sticks out compared to other percpu interrupt users in arch/* is that you provide the ability to assign different handlers on different CPUs to a given PPI interrupt number. Most other percpu implementations setup the interrupt with a unique percpu aware handler and just enable/disable it per core in the low level setup/shutdown code. Is running different handlers on different cores a real requirement or just a nice feature with no usecase?

Quoted 5 lines

At the moment, it sort of falls into the second category. MSM has "asymmetric" timers (each core has its private timer on a different PPI), but that doesn't mandate having separate handlers per core, unless someone decides to connect something on another CPU, using the same PPI... The architecture would probably allow it.

The question is whether we really want to allow it from the OS side. That makes irq accounting an utter mess as you end up with devA,B,C,D on the same interrupt line and each counts on the corresponding CPU0,1,2,3

Quoted 4 lines

But a clear requirement we have is that the handler has to be called with a per-cpu dev_id pointer (we use this to obtain the clock_event_device in the timer handler, for example). Which makes having something similar to request_irq() quite the natural thing.

That makes a lot of sense, but it requires your extra percpu handler registration/free interface ....

Looking at the other PERCPU irq users there might be a general interest for this.

If we can apply the following set of (sane) restricitions to this:

- interrupt is never shared
- interrupt is never threaded
- handler is common for all CPUs

then we could do something like the patch below. Warning, this is incomplete and requires a bunch of other changes like adding per cpu aware enable/disable functions and excluding the other interfaces from fiddling with such an interrupt.

So a request/setup_irq() of such an interrupt would require the following steps.

  irq_set_percpu_devid(irq);

This would set: IRQ_NOAUTOEN | IRQ_PER_CPU | IRQ_NOPROBE | IRQ_PER_CPU_DEVID and

  irq_set_handler(irq, handle_irq_per_cpu_devid);

  setup/request_percpu_irq(irq, .....);

The dev_id pointer for those interrupts would be a percpu pointer which holds the real dev_ids, e.g. the percpu clockevents

Due to the restricted nature of those interrupts we probably can ignore nested disable/enable_percpu_irq() calls and just keep the *_percpu_irq API to a bare minimum.

Thoughts ?

Thanks,

tglx

include/linux/interrupt.h +15 -11
Index: linux-2.6/include/linux/interrupt.h
===================================================================
@@ -95,6 +97,7 @@ typedef irqreturn_t (*irq_handler_t)(int
  * @flags:	flags (see IRQF_* above)
  * @name:	name of the device
  * @dev_id:	cookie to identify the device
+ * @percpu_dev_id:	cookie to identify the device
  * @next:	pointer to the next irqaction for shared interrupts
  * @irq:	interrupt number
  * @dir:	pointer to the proc/irq/NN/name entry
@@ -104,17 +107,20 @@ typedef irqreturn_t (*irq_handler_t)(int
  * @thread_mask:	bitmask for keeping track of @thread activity
  */
 struct irqaction {
-	irq_handler_t handler;
-	unsigned long flags;
-	void *dev_id;
-	struct irqaction *next;
-	int irq;
-	irq_handler_t thread_fn;
-	struct task_struct *thread;
-	unsigned long thread_flags;
-	unsigned long thread_mask;
-	const char *name;
-	struct proc_dir_entry *dir;
+	irq_handler_t		handler;
+	unsigned long		flags;
+	union {
+		void		*dev_id;
+		void __percpu	*percpu_dev_id;
+	};
+	struct irqaction	*next;
+	int			irq;
+	irq_handler_t		thread_fn;
+	struct task_struct	*thread;
+	unsigned long		thread_flags;
+	unsigned long		thread_mask;
+	const char		*name;
+	struct proc_dir_entry	*dir;
 } ____cacheline_internodealigned_in_smp;
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
kernel/irq/chip.c +31 -0
Index: linux-2.6/kernel/irq/chip.c
===================================================================
@@ -544,6 +544,37 @@ handle_percpu_irq(unsigned int irq, stru
 		chip->irq_eoi(&desc->irq_data);
 }
 
+/**
+ * handle_percpu_devid_irq - Per CPU local irq handler with per cpu dev ids
+ * @irq:	the interrupt number
+ * @desc:	the interrupt description structure for this irq
+ *
+ * Per CPU interrupts on SMP machines without locking requirements. Same as
+ * handle_percpu_irq() above but with the following extras:
+ *
+ * action->dev_id is a pointer to percpu variables which contain the
+ * real device id for the cpu on which this handler is called
+ */
+void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irqaction *action = desc->action;
+	void *dev_id = __this_cpu_ptr(action->percpu_dev_id);
+	irqreturn_t res;
+
+	kstat_incr_irqs_this_cpu(irq, desc);
+
+	if (chip->irq_ack)
+		chip->irq_ack(&desc->irq_data);
+
+	trace_irq_handler_entry(irq, action);
+	res = action->handler(irq, dev_id);
+	trace_irq_handler_exit(irq, action, res);
+
+	if (chip->irq_eoi)
+		chip->irq_eoi(&desc->irq_data);
+}
+
 void
 __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
 		  const char *name)
#9 lore raw

v11 cover Consolidating GIC per-cpu interrupts

Thomas,

Quoted 60 lines · Thomas Gleixner

Marc,

Quoted 12 lines · Marc Zyngier

Thomas,

Quoted 7 lines · Thomas Gleixner

Another thing, which sticks out compared to other percpu interrupt users in arch/* is that you provide the ability to assign different handlers on different CPUs to a given PPI interrupt number. Most other percpu implementations setup the interrupt with a unique percpu aware handler and just enable/disable it per core in the low level setup/shutdown code. Is running different handlers on different cores a real requirement or just a nice feature with no usecase?

Quoted 5 lines

At the moment, it sort of falls into the second category. MSM has "asymmetric" timers (each core has its private timer on a different PPI), but that doesn't mandate having separate handlers per core, unless someone decides to connect something on another CPU, using the same PPI... The architecture would probably allow it.

The question is whether we really want to allow it from the OS side. That makes irq accounting an utter mess as you end up with devA,B,C,D on the same interrupt line and each counts on the corresponding CPU0,1,2,3

Quoted 4 lines

But a clear requirement we have is that the handler has to be called with a per-cpu dev_id pointer (we use this to obtain the clock_event_device in the timer handler, for example). Which makes having something similar to request_irq() quite the natural thing.

That makes a lot of sense, but it requires your extra percpu handler registration/free interface ....

Looking at the other PERCPU irq users there might be a general interest for this.

If we can apply the following set of (sane) restricitions to this:

- interrupt is never shared
- interrupt is never threaded
- handler is common for all CPUs

then we could do something like the patch below. Warning, this is incomplete and requires a bunch of other changes like adding per cpu aware enable/disable functions and excluding the other interfaces from fiddling with such an interrupt.

So a request/setup_irq() of such an interrupt would require the following steps.

  irq_set_percpu_devid(irq);

This would set: IRQ_NOAUTOEN | IRQ_PER_CPU | IRQ_NOPROBE | IRQ_PER_CPU_DEVID and

  irq_set_handler(irq, handle_irq_per_cpu_devid);

  setup/request_percpu_irq(irq, .....);
Quoted 8 lines

The dev_id pointer for those interrupts would be a percpu pointer which holds the real dev_ids, e.g. the percpu clockevents

Due to the restricted nature of those interrupts we probably can ignore nested disable/enable_percpu_irq() calls and just keep the *_percpu_irq API to a bare minimum.

Thoughts ?

I quite like it. Specially if it can be useful to other architectures. Let me glue all that together to get a feel of how it would work, and I'll get back to you.

Thanks,

	M.
Signature · 1 line
Jazz is not dead. It just smells funny...