[PATCH v3 0/5] stm32-ddr-pmu driver creation

[PATCH v3 0/5] stm32-ddr-pmu driver creation

[Gerald BAEZA]

The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.

This series adds support for the DDRPERFM via a new stm32-ddr-pmu driver,
registered into the perf framework.

This driver is inspired from arch/arm/mm/cache-l2x0-pmu.c

---
Changes from v1:
- add 'resets' description (bindings) and using (driver). Thanks Rob.
- rebase on 5.2-rc1 (that includes the ddrperfm clock control patch).

Changes from v2:
- rebase on 5.3-rc6 that has to be completed with
  'perf tools: fix alignment trap in perf stat': mandatory.
  'Documentation: add link to stm32mp157 docs': referenced from this series.
- take into account all remarks from Mark Rutland: thanks for your time!
  https://lkml.org/lkml/2019/6/26/388
- fix for event type filtering in stm32_ddr_pmu_event_init()

Gerald Baeza (5):
  Documentation: perf: stm32: ddrperfm support
  dt-bindings: perf: stm32: ddrperfm support
  perf: stm32: ddrperfm driver creation
  ARM: configs: enable STM32_DDR_PMU
  ARM: dts: stm32: add ddrperfm on stm32mp157c

 .../devicetree/bindings/perf/stm32-ddr-pmu.txt     |  16 +
 Documentation/perf/stm32-ddr-pmu.txt               |  37 ++
 arch/arm/boot/dts/stm32mp157c.dtsi                 |   8 +
 arch/arm/configs/multi_v7_defconfig                |   1 +
 drivers/perf/Kconfig                               |   6 +
 drivers/perf/Makefile                              |   1 +
 drivers/perf/stm32_ddr_pmu.c                       | 426 +++++++++++++++++++++
 7 files changed, 495 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/stm32-ddr-pmu.txt
 create mode 100644 Documentation/perf/stm32-ddr-pmu.txt
 create mode 100644 drivers/perf/stm32_ddr_pmu.c

-- 
2.7.4

[PATCH v3 2/5] dt-bindings: perf: stm32: ddrperfm support

[Gerald BAEZA]

The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.

This documentation indicates how to enable stm32-ddr-pmu driver on
DDRPERFM peripheral, via the device tree.

Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
---
 Documentation/devicetree/bindings/perf/stm32-ddr-pmu.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/stm32-ddr-pmu.txt

diff --git a/Documentation/devicetree/bindings/perf/stm32-ddr-pmu.txt b/Documentation/devicetree/bindings/perf/stm32-ddr-pmu.txt
new file mode 100644
index 0000000..87ab12e
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/stm32-ddr-pmu.txt
@@ -0,0 +1,16 @@
+* STM32 DDR Performance Monitor (DDRPERFM)
+
+Required properties:
+- compatible: must be "st,stm32-ddr-pmu".
+- reg: physical address and length of the registers set.
+- clocks: phandle and specifier for DDRPERFM input clock
+- resets: phandle and specifier for DDRPERFM reset
+
+Example:
+	ddrperfm: perf@5a007000 {
+		compatible = "st,stm32-ddr-pmu";
+		reg = <0x5a007000 0x400>;
+		clocks = <&rcc DDRPERFM>;
+		resets = <&rcc DDRPERFM_R>;
+	};
+
-- 
2.7.4

[PATCH v3 1/5] Documentation: perf: stm32: ddrperfm support

[Gerald BAEZA]

The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.

This documentation introduces the DDRPERFM, the stm32-ddr-pmu driver
supporting it and how to use it with the perf tool.

Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
---
 Documentation/perf/stm32-ddr-pmu.txt | 37 ++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/perf/stm32-ddr-pmu.txt

diff --git a/Documentation/perf/stm32-ddr-pmu.txt b/Documentation/perf/stm32-ddr-pmu.txt
new file mode 100644
index 0000000..557bf47
--- /dev/null
+++ b/Documentation/perf/stm32-ddr-pmu.txt
@@ -0,0 +1,37 @@
+STM32 DDR Performance Monitor (DDRPERFM)
+========================================
+
+The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.
+See Documentation/arm/stm32/stm32mp157-overview.rst to get access to
+STM32MP157 reference manual RM0436 where DDRPERFM is described.
+
+
+The five following counters are supported by stm32-ddr-pmu driver:
+	cnt0: read operations counters		(read_cnt)
+	cnt1: write operations counters		(write_cnt)
+	cnt2: active state counters		(activate_cnt)
+	cnt3: idle state counters		(idle_cnt)
+	tcnt: time count, present for all sets	(time_cnt)
+
+The stm32-ddr-pmu driver relies on the perf PMU framework to expose the
+counters via sysfs:
+	$ ls /sys/bus/event_source/devices/ddrperfm/events
+	activate_cnt  idle_cnt  read_cnt  time_cnt  write_cnt
+
+
+The perf PMU framework is usually invoked via the 'perf stat' tool.
+
+The DDRPERFM is a system monitor that cannot isolate the traffic coming from a
+given thread or CPU, that is why stm32-ddr-pmu driver rejects any 'perf stat'
+call that does not request a system-wide collection: the '-a, --all-cpus'
+option is mandatory!
+
+Example:
+	$ perf stat -e ddrperfm/read_cnt/,ddrperfm/time_cnt/ -a sleep 20
+	Performance counter stats for 'system wide':
+
+	         342541560      ddrperfm/read_cnt/
+	       10660011400      ddrperfm/time_cnt/
+
+	      20.021068551 seconds time elapsed
+
-- 
2.7.4

[PATCH v3 3/5] perf: stm32: ddrperfm driver creation

[Gerald BAEZA]

The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.

This perf drivers supports the read, write, activate, idle and total
time counters, described in the reference manual RM0436 that is
accessible from Documentation/arm/stm32/stm32mp157-overview.rst

Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
---
 drivers/perf/Kconfig         |   6 +
 drivers/perf/Makefile        |   1 +
 drivers/perf/stm32_ddr_pmu.c | 426 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 433 insertions(+)
 create mode 100644 drivers/perf/stm32_ddr_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 09ae8a9..a3d917e 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -114,6 +114,12 @@ config THUNDERX2_PMU
 	   The SoC has PMU support in its L3 cache controller (L3C) and
 	   in the DDR4 Memory Controller (DMC).
 
+config STM32_DDR_PMU
+       tristate "STM32 DDR PMU"
+       depends on MACH_STM32MP157
+       help
+         Support for STM32 DDR performance monitor (DDRPERFM).
+
 config XGENE_PMU
         depends on ARCH_XGENE
         bool "APM X-Gene SoC PMU"
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index 2ebb4de..fd3368c 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_FSL_IMX8_DDR_PMU) += fsl_imx8_ddr_perf.o
 obj-$(CONFIG_HISI_PMU) += hisilicon/
 obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
 obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
+obj-$(CONFIG_STM32_DDR_PMU) += stm32_ddr_pmu.o
 obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
 obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
 obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
