Skip to contentSkip to conversation

ARM: perf_event: allow platform-specific interrupt handler

PATCH New Rabin Vincent 3 participants 7 messages Feb 8, 2011 – Feb 18, 2011 2 messages not in the archive

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

PATCH v2 1/2 ARM: perf_event: allow platform-specific interrupt handler

Allow a platform-specific IRQ handler to be specified via platform data. This will be used to implement the single-irq workaround for the DB8500.

---
 arch/arm/include/asm/pmu.h   |   14 ++++++++++++++
 arch/arm/kernel/perf_event.c |   17 ++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)
arch/arm/include/asm/pmu.h +14 -0
index 8ccea01..7544ce6 100644
@@ -12,11 +12,25 @@
 #ifndef __ARM_PMU_H__
 #define __ARM_PMU_H__
 
+#include <linux/interrupt.h>
+
 enum arm_pmu_type {
 	ARM_PMU_DEVICE_CPU	= 0,
 	ARM_NUM_PMU_DEVICES,
 };
 
+/*
+ * struct arm_pmu_platdata - ARM PMU platform data
+ *
+ * @handle_irq: an optional handler which will be called from the interrupt and
+ * passed the address of the low level handler, and can be used to implement
+ * any platform specific handling before or after calling it.
+ */
+struct arm_pmu_platdata {
+	irqreturn_t (*handle_irq)(int irq, void *dev,
+				  irq_handler_t pmu_handler);
+};
+
 #ifdef CONFIG_CPU_HAS_PMU
 
 /**
arch/arm/kernel/perf_event.c +16 -1
index 5efa264..3239511 100644
@@ -377,9 +377,18 @@ validate_group(struct perf_event *event)
 	return 0;
 }
 
+static irqreturn_t armpmu_platform_irq(int irq, void *dev)
+{
+	struct arm_pmu_platdata *plat = dev_get_platdata(&pmu_device->dev);
+
+	return plat->handle_irq(irq, dev, armpmu->handle_irq);
+}
+
 static int
 armpmu_reserve_hardware(void)
 {
+	struct arm_pmu_platdata *plat;
+	irq_handler_t handle_irq;
 	int i, err = -ENODEV, irq;
 
 	pmu_device = reserve_pmu(ARM_PMU_DEVICE_CPU);
@@ -390,6 +399,12 @@ armpmu_reserve_hardware(void)
 
 	init_pmu(ARM_PMU_DEVICE_CPU);
 
+	plat = dev_get_platdata(&pmu_device->dev);
+	if (plat && plat->handle_irq)
+		handle_irq = armpmu_platform_irq;
+	else
+		handle_irq = armpmu->handle_irq;
+
 	if (pmu_device->num_resources < 1) {
 		pr_err("no irqs for PMUs defined\n");
 		return -ENODEV;
@@ -400,7 +415,7 @@ armpmu_reserve_hardware(void)
 		if (irq < 0)
 			continue;
 
-		err = request_irq(irq, armpmu->handle_irq,
+		err = request_irq(irq, handle_irq,
 				  IRQF_DISABLED | IRQF_NOBALANCING,
 				  "armpmu", NULL);
 		if (err) {
Signature · 1 line
1.7.2.dirty
#1 lore raw

PATCH v2 2/2 ux500: DB8500 PMU support

DB8500 has irqs from two cores ORed into one. Implement a workaround to handle this by bouncing the interrupt by setting the affinity to the other core when the interrupt appears to be spurious on the current core.

---
 arch/arm/mach-ux500/cpu-db8500.c |   51 +++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 4 deletions(-)
arch/arm/mach-ux500/cpu-db8500.c +47 -4
index 706bc95..516126c 100644
@@ -12,12 +12,14 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/amba/bus.h>
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/gpio.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
 
 #include <asm/mach/map.h>
+#include <asm/pmu.h>
 #include <mach/hardware.h>
 #include <mach/setup.h>
 #include <mach/devices.h>
@@ -26,10 +28,6 @@
 #include "devices-db8500.h"
 #include "ste-dma40-db8500.h"
 
-static struct platform_device *platform_devs[] __initdata = {
-	&u8500_dma40_device,
-};
-
 /* minimum static i/o mapping required to boot U8500 platforms */
 static struct map_desc u8500_uart_io_desc[] __initdata = {
 	__IO_DEV_DESC(U8500_UART0_BASE, SZ_4K),
@@ -91,6 +89,51 @@ void __init u8500_map_io(void)
 		iotable_init(u8500_v2_io_desc, ARRAY_SIZE(u8500_v2_io_desc));
 }
 
+static struct resource db8500_pmu_resources[] = {
+	[0] = {
+		.start		= IRQ_DB8500_PMU,
+		.end		= IRQ_DB8500_PMU,
+		.flags		= IORESOURCE_IRQ,
+	},
+};
+
+/*
+ * The PMU IRQ lines of two cores are wired together into a single interrupt.
+ * Bounce the interrupt to the other core if it's not ours.
+ */
+static irqreturn_t db8500_pmu_handler(int irq, void *dev, irq_handler_t handler)
+{
+	irqreturn_t ret = handler(irq, dev);
+	int other = !smp_processor_id();
+
+	if (ret == IRQ_NONE && cpu_online(other))
+		irq_set_affinity(irq, cpumask_of(other));
+
+	/*
+	 * We should be able to get away with the amount of IRQ_NONEs we give,
+	 * while still having the spurious IRQ detection code kick in if the
+	 * interrupt really starts hitting spuriously.
+	 */
+	return ret;
+}
+
+static struct arm_pmu_platdata db8500_pmu_platdata = {
+	.handle_irq		= db8500_pmu_handler,
+};
+
+static struct platform_device db8500_pmu_device = {
+	.name			= "arm-pmu",
+	.id			= ARM_PMU_DEVICE_CPU,
+	.num_resources		= ARRAY_SIZE(db8500_pmu_resources),
+	.resource		= db8500_pmu_resources,
+	.dev.platform_data	= &db8500_pmu_platdata,
+};
+
+static struct platform_device *platform_devs[] __initdata = {
+	&u8500_dma40_device,
+	&db8500_pmu_device,
+};
+
 static resource_size_t __initdata db8500_gpio_base[] = {
 	U8500_GPIOBANK0_BASE,
 	U8500_GPIOBANK1_BASE,
Signature · 1 line
1.7.2.dirty
#2 lore raw

PATCH v2 1/2 ARM: perf_event: allow platform-specific interrupt handler

In reply to Rabin Vincent

Hi Rabin,

Quoted 8 lines

Allow a platform-specific IRQ handler to be specified via platform data. This will be used to implement the single-irq workaround for the DB8500.

---
 arch/arm/include/asm/pmu.h   |   14 ++++++++++++++
 arch/arm/kernel/perf_event.c |   17 ++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)

If you're happy with this as a workaround for your platform, then it looks alright to me.

One thing you could try is using the GIC patch I posted the other day:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/041496.html

If you then do:

ARM: gic: allow per-cpu SPIs to be affine to multiple CPUs

The concept of a per-cpu SPI is somewhat a contradiction, but can occur in systems where SPIs from different CPUs are ORd together into a single line.

An example of this is the PMU interrupt on the u8500 platform.

This patch allows SPIs with the IRQF_PERCPU flag to be affine to multiple CPUs in a CPU mask. This, of course, assumes that the driver knows what it is doing and can handle such a configuration.

arch/arm/common/gic.c +12 -2
index 9def30b..512f55f 100644
@@ -145,7 +145,7 @@ gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
 {
        void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
        unsigned int shift = (d->irq % 4) * 8;
-       unsigned int cpu = cpumask_first(mask_val);
+       unsigned int cpu_map, cpu = cpumask_first(mask_val);
        u32 val;
        struct irq_desc *desc;
 
@@ -155,9 +155,19 @@ gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
                spin_unlock(&irq_controller_lock);
                return -EINVAL;
        }
+
        d->node = cpu;
+
+       if (CHECK_IRQ_PER_CPU(desc->status)) {
+               cpu_map = 0;
+               for_each_cpu(cpu, mask_val)
+                       cpu_map |= 1 << (cpu + shift);
+       } else {
+               cpu_map = 1 << (cpu + shift);
+       }
+
        val = readl(reg) & ~(0xff << shift);
-       val |= 1 << (cpu + shift);
+       val |= cpu_map;
        writel(val, reg);
        spin_unlock(&irq_controller_lock);
 

You'll be able to target the PMU IRQ to both CPUs and avoid the need for ping-ponging the affinity. This is a bit weird though as usually you'd have a PPI for a percpu interrupt so this might be better off staying inside platform code and leaving the GIC code alone. I also think this approach is more invasive from the perf point of view.

Unless this approach gives markedly better profiling results than your proposal, I think we should go with what you've got.

Will

A message that is not in the archive (sent off-list or not yet received): -6723806473392693428@unknownmsgid

#4 lore raw

PATCH v2 1/2 ARM: perf_event: allow platform-specific interrupt handler

Quoted 9 lines · Will Deacon
Quoted 8 lines

Allow a platform-specific IRQ handler to be specified via platform data. ?This will be used to implement the single-irq workaround for the DB8500.

---
?arch/arm/include/asm/pmu.h ? | ? 14 ++++++++++++++ ?arch/arm/kernel/perf_event.c | ? 17 ++++++++++++++++- ?2 files changed, 30 insertions(+), 1 deletions(-)

Quoted 4 lines

If you're happy with this as a workaround for your platform, then it looks alright to me.

Thanks.

Quoted 55 lines

One thing you could try is using the GIC patch I posted the other day:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/041496.html

If you then do:

? ?ARM: gic: allow per-cpu SPIs to be affine to multiple CPUs

? ?The concept of a per-cpu SPI is somewhat a contradiction, but can occur in ? ?systems where SPIs from different CPUs are ORd together into a single line.

? ?An example of this is the PMU interrupt on the u8500 platform.

? ?This patch allows SPIs with the IRQF_PERCPU flag to be affine to multiple ? ?CPUs in a CPU mask. This, of course, assumes that the driver knows what it ? ?is doing and can handle such a configuration.

