[PATCH v2 0/4] cn10k DDR Performance monitor support

[PATCH v2 0/4] cn10k DDR Performance monitor support

[Bharat Bhushan]

This patch series adds DDR performance monitor support
on Marvell cn10k series of processor.

First patch adds device tree binding changes.
Second patch add basic support (without overflow and event
ownership). Third and fourth patch adds overflow and event
ownership respectively.

Seems like 4th patch can be merged in second patch,
For easy review it is currently separate
 
v1->v2:
 - DT binding changed to new DT Schema
 - writeq/readq changed to respective relaxed
 - Using PMU_EVENT_ATTR_ID

Bharat Bhushan (4):
  dt-bindings: perf: marvell: cn10k ddr performance monitor
  perf/marvell: CN10k DDR performance monitor support
  perf/marvell: cn10k DDR perfmon event overflow handling
  perf/marvell: cn10k DDR perf event core ownership

 .../bindings/perf/marvell-cn10k-ddr.yaml      |  37 +
 drivers/perf/Kconfig                          |   7 +
 drivers/perf/Makefile                         |   1 +
 drivers/perf/marvell_cn10k_ddr_pmu.c          | 763 ++++++++++++++++++
 include/linux/cpuhotplug.h                    |   1 +
 5 files changed, 809 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
 create mode 100644 drivers/perf/marvell_cn10k_ddr_pmu.c

-- 
2.17.1

[PATCH v2 1/4] dt-bindings: perf: marvell: cn10k ddr performance monitor

[Bharat Bhushan]

Add binding documentation for the Marvell CN10k DDR
performance monitor unit.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v1->v2:
 - DT binding changed to new DT Schema

 .../bindings/perf/marvell-cn10k-ddr.yaml      | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml

diff --git a/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml b/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
new file mode 100644
index 000000000000..2a335444cf53
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/marvell-cn10k-ddr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell CN10K DDR performance monitor
+
+maintainers:
+  - Bharat Bhushan <bbhushan2@marvell.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - marvell,cn10k-ddr-pmu
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        ddrcpmu@1 {
+            compatible = "marvell,cn10k-ddr-pmu";
+            reg = <0x87e1 0xc0000000 0x0 0x10000>;
+        };
+    };
-- 
2.17.1

[PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor support

[Bharat Bhushan]

Marvell CN10k DRAM Subsystem (DSS) supports eight
event counters for monitoring performance and software
can program each counter to monitor any of the defined
performance event. Performance events are for interface
between the DDR controller and the PHY, interface between
the DDR Controller and the CHI interconnect, or within
the DDR Controller. Additionally DSS also supports two
fixed performance event counters, one for number of
ddr reads and other for ddr writes.

This patch add basic support for these performance
monitoring events on CN10k.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v1->v2:
 - writeq/readq changed to respective relaxed version
 - Using PMU_EVENT_ATTR_ID

 drivers/perf/Kconfig                 |   7 +
 drivers/perf/Makefile                |   1 +
 drivers/perf/marvell_cn10k_ddr_pmu.c | 606 +++++++++++++++++++++++++++
 3 files changed, 614 insertions(+)
 create mode 100644 drivers/perf/marvell_cn10k_ddr_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 77522e5efe11..41a80a4b8d29 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -139,4 +139,11 @@ config ARM_DMC620_PMU
 
 source "drivers/perf/hisilicon/Kconfig"
 
+config MARVELL_CN10K_DDR_PMU
+	tristate "Enable MARVELL CN10K DRAM Subsystem(DSS) PMU Support"
+	depends on ARM64
+	help
+	  Enable perf support for Marvell DDR Performance monitoring
+	  event on CN10K platform.
+
 endmenu
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index 5260b116c7da..ee1126219d8d 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
 obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
 obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
 obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
+obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
new file mode 100644
index 000000000000..8f9e3d1fcd8d
--- /dev/null
+++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
@@ -0,0 +1,606 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
+ *
+ * Copyright (C) 2021 Marvell.
+ */
+
+#include <linux/init.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/perf_event.h>
+
+/* Performance Counters Operating Mode Control Registers */
+#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
+#define OP_MODE_CTRL_VAL_MANNUAL	0x1
+
+/* Performance Counters Start Operation Control Registers */
+#define DDRC_PERF_CNT_START_OP_CTRL	0x8028
+#define START_OP_CTRL_VAL_START		0x1ULL
+#define START_OP_CTRL_VAL_ACTIVE	0x2
+
+/* Performance Counters End Operation Control Registers */
+#define DDRC_PERF_CNT_END_OP_CTRL	0x8030
+#define END_OP_CTRL_VAL_END		0x1ULL
+
+/* Performance Counters End Status Registers */
+#define DDRC_PERF_CNT_END_STATUS		0x8038
+#define END_STATUS_VAL_END_TIMER_MODE_END	0x1
+
+/* Performance Counters Configuration Registers */
+#define DDRC_PERF_CFG_BASE		0x8040
+
+/* 8 Generic event counter + 2 fixed event counters */
+#define DDRC_PERF_NUM_GEN_COUNTERS	8
+#define DDRC_PERF_NUM_FIX_COUNTERS	2
+#define DDRC_PERF_READ_COUNTER_IDX	DDRC_PERF_NUM_GEN_COUNTERS
+#define DDRC_PERF_WRITE_COUNTER_IDX	(DDRC_PERF_NUM_GEN_COUNTERS + 1)
+#define DDRC_PERF_NUM_COUNTERS		(DDRC_PERF_NUM_GEN_COUNTERS + \
+					 DDRC_PERF_NUM_FIX_COUNTERS)
+
+/* Generic event counter registers */
+#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
+#define EVENT_ENABLE			BIT_ULL(63)
+
+/* Two dedicated event counters for DDR reads and writes */
+#define EVENT_DDR_READS			101
+#define EVENT_DDR_WRITES		100
+
+/*
+ * programmable events IDs in programmable event counters.
+ * DO NOT change these event-id numbers, they are used to
+ * program event bitmap in h/w.
+ */
+#define EVENT_OP_IS_ZQLATCH			55
+#define EVENT_OP_IS_ZQSTART			54
+#define EVENT_OP_IS_TCR_MRR			53
+#define EVENT_OP_IS_DQSOSC_MRR			52
+#define EVENT_OP_IS_DQSOSC_MPC			51
+#define EVENT_VISIBLE_WIN_LIMIT_REACHED_WR	50
+#define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
+#define EVENT_BSM_STARVATION			48
+#define EVENT_BSM_ALLOC				47
+#define EVENT_LPR_REQ_WITH_NOCREDIT		46
+#define EVENT_HPR_REQ_WITH_NOCREDIT		45
+#define EVENT_OP_IS_ZQCS			44
+#define EVENT_OP_IS_ZQCL			43
+#define EVENT_OP_IS_LOAD_MODE			42
+#define EVENT_OP_IS_SPEC_REF			41
+#define EVENT_OP_IS_CRIT_REF			40
+#define EVENT_OP_IS_REFRESH			39
+#define EVENT_OP_IS_ENTER_MPSM			35
+#define EVENT_OP_IS_ENTER_POWERDOWN		31
+#define EVENT_OP_IS_ENTER_SELFREF		27
+#define EVENT_WAW_HAZARD			26
+#define EVENT_RAW_HAZARD			25
+#define EVENT_WAR_HAZARD			24
+#define EVENT_WRITE_COMBINE			23
+#define EVENT_RDWR_TRANSITIONS			22
+#define EVENT_PRECHARGE_FOR_OTHER		21
+#define EVENT_PRECHARGE_FOR_RDWR		20
+#define EVENT_OP_IS_PRECHARGE			19
+#define EVENT_OP_IS_MWR				18
+#define EVENT_OP_IS_WR				17
+#define EVENT_OP_IS_RD				16
+#define EVENT_OP_IS_RD_ACTIVATE			15
+#define EVENT_OP_IS_RD_OR_WR			14
+#define EVENT_OP_IS_ACTIVATE			13
+#define EVENT_WR_XACT_WHEN_CRITICAL		12
+#define EVENT_LPR_XACT_WHEN_CRITICAL		11
+#define EVENT_HPR_XACT_WHEN_CRITICAL		10
+#define EVENT_DFI_RD_DATA_CYCLES		9
+#define EVENT_DFI_WR_DATA_CYCLES		8
+#define EVENT_ACT_BYPASS			7
+#define EVENT_READ_BYPASS			6
+#define EVENT_HIF_HI_PRI_RD			5
+#define EVENT_HIF_RMW				4
+#define EVENT_HIF_RD				3
+#define EVENT_HIF_WR				2
+#define EVENT_HIF_RD_OR_WR			1
+
+/* Event counter value registers */
+#define DDRC_PERF_CNT_VALUE_BASE		0x8080
+#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
+
+/* Fixed event counter enable/disable register */
+#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
+#define DDRC_PERF_FREERUN_WRITE_EN	0x1
+#define DDRC_PERF_FREERUN_READ_EN	0x2
+
+/* Fixed event counter control register */
+#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
+#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
+#define DDRC_FREERUN_READ_CNT_CLR	0x2
+
+/* Fixed event counter value register */
+#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
+#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
+#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
+#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
+
+struct cn10k_ddr_pmu {
+	struct pmu pmu;
+	int id;
+	void __iomem *base;
+	unsigned int cpu;
+	struct	device *dev;
+	int active_events;
+	struct perf_event *events[DDRC_PERF_NUM_COUNTERS];
+};
+
+#define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
+
+static ssize_t cn10k_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 CN10K_DDR_PMU_EVENT_ATTR(_name, _id)				     \
+	PMU_EVENT_ATTR_ID(_name, cn10k_ddr_pmu_event_show, _id)
+
+static struct attribute *cn10k_ddr_perf_events_attrs[] = {
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_or_wr_access, EVENT_HIF_RD_OR_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_wr_access, EVENT_HIF_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_access, EVENT_HIF_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rmw_access, EVENT_HIF_RMW),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_pri_rdaccess, EVENT_HIF_HI_PRI_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_bypass_access, EVENT_READ_BYPASS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_act_bypass_access, EVENT_ACT_BYPASS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_wr_data_access, EVENT_DFI_WR_DATA_CYCLES),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_rd_data_access, EVENT_DFI_RD_DATA_CYCLES),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpri_sched_rd_crit_access,
+					EVENT_HPR_XACT_WHEN_CRITICAL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpri_sched_rd_crit_access,
+					EVENT_LPR_XACT_WHEN_CRITICAL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_trxn_crit_access,
+					EVENT_WR_XACT_WHEN_CRITICAL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_active_access, EVENT_OP_IS_ACTIVATE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_or_wr_access, EVENT_OP_IS_RD_OR_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_active_access, EVENT_OP_IS_RD_ACTIVATE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_read, EVENT_OP_IS_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_write, EVENT_OP_IS_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_mwr, EVENT_OP_IS_MWR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge, EVENT_OP_IS_PRECHARGE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_rdwr, EVENT_PRECHARGE_FOR_RDWR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_other,
+					EVENT_PRECHARGE_FOR_OTHER),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_rdwr_transitions, EVENT_RDWR_TRANSITIONS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_write_combine, EVENT_WRITE_COMBINE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_war_hazard, EVENT_WAR_HAZARD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_raw_hazard, EVENT_RAW_HAZARD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_waw_hazard, EVENT_WAW_HAZARD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_selfref, EVENT_OP_IS_ENTER_SELFREF),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_powerdown, EVENT_OP_IS_ENTER_POWERDOWN),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_mpsm, EVENT_OP_IS_ENTER_MPSM),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_refresh, EVENT_OP_IS_REFRESH),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_crit_ref, EVENT_OP_IS_CRIT_REF),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_spec_ref, EVENT_OP_IS_SPEC_REF),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_load_mode, EVENT_OP_IS_LOAD_MODE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access, EVENT_OP_IS_ZQCS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpr_req_with_nocredit,
+					EVENT_HPR_REQ_WITH_NOCREDIT),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpr_req_with_nocredit,
+					EVENT_LPR_REQ_WITH_NOCREDIT),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation, EVENT_BSM_STARVATION),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
+					EVENT_VISIBLE_WIN_LIMIT_REACHED_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_wr,
+					EVENT_VISIBLE_WIN_LIMIT_REACHED_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mpc, EVENT_OP_IS_DQSOSC_MPC),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mrr, EVENT_OP_IS_DQSOSC_MRR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_tcr_mrr, EVENT_OP_IS_TCR_MRR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqstart, EVENT_OP_IS_ZQSTART),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqlatch, EVENT_OP_IS_ZQLATCH),
+	/* Free run event counters */
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_reads, EVENT_DDR_READS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_writes, EVENT_DDR_WRITES),
+	NULL,
+};
+
+static struct attribute_group cn10k_ddr_perf_events_attr_group = {
+	.name = "events",
+	.attrs = cn10k_ddr_perf_events_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-8");
+
+static struct attribute *cn10k_ddr_perf_format_attrs[] = {
+	&format_attr_event.attr,
+	NULL,
+};
+
+static struct attribute_group cn10k_ddr_perf_format_attr_group = {
+	.name = "format",
+	.attrs = cn10k_ddr_perf_format_attrs,
+};
+
+static ssize_t cn10k_ddr_perf_cpumask_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct cn10k_ddr_pmu *pmu = dev_get_drvdata(dev);
+
+	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu));
+}
+
+static struct device_attribute cn10k_ddr_perf_cpumask_attr =
+	__ATTR(cpumask, 0444, cn10k_ddr_perf_cpumask_show, NULL);
+
+static struct attribute *cn10k_ddr_perf_cpumask_attrs[] = {
+	&cn10k_ddr_perf_cpumask_attr.attr,
+	NULL,
+};
+
+static struct attribute_group cn10k_ddr_perf_cpumask_attr_group = {
+	.attrs = cn10k_ddr_perf_cpumask_attrs,
+};
+
+static const struct attribute_group *cn10k_attr_groups[] = {
+	&cn10k_ddr_perf_events_attr_group,
+	&cn10k_ddr_perf_format_attr_group,
+	&cn10k_ddr_perf_cpumask_attr_group,
+	NULL,
+};
+
+static uint64_t ddr_perf_get_event_bitmap(int eventid)
+{
+	uint64_t event_bitmap = 0;
+
+	switch (eventid) {
+	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
+	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
+		event_bitmap = (1ULL << (eventid - 1));
+		break;
+
+	case EVENT_OP_IS_ENTER_SELFREF:
+	case EVENT_OP_IS_ENTER_POWERDOWN:
+	case EVENT_OP_IS_ENTER_MPSM:
+		event_bitmap = (0xFULL << (eventid - 1));
+		break;
+	default:
+		pr_err("%s Invalid eventid %d\n", __func__, eventid);
+		break;
+	}
+
+	return event_bitmap;
+}
+
+static int cn10k_ddr_perf_alloc_counter(struct cn10k_ddr_pmu *pmu,
+					struct perf_event *event)
+{
+	uint8_t config = event->attr.config;
+	int i;
+
+	/* DDR read free-run counter index */
+	if (config == EVENT_DDR_READS) {
+		pmu->events[DDRC_PERF_READ_COUNTER_IDX] = event;
+		return DDRC_PERF_READ_COUNTER_IDX;
+	}
+
+	/* DDR write free-run counter index */
+	if (config == EVENT_DDR_WRITES) {
+		pmu->events[DDRC_PERF_WRITE_COUNTER_IDX] = event;
+		return DDRC_PERF_WRITE_COUNTER_IDX;
+	}
+
+	/* Allocate DDR generic counters */
+	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
+		if (pmu->events[i] == NULL) {
+			pmu->events[i] = event;
+			return i;
+		}
+	}
+
+	return -ENOENT;
+}
+
+static void cn10k_ddr_perf_free_counter(struct cn10k_ddr_pmu *pmu, int counter)
+{
+	pmu->events[counter] = NULL;
+}
+
+static int cn10k_ddr_perf_event_init(struct perf_event *event)
+{
+	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (is_sampling_event(event)) {
+		dev_info(pmu->dev, "Sampling not supported!\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (event->cpu < 0) {
+		dev_warn(pmu->dev, "Can't provide per-task data!\n");
+		return -EOPNOTSUPP;
+	}
+
+	/*  We must NOT create groups containing mixed PMUs */
+	if (event->group_leader->pmu != event->pmu &&
+			!is_software_event(event->group_leader))
+		return -EINVAL;
+
+	/* Set ownership of event to one CPU, same event can not be observed
+	 * on multiple cpus at same time.
+	 */
+	event->cpu = pmu->cpu;
+	hwc->idx = -1;
+	return 0;
+}
+
+static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
+					  int counter, bool enable)
+{
+	uint32_t reg;
+	uint64_t val;
+
+	if (counter > DDRC_PERF_NUM_COUNTERS) {
+		pr_err("Error: unsupported counter %d\n", counter);
+		return;
+	}
+
+	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
+		reg = DDRC_PERF_CFG(counter);
+		val = readq_relaxed(pmu->base + reg);
+
+		if (enable)
+			val |= EVENT_ENABLE;
+		else
+			val &= ~EVENT_ENABLE;
+
+		writeq_relaxed(val, pmu->base + reg);
+	} else {
+		val = readq_relaxed(pmu->base + DDRC_PERF_CNT_FREERUN_EN);
+		if (enable) {
+			if (counter == DDRC_PERF_READ_COUNTER_IDX)
+				val |= DDRC_PERF_FREERUN_READ_EN;
+			else
+				val |= DDRC_PERF_FREERUN_WRITE_EN;
+		} else {
+			if (counter == DDRC_PERF_READ_COUNTER_IDX)
+				val &= ~DDRC_PERF_FREERUN_READ_EN;
+			else
+				val &= ~DDRC_PERF_FREERUN_WRITE_EN;
+		}
+		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_EN);
+	}
+}
+
+static uint64_t cn10k_ddr_perf_read_counter(struct cn10k_ddr_pmu *pmu,
+					    int counter)
+{
+	uint64_t val;
+
+	if (counter == DDRC_PERF_READ_COUNTER_IDX)
+		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_RD_OP);
+
+	if (counter == DDRC_PERF_WRITE_COUNTER_IDX)
+		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_WR_OP);
+
+	val = readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE(counter));
+	return val;
+}
+
+static void cn10k_ddr_perf_event_update(struct perf_event *event)
+{
+	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	uint64_t prev_count, new_count, mask;
+
+	do {
+		prev_count = local64_read(&hwc->prev_count);
+		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
+	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
+
+	mask = DDRC_PERF_CNT_MAX_VALUE;
+
+	local64_add((new_count - prev_count) & mask, &event->count);
+}
+
+static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
+{
+	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter = hwc->idx;
+
+	local64_set(&hwc->prev_count, 0);
+
+	cn10k_ddr_perf_counter_enable(pmu, counter, true);
+
+	hwc->state = 0;
+}
+
+static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
+{
+	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	uint8_t config = event->attr.config;
+	uint32_t reg_offset;
+	uint64_t val;
+	int counter;
+
+	counter = cn10k_ddr_perf_alloc_counter(pmu, event);
+	if (counter < 0) {
+		dev_dbg(pmu->dev, "There are not enough counters\n");
+		return -EOPNOTSUPP;
+	}
+
+	pmu->active_events++;
+	hwc->idx = counter;
+
+	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
+		/* Generic counters, configure event id */
+		reg_offset = DDRC_PERF_CFG(counter);
+		val = ddr_perf_get_event_bitmap(config);
+		writeq_relaxed(val, pmu->base + reg_offset);
+	} else {
+		/* fixed event counter, clear counter value */
+		if (counter == DDRC_PERF_READ_COUNTER_IDX)
+			val = DDRC_FREERUN_READ_CNT_CLR;
+		else
+			val = DDRC_FREERUN_WRITE_CNT_CLR;
+
+		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_CTRL);
+	}
+
+	hwc->state |= PERF_HES_STOPPED;
+
+	if (flags & PERF_EF_START)
+		cn10k_ddr_perf_event_start(event, flags);
+
+	return 0;
+}
+
+static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
+{
+	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter = hwc->idx;
+
+	cn10k_ddr_perf_counter_enable(pmu, counter, false);
+
+	if (flags & PERF_EF_UPDATE)
+		cn10k_ddr_perf_event_update(event);
+
+	hwc->state |= PERF_HES_STOPPED;
+}
+
+static void cn10k_ddr_perf_event_del(struct perf_event *event, int flags)
+{
+	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int counter = hwc->idx;
+
+	cn10k_ddr_perf_event_stop(event, PERF_EF_UPDATE);
+
+	cn10k_ddr_perf_free_counter(pmu, counter);
+	pmu->active_events--;
+	hwc->idx = -1;
+}
+
+static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu)
+{
+	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
+
+	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
+		       DDRC_PERF_CNT_START_OP_CTRL);
+}
+
+static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu)
+{
+	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
+
+	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
+		       DDRC_PERF_CNT_END_OP_CTRL);
+}
+
+static int cn10k_ddr_perf_probe(struct platform_device *pdev)
+{
+	struct cn10k_ddr_pmu *ddr_pmu;
+	struct resource *res;
+	void __iomem *base;
+	static int index;
+	char *name;
+	int ret;
+
+	ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(*ddr_pmu), GFP_KERNEL);
+	if (!ddr_pmu)
+		return -ENOMEM;
+
+	ddr_pmu->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ddr_pmu);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	ddr_pmu->base = base;
+
+	/* Setup the PMU counter to work in mannual mode */
+	writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base +
+		       DDRC_PERF_CNT_OP_MODE_CTRL);
+
+	ddr_pmu->pmu = (struct pmu) {
+		.module	      = THIS_MODULE,
+		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
+		.task_ctx_nr = perf_invalid_context,
+		.attr_groups = cn10k_attr_groups,
+		.event_init  = cn10k_ddr_perf_event_init,
+		.add	     = cn10k_ddr_perf_event_add,
+		.del	     = cn10k_ddr_perf_event_del,
+		.start	     = cn10k_ddr_perf_event_start,
+		.stop	     = cn10k_ddr_perf_event_stop,
+		.read	     = cn10k_ddr_perf_event_update,
+		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
+		.pmu_disable = cn10k_ddr_perf_pmu_disable,
+	};
+
+	/* Choose this cpu to collect perf data */
+	ddr_pmu->cpu = raw_smp_processor_id();
+
+	name = devm_kasprintf(ddr_pmu->dev, GFP_KERNEL, "mrvl_ddr_pmu@%llx",
+			      res->start);
+	if (!name)
+		return -ENOMEM;
+
+	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
+	if (ret)
+		return ret;
+
+	ddr_pmu->id = index++;
+	pr_info("CN10K DDR PMU Driver for ddrc@%llx - id-%d\n",
+		res->start, ddr_pmu->id);
+	return 0;
+}
+
+static int cn10k_ddr_perf_remove(struct platform_device *pdev)
+{
+	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&ddr_pmu->pmu);
+	return 0;
+}
+
+static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
+	{ .compatible = "marvell,cn10k-ddr-pmu", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
+
+static struct platform_driver cn10k_ddr_pmu_driver = {
+	.driver	= {
+		.name   = "cn10k-ddr-pmu",
+		.of_match_table = cn10k_ddr_pmu_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe		= cn10k_ddr_perf_probe,
+	.remove		= cn10k_ddr_perf_remove,
+};
+
+static int __init cn10k_ddr_pmu_init(void)
+{
+	return platform_driver_register(&cn10k_ddr_pmu_driver);
+}
+
+static void __exit cn10k_ddr_pmu_exit(void)
+{
+	platform_driver_unregister(&cn10k_ddr_pmu_driver);
+}
+
+module_init(cn10k_ddr_pmu_init);
+module_exit(cn10k_ddr_pmu_exit);
+
+MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v2 3/4] perf/marvell: cn10k DDR perfmon event overflow handling

[Bharat Bhushan]

CN10k DSS h/w perfmon does not support event overflow
interrupt, so periodic timer is being used. Each
event counter is 48bit, which in worst case scenario
can increment at maximum 5.6 GT/s. At this rate it may
take many hours to overflow these counters. Therefore
polling period for overflow is set to 100 sec, which
can be changed using sysfs parameter.

Two fixed event counters starts counting from zero on
overflow, so overflow condition is when new count less
than previous count. While eight programmable event
counters freezes at maximum value. Also individual
counter cannot be restarted, so need to restart all
eight counters.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v1->v2:
 - No change

 drivers/perf/marvell_cn10k_ddr_pmu.c | 111 +++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
index 8f9e3d1fcd8d..80f1441961d0 100644
--- a/drivers/perf/marvell_cn10k_ddr_pmu.c
+++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
@@ -11,6 +11,7 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/perf_event.h>
+#include <linux/hrtimer.h>
 
 /* Performance Counters Operating Mode Control Registers */
 #define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
@@ -128,6 +129,7 @@ struct cn10k_ddr_pmu {
 	struct	device *dev;
 	int active_events;
 	struct perf_event *events[DDRC_PERF_NUM_COUNTERS];
+	struct hrtimer hrtimer;
 };
 
 #define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
@@ -251,6 +253,18 @@ static const struct attribute_group *cn10k_attr_groups[] = {
 	NULL,
 };
 
+/* Default poll timeout is 100 sec, which is very sufficient for
+ * 48 bit counter incremented max at 5.6 GT/s, which may take many
+ * hours to overflow.
+ */
+static unsigned long cn10k_ddr_pmu_poll_period_sec = 100;
+module_param_named(poll_period_sec, cn10k_ddr_pmu_poll_period_sec, ulong, 0644);
+
+static ktime_t cn10k_ddr_pmu_timer_period(void)
+{
+	return ms_to_ktime((u64)cn10k_ddr_pmu_poll_period_sec * USEC_PER_SEC);
+}
+
 static uint64_t ddr_perf_get_event_bitmap(int eventid)
 {
 	uint64_t event_bitmap = 0;
@@ -439,6 +453,10 @@ static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
 	pmu->active_events++;
 	hwc->idx = counter;
 
+	if (pmu->active_events == 1)
+		hrtimer_start(&pmu->hrtimer, cn10k_ddr_pmu_timer_period(),
+			      HRTIMER_MODE_REL_PINNED);
+
 	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
 		/* Generic counters, configure event id */
 		reg_offset = DDRC_PERF_CFG(counter);
@@ -487,6 +505,10 @@ static void cn10k_ddr_perf_event_del(struct perf_event *event, int flags)
 	cn10k_ddr_perf_free_counter(pmu, counter);
 	pmu->active_events--;
 	hwc->idx = -1;
+
+	/* Cancel timer when no events to capture */
+	if (pmu->active_events == 0)
+		hrtimer_cancel(&pmu->hrtimer);
 }
 
 static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu)
