diff mbox series

[v2,2/3] perf/imx_ddr: adjust counter result after read cycle counter

Message ID 20230713103758.2627269-2-xu.yang_2@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/3] perf/imx_ddr: speed up overflow frequency of cycle counter | expand

Commit Message

Xu Yang July 13, 2023, 10:37 a.m. UTC
Because we initialize CP filed to shorten counter0 overflow time, the cycle
counter will start couting from a fixed/base value each time. We need to
remove the base from the result too. Therefore, we could get precise result
from cycle counter.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - improve if condition
---
 drivers/perf/fsl_imx8_ddr_perf.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Frank Li July 13, 2023, 3:36 p.m. UTC | #1
> -----Original Message-----
> From: Xu Yang <xu.yang_2@nxp.com>
> Sent: Thursday, July 13, 2023 5:38 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: will@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; dl-linux-imx <linux-
> imx@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v2 2/3] perf/imx_ddr: adjust counter result after read cycle
> counter
> 
> Because we initialize CP filed to shorten counter0 overflow time, the cycle
> counter will start couting from a fixed/base value each time. We need to
> remove the base from the result too. Therefore, we could get precise result
> from cycle counter.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v2:
>  - improve if condition
> ---
>  drivers/perf/fsl_imx8_ddr_perf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> b/drivers/perf/fsl_imx8_ddr_perf.c
> index 039069756bbc..d65200d4e96e 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -481,6 +481,12 @@ static void ddr_perf_event_update(struct
> perf_event *event)
>  	int ret;
> 
>  	new_raw_count = ddr_perf_read_counter(pmu, counter);
> +	/* Workaround for i.MX8MP */
> +	if (pmu->devtype_data->quirks &
> DDR_CAP_AXI_ID_FILTER_ENHANCED) {
> +		if (counter == EVENT_CYCLES_COUNTER)
> +			new_raw_count -= 0xF0000000;

[Frank Li] Do you means
	new_raw_count &= 0x0FFFFFFF? to mask bit 24..31?

> +	}
> +
>  	local64_add(new_raw_count, &event->count);
> 
>  	/*
> --
> 2.34.1
Xu Yang July 14, 2023, 1:44 a.m. UTC | #2
-----Original Message-----
> 
> Because we initialize CP filed to shorten counter0 overflow time, the cycle
> counter will start couting from a fixed/base value each time. We need to
> remove the base from the result too. Therefore, we could get precise result
> from cycle counter.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v2:
>  - improve if condition
> ---
>  drivers/perf/fsl_imx8_ddr_perf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> b/drivers/perf/fsl_imx8_ddr_perf.c
> index 039069756bbc..d65200d4e96e 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -481,6 +481,12 @@ static void ddr_perf_event_update(struct
> perf_event *event)
>  	int ret;
> 
>  	new_raw_count = ddr_perf_read_counter(pmu, counter);
> +	/* Workaround for i.MX8MP */
> +	if (pmu->devtype_data->quirks &
> DDR_CAP_AXI_ID_FILTER_ENHANCED) {
> +		if (counter == EVENT_CYCLES_COUNTER)
> +			new_raw_count -= 0xF0000000;

[Frank Li] Do you means
	new_raw_count &= 0x0FFFFFFF? to mask bit 24..31?

Yes, it can be this way too. Do I need change it in v3?

Thanks,
Xu Yang

> +	}
> +
>  	local64_add(new_raw_count, &event->count);
> 
>  	/*
> --
> 2.34.1
Mark Rutland July 28, 2023, 1:36 p.m. UTC | #3
On Thu, Jul 13, 2023 at 06:37:57PM +0800, Xu Yang wrote:
> Because we initialize CP filed to shorten counter0 overflow time, the cycle
> counter will start couting from a fixed/base value each time. We need to
> remove the base from the result too. Therefore, we could get precise result
> from cycle counter.

This means that patch 1 is incomplete; please fold this into patch 1.

> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v2:
>  - improve if condition
> ---
>  drivers/perf/fsl_imx8_ddr_perf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index 039069756bbc..d65200d4e96e 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -481,6 +481,12 @@ static void ddr_perf_event_update(struct perf_event *event)
>  	int ret;
>  
>  	new_raw_count = ddr_perf_read_counter(pmu, counter);
> +	/* Workaround for i.MX8MP */
> +	if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) {
> +		if (counter == EVENT_CYCLES_COUNTER)
> +			new_raw_count -= 0xF0000000;
> +	}

I think as Frank suggested, it would be clearer to have a mask, and I think
this should have a comment:

	/*
	 * Remove the bias applied in ddr_perf_counter_enable().
	 */
	if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) {
		if (counter == EVENT_CYCLES_COUNTER)
			new_raw_count &= 0x0fffffff;
	}

Thanks,
Mark.

> +
>  	local64_add(new_raw_count, &event->count);
>  
>  	/*
> -- 
> 2.34.1
>
Xu Yang July 29, 2023, 2:08 a.m. UTC | #4
Hi Mark,

> On Thu, Jul 13, 2023 at 06:37:57PM +0800, Xu Yang wrote:
> > Because we initialize CP filed to shorten counter0 overflow time, the cycle
> > counter will start couting from a fixed/base value each time. We need to
> > remove the base from the result too. Therefore, we could get precise result
> > from cycle counter.
> 
> This means that patch 1 is incomplete; please fold this into patch 1.

Okay.

> 
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >
> > ---
> > Changes in v2:
> >  - improve if condition
> > ---
> >  drivers/perf/fsl_imx8_ddr_perf.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> > index 039069756bbc..d65200d4e96e 100644
> > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > @@ -481,6 +481,12 @@ static void ddr_perf_event_update(struct perf_event *event)
> >       int ret;
> >
> >       new_raw_count = ddr_perf_read_counter(pmu, counter);
> > +     /* Workaround for i.MX8MP */
> > +     if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) {
> > +             if (counter == EVENT_CYCLES_COUNTER)
> > +                     new_raw_count -= 0xF0000000;
> > +     }
> 
> I think as Frank suggested, it would be clearer to have a mask, and I think
> this should have a comment:

Agree, I'm going to update the code as following too.

Thanks,
Xu Yang

> 
>         /*
>          * Remove the bias applied in ddr_perf_counter_enable().
>          */
>         if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) {
>                 if (counter == EVENT_CYCLES_COUNTER)
>                         new_raw_count &= 0x0fffffff;
>         }
> 
> Thanks,
> Mark.
> 
> > +
> >       local64_add(new_raw_count, &event->count);
> >
> >       /*
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
index 039069756bbc..d65200d4e96e 100644
--- a/drivers/perf/fsl_imx8_ddr_perf.c
+++ b/drivers/perf/fsl_imx8_ddr_perf.c
@@ -481,6 +481,12 @@  static void ddr_perf_event_update(struct perf_event *event)
 	int ret;
 
 	new_raw_count = ddr_perf_read_counter(pmu, counter);
+	/* Workaround for i.MX8MP */
+	if (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) {
+		if (counter == EVENT_CYCLES_COUNTER)
+			new_raw_count -= 0xF0000000;
+	}
+
 	local64_add(new_raw_count, &event->count);
 
 	/*