[PATCH 1/2] perf/imx_ddr: Correct the CLEAR bit definition

[PATCH 1/2] perf/imx_ddr: Correct the CLEAR bit definition

[Joakim Zhang]

ddr_perf_event_stop will firstly call ddr_perf_counter_enable to disable
the counter, and then call ddr_perf_event_update to read the counter value.

When disable the counter, it will write 0 into COUNTER_CNTL[CLEAR] bit
which cause the counter value cleared. Counter value will always be 0
when update the counter.

The correct definition of CLEAR bit is that write 0 to clear the counter
value.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/perf/fsl_imx8_ddr_perf.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
index 95dca2cb5265..90884d14f95f 100644
--- a/drivers/perf/fsl_imx8_ddr_perf.c
+++ b/drivers/perf/fsl_imx8_ddr_perf.c
@@ -388,9 +388,10 @@ static void ddr_perf_counter_enable(struct ddr_pmu *pmu, int config,
 
 	if (enable) {
 		/*
-		 * must disable first, then enable again
-		 * otherwise, cycle counter will not work
-		 * if previous state is enabled.
+		 * cycle counter is special which should firstly write 0 then
+		 * write 1 into CLEAR bit to clear it. Other counters only
+		 * need write 0 into CLEAR bit and it turns out to be 1 by
+		 * hardware. Below enable flow is harmless for all counters.
 		 */
 		writel(0, pmu->base + reg);
 		val = CNTL_EN | CNTL_CLEAR;
@@ -398,7 +399,8 @@ static void ddr_perf_counter_enable(struct ddr_pmu *pmu, int config,
 		writel(val, pmu->base + reg);
 	} else {
 		/* Disable counter */
-		writel(0, pmu->base + reg);
+		val = readl_relaxed(pmu->base + reg) & CNTL_EN_MASK;
+		writel(val, pmu->base + reg);
 	}
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP

[Joakim Zhang]

DDR perf driver now only supports free-running event counters
(counter1/2/3), which means that event counters will continue counting
even they are overflow.

However, the situation is changed on i.MX8MP, event counters are not
free-running any more. Event counters would stop counting if they are
overflow. So we need clear event counters when cycle counter overflow.

The patch adds stop counter support which would be compatible to
free-running counter.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/perf/fsl_imx8_ddr_perf.c | 37 ++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
index 90884d14f95f..5713f0631f79 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);
+
+	spin_unlock_irqrestore(&pmu->lock, flags);
 }
 
 static void ddr_perf_counter_enable(struct ddr_pmu *pmu, int config,
@@ -546,7 +551,7 @@ static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
 	/*
 	 * When the cycle counter overflows, all counters are stopped,
 	 * and an IRQ is raised. If any other counter overflows, it
-	 * continues counting, and no IRQ is raised.
+	 * stop counting, and no IRQ is raised.
 	 *
 	 * Cycles occur at least 4 times as often as other events, so we
 	 * can update all events on a cycle counter overflow and not
@@ -566,6 +571,25 @@ static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
 			cycle_event = event;
 	}
 
+	spin_lock(&pmu->lock);
+
+	for (i = 0; i < NUM_COUNTERS; i++) {
+		if (!pmu->events[i])
+			continue;
+
+		event = pmu->events[i];
+
+		if (event->hw.idx == EVENT_CYCLES_COUNTER)
+			continue;
+
+		/* clear non-cycle counters */
+		ddr_perf_counter_enable(pmu, event->attr.config, event->hw.idx, true);
+
+		local64_set(&event->hw.prev_count, 0);
+	}
+
+	spin_unlock(&pmu->lock);
+
 	ddr_perf_counter_enable(pmu,
 			      EVENT_CYCLES_ID,
 			      EVENT_CYCLES_COUNTER,
@@ -619,6 +643,7 @@ static int ddr_perf_probe(struct platform_device *pdev)
 	num = ddr_perf_init(pmu, base, &pdev->dev);
 
 	platform_set_drvdata(pdev, pmu);
+	spin_lock_init(&pmu->lock);
 
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, DDR_PERF_DEV_NAME "%d",
 			      num);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP

[Will Deacon]

On Tue, Feb 25, 2020 at 08:56:44PM +0800, Joakim Zhang wrote:
> DDR perf driver now only supports free-running event counters
> (counter1/2/3), which means that event counters will continue counting
> even they are overflow.
> 
> However, the situation is changed on i.MX8MP, event counters are not
> free-running any more. Event counters would stop counting if they are
> overflow. So we need clear event counters when cycle counter overflow.
> 
> The patch adds stop counter support which would be compatible to
> free-running counter.

