[PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support

[PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support

[Joakim Zhang]

With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter
which only can get bursts of reading/writing DDR, i.e. DDR read/write
request.

This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW
supports AXI ID filter which can get bytes of reading/writing DDR. This
feature is more meaningful due to we always care more about bandwidth.

Need select both above two qiurks together when HW support enhanced AXI
ID filter.

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

diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
index ce7345745b42..5f70dbfa9607 100644
--- a/drivers/perf/fsl_imx8_ddr_perf.c
+++ b/drivers/perf/fsl_imx8_ddr_perf.c
@@ -45,7 +45,8 @@
 static DEFINE_IDA(ddr_ida);
 
 /* DDR Perf hardware feature */
-#define DDR_CAP_AXI_ID_FILTER          0x1     /* support AXI ID filter */
+#define DDR_CAP_AXI_ID_FILTER			BIT(1)     /* support AXI ID filter */
+#define DDR_CAP_AXI_ID_FILTER_ENHANCED		BIT(2)     /* support enhanced AXI ID filter */
 
 struct fsl_ddr_devtype_data {
 	unsigned int quirks;    /* quirks needed for different DDR Perf core */
@@ -209,7 +210,15 @@ static void ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
 
 static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
 {
-	return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
+	if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) &&
+	    (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED)) {
+		if ((pmu->events[counter]->attr.config == 0x41) ||
+		    (pmu->events[counter]->attr.config == 0x42))
+			return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter * 4);
+		else
+			return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
+	} else
+		return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
 }
 
 static bool ddr_perf_is_filtered(struct perf_event *event)
-- 
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] doc/perf: add DDR_CAP_AXI_ID_FILTER_ENHANCED quirk explanation

[Joakim Zhang]

Add DDR_CAP_AXI_ID_FILTER_ENHANCED quirk explanation.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 Documentation/admin-guide/perf/imx-ddr.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/admin-guide/perf/imx-ddr.rst b/Documentation/admin-guide/perf/imx-ddr.rst
index 45943c7325e3..0f2170974701 100644
--- a/Documentation/admin-guide/perf/imx-ddr.rst
+++ b/Documentation/admin-guide/perf/imx-ddr.rst
@@ -51,3 +51,10 @@ in the driver.
   e.g.::
         perf stat -a -e imx8_ddr0/axid-read,axi_id=0x12/ cmd, which will monitor ARID=0x12
 
+* With DDR_CAP_AXI_ID_FILTER_ENHANCED quirk.
+  This is the extension of DDR_CAP_AXI_ID_FILTER quirk which is only support getting
+  bursts from DDR reading or writing, i.e. only can get DDR read or write requests.
+  You can select both DDR_CAP_AXI_ID_FILTER and DDR_CAP_AXI_ID_FILTER_ENHANCED
+  quirks together to get bytes of reading or writing DDR if HW supports this
+  enhanced filter. It is meaningful as user always care more about each master's
+  bandwidth.
-- 
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 1/2] perf/imx_ddr: add enhanced AXI ID filter support

[Joakim Zhang]

Kindly Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2019年9月6日 16:27
> To: will@kernel.org; mark.rutland@arm.com; robin.murphy@arm.com; Frank
> Li <frank.li@nxp.com>
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> Joakim Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support
> 
> With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter
> which only can get bursts of reading/writing DDR, i.e. DDR read/write request.
> 
> This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW
> supports AXI ID filter which can get bytes of reading/writing DDR. This feature
> is more meaningful due to we always care more about bandwidth.
> 
> Need select both above two qiurks together when HW support enhanced AXI ID
> filter.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index ce7345745b42..5f70dbfa9607 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -45,7 +45,8 @@
>  static DEFINE_IDA(ddr_ida);
> 
>  /* DDR Perf hardware feature */
> -#define DDR_CAP_AXI_ID_FILTER          0x1     /* support AXI ID filter
> */
> +#define DDR_CAP_AXI_ID_FILTER			BIT(1)     /* support AXI ID
> filter */
> +#define DDR_CAP_AXI_ID_FILTER_ENHANCED		BIT(2)     /* support
> enhanced AXI ID filter */
> 
>  struct fsl_ddr_devtype_data {
>  	unsigned int quirks;    /* quirks needed for different DDR Perf core */
> @@ -209,7 +210,15 @@ static void ddr_perf_free_counter(struct ddr_pmu
> *pmu, int counter)
> 
>  static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)  {
> -	return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
> +	if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) &&
> +	    (pmu->devtype_data->quirks &
> DDR_CAP_AXI_ID_FILTER_ENHANCED)) {
> +		if ((pmu->events[counter]->attr.config == 0x41) ||
> +		    (pmu->events[counter]->attr.config == 0x42))
> +			return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter
> * 4);
> +		else
> +			return readl_relaxed(pmu->base + COUNTER_READ + counter *
> 4);
> +	} else
> +		return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
>  }
> 
>  static bool ddr_perf_is_filtered(struct perf_event *event)
> --
> 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 1/2] perf/imx_ddr: add enhanced AXI ID filter support

[Will Deacon]

On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote:
> With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter
> which only can get bursts of reading/writing DDR, i.e. DDR read/write
> request.
> 
> This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW
> supports AXI ID filter which can get bytes of reading/writing DDR. This
> feature is more meaningful due to we always care more about bandwidth.
> 
> Need select both above two qiurks together when HW support enhanced AXI
> ID filter.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index ce7345745b42..5f70dbfa9607 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -45,7 +45,8 @@
>  static DEFINE_IDA(ddr_ida);
>  
>  /* DDR Perf hardware feature */
> -#define DDR_CAP_AXI_ID_FILTER          0x1     /* support AXI ID filter */
> +#define DDR_CAP_AXI_ID_FILTER			BIT(1)     /* support AXI ID filter */

Is this a deliberate change from the previous code and, if so, why?

> +#define DDR_CAP_AXI_ID_FILTER_ENHANCED		BIT(2)     /* support enhanced AXI ID filter */

What does it mean to have DDR_CAP_AXI_ID_FILTER_ENHANCED but not
DDR_CAP_AXI_ID_FILTER? Could we just say that DDR_CAP_AXI_ID_FILTER_ENHANCED
implies DDR_CAP_AXI_ID_FILTER?

>  struct fsl_ddr_devtype_data {
>  	unsigned int quirks;    /* quirks needed for different DDR Perf core */
> @@ -209,7 +210,15 @@ static void ddr_perf_free_counter(struct ddr_pmu *pmu, int counter)
>  
>  static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)
>  {
> -	return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
> +	if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) &&
> +	    (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED)) {
> +		if ((pmu->events[counter]->attr.config == 0x41) ||
> +		    (pmu->events[counter]->attr.config == 0x42))
> +			return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter * 4);

In which case, this could be slightly simplified.

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: add enhanced AXI ID filter support

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: 2019年10月28日 22:55
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mark.rutland@arm.com; robin.murphy@arm.com; Frank Li
> <frank.li@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support
> 
> On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote:
> > With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter
> > which only can get bursts of reading/writing DDR, i.e. DDR read/write
> > request.
> >
> > This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW
> > supports AXI ID filter which can get bytes of reading/writing DDR.
> > This feature is more meaningful due to we always care more about
> bandwidth.
> >
> > Need select both above two qiurks together when HW support enhanced
> > AXI ID filter.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> > b/drivers/perf/fsl_imx8_ddr_perf.c
> > index ce7345745b42..5f70dbfa9607 100644
> > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > @@ -45,7 +45,8 @@
> >  static DEFINE_IDA(ddr_ida);
> >
> >  /* DDR Perf hardware feature */
> > -#define DDR_CAP_AXI_ID_FILTER          0x1     /* support AXI ID
> filter */
> > +#define DDR_CAP_AXI_ID_FILTER			BIT(1)     /* support AXI ID
> filter */
>
> Is this a deliberate change from the previous code and, if so, why?

[Joakim] DDR PMU integrated in various SoCs may have different features(extend more features), use BIT(1)/BIT(2)/BIT(3)... instead of 0x1/0x2/0x4..., I think it is more easier.
        This deliberate change from previous code has no functional effect.

> > +#define DDR_CAP_AXI_ID_FILTER_ENHANCED		BIT(2)     /*
> support enhanced AXI ID filter */
> 
> What does it mean to have DDR_CAP_AXI_ID_FILTER_ENHANCED but not
> DDR_CAP_AXI_ID_FILTER? Could we just say that
> DDR_CAP_AXI_ID_FILTER_ENHANCED implies DDR_CAP_AXI_ID_FILTER?

[Joakim] Yes, DDR_CAP_AXI_ID_FILTER_ENHANCED is based on DDR_CAP_AXI_ID_FILTER and extend it. They are both for event 0x41 and event 0x42.
        1)HW(i.MX8M Mini/Quad/Nano) support DDR_CAP_AXI_ID_FILTER, counters only can count DDR burst transaction(read/write requests).
		2)HW(i.MX8M Plus) support both DDR_CAP_AXI_ID_FILTER and DDR_CAP_AXI_ID_FILTER_ENHANCED, means that counters can count DDR burst(read/write requests) and data(bytes) transaction at the same time due to it extend another set of data counters(registers address: 0x34,0x38,0x3c). So we can read these counters value to get the *bytes* from DDR transaction. This is more meaningful for users.

> >  struct fsl_ddr_devtype_data {
> >  	unsigned int quirks;    /* quirks needed for different DDR Perf core */
> > @@ -209,7 +210,15 @@ static void ddr_perf_free_counter(struct ddr_pmu
> > *pmu, int counter)
> >
> >  static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter)  {
> > -	return readl_relaxed(pmu->base + COUNTER_READ + counter * 4);
> > +	if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) &&
> > +	    (pmu->devtype_data->quirks &
> DDR_CAP_AXI_ID_FILTER_ENHANCED)) {
> > +		if ((pmu->events[counter]->attr.config == 0x41) ||
> > +		    (pmu->events[counter]->attr.config == 0x42))
> > +			return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter
> * 4);
> 
> In which case, this could be slightly simplified.

[Joakim] I will use the ddr_perf_is_filtered() helper to simply the code. A v2 is sending, feel free to give comments.

Thanks Will.

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: add enhanced AXI ID filter support

[Will Deacon]

Hi Joakim,

Please can you try to configure your email client correctly, so that you
don't have to prefix your replies with '[Joakim]'? I use mutt, which works
pretty well, but you can look at Documentation/process/email-clients.rst
for hints about using other clients.

On Tue, Oct 29, 2019 at 07:02:28AM +0000, Joakim Zhang wrote:
> > From: Will Deacon <will@kernel.org>
> > On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote:
> > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> > > b/drivers/perf/fsl_imx8_ddr_perf.c
> > > index ce7345745b42..5f70dbfa9607 100644
> > > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > > @@ -45,7 +45,8 @@
> > >  static DEFINE_IDA(ddr_ida);
> > >
> > >  /* DDR Perf hardware feature */
> > > -#define DDR_CAP_AXI_ID_FILTER          0x1     /* support AXI ID
> > filter */
> > > +#define DDR_CAP_AXI_ID_FILTER			BIT(1)     /* support AXI ID
> > filter */
> >
> > Is this a deliberate change from the previous code and, if so, why?
> 
> [Joakim] DDR PMU integrated in various SoCs may have different
> features(extend more features), use BIT(1)/BIT(2)/BIT(3)... instead of
> 0x1/0x2/0x4..., I think it is more easier.  This deliberate change from
> previous code has no functional effect.

My point was that I don't think BIT(1) == 0x1.

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: add enhanced AXI ID filter support

[Joakim Zhang]

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: 2019年10月30日 23:37
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: mark.rutland@arm.com; robin.murphy@arm.com; Frank Li
> <frank.li@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support
> 
> Hi Joakim,
> 
> Please can you try to configure your email client correctly, so that you don't
> have to prefix your replies with '[Joakim]'? I use mutt, which works pretty well,
> but you can look at Documentation/process/email-clients.rst
> for hints about using other clients.

Hi Will,

I use outlook, and the tag [Joakim] added manually by me for clear indication. I will reply this email without it. If you find the incorrect format in replied email, please point to me in detail, as this configuration of my client can work fine in local patch review. Thanks Will.

> On Tue, Oct 29, 2019 at 07:02:28AM +0000, Joakim Zhang wrote:
> > > From: Will Deacon <will@kernel.org>
> > > On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote:
> > > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> > > > b/drivers/perf/fsl_imx8_ddr_perf.c
> > > > index ce7345745b42..5f70dbfa9607 100644
> > > > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > > > @@ -45,7 +45,8 @@
> > > >  static DEFINE_IDA(ddr_ida);
> > > >
> > > >  /* DDR Perf hardware feature */
> > > > -#define DDR_CAP_AXI_ID_FILTER          0x1     /* support AXI ID
> > > filter */
> > > > +#define DDR_CAP_AXI_ID_FILTER			BIT(1)     /* support
> AXI ID
> > > filter */
> > >
> > > Is this a deliberate change from the previous code and, if so, why?
> >
> > [Joakim] DDR PMU integrated in various SoCs may have different
> > features(extend more features), use BIT(1)/BIT(2)/BIT(3)... instead of
> > 0x1/0x2/0x4..., I think it is more easier.  This deliberate change
> > from previous code has no functional effect.
> 
> My point was that I don't think BIT(1) == 0x1.

Sorry for my mistake. I will change into BIT(0) for 0x1 to align to previous code in next version. Could you firstly ignore this and then review others in V2? I will fix all of them together in V3.

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