Cortex A53/57 (juno board) provides six 32bit counters
Details
- Reviewers
andrew gnn - Group Reviewers
ARM pmc - Commits
- rS283112: Add Performance Monitoring Counters support for AArch64.
tested on Juno board
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Check the #if checks with Andy; I have not looked at the logic yet but the separation sounds good to me.
lib/libpmc/libpmc.c | ||
---|---|---|
85 | Talk to Andy about these; I think it should be an arm version check there these days. | |
382 | Yeah the #if checks here need changing. And all the way down. Andy! | |
sys/sys/pmc.h | ||
109 | The list is sorted differently. I was wondering if we want some "sparser" allocation scheme... |
lib/libpmc/libpmc.c | ||
---|---|---|
382 | It depends on if we are checking for 32 vs 64-bit ARM, or for the version of the architecture within 32 or 64-bit. | |
sys/arm/arm/pmu.c | ||
142 | It should be the parent that sets the interrupt resources, not the device. This will also be problematic with ACPI. | |
sys/dev/hwpmc/hwpmc_armv8.c | ||
1 ↗ | (On Diff #5408) | How similar is this to the ARMv7 code? It also doesn't appear to be ARMv8 specific, but arm64 specific as it used 64-bit special register accesses. |
58 ↗ | (On Diff #5408) | Does this need to be a uint32_t? |
63 ↗ | (On Diff #5408) | If after calling WRITE_SPECIALREG you rely on the write to have completed you will need to add an isb(). |
sys/dev/hwpmc/hwpmc_armv8.h | ||
38 ↗ | (On Diff #5408) | In general I've been putting special registers macros in sys/arm64/include/armreg.h. |
sys/dev/hwpmc/hwpmc_armv8_md.c | ||
1 ↗ | (On Diff #5408) | This file looks to be arm64 specific, it should be named something like hwpmc_arm64_md.c. |
sys/dev/hwpmc/hwpmc_armv8.c | ||
---|---|---|
1 ↗ | (On Diff #5408) | mostly the same, except using 64-bit special register access and also armv7 has no 'cpu id specific' counters. I plan to implement specific counters to armv7, so maybe this code can be merged later |
lib/libpmc/libpmc.c | ||
---|---|---|
85 | @bz, do you mean something akin to __armv8__? __aarch64__ should sort before __arm__, but i386 above breaks an attempt to sort the full list properly. I'd still put it above. | |
sys/sys/pmc.h | ||
109 | Yeah, the sorting here is rather unfortunate. I'd suggest we rearrange the list and group by processor family, with comments between. Having ARMV7 up near the top with 0x500 is confusing. |
lib/libpmc/libpmc.c | ||
---|---|---|
85 | __armv8__ would be spelt __ARM_ARCH == 8, however this is not armv8 code, it's for arm64 so the check is correct, however the functions name should be something like arm64_allocate_pmc. |
Mostly style issues left.
sys/dev/hwpmc/hwpmc_arm64.c | ||
---|---|---|
182 | What's magic about 0xff? | |
230 | Why no spaces? | |
346 | How large could arm64_npmcs be? It may overflow an int if it's too large. | |
405 | return (0);. And the same below. | |
440 | Missing space | |
443 | And here | |
484 | These should be named PMCR_N_SHIFT and PMCR_N_MASK, and used as follows. arm64_npmcs = (reg & PMCR_N_MASK) >> PMCR_N_SHIFT; | |
485 | And here |
sys/sys/pmc.h | ||
---|---|---|
109 | I don't think it makes sence to guess what ARM will name their next design. I don't think it will be an A5x given they have jumped to Cortex-A72. |
sys/sys/pmc.h | ||
---|---|---|
138 | Note that this breaks the ABI since these enums get implicit values rather than explicit ones like __PMC_CPU() does. I would really like us to fix this to also use explicit values, but that is outside the scope of this change. |
sys/sys/pmc.h | ||
---|---|---|
138 | that is true, all the classes below will require recompile libpmc |