@@ -505,6 +527,92 @@ static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu)
 		       DDRC_PERF_CNT_END_OP_CTRL);
 }
 
+static void cn10k_ddr_perf_event_update_all(struct cn10k_ddr_pmu *pmu)
+{
+	struct hw_perf_event *hwc;
+	int i;
+
+	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
+		if (pmu->events[i] == NULL)
+			continue;
+
+		cn10k_ddr_perf_event_update(pmu->events[i]);
+	}
+
+	/* Reset previous count as h/w counter are reset */
+	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
+		if (pmu->events[i] == NULL)
+			continue;
+
+		hwc = &pmu->events[i]->hw;
+		local64_set(&hwc->prev_count, 0);
+	}
+}
+
+static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu)
+{
+	struct perf_event *event;
+	struct hw_perf_event *hwc;
+	uint64_t prev_count, new_count;
+	uint64_t value;
+	int i;
+
+	event = pmu->events[DDRC_PERF_READ_COUNTER_IDX];
+	if (event) {
+		hwc = &event->hw;
+		prev_count = local64_read(&hwc->prev_count);
+		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
+
+		/* Overflow condition is when new count less than
+		 * previous count
+		 */
+		if (new_count < prev_count)
+			cn10k_ddr_perf_event_update(event);
+	}
+
+	event = pmu->events[DDRC_PERF_WRITE_COUNTER_IDX];
+	if (event) {
+		hwc = &event->hw;
+		prev_count = local64_read(&hwc->prev_count);
+		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
+
+		/* Overflow condition is when new count less than
+		 * previous count
+		 */
+		if (new_count < prev_count)
+			cn10k_ddr_perf_event_update(event);
+	}
+
+	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
+		if (pmu->events[i] == NULL)
+			continue;
+
+		value = cn10k_ddr_perf_read_counter(pmu, i);
+		if (value == DDRC_PERF_CNT_MAX_VALUE) {
+			pr_info("Counter-(%d) reached max value\n", i);
+			cn10k_ddr_perf_event_update_all(pmu);
+			cn10k_ddr_perf_pmu_disable(&pmu->pmu);
+			cn10k_ddr_perf_pmu_enable(&pmu->pmu);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static enum hrtimer_restart cn10k_ddr_pmu_timer_handler(struct hrtimer *hrtimer)
+{
+	struct cn10k_ddr_pmu *pmu = container_of(hrtimer, struct cn10k_ddr_pmu,
+						 hrtimer);
+	unsigned long flags;
+
+	local_irq_save(flags);
+	cn10k_ddr_pmu_overflow_handler(pmu);
+	local_irq_restore(flags);
+
+	hrtimer_forward_now(hrtimer, cn10k_ddr_pmu_timer_period());
+	return HRTIMER_RESTART;
+}
+
 static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 {
 	struct cn10k_ddr_pmu *ddr_pmu;
@@ -555,6 +663,9 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 	if (!name)
 		return -ENOMEM;
 
+	hrtimer_init(&ddr_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	ddr_pmu->hrtimer.function = cn10k_ddr_pmu_timer_handler;
+
 	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
 	if (ret)
 		return ret;
-- 
2.17.1

[PATCH v2 4/4] perf/marvell: cn10k DDR perf event core ownership

[Bharat Bhushan]

As DDR perf event counters are not per core, so they
should be accessed only by one core at a time.
Select new core when previously owning core is going
offline.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v1->v2:
 - No change

 drivers/perf/marvell_cn10k_ddr_pmu.c | 50 ++++++++++++++++++++++++++--
 include/linux/cpuhotplug.h           |  1 +
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
index 80f1441961d0..3bf810fe0cf5 100644
--- a/drivers/perf/marvell_cn10k_ddr_pmu.c
+++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
@@ -130,6 +130,7 @@ struct cn10k_ddr_pmu {
 	int active_events;
 	struct perf_event *events[DDRC_PERF_NUM_COUNTERS];
 	struct hrtimer hrtimer;
+	struct hlist_node node;
 };
 
 #define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
@@ -613,6 +614,24 @@ static enum hrtimer_restart cn10k_ddr_pmu_timer_handler(struct hrtimer *hrtimer)
 	return HRTIMER_RESTART;
 }
 
+static int cn10k_ddr_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+	struct cn10k_ddr_pmu *pmu = hlist_entry_safe(node, struct cn10k_ddr_pmu,
+						     node);
+	unsigned 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;
+	return 0;
+}
+
 static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 {
 	struct cn10k_ddr_pmu *ddr_pmu;
@@ -666,20 +685,33 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 	hrtimer_init(&ddr_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	ddr_pmu->hrtimer.function = cn10k_ddr_pmu_timer_handler;
 
+	cpuhp_state_add_instance_nocalls(
+				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
+				&ddr_pmu->node);
+
 	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
 	if (ret)
-		return ret;
+		goto error;
 
 	ddr_pmu->id = index++;
 	pr_info("CN10K DDR PMU Driver for ddrc@%llx - id-%d\n",
 		res->start, ddr_pmu->id);
 	return 0;
+error:
+	cpuhp_state_remove_instance_nocalls(
+				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
+				&ddr_pmu->node);
+	return ret;
 }
 
 static int cn10k_ddr_perf_remove(struct platform_device *pdev)
 {
 	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
 
+	cpuhp_state_remove_instance_nocalls(
+				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
+				&ddr_pmu->node);
+
 	perf_pmu_unregister(&ddr_pmu->pmu);
 	return 0;
 }
@@ -702,12 +734,26 @@ static struct platform_driver cn10k_ddr_pmu_driver = {
 
 static int __init cn10k_ddr_pmu_init(void)
 {
-	return platform_driver_register(&cn10k_ddr_pmu_driver);
+	int ret;
+
+	ret = cpuhp_setup_state_multi(
+				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
+				"perf/marvell/cn10k/ddr:online", NULL,
+				cn10k_ddr_pmu_offline_cpu);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&cn10k_ddr_pmu_driver);
+	if (ret)
+		cpuhp_remove_multi_state(
+				CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE);
+	return ret;
 }
 
 static void __exit cn10k_ddr_pmu_exit(void)
 {
 	platform_driver_unregister(&cn10k_ddr_pmu_driver);
+	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE);
 }
 
 module_init(cn10k_ddr_pmu_init);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 47e13582d9fc..ce7f2740d7db 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -185,6 +185,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,
 	CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE,
 	CPUHP_AP_PERF_ARM_CAVIUM_TX2_UNCORE_ONLINE,
+	CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
 	CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE,
 	CPUHP_AP_PERF_POWERPC_CORE_IMC_ONLINE,
 	CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE,
-- 
2.17.1

Re: [PATCH v2 1/4] dt-bindings: perf: marvell: cn10k ddr performance monitor

[Rob Herring]

On Tue, Aug 10, 2021 at 03:13:04PM +0530, Bharat Bhushan wrote:
> Add binding documentation for the Marvell CN10k DDR
> performance monitor unit.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v1->v2:
>  - DT binding changed to new DT Schema
> 
>  .../bindings/perf/marvell-cn10k-ddr.yaml      | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml b/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
> new file mode 100644
> index 000000000000..2a335444cf53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/perf/marvell-cn10k-ddr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell CN10K DDR performance monitor
> +
> +maintainers:
> +  - Bharat Bhushan <bbhushan2@marvell.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - marvell,cn10k-ddr-pmu
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        ddrcpmu@1 {

pmu@87e1c0000000

> +            compatible = "marvell,cn10k-ddr-pmu";
> +            reg = <0x87e1 0xc0000000 0x0 0x10000>;
> +        };
> +    };
> -- 
> 2.17.1
> 
> 

Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor support

[kajoljain]

On 8/10/21 3:13 PM, Bharat Bhushan wrote:
> Marvell CN10k DRAM Subsystem (DSS) supports eight
> event counters for monitoring performance and software
> can program each counter to monitor any of the defined
> performance event. Performance events are for interface
> between the DDR controller and the PHY, interface between
> the DDR Controller and the CHI interconnect, or within
> the DDR Controller. Additionally DSS also supports two
> fixed performance event counters, one for number of
> ddr reads and other for ddr writes.
> 
> This patch add basic support for these performance
> monitoring events on CN10k.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v1->v2:
>  - writeq/readq changed to respective relaxed version
>  - Using PMU_EVENT_ATTR_ID
> 
>  drivers/perf/Kconfig                 |   7 +
>  drivers/perf/Makefile                |   1 +
>  drivers/perf/marvell_cn10k_ddr_pmu.c | 606 +++++++++++++++++++++++++++
>  3 files changed, 614 insertions(+)
>  create mode 100644 drivers/perf/marvell_cn10k_ddr_pmu.c
> 
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 77522e5efe11..41a80a4b8d29 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -139,4 +139,11 @@ config ARM_DMC620_PMU
>  
>  source "drivers/perf/hisilicon/Kconfig"
>  
> +config MARVELL_CN10K_DDR_PMU
> +	tristate "Enable MARVELL CN10K DRAM Subsystem(DSS) PMU Support"
> +	depends on ARM64
> +	help
> +	  Enable perf support for Marvell DDR Performance monitoring
> +	  event on CN10K platform.
> +
>  endmenu
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 5260b116c7da..ee1126219d8d 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
>  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>  obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
>  obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
> +obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> new file mode 100644
> index 000000000000..8f9e3d1fcd8d
> --- /dev/null
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> @@ -0,0 +1,606 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
> + *
> + * Copyright (C) 2021 Marvell.
> + */
> +
> +#include <linux/init.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/perf_event.h>
> +
> +/* Performance Counters Operating Mode Control Registers */
> +#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
> +#define OP_MODE_CTRL_VAL_MANNUAL	0x1
> +
> +/* Performance Counters Start Operation Control Registers */
> +#define DDRC_PERF_CNT_START_OP_CTRL	0x8028
> +#define START_OP_CTRL_VAL_START		0x1ULL
> +#define START_OP_CTRL_VAL_ACTIVE	0x2
> +
> +/* Performance Counters End Operation Control Registers */
> +#define DDRC_PERF_CNT_END_OP_CTRL	0x8030
> +#define END_OP_CTRL_VAL_END		0x1ULL
> +
> +/* Performance Counters End Status Registers */
> +#define DDRC_PERF_CNT_END_STATUS		0x8038
> +#define END_STATUS_VAL_END_TIMER_MODE_END	0x1
> +
> +/* Performance Counters Configuration Registers */
> +#define DDRC_PERF_CFG_BASE		0x8040
> +
> +/* 8 Generic event counter + 2 fixed event counters */
> +#define DDRC_PERF_NUM_GEN_COUNTERS	8
> +#define DDRC_PERF_NUM_FIX_COUNTERS	2
> +#define DDRC_PERF_READ_COUNTER_IDX	DDRC_PERF_NUM_GEN_COUNTERS
> +#define DDRC_PERF_WRITE_COUNTER_IDX	(DDRC_PERF_NUM_GEN_COUNTERS + 1)
> +#define DDRC_PERF_NUM_COUNTERS		(DDRC_PERF_NUM_GEN_COUNTERS + \
> +					 DDRC_PERF_NUM_FIX_COUNTERS)
> +
> +/* Generic event counter registers */
> +#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
> +#define EVENT_ENABLE			BIT_ULL(63)
> +
> +/* Two dedicated event counters for DDR reads and writes */
> +#define EVENT_DDR_READS			101
> +#define EVENT_DDR_WRITES		100
> +
> +/*
> + * programmable events IDs in programmable event counters.
> + * DO NOT change these event-id numbers, they are used to
> + * program event bitmap in h/w.
> + */
> +#define EVENT_OP_IS_ZQLATCH			55
> +#define EVENT_OP_IS_ZQSTART			54
> +#define EVENT_OP_IS_TCR_MRR			53
> +#define EVENT_OP_IS_DQSOSC_MRR			52
> +#define EVENT_OP_IS_DQSOSC_MPC			51
> +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_WR	50
> +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
> +#define EVENT_BSM_STARVATION			48
> +#define EVENT_BSM_ALLOC				47
> +#define EVENT_LPR_REQ_WITH_NOCREDIT		46
> +#define EVENT_HPR_REQ_WITH_NOCREDIT		45
> +#define EVENT_OP_IS_ZQCS			44
> +#define EVENT_OP_IS_ZQCL			43
> +#define EVENT_OP_IS_LOAD_MODE			42
> +#define EVENT_OP_IS_SPEC_REF			41
> +#define EVENT_OP_IS_CRIT_REF			40
> +#define EVENT_OP_IS_REFRESH			39
> +#define EVENT_OP_IS_ENTER_MPSM			35
> +#define EVENT_OP_IS_ENTER_POWERDOWN		31
> +#define EVENT_OP_IS_ENTER_SELFREF		27
> +#define EVENT_WAW_HAZARD			26
> +#define EVENT_RAW_HAZARD			25
> +#define EVENT_WAR_HAZARD			24
> +#define EVENT_WRITE_COMBINE			23
> +#define EVENT_RDWR_TRANSITIONS			22
> +#define EVENT_PRECHARGE_FOR_OTHER		21
> +#define EVENT_PRECHARGE_FOR_RDWR		20
> +#define EVENT_OP_IS_PRECHARGE			19
> +#define EVENT_OP_IS_MWR				18
> +#define EVENT_OP_IS_WR				17
> +#define EVENT_OP_IS_RD				16
> +#define EVENT_OP_IS_RD_ACTIVATE			15
> +#define EVENT_OP_IS_RD_OR_WR			14
> +#define EVENT_OP_IS_ACTIVATE			13
> +#define EVENT_WR_XACT_WHEN_CRITICAL		12
> +#define EVENT_LPR_XACT_WHEN_CRITICAL		11
> +#define EVENT_HPR_XACT_WHEN_CRITICAL		10
> +#define EVENT_DFI_RD_DATA_CYCLES		9
> +#define EVENT_DFI_WR_DATA_CYCLES		8
> +#define EVENT_ACT_BYPASS			7
> +#define EVENT_READ_BYPASS			6
> +#define EVENT_HIF_HI_PRI_RD			5
> +#define EVENT_HIF_RMW				4
> +#define EVENT_HIF_RD				3
> +#define EVENT_HIF_WR				2
> +#define EVENT_HIF_RD_OR_WR			1
> +
> +/* Event counter value registers */
> +#define DDRC_PERF_CNT_VALUE_BASE		0x8080
> +#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
> +
> +/* Fixed event counter enable/disable register */
> +#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
> +#define DDRC_PERF_FREERUN_WRITE_EN	0x1
> +#define DDRC_PERF_FREERUN_READ_EN	0x2
> +
> +/* Fixed event counter control register */
> +#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
> +#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
> +#define DDRC_FREERUN_READ_CNT_CLR	0x2
> +
> +/* Fixed event counter value register */
> +#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
> +#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
> +#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
> +#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
> +
> +struct cn10k_ddr_pmu {
> +	struct pmu pmu;
> +	int id;
> +	void __iomem *base;
> +	unsigned int cpu;
> +	struct	device *dev;
> +	int active_events;
> +	struct perf_event *events[DDRC_PERF_NUM_COUNTERS];
> +};
> +
> +#define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
> +
> +static ssize_t cn10k_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 CN10K_DDR_PMU_EVENT_ATTR(_name, _id)				     \
> +	PMU_EVENT_ATTR_ID(_name, cn10k_ddr_pmu_event_show, _id)
> +
> +static struct attribute *cn10k_ddr_perf_events_attrs[] = {
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_or_wr_access, EVENT_HIF_RD_OR_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_wr_access, EVENT_HIF_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_access, EVENT_HIF_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rmw_access, EVENT_HIF_RMW),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_pri_rdaccess, EVENT_HIF_HI_PRI_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_bypass_access, EVENT_READ_BYPASS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_act_bypass_access, EVENT_ACT_BYPASS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_wr_data_access, EVENT_DFI_WR_DATA_CYCLES),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_rd_data_access, EVENT_DFI_RD_DATA_CYCLES),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpri_sched_rd_crit_access,
> +					EVENT_HPR_XACT_WHEN_CRITICAL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpri_sched_rd_crit_access,
> +					EVENT_LPR_XACT_WHEN_CRITICAL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_trxn_crit_access,
> +					EVENT_WR_XACT_WHEN_CRITICAL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_active_access, EVENT_OP_IS_ACTIVATE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_or_wr_access, EVENT_OP_IS_RD_OR_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_active_access, EVENT_OP_IS_RD_ACTIVATE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_read, EVENT_OP_IS_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_write, EVENT_OP_IS_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_mwr, EVENT_OP_IS_MWR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge, EVENT_OP_IS_PRECHARGE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_rdwr, EVENT_PRECHARGE_FOR_RDWR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_other,
> +					EVENT_PRECHARGE_FOR_OTHER),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rdwr_transitions, EVENT_RDWR_TRANSITIONS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_write_combine, EVENT_WRITE_COMBINE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_war_hazard, EVENT_WAR_HAZARD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_raw_hazard, EVENT_RAW_HAZARD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_waw_hazard, EVENT_WAW_HAZARD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_selfref, EVENT_OP_IS_ENTER_SELFREF),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_powerdown, EVENT_OP_IS_ENTER_POWERDOWN),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_mpsm, EVENT_OP_IS_ENTER_MPSM),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_refresh, EVENT_OP_IS_REFRESH),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_crit_ref, EVENT_OP_IS_CRIT_REF),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_spec_ref, EVENT_OP_IS_SPEC_REF),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_load_mode, EVENT_OP_IS_LOAD_MODE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access, EVENT_OP_IS_ZQCS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpr_req_with_nocredit,
> +					EVENT_HPR_REQ_WITH_NOCREDIT),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpr_req_with_nocredit,
> +					EVENT_LPR_REQ_WITH_NOCREDIT),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation, EVENT_BSM_STARVATION),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
> +					EVENT_VISIBLE_WIN_LIMIT_REACHED_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_wr,
> +					EVENT_VISIBLE_WIN_LIMIT_REACHED_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mpc, EVENT_OP_IS_DQSOSC_MPC),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mrr, EVENT_OP_IS_DQSOSC_MRR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_tcr_mrr, EVENT_OP_IS_TCR_MRR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqstart, EVENT_OP_IS_ZQSTART),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqlatch, EVENT_OP_IS_ZQLATCH),
> +	/* Free run event counters */
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_reads, EVENT_DDR_READS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_writes, EVENT_DDR_WRITES),
> +	NULL,
> +};
> +
> +static struct attribute_group cn10k_ddr_perf_events_attr_group = {
> +	.name = "events",
> +	.attrs = cn10k_ddr_perf_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-8");
> +
> +static struct attribute *cn10k_ddr_perf_format_attrs[] = {
> +	&format_attr_event.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cn10k_ddr_perf_format_attr_group = {
> +	.name = "format",
> +	.attrs = cn10k_ddr_perf_format_attrs,
> +};
> +
> +static ssize_t cn10k_ddr_perf_cpumask_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct cn10k_ddr_pmu *pmu = dev_get_drvdata(dev);
> +
> +	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu));
> +}
> +
> +static struct device_attribute cn10k_ddr_perf_cpumask_attr =
> +	__ATTR(cpumask, 0444, cn10k_ddr_perf_cpumask_show, NULL);
> +
> +static struct attribute *cn10k_ddr_perf_cpumask_attrs[] = {
> +	&cn10k_ddr_perf_cpumask_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cn10k_ddr_perf_cpumask_attr_group = {
> +	.attrs = cn10k_ddr_perf_cpumask_attrs,
> +};
> +
> +static const struct attribute_group *cn10k_attr_groups[] = {
> +	&cn10k_ddr_perf_events_attr_group,
> +	&cn10k_ddr_perf_format_attr_group,
> +	&cn10k_ddr_perf_cpumask_attr_group,
> +	NULL,
> +};
> +
> +static uint64_t ddr_perf_get_event_bitmap(int eventid)
> +{
> +	uint64_t event_bitmap = 0;
> +
> +	switch (eventid) {
> +	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
> +	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
> +		event_bitmap = (1ULL << (eventid - 1));
> +		break;
> +
> +	case EVENT_OP_IS_ENTER_SELFREF:
> +	case EVENT_OP_IS_ENTER_POWERDOWN:
> +	case EVENT_OP_IS_ENTER_MPSM:
> +		event_bitmap = (0xFULL << (eventid - 1));
> +		break;
> +	default:
> +		pr_err("%s Invalid eventid %d\n", __func__, eventid);
> +		break;
> +	}
> +
> +	return event_bitmap;
> +}
> +
> +static int cn10k_ddr_perf_alloc_counter(struct cn10k_ddr_pmu *pmu,
> +					struct perf_event *event)
> +{
> +	uint8_t config = event->attr.config;
> +	int i;
> +
> +	/* DDR read free-run counter index */
> +	if (config == EVENT_DDR_READS) {
> +		pmu->events[DDRC_PERF_READ_COUNTER_IDX] = event;
> +		return DDRC_PERF_READ_COUNTER_IDX;
> +	}
> +
> +	/* DDR write free-run counter index */
> +	if (config == EVENT_DDR_WRITES) {
> +		pmu->events[DDRC_PERF_WRITE_COUNTER_IDX] = event;
> +		return DDRC_PERF_WRITE_COUNTER_IDX;
> +	}
> +
> +	/* Allocate DDR generic counters */
> +	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
> +		if (pmu->events[i] == NULL) {
> +			pmu->events[i] = event;
> +			return i;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +static void cn10k_ddr_perf_free_counter(struct cn10k_ddr_pmu *pmu, int counter)
> +{
> +	pmu->events[counter] = NULL;
> +}
> +
> +static int cn10k_ddr_perf_event_init(struct perf_event *event)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (is_sampling_event(event)) {
> +		dev_info(pmu->dev, "Sampling not supported!\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (event->cpu < 0) {
> +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/*  We must NOT create groups containing mixed PMUs */
> +	if (event->group_leader->pmu != event->pmu &&
> +			!is_software_event(event->group_leader))
> +		return -EINVAL;
> +
> +	/* Set ownership of event to one CPU, same event can not be observed
> +	 * on multiple cpus at same time.
> +	 */
> +	event->cpu = pmu->cpu;
> +	hwc->idx = -1;
> +	return 0;
> +}
> +
> +static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
> +					  int counter, bool enable)
> +{
> +	uint32_t reg;
> +	uint64_t val;
> +
> +	if (counter > DDRC_PERF_NUM_COUNTERS) {
> +		pr_err("Error: unsupported counter %d\n", counter);
> +		return;
> +	}
> +
> +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> +		reg = DDRC_PERF_CFG(counter);
> +		val = readq_relaxed(pmu->base + reg);
> +
> +		if (enable)
> +			val |= EVENT_ENABLE;
> +		else
> +			val &= ~EVENT_ENABLE;
> +
> +		writeq_relaxed(val, pmu->base + reg);
> +	} else {
> +		val = readq_relaxed(pmu->base + DDRC_PERF_CNT_FREERUN_EN);
> +		if (enable) {
> +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +				val |= DDRC_PERF_FREERUN_READ_EN;
> +			else
> +				val |= DDRC_PERF_FREERUN_WRITE_EN;
> +		} else {
> +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +				val &= ~DDRC_PERF_FREERUN_READ_EN;
> +			else
> +				val &= ~DDRC_PERF_FREERUN_WRITE_EN;
> +		}
> +		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_EN);
> +	}
> +}
> +
> +static uint64_t cn10k_ddr_perf_read_counter(struct cn10k_ddr_pmu *pmu,
> +					    int counter)
> +{
> +	uint64_t val;
> +
> +	if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_RD_OP);
> +
> +	if (counter == DDRC_PERF_WRITE_COUNTER_IDX)
> +		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_WR_OP);
> +
> +	val = readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE(counter));
> +	return val;
> +}
> +
> +static void cn10k_ddr_perf_event_update(struct perf_event *event)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	uint64_t prev_count, new_count, mask;
> +
> +	do {
> +		prev_count = local64_read(&hwc->prev_count);
> +		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
> +	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
> +
> +	mask = DDRC_PERF_CNT_MAX_VALUE;
> +
> +	local64_add((new_count - prev_count) & mask, &event->count);
> +}
> +
> +static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	local64_set(&hwc->prev_count, 0);
> +
> +	cn10k_ddr_perf_counter_enable(pmu, counter, true);
> +
> +	hwc->state = 0;
> +}
> +
> +static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	uint8_t config = event->attr.config;
> +	uint32_t reg_offset;
> +	uint64_t val;
> +	int counter;
> +
> +	counter = cn10k_ddr_perf_alloc_counter(pmu, event);
> +	if (counter < 0) {
> +		dev_dbg(pmu->dev, "There are not enough counters\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	pmu->active_events++;
> +	hwc->idx = counter;
> +
> +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> +		/* Generic counters, configure event id */
> +		reg_offset = DDRC_PERF_CFG(counter);
> +		val = ddr_perf_get_event_bitmap(config);
> +		writeq_relaxed(val, pmu->base + reg_offset);
> +	} else {
> +		/* fixed event counter, clear counter value */
> +		if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +			val = DDRC_FREERUN_READ_CNT_CLR;
> +		else
> +			val = DDRC_FREERUN_WRITE_CNT_CLR;
> +
> +		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_CTRL);
> +	}
> +
> +	hwc->state |= PERF_HES_STOPPED;
> +
> +	if (flags & PERF_EF_START)
> +		cn10k_ddr_perf_event_start(event, flags);
> +
> +	return 0;
> +}
> +
> +static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	cn10k_ddr_perf_counter_enable(pmu, counter, false);
> +
> +	if (flags & PERF_EF_UPDATE)
> +		cn10k_ddr_perf_event_update(event);
> +
> +	hwc->state |= PERF_HES_STOPPED;
> +}
> +
> +static void cn10k_ddr_perf_event_del(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	cn10k_ddr_perf_event_stop(event, PERF_EF_UPDATE);
> +
> +	cn10k_ddr_perf_free_counter(pmu, counter);
> +	pmu->active_events--;
> +	hwc->idx = -1;
> +}
> +
> +static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> +
> +	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
> +		       DDRC_PERF_CNT_START_OP_CTRL);
> +}
> +
> +static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> +
> +	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
> +		       DDRC_PERF_CNT_END_OP_CTRL);
> +}
> +
> +static int cn10k_ddr_perf_probe(struct platform_device *pdev)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu;
> +	struct resource *res;
> +	void __iomem *base;
> +	static int index;
> +	char *name;
> +	int ret;
> +
> +	ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(*ddr_pmu), GFP_KERNEL);
> +	if (!ddr_pmu)
> +		return -ENOMEM;
> +
> +	ddr_pmu->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, ddr_pmu);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	ddr_pmu->base = base;
> +
> +	/* Setup the PMU counter to work in mannual mode */
> +	writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base +
> +		       DDRC_PERF_CNT_OP_MODE_CTRL);
> +
> +	ddr_pmu->pmu = (struct pmu) {
> +		.module	      = THIS_MODULE,
> +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> +		.task_ctx_nr = perf_invalid_context,
> +		.attr_groups = cn10k_attr_groups,
> +		.event_init  = cn10k_ddr_perf_event_init,
> +		.add	     = cn10k_ddr_perf_event_add,
> +		.del	     = cn10k_ddr_perf_event_del,
> +		.start	     = cn10k_ddr_perf_event_start,
> +		.stop	     = cn10k_ddr_perf_event_stop,
> +		.read	     = cn10k_ddr_perf_event_update,
> +		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
> +		.pmu_disable = cn10k_ddr_perf_pmu_disable,
> +	};
> +
> +	/* Choose this cpu to collect perf data */
> +	ddr_pmu->cpu = raw_smp_processor_id();
> +
> +	name = devm_kasprintf(ddr_pmu->dev, GFP_KERNEL, "mrvl_ddr_pmu@%llx",
> +			      res->start);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
> +	if (ret)
> +		return ret;

