[PATCH] perf/imx_ddr: Add stop event counters support for i.MX8MP

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will at kernel.org>
> Sent: 2020年9月8日 1:07
> To: Joakim Zhang <qiangqing.zhang at nxp.com>
> Cc: mark.rutland at arm.com; robin.murphy at arm.com; dl-linux-imx
> <linux-imx at nxp.com>; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] perf/imx_ddr: Add stop event counters support for
> i.MX8MP
> 
> 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...
Per IC guys' perspective, they think this is an improvement. Event counters should also stop counting when they are overflow. So they fix it as a bug.
Per software perspective, we more hope event counters are free-running. However, IC guys has not informed us when they do this change.


> Do you _have_ to write zeroes back to the event counters to get them going
> again, or will any value do?
No, event counters also have a CLEAR bit, only need clear this CLEAR bit, then event counters start counting again from zero.


> > 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?
Do you mean that:
for cycle counter, keep the original routine,
for event counters, add counter value to event->count, and then clear event counters to let them counting from zero again?

Sounds great! I will have a try. Thanks.

Best Regards,
Joakim Zhang
> Will