Consolidating GIC per-cpu interrupts

arch/arm/common/gic.c +0 -0

index e9c2ff8..60f8915 100644

arch/arm/common/gic.c +0 -0

 static void __init gic_dist_init(struct gic_chip_data *gic,
 	unsigned int irq_start)

{

arch/arm/common/gic.c +37 -5

-	unsigned int gic_irqs, irq_limit, i;
+	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
+	u32 dist_ctr, nrcpus;
 
 	cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;

@@ -278,11 +359,32 @@ 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(base + GIC_DIST_CTR) & 0x1f;
-	gic_irqs = (gic_irqs + 1) * 32;
+	dist_ctr = readl(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;
+
+#ifdef CONFIG_ARM_GIC_VPPI
+	/*
+	 * Nobody would be insane enough to use PPIs on a secondary
+	 * GIC, right?
+	 */
+	if (gic == &gic_data[0]) {
+		gic->nrppis = 16 - (irq_start % 16);
+		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
+		nrvppis = gic->nrppis * nrcpus;
+	} else {
+		gic->ppi_base = 0;
+		gic->vppi_base = 0;
+	}
+#endif
+
+	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
+		gic_irqs, nrvppis);
+
 	/*
 	 * Set all global interrupts to be level triggered, active low.
 	 */

@@ -312,17 +414,39 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	 * Limit number of interrupts registered to the platform maximum
 	 */
 	irq_limit = gic->irq_offset + gic_irqs;
-	if (WARN_ON(irq_limit > NR_IRQS))
+	if (WARN_ON((irq_limit) > NR_IRQS))
 		irq_limit = NR_IRQS;
 
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
 	for (i = irq_start; i < irq_limit; i++) {
-		irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
+#ifdef CONFIG_ARM_GIC_VPPI
+		if (nrvppis && gic_irq_is_ppi(gic, i))
+			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		else
+#endif
+		{
+			irq_set_chip_and_handler(i, &gic_chip,
+						 handle_fasteoi_irq);
+			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+		}
+		irq_set_chip_data(i, gic);

Quoted 9 lines · Marc Zyngier

arch/arm/common/gic.c +1 -0

index e9c2ff8..60f8915 100644
@@ -42,6 +42,11 @@ struct gic_chip_data {
 	unsigned int irq_offset;
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
+#ifdef CONFIG_ARM_GIC_VPPI

arch/arm/common/gic.c struct gic_chip_data { · line 50

Quoted 5 lines

arch/arm/common/gic.c +4 -0

+	int	     ppi_base;
+	int	     vppi_base;
+	u16	     nrppis;
+#endif
 };

Can you put some document around this struct? Mention that ppi_base, vppi_base and nrppis will be zero for non-primary gic.

Quoted 74 lines

arch/arm/common/gic.c +0 -0

 static void __init gic_dist_init(struct gic_chip_data *gic,
 	unsigned int irq_start)

{

arch/arm/common/gic.c +41 -5

-	unsigned int gic_irqs, irq_limit, i;
+	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
+	u32 dist_ctr, nrcpus;
 
 	cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;

@@ -278,11 +359,32 @@ 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(base + GIC_DIST_CTR) & 0x1f;
-	gic_irqs = (gic_irqs + 1) * 32;
+	dist_ctr = readl(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;
+
+#ifdef CONFIG_ARM_GIC_VPPI
+	/*
+	 * Nobody would be insane enough to use PPIs on a secondary
+	 * GIC, right?
+	 */
+	if (gic == &gic_data[0]) {
+		gic->nrppis = 16 - (irq_start % 16);
+		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
+		nrvppis = gic->nrppis * nrcpus;
+	} else {
+		gic->ppi_base = 0;
+		gic->vppi_base = 0;
+	}
+#endif
+
+	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
+		gic_irqs, nrvppis);
+
 	/*
 	 * Set all global interrupts to be level triggered, active low.
 	 */

@@ -312,17 +414,39 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	 * Limit number of interrupts registered to the platform maximum
 	 */
 	irq_limit = gic->irq_offset + gic_irqs;
-	if (WARN_ON(irq_limit > NR_IRQS))
+	if (WARN_ON((irq_limit) > NR_IRQS))
 		irq_limit = NR_IRQS;
 
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
 	for (i = irq_start; i < irq_limit; i++) {
-		irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
+#ifdef CONFIG_ARM_GIC_VPPI
+		if (nrvppis && gic_irq_is_ppi(gic, i))
+			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		else
+#endif
+		{
+			irq_set_chip_and_handler(i, &gic_chip,
+						 handle_fasteoi_irq);
+			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+		}
+		irq_set_chip_data(i, gic);
+	}
+
+#ifdef CONFIG_ARM_GIC_VPPI
+	if (!nrvppis)

arch/arm/common/gic.c static void __init gic_dist_init(struct gic_chip_data *gic, · line 444

Quoted 6 lines

arch/arm/common/gic.c +6 -0

+		goto out;
+	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
+	if (unlikely(WARN_ON(gic->vppi_base < 0)))
+		goto out;
+	for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
+		irq_set_chip_and_handler(i, &gic_ppi_chip, handle_fasteoi_irq);

Can we do with handle_percpu_irq instead of handle_fasteoi_irq?

arch/arm/common/gic.c +8 -1

 		irq_limit = NR_IRQS;
 
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
 	for (i = irq_start; i < irq_limit; i++) {
-		irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
+#ifdef CONFIG_ARM_GIC_VPPI
+		if (nrvppis && gic_irq_is_ppi(gic, i))
+			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		else
+#endif
+		{
+			irq_set_chip_and_handler(i, &gic_chip,
+						 handle_fasteoi_irq);

Just minor comments. I'm not a GIC or IRQ expert (yet).

arch/arm/common/gic.c static void __init gic_dist_init(struct gic_chip_data *gic, · line 417

Quoted 6 lines · Marc Zyngier

arch/arm/common/gic.c +1 -1

@@ -312,17 +414,39 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	 * Limit number of interrupts registered to the platform maximum
 	 */
 	irq_limit = gic->irq_offset + gic_irqs;
-	if (WARN_ON(irq_limit > NR_IRQS))
+	if (WARN_ON((irq_limit) > NR_IRQS))

() are RFC leftovers?

Quoted 19 lines

arch/arm/common/gic.c +12 -1

 		irq_limit = NR_IRQS;
 
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
 	for (i = irq_start; i < irq_limit; i++) {
-		irq_set_chip_and_handler(i, &gic_chip, handle_fasteoi_irq);
+#ifdef CONFIG_ARM_GIC_VPPI
+		if (nrvppis && gic_irq_is_ppi(gic, i))
+			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		else
+#endif
+		{
+			irq_set_chip_and_handler(i, &gic_chip,
+						 handle_fasteoi_irq);
+			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+		}
+		irq_set_chip_data(i, gic);
+	}

arch/arm/common/gic.c static void __init gic_dist_init(struct gic_chip_data *gic, · line 441

Quoted 6 lines

arch/arm/common/gic.c +6 -0

+
+#ifdef CONFIG_ARM_GIC_VPPI
+	if (!nrvppis)
+		goto out;
+	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
+	if (unlikely(WARN_ON(gic->vppi_base < 0)))

WARN_ON should already have an unlikely in it.

arch/arm/include/asm/entry-macro-multi.S +1 -1

index ec0bbf7..c599795 100644
@@ -24,7 +24,7 @@
 	adrne	lr, BSYM(1b)
 	bne	do_IPI
 
-#ifdef CONFIG_LOCAL_TIMERS
+#if defined(CONFIG_LOCAL_TIMERS) && !defined(CONFIG_ARM_GIC_VPPI)
 	test_for_ltirq r0, r6, r5, lr
 	movne	r0, sp
 	adrne	lr, BSYM(1b)

arch/arm/include/asm/hardware/entry-macro-gic.S +11 -7

index c115b82..14a3363 100644
@@ -17,23 +17,26 @@
 	.endm
 #endif
 
+#ifdef CONFIG_ARM_GIC_VPPI
+#define DO_IRQ_BASE	16
+#else
+#define DO_IRQ_BASE	30
+#endif
+
 /*
  * The interrupt numbering scheme is defined in the
  * 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).
+ * valid range for an IRQ (30-1020 inclusive). If CONFIG_ARM_GIC_VPPI is
+ * enabled, local interrupts are handled the same way as global ones.
  */
 
 	.macro  get_irqnr_and_base, irqnr, irqstat, base, tmp

@@ -43,7 +46,7 @@
 
 	ldr	\tmp, =1021
 	bic     \irqnr, \irqstat, #0x1c00
-	cmp     \irqnr, #29
+	cmp     \irqnr, #(DO_IRQ_BASE - 1)
 	cmpcc	\irqnr, \irqnr
 	cmpne	\irqnr, \tmp
 	cmpcs	\irqnr, \irqnr

@@ -63,6 +66,7 @@
 	cmpcs	\irqnr, \irqnr
 	.endm
 
+#ifndef CONFIG_ARM_GIC_VPPI
 /* As above, this assumes that irqstat and base are preserved.. */
 
 	.macro test_for_ltirq, irqnr, irqstat, base, tmp

Quoted 66 lines

arch/arm/include/asm/entry-macro-multi.S +1 -1

index ec0bbf7..c599795 100644
@@ -24,7 +24,7 @@
 	adrne	lr, BSYM(1b)
 	bne	do_IPI
 
-#ifdef CONFIG_LOCAL_TIMERS
+#if defined(CONFIG_LOCAL_TIMERS) && !defined(CONFIG_ARM_GIC_VPPI)
 	test_for_ltirq r0, r6, r5, lr
 	movne	r0, sp
 	adrne	lr, BSYM(1b)

arch/arm/include/asm/hardware/entry-macro-gic.S +11 -7

index c115b82..14a3363 100644
@@ -17,23 +17,26 @@
 	.endm
 #endif
 
+#ifdef CONFIG_ARM_GIC_VPPI
+#define DO_IRQ_BASE	16
+#else
+#define DO_IRQ_BASE	30
+#endif
+
 /*
  * The interrupt numbering scheme is defined in the
  * 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).
+ * valid range for an IRQ (30-1020 inclusive). If CONFIG_ARM_GIC_VPPI is
+ * enabled, local interrupts are handled the same way as global ones.
  */
 
 	.macro  get_irqnr_and_base, irqnr, irqstat, base, tmp

@@ -43,7 +46,7 @@
 
 	ldr	\tmp, =1021
 	bic     \irqnr, \irqstat, #0x1c00
-	cmp     \irqnr, #29
+	cmp     \irqnr, #(DO_IRQ_BASE - 1)
 	cmpcc	\irqnr, \irqnr
 	cmpne	\irqnr, \tmp
 	cmpcs	\irqnr, \irqnr

@@ -63,6 +66,7 @@
 	cmpcs	\irqnr, \irqnr
 	.endm
 
+#ifndef CONFIG_ARM_GIC_VPPI
 /* As above, this assumes that irqstat and base are preserved.. */
 

arch/arm/include/asm/hardware/entry-macro-gic.S line 80

Quoted 6 lines

arch/arm/include/asm/hardware/entry-macro-gic.S +1 -0

 	.macro test_for_ltirq, irqnr, irqstat, base, tmp

@@ -73,3 +77,4 @@
 	streq	\irqstat, [\base, #GIC_CPU_EOI]
 	cmp	\tmp, #0
 	.endm
+#endif

I would expect these bits to be part of the first patch in this series because they're necessary to actually use the CONFIG_ARM_GIC_VPPI option. Can you move them to the first patch?

I'd tend to agree with you, though local timer being the only user of PPIs, it doesn't matter that much... ;-)

I'll move these changes to the first patch on the next iteration of the patch set.

Quoted 3 lines · Marc Zyngier

I'll move these changes to the first patch on the next iteration of the patch set.

Thanks. It mattered to me when I only pulled down the first patch and the MSM patch and tested it out and was confused why the timer tick wasn't happening. MSM doesn't have the ARM twd.

Use the normal interrupt scheme for the local timers by using a remapped PPI interrupt.

Tested on a Pandaboard.

Cc Tony Lindgren<[email protected]>

Cc Santosh Shilimkar<[email protected]>

Signed-off-by Marc Zyngier<[email protected]>

---

Quoted 9 lines

Use the normal interrupt scheme for the local timers by using a remapped PPI interrupt.

Tested on a Pandaboard.

Cc Tony Lindgren<[email protected]>

Cc Santosh Shilimkar<[email protected]>

Signed-off-by Marc Zyngier<[email protected]>

---

Have reviewed and tested your series along with OMAP changes. It boots fine on OMAP4430 SDP.

# cat /proc/interrupts
            CPU0       CPU1
.
410:       3678          0   GIC-PPI  local_timer
413:          0       1042   GIC-PPI  local_timer
.
LOC:          0          0  Local timer interrupts
.

Though above output would be bit miss leading, this series removes the duplicate code from platforms and consolidate it at one place.

FWIW, you can add my

Reviewedd-by Santosh Shilimkar <[email protected]>

Tested-by Santosh Shilimkar <[email protected]>

-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier at arm.com]
Sent: Thursday, April 21, 2011 12:38 AM
To: LAK
Cc: Srinidhi KASAGAR; Linus WALLEIJ
Subject: [RFC PATCH 06/12] ARM: ux500: use remapped PPI interrupts for local timer

Use the normal interrupt scheme for the local timers by using a remapped PPI interrupt.

Cc Srinidhi Kasagar <[email protected]>

Cc Linus Walleij <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/mach-ux500/Kconfig      |    1 +
 arch/arm/mach-ux500/localtimer.c |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

arch/arm/mach-ux500/Kconfig +0 -0

index 5862601..6f3eb99 100644

Quoted 19 lines

-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier at arm.com]
Sent: Thursday, April 21, 2011 12:38 AM
To: LAK
Cc: Srinidhi KASAGAR; Linus WALLEIJ
Subject: [RFC PATCH 06/12] ARM: ux500: use remapped PPI interrupts for local timer

Use the normal interrupt scheme for the local timers by using a remapped PPI interrupt.

Cc Srinidhi Kasagar <[email protected]>

Cc Linus Walleij <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/mach-ux500/Kconfig      |    1 +
 arch/arm/mach-ux500/localtimer.c |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

arch/arm/mach-ux500/Kconfig config UX500_SOC_COMMON · line 7

Quoted 9 lines

arch/arm/mach-ux500/Kconfig +1 -0

index 5862601..6f3eb99 100644
@@ -4,6 +4,7 @@ config UX500_SOC_COMMON
 	bool
 	default y
 	select ARM_GIC
+	select ARM_GIC_VPPI

Here you added VPPI and in the other patch 12/12 you remove this again and make it one time selectable based on CONFIG_SMP. is that done just for readability purpose?

Otherwise, for mach-ux500:

Acked-by Srinidhi Kasagar <[email protected]>

arch/arm/mach-msm/timer.c +1 -0

index 38b95e9..68eab51 100644
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 
 #include <asm/mach/time.h>
+#include <asm/hardware/gic.h>
 #include <mach/msm_iomap.h>
 #include <mach/cpu.h>
 

Quoted 13 lines

arch/arm/mach-msm/timer.c +1 -0

index 38b95e9..68eab51 100644
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 
 #include <asm/mach/time.h>
+#include <asm/hardware/gic.h>
 #include <mach/msm_iomap.h>
 #include <mach/cpu.h>
 

arch/arm/mach-msm/timer.c enum timer_location { · line 72

Quoted 6 lines

arch/arm/mach-msm/timer.c +1 -1

@@ -67,8 +68,8 @@ enum timer_location {
 struct msm_clock {
 	struct clock_event_device   clockevent;
 	struct clocksource          clocksource;
-	struct irqaction            irq;
 	void __iomem                *regbase;
+	unsigned int		    irq;

I don't know why this moved down below regbase, but ok.

arch/arm/mach-msm/timer.c int __cpuinit local_timer_setup(struct clock_event_device *evt) · line 286

Quoted 3 lines

arch/arm/mach-msm/timer.c +0 -0

@@ -292,7 +285,14 @@ int __cpuinit local_timer_setup(struct clock_event_device *evt)
 
 	local_clock_event = evt;

I think we can get rid of this variable now. We should be able to pass evt to request_irq() and use that in the interrupt handler. See far below.

This patch doesn't compile for me though. I applied the below fixup:

8<---------------

arch/arm/mach-msm/timer.c +6 -8

index 68eab51..38e51cc 100644
@@ -246,10 +246,10 @@ static void __init msm_timer_init(void)
                irq = gic_ppi_to_vppi(clock->irq);
                res = request_irq(irq, msm_timer_interrupt,
                                  IRQF_TIMER | IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
-                                 clock->name, &clock->clockevent);
+                                 ce->name, ce);
                if (res)
                        pr_err("msm_timer_init: request_irq failed for %s\n",
-                              clock->name);
+                              ce->name);
 
                clockevents_register_device(ce);
        }

@@ -258,6 +258,7 @@ static void __init msm_timer_init(void)
 #ifdef CONFIG_SMP
 int __cpuinit local_timer_setup(struct clock_event_device *evt)
 {
+       int res;
        struct msm_clock *clock = &msm_clocks[MSM_GLOBAL_TIMER];
 
        /* Use existing clock_event for cpu 0 */

@@ -271,7 +272,7 @@ int __cpuinit local_timer_setup(struct clock_event_device *evt)
                writel(0, clock->regbase + TIMER_CLEAR);
                writel(~0, clock->regbase + TIMER_MATCH_VAL);
        }
-       evt->irq = clock->irq.irq;
+       evt->irq = gic_ppi_to_vppi(clock->irq);
        evt->name = "local_timer";
        evt->features = CLOCK_EVT_FEAT_ONESHOT;
        evt->rating = clock->clockevent.rating;

@@ -285,13 +286,13 @@ int __cpuinit local_timer_setup(struct clock_event_device *evt)
 
        local_clock_event = evt;
 
-       res = request_irq(gic_ppi_to_vppi(irq), msm_timer_interrupt,
+       res = request_irq(evt->irq, msm_timer_interrupt,
                          IRQF_TIMER | IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
-                         clock->name, &clock->clockevent);
+                         clock->clockevent.name, &clock->clockevent);
        if (res) {
                pr_err("local_timer_setup: request_irq failed for %s\n",
-                      clock->name);
-               return err;
 
 
----

And then I figured that we shouldn't be overriding the irq handler for the PPIs anymore in msm code so I applied this patch (not sure about handle_percpu_irq though, see Abhijeet's response):

8<--------------

arch/arm/mach-msm/board-msm8x60.c +0 -12

index 35ed594..f66e1f0 100644
@@ -42,8 +42,6 @@ static struct platform_device *devices[] __initdata = {
 
 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);
 

@@ -55,15 +53,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);
-       }
 }
 
 
---

And then well I thought maybe we could apply this patch on top to actually pass the clockevent to msm_timer_interrupt() instead of statically limiting ourselves to one local clockevent (and only two cores):

8<------------

arch/arm/mach-msm/timer.c +4 -12

index 38e51cc..009faea 100644
@@ -84,13 +84,10 @@ enum {
 
 
 static struct msm_clock msm_clocks[];
-static struct clock_event_device *local_clock_event;
 
 static irqreturn_t msm_timer_interrupt(int irq, void *dev_id)
 {
        struct clock_event_device *evt = dev_id;
-       if (smp_processor_id() != 0)
-               evt = local_clock_event;
        if (evt->event_handler == NULL)
                return IRQ_HANDLED;
        evt->event_handler(evt);

@@ -267,11 +264,9 @@ int __cpuinit local_timer_setup(struct clock_event_device *evt)
 
        writel(DGT_CLK_CTL_DIV_4, MSM_TMR_BASE + DGT_CLK_CTL);
 
-       if (!local_clock_event) {
-               writel(0, clock->regbase  + TIMER_ENABLE);
-               writel(0, clock->regbase + TIMER_CLEAR);
-               writel(~0, clock->regbase + TIMER_MATCH_VAL);
-       }
+       writel(0, clock->regbase  + TIMER_ENABLE);
+       writel(0, clock->regbase + TIMER_CLEAR);
+       writel(~0, clock->regbase + TIMER_MATCH_VAL);
        evt->irq = gic_ppi_to_vppi(clock->irq);
        evt->name = "local_timer";
        evt->features = CLOCK_EVT_FEAT_ONESHOT;

@@ -284,11 +279,9 @@ int __cpuinit local_timer_setup(struct clock_event_device *evt)
                clockevent_delta2ns(0xf0000000 >> clock->shift, evt);
        evt->min_delta_ns = clockevent_delta2ns(4, evt);
 
-       local_clock_event = evt;
-
        res = request_irq(evt->irq, msm_timer_interrupt,
                          IRQF_TIMER | IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
-                         clock->clockevent.name, &clock->clockevent);
+                         clock->clockevent.name, evt);
        if (res) {
                pr_err("local_timer_setup: request_irq failed for %s\n",
                       clock->clockevent.name);
 
 
---

So after all that it looks like it sort of works (at least it boots and sits at a console). I actually wanted to do something like this a few days ago and by coincidence you've done it. Telepathy? Perhaps.

I found one issue. I don't quite understand it, but we call local_timer_setup() on each cpu hotplug add event, and thus the request_irq() call is going to fail the second time you hotplug add a cpu. Simplest fix I see is to have a flag so we don't do boot stuff more than once (similar to our check for !local_clock_event which I mistakenly dropped up there for the wrong reason!).

Why do we need to call local_timer_setup() on each CPU up though? It would be nice to be able to just add local timers for all possible CPUs at boot time and then have the clockevents core take care of enabling and disabling the events as appropriate without ARM code forcing the events to be UNUSED and then re-adding the events back on CPU up. I'm probably missing something really obvious here.

Thanks for having a look at all of this. I think the lack of an MSM platform shows here... Can I fold your patches into mine?

Why do we need to call local_timer_setup() on each CPU up though? It would be nice to be able to just add local timers for all possible CPUs at boot time and then have the clockevents core take care of enabling and disabling the events as appropriate without ARM code forcing the events to be UNUSED and then re-adding the events back on CPU up. I'm probably missing something really obvious here.

Quoted 6 lines · Stephen Boyd

Why do we need to call local_timer_setup() on each CPU up though? It would be nice to be able to just add local timers for all possible CPUs at boot time and then have the clockevents core take care of enabling and disabling the events as appropriate without ARM code forcing the events to be UNUSED and then re-adding the events back on CPU up. I'm probably missing something really obvious here.

Firstly, because you can't access the local timer registers from non- local CPUs. Secondly, when you offline a CPU, generic code unregisters the local timer, so whenever you online a CPU you have to add its local timer clockevent back again.

Thirdly, you may have a system where the local timer gets powered down when a CPU is offlined, and so it would need reinitialization when you online it again.

I took a peek at the APB timer on x86 and I see that apbt_setup_irq() does an

        if (system_state == SYSTEM_BOOTING) {
                if (request_irq(adev->irq, apbt_interrupt_handler,

which is probably exactly what we want to do to handle the crash I'm talking about.

This won't work right with the maxcpus commandline option set to something lower than the number of cpus present on the system. This option controls how many cpus the system tries to bring up during boot, while still allowing one to hotplug the rest of them later. If you do hotplug them later, system_state will no longer be SYSTEM_BOOTING, and this code won't do what it needs to do.