[PATCH V9 1/4] dt-bindings: perf: imx8-ddr: add imx8qxp ddr performance monitor

[PATCH V9 1/4] dt-bindings: perf: imx8-ddr: add imx8qxp ddr performance monitor

[Frank Li]

Added binding doc for imx8qxp ddr performance monitor

Signed-off-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change from v8 to v9
* use 32bit address width 

No change from v4 to v8

Change from v4 to v4
* remove "standard xxx"

Change from v2 to v3
* ddr_pmu0 -> ddr-pmu


 .../devicetree/bindings/perf/fsl-imx-ddr.txt       | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/fsl-imx-ddr.txt

diff --git a/Documentation/devicetree/bindings/perf/fsl-imx-ddr.txt b/Documentation/devicetree/bindings/perf/fsl-imx-ddr.txt
new file mode 100644
index 0000000..9b9cda6a
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/fsl-imx-ddr.txt
@@ -0,0 +1,22 @@
+* Freescale(NXP) IMX8 DDR performance monitor
+
+Required properties:
+
+- compatible: should be one of:
+	"fsl,imx8-ddr-pmu"
+	"fsl,imx8m-ddr-pmu"
+
+- reg: physical address and size
+
+- interrupts: single interrupt
+	generated by the control block
+
+Example:
+
+	ddr-pmu@5c020000 {
+		compatible = "fsl,imx8-ddr-pmu";
+		reg = <0x5c020000 0x10000>;
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
-- 
2.5.2

[PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter support

[Frank Li]

Add ddr performance monitor support for iMX8QXP

There are 4 counters for ddr perfomance events.
counter 0 is dedicated for cycles.
you choose any up to 3 no cycles events.

for example:

perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls

Support below events.

  ddr0/activate/                                     [Kernel PMU event]
  ddr0/axid-read/                                    [Kernel PMU event]
  ddr0/axid-write/                                   [Kernel PMU event]
  ddr0/cycles/                                       [Kernel PMU event]
  ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
  ddr0/hp-read/                                      [Kernel PMU event]
  ddr0/hp-req-nodcredit/                             [Kernel PMU event]
  ddr0/hp-xact-credit/                               [Kernel PMU event]
  ddr0/load-mode/                                    [Kernel PMU event]
  ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
  ddr0/lp-req-nocredit/                              [Kernel PMU event]
  ddr0/lp-xact-credit/                               [Kernel PMU event]
  ddr0/mwr/                                          [Kernel PMU event]
  ddr0/precharge/                                    [Kernel PMU event]
  ddr0/raw-hazard/                                   [Kernel PMU event]
  ddr0/read-access/                                  [Kernel PMU event]
  ddr0/read-activate/                                [Kernel PMU event]
  ddr0/read-command/                                 [Kernel PMU event]
  ddr0/read-cycles/                                  [Kernel PMU event]
  ddr0/read-modify-write-command/                    [Kernel PMU event]
  ddr0/read-queue-depth/                             [Kernel PMU event]
  ddr0/read-write-transition/                        [Kernel PMU event]
  ddr0/read/                                         [Kernel PMU event]
  ddr0/refresh/                                      [Kernel PMU event]
  ddr0/selfresh/                                     [Kernel PMU event]
  ddr0/wr-xact-credit/                               [Kernel PMU event]
  ddr0/write-access/                                 [Kernel PMU event]
  ddr0/write-command/                                [Kernel PMU event]
  ddr0/write-credit-cnt/                             [Kernel PMU event]
  ddr0/write-cycles/                                 [Kernel PMU event]
  ddr0/write-queue-depth/                            [Kernel PMU event]
  ddr0/write/

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
No change from V8 to V9

Change from v7 to v8
 * remove unused define
 * change total_events to active_events, change active_events to events
 * remove flags, 
 * fix multi line comments code sytle
 * add pmu_enable\disable function
 * disable event at irq handle
 * remove counter check at ddr_perf_free_counter
 * remove pmu->irq check
 * add group check

Change from v6 to v7
 * added irq affinity handle, ref arm-ccn.c
 * added IRQF_NOBALANCING | IRQF_NO_THREAD
 * added ida_simple_remove at failure path

Change from v5 to v6
 * fix insmod\rmmod problem
 * remove randunt register read at irq handle
 * change u32 irq to int
 * devm_request_irq use default flags.

Change from v4 to v5
 * Remove AXI ID filter function

Change from v3 to v4
 * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
 * sort include
 * remove struct fsl_ddr_devtype_data
 * Added comment need disable control first
 * Added comment about must enable cycle counter
 * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
 * Added comment about cycle counter is fastest one

Change from v2 to v3
 * remove kfree

Change from V1 to V2
 * update Kconfig by use i.MX8 instead of i.MX8 QXP
 * remove gpl statememnt since SPDX tag
 * use dev_kzalloc
 * use dev_err
 * commit message show axi_read 0x41\axi_write 0x42
 * commit message show cycles must be enabled
 * Irq only issue at cycles overflow
 * use NUM_COUNTER
 * use devm_request_irq
 * add hotplug callback to handle context migration
 drivers/perf/Kconfig             |   7 +
 drivers/perf/Makefile            |   1 +
 drivers/perf/fsl_imx8_ddr_perf.c | 589 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 597 insertions(+)
 create mode 100644 drivers/perf/fsl_imx8_ddr_perf.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index a94e586..9bc3785 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -70,6 +70,13 @@ config ARM_DSU_PMU
 	  system, control logic. The PMU allows counting various events related
 	  to DSU.
 
+config FSL_IMX8_DDR_PMU
+	tristate "Freescale i.MX8 DDR perf monitor"
+	depends on ARCH_MXC
+	  help
+	  Provides support for ddr perfomance monitor in i.MX8. Provide memory
+	  througput information.
+
 config HISI_PMU
        bool "HiSilicon SoC PMU"
        depends on ARM64 && ACPI
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index 3048994..2ebb4de 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_ARM_DSU_PMU) += arm_dsu_pmu.o
 obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o
 obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
 obj-$(CONFIG_ARM_SMMU_V3_PMU) += arm_smmuv3_pmu.o
+obj-$(CONFIG_FSL_IMX8_DDR_PMU) += fsl_imx8_ddr_perf.o
 obj-$(CONFIG_HISI_PMU) += hisilicon/
 obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
 obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
new file mode 100644
index 0000000..087d75a
--- /dev/null
+++ b/drivers/perf/fsl_imx8_ddr_perf.c
@@ -0,0 +1,589 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2017 NXP
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ */
+
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/perf_event.h>
+#include <linux/slab.h>
+
+#define COUNTER_CNTL		0x0
+#define COUNTER_READ		0x20
+
+#define COUNTER_DPCR1		0x30
+
+#define CNTL_OVER		0x1
+#define CNTL_CLEAR		0x2
+#define CNTL_EN			0x4
+#define CNTL_EN_MASK		0xFFFFFFFB
+#define CNTL_CLEAR_MASK		0xFFFFFFFD
+#define CNTL_OVER_MASK		0xFFFFFFFE
+
+#define CNTL_CSV_SHIFT		24
+#define CNTL_CSV_MASK		(0xFF << CNTL_CSV_SHIFT)
+
+#define EVENT_CYCLES_ID		0
+#define EVENT_CYCLES_COUNTER	0
+#define NUM_COUNTERS		4
+
+#define to_ddr_pmu(p)		container_of(p, struct ddr_pmu, pmu)
+
+#define DDR_PERF_DEV_NAME	"ddr_perf"
+
+static DEFINE_IDA(ddr_ida);
+
+PMU_EVENT_ATTR_STRING(cycles, ddr_perf_cycles, "event=0x00");
+PMU_EVENT_ATTR_STRING(selfresh, ddr_perf_selfresh, "event=0x01");
+PMU_EVENT_ATTR_STRING(read-access, ddr_perf_read_accesses, "event=0x04");
+PMU_EVENT_ATTR_STRING(write-access, ddr_perf_write_accesses, "event=0x05");
+PMU_EVENT_ATTR_STRING(read-queue-depth, ddr_perf_read_queue_depth,
+			"event=0x08");
+PMU_EVENT_ATTR_STRING(write-queue-depth, ddr_perf_write_queue_depth,
+			"event=0x09");
+PMU_EVENT_ATTR_STRING(lp-read-credit-cnt, ddr_perf_lp_read_credit_cnt,
+			"event=0x10");
+PMU_EVENT_ATTR_STRING(hp-read-credit-cnt, ddr_perf_hp_read_credit_cnt,
+			"event=0x11");
+PMU_EVENT_ATTR_STRING(write-credit-cnt, ddr_perf_write_credit_cnt,
+			"event=0x12");
+PMU_EVENT_ATTR_STRING(read-command, ddr_perf_read_command, "event=0x20");
+PMU_EVENT_ATTR_STRING(write-command, ddr_perf_write_command, "event=0x21");
+PMU_EVENT_ATTR_STRING(read-modify-write-command,
+		ddr_perf_read_modify_write_command, "event=0x22");
+PMU_EVENT_ATTR_STRING(hp-read, ddr_perf_hp_read, "event=0x23");
+PMU_EVENT_ATTR_STRING(hp-req-nodcredit, ddr_perf_hp_req_nocredit, "event=0x24");
+PMU_EVENT_ATTR_STRING(hp-xact-credit, ddr_perf_hp_xact_credit, "event=0x25");
+PMU_EVENT_ATTR_STRING(lp-req-nocredit, ddr_perf_lp_req_nocredit, "event=0x26");
+PMU_EVENT_ATTR_STRING(lp-xact-credit, ddr_perf_lp_xact_credit, "event=0x27");
+PMU_EVENT_ATTR_STRING(wr-xact-credit, ddr_perf_wr_xact_credit, "event=0x29");
+PMU_EVENT_ATTR_STRING(read-cycles, ddr_perf_read_cycles, "event=0x2a");
+PMU_EVENT_ATTR_STRING(write-cycles, ddr_perf_write_cycles, "event=0x2b");
+PMU_EVENT_ATTR_STRING(read-write-transition, ddr_perf_read_write_transition,
+			"event=0x30");
+PMU_EVENT_ATTR_STRING(precharge, ddr_perf_precharge, "event=0x31");
+PMU_EVENT_ATTR_STRING(activate, ddr_perf_activate, "event=0x32");
+PMU_EVENT_ATTR_STRING(load-mode, ddr_perf_load_mode, "event=0x33");
+PMU_EVENT_ATTR_STRING(mwr, ddr_perf_mwr, "event=0x34");
+PMU_EVENT_ATTR_STRING(read, ddr_perf_read, "event=0x35");
+PMU_EVENT_ATTR_STRING(read-activate, ddr_perf_read_activate, "event=0x36");
+PMU_EVENT_ATTR_STRING(refresh, ddr_perf_refresh, "event=0x37");
+PMU_EVENT_ATTR_STRING(write, ddr_perf_write, "event=0x38");
+PMU_EVENT_ATTR_STRING(raw-hazard, ddr_perf_raw_hazard, "event=0x39");
+
+static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
+	{ .compatible = "fsl,imx8-ddr-pmu",},
+	{ .compatible = "fsl,imx8m-ddr-pmu",},
+	{ /* sentinel */ }
+};
+
+struct ddr_pmu {
+	struct pmu pmu;
+	void __iomem *base;
+	unsigned int cpu;
+	struct	hlist_node node;
+	struct	device *dev;
+	struct perf_event *events[NUM_COUNTERS];
+	int active_events;
+	enum cpuhp_state cpuhp_state;
+	int irq;
+	int id;
+};
+
+static ssize_t ddr_perf_cpumask_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct ddr_pmu *pmu = dev_get_drvdata(dev);
+
+	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu));
+}
+
+static struct device_attribute ddr_perf_cpumask_attr =
+	__ATTR(cpumask, 0444, ddr_perf_cpumask_show, NULL);
+
+static struct attribute *ddr_perf_cpumask_attrs[] = {
+	&ddr_perf_cpumask_attr.attr,
+	NULL,
+};
+
+static struct attribute_group ddr_perf_cpumask_attr_group = {
+	.attrs = ddr_perf_cpumask_attrs,
+};
+
+static struct attribute *ddr_perf_events_attrs[] = {
+	&ddr_perf_cycles.attr.attr,
+	&ddr_perf_selfresh.attr.attr,
+	&ddr_perf_read_accesses.attr.attr,
+	&ddr_perf_write_accesses.attr.attr,
+	&ddr_perf_read_queue_depth.attr.attr,
+	&ddr_perf_write_queue_depth.attr.attr,
+	&ddr_perf_lp_read_credit_cnt.attr.attr,
+	&ddr_perf_hp_read_credit_cnt.attr.attr,
+	&ddr_perf_write_credit_cnt.attr.attr,
+	&ddr_perf_read_command.attr.attr,
+	&ddr_perf_write_command.attr.attr,
+	&ddr_perf_read_modify_write_command.attr.attr,
+	&ddr_perf_hp_read.attr.attr,
+	&ddr_perf_hp_req_nocredit.attr.attr,
+	&ddr_perf_hp_xact_credit.attr.attr,
+	&ddr_perf_lp_req_nocredit.attr.attr,
+	&ddr_perf_lp_xact_credit.attr.attr,
+	&ddr_perf_wr_xact_credit.attr.attr,
+	&ddr_perf_read_cycles.attr.attr,
+	&ddr_perf_write_cycles.attr.attr,
+	&ddr_perf_read_write_transition.attr.attr,
+	&ddr_perf_precharge.attr.attr,
+	&ddr_perf_activate.attr.attr,
+	&ddr_perf_load_mode.attr.attr,
+	&ddr_perf_mwr.attr.attr,
+	&ddr_perf_read.attr.attr,
+	&ddr_perf_read_activate.attr.attr,
+	&ddr_perf_refresh.attr.attr,
+	&ddr_perf_write.attr.attr,
+	&ddr_perf_raw_hazard.attr.attr,
+	NULL,
+};
+
+static struct attribute_group ddr_perf_events_attr_group = {
+	.name = "events",
+	.attrs = ddr_perf_events_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-63");
+
+static struct attribute *ddr_perf_format_attrs[] = {
+	&format_attr_event.attr,
+	NULL,
+};
+
+static struct attribute_group ddr_perf_format_attr_group = {
+	.name = "format",
+	.attrs = ddr_perf_format_attrs,
+};
+
+static const struct attribute_group *attr_groups[] = {
+	&ddr_perf_events_attr_group,
+	&ddr_perf_format_attr_group,
+	&ddr_perf_cpumask_attr_group,
+	NULL,
+};
+
+static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
+{
+	int i;
+
+	/*
+	 * Always map cycle event to counter 0
+	 * Cycles counter is dedicated for cycle event
+	 * can't used for the other events
+	 */
+	if (event == EVENT_CYCLES_ID) {
+		if (pmu->events[EVENT_CYCLES_COUNTER] == NULL)
+			return EVENT_CYCLES_COUNTER;
+		else
+			return -ENOENT;
+	}
+
+	for (i = 1; i < NUM_COUNTERS; i++) {
+		if (pmu->events[i] == NULL)
+			return i;
+	}
+
+	return -ENOENT;
+}
+
+static u32 ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
+{
+	pmu->events[counter] = NULL;
+
+	return 0;
+}
+
+static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
+{
+	return readl(pmu->base + COUNTER_READ + counter * 4);
+}
+
+static int ddr_perf_event_init(struct perf_event *event)
+{
+	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	struct perf_event *sibling;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
+	if (event->cpu < 0) {
+		dev_warn(pmu->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;
+
+	/*
+	 * We must NOT create groups containing mixed PMUs, although software
+	 * events are acceptable (for example to create a CCN group
+	 * periodically read when a hrtimer aka cpu-clock leader triggers).
+	 */
+	if (event->group_leader->pmu != event->pmu &&
+			!is_software_event(event->group_leader))
+		return -EINVAL;
+
+	for_each_sibling_event(sibling, event->group_leader) {
+		if (sibling->pmu != event->pmu &&
+				!is_software_event(sibling))
+			return -EINVAL;
+	}
+
+	event->cpu = pmu->cpu;
+	hwc->idx = -1;
+
+	return 0;
+}
+
+
+static void ddr_perf_event_update(struct perf_event *event)
+{
+	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u64 delta, prev_raw_count, new_raw_count;
+	int counter = hwc->idx;
+
+	do {
+		prev_raw_count = local64_read(&hwc->prev_count);
+		new_raw_count = ddr_perf_read_counter(pmu, counter);
+	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
+			new_raw_count) != prev_raw_count);
+
+	delta = (new_raw_count - prev_raw_count) & 0xFFFFFFFF;
+
+	local64_add(delta, &event->count);
+}
+
+static void ddr_perf_event_enable(struct ddr_pmu *pmu, int config,
+				  int counter, bool enable)
+{
+	u8 reg = counter * 4 + COUNTER_CNTL;
+	int val;
+
+	if (enable) {
+		/*
+		 * must disable first, then enable again
+		 * otherwise, cycle counter will not work
+		 * if previous state is enabled.
+		 */
+		writel(0, pmu->base + reg);
+		val = CNTL_EN | CNTL_CLEAR;
+		val |= (config << CNTL_CSV_SHIFT) & CNTL_CSV_MASK;
+	} else {
+		/* Disable counter */
+		val = readl(pmu->base + reg) & CNTL_EN_MASK;
+	}
+
+	writel(val, pmu->base + reg);
+}
+
+static void ddr_perf_event_start(struct perf_event *event, int flags)
+{
+	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter = hwc->idx;
+
+	local64_set(&hwc->prev_count, 0);
+
+	ddr_perf_event_enable(pmu, event->attr.config, counter, true);
+}
+
+static int ddr_perf_event_add(struct perf_event *event, int flags)
+{
+	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter;
+	int cfg = event->attr.config;
+
+	counter = ddr_perf_alloc_counter(pmu, cfg);
+	if (counter < 0) {
+		dev_dbg(pmu->dev, "There are not enough counters\n");
+		return -EOPNOTSUPP;
+	}
+
+	pmu->events[counter] = event;
+	pmu->active_events++;
+	hwc->idx = counter;
+
+	if (flags & PERF_EF_START)
+		ddr_perf_event_start(event, flags);
+
+	local64_set(&hwc->prev_count, ddr_perf_read_counter(pmu, counter));
+
+	return 0;
+}
+
+static void ddr_perf_event_stop(struct perf_event *event, int flags)
+{
+	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter = hwc->idx;
+
+	ddr_perf_event_enable(pmu, event->attr.config, counter, false);
+	ddr_perf_event_update(event);
+}
+
+static void ddr_perf_event_del(struct perf_event *event, int flags)
+{
+	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter = hwc->idx;
+
+	ddr_perf_event_stop(event, PERF_EF_UPDATE);
+
+	ddr_perf_free_counter(pmu, counter);
+	pmu->active_events--;
+	hwc->idx = -1;
+}
+
+static void ddr_perf_pmu_enable(struct pmu *pmu)
+{
+	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
+
+	/* enable cycle counter if cycle is not active event list */
+	if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
+		ddr_perf_event_enable(ddr_pmu,
+				      EVENT_CYCLES_ID,
+				      EVENT_CYCLES_COUNTER,
+				      true);
+}
+
+static void ddr_perf_pmu_disable(struct pmu *pmu)
+{
+	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
+
+	if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
+		ddr_perf_event_enable(ddr_pmu,
+				      EVENT_CYCLES_ID,
+				      EVENT_CYCLES_COUNTER,
+				      false);
+}
+
+static int ddr_perf_init(struct ddr_pmu *pmu, void __iomem *base,
+			 struct device *dev)
+{
+	*pmu = (struct ddr_pmu) {
+		.pmu = (struct pmu) {
+			.task_ctx_nr = perf_invalid_context,
+			.attr_groups = attr_groups,
+			.event_init  = ddr_perf_event_init,
+			.add	     = ddr_perf_event_add,
+			.del	     = ddr_perf_event_del,
+			.start	     = ddr_perf_event_start,
+			.stop	     = ddr_perf_event_stop,
+			.read	     = ddr_perf_event_update,
+			.pmu_enable  = ddr_perf_pmu_enable,
+			.pmu_disable = ddr_perf_pmu_disable,
+		},
+		.base = base,
+		.dev = dev,
+	};
+
+	pmu->id = ida_simple_get(&ddr_ida, 0, 0, GFP_KERNEL);
+	return pmu->id;
+}
+
+static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
+{
+	int i;
+	struct ddr_pmu *pmu = (struct ddr_pmu *) p;
+	struct perf_event *event, *cycle_event = NULL;
+
+	/* all counter will stop if cycle counter disabled */
+	ddr_perf_event_enable(pmu,
+			      EVENT_CYCLES_ID,
+			      EVENT_CYCLES_COUNTER,
+			      false);
+	/*
+	 * When the cycle counter overflows, all counters are stopped,
+	 * and an IRQ is raised. If any other counter overflows, it
+	 * continues counting, and no IRQ is raised.
+	 *
+	 * Cycles occur at least 4 times as often as other events, so we
+	 * can update all events on a cycle counter overflow and not
+	 * lose events.
+	 *
+	 */
+	for (i = 0; i < NUM_COUNTERS; i++) {
+
+		if (!pmu->events[i])
+			continue;
+
+		event = pmu->events[i];
+
+		ddr_perf_event_update(event);
+
+		if (event->hw.idx == EVENT_CYCLES_COUNTER)
+			cycle_event = event;
+	}
+
+	ddr_perf_event_enable(pmu,
+			      EVENT_CYCLES_ID,
+			      EVENT_CYCLES_COUNTER,
+			      true);
+	if (cycle_event)
+		ddr_perf_event_update(cycle_event);
+
+	return IRQ_HANDLED;
+}
+
+static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+	struct ddr_pmu *pmu = hlist_entry_safe(node, struct ddr_pmu, node);
+	int target;
+
+	if (cpu != pmu->cpu)
+		return 0;
+
+	target = cpumask_any_but(cpu_online_mask, cpu);
+	if (target >= nr_cpu_ids)
+		return 0;
+
+	perf_pmu_migrate_context(&pmu->pmu, cpu, target);
+	pmu->cpu = target;
+
+	WARN_ON(irq_set_affinity_hint(pmu->irq, cpumask_of(pmu->cpu)));
+
+	return 0;
+}
+
+static int ddr_perf_probe(struct platform_device *pdev)
+{
+	struct ddr_pmu *pmu;
+	struct device_node *np;
+	void __iomem *base;
+	struct resource *iomem;
+	char *name;
+	char *hpname;
+	int num;
+	int ret;
+	int irq;
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	np = pdev->dev.of_node;
+
+	pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	num = ddr_perf_init(pmu, base, &pdev->dev);
+
+	platform_set_drvdata(pdev, pmu);
+
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
+	if (!name)
+		return -ENOMEM;
+
+	hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+				"perf/imx/ddr%d:online", num);
+	if (!hpname)
+		return -ENOMEM;
+
+	pmu->cpu = raw_smp_processor_id();
+	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
+					 ddr_perf_offline_cpu);
+
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
+		goto ddr_perf_err;
+	}
+
+	pmu->cpuhp_state = ret;
+
+	/* Register the pmu instance for cpu hotplug */
+	cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
+
+	ret = perf_pmu_register(&pmu->pmu, name, -1);
+	if (ret)
+		goto ddr_perf_err;
+
+	/* Request irq */
+	irq = of_irq_get(np, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Failed to get irq: %d", irq);
+		ret = irq;
+		goto ddr_perf_irq_err;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq,
+					ddr_perf_irq_handler,
+					IRQF_NOBALANCING | IRQF_NO_THREAD,
+					DDR_PERF_DEV_NAME,
+					pmu);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Request irq failed: %d", ret);
+		goto ddr_perf_irq_err;
+	}
+
+	pmu->irq = irq;
+	ret = irq_set_affinity_hint(pmu->irq, cpumask_of(pmu->cpu));
+	if (ret) {
+		dev_err(pmu->dev, "Failed to set interrupt affinity!\n");
+		goto ddr_perf_irq_err;
+	}
+
+	return 0;
+
+ddr_perf_irq_err:
+	perf_pmu_unregister(&(pmu->pmu));
+
+ddr_perf_err:
+	if (pmu->cpuhp_state)
+		cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
+
+	ida_simple_remove(&ddr_ida, pmu->id);
+	dev_warn(&pdev->dev, "i.MX8 DDR Perf PMU failed (%d), disabled\n", ret);
+	return ret;
+}
+
+static int ddr_perf_remove(struct platform_device *pdev)
+{
+	struct ddr_pmu *pmu = platform_get_drvdata(pdev);
+
+	cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
+	irq_set_affinity_hint(pmu->irq, NULL);
+
+	perf_pmu_unregister(&pmu->pmu);
+
+	ida_simple_remove(&ddr_ida, pmu->id);
+	return 0;
+}
+
+static struct platform_driver imx_ddr_pmu_driver = {
+	.driver         = {
+		.name   = "imx-ddr-pmu",
+		.of_match_table = imx_ddr_pmu_dt_ids,
+	},
+	.probe          = ddr_perf_probe,
+	.remove         = ddr_perf_remove,
+};
+
+module_platform_driver(imx_ddr_pmu_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.5.2

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter support

[Will Deacon]

On Mon, Apr 29, 2019 at 04:44:32PM +0000, Frank Li wrote:
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> new file mode 100644
> index 0000000..087d75a
> --- /dev/null
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -0,0 +1,589 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2017 NXP
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +
> +#define COUNTER_CNTL		0x0
> +#define COUNTER_READ		0x20
> +
> +#define COUNTER_DPCR1		0x30
> +
> +#define CNTL_OVER		0x1
> +#define CNTL_CLEAR		0x2
> +#define CNTL_EN			0x4
> +#define CNTL_EN_MASK		0xFFFFFFFB
> +#define CNTL_CLEAR_MASK		0xFFFFFFFD
> +#define CNTL_OVER_MASK		0xFFFFFFFE
> +
> +#define CNTL_CSV_SHIFT		24
> +#define CNTL_CSV_MASK		(0xFF << CNTL_CSV_SHIFT)
> +
> +#define EVENT_CYCLES_ID		0
> +#define EVENT_CYCLES_COUNTER	0
> +#define NUM_COUNTERS		4
> +
> +#define to_ddr_pmu(p)		container_of(p, struct ddr_pmu, pmu)
> +
> +#define DDR_PERF_DEV_NAME	"ddr_perf"

This is far too generic. Please make it something like "imx8_ddr_perf_pmu".

> +static int ddr_perf_probe(struct platform_device *pdev)
> +{
> +	struct ddr_pmu *pmu;
> +	struct device_node *np;
> +	void __iomem *base;
> +	struct resource *iomem;
> +	char *name;
> +	char *hpname;
> +	int num;
> +	int ret;
> +	int irq;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	np = pdev->dev.of_node;
> +
> +	pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	num = ddr_perf_init(pmu, base, &pdev->dev);
> +
> +	platform_set_drvdata(pdev, pmu);
> +
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +				"perf/imx/ddr%d:online", num);
> +	if (!hpname)
> +		return -ENOMEM;
> +
> +	pmu->cpu = raw_smp_processor_id();
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> +					 ddr_perf_offline_cpu);

