On 02/12/15 04:49, Mark Rutland wrote: > Hi, > > I haven't given this a thorough review, but I spotted a couple of items > below. > > On Wed, Feb 11, 2015 at 01:05:24AM +0000, Stephen Boyd wrote: >> Scorpion supports a set of local performance monitor event >> selection registers (LPM) sitting behind a cp15 based interface >> that extend the architected PMU events to include Scorpion CPU >> and Venum VFP specific events. To use these events the user is >> expected to program the lpm register with the event code shifted >> into the group they care about and then point the PMNx event at >> that region+group combo by writing a LPMn_GROUPx event. Add >> support for this hardware. >> >> Note: the raw event number is a pure software construct that >> allows us to map the multi-dimensional number space of regions, >> groups, and event codes into a flat event number space suitable >> for use by the perf framework. >> >> This is based on code originally written by Ashwin Chaugule and >> Neil Leeder [1] massed to become similar to the Krait PMU support >> code. >> >> [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/kernel/perf_event_msm.c?h=msm-3.4 >> >> Cc: Neil Leeder <nleeder@xxxxxxxxxxxxxx> >> Cc: Ashwin Chaugule <ashwinc@xxxxxxxxxxxxxx> >> Cc: <devicetree@xxxxxxxxxxxxxxx> >> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx> >> --- >> Documentation/devicetree/bindings/arm/pmu.txt | 2 + >> arch/arm/kernel/perf_event_cpu.c | 2 + >> arch/arm/kernel/perf_event_v7.c | 395 ++++++++++++++++++++++++++ >> 3 files changed, 399 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt >> index 75ef91d08f3b..6e54a9d88b7a 100644 >> --- a/Documentation/devicetree/bindings/arm/pmu.txt >> +++ b/Documentation/devicetree/bindings/arm/pmu.txt >> @@ -18,6 +18,8 @@ Required properties: >> "arm,arm11mpcore-pmu" >> "arm,arm1176-pmu" >> "arm,arm1136-pmu" >> + "qcom,scorpion-pmu" >> + "qcom,scorpion-mp-pmu" > Is the PMU any different in the MP and !MP variants? The code doesn't > seem to handle the two any differently and will pass either to userspace > as "armv7_scorpion". > > If there is some difference that we don't handle right now, that's fine, > it just looks a little odd. It seems that on MP there are two event encodings on MP that aren't there on !MP and vice versa[1]. So I made two compatibles to reflect that. I'll make two names that go to userspace to clarify this. >> +static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX] >> + [PERF_COUNT_HW_CACHE_OP_MAX] >> + [PERF_COUNT_HW_CACHE_RESULT_MAX] = { >> + PERF_CACHE_MAP_ALL_UNSUPPORTED, >> + /* >> + * The performance counters don't differentiate between read and write >> + * accesses/misses so this isn't strictly correct, but it's the best we >> + * can do. Writes and reads get combined. >> + */ >> + [C(L1D)][C(OP_READ)][C(RESULT_ACCESS)] = ARMV7_PERFCTR_L1_DCACHE_ACCESS, >> + [C(L1D)][C(OP_READ)][C(RESULT_MISS)] = ARMV7_PERFCTR_L1_DCACHE_REFILL, >> + [C(L1D)][C(OP_WRITE)][C(RESULT_ACCESS)] = ARMV7_PERFCTR_L1_DCACHE_ACCESS, >> + [C(L1D)][C(OP_WRITE)][C(RESULT_MISS)] = ARMV7_PERFCTR_L1_DCACHE_REFILL, >> + [C(L1I)][C(OP_READ)][C(RESULT_ACCESS)] = SCORPION_ICACHE_ACCESS, >> + [C(L1I)][C(OP_READ)][C(RESULT_MISS)] = SCORPION_ICACHE_MISS, >> + [C(L1I)][C(OP_WRITE)][C(RESULT_ACCESS)] = SCORPION_ICACHE_ACCESS, >> + [C(L1I)][C(OP_WRITE)][C(RESULT_MISS)] = SCORPION_ICACHE_MISS, > These last two entries go against the policy we set in commit > 40c390c768f89849: "ARM: perf: don't pretend to support counting of L1I > writes", so I think they should be dropped. Fair enough. Thanks for the pointer. > >> + /* >> + * Only ITLB misses and DTLB refills are supported. If users want the >> + * DTLB refills misses a raw counter must be used. >> + */ >> + [C(DTLB)][C(OP_READ)][C(RESULT_ACCESS)] = SCORPION_DTLB_ACCESS, >> + [C(DTLB)][C(OP_READ)][C(RESULT_MISS)] = SCORPION_DTLB_MISS, >> + [C(DTLB)][C(OP_WRITE)][C(RESULT_ACCESS)] = SCORPION_DTLB_ACCESS, >> + [C(DTLB)][C(OP_WRITE)][C(RESULT_MISS)] = SCORPION_DTLB_MISS, >> + [C(ITLB)][C(OP_READ)][C(RESULT_MISS)] = SCORPION_ITLB_MISS, >> + [C(ITLB)][C(OP_WRITE)][C(RESULT_MISS)] = SCORPION_ITLB_MISS, >> + [C(BPU)][C(OP_READ)][C(RESULT_ACCESS)] = ARMV7_PERFCTR_PC_BRANCH_PRED, >> + [C(BPU)][C(OP_READ)][C(RESULT_MISS)] = ARMV7_PERFCTR_PC_BRANCH_PRED, >> + [C(BPU)][C(OP_WRITE)][C(RESULT_ACCESS)] = ARMV7_PERFCTR_PC_BRANCH_PRED, >> + [C(BPU)][C(OP_WRITE)][C(RESULT_MISS)] = ARMV7_PERFCTR_PC_BRANCH_PRED, >> +}; > Not ARMV7_PERFCTR_PC_BRANCH_MIS_PRED for the RESULT_MISS cases as with > all other ARMv7 instances (Krait included)? I was just copying the stuff from downstream. I think it's a bug that nobody noticed because the same problem was there on Krait and I fixed it before sending upstream. Thanks for catching it. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
