Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752452AbcJJURk (ORCPT ); Mon, 10 Oct 2016 16:17:40 -0400 Received: from mail-qk0-f195.google.com ([209.85.220.195]:33257 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752003AbcJJURj (ORCPT ); Mon, 10 Oct 2016 16:17:39 -0400 MIME-Version: 1.0 In-Reply-To: References: <1474307849-7341-1-git-send-email-Frank.Li@nxp.com> From: Zhi Li Date: Mon, 10 Oct 2016 15:17:36 -0500 Message-ID: Subject: Re: [PATCH 1/1 v8] ARM: imx: Added perf functionality to mmdc driver To: Frank Li Cc: Shawn Guo , "linux-arm-kernel@lists.infradead.org" , kernel list , peterz@infradead.org, mingo@redhat.com, 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 Content-Length: 20816 Lines: 596 On Mon, Sep 26, 2016 at 11:40 AM, Zhi Li wrote: > On Mon, Sep 19, 2016 at 12:57 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 >> --- > > Mark: > Any additional comments for this version? > Shawn: No any new comment for more than 2 weeks. Did you plan accept this patch? > best regards > Frank Li > >> Changes from v7 to v8 >> fix a group event check problem, should be slibling. >> >> 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..d82d14c 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); >> + >> +static ktime_t mmdc_pmu_timer_period(void) >> +{ >> + return ns_to_ktime((u64)mmdc_pmu_poll_period_us * 1000); >> +} >> + >> +static ssize_t mmdc_pmu_cpumask_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct mmdc_pmu *pmu_mmdc = dev_get_drvdata(dev); >> + >> + return cpumap_print_to_pagebuf(true, buf, &pmu_mmdc->cpu); >> +} >> + >> +static struct device_attribute mmdc_pmu_cpumask_attr = >> + __ATTR(cpumask, S_IRUGO, mmdc_pmu_cpumask_show, NULL); >> + >> +static struct attribute *mmdc_pmu_cpumask_attrs[] = { >> + &mmdc_pmu_cpumask_attr.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group mmdc_pmu_cpumask_attr_group = { >> + .attrs = mmdc_pmu_cpumask_attrs, >> +}; >> + >> +static struct attribute *mmdc_pmu_events_attrs[] = { >> + &mmdc_pmu_total_cycles.attr.attr, >> + &mmdc_pmu_busy_cycles.attr.attr, >> + &mmdc_pmu_read_accesses.attr.attr, >> + &mmdc_pmu_write_accesses.attr.attr, >> + &mmdc_pmu_read_bytes.attr.attr, >> + &mmdc_pmu_read_bytes_unit.attr.attr, >> + &mmdc_pmu_read_bytes_scale.attr.attr, >> + &mmdc_pmu_write_bytes.attr.attr, >> + &mmdc_pmu_write_bytes_unit.attr.attr, >> + &mmdc_pmu_write_bytes_scale.attr.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group mmdc_pmu_events_attr_group = { >> + .name = "events", >> + .attrs = mmdc_pmu_events_attrs, >> +}; >> + >> +PMU_FORMAT_ATTR(event, "config:0-63"); >> +static struct attribute *mmdc_pmu_format_attrs[] = { >> + &format_attr_event.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group mmdc_pmu_format_attr_group = { >> + .name = "format", >> + .attrs = mmdc_pmu_format_attrs, >> +}; >> + >> +static const struct attribute_group *attr_groups[] = { >> + &mmdc_pmu_events_attr_group, >> + &mmdc_pmu_format_attr_group, >> + &mmdc_pmu_cpumask_attr_group, >> + NULL, >> +}; >> + >> +static u32 mmdc_pmu_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg) >> +{ >> + void __iomem *mmdc_base, *reg; >> + >> + mmdc_base = pmu_mmdc->mmdc_base; >> + >> + switch (cfg) { >> + case TOTAL_CYCLES: >> + reg = mmdc_base + MMDC_MADPSR0; >> + break; >> + case BUSY_CYCLES: >> + reg = mmdc_base + MMDC_MADPSR1; >> + break; >> + case READ_ACCESSES: >> + reg = mmdc_base + MMDC_MADPSR2; >> + break; >> + case WRITE_ACCESSES: >> + reg = mmdc_base + MMDC_MADPSR3; >> + break; >> + case READ_BYTES: >> + reg = mmdc_base + MMDC_MADPSR4; >> + break; >> + case WRITE_BYTES: >> + reg = mmdc_base + MMDC_MADPSR5; >> + break; >> + default: >> + return WARN_ONCE(1, >> + "invalid configuration %d for mmdc counter", cfg); >> + } >> + return readl(reg); >> +} >> + >> +static int mmdc_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node) >> +{ >> + struct mmdc_pmu *pmu_mmdc = hlist_entry_safe(node, struct mmdc_pmu, node); >> + int target; >> + >> + if (!cpumask_test_and_clear_cpu(cpu, &pmu_mmdc->cpu)) >> + return 0; >> + >> + target = cpumask_any_but(cpu_online_mask, cpu); >> + if (target >= nr_cpu_ids) >> + return 0; >> + >> + perf_pmu_migrate_context(&pmu_mmdc->pmu, cpu, target); >> + cpumask_set_cpu(target, &pmu_mmdc->cpu); >> + >> + return 0; >> +} >> + >> +static bool mmdc_pmu_group_event_is_valid(struct perf_event *event, >> + struct pmu *pmu, >> + unsigned long *used_counters) >> +{ >> + int cfg = event->attr.config; >> + >> + if (is_software_event(event)) >> + return true; >> + >> + if (event->pmu != pmu) >> + return false; >> + >> + return !test_and_set_bit(cfg, used_counters); >> +} >> + >> +/* >> + * Each event has a single fixed-purpose counter, so we can only have a >> + * single active event for each at any point in time. Here we just check >> + * for duplicates, and rely on mmdc_pmu_event_init to verify that the HW >> + * event numbers are valid. >> + */ >> +static bool mmdc_pmu_group_is_valid(struct perf_event *event) >> +{ >> + struct pmu *pmu = event->pmu; >> + struct perf_event *leader = event->group_leader; >> + struct perf_event *sibling; >> + unsigned long counter_mask = 0; >> + >> + set_bit(leader->attr.config, &counter_mask); >> + >> + if (event != leader) { >> + if (!mmdc_pmu_group_event_is_valid(event, pmu, &counter_mask)) >> + return false; >> + } >> + >> + list_for_each_entry(sibling, &leader->sibling_list, group_entry) { >> + if (!mmdc_pmu_group_event_is_valid(sibling, pmu, &counter_mask)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static int mmdc_pmu_event_init(struct perf_event *event) >> +{ >> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu); >> + int cfg = event->attr.config; >> + >> + if (event->attr.type != event->pmu->type) >> + return -ENOENT; >> + >> + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK) >> + return -EOPNOTSUPP; >> + >> + if (event->cpu < 0) { >> + dev_warn(pmu_mmdc->dev, "Can't provide per-task data!\n"); >> + return -EOPNOTSUPP; >> + } >> + >> + if (event->attr.exclude_user || >> + event->attr.exclude_kernel || >> + event->attr.exclude_hv || >> + event->attr.exclude_idle || >> + event->attr.exclude_host || >> + event->attr.exclude_guest || >> + event->attr.sample_period) >> + return -EINVAL; >> + >> + if (cfg < 0 || cfg >= MMDC_NUM_COUNTERS) >> + return -EINVAL; >> + >> + if (!mmdc_pmu_group_is_valid(event)) >> + return -EINVAL; >> + >> + event->cpu = cpumask_first(&pmu_mmdc->cpu); >> + return 0; >> +} >> + >> +static void mmdc_pmu_event_update(struct perf_event *event) >> +{ >> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu); >> + struct hw_perf_event *hwc = &event->hw; >> + u64 delta, prev_raw_count, new_raw_count; >> + >> + do { >> + prev_raw_count = local64_read(&hwc->prev_count); >> + new_raw_count = mmdc_pmu_read_counter(pmu_mmdc, >> + event->attr.config); >> + } while (local64_cmpxchg(&hwc->prev_count, prev_raw_count, >> + new_raw_count) != prev_raw_count); >> + >> + delta = (new_raw_count - prev_raw_count) & 0xFFFFFFFF; >> + >> + local64_add(delta, &event->count); >> +} >> + >> +static void mmdc_pmu_event_start(struct perf_event *event, int flags) >> +{ >> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu); >> + struct hw_perf_event *hwc = &event->hw; >> + void __iomem *mmdc_base, *reg; >> + >> + mmdc_base = pmu_mmdc->mmdc_base; >> + reg = mmdc_base + MMDC_MADPCR0; >> + >> + /* >> + * hrtimer is required because mmdc does not provide an interrupt so >> + * polling is necessary >> + */ >> + hrtimer_start(&pmu_mmdc->hrtimer, mmdc_pmu_timer_period(), >> + HRTIMER_MODE_REL_PINNED); >> + >> + local64_set(&hwc->prev_count, 0); >> + >> + writel(DBG_RST, reg); >> + writel(DBG_EN, reg); >> +} >> + >> +static int mmdc_pmu_event_add(struct perf_event *event, int flags) >> +{ >> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu); >> + struct hw_perf_event *hwc = &event->hw; >> + >> + int cfg = event->attr.config; >> + >> + if (flags & PERF_EF_START) >> + mmdc_pmu_event_start(event, flags); >> + >> + if (pmu_mmdc->mmdc_events[cfg] != NULL) >> + return -EAGAIN; >> + >> + pmu_mmdc->mmdc_events[cfg] = event; >> + pmu_mmdc->active_events++; >> + >> + local64_set(&hwc->prev_count, mmdc_pmu_read_counter(pmu_mmdc, cfg)); >> + >> + return 0; >> +} >> + >> +static void mmdc_pmu_event_stop(struct perf_event *event, int flags) >> +{ >> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu); >> + void __iomem *mmdc_base, *reg; >> + >> + mmdc_base = pmu_mmdc->mmdc_base; >> + reg = mmdc_base + MMDC_MADPCR0; >> + >> + writel(PRF_FRZ, reg); >> + mmdc_pmu_event_update(event); >> +} >> + >> +static void mmdc_pmu_event_del(struct perf_event *event, int flags) >> +{ >> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu); >> + int cfg = event->attr.config; >> + >> + pmu_mmdc->mmdc_events[cfg] = NULL; >> + pmu_mmdc->active_events--; >> + >> + if (pmu_mmdc->active_events == 0) >> + hrtimer_cancel(&pmu_mmdc->hrtimer); >> + >> + mmdc_pmu_event_stop(event, PERF_EF_UPDATE); >> +} >> + >> +static void mmdc_pmu_overflow_handler(struct mmdc_pmu *pmu_mmdc) >> +{ >> + int i; >> + >> + for (i = 0; i < MMDC_NUM_COUNTERS; i++) { >> + struct perf_event *event = pmu_mmdc->mmdc_events[i]; >> + >> + if (event) >> + mmdc_pmu_event_update(event); >> + } >> +} >> + >> +static enum hrtimer_restart mmdc_pmu_timer_handler(struct hrtimer *hrtimer) >> +{ >> + struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu, >> + hrtimer); >> + >> + mmdc_pmu_overflow_handler(pmu_mmdc); >> + hrtimer_forward_now(hrtimer, mmdc_pmu_timer_period()); >> + >> + return HRTIMER_RESTART; >> +} >> + >> +static int mmdc_pmu_init(struct mmdc_pmu *pmu_mmdc, >> + void __iomem *mmdc_base, struct device *dev) >> +{ >> + int mmdc_num; >> + >> + *pmu_mmdc = (struct mmdc_pmu) { >> + .pmu = (struct pmu) { >> + .task_ctx_nr = perf_invalid_context, >> + .attr_groups = attr_groups, >> + .event_init = mmdc_pmu_event_init, >> + .add = mmdc_pmu_event_add, >> + .del = mmdc_pmu_event_del, >> + .start = mmdc_pmu_event_start, >> + .stop = mmdc_pmu_event_stop, >> + .read = mmdc_pmu_event_update, >> + }, >> + .mmdc_base = mmdc_base, >> + .dev = dev, >> + .active_events = 0, >> + }; >> + >> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL); >> + >> + return mmdc_num; >> +} >> + >> +static int imx_mmdc_remove(struct platform_device *pdev) >> +{ >> + struct mmdc_pmu *pmu_mmdc = platform_get_drvdata(pdev); >> + >> + perf_pmu_unregister(&pmu_mmdc->pmu); >> + cpuhp_remove_state_nocalls(CPUHP_ONLINE); >> + kfree(pmu_mmdc); >> + return 0; >> +} >> + >> +static int imx_mmdc_perf_init(struct platform_device *pdev, void __iomem *mmdc_base) >> +{ >> + struct mmdc_pmu *pmu_mmdc; >> + char *name; >> + int mmdc_num; >> + int ret; >> + >> + pmu_mmdc = kzalloc(sizeof(*pmu_mmdc), GFP_KERNEL); >> + if (!pmu_mmdc) { >> + pr_err("failed to allocate PMU device!\n"); >> + return -ENOMEM; >> + } >> + >> + mmdc_num = mmdc_pmu_init(pmu_mmdc, mmdc_base, &pdev->dev); >> + if (mmdc_num == 0) >> + name = "mmdc"; >> + else >> + name = devm_kasprintf(&pdev->dev, >> + GFP_KERNEL, "mmdc%d", mmdc_num); >> + >> + hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC, >> + HRTIMER_MODE_REL); >> + pmu_mmdc->hrtimer.function = mmdc_pmu_timer_handler; >> + >> + cpuhp_state_add_instance_nocalls(CPUHP_ONLINE, >> + &pmu_mmdc->node); >> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu); >> + ret = cpuhp_setup_state_multi(CPUHP_AP_NOTIFY_ONLINE, >> + "MMDC_ONLINE", NULL, >> + mmdc_pmu_offline_cpu); >> + if (ret) { >> + pr_err("cpuhp_setup_state_multi failure\n"); >> + goto pmu_register_err; >> + } >> + >> + ret = perf_pmu_register(&(pmu_mmdc->pmu), name, -1); >> + platform_set_drvdata(pdev, pmu_mmdc); >> + if (ret) >> + goto pmu_register_err; >> + return 0; >> + >> +pmu_register_err: >> + pr_warn("MMDC Perf PMU failed (%d), disabled\n", ret); >> + hrtimer_cancel(&pmu_mmdc->hrtimer); >> + kfree(pmu_mmdc); >> + return ret; >> +} >> + >> +#else >> +#define imx_mmdc_remove NULL >> +#define imx_mmdc_perf_init(pdev, mmdc_base) 0 >> +#endif >> + >> static int imx_mmdc_probe(struct platform_device *pdev) >> { >> struct device_node *np = pdev->dev.of_node; >> @@ -62,7 +516,7 @@ static int imx_mmdc_probe(struct platform_device *pdev) >> return -EBUSY; >> } >> >> - return 0; >> + return imx_mmdc_perf_init(pdev, mmdc_base); >> } >> >> int imx_mmdc_get_ddr_type(void) >> @@ -81,6 +535,7 @@ static struct platform_driver imx_mmdc_driver = { >> .of_match_table = imx_mmdc_dt_ids, >> }, >> .probe = imx_mmdc_probe, >> + .remove = imx_mmdc_remove, >> }; >> >> static int __init imx_mmdc_init(void) >> -- >> 2.5.2 >>