Hi Bharat,
   Do we need to free memory allocated to ddr_pmu incase perf_pmu_register fails? I think right now
we might have memory leaks here.
Also you added cpu hotplug support in patch 4, but cpu variable is added in this patch. Will it be better
to move addition of cpu part also in patch 4?

Thanks,
Kajol Jain

> +
> +	ddr_pmu->id = index++;
> +	pr_info("CN10K DDR PMU Driver for ddrc@%llx - id-%d\n",
> +		res->start, ddr_pmu->id);
> +	return 0;
> +}
> +
> +static int cn10k_ddr_perf_remove(struct platform_device *pdev)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&ddr_pmu->pmu);
> +	return 0;
> +}
> +
> +static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
> +	{ .compatible = "marvell,cn10k-ddr-pmu", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
> +
> +static struct platform_driver cn10k_ddr_pmu_driver = {
> +	.driver	= {
> +		.name   = "cn10k-ddr-pmu",
> +		.of_match_table = cn10k_ddr_pmu_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= cn10k_ddr_perf_probe,
> +	.remove		= cn10k_ddr_perf_remove,
> +};
> +
> +static int __init cn10k_ddr_pmu_init(void)
> +{
> +	return platform_driver_register(&cn10k_ddr_pmu_driver);
> +}
> +
> +static void __exit cn10k_ddr_pmu_exit(void)
> +{
> +	platform_driver_unregister(&cn10k_ddr_pmu_driver);
> +}
> +
> +module_init(cn10k_ddr_pmu_init);
> +module_exit(cn10k_ddr_pmu_exit);
> +
> +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor support

[John Garry]

On 10/08/2021 10:43, Bharat Bhushan wrote:
> Marvell CN10k DRAM Subsystem (DSS) supports eight
> event counters for monitoring performance and software
> can program each counter to monitor any of the defined
> performance event. Performance events are for interface
> between the DDR controller and the PHY, interface between
> the DDR Controller and the CHI interconnect, or within
> the DDR Controller. Additionally DSS also supports two
> fixed performance event counters, one for number of
> ddr reads and other for ddr writes.
> 
> This patch add basic support for these performance
> monitoring events on CN10k.

please use full 75 char width

> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v1->v2:
>   - writeq/readq changed to respective relaxed version
>   - Using PMU_EVENT_ATTR_ID
> 
>   drivers/perf/Kconfig                 |   7 +
>   drivers/perf/Makefile                |   1 +
>   drivers/perf/marvell_cn10k_ddr_pmu.c | 606 +++++++++++++++++++++++++++
>   3 files changed, 614 insertions(+)
>   create mode 100644 drivers/perf/marvell_cn10k_ddr_pmu.c
> 
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 77522e5efe11..41a80a4b8d29 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -139,4 +139,11 @@ config ARM_DMC620_PMU
>   
>   source "drivers/perf/hisilicon/Kconfig"
>   
> +config MARVELL_CN10K_DDR_PMU
> +	tristate "Enable MARVELL CN10K DRAM Subsystem(DSS) PMU Support"
> +	depends on ARM64

Is there anything to stop using adding COMPILE_TEST as a dependency? 
This really helps build coverage testing for other archs

> +	help
> +	  Enable perf support for Marvell DDR Performance monitoring
> +	  event on CN10K platform.
> +
>   endmenu
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 5260b116c7da..ee1126219d8d 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
>   obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>   obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
>   obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
> +obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> new file mode 100644
> index 000000000000..8f9e3d1fcd8d
> --- /dev/null
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> @@ -0,0 +1,606 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
> + *
> + * Copyright (C) 2021 Marvell.
> + */
> +
> +#include <linux/init.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/perf_event.h>
> +
> +/* Performance Counters Operating Mode Control Registers */
> +#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
> +#define OP_MODE_CTRL_VAL_MANNUAL	0x1
> +
> +/* Performance Counters Start Operation Control Registers */
> +#define DDRC_PERF_CNT_START_OP_CTRL	0x8028
> +#define START_OP_CTRL_VAL_START		0x1ULL
> +#define START_OP_CTRL_VAL_ACTIVE	0x2
> +
> +/* Performance Counters End Operation Control Registers */
> +#define DDRC_PERF_CNT_END_OP_CTRL	0x8030
> +#define END_OP_CTRL_VAL_END		0x1ULL
> +
> +/* Performance Counters End Status Registers */
> +#define DDRC_PERF_CNT_END_STATUS		0x8038
> +#define END_STATUS_VAL_END_TIMER_MODE_END	0x1
> +
> +/* Performance Counters Configuration Registers */
> +#define DDRC_PERF_CFG_BASE		0x8040
> +
> +/* 8 Generic event counter + 2 fixed event counters */
> +#define DDRC_PERF_NUM_GEN_COUNTERS	8
> +#define DDRC_PERF_NUM_FIX_COUNTERS	2
> +#define DDRC_PERF_READ_COUNTER_IDX	DDRC_PERF_NUM_GEN_COUNTERS
> +#define DDRC_PERF_WRITE_COUNTER_IDX	(DDRC_PERF_NUM_GEN_COUNTERS + 1)
> +#define DDRC_PERF_NUM_COUNTERS		(DDRC_PERF_NUM_GEN_COUNTERS + \
> +					 DDRC_PERF_NUM_FIX_COUNTERS)
> +
> +/* Generic event counter registers */
> +#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
> +#define EVENT_ENABLE			BIT_ULL(63)
> +
> +/* Two dedicated event counters for DDR reads and writes */
> +#define EVENT_DDR_READS			101
> +#define EVENT_DDR_WRITES		100
> +
> +/*
> + * programmable events IDs in programmable event counters.
> + * DO NOT change these event-id numbers, they are used to
> + * program event bitmap in h/w.
> + */
> +#define EVENT_OP_IS_ZQLATCH			55
> +#define EVENT_OP_IS_ZQSTART			54
> +#define EVENT_OP_IS_TCR_MRR			53
> +#define EVENT_OP_IS_DQSOSC_MRR			52
> +#define EVENT_OP_IS_DQSOSC_MPC			51
> +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_WR	50
> +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
> +#define EVENT_BSM_STARVATION			48
> +#define EVENT_BSM_ALLOC				47
> +#define EVENT_LPR_REQ_WITH_NOCREDIT		46
> +#define EVENT_HPR_REQ_WITH_NOCREDIT		45
> +#define EVENT_OP_IS_ZQCS			44
> +#define EVENT_OP_IS_ZQCL			43
> +#define EVENT_OP_IS_LOAD_MODE			42
> +#define EVENT_OP_IS_SPEC_REF			41
> +#define EVENT_OP_IS_CRIT_REF			40
> +#define EVENT_OP_IS_REFRESH			39
> +#define EVENT_OP_IS_ENTER_MPSM			35
> +#define EVENT_OP_IS_ENTER_POWERDOWN		31
> +#define EVENT_OP_IS_ENTER_SELFREF		27
> +#define EVENT_WAW_HAZARD			26
> +#define EVENT_RAW_HAZARD			25
> +#define EVENT_WAR_HAZARD			24
> +#define EVENT_WRITE_COMBINE			23
> +#define EVENT_RDWR_TRANSITIONS			22
> +#define EVENT_PRECHARGE_FOR_OTHER		21
> +#define EVENT_PRECHARGE_FOR_RDWR		20
> +#define EVENT_OP_IS_PRECHARGE			19
> +#define EVENT_OP_IS_MWR				18
> +#define EVENT_OP_IS_WR				17
> +#define EVENT_OP_IS_RD				16
> +#define EVENT_OP_IS_RD_ACTIVATE			15
> +#define EVENT_OP_IS_RD_OR_WR			14
> +#define EVENT_OP_IS_ACTIVATE			13
> +#define EVENT_WR_XACT_WHEN_CRITICAL		12
> +#define EVENT_LPR_XACT_WHEN_CRITICAL		11
> +#define EVENT_HPR_XACT_WHEN_CRITICAL		10
> +#define EVENT_DFI_RD_DATA_CYCLES		9
> +#define EVENT_DFI_WR_DATA_CYCLES		8
> +#define EVENT_ACT_BYPASS			7
> +#define EVENT_READ_BYPASS			6
> +#define EVENT_HIF_HI_PRI_RD			5
> +#define EVENT_HIF_RMW				4
> +#define EVENT_HIF_RD				3
> +#define EVENT_HIF_WR				2
> +#define EVENT_HIF_RD_OR_WR			1
> +
> +/* Event counter value registers */
> +#define DDRC_PERF_CNT_VALUE_BASE		0x8080
> +#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
> +
> +/* Fixed event counter enable/disable register */
> +#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
> +#define DDRC_PERF_FREERUN_WRITE_EN	0x1
> +#define DDRC_PERF_FREERUN_READ_EN	0x2
> +
> +/* Fixed event counter control register */
> +#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
> +#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
> +#define DDRC_FREERUN_READ_CNT_CLR	0x2
> +
> +/* Fixed event counter value register */
> +#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
> +#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
> +#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
> +#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)

I assume all these macros are used...

> +
> +struct cn10k_ddr_pmu {
> +	struct pmu pmu;
> +	int id;
> +	void __iomem *base;
> +	unsigned int cpu;
> +	struct	device *dev;
> +	int active_events;
> +	struct perf_event *events[DDRC_PERF_NUM_COUNTERS];
> +};
> +
> +#define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
> +
> +static ssize_t cn10k_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);