new file mode 100644
index 0000000..d0480e0
--- /dev/null
+++ b/drivers/perf/stm32_ddr_pmu.c
@@ -0,0 +1,426 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file is the STM32 DDR performance monitor (DDRPERFM) driver
+ *
+ * Copyright (C) 2019, STMicroelectronics - All Rights Reserved
+ * Author: Gerald Baeza <gerald.baeza@st.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/hrtimer.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/perf_event.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+/*
+ * The PMU is able to freeze all counters and generate an interrupt when there
+ * is a counter overflow. But, relying on this means that we lose all the
+ * events that occur between the freeze and the interrupt handler execution.
+ * So we use a polling mechanism to avoid this lose of information.
+ * The fastest counter can overflow in ~8s @533MHz (that is the maximum DDR
+ * frequency supported on STM32MP157), so we poll in 4s intervals to ensure
+ * we don't reach this limit.
+ */
+#define POLL_MS		4000
+
+#define DDRPERFM_CTL	0x000
+#define DDRPERFM_CFG	0x004
+#define DDRPERFM_STATUS 0x008
+#define DDRPERFM_CCR	0x00C
+#define DDRPERFM_IER	0x010
+#define DDRPERFM_ISR	0x014
+#define DDRPERFM_ICR	0x018
+#define DDRPERFM_TCNT	0x020
+#define DDRPERFM_CNT(X)	(0x030 + 8 * (X))
+#define DDRPERFM_HWCFG	0x3F0
+#define DDRPERFM_VER	0x3F4
+#define DDRPERFM_ID	0x3F8
+#define DDRPERFM_SID	0x3FC
+
+#define CTL_START	0x00000001
+#define CTL_STOP	0x00000002
+#define CCR_CLEAR_ALL	0x8000000F
+#define SID_MAGIC_ID	0xA3C5DD01
+
+enum {
+	READ_CNT,
+	WRITE_CNT,
+	ACTIVATE_CNT,
+	IDLE_CNT,
+	TIME_CNT,
+	PMU_NR_COUNTERS
+};
+
+struct stm32_ddr_pmu {
+	struct pmu pmu;
+	void __iomem *membase;
+	struct clk *clk;
+	struct hrtimer hrtimer;
+	cpumask_t pmu_cpu;
+	ktime_t poll_period;
+	struct perf_event *events[PMU_NR_COUNTERS];
+	u64 events_cnt[PMU_NR_COUNTERS];
+};
+
+static inline struct stm32_ddr_pmu *pmu_to_stm32_ddr_pmu(struct pmu *p)
+{
+	return container_of(p, struct stm32_ddr_pmu, pmu);
+}
+
+static inline struct stm32_ddr_pmu *hrtimer_to_stm32_ddr_pmu(struct hrtimer *h)
+{
+	return container_of(h, struct stm32_ddr_pmu, hrtimer);
+}
+
+static void stm32_ddr_pmu_event_configure(struct perf_event *event)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	unsigned long config_base = event->hw.config_base;
+	u32 val;
+
+	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
+
+	if (config_base < TIME_CNT) {
+		val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
+		val |= (1 << config_base);
+		writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
+	}
+}
+
+static void stm32_ddr_pmu_event_read(struct perf_event *event)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	unsigned long config_base = event->hw.config_base;
+	struct hw_perf_event *hw = &event->hw;
+	u64 prev_count, new_count, mask;
+	u32 val, offset, bit;
+
+	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
+
+	if (config_base == TIME_CNT) {
+		offset = DDRPERFM_TCNT;
+		bit = 1 << 31;
+	} else {
+		offset = DDRPERFM_CNT(config_base);
+		bit = 1 << config_base;
+	}
+	val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_STATUS);
+	if (val & bit)
+		pr_warn("STM32 DDR PMU hardware counter overflow\n");
+	val = readl_relaxed(stm32_ddr_pmu->membase + offset);
+	writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
+	writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
+
+	do {
+		prev_count = local64_read(&hw->prev_count);
+		new_count = prev_count + val;
+	} while (local64_xchg(&hw->prev_count, new_count) != prev_count);
+
+	mask = GENMASK_ULL(31, 0);
+	local64_add(val & mask, &event->count);
+
+	if (new_count < prev_count)
+		pr_warn("STM32 DDR PMU software counter rollover\n");
+}
+
+static void stm32_ddr_pmu_event_start(struct perf_event *event, int flags)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	struct hw_perf_event *hw = &event->hw;
+
+	if (WARN_ON_ONCE(!(hw->state & PERF_HES_STOPPED)))
+		return;
+
+	if (flags & PERF_EF_RELOAD)
+		WARN_ON_ONCE(!(hw->state & PERF_HES_UPTODATE));
+
+	stm32_ddr_pmu_event_configure(event);
+
+	/* Clear all counters to synchronize them, then start */
+	writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase + DDRPERFM_CCR);
+	writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
+	local64_set(&hw->prev_count, 0);
+	hw->state = 0;
+}
+
+static void stm32_ddr_pmu_event_stop(struct perf_event *event, int flags)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	unsigned long config_base = event->hw.config_base;
+	struct hw_perf_event *hw = &event->hw;
+	u32 val, bit;
+
+	if (WARN_ON_ONCE(hw->state & PERF_HES_STOPPED))
+		return;
+
+	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
+	if (config_base == TIME_CNT)
+		bit = 1 << 31;
+	else
+		bit = 1 << config_base;
+	writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
+	if (config_base < TIME_CNT) {
+		val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
+		val &= ~bit;
+		writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
+	}
+
+	hw->state |= PERF_HES_STOPPED;
+
+	if (flags & PERF_EF_UPDATE) {
+		stm32_ddr_pmu_event_read(event);
+		hw->state |= PERF_HES_UPTODATE;
+	}
+}
+
+static int stm32_ddr_pmu_event_add(struct perf_event *event, int flags)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	unsigned long config_base = event->hw.config_base;
+	struct hw_perf_event *hw = &event->hw;
+
+	stm32_ddr_pmu->events_cnt[config_base] = 0;
+	stm32_ddr_pmu->events[config_base] = event;
+
+	clk_enable(stm32_ddr_pmu->clk);
+	/*
+	 * Pin the timer, so that the overflows are handled by the chosen
+	 * event->cpu (this is the same one as presented in "cpumask"
+	 * attribute).
+	 */
+	hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu->poll_period,
+		      HRTIMER_MODE_REL_PINNED);
+
+	stm32_ddr_pmu_event_configure(event);
+
+	hw->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
+
+	if (flags & PERF_EF_START)
+		stm32_ddr_pmu_event_start(event, 0);
+
+	return 0;
+}
+
+static void stm32_ddr_pmu_event_del(struct perf_event *event, int flags)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	unsigned long config_base = event->hw.config_base;
+	bool stop = true;
+	int i;
+
+	stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
+
+	stm32_ddr_pmu->events_cnt[config_base] += local64_read(&event->count);
+	stm32_ddr_pmu->events[config_base] = NULL;
+
+	for (i = 0; i < PMU_NR_COUNTERS; i++)
+		if (stm32_ddr_pmu->events[i])
+			stop = false;
+	if (stop)
+		hrtimer_cancel(&stm32_ddr_pmu->hrtimer);
+
+	clk_disable(stm32_ddr_pmu->clk);
+}
+
+static int stm32_ddr_pmu_event_init(struct perf_event *event)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
+	struct hw_perf_event *hw = &event->hw;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (is_sampling_event(event))
+		return -EINVAL;
+
+	if (event->attach_state & PERF_ATTACH_TASK)
+		return -EINVAL;
+
+	if (event->attr.exclude_user   ||
+	    event->attr.exclude_kernel ||
+	    event->attr.exclude_hv     ||
+	    event->attr.exclude_idle   ||
+	    event->attr.exclude_host   ||
+	    event->attr.exclude_guest)
+		return -EINVAL;
+
+	if (event->cpu < 0)
+		return -EINVAL;
+
+	hw->config_base = event->attr.config;
+	event->cpu = cpumask_first(&stm32_ddr_pmu->pmu_cpu);
+
+	return 0;
+}
+
+static enum hrtimer_restart stm32_ddr_pmu_poll(struct hrtimer *hrtimer)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = hrtimer_to_stm32_ddr_pmu(hrtimer);
+	int i;
+
+	for (i = 0; i < PMU_NR_COUNTERS; i++)
+		if (stm32_ddr_pmu->events[i])
+			stm32_ddr_pmu_event_read(stm32_ddr_pmu->events[i]);
+
+	hrtimer_forward_now(hrtimer, stm32_ddr_pmu->poll_period);
+
+	return HRTIMER_RESTART;
+}
+
+static ssize_t stm32_ddr_pmu_sysfs_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct dev_ext_attribute *eattr;
+
+	eattr = container_of(attr, struct dev_ext_attribute, attr);
+
+	return sprintf(buf, "config=0x%lx\n", (unsigned long)eattr->var);
+}
+
+#define STM32_DDR_PMU_ATTR(_name, _func, _config)			\
+	(&((struct dev_ext_attribute[]) {				\
+		{ __ATTR(_name, 0444, _func, NULL), (void *)_config }   \
+	})[0].attr.attr)
+
+#define STM32_DDR_PMU_EVENT_ATTR(_name, _config)		\
+	STM32_DDR_PMU_ATTR(_name, stm32_ddr_pmu_sysfs_show,	\
+			   (unsigned long)_config)
+
+static struct attribute *stm32_ddr_pmu_event_attrs[] = {
+	STM32_DDR_PMU_EVENT_ATTR(read_cnt, READ_CNT),
+	STM32_DDR_PMU_EVENT_ATTR(write_cnt, WRITE_CNT),
+	STM32_DDR_PMU_EVENT_ATTR(activate_cnt, ACTIVATE_CNT),
+	STM32_DDR_PMU_EVENT_ATTR(idle_cnt, IDLE_CNT),
+	STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT),
+	NULL
+};
+
+static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
+	.name = "events",
+	.attrs = stm32_ddr_pmu_event_attrs,
+};
+
+static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
+	&stm32_ddr_pmu_event_attrs_group,
+	NULL,
+};
+
+static int stm32_ddr_pmu_device_probe(struct platform_device *pdev)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu;
+	struct reset_control *rst;
+	struct resource *res;
+	int i, ret;
+	u32 val;
+
+	stm32_ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(struct stm32_ddr_pmu),
+				     GFP_KERNEL);
+	if (!stm32_ddr_pmu)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, stm32_ddr_pmu);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	stm32_ddr_pmu->membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(stm32_ddr_pmu->membase)) {
+		pr_warn("Unable to get STM32 DDR PMU membase\n");
+		return PTR_ERR(stm32_ddr_pmu->membase);
+	}
+
+	stm32_ddr_pmu->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(stm32_ddr_pmu->clk)) {
+		pr_warn("Unable to get STM32 DDR PMU clock\n");
+		return PTR_ERR(stm32_ddr_pmu->clk);
+	}
+
+	ret = clk_prepare_enable(stm32_ddr_pmu->clk);
+	if (ret) {
+		pr_warn("Unable to prepare STM32 DDR PMU clock\n");
+		return ret;
+	}
+
+	stm32_ddr_pmu->poll_period = ms_to_ktime(POLL_MS);
+	hrtimer_init(&stm32_ddr_pmu->hrtimer, CLOCK_MONOTONIC,
+		     HRTIMER_MODE_REL);
+	stm32_ddr_pmu->hrtimer.function = stm32_ddr_pmu_poll;
+
+	/*
+	 * The PMU is assigned to the cpu0 and there is no need to manage cpu
+	 * hot plug migration because cpu0 is always the first/last active cpu
+	 * during low power transitions.
+	 */
+	cpumask_set_cpu(0, &stm32_ddr_pmu->pmu_cpu);
+
+	for (i = 0; i < PMU_NR_COUNTERS; i++) {
+		stm32_ddr_pmu->events[i] = NULL;
+		stm32_ddr_pmu->events_cnt[i] = 0;
+	}
+
+	val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_SID);
+	if (val != SID_MAGIC_ID)
+		return -EINVAL;
+
+	stm32_ddr_pmu->pmu = (struct pmu) {
+		.task_ctx_nr = perf_invalid_context,
+		.start = stm32_ddr_pmu_event_start,
+		.stop = stm32_ddr_pmu_event_stop,
+		.add = stm32_ddr_pmu_event_add,
+		.del = stm32_ddr_pmu_event_del,
+		.event_init = stm32_ddr_pmu_event_init,
+		.attr_groups = stm32_ddr_pmu_attr_groups,
+	};
+	ret = perf_pmu_register(&stm32_ddr_pmu->pmu, "stm32_ddr_pmu", -1);
+	if (ret) {
+		pr_warn("Unable to register STM32 DDR PMU\n");
+		return ret;
+	}
+
+	rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (!IS_ERR(rst)) {
+		reset_control_assert(rst);
+		udelay(2);
+		reset_control_deassert(rst);
+	}
+
+	pr_info("stm32-ddr-pmu: probed (DDRPERFM ID=0x%08x VER=0x%08x)\n",
+		readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_ID),
+		readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_VER));
+
+	clk_disable(stm32_ddr_pmu->clk);
+
+	return 0;
+}
+
+static int stm32_ddr_pmu_device_remove(struct platform_device *pdev)
+{
+	struct stm32_ddr_pmu *stm32_ddr_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&stm32_ddr_pmu->pmu);
+
+	return 0;
+}
+
+static const struct of_device_id stm32_ddr_pmu_of_match[] = {
+	{ .compatible = "st,stm32-ddr-pmu" },
+	{ },
+};
+
+static struct platform_driver stm32_ddr_pmu_driver = {
+	.driver = {
+		.name	= "stm32-ddr-pmu",
+		.of_match_table = of_match_ptr(stm32_ddr_pmu_of_match),
+	},
+	.probe = stm32_ddr_pmu_device_probe,
+	.remove = stm32_ddr_pmu_device_remove,
+};
+
+module_platform_driver(stm32_ddr_pmu_driver);
+
+MODULE_DESCRIPTION("Perf driver for STM32 DDR performance monitor");
+MODULE_AUTHOR("Gerald Baeza <gerald.baeza@st.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v3 5/5] ARM: dts: stm32: add ddrperfm on stm32mp157c

[Gerald BAEZA]

The DDRPERFM is the DDR Performance Monitor embedded
in STM32MP1 SOC.

Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
---
 arch/arm/boot/dts/stm32mp157c.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
index 0c4e6eb..6ea6933 100644
--- a/arch/arm/boot/dts/stm32mp157c.dtsi
+++ b/arch/arm/boot/dts/stm32mp157c.dtsi
@@ -1378,6 +1378,14 @@
 			};
 		};
 