Hmm, are you saying that the hardware behaviour has changed here, so that
newer SoCs force the behaviour where all the counters stop when one
overflows? Is there any way to control that behaviour?

> @@ -566,6 +571,25 @@ static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
>  			cycle_event = event;
>  	}
>  
> +	spin_lock(&pmu->lock);
> +
> +	for (i = 0; i < NUM_COUNTERS; i++) {
> +		if (!pmu->events[i])
> +			continue;
> +
> +		event = pmu->events[i];
> +
> +		if (event->hw.idx == EVENT_CYCLES_COUNTER)
> +			continue;
> +
> +		/* clear non-cycle counters */
> +		ddr_perf_counter_enable(pmu, event->attr.config, event->hw.idx, true);
> +
> +		local64_set(&event->hw.prev_count, 0);
> +	}
> +
> +	spin_unlock(&pmu->lock);

I'm suspicious of this locking, as it's /very/ fine-grained. What prevents
racing with a concurrent ddr_perf_counter_enable() call? Also, doesn't perf
core need to be aware that you're stopping counters here?

Finally, this looks like it's an unconditional change in user-visible
behaviour. Why doesn't it break existing usage of the perf tool?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] perf/imx_ddr: Correct the CLEAR bit definition

[Will Deacon]

On Tue, Feb 25, 2020 at 08:56:43PM +0800, Joakim Zhang wrote:
> ddr_perf_event_stop will firstly call ddr_perf_counter_enable to disable
> the counter, and then call ddr_perf_event_update to read the counter value.
> 
> When disable the counter, it will write 0 into COUNTER_CNTL[CLEAR] bit
> which cause the counter value cleared. Counter value will always be 0
> when update the counter.
> 
> The correct definition of CLEAR bit is that write 0 to clear the counter
> value.

Ok, so the issue is that when disabling the counter we clear the counter
value at the same time. I'll update the text and apply this fix.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: 2020年3月2日 19:25
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP
> 
> On Tue, Feb 25, 2020 at 08:56:44PM +0800, Joakim Zhang wrote:
> > DDR perf driver now only supports free-running event counters
> > (counter1/2/3), which means that event counters will continue counting
> > even they are overflow.
> >
> > However, the situation is changed on i.MX8MP, event counters are not
> > free-running any more. Event counters would stop counting if they are
> > overflow. So we need clear event counters when cycle counter overflow.
> >
> > The patch adds stop counter support which would be compatible to
> > free-running counter.
> 
> Hmm, are you saying that the hardware behaviour has changed here, so that
> newer SoCs force the behaviour where all the counters stop when one
> overflows? Is there any way to control that behaviour?
Hi Will,

Yes, the hardware behavior has changed in i.MX8MP.

Legacy SoCs, when counter0(cycle counter) overflows, it will lock itself and other counters, then generate an interrupt. But, when other counters(counter1/2/3) overflow, it will continue counting and not stop.
In i.MX8MP, all is the same as legacy SoCs, besides that when other counters(counter1/2/3) overflow, itself will stop counting, need clear counter value manually, then it will start counting again.
So, in counter0 overflow interrupt handler, we need clear counter1/2/3 value, since counter0 will always overflow before other counters, that can ensure counter1/2/3 doesn't lose data.

NOT all counters stop when one overflow. Counter0 overflow will lock itself and other counters, other counters overflow just stop themselves.

No way to control the behavior, it is the hardware behavior.

> > @@ -566,6 +571,25 @@ static irqreturn_t ddr_perf_irq_handler(int irq, void
> *p)
> >  			cycle_event = event;
> >  	}
> >
> > +	spin_lock(&pmu->lock);
> > +
> > +	for (i = 0; i < NUM_COUNTERS; i++) {
> > +		if (!pmu->events[i])
> > +			continue;
> > +
> > +		event = pmu->events[i];
> > +
> > +		if (event->hw.idx == EVENT_CYCLES_COUNTER)
> > +			continue;
> > +
> > +		/* clear non-cycle counters */
> > +		ddr_perf_counter_enable(pmu, event->attr.config, event->hw.idx,
> > +true);
> > +
> > +		local64_set(&event->hw.prev_count, 0);
> > +	}
> > +
> > +	spin_unlock(&pmu->lock);
> 
> I'm suspicious of this locking, as it's /very/ fine-grained. What prevents racing
> with a concurrent ddr_perf_counter_enable() call? Also, doesn't perf core need
> to be aware that you're stopping counters here?

I also try to ONLY clear counters(counter1/2/3) from interrupt handler, like below, without a spinlock. It can work, but I think may there are some pitfalls
+       for (i = 0; i < NUM_COUNTERS; i++) {
+               if (!pmu->events[i])
+                       continue;
+
+               event = pmu->events[i];
+
+               if (event->hw.idx == EVENT_CYCLES_COUNTER)
+                       continue;
+
+               /* clear non-cycle counters */
+               ddr_perf_counter_enable(pmu, event->attr.config, event->hw.idx, true);
+
+               local64_set(&event->hw.prev_count, 0);
+       }