isn't sysfs_emit() preferred now? Need to check.

> +}
> +
> +#define CN10K_DDR_PMU_EVENT_ATTR(_name, _id)				     \
> +	PMU_EVENT_ATTR_ID(_name, cn10k_ddr_pmu_event_show, _id)
> +
> +static struct attribute *cn10k_ddr_perf_events_attrs[] = {
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_or_wr_access, EVENT_HIF_RD_OR_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_wr_access, EVENT_HIF_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_access, EVENT_HIF_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rmw_access, EVENT_HIF_RMW),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_pri_rdaccess, EVENT_HIF_HI_PRI_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_bypass_access, EVENT_READ_BYPASS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_act_bypass_access, EVENT_ACT_BYPASS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_wr_data_access, EVENT_DFI_WR_DATA_CYCLES),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_rd_data_access, EVENT_DFI_RD_DATA_CYCLES),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpri_sched_rd_crit_access,
> +					EVENT_HPR_XACT_WHEN_CRITICAL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpri_sched_rd_crit_access,
> +					EVENT_LPR_XACT_WHEN_CRITICAL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_trxn_crit_access,
> +					EVENT_WR_XACT_WHEN_CRITICAL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_active_access, EVENT_OP_IS_ACTIVATE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_or_wr_access, EVENT_OP_IS_RD_OR_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_active_access, EVENT_OP_IS_RD_ACTIVATE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_read, EVENT_OP_IS_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_write, EVENT_OP_IS_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_mwr, EVENT_OP_IS_MWR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge, EVENT_OP_IS_PRECHARGE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_rdwr, EVENT_PRECHARGE_FOR_RDWR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_other,
> +					EVENT_PRECHARGE_FOR_OTHER),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rdwr_transitions, EVENT_RDWR_TRANSITIONS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_write_combine, EVENT_WRITE_COMBINE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_war_hazard, EVENT_WAR_HAZARD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_raw_hazard, EVENT_RAW_HAZARD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_waw_hazard, EVENT_WAW_HAZARD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_selfref, EVENT_OP_IS_ENTER_SELFREF),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_powerdown, EVENT_OP_IS_ENTER_POWERDOWN),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_mpsm, EVENT_OP_IS_ENTER_MPSM),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_refresh, EVENT_OP_IS_REFRESH),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_crit_ref, EVENT_OP_IS_CRIT_REF),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_spec_ref, EVENT_OP_IS_SPEC_REF),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_load_mode, EVENT_OP_IS_LOAD_MODE),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access, EVENT_OP_IS_ZQCS),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpr_req_with_nocredit,
> +					EVENT_HPR_REQ_WITH_NOCREDIT),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpr_req_with_nocredit,
> +					EVENT_LPR_REQ_WITH_NOCREDIT),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation, EVENT_BSM_STARVATION),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
> +					EVENT_VISIBLE_WIN_LIMIT_REACHED_RD),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_wr,
> +					EVENT_VISIBLE_WIN_LIMIT_REACHED_WR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mpc, EVENT_OP_IS_DQSOSC_MPC),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mrr, EVENT_OP_IS_DQSOSC_MRR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_tcr_mrr, EVENT_OP_IS_TCR_MRR),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqstart, EVENT_OP_IS_ZQSTART),
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqlatch, EVENT_OP_IS_ZQLATCH),
> +	/* Free run event counters */
> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_reads, EVENT_DDR_READS),