+		ddrperfm: perf@5a007000 {
+			compatible = "st,stm32-ddr-pmu";
+			reg = <0x5a007000 0x400>;
+			clocks = <&rcc DDRPERFM>;
+			resets = <&rcc DDRPERFM_R>;
+			status = "okay";
+		};
+
 		usart1: serial@5c000000 {
 			compatible = "st,stm32h7-uart";
 			reg = <0x5c000000 0x400>;
-- 
2.7.4

[PATCH v3 4/5] ARM: configs: enable STM32_DDR_PMU

[Gerald BAEZA]

STM32_DDR_PMU enables the perf driver that
controls the DDR Performance Monitor (DDRPERFM)

Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 6a40bc2..8fa4690 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -1011,6 +1011,7 @@ CONFIG_PHY_DM816X_USB=m
 CONFIG_OMAP_USB2=y
 CONFIG_TI_PIPE3=y
 CONFIG_TWL4030_USB=m
+CONFIG_STM32_DDR_PMU=m
 CONFIG_MESON_MX_EFUSE=m
 CONFIG_ROCKCHIP_EFUSE=m
 CONFIG_NVMEM_IMX_OCOTP=y
-- 
2.7.4

Re: [PATCH v3 5/5] ARM: dts: stm32: add ddrperfm on stm32mp157c

[Alexandre Torgue]

Hi Gerald

On 8/27/19 5:08 PM, Gerald BAEZA wrote:
> The DDRPERFM is the DDR Performance Monitor embedded
> in STM32MP1 SOC.
> 
> Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> ---
>   arch/arm/boot/dts/stm32mp157c.dtsi | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi 
> b/arch/arm/boot/dts/stm32mp157c.dtsi
> index 0c4e6eb..6ea6933 100644
> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
> @@ -1378,6 +1378,14 @@
>                           };
>                   };
> 
> +               ddrperfm: perf@5a007000 {
> +                       compatible = "st,stm32-ddr-pmu";
> +                       reg = <0x5a007000 0x400>;
> +                       clocks = <&rcc DDRPERFM>;
> +                       resets = <&rcc DDRPERFM_R>;
> +                       status = "okay";

No need to add "status = "okay"" here.

regards
Alex

> +               };
> +
>                   usart1: serial@5c000000 {
>                           compatible = "st,stm32h7-uart";
>                           reg = <0x5c000000 0x400>;
> -- 
> 2.7.4

RE: [PATCH v3 0/5] stm32-ddr-pmu driver creation

[Gerald BAEZA]

Dear all

Gentle reminder for this review.

Thanks in advance !

Gérald

> From: Gerald BAEZA <gerald.baeza@st.com>
> 
> The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1
> SOC.
> 
> This series adds support for the DDRPERFM via a new stm32-ddr-pmu driver,
> registered into the perf framework.
> 
> This driver is inspired from arch/arm/mm/cache-l2x0-pmu.c
> 
> ---
> Changes from v1:
> - add 'resets' description (bindings) and using (driver). Thanks Rob.
> - rebase on 5.2-rc1 (that includes the ddrperfm clock control patch).
> 
> Changes from v2:
> - rebase on 5.3-rc6 that has to be completed with
>   'perf tools: fix alignment trap in perf stat': mandatory.
>   'Documentation: add link to stm32mp157 docs': referenced from this series.
> - take into account all remarks from Mark Rutland: thanks for your time!
>   https://lkml.org/lkml/2019/6/26/388
> - fix for event type filtering in stm32_ddr_pmu_event_init()
> 
> Gerald Baeza (5):
>   Documentation: perf: stm32: ddrperfm support
>   dt-bindings: perf: stm32: ddrperfm support
>   perf: stm32: ddrperfm driver creation
>   ARM: configs: enable STM32_DDR_PMU
>   ARM: dts: stm32: add ddrperfm on stm32mp157c
> 
>  .../devicetree/bindings/perf/stm32-ddr-pmu.txt     |  16 +
>  Documentation/perf/stm32-ddr-pmu.txt               |  37 ++
>  arch/arm/boot/dts/stm32mp157c.dtsi                 |   8 +
>  arch/arm/configs/multi_v7_defconfig                |   1 +
>  drivers/perf/Kconfig                               |   6 +
>  drivers/perf/Makefile                              |   1 +
>  drivers/perf/stm32_ddr_pmu.c                       | 426 +++++++++++++++++++++
>  7 files changed, 495 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/stm32-ddr-
> pmu.txt
>  create mode 100644 Documentation/perf/stm32-ddr-pmu.txt
>  create mode 100644 drivers/perf/stm32_ddr_pmu.c
> 
> --
> 2.7.4

Re: [PATCH v3 3/5] perf: stm32: ddrperfm driver creation

[Will Deacon]

On Tue, Aug 27, 2019 at 03:08:20PM +0000, Gerald BAEZA wrote:
> The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.
> 
> This perf drivers supports the read, write, activate, idle and total
> time counters, described in the reference manual RM0436 that is
> accessible from Documentation/arm/stm32/stm32mp157-overview.rst
> 
> Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> ---
>  drivers/perf/Kconfig         |   6 +
>  drivers/perf/Makefile        |   1 +
>  drivers/perf/stm32_ddr_pmu.c | 426 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 433 insertions(+)
>  create mode 100644 drivers/perf/stm32_ddr_pmu.c
> 
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 09ae8a9..a3d917e 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -114,6 +114,12 @@ config THUNDERX2_PMU
>  	   The SoC has PMU support in its L3 cache controller (L3C) and
>  	   in the DDR4 Memory Controller (DMC).
>  
> +config STM32_DDR_PMU
> +       tristate "STM32 DDR PMU"
> +       depends on MACH_STM32MP157
> +       help
> +         Support for STM32 DDR performance monitor (DDRPERFM).

Weird indentation here (spaces not tabes?).

>  config XGENE_PMU
>          depends on ARCH_XGENE
>          bool "APM X-Gene SoC PMU"
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 2ebb4de..fd3368c 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_FSL_IMX8_DDR_PMU) += fsl_imx8_ddr_perf.o
>  obj-$(CONFIG_HISI_PMU) += hisilicon/
>  obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
>  obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
> +obj-$(CONFIG_STM32_DDR_PMU) += stm32_ddr_pmu.o
>  obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
>  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>  obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
> diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
> new file mode 100644
> index 0000000..d0480e0
> --- /dev/null
> +++ b/drivers/perf/stm32_ddr_pmu.c
> @@ -0,0 +1,426 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This file is the STM32 DDR performance monitor (DDRPERFM) driver
> + *
> + * Copyright (C) 2019, STMicroelectronics - All Rights Reserved
> + * Author: Gerald Baeza <gerald.baeza@st.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/hrtimer.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/perf_event.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +/*
> + * The PMU is able to freeze all counters and generate an interrupt when there
> + * is a counter overflow. But, relying on this means that we lose all the
> + * events that occur between the freeze and the interrupt handler execution.
> + * So we use a polling mechanism to avoid this lose of information.
> + * The fastest counter can overflow in ~8s @533MHz (that is the maximum DDR
> + * frequency supported on STM32MP157), so we poll in 4s intervals to ensure
> + * we don't reach this limit.
> + */
> +#define POLL_MS		4000
> +
> +#define DDRPERFM_CTL	0x000
> +#define DDRPERFM_CFG	0x004
> +#define DDRPERFM_STATUS 0x008
> +#define DDRPERFM_CCR	0x00C
> +#define DDRPERFM_IER	0x010
> +#define DDRPERFM_ISR	0x014
> +#define DDRPERFM_ICR	0x018
> +#define DDRPERFM_TCNT	0x020
> +#define DDRPERFM_CNT(X)	(0x030 + 8 * (X))
> +#define DDRPERFM_HWCFG	0x3F0
> +#define DDRPERFM_VER	0x3F4
> +#define DDRPERFM_ID	0x3F8
> +#define DDRPERFM_SID	0x3FC
> +
> +#define CTL_START	0x00000001
> +#define CTL_STOP	0x00000002
> +#define CCR_CLEAR_ALL	0x8000000F
> +#define SID_MAGIC_ID	0xA3C5DD01

