From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933192AbcKHTAo (ORCPT ); Tue, 8 Nov 2016 14:00:44 -0500 Received: from mail-vk0-f66.google.com ([209.85.213.66]:34786 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751427AbcKHTAj (ORCPT ); Tue, 8 Nov 2016 14:00:39 -0500 MIME-Version: 1.0 In-Reply-To: <1474307267-3081-1-git-send-email-Frank.Li@nxp.com> References: <1474307267-3081-1-git-send-email-Frank.Li@nxp.com> From: Paul Gortmaker Date: Tue, 8 Nov 2016 14:00:07 -0500 X-Google-Sender-Auth: ebvezSQGhCxgDX1lSPvA-H_YK2A Message-ID: Subject: Re: [PATCH 1/1 v7] ARM: imx: Added perf functionality to mmdc driver To: Frank Li Cc: shawnguo@kernel.org, "linux-arm-kernel@lists.infradead.org" , LKML , Peter Zijlstra , Ingo Molnar , acme@kernel.org, alexander.shishkin@linux.intel.com, lznuaa@gmail.com, mark.rutland@arm.com, jerry.zhengyu.shen@gmail.com, Zhengyu Shen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 19, 2016 at 1:47 PM, Frank Li wrote: > From: Zhengyu Shen > > MMDC is a multi-mode DDR controller that supports DDR3/DDR3L x16/x32/x64 > and LPDDR2 two channel x16/x32 memory types. MMDC is configurable, high > performance, and optimized. MMDC is present on i.MX6 Quad and i.MX6 > QuadPlus devices, but this driver only supports i.MX6 Quad at the moment. > MMDC provides registers for performance counters which read via this > driver to help debug memory throughput and similar issues. > > $ perf stat -a -e mmdc/busy-cycles/,mmdc/read-accesses/,mmdc/read-bytes/,mmdc/total-cycles/,mmdc/write-accesses/,mmdc/write-bytes/ dd if=/dev/zero of=/dev/null bs=1M count=5000 > Performance counter stats for 'dd if=/dev/zero of=/dev/null bs=1M count=5000': > > 898021787 mmdc/busy-cycles/ > 14819600 mmdc/read-accesses/ > 471.30 MB mmdc/read-bytes/ > 2815419216 mmdc/total-cycles/ > 13367354 mmdc/write-accesses/ > 427.76 MB mmdc/write-bytes/ > > 5.334757334 seconds time elapsed > > Signed-off-by: Zhengyu Shen > Signed-off-by: Frank Li > --- > Changes from v6 to v7 > use mmdc_pmu prefix > remove unnecessary check > improve group event check according to mark's feedback. > check pmu_mmdc->mmdc_events[cfg] at event_add > only check == 0 at event_del > > Changes from v5 to v6 > Improve group event error handle > > Changes from v4 to v5 > Remove mmdc_pmu:irq > remove static variable cpuhp_mmdc_pmu > remove spin_lock > check is_sampling_event(event) > remove unnecessary cast > use hw_perf_event::prev_count > > Changes from v3 to v4: > Tested and fixed crash relating to removing events with perf fuzzer > Adjusted formatting > Moved all perf event code under CONFIG_PERF_EVENTS > Switched cpuhp_setup_state to cpuhp_setup_state_nocalls > > Changes from v2 to v3: > Use WARN_ONCE instead of returning generic error values > Replace CPU Notifiers with newer state machine hotplug > Added additional checks on event_init for grouping and sampling > Remove useless mmdc_enable_profiling function > Added comments > Moved start index of events from 0x01 to 0x00 > Added a counter to pmu_mmdc to only stop hrtimer after all events are finished > Replace readl_relaxed and writel_relaxed with readl and writel > Removed duplicate update function > Used devm_kasprintf when naming mmdcs probed > > Changes from v1 to v2: > Added cpumask and migration handling support to driver > Validated event during event_init > Added code to properly stop counters > Used perf_invalid_context instead of perf_sw_context > Added hrtimer to poll for overflow > Added better description > Added support for multiple mmdcs > > arch/arm/mach-imx/mmdc.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 457 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c > index db9621c..1f70376 100644 > --- a/arch/arm/mach-imx/mmdc.c > +++ b/arch/arm/mach-imx/mmdc.c > @@ -1,5 +1,5 @@ > /* > - * Copyright 2011 Freescale Semiconductor, Inc. > + * Copyright 2011,2016 Freescale Semiconductor, Inc. > * Copyright 2011 Linaro Ltd. > * > * The code contained herein is licensed under the GNU General Public > @@ -10,12 +10,16 @@ > * http://www.gnu.org/copyleft/gpl.html > */ > > +#include > #include > +#include > #include > #include > #include > #include > #include > +#include > +#include > > #include "common.h" > > @@ -27,8 +31,458 @@ > #define BM_MMDC_MDMISC_DDR_TYPE 0x18 > #define BP_MMDC_MDMISC_DDR_TYPE 0x3 > > +#define TOTAL_CYCLES 0x0 > +#define BUSY_CYCLES 0x1 > +#define READ_ACCESSES 0x2 > +#define WRITE_ACCESSES 0x3 > +#define READ_BYTES 0x4 > +#define WRITE_BYTES 0x5 > + > +/* Enables, resets, freezes, overflow profiling*/ > +#define DBG_DIS 0x0 > +#define DBG_EN 0x1 > +#define DBG_RST 0x2 > +#define PRF_FRZ 0x4 > +#define CYC_OVF 0x8 > + > +#define MMDC_MADPCR0 0x410 > +#define MMDC_MADPSR0 0x418 > +#define MMDC_MADPSR1 0x41C > +#define MMDC_MADPSR2 0x420 > +#define MMDC_MADPSR3 0x424 > +#define MMDC_MADPSR4 0x428 > +#define MMDC_MADPSR5 0x42C > + > +#define MMDC_NUM_COUNTERS 6 > + > +#define to_mmdc_pmu(p) container_of(p, struct mmdc_pmu, pmu) > + > static int ddr_type; > > +#ifdef CONFIG_PERF_EVENTS > + > +static DEFINE_IDA(mmdc_ida); > + > +PMU_EVENT_ATTR_STRING(total-cycles, mmdc_pmu_total_cycles, "event=0x00") > +PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_pmu_busy_cycles, "event=0x01") > +PMU_EVENT_ATTR_STRING(read-accesses, mmdc_pmu_read_accesses, "event=0x02") > +PMU_EVENT_ATTR_STRING(write-accesses, mmdc_pmu_write_accesses, "config=0x03") > +PMU_EVENT_ATTR_STRING(read-bytes, mmdc_pmu_read_bytes, "event=0x04") > +PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_pmu_read_bytes_unit, "MB"); > +PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_pmu_read_bytes_scale, "0.000001"); > +PMU_EVENT_ATTR_STRING(write-bytes, mmdc_pmu_write_bytes, "event=0x05") > +PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_pmu_write_bytes_unit, "MB"); > +PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_pmu_write_bytes_scale, "0.000001"); > + > +struct mmdc_pmu { > + struct pmu pmu; > + void __iomem *mmdc_base; > + cpumask_t cpu; > + struct hrtimer hrtimer; > + unsigned int active_events; > + struct device *dev; > + struct perf_event *mmdc_events[MMDC_NUM_COUNTERS]; > + struct hlist_node node; > +}; > + > +/* > + * Polling period is set to one second, overflow of total-cycles (the fastest > + * increasing counter) takes ten seconds so one second is safe > + */ > +static unsigned int mmdc_pmu_poll_period_us = 1000000; > + > +module_param_named(pmu_pmu_poll_period_us, mmdc_pmu_poll_period_us, uint, > + S_IRUGO | S_IWUSR); I just noticed this commit now that linux-next is back after the week off. This should probably use core_param or setup_param since the Kconfig for this is bool and not tristate. Similarly, that means that the .remove function you've added is dead code -- unless you envision a case where the user needs to forcibly unbind the driver... Do you want to redo the existing commit or do you want me to submit a follow-up fixup patch? Thanks Paul. -- > + > +static ktime_t mmdc_pmu_timer_period(void) > +{ > + return ns_to_ktime((u64)mmdc_pmu_poll_period_us * 1000); > +} > +