This doesn't seem right to me. My understanding of the cpuhp mechanism
is that you register a single multi-instance state as part of driver
initialisation, and then add an instance for each device you probe.
That means you don't need to kasprintf the callback name as you are above --
you can just use DDR_PERF_DEV_NAME instead.

Please see how other perf drivers manage this on my for-next/perf branch.

> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> +		goto ddr_perf_err;
> +	}
> +
> +	pmu->cpuhp_state = ret;
> +
> +	/* Register the pmu instance for cpu hotplug */
> +	cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +
> +	ret = perf_pmu_register(&pmu->pmu, name, -1);

Again, the string you're passing in here is too generic. I suggest taking
DDR_PERF_DEV_NAME and adding "_%d" to the end to paste in your 'num'
identifier.

Sorry if this feels like pedantry, but this gets exposed to userspace
via sysfs, so we need to be careful with the namespace.

Will

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter support

[Zhi Li]

On Tue, Apr 30, 2019 at 5:51 AM Will Deacon <will.deacon@arm.com> wrote:
>
> On Mon, Apr 29, 2019 at 04:44:32PM +0000, Frank Li wrote:
> > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> > new file mode 100644
> > index 0000000..087d75a
> > --- /dev/null
> > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > @@ -0,0 +1,589 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2017 NXP
> > + * Copyright 2016 Freescale Semiconductor, Inc.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/slab.h>
> > +
> > +#define COUNTER_CNTL         0x0
> > +#define COUNTER_READ         0x20
> > +
> > +#define COUNTER_DPCR1                0x30
> > +
> > +#define CNTL_OVER            0x1
> > +#define CNTL_CLEAR           0x2
> > +#define CNTL_EN                      0x4
> > +#define CNTL_EN_MASK         0xFFFFFFFB
> > +#define CNTL_CLEAR_MASK              0xFFFFFFFD
> > +#define CNTL_OVER_MASK               0xFFFFFFFE
> > +
> > +#define CNTL_CSV_SHIFT               24
> > +#define CNTL_CSV_MASK                (0xFF << CNTL_CSV_SHIFT)
> > +
> > +#define EVENT_CYCLES_ID              0
> > +#define EVENT_CYCLES_COUNTER 0
> > +#define NUM_COUNTERS         4
> > +
> > +#define to_ddr_pmu(p)                container_of(p, struct ddr_pmu, pmu)
> > +
> > +#define DDR_PERF_DEV_NAME    "ddr_perf"
>
> This is far too generic. Please make it something like "imx8_ddr_perf_pmu".
>
> > +static int ddr_perf_probe(struct platform_device *pdev)
> > +{
> > +     struct ddr_pmu *pmu;
> > +     struct device_node *np;
> > +     void __iomem *base;
> > +     struct resource *iomem;
> > +     char *name;
> > +     char *hpname;
> > +     int num;
> > +     int ret;
> > +     int irq;
> > +
> > +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(&pdev->dev, iomem);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     np = pdev->dev.of_node;
> > +
> > +     pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> > +     if (!pmu)
> > +             return -ENOMEM;
> > +
> > +     num = ddr_perf_init(pmu, base, &pdev->dev);
> > +
> > +     platform_set_drvdata(pdev, pmu);
> > +
> > +     name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> > +     if (!name)
> > +             return -ENOMEM;
> > +
> > +     hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +                             "perf/imx/ddr%d:online", num);
> > +     if (!hpname)
> > +             return -ENOMEM;
> > +
> > +     pmu->cpu = raw_smp_processor_id();
> > +     ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> > +                                      ddr_perf_offline_cpu);
>
> This doesn't seem right to me. My understanding of the cpuhp mechanism
> is that you register a single multi-instance state as part of driver
> initialisation, and then add an instance for each device you probe.
> That means you don't need to kasprintf the callback name as you are above --
> you can just use DDR_PERF_DEV_NAME instead.
>
> Please see how other perf drivers manage this on my for-next/perf branch.
>
> > +
> > +     if (ret < 0) {
> > +             dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> > +             goto ddr_perf_err;
> > +     }
> > +
> > +     pmu->cpuhp_state = ret;
> > +
> > +     /* Register the pmu instance for cpu hotplug */
> > +     cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> > +
> > +     ret = perf_pmu_register(&pmu->pmu, name, -1);
>
> Again, the string you're passing in here is too generic. I suggest taking
> DDR_PERF_DEV_NAME and adding "_%d" to the end to paste in your 'num'
> identifier.
>
> Sorry if this feels like pedantry, but this gets exposed to userspace
> via sysfs, so we need to be careful with the namespace.

imx8_ddr_perf_pmu is too long to use.  how about imx_ddr%d?

best regards
Frank Li

>
> Will

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter support

[Will Deacon]

On Tue, Apr 30, 2019 at 11:30:17AM -0500, Zhi Li wrote:
> On Tue, Apr 30, 2019 at 5:51 AM Will Deacon <will.deacon@arm.com> wrote:
> >
> > On Mon, Apr 29, 2019 at 04:44:32PM +0000, Frank Li wrote:
> > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> > > new file mode 100644
> > > index 0000000..087d75a
> > > --- /dev/null
> > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > > @@ -0,0 +1,589 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2017 NXP
> > > + * Copyright 2016 Freescale Semiconductor, Inc.
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/perf_event.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#define COUNTER_CNTL         0x0
> > > +#define COUNTER_READ         0x20
> > > +
> > > +#define COUNTER_DPCR1                0x30
> > > +
> > > +#define CNTL_OVER            0x1
> > > +#define CNTL_CLEAR           0x2
> > > +#define CNTL_EN                      0x4
> > > +#define CNTL_EN_MASK         0xFFFFFFFB
> > > +#define CNTL_CLEAR_MASK              0xFFFFFFFD
> > > +#define CNTL_OVER_MASK               0xFFFFFFFE
> > > +
> > > +#define CNTL_CSV_SHIFT               24
> > > +#define CNTL_CSV_MASK                (0xFF << CNTL_CSV_SHIFT)
> > > +
> > > +#define EVENT_CYCLES_ID              0
> > > +#define EVENT_CYCLES_COUNTER 0
> > > +#define NUM_COUNTERS         4
> > > +
> > > +#define to_ddr_pmu(p)                container_of(p, struct ddr_pmu, pmu)
> > > +
> > > +#define DDR_PERF_DEV_NAME    "ddr_perf"
> >
> > This is far too generic. Please make it something like "imx8_ddr_perf_pmu".
> >
> > > +static int ddr_perf_probe(struct platform_device *pdev)
> > > +{
> > > +     struct ddr_pmu *pmu;
> > > +     struct device_node *np;
> > > +     void __iomem *base;
> > > +     struct resource *iomem;
> > > +     char *name;
> > > +     char *hpname;
> > > +     int num;
> > > +     int ret;
> > > +     int irq;
> > > +
> > > +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     base = devm_ioremap_resource(&pdev->dev, iomem);
> > > +     if (IS_ERR(base))
> > > +             return PTR_ERR(base);
> > > +
> > > +     np = pdev->dev.of_node;
> > > +
> > > +     pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> > > +     if (!pmu)
> > > +             return -ENOMEM;
> > > +
> > > +     num = ddr_perf_init(pmu, base, &pdev->dev);
> > > +
> > > +     platform_set_drvdata(pdev, pmu);
> > > +
> > > +     name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> > > +     if (!name)
> > > +             return -ENOMEM;
> > > +
> > > +     hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > > +                             "perf/imx/ddr%d:online", num);
> > > +     if (!hpname)
> > > +             return -ENOMEM;
> > > +
> > > +     pmu->cpu = raw_smp_processor_id();
> > > +     ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> > > +                                      ddr_perf_offline_cpu);
> >
> > This doesn't seem right to me. My understanding of the cpuhp mechanism
> > is that you register a single multi-instance state as part of driver
> > initialisation, and then add an instance for each device you probe.
> > That means you don't need to kasprintf the callback name as you are above --
> > you can just use DDR_PERF_DEV_NAME instead.
> >
> > Please see how other perf drivers manage this on my for-next/perf branch.
> >
> > > +
> > > +     if (ret < 0) {
> > > +             dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> > > +             goto ddr_perf_err;
> > > +     }
> > > +
> > > +     pmu->cpuhp_state = ret;
> > > +
> > > +     /* Register the pmu instance for cpu hotplug */
> > > +     cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> > > +
> > > +     ret = perf_pmu_register(&pmu->pmu, name, -1);
> >
> > Again, the string you're passing in here is too generic. I suggest taking
> > DDR_PERF_DEV_NAME and adding "_%d" to the end to paste in your 'num'
> > identifier.
> >
> > Sorry if this feels like pedantry, but this gets exposed to userspace
> > via sysfs, so we need to be careful with the namespace.
> 
> imx8_ddr_perf_pmu is too long to use.  how about imx_ddr%d?

Too long for what? Drop the "perf" bit if you want, but I think we need
"pmu" in there.

Will

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter

[Andrey Smirnov]

> Add ddr performance monitor support for iMX8QXP

> There are 4 counters for ddr perfomance events.
> counter 0 is dedicated for cycles.
> you choose any up to 3 no cycles events.

> for example:

> perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
> perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls

> Support below events.

>   ddr0/activate/                                     [Kernel PMU event]
>   ddr0/axid-read/                                    [Kernel PMU event]
>   ddr0/axid-write/                                   [Kernel PMU event]
>   ddr0/cycles/                                       [Kernel PMU event]
>   ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
>   ddr0/hp-read/                                      [Kernel PMU event]
>   ddr0/hp-req-nodcredit/                             [Kernel PMU event]
>   ddr0/hp-xact-credit/                               [Kernel PMU event]
>   ddr0/load-mode/                                    [Kernel PMU event]
>   ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
>   ddr0/lp-req-nocredit/                              [Kernel PMU event]
>   ddr0/lp-xact-credit/                               [Kernel PMU event]
>   ddr0/mwr/                                          [Kernel PMU event]
>   ddr0/precharge/                                    [Kernel PMU event]
>   ddr0/raw-hazard/                                   [Kernel PMU event]
>   ddr0/read-access/                                  [Kernel PMU event]
>   ddr0/read-activate/                                [Kernel PMU event]
>   ddr0/read-command/                                 [Kernel PMU event]
>   ddr0/read-cycles/                                  [Kernel PMU event]
>   ddr0/read-modify-write-command/                    [Kernel PMU event]
>   ddr0/read-queue-depth/                             [Kernel PMU event]
>   ddr0/read-write-transition/                        [Kernel PMU event]
>   ddr0/read/                                         [Kernel PMU event]
>   ddr0/refresh/                                      [Kernel PMU event]
>   ddr0/selfresh/                                     [Kernel PMU event]
>   ddr0/wr-xact-credit/                               [Kernel PMU event]
>   ddr0/write-access/                                 [Kernel PMU event]
>   ddr0/write-command/                                [Kernel PMU event]
>   ddr0/write-credit-cnt/                             [Kernel PMU event]
>   ddr0/write-cycles/                                 [Kernel PMU event]
>   ddr0/write-queue-depth/                            [Kernel PMU event]
>   ddr0/write/

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> No change from V8 to V9

> Change from v7 to v8
>  * remove unused define
>  * change total_events to active_events, change active_events to events
>  * remove flags, 
>  * fix multi line comments code sytle
>  * add pmu_enable\disable function
>  * disable event at irq handle
>  * remove counter check at ddr_perf_free_counter
>  * remove pmu->irq check
>  * add group check

> Change from v6 to v7
>  * added irq affinity handle, ref arm-ccn.c
>  * added IRQF_NOBALANCING | IRQF_NO_THREAD
>  * added ida_simple_remove at failure path

> Change from v5 to v6
>  * fix insmod\rmmod problem
>  * remove randunt register read at irq handle
>  * change u32 irq to int
>  * devm_request_irq use default flags.

> Change from v4 to v5
>  * Remove AXI ID filter function

> Change from v3 to v4
>  * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
>  * sort include
>  * remove struct fsl_ddr_devtype_data
>  * Added comment need disable control first
>  * Added comment about must enable cycle counter
>  * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
>  * Added comment about cycle counter is fastest one

> Change from v2 to v3
>  * remove kfree

> Change from V1 to V2
>  * update Kconfig by use i.MX8 instead of i.MX8 QXP
>  * remove gpl statememnt since SPDX tag
>  * use dev_kzalloc
>  * use dev_err
>  * commit message show axi_read 0x41\axi_write 0x42
>  * commit message show cycles must be enabled
>  * Irq only issue at cycles overflow
>  * use NUM_COUNTER
>  * use devm_request_irq
>  * add hotplug callback to handle context migration

Hey Frank,

I missed your effort to upstream this and ended up spening some time
working on the same thing in parallel, so I have some comments below.

