| Message ID | 20200225125644.18853-1-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] perf/imx_ddr: Correct the CLEAR bit definition | expand |
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
> -----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
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); } }
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(-)