if you want perf tool to support aliases / metrics for these then a hw 
identifer sysfs file is required, like the freescale imx8 ddr pmu driver 
has, as I assume that this PMU is not tightly coupled always to a 
specific CPU

> +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_writes, EVENT_DDR_WRITES),
> +	NULL,

no need for ',' when the array is not going to be expanded

> +};
> +
> +static struct attribute_group cn10k_ddr_perf_events_attr_group = {
> +	.name = "events",
> +	.attrs = cn10k_ddr_perf_events_attrs,
> +};
> +
> +PMU_FORMAT_ATTR(event, "config:0-8");
> +
> +static struct attribute *cn10k_ddr_perf_format_attrs[] = {
> +	&format_attr_event.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cn10k_ddr_perf_format_attr_group = {
> +	.name = "format",
> +	.attrs = cn10k_ddr_perf_format_attrs,
> +};
> +
> +static ssize_t cn10k_ddr_perf_cpumask_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct cn10k_ddr_pmu *pmu = dev_get_drvdata(dev);
> +
> +	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu));
> +}
> +
> +static struct device_attribute cn10k_ddr_perf_cpumask_attr =
> +	__ATTR(cpumask, 0444, cn10k_ddr_perf_cpumask_show, NULL);
> +
> +static struct attribute *cn10k_ddr_perf_cpumask_attrs[] = {
> +	&cn10k_ddr_perf_cpumask_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group cn10k_ddr_perf_cpumask_attr_group = {
> +	.attrs = cn10k_ddr_perf_cpumask_attrs,
> +};
> +
> +static const struct attribute_group *cn10k_attr_groups[] = {
> +	&cn10k_ddr_perf_events_attr_group,
> +	&cn10k_ddr_perf_format_attr_group,
> +	&cn10k_ddr_perf_cpumask_attr_group,
> +	NULL,
> +};
> +
> +static uint64_t ddr_perf_get_event_bitmap(int eventid)

Don't we use u64 in kernel land as preference?

> +{
> +	uint64_t event_bitmap = 0;
> +
> +	switch (eventid) {
> +	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
> +	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
> +		event_bitmap = (1ULL << (eventid - 1));
> +		break;
> +
> +	case EVENT_OP_IS_ENTER_SELFREF:
> +	case EVENT_OP_IS_ENTER_POWERDOWN:
> +	case EVENT_OP_IS_ENTER_MPSM:
> +		event_bitmap = (0xFULL << (eventid - 1));
> +		break;
> +	default:
> +		pr_err("%s Invalid eventid %d\n", __func__, eventid);

shouldn't this generate a proper error to its only caller (which can 
actually error)?

> +		break;
> +	}
> +
> +	return event_bitmap;
> +}
> +
> +static int cn10k_ddr_perf_alloc_counter(struct cn10k_ddr_pmu *pmu,
> +					struct perf_event *event)
> +{
> +	uint8_t config = event->attr.config;
> +	int i;
> +
> +	/* DDR read free-run counter index */
> +	if (config == EVENT_DDR_READS) {
> +		pmu->events[DDRC_PERF_READ_COUNTER_IDX] = event;
> +		return DDRC_PERF_READ_COUNTER_IDX;
> +	}
> +
> +	/* DDR write free-run counter index */
> +	if (config == EVENT_DDR_WRITES) {
> +		pmu->events[DDRC_PERF_WRITE_COUNTER_IDX] = event;
> +		return DDRC_PERF_WRITE_COUNTER_IDX;
> +	}
> +
> +	/* Allocate DDR generic counters */
> +	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
> +		if (pmu->events[i] == NULL) {
> +			pmu->events[i] = event;
> +			return i;
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +static void cn10k_ddr_perf_free_counter(struct cn10k_ddr_pmu *pmu, int counter)
> +{
> +	pmu->events[counter] = NULL;
> +}
> +
> +static int cn10k_ddr_perf_event_init(struct perf_event *event)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (is_sampling_event(event)) {
> +		dev_info(pmu->dev, "Sampling not supported!\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (event->cpu < 0) {
> +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/*  We must NOT create groups containing mixed PMUs */
> +	if (event->group_leader->pmu != event->pmu &&
> +			!is_software_event(event->group_leader))

pay attention to indentation

> +		return -EINVAL;
> +
> +	/* Set ownership of event to one CPU, same event can not be observed
> +	 * on multiple cpus at same time.
> +	 */
> +	event->cpu = pmu->cpu;
> +	hwc->idx = -1;
> +	return 0;
> +}
> +
> +static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
> +					  int counter, bool enable)
> +{
> +	uint32_t reg;
> +	uint64_t val;
> +
> +	if (counter > DDRC_PERF_NUM_COUNTERS) {

is this paranoia?

> +		pr_err("Error: unsupported counter %d\n", counter);
> +		return;
> +	}
> +
> +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> +		reg = DDRC_PERF_CFG(counter);
> +		val = readq_relaxed(pmu->base + reg);
> +
> +		if (enable)
> +			val |= EVENT_ENABLE;
> +		else
> +			val &= ~EVENT_ENABLE;
> +
> +		writeq_relaxed(val, pmu->base + reg);
> +	} else {
> +		val = readq_relaxed(pmu->base + DDRC_PERF_CNT_FREERUN_EN);
> +		if (enable) {
> +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +				val |= DDRC_PERF_FREERUN_READ_EN;
> +			else
> +				val |= DDRC_PERF_FREERUN_WRITE_EN;
> +		} else {
> +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +				val &= ~DDRC_PERF_FREERUN_READ_EN;
> +			else
> +				val &= ~DDRC_PERF_FREERUN_WRITE_EN;
> +		}
> +		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_EN);
> +	}
> +}
> +
> +static uint64_t cn10k_ddr_perf_read_counter(struct cn10k_ddr_pmu *pmu,
> +					    int counter)
> +{
> +	uint64_t val;
> +
> +	if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_RD_OP);
> +
> +	if (counter == DDRC_PERF_WRITE_COUNTER_IDX)
> +		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_WR_OP);
> +
> +	val = readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE(counter));
> +	return val;
> +}
> +
> +static void cn10k_ddr_perf_event_update(struct perf_event *event)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	uint64_t prev_count, new_count, mask;
> +
> +	do {
> +		prev_count = local64_read(&hwc->prev_count);
> +		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
> +	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
> +
> +	mask = DDRC_PERF_CNT_MAX_VALUE;
> +
> +	local64_add((new_count - prev_count) & mask, &event->count);
> +}
> +
> +static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	local64_set(&hwc->prev_count, 0);
> +
> +	cn10k_ddr_perf_counter_enable(pmu, counter, true);
> +
> +	hwc->state = 0;
> +}
> +
> +static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	uint8_t config = event->attr.config;
> +	uint32_t reg_offset;
> +	uint64_t val;
> +	int counter;
> +
> +	counter = cn10k_ddr_perf_alloc_counter(pmu, event);
> +	if (counter < 0) {
> +		dev_dbg(pmu->dev, "There are not enough counters\n");

hmmm .. I'd be inclined to say that there are not enough available. And 
is dev_dbg() really any use?

> +		return -EOPNOTSUPP;

is that the best error code?

> +	}
> +
> +	pmu->active_events++;
> +	hwc->idx = counter;
> +
> +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> +		/* Generic counters, configure event id */
> +		reg_offset = DDRC_PERF_CFG(counter);
> +		val = ddr_perf_get_event_bitmap(config);
> +		writeq_relaxed(val, pmu->base + reg_offset);
> +	} else {
> +		/* fixed event counter, clear counter value */
> +		if (counter == DDRC_PERF_READ_COUNTER_IDX)
> +			val = DDRC_FREERUN_READ_CNT_CLR;
> +		else
> +			val = DDRC_FREERUN_WRITE_CNT_CLR;
> +
> +		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_CTRL);
> +	}
> +
> +	hwc->state |= PERF_HES_STOPPED;
> +
> +	if (flags & PERF_EF_START)
> +		cn10k_ddr_perf_event_start(event, flags);
> +
> +	return 0;
> +}
> +
> +static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	cn10k_ddr_perf_counter_enable(pmu, counter, false);
> +
> +	if (flags & PERF_EF_UPDATE)
> +		cn10k_ddr_perf_event_update(event);
> +
> +	hwc->state |= PERF_HES_STOPPED;
> +}
> +
> +static void cn10k_ddr_perf_event_del(struct perf_event *event, int flags)
> +{
> +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter = hwc->idx;
> +
> +	cn10k_ddr_perf_event_stop(event, PERF_EF_UPDATE);
> +
> +	cn10k_ddr_perf_free_counter(pmu, counter);
> +	pmu->active_events--;
> +	hwc->idx = -1;
> +}
> +
> +static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> +
> +	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
> +		       DDRC_PERF_CNT_START_OP_CTRL);
> +}
> +
> +static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> +
> +	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
> +		       DDRC_PERF_CNT_END_OP_CTRL);
> +}
> +
> +static int cn10k_ddr_perf_probe(struct platform_device *pdev)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu;
> +	struct resource *res;
> +	void __iomem *base;
> +	static int index;
> +	char *name;
> +	int ret;
> +
> +	ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(*ddr_pmu), GFP_KERNEL);
> +	if (!ddr_pmu)
> +		return -ENOMEM;
> +
> +	ddr_pmu->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, ddr_pmu);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);

can you use devm_platform_get_and_ioremap_resource()? I think that there 
is some helper that does both these steps

> +
> +	ddr_pmu->base = base;
> +
> +	/* Setup the PMU counter to work in mannual mode */

manual

> +	writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base +
> +		       DDRC_PERF_CNT_OP_MODE_CTRL);
> +
> +	ddr_pmu->pmu = (struct pmu) {
> +		.module	      = THIS_MODULE,
> +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> +		.task_ctx_nr = perf_invalid_context,
> +		.attr_groups = cn10k_attr_groups,
> +		.event_init  = cn10k_ddr_perf_event_init,
> +		.add	     = cn10k_ddr_perf_event_add,
> +		.del	     = cn10k_ddr_perf_event_del,
> +		.start	     = cn10k_ddr_perf_event_start,
> +		.stop	     = cn10k_ddr_perf_event_stop,
> +		.read	     = cn10k_ddr_perf_event_update,
> +		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
> +		.pmu_disable = cn10k_ddr_perf_pmu_disable,
> +	};
> +
> +	/* Choose this cpu to collect perf data */
> +	ddr_pmu->cpu = raw_smp_processor_id();
> +
> +	name = devm_kasprintf(ddr_pmu->dev, GFP_KERNEL, "mrvl_ddr_pmu@%llx",

mostly _%llx would be used elsewhere, and I am not sure how perf tool 
likes @ at all

> +			      res->start);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
> +	if (ret)
> +		return ret;
> +
> +	ddr_pmu->id = index++;

where is this used?

> +	pr_info("CN10K DDR PMU Driver for ddrc@%llx - id-%d\n",
> +		res->start, ddr_pmu->id);
> +	return 0;
> +}
> +
> +static int cn10k_ddr_perf_remove(struct platform_device *pdev)
> +{
> +	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&ddr_pmu->pmu);
> +	return 0;
> +}
> +
> +static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
> +	{ .compatible = "marvell,cn10k-ddr-pmu", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
> +
> +static struct platform_driver cn10k_ddr_pmu_driver = {
> +	.driver	= {
> +		.name   = "cn10k-ddr-pmu",
> +		.of_match_table = cn10k_ddr_pmu_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= cn10k_ddr_perf_probe,
> +	.remove		= cn10k_ddr_perf_remove,
> +};
> +
> +static int __init cn10k_ddr_pmu_init(void)
> +{
> +	return platform_driver_register(&cn10k_ddr_pmu_driver);
> +}
> +
> +static void __exit cn10k_ddr_pmu_exit(void)
> +{
> +	platform_driver_unregister(&cn10k_ddr_pmu_driver);
> +}
> +
> +module_init(cn10k_ddr_pmu_init);
> +module_exit(cn10k_ddr_pmu_exit);
> +
> +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> +MODULE_LICENSE("GPL v2");
> 

RE: [EXT] Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor support

[Bharat Bhushan]

Please see inline

> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: Wednesday, August 18, 2021 7:19 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>; will@kernel.org;
> mark.rutland@arm.com; robh+dt@kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor
> support
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 10/08/2021 10:43, Bharat Bhushan wrote:
> > Marvell CN10k DRAM Subsystem (DSS) supports eight
> > event counters for monitoring performance and software
> > can program each counter to monitor any of the defined
> > performance event. Performance events are for interface
> > between the DDR controller and the PHY, interface between
> > the DDR Controller and the CHI interconnect, or within
> > the DDR Controller. Additionally DSS also supports two
> > fixed performance event counters, one for number of
> > ddr reads and other for ddr writes.
> >
> > This patch add basic support for these performance
> > monitoring events on CN10k.
> 
> please use full 75 char width

Ok,

> 
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> > v1->v2:
> >   - writeq/readq changed to respective relaxed version
> >   - Using PMU_EVENT_ATTR_ID
> >
> >   drivers/perf/Kconfig                 |   7 +
> >   drivers/perf/Makefile                |   1 +
> >   drivers/perf/marvell_cn10k_ddr_pmu.c | 606 +++++++++++++++++++++++++++
> >   3 files changed, 614 insertions(+)
> >   create mode 100644 drivers/perf/marvell_cn10k_ddr_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index 77522e5efe11..41a80a4b8d29 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -139,4 +139,11 @@ config ARM_DMC620_PMU
> >
> >   source "drivers/perf/hisilicon/Kconfig"
> >
> > +config MARVELL_CN10K_DDR_PMU
> > +	tristate "Enable MARVELL CN10K DRAM Subsystem(DSS) PMU Support"
> > +	depends on ARM64
> 
> Is there anything to stop using adding COMPILE_TEST as a dependency?
> This really helps build coverage testing for other archs

Just keeping same as other drivers

> 
> > +	help
> > +	  Enable perf support for Marvell DDR Performance monitoring
> > +	  event on CN10K platform.
> > +
> >   endmenu
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> > index 5260b116c7da..ee1126219d8d 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
> >   obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> >   obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
> >   obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
> > +obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
> > diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c
> b/drivers/perf/marvell_cn10k_ddr_pmu.c
> > new file mode 100644
> > index 000000000000..8f9e3d1fcd8d
> > --- /dev/null
> > +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> > @@ -0,0 +1,606 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
> > + *
> > + * Copyright (C) 2021 Marvell.
> > + */
> > +
> > +#include <linux/init.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/perf_event.h>
> > +
> > +/* Performance Counters Operating Mode Control Registers */
> > +#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
> > +#define OP_MODE_CTRL_VAL_MANNUAL	0x1
> > +
> > +/* Performance Counters Start Operation Control Registers */
> > +#define DDRC_PERF_CNT_START_OP_CTRL	0x8028
> > +#define START_OP_CTRL_VAL_START		0x1ULL
> > +#define START_OP_CTRL_VAL_ACTIVE	0x2
> > +
> > +/* Performance Counters End Operation Control Registers */
> > +#define DDRC_PERF_CNT_END_OP_CTRL	0x8030
> > +#define END_OP_CTRL_VAL_END		0x1ULL
> > +
> > +/* Performance Counters End Status Registers */
> > +#define DDRC_PERF_CNT_END_STATUS		0x8038
> > +#define END_STATUS_VAL_END_TIMER_MODE_END	0x1
> > +
> > +/* Performance Counters Configuration Registers */
> > +#define DDRC_PERF_CFG_BASE		0x8040
> > +
> > +/* 8 Generic event counter + 2 fixed event counters */
> > +#define DDRC_PERF_NUM_GEN_COUNTERS	8
> > +#define DDRC_PERF_NUM_FIX_COUNTERS	2
> > +#define DDRC_PERF_READ_COUNTER_IDX
> 	DDRC_PERF_NUM_GEN_COUNTERS
> > +#define DDRC_PERF_WRITE_COUNTER_IDX
> 	(DDRC_PERF_NUM_GEN_COUNTERS + 1)
> > +#define DDRC_PERF_NUM_COUNTERS
> 	(DDRC_PERF_NUM_GEN_COUNTERS + \
> > +					 DDRC_PERF_NUM_FIX_COUNTERS)
> > +
> > +/* Generic event counter registers */
> > +#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
> > +#define EVENT_ENABLE			BIT_ULL(63)
> > +
> > +/* Two dedicated event counters for DDR reads and writes */
> > +#define EVENT_DDR_READS			101
> > +#define EVENT_DDR_WRITES		100
> > +
> > +/*
> > + * programmable events IDs in programmable event counters.
> > + * DO NOT change these event-id numbers, they are used to
> > + * program event bitmap in h/w.
> > + */
> > +#define EVENT_OP_IS_ZQLATCH			55
> > +#define EVENT_OP_IS_ZQSTART			54
> > +#define EVENT_OP_IS_TCR_MRR			53
> > +#define EVENT_OP_IS_DQSOSC_MRR			52
> > +#define EVENT_OP_IS_DQSOSC_MPC			51
> > +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_WR	50
> > +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
> > +#define EVENT_BSM_STARVATION			48
> > +#define EVENT_BSM_ALLOC				47
> > +#define EVENT_LPR_REQ_WITH_NOCREDIT		46
> > +#define EVENT_HPR_REQ_WITH_NOCREDIT		45
> > +#define EVENT_OP_IS_ZQCS			44
> > +#define EVENT_OP_IS_ZQCL			43
> > +#define EVENT_OP_IS_LOAD_MODE			42
> > +#define EVENT_OP_IS_SPEC_REF			41
> > +#define EVENT_OP_IS_CRIT_REF			40
> > +#define EVENT_OP_IS_REFRESH			39
> > +#define EVENT_OP_IS_ENTER_MPSM			35
> > +#define EVENT_OP_IS_ENTER_POWERDOWN		31
> > +#define EVENT_OP_IS_ENTER_SELFREF		27
> > +#define EVENT_WAW_HAZARD			26
> > +#define EVENT_RAW_HAZARD			25
> > +#define EVENT_WAR_HAZARD			24
> > +#define EVENT_WRITE_COMBINE			23
> > +#define EVENT_RDWR_TRANSITIONS			22
> > +#define EVENT_PRECHARGE_FOR_OTHER		21
> > +#define EVENT_PRECHARGE_FOR_RDWR		20
> > +#define EVENT_OP_IS_PRECHARGE			19
> > +#define EVENT_OP_IS_MWR				18
> > +#define EVENT_OP_IS_WR				17
> > +#define EVENT_OP_IS_RD				16
> > +#define EVENT_OP_IS_RD_ACTIVATE			15
> > +#define EVENT_OP_IS_RD_OR_WR			14
> > +#define EVENT_OP_IS_ACTIVATE			13
> > +#define EVENT_WR_XACT_WHEN_CRITICAL		12
> > +#define EVENT_LPR_XACT_WHEN_CRITICAL		11
> > +#define EVENT_HPR_XACT_WHEN_CRITICAL		10
> > +#define EVENT_DFI_RD_DATA_CYCLES		9
> > +#define EVENT_DFI_WR_DATA_CYCLES		8
> > +#define EVENT_ACT_BYPASS			7
> > +#define EVENT_READ_BYPASS			6
> > +#define EVENT_HIF_HI_PRI_RD			5
> > +#define EVENT_HIF_RMW				4
> > +#define EVENT_HIF_RD				3
> > +#define EVENT_HIF_WR				2
> > +#define EVENT_HIF_RD_OR_WR			1
> > +
> > +/* Event counter value registers */
> > +#define DDRC_PERF_CNT_VALUE_BASE		0x8080
> > +#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
> > +
> > +/* Fixed event counter enable/disable register */
> > +#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
> > +#define DDRC_PERF_FREERUN_WRITE_EN	0x1
> > +#define DDRC_PERF_FREERUN_READ_EN	0x2
> > +
> > +/* Fixed event counter control register */
> > +#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
> > +#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
> > +#define DDRC_FREERUN_READ_CNT_CLR	0x2
> > +
> > +/* Fixed event counter value register */
> > +#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
> > +#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
> > +#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
> > +#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
> 
> I assume all these macros are used...

Yes, do you see any unused?

> 
> > +
> > +struct cn10k_ddr_pmu {
> > +	struct pmu pmu;
> > +	int id;
> > +	void __iomem *base;
> > +	unsigned int cpu;
> > +	struct	device *dev;
> > +	int active_events;
> > +	struct perf_event *events[DDRC_PERF_NUM_COUNTERS];
> > +};
> > +
> > +#define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu,
> pmu)
> > +
> > +static ssize_t cn10k_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);
> 
> isn't sysfs_emit() preferred now? Need to check.

