[PATCH] perf/imx_ddr: Add stop event counters support for i.MX8MP
Will Deacon
will at kernel.org
Mon Sep 7 13:06:52 EDT 2020
On Mon, Sep 07, 2020 at 05:53:59PM +0800, Joakim Zhang wrote:
> DDR Perf driver only supports free-running event counters(counter1/2/3)
> now, this patch adds support for stop event counters.
>
> Legacy SoCs:
> Cycle counter(counter0) is a special counter, only count cycles. When
> cycle counter overflow, it will lock all counters and generate an
> interrupt. In ddr_perf_irq_handler, disable cycle counter then all
> counters would stop at the same time, update all counters' count, then
> enable cycle counter that all counters count again. During this process,
> only clear cycle counter, no need to clear event counters since they are
> free-running counters. They would continue counting after overflow and
> do/while loop from ddr_perf_event_update can handle event counters
> overflow case.
>
> i.MX8MP:
> Almost all is the same as legacy SoCs, the only difference is that, event
> counters are not free-running any more. Like cycle counter, when event
> counters overflow, they would stop counting unless clear the counter,
> and no interrupt generate for event counters. So we should clear event
> counters that let them re-count when cycle counter overflow, which ensure
> event counters will not lose data.
Was this supposed to be an improvement over the "Legacy SoCs"
implementation? It seems even worse...
Do you _have_ to write zeroes back to the event counters to get them going
again, or will any value do?
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index 90884d14f95f..057e361eb391 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -14,6 +14,7 @@
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> #include <linux/perf_event.h>
> +#include <linux/spinlock.h>
> #include <linux/slab.h>
>
> #define COUNTER_CNTL 0x0
> @@ -82,6 +83,7 @@ struct ddr_pmu {
> const struct fsl_ddr_devtype_data *devtype_data;
> int irq;
> int id;
> + spinlock_t lock;
> };
>
> enum ddr_perf_filter_capabilities {
> @@ -368,16 +370,19 @@ static void ddr_perf_event_update(struct perf_event *event)
> struct hw_perf_event *hwc = &event->hw;
> u64 delta, prev_raw_count, new_raw_count;
> int counter = hwc->idx;
> + unsigned long flags;
>
> - do {
> - prev_raw_count = local64_read(&hwc->prev_count);
> - new_raw_count = ddr_perf_read_counter(pmu, counter);
> - } while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> - new_raw_count) != prev_raw_count);
> + spin_lock_irqsave(&pmu->lock, flags);
> +
> + prev_raw_count = local64_read(&hwc->prev_count);
> + new_raw_count = ddr_perf_read_counter(pmu, counter);
>
> delta = (new_raw_count - prev_raw_count) & 0xFFFFFFFF;
>
> local64_add(delta, &event->count);
> + local64_set(&hwc->prev_count, new_raw_count);
Hmm, assuming that the event counters never overflow, why do we care about
the prev count at all? In other words, why don't we just add the counter
value to event->count and reset the hardware to zero every time?
Will
More information about the linux-arm-kernel
mailing list