Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755599AbbBLMu1 (ORCPT ); Thu, 12 Feb 2015 07:50:27 -0500 Received: from foss-mx-na.arm.com ([217.140.108.86]:43402 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752766AbbBLMuZ (ORCPT ); Thu, 12 Feb 2015 07:50:25 -0500 Date: Thu, 12 Feb 2015 12:49:49 +0000 From: Mark Rutland To: Stephen Boyd Cc: Will Deacon , "linux-kernel@vger.kernel.org" , "linux-arm-msm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Neil Leeder , Ashwin Chaugule , "devicetree@vger.kernel.org" Subject: Re: [PATCH 2/2] ARM: perf: Add support for Scorpion PMUs Message-ID: <20150212124949.GE1522@leverpostej> References: <1423616724-20849-1-git-send-email-sboyd@codeaurora.org> <1423616724-20849-3-git-send-email-sboyd@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1423616724-20849-3-git-send-email-sboyd@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4885 Lines: 104 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 > Cc: Ashwin Chaugule > Cc: > Signed-off-by: Stephen Boyd > --- > 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. [...] > +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. > + /* > + * 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)? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/