Yes, Thanks for pointing.

> 
> > +}
> > +
> > +#define CN10K_DDR_PMU_EVENT_ATTR(_name, _id)
> 	     \
> > +	PMU_EVENT_ATTR_ID(_name, cn10k_ddr_pmu_event_show, _id)
> > +
> > +static struct attribute *cn10k_ddr_perf_events_attrs[] = {
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_or_wr_access,
> EVENT_HIF_RD_OR_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_wr_access, EVENT_HIF_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_access, EVENT_HIF_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rmw_access,
> EVENT_HIF_RMW),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_pri_rdaccess,
> EVENT_HIF_HI_PRI_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_bypass_access,
> EVENT_READ_BYPASS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_act_bypass_access,
> EVENT_ACT_BYPASS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_wr_data_access,
> EVENT_DFI_WR_DATA_CYCLES),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_rd_data_access,
> EVENT_DFI_RD_DATA_CYCLES),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpri_sched_rd_crit_access,
> > +					EVENT_HPR_XACT_WHEN_CRITICAL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpri_sched_rd_crit_access,
> > +					EVENT_LPR_XACT_WHEN_CRITICAL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_trxn_crit_access,
> > +					EVENT_WR_XACT_WHEN_CRITICAL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_active_access,
> EVENT_OP_IS_ACTIVATE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_or_wr_access,
> EVENT_OP_IS_RD_OR_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_active_access,
> EVENT_OP_IS_RD_ACTIVATE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_read, EVENT_OP_IS_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_write, EVENT_OP_IS_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_mwr, EVENT_OP_IS_MWR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge,
> EVENT_OP_IS_PRECHARGE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_rdwr,
> EVENT_PRECHARGE_FOR_RDWR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_other,
> > +					EVENT_PRECHARGE_FOR_OTHER),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rdwr_transitions,
> EVENT_RDWR_TRANSITIONS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_write_combine,
> EVENT_WRITE_COMBINE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_war_hazard,
> EVENT_WAR_HAZARD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_raw_hazard,
> EVENT_RAW_HAZARD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_waw_hazard,
> EVENT_WAW_HAZARD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_selfref,
> EVENT_OP_IS_ENTER_SELFREF),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_powerdown,
> EVENT_OP_IS_ENTER_POWERDOWN),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_mpsm,
> EVENT_OP_IS_ENTER_MPSM),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_refresh, EVENT_OP_IS_REFRESH),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_crit_ref, EVENT_OP_IS_CRIT_REF),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_spec_ref, EVENT_OP_IS_SPEC_REF),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_load_mode,
> EVENT_OP_IS_LOAD_MODE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access,
> EVENT_OP_IS_ZQCS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpr_req_with_nocredit,
> > +					EVENT_HPR_REQ_WITH_NOCREDIT),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpr_req_with_nocredit,
> > +					EVENT_LPR_REQ_WITH_NOCREDIT),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation,
> EVENT_BSM_STARVATION),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
> > +
> 	EVENT_VISIBLE_WIN_LIMIT_REACHED_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_wr,
> > +
> 	EVENT_VISIBLE_WIN_LIMIT_REACHED_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mpc,
> EVENT_OP_IS_DQSOSC_MPC),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mrr,
> EVENT_OP_IS_DQSOSC_MRR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_tcr_mrr, EVENT_OP_IS_TCR_MRR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqstart, EVENT_OP_IS_ZQSTART),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqlatch, EVENT_OP_IS_ZQLATCH),
> > +	/* Free run event counters */
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_reads, EVENT_DDR_READS),
> 
> if you want perf tool to support aliases / metrics for these then a hw
> identifer sysfs file is required, like the freescale imx8 ddr pmu driver
> has, as I assume that this PMU is not tightly coupled always to a
> specific CPU

As of now we do not have ddr pmu versioning.

> 
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_writes, EVENT_DDR_WRITES),
> > +	NULL,
> 
> no need for ',' when the array is not going to be expanded

Oh yes,

> 
> > +};
> > +
> > +static struct attribute_group cn10k_ddr_perf_events_attr_group = {
> > +	.name = "events",
> > +	.attrs = cn10k_ddr_perf_events_attrs,
> > +};
> > +
> > +PMU_FORMAT_ATTR(event, "config:0-8");
> > +
> > +static struct attribute *cn10k_ddr_perf_format_attrs[] = {
> > +	&format_attr_event.attr,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group cn10k_ddr_perf_format_attr_group = {
> > +	.name = "format",
> > +	.attrs = cn10k_ddr_perf_format_attrs,
> > +};
> > +
> > +static ssize_t cn10k_ddr_perf_cpumask_show(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   char *buf)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = dev_get_drvdata(dev);
> > +
> > +	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu));
> > +}
> > +
> > +static struct device_attribute cn10k_ddr_perf_cpumask_attr =
> > +	__ATTR(cpumask, 0444, cn10k_ddr_perf_cpumask_show, NULL);
> > +
> > +static struct attribute *cn10k_ddr_perf_cpumask_attrs[] = {
> > +	&cn10k_ddr_perf_cpumask_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group cn10k_ddr_perf_cpumask_attr_group = {
> > +	.attrs = cn10k_ddr_perf_cpumask_attrs,
> > +};
> > +
> > +static const struct attribute_group *cn10k_attr_groups[] = {
> > +	&cn10k_ddr_perf_events_attr_group,
> > +	&cn10k_ddr_perf_format_attr_group,
> > +	&cn10k_ddr_perf_cpumask_attr_group,
> > +	NULL,
> > +};
> > +
> > +static uint64_t ddr_perf_get_event_bitmap(int eventid)
> 
> Don't we use u64 in kernel land as preference?

I am not aware of any guidance or rule, both are being used.
I end up using uint64_t and u64 both, will use one only with next version. 

> 
> > +{
> > +	uint64_t event_bitmap = 0;
> > +
> > +	switch (eventid) {
> > +	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
> > +	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
> > +		event_bitmap = (1ULL << (eventid - 1));
> > +		break;
> > +
> > +	case EVENT_OP_IS_ENTER_SELFREF:
> > +	case EVENT_OP_IS_ENTER_POWERDOWN:
> > +	case EVENT_OP_IS_ENTER_MPSM:
> > +		event_bitmap = (0xFULL << (eventid - 1));
> > +		break;
> > +	default:
> > +		pr_err("%s Invalid eventid %d\n", __func__, eventid);
> 
> shouldn't this generate a proper error to its only caller (which can
> actually error)?

Ack, currently it just through error but does not error out.

> 
> > +		break;
> > +	}
> > +
> > +	return event_bitmap;
> > +}
> > +
> > +static int cn10k_ddr_perf_alloc_counter(struct cn10k_ddr_pmu *pmu,
> > +					struct perf_event *event)
> > +{
> > +	uint8_t config = event->attr.config;
> > +	int i;
> > +
> > +	/* DDR read free-run counter index */
> > +	if (config == EVENT_DDR_READS) {
> > +		pmu->events[DDRC_PERF_READ_COUNTER_IDX] = event;
> > +		return DDRC_PERF_READ_COUNTER_IDX;
> > +	}
> > +
> > +	/* DDR write free-run counter index */
> > +	if (config == EVENT_DDR_WRITES) {
> > +		pmu->events[DDRC_PERF_WRITE_COUNTER_IDX] = event;
> > +		return DDRC_PERF_WRITE_COUNTER_IDX;
> > +	}
> > +
> > +	/* Allocate DDR generic counters */
> > +	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
> > +		if (pmu->events[i] == NULL) {
> > +			pmu->events[i] = event;
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return -ENOENT;
> > +}
> > +
> > +static void cn10k_ddr_perf_free_counter(struct cn10k_ddr_pmu *pmu, int
> counter)
> > +{
> > +	pmu->events[counter] = NULL;
> > +}
> > +
> > +static int cn10k_ddr_perf_event_init(struct perf_event *event)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (is_sampling_event(event)) {
> > +		dev_info(pmu->dev, "Sampling not supported!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (event->cpu < 0) {
> > +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	/*  We must NOT create groups containing mixed PMUs */
> > +	if (event->group_leader->pmu != event->pmu &&
> > +			!is_software_event(event->group_leader))
> 
> pay attention to indentation

Accepted, not sure how this got in.

> 
> > +		return -EINVAL;
> > +
> > +	/* Set ownership of event to one CPU, same event can not be observed
> > +	 * on multiple cpus at same time.
> > +	 */
> > +	event->cpu = pmu->cpu;
> > +	hwc->idx = -1;
> > +	return 0;
> > +}
> > +
> > +static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
> > +					  int counter, bool enable)
> > +{
> > +	uint32_t reg;
> > +	uint64_t val;
> > +
> > +	if (counter > DDRC_PERF_NUM_COUNTERS) {
> 
> is this paranoia?

We can not program if counter by any means is out or range.

> 
> > +		pr_err("Error: unsupported counter %d\n", counter);
> > +		return;
> > +	}
> > +
> > +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> > +		reg = DDRC_PERF_CFG(counter);
> > +		val = readq_relaxed(pmu->base + reg);
> > +
> > +		if (enable)
> > +			val |= EVENT_ENABLE;
> > +		else
> > +			val &= ~EVENT_ENABLE;
> > +
> > +		writeq_relaxed(val, pmu->base + reg);
> > +	} else {
> > +		val = readq_relaxed(pmu->base +
> DDRC_PERF_CNT_FREERUN_EN);
> > +		if (enable) {
> > +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +				val |= DDRC_PERF_FREERUN_READ_EN;
> > +			else
> > +				val |= DDRC_PERF_FREERUN_WRITE_EN;
> > +		} else {
> > +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +				val &= ~DDRC_PERF_FREERUN_READ_EN;
> > +			else
> > +				val &= ~DDRC_PERF_FREERUN_WRITE_EN;
> > +		}
> > +		writeq_relaxed(val, pmu->base +
> DDRC_PERF_CNT_FREERUN_EN);
> > +	}
> > +}
> > +
> > +static uint64_t cn10k_ddr_perf_read_counter(struct cn10k_ddr_pmu *pmu,
> > +					    int counter)
> > +{
> > +	uint64_t val;
> > +
> > +	if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +		return readq_relaxed(pmu->base +
> DDRC_PERF_CNT_VALUE_RD_OP);
> > +
> > +	if (counter == DDRC_PERF_WRITE_COUNTER_IDX)
> > +		return readq_relaxed(pmu->base +
> DDRC_PERF_CNT_VALUE_WR_OP);
> > +
> > +	val = readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE(counter));
> > +	return val;
> > +}
> > +
> > +static void cn10k_ddr_perf_event_update(struct perf_event *event)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	uint64_t prev_count, new_count, mask;
> > +
> > +	do {
> > +		prev_count = local64_read(&hwc->prev_count);
> > +		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
> > +	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
> > +
> > +	mask = DDRC_PERF_CNT_MAX_VALUE;
> > +
> > +	local64_add((new_count - prev_count) & mask, &event->count);
> > +}
> > +
> > +static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int counter = hwc->idx;
> > +
> > +	local64_set(&hwc->prev_count, 0);
> > +
> > +	cn10k_ddr_perf_counter_enable(pmu, counter, true);
> > +
> > +	hwc->state = 0;
> > +}
> > +
> > +static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	uint8_t config = event->attr.config;
> > +	uint32_t reg_offset;
> > +	uint64_t val;
> > +	int counter;
> > +
> > +	counter = cn10k_ddr_perf_alloc_counter(pmu, event);
> > +	if (counter < 0) {
> > +		dev_dbg(pmu->dev, "There are not enough counters\n");
> 
> hmmm .. I'd be inclined to say that there are not enough available. And
> is dev_dbg() really any use?

Maybe we can remove the error print now, and return 

> 
> > +		return -EOPNOTSUPP;
> 
> is that the best error code?

Looks like we should let it try again later  "-EAGAIN"

> 
> > +	}
> > +
> > +	pmu->active_events++;
> > +	hwc->idx = counter;
> > +
> > +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> > +		/* Generic counters, configure event id */
> > +		reg_offset = DDRC_PERF_CFG(counter);
> > +		val = ddr_perf_get_event_bitmap(config);
> > +		writeq_relaxed(val, pmu->base + reg_offset);
> > +	} else {
> > +		/* fixed event counter, clear counter value */
> > +		if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +			val = DDRC_FREERUN_READ_CNT_CLR;
> > +		else
> > +			val = DDRC_FREERUN_WRITE_CNT_CLR;
> > +
> > +		writeq_relaxed(val, pmu->base +
> DDRC_PERF_CNT_FREERUN_CTRL);
> > +	}
> > +
> > +	hwc->state |= PERF_HES_STOPPED;
> > +
> > +	if (flags & PERF_EF_START)
> > +		cn10k_ddr_perf_event_start(event, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int counter = hwc->idx;
> > +
> > +	cn10k_ddr_perf_counter_enable(pmu, counter, false);
> > +
> > +	if (flags & PERF_EF_UPDATE)
> > +		cn10k_ddr_perf_event_update(event);
> > +
> > +	hwc->state |= PERF_HES_STOPPED;
> > +}
> > +
> > +static void cn10k_ddr_perf_event_del(struct perf_event *event, int flags)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int counter = hwc->idx;
> > +
> > +	cn10k_ddr_perf_event_stop(event, PERF_EF_UPDATE);
> > +
> > +	cn10k_ddr_perf_free_counter(pmu, counter);
> > +	pmu->active_events--;
> > +	hwc->idx = -1;
> > +}
> > +
> > +static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu)
> > +{
> > +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> > +
> > +	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
> > +		       DDRC_PERF_CNT_START_OP_CTRL);
> > +}
> > +
> > +static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu)
> > +{
> > +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> > +
> > +	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
> > +		       DDRC_PERF_CNT_END_OP_CTRL);
> > +}
> > +
> > +static int cn10k_ddr_perf_probe(struct platform_device *pdev)
> > +{
> > +	struct cn10k_ddr_pmu *ddr_pmu;
> > +	struct resource *res;
> > +	void __iomem *base;
> > +	static int index;
> > +	char *name;
> > +	int ret;
> > +
> > +	ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(*ddr_pmu), GFP_KERNEL);
> > +	if (!ddr_pmu)
> > +		return -ENOMEM;
> > +
> > +	ddr_pmu->dev = &pdev->dev;
> > +	platform_set_drvdata(pdev, ddr_pmu);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> 
> can you use devm_platform_get_and_ioremap_resource()? I think that there
> is some helper that does both these steps