> +PMU_EVENT_ATTR_STRING(cycles, ddr_perf_cycles, "event=0x00");
> +PMU_EVENT_ATTR_STRING(selfresh, ddr_perf_selfresh, "event=0x01");
> +PMU_EVENT_ATTR_STRING(read-access, ddr_perf_read_accesses, "event=0x04");
> +PMU_EVENT_ATTR_STRING(write-access, ddr_perf_write_accesses, "event=0x05");
> +PMU_EVENT_ATTR_STRING(read-queue-depth, ddr_perf_read_queue_depth,
> +			"event=0x08");
> +PMU_EVENT_ATTR_STRING(write-queue-depth, ddr_perf_write_queue_depth,
> +			"event=0x09");
> +PMU_EVENT_ATTR_STRING(lp-read-credit-cnt, ddr_perf_lp_read_credit_cnt,
> +			"event=0x10");
> +PMU_EVENT_ATTR_STRING(hp-read-credit-cnt, ddr_perf_hp_read_credit_cnt,
> +			"event=0x11");
> +PMU_EVENT_ATTR_STRING(write-credit-cnt, ddr_perf_write_credit_cnt,
> +			"event=0x12");
> +PMU_EVENT_ATTR_STRING(read-command, ddr_perf_read_command, "event=0x20");
> +PMU_EVENT_ATTR_STRING(write-command, ddr_perf_write_command, "event=0x21");
> +PMU_EVENT_ATTR_STRING(read-modify-write-command,
> +		ddr_perf_read_modify_write_command, "event=0x22");
> +PMU_EVENT_ATTR_STRING(hp-read, ddr_perf_hp_read, "event=0x23");
> +PMU_EVENT_ATTR_STRING(hp-req-nodcredit, ddr_perf_hp_req_nocredit, "event=0x24");
> +PMU_EVENT_ATTR_STRING(hp-xact-credit, ddr_perf_hp_xact_credit, "event=0x25");
> +PMU_EVENT_ATTR_STRING(lp-req-nocredit, ddr_perf_lp_req_nocredit, "event=0x26");
> +PMU_EVENT_ATTR_STRING(lp-xact-credit, ddr_perf_lp_xact_credit, "event=0x27");
> +PMU_EVENT_ATTR_STRING(wr-xact-credit, ddr_perf_wr_xact_credit, "event=0x29");
> +PMU_EVENT_ATTR_STRING(read-cycles, ddr_perf_read_cycles, "event=0x2a");
> +PMU_EVENT_ATTR_STRING(write-cycles, ddr_perf_write_cycles, "event=0x2b");
> +PMU_EVENT_ATTR_STRING(read-write-transition, ddr_perf_read_write_transition,
> +			"event=0x30");
> +PMU_EVENT_ATTR_STRING(precharge, ddr_perf_precharge, "event=0x31");
> +PMU_EVENT_ATTR_STRING(activate, ddr_perf_activate, "event=0x32");
> +PMU_EVENT_ATTR_STRING(load-mode, ddr_perf_load_mode, "event=0x33");
> +PMU_EVENT_ATTR_STRING(mwr, ddr_perf_mwr, "event=0x34");
> +PMU_EVENT_ATTR_STRING(read, ddr_perf_read, "event=0x35");
> +PMU_EVENT_ATTR_STRING(read-activate, ddr_perf_read_activate, "event=0x36");
> +PMU_EVENT_ATTR_STRING(refresh, ddr_perf_refresh, "event=0x37");
> +PMU_EVENT_ATTR_STRING(write, ddr_perf_write, "event=0x38");
> +PMU_EVENT_ATTR_STRING(raw-hazard, ddr_perf_raw_hazard, "event=0x39");

You can really save quite a bit of boilerplate if you define those
inplace with a custom macro and a custom show function:

static ssize_t
ddr_pmu_event_show(struct device *dev, struct device_attribute *attr,
		   char *page)
{
	struct perf_pmu_events_attr *pmu_attr;

	pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
	return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
}

#define IMX8_DDR_PMU_EVENT_ATTR(_name, _id)				\
	(&((struct perf_pmu_events_attr[]) {				\
		{ .attr = __ATTR(_name, 0444, ddr_pmu_event_show, NULL), \
		  .id = _id, }						\
	})[0].attr.attr)