What's this for? The check during probe looks weird.

> +
> +enum {
> +	READ_CNT,
> +	WRITE_CNT,
> +	ACTIVATE_CNT,
> +	IDLE_CNT,
> +	TIME_CNT,
> +	PMU_NR_COUNTERS
> +};

I think these correspond directly to the values set by userspace in
attr.config, so you probably want to clamp attr.config to be <
PMU_NR_COUNTERS in stm32_ddr_pmu_event_init().

> +struct stm32_ddr_pmu {
> +	struct pmu pmu;
> +	void __iomem *membase;
> +	struct clk *clk;
> +	struct hrtimer hrtimer;
> +	cpumask_t pmu_cpu;
> +	ktime_t poll_period;
> +	struct perf_event *events[PMU_NR_COUNTERS];
> +	u64 events_cnt[PMU_NR_COUNTERS];
> +};
> +
> +static inline struct stm32_ddr_pmu *pmu_to_stm32_ddr_pmu(struct pmu *p)
> +{
> +	return container_of(p, struct stm32_ddr_pmu, pmu);
> +}
> +
> +static inline struct stm32_ddr_pmu *hrtimer_to_stm32_ddr_pmu(struct hrtimer *h)
> +{
> +	return container_of(h, struct stm32_ddr_pmu, hrtimer);
> +}
> +
> +static void stm32_ddr_pmu_event_configure(struct perf_event *event)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	unsigned long config_base = event->hw.config_base;
> +	u32 val;
> +
> +	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +
> +	if (config_base < TIME_CNT) {
> +		val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +		val |= (1 << config_base);
> +		writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +	}
> +}
> +
> +static void stm32_ddr_pmu_event_read(struct perf_event *event)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	unsigned long config_base = event->hw.config_base;
> +	struct hw_perf_event *hw = &event->hw;
> +	u64 prev_count, new_count, mask;
> +	u32 val, offset, bit;
> +
> +	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +
> +	if (config_base == TIME_CNT) {
> +		offset = DDRPERFM_TCNT;
> +		bit = 1 << 31;
> +	} else {
> +		offset = DDRPERFM_CNT(config_base);
> +		bit = 1 << config_base;
> +	}
> +	val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_STATUS);
> +	if (val & bit)
> +		pr_warn("STM32 DDR PMU hardware counter overflow\n");

I don't think this print is useful. Surely overflow is fatal and you should
do something like put the event into an error state?

> +	val = readl_relaxed(stm32_ddr_pmu->membase + offset);
> +	writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> +	writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +
> +	do {
> +		prev_count = local64_read(&hw->prev_count);
> +		new_count = prev_count + val;
> +	} while (local64_xchg(&hw->prev_count, new_count) != prev_count);
> +
> +	mask = GENMASK_ULL(31, 0);
> +	local64_add(val & mask, &event->count);
> +
> +	if (new_count < prev_count)
> +		pr_warn("STM32 DDR PMU software counter rollover\n");

These are 64-bit. How fast do you expect the counters to tick?

> +static void stm32_ddr_pmu_event_start(struct perf_event *event, int flags)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hw = &event->hw;
> +
> +	if (WARN_ON_ONCE(!(hw->state & PERF_HES_STOPPED)))
> +		return;
> +
> +	if (flags & PERF_EF_RELOAD)
> +		WARN_ON_ONCE(!(hw->state & PERF_HES_UPTODATE));
> +
> +	stm32_ddr_pmu_event_configure(event);
> +
> +	/* Clear all counters to synchronize them, then start */
> +	writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> +	writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +	local64_set(&hw->prev_count, 0);
> +	hw->state = 0;
> +}
> +
> +static void stm32_ddr_pmu_event_stop(struct perf_event *event, int flags)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	unsigned long config_base = event->hw.config_base;
> +	struct hw_perf_event *hw = &event->hw;
> +	u32 val, bit;
> +
> +	if (WARN_ON_ONCE(hw->state & PERF_HES_STOPPED))
> +		return;
> +
> +	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +	if (config_base == TIME_CNT)
> +		bit = 1 << 31;
> +	else
> +		bit = 1 << config_base;
> +	writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> +	if (config_base < TIME_CNT) {
> +		val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +		val &= ~bit;
> +		writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +	}
> +
> +	hw->state |= PERF_HES_STOPPED;
> +
> +	if (flags & PERF_EF_UPDATE) {
> +		stm32_ddr_pmu_event_read(event);
> +		hw->state |= PERF_HES_UPTODATE;
> +	}
> +}
> +
> +static int stm32_ddr_pmu_event_add(struct perf_event *event, int flags)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	unsigned long config_base = event->hw.config_base;
> +	struct hw_perf_event *hw = &event->hw;
> +
> +	stm32_ddr_pmu->events_cnt[config_base] = 0;
> +	stm32_ddr_pmu->events[config_base] = event;
> +
> +	clk_enable(stm32_ddr_pmu->clk);
> +	/*
> +	 * Pin the timer, so that the overflows are handled by the chosen
> +	 * event->cpu (this is the same one as presented in "cpumask"
> +	 * attribute).
> +	 */
> +	hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu->poll_period,
> +		      HRTIMER_MODE_REL_PINNED);
> +
> +	stm32_ddr_pmu_event_configure(event);
> +
> +	hw->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +
> +	if (flags & PERF_EF_START)
> +		stm32_ddr_pmu_event_start(event, 0);
> +
> +	return 0;
> +}
> +
> +static void stm32_ddr_pmu_event_del(struct perf_event *event, int flags)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	unsigned long config_base = event->hw.config_base;
> +	bool stop = true;
> +	int i;
> +
> +	stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
> +
> +	stm32_ddr_pmu->events_cnt[config_base] += local64_read(&event->count);
> +	stm32_ddr_pmu->events[config_base] = NULL;
> +
> +	for (i = 0; i < PMU_NR_COUNTERS; i++)
> +		if (stm32_ddr_pmu->events[i])
> +			stop = false;
> +	if (stop)

This is just i == PMU_NR_COUNTERS if you add a break in the if clause.

> +		hrtimer_cancel(&stm32_ddr_pmu->hrtimer);
> +
> +	clk_disable(stm32_ddr_pmu->clk);
> +}
> +
> +static int stm32_ddr_pmu_event_init(struct perf_event *event)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +	struct hw_perf_event *hw = &event->hw;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (is_sampling_event(event))
> +		return -EINVAL;
> +
> +	if (event->attach_state & PERF_ATTACH_TASK)
> +		return -EINVAL;
> +
> +	if (event->attr.exclude_user   ||
> +	    event->attr.exclude_kernel ||
> +	    event->attr.exclude_hv     ||
> +	    event->attr.exclude_idle   ||
> +	    event->attr.exclude_host   ||
> +	    event->attr.exclude_guest)
> +		return -EINVAL;
> +
> +	if (event->cpu < 0)
> +		return -EINVAL;
> +
> +	hw->config_base = event->attr.config;
> +	event->cpu = cpumask_first(&stm32_ddr_pmu->pmu_cpu);
> +
> +	return 0;
> +}
> +
> +static enum hrtimer_restart stm32_ddr_pmu_poll(struct hrtimer *hrtimer)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = hrtimer_to_stm32_ddr_pmu(hrtimer);
> +	int i;
> +
> +	for (i = 0; i < PMU_NR_COUNTERS; i++)
> +		if (stm32_ddr_pmu->events[i])
> +			stm32_ddr_pmu_event_read(stm32_ddr_pmu->events[i]);
> +
> +	hrtimer_forward_now(hrtimer, stm32_ddr_pmu->poll_period);
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static ssize_t stm32_ddr_pmu_sysfs_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct dev_ext_attribute *eattr;
> +
> +	eattr = container_of(attr, struct dev_ext_attribute, attr);
> +
> +	return sprintf(buf, "config=0x%lx\n", (unsigned long)eattr->var);
> +}

Will you ever want to use other bits in the config to configure the PMU?
If so, perhaps its worth carving out a smaller event field, a bit like
fsl_imx8_ddr_perf.c does.

> +
> +#define STM32_DDR_PMU_ATTR(_name, _func, _config)			\
> +	(&((struct dev_ext_attribute[]) {				\
> +		{ __ATTR(_name, 0444, _func, NULL), (void *)_config }   \
> +	})[0].attr.attr)
> +
> +#define STM32_DDR_PMU_EVENT_ATTR(_name, _config)		\
> +	STM32_DDR_PMU_ATTR(_name, stm32_ddr_pmu_sysfs_show,	\
> +			   (unsigned long)_config)
> +
> +static struct attribute *stm32_ddr_pmu_event_attrs[] = {
> +	STM32_DDR_PMU_EVENT_ATTR(read_cnt, READ_CNT),
> +	STM32_DDR_PMU_EVENT_ATTR(write_cnt, WRITE_CNT),
> +	STM32_DDR_PMU_EVENT_ATTR(activate_cnt, ACTIVATE_CNT),
> +	STM32_DDR_PMU_EVENT_ATTR(idle_cnt, IDLE_CNT),
> +	STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT),
> +	NULL
> +};
> +
> +static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
> +	.name = "events",
> +	.attrs = stm32_ddr_pmu_event_attrs,
> +};
> +
> +static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
> +	&stm32_ddr_pmu_event_attrs_group,
> +	NULL,
> +};
> +
> +static int stm32_ddr_pmu_device_probe(struct platform_device *pdev)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu;
> +	struct reset_control *rst;
> +	struct resource *res;
> +	int i, ret;
> +	u32 val;
> +
> +	stm32_ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(struct stm32_ddr_pmu),
> +				     GFP_KERNEL);
> +	if (!stm32_ddr_pmu)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, stm32_ddr_pmu);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	stm32_ddr_pmu->membase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(stm32_ddr_pmu->membase)) {
> +		pr_warn("Unable to get STM32 DDR PMU membase\n");
> +		return PTR_ERR(stm32_ddr_pmu->membase);
> +	}
> +
> +	stm32_ddr_pmu->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(stm32_ddr_pmu->clk)) {
> +		pr_warn("Unable to get STM32 DDR PMU clock\n");
> +		return PTR_ERR(stm32_ddr_pmu->clk);
> +	}
> +
> +	ret = clk_prepare_enable(stm32_ddr_pmu->clk);
> +	if (ret) {
> +		pr_warn("Unable to prepare STM32 DDR PMU clock\n");
> +		return ret;
> +	}
> +
> +	stm32_ddr_pmu->poll_period = ms_to_ktime(POLL_MS);
> +	hrtimer_init(&stm32_ddr_pmu->hrtimer, CLOCK_MONOTONIC,
> +		     HRTIMER_MODE_REL);