Yes, devm_platform_get_and_ioremap_resource() does both.
 

> 
> > +
> > +	ddr_pmu->base = base;
> > +
> > +	/* Setup the PMU counter to work in mannual mode */
> 
> manual
> 
> > +	writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base +
> > +		       DDRC_PERF_CNT_OP_MODE_CTRL);
> > +
> > +	ddr_pmu->pmu = (struct pmu) {
> > +		.module	      = THIS_MODULE,
> > +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> > +		.task_ctx_nr = perf_invalid_context,
> > +		.attr_groups = cn10k_attr_groups,
> > +		.event_init  = cn10k_ddr_perf_event_init,
> > +		.add	     = cn10k_ddr_perf_event_add,
> > +		.del	     = cn10k_ddr_perf_event_del,
> > +		.start	     = cn10k_ddr_perf_event_start,
> > +		.stop	     = cn10k_ddr_perf_event_stop,
> > +		.read	     = cn10k_ddr_perf_event_update,
> > +		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
> > +		.pmu_disable = cn10k_ddr_perf_pmu_disable,
> > +	};
> > +
> > +	/* Choose this cpu to collect perf data */
> > +	ddr_pmu->cpu = raw_smp_processor_id();
> > +
> > +	name = devm_kasprintf(ddr_pmu->dev, GFP_KERNEL,
> "mrvl_ddr_pmu@%llx",
> 
> mostly _%llx would be used elsewhere, and I am not sure how perf tool
> likes @ at all

Will be consistent with other,

> 
> > +			      res->start);
> > +	if (!name)
> > +		return -ENOMEM;
> > +
> > +	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ddr_pmu->id = index++;
> 
> where is this used?

Just below to keep count of number of instance.

Thanks for review, will do required changes in next version.

