Re: [PATCH] perf: imx9_perf: introduce axi filter version

[Xu Yang <xu.yang_2@nxp.com>]

On Wed, Nov 20, 2024 at 11:35:10AM -0500, Frank Li wrote:
> On Wed, Nov 20, 2024 at 11:21:09AM +0800, Xu Yang wrote:
> > The imx93 is the first supported DDR PMU that supports read transaction,
> > write transaction and read beats events which corresponding respecitively
> > to counter 2, 3 and 4.
> 
> Introduce AXI filter version to refactor the driver and better extension.

Okay.

> 
> >
> > However, transaction-based AXI match has low accuracy when get total bits
> > compared to beats-based. And imx93 doesn't assign AXI_ID to each master.
> > So axi filter is not used widely on imx93. This could be regards as AXI
> > filter version 1.
> >
> > To improve the AXI filter capability, imx95 supports 1 read beats and 3
> > write beats event which corresponding respecitively to counter 2-5. imx95
> > also detailed AXI_ID allocation so that most of the master could be count
> > individually. This could be regards as AXI filter version 2.
> 
> Such informaiton is not related with your this change. I suggest put it
> to comment for DDR_PERF_AXI_FILTER_V1 and DDR_PERF_AXI_FILTER_V2.

Okay.

> 
> >
> > This will introduce AXI filter version to refactor the driver and support
> > better extension, such as coming imx943.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/perf/fsl_imx9_ddr_perf.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> > index 3c856d9a4e97..965345d78f7a 100644
> > --- a/drivers/perf/fsl_imx9_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> > @@ -63,8 +63,12 @@
> >
> >  static DEFINE_IDA(ddr_ida);
> >
> > +#define DDR_PERF_AXI_FILTER_V1          0x1
> > +#define DDR_PERF_AXI_FILTER_V2          0x2
> 
> V1 V2 is quite difficult to know the difference. Please add comments to
> show the more detail about V1 and V2, you can resue from commit message.

Sure.

> 
> > +
> >  struct imx_ddr_devtype_data {
> >  	const char *identifier;		/* system PMU identifier for userspace */
> > +	unsigned int type;		/* AXI filter version */
> 
> variable name is better match function
> 	"filter_ver"

Okay.

Thanks,
Xu Yang

> 
> >  };
> >
> >  struct ddr_pmu {
> > @@ -83,24 +87,27 @@ struct ddr_pmu {
> >
> >  static const struct imx_ddr_devtype_data imx91_devtype_data = {
> >  	.identifier = "imx91",
> > +	.type = DDR_PERF_AXI_FILTER_V1
> >  };
> >
> >  static const struct imx_ddr_devtype_data imx93_devtype_data = {
> >  	.identifier = "imx93",
> > +	.type = DDR_PERF_AXI_FILTER_V1
> >  };
> >
> >  static const struct imx_ddr_devtype_data imx95_devtype_data = {
> >  	.identifier = "imx95",
> > +	.type = DDR_PERF_AXI_FILTER_V2
> >  };
> >
> > -static inline bool is_imx93(struct ddr_pmu *pmu)
> > +static inline bool axi_filter_v1(struct ddr_pmu *pmu)
> >  {
> > -	return pmu->devtype_data == &imx93_devtype_data;
> > +	return pmu->devtype_data->type == DDR_PERF_AXI_FILTER_V1;
> >  }
> >
> > -static inline bool is_imx95(struct ddr_pmu *pmu)
> > +static inline bool axi_filter_v2(struct ddr_pmu *pmu)
> >  {
> > -	return pmu->devtype_data == &imx95_devtype_data;
> > +	return pmu->devtype_data->type == DDR_PERF_AXI_FILTER_V2;
> >  }
> >
> >  static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
> > @@ -155,7 +162,7 @@ static const struct attribute_group ddr_perf_cpumask_attr_group = {
> >  struct imx9_pmu_events_attr {
> >  	struct device_attribute attr;
> >  	u64 id;
> > -	const void *devtype_data;
> > +	const struct imx_ddr_devtype_data *devtype_data;
> >  };
> >
> >  static ssize_t ddr_pmu_event_show(struct device *dev,
> > @@ -307,7 +314,8 @@ ddr_perf_events_attrs_is_visible(struct kobject *kobj,
> >  	if (!eattr->devtype_data)
> >  		return attr->mode;
> >
> > -	if (eattr->devtype_data != ddr_pmu->devtype_data)
> > +	if ((eattr->devtype_data != ddr_pmu->devtype_data) &&
> > +		(eattr->devtype_data->type != ddr_pmu->devtype_data->type))
> >  		return 0;
> >
> >  	return attr->mode;
> > @@ -624,11 +632,11 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> >  	hwc->idx = counter;
> >  	hwc->state |= PERF_HES_STOPPED;
> >
> > -	if (is_imx93(pmu))
> > +	if (axi_filter_v1(pmu))
> >  		/* read trans, write trans, read beat */
> >  		imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> >
> > -	if (is_imx95(pmu))
> > +	if (axi_filter_v2(pmu))
> >  		/* write beat, read beat2, read beat1, read beat */
> >  		imx95_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> >
> > --
> > 2.34.1
> >