Page MenuHomeFreeBSD

Add PMC support for AArch64
ClosedPublic

Authored by br on May 15 2015, 2:53 PM.

Details

Summary

Cortex A53/57 (juno board) provides six 32bit counters

Test Plan

tested on Juno board

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

br updated this revision to Diff 5408.May 15 2015, 2:53 PM
br retitled this revision from to Add PMC support for AArch64.
br updated this object.
br edited the test plan for this revision. (Show Details)
br added a reviewer: ARM.
br updated this object.May 15 2015, 2:56 PM
br added a reviewer: pmc.
emaste added a subscriber: zbb.May 15 2015, 4:35 PM
gnn accepted this revision.May 15 2015, 4:44 PM
gnn added a reviewer: gnn.
This revision is now accepted and ready to land.May 15 2015, 4:44 PM
bz added a subscriber: bz.May 15 2015, 5:33 PM

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 ↗(On Diff #5408)

Talk to Andy about these; I think it should be an arm version check there these days.

382 ↗(On Diff #5408)

Yeah the #if checks here need changing. And all the way down. Andy!

sys/sys/pmc.h
109 ↗(On Diff #5408)

The list is sorted differently. I was wondering if we want some "sparser" allocation scheme...

andrew added a subscriber: andrew.May 16 2015, 1:07 PM
andrew added inline comments.
lib/libpmc/libpmc.c
382 ↗(On Diff #5408)

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
130 ↗(On Diff #5408)

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.

br updated this revision to Diff 5450.May 18 2015, 10:55 AM
br edited edge metadata.
This revision now requires review to proceed.May 18 2015, 10:55 AM
br updated this revision to Diff 5451.May 18 2015, 10:55 AM
br edited edge metadata.
br marked 3 inline comments as done.May 18 2015, 10:58 AM
br added inline comments.
sys/arm/arm/pmu.c
130 ↗(On Diff #5408)

sounds reasonable, so I rewrote this

sys/dev/hwpmc/hwpmc_armv8.c
58 ↗(On Diff #5408)

PMINTENSET is a 32 bit register, so why not ?

br marked 2 inline comments as done.May 18 2015, 11:05 AM
br added inline comments.
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

br updated this revision to Diff 5452.May 18 2015, 12:49 PM
br marked an inline comment as done.

add more isb()

br updated this revision to Diff 5454.May 18 2015, 1:11 PM

special registers moved to sys/arm64/include/armreg.h

br marked 2 inline comments as done.May 18 2015, 1:13 PM
br marked an inline comment as done.May 18 2015, 1:13 PM
emaste added inline comments.May 18 2015, 3:14 PM
lib/libpmc/libpmc.c
85 ↗(On Diff #5454)

@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 ↗(On Diff #5454)

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.

andrew added inline comments.May 18 2015, 3:21 PM
lib/libpmc/libpmc.c
85 ↗(On Diff #5454)

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

br updated this revision to Diff 5457.May 18 2015, 4:27 PM

armv8_* functions renamed to arm64_*
PMC_CPU list grouped by processor family

br marked 3 inline comments as done.May 18 2015, 4:28 PM
br marked an inline comment as done.May 18 2015, 4:31 PM

Mostly style issues left.

sys/dev/hwpmc/hwpmc_arm64.c
181 ↗(On Diff #5457)

What's magic about 0xff?

229 ↗(On Diff #5457)

Why no spaces?

345 ↗(On Diff #5457)

How large could arm64_npmcs be? It may overflow an int if it's too large.

404 ↗(On Diff #5457)

return (0);. And the same below.

439 ↗(On Diff #5457)

Missing space

442 ↗(On Diff #5457)

And here

483 ↗(On Diff #5457)

These should be named PMCR_N_SHIFT and PMCR_N_MASK, and used as follows.

arm64_npmcs = (reg & PMCR_N_MASK) >> PMCR_N_SHIFT;
484 ↗(On Diff #5457)

And here

br marked 8 inline comments as done.May 19 2015, 1:45 PM
br added inline comments.
sys/dev/hwpmc/hwpmc_arm64.c
181 ↗(On Diff #5457)

it is EVENT ID mask. fixed

345 ↗(On Diff #5457)

no more than 31 counters can be implemented:

#define PMCR_N_SHIFT 11 /* Number of counters implemented */
#define PMCR_N_MASK (0x1f << PMCR_N_SHIFT)

br updated this revision to Diff 5473.May 19 2015, 1:45 PM
br marked 2 inline comments as done.
andrew accepted this revision.May 19 2015, 2:04 PM
andrew added a reviewer: andrew.
andrew added inline comments.
sys/sys/pmc.h
108 ↗(On Diff #5473)

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.

This revision is now accepted and ready to land.May 19 2015, 2:04 PM
jhb added a subscriber: jhb.May 19 2015, 3:00 PM
jhb added inline comments.
sys/sys/pmc.h
138 ↗(On Diff #5473)

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.

br added inline comments.May 19 2015, 3:11 PM
sys/sys/pmc.h
138 ↗(On Diff #5473)

that is true, all the classes below will require recompile libpmc

This revision was automatically updated to reflect the committed changes.