Skip to contentSkip to conversation

ARM: perf: Support i.MX53

PATCH New Martin Fuzzey 2 participants 8 messages Jul 29, 2014 – Jul 30, 2014

Subsystems (6)
Device tree bindings Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape +2 more subsystems
lore
Share & tools
lore

Martin Fuzzey

author
#0 lore raw

cover ARM: perf: Support i.MX53

This series enables hardware performance counters on the i.MX53 SoC

This requires setting registers at both the ARM V7 core level and the i.MX53 SoC level.

author
#1 lore raw

PATCH 1/4 ARM: perf: Set suniden bit.

Counters other than the CPU cycle counter only work if the security module SUNIDEN bit is set.

Without this:

# perf stat -e cycles,instructions sleep 1

Performance counter stats for 'sleep 1':

          14606094 cycles                    #    0.000 GHz
                 0 instructions              #    0.00  insns per cycle

Some platforms (eg i.MX53) may also need additional platform specific setup.

Signed-off-by Martin Fuzzey <[email protected]>
---
 arch/arm/include/asm/pmu.h      |    7 +++++++
 arch/arm/kernel/perf_event_v7.c |   23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+)
arch/arm/include/asm/pmu.h +7 -0
index ae1919b..0bd181f 100644
@@ -60,6 +60,13 @@ struct pmu_hw_events {
 	 * read/modify/write sequences.
 	 */
 	raw_spinlock_t		pmu_lock;
+
+	/*
+	 * Bits indicating any CPU or platform specific activations that have
+	 * been done so we can undo them when stopping
+	 */
+	unsigned int		activated_flags;
+	#define ARM_PMU_ACTIVATED_SECURE_DEBUG	(1 << 0)
 };
 
 struct arm_pmu {
arch/arm/kernel/perf_event_v7.c +23 -0
index 1d37568..91a41bd 100644
@@ -1377,12 +1377,26 @@ static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev)
 	return IRQ_HANDLED;
 }
 
+#define SDER_SUNIDEN (1 << 1)
+
 static void armv7pmu_start(struct arm_pmu *cpu_pmu)
 {
 	unsigned long flags;
 	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
+	u32 sder;
 
 	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+
+	/* Counters other than cycle counter require SUNIDEN bit set */
+	asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (sder));
+	if (sder & SDER_SUNIDEN) {
+		events->activated_flags &= ~ARM_PMU_ACTIVATED_SECURE_DEBUG;
+	} else {
+		sder |= SDER_SUNIDEN;
+		asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (sder));
+		events->activated_flags |= ARM_PMU_ACTIVATED_SECURE_DEBUG;
+	}
+
 	/* Enable all counters */
 	armv7_pmnc_write(armv7_pmnc_read() | ARMV7_PMNC_E);
 	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);

@@ -1392,8 +1406,17 @@ static void armv7pmu_stop(struct arm_pmu *cpu_pmu)
 {
 	unsigned long flags;
 	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
+	u32 sder;
 
 	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+
+	if (events->activated_flags & ARM_PMU_ACTIVATED_SECURE_DEBUG) {
+		asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (sder));
+		sder &= ~SDER_SUNIDEN;
+		asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (sder));
+		events->activated_flags &= ~ARM_PMU_ACTIVATED_SECURE_DEBUG;
+	}
+
 	/* Disable all counters */
 	armv7_pmnc_write(armv7_pmnc_read() & ~ARMV7_PMNC_E);
 	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
author
#2 lore raw

PATCH 2/4 ARM: perf: Add platform specific start/stop callbacks.

In reply to Martin Fuzzey

Some platforms (such as i.MX53) require SOC specific registers to be manipulated to make the PMU work.

Add callback hooks to support this.

Signed-off-by Martin Fuzzey <[email protected]>
---
 arch/arm/include/asm/pmu.h   |    9 +++++++++
 arch/arm/kernel/perf_event.c |   13 ++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)
arch/arm/include/asm/pmu.h +9 -0
index 0bd181f..0f361c9 100644
@@ -15,6 +15,8 @@
 #include <linux/interrupt.h>
 #include <linux/perf_event.h>
 
+struct arm_pmu;
+
 /*
  * struct arm_pmu_platdata - ARM PMU platform data
  *

@@ -32,12 +34,18 @@
  *	succession this handler will only be called following the
  *	final call to pm_runtime_put() that actually disables the
  *	hardware.
+ * @start: an optional handler which will be called before starting
+ *	the PMU to do any platform specific setup
+ * @stop: an optional handler which be called after stopping the PMU
+ *	to do any platform specific teardown
  */
 struct arm_pmu_platdata {
 	irqreturn_t (*handle_irq)(int irq, void *dev,
 				  irq_handler_t pmu_handler);
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_suspend)(struct device *dev);
+	void (*start)(struct arm_pmu *arm_pmu);
+	void (*stop)(struct arm_pmu *arm_pmu);
 };
 
 #ifdef CONFIG_HW_PERF_EVENTS

@@ -67,6 +75,7 @@ struct pmu_hw_events {
 	 */
 	unsigned int		activated_flags;
 	#define ARM_PMU_ACTIVATED_SECURE_DEBUG	(1 << 0)
+	#define ARM_PMU_ACTIVATED_PLATFORM	(1 << 1)
 };
 
 struct arm_pmu {
arch/arm/kernel/perf_event.c +12 -1
index 4238bcb..d0d9a25 100644
@@ -469,16 +469,27 @@ static void armpmu_enable(struct pmu *pmu)
 {
 	struct arm_pmu *armpmu = to_arm_pmu(pmu);
 	struct pmu_hw_events *hw_events = armpmu->get_hw_events();
+	struct arm_pmu_platdata *plat =
+				dev_get_platdata(&armpmu->plat_device->dev);
+
 	int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
 
-	if (enabled)
+	if (enabled) {
+		if (plat && plat->start)
+			plat->start(armpmu);
 		armpmu->start(armpmu);
+	}
 }
 
 static void armpmu_disable(struct pmu *pmu)
 {
 	struct arm_pmu *armpmu = to_arm_pmu(pmu);
+	struct arm_pmu_platdata *plat =
+				dev_get_platdata(&armpmu->plat_device->dev);
+
 	armpmu->stop(armpmu);
+	if (plat && plat->stop)
+		plat->stop(armpmu);
 }
 
 #ifdef CONFIG_PM_RUNTIME
author
#3 lore raw

PATCH 3/4 ARM: i.MX53: Add Soc specific PMU setup.

In reply to Martin Fuzzey

On i.MX53 it is necessary to set the DBG_EN bit in the platform GPC register to enable access to PMU counters other than the cycle counter.

Signed-off-by Martin Fuzzey <[email protected]>
---
 arch/arm/mach-imx/mach-imx53.c |   55 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)
arch/arm/mach-imx/mach-imx53.c +54 -1
index 2bad387..b2cc235 100644
@@ -19,16 +19,69 @@
 #include <linux/of_platform.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
+#include <asm/pmu.h>
 
 #include "common.h"
 #include "hardware.h"
 #include "mx53.h"
+#include "crm-regs-imx5.h"
+
+#define GPC_DBG_EN (1 << 16)
+
+static void imx53_pmu_start(struct arm_pmu *arm_pmu)
+{
+	unsigned long flags;
+	struct pmu_hw_events *events = arm_pmu->get_hw_events();
+	u32 gpc;
+
+	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+
+	gpc = __raw_readl(MXC_CORTEXA8_PLAT_GPC);
+	if (gpc & GPC_DBG_EN) {
+		events->activated_flags &= ~ARM_PMU_ACTIVATED_PLATFORM;
+	} else {
+		gpc |= GPC_DBG_EN;
+		__raw_writel(gpc, MXC_CORTEXA8_PLAT_GPC);
+		events->activated_flags |= ARM_PMU_ACTIVATED_PLATFORM;
+	}
+
+	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
+}
+
+static void imx53_pmu_stop(struct arm_pmu *arm_pmu)
+{
+	unsigned long flags;
+	struct pmu_hw_events *events = arm_pmu->get_hw_events();
+	u32 gpc;
+
+	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+
+	if (events->activated_flags & ARM_PMU_ACTIVATED_PLATFORM) {
+		gpc = __raw_readl(MXC_CORTEXA8_PLAT_GPC);
+		gpc &= ~GPC_DBG_EN;
+		__raw_writel(gpc, MXC_CORTEXA8_PLAT_GPC);
+		events->activated_flags &= ~ARM_PMU_ACTIVATED_PLATFORM;
+	}
+
+	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
+}
+
+static struct arm_pmu_platdata imx53_pmu_platdata = {
+	.start		= imx53_pmu_start,
+	.stop		= imx53_pmu_stop,
+};
+
+static struct of_dev_auxdata imx53_auxdata_lookup[] __initdata = {
+	OF_DEV_AUXDATA("arm,cortex-a8-pmu", 0, "arm-pmu", &imx53_pmu_platdata),
+	{}
+};
 
 static void __init imx53_dt_init(void)
 {
 	mxc_arch_reset_init_dt();
 
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	of_platform_populate(NULL, of_default_bus_match_table,
+					imx53_auxdata_lookup, NULL);
 }
 
 static const char *imx53_dt_board_compat[] __initconst = {
author
#4 lore raw

PATCH 4/4 ARM: dts: i.MX53: Add PMU DT entry

In reply to Martin Fuzzey

Add device tree node for PMU on i.MX53

Signed-off-by Martin Fuzzey <[email protected]>
---
 arch/arm/boot/dts/imx53.dtsi |    5 +++++
 1 file changed, 5 insertions(+)
arch/arm/boot/dts/imx53.dtsi +5 -0
index 6456a00..ab3afe0 100644
@@ -101,6 +101,11 @@
 		interrupt-parent = <&tzic>;
 		ranges;
 
+		pmu {
+			compatible = "arm,cortex-a8-pmu";
+			interrupts = <77>;
+		};
+
 		sata: sata at 10000000 {
 			compatible = "fsl,imx53-ahci";
 			reg = <0x10000000 0x1000>;
#5 lore raw

cover ARM: perf: Support i.MX53

In reply to Martin Fuzzey

Hi Martin,

Quoted 6 lines · Martin Fuzzey

This series enables hardware performance counters on the i.MX53 SoC

This requires setting registers at both the ARM V7 core level and the i.MX53 SoC level.

From a quick look at the patches, it sounds like you're booting in secure

mode and are trying to use perf there. Whilst I understand that you may want to do this, I don't think the code you currently have is quite right:

- It accesses the SDER unconditionally, which is undefined for non-secure modes.

- It adds yet more callbacks to arm_pmu_platdata

Instead, how about we add a new property on the PMU node that says it requires secure access, then we use that to configuer SDER in the perf code. Then you can do your SoC-specific magic as part of the runtime PM hooks we already have (it looks like enabled/disabling a clock really).

Will

author
#6 lore raw

cover ARM: perf: Support i.MX53

Hi Will ,

thanks for your reply.

Quoted 10 lines · Will Deacon

Hi Martin,

Quoted 4 lines · Martin Fuzzey

This series enables hardware performance counters on the i.MX53 SoC

This requires setting registers at both the ARM V7 core level and the i.MX53 SoC level.

From a quick look at the patches, it sounds like you're booting in secure
mode and are trying to use perf there. Whilst I understand that you may want to do this, I don't think the code you currently have is quite right:

Yes that's what I'm doing (I'm not doing any security stuff at all so the SoC is coming up in secure mode)

Quoted 2 lines

- It accesses the SDER unconditionally, which is undefined for non-secure modes.

Ah yes I see what you mean that is a problem...

Quoted 4 lines

- It adds yet more callbacks to arm_pmu_platdata

Instead, how about we add a new property on the PMU node that says it requires secure access, then we use that to configuer SDER in the perf code.

Ok but how would the new property be used?
Do you just mean making the code that sets the SUNIDEN bit conditional on this property?

So a DT for a board booting in secure mode would have the property set and one booting in non secure mode would not?

But is it the DT's business to know how the kernel has been booted?

What happens in non secure mode?
I presume the kernel won't touch SDER in that case (so avoiding an exception) but the counters will only work if code outside the kernel (bootloader, secure monitor?) has already set things up correctly?

Wouldn't it be possible to detect if the kernel is in secure mode and use that instead of a DT property?
Does the kernel have that information?

Quoted 2 lines

Then you can do your SoC-specific magic as part of the runtime PM hooks we already have (it looks like enabled/disabling a clock really).

This is orthogonal to the above right?

Indeed, I've tried that and it works without any extra hooks.

But what happens if we don't have CONFIG_PM_RUNTIME ?

Is "we don't care, enable it if you want perf to work" an acceptable answer?

Regards,

Martin

#7 lore raw

cover ARM: perf: Support i.MX53

Quoted 7 lines · Martin Fuzzey
Quoted 2 lines · Will Deacon

Instead, how about we add a new property on the PMU node that says it requires secure access, then we use that to configuer SDER in the perf code.

Ok but how would the new property be used?
Do you just mean making the code that sets the SUNIDEN bit conditional on this property?

We'd add a property to the PMU, so it would look something like:

pmu {
	compatible = "arm,cortex-a8-pmu";
	interrupts = < ... >;
	secure-reg-access;
};

Then we can match that last property in the driver and only configure the SDER if it's present.

Quoted 2 lines

So a DT for a board booting in secure mode would have the property set and one booting in non secure mode would not?

Yes. It's also slightly more general in that you can set the property for any PMU that requires SDER configuration. In practice, that probable means `booted in secure mode' but you never know what people build.

Quoted 1 line

But is it the DT's business to know how the kernel has been booted?

I don't have an alternative other than only supporting perf in non-secure mode.

Quoted 1 line

What happens in non secure mode?

Exactly the same as what happens now; the property would be absent so existing behaviour would be preserved.

Quoted 3 lines

I presume the kernel won't touch SDER in that case (so avoiding an exception) but the counters will only work if code outside the kernel (bootloader, secure monitor?) has already set things up correctly?

Right. Secure software could restrict non-secure debug visibility, but that's not something Linux can do anything about.

Quoted 3 lines

Wouldn't it be possible to detect if the kernel is in secure mode and use that instead of a DT property?
Does the kernel have that information?

No, you can't reliably detect that in the architecture.

Quoted 7 lines
Quoted 2 lines

Then you can do your SoC-specific magic as part of the runtime PM hooks we already have (it looks like enabled/disabling a clock really).

This is orthogonal to the above right?

Indeed, I've tried that and it works without any extra hooks.

But what happens if we don't have CONFIG_PM_RUNTIME ?

Then you don't get perf unless you configure things correctly in your firmware. This is just like configuring clocks.

Will