Message-ID: <20190626122524.GA20635@lakrids.cambridge.arm.com>
Date:   Wed, 26 Jun 2019 13:25:24 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Gerald BAEZA <gerald.baeza@...com>
Cc:     Will Deacon <Will.Deacon@....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>,
        "horms+renesas@...ge.net.au" <horms+renesas@...ge.net.au>,
        "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 v2 3/5] perf: stm32: ddrperfm driver creation

Hi,

Sorry for the (quite horrific) mangling my mailserver has inflicted via
my reply. Obviously, the confidentiality disclaimer is bogus, too.

I'll make sure I use the right SMTP server in future.

Mark.

On Wed, Jun 26, 2019 at 12:22:31PM +0000, Mark Rutland wrote:
> On Mon, May 20, 2019 at 03:27:17PM +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.
> 
> Is this document publicly accessible anywhere?
> 
> If so, could you please provide a link?
> 
> > A 'bandwidth' attribute is added in the 'ddrperfm' event_source in order
> > to directly get the read and write bandwidths (in MB/s), from the last
> > read, write and total time counters reading.
> > This attribute is aside the 'events' attributes group because it is not
> > a counter, as seen by perf tool.
> 
> This should be removed. This is unusually stateful, and this can be
> calculated in userspace by using the events. I'm also not keen on
> creating a precedent for weird sysfs bits for PMUs.
> 
> > Signed-off-by: Gerald Baeza <gerald.baeza@...com>
> > ---
> >  drivers/perf/Kconfig         |   6 +
> >  drivers/perf/Makefile        |   1 +
> >  drivers/perf/stm32_ddr_pmu.c | 512 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 519 insertions(+)
> >  create mode 100644 drivers/perf/stm32_ddr_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index a94e586..9add8a7 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -105,6 +105,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 3048994..fa64719 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARM_SMMU_V3_PMU) += arm_smmuv3_pmu.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..ae4a813
> > --- /dev/null
> > +++ b/drivers/perf/stm32_ddr_pmu.c
> > @@ -0,0 +1,512 @@
> > +// 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>
> > +
> > +#define POLL_MS4000/* The counter roll over after ~8s @533MHz */
> 
> I take it this is because there's no IRQ? If so, please expand the
> comment to call that out, e.g.
> 
> /*
>  * The PMU has no overflow IRQ, so we must poll to avoid losing events.
>  * The fastest counter can overflow in ~8s @533MHz, so we poll in 4s
>  * intervals to ensure we don't miss a rollover.
>  */
> #define POLL_MS4000
> 
> Which clock specifically is operating at 533MHz, and is this something
> that may vary? Is it possible that this may go higher in future?
> 
> > +#define WORD_LENGTH4/* Bytes */
> > +#define BURST_LENGTH8/* Words */
> > +
> > +#define DDRPERFM_CTL0x000
> > +#define DDRPERFM_CFG0x004
> > +#define DDRPERFM_STATUS 0x008
> > +#define DDRPERFM_CCR0x00C
> > +#define DDRPERFM_IER0x010
> > +#define DDRPERFM_ISR0x014
> > +#define DDRPERFM_ICR0x018
> > +#define DDRPERFM_TCNT0x020
> > +#define DDRPERFM_CNT(X)(0x030 + 8 * (X))
> > +#define DDRPERFM_HWCFG0x3F0
> > +#define DDRPERFM_VER0x3F4
> > +#define DDRPERFM_ID0x3F8
> > +#define DDRPERFM_SID0x3FC
> > +
> > +#define CTL_START0x00000001
> > +#define CTL_STOP0x00000002
> > +#define CCR_CLEAR_ALL0x8000000F
> > +#define SID_MAGIC_ID0xA3C5DD01
> > +
> > +#define STRING "Read = %llu, Write = %llu, Read & Write = %llu (MB/s)\n"
> 
> If you need this, please expand it in-place. As-is, this is needless
> obfuscation.
> 
> > +
> > +enum {
> > +READ_CNT,
> > +WRITE_CNT,
> > +ACTIVATE_CNT,
> > +IDLE_CNT,
> > +TIME_CNT,
> > +PMU_NR_COUNTERS
> > +};
> 
> I take it these are fixed-purpose counters in the hardware?
> 
> > +
> > +struct stm32_ddr_pmu {
> > +struct pmu pmu;
> > +void __iomem *membase;
> > +struct clk *clk;
> > +struct clk *clk_ddr;
> > +unsigned long clk_ddr_rate;
> > +struct hrtimer hrtimer;
> > +ktime_t poll_period;
> > +spinlock_t lock; /* for shared registers access */
> 
> All accesses to a PMU instance should be serialized on the same CPU,
> so you shouldn't need a lock (though you will need to disable IRQs to
> serialize with the interrupt handler).
> 
> The perf subsystem cannot sanely access the PMU across multiple CPUs.
> 
> > +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 u64 stm32_ddr_pmu_compute_bw(struct stm32_ddr_pmu *stm32_ddr_pmu,
> > +    int counter)
> > +{
> > +u64 bw = stm32_ddr_pmu->events_cnt[counter], tmp;
> > +u64 div = stm32_ddr_pmu->events_cnt[TIME_CNT];
> > +u32 prediv = 1, premul = 1;
> > +
> > +if (bw && div) {
> > +/* Maximize the dividend into 64 bits */
> > +while ((bw < 0x8000000000000000ULL) &&
> > +       (premul < 0x40000000UL)) {
> > +bw = bw << 1;
> > +premul *= 2;
> > +}
> > +/* Minimize the dividor to fit in 32 bits */
> > +while ((div > 0xffffffffUL) && (prediv < 0x40000000UL)) {
> > +div = div >> 1;
> > +prediv *= 2;
> > +}
> > +/* Divide counter per time and multiply per DDR settings */
> > +do_div(bw, div);
> > +tmp = bw * BURST_LENGTH * WORD_LENGTH;
> > +tmp *= stm32_ddr_pmu->clk_ddr_rate;
> > +if (tmp < bw)
> > +goto error;
> > +bw = tmp;
> > +/* Cancel the prediv and premul factors */
> > +while (prediv > 1) {
> > +bw = bw >> 1;
> > +prediv /= 2;
> > +}
> > +while (premul > 1) {
> > +bw = bw >> 1;
> > +premul /= 2;
> > +}
> > +/* Convert MHz to Hz and B to MB, to finally get MB/s */
> > +tmp = bw * 1000000;
> > +if (tmp < bw)
> > +goto error;
> > +bw = tmp;
> > +premul = 1024 * 1024;
> > +while (premul > 1) {
> > +bw = bw >> 1;
> > +premul /= 2;
> > +}
> > +}
> > +return bw;
> > +
> > +error:
> > +pr_warn("stm32-ddr-pmu: overflow detected\n");
> > +return 0;
> > +}
> 
> IIUC this is for the stateful sysfs stuff, which should be removed.
> 
> > +
> > +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 lock_flags, config_base = event->hw.config_base;
> > +u32 val;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +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);
> > +}
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> > +}
> 
> You don't need the lock here. This is called from your pmu::{start,add}
> callbacks, and the perf core already ensures those are serialized (via
> the context lock), and called with IRQs disabled.
> 
> > +
> > +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 flags, config_base = event->hw.config_base;
> > +struct hw_perf_event *hw = &event->hw;
> > +u64 prev_count, new_count, mask;
> > +u32 val, offset, bit;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, flags);
> > +
> > +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 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 counter saturated\n");
> 
> Do the counter saturate, or do they roll-over?
> 
> I think that the message is misleading here, but I just want to check.
> 
> > +
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, flags);
> > +}
> > +
> > +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;
> > +unsigned long lock_flags;
> > +
> > +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 */
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> 
> By 'clear' do you mean set the counters to zero?
> 
> Or is there a control bit that determine whether they count?
> 
> If we're updating the counter, we could update prev_count at the same
> instant.
> 
> > +
> > +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 lock_flags, 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;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +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);
> > +}
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> > +
> > +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);
> > +hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu->poll_period,
> > +      HRTIMER_MODE_REL);
> > +
> > +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);
> 
> This doesn't look right. If I add two events A and B, then delete event
> A, surely we want the clock on for event B?
> 
> Does the clock only affect whether the counters count, or does it also
> affect whether the register file is usable?
> 
> > +}
> > +
> > +static int stm32_ddr_pmu_event_init(struct perf_event *event)
> > +{
> > +struct hw_perf_event *hw = &event->hw;
> > +
> > +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;
> 
> For a system PMU like this, you must ensure that all events are assigned
> to the same CPU.
> 
> You'll need to designate some arbitrary CPU to mange the PMU, expose
> that under sysfs, and upon hotplug events you must choose a new CPU and
> migrate existing events.
> 
> For a simple example, see arch/arm/mm/cache-l2x0-pmu.c.
> 
> > +
> > +hw->config_base = event->attr.config;
> > +
> > +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);
> > +}
> > +
> > +static ssize_t bandwidth_show(struct device *dev,
> > +      struct device_attribute *attr,
> > +      char *buf)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = dev_get_drvdata(dev);
> > +u64 r_bw, w_bw;
> > +int ret;
> > +
> > +if (stm32_ddr_pmu->events_cnt[TIME_CNT]) {
> > +r_bw = stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, READ_CNT);
> > +w_bw = stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, WRITE_CNT);
> > +
> > +ret = snprintf(buf, PAGE_SIZE, STRING,
> > +       r_bw, w_bw, (r_bw + w_bw));
> > +} else {
> > +ret = snprintf(buf, PAGE_SIZE, "No data available\n");
> > +}
> > +
> > +return ret;
> > +}
> 
> As commented above, this should not be exposed under sysfs. It doesn't
> follow the usual ABI rules, it's weirdly stateful, and it's redundant.
> 
> > +
> > +#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 DEVICE_ATTR_RO(bandwidth);
> > +static struct attribute *stm32_ddr_pmu_bandwidth_attrs[] = {
> > +&dev_attr_bandwidth.attr,
> > +NULL,
> > +};
> > +
> > +static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
> > +.name = "events",
> > +.attrs = stm32_ddr_pmu_event_attrs,
> > +};
> > +
> > +static struct attribute_group stm32_ddr_pmu_bandwidth_attrs_group = {
> > +.attrs = stm32_ddr_pmu_bandwidth_attrs,
> > +};
> > +
> > +static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
> > +&stm32_ddr_pmu_event_attrs_group,
> > +&stm32_ddr_pmu_bandwidth_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, "bus");
> > +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->clk_ddr = devm_clk_get(&pdev->dev, "ddr");
> > +if (IS_ERR(stm32_ddr_pmu->clk_ddr)) {
> > +pr_warn("Unable to get STM32 DDR clock\n");
> > +return PTR_ERR(stm32_ddr_pmu->clk_ddr);
> > +}
> > +stm32_ddr_pmu->clk_ddr_rate = clk_get_rate(stm32_ddr_pmu->clk_ddr);
> > +stm32_ddr_pmu->clk_ddr_rate /= 1000000;
> > +
> > +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;
> > +spin_lock_init(&stm32_ddr_pmu->lock);
> > +
> > +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, "ddrperfm", -1);
> 
> Please give this a better name. The usual convention is to use "_pmu" as
> the suffix, so we should use that rather than "perfm".
> 
> To be unambiguous, something like "stm32_ddr_pmu" would be good.
> 
> Thanks,
> Mark.
> 
> > +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 (ID=0x%08x VER=0x%08x), DDR@...MHz\n",
> > +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_ID),
> > +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_VER),
> > +stm32_ddr_pmu->clk_ddr_rate);
> > +
> > +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@...com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Powered by blists - more mailing lists