I would /much/ prefer for the timer to be handled by the perf core
automatically when a PMU is registered with PERF_PMU_CAP_NO_INTERRUPT. That
way, other drivers can benefit from this without tonnes of code duplication.

> +	stm32_ddr_pmu->hrtimer.function = stm32_ddr_pmu_poll;
> +
> +	/*
> +	 * The PMU is assigned to the cpu0 and there is no need to manage cpu
> +	 * hot plug migration because cpu0 is always the first/last active cpu
> +	 * during low power transitions.
> +	 */
> +	cpumask_set_cpu(0, &stm32_ddr_pmu->pmu_cpu);
> +
> +	for (i = 0; i < PMU_NR_COUNTERS; i++) {
> +		stm32_ddr_pmu->events[i] = NULL;
> +		stm32_ddr_pmu->events_cnt[i] = 0;
> +	}
> +
> +	val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_SID);
> +	if (val != SID_MAGIC_ID)
> +		return -EINVAL;
> +
> +	stm32_ddr_pmu->pmu = (struct pmu) {
> +		.task_ctx_nr = perf_invalid_context,
> +		.start = stm32_ddr_pmu_event_start,
> +		.stop = stm32_ddr_pmu_event_stop,
> +		.add = stm32_ddr_pmu_event_add,
> +		.del = stm32_ddr_pmu_event_del,
> +		.event_init = stm32_ddr_pmu_event_init,
> +		.attr_groups = stm32_ddr_pmu_attr_groups,
> +	};
> +	ret = perf_pmu_register(&stm32_ddr_pmu->pmu, "stm32_ddr_pmu", -1);

You might want an index on the end of this name in case you ever want to
support more than one in a given SoC.

> +	if (ret) {
> +		pr_warn("Unable to register STM32 DDR PMU\n");
> +		return ret;
> +	}
> +
> +	rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (!IS_ERR(rst)) {
> +		reset_control_assert(rst);
> +		udelay(2);
> +		reset_control_deassert(rst);
> +	}
> +
> +	pr_info("stm32-ddr-pmu: probed (DDRPERFM ID=0x%08x VER=0x%08x)\n",
> +		readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_ID),
> +		readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_VER));

dev_info(). Similarly for many of your other pr_*() calls.

Will

RE: [PATCH v3 3/5] perf: stm32: ddrperfm driver creation

[Gerald BAEZA]

Hi Will

Thanks a lot for your detailed review of my v3 below and sorry for the delay of
my answer : those last months were rather focused on the pmu driver using
than its improvement.

I prepared a v4 on kernel 5.10, taking into account most of your remarks below 
but I still have some open points were I would like your feedback, that I will 
take into account... quickly !

Best regards

Gérald



> From: Will Deacon <will@kernel.org>
> To: Gerald BAEZA <gerald.baeza@st.com>
> Subject: Re: [PATCH v3 3/5] perf: stm32: ddrperfm driver creation
> 
> On Tue, Aug 27, 2019 at 03:08:20PM +0000, Gerald BAEZA wrote:
> > The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1
> SOC.
> >
> > This perf drivers supports the read, write, activate, idle and total
> > time counters, described in the reference manual RM0436 that is
> > accessible from Documentation/arm/stm32/stm32mp157-overview.rst
> >
> > Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> > ---
> >  drivers/perf/Kconfig         |   6 +
> >  drivers/perf/Makefile        |   1 +
> >  drivers/perf/stm32_ddr_pmu.c | 426
> > +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 433 insertions(+)
> >  create mode 100644 drivers/perf/stm32_ddr_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig index
> > 09ae8a9..a3d917e 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -114,6 +114,12 @@ config THUNDERX2_PMU
> >  	   The SoC has PMU support in its L3 cache controller (L3C) and
> >  	   in the DDR4 Memory Controller (DMC).
> >
> > +config STM32_DDR_PMU
> > +       tristate "STM32 DDR PMU"
> > +       depends on MACH_STM32MP157
> > +       help
> > +         Support for STM32 DDR performance monitor (DDRPERFM).
> 
> Weird indentation here (spaces not tabes?).

Done

> 
> >  config XGENE_PMU
> >          depends on ARCH_XGENE
> >          bool "APM X-Gene SoC PMU"
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile index
> > 2ebb4de..fd3368c 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_FSL_IMX8_DDR_PMU) +=
> fsl_imx8_ddr_perf.o
> >  obj-$(CONFIG_HISI_PMU) += hisilicon/
> >  obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
> >  obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
> > +obj-$(CONFIG_STM32_DDR_PMU) += stm32_ddr_pmu.o
> >  obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
> >  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> >  obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o diff --git
> > a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c new
> file
> > mode 100644 index 0000000..d0480e0
> > --- /dev/null
> > +++ b/drivers/perf/stm32_ddr_pmu.c
> > @@ -0,0 +1,426 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file is the STM32 DDR performance monitor (DDRPERFM) driver
> > + *
> > + * Copyright (C) 2019, STMicroelectronics - All Rights Reserved
> > + * Author: Gerald Baeza <gerald.baeza@st.com>  */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +/*
> > + * The PMU is able to freeze all counters and generate an interrupt
> > +when there
> > + * is a counter overflow. But, relying on this means that we lose all
> > +the
> > + * events that occur between the freeze and the interrupt handler
> execution.
> > + * So we use a polling mechanism to avoid this lose of information.
> > + * The fastest counter can overflow in ~8s @533MHz (that is the
> > +maximum DDR
> > + * frequency supported on STM32MP157), so we poll in 4s intervals to
> > +ensure
> > + * we don't reach this limit.
> > + */
> > +#define POLL_MS		4000
> > +
> > +#define DDRPERFM_CTL	0x000
> > +#define DDRPERFM_CFG	0x004
> > +#define DDRPERFM_STATUS 0x008
> > +#define DDRPERFM_CCR	0x00C
> > +#define DDRPERFM_IER	0x010
> > +#define DDRPERFM_ISR	0x014
> > +#define DDRPERFM_ICR	0x018
> > +#define DDRPERFM_TCNT	0x020
> > +#define DDRPERFM_CNT(X)	(0x030 + 8 * (X))
> > +#define DDRPERFM_HWCFG	0x3F0
> > +#define DDRPERFM_VER	0x3F4
> > +#define DDRPERFM_ID	0x3F8
> > +#define DDRPERFM_SID	0x3FC
> > +
> > +#define CTL_START	0x00000001
> > +#define CTL_STOP	0x00000002
> > +#define CCR_CLEAR_ALL	0x8000000F
> > +#define SID_MAGIC_ID	0xA3C5DD01
> 
> What's this for? The check during probe looks weird.

Indeed, I removed it.

