* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-17 19:42 ` Zhengyu Shen
0 siblings, 0 replies; 20+ messages in thread
From: Zhengyu Shen @ 2016-08-17 19:42 UTC (permalink / raw)
To: shawnguo
Cc: frank.li, linux-arm-kernel, linux-kernel, peterz, mingo, acme,
alexander.shishkin, lznuaa, mark.rutland, 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 <zhengyu.shen@nxp.com>
Signed-off-by: Frank Li <frank.li@nxp.com>
---
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 | 384 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 383 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
index db9621c..5fe7696 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
@@ -16,6 +16,11 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
+#include <linux/perf_event.h>
+#include <linux/slab.h>
+#include <linux/hrtimer.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
#include "common.h"
@@ -27,14 +32,363 @@
#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 DEFINE_IDA(mmdc_ida);
+
static int ddr_type;
+PMU_EVENT_ATTR_STRING(total-cycles, mmdc_total_cycles, "event=0x00")
+PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_busy_cycles, "event=0x01")
+PMU_EVENT_ATTR_STRING(read-accesses, mmdc_read_accesses, "event=0x02")
+PMU_EVENT_ATTR_STRING(write-accesses, mmdc_write_accesses, "config=0x03")
+PMU_EVENT_ATTR_STRING(read-bytes, mmdc_read_bytes, "event=0x04")
+PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_read_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_read_bytes_scale, "0.000001");
+PMU_EVENT_ATTR_STRING(write-bytes, mmdc_write_bytes, "event=0x05")
+PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_write_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_write_bytes_scale, "0.000001");
+
+struct mmdc_pmu {
+ struct pmu pmu;
+ void __iomem *mmdc_base;
+ cpumask_t cpu;
+ struct hrtimer hrtimer;
+ unsigned int irq;
+ unsigned int active_events;
+ struct device *dev;
+ struct perf_event *mmdc_events[MMDC_NUM_COUNTERS];
+ spinlock_t mmdc_active_events_lock;
+};
+static struct mmdc_pmu *cpuhp_mmdc_pmu;
+
+/* 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_poll_period_us = 1000000;
+module_param_named(pmu_poll_period_us, mmdc_poll_period_us, uint,
+ S_IRUGO | S_IWUSR);
+
+static ktime_t mmdc_timer_period(void)
+{
+ return ns_to_ktime((u64)mmdc_poll_period_us * 1000);
+}
+
+static ssize_t mmdc_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_cpumask_attr =
+__ATTR(cpumask, S_IRUGO, mmdc_cpumask_show, NULL);
+
+static struct attribute *mmdc_cpumask_attrs[] = {
+ &mmdc_cpumask_attr.attr,
+ NULL,
+};
+
+static struct attribute_group mmdc_cpumask_attr_group = {
+ .attrs = mmdc_cpumask_attrs,
+};
+
+static struct attribute *mmdc_events_attrs[] = {
+ &mmdc_total_cycles.attr.attr,
+ &mmdc_busy_cycles.attr.attr,
+ &mmdc_read_accesses.attr.attr,
+ &mmdc_write_accesses.attr.attr,
+ &mmdc_read_bytes.attr.attr,
+ &mmdc_read_bytes_unit.attr.attr,
+ &mmdc_read_bytes_scale.attr.attr,
+ &mmdc_write_bytes.attr.attr,
+ &mmdc_write_bytes_unit.attr.attr,
+ &mmdc_write_bytes_scale.attr.attr,
+ NULL,
+};
+
+static struct attribute_group mmdc_events_attr_group = {
+ .name = "events",
+ .attrs = mmdc_events_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-63");
+static struct attribute *mmdc_format_attrs[] = {
+ &format_attr_event.attr,
+ NULL,
+};
+
+static struct attribute_group mmdc_format_attr_group = {
+ .name = "format",
+ .attrs = mmdc_format_attrs,
+};
+
+static const struct attribute_group *attr_groups[] = {
+ &mmdc_events_attr_group,
+ &mmdc_format_attr_group,
+ &mmdc_cpumask_attr_group,
+ NULL,
+};
+
+static u32 mmdc_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg, u64 prev_val)
+{
+ u32 val;
+ 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);
+ }
+ val = readl(reg);
+ return val;
+}
+
+static int mmdc_pmu_offline_cpu(unsigned int cpu)
+{
+ struct mmdc_pmu *pmu_mmdc = cpuhp_mmdc_pmu;
+ 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);
+ if (pmu_mmdc->irq)
+ WARN_ON(irq_set_affinity_hint(
+ pmu_mmdc->irq, &pmu_mmdc->cpu) != 0);
+ return 0;
+}
+
+static int mmdc_event_init(struct perf_event *event)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ int cfg = event->attr.config;
+ struct perf_event *sibling;
+
+ if (event->attr.type != event->pmu->type)
+ return -ENOENT;
+
+ 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 (event->group_leader->pmu != event->pmu &&
+ !is_software_event(event->group_leader))
+ return -EINVAL;
+
+ list_for_each_entry(sibling, &event->group_leader->sibling_list,
+ group_entry)
+ if (sibling->pmu != event->pmu &&
+ !is_software_event(sibling))
+ return -EINVAL;
+
+ event->cpu = cpumask_first(&pmu_mmdc->cpu);
+ return 0;
+}
+
+static void mmdc_event_update(struct perf_event *event)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ u32 val;
+ u64 prev_val;
+
+ prev_val = local64_read(&event->count);
+ val = mmdc_read_counter(pmu_mmdc, (int) event->attr.config, prev_val);
+ local64_add(val - (u32)(prev_val & 0xFFFFFFFF), &event->count);
+}
+
+static void mmdc_event_start(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;
+ /* hrtimer is required because mmdc does not provide an interrupt so
+ * polling is necessary
+ */
+ hrtimer_start(&pmu_mmdc->hrtimer, mmdc_timer_period(),
+ HRTIMER_MODE_REL_PINNED);
+
+ writel(DBG_RST, reg);
+ writel(DBG_EN, reg);
+}
+
+static int mmdc_event_add(struct perf_event *event, int flags)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ int cfg = (int)event->attr.config;
+
+ if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
+ "invalid configuration %d for mmdc", cfg))
+ return -1;
+ pmu_mmdc->mmdc_events[cfg] = event;
+ local64_set(&event->count, 0);
+ if (flags & PERF_EF_START)
+ mmdc_event_start(event, flags);
+ spin_lock(&pmu_mmdc->mmdc_active_events_lock);
+ pmu_mmdc->active_events++;
+ spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
+ return 0;
+}
+
+static void mmdc_event_stop(struct perf_event *event, int flags)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ void __iomem *mmdc_base, *reg;
+ int cfg = (int)event->attr.config;
+
+ mmdc_base = pmu_mmdc->mmdc_base;
+ reg = mmdc_base + MMDC_MADPCR0;
+ if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
+ "invalid configuration %d for mmdc counter", cfg))
+ return;
+ spin_lock(&pmu_mmdc->mmdc_active_events_lock);
+ if (pmu_mmdc->active_events <= 0)
+ hrtimer_cancel(&pmu_mmdc->hrtimer);
+ spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
+ writel(PRF_FRZ, reg);
+ mmdc_event_update(event);
+}
+
+static void mmdc_event_del(struct perf_event *event, int flags)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+
+ spin_lock(&pmu_mmdc->mmdc_active_events_lock);
+ pmu_mmdc->active_events--;
+ spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
+ mmdc_event_stop(event, PERF_EF_UPDATE);
+}
+
+static void mmdc_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_event_update(event);
+ }
+}
+
+static enum hrtimer_restart mmdc_timer_handler(struct hrtimer *hrtimer)
+{
+ struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu,
+ hrtimer);
+
+ mmdc_overflow_handler(pmu_mmdc);
+
+ hrtimer_forward_now(hrtimer, mmdc_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_event_init,
+ .add = mmdc_event_add,
+ .del = mmdc_event_del,
+ .start = mmdc_event_start,
+ .stop = mmdc_event_stop,
+ .read = mmdc_event_update,
+ },
+ .mmdc_base = mmdc_base,
+ };
+
+ mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
+
+ cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
+
+ pmu_mmdc->dev = dev;
+ pmu_mmdc->active_events = 0;
+ spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
+
+ cpuhp_mmdc_pmu = pmu_mmdc;
+ cpuhp_setup_state(CPUHP_ONLINE,
+ "PERF_MMDC_ONLINE", NULL,
+ mmdc_pmu_offline_cpu);
+
+ return mmdc_num;
+}
+
static int imx_mmdc_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
void __iomem *mmdc_base, *reg;
+ struct mmdc_pmu *pmu_mmdc;
+ char *name;
u32 val;
int timeout = 0x400;
+ int mmdc_num;
mmdc_base = of_iomap(np, 0);
WARN_ON(!mmdc_base);
@@ -61,7 +415,34 @@ static int imx_mmdc_probe(struct platform_device *pdev)
__func__);
return -EBUSY;
}
+ 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);
+ hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC,
+ HRTIMER_MODE_REL);
+ pmu_mmdc->hrtimer.function = mmdc_timer_handler;
+ if (mmdc_num == 0)
+ name = "mmdc";
+ else
+ name = devm_kasprintf(&pdev->dev,
+ GFP_KERNEL, "mmdc_%d", mmdc_num);
+ platform_set_drvdata(pdev, pmu_mmdc);
+ perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
+ return 0;
+}
+
+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);
+ cpuhp_mmdc_pmu = NULL;
+ kfree(pmu_mmdc);
return 0;
}
@@ -81,6 +462,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.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-17 19:42 ` Zhengyu Shen
0 siblings, 0 replies; 20+ messages in thread
From: Zhengyu Shen @ 2016-08-17 19:42 UTC (permalink / raw)
To: linux-arm-kernel
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 <zhengyu.shen@nxp.com>
Signed-off-by: Frank Li <frank.li@nxp.com>
---
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 | 384 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 383 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
index db9621c..5fe7696 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
@@ -16,6 +16,11 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
+#include <linux/perf_event.h>
+#include <linux/slab.h>
+#include <linux/hrtimer.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
#include "common.h"
@@ -27,14 +32,363 @@
#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 DEFINE_IDA(mmdc_ida);
+
static int ddr_type;
+PMU_EVENT_ATTR_STRING(total-cycles, mmdc_total_cycles, "event=0x00")
+PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_busy_cycles, "event=0x01")
+PMU_EVENT_ATTR_STRING(read-accesses, mmdc_read_accesses, "event=0x02")
+PMU_EVENT_ATTR_STRING(write-accesses, mmdc_write_accesses, "config=0x03")
+PMU_EVENT_ATTR_STRING(read-bytes, mmdc_read_bytes, "event=0x04")
+PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_read_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_read_bytes_scale, "0.000001");
+PMU_EVENT_ATTR_STRING(write-bytes, mmdc_write_bytes, "event=0x05")
+PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_write_bytes_unit, "MB");
+PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_write_bytes_scale, "0.000001");
+
+struct mmdc_pmu {
+ struct pmu pmu;
+ void __iomem *mmdc_base;
+ cpumask_t cpu;
+ struct hrtimer hrtimer;
+ unsigned int irq;
+ unsigned int active_events;
+ struct device *dev;
+ struct perf_event *mmdc_events[MMDC_NUM_COUNTERS];
+ spinlock_t mmdc_active_events_lock;
+};
+static struct mmdc_pmu *cpuhp_mmdc_pmu;
+
+/* 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_poll_period_us = 1000000;
+module_param_named(pmu_poll_period_us, mmdc_poll_period_us, uint,
+ S_IRUGO | S_IWUSR);
+
+static ktime_t mmdc_timer_period(void)
+{
+ return ns_to_ktime((u64)mmdc_poll_period_us * 1000);
+}
+
+static ssize_t mmdc_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_cpumask_attr =
+__ATTR(cpumask, S_IRUGO, mmdc_cpumask_show, NULL);
+
+static struct attribute *mmdc_cpumask_attrs[] = {
+ &mmdc_cpumask_attr.attr,
+ NULL,
+};
+
+static struct attribute_group mmdc_cpumask_attr_group = {
+ .attrs = mmdc_cpumask_attrs,
+};
+
+static struct attribute *mmdc_events_attrs[] = {
+ &mmdc_total_cycles.attr.attr,
+ &mmdc_busy_cycles.attr.attr,
+ &mmdc_read_accesses.attr.attr,
+ &mmdc_write_accesses.attr.attr,
+ &mmdc_read_bytes.attr.attr,
+ &mmdc_read_bytes_unit.attr.attr,
+ &mmdc_read_bytes_scale.attr.attr,
+ &mmdc_write_bytes.attr.attr,
+ &mmdc_write_bytes_unit.attr.attr,
+ &mmdc_write_bytes_scale.attr.attr,
+ NULL,
+};
+
+static struct attribute_group mmdc_events_attr_group = {
+ .name = "events",
+ .attrs = mmdc_events_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-63");
+static struct attribute *mmdc_format_attrs[] = {
+ &format_attr_event.attr,
+ NULL,
+};
+
+static struct attribute_group mmdc_format_attr_group = {
+ .name = "format",
+ .attrs = mmdc_format_attrs,
+};
+
+static const struct attribute_group *attr_groups[] = {
+ &mmdc_events_attr_group,
+ &mmdc_format_attr_group,
+ &mmdc_cpumask_attr_group,
+ NULL,
+};
+
+static u32 mmdc_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg, u64 prev_val)
+{
+ u32 val;
+ 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);
+ }
+ val = readl(reg);
+ return val;
+}
+
+static int mmdc_pmu_offline_cpu(unsigned int cpu)
+{
+ struct mmdc_pmu *pmu_mmdc = cpuhp_mmdc_pmu;
+ 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);
+ if (pmu_mmdc->irq)
+ WARN_ON(irq_set_affinity_hint(
+ pmu_mmdc->irq, &pmu_mmdc->cpu) != 0);
+ return 0;
+}
+
+static int mmdc_event_init(struct perf_event *event)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ int cfg = event->attr.config;
+ struct perf_event *sibling;
+
+ if (event->attr.type != event->pmu->type)
+ return -ENOENT;
+
+ 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 (event->group_leader->pmu != event->pmu &&
+ !is_software_event(event->group_leader))
+ return -EINVAL;
+
+ list_for_each_entry(sibling, &event->group_leader->sibling_list,
+ group_entry)
+ if (sibling->pmu != event->pmu &&
+ !is_software_event(sibling))
+ return -EINVAL;
+
+ event->cpu = cpumask_first(&pmu_mmdc->cpu);
+ return 0;
+}
+
+static void mmdc_event_update(struct perf_event *event)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ u32 val;
+ u64 prev_val;
+
+ prev_val = local64_read(&event->count);
+ val = mmdc_read_counter(pmu_mmdc, (int) event->attr.config, prev_val);
+ local64_add(val - (u32)(prev_val & 0xFFFFFFFF), &event->count);
+}
+
+static void mmdc_event_start(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;
+ /* hrtimer is required because mmdc does not provide an interrupt so
+ * polling is necessary
+ */
+ hrtimer_start(&pmu_mmdc->hrtimer, mmdc_timer_period(),
+ HRTIMER_MODE_REL_PINNED);
+
+ writel(DBG_RST, reg);
+ writel(DBG_EN, reg);
+}
+
+static int mmdc_event_add(struct perf_event *event, int flags)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ int cfg = (int)event->attr.config;
+
+ if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
+ "invalid configuration %d for mmdc", cfg))
+ return -1;
+ pmu_mmdc->mmdc_events[cfg] = event;
+ local64_set(&event->count, 0);
+ if (flags & PERF_EF_START)
+ mmdc_event_start(event, flags);
+ spin_lock(&pmu_mmdc->mmdc_active_events_lock);
+ pmu_mmdc->active_events++;
+ spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
+ return 0;
+}
+
+static void mmdc_event_stop(struct perf_event *event, int flags)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+ void __iomem *mmdc_base, *reg;
+ int cfg = (int)event->attr.config;
+
+ mmdc_base = pmu_mmdc->mmdc_base;
+ reg = mmdc_base + MMDC_MADPCR0;
+ if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
+ "invalid configuration %d for mmdc counter", cfg))
+ return;
+ spin_lock(&pmu_mmdc->mmdc_active_events_lock);
+ if (pmu_mmdc->active_events <= 0)
+ hrtimer_cancel(&pmu_mmdc->hrtimer);
+ spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
+ writel(PRF_FRZ, reg);
+ mmdc_event_update(event);
+}
+
+static void mmdc_event_del(struct perf_event *event, int flags)
+{
+ struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
+
+ spin_lock(&pmu_mmdc->mmdc_active_events_lock);
+ pmu_mmdc->active_events--;
+ spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
+ mmdc_event_stop(event, PERF_EF_UPDATE);
+}
+
+static void mmdc_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_event_update(event);
+ }
+}
+
+static enum hrtimer_restart mmdc_timer_handler(struct hrtimer *hrtimer)
+{
+ struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu,
+ hrtimer);
+
+ mmdc_overflow_handler(pmu_mmdc);
+
+ hrtimer_forward_now(hrtimer, mmdc_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_event_init,
+ .add = mmdc_event_add,
+ .del = mmdc_event_del,
+ .start = mmdc_event_start,
+ .stop = mmdc_event_stop,
+ .read = mmdc_event_update,
+ },
+ .mmdc_base = mmdc_base,
+ };
+
+ mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
+
+ cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
+
+ pmu_mmdc->dev = dev;
+ pmu_mmdc->active_events = 0;
+ spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
+
+ cpuhp_mmdc_pmu = pmu_mmdc;
+ cpuhp_setup_state(CPUHP_ONLINE,
+ "PERF_MMDC_ONLINE", NULL,
+ mmdc_pmu_offline_cpu);
+
+ return mmdc_num;
+}
+
static int imx_mmdc_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
void __iomem *mmdc_base, *reg;
+ struct mmdc_pmu *pmu_mmdc;
+ char *name;
u32 val;
int timeout = 0x400;
+ int mmdc_num;
mmdc_base = of_iomap(np, 0);
WARN_ON(!mmdc_base);
@@ -61,7 +415,34 @@ static int imx_mmdc_probe(struct platform_device *pdev)
__func__);
return -EBUSY;
}
+ 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);
+ hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC,
+ HRTIMER_MODE_REL);
+ pmu_mmdc->hrtimer.function = mmdc_timer_handler;
+ if (mmdc_num == 0)
+ name = "mmdc";
+ else
+ name = devm_kasprintf(&pdev->dev,
+ GFP_KERNEL, "mmdc_%d", mmdc_num);
+ platform_set_drvdata(pdev, pmu_mmdc);
+ perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
+ return 0;
+}
+
+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);
+ cpuhp_mmdc_pmu = NULL;
+ kfree(pmu_mmdc);
return 0;
}
@@ -81,6 +462,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.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-17 19:42 ` Zhengyu Shen
@ 2016-08-29 16:06 ` Zhi Li
-1 siblings, 0 replies; 20+ messages in thread
From: Zhi Li @ 2016-08-29 16:06 UTC (permalink / raw)
To: Zhengyu Shen
Cc: Shawn Guo, Frank Li, linux-arm-kernel, kernel list, peterz,
mingo, acme, alexander.shishkin, Mark Rutland
On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen <zhengyu.shen@nxp.com> wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
Shawn Guo:
No new comments got more than 1 weeks.
Did you plan accept it?
best regards
Frank Li
> ---
> 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 | 384 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
> index db9621c..5fe7696 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
> @@ -16,6 +16,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +#include <linux/hrtimer.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
>
> #include "common.h"
>
> @@ -27,14 +32,363 @@
> #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 DEFINE_IDA(mmdc_ida);
> +
> static int ddr_type;
>
> +PMU_EVENT_ATTR_STRING(total-cycles, mmdc_total_cycles, "event=0x00")
> +PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_busy_cycles, "event=0x01")
> +PMU_EVENT_ATTR_STRING(read-accesses, mmdc_read_accesses, "event=0x02")
> +PMU_EVENT_ATTR_STRING(write-accesses, mmdc_write_accesses, "config=0x03")
> +PMU_EVENT_ATTR_STRING(read-bytes, mmdc_read_bytes, "event=0x04")
> +PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_read_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_read_bytes_scale, "0.000001");
> +PMU_EVENT_ATTR_STRING(write-bytes, mmdc_write_bytes, "event=0x05")
> +PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_write_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_write_bytes_scale, "0.000001");
> +
> +struct mmdc_pmu {
> + struct pmu pmu;
> + void __iomem *mmdc_base;
> + cpumask_t cpu;
> + struct hrtimer hrtimer;
> + unsigned int irq;
> + unsigned int active_events;
> + struct device *dev;
> + struct perf_event *mmdc_events[MMDC_NUM_COUNTERS];
> + spinlock_t mmdc_active_events_lock;
> +};
> +static struct mmdc_pmu *cpuhp_mmdc_pmu;
> +
> +/* 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_poll_period_us = 1000000;
> +module_param_named(pmu_poll_period_us, mmdc_poll_period_us, uint,
> + S_IRUGO | S_IWUSR);
> +
> +static ktime_t mmdc_timer_period(void)
> +{
> + return ns_to_ktime((u64)mmdc_poll_period_us * 1000);
> +}
> +
> +static ssize_t mmdc_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_cpumask_attr =
> +__ATTR(cpumask, S_IRUGO, mmdc_cpumask_show, NULL);
> +
> +static struct attribute *mmdc_cpumask_attrs[] = {
> + &mmdc_cpumask_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_cpumask_attr_group = {
> + .attrs = mmdc_cpumask_attrs,
> +};
> +
> +static struct attribute *mmdc_events_attrs[] = {
> + &mmdc_total_cycles.attr.attr,
> + &mmdc_busy_cycles.attr.attr,
> + &mmdc_read_accesses.attr.attr,
> + &mmdc_write_accesses.attr.attr,
> + &mmdc_read_bytes.attr.attr,
> + &mmdc_read_bytes_unit.attr.attr,
> + &mmdc_read_bytes_scale.attr.attr,
> + &mmdc_write_bytes.attr.attr,
> + &mmdc_write_bytes_unit.attr.attr,
> + &mmdc_write_bytes_scale.attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_events_attr_group = {
> + .name = "events",
> + .attrs = mmdc_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-63");
> +static struct attribute *mmdc_format_attrs[] = {
> + &format_attr_event.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_format_attr_group = {
> + .name = "format",
> + .attrs = mmdc_format_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &mmdc_events_attr_group,
> + &mmdc_format_attr_group,
> + &mmdc_cpumask_attr_group,
> + NULL,
> +};
> +
> +static u32 mmdc_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg, u64 prev_val)
> +{
> + u32 val;
> + 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);
> + }
> + val = readl(reg);
> + return val;
> +}
> +
> +static int mmdc_pmu_offline_cpu(unsigned int cpu)
> +{
> + struct mmdc_pmu *pmu_mmdc = cpuhp_mmdc_pmu;
> + 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);
> + if (pmu_mmdc->irq)
> + WARN_ON(irq_set_affinity_hint(
> + pmu_mmdc->irq, &pmu_mmdc->cpu) != 0);
> + return 0;
> +}
> +
> +static int mmdc_event_init(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = event->attr.config;
> + struct perf_event *sibling;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + 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 (event->group_leader->pmu != event->pmu &&
> + !is_software_event(event->group_leader))
> + return -EINVAL;
> +
> + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> + group_entry)
> + if (sibling->pmu != event->pmu &&
> + !is_software_event(sibling))
> + return -EINVAL;
> +
> + event->cpu = cpumask_first(&pmu_mmdc->cpu);
> + return 0;
> +}
> +
> +static void mmdc_event_update(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + u32 val;
> + u64 prev_val;
> +
> + prev_val = local64_read(&event->count);
> + val = mmdc_read_counter(pmu_mmdc, (int) event->attr.config, prev_val);
> + local64_add(val - (u32)(prev_val & 0xFFFFFFFF), &event->count);
> +}
> +
> +static void mmdc_event_start(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;
> + /* hrtimer is required because mmdc does not provide an interrupt so
> + * polling is necessary
> + */
> + hrtimer_start(&pmu_mmdc->hrtimer, mmdc_timer_period(),
> + HRTIMER_MODE_REL_PINNED);
> +
> + writel(DBG_RST, reg);
> + writel(DBG_EN, reg);
> +}
> +
> +static int mmdc_event_add(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = (int)event->attr.config;
> +
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc", cfg))
> + return -1;
> + pmu_mmdc->mmdc_events[cfg] = event;
> + local64_set(&event->count, 0);
> + if (flags & PERF_EF_START)
> + mmdc_event_start(event, flags);
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events++;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + return 0;
> +}
> +
> +static void mmdc_event_stop(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + void __iomem *mmdc_base, *reg;
> + int cfg = (int)event->attr.config;
> +
> + mmdc_base = pmu_mmdc->mmdc_base;
> + reg = mmdc_base + MMDC_MADPCR0;
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc counter", cfg))
> + return;
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + if (pmu_mmdc->active_events <= 0)
> + hrtimer_cancel(&pmu_mmdc->hrtimer);
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + writel(PRF_FRZ, reg);
> + mmdc_event_update(event);
> +}
> +
> +static void mmdc_event_del(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> +
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events--;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + mmdc_event_stop(event, PERF_EF_UPDATE);
> +}
> +
> +static void mmdc_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_event_update(event);
> + }
> +}
> +
> +static enum hrtimer_restart mmdc_timer_handler(struct hrtimer *hrtimer)
> +{
> + struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu,
> + hrtimer);
> +
> + mmdc_overflow_handler(pmu_mmdc);
> +
> + hrtimer_forward_now(hrtimer, mmdc_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_event_init,
> + .add = mmdc_event_add,
> + .del = mmdc_event_del,
> + .start = mmdc_event_start,
> + .stop = mmdc_event_stop,
> + .read = mmdc_event_update,
> + },
> + .mmdc_base = mmdc_base,
> + };
> +
> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
> +
> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
> +
> + pmu_mmdc->dev = dev;
> + pmu_mmdc->active_events = 0;
> + spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
> +
> + cpuhp_mmdc_pmu = pmu_mmdc;
> + cpuhp_setup_state(CPUHP_ONLINE,
> + "PERF_MMDC_ONLINE", NULL,
> + mmdc_pmu_offline_cpu);
> +
> + return mmdc_num;
> +}
> +
> static int imx_mmdc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> void __iomem *mmdc_base, *reg;
> + struct mmdc_pmu *pmu_mmdc;
> + char *name;
> u32 val;
> int timeout = 0x400;
> + int mmdc_num;
>
> mmdc_base = of_iomap(np, 0);
> WARN_ON(!mmdc_base);
> @@ -61,7 +415,34 @@ static int imx_mmdc_probe(struct platform_device *pdev)
> __func__);
> return -EBUSY;
> }
> + 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);
> + hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC,
> + HRTIMER_MODE_REL);
> + pmu_mmdc->hrtimer.function = mmdc_timer_handler;
> + if (mmdc_num == 0)
> + name = "mmdc";
> + else
> + name = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "mmdc_%d", mmdc_num);
> + platform_set_drvdata(pdev, pmu_mmdc);
> + perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
> + return 0;
> +}
> +
> +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);
> + cpuhp_mmdc_pmu = NULL;
> + kfree(pmu_mmdc);
> return 0;
> }
>
> @@ -81,6 +462,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.9.3
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-29 16:06 ` Zhi Li
0 siblings, 0 replies; 20+ messages in thread
From: Zhi Li @ 2016-08-29 16:06 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen <zhengyu.shen@nxp.com> wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
Shawn Guo:
No new comments got more than 1 weeks.
Did you plan accept it?
best regards
Frank Li
> ---
> 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 | 384 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
> index db9621c..5fe7696 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
> @@ -16,6 +16,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +#include <linux/hrtimer.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
>
> #include "common.h"
>
> @@ -27,14 +32,363 @@
> #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 DEFINE_IDA(mmdc_ida);
> +
> static int ddr_type;
>
> +PMU_EVENT_ATTR_STRING(total-cycles, mmdc_total_cycles, "event=0x00")
> +PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_busy_cycles, "event=0x01")
> +PMU_EVENT_ATTR_STRING(read-accesses, mmdc_read_accesses, "event=0x02")
> +PMU_EVENT_ATTR_STRING(write-accesses, mmdc_write_accesses, "config=0x03")
> +PMU_EVENT_ATTR_STRING(read-bytes, mmdc_read_bytes, "event=0x04")
> +PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_read_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_read_bytes_scale, "0.000001");
> +PMU_EVENT_ATTR_STRING(write-bytes, mmdc_write_bytes, "event=0x05")
> +PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_write_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_write_bytes_scale, "0.000001");
> +
> +struct mmdc_pmu {
> + struct pmu pmu;
> + void __iomem *mmdc_base;
> + cpumask_t cpu;
> + struct hrtimer hrtimer;
> + unsigned int irq;
> + unsigned int active_events;
> + struct device *dev;
> + struct perf_event *mmdc_events[MMDC_NUM_COUNTERS];
> + spinlock_t mmdc_active_events_lock;
> +};
> +static struct mmdc_pmu *cpuhp_mmdc_pmu;
> +
> +/* 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_poll_period_us = 1000000;
> +module_param_named(pmu_poll_period_us, mmdc_poll_period_us, uint,
> + S_IRUGO | S_IWUSR);
> +
> +static ktime_t mmdc_timer_period(void)
> +{
> + return ns_to_ktime((u64)mmdc_poll_period_us * 1000);
> +}
> +
> +static ssize_t mmdc_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_cpumask_attr =
> +__ATTR(cpumask, S_IRUGO, mmdc_cpumask_show, NULL);
> +
> +static struct attribute *mmdc_cpumask_attrs[] = {
> + &mmdc_cpumask_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_cpumask_attr_group = {
> + .attrs = mmdc_cpumask_attrs,
> +};
> +
> +static struct attribute *mmdc_events_attrs[] = {
> + &mmdc_total_cycles.attr.attr,
> + &mmdc_busy_cycles.attr.attr,
> + &mmdc_read_accesses.attr.attr,
> + &mmdc_write_accesses.attr.attr,
> + &mmdc_read_bytes.attr.attr,
> + &mmdc_read_bytes_unit.attr.attr,
> + &mmdc_read_bytes_scale.attr.attr,
> + &mmdc_write_bytes.attr.attr,
> + &mmdc_write_bytes_unit.attr.attr,
> + &mmdc_write_bytes_scale.attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_events_attr_group = {
> + .name = "events",
> + .attrs = mmdc_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-63");
> +static struct attribute *mmdc_format_attrs[] = {
> + &format_attr_event.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_format_attr_group = {
> + .name = "format",
> + .attrs = mmdc_format_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &mmdc_events_attr_group,
> + &mmdc_format_attr_group,
> + &mmdc_cpumask_attr_group,
> + NULL,
> +};
> +
> +static u32 mmdc_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg, u64 prev_val)
> +{
> + u32 val;
> + 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);
> + }
> + val = readl(reg);
> + return val;
> +}
> +
> +static int mmdc_pmu_offline_cpu(unsigned int cpu)
> +{
> + struct mmdc_pmu *pmu_mmdc = cpuhp_mmdc_pmu;
> + 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);
> + if (pmu_mmdc->irq)
> + WARN_ON(irq_set_affinity_hint(
> + pmu_mmdc->irq, &pmu_mmdc->cpu) != 0);
> + return 0;
> +}
> +
> +static int mmdc_event_init(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = event->attr.config;
> + struct perf_event *sibling;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + 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 (event->group_leader->pmu != event->pmu &&
> + !is_software_event(event->group_leader))
> + return -EINVAL;
> +
> + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> + group_entry)
> + if (sibling->pmu != event->pmu &&
> + !is_software_event(sibling))
> + return -EINVAL;
> +
> + event->cpu = cpumask_first(&pmu_mmdc->cpu);
> + return 0;
> +}
> +
> +static void mmdc_event_update(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + u32 val;
> + u64 prev_val;
> +
> + prev_val = local64_read(&event->count);
> + val = mmdc_read_counter(pmu_mmdc, (int) event->attr.config, prev_val);
> + local64_add(val - (u32)(prev_val & 0xFFFFFFFF), &event->count);
> +}
> +
> +static void mmdc_event_start(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;
> + /* hrtimer is required because mmdc does not provide an interrupt so
> + * polling is necessary
> + */
> + hrtimer_start(&pmu_mmdc->hrtimer, mmdc_timer_period(),
> + HRTIMER_MODE_REL_PINNED);
> +
> + writel(DBG_RST, reg);
> + writel(DBG_EN, reg);
> +}
> +
> +static int mmdc_event_add(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = (int)event->attr.config;
> +
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc", cfg))
> + return -1;
> + pmu_mmdc->mmdc_events[cfg] = event;
> + local64_set(&event->count, 0);
> + if (flags & PERF_EF_START)
> + mmdc_event_start(event, flags);
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events++;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + return 0;
> +}
> +
> +static void mmdc_event_stop(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + void __iomem *mmdc_base, *reg;
> + int cfg = (int)event->attr.config;
> +
> + mmdc_base = pmu_mmdc->mmdc_base;
> + reg = mmdc_base + MMDC_MADPCR0;
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc counter", cfg))
> + return;
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + if (pmu_mmdc->active_events <= 0)
> + hrtimer_cancel(&pmu_mmdc->hrtimer);
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + writel(PRF_FRZ, reg);
> + mmdc_event_update(event);
> +}
> +
> +static void mmdc_event_del(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> +
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events--;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + mmdc_event_stop(event, PERF_EF_UPDATE);
> +}
> +
> +static void mmdc_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_event_update(event);
> + }
> +}
> +
> +static enum hrtimer_restart mmdc_timer_handler(struct hrtimer *hrtimer)
> +{
> + struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu,
> + hrtimer);
> +
> + mmdc_overflow_handler(pmu_mmdc);
> +
> + hrtimer_forward_now(hrtimer, mmdc_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_event_init,
> + .add = mmdc_event_add,
> + .del = mmdc_event_del,
> + .start = mmdc_event_start,
> + .stop = mmdc_event_stop,
> + .read = mmdc_event_update,
> + },
> + .mmdc_base = mmdc_base,
> + };
> +
> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
> +
> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
> +
> + pmu_mmdc->dev = dev;
> + pmu_mmdc->active_events = 0;
> + spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
> +
> + cpuhp_mmdc_pmu = pmu_mmdc;
> + cpuhp_setup_state(CPUHP_ONLINE,
> + "PERF_MMDC_ONLINE", NULL,
> + mmdc_pmu_offline_cpu);
> +
> + return mmdc_num;
> +}
> +
> static int imx_mmdc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> void __iomem *mmdc_base, *reg;
> + struct mmdc_pmu *pmu_mmdc;
> + char *name;
> u32 val;
> int timeout = 0x400;
> + int mmdc_num;
>
> mmdc_base = of_iomap(np, 0);
> WARN_ON(!mmdc_base);
> @@ -61,7 +415,34 @@ static int imx_mmdc_probe(struct platform_device *pdev)
> __func__);
> return -EBUSY;
> }
> + 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);
> + hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC,
> + HRTIMER_MODE_REL);
> + pmu_mmdc->hrtimer.function = mmdc_timer_handler;
> + if (mmdc_num == 0)
> + name = "mmdc";
> + else
> + name = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "mmdc_%d", mmdc_num);
> + platform_set_drvdata(pdev, pmu_mmdc);
> + perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
> + return 0;
> +}
> +
> +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);
> + cpuhp_mmdc_pmu = NULL;
> + kfree(pmu_mmdc);
> return 0;
> }
>
> @@ -81,6 +462,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.9.3
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-29 16:06 ` Zhi Li
@ 2016-08-30 11:43 ` Shawn Guo
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Guo @ 2016-08-30 11:43 UTC (permalink / raw)
To: Zhi Li, Mark Rutland
Cc: Zhengyu Shen, Frank Li, linux-arm-kernel, kernel list, peterz,
mingo, acme, alexander.shishkin
On Mon, Aug 29, 2016 at 11:06:44AM -0500, Zhi Li wrote:
> On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen <zhengyu.shen@nxp.com> wrote:
> > 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 <zhengyu.shen@nxp.com>
> > Signed-off-by: Frank Li <frank.li@nxp.com>
>
> Shawn Guo:
>
> No new comments got more than 1 weeks.
> Did you plan accept it?
@Mark, how do you think of this version?
Shawn
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-30 11:43 ` Shawn Guo
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Guo @ 2016-08-30 11:43 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Aug 29, 2016 at 11:06:44AM -0500, Zhi Li wrote:
> On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen <zhengyu.shen@nxp.com> wrote:
> > 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 <zhengyu.shen@nxp.com>
> > Signed-off-by: Frank Li <frank.li@nxp.com>
>
> Shawn Guo:
>
> No new comments got more than 1 weeks.
> Did you plan accept it?
@Mark, how do you think of this version?
Shawn
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-30 11:43 ` Shawn Guo
@ 2016-08-30 12:54 ` Mark Rutland
-1 siblings, 0 replies; 20+ messages in thread
From: Mark Rutland @ 2016-08-30 12:54 UTC (permalink / raw)
To: Shawn Guo
Cc: Zhi Li, Zhengyu Shen, Frank Li, linux-arm-kernel, kernel list,
peterz, mingo, acme, alexander.shishkin
Hi,
On Tue, Aug 30, 2016 at 07:43:29PM +0800, Shawn Guo wrote:
> On Mon, Aug 29, 2016 at 11:06:44AM -0500, Zhi Li wrote:
> > On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen <zhengyu.shen@nxp.com> wrote:
> > > 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 <zhengyu.shen@nxp.com>
> > > Signed-off-by: Frank Li <frank.li@nxp.com>
> >
> > Shawn Guo:
> >
> > No new comments got more than 1 weeks.
> > Did you plan accept it?
>
> @Mark, how do you think of this version?
Sorry, I've been away for the last week and haven't had the chance to
look at this yet. I will try to get round to it in the next few days.
In the meantime, could you please try attacking this with Vince's perf
fuzzer [1] (as root, or with perf_event_paranoid dropped to -1)? It's
rather good at finding (subtle) issues in drivers.
Thanks,
Mark.
[1] https://github.com/deater/perf_event_tests
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-30 12:54 ` Mark Rutland
0 siblings, 0 replies; 20+ messages in thread
From: Mark Rutland @ 2016-08-30 12:54 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Tue, Aug 30, 2016 at 07:43:29PM +0800, Shawn Guo wrote:
> On Mon, Aug 29, 2016 at 11:06:44AM -0500, Zhi Li wrote:
> > On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen <zhengyu.shen@nxp.com> wrote:
> > > 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 <zhengyu.shen@nxp.com>
> > > Signed-off-by: Frank Li <frank.li@nxp.com>
> >
> > Shawn Guo:
> >
> > No new comments got more than 1 weeks.
> > Did you plan accept it?
>
> @Mark, how do you think of this version?
Sorry, I've been away for the last week and haven't had the chance to
look at this yet. I will try to get round to it in the next few days.
In the meantime, could you please try attacking this with Vince's perf
fuzzer [1] (as root, or with perf_event_paranoid dropped to -1)? It's
rather good at finding (subtle) issues in drivers.
Thanks,
Mark.
[1] https://github.com/deater/perf_event_tests
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-17 19:42 ` Zhengyu Shen
@ 2016-08-30 14:06 ` Shawn Guo
-1 siblings, 0 replies; 20+ messages in thread
From: Shawn Guo @ 2016-08-30 14:06 UTC (permalink / raw)
To: Zhengyu Shen
Cc: mark.rutland, peterz, frank.li, linux-kernel, acme,
alexander.shishkin, mingo, lznuaa, linux-arm-kernel
On Wed, Aug 17, 2016 at 02:42:53PM -0500, Zhengyu Shen wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
Please add 'ARM: imx: ...' prefix for the patch subject.
> ---
> 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 | 384 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
> index db9621c..5fe7696 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
> @@ -16,6 +16,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +#include <linux/hrtimer.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
Please keep the list sort alphabetically.
>
> #include "common.h"
>
> @@ -27,14 +32,363 @@
> #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
The indention for these macros is not aligned.
> +
> +/* 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))
The parentheses around container_of is not necessary.
> +
> +static DEFINE_IDA(mmdc_ida);
> +
> static int ddr_type;
>
> +PMU_EVENT_ATTR_STRING(total-cycles, mmdc_total_cycles, "event=0x00")
> +PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_busy_cycles, "event=0x01")
> +PMU_EVENT_ATTR_STRING(read-accesses, mmdc_read_accesses, "event=0x02")
> +PMU_EVENT_ATTR_STRING(write-accesses, mmdc_write_accesses, "config=0x03")
> +PMU_EVENT_ATTR_STRING(read-bytes, mmdc_read_bytes, "event=0x04")
> +PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_read_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_read_bytes_scale, "0.000001");
> +PMU_EVENT_ATTR_STRING(write-bytes, mmdc_write_bytes, "event=0x05")
> +PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_write_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_write_bytes_scale, "0.000001");
> +
> +struct mmdc_pmu {
> + struct pmu pmu;
> + void __iomem *mmdc_base;
> + cpumask_t cpu;
> + struct hrtimer hrtimer;
> + unsigned int irq;
> + unsigned int active_events;
> + struct device *dev;
> + struct perf_event *mmdc_events[MMDC_NUM_COUNTERS];
> + spinlock_t mmdc_active_events_lock;
> +};
Have a newline here please.
> +static struct mmdc_pmu *cpuhp_mmdc_pmu;
> +
> +/* polling period is set to one second, overflow of total-cycles (the fastest
> + * increasing counter) takes ten seconds so one second is safe
> + */
/*
* Multiple lines comment ...
*/
> +static unsigned int mmdc_poll_period_us = 1000000;
> +module_param_named(pmu_poll_period_us, mmdc_poll_period_us, uint,
> + S_IRUGO | S_IWUSR);
> +
> +static ktime_t mmdc_timer_period(void)
> +{
> + return ns_to_ktime((u64)mmdc_poll_period_us * 1000);
> +}
> +
> +static ssize_t mmdc_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_cpumask_attr =
> +__ATTR(cpumask, S_IRUGO, mmdc_cpumask_show, NULL);
> +
> +static struct attribute *mmdc_cpumask_attrs[] = {
> + &mmdc_cpumask_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_cpumask_attr_group = {
> + .attrs = mmdc_cpumask_attrs,
> +};
> +
> +static struct attribute *mmdc_events_attrs[] = {
> + &mmdc_total_cycles.attr.attr,
> + &mmdc_busy_cycles.attr.attr,
> + &mmdc_read_accesses.attr.attr,
> + &mmdc_write_accesses.attr.attr,
> + &mmdc_read_bytes.attr.attr,
> + &mmdc_read_bytes_unit.attr.attr,
> + &mmdc_read_bytes_scale.attr.attr,
> + &mmdc_write_bytes.attr.attr,
> + &mmdc_write_bytes_unit.attr.attr,
> + &mmdc_write_bytes_scale.attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_events_attr_group = {
> + .name = "events",
> + .attrs = mmdc_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-63");
> +static struct attribute *mmdc_format_attrs[] = {
> + &format_attr_event.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_format_attr_group = {
> + .name = "format",
> + .attrs = mmdc_format_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &mmdc_events_attr_group,
> + &mmdc_format_attr_group,
> + &mmdc_cpumask_attr_group,
> + NULL,
> +};
> +
> +static u32 mmdc_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg, u64 prev_val)
> +{
> + u32 val;
> + 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);
> + }
> + val = readl(reg);
> + return val;
return readl(reg);
> +}
> +
> +static int mmdc_pmu_offline_cpu(unsigned int cpu)
> +{
> + struct mmdc_pmu *pmu_mmdc = cpuhp_mmdc_pmu;
> + 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;
Add newline to make code a bit easy to read.
> + perf_pmu_migrate_context(&pmu_mmdc->pmu, cpu, target);
> + cpumask_set_cpu(target, &pmu_mmdc->cpu);
> + if (pmu_mmdc->irq)
> + WARN_ON(irq_set_affinity_hint(
> + pmu_mmdc->irq, &pmu_mmdc->cpu) != 0);
WARN_ON(irq_set_affinity_hint(pmu_mmdc->irq, &pmu_mmdc->cpu));
The bonus point is that the line does not exceed 80 columns, and thus
we do not need to break it into two lines.
> + return 0;
> +}
> +
> +static int mmdc_event_init(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = event->attr.config;
> + struct perf_event *sibling;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + 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 (event->group_leader->pmu != event->pmu &&
> + !is_software_event(event->group_leader))
> + return -EINVAL;
> +
> + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> + group_entry)
> + if (sibling->pmu != event->pmu &&
> + !is_software_event(sibling))
> + return -EINVAL;
> +
> + event->cpu = cpumask_first(&pmu_mmdc->cpu);
> + return 0;
> +}
> +
> +static void mmdc_event_update(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + u32 val;
> + u64 prev_val;
> +
> + prev_val = local64_read(&event->count);
> + val = mmdc_read_counter(pmu_mmdc, (int) event->attr.config, prev_val);
> + local64_add(val - (u32)(prev_val & 0xFFFFFFFF), &event->count);
> +}
> +
> +static void mmdc_event_start(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;
> + /* hrtimer is required because mmdc does not provide an interrupt so
> + * polling is necessary
> + */
/*
* Bla, Bla ...
*/
> + hrtimer_start(&pmu_mmdc->hrtimer, mmdc_timer_period(),
> + HRTIMER_MODE_REL_PINNED);
> +
> + writel(DBG_RST, reg);
> + writel(DBG_EN, reg);
> +}
> +
> +static int mmdc_event_add(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = (int)event->attr.config;
> +
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc", cfg))
> + return -1;
> + pmu_mmdc->mmdc_events[cfg] = event;
> + local64_set(&event->count, 0);
> + if (flags & PERF_EF_START)
> + mmdc_event_start(event, flags);
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events++;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + return 0;
Nit: have some newlines to make the code more readable.
> +}
> +
> +static void mmdc_event_stop(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + void __iomem *mmdc_base, *reg;
> + int cfg = (int)event->attr.config;
> +
> + mmdc_base = pmu_mmdc->mmdc_base;
> + reg = mmdc_base + MMDC_MADPCR0;
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc counter", cfg))
> + return;
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + if (pmu_mmdc->active_events <= 0)
> + hrtimer_cancel(&pmu_mmdc->hrtimer);
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + writel(PRF_FRZ, reg);
> + mmdc_event_update(event);
Ditto
> +}
> +
> +static void mmdc_event_del(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> +
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events--;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + mmdc_event_stop(event, PERF_EF_UPDATE);
> +}
> +
> +static void mmdc_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_event_update(event);
> + }
> +}
> +
> +static enum hrtimer_restart mmdc_timer_handler(struct hrtimer *hrtimer)
> +{
> + struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu,
> + hrtimer);
> +
> + mmdc_overflow_handler(pmu_mmdc);
> +
> + hrtimer_forward_now(hrtimer, mmdc_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_event_init,
> + .add = mmdc_event_add,
> + .del = mmdc_event_del,
> + .start = mmdc_event_start,
> + .stop = mmdc_event_stop,
> + .read = mmdc_event_update,
> + },
> + .mmdc_base = mmdc_base,
> + };
> +
> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
> +
> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
> +
> + pmu_mmdc->dev = dev;
> + pmu_mmdc->active_events = 0;
It's a bit odd that some of the members are initialized by above
initializer and some are done here.
> + spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
> +
> + cpuhp_mmdc_pmu = pmu_mmdc;
> + cpuhp_setup_state(CPUHP_ONLINE,
> + "PERF_MMDC_ONLINE", NULL,
> + mmdc_pmu_offline_cpu);
> +
> + return mmdc_num;
> +}
> +
> static int imx_mmdc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> void __iomem *mmdc_base, *reg;
> + struct mmdc_pmu *pmu_mmdc;
> + char *name;
> u32 val;
> int timeout = 0x400;
> + int mmdc_num;
>
> mmdc_base = of_iomap(np, 0);
> WARN_ON(!mmdc_base);
> @@ -61,7 +415,34 @@ static int imx_mmdc_probe(struct platform_device *pdev)
> __func__);
> return -EBUSY;
> }
Have a newline here.
> + pmu_mmdc = kzalloc(sizeof(*pmu_mmdc), GFP_KERNEL);
> +
Drop this newline. Hmmm, the way you are putting newline or not is quite
uncommon/odd.
> + if (!pmu_mmdc) {
> + pr_err("failed to allocate PMU device!\n");
> + return -ENOMEM;
> + }
> + mmdc_num = mmdc_pmu_init(pmu_mmdc, mmdc_base, &pdev->dev);
> + hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC,
> + HRTIMER_MODE_REL);
> + pmu_mmdc->hrtimer.function = mmdc_timer_handler;
> + if (mmdc_num == 0)
> + name = "mmdc";
> + else
> + name = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "mmdc_%d", mmdc_num);
> + platform_set_drvdata(pdev, pmu_mmdc);
> + perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
No return check?
Also if I were you, I would order the code blocks like following (if
there is no dependency):
hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
pmu_mmdc->hrtimer.function = mmdc_timer_handler;
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);
perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
platform_set_drvdata(pdev, pmu_mmdc);
Overall, I do not like how you format the code, but that's personal
taste, I guess, so shouldn't be a problem.
Shawn
> + return 0;
> +}
> +
> +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);
> + cpuhp_mmdc_pmu = NULL;
> + kfree(pmu_mmdc);
> return 0;
> }
>
> @@ -81,6 +462,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.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-30 14:06 ` Shawn Guo
0 siblings, 0 replies; 20+ messages in thread
From: Shawn Guo @ 2016-08-30 14:06 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 17, 2016 at 02:42:53PM -0500, Zhengyu Shen wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
Please add 'ARM: imx: ...' prefix for the patch subject.
> ---
> 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 | 384 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/mmdc.c b/arch/arm/mach-imx/mmdc.c
> index db9621c..5fe7696 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
> @@ -16,6 +16,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +#include <linux/hrtimer.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
Please keep the list sort alphabetically.
>
> #include "common.h"
>
> @@ -27,14 +32,363 @@
> #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
The indention for these macros is not aligned.
> +
> +/* 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))
The parentheses around container_of is not necessary.
> +
> +static DEFINE_IDA(mmdc_ida);
> +
> static int ddr_type;
>
> +PMU_EVENT_ATTR_STRING(total-cycles, mmdc_total_cycles, "event=0x00")
> +PMU_EVENT_ATTR_STRING(busy-cycles, mmdc_busy_cycles, "event=0x01")
> +PMU_EVENT_ATTR_STRING(read-accesses, mmdc_read_accesses, "event=0x02")
> +PMU_EVENT_ATTR_STRING(write-accesses, mmdc_write_accesses, "config=0x03")
> +PMU_EVENT_ATTR_STRING(read-bytes, mmdc_read_bytes, "event=0x04")
> +PMU_EVENT_ATTR_STRING(read-bytes.unit, mmdc_read_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(read-bytes.scale, mmdc_read_bytes_scale, "0.000001");
> +PMU_EVENT_ATTR_STRING(write-bytes, mmdc_write_bytes, "event=0x05")
> +PMU_EVENT_ATTR_STRING(write-bytes.unit, mmdc_write_bytes_unit, "MB");
> +PMU_EVENT_ATTR_STRING(write-bytes.scale, mmdc_write_bytes_scale, "0.000001");
> +
> +struct mmdc_pmu {
> + struct pmu pmu;
> + void __iomem *mmdc_base;
> + cpumask_t cpu;
> + struct hrtimer hrtimer;
> + unsigned int irq;
> + unsigned int active_events;
> + struct device *dev;
> + struct perf_event *mmdc_events[MMDC_NUM_COUNTERS];
> + spinlock_t mmdc_active_events_lock;
> +};
Have a newline here please.
> +static struct mmdc_pmu *cpuhp_mmdc_pmu;
> +
> +/* polling period is set to one second, overflow of total-cycles (the fastest
> + * increasing counter) takes ten seconds so one second is safe
> + */
/*
* Multiple lines comment ...
*/
> +static unsigned int mmdc_poll_period_us = 1000000;
> +module_param_named(pmu_poll_period_us, mmdc_poll_period_us, uint,
> + S_IRUGO | S_IWUSR);
> +
> +static ktime_t mmdc_timer_period(void)
> +{
> + return ns_to_ktime((u64)mmdc_poll_period_us * 1000);
> +}
> +
> +static ssize_t mmdc_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_cpumask_attr =
> +__ATTR(cpumask, S_IRUGO, mmdc_cpumask_show, NULL);
> +
> +static struct attribute *mmdc_cpumask_attrs[] = {
> + &mmdc_cpumask_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_cpumask_attr_group = {
> + .attrs = mmdc_cpumask_attrs,
> +};
> +
> +static struct attribute *mmdc_events_attrs[] = {
> + &mmdc_total_cycles.attr.attr,
> + &mmdc_busy_cycles.attr.attr,
> + &mmdc_read_accesses.attr.attr,
> + &mmdc_write_accesses.attr.attr,
> + &mmdc_read_bytes.attr.attr,
> + &mmdc_read_bytes_unit.attr.attr,
> + &mmdc_read_bytes_scale.attr.attr,
> + &mmdc_write_bytes.attr.attr,
> + &mmdc_write_bytes_unit.attr.attr,
> + &mmdc_write_bytes_scale.attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_events_attr_group = {
> + .name = "events",
> + .attrs = mmdc_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-63");
> +static struct attribute *mmdc_format_attrs[] = {
> + &format_attr_event.attr,
> + NULL,
> +};
> +
> +static struct attribute_group mmdc_format_attr_group = {
> + .name = "format",
> + .attrs = mmdc_format_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> + &mmdc_events_attr_group,
> + &mmdc_format_attr_group,
> + &mmdc_cpumask_attr_group,
> + NULL,
> +};
> +
> +static u32 mmdc_read_counter(struct mmdc_pmu *pmu_mmdc, int cfg, u64 prev_val)
> +{
> + u32 val;
> + 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);
> + }
> + val = readl(reg);
> + return val;
return readl(reg);
> +}
> +
> +static int mmdc_pmu_offline_cpu(unsigned int cpu)
> +{
> + struct mmdc_pmu *pmu_mmdc = cpuhp_mmdc_pmu;
> + 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;
Add newline to make code a bit easy to read.
> + perf_pmu_migrate_context(&pmu_mmdc->pmu, cpu, target);
> + cpumask_set_cpu(target, &pmu_mmdc->cpu);
> + if (pmu_mmdc->irq)
> + WARN_ON(irq_set_affinity_hint(
> + pmu_mmdc->irq, &pmu_mmdc->cpu) != 0);
WARN_ON(irq_set_affinity_hint(pmu_mmdc->irq, &pmu_mmdc->cpu));
The bonus point is that the line does not exceed 80 columns, and thus
we do not need to break it into two lines.
> + return 0;
> +}
> +
> +static int mmdc_event_init(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = event->attr.config;
> + struct perf_event *sibling;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + 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 (event->group_leader->pmu != event->pmu &&
> + !is_software_event(event->group_leader))
> + return -EINVAL;
> +
> + list_for_each_entry(sibling, &event->group_leader->sibling_list,
> + group_entry)
> + if (sibling->pmu != event->pmu &&
> + !is_software_event(sibling))
> + return -EINVAL;
> +
> + event->cpu = cpumask_first(&pmu_mmdc->cpu);
> + return 0;
> +}
> +
> +static void mmdc_event_update(struct perf_event *event)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + u32 val;
> + u64 prev_val;
> +
> + prev_val = local64_read(&event->count);
> + val = mmdc_read_counter(pmu_mmdc, (int) event->attr.config, prev_val);
> + local64_add(val - (u32)(prev_val & 0xFFFFFFFF), &event->count);
> +}
> +
> +static void mmdc_event_start(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;
> + /* hrtimer is required because mmdc does not provide an interrupt so
> + * polling is necessary
> + */
/*
* Bla, Bla ...
*/
> + hrtimer_start(&pmu_mmdc->hrtimer, mmdc_timer_period(),
> + HRTIMER_MODE_REL_PINNED);
> +
> + writel(DBG_RST, reg);
> + writel(DBG_EN, reg);
> +}
> +
> +static int mmdc_event_add(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + int cfg = (int)event->attr.config;
> +
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc", cfg))
> + return -1;
> + pmu_mmdc->mmdc_events[cfg] = event;
> + local64_set(&event->count, 0);
> + if (flags & PERF_EF_START)
> + mmdc_event_start(event, flags);
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events++;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + return 0;
Nit: have some newlines to make the code more readable.
> +}
> +
> +static void mmdc_event_stop(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> + void __iomem *mmdc_base, *reg;
> + int cfg = (int)event->attr.config;
> +
> + mmdc_base = pmu_mmdc->mmdc_base;
> + reg = mmdc_base + MMDC_MADPCR0;
> + if (WARN_ONCE((cfg < 0 || cfg >= MMDC_NUM_COUNTERS),
> + "invalid configuration %d for mmdc counter", cfg))
> + return;
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + if (pmu_mmdc->active_events <= 0)
> + hrtimer_cancel(&pmu_mmdc->hrtimer);
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + writel(PRF_FRZ, reg);
> + mmdc_event_update(event);
Ditto
> +}
> +
> +static void mmdc_event_del(struct perf_event *event, int flags)
> +{
> + struct mmdc_pmu *pmu_mmdc = to_mmdc_pmu(event->pmu);
> +
> + spin_lock(&pmu_mmdc->mmdc_active_events_lock);
> + pmu_mmdc->active_events--;
> + spin_unlock(&pmu_mmdc->mmdc_active_events_lock);
> + mmdc_event_stop(event, PERF_EF_UPDATE);
> +}
> +
> +static void mmdc_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_event_update(event);
> + }
> +}
> +
> +static enum hrtimer_restart mmdc_timer_handler(struct hrtimer *hrtimer)
> +{
> + struct mmdc_pmu *pmu_mmdc = container_of(hrtimer, struct mmdc_pmu,
> + hrtimer);
> +
> + mmdc_overflow_handler(pmu_mmdc);
> +
> + hrtimer_forward_now(hrtimer, mmdc_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_event_init,
> + .add = mmdc_event_add,
> + .del = mmdc_event_del,
> + .start = mmdc_event_start,
> + .stop = mmdc_event_stop,
> + .read = mmdc_event_update,
> + },
> + .mmdc_base = mmdc_base,
> + };
> +
> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
> +
> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
> +
> + pmu_mmdc->dev = dev;
> + pmu_mmdc->active_events = 0;
It's a bit odd that some of the members are initialized by above
initializer and some are done here.
> + spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
> +
> + cpuhp_mmdc_pmu = pmu_mmdc;
> + cpuhp_setup_state(CPUHP_ONLINE,
> + "PERF_MMDC_ONLINE", NULL,
> + mmdc_pmu_offline_cpu);
> +
> + return mmdc_num;
> +}
> +
> static int imx_mmdc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> void __iomem *mmdc_base, *reg;
> + struct mmdc_pmu *pmu_mmdc;
> + char *name;
> u32 val;
> int timeout = 0x400;
> + int mmdc_num;
>
> mmdc_base = of_iomap(np, 0);
> WARN_ON(!mmdc_base);
> @@ -61,7 +415,34 @@ static int imx_mmdc_probe(struct platform_device *pdev)
> __func__);
> return -EBUSY;
> }
Have a newline here.
> + pmu_mmdc = kzalloc(sizeof(*pmu_mmdc), GFP_KERNEL);
> +
Drop this newline. Hmmm, the way you are putting newline or not is quite
uncommon/odd.
> + if (!pmu_mmdc) {
> + pr_err("failed to allocate PMU device!\n");
> + return -ENOMEM;
> + }
> + mmdc_num = mmdc_pmu_init(pmu_mmdc, mmdc_base, &pdev->dev);
> + hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC,
> + HRTIMER_MODE_REL);
> + pmu_mmdc->hrtimer.function = mmdc_timer_handler;
> + if (mmdc_num == 0)
> + name = "mmdc";
> + else
> + name = devm_kasprintf(&pdev->dev,
> + GFP_KERNEL, "mmdc_%d", mmdc_num);
> + platform_set_drvdata(pdev, pmu_mmdc);
> + perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
No return check?
Also if I were you, I would order the code blocks like following (if
there is no dependency):
hrtimer_init(&pmu_mmdc->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
pmu_mmdc->hrtimer.function = mmdc_timer_handler;
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);
perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
platform_set_drvdata(pdev, pmu_mmdc);
Overall, I do not like how you format the code, but that's personal
taste, I guess, so shouldn't be a problem.
Shawn
> + return 0;
> +}
> +
> +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);
> + cpuhp_mmdc_pmu = NULL;
> + kfree(pmu_mmdc);
> return 0;
> }
>
> @@ -81,6 +462,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.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3] Added perf functionality to mmdc driver
2016-08-30 12:54 ` Mark Rutland
@ 2016-08-30 20:01 ` Zhengyu Shen
-1 siblings, 0 replies; 20+ messages in thread
From: Zhengyu Shen @ 2016-08-30 20:01 UTC (permalink / raw)
To: Mark Rutland, Shawn Guo
Cc: Zhi Li, Frank Li, linux-arm-kernel, kernel list, peterz, mingo,
acme, alexander.shishkin
> Hi,
>
> On Tue, Aug 30, 2016 at 07:43:29PM +0800, Shawn Guo wrote:
> > On Mon, Aug 29, 2016 at 11:06:44AM -0500, Zhi Li wrote:
> > > On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen
> <zhengyu.shen@nxp.com> wrote:
> > > > 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 <zhengyu.shen@nxp.com>
> > > > Signed-off-by: Frank Li <frank.li@nxp.com>
> > >
> > > Shawn Guo:
> > >
> > > No new comments got more than 1 weeks.
> > > Did you plan accept it?
> >
> > @Mark, how do you think of this version?
>
> Sorry, I've been away for the last week and haven't had the chance to look at
> this yet. I will try to get round to it in the next few days.
>
> In the meantime, could you please try attacking this with Vince's perf fuzzer
> [1] (as root, or with perf_event_paranoid dropped to -1)? It's rather good at
> finding (subtle) issues in drivers.
>
> Thanks,
> Mark.
>
> [1] https://github.com/deater/perf_event_tests
Hi, I've done some testing with the fuzzer. Mmdc was only responsible
for one crash which I fixed (had to remove the event from the pmu properly).
Other drivers also cause crashes and the program reports that events are
Throttling. Is this normal?
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-30 20:01 ` Zhengyu Shen
0 siblings, 0 replies; 20+ messages in thread
From: Zhengyu Shen @ 2016-08-30 20:01 UTC (permalink / raw)
To: linux-arm-kernel
> Hi,
>
> On Tue, Aug 30, 2016 at 07:43:29PM +0800, Shawn Guo wrote:
> > On Mon, Aug 29, 2016 at 11:06:44AM -0500, Zhi Li wrote:
> > > On Wed, Aug 17, 2016 at 2:42 PM, Zhengyu Shen
> <zhengyu.shen@nxp.com> wrote:
> > > > 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 <zhengyu.shen@nxp.com>
> > > > Signed-off-by: Frank Li <frank.li@nxp.com>
> > >
> > > Shawn Guo:
> > >
> > > No new comments got more than 1 weeks.
> > > Did you plan accept it?
> >
> > @Mark, how do you think of this version?
>
> Sorry, I've been away for the last week and haven't had the chance to look at
> this yet. I will try to get round to it in the next few days.
>
> In the meantime, could you please try attacking this with Vince's perf fuzzer
> [1] (as root, or with perf_event_paranoid dropped to -1)? It's rather good at
> finding (subtle) issues in drivers.
>
> Thanks,
> Mark.
>
> [1] https://github.com/deater/perf_event_tests
Hi, I've done some testing with the fuzzer. Mmdc was only responsible
for one crash which I fixed (had to remove the event from the pmu properly).
Other drivers also cause crashes and the program reports that events are
Throttling. Is this normal?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-30 20:01 ` Zhengyu Shen
@ 2016-08-31 10:30 ` Mark Rutland
-1 siblings, 0 replies; 20+ messages in thread
From: Mark Rutland @ 2016-08-31 10:30 UTC (permalink / raw)
To: Zhengyu Shen
Cc: Shawn Guo, Zhi Li, Frank Li, linux-arm-kernel, kernel list,
peterz, mingo, acme, alexander.shishkin
On Tue, Aug 30, 2016 at 08:01:18PM +0000, Zhengyu Shen wrote:
> Hi, I've done some testing with the fuzzer.
Great! Many thanks for doing this.
> Mmdc was only responsible for one crash which I fixed (had to remove
> the event from the pmu properly).
Ok. I take it that there will be a v4 appearing shortly with that fix?
> Other drivers also cause crashes and the program reports that events are
> Throttling. Is this normal?
Throttling is normal, and warnings about throttling can (generally) be
safely ignored.
Crashes are very bad. Do you have any logs from those crashes in other
drivers that you can share?
It is possible that a bug in one driver causes failures in another (e.g.
if events are erroneously grouped together), so it's not alwyas clear if
this is an unrelated bug (though if you can trigger the issue without
your driver present, it clearly is).
Thanks,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-31 10:30 ` Mark Rutland
0 siblings, 0 replies; 20+ messages in thread
From: Mark Rutland @ 2016-08-31 10:30 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 30, 2016 at 08:01:18PM +0000, Zhengyu Shen wrote:
> Hi, I've done some testing with the fuzzer.
Great! Many thanks for doing this.
> Mmdc was only responsible for one crash which I fixed (had to remove
> the event from the pmu properly).
Ok. I take it that there will be a v4 appearing shortly with that fix?
> Other drivers also cause crashes and the program reports that events are
> Throttling. Is this normal?
Throttling is normal, and warnings about throttling can (generally) be
safely ignored.
Crashes are very bad. Do you have any logs from those crashes in other
drivers that you can share?
It is possible that a bug in one driver causes failures in another (e.g.
if events are erroneously grouped together), so it's not alwyas clear if
this is an unrelated bug (though if you can trigger the issue without
your driver present, it clearly is).
Thanks,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-17 19:42 ` Zhengyu Shen
@ 2016-08-31 13:08 ` Suzuki K Poulose
-1 siblings, 0 replies; 20+ messages in thread
From: Suzuki K Poulose @ 2016-08-31 13:08 UTC (permalink / raw)
To: Zhengyu Shen, shawnguo
Cc: mark.rutland, peterz, frank.li, linux-kernel, acme,
alexander.shishkin, mingo, lznuaa, linux-arm-kernel
On 17/08/16 20:42, Zhengyu Shen wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
> ---
> 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
Should we move all the PMU specific code under at least CONFIG_PERF_EVENTS ?
Suzuki
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-31 13:08 ` Suzuki K Poulose
0 siblings, 0 replies; 20+ messages in thread
From: Suzuki K Poulose @ 2016-08-31 13:08 UTC (permalink / raw)
To: linux-arm-kernel
On 17/08/16 20:42, Zhengyu Shen wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
> ---
> 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
Should we move all the PMU specific code under at least CONFIG_PERF_EVENTS ?
Suzuki
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3] Added perf functionality to mmdc driver
2016-08-31 10:30 ` Mark Rutland
@ 2016-08-31 13:46 ` Zhengyu Shen
-1 siblings, 0 replies; 20+ messages in thread
From: Zhengyu Shen @ 2016-08-31 13:46 UTC (permalink / raw)
To: Mark Rutland
Cc: Shawn Guo, Zhi Li, Frank Li, linux-arm-kernel, kernel list,
peterz, mingo, acme, alexander.shishkin
> > Mmdc was only responsible for one crash which I fixed (had to remove
> > the event from the pmu properly).
>
> Ok. I take it that there will be a v4 appearing shortly with that fix?
Hopefully I'll get that out by the end of today.
> Crashes are very bad. Do you have any logs from those crashes in other
> drivers that you can share?
It seems my kernel source was out of date, updating it seems to have fixed
The issue.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-31 13:46 ` Zhengyu Shen
0 siblings, 0 replies; 20+ messages in thread
From: Zhengyu Shen @ 2016-08-31 13:46 UTC (permalink / raw)
To: linux-arm-kernel
> > Mmdc was only responsible for one crash which I fixed (had to remove
> > the event from the pmu properly).
>
> Ok. I take it that there will be a v4 appearing shortly with that fix?
Hopefully I'll get that out by the end of today.
> Crashes are very bad. Do you have any logs from those crashes in other
> drivers that you can share?
It seems my kernel source was out of date, updating it seems to have fixed
The issue.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3] Added perf functionality to mmdc driver
2016-08-17 19:42 ` Zhengyu Shen
@ 2016-08-31 16:20 ` Suzuki K Poulose
-1 siblings, 0 replies; 20+ messages in thread
From: Suzuki K Poulose @ 2016-08-31 16:20 UTC (permalink / raw)
To: Zhengyu Shen, shawnguo
Cc: mark.rutland, peterz, frank.li, linux-kernel, acme,
alexander.shishkin, mingo, lznuaa, linux-arm-kernel
On 17/08/16 20:42, Zhengyu Shen wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
> +
> +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_event_init,
> + .add = mmdc_event_add,
> + .del = mmdc_event_del,
> + .start = mmdc_event_start,
> + .stop = mmdc_event_stop,
> + .read = mmdc_event_update,
> + },
> + .mmdc_base = mmdc_base,
> + };
> +
> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
> +
> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
> +
> + pmu_mmdc->dev = dev;
> + pmu_mmdc->active_events = 0;
> + spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
> +
> + cpuhp_mmdc_pmu = pmu_mmdc;
> + cpuhp_setup_state(CPUHP_ONLINE,
> + "PERF_MMDC_ONLINE", NULL,
> + mmdc_pmu_offline_cpu);
You may want cpuhp_setup_state_nocalls instead here ?
Cheers
Suzuki
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] Added perf functionality to mmdc driver
@ 2016-08-31 16:20 ` Suzuki K Poulose
0 siblings, 0 replies; 20+ messages in thread
From: Suzuki K Poulose @ 2016-08-31 16:20 UTC (permalink / raw)
To: linux-arm-kernel
On 17/08/16 20:42, Zhengyu Shen wrote:
> 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 <zhengyu.shen@nxp.com>
> Signed-off-by: Frank Li <frank.li@nxp.com>
> +
> +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_event_init,
> + .add = mmdc_event_add,
> + .del = mmdc_event_del,
> + .start = mmdc_event_start,
> + .stop = mmdc_event_stop,
> + .read = mmdc_event_update,
> + },
> + .mmdc_base = mmdc_base,
> + };
> +
> + mmdc_num = ida_simple_get(&mmdc_ida, 0, 0, GFP_KERNEL);
> +
> + cpumask_set_cpu(smp_processor_id(), &pmu_mmdc->cpu);
> +
> + pmu_mmdc->dev = dev;
> + pmu_mmdc->active_events = 0;
> + spin_lock_init(&pmu_mmdc->mmdc_active_events_lock);
> +
> + cpuhp_mmdc_pmu = pmu_mmdc;
> + cpuhp_setup_state(CPUHP_ONLINE,
> + "PERF_MMDC_ONLINE", NULL,
> + mmdc_pmu_offline_cpu);
You may want cpuhp_setup_state_nocalls instead here ?
Cheers
Suzuki
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-08-31 16:20 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-17 19:42 [PATCH v3] Added perf functionality to mmdc driver Zhengyu Shen
2016-08-17 19:42 ` Zhengyu Shen
2016-08-29 16:06 ` Zhi Li
2016-08-29 16:06 ` Zhi Li
2016-08-30 11:43 ` Shawn Guo
2016-08-30 11:43 ` Shawn Guo
2016-08-30 12:54 ` Mark Rutland
2016-08-30 12:54 ` Mark Rutland
2016-08-30 20:01 ` Zhengyu Shen
2016-08-30 20:01 ` Zhengyu Shen
2016-08-31 10:30 ` Mark Rutland
2016-08-31 10:30 ` Mark Rutland
2016-08-31 13:46 ` Zhengyu Shen
2016-08-31 13:46 ` Zhengyu Shen
2016-08-30 14:06 ` Shawn Guo
2016-08-30 14:06 ` Shawn Guo
2016-08-31 13:08 ` Suzuki K Poulose
2016-08-31 13:08 ` Suzuki K Poulose
2016-08-31 16:20 ` Suzuki K Poulose
2016-08-31 16:20 ` Suzuki K Poulose
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.