static struct attribute *ddr_pmu_events_attrs[] = {
	IMX8_DDR_PMU_EVENT_ATTR(cycles,		     CYCLES_EVENT_ID),
	IMX8_DDR_PMU_EVENT_ATTR(selfresh,			0x01),
	IMX8_DDR_PMU_EVENT_ATTR(read-access,			0x04),
	IMX8_DDR_PMU_EVENT_ATTR(write-access,			0x05),
	IMX8_DDR_PMU_EVENT_ATTR(read-queue-depth,		0x08),

...

> +
> +struct ddr_pmu {
> +	struct pmu pmu;
> +	void __iomem *base;
> +	unsigned int cpu;
> +	struct	hlist_node node;
> +	struct	device *dev;

This device pointer is used only once in ddr_perf_event_init() and
even in that function not all error cases get a dedicated kernel
message. I'd consider just dropping it and the message it is used in.

> +	struct perf_event *events[NUM_COUNTERS];
> +	int active_events;

I'd very strongly encourage you to convert the driver to use a proper
bitmask instead of this counter. E.g:

DECLARE_BITMAP(active_mask, NUM_COUNTERS);

I'll comment more on that below.

> +	enum cpuhp_state cpuhp_state;
> +	int irq;
> +	int id;
> +};

> +static struct attribute_group ddr_perf_events_attr_group = {
> +	.name = "events",
> +	.attrs = ddr_perf_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-63");
> +

Event ID is really only 8-bits wide, AFAIK. Is there any reason to
reserve all 64 for it in config?

> +static struct attribute *ddr_perf_format_attrs[] = {
> +	&format_attr_event.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group ddr_perf_format_attr_group = {
> +	.name = "format",
> +	.attrs = ddr_perf_format_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> +	&ddr_perf_events_attr_group,
> +	&ddr_perf_format_attr_group,
> +	&ddr_perf_cpumask_attr_group,
> +	NULL,
> +};
> +
> +static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
> +{
> +	int i;
> +
> +	/*
> +	 * Always map cycle event to counter 0
> +	 * Cycles counter is dedicated for cycle event
> +	 * can't used for the other events
> +	 */
> +	if (event == EVENT_CYCLES_ID) {
> +		if (pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> +			return EVENT_CYCLES_COUNTER;
> +		else
> +			return -ENOENT;
> +	}
> +
> +	for (i = 1; i < NUM_COUNTERS; i++) {
> +		if (pmu->events[i] == NULL)
> +			return i;
> +	}

This is the first place where using a bitmap would simplify the
driver. Here all you'd need to do is:


if (event == CYCLES_EVENT_ID) {
   ...	   
} else {
  i =  find_next_zero_bit(pmu->active_mask, NUM_COUNTERS,
  		          EVENT_CYCLES_COUNTER + 1);
  if (i == NUM_COUNTERS)
	return -EAGAIN;

}

set_bit(i, pmu->active_mask);

...

> +
> +	return -ENOENT;
> +}
> +
> +static u32 ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
> +{
> +	pmu->events[counter] = NULL;
> +
> +	return 0;
> +}

Is this function even necessary? It is used only once in the code and
it's return value is ignored

> +
> +static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
> +{
> +	return readl(pmu->base + COUNTER_READ + counter * 4);
> +}

Would using readl_relaxed() be beneficial here?

> +
> +static int ddr_perf_event_init(struct perf_event *event)
> +{
> +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct perf_event *sibling;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> +		return -EOPNOTSUPP;
> +
> +	if (event->cpu < 0) {
> +		dev_warn(pmu->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       ||

You don't need any of the above if you pass:

.capabilities = PERF_PMU_CAP_NO_EXCLUDE

in your struct pmu initialization

> +	    event->attr.sample_period)
> +		return -EINVAL;
> +
> +	/*
> +	 * We must NOT create groups containing mixed PMUs, although software
> +	 * events are acceptable (for example to create a CCN group
> +	 * periodically read when a hrtimer aka cpu-clock leader triggers).
> +	 */
> +	if (event->group_leader->pmu != event->pmu &&
> +			!is_software_event(event->group_leader))
> +		return -EINVAL;
> +
> +	for_each_sibling_event(sibling, event->group_leader) {
> +		if (sibling->pmu != event->pmu &&
> +				!is_software_event(sibling))
> +			return -EINVAL;
> +	}
> +
> +	event->cpu = pmu->cpu;
> +	hwc->idx = -1;
> +
> +	return 0;
> +}

> +
> +static void ddr_perf_event_enable(struct ddr_pmu *pmu, int config,
> +				  int counter, bool enable)
> +{

This function doesn't really have anything to do with events (unlike,
for example, ddr_perf_event_start() below). Maybe it would be better
to rename it to ddr_perf_counter_enable()?

> +	u8 reg = counter * 4 + COUNTER_CNTL;
> +	int val;
> +
> +	if (enable) {
> +		/*
> +		 * must disable first, then enable again
> +		 * otherwise, cycle counter will not work
> +		 * if previous state is enabled.
> +		 */
> +		writel(0, pmu->base + reg);
> +		val = CNTL_EN | CNTL_CLEAR;
> +		val |= (config << CNTL_CSV_SHIFT) & CNTL_CSV_MASK;

You can simplify the above with FIELD_PREP(CNTL_CSV_MASK, config)

> +	} else {
> +		/* Disable counter */
> +		val = readl(pmu->base + reg) & CNTL_EN_MASK;

This register will be blown away by the code in the other if branch
next time the counter is enabled. What's the point of doing
read-modify-write and trying to preserve all but EN bits as they were here?

Why not just do:

writel(CNTL_CLEAR, pmu->base + reg);

?

> +	}
> +
> +	writel(val, pmu->base + reg);
> +}
> +
> +static void ddr_perf_event_start(struct perf_event *event, int flags)
> +{
> +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	local64_set(&hwc->prev_count, 0);
> +
> +	ddr_perf_event_enable(pmu, event->attr.config, counter, true);
> +}
> +
> +static int ddr_perf_event_add(struct perf_event *event, int flags)
> +{
> +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter;
> +	int cfg = event->attr.config;
> +
> +	counter = ddr_perf_alloc_counter(pmu, cfg);
> +	if (counter < 0) {
> +		dev_dbg(pmu->dev, "There are not enough counters\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	pmu->events[counter] = event;
> +	pmu->active_events++;
> +	hwc->idx = counter;

What about hw->state?

> +
> +	if (flags & PERF_EF_START)
> +		ddr_perf_event_start(event, flags);
> +
> +	local64_set(&hwc->prev_count, ddr_perf_read_counter(pmu, counter));

What's this local64_set() for? You already clear prev_counter and HW
counter when event is started. This just seems redundant.

> +
> +	return 0;
> +}
> +
> +static void ddr_perf_event_stop(struct perf_event *event, int flags)
> +{
> +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	ddr_perf_event_enable(pmu, event->attr.config, counter, false);
> +	ddr_perf_event_update(event);

hw->state isn't updated here either? I'm no expert on perf subsystem,
so maybe it's OK, but most of the other drivers in this category do

hw->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;

> +}
> +
> +static void ddr_perf_event_del(struct perf_event *event, int flags)
> +{
> +	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	ddr_perf_event_stop(event, PERF_EF_UPDATE);
> +
> +	ddr_perf_free_counter(pmu, counter);
> +	pmu->active_events--;
> +	hwc->idx = -1;
> +}
> +
> +static void ddr_perf_pmu_enable(struct pmu *pmu)
> +{
> +	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> +
> +	/* enable cycle counter if cycle is not active event list */
> +	if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> +		ddr_perf_event_enable(ddr_pmu,
> +				      EVENT_CYCLES_ID,
> +				      EVENT_CYCLES_COUNTER,
> +				      true);
> +}
> +
> +static void ddr_perf_pmu_disable(struct pmu *pmu)
> +{
> +	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> +
> +	if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> +		ddr_perf_event_enable(ddr_pmu,
> +				      EVENT_CYCLES_ID,
> +				      EVENT_CYCLES_COUNTER,
> +				      false);
> +}

It seems that both of those functions will enable/disable cycles
counter multiple times if "perf" is called with a list specifying
multiple events but not cycles counter. Not sure if this is
intentional, in case it is not, using a bitmask would allow you to
avoid this by using bitmap_weight(), e.g:

if (bitmap_weight(pmu->active_mask, NUM_COUNTERS) == 1 &&
    ...

> +
> +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> +{
> +	int i;
> +	struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> +	struct perf_event *event, *cycle_event = NULL;
> +
> +	/* all counter will stop if cycle counter disabled */
> +	ddr_perf_event_enable(pmu,
> +			      EVENT_CYCLES_ID,
> +			      EVENT_CYCLES_COUNTER,
> +			      false);

The comment below says that IRQ is only raised when cycles counter
overflow and when that happens all of the counters are stopped. What's
the goal of the code disabling cycles counter above then?

> +	/*
> +	 * When the cycle counter overflows, all counters are stopped,
> +	 * and an IRQ is raised. If any other counter overflows, it
> +	 * continues counting, and no IRQ is raised.
> +	 *
> +	 * Cycles occur at least 4 times as often as other events, so we
> +	 * can update all events on a cycle counter overflow and not
> +	 * lose events.
> +	 *
> +	 */
> +	for (i = 0; i < NUM_COUNTERS; i++) {
> +
> +		if (!pmu->events[i])
> +			continue;
> +
> +		event = pmu->events[i];
> +
> +		ddr_perf_event_update(event);

If you already reading this counter out, why not clear its value to 0
to avoid having it overflow while at it?

> +
> +		if (event->hw.idx == EVENT_CYCLES_COUNTER)
> +			cycle_event = event;
> +	}
> +
> +	ddr_perf_event_enable(pmu,
> +			      EVENT_CYCLES_ID,
> +			      EVENT_CYCLES_COUNTER,
> +			      true);
> +	if (cycle_event)
> +		ddr_perf_event_update(cycle_event);

Using a bitmaks would allow you to simplify the above to:

i = EVENT_CYCLES_COUNTER + 1;
for_each_set_bit_from(i, pmu->active_mask, NUM_COUNTERS)
   ddr_perf_event_update(pmu->active_events[i]);

ddr_perf_event_enable(pmu,
		      EVENT_CYCLES_ID,
		      EVENT_CYCLES_COUNTER,
		      true);

if (test_bit(EVENT_CYCLES_COUNTER, pmu->active_mask))
    ddr_perf_event_update(pmu->active_events[EVENT_CYCLES_COUNTER]);

> +
> +	return IRQ_HANDLED;
> +}

> +
> +static int ddr_perf_probe(struct platform_device *pdev)
> +{
> +	struct ddr_pmu *pmu;
> +	struct device_node *np;
> +	void __iomem *base;
> +	struct resource *iomem;
> +	char *name;
> +	char *hpname;
> +	int num;
> +	int ret;
> +	int irq;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	np = pdev->dev.of_node;
> +
> +	pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	num = ddr_perf_init(pmu, base, &pdev->dev);

There's already a unique ID availible - IP block's physical address
(iomem->start). Why not use that instead of setting up an IDA?

> +
> +	platform_set_drvdata(pdev, pmu);
> +
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +				"perf/imx/ddr%d:online", num);
> +	if (!hpname)
> +		return -ENOMEM;
> +
> +	pmu->cpu = raw_smp_processor_id();
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> +					 ddr_perf_offline_cpu);
> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> +		goto ddr_perf_err;
> +	}
> +
> +	pmu->cpuhp_state = ret;
> +
> +	/* Register the pmu instance for cpu hotplug */
> +	cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +
> +	ret = perf_pmu_register(&pmu->pmu, name, -1);
> +	if (ret)
> +		goto ddr_perf_err;

Can these two be done after IRQ registration to simplify error
handling?

> +
> +	/* Request irq */
> +	irq = of_irq_get(np, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq: %d", irq);
> +		ret = irq;
> +		goto ddr_perf_irq_err;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq,
> +					ddr_perf_irq_handler,
> +					IRQF_NOBALANCING | IRQF_NO_THREAD,
> +					DDR_PERF_DEV_NAME,
> +					pmu);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Request irq failed: %d", ret);
> +		goto ddr_perf_irq_err;
> +	}
> +
> +	pmu->irq = irq;
> +	ret = irq_set_affinity_hint(pmu->irq, cpumask_of(pmu->cpu));
> +	if (ret) {
> +		dev_err(pmu->dev, "Failed to set interrupt affinity!\n");
> +		goto ddr_perf_irq_err;
> +	}
> +
> +	return 0;
> +
> +ddr_perf_irq_err:
> +	perf_pmu_unregister(&(pmu->pmu));
> +
> +ddr_perf_err:
> +	if (pmu->cpuhp_state)
> +		cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +
> +	ida_simple_remove(&ddr_ida, pmu->id);
> +	dev_warn(&pdev->dev, "i.MX8 DDR Perf PMU failed (%d), disabled\n", ret);
> +	return ret;
> +}


Thanks,
Andrey Smirnov

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter

[Zhi Li]

On Tue, Apr 30, 2019 at 1:59 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> > Add ddr performance monitor support for iMX8QXP
>
> > There are 4 counters for ddr perfomance events.
> > counter 0 is dedicated for cycles.
> > you choose any up to 3 no cycles events.
>
> > for example:
>
> > perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
> > perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls
>
> > Support below events.
>
> >   ddr0/activate/                                     [Kernel PMU event]
> >   ddr0/axid-read/                                    [Kernel PMU event]
> >   ddr0/axid-write/                                   [Kernel PMU event]
> >   ddr0/cycles/                                       [Kernel PMU event]
> >   ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
> >   ddr0/hp-read/                                      [Kernel PMU event]
> >   ddr0/hp-req-nodcredit/                             [Kernel PMU event]
> >   ddr0/hp-xact-credit/                               [Kernel PMU event]
> >   ddr0/load-mode/                                    [Kernel PMU event]
> >   ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
> >   ddr0/lp-req-nocredit/                              [Kernel PMU event]
> >   ddr0/lp-xact-credit/                               [Kernel PMU event]
> >   ddr0/mwr/                                          [Kernel PMU event]
> >   ddr0/precharge/                                    [Kernel PMU event]
> >   ddr0/raw-hazard/                                   [Kernel PMU event]
> >   ddr0/read-access/                                  [Kernel PMU event]
> >   ddr0/read-activate/                                [Kernel PMU event]
> >   ddr0/read-command/                                 [Kernel PMU event]
> >   ddr0/read-cycles/                                  [Kernel PMU event]
> >   ddr0/read-modify-write-command/                    [Kernel PMU event]
> >   ddr0/read-queue-depth/                             [Kernel PMU event]
> >   ddr0/read-write-transition/                        [Kernel PMU event]
> >   ddr0/read/                                         [Kernel PMU event]
> >   ddr0/refresh/                                      [Kernel PMU event]
> >   ddr0/selfresh/                                     [Kernel PMU event]
> >   ddr0/wr-xact-credit/                               [Kernel PMU event]
> >   ddr0/write-access/                                 [Kernel PMU event]
> >   ddr0/write-command/                                [Kernel PMU event]
> >   ddr0/write-credit-cnt/                             [Kernel PMU event]
> >   ddr0/write-cycles/                                 [Kernel PMU event]
> >   ddr0/write-queue-depth/                            [Kernel PMU event]
> >   ddr0/write/
>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > No change from V8 to V9
>
> > Change from v7 to v8
> >  * remove unused define
> >  * change total_events to active_events, change active_events to events
> >  * remove flags,
> >  * fix multi line comments code sytle
> >  * add pmu_enable\disable function
> >  * disable event at irq handle
> >  * remove counter check at ddr_perf_free_counter
> >  * remove pmu->irq check
> >  * add group check
>
> > Change from v6 to v7
> >  * added irq affinity handle, ref arm-ccn.c
> >  * added IRQF_NOBALANCING | IRQF_NO_THREAD
> >  * added ida_simple_remove at failure path
>
> > Change from v5 to v6
> >  * fix insmod\rmmod problem
> >  * remove randunt register read at irq handle
> >  * change u32 irq to int
> >  * devm_request_irq use default flags.
>
> > Change from v4 to v5
> >  * Remove AXI ID filter function
>
> > Change from v3 to v4
> >  * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
> >  * sort include
> >  * remove struct fsl_ddr_devtype_data
> >  * Added comment need disable control first
> >  * Added comment about must enable cycle counter
> >  * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
> >  * Added comment about cycle counter is fastest one
>
> > Change from v2 to v3
> >  * remove kfree
>
> > Change from V1 to V2
> >  * update Kconfig by use i.MX8 instead of i.MX8 QXP
> >  * remove gpl statememnt since SPDX tag
> >  * use dev_kzalloc
> >  * use dev_err
> >  * commit message show axi_read 0x41\axi_write 0x42
> >  * commit message show cycles must be enabled
> >  * Irq only issue at cycles overflow
> >  * use NUM_COUNTER
> >  * use devm_request_irq
> >  * add hotplug callback to handle context migration
>
> Hey Frank,
>
> I missed your effort to upstream this and ended up spening some time
> working on the same thing in parallel, so I have some comments below.
>
> > +PMU_EVENT_ATTR_STRING(cycles, ddr_perf_cycles, "event=0x00");
> > +PMU_EVENT_ATTR_STRING(selfresh, ddr_perf_selfresh, "event=0x01");
> > +PMU_EVENT_ATTR_STRING(read-access, ddr_perf_read_accesses, "event=0x04");
> > +PMU_EVENT_ATTR_STRING(write-access, ddr_perf_write_accesses, "event=0x05");
> > +PMU_EVENT_ATTR_STRING(read-queue-depth, ddr_perf_read_queue_depth,
> > +                     "event=0x08");
> > +PMU_EVENT_ATTR_STRING(write-queue-depth, ddr_perf_write_queue_depth,
> > +                     "event=0x09");
> > +PMU_EVENT_ATTR_STRING(lp-read-credit-cnt, ddr_perf_lp_read_credit_cnt,
> > +                     "event=0x10");
> > +PMU_EVENT_ATTR_STRING(hp-read-credit-cnt, ddr_perf_hp_read_credit_cnt,
> > +                     "event=0x11");
> > +PMU_EVENT_ATTR_STRING(write-credit-cnt, ddr_perf_write_credit_cnt,
> > +                     "event=0x12");
> > +PMU_EVENT_ATTR_STRING(read-command, ddr_perf_read_command, "event=0x20");
> > +PMU_EVENT_ATTR_STRING(write-command, ddr_perf_write_command, "event=0x21");
> > +PMU_EVENT_ATTR_STRING(read-modify-write-command,
> > +             ddr_perf_read_modify_write_command, "event=0x22");
> > +PMU_EVENT_ATTR_STRING(hp-read, ddr_perf_hp_read, "event=0x23");
> > +PMU_EVENT_ATTR_STRING(hp-req-nodcredit, ddr_perf_hp_req_nocredit, "event=0x24");
> > +PMU_EVENT_ATTR_STRING(hp-xact-credit, ddr_perf_hp_xact_credit, "event=0x25");
> > +PMU_EVENT_ATTR_STRING(lp-req-nocredit, ddr_perf_lp_req_nocredit, "event=0x26");
> > +PMU_EVENT_ATTR_STRING(lp-xact-credit, ddr_perf_lp_xact_credit, "event=0x27");
> > +PMU_EVENT_ATTR_STRING(wr-xact-credit, ddr_perf_wr_xact_credit, "event=0x29");
> > +PMU_EVENT_ATTR_STRING(read-cycles, ddr_perf_read_cycles, "event=0x2a");
> > +PMU_EVENT_ATTR_STRING(write-cycles, ddr_perf_write_cycles, "event=0x2b");
> > +PMU_EVENT_ATTR_STRING(read-write-transition, ddr_perf_read_write_transition,
> > +                     "event=0x30");
> > +PMU_EVENT_ATTR_STRING(precharge, ddr_perf_precharge, "event=0x31");
> > +PMU_EVENT_ATTR_STRING(activate, ddr_perf_activate, "event=0x32");
> > +PMU_EVENT_ATTR_STRING(load-mode, ddr_perf_load_mode, "event=0x33");
> > +PMU_EVENT_ATTR_STRING(mwr, ddr_perf_mwr, "event=0x34");
> > +PMU_EVENT_ATTR_STRING(read, ddr_perf_read, "event=0x35");
> > +PMU_EVENT_ATTR_STRING(read-activate, ddr_perf_read_activate, "event=0x36");
> > +PMU_EVENT_ATTR_STRING(refresh, ddr_perf_refresh, "event=0x37");
> > +PMU_EVENT_ATTR_STRING(write, ddr_perf_write, "event=0x38");
> > +PMU_EVENT_ATTR_STRING(raw-hazard, ddr_perf_raw_hazard, "event=0x39");
>
> You can really save quite a bit of boilerplate if you define those
> inplace with a custom macro and a custom show function:
>
> static ssize_t
> ddr_pmu_event_show(struct device *dev, struct device_attribute *attr,
>                    char *page)
> {
>         struct perf_pmu_events_attr *pmu_attr;
>
>         pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
>         return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
> }
>
> #define IMX8_DDR_PMU_EVENT_ATTR(_name, _id)                             \
>         (&((struct perf_pmu_events_attr[]) {                            \
>                 { .attr = __ATTR(_name, 0444, ddr_pmu_event_show, NULL), \
>                   .id = _id, }                                          \
>         })[0].attr.attr)
>
> static struct attribute *ddr_pmu_events_attrs[] = {
>         IMX8_DDR_PMU_EVENT_ATTR(cycles,              CYCLES_EVENT_ID),
>         IMX8_DDR_PMU_EVENT_ATTR(selfresh,                       0x01),
>         IMX8_DDR_PMU_EVENT_ATTR(read-access,                    0x04),
>         IMX8_DDR_PMU_EVENT_ATTR(write-access,                   0x05),
>         IMX8_DDR_PMU_EVENT_ATTR(read-queue-depth,               0x08),
>

I don't think it make any difference.

best regards
Frank Li

> ...
>
> > +
> > +struct ddr_pmu {
> > +     struct pmu pmu;
> > +     void __iomem *base;
> > +     unsigned int cpu;
> > +     struct  hlist_node node;
> > +     struct  device *dev;
>
> This device pointer is used only once in ddr_perf_event_init() and
> even in that function not all error cases get a dedicated kernel
> message. I'd consider just dropping it and the message it is used in.
>
> > +     struct perf_event *events[NUM_COUNTERS];
> > +     int active_events;
>
> I'd very strongly encourage you to convert the driver to use a proper
> bitmask instead of this counter. E.g:
>
> DECLARE_BITMAP(active_mask, NUM_COUNTERS);

I don't think it has big difference at these case.

best regards
Frank Li

>
> I'll comment more on that below.
>
> > +     enum cpuhp_state cpuhp_state;
> > +     int irq;
> > +     int id;
> > +};
>
> > +static struct attribute_group ddr_perf_events_attr_group = {
> > +     .name = "events",
> > +     .attrs = ddr_perf_events_attrs,
> > +};
> > +
> > +PMU_FORMAT_ATTR(event, "config:0-63");
> > +
>
> Event ID is really only 8-bits wide, AFAIK. Is there any reason to
> reserve all 64 for it in config?
>
> > +static struct attribute *ddr_perf_format_attrs[] = {
> > +     &format_attr_event.attr,
> > +     NULL,
> > +};
> > +
> > +static struct attribute_group ddr_perf_format_attr_group = {
> > +     .name = "format",
> > +     .attrs = ddr_perf_format_attrs,
> > +};
> > +
> > +static const struct attribute_group *attr_groups[] = {
> > +     &ddr_perf_events_attr_group,
> > +     &ddr_perf_format_attr_group,
> > +     &ddr_perf_cpumask_attr_group,
> > +     NULL,
> > +};
> > +
> > +static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
> > +{
> > +     int i;
> > +
> > +     /*
> > +      * Always map cycle event to counter 0
> > +      * Cycles counter is dedicated for cycle event
> > +      * can't used for the other events
> > +      */
> > +     if (event == EVENT_CYCLES_ID) {
> > +             if (pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +                     return EVENT_CYCLES_COUNTER;
> > +             else
> > +                     return -ENOENT;
> > +     }
> > +
> > +     for (i = 1; i < NUM_COUNTERS; i++) {
> > +             if (pmu->events[i] == NULL)
> > +                     return i;
> > +     }
>
> This is the first place where using a bitmap would simplify the
> driver. Here all you'd need to do is:
>
>
> if (event == CYCLES_EVENT_ID) {
>    ...
> } else {
>   i =  find_next_zero_bit(pmu->active_mask, NUM_COUNTERS,
>                           EVENT_CYCLES_COUNTER + 1);
>   if (i == NUM_COUNTERS)
>         return -EAGAIN;
>
> }
>
> set_bit(i, pmu->active_mask);
>
> ...
>
> > +
> > +     return -ENOENT;
> > +}
> > +
> > +static u32 ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
> > +{
> > +     pmu->events[counter] = NULL;
> > +
> > +     return 0;
> > +}
>
> Is this function even necessary? It is used only once in the code and
> it's return value is ignored

It help improve code read. Generally allocate and free always a pair.
I change change to void.

best regards
Frank Li

>
> > +
> > +static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
> > +{
> > +     return readl(pmu->base + COUNTER_READ + counter * 4);
> > +}
>
> Would using readl_relaxed() be beneficial here?

Maybe. But only few register read when run perf.

best regards
Frank Li

>
> > +
> > +static int ddr_perf_event_init(struct perf_event *event)
> > +{
> > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     struct perf_event *sibling;
> > +
> > +     if (event->attr.type != event->pmu->type)
> > +             return -ENOENT;
> > +
> > +     if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (event->cpu < 0) {
> > +             dev_warn(pmu->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       ||
>
> You don't need any of the above if you pass:
>
> .capabilities = PERF_PMU_CAP_NO_EXCLUDE
>
> in your struct pmu initialization
>
> > +         event->attr.sample_period)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * We must NOT create groups containing mixed PMUs, although software
> > +      * events are acceptable (for example to create a CCN group
> > +      * periodically read when a hrtimer aka cpu-clock leader triggers).
> > +      */
> > +     if (event->group_leader->pmu != event->pmu &&
> > +                     !is_software_event(event->group_leader))
> > +             return -EINVAL;
> > +
> > +     for_each_sibling_event(sibling, event->group_leader) {
> > +             if (sibling->pmu != event->pmu &&
> > +                             !is_software_event(sibling))
> > +                     return -EINVAL;
> > +     }
> > +
> > +     event->cpu = pmu->cpu;
> > +     hwc->idx = -1;
> > +
> > +     return 0;
> > +}
>
> > +
> > +static void ddr_perf_event_enable(struct ddr_pmu *pmu, int config,
> > +                               int counter, bool enable)
> > +{
>
> This function doesn't really have anything to do with events (unlike,
> for example, ddr_perf_event_start() below). Maybe it would be better
> to rename it to ddr_perf_counter_enable()?
>
> > +     u8 reg = counter * 4 + COUNTER_CNTL;
> > +     int val;
> > +
> > +     if (enable) {
> > +             /*
> > +              * must disable first, then enable again
> > +              * otherwise, cycle counter will not work
> > +              * if previous state is enabled.
> > +              */
> > +             writel(0, pmu->base + reg);
> > +             val = CNTL_EN | CNTL_CLEAR;
> > +             val |= (config << CNTL_CSV_SHIFT) & CNTL_CSV_MASK;
>
> You can simplify the above with FIELD_PREP(CNTL_CSV_MASK, config)
>
> > +     } else {
> > +             /* Disable counter */
> > +             val = readl(pmu->base + reg) & CNTL_EN_MASK;
>
> This register will be blown away by the code in the other if branch
> next time the counter is enabled. What's the point of doing
> read-modify-write and trying to preserve all but EN bits as they were here?
>
> Why not just do:
>
> writel(CNTL_CLEAR, pmu->base + reg);
>
> ?
>
> > +     }
> > +
> > +     writel(val, pmu->base + reg);
> > +}
> > +
> > +static void ddr_perf_event_start(struct perf_event *event, int flags)
> > +{
> > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     int counter = hwc->idx;
> > +
> > +     local64_set(&hwc->prev_count, 0);
> > +
> > +     ddr_perf_event_enable(pmu, event->attr.config, counter, true);
> > +}
> > +
> > +static int ddr_perf_event_add(struct perf_event *event, int flags)
> > +{
> > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     int counter;
> > +     int cfg = event->attr.config;
> > +
> > +     counter = ddr_perf_alloc_counter(pmu, cfg);
> > +     if (counter < 0) {
> > +             dev_dbg(pmu->dev, "There are not enough counters\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     pmu->events[counter] = event;
> > +     pmu->active_events++;
> > +     hwc->idx = counter;
>
> What about hw->state?
>
> > +
> > +     if (flags & PERF_EF_START)
> > +             ddr_perf_event_start(event, flags);
> > +
> > +     local64_set(&hwc->prev_count, ddr_perf_read_counter(pmu, counter));
>
> What's this local64_set() for? You already clear prev_counter and HW
> counter when event is started. This just seems redundant.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static void ddr_perf_event_stop(struct perf_event *event, int flags)
> > +{
> > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     int counter = hwc->idx;
> > +
> > +     ddr_perf_event_enable(pmu, event->attr.config, counter, false);
> > +     ddr_perf_event_update(event);
>
> hw->state isn't updated here either? I'm no expert on perf subsystem,
> so maybe it's OK, but most of the other drivers in this category do
>
> hw->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
>
> > +}
> > +
> > +static void ddr_perf_event_del(struct perf_event *event, int flags)
> > +{
> > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     int counter = hwc->idx;
> > +
> > +     ddr_perf_event_stop(event, PERF_EF_UPDATE);
> > +
> > +     ddr_perf_free_counter(pmu, counter);
> > +     pmu->active_events--;
> > +     hwc->idx = -1;
> > +}
> > +
> > +static void ddr_perf_pmu_enable(struct pmu *pmu)
> > +{
> > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > +
> > +     /* enable cycle counter if cycle is not active event list */
> > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +             ddr_perf_event_enable(ddr_pmu,
> > +                                   EVENT_CYCLES_ID,
> > +                                   EVENT_CYCLES_COUNTER,
> > +                                   true);
> > +}
> > +
> > +static void ddr_perf_pmu_disable(struct pmu *pmu)
> > +{
> > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > +
> > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +             ddr_perf_event_enable(ddr_pmu,
> > +                                   EVENT_CYCLES_ID,
> > +                                   EVENT_CYCLES_COUNTER,
> > +                                   false);
> > +}
>
> It seems that both of those functions will enable/disable cycles
> counter multiple times if "perf" is called with a list specifying
> multiple events but not cycles counter. Not sure if this is
> intentional, in case it is not, using a bitmask would allow you to
> avoid this by using bitmap_weight(), e.g:
>
> if (bitmap_weight(pmu->active_mask, NUM_COUNTERS) == 1 &&
>     ...
>
> > +
> > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > +{
> > +     int i;
> > +     struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > +     struct perf_event *event, *cycle_event = NULL;
> > +
> > +     /* all counter will stop if cycle counter disabled */
> > +     ddr_perf_event_enable(pmu,
> > +                           EVENT_CYCLES_ID,
> > +                           EVENT_CYCLES_COUNTER,
> > +                           false);
>
> The comment below says that IRQ is only raised when cycles counter
> overflow and when that happens all of the counters are stopped. What's
> the goal of the code disabling cycles counter above then?

Mark suggest disable PMU at irq beginning.  The below is V7 review comments.

"That's true (and I had forgotten this), but there's still a potential
problem depending on IRQ latency.

For example, an overflow might occur just before we do some other
programming of the PMU (while the CPU has IRQs disabled) where we
restart the cycle counter (and the IRQ is de-asserted).

Depending on when the interrupt controller samples the state of that
IRQ, and when the CPU takes a resulting interrupt, we may be able to end
up in the IRQ handler with the cycle counter enabled. Explicitly
disabling the cycle counter avoids that possibility.

Regardless, we'll want to move the enable of the cycle counter last to
ensure that groups aren't skewed.
"
best regards
Frank Li

>
> > +     /*
> > +      * When the cycle counter overflows, all counters are stopped,
> > +      * and an IRQ is raised. If any other counter overflows, it
> > +      * continues counting, and no IRQ is raised.
> > +      *
> > +      * Cycles occur at least 4 times as often as other events, so we
> > +      * can update all events on a cycle counter overflow and not
> > +      * lose events.
> > +      *
> > +      */
> > +     for (i = 0; i < NUM_COUNTERS; i++) {
> > +
> > +             if (!pmu->events[i])
> > +                     continue;
> > +
> > +             event = pmu->events[i];
> > +
> > +             ddr_perf_event_update(event);
>
> If you already reading this counter out, why not clear its value to 0
> to avoid having it overflow while at it?

Clear counter need additional lock to make sure previous value is the
same as hardware one.
At this time, only cycle counter over flow, the other counter is not over flow.

Overflow bit only work on cycle counter,  the other counter is free running.

best regards
Frank Li

>
> > +
> > +             if (event->hw.idx == EVENT_CYCLES_COUNTER)
> > +                     cycle_event = event;
> > +     }
> > +
> > +     ddr_perf_event_enable(pmu,
> > +                           EVENT_CYCLES_ID,
> > +                           EVENT_CYCLES_COUNTER,
> > +                           true);
> > +     if (cycle_event)
> > +             ddr_perf_event_update(cycle_event);
>
> Using a bitmaks would allow you to simplify the above to:
>
> i = EVENT_CYCLES_COUNTER + 1;
> for_each_set_bit_from(i, pmu->active_mask, NUM_COUNTERS)
>    ddr_perf_event_update(pmu->active_events[i]);
>
> ddr_perf_event_enable(pmu,
>                       EVENT_CYCLES_ID,
>                       EVENT_CYCLES_COUNTER,
>                       true);
>
> if (test_bit(EVENT_CYCLES_COUNTER, pmu->active_mask))
>     ddr_perf_event_update(pmu->active_events[EVENT_CYCLES_COUNTER]);
>
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> > +
> > +static int ddr_perf_probe(struct platform_device *pdev)
> > +{
> > +     struct ddr_pmu *pmu;
> > +     struct device_node *np;
> > +     void __iomem *base;
> > +     struct resource *iomem;
> > +     char *name;
> > +     char *hpname;
> > +     int num;
> > +     int ret;
> > +     int irq;
> > +
> > +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     base = devm_ioremap_resource(&pdev->dev, iomem);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     np = pdev->dev.of_node;
> > +
> > +     pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> > +     if (!pmu)
> > +             return -ENOMEM;
> > +
> > +     num = ddr_perf_init(pmu, base, &pdev->dev);
>
> There's already a unique ID availible - IP block's physical address
> (iomem->start). Why not use that instead of setting up an IDA?

We want to use 0 based sequence for ddr controller.
like ddr0/xxx, ddr1/xxxx

If physical address, some user space test script have to update for
difference chips.

best regards
Frank Li

>
> > +
> > +     platform_set_drvdata(pdev, pmu);
> > +
> > +     name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> > +     if (!name)
> > +             return -ENOMEM;
> > +
> > +     hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +                             "perf/imx/ddr%d:online", num);
> > +     if (!hpname)
> > +             return -ENOMEM;
> > +
> > +     pmu->cpu = raw_smp_processor_id();
> > +     ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> > +                                      ddr_perf_offline_cpu);
> > +
> > +     if (ret < 0) {
> > +             dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> > +             goto ddr_perf_err;
> > +     }
> > +
> > +     pmu->cpuhp_state = ret;
> > +
> > +     /* Register the pmu instance for cpu hotplug */
> > +     cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> > +
> > +     ret = perf_pmu_register(&pmu->pmu, name, -1);
> > +     if (ret)
> > +             goto ddr_perf_err;
>
> Can these two be done after IRQ registration to simplify error
> handling?
>
> > +
> > +     /* Request irq */
> > +     irq = of_irq_get(np, 0);
> > +     if (irq < 0) {
> > +             dev_err(&pdev->dev, "Failed to get irq: %d", irq);
> > +             ret = irq;
> > +             goto ddr_perf_irq_err;
> > +     }
> > +
> > +     ret = devm_request_irq(&pdev->dev, irq,
> > +                                     ddr_perf_irq_handler,
> > +                                     IRQF_NOBALANCING | IRQF_NO_THREAD,
> > +                                     DDR_PERF_DEV_NAME,
> > +                                     pmu);
> > +     if (ret < 0) {
> > +             dev_err(&pdev->dev, "Request irq failed: %d", ret);
> > +             goto ddr_perf_irq_err;
> > +     }
> > +
> > +     pmu->irq = irq;
> > +     ret = irq_set_affinity_hint(pmu->irq, cpumask_of(pmu->cpu));
> > +     if (ret) {
> > +             dev_err(pmu->dev, "Failed to set interrupt affinity!\n");
> > +             goto ddr_perf_irq_err;
> > +     }
> > +
> > +     return 0;
> > +
> > +ddr_perf_irq_err:
> > +     perf_pmu_unregister(&(pmu->pmu));
> > +
> > +ddr_perf_err:
> > +     if (pmu->cpuhp_state)
> > +             cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> > +
> > +     ida_simple_remove(&ddr_ida, pmu->id);
> > +     dev_warn(&pdev->dev, "i.MX8 DDR Perf PMU failed (%d), disabled\n", ret);
> > +     return ret;
> > +}
>
>
> Thanks,
> Andrey Smirnov

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter

[Andrey Smirnov]

On Tue, Apr 30, 2019 at 1:08 PM Zhi Li <lznuaa@gmail.com> wrote:
>
> On Tue, Apr 30, 2019 at 1:59 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > > Add ddr performance monitor support for iMX8QXP
> >
> > > There are 4 counters for ddr perfomance events.
> > > counter 0 is dedicated for cycles.
> > > you choose any up to 3 no cycles events.
> >
> > > for example:
> >
> > > perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
> > > perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls
> >
> > > Support below events.
> >
> > >   ddr0/activate/                                     [Kernel PMU event]
> > >   ddr0/axid-read/                                    [Kernel PMU event]
> > >   ddr0/axid-write/                                   [Kernel PMU event]
> > >   ddr0/cycles/                                       [Kernel PMU event]
> > >   ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
> > >   ddr0/hp-read/                                      [Kernel PMU event]
> > >   ddr0/hp-req-nodcredit/                             [Kernel PMU event]
> > >   ddr0/hp-xact-credit/                               [Kernel PMU event]
> > >   ddr0/load-mode/                                    [Kernel PMU event]
> > >   ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
> > >   ddr0/lp-req-nocredit/                              [Kernel PMU event]
> > >   ddr0/lp-xact-credit/                               [Kernel PMU event]
> > >   ddr0/mwr/                                          [Kernel PMU event]
> > >   ddr0/precharge/                                    [Kernel PMU event]
> > >   ddr0/raw-hazard/                                   [Kernel PMU event]
> > >   ddr0/read-access/                                  [Kernel PMU event]
> > >   ddr0/read-activate/                                [Kernel PMU event]
> > >   ddr0/read-command/                                 [Kernel PMU event]
> > >   ddr0/read-cycles/                                  [Kernel PMU event]
> > >   ddr0/read-modify-write-command/                    [Kernel PMU event]
> > >   ddr0/read-queue-depth/                             [Kernel PMU event]
> > >   ddr0/read-write-transition/                        [Kernel PMU event]
> > >   ddr0/read/                                         [Kernel PMU event]
> > >   ddr0/refresh/                                      [Kernel PMU event]
> > >   ddr0/selfresh/                                     [Kernel PMU event]
> > >   ddr0/wr-xact-credit/                               [Kernel PMU event]
> > >   ddr0/write-access/                                 [Kernel PMU event]
> > >   ddr0/write-command/                                [Kernel PMU event]
> > >   ddr0/write-credit-cnt/                             [Kernel PMU event]
> > >   ddr0/write-cycles/                                 [Kernel PMU event]
> > >   ddr0/write-queue-depth/                            [Kernel PMU event]
> > >   ddr0/write/
> >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > No change from V8 to V9
> >
> > > Change from v7 to v8
> > >  * remove unused define
> > >  * change total_events to active_events, change active_events to events
> > >  * remove flags,
> > >  * fix multi line comments code sytle
> > >  * add pmu_enable\disable function
> > >  * disable event at irq handle
> > >  * remove counter check at ddr_perf_free_counter
> > >  * remove pmu->irq check
> > >  * add group check
> >
> > > Change from v6 to v7
> > >  * added irq affinity handle, ref arm-ccn.c
> > >  * added IRQF_NOBALANCING | IRQF_NO_THREAD
> > >  * added ida_simple_remove at failure path
> >
> > > Change from v5 to v6
> > >  * fix insmod\rmmod problem
> > >  * remove randunt register read at irq handle
> > >  * change u32 irq to int
> > >  * devm_request_irq use default flags.
> >
> > > Change from v4 to v5
> > >  * Remove AXI ID filter function
> >
> > > Change from v3 to v4
> > >  * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
> > >  * sort include
> > >  * remove struct fsl_ddr_devtype_data
> > >  * Added comment need disable control first
> > >  * Added comment about must enable cycle counter
> > >  * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
> > >  * Added comment about cycle counter is fastest one
> >
> > > Change from v2 to v3
> > >  * remove kfree
> >
> > > Change from V1 to V2
> > >  * update Kconfig by use i.MX8 instead of i.MX8 QXP
> > >  * remove gpl statememnt since SPDX tag
> > >  * use dev_kzalloc
> > >  * use dev_err
> > >  * commit message show axi_read 0x41\axi_write 0x42
> > >  * commit message show cycles must be enabled
> > >  * Irq only issue at cycles overflow
> > >  * use NUM_COUNTER
> > >  * use devm_request_irq
> > >  * add hotplug callback to handle context migration
> >
> > Hey Frank,
> >
> > I missed your effort to upstream this and ended up spening some time
> > working on the same thing in parallel, so I have some comments below.
> >
> > > +PMU_EVENT_ATTR_STRING(cycles, ddr_perf_cycles, "event=0x00");
> > > +PMU_EVENT_ATTR_STRING(selfresh, ddr_perf_selfresh, "event=0x01");
> > > +PMU_EVENT_ATTR_STRING(read-access, ddr_perf_read_accesses, "event=0x04");
> > > +PMU_EVENT_ATTR_STRING(write-access, ddr_perf_write_accesses, "event=0x05");
> > > +PMU_EVENT_ATTR_STRING(read-queue-depth, ddr_perf_read_queue_depth,
> > > +                     "event=0x08");
> > > +PMU_EVENT_ATTR_STRING(write-queue-depth, ddr_perf_write_queue_depth,
> > > +                     "event=0x09");
> > > +PMU_EVENT_ATTR_STRING(lp-read-credit-cnt, ddr_perf_lp_read_credit_cnt,
> > > +                     "event=0x10");
> > > +PMU_EVENT_ATTR_STRING(hp-read-credit-cnt, ddr_perf_hp_read_credit_cnt,
> > > +                     "event=0x11");
> > > +PMU_EVENT_ATTR_STRING(write-credit-cnt, ddr_perf_write_credit_cnt,
> > > +                     "event=0x12");
> > > +PMU_EVENT_ATTR_STRING(read-command, ddr_perf_read_command, "event=0x20");
> > > +PMU_EVENT_ATTR_STRING(write-command, ddr_perf_write_command, "event=0x21");
> > > +PMU_EVENT_ATTR_STRING(read-modify-write-command,
> > > +             ddr_perf_read_modify_write_command, "event=0x22");
> > > +PMU_EVENT_ATTR_STRING(hp-read, ddr_perf_hp_read, "event=0x23");
> > > +PMU_EVENT_ATTR_STRING(hp-req-nodcredit, ddr_perf_hp_req_nocredit, "event=0x24");
> > > +PMU_EVENT_ATTR_STRING(hp-xact-credit, ddr_perf_hp_xact_credit, "event=0x25");
> > > +PMU_EVENT_ATTR_STRING(lp-req-nocredit, ddr_perf_lp_req_nocredit, "event=0x26");
> > > +PMU_EVENT_ATTR_STRING(lp-xact-credit, ddr_perf_lp_xact_credit, "event=0x27");
> > > +PMU_EVENT_ATTR_STRING(wr-xact-credit, ddr_perf_wr_xact_credit, "event=0x29");
> > > +PMU_EVENT_ATTR_STRING(read-cycles, ddr_perf_read_cycles, "event=0x2a");
> > > +PMU_EVENT_ATTR_STRING(write-cycles, ddr_perf_write_cycles, "event=0x2b");
> > > +PMU_EVENT_ATTR_STRING(read-write-transition, ddr_perf_read_write_transition,
> > > +                     "event=0x30");
> > > +PMU_EVENT_ATTR_STRING(precharge, ddr_perf_precharge, "event=0x31");
> > > +PMU_EVENT_ATTR_STRING(activate, ddr_perf_activate, "event=0x32");
> > > +PMU_EVENT_ATTR_STRING(load-mode, ddr_perf_load_mode, "event=0x33");
> > > +PMU_EVENT_ATTR_STRING(mwr, ddr_perf_mwr, "event=0x34");
> > > +PMU_EVENT_ATTR_STRING(read, ddr_perf_read, "event=0x35");
> > > +PMU_EVENT_ATTR_STRING(read-activate, ddr_perf_read_activate, "event=0x36");
> > > +PMU_EVENT_ATTR_STRING(refresh, ddr_perf_refresh, "event=0x37");
> > > +PMU_EVENT_ATTR_STRING(write, ddr_perf_write, "event=0x38");
> > > +PMU_EVENT_ATTR_STRING(raw-hazard, ddr_perf_raw_hazard, "event=0x39");
> >
> > You can really save quite a bit of boilerplate if you define those
> > inplace with a custom macro and a custom show function:
> >
> > static ssize_t
> > ddr_pmu_event_show(struct device *dev, struct device_attribute *attr,
> >                    char *page)
> > {
> >         struct perf_pmu_events_attr *pmu_attr;
> >
> >         pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
> >         return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
> > }
> >
> > #define IMX8_DDR_PMU_EVENT_ATTR(_name, _id)                             \
> >         (&((struct perf_pmu_events_attr[]) {                            \
> >                 { .attr = __ATTR(_name, 0444, ddr_pmu_event_show, NULL), \
> >                   .id = _id, }                                          \
> >         })[0].attr.attr)
> >
> > static struct attribute *ddr_pmu_events_attrs[] = {
> >         IMX8_DDR_PMU_EVENT_ATTR(cycles,              CYCLES_EVENT_ID),
> >         IMX8_DDR_PMU_EVENT_ATTR(selfresh,                       0x01),
> >         IMX8_DDR_PMU_EVENT_ATTR(read-access,                    0x04),
> >         IMX8_DDR_PMU_EVENT_ATTR(write-access,                   0x05),
> >         IMX8_DDR_PMU_EVENT_ATTR(read-queue-depth,               0x08),
> >
>
> I don't think it make any difference.
>
> best regards
> Frank Li
>
> > ...
> >
> > > +
> > > +struct ddr_pmu {
> > > +     struct pmu pmu;
> > > +     void __iomem *base;
> > > +     unsigned int cpu;
> > > +     struct  hlist_node node;
> > > +     struct  device *dev;
> >
> > This device pointer is used only once in ddr_perf_event_init() and
> > even in that function not all error cases get a dedicated kernel
> > message. I'd consider just dropping it and the message it is used in.
> >
> > > +     struct perf_event *events[NUM_COUNTERS];
> > > +     int active_events;
> >
> > I'd very strongly encourage you to convert the driver to use a proper
> > bitmask instead of this counter. E.g:
> >
> > DECLARE_BITMAP(active_mask, NUM_COUNTERS);
>
> I don't think it has big difference at these case.
>
> best regards
> Frank Li
>
> >
> > I'll comment more on that below.
> >
> > > +     enum cpuhp_state cpuhp_state;
> > > +     int irq;
> > > +     int id;
> > > +};
> >
> > > +static struct attribute_group ddr_perf_events_attr_group = {
> > > +     .name = "events",
> > > +     .attrs = ddr_perf_events_attrs,
> > > +};
> > > +
> > > +PMU_FORMAT_ATTR(event, "config:0-63");
> > > +
> >
> > Event ID is really only 8-bits wide, AFAIK. Is there any reason to
> > reserve all 64 for it in config?
> >
> > > +static struct attribute *ddr_perf_format_attrs[] = {
> > > +     &format_attr_event.attr,
> > > +     NULL,
> > > +};
> > > +
> > > +static struct attribute_group ddr_perf_format_attr_group = {
> > > +     .name = "format",
> > > +     .attrs = ddr_perf_format_attrs,
> > > +};
> > > +
> > > +static const struct attribute_group *attr_groups[] = {
> > > +     &ddr_perf_events_attr_group,
> > > +     &ddr_perf_format_attr_group,
> > > +     &ddr_perf_cpumask_attr_group,
> > > +     NULL,
> > > +};
> > > +
> > > +static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
> > > +{
> > > +     int i;
> > > +
> > > +     /*
> > > +      * Always map cycle event to counter 0
> > > +      * Cycles counter is dedicated for cycle event
> > > +      * can't used for the other events
> > > +      */
> > > +     if (event == EVENT_CYCLES_ID) {
> > > +             if (pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > +                     return EVENT_CYCLES_COUNTER;
> > > +             else
> > > +                     return -ENOENT;
> > > +     }
> > > +
> > > +     for (i = 1; i < NUM_COUNTERS; i++) {
> > > +             if (pmu->events[i] == NULL)
> > > +                     return i;
> > > +     }
> >
> > This is the first place where using a bitmap would simplify the
> > driver. Here all you'd need to do is:
> >
> >
> > if (event == CYCLES_EVENT_ID) {
> >    ...
> > } else {
> >   i =  find_next_zero_bit(pmu->active_mask, NUM_COUNTERS,
> >                           EVENT_CYCLES_COUNTER + 1);
> >   if (i == NUM_COUNTERS)
> >         return -EAGAIN;
> >
> > }
> >
> > set_bit(i, pmu->active_mask);
> >
> > ...
> >
> > > +
> > > +     return -ENOENT;
> > > +}
> > > +
> > > +static u32 ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
> > > +{
> > > +     pmu->events[counter] = NULL;
> > > +
> > > +     return 0;
> > > +}
> >
> > Is this function even necessary? It is used only once in the code and
> > it's return value is ignored
>
> It help improve code read. Generally allocate and free always a pair.
> I change change to void.
>
> best regards
> Frank Li
>
> >
> > > +
> > > +static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
> > > +{
> > > +     return readl(pmu->base + COUNTER_READ + counter * 4);
> > > +}
> >
> > Would using readl_relaxed() be beneficial here?
>
> Maybe. But only few register read when run perf.
>
> best regards
> Frank Li
>
> >
> > > +
> > > +static int ddr_perf_event_init(struct perf_event *event)
> > > +{
> > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > +     struct hw_perf_event *hwc = &event->hw;
> > > +     struct perf_event *sibling;
> > > +
> > > +     if (event->attr.type != event->pmu->type)
> > > +             return -ENOENT;
> > > +
> > > +     if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (event->cpu < 0) {
> > > +             dev_warn(pmu->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       ||
> >
> > You don't need any of the above if you pass:
> >
> > .capabilities = PERF_PMU_CAP_NO_EXCLUDE
> >
> > in your struct pmu initialization
> >
> > > +         event->attr.sample_period)
> > > +             return -EINVAL;
> > > +
> > > +     /*
> > > +      * We must NOT create groups containing mixed PMUs, although software
> > > +      * events are acceptable (for example to create a CCN group
> > > +      * periodically read when a hrtimer aka cpu-clock leader triggers).
> > > +      */
> > > +     if (event->group_leader->pmu != event->pmu &&
> > > +                     !is_software_event(event->group_leader))
> > > +             return -EINVAL;
> > > +
> > > +     for_each_sibling_event(sibling, event->group_leader) {
> > > +             if (sibling->pmu != event->pmu &&
> > > +                             !is_software_event(sibling))
> > > +                     return -EINVAL;
> > > +     }
> > > +
> > > +     event->cpu = pmu->cpu;
> > > +     hwc->idx = -1;
> > > +
> > > +     return 0;
> > > +}
> >
> > > +
> > > +static void ddr_perf_event_enable(struct ddr_pmu *pmu, int config,
> > > +                               int counter, bool enable)
> > > +{
> >
> > This function doesn't really have anything to do with events (unlike,
> > for example, ddr_perf_event_start() below). Maybe it would be better
> > to rename it to ddr_perf_counter_enable()?
> >
> > > +     u8 reg = counter * 4 + COUNTER_CNTL;
> > > +     int val;
> > > +
> > > +     if (enable) {
> > > +             /*
> > > +              * must disable first, then enable again
> > > +              * otherwise, cycle counter will not work
> > > +              * if previous state is enabled.
> > > +              */
> > > +             writel(0, pmu->base + reg);
> > > +             val = CNTL_EN | CNTL_CLEAR;
> > > +             val |= (config << CNTL_CSV_SHIFT) & CNTL_CSV_MASK;
> >
> > You can simplify the above with FIELD_PREP(CNTL_CSV_MASK, config)
> >
> > > +     } else {
> > > +             /* Disable counter */
> > > +             val = readl(pmu->base + reg) & CNTL_EN_MASK;
> >
> > This register will be blown away by the code in the other if branch
> > next time the counter is enabled. What's the point of doing
> > read-modify-write and trying to preserve all but EN bits as they were here?
> >
> > Why not just do:
> >
> > writel(CNTL_CLEAR, pmu->base + reg);
> >
> > ?
> >
> > > +     }
> > > +
> > > +     writel(val, pmu->base + reg);
> > > +}
> > > +
> > > +static void ddr_perf_event_start(struct perf_event *event, int flags)
> > > +{
> > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > +     struct hw_perf_event *hwc = &event->hw;
> > > +     int counter = hwc->idx;
> > > +
> > > +     local64_set(&hwc->prev_count, 0);
> > > +
> > > +     ddr_perf_event_enable(pmu, event->attr.config, counter, true);
> > > +}
> > > +
> > > +static int ddr_perf_event_add(struct perf_event *event, int flags)
> > > +{
> > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > +     struct hw_perf_event *hwc = &event->hw;
> > > +     int counter;
> > > +     int cfg = event->attr.config;
> > > +
> > > +     counter = ddr_perf_alloc_counter(pmu, cfg);
> > > +     if (counter < 0) {
> > > +             dev_dbg(pmu->dev, "There are not enough counters\n");
> > > +             return -EOPNOTSUPP;
> > > +     }
> > > +
> > > +     pmu->events[counter] = event;
> > > +     pmu->active_events++;
> > > +     hwc->idx = counter;
> >
> > What about hw->state?
> >
> > > +
> > > +     if (flags & PERF_EF_START)
> > > +             ddr_perf_event_start(event, flags);
> > > +
> > > +     local64_set(&hwc->prev_count, ddr_perf_read_counter(pmu, counter));
> >
> > What's this local64_set() for? You already clear prev_counter and HW
> > counter when event is started. This just seems redundant.
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void ddr_perf_event_stop(struct perf_event *event, int flags)
> > > +{
> > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > +     struct hw_perf_event *hwc = &event->hw;
> > > +     int counter = hwc->idx;
> > > +
> > > +     ddr_perf_event_enable(pmu, event->attr.config, counter, false);
> > > +     ddr_perf_event_update(event);
> >
> > hw->state isn't updated here either? I'm no expert on perf subsystem,
> > so maybe it's OK, but most of the other drivers in this category do
> >
> > hw->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
> >
> > > +}
> > > +
> > > +static void ddr_perf_event_del(struct perf_event *event, int flags)
> > > +{
> > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > +     struct hw_perf_event *hwc = &event->hw;
> > > +     int counter = hwc->idx;
> > > +
> > > +     ddr_perf_event_stop(event, PERF_EF_UPDATE);
> > > +
> > > +     ddr_perf_free_counter(pmu, counter);
> > > +     pmu->active_events--;
> > > +     hwc->idx = -1;
> > > +}
> > > +
> > > +static void ddr_perf_pmu_enable(struct pmu *pmu)
> > > +{
> > > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > > +
> > > +     /* enable cycle counter if cycle is not active event list */
> > > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > +             ddr_perf_event_enable(ddr_pmu,
> > > +                                   EVENT_CYCLES_ID,
> > > +                                   EVENT_CYCLES_COUNTER,
> > > +                                   true);
> > > +}
> > > +
> > > +static void ddr_perf_pmu_disable(struct pmu *pmu)
> > > +{
> > > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > > +
> > > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > +             ddr_perf_event_enable(ddr_pmu,
> > > +                                   EVENT_CYCLES_ID,
> > > +                                   EVENT_CYCLES_COUNTER,
> > > +                                   false);
> > > +}
> >
> > It seems that both of those functions will enable/disable cycles
> > counter multiple times if "perf" is called with a list specifying
> > multiple events but not cycles counter. Not sure if this is
> > intentional, in case it is not, using a bitmask would allow you to
> > avoid this by using bitmap_weight(), e.g:
> >
> > if (bitmap_weight(pmu->active_mask, NUM_COUNTERS) == 1 &&
> >     ...
> >
> > > +
> > > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > > +{
> > > +     int i;
> > > +     struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > > +     struct perf_event *event, *cycle_event = NULL;
> > > +
> > > +     /* all counter will stop if cycle counter disabled */
> > > +     ddr_perf_event_enable(pmu,
> > > +                           EVENT_CYCLES_ID,
> > > +                           EVENT_CYCLES_COUNTER,
> > > +                           false);
> >
> > The comment below says that IRQ is only raised when cycles counter
> > overflow and when that happens all of the counters are stopped. What's
> > the goal of the code disabling cycles counter above then?
>
> Mark suggest disable PMU at irq beginning.  The below is V7 review comments.
>
> "That's true (and I had forgotten this), but there's still a potential
> problem depending on IRQ latency.
>
> For example, an overflow might occur just before we do some other
> programming of the PMU (while the CPU has IRQs disabled) where we
> restart the cycle counter (and the IRQ is de-asserted).
>
> Depending on when the interrupt controller samples the state of that
> IRQ, and when the CPU takes a resulting interrupt, we may be able to end
> up in the IRQ handler with the cycle counter enabled. Explicitly
> disabling the cycle counter avoids that possibility.
>
> Regardless, we'll want to move the enable of the cycle counter last to
> ensure that groups aren't skewed.
> "

Ah, I'd add this as a comment then.

> >
> > > +     /*
> > > +      * When the cycle counter overflows, all counters are stopped,
> > > +      * and an IRQ is raised. If any other counter overflows, it
> > > +      * continues counting, and no IRQ is raised.
> > > +      *
> > > +      * Cycles occur at least 4 times as often as other events, so we
> > > +      * can update all events on a cycle counter overflow and not
> > > +      * lose events.
> > > +      *
> > > +      */
> > > +     for (i = 0; i < NUM_COUNTERS; i++) {
> > > +
> > > +             if (!pmu->events[i])
> > > +                     continue;
> > > +
> > > +             event = pmu->events[i];
> > > +
> > > +             ddr_perf_event_update(event);
> >
> > If you already reading this counter out, why not clear its value to 0
> > to avoid having it overflow while at it?
>
> Clear counter need additional lock to make sure previous value is the
> same as hardware one.
> At this time, only cycle counter over flow, the other counter is not over flow.
>
> Overflow bit only work on cycle counter,  the other counter is free running.
>

I am not sure I follow. Sure the counter is free-running, but that
doesn't change the fact that it will eventually overflow. What happens
when during Nth interrupt the value of the given counter is
0xFFFF_FFFF and during N + 1 st interrupt it becomes 0? Perhaps I am
just missing something.

>
> >
> > > +
> > > +             if (event->hw.idx == EVENT_CYCLES_COUNTER)
> > > +                     cycle_event = event;
> > > +     }
> > > +
> > > +     ddr_perf_event_enable(pmu,
> > > +                           EVENT_CYCLES_ID,
> > > +                           EVENT_CYCLES_COUNTER,
> > > +                           true);
> > > +     if (cycle_event)
> > > +             ddr_perf_event_update(cycle_event);
> >
> > Using a bitmaks would allow you to simplify the above to:
> >
> > i = EVENT_CYCLES_COUNTER + 1;
> > for_each_set_bit_from(i, pmu->active_mask, NUM_COUNTERS)
> >    ddr_perf_event_update(pmu->active_events[i]);
> >
> > ddr_perf_event_enable(pmu,
> >                       EVENT_CYCLES_ID,
> >                       EVENT_CYCLES_COUNTER,
> >                       true);
> >
> > if (test_bit(EVENT_CYCLES_COUNTER, pmu->active_mask))
> >     ddr_perf_event_update(pmu->active_events[EVENT_CYCLES_COUNTER]);
> >
> > > +
> > > +     return IRQ_HANDLED;
> > > +}
> >
> > > +
> > > +static int ddr_perf_probe(struct platform_device *pdev)
> > > +{
> > > +     struct ddr_pmu *pmu;
> > > +     struct device_node *np;
> > > +     void __iomem *base;
> > > +     struct resource *iomem;
> > > +     char *name;
> > > +     char *hpname;
> > > +     int num;
> > > +     int ret;
> > > +     int irq;
> > > +
> > > +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +     base = devm_ioremap_resource(&pdev->dev, iomem);
> > > +     if (IS_ERR(base))
> > > +             return PTR_ERR(base);
> > > +
> > > +     np = pdev->dev.of_node;
> > > +
> > > +     pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> > > +     if (!pmu)
> > > +             return -ENOMEM;
> > > +
> > > +     num = ddr_perf_init(pmu, base, &pdev->dev);
> >
> > There's already a unique ID availible - IP block's physical address
> > (iomem->start). Why not use that instead of setting up an IDA?
>
> We want to use 0 based sequence for ddr controller.
> like ddr0/xxx, ddr1/xxxx
>
> If physical address, some user space test script have to update for
> difference chips.
>

That's scheme isn't 100% deterministic and depends on order of
initialization. But if you are dead set on using it, please replace

iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
base = devm_ioremap_resource(&pdev->dev, iomem);

with devm_platform_ioremap_resource()

then.

Thanks,
Andrey Smirnov

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter

[Zhi Li]

On Tue, Apr 30, 2019 at 8:06 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Tue, Apr 30, 2019 at 1:08 PM Zhi Li <lznuaa@gmail.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 1:59 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > >
> > > > Add ddr performance monitor support for iMX8QXP
> > >
> > > > There are 4 counters for ddr perfomance events.
> > > > counter 0 is dedicated for cycles.
> > > > you choose any up to 3 no cycles events.
> > >
> > > > for example:
> > >
> > > > perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
> > > > perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls
> > >
> > > > Support below events.
> > >
> > > >   ddr0/activate/                                     [Kernel PMU event]
> > > >   ddr0/axid-read/                                    [Kernel PMU event]
> > > >   ddr0/axid-write/                                   [Kernel PMU event]
> > > >   ddr0/cycles/                                       [Kernel PMU event]
> > > >   ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
> > > >   ddr0/hp-read/                                      [Kernel PMU event]
> > > >   ddr0/hp-req-nodcredit/                             [Kernel PMU event]
> > > >   ddr0/hp-xact-credit/                               [Kernel PMU event]
> > > >   ddr0/load-mode/                                    [Kernel PMU event]
> > > >   ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
> > > >   ddr0/lp-req-nocredit/                              [Kernel PMU event]
> > > >   ddr0/lp-xact-credit/                               [Kernel PMU event]
> > > >   ddr0/mwr/                                          [Kernel PMU event]
> > > >   ddr0/precharge/                                    [Kernel PMU event]
> > > >   ddr0/raw-hazard/                                   [Kernel PMU event]
> > > >   ddr0/read-access/                                  [Kernel PMU event]
> > > >   ddr0/read-activate/                                [Kernel PMU event]
> > > >   ddr0/read-command/                                 [Kernel PMU event]
> > > >   ddr0/read-cycles/                                  [Kernel PMU event]
> > > >   ddr0/read-modify-write-command/                    [Kernel PMU event]
> > > >   ddr0/read-queue-depth/                             [Kernel PMU event]
> > > >   ddr0/read-write-transition/                        [Kernel PMU event]
> > > >   ddr0/read/                                         [Kernel PMU event]
> > > >   ddr0/refresh/                                      [Kernel PMU event]
> > > >   ddr0/selfresh/                                     [Kernel PMU event]
> > > >   ddr0/wr-xact-credit/                               [Kernel PMU event]
> > > >   ddr0/write-access/                                 [Kernel PMU event]
> > > >   ddr0/write-command/                                [Kernel PMU event]
> > > >   ddr0/write-credit-cnt/                             [Kernel PMU event]
> > > >   ddr0/write-cycles/                                 [Kernel PMU event]
> > > >   ddr0/write-queue-depth/                            [Kernel PMU event]
> > > >   ddr0/write/
> > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > No change from V8 to V9
> > >
> > > > Change from v7 to v8
> > > >  * remove unused define
> > > >  * change total_events to active_events, change active_events to events
> > > >  * remove flags,
> > > >  * fix multi line comments code sytle
> > > >  * add pmu_enable\disable function
> > > >  * disable event at irq handle
> > > >  * remove counter check at ddr_perf_free_counter
> > > >  * remove pmu->irq check
> > > >  * add group check
> > >
> > > > Change from v6 to v7
> > > >  * added irq affinity handle, ref arm-ccn.c
> > > >  * added IRQF_NOBALANCING | IRQF_NO_THREAD
> > > >  * added ida_simple_remove at failure path
> > >
> > > > Change from v5 to v6
> > > >  * fix insmod\rmmod problem
> > > >  * remove randunt register read at irq handle
> > > >  * change u32 irq to int
> > > >  * devm_request_irq use default flags.
> > >
> > > > Change from v4 to v5
> > > >  * Remove AXI ID filter function
> > >
> > > > Change from v3 to v4
> > > >  * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
> > > >  * sort include
> > > >  * remove struct fsl_ddr_devtype_data
> > > >  * Added comment need disable control first
> > > >  * Added comment about must enable cycle counter
> > > >  * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
> > > >  * Added comment about cycle counter is fastest one
> > >
> > > > Change from v2 to v3
> > > >  * remove kfree
> > >
> > > > Change from V1 to V2
> > > >  * update Kconfig by use i.MX8 instead of i.MX8 QXP
> > > >  * remove gpl statememnt since SPDX tag
> > > >  * use dev_kzalloc
> > > >  * use dev_err
> > > >  * commit message show axi_read 0x41\axi_write 0x42
> > > >  * commit message show cycles must be enabled
> > > >  * Irq only issue at cycles overflow
> > > >  * use NUM_COUNTER
> > > >  * use devm_request_irq
> > > >  * add hotplug callback to handle context migration
> > >
> > > Hey Frank,
> > >
> > > I missed your effort to upstream this and ended up spening some time
> > > working on the same thing in parallel, so I have some comments below.
> > >
> > > > +PMU_EVENT_ATTR_STRING(cycles, ddr_perf_cycles, "event=0x00");
> > > > +PMU_EVENT_ATTR_STRING(selfresh, ddr_perf_selfresh, "event=0x01");
> > > > +PMU_EVENT_ATTR_STRING(read-access, ddr_perf_read_accesses, "event=0x04");
> > > > +PMU_EVENT_ATTR_STRING(write-access, ddr_perf_write_accesses, "event=0x05");
> > > > +PMU_EVENT_ATTR_STRING(read-queue-depth, ddr_perf_read_queue_depth,
> > > > +                     "event=0x08");
> > > > +PMU_EVENT_ATTR_STRING(write-queue-depth, ddr_perf_write_queue_depth,
> > > > +                     "event=0x09");
> > > > +PMU_EVENT_ATTR_STRING(lp-read-credit-cnt, ddr_perf_lp_read_credit_cnt,
> > > > +                     "event=0x10");
> > > > +PMU_EVENT_ATTR_STRING(hp-read-credit-cnt, ddr_perf_hp_read_credit_cnt,
> > > > +                     "event=0x11");
> > > > +PMU_EVENT_ATTR_STRING(write-credit-cnt, ddr_perf_write_credit_cnt,
> > > > +                     "event=0x12");
> > > > +PMU_EVENT_ATTR_STRING(read-command, ddr_perf_read_command, "event=0x20");
> > > > +PMU_EVENT_ATTR_STRING(write-command, ddr_perf_write_command, "event=0x21");
> > > > +PMU_EVENT_ATTR_STRING(read-modify-write-command,
> > > > +             ddr_perf_read_modify_write_command, "event=0x22");
> > > > +PMU_EVENT_ATTR_STRING(hp-read, ddr_perf_hp_read, "event=0x23");
> > > > +PMU_EVENT_ATTR_STRING(hp-req-nodcredit, ddr_perf_hp_req_nocredit, "event=0x24");
> > > > +PMU_EVENT_ATTR_STRING(hp-xact-credit, ddr_perf_hp_xact_credit, "event=0x25");
> > > > +PMU_EVENT_ATTR_STRING(lp-req-nocredit, ddr_perf_lp_req_nocredit, "event=0x26");
> > > > +PMU_EVENT_ATTR_STRING(lp-xact-credit, ddr_perf_lp_xact_credit, "event=0x27");
> > > > +PMU_EVENT_ATTR_STRING(wr-xact-credit, ddr_perf_wr_xact_credit, "event=0x29");
> > > > +PMU_EVENT_ATTR_STRING(read-cycles, ddr_perf_read_cycles, "event=0x2a");
> > > > +PMU_EVENT_ATTR_STRING(write-cycles, ddr_perf_write_cycles, "event=0x2b");
> > > > +PMU_EVENT_ATTR_STRING(read-write-transition, ddr_perf_read_write_transition,
> > > > +                     "event=0x30");
> > > > +PMU_EVENT_ATTR_STRING(precharge, ddr_perf_precharge, "event=0x31");
> > > > +PMU_EVENT_ATTR_STRING(activate, ddr_perf_activate, "event=0x32");
> > > > +PMU_EVENT_ATTR_STRING(load-mode, ddr_perf_load_mode, "event=0x33");
> > > > +PMU_EVENT_ATTR_STRING(mwr, ddr_perf_mwr, "event=0x34");
> > > > +PMU_EVENT_ATTR_STRING(read, ddr_perf_read, "event=0x35");
> > > > +PMU_EVENT_ATTR_STRING(read-activate, ddr_perf_read_activate, "event=0x36");
> > > > +PMU_EVENT_ATTR_STRING(refresh, ddr_perf_refresh, "event=0x37");
> > > > +PMU_EVENT_ATTR_STRING(write, ddr_perf_write, "event=0x38");
> > > > +PMU_EVENT_ATTR_STRING(raw-hazard, ddr_perf_raw_hazard, "event=0x39");
> > >
> > > You can really save quite a bit of boilerplate if you define those
> > > inplace with a custom macro and a custom show function:
> > >
> > > static ssize_t
> > > ddr_pmu_event_show(struct device *dev, struct device_attribute *attr,
> > >                    char *page)
> > > {
> > >         struct perf_pmu_events_attr *pmu_attr;
> > >
> > >         pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
> > >         return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
> > > }
> > >
> > > #define IMX8_DDR_PMU_EVENT_ATTR(_name, _id)                             \
> > >         (&((struct perf_pmu_events_attr[]) {                            \
> > >                 { .attr = __ATTR(_name, 0444, ddr_pmu_event_show, NULL), \
> > >                   .id = _id, }                                          \
> > >         })[0].attr.attr)
> > >
> > > static struct attribute *ddr_pmu_events_attrs[] = {
> > >         IMX8_DDR_PMU_EVENT_ATTR(cycles,              CYCLES_EVENT_ID),
> > >         IMX8_DDR_PMU_EVENT_ATTR(selfresh,                       0x01),
> > >         IMX8_DDR_PMU_EVENT_ATTR(read-access,                    0x04),
> > >         IMX8_DDR_PMU_EVENT_ATTR(write-access,                   0x05),
> > >         IMX8_DDR_PMU_EVENT_ATTR(read-queue-depth,               0x08),
> > >
> >
> > I don't think it make any difference.
> >
> > best regards
> > Frank Li
> >
> > > ...
> > >
> > > > +
> > > > +struct ddr_pmu {
> > > > +     struct pmu pmu;
> > > > +     void __iomem *base;
> > > > +     unsigned int cpu;
> > > > +     struct  hlist_node node;
> > > > +     struct  device *dev;
> > >
> > > This device pointer is used only once in ddr_perf_event_init() and
> > > even in that function not all error cases get a dedicated kernel
> > > message. I'd consider just dropping it and the message it is used in.
> > >
> > > > +     struct perf_event *events[NUM_COUNTERS];
> > > > +     int active_events;
> > >
> > > I'd very strongly encourage you to convert the driver to use a proper
> > > bitmask instead of this counter. E.g:
> > >
> > > DECLARE_BITMAP(active_mask, NUM_COUNTERS);
> >
> > I don't think it has big difference at these case.
> >
> > best regards
> > Frank Li
> >
> > >
> > > I'll comment more on that below.
> > >
> > > > +     enum cpuhp_state cpuhp_state;
> > > > +     int irq;
> > > > +     int id;
> > > > +};
> > >
> > > > +static struct attribute_group ddr_perf_events_attr_group = {
> > > > +     .name = "events",
> > > > +     .attrs = ddr_perf_events_attrs,
> > > > +};
> > > > +
> > > > +PMU_FORMAT_ATTR(event, "config:0-63");
> > > > +
> > >
> > > Event ID is really only 8-bits wide, AFAIK. Is there any reason to
> > > reserve all 64 for it in config?
> > >
> > > > +static struct attribute *ddr_perf_format_attrs[] = {
> > > > +     &format_attr_event.attr,
> > > > +     NULL,
> > > > +};
> > > > +
> > > > +static struct attribute_group ddr_perf_format_attr_group = {
> > > > +     .name = "format",
> > > > +     .attrs = ddr_perf_format_attrs,
> > > > +};
> > > > +
> > > > +static const struct attribute_group *attr_groups[] = {
> > > > +     &ddr_perf_events_attr_group,
> > > > +     &ddr_perf_format_attr_group,
> > > > +     &ddr_perf_cpumask_attr_group,
> > > > +     NULL,
> > > > +};
> > > > +
> > > > +static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
> > > > +{
> > > > +     int i;
> > > > +
> > > > +     /*
> > > > +      * Always map cycle event to counter 0
> > > > +      * Cycles counter is dedicated for cycle event
> > > > +      * can't used for the other events
> > > > +      */
> > > > +     if (event == EVENT_CYCLES_ID) {
> > > > +             if (pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > > +                     return EVENT_CYCLES_COUNTER;
> > > > +             else
> > > > +                     return -ENOENT;
> > > > +     }
> > > > +
> > > > +     for (i = 1; i < NUM_COUNTERS; i++) {
> > > > +             if (pmu->events[i] == NULL)
> > > > +                     return i;
> > > > +     }
> > >
> > > This is the first place where using a bitmap would simplify the
> > > driver. Here all you'd need to do is:
> > >
> > >
> > > if (event == CYCLES_EVENT_ID) {
> > >    ...
> > > } else {
> > >   i =  find_next_zero_bit(pmu->active_mask, NUM_COUNTERS,
> > >                           EVENT_CYCLES_COUNTER + 1);
> > >   if (i == NUM_COUNTERS)
> > >         return -EAGAIN;
> > >
> > > }
> > >
> > > set_bit(i, pmu->active_mask);
> > >
> > > ...
> > >
> > > > +
> > > > +     return -ENOENT;
> > > > +}
> > > > +
> > > > +static u32 ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
> > > > +{
> > > > +     pmu->events[counter] = NULL;
> > > > +
> > > > +     return 0;
> > > > +}
> > >
> > > Is this function even necessary? It is used only once in the code and
> > > it's return value is ignored
> >
> > It help improve code read. Generally allocate and free always a pair.
> > I change change to void.
> >
> > best regards
> > Frank Li
> >
> > >
> > > > +
> > > > +static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
> > > > +{
> > > > +     return readl(pmu->base + COUNTER_READ + counter * 4);
> > > > +}
> > >
> > > Would using readl_relaxed() be beneficial here?
> >
> > Maybe. But only few register read when run perf.
> >
> > best regards
> > Frank Li
> >
> > >
> > > > +
> > > > +static int ddr_perf_event_init(struct perf_event *event)
> > > > +{
> > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > +     struct perf_event *sibling;
> > > > +
> > > > +     if (event->attr.type != event->pmu->type)
> > > > +             return -ENOENT;
> > > > +
> > > > +     if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> > > > +             return -EOPNOTSUPP;
> > > > +
> > > > +     if (event->cpu < 0) {
> > > > +             dev_warn(pmu->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       ||
> > >
> > > You don't need any of the above if you pass:
> > >
> > > .capabilities = PERF_PMU_CAP_NO_EXCLUDE
> > >
> > > in your struct pmu initialization
> > >
> > > > +         event->attr.sample_period)
> > > > +             return -EINVAL;
> > > > +
> > > > +     /*
> > > > +      * We must NOT create groups containing mixed PMUs, although software
> > > > +      * events are acceptable (for example to create a CCN group
> > > > +      * periodically read when a hrtimer aka cpu-clock leader triggers).
> > > > +      */
> > > > +     if (event->group_leader->pmu != event->pmu &&
> > > > +                     !is_software_event(event->group_leader))
> > > > +             return -EINVAL;
> > > > +
> > > > +     for_each_sibling_event(sibling, event->group_leader) {
> > > > +             if (sibling->pmu != event->pmu &&
> > > > +                             !is_software_event(sibling))
> > > > +                     return -EINVAL;
> > > > +     }
> > > > +
> > > > +     event->cpu = pmu->cpu;
> > > > +     hwc->idx = -1;
> > > > +
> > > > +     return 0;
> > > > +}
> > >
> > > > +
> > > > +static void ddr_perf_event_enable(struct ddr_pmu *pmu, int config,
> > > > +                               int counter, bool enable)
> > > > +{
> > >
> > > This function doesn't really have anything to do with events (unlike,
> > > for example, ddr_perf_event_start() below). Maybe it would be better
> > > to rename it to ddr_perf_counter_enable()?
> > >
> > > > +     u8 reg = counter * 4 + COUNTER_CNTL;
> > > > +     int val;
> > > > +
> > > > +     if (enable) {
> > > > +             /*
> > > > +              * must disable first, then enable again
> > > > +              * otherwise, cycle counter will not work
> > > > +              * if previous state is enabled.
> > > > +              */
> > > > +             writel(0, pmu->base + reg);
> > > > +             val = CNTL_EN | CNTL_CLEAR;
> > > > +             val |= (config << CNTL_CSV_SHIFT) & CNTL_CSV_MASK;
> > >
> > > You can simplify the above with FIELD_PREP(CNTL_CSV_MASK, config)
> > >
> > > > +     } else {
> > > > +             /* Disable counter */
> > > > +             val = readl(pmu->base + reg) & CNTL_EN_MASK;
> > >
> > > This register will be blown away by the code in the other if branch
> > > next time the counter is enabled. What's the point of doing
> > > read-modify-write and trying to preserve all but EN bits as they were here?
> > >
> > > Why not just do:
> > >
> > > writel(CNTL_CLEAR, pmu->base + reg);
> > >
> > > ?
> > >
> > > > +     }
> > > > +
> > > > +     writel(val, pmu->base + reg);
> > > > +}
> > > > +
> > > > +static void ddr_perf_event_start(struct perf_event *event, int flags)
> > > > +{
> > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > +     int counter = hwc->idx;
> > > > +
> > > > +     local64_set(&hwc->prev_count, 0);
> > > > +
> > > > +     ddr_perf_event_enable(pmu, event->attr.config, counter, true);
> > > > +}
> > > > +
> > > > +static int ddr_perf_event_add(struct perf_event *event, int flags)
> > > > +{
> > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > +     int counter;
> > > > +     int cfg = event->attr.config;
> > > > +
> > > > +     counter = ddr_perf_alloc_counter(pmu, cfg);
> > > > +     if (counter < 0) {
> > > > +             dev_dbg(pmu->dev, "There are not enough counters\n");
> > > > +             return -EOPNOTSUPP;
> > > > +     }
> > > > +
> > > > +     pmu->events[counter] = event;
> > > > +     pmu->active_events++;
> > > > +     hwc->idx = counter;
> > >
> > > What about hw->state?
> > >
> > > > +
> > > > +     if (flags & PERF_EF_START)
> > > > +             ddr_perf_event_start(event, flags);
> > > > +
> > > > +     local64_set(&hwc->prev_count, ddr_perf_read_counter(pmu, counter));
> > >
> > > What's this local64_set() for? You already clear prev_counter and HW
> > > counter when event is started. This just seems redundant.
> > >
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void ddr_perf_event_stop(struct perf_event *event, int flags)
> > > > +{
> > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > +     int counter = hwc->idx;
> > > > +
> > > > +     ddr_perf_event_enable(pmu, event->attr.config, counter, false);
> > > > +     ddr_perf_event_update(event);
> > >
> > > hw->state isn't updated here either? I'm no expert on perf subsystem,
> > > so maybe it's OK, but most of the other drivers in this category do
> > >
> > > hw->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > >
> > > > +}
> > > > +
> > > > +static void ddr_perf_event_del(struct perf_event *event, int flags)
> > > > +{
> > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > +     int counter = hwc->idx;
> > > > +
> > > > +     ddr_perf_event_stop(event, PERF_EF_UPDATE);
> > > > +
> > > > +     ddr_perf_free_counter(pmu, counter);
> > > > +     pmu->active_events--;
> > > > +     hwc->idx = -1;
> > > > +}
> > > > +
> > > > +static void ddr_perf_pmu_enable(struct pmu *pmu)
> > > > +{
> > > > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > > > +
> > > > +     /* enable cycle counter if cycle is not active event list */
> > > > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > > +             ddr_perf_event_enable(ddr_pmu,
> > > > +                                   EVENT_CYCLES_ID,
> > > > +                                   EVENT_CYCLES_COUNTER,
> > > > +                                   true);
> > > > +}
> > > > +
> > > > +static void ddr_perf_pmu_disable(struct pmu *pmu)
> > > > +{
> > > > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > > > +
> > > > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > > +             ddr_perf_event_enable(ddr_pmu,
> > > > +                                   EVENT_CYCLES_ID,
> > > > +                                   EVENT_CYCLES_COUNTER,
> > > > +                                   false);
> > > > +}
> > >
> > > It seems that both of those functions will enable/disable cycles
> > > counter multiple times if "perf" is called with a list specifying
> > > multiple events but not cycles counter. Not sure if this is
> > > intentional, in case it is not, using a bitmask would allow you to
> > > avoid this by using bitmap_weight(), e.g:
> > >
> > > if (bitmap_weight(pmu->active_mask, NUM_COUNTERS) == 1 &&
> > >     ...
> > >
> > > > +
> > > > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > > > +{
> > > > +     int i;
> > > > +     struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > > > +     struct perf_event *event, *cycle_event = NULL;
> > > > +
> > > > +     /* all counter will stop if cycle counter disabled */
> > > > +     ddr_perf_event_enable(pmu,
> > > > +                           EVENT_CYCLES_ID,
> > > > +                           EVENT_CYCLES_COUNTER,
> > > > +                           false);
> > >
> > > The comment below says that IRQ is only raised when cycles counter
> > > overflow and when that happens all of the counters are stopped. What's
> > > the goal of the code disabling cycles counter above then?
> >
> > Mark suggest disable PMU at irq beginning.  The below is V7 review comments.
> >
> > "That's true (and I had forgotten this), but there's still a potential
> > problem depending on IRQ latency.
> >
> > For example, an overflow might occur just before we do some other
> > programming of the PMU (while the CPU has IRQs disabled) where we
> > restart the cycle counter (and the IRQ is de-asserted).
> >
> > Depending on when the interrupt controller samples the state of that
> > IRQ, and when the CPU takes a resulting interrupt, we may be able to end
> > up in the IRQ handler with the cycle counter enabled. Explicitly
> > disabling the cycle counter avoids that possibility.
> >
> > Regardless, we'll want to move the enable of the cycle counter last to
> > ensure that groups aren't skewed.
> > "
>
> Ah, I'd add this as a comment then.
>
> > >
> > > > +     /*
> > > > +      * When the cycle counter overflows, all counters are stopped,
> > > > +      * and an IRQ is raised. If any other counter overflows, it
> > > > +      * continues counting, and no IRQ is raised.
> > > > +      *
> > > > +      * Cycles occur at least 4 times as often as other events, so we
> > > > +      * can update all events on a cycle counter overflow and not
> > > > +      * lose events.
> > > > +      *
> > > > +      */
> > > > +     for (i = 0; i < NUM_COUNTERS; i++) {
> > > > +
> > > > +             if (!pmu->events[i])
> > > > +                     continue;
> > > > +
> > > > +             event = pmu->events[i];
> > > > +
> > > > +             ddr_perf_event_update(event);
> > >
> > > If you already reading this counter out, why not clear its value to 0
> > > to avoid having it overflow while at it?
> >
> > Clear counter need additional lock to make sure previous value is the
> > same as hardware one.
> > At this time, only cycle counter over flow, the other counter is not over flow.
> >
> > Overflow bit only work on cycle counter,  the other counter is free running.
> >
>
> I am not sure I follow. Sure the counter is free-running, but that
> doesn't change the fact that it will eventually overflow. What happens
> when during Nth interrupt the value of the given counter is
> 0xFFFF_FFFF and during N + 1 st interrupt it becomes 0? Perhaps I am
> just missing something.

it is not problem.
If previous value =0xFFFF_FFFF,  the current counter overflow into 0.
The difference is 0 - 0xFFFF_FFFF in 32bit unsigned int,
which is 1.  1 will be added into internal 64bit variable.

if cur - prev < 0x7FFF_FFFF,  the diff = cur-prev is always correct.

cycle increase speed > 4 times the other events.  so all other events
(cur->prev) always below 0x7FFF_FFFF.

the above method is popular.

>
> >
> > >
> > > > +
> > > > +             if (event->hw.idx == EVENT_CYCLES_COUNTER)
> > > > +                     cycle_event = event;
> > > > +     }
> > > > +
> > > > +     ddr_perf_event_enable(pmu,
> > > > +                           EVENT_CYCLES_ID,
> > > > +                           EVENT_CYCLES_COUNTER,
> > > > +                           true);
> > > > +     if (cycle_event)
> > > > +             ddr_perf_event_update(cycle_event);
> > >
> > > Using a bitmaks would allow you to simplify the above to:
> > >
> > > i = EVENT_CYCLES_COUNTER + 1;
> > > for_each_set_bit_from(i, pmu->active_mask, NUM_COUNTERS)
> > >    ddr_perf_event_update(pmu->active_events[i]);
> > >
> > > ddr_perf_event_enable(pmu,
> > >                       EVENT_CYCLES_ID,
> > >                       EVENT_CYCLES_COUNTER,
> > >                       true);
> > >
> > > if (test_bit(EVENT_CYCLES_COUNTER, pmu->active_mask))
> > >     ddr_perf_event_update(pmu->active_events[EVENT_CYCLES_COUNTER]);
> > >
> > > > +
> > > > +     return IRQ_HANDLED;
> > > > +}
> > >
> > > > +
> > > > +static int ddr_perf_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct ddr_pmu *pmu;
> > > > +     struct device_node *np;
> > > > +     void __iomem *base;
> > > > +     struct resource *iomem;
> > > > +     char *name;
> > > > +     char *hpname;
> > > > +     int num;
> > > > +     int ret;
> > > > +     int irq;
> > > > +
> > > > +     iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +     base = devm_ioremap_resource(&pdev->dev, iomem);
> > > > +     if (IS_ERR(base))
> > > > +             return PTR_ERR(base);
> > > > +
> > > > +     np = pdev->dev.of_node;
> > > > +
> > > > +     pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> > > > +     if (!pmu)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     num = ddr_perf_init(pmu, base, &pdev->dev);
> > >
> > > There's already a unique ID availible - IP block's physical address
> > > (iomem->start). Why not use that instead of setting up an IDA?
> >
> > We want to use 0 based sequence for ddr controller.
> > like ddr0/xxx, ddr1/xxxx
> >
> > If physical address, some user space test script have to update for
> > difference chips.
> >
>
> That's scheme isn't 100% deterministic and depends on order of
> initialization. But if you are dead set on using it, please replace

We don't care exactly DDR channel at all.
We just care total bandwidth like ddr0/read-cycles + ddr1/read-cycles.
and balance between ddr0 and ddr1.

if ddr0/read-cycles > ddr1/read-cycles,  it should be some interleave
setting wrong.

best regards
Frank Li

>
> iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> base = devm_ioremap_resource(&pdev->dev, iomem);
>
> with devm_platform_ioremap_resource()

Okay, will update at v11.


>
> then.
>
> Thanks,
> Andrey Smirnov

Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter

[Andrey Smirnov]

On Wed, May 1, 2019 at 7:16 AM Zhi Li <lznuaa@gmail.com> wrote:
>
> On Tue, Apr 30, 2019 at 8:06 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > On Tue, Apr 30, 2019 at 1:08 PM Zhi Li <lznuaa@gmail.com> wrote:
> > >
> > > On Tue, Apr 30, 2019 at 1:59 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > >
> > > > > Add ddr performance monitor support for iMX8QXP
> > > >
> > > > > There are 4 counters for ddr perfomance events.
> > > > > counter 0 is dedicated for cycles.
> > > > > you choose any up to 3 no cycles events.
> > > >
> > > > > for example:
> > > >
> > > > > perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
> > > > > perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls
> > > >
> > > > > Support below events.
> > > >
> > > > >   ddr0/activate/                                     [Kernel PMU event]
> > > > >   ddr0/axid-read/                                    [Kernel PMU event]
> > > > >   ddr0/axid-write/                                   [Kernel PMU event]
> > > > >   ddr0/cycles/                                       [Kernel PMU event]
> > > > >   ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
> > > > >   ddr0/hp-read/                                      [Kernel PMU event]
> > > > >   ddr0/hp-req-nodcredit/                             [Kernel PMU event]
> > > > >   ddr0/hp-xact-credit/                               [Kernel PMU event]
> > > > >   ddr0/load-mode/                                    [Kernel PMU event]
> > > > >   ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
> > > > >   ddr0/lp-req-nocredit/                              [Kernel PMU event]
> > > > >   ddr0/lp-xact-credit/                               [Kernel PMU event]
> > > > >   ddr0/mwr/                                          [Kernel PMU event]
> > > > >   ddr0/precharge/                                    [Kernel PMU event]
> > > > >   ddr0/raw-hazard/                                   [Kernel PMU event]
> > > > >   ddr0/read-access/                                  [Kernel PMU event]
> > > > >   ddr0/read-activate/                                [Kernel PMU event]
> > > > >   ddr0/read-command/                                 [Kernel PMU event]
> > > > >   ddr0/read-cycles/                                  [Kernel PMU event]
> > > > >   ddr0/read-modify-write-command/                    [Kernel PMU event]
> > > > >   ddr0/read-queue-depth/                             [Kernel PMU event]
> > > > >   ddr0/read-write-transition/                        [Kernel PMU event]
> > > > >   ddr0/read/                                         [Kernel PMU event]
> > > > >   ddr0/refresh/                                      [Kernel PMU event]
> > > > >   ddr0/selfresh/                                     [Kernel PMU event]
> > > > >   ddr0/wr-xact-credit/                               [Kernel PMU event]
> > > > >   ddr0/write-access/                                 [Kernel PMU event]
> > > > >   ddr0/write-command/                                [Kernel PMU event]
> > > > >   ddr0/write-credit-cnt/                             [Kernel PMU event]
> > > > >   ddr0/write-cycles/                                 [Kernel PMU event]
> > > > >   ddr0/write-queue-depth/                            [Kernel PMU event]
> > > > >   ddr0/write/
> > > >
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > > No change from V8 to V9
> > > >
> > > > > Change from v7 to v8
> > > > >  * remove unused define
> > > > >  * change total_events to active_events, change active_events to events
> > > > >  * remove flags,
> > > > >  * fix multi line comments code sytle
> > > > >  * add pmu_enable\disable function
> > > > >  * disable event at irq handle
> > > > >  * remove counter check at ddr_perf_free_counter
> > > > >  * remove pmu->irq check
> > > > >  * add group check
> > > >
> > > > > Change from v6 to v7
> > > > >  * added irq affinity handle, ref arm-ccn.c
> > > > >  * added IRQF_NOBALANCING | IRQF_NO_THREAD
> > > > >  * added ida_simple_remove at failure path
> > > >
> > > > > Change from v5 to v6
> > > > >  * fix insmod\rmmod problem
> > > > >  * remove randunt register read at irq handle
> > > > >  * change u32 irq to int
> > > > >  * devm_request_irq use default flags.
> > > >
> > > > > Change from v4 to v5
> > > > >  * Remove AXI ID filter function
> > > >
> > > > > Change from v3 to v4
> > > > >  * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
> > > > >  * sort include
> > > > >  * remove struct fsl_ddr_devtype_data
> > > > >  * Added comment need disable control first
> > > > >  * Added comment about must enable cycle counter
> > > > >  * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
> > > > >  * Added comment about cycle counter is fastest one
> > > >
> > > > > Change from v2 to v3
> > > > >  * remove kfree
> > > >
> > > > > Change from V1 to V2
> > > > >  * update Kconfig by use i.MX8 instead of i.MX8 QXP
> > > > >  * remove gpl statememnt since SPDX tag
> > > > >  * use dev_kzalloc
> > > > >  * use dev_err
> > > > >  * commit message show axi_read 0x41\axi_write 0x42
> > > > >  * commit message show cycles must be enabled
> > > > >  * Irq only issue at cycles overflow
> > > > >  * use NUM_COUNTER
> > > > >  * use devm_request_irq
> > > > >  * add hotplug callback to handle context migration
> > > >
> > > > Hey Frank,
> > > >
> > > > I missed your effort to upstream this and ended up spening some time
> > > > working on the same thing in parallel, so I have some comments below.
> > > >
> > > > > +PMU_EVENT_ATTR_STRING(cycles, ddr_perf_cycles, "event=0x00");
> > > > > +PMU_EVENT_ATTR_STRING(selfresh, ddr_perf_selfresh, "event=0x01");
> > > > > +PMU_EVENT_ATTR_STRING(read-access, ddr_perf_read_accesses, "event=0x04");
> > > > > +PMU_EVENT_ATTR_STRING(write-access, ddr_perf_write_accesses, "event=0x05");
> > > > > +PMU_EVENT_ATTR_STRING(read-queue-depth, ddr_perf_read_queue_depth,
> > > > > +                     "event=0x08");
> > > > > +PMU_EVENT_ATTR_STRING(write-queue-depth, ddr_perf_write_queue_depth,
> > > > > +                     "event=0x09");
> > > > > +PMU_EVENT_ATTR_STRING(lp-read-credit-cnt, ddr_perf_lp_read_credit_cnt,
> > > > > +                     "event=0x10");
> > > > > +PMU_EVENT_ATTR_STRING(hp-read-credit-cnt, ddr_perf_hp_read_credit_cnt,
> > > > > +                     "event=0x11");
> > > > > +PMU_EVENT_ATTR_STRING(write-credit-cnt, ddr_perf_write_credit_cnt,
> > > > > +                     "event=0x12");
> > > > > +PMU_EVENT_ATTR_STRING(read-command, ddr_perf_read_command, "event=0x20");
> > > > > +PMU_EVENT_ATTR_STRING(write-command, ddr_perf_write_command, "event=0x21");
> > > > > +PMU_EVENT_ATTR_STRING(read-modify-write-command,
> > > > > +             ddr_perf_read_modify_write_command, "event=0x22");
> > > > > +PMU_EVENT_ATTR_STRING(hp-read, ddr_perf_hp_read, "event=0x23");
> > > > > +PMU_EVENT_ATTR_STRING(hp-req-nodcredit, ddr_perf_hp_req_nocredit, "event=0x24");
> > > > > +PMU_EVENT_ATTR_STRING(hp-xact-credit, ddr_perf_hp_xact_credit, "event=0x25");
> > > > > +PMU_EVENT_ATTR_STRING(lp-req-nocredit, ddr_perf_lp_req_nocredit, "event=0x26");
> > > > > +PMU_EVENT_ATTR_STRING(lp-xact-credit, ddr_perf_lp_xact_credit, "event=0x27");
> > > > > +PMU_EVENT_ATTR_STRING(wr-xact-credit, ddr_perf_wr_xact_credit, "event=0x29");
> > > > > +PMU_EVENT_ATTR_STRING(read-cycles, ddr_perf_read_cycles, "event=0x2a");
> > > > > +PMU_EVENT_ATTR_STRING(write-cycles, ddr_perf_write_cycles, "event=0x2b");
> > > > > +PMU_EVENT_ATTR_STRING(read-write-transition, ddr_perf_read_write_transition,
> > > > > +                     "event=0x30");
> > > > > +PMU_EVENT_ATTR_STRING(precharge, ddr_perf_precharge, "event=0x31");
> > > > > +PMU_EVENT_ATTR_STRING(activate, ddr_perf_activate, "event=0x32");
> > > > > +PMU_EVENT_ATTR_STRING(load-mode, ddr_perf_load_mode, "event=0x33");
> > > > > +PMU_EVENT_ATTR_STRING(mwr, ddr_perf_mwr, "event=0x34");
> > > > > +PMU_EVENT_ATTR_STRING(read, ddr_perf_read, "event=0x35");
> > > > > +PMU_EVENT_ATTR_STRING(read-activate, ddr_perf_read_activate, "event=0x36");
> > > > > +PMU_EVENT_ATTR_STRING(refresh, ddr_perf_refresh, "event=0x37");
> > > > > +PMU_EVENT_ATTR_STRING(write, ddr_perf_write, "event=0x38");
> > > > > +PMU_EVENT_ATTR_STRING(raw-hazard, ddr_perf_raw_hazard, "event=0x39");
> > > >
> > > > You can really save quite a bit of boilerplate if you define those
> > > > inplace with a custom macro and a custom show function:
> > > >
> > > > static ssize_t
> > > > ddr_pmu_event_show(struct device *dev, struct device_attribute *attr,
> > > >                    char *page)
> > > > {
> > > >         struct perf_pmu_events_attr *pmu_attr;
> > > >
> > > >         pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
> > > >         return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
> > > > }
> > > >
> > > > #define IMX8_DDR_PMU_EVENT_ATTR(_name, _id)                             \
> > > >         (&((struct perf_pmu_events_attr[]) {                            \
> > > >                 { .attr = __ATTR(_name, 0444, ddr_pmu_event_show, NULL), \
> > > >                   .id = _id, }                                          \
> > > >         })[0].attr.attr)
> > > >
> > > > static struct attribute *ddr_pmu_events_attrs[] = {
> > > >         IMX8_DDR_PMU_EVENT_ATTR(cycles,              CYCLES_EVENT_ID),
> > > >         IMX8_DDR_PMU_EVENT_ATTR(selfresh,                       0x01),
> > > >         IMX8_DDR_PMU_EVENT_ATTR(read-access,                    0x04),
> > > >         IMX8_DDR_PMU_EVENT_ATTR(write-access,                   0x05),
> > > >         IMX8_DDR_PMU_EVENT_ATTR(read-queue-depth,               0x08),
> > > >
> > >
> > > I don't think it make any difference.
> > >
> > > best regards
> > > Frank Li
> > >
> > > > ...
> > > >
> > > > > +
> > > > > +struct ddr_pmu {
> > > > > +     struct pmu pmu;
> > > > > +     void __iomem *base;
> > > > > +     unsigned int cpu;
> > > > > +     struct  hlist_node node;
> > > > > +     struct  device *dev;
> > > >
> > > > This device pointer is used only once in ddr_perf_event_init() and
> > > > even in that function not all error cases get a dedicated kernel
> > > > message. I'd consider just dropping it and the message it is used in.
> > > >
> > > > > +     struct perf_event *events[NUM_COUNTERS];
> > > > > +     int active_events;
> > > >
> > > > I'd very strongly encourage you to convert the driver to use a proper
> > > > bitmask instead of this counter. E.g:
> > > >
> > > > DECLARE_BITMAP(active_mask, NUM_COUNTERS);
> > >
> > > I don't think it has big difference at these case.
> > >
> > > best regards
> > > Frank Li
> > >
> > > >
> > > > I'll comment more on that below.
> > > >
> > > > > +     enum cpuhp_state cpuhp_state;
> > > > > +     int irq;
> > > > > +     int id;
> > > > > +};
> > > >
> > > > > +static struct attribute_group ddr_perf_events_attr_group = {
> > > > > +     .name = "events",
> > > > > +     .attrs = ddr_perf_events_attrs,
> > > > > +};
> > > > > +
> > > > > +PMU_FORMAT_ATTR(event, "config:0-63");
> > > > > +
> > > >
> > > > Event ID is really only 8-bits wide, AFAIK. Is there any reason to
> > > > reserve all 64 for it in config?
> > > >
> > > > > +static struct attribute *ddr_perf_format_attrs[] = {
> > > > > +     &format_attr_event.attr,
> > > > > +     NULL,
> > > > > +};
> > > > > +
> > > > > +static struct attribute_group ddr_perf_format_attr_group = {
> > > > > +     .name = "format",
> > > > > +     .attrs = ddr_perf_format_attrs,
> > > > > +};
> > > > > +
> > > > > +static const struct attribute_group *attr_groups[] = {
> > > > > +     &ddr_perf_events_attr_group,
> > > > > +     &ddr_perf_format_attr_group,
> > > > > +     &ddr_perf_cpumask_attr_group,
> > > > > +     NULL,
> > > > > +};
> > > > > +
> > > > > +static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event)
> > > > > +{
> > > > > +     int i;
> > > > > +
> > > > > +     /*
> > > > > +      * Always map cycle event to counter 0
> > > > > +      * Cycles counter is dedicated for cycle event
> > > > > +      * can't used for the other events
> > > > > +      */
> > > > > +     if (event == EVENT_CYCLES_ID) {
> > > > > +             if (pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > > > +                     return EVENT_CYCLES_COUNTER;
> > > > > +             else
> > > > > +                     return -ENOENT;
> > > > > +     }
> > > > > +
> > > > > +     for (i = 1; i < NUM_COUNTERS; i++) {
> > > > > +             if (pmu->events[i] == NULL)
> > > > > +                     return i;
> > > > > +     }
> > > >
> > > > This is the first place where using a bitmap would simplify the
> > > > driver. Here all you'd need to do is:
> > > >
> > > >
> > > > if (event == CYCLES_EVENT_ID) {
> > > >    ...
> > > > } else {
> > > >   i =  find_next_zero_bit(pmu->active_mask, NUM_COUNTERS,
> > > >                           EVENT_CYCLES_COUNTER + 1);
> > > >   if (i == NUM_COUNTERS)
> > > >         return -EAGAIN;
> > > >
> > > > }
> > > >
> > > > set_bit(i, pmu->active_mask);
> > > >
> > > > ...
> > > >
> > > > > +
> > > > > +     return -ENOENT;
> > > > > +}
> > > > > +
> > > > > +static u32 ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
> > > > > +{
> > > > > +     pmu->events[counter] = NULL;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > >
> > > > Is this function even necessary? It is used only once in the code and
> > > > it's return value is ignored
> > >
> > > It help improve code read. Generally allocate and free always a pair.
> > > I change change to void.
> > >
> > > best regards
> > > Frank Li
> > >
> > > >
> > > > > +
> > > > > +static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
> > > > > +{
> > > > > +     return readl(pmu->base + COUNTER_READ + counter * 4);
> > > > > +}
> > > >
> > > > Would using readl_relaxed() be beneficial here?
> > >
> > > Maybe. But only few register read when run perf.
> > >
> > > best regards
> > > Frank Li
> > >
> > > >
> > > > > +
> > > > > +static int ddr_perf_event_init(struct perf_event *event)
> > > > > +{
> > > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > > +     struct perf_event *sibling;
> > > > > +
> > > > > +     if (event->attr.type != event->pmu->type)
> > > > > +             return -ENOENT;
> > > > > +
> > > > > +     if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> > > > > +             return -EOPNOTSUPP;
> > > > > +
> > > > > +     if (event->cpu < 0) {
> > > > > +             dev_warn(pmu->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       ||
> > > >
> > > > You don't need any of the above if you pass:
> > > >
> > > > .capabilities = PERF_PMU_CAP_NO_EXCLUDE
> > > >
> > > > in your struct pmu initialization
> > > >
> > > > > +         event->attr.sample_period)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     /*
> > > > > +      * We must NOT create groups containing mixed PMUs, although software
> > > > > +      * events are acceptable (for example to create a CCN group
> > > > > +      * periodically read when a hrtimer aka cpu-clock leader triggers).
> > > > > +      */
> > > > > +     if (event->group_leader->pmu != event->pmu &&
> > > > > +                     !is_software_event(event->group_leader))
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     for_each_sibling_event(sibling, event->group_leader) {
> > > > > +             if (sibling->pmu != event->pmu &&
> > > > > +                             !is_software_event(sibling))
> > > > > +                     return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     event->cpu = pmu->cpu;
> > > > > +     hwc->idx = -1;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > >
> > > > > +
> > > > > +static void ddr_perf_event_enable(struct ddr_pmu *pmu, int config,
> > > > > +                               int counter, bool enable)
> > > > > +{
> > > >
> > > > This function doesn't really have anything to do with events (unlike,
> > > > for example, ddr_perf_event_start() below). Maybe it would be better
> > > > to rename it to ddr_perf_counter_enable()?
> > > >
> > > > > +     u8 reg = counter * 4 + COUNTER_CNTL;
> > > > > +     int val;
> > > > > +
> > > > > +     if (enable) {
> > > > > +             /*
> > > > > +              * must disable first, then enable again
> > > > > +              * otherwise, cycle counter will not work
> > > > > +              * if previous state is enabled.
> > > > > +              */
> > > > > +             writel(0, pmu->base + reg);
> > > > > +             val = CNTL_EN | CNTL_CLEAR;
> > > > > +             val |= (config << CNTL_CSV_SHIFT) & CNTL_CSV_MASK;
> > > >
> > > > You can simplify the above with FIELD_PREP(CNTL_CSV_MASK, config)
> > > >
> > > > > +     } else {
> > > > > +             /* Disable counter */
> > > > > +             val = readl(pmu->base + reg) & CNTL_EN_MASK;
> > > >
> > > > This register will be blown away by the code in the other if branch
> > > > next time the counter is enabled. What's the point of doing
> > > > read-modify-write and trying to preserve all but EN bits as they were here?
> > > >
> > > > Why not just do:
> > > >
> > > > writel(CNTL_CLEAR, pmu->base + reg);
> > > >
> > > > ?
> > > >
> > > > > +     }
> > > > > +
> > > > > +     writel(val, pmu->base + reg);
> > > > > +}
> > > > > +
> > > > > +static void ddr_perf_event_start(struct perf_event *event, int flags)
> > > > > +{
> > > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > > +     int counter = hwc->idx;
> > > > > +
> > > > > +     local64_set(&hwc->prev_count, 0);
> > > > > +
> > > > > +     ddr_perf_event_enable(pmu, event->attr.config, counter, true);
> > > > > +}
> > > > > +
> > > > > +static int ddr_perf_event_add(struct perf_event *event, int flags)
> > > > > +{
> > > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > > +     int counter;
> > > > > +     int cfg = event->attr.config;
> > > > > +
> > > > > +     counter = ddr_perf_alloc_counter(pmu, cfg);
> > > > > +     if (counter < 0) {
> > > > > +             dev_dbg(pmu->dev, "There are not enough counters\n");
> > > > > +             return -EOPNOTSUPP;
> > > > > +     }
> > > > > +
> > > > > +     pmu->events[counter] = event;
> > > > > +     pmu->active_events++;
> > > > > +     hwc->idx = counter;
> > > >
> > > > What about hw->state?
> > > >
> > > > > +
> > > > > +     if (flags & PERF_EF_START)
> > > > > +             ddr_perf_event_start(event, flags);
> > > > > +
> > > > > +     local64_set(&hwc->prev_count, ddr_perf_read_counter(pmu, counter));
> > > >
> > > > What's this local64_set() for? You already clear prev_counter and HW
> > > > counter when event is started. This just seems redundant.
> > > >
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static void ddr_perf_event_stop(struct perf_event *event, int flags)
> > > > > +{
> > > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > > +     int counter = hwc->idx;
> > > > > +
> > > > > +     ddr_perf_event_enable(pmu, event->attr.config, counter, false);
> > > > > +     ddr_perf_event_update(event);
> > > >
> > > > hw->state isn't updated here either? I'm no expert on perf subsystem,
> > > > so maybe it's OK, but most of the other drivers in this category do
> > > >
> > > > hw->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > > >
> > > > > +}
> > > > > +
> > > > > +static void ddr_perf_event_del(struct perf_event *event, int flags)
> > > > > +{
> > > > > +     struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > > > > +     struct hw_perf_event *hwc = &event->hw;
> > > > > +     int counter = hwc->idx;
> > > > > +
> > > > > +     ddr_perf_event_stop(event, PERF_EF_UPDATE);
> > > > > +
> > > > > +     ddr_perf_free_counter(pmu, counter);
> > > > > +     pmu->active_events--;
> > > > > +     hwc->idx = -1;
> > > > > +}
> > > > > +
> > > > > +static void ddr_perf_pmu_enable(struct pmu *pmu)
> > > > > +{
> > > > > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > > > > +
> > > > > +     /* enable cycle counter if cycle is not active event list */
> > > > > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > > > +             ddr_perf_event_enable(ddr_pmu,
> > > > > +                                   EVENT_CYCLES_ID,
> > > > > +                                   EVENT_CYCLES_COUNTER,
> > > > > +                                   true);
> > > > > +}
> > > > > +
> > > > > +static void ddr_perf_pmu_disable(struct pmu *pmu)
> > > > > +{
> > > > > +     struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > > > > +
> > > > > +     if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > > > > +             ddr_perf_event_enable(ddr_pmu,
> > > > > +                                   EVENT_CYCLES_ID,
> > > > > +                                   EVENT_CYCLES_COUNTER,
> > > > > +                                   false);
> > > > > +}
> > > >
> > > > It seems that both of those functions will enable/disable cycles
> > > > counter multiple times if "perf" is called with a list specifying
> > > > multiple events but not cycles counter. Not sure if this is
> > > > intentional, in case it is not, using a bitmask would allow you to
> > > > avoid this by using bitmap_weight(), e.g:
> > > >
> > > > if (bitmap_weight(pmu->active_mask, NUM_COUNTERS) == 1 &&
> > > >     ...
> > > >
> > > > > +
> > > > > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > > > > +{
> > > > > +     int i;
> > > > > +     struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > > > > +     struct perf_event *event, *cycle_event = NULL;
> > > > > +
> > > > > +     /* all counter will stop if cycle counter disabled */
> > > > > +     ddr_perf_event_enable(pmu,
> > > > > +                           EVENT_CYCLES_ID,
> > > > > +                           EVENT_CYCLES_COUNTER,
> > > > > +                           false);
> > > >
> > > > The comment below says that IRQ is only raised when cycles counter
> > > > overflow and when that happens all of the counters are stopped. What's
> > > > the goal of the code disabling cycles counter above then?
> > >
> > > Mark suggest disable PMU at irq beginning.  The below is V7 review comments.
> > >
> > > "That's true (and I had forgotten this), but there's still a potential
> > > problem depending on IRQ latency.
> > >
> > > For example, an overflow might occur just before we do some other
> > > programming of the PMU (while the CPU has IRQs disabled) where we
> > > restart the cycle counter (and the IRQ is de-asserted).
> > >
> > > Depending on when the interrupt controller samples the state of that
> > > IRQ, and when the CPU takes a resulting interrupt, we may be able to end
> > > up in the IRQ handler with the cycle counter enabled. Explicitly
> > > disabling the cycle counter avoids that possibility.
> > >
> > > Regardless, we'll want to move the enable of the cycle counter last to
> > > ensure that groups aren't skewed.
> > > "
> >
> > Ah, I'd add this as a comment then.
> >
> > > >
> > > > > +     /*
> > > > > +      * When the cycle counter overflows, all counters are stopped,
> > > > > +      * and an IRQ is raised. If any other counter overflows, it
> > > > > +      * continues counting, and no IRQ is raised.
> > > > > +      *
> > > > > +      * Cycles occur at least 4 times as often as other events, so we
> > > > > +      * can update all events on a cycle counter overflow and not
> > > > > +      * lose events.
> > > > > +      *
> > > > > +      */
> > > > > +     for (i = 0; i < NUM_COUNTERS; i++) {
> > > > > +
> > > > > +             if (!pmu->events[i])
> > > > > +                     continue;
> > > > > +
> > > > > +             event = pmu->events[i];
> > > > > +
> > > > > +             ddr_perf_event_update(event);
> > > >
> > > > If you already reading this counter out, why not clear its value to 0
> > > > to avoid having it overflow while at it?
> > >
> > > Clear counter need additional lock to make sure previous value is the
> > > same as hardware one.
> > > At this time, only cycle counter over flow, the other counter is not over flow.
> > >
> > > Overflow bit only work on cycle counter,  the other counter is free running.
> > >
> >
> > I am not sure I follow. Sure the counter is free-running, but that
> > doesn't change the fact that it will eventually overflow. What happens
> > when during Nth interrupt the value of the given counter is
> > 0xFFFF_FFFF and during N + 1 st interrupt it becomes 0? Perhaps I am
> > just missing something.
>
> it is not problem.
> If previous value =0xFFFF_FFFF,  the current counter overflow into 0.
> The difference is 0 - 0xFFFF_FFFF in 32bit unsigned int,
> which is 1.  1 will be added into internal 64bit variable.
>
> if cur - prev < 0x7FFF_FFFF,  the diff = cur-prev is always correct.
>
> cycle increase speed > 4 times the other events.  so all other events
> (cur->prev) always below 0x7FFF_FFFF.
>
> the above method is popular.
>

Ah, I see, thanks for the explanation.

Thanks,
Andrey Smirnov

[PATCH V9 3/4] arm64: dts: imx8qxp: added ddr performance monitor nodes

[Frank Li]

Add ddr performance monitor

Signed-off-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Change from v8 to v9
* put ddr-pmu under ddr_subsystem bus

Change from v3 to v8
* none

Change from v2 to v3
* ddr_pmu0 -> ddr-pmu

 arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 0683ee2..b66f713 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -378,6 +378,20 @@
 		};
 	};
 
+	ddr_subsyss: bus@5c000000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x5c000000 0x0 0x5c000000 0x1000000>;
+
+	        ddr-pmu@5c020000 {
+			compatible = "fsl,imx8-ddr-pmu";
+			reg = <0x5c020000 0x10000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>;
+		};
+	};
+
 	lsio_subsys: bus@5d000000 {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.5.2

[PATCH V9 4/4] MAINTAINERS: Added imx DDR performonitor driver maintainer information

[Frank Li]

Add DDR perf counter driver maintainer information

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
No change from v1 to v9

 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2c7d4e1..6a9868b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6264,6 +6264,13 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/busses/i2c-cpm.c
 
+FREESCALE IMX DDR Performance Monitor DRIVER
+M:	Frank Li <Frank.li@nxp.com>
+L:	linux-arm-kernel@lists.infradead.org
+S:	Maintained
+F:	drivers/perf/fsl_imx8_ddr_perf.c
+F:	Documentation/devicetree/bindings/perf/fsl-imx-ddr.txt
+
 FREESCALE IMX LPI2C DRIVER
 M:	Dong Aisheng <aisheng.dong@nxp.com>
 L:	linux-i2c@vger.kernel.org
-- 
2.5.2