Such case, when call ddr_perf_counter_enable to clear counter1, but have not set prev_count equal 0 yet in the interrupt handler. Concurrently, ddr_perf_event_update call ddr_perf_read_counter to read the counter1, it will return 0. Delta (new_raw_count - prev_raw_count) we calculate would be incorrect. So I add a spinlock, for that clear counter1/2/3 and update counter1/2/3 never concurrently happen. It is safe for counter0 to clear then update the counter, since it is actually overflow.

This is the way I take to support stop counter, may exist a better solution. Will, could you share me how to implement it more reasonable? Thanks.

> Finally, this looks like it's an unconditional change in user-visible behaviour. Why
> doesn't it break existing usage of the perf tool?

I don't quite follow you, could you explain more?

Best Regards,
Joakim Zhang
> Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/2] perf/imx_ddr: Correct the CLEAR bit definition

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: 2020年3月2日 19:26
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/2] perf/imx_ddr: Correct the CLEAR bit definition
> 
> On Tue, Feb 25, 2020 at 08:56:43PM +0800, Joakim Zhang wrote:
> > ddr_perf_event_stop will firstly call ddr_perf_counter_enable to
> > disable the counter, and then call ddr_perf_event_update to read the counter
> value.
> >
> > When disable the counter, it will write 0 into COUNTER_CNTL[CLEAR] bit
> > which cause the counter value cleared. Counter value will always be 0
> > when update the counter.
> >
> > The correct definition of CLEAR bit is that write 0 to clear the
> > counter value.
> 
> Ok, so the issue is that when disabling the counter we clear the counter value
> at the same time. I'll update the text and apply this fix.

Yes, Will, you are right!

Best Regards,
Joakim Zhang
> Cheers,
> 
> Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP

[Joakim Zhang]

Hi Will,

Any comments about this issue? Thanks a lot!

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2020年3月3日 13:35
> To: Will Deacon <will@kernel.org>
> Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP
> 
> 
> > -----Original Message-----
> > From: Will Deacon <will@kernel.org>
> > Sent: 2020年3月2日 19:25
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx
> > <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH 2/2] perf/imx_ddr: Add stop counter support for
> > i.MX8MP
> >
> > On Tue, Feb 25, 2020 at 08:56:44PM +0800, Joakim Zhang wrote:
> > > DDR perf driver now only supports free-running event counters
> > > (counter1/2/3), which means that event counters will continue
> > > counting even they are overflow.
> > >
> > > However, the situation is changed on i.MX8MP, event counters are not
> > > free-running any more. Event counters would stop counting if they
> > > are overflow. So we need clear event counters when cycle counter overflow.
> > >
> > > The patch adds stop counter support which would be compatible to
> > > free-running counter.
> >
> > Hmm, are you saying that the hardware behaviour has changed here, so
> > that newer SoCs force the behaviour where all the counters stop when
> > one overflows? Is there any way to control that behaviour?
> Hi Will,
> 
> Yes, the hardware behavior has changed in i.MX8MP.
> 
> Legacy SoCs, when counter0(cycle counter) overflows, it will lock itself and
> other counters, then generate an interrupt. But, when other
> counters(counter1/2/3) overflow, it will continue counting and not stop.
> In i.MX8MP, all is the same as legacy SoCs, besides that when other
> counters(counter1/2/3) overflow, itself will stop counting, need clear counter
> value manually, then it will start counting again.
> So, in counter0 overflow interrupt handler, we need clear counter1/2/3 value,
> since counter0 will always overflow before other counters, that can ensure
> counter1/2/3 doesn't lose data.
> 
> NOT all counters stop when one overflow. Counter0 overflow will lock itself and
> other counters, other counters overflow just stop themselves.
> 
> No way to control the behavior, it is the hardware behavior.
> 
> > > @@ -566,6 +571,25 @@ static irqreturn_t ddr_perf_irq_handler(int
> > > irq, void
> > *p)
> > >  			cycle_event = event;
> > >  	}
> > >
> > > +	spin_lock(&pmu->lock);
> > > +
> > > +	for (i = 0; i < NUM_COUNTERS; i++) {
> > > +		if (!pmu->events[i])
> > > +			continue;
> > > +
> > > +		event = pmu->events[i];
> > > +
> > > +		if (event->hw.idx == EVENT_CYCLES_COUNTER)
> > > +			continue;
> > > +
> > > +		/* clear non-cycle counters */
> > > +		ddr_perf_counter_enable(pmu, event->attr.config,
> event->hw.idx,
> > > +true);
> > > +
> > > +		local64_set(&event->hw.prev_count, 0);
> > > +	}
> > > +
> > > +	spin_unlock(&pmu->lock);
> >
> > I'm suspicious of this locking, as it's /very/ fine-grained. What
> > prevents racing with a concurrent ddr_perf_counter_enable() call?
> > Also, doesn't perf core need to be aware that you're stopping counters here?
> 
> I also try to ONLY clear counters(counter1/2/3) from interrupt handler, like
> below, without a spinlock. It can work, but I think may there are some pitfalls
> +       for (i = 0; i < NUM_COUNTERS; i++) {
> +               if (!pmu->events[i])
> +                       continue;
> +
> +               event = pmu->events[i];
> +
> +               if (event->hw.idx == EVENT_CYCLES_COUNTER)
> +                       continue;
> +
> +               /* clear non-cycle counters */
> +               ddr_perf_counter_enable(pmu, event->attr.config,
> + event->hw.idx, true);
> +
> +               local64_set(&event->hw.prev_count, 0);
> +       }
> 
> Such case, when call ddr_perf_counter_enable to clear counter1, but have not
> set prev_count equal 0 yet in the interrupt handler. Concurrently,
> ddr_perf_event_update call ddr_perf_read_counter to read the counter1, it
> will return 0. Delta (new_raw_count - prev_raw_count) we calculate would be
> incorrect. So I add a spinlock, for that clear counter1/2/3 and update
> counter1/2/3 never concurrently happen. It is safe for counter0 to clear then
> update the counter, since it is actually overflow.
> 
> This is the way I take to support stop counter, may exist a better solution. Will,
> could you share me how to implement it more reasonable? Thanks.
> 
> > Finally, this looks like it's an unconditional change in user-visible
> > behaviour. Why doesn't it break existing usage of the perf tool?
> 
> I don't quite follow you, could you explain more?
> 
> Best Regards,
> Joakim Zhang
> > Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP

[Will Deacon]

On Thu, Apr 16, 2020 at 09:51:13AM +0000, Joakim Zhang wrote:
> Any comments about this issue? Thanks a lot!

You didn't really answer any of my questions, so I don't really know what to
do with this.

  - The locking appears to be broken. Your solution was to remove it
    entirely.

  - It appears to be a user visible change and you haven't explained how it
    continues to work with old userspace

  - Perf core is not aware of you stopping counters and you haven't said why
    that's not an issue.

While these issues are outstanding, I cannot merge the patch. Sorry.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: 2020年5月20日 15:52
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 2/2] perf/imx_ddr: Add stop counter support for i.MX8MP
> 
> On Thu, Apr 16, 2020 at 09:51:13AM +0000, Joakim Zhang wrote:
> > Any comments about this issue? Thanks a lot!
> 
> You didn't really answer any of my questions, so I don't really know what to do
> with this.
> 
>   - The locking appears to be broken. Your solution was to remove it
>     entirely.
> 
>   - It appears to be a user visible change and you haven't explained how it
>     continues to work with old userspace
> 
>   - Perf core is not aware of you stopping counters and you haven't said why
>     that's not an issue.
> 
> While these issues are outstanding, I cannot merge the patch. Sorry.

Hi Will,

You are really kind. Sorry for that, sometimes I am not quite understand what you want. I send out this patch, just want to talk with you to find a better solution, you could provide profession opinion.

Actually new SoC has a hardware change:

Old SoC:
Counter0 is a special counter, only count cycles. Counter1-3 are event counters. When counter0 overflow, it will lock all counters and generate an interrupt. In ddr_perf_irq_handler(), disable counter0 then all counter1-3 will stop at the same time, update all counters' count, then enable counter0 that all counters would count again. You can see that when enable counter0 it would clear overflow bit, but counter1-3 are free-running, need not clear it. Do/while() from ddr_perf_event_update() can handle counter1-3 overflow case.

MX8MP:
Almost is same with old SoC, the only different is that, counter1-3 are not free-running now. Like counter0, when counter1-3 are overflow, they would stop counting unless clear their overflow bit. Counter0 overflow occurs at least 4 times as often as other counters, so I want to re-enable counter1-3 then they can re-count again, to ensure that counter1-3 will not lose data. The key is that I need clear counter1-3 in counter0 irq handler.

The count updating would happen at irq handler or perf core(read callback). I add a spinlock to avoid updating counter1-3 while clearing counter1-3, but I am not sure if it needs. Looking forward to your feedbacks, please point out my mistakes. Thanks a lot.

Best Regards,
Joakim Zhang
> Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel