[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[Rabin Vincent]

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.

Signed-off-by: Rabin Vincent <rabin.vincent@stericsson.com>
---
 arch/arm/include/asm/pmu.h   |   14 ++++++++++++++
 arch/arm/kernel/perf_event.c |   17 ++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index 8ccea01..7544ce6 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -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
 
 /**
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 5efa264..3239511 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -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) {
-- 
1.7.2.dirty

[PATCHv2 2/2] ux500: DB8500 PMU support

[Rabin Vincent]

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.

Signed-off-by: Rabin Vincent <rabin.vincent@stericsson.com>
---
 arch/arm/mach-ux500/cpu-db8500.c |   51 +++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 706bc95..516126c 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -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,
-- 
1.7.2.dirty

[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[Will Deacon]

Hi Rabin,

> 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.
> 
> Signed-off-by: Rabin Vincent <rabin.vincent@stericsson.com>
> ---
>  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.

Acked-by: Will Deacon <will.deacon@arm.com>

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 <will.deacon@arm.com>

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 9def30b..512f55f 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -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

[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[Rabin Vincent]

On Fri, Feb 11, 2011 at 22:03, Will Deacon <will.deacon@arm.com> wrote:
>> 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.
>>
>> Signed-off-by: Rabin Vincent <rabin.vincent@stericsson.com>
>> ---
>> ?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.
>
> Acked-by: Will Deacon <will.deacon@arm.com>

Thanks.

>
> 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 <will.deacon@arm.com>
>
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 9def30b..512f55f 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -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.

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.

[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[Will Deacon]

Hi Rabin,

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

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

[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[Rabin Vincent]

On Thu, Feb 17, 2011 at 22:03, Will Deacon <will.deacon@arm.com> wrote:
>> 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?

[PATCHv2 1/2] ARM: perf_event: allow platform-specific interrupt handler

[Will Deacon]

> On Thu, Feb 17, 2011 at 22:03, Will Deacon <will.deacon@arm.com> wrote:
> >> 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