Message-ID: <20191029143528.GB12800@willie-the-truck>
Date:   Tue, 29 Oct 2019 14:35:31 +0000
From:   Will Deacon <will@...nel.org>
To:     Gerald BAEZA <gerald.baeza@...com>
Cc:     "mark.rutland@....com" <mark.rutland@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        "corbet@....net" <corbet@....net>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "olof@...om.net" <olof@...om.net>, "arnd@...db.de" <arnd@...db.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
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@...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@...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

Powered by blists - more mailing lists