From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932178AbcKHTVv (ORCPT ); Tue, 8 Nov 2016 14:21:51 -0500 Received: from mail-qt0-f193.google.com ([209.85.216.193]:32991 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751800AbcKHTVt (ORCPT ); Tue, 8 Nov 2016 14:21:49 -0500 MIME-Version: 1.0 In-Reply-To: References: <1474307267-3081-1-git-send-email-Frank.Li@nxp.com> From: Zhi Li Date: Tue, 8 Nov 2016 13:21:47 -0600 Message-ID: Subject: Re: [PATCH 1/1 v7] ARM: imx: Added perf functionality to mmdc driver To: Paul Gortmaker Cc: Frank Li , Shawn Guo , "linux-arm-kernel@lists.infradead.org" , LKML , Peter Zijlstra , Ingo Molnar , acme@kernel.org, alexander.shishkin@linux.intel.com, Mark Rutland , jerry shen , 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 Tue, Nov 8, 2016 at 1:00 PM, Paul Gortmaker wrote: > 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? I will do follow-up fixup patch. I think pmu_pmu_poll_period_us should be removed because no benefit to change it. I can delete .remove best regards Frank Li > > Thanks > Paul. > -- > >> + >> +static ktime_t mmdc_pmu_timer_period(void) >> +{ >> + return ns_to_ktime((u64)mmdc_pmu_poll_period_us * 1000); >> +} >> +