> 
> > +
> > +enum {
> > +	READ_CNT,
> > +	WRITE_CNT,
> > +	ACTIVATE_CNT,
> > +	IDLE_CNT,
> > +	TIME_CNT,
> > +	PMU_NR_COUNTERS
> > +};
> 
> I think these correspond directly to the values set by userspace in attr.config,
> so you probably want to clamp attr.config to be < PMU_NR_COUNTERS in
> stm32_ddr_pmu_event_init().
> 

Done

> > +struct stm32_ddr_pmu {
> > +	struct pmu pmu;
> > +	void __iomem *membase;
> > +	struct clk *clk;
> > +	struct hrtimer hrtimer;
> > +	cpumask_t pmu_cpu;
> > +	ktime_t poll_period;
> > +	struct perf_event *events[PMU_NR_COUNTERS];
> > +	u64 events_cnt[PMU_NR_COUNTERS];
> > +};
> > +
> > +static inline struct stm32_ddr_pmu *pmu_to_stm32_ddr_pmu(struct
> pmu
> > +*p) {
> > +	return container_of(p, struct stm32_ddr_pmu, pmu); }
> > +
> > +static inline struct stm32_ddr_pmu *hrtimer_to_stm32_ddr_pmu(struct
> > +hrtimer *h) {
> > +	return container_of(h, struct stm32_ddr_pmu, hrtimer); }
> > +
> > +static void stm32_ddr_pmu_event_configure(struct perf_event *event) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	unsigned long config_base = event->hw.config_base;
> > +	u32 val;
> > +
> > +	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase +
> DDRPERFM_CTL);
> > +
> > +	if (config_base < TIME_CNT) {
> > +		val = readl_relaxed(stm32_ddr_pmu->membase +
> DDRPERFM_CFG);
> > +		val |= (1 << config_base);
> > +		writel_relaxed(val, stm32_ddr_pmu->membase +
> DDRPERFM_CFG);
> > +	}
> > +}
> > +
> > +static void stm32_ddr_pmu_event_read(struct perf_event *event) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	unsigned long config_base = event->hw.config_base;
> > +	struct hw_perf_event *hw = &event->hw;
> > +	u64 prev_count, new_count, mask;
> > +	u32 val, offset, bit;
> > +
> > +	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase +
> DDRPERFM_CTL);
> > +
> > +	if (config_base == TIME_CNT) {
> > +		offset = DDRPERFM_TCNT;
> > +		bit = 1 << 31;
> > +	} else {
> > +		offset = DDRPERFM_CNT(config_base);
> > +		bit = 1 << config_base;
> > +	}
> > +	val = readl_relaxed(stm32_ddr_pmu->membase +
> DDRPERFM_STATUS);
> > +	if (val & bit)
> > +		pr_warn("STM32 DDR PMU hardware counter overflow\n");
> 
> I don't think this print is useful. Surely overflow is fatal and you should do
> something like put the event into an error state?

The polling period is adjusted with 100% margin so we never saw this overflow
while using the driver during the past four years and we won't see it, as far
as the driver is not modified.

So I would propose to keep the overflow detection just in case a future change 
leads to get it but I do not see how to do this (with regards to the user land 
notification) : any example I could have a look on ?

> 
> > +	val = readl_relaxed(stm32_ddr_pmu->membase + offset);
> > +	writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +	writel_relaxed(CTL_START, stm32_ddr_pmu->membase +
> DDRPERFM_CTL);
> > +
> > +	do {
> > +		prev_count = local64_read(&hw->prev_count);
> > +		new_count = prev_count + val;
> > +	} while (local64_xchg(&hw->prev_count, new_count) !=
> prev_count);
> > +
> > +	mask = GENMASK_ULL(31, 0);
> > +	local64_add(val & mask, &event->count);
> > +
> > +	if (new_count < prev_count)
> > +		pr_warn("STM32 DDR PMU software counter rollover\n");
> 
> These are 64-bit. How fast do you expect the counters to tick?

The total time count is increment at the frequency of the DDR, 
533 MHz on STM32MP15, so the rollover will happen in around 1000
years... so I am sad to discover that I will never see this warning :(
I removed it, thanks for pointing this out !

> 
> > +static void stm32_ddr_pmu_event_start(struct perf_event *event, int
> > +flags) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hw = &event->hw;
> > +
> > +	if (WARN_ON_ONCE(!(hw->state & PERF_HES_STOPPED)))
> > +		return;
> > +
> > +	if (flags & PERF_EF_RELOAD)
> > +		WARN_ON_ONCE(!(hw->state & PERF_HES_UPTODATE));
> > +
> > +	stm32_ddr_pmu_event_configure(event);
> > +
> > +	/* Clear all counters to synchronize them, then start */
> > +	writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase +
> DDRPERFM_CCR);
> > +	writel_relaxed(CTL_START, stm32_ddr_pmu->membase +
> DDRPERFM_CTL);
> > +	local64_set(&hw->prev_count, 0);
> > +	hw->state = 0;
> > +}
> > +
> > +static void stm32_ddr_pmu_event_stop(struct perf_event *event, int
> > +flags) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	unsigned long config_base = event->hw.config_base;
> > +	struct hw_perf_event *hw = &event->hw;
> > +	u32 val, bit;
> > +
> > +	if (WARN_ON_ONCE(hw->state & PERF_HES_STOPPED))
> > +		return;
> > +
> > +	writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase +
> DDRPERFM_CTL);
> > +	if (config_base == TIME_CNT)
> > +		bit = 1 << 31;
> > +	else
> > +		bit = 1 << config_base;
> > +	writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +	if (config_base < TIME_CNT) {
> > +		val = readl_relaxed(stm32_ddr_pmu->membase +
> DDRPERFM_CFG);
> > +		val &= ~bit;
> > +		writel_relaxed(val, stm32_ddr_pmu->membase +
> DDRPERFM_CFG);
> > +	}
> > +
> > +	hw->state |= PERF_HES_STOPPED;
> > +
> > +	if (flags & PERF_EF_UPDATE) {
> > +		stm32_ddr_pmu_event_read(event);
> > +		hw->state |= PERF_HES_UPTODATE;
> > +	}
> > +}
> > +
> > +static int stm32_ddr_pmu_event_add(struct perf_event *event, int
> > +flags) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	unsigned long config_base = event->hw.config_base;
> > +	struct hw_perf_event *hw = &event->hw;
> > +
> > +	stm32_ddr_pmu->events_cnt[config_base] = 0;
> > +	stm32_ddr_pmu->events[config_base] = event;
> > +
> > +	clk_enable(stm32_ddr_pmu->clk);
> > +	/*
> > +	 * Pin the timer, so that the overflows are handled by the chosen
> > +	 * event->cpu (this is the same one as presented in "cpumask"
> > +	 * attribute).
> > +	 */
> > +	hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu-
> >poll_period,
> > +		      HRTIMER_MODE_REL_PINNED);
> > +
> > +	stm32_ddr_pmu_event_configure(event);
> > +
> > +	hw->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > +
> > +	if (flags & PERF_EF_START)
> > +		stm32_ddr_pmu_event_start(event, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static void stm32_ddr_pmu_event_del(struct perf_event *event, int
> > +flags) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	unsigned long config_base = event->hw.config_base;
> > +	bool stop = true;
> > +	int i;
> > +
> > +	stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
> > +
> > +	stm32_ddr_pmu->events_cnt[config_base] +=
> local64_read(&event->count);
> > +	stm32_ddr_pmu->events[config_base] = NULL;
> > +
> > +	for (i = 0; i < PMU_NR_COUNTERS; i++)
> > +		if (stm32_ddr_pmu->events[i])
> > +			stop = false;
> > +	if (stop)
> 
> This is just i == PMU_NR_COUNTERS if you add a break in the if clause.

Done

> 
> > +		hrtimer_cancel(&stm32_ddr_pmu->hrtimer);
> > +
> > +	clk_disable(stm32_ddr_pmu->clk);
> > +}
> > +
> > +static int stm32_ddr_pmu_event_init(struct perf_event *event) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> pmu_to_stm32_ddr_pmu(event->pmu);
> > +	struct hw_perf_event *hw = &event->hw;
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (is_sampling_event(event))
> > +		return -EINVAL;
> > +
> > +	if (event->attach_state & PERF_ATTACH_TASK)
> > +		return -EINVAL;
> > +
> > +	if (event->attr.exclude_user   ||
> > +	    event->attr.exclude_kernel ||
> > +	    event->attr.exclude_hv     ||
> > +	    event->attr.exclude_idle   ||
> > +	    event->attr.exclude_host   ||
> > +	    event->attr.exclude_guest)
> > +		return -EINVAL;
> > +
> > +	if (event->cpu < 0)
> > +		return -EINVAL;
> > +
> > +	hw->config_base = event->attr.config;
> > +	event->cpu = cpumask_first(&stm32_ddr_pmu->pmu_cpu);
> > +
> > +	return 0;
> > +}
> > +
> > +static enum hrtimer_restart stm32_ddr_pmu_poll(struct hrtimer
> > +*hrtimer) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu =
> hrtimer_to_stm32_ddr_pmu(hrtimer);
> > +	int i;
> > +
> > +	for (i = 0; i < PMU_NR_COUNTERS; i++)
> > +		if (stm32_ddr_pmu->events[i])
> > +			stm32_ddr_pmu_event_read(stm32_ddr_pmu-
> >events[i]);
> > +
> > +	hrtimer_forward_now(hrtimer, stm32_ddr_pmu->poll_period);
> > +
> > +	return HRTIMER_RESTART;
> > +}
> > +
> > +static ssize_t stm32_ddr_pmu_sysfs_show(struct device *dev,
> > +					struct device_attribute *attr,
> > +					char *buf)
> > +{
> > +	struct dev_ext_attribute *eattr;
> > +
> > +	eattr = container_of(attr, struct dev_ext_attribute, attr);
> > +
> > +	return sprintf(buf, "config=0x%lx\n", (unsigned long)eattr->var); }
> 
> Will you ever want to use other bits in the config to configure the PMU?
> If so, perhaps its worth carving out a smaller event field, a bit like
> fsl_imx8_ddr_perf.c does.
> 

I do not see any application for this, now or later on.

> > +
> > +#define STM32_DDR_PMU_ATTR(_name, _func, _config)
> 	\
> > +	(&((struct dev_ext_attribute[]) {				\
> > +		{ __ATTR(_name, 0444, _func, NULL), (void *)_config }   \
> > +	})[0].attr.attr)
> > +
> > +#define STM32_DDR_PMU_EVENT_ATTR(_name, _config)		\
> > +	STM32_DDR_PMU_ATTR(_name, stm32_ddr_pmu_sysfs_show,
> 	\
> > +			   (unsigned long)_config)
> > +
> > +static struct attribute *stm32_ddr_pmu_event_attrs[] = {
> > +	STM32_DDR_PMU_EVENT_ATTR(read_cnt, READ_CNT),
> > +	STM32_DDR_PMU_EVENT_ATTR(write_cnt, WRITE_CNT),
> > +	STM32_DDR_PMU_EVENT_ATTR(activate_cnt, ACTIVATE_CNT),
> > +	STM32_DDR_PMU_EVENT_ATTR(idle_cnt, IDLE_CNT),
> > +	STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT),
> > +	NULL
> > +};
> > +
> > +static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
> > +	.name = "events",
> > +	.attrs = stm32_ddr_pmu_event_attrs,
> > +};
> > +
> > +static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
> > +	&stm32_ddr_pmu_event_attrs_group,
> > +	NULL,
> > +};
> > +
> > +static int stm32_ddr_pmu_device_probe(struct platform_device *pdev) {
> > +	struct stm32_ddr_pmu *stm32_ddr_pmu;
> > +	struct reset_control *rst;
> > +	struct resource *res;
> > +	int i, ret;
> > +	u32 val;
> > +
> > +	stm32_ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(struct
> stm32_ddr_pmu),
> > +				     GFP_KERNEL);
> > +	if (!stm32_ddr_pmu)
> > +		return -ENOMEM;
> > +	platform_set_drvdata(pdev, stm32_ddr_pmu);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	stm32_ddr_pmu->membase = devm_ioremap_resource(&pdev-
> >dev, res);
> > +	if (IS_ERR(stm32_ddr_pmu->membase)) {
> > +		pr_warn("Unable to get STM32 DDR PMU membase\n");
> > +		return PTR_ERR(stm32_ddr_pmu->membase);
> > +	}
> > +
> > +	stm32_ddr_pmu->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(stm32_ddr_pmu->clk)) {
> > +		pr_warn("Unable to get STM32 DDR PMU clock\n");
> > +		return PTR_ERR(stm32_ddr_pmu->clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(stm32_ddr_pmu->clk);
> > +	if (ret) {
> > +		pr_warn("Unable to prepare STM32 DDR PMU clock\n");
> > +		return ret;
> > +	}
> > +
> > +	stm32_ddr_pmu->poll_period = ms_to_ktime(POLL_MS);
> > +	hrtimer_init(&stm32_ddr_pmu->hrtimer, CLOCK_MONOTONIC,
> > +		     HRTIMER_MODE_REL);
> 
> I would /much/ prefer for the timer to be handled by the perf core
> automatically when a PMU is registered with
> PERF_PMU_CAP_NO_INTERRUPT. That way, other drivers can benefit from
> this without tonnes of code duplication.

Do you mean that the perf core offers such a possibility or do you suggest
to add it ?
Because, in kernel 5.10, I can still see such code duplication in several drivers, 
indeed  :(

> > +	stm32_ddr_pmu->hrtimer.function = stm32_ddr_pmu_poll;
> > +
> > +	/*
> > +	 * The PMU is assigned to the cpu0 and there is no need to manage
> cpu
> > +	 * hot plug migration because cpu0 is always the first/last active cpu
> > +	 * during low power transitions.
> > +	 */
> > +	cpumask_set_cpu(0, &stm32_ddr_pmu->pmu_cpu);
> > +
> > +	for (i = 0; i < PMU_NR_COUNTERS; i++) {
> > +		stm32_ddr_pmu->events[i] = NULL;
> > +		stm32_ddr_pmu->events_cnt[i] = 0;
> > +	}
> > +
> > +	val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_SID);
> > +	if (val != SID_MAGIC_ID)
> > +		return -EINVAL;
> > +
> > +	stm32_ddr_pmu->pmu = (struct pmu) {
> > +		.task_ctx_nr = perf_invalid_context,
> > +		.start = stm32_ddr_pmu_event_start,
> > +		.stop = stm32_ddr_pmu_event_stop,
> > +		.add = stm32_ddr_pmu_event_add,
> > +		.del = stm32_ddr_pmu_event_del,
> > +		.event_init = stm32_ddr_pmu_event_init,
> > +		.attr_groups = stm32_ddr_pmu_attr_groups,
> > +	};
> > +	ret = perf_pmu_register(&stm32_ddr_pmu->pmu,
> "stm32_ddr_pmu", -1);
> 
> You might want an index on the end of this name in case you ever want to
> support more than one in a given SoC.

There is no plan to embed multiple instances of this PMU in a SoC.

> 
> > +	if (ret) {
> > +		pr_warn("Unable to register STM32 DDR PMU\n");
> > +		return ret;
> > +	}
> > +
> > +	rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > +	if (!IS_ERR(rst)) {
> > +		reset_control_assert(rst);
> > +		udelay(2);
> > +		reset_control_deassert(rst);
> > +	}
> > +
> > +	pr_info("stm32-ddr-pmu: probed (DDRPERFM ID=0x%08x
> VER=0x%08x)\n",
> > +		readl_relaxed(stm32_ddr_pmu->membase +
> DDRPERFM_ID),
> > +		readl_relaxed(stm32_ddr_pmu->membase +
> DDRPERFM_VER));
> 
> dev_info(). Similarly for many of your other pr_*() calls.

Done (for all occurrences)

> 
> Will