? ?Signed-off-by: Will Deacon <[email protected]>

arch/arm/common/gic.c +12 -2
index 9def30b..512f55f 100644
@@ -145,7 +145,7 @@ gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
 ?{
 ? ? ? ?void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
 ? ? ? ?unsigned int shift = (d->irq % 4) * 8;
- ? ? ? unsigned int cpu = cpumask_first(mask_val);
+ ? ? ? unsigned int cpu_map, cpu = cpumask_first(mask_val);
 ? ? ? ?u32 val;
 ? ? ? ?struct irq_desc *desc;
 
@@ -155,9 +155,19 @@ gic_set_cpu(struct irq_data *d, const struct cpumask *mask_val, bool force)
 ? ? ? ? ? ? ? ?spin_unlock(&irq_controller_lock);
 ? ? ? ? ? ? ? ?return -EINVAL;
 ? ? ? ?}
+
 ? ? ? ?d->node = cpu;
+
+ ? ? ? if (CHECK_IRQ_PER_CPU(desc->status)) {
+ ? ? ? ? ? ? ? cpu_map = 0;
+ ? ? ? ? ? ? ? for_each_cpu(cpu, mask_val)
+ ? ? ? ? ? ? ? ? ? ? ? cpu_map |= 1 << (cpu + shift);
+ ? ? ? } else {
+ ? ? ? ? ? ? ? cpu_map = 1 << (cpu + shift);
+ ? ? ? }
+
 ? ? ? ?val = readl(reg) & ~(0xff << shift);
- ? ? ? val |= 1 << (cpu + shift);
+ ? ? ? val |= cpu_map;
 ? ? ? ?writel(val, reg);
 ? ? ? ?spin_unlock(&irq_controller_lock);
 
Quoted 8 lines

You'll be able to target the PMU IRQ to both CPUs and avoid the need for ping-ponging the affinity. This is a bit weird though as usually you'd have a PPI for a percpu interrupt so this might be better off staying inside platform code and leaving the GIC code alone. I also think this approach is more invasive from the perf point of view.

Unless this approach gives markedly better profiling results than your proposal, I think we should go with what you've got.

I gave this a try, along with the modifications to enable IRQ_PER_CPU and have the pmu code use the appropriate flags and set the affinity. Didn't work though; it always ends up triggering the spurious IRQ check.

#5 lore raw

PATCH v2 1/2 ARM: perf_event: allow platform-specific interrupt handler

Hi Rabin,

Quoted 6 lines
Quoted 6 lines

You'll be able to target the PMU IRQ to both CPUs and avoid the need for ping-ponging the affinity. This is a bit weird though as usually you'd have a PPI for a percpu interrupt so this might be better off staying inside platform code and leaving the GIC code alone. I also think this approach is more invasive from the perf point of view.

Quoted 6 lines
Quoted 2 lines

Unless this approach gives markedly better profiling results than your proposal, I think we should go with what you've got.

I gave this a try, along with the modifications to enable IRQ_PER_CPU and have the pmu code use the appropriate flags and set the affinity. Didn't work though; it always ends up triggering the spurious IRQ check.

Hmm, that doesn't sound right. Did you have any synchronisation to ensure that the CPU without the overflow didn't return IRQ_NONE until the handling CPU had returned IRQ_HANDLED?

Will

A message that is not in the archive (sent off-list or not yet received): -3663809140750500272@unknownmsgid

#7 lore raw

PATCH v2 1/2 ARM: perf_event: allow platform-specific interrupt handler

Quoted 7 lines · Will Deacon
Quoted 3 lines

I gave this a try, along with the modifications to enable IRQ_PER_CPU and have the pmu code use the appropriate flags and set the affinity. Didn't work though; it always ends up triggering the spurious IRQ check.

Hmm, that doesn't sound right. Did you have any synchronisation to ensure that the CPU without the overflow didn't return IRQ_NONE until the handling CPU had returned IRQ_HANDLED?

No. What kind of synchronization do you mean?

#8 lore raw

PATCH v2 1/2 ARM: perf_event: allow platform-specific interrupt handler

Quoted 10 lines · Will Deacon
Quoted 7 lines
Quoted 3 lines

I gave this a try, along with the modifications to enable IRQ_PER_CPU and have the pmu code use the appropriate flags and set the affinity. Didn't work though; it always ends up triggering the spurious IRQ check.

Hmm, that doesn't sound right. Did you have any synchronisation to ensure that the CPU without the overflow didn't return IRQ_NONE until the handling CPU had returned IRQ_HANDLED?

No. What kind of synchronization do you mean?

Well my guess is that the CPU returning IRQ_NONE is perpetually taking the same interrupt until the other CPU has poked the PMU to stop it asserting the line (otherwise I can't see how the spurious check would trip). You could have a lock in the driver to synchronise between the two CPUs so that if you don't have an overflow, you wait for the other CPU to clear the interrupt before returning.

This has quickly started to become horrible but I was just curious as to whether it provides better results than the affinity-setting approach (which is certainly less invasive).

Cheers,

Will