[PATCH 0/4] ARM: perf: Support i.MX53

[PATCH 0/4] ARM: perf: Support i.MX53

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

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

[Martin Fuzzey]

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 <mfuzzey@parkeon.com>
---
 arch/arm/include/asm/pmu.h      |    7 +++++++
 arch/arm/kernel/perf_event_v7.c |   23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index ae1919b..0bd181f 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -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 {
diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 1d37568..91a41bd 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -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);

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

[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 <mfuzzey@parkeon.com>
---
 arch/arm/include/asm/pmu.h   |    9 +++++++++
 arch/arm/kernel/perf_event.c |   13 ++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index 0bd181f..0f361c9 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -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 {
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 4238bcb..d0d9a25 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -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

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

[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 <mfuzzey@parkeon.com>
---
 arch/arm/mach-imx/mach-imx53.c |   55 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c
index 2bad387..b2cc235 100644
--- a/arch/arm/mach-imx/mach-imx53.c
+++ b/arch/arm/mach-imx/mach-imx53.c
@@ -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 = {

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

[Martin Fuzzey]

Add device tree node for PMU on i.MX53

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 arch/arm/boot/dts/imx53.dtsi |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index 6456a00..ab3afe0 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -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>;

[PATCH 0/4] ARM: perf: Support i.MX53

[Will Deacon]

Hi Martin,

On Tue, Jul 29, 2014 at 01:32:56PM +0100, Martin Fuzzey wrote:
> 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

[PATCH 0/4] ARM: perf: Support i.MX53

[Martin Fuzzey]

Hi Will ,

thanks for your reply.

On 29/07/14 14:52, Will Deacon wrote:
> Hi Martin,
>
> On Tue, Jul 29, 2014 at 01:32:56PM +0100, Martin Fuzzey wrote:
>> 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)
>   - It accesses the SDER unconditionally, which is undefined for non-secure
>     modes.
Ah yes I see what you mean that is a problem...

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

> 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

[PATCH 0/4] ARM: perf: Support i.MX53

[Will Deacon]

On Tue, Jul 29, 2014 at 05:40:46PM +0100, Martin Fuzzey wrote:
> On 29/07/14 14:52, Will Deacon wrote:
> > 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.

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

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

> What happens in non secure mode?

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

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

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

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