Thanks
-Bharat
> 
> > +	pr_info("CN10K DDR PMU Driver for ddrc@%llx - id-%d\n",
> > +		res->start, ddr_pmu->id);
> > +	return 0;
> > +}
> > +
> > +static int cn10k_ddr_perf_remove(struct platform_device *pdev)
> > +{
> > +	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
> > +
> > +	perf_pmu_unregister(&ddr_pmu->pmu);
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
> > +	{ .compatible = "marvell,cn10k-ddr-pmu", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
> > +
> > +static struct platform_driver cn10k_ddr_pmu_driver = {
> > +	.driver	= {
> > +		.name   = "cn10k-ddr-pmu",
> > +		.of_match_table = cn10k_ddr_pmu_of_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe		= cn10k_ddr_perf_probe,
> > +	.remove		= cn10k_ddr_perf_remove,
> > +};
> > +
> > +static int __init cn10k_ddr_pmu_init(void)
> > +{
> > +	return platform_driver_register(&cn10k_ddr_pmu_driver);
> > +}
> > +
> > +static void __exit cn10k_ddr_pmu_exit(void)
> > +{
> > +	platform_driver_unregister(&cn10k_ddr_pmu_driver);
> > +}
> > +
> > +module_init(cn10k_ddr_pmu_init);
> > +module_exit(cn10k_ddr_pmu_exit);
> > +
> > +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> > +MODULE_LICENSE("GPL v2");
> >

RE: [EXT] Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor support

[Bharat Bhushan]

Please see inline 

> -----Original Message-----
> From: kajoljain <kjain@linux.ibm.com>
> Sent: Wednesday, August 18, 2021 5:58 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>; will@kernel.org;
> mark.rutland@arm.com; robh+dt@kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor
> support
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> 
> On 8/10/21 3:13 PM, Bharat Bhushan wrote:
> > Marvell CN10k DRAM Subsystem (DSS) supports eight event counters for
> > monitoring performance and software can program each counter to
> > monitor any of the defined performance event. Performance events are
> > for interface between the DDR controller and the PHY, interface
> > between the DDR Controller and the CHI interconnect, or within the DDR
> > Controller. Additionally DSS also supports two fixed performance event
> > counters, one for number of ddr reads and other for ddr writes.
> >
> > This patch add basic support for these performance monitoring events
> > on CN10k.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> > v1->v2:
> >  - writeq/readq changed to respective relaxed version
> >  - Using PMU_EVENT_ATTR_ID
> >
> >  drivers/perf/Kconfig                 |   7 +
> >  drivers/perf/Makefile                |   1 +
> >  drivers/perf/marvell_cn10k_ddr_pmu.c | 606
> > +++++++++++++++++++++++++++
> >  3 files changed, 614 insertions(+)
> >  create mode 100644 drivers/perf/marvell_cn10k_ddr_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig index
> > 77522e5efe11..41a80a4b8d29 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -139,4 +139,11 @@ config ARM_DMC620_PMU
> >
> >  source "drivers/perf/hisilicon/Kconfig"
> >
> > +config MARVELL_CN10K_DDR_PMU
> > +	tristate "Enable MARVELL CN10K DRAM Subsystem(DSS) PMU Support"
> > +	depends on ARM64
> > +	help
> > +	  Enable perf support for Marvell DDR Performance monitoring
> > +	  event on CN10K platform.
> > +
> >  endmenu
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile index
> > 5260b116c7da..ee1126219d8d 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
> >  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> >  obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
> >  obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
> > +obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
> > diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c
> > b/drivers/perf/marvell_cn10k_ddr_pmu.c
> > new file mode 100644
> > index 000000000000..8f9e3d1fcd8d
> > --- /dev/null
> > +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> > @@ -0,0 +1,606 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
> > + *
> > + * Copyright (C) 2021 Marvell.
> > + */
> > +
> > +#include <linux/init.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/perf_event.h>
> > +
> > +/* Performance Counters Operating Mode Control Registers */
> > +#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
> > +#define OP_MODE_CTRL_VAL_MANNUAL	0x1
> > +
> > +/* Performance Counters Start Operation Control Registers */
> > +#define DDRC_PERF_CNT_START_OP_CTRL	0x8028
> > +#define START_OP_CTRL_VAL_START		0x1ULL
> > +#define START_OP_CTRL_VAL_ACTIVE	0x2
> > +
> > +/* Performance Counters End Operation Control Registers */
> > +#define DDRC_PERF_CNT_END_OP_CTRL	0x8030
> > +#define END_OP_CTRL_VAL_END		0x1ULL
> > +
> > +/* Performance Counters End Status Registers */
> > +#define DDRC_PERF_CNT_END_STATUS		0x8038
> > +#define END_STATUS_VAL_END_TIMER_MODE_END	0x1
> > +
> > +/* Performance Counters Configuration Registers */
> > +#define DDRC_PERF_CFG_BASE		0x8040
> > +
> > +/* 8 Generic event counter + 2 fixed event counters */
> > +#define DDRC_PERF_NUM_GEN_COUNTERS	8
> > +#define DDRC_PERF_NUM_FIX_COUNTERS	2
> > +#define DDRC_PERF_READ_COUNTER_IDX
> 	DDRC_PERF_NUM_GEN_COUNTERS
> > +#define DDRC_PERF_WRITE_COUNTER_IDX
> 	(DDRC_PERF_NUM_GEN_COUNTERS + 1)
> > +#define DDRC_PERF_NUM_COUNTERS
> 	(DDRC_PERF_NUM_GEN_COUNTERS + \
> > +					 DDRC_PERF_NUM_FIX_COUNTERS)
> > +
> > +/* Generic event counter registers */
> > +#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
> > +#define EVENT_ENABLE			BIT_ULL(63)
> > +
> > +/* Two dedicated event counters for DDR reads and writes */
> > +#define EVENT_DDR_READS			101
> > +#define EVENT_DDR_WRITES		100
> > +
> > +/*
> > + * programmable events IDs in programmable event counters.
> > + * DO NOT change these event-id numbers, they are used to
> > + * program event bitmap in h/w.
> > + */
> > +#define EVENT_OP_IS_ZQLATCH			55
> > +#define EVENT_OP_IS_ZQSTART			54
> > +#define EVENT_OP_IS_TCR_MRR			53
> > +#define EVENT_OP_IS_DQSOSC_MRR			52
> > +#define EVENT_OP_IS_DQSOSC_MPC			51
> > +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_WR	50
> > +#define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
> > +#define EVENT_BSM_STARVATION			48
> > +#define EVENT_BSM_ALLOC				47
> > +#define EVENT_LPR_REQ_WITH_NOCREDIT		46
> > +#define EVENT_HPR_REQ_WITH_NOCREDIT		45
> > +#define EVENT_OP_IS_ZQCS			44
> > +#define EVENT_OP_IS_ZQCL			43
> > +#define EVENT_OP_IS_LOAD_MODE			42
> > +#define EVENT_OP_IS_SPEC_REF			41
> > +#define EVENT_OP_IS_CRIT_REF			40
> > +#define EVENT_OP_IS_REFRESH			39
> > +#define EVENT_OP_IS_ENTER_MPSM			35
> > +#define EVENT_OP_IS_ENTER_POWERDOWN		31
> > +#define EVENT_OP_IS_ENTER_SELFREF		27
> > +#define EVENT_WAW_HAZARD			26
> > +#define EVENT_RAW_HAZARD			25
> > +#define EVENT_WAR_HAZARD			24
> > +#define EVENT_WRITE_COMBINE			23
> > +#define EVENT_RDWR_TRANSITIONS			22
> > +#define EVENT_PRECHARGE_FOR_OTHER		21
> > +#define EVENT_PRECHARGE_FOR_RDWR		20
> > +#define EVENT_OP_IS_PRECHARGE			19
> > +#define EVENT_OP_IS_MWR				18
> > +#define EVENT_OP_IS_WR				17
> > +#define EVENT_OP_IS_RD				16
> > +#define EVENT_OP_IS_RD_ACTIVATE			15
> > +#define EVENT_OP_IS_RD_OR_WR			14
> > +#define EVENT_OP_IS_ACTIVATE			13
> > +#define EVENT_WR_XACT_WHEN_CRITICAL		12
> > +#define EVENT_LPR_XACT_WHEN_CRITICAL		11
> > +#define EVENT_HPR_XACT_WHEN_CRITICAL		10
> > +#define EVENT_DFI_RD_DATA_CYCLES		9
> > +#define EVENT_DFI_WR_DATA_CYCLES		8
> > +#define EVENT_ACT_BYPASS			7
> > +#define EVENT_READ_BYPASS			6
> > +#define EVENT_HIF_HI_PRI_RD			5
> > +#define EVENT_HIF_RMW				4
> > +#define EVENT_HIF_RD				3
> > +#define EVENT_HIF_WR				2
> > +#define EVENT_HIF_RD_OR_WR			1
> > +
> > +/* Event counter value registers */
> > +#define DDRC_PERF_CNT_VALUE_BASE		0x8080
> > +#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
> > +
> > +/* Fixed event counter enable/disable register */
> > +#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
> > +#define DDRC_PERF_FREERUN_WRITE_EN	0x1
> > +#define DDRC_PERF_FREERUN_READ_EN	0x2
> > +
> > +/* Fixed event counter control register */
> > +#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
> > +#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
> > +#define DDRC_FREERUN_READ_CNT_CLR	0x2
> > +
> > +/* Fixed event counter value register */
> > +#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
> > +#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
> > +#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
> > +#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
> > +
> > +struct cn10k_ddr_pmu {
> > +	struct pmu pmu;
> > +	int id;
> > +	void __iomem *base;
> > +	unsigned int cpu;
> > +	struct	device *dev;
> > +	int active_events;
> > +	struct perf_event *events[DDRC_PERF_NUM_COUNTERS]; };
> > +
> > +#define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu,
> pmu)
> > +
> > +static ssize_t cn10k_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 CN10K_DDR_PMU_EVENT_ATTR(_name, _id)
> 	     \
> > +	PMU_EVENT_ATTR_ID(_name, cn10k_ddr_pmu_event_show, _id)
> > +
> > +static struct attribute *cn10k_ddr_perf_events_attrs[] = {
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_or_wr_access,
> EVENT_HIF_RD_OR_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_wr_access, EVENT_HIF_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_access, EVENT_HIF_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rmw_access,
> EVENT_HIF_RMW),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_pri_rdaccess,
> EVENT_HIF_HI_PRI_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_bypass_access,
> EVENT_READ_BYPASS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_act_bypass_access,
> EVENT_ACT_BYPASS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_wr_data_access,
> EVENT_DFI_WR_DATA_CYCLES),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dif_rd_data_access,
> EVENT_DFI_RD_DATA_CYCLES),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpri_sched_rd_crit_access,
> > +					EVENT_HPR_XACT_WHEN_CRITICAL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpri_sched_rd_crit_access,
> > +					EVENT_LPR_XACT_WHEN_CRITICAL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_trxn_crit_access,
> > +					EVENT_WR_XACT_WHEN_CRITICAL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_active_access,
> EVENT_OP_IS_ACTIVATE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_or_wr_access,
> EVENT_OP_IS_RD_OR_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_active_access,
> EVENT_OP_IS_RD_ACTIVATE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_read, EVENT_OP_IS_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_write, EVENT_OP_IS_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_mwr, EVENT_OP_IS_MWR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge,
> EVENT_OP_IS_PRECHARGE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_rdwr,
> EVENT_PRECHARGE_FOR_RDWR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_other,
> > +					EVENT_PRECHARGE_FOR_OTHER),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_rdwr_transitions,
> EVENT_RDWR_TRANSITIONS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_write_combine,
> EVENT_WRITE_COMBINE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_war_hazard,
> EVENT_WAR_HAZARD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_raw_hazard,
> EVENT_RAW_HAZARD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_waw_hazard,
> EVENT_WAW_HAZARD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_selfref,
> EVENT_OP_IS_ENTER_SELFREF),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_powerdown,
> EVENT_OP_IS_ENTER_POWERDOWN),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_mpsm,
> EVENT_OP_IS_ENTER_MPSM),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_refresh, EVENT_OP_IS_REFRESH),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_crit_ref, EVENT_OP_IS_CRIT_REF),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_spec_ref, EVENT_OP_IS_SPEC_REF),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_load_mode,
> EVENT_OP_IS_LOAD_MODE),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access,
> EVENT_OP_IS_ZQCS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpr_req_with_nocredit,
> > +					EVENT_HPR_REQ_WITH_NOCREDIT),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpr_req_with_nocredit,
> > +					EVENT_LPR_REQ_WITH_NOCREDIT),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation,
> EVENT_BSM_STARVATION),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
> > +
> 	EVENT_VISIBLE_WIN_LIMIT_REACHED_RD),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_wr,
> > +
> 	EVENT_VISIBLE_WIN_LIMIT_REACHED_WR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mpc,
> EVENT_OP_IS_DQSOSC_MPC),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mrr,
> EVENT_OP_IS_DQSOSC_MRR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_tcr_mrr, EVENT_OP_IS_TCR_MRR),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqstart, EVENT_OP_IS_ZQSTART),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqlatch, EVENT_OP_IS_ZQLATCH),
> > +	/* Free run event counters */
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_reads, EVENT_DDR_READS),
> > +	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_writes, EVENT_DDR_WRITES),
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group cn10k_ddr_perf_events_attr_group = {
> > +	.name = "events",
> > +	.attrs = cn10k_ddr_perf_events_attrs, };
> > +
> > +PMU_FORMAT_ATTR(event, "config:0-8");
> > +
> > +static struct attribute *cn10k_ddr_perf_format_attrs[] = {
> > +	&format_attr_event.attr,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group cn10k_ddr_perf_format_attr_group = {
> > +	.name = "format",
> > +	.attrs = cn10k_ddr_perf_format_attrs, };
> > +
> > +static ssize_t cn10k_ddr_perf_cpumask_show(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   char *buf)
> > +{
> > +	struct cn10k_ddr_pmu *pmu = dev_get_drvdata(dev);
> > +
> > +	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu)); }
> > +
> > +static struct device_attribute cn10k_ddr_perf_cpumask_attr =
> > +	__ATTR(cpumask, 0444, cn10k_ddr_perf_cpumask_show, NULL);
> > +
> > +static struct attribute *cn10k_ddr_perf_cpumask_attrs[] = {
> > +	&cn10k_ddr_perf_cpumask_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group cn10k_ddr_perf_cpumask_attr_group = {
> > +	.attrs = cn10k_ddr_perf_cpumask_attrs, };
> > +
> > +static const struct attribute_group *cn10k_attr_groups[] = {
> > +	&cn10k_ddr_perf_events_attr_group,
> > +	&cn10k_ddr_perf_format_attr_group,
> > +	&cn10k_ddr_perf_cpumask_attr_group,
> > +	NULL,
> > +};
> > +
> > +static uint64_t ddr_perf_get_event_bitmap(int eventid) {
> > +	uint64_t event_bitmap = 0;
> > +
> > +	switch (eventid) {
> > +	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
> > +	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
> > +		event_bitmap = (1ULL << (eventid - 1));
> > +		break;
> > +
> > +	case EVENT_OP_IS_ENTER_SELFREF:
> > +	case EVENT_OP_IS_ENTER_POWERDOWN:
> > +	case EVENT_OP_IS_ENTER_MPSM:
> > +		event_bitmap = (0xFULL << (eventid - 1));
> > +		break;
> > +	default:
> > +		pr_err("%s Invalid eventid %d\n", __func__, eventid);
> > +		break;
> > +	}
> > +
> > +	return event_bitmap;
> > +}
> > +
> > +static int cn10k_ddr_perf_alloc_counter(struct cn10k_ddr_pmu *pmu,
> > +					struct perf_event *event)
> > +{
> > +	uint8_t config = event->attr.config;
> > +	int i;
> > +
> > +	/* DDR read free-run counter index */
> > +	if (config == EVENT_DDR_READS) {
> > +		pmu->events[DDRC_PERF_READ_COUNTER_IDX] = event;
> > +		return DDRC_PERF_READ_COUNTER_IDX;
> > +	}
> > +
> > +	/* DDR write free-run counter index */
> > +	if (config == EVENT_DDR_WRITES) {
> > +		pmu->events[DDRC_PERF_WRITE_COUNTER_IDX] = event;
> > +		return DDRC_PERF_WRITE_COUNTER_IDX;
> > +	}
> > +
> > +	/* Allocate DDR generic counters */
> > +	for (i = 0; i < DDRC_PERF_NUM_GEN_COUNTERS; i++) {
> > +		if (pmu->events[i] == NULL) {
> > +			pmu->events[i] = event;
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return -ENOENT;
> > +}
> > +
> > +static void cn10k_ddr_perf_free_counter(struct cn10k_ddr_pmu *pmu,
> > +int counter) {
> > +	pmu->events[counter] = NULL;
> > +}
> > +
> > +static int cn10k_ddr_perf_event_init(struct perf_event *event) {
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (is_sampling_event(event)) {
> > +		dev_info(pmu->dev, "Sampling not supported!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (event->cpu < 0) {
> > +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	/*  We must NOT create groups containing mixed PMUs */
> > +	if (event->group_leader->pmu != event->pmu &&
> > +			!is_software_event(event->group_leader))
> > +		return -EINVAL;
> > +
> > +	/* Set ownership of event to one CPU, same event can not be observed
> > +	 * on multiple cpus at same time.
> > +	 */
> > +	event->cpu = pmu->cpu;
> > +	hwc->idx = -1;
> > +	return 0;
> > +}
> > +
> > +static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
> > +					  int counter, bool enable)
> > +{
> > +	uint32_t reg;
> > +	uint64_t val;
> > +
> > +	if (counter > DDRC_PERF_NUM_COUNTERS) {
> > +		pr_err("Error: unsupported counter %d\n", counter);
> > +		return;
> > +	}
> > +
> > +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> > +		reg = DDRC_PERF_CFG(counter);
> > +		val = readq_relaxed(pmu->base + reg);
> > +
> > +		if (enable)
> > +			val |= EVENT_ENABLE;
> > +		else
> > +			val &= ~EVENT_ENABLE;
> > +
> > +		writeq_relaxed(val, pmu->base + reg);
> > +	} else {
> > +		val = readq_relaxed(pmu->base +
> DDRC_PERF_CNT_FREERUN_EN);
> > +		if (enable) {
> > +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +				val |= DDRC_PERF_FREERUN_READ_EN;
> > +			else
> > +				val |= DDRC_PERF_FREERUN_WRITE_EN;
> > +		} else {
> > +			if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +				val &= ~DDRC_PERF_FREERUN_READ_EN;
> > +			else
> > +				val &= ~DDRC_PERF_FREERUN_WRITE_EN;
> > +		}
> > +		writeq_relaxed(val, pmu->base +
> DDRC_PERF_CNT_FREERUN_EN);
> > +	}
> > +}
> > +
> > +static uint64_t cn10k_ddr_perf_read_counter(struct cn10k_ddr_pmu *pmu,
> > +					    int counter)
> > +{
> > +	uint64_t val;
> > +
> > +	if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +		return readq_relaxed(pmu->base +
> DDRC_PERF_CNT_VALUE_RD_OP);
> > +
> > +	if (counter == DDRC_PERF_WRITE_COUNTER_IDX)
> > +		return readq_relaxed(pmu->base +
> DDRC_PERF_CNT_VALUE_WR_OP);
> > +
> > +	val = readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE(counter));
> > +	return val;
> > +}
> > +
> > +static void cn10k_ddr_perf_event_update(struct perf_event *event) {
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	uint64_t prev_count, new_count, mask;
> > +
> > +	do {
> > +		prev_count = local64_read(&hwc->prev_count);
> > +		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
> > +	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
> > +
> > +	mask = DDRC_PERF_CNT_MAX_VALUE;
> > +
> > +	local64_add((new_count - prev_count) & mask, &event->count); }
> > +
> > +static void cn10k_ddr_perf_event_start(struct perf_event *event, int
> > +flags) {
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int counter = hwc->idx;
> > +
> > +	local64_set(&hwc->prev_count, 0);
> > +
> > +	cn10k_ddr_perf_counter_enable(pmu, counter, true);
> > +
> > +	hwc->state = 0;
> > +}
> > +
> > +static int cn10k_ddr_perf_event_add(struct perf_event *event, int
> > +flags) {
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	uint8_t config = event->attr.config;
> > +	uint32_t reg_offset;
> > +	uint64_t val;
> > +	int counter;
> > +
> > +	counter = cn10k_ddr_perf_alloc_counter(pmu, event);
> > +	if (counter < 0) {
> > +		dev_dbg(pmu->dev, "There are not enough counters\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	pmu->active_events++;
> > +	hwc->idx = counter;
> > +
> > +	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
> > +		/* Generic counters, configure event id */
> > +		reg_offset = DDRC_PERF_CFG(counter);
> > +		val = ddr_perf_get_event_bitmap(config);
> > +		writeq_relaxed(val, pmu->base + reg_offset);
> > +	} else {
> > +		/* fixed event counter, clear counter value */
> > +		if (counter == DDRC_PERF_READ_COUNTER_IDX)
> > +			val = DDRC_FREERUN_READ_CNT_CLR;
> > +		else
> > +			val = DDRC_FREERUN_WRITE_CNT_CLR;
> > +
> > +		writeq_relaxed(val, pmu->base +
> DDRC_PERF_CNT_FREERUN_CTRL);
> > +	}
> > +
> > +	hwc->state |= PERF_HES_STOPPED;
> > +
> > +	if (flags & PERF_EF_START)
> > +		cn10k_ddr_perf_event_start(event, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void cn10k_ddr_perf_event_stop(struct perf_event *event, int
> > +flags) {
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int counter = hwc->idx;
> > +
> > +	cn10k_ddr_perf_counter_enable(pmu, counter, false);
> > +
> > +	if (flags & PERF_EF_UPDATE)
> > +		cn10k_ddr_perf_event_update(event);
> > +
> > +	hwc->state |= PERF_HES_STOPPED;
> > +}
> > +
> > +static void cn10k_ddr_perf_event_del(struct perf_event *event, int
> > +flags) {
> > +	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int counter = hwc->idx;
> > +
> > +	cn10k_ddr_perf_event_stop(event, PERF_EF_UPDATE);
> > +
> > +	cn10k_ddr_perf_free_counter(pmu, counter);
> > +	pmu->active_events--;
> > +	hwc->idx = -1;
> > +}
> > +
> > +static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu) {
> > +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> > +
> > +	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
> > +		       DDRC_PERF_CNT_START_OP_CTRL); }
> > +
> > +static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu) {
> > +	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
> > +
> > +	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
> > +		       DDRC_PERF_CNT_END_OP_CTRL);
> > +}
> > +
> > +static int cn10k_ddr_perf_probe(struct platform_device *pdev) {
> > +	struct cn10k_ddr_pmu *ddr_pmu;
> > +	struct resource *res;
> > +	void __iomem *base;
> > +	static int index;
> > +	char *name;
> > +	int ret;
> > +
> > +	ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(*ddr_pmu), GFP_KERNEL);
> > +	if (!ddr_pmu)
> > +		return -ENOMEM;
> > +
> > +	ddr_pmu->dev = &pdev->dev;
> > +	platform_set_drvdata(pdev, ddr_pmu);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	ddr_pmu->base = base;
> > +
> > +	/* Setup the PMU counter to work in mannual mode */
> > +	writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base +
> > +		       DDRC_PERF_CNT_OP_MODE_CTRL);
> > +
> > +	ddr_pmu->pmu = (struct pmu) {
> > +		.module	      = THIS_MODULE,
> > +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> > +		.task_ctx_nr = perf_invalid_context,
> > +		.attr_groups = cn10k_attr_groups,
> > +		.event_init  = cn10k_ddr_perf_event_init,
> > +		.add	     = cn10k_ddr_perf_event_add,
> > +		.del	     = cn10k_ddr_perf_event_del,
> > +		.start	     = cn10k_ddr_perf_event_start,
> > +		.stop	     = cn10k_ddr_perf_event_stop,
> > +		.read	     = cn10k_ddr_perf_event_update,
> > +		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
> > +		.pmu_disable = cn10k_ddr_perf_pmu_disable,
> > +	};
> > +
> > +	/* Choose this cpu to collect perf data */
> > +	ddr_pmu->cpu = raw_smp_processor_id();
> > +
> > +	name = devm_kasprintf(ddr_pmu->dev, GFP_KERNEL,
> "mrvl_ddr_pmu@%llx",
> > +			      res->start);
> > +	if (!name)
> > +		return -ENOMEM;
> > +
> > +	ret = perf_pmu_register(&ddr_pmu->pmu, name, -1);
> > +	if (ret)
> > +		return ret;
> 
> Hi Bharat,
>    Do we need to free memory allocated to ddr_pmu incase perf_pmu_register
> fails? I think right now we might have memory leaks here.

As we used devm_kzalloc() for allocation, it will be freed if probe fails for any reason.

> Also you added cpu hotplug support in patch 4, but cpu variable is added in this
> patch. Will it be better to move addition of cpu part also in patch 4?

This patch set default CPU, while patch 4/4 ad support for cpu hotplug.

Thanks
-Bharat

> 
> Thanks,
> Kajol Jain
> 
> > +
> > +	ddr_pmu->id = index++;
> > +	pr_info("CN10K DDR PMU Driver for ddrc@%llx - id-%d\n",
> > +		res->start, ddr_pmu->id);
> > +	return 0;
> > +}
> > +
> > +static int cn10k_ddr_perf_remove(struct platform_device *pdev) {
> > +	struct cn10k_ddr_pmu *ddr_pmu = platform_get_drvdata(pdev);
> > +
> > +	perf_pmu_unregister(&ddr_pmu->pmu);
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
> > +	{ .compatible = "marvell,cn10k-ddr-pmu", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
> > +
> > +static struct platform_driver cn10k_ddr_pmu_driver = {
> > +	.driver	= {
> > +		.name   = "cn10k-ddr-pmu",
> > +		.of_match_table = cn10k_ddr_pmu_of_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe		= cn10k_ddr_perf_probe,
> > +	.remove		= cn10k_ddr_perf_remove,
> > +};
> > +
> > +static int __init cn10k_ddr_pmu_init(void) {
> > +	return platform_driver_register(&cn10k_ddr_pmu_driver);
> > +}
> > +
> > +static void __exit cn10k_ddr_pmu_exit(void) {
> > +	platform_driver_unregister(&cn10k_ddr_pmu_driver);
> > +}
> > +
> > +module_init(cn10k_ddr_pmu_init);
> > +module_exit(cn10k_ddr_pmu_exit);
> > +
> > +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> > +MODULE_LICENSE("GPL v2");
> >

Re: [EXT] Re: [PATCH v2 2/4] perf/marvell: CN10k DDR performance monitor support

[John Garry]

On 19/08/2021 12:52, Bharat Bhushan wrote:
>> Is there anything to stop using adding COMPILE_TEST as a dependency?
>> This really helps build coverage testing for other archs
> Just keeping same as other drivers

I think then that may be something which could be improved for other 
drivers.

> 
>>> +	help
>>> +	  Enable perf support for Marvell DDR Performance monitoring
>>> +	  event on CN10K platform.
>>> +
>>>    endmenu
>>> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
>>> index 5260b116c7da..ee1126219d8d 100644
>>> --- a/drivers/perf/Makefile
>>> +++ b/drivers/perf/Makefile
>>> @@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
>>>    obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>>>    obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
>>>    obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
>>> +obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
>>> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c
>> b/drivers/perf/marvell_cn10k_ddr_pmu.c
>>> new file mode 100644
>>> index 000000000000..8f9e3d1fcd8d
>>> --- /dev/null
>>> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
>>> @@ -0,0 +1,606 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
>>> + *
>>> + * Copyright (C) 2021 Marvell.
>>> + */
>>> +

...

>>> +/* Event counter value registers */
>>> +#define DDRC_PERF_CNT_VALUE_BASE		0x8080
>>> +#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
>>> +
>>> +/* Fixed event counter enable/disable register */
>>> +#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
>>> +#define DDRC_PERF_FREERUN_WRITE_EN	0x1
>>> +#define DDRC_PERF_FREERUN_READ_EN	0x2
>>> +
>>> +/* Fixed event counter control register */
>>> +#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
>>> +#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
>>> +#define DDRC_FREERUN_READ_CNT_CLR	0x2
>>> +
>>> +/* Fixed event counter value register */
>>> +#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
>>> +#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
>>> +#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
>>> +#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
>> I assume all these macros are used...
> Yes, do you see any unused?

I didn't check

> 
>>> +
>>> +struct cn10k_ddr_pmu {
>>> +	struct pmu pmu;

Thanks,
john