ARM: perf: Support i.MX53
Subsystems (6)
v2 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.
V2: Rework according to suggestions from Will Deacon:
* Make SDER access conditional on DT property
* Use existing runtime PM hooks rather than inventing new ones
PATCH v2 1/4 ARM: perf: Set suniden bit.
Counters other than the CPU cycle counter only work if the security module SUNIDEN bit is set.
Since access to this register is only possible in secure mode it will only be done if the device tree property "secure-reg-access" 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 cycleSome platforms (eg i.MX53) may also need additional platform specific setup.
--- Documentation/devicetree/bindings/arm/pmu.txt | 7 ++++ arch/arm/include/asm/pmu.h | 10 ++++++ arch/arm/kernel/perf_event_cpu.c | 3 ++ arch/arm/kernel/perf_event_v7.c | 44 +++++++++++++++++++++++++ 4 files changed, 64 insertions(+)
Documentation/devicetree/bindings/arm/pmu.txt +7 -0
@@ -27,6 +27,13 @@ Optional properties: - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd events. +- secure-reg-access : Indicates that secure mode access is available. + This will cause the driver to do any setup required that + is only possible in secure mode. + If not present the secure registers will not be touched, + which means the PMU may fail to operate unless external + code (bootloader or security monitor) has performed the + appropriate initialisation. Example: pmu {
arch/arm/include/asm/pmu.h +10 -0
@@ -89,6 +89,16 @@ struct arm_pmu {u64 max_period;struct platform_device *plat_device;struct pmu_hw_events *(*get_hw_events)(void ); + +/* +* Bits indicating any CPU or platform specific activations that have +* been done so we can undo them when stopping +*/ +struct { +unsigned int secure_regs_available :1 ; +unsigned int secure_debug_requested :1 ; +unsigned int platform_enabled :1 ; + } activated_flags; };#define to_arm_pmu (p) (container_of(p, struct arm_pmu, pmu))
arch/arm/kernel/perf_event_cpu.c +3 -0
@@ -313,6 +313,9 @@ static int cpu_pmu_device_probe(struct platform_device *pdev) cpu_pmu->plat_device = pdev;if (node && (of_id =of_match_node (cpu_pmu_of_device_ids, pdev->dev.of_node))) { + pmu->activated_flags.secure_regs_available = +of_property_read_bool (pdev->dev.of_node, +"secure-reg-access" ); init_fn = of_id->data; ret =init_fn (pmu); }else {
arch/arm/kernel/perf_event_v7.c +44 -0
62 lines not on this page · raw message
PATCH v2 1/4 ARM: perf: Set suniden bit.
Hi Martin,
Quoted 50 lines · Martin Fuzzey
Counters other than the CPU cycle counter only work if the security module SUNIDEN bit is set.
Since access to this register is only possible in secure mode it will only be done if the device tree property "secure-reg-access" is set.
Without this:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
14606094 cycles # 0.000 GHz 0 instructions # 0.00 insns per cycleSome platforms (eg i.MX53) may also need additional platform specific setup.
Signed-off-by Martin Fuzzey <[email protected]>--- Documentation/devicetree/bindings/arm/pmu.txt | 7 ++++ arch/arm/include/asm/pmu.h | 10 ++++++ arch/arm/kernel/perf_event_cpu.c | 3 ++ arch/arm/kernel/perf_event_v7.c | 44 +++++++++++++++++++++++++ 4 files changed, 64 insertions(+)Documentation/devicetree/bindings/arm/pmu.txt +7
-0@@ -27,6 +27,13 @@ Optional properties: - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd events. +- secure-reg-access : Indicates that secure mode access is available. + This will cause the driver to do any setup required that + is only possible in secure mode. + If not present the secure registers will not be touched, + which means the PMU may fail to operate unless external + code (bootloader or security monitor) has performed the + appropriate initialisation. Example: pmu {arch/arm/include/asm/pmu.h +0
-0@@ -89,6 +89,16 @@ struct arm_pmu {u64 max_period;struct platform_device *plat_device;struct pmu_hw_events *(*get_hw_events)(void );
arch/arm/include/asm/pmu.h struct arm_pmu { · line 101
Quoted 10 lines
arch/arm/include/asm/pmu.h +10
-0+ +/* +* Bits indicating any CPU or platform specific activations that have +* been done so we can undo them when stopping +*/ +struct { +unsigned int secure_regs_available :1 ; +unsigned int secure_debug_requested :1 ; +unsigned int platform_enabled :1 ; + } activated_flags;
This should be a single bool secure_access; field.
Quoted 30 lines
arch/arm/include/asm/pmu.h +0
-0};#define to_arm_pmu (p) (container_of(p, struct arm_pmu, pmu))arch/arm/kernel/perf_event_cpu.c +3
-0@@ -313,6 +313,9 @@ static int cpu_pmu_device_probe(struct platform_device *pdev) cpu_pmu->plat_device = pdev;if (node && (of_id =of_match_node (cpu_pmu_of_device_ids, pdev->dev.of_node))) { + pmu->activated_flags.secure_regs_available = +of_property_read_bool (pdev->dev.of_node, +"secure-reg-access" ); init_fn = of_id->data; ret =init_fn (pmu); }else {arch/arm/kernel/perf_event_v7.c +5
-0@@ -1377,12 +1377,47 @@ static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev)return IRQ_HANDLED ; } +#define SDER_SUNIDEN (1 << 1) + +static inline u32 armv2pmu_read_sder (void ) +{ +u32 sder;
arch/arm/kernel/perf_event_v7.c static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev) · line 1394
Quoted 10 lines
arch/arm/kernel/perf_event_v7.c +10
-0+ + asmvolatile ("mrc p15, 0, %0, c1, c1, 1" :"=r" (sder)); + +return sder; +} + +static inline void armv2pmu_write_sder (u32 sder) +{ + asmvolatile ("mcr p15, 0, %0, c1, c1, 1" : :"r" (sder)); +}
Please stick with the naming convention of the file (armv7pmu_*). You can also combine these functions into armv7pmu_enable_secure_access...
Quoted 13 lines
arch/arm/kernel/perf_event_v7.c +7
-0static void armv7pmu_start (struct arm_pmu *cpu_pmu) {unsigned long flags;struct pmu_hw_events *events = cpu_pmu->get_hw_events ();raw_spin_lock_irqsave (&events->pmu_lock, flags); + +/* +* Counters other than cycle counter require SUNIDEN bit set +* Set the bit if the DT configuration allows it (secure mode) +* Otherwise do nothing and hope bootloader / secure monitor did the +* setup. +*/
arch/arm/kernel/perf_event_v7.c static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev) · line 1418
Quoted 10 lines
arch/arm/kernel/perf_event_v7.c +10
-0+if (cpu_pmu->activated_flags.secure_regs_available) { +u32 sder =armv2pmu_read_sder (); + +if (sder &SDER_SUNIDEN ) { + cpu_pmu->activated_flags.secure_debug_requested =0 ; + }else { + sder |=SDER_SUNIDEN ; +armv2pmu_write_sder (sder); + cpu_pmu->activated_flags.secure_debug_requested =1 ; + }
... then just call that here.
Quoted 7 lines
arch/arm/kernel/perf_event_v7.c +2
-0+ } +/* Enable all counters */ armv7_pmnc_write (armv7_pmnc_read () |ARMV7_PMNC_E );raw_spin_unlock_irqrestore (&events->pmu_lock, flags);@@ -1394,6 +1429,15 @@ static void armv7pmu_stop(struct arm_pmu *cpu_pmu)struct pmu_hw_events *events = cpu_pmu->get_hw_events ();
arch/arm/kernel/perf_event_v7.c static void armv7pmu_stop(struct arm_pmu *cpu_pmu) · line 1439
Quoted 10 lines
arch/arm/kernel/perf_event_v7.c +8
-0raw_spin_lock_irqsave (&events->pmu_lock, flags); + +if (cpu_pmu->activated_flags.secure_debug_requested) { +u32 sder =armv2pmu_read_sder (); + + sder &= ~SDER_SUNIDEN ; +armv2pmu_write_sder (sder); + cpu_pmu->activated_flags.secure_debug_requested =0 ; + }
Why do we need to disable this? Couldn't we just set this once during probe and be done with it?
Will
PATCH v2 1/4 ARM: perf: Set suniden bit.
Hi Will,
thanks for the review.
Quoted 10 lines · Will Deacon
arch/arm/include/asm/pmu.h +9
-0+/* +* Bits indicating any CPU or platform specific activations that have +* been done so we can undo them when stopping +*/ +struct { +unsigned int secure_regs_available :1 ; +unsigned int secure_debug_requested :1 ; +unsigned int platform_enabled :1 ; + } activated_flags;This should be a single bool
secure_access; field.
The idea was to only actually modify the security settings while someone actually uses perf rather than just anytime the driver is loaded. But I didn't want to disable the bit when exiting perf mode if it had already been enabled elsewhere (eg by the bootloader).
Hence the first two bits.
I'm not sure how important this really is but it seemed sensible and not too hard.
The third one is no longer necessary if the platform code handles all that itself as you suggest in comments on subsequent patch.
Quoted 10 lines
Quoted 10 lines
arch/arm/kernel/perf_event_v7.c +9
-0+#define SDER_SUNIDEN (1 << 1) + +static inline u32 armv2pmu_read_sder (void ) +{ +u32 sder; + + asmvolatile ("mrc p15, 0, %0, c1, c1, 1" :"=r" (sder)); + +return sder;
Quoted 8 lines
arch/arm/kernel/perf_event_v7.c static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev) · line 1394
Quoted 6 lines
arch/arm/kernel/perf_event_v7.c +6
-0+} + +static inline void armv2pmu_write_sder (u32 sder) +{ + asmvolatile ("mcr p15, 0, %0, c1, c1, 1" : :"r" (sder)); +}Please stick with the naming convention of the file (armv7pmu_*). You can also combine these functions into
armv7pmu_enable_secure_access...
Argh typo, sorry
Quoted 9 lines
.
Quoted 8 lines
arch/arm/kernel/perf_event_v7.c +2
-0+ } +/* Enable all counters */ armv7_pmnc_write (armv7_pmnc_read () |ARMV7_PMNC_E );raw_spin_unlock_irqrestore (&events->pmu_lock, flags);@@ -1394,6 +1429,15 @@ static void armv7pmu_stop(struct arm_pmu *cpu_pmu)struct pmu_hw_events *events = cpu_pmu->get_hw_events ();
Quoted 11 lines
arch/arm/kernel/perf_event_v7.c static void armv7pmu_stop(struct arm_pmu *cpu_pmu) · line 1439
Quoted 9 lines
arch/arm/kernel/perf_event_v7.c +8
-0raw_spin_lock_irqsave (&events->pmu_lock, flags); + +if (cpu_pmu->activated_flags.secure_debug_requested) { +u32 sder =armv2pmu_read_sder (); + + sder &= ~SDER_SUNIDEN ; +armv2pmu_write_sder (sder); + cpu_pmu->activated_flags.secure_debug_requested =0 ; + }Why do we need to disable this? Couldn't we just set this once during probe and be done with it?
Same as my remark above
Martin
PATCH v2 1/4 ARM: perf: Set suniden bit.
Quoted 18 lines · Martin Fuzzey
Quoted 10 lines · Will Deacon
arch/arm/include/asm/pmu.h +9
-0+/* +* Bits indicating any CPU or platform specific activations that have +* been done so we can undo them when stopping +*/ +struct { +unsigned int secure_regs_available :1 ; +unsigned int secure_debug_requested :1 ; +unsigned int platform_enabled :1 ; + } activated_flags;This should be a single bool
secure_access; field.The idea was to only actually modify the security settings while someone actually uses perf rather than just anytime the driver is loaded. But I didn't want to disable the bit when exiting perf mode if it had already been enabled elsewhere (eg by the bootloader).
Hence the first two bits.
Quoted 2 lines
I'm not sure how important this really is but it seemed sensible and not too hard.
I think it's unnecessary complication. We've booted secure, we're not spawning a non-secure OS and we're not doing any strange soft-virtualisation. This means that we own the PMU and can configure it once during probe.
Will
PATCH v3 1/1 ARM: perf: Set suniden bit
From: Martin Fuzzey <[email protected]>
Counters other than the CPU cycle counter only work if the security module SUNIDEN bit is set.
Since access to this register is only possible in secure mode it will only be done if the device tree property "secure-reg-access" 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 cycleAfter applying:
# perf stat -e cycles,instructions sleep 1
Performance counter stats for 'sleep 1':
5843809 cycles
2566484 instructions # 0.44 insns per cycle
1.020144000 seconds time elapsedSome platforms (eg i.MX53) may also need additional platform specific setup.
---
Changes in v3:
- Pooya Keshavarzi:
* v2 review comment fixups
* Use on_each_cpu() to set SUNIDEN on all CPUs
* Move armv7pmu_enable_secure_access() call from armv7pmu_start() to armv7_a8_map_event() such that is called only once instead of each time perf is executed
- George G. Davis:
* Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
factor arm_pmu core out to drivers)
* Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
Documentation/devicetree/bindings/arm/pmu.txt | 8 ++++++++ arch/arm/kernel/perf_event_v7.c | 17 +++++++++++++++++ drivers/perf/arm_pmu.c | 3 +++ include/linux/perf/arm_pmu.h | 1 + 4 files changed, 29 insertions(+)
Documentation/devicetree/bindings/arm/pmu.txt +8 -0
@@ -45,6 +45,14 @@ Optional properties: - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd events. +- secure-reg-access : Indicates that secure mode access is available. + This will cause the driver to do any setup required that + is only possible in secure mode. + If not present the secure registers will not be touched, + which means the PMU may fail to operate unless external + code (bootloader or security monitor) has performed the + appropriate initialisation. + Example: pmu {
arch/arm/kernel/perf_event_v7.c +17 -0
@@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]#define ARMV7_EXCLUDE_USER (1 << 30)#define ARMV7_INCLUDE_HYP (1 << 27) +/* +* Secure debug enable reg +*/ +#define ARMV7_SDER_SUNIDEN BIT(1)/* Permit non-invasive debug */ +static inline u32 armv7_pmnc_read (void ) {u32 val;
@@ -994,6 +999,15 @@ static void armv7pmu_reset(void *info)armv7_pmnc_write (ARMV7_PMNC_P |ARMV7_PMNC_C ); } +static void armv7pmu_enable_secure_access (void *data) +{ +u32 val; + + asmvolatile ("mrc p15, 0, %0, c1, c1, 1" :"=r" (val)); + val |=ARMV7_SDER_SUNIDEN ; + asmvolatile ("mcr p15, 0, %0, c1, c1, 1" : :"r" (val)); +} +static int armv7_a8_map_event (struct perf_event *event) { return armpmu_map_event(event, &armv7_a8_perf_map,
@@ -1060,6 +1074,9 @@ static void armv7pmu_init(struct arm_pmu *cpu_pmu) cpu_pmu->stop = armv7pmu_stop; cpu_pmu->reset = armv7pmu_reset; cpu_pmu->max_period = (1LLU <<32 ) -1 ; + +if (cpu_pmu->secure_access) +on_each_cpu (armv7pmu_enable_secure_access,NULL ,1 ); };static void armv7_read_num_pmnc_events (void *info)
drivers/perf/arm_pmu.c +3 -0
@@ -895,6 +895,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,if (node && (of_id =of_match_node (of_table, pdev->dev.of_node))) { init_fn = of_id->data; + pmu->secure_access =of_property_read_bool (pdev->dev.of_node, +"secure-reg-access" ); + ret =of_pmu_irq_cfg (pmu);if (!ret) ret =init_fn (pmu);
include/linux/perf/arm_pmu.h +1 -0
@@ -104,6 +104,7 @@ struct arm_pmu {atomic_t active_events;struct mutex reserve_mutex;u64 max_period; +bool secure_access;struct platform_device *plat_device;struct pmu_hw_events __percpu *hw_events;struct notifier_block hotplug_nb;
Signature · 1 line
1.9.3
PATCH v3 1/1 ARM: perf: Set suniden bit
Quoted 57 lines · George G. Davis
From: Martin Fuzzey <[email protected]>
Counters other than the CPU cycle counter only work if the security module SUNIDEN bit is set.
Since access to this register is only possible in secure mode it will only be done if the device tree property "secure-reg-access" is set.
Without this:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
14606094 cycles # 0.000 GHz 0 instructions # 0.00 insns per cycleAfter applying:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
5843809 cycles 2566484 instructions # 0.44 insns per cycle 1.020144000 seconds time elapsedSome platforms (eg i.MX53) may also need additional platform specific setup.
Signed-off-by Martin Fuzzey <[email protected]>Signed-off-by Pooya Keshavarzi <[email protected]>Signed-off-by George G. Davis <[email protected]>---
Changes in v3:
- Pooya Keshavarzi:
* v2 review comment fixups
* Useon_each_cpu()to set SUNIDEN on all CPUs
* Movearmv7pmu_enable_secure_access()call fromarmv7pmu_start()toarmv7_a8_map_event()such that is called only once instead of each timeperfis executed
- George G. Davis:
* Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
factorarm_pmucore out to drivers)
* Fix checkpatch 'CHECK: Prefer using the BIT macro' issueDocumentation/devicetree/bindings/arm/pmu.txt | 8 ++++++++ arch/arm/kernel/perf_event_v7.c | 17 +++++++++++++++++ drivers/perf/arm_pmu.c | 3 +++ include/linux/perf/arm_pmu.h | 1 + 4 files changed, 29 insertions(+)Documentation/devicetree/bindings/arm/pmu.txt +0
-0
Documentation/devicetree/bindings/arm/pmu.txt Optional properties: · line 54
Quoted 10 lines
Documentation/devicetree/bindings/arm/pmu.txt +7
-0@@ -45,6 +45,14 @@ Optional properties: - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd events. +- secure-reg-access : Indicates that secure mode access is available. + This will cause the driver to do any setup required that + is only possible in secure mode. + If not present the secure registers will not be touched, + which means the PMU may fail to operate unless external + code (bootloader or security monitor) has performed the + appropriate initialisation.
You should make this clear that it's for ARMv7 CPUs only and is not supported on anything else (in particular, the arm64 port requires you to boot in non-secure mode).
You should also run this binding change by a devicetree maintainer.
Quoted 22 lines
Documentation/devicetree/bindings/arm/pmu.txt +1
-0+ Example: pmu {arch/arm/kernel/perf_event_v7.c +5
-0@@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]#define ARMV7_EXCLUDE_USER (1 << 30)#define ARMV7_INCLUDE_HYP (1 << 27) +/* +* Secure debug enable reg +*/ +#define ARMV7_SDER_SUNIDEN BIT(1)/* Permit non-invasive debug */ +static inline u32 armv7_pmnc_read (void ) {u32 val;@@ -994,6 +999,15 @@ static void armv7pmu_reset(void *info)armv7_pmnc_write (ARMV7_PMNC_P |ARMV7_PMNC_C );
arch/arm/kernel/perf_event_v7.c static void armv7pmu_reset(void *info) · line 1009
Quoted 10 lines
arch/arm/kernel/perf_event_v7.c +8
-0} +static void armv7pmu_enable_secure_access (void *data) +{ +u32 val; + + asmvolatile ("mrc p15, 0, %0, c1, c1, 1" :"=r" (val)); + val |=ARMV7_SDER_SUNIDEN ; + asmvolatile ("mcr p15, 0, %0, c1, c1, 1" : :"r" (val)); +}
This is probably better off in the ->reset callback, since that it called off the back of a CPU hotplug notifier when the PMU may need to be reinitialised.
arch/arm/kernel/perf_event_v7.c static void armv7pmu_init(struct arm_pmu *cpu_pmu) · line 1079
Quoted 10 lines
arch/arm/kernel/perf_event_v7.c +3
-0static int armv7_a8_map_event (struct perf_event *event) { return armpmu_map_event(event, &armv7_a8_perf_map,@@ -1060,6 +1074,9 @@ static void armv7pmu_init(struct arm_pmu *cpu_pmu) cpu_pmu->stop = armv7pmu_stop; cpu_pmu->reset = armv7pmu_reset; cpu_pmu->max_period = (1LLU <<32 ) -1 ; + +if (cpu_pmu->secure_access) +on_each_cpu (armv7pmu_enable_secure_access,NULL ,1 );
Then you can remove this too, since ->reset is called on the relevant cores already.
Will
PATCH v4 1/1 ARM: perf: Set ARMv7 SDER SUNIDEN bit
From: Martin Fuzzey <[email protected]>
ARMv7 counters other than the CPU cycle counter only work if the Secure Debug Enable Register (SDER) SUNIDEN bit is set.
Since access to the SDER is only possible in secure state, it will only be done if the device tree property "secure-reg-access" 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 cycleAfter applying:
# perf stat -e cycles,instructions sleep 1
Performance counter stats for 'sleep 1':
5843809 cycles
2566484 instructions # 0.44 insns per cycle
1.020144000 seconds time elapsedSome platforms (eg i.MX53) may also need additional platform specific setup.
---
Changes in v4:
- Reword commit message to clarify that this change is ARMv7 specific.
- Clarify that secure-reg-access property is only valid for ARMv7 CPUs and is not supported on anything else (in particular, the arm64 port requires you to boot in non-secure mode).
- Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in armv7pmu_init() to in-lined code in ->reset callback, since that is called off the back of a CPU hotplug notifier when the PMU may need to be reinitialised.
Changes in v3:
- Pooya Keshavarzi:
* v2 review comment fixups
* Use on_each_cpu() to set SUNIDEN on all CPUs
* Move armv7pmu_enable_secure_access() call from armv7pmu_start() to armv7_a8_map_event() such that is called only once instead of each time perf is executed
- George G. Davis:
* Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
factor arm_pmu core out to drivers)
* Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
--- Documentation/devicetree/bindings/arm/pmu.txt | 10 ++++++++++ arch/arm/kernel/perf_event_v7.c | 13 ++++++++++++- drivers/perf/arm_pmu.c | 3 +++ include/linux/perf/arm_pmu.h | 1 + 4 files changed, 26 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/arm/pmu.txt +10 -0
@@ -45,6 +45,16 @@ Optional properties: - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd events. +- secure-reg-access : Indicates that the ARMv7 Secure Debug Enable Register + (SDER) is accessible. This will cause the driver to do + any setup required that is only possible in ARMv7 secure + state. If not present the ARMv7 SDER will not be touched, + which means the PMU may fail to operate unless external + code (bootloader or security monitor) has performed the + appropriate initialisation. Note that this property is + not valid for non-ARMv7 CPUs or ARMv7 CPUs booting Linux + in Non-secure state. + Example: pmu {
arch/arm/kernel/perf_event_v7.c +12 -1
@@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]#define ARMV7_EXCLUDE_USER (1 << 30)#define ARMV7_INCLUDE_HYP (1 << 27) +/* +* Secure debug enable reg +*/ +#define ARMV7_SDER_SUNIDEN BIT(1)/* Permit non-invasive debug */ +static inline u32 armv7_pmnc_read (void ) {u32 val;
@@ -982,7 +987,13 @@ static int armv7pmu_set_event_filter(struct hw_perf_event *event,static void armv7pmu_reset (void *info) {struct arm_pmu *cpu_pmu = (struct arm_pmu *)info; -u32 idx, nb_cnt = cpu_pmu->num_events; +u32 idx, nb_cnt = cpu_pmu->num_events, val; + +if (cpu_pmu->secure_access) { + asmvolatile ("mrc p15, 0, %0, c1, c1, 1" :"=r" (val)); + val |=ARMV7_SDER_SUNIDEN ; + asmvolatile ("mcr p15, 0, %0, c1, c1, 1" : :"r" (val)); + }/* The counter and interrupt enable registers are unknown at reset. */ for (idx =ARMV7_IDX_CYCLE_COUNTER ; idx < nb_cnt; ++idx) {
drivers/perf/arm_pmu.c +3 -0
@@ -895,6 +895,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,if (node && (of_id =of_match_node (of_table, pdev->dev.of_node))) { init_fn = of_id->data; + pmu->secure_access =of_property_read_bool (pdev->dev.of_node, +"secure-reg-access" ); + ret =of_pmu_irq_cfg (pmu);if (!ret) ret =init_fn (pmu);
include/linux/perf/arm_pmu.h +1 -0
@@ -104,6 +104,7 @@ struct arm_pmu {atomic_t active_events;struct mutex reserve_mutex;u64 max_period; +bool secure_access;struct platform_device *plat_device;struct pmu_hw_events __percpu *hw_events;struct notifier_block hotplug_nb;
Signature · 1 line
1.9.3
Rob Herring
maintainerPATCH v4 1/1 ARM: perf: Set ARMv7 SDER SUNIDEN bit
Quoted 53 lines · George G. Davis
From: Martin Fuzzey <[email protected]>
ARMv7 counters other than the CPU cycle counter only work if the Secure Debug Enable Register (SDER) SUNIDEN bit is set.
Since access to the SDER is only possible in secure state, it will only be done if the device tree property "secure-reg-access" is set.
Without this:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
14606094 cycles # 0.000 GHz 0 instructions # 0.00 insns per cycleAfter applying:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
5843809 cycles 2566484 instructions # 0.44 insns per cycle 1.020144000 seconds time elapsedSome platforms (eg i.MX53) may also need additional platform specific setup.
Signed-off-by Martin Fuzzey <[email protected]>Signed-off-by Pooya Keshavarzi <[email protected]>Signed-off-by George G. Davis <[email protected]>---
Changes in v4:
- Reword commit message to clarify that this change is ARMv7 specific.
- Clarify that secure-reg-access property is only valid for ARMv7 CPUs and is not supported on anything else (in particular, the arm64 port requires you to boot in non-secure mode).
- Converton_each_cpu(armv7pmu_enable_secure_access, NULL, 1)call inarmv7pmu_init()to in-lined code in ->reset callback, since that is called off the back of a CPU hotplug notifier when the PMU may need to be reinitialised.
Changes in v3:
- Pooya Keshavarzi:
* v2 review comment fixups
* Useon_each_cpu()to set SUNIDEN on all CPUs
* Movearmv7pmu_enable_secure_access()call fromarmv7pmu_start()toarmv7_a8_map_event()such that is called only once instead of each timeperfis executed
- George G. Davis:
* Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
factorarm_pmucore out to drivers)
* Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
Quoted 2 lines
--- Documentation/devicetree/bindings/arm/pmu.txt | 10 ++++++++++
Rob
PATCH v4 1/1 ARM: perf: Set ARMv7 SDER SUNIDEN bit
In reply to George G. DavisQuoted 32 lines · George G. Davis
From: Martin Fuzzey <[email protected]>
ARMv7 counters other than the CPU cycle counter only work if the Secure Debug Enable Register (SDER) SUNIDEN bit is set.
Since access to the SDER is only possible in secure state, it will only be done if the device tree property "secure-reg-access" is set.
Without this:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
14606094 cycles # 0.000 GHz 0 instructions # 0.00 insns per cycleAfter applying:
# perf stat -e cycles,instructions sleep 1Performance counter stats for 'sleep 1':
5843809 cycles 2566484 instructions # 0.44 insns per cycle 1.020144000 seconds time elapsedSome platforms (eg i.MX53) may also need additional platform specific setup.
Signed-off-by Martin Fuzzey <[email protected]>Signed-off-by Pooya Keshavarzi <[email protected]>Signed-off-by George G. Davis <[email protected]>
Quoted 10 lines
---
Changes in v4:
- Reword commit message to clarify that this change is ARMv7 specific.
- Clarify that secure-reg-access property is only valid for ARMv7 CPUs and is not supported on anything else (in particular, the arm64 port requires you to boot in non-secure mode).
- Converton_each_cpu(armv7pmu_enable_secure_access, NULL, 1)call inarmv7pmu_init()to in-lined code in ->reset callback, since that is called off the back of a CPU hotplug notifier when the PMU may need to be reinitialised.
Thanks. I'll queue this for 4.6 along with Rob's ack.
Will
PATCH v2 2/4 ARM: perf: Associate PMU data with driver.
In reply to Martin FuzzeyIn order to use the PM hooks for platform specific control we sometimes need access to the PMU driver data.
--- arch/arm/kernel/perf_event_cpu.c | 1 + 1 file changed, 1 insertion(+)
arch/arm/kernel/perf_event_cpu.c +1 -0
@@ -311,6 +311,7 @@ static int cpu_pmu_device_probe(struct platform_device *pdev) cpu_pmu = pmu; cpu_pmu->plat_device = pdev; +dev_set_drvdata (&pdev->dev, pmu);if (node && (of_id =of_match_node (cpu_pmu_of_device_ids, pdev->dev.of_node))) { pmu->activated_flags.secure_regs_available =
PATCH v2 2/4 ARM: perf: Associate PMU data with driver.
Quoted 13 lines · Martin Fuzzey
In order to use the PM hooks for platform specific control we sometimes need access to the PMU driver data.
Signed-off-by Martin Fuzzey <[email protected]>--- arch/arm/kernel/perf_event_cpu.c | 1 + 1 file changed, 1 insertion(+)arch/arm/kernel/perf_event_cpu.c +0
-0
arch/arm/kernel/perf_event_cpu.c static int cpu_pmu_device_probe(struct platform_device *pdev) · line 314
Quoted 4 lines
arch/arm/kernel/perf_event_cpu.c +1
-0@@ -311,6 +311,7 @@ static int cpu_pmu_device_probe(struct platform_device *pdev) cpu_pmu = pmu; cpu_pmu->plat_device = pdev; +dev_set_drvdata (&pdev->dev, pmu);
I'd rather the platform-specific code used its own structures to keep track of what it's doing. Exposing the PMU like this is almost certainly going to cause us problems later on.
Will
PATCH v2 2/4 ARM: perf: Associate PMU data with driver.
Quoted 15 lines · Will Deacon
Quoted 14 lines · Martin Fuzzey
In order to use the PM hooks for platform specific control we sometimes need access to the PMU driver data.
Signed-off-by Martin Fuzzey <[email protected]>--- arch/arm/kernel/perf_event_cpu.c | 1 + 1 file changed, 1 insertion(+)arch/arm/kernel/perf_event_cpu.c +0
-0@@ -311,6 +311,7 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
Quoted 7 lines
arch/arm/kernel/perf_event_cpu.c static int cpu_pmu_device_probe(struct platform_device *pdev) · line 314
Quoted 3 lines
arch/arm/kernel/perf_event_cpu.c +1
-0cpu_pmu = pmu; cpu_pmu->plat_device = pdev; +dev_set_drvdata (&pdev->dev, pmu);I'd rather the platform-specific code used its own structures to keep track of what it's doing. Exposing the PMU like this is almost certainly going to cause us problems later on.
+1 - it's fine for the driver itself to access its private data in the device struct, but this is not supposed to be a mechanism for passing stuff between drivers, or indeed code outside of the driver.
A better solution would be a proper API to obtain a reference to this data - and that must have some side effects to protect against the driver being unbound at /any/ moment, and therefore must also have an interface to tell the driver when it can proceed with being unbound.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.
PATCH v2 3/4 ARM: i.MX53: Add Soc specific PMU setup.
In reply to Martin FuzzeyOn 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.
--- arch/arm/mach-imx/mach-imx53.c | 50 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-)
arch/arm/mach-imx/mach-imx53.c +49 -1
@@ -19,16 +19,64 @@#include <linux/of_platform.h> #include <asm/mach/arch.h> #include <asm/mach/time.h> +#include <asm/pmu.h> #include "common.h" +#include "crm-regs-imx5.h" #include "hardware.h" #include "mx53.h" +#define GPC_DBG_EN (1 << 16) + +static int imx53_pmu_resume (struct device *dev) +{ +struct arm_pmu *arm_pmu =dev_get_drvdata (dev); +u32 gpc; + + gpc =__raw_readl (MXC_CORTEXA8_PLAT_GPC ); +if (gpc &GPC_DBG_EN ) { + arm_pmu->activated_flags.platform_enabled =0 ; + }else { + gpc |=GPC_DBG_EN ; +__raw_writel (gpc,MXC_CORTEXA8_PLAT_GPC ); + arm_pmu->activated_flags.platform_enabled =1 ; + } + +return 0 ; +} + +static int imx53_pmu_suspend (struct device *dev) +{ +struct arm_pmu *arm_pmu =dev_get_drvdata (dev); +u32 gpc; + +if (arm_pmu->activated_flags.platform_enabled) { + gpc =__raw_readl (MXC_CORTEXA8_PLAT_GPC ); + gpc &= ~GPC_DBG_EN ; +__raw_writel (gpc,MXC_CORTEXA8_PLAT_GPC ); + arm_pmu->activated_flags.platform_enabled =0 ; + } + +return 0 ; +} + + +static struct arm_pmu_platdata imx53_pmu_platdata = { + .runtime_resume = imx53_pmu_resume, + .runtime_suspend = imx53_pmu_suspend, +}; + +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 v2 4/4 ARM: dts: i.MX53: Add PMU DT entry
In reply to Martin FuzzeyAdd device tree node for PMU on i.MX53
--- arch/arm/boot/dts/imx53.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm/boot/dts/imx53.dtsi +5 -0
@@ -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 v2 4/4 ARM: dts: i.MX53: Add PMU DT entry
Quoted 12 lines · 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 +0
-0
arch/arm/boot/dts/imx53.dtsi line 107
Quoted 7 lines
arch/arm/boot/dts/imx53.dtsi +4
-0@@ -101,6 +101,11 @@ interrupt-parent = <&tzic>; ranges; + pmu { + compatible = "arm,cortex-a8-pmu"; + interrupts = <77>; + };
You're missing the new property.
Will
PATCH v2 4/4 ARM: dts: i.MX53: Add PMU DT entry
Quoted 16 lines · Will Deacon
Quoted 15 lines · 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 +0
-0@@ -101,6 +101,11 @@ interrupt-parent = <&tzic>; ranges;
Quoted 5 lines
arch/arm/boot/dts/imx53.dtsi line 107
Quoted 4 lines
arch/arm/boot/dts/imx53.dtsi +4
-0+ pmu { + compatible = "arm,cortex-a8-pmu"; + interrupts = <77>; + };You're missing the new property.
Actually this was deliberate.
Since the property should only be used if booting in secure mode this generic dtsi file can't know if it is ok or not.
I presume a (probably out of tree) board / application dts file will add the property.
Maybe I should put a comment here about that though?
Martin
PATCH v2 4/4 ARM: dts: i.MX53: Add PMU DT entry
Quoted 22 lines · Martin Fuzzey
Quoted 8 lines · Will Deacon
Quoted 7 lines · 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(+)Quoted 12 lines
arch/arm/boot/dts/imx53.dtsi line 106
Quoted 11 lines
arch/arm/boot/dts/imx53.dtsi +4
-0@@ -101,6 +101,11 @@ interrupt-parent = <&tzic>; ranges; + pmu { + compatible = "arm,cortex-a8-pmu"; + interrupts = <77>; + };You're missing the new property.
Quoted 4 lines
Actually this was deliberate.
Since the property should only be used if booting in secure mode this generic dtsi file can't know if it is ok or not.
As far as I know, most of the i.MX53 systems boot Linux kernel in secure mode. In that case, we should probably have the new property by default?
Shawn
Quoted 6 lines
I presume a (probably out of tree) board / application dts file will add the property.
Maybe I should put a comment here about that though?
Martin
PATCH v2 4/4 ARM: dts: i.MX53: Add PMU DT entry
Quoted 11 lines · Shawn Guo
Quoted 7 lines · Martin Fuzzey
Quoted 2 lines · Will Deacon
You're missing the new property.
Actually this was deliberate.
Since the property should only be used if booting in secure mode this generic dtsi file can't know if it is ok or not.
As far as I know, most of the i.MX53 systems boot Linux kernel in secure mode. In that case, we should probably have the new property by default?
Is it possible to override a boolean property to remove it (set to false) in a board specific dts?
If it can be done then yes I agree it should be the default.
However seeing as the dts syntax doesn't give a value for boolean properties I don't see how.
But I don't think people booting non secure should have to modify imx53.dtsi
Martin
PATCH v2 4/4 ARM: dts: i.MX53: Add PMU DT entry
Quoted 16 lines · Martin Fuzzey
Quoted 11 lines · Shawn Guo
Quoted 7 lines · Martin Fuzzey
Quoted 2 lines · Will Deacon
You're missing the new property.
Actually this was deliberate.
Since the property should only be used if booting in secure mode this generic dtsi file can't know if it is ok or not.
As far as I know, most of the i.MX53 systems boot Linux kernel in secure mode. In that case, we should probably have the new property by default?
Is it possible to override a boolean property to remove it (set to false) in a board specific dts?
Quoted 6 lines
If it can be done then yes I agree it should be the default.
However seeing as the dts syntax doesn't give a value for boolean properties I don't see how.
But I don't think people booting non secure should have to modify
imx53.dtsi
Ah, yes. I missed the fact that the property is a boolean.
Shawn