diff mbox series

[v6,1/7] dt-bindings: perf: fsl-imx-ddr: Add i.MX95 compatible

Message ID 20240307095730.3792680-1-xu.yang_2@nxp.com
State Not Applicable
Headers show
Series [v6,1/7] dt-bindings: perf: fsl-imx-ddr: Add i.MX95 compatible | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Xu Yang March 7, 2024, 9:57 a.m. UTC
i.MX95 has a DDR pmu. This will add a compatible for it.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - no changes
Changes in v3:
 - let imx95 compatilbe with imx93
Changes in v4:
 - add Acked-by tag
Changes in v5:
 - no changes
Changes in v6:
 - no changes
---
 Documentation/devicetree/bindings/perf/fsl-imx-ddr.yaml | 3 +++
 1 file changed, 3 insertions(+)

Comments

John Garry March 7, 2024, 1:02 p.m. UTC | #1
On 07/03/2024 09:57, Xu Yang wrote:
> Add JSON metrics for i.MX95 DDR Performance Monitor.
> 
> Reviewed-by: Ian Rogers<irogers@google.com>
> Reviewed-by: Frank Li<Frank.Li@nxp.com>
> Signed-off-by: Xu Yang<xu.yang_2@nxp.com>

FWIW, Please note that I gave a RB tag here:
https://lore.kernel.org/linux-perf-users/dd69cee0-ac14-4e20-9255-367240b1dbc8@oracle.com/
Frank Li March 7, 2024, 4:42 p.m. UTC | #2
On Thu, Mar 07, 2024 at 05:57:25PM +0800, Xu Yang wrote:
> The user can set event and counter in cmdline and the driver need to parse
> it using 'config' attr value. This will add macro definitions to avoid
> hard-code in driver.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v4:
>  - new patch
> Changes in v5:
>  - move this patch earlier
> Changes in v6:
>  - no changes
> ---
>  drivers/perf/fsl_imx9_ddr_perf.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> index 9685645bfe04..d1c566e661d8 100644
> --- a/drivers/perf/fsl_imx9_ddr_perf.c
> +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> @@ -42,6 +42,11 @@
>  #define NUM_COUNTERS		11
>  #define CYCLES_COUNTER		0
>  
> +#define CONFIG_EVENT_MASK	0x00FF
> +#define CONFIG_EVENT_OFFSET	0

Needn't need OFFSET if use FIELD_*

> +#define CONFIG_COUNTER_MASK	0xFF00
> +#define CONFIG_COUNTER_OFFSET	8

The same

> +
>  #define to_ddr_pmu(p)		container_of(p, struct ddr_pmu, pmu)
>  
>  #define DDR_PERF_DEV_NAME	"imx9_ddr"
> @@ -339,8 +344,10 @@ static void ddr_perf_counter_local_config(struct ddr_pmu *pmu, int config,
>  				    int counter, bool enable)
>  {
>  	u32 ctrl_a;
> +	int event;
>  
>  	ctrl_a = readl_relaxed(pmu->base + PMLCA(counter));
> +	event = (config & CONFIG_EVENT_MASK) >> CONFIG_EVENT_OFFSET;

FIELD_GET(CONFIG_EVENT_MASK, config);

same below all code about config.

>  
>  	if (enable) {
>  		ctrl_a |= PMLCA_FC;
> @@ -352,7 +359,7 @@ static void ddr_perf_counter_local_config(struct ddr_pmu *pmu, int config,
>  		ctrl_a &= ~PMLCA_FC;
>  		ctrl_a |= PMLCA_CE;
>  		ctrl_a &= ~FIELD_PREP(PMLCA_EVENT, 0x7F);
> -		ctrl_a |= FIELD_PREP(PMLCA_EVENT, (config & 0x000000FF));
> +		ctrl_a |= FIELD_PREP(PMLCA_EVENT, event);
>  		writel(ctrl_a, pmu->base + PMLCA(counter));
>  	} else {
>  		/* Freeze counter. */
> @@ -366,8 +373,8 @@ static void ddr_perf_monitor_config(struct ddr_pmu *pmu, int cfg, int cfg1, int
>  	u32 pmcfg1, pmcfg2;
>  	int event, counter;
>  
> -	event = cfg & 0x000000FF;
> -	counter = (cfg & 0x0000FF00) >> 8;
> +	event = (cfg & CONFIG_EVENT_MASK) >> CONFIG_EVENT_OFFSET;
> +	counter = (cfg & CONFIG_COUNTER_MASK) >> CONFIG_COUNTER_OFFSET;
>  
>  	pmcfg1 = readl_relaxed(pmu->base + PMCFG1);
>  
> @@ -469,7 +476,7 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
>  	int cfg2 = event->attr.config2;
>  	int counter;
>  
> -	counter = (cfg & 0x0000FF00) >> 8;
> +	counter = (cfg & CONFIG_COUNTER_MASK) >> CONFIG_COUNTER_OFFSET;
>  
>  	pmu->events[counter] = event;
>  	pmu->active_events++;
> -- 
> 2.34.1
>
Frank Li March 7, 2024, 4:55 p.m. UTC | #3
On Thu, Mar 07, 2024 at 05:57:27PM +0800, Xu Yang wrote:
> This driver is initinally used to support imx93 Soc and now it's time to
> add support for imx95 Soc. However, some macro definitions and events are
> different on these two Socs. For preparing imx95 supports, this will
> refactor driver for imx93.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v4:
>  - new patch
> Changes in v5:
>  - use is_visible to hide unwanted attributes as suggested by Will
> Changes in v6:
>  - improve imx93_ddr_perf_monitor_config()
> ---
>  drivers/perf/fsl_imx9_ddr_perf.c | 99 +++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> index 8d85b4d98544..4e8a3a4349c5 100644
> --- a/drivers/perf/fsl_imx9_ddr_perf.c
> +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> @@ -11,14 +11,14 @@
>  #include <linux/perf_event.h>
>  
>  /* Performance monitor configuration */
> -#define PMCFG1  			0x00
> -#define PMCFG1_RD_TRANS_FILT_EN 	BIT(31)
> -#define PMCFG1_WR_TRANS_FILT_EN 	BIT(30)
> -#define PMCFG1_RD_BT_FILT_EN 		BIT(29)
> -#define PMCFG1_ID_MASK  		GENMASK(17, 0)
> +#define PMCFG1				0x00
> +#define MX93_PMCFG1_RD_TRANS_FILT_EN	BIT(31)
> +#define MX93_PMCFG1_WR_TRANS_FILT_EN	BIT(30)
> +#define MX93_PMCFG1_RD_BT_FILT_EN	BIT(29)
> +#define MX93_PMCFG1_ID_MASK		GENMASK(17, 0)
>  
> -#define PMCFG2  			0x04
> -#define PMCFG2_ID			GENMASK(17, 0)
> +#define PMCFG2				0x04
> +#define MX93_PMCFG2_ID			GENMASK(17, 0)
>  
>  /* Global control register affects all counters and takes priority over local control registers */
>  #define PMGC0		0x40
> @@ -77,6 +77,11 @@ static const struct imx_ddr_devtype_data imx93_devtype_data = {
>  	.identifier = "imx93",
>  };
>  
> +static inline bool is_imx93(struct ddr_pmu *pmu)
> +{
> +	return pmu->devtype_data == &imx93_devtype_data;
> +}
> +
>  static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
>  	{.compatible = "fsl,imx93-ddr-pmu", .data = &imx93_devtype_data},
>  	{ /* sentinel */ }
> @@ -186,7 +191,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_ld_wiq_6, ID(2, 70)),
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_ld_wiq_7, ID(2, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_empty, ID(2, 72)),
> -	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_trans_filt, ID(2, 73)),
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_trans_filt, ID(2, 73)),	/* imx93 specific*/
>  
>  	/* counter3 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_0, ID(3, 64)),
> @@ -198,7 +203,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_6, ID(3, 70)),
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_7, ID(3, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_full, ID(3, 72)),
> -	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_trans_filt, ID(3, 73)),
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_trans_filt, ID(3, 73)),	/* imx93 specific*/
>  
>  	/* counter4 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_0, ID(4, 64)),
> @@ -210,7 +215,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_6, ID(4, 70)),
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_7, ID(4, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_ld_rdq2_rmw, ID(4, 72)),
> -	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt, ID(4, 73)),
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt, ID(4, 73)),	/* imx93 specific*/
>  
>  	/* counter5 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_0, ID(5, 64)),
> @@ -245,9 +250,26 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	NULL,
>  };
>  
> +static umode_t
> +ddr_perf_events_attrs_is_visible(struct kobject *kobj,
> +				       struct attribute *attr, int unused)
> +{
> +	struct pmu *pmu = dev_get_drvdata(kobj_to_dev(kobj));
> +	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> +
> +	if ((!strcmp(attr->name, "eddrtq_pm_rd_trans_filt") ||
> +		!strcmp(attr->name, "eddrtq_pm_wr_trans_filt") ||
> +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt")) &&
> +		!is_imx93(ddr_pmu))
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
>  static const struct attribute_group ddr_perf_events_attr_group = {
>  	.name = "events",
>  	.attrs = ddr_perf_events_attrs,
> +	.is_visible = ddr_perf_events_attrs_is_visible,
>  };
>  
>  PMU_FORMAT_ATTR(event, "config:0-15");
> @@ -369,36 +391,47 @@ static void ddr_perf_counter_local_config(struct ddr_pmu *pmu, int config,
>  	}
>  }
>  
> -static void ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> -				    int counter, int axi_id, int axi_mask)
> +static void imx93_ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> +					  int counter, int axi_id, int axi_mask)
>  {
>  	u32 pmcfg1, pmcfg2;
>  
>  	pmcfg1 = readl_relaxed(pmu->base + PMCFG1);
>  
> -	if (counter == 2 && event == 73)
> -		pmcfg1 |= PMCFG1_RD_TRANS_FILT_EN;
> -	else if (counter == 2 && event != 73)
> -		pmcfg1 &= ~PMCFG1_RD_TRANS_FILT_EN;
> -
> -	if (counter == 3 && event == 73)
> -		pmcfg1 |= PMCFG1_WR_TRANS_FILT_EN;
> -	else if (counter == 3 && event != 73)
> -		pmcfg1 &= ~PMCFG1_WR_TRANS_FILT_EN;
> -
> -	if (counter == 4 && event == 73)
> -		pmcfg1 |= PMCFG1_RD_BT_FILT_EN;
> -	else if (counter == 4 && event != 73)
> -		pmcfg1 &= ~PMCFG1_RD_BT_FILT_EN;
> +	if (event == 73) {
> +		switch (counter) {
> +		case 2:
> +			pmcfg1 |= MX93_PMCFG1_RD_TRANS_FILT_EN;
> +			break;
> +		case 3:
> +			pmcfg1 |= MX93_PMCFG1_WR_TRANS_FILT_EN;
> +			break;
> +		case 4:
> +			pmcfg1 |= MX93_PMCFG1_RD_BT_FILT_EN;
> +			break;
> +		}
> +	} else {
> +		switch (counter) {
> +		case 2:
> +			pmcfg1 &= ~MX93_PMCFG1_RD_TRANS_FILT_EN;
> +			break;
> +		case 3:
> +			pmcfg1 &= ~MX93_PMCFG1_WR_TRANS_FILT_EN;
> +			break;
> +		case 4:
> +			pmcfg1 &= ~MX93_PMCFG1_RD_BT_FILT_EN;
> +			break;
> +		}
> +	}

how about
	u32 mask[] = { 	MX93_PMCFG1_RD_TRANS_FILT_EN, 
			MX93_PMCFG1_WR_TRANS_FILT_EN,
			MX93_PMCFG1_RD_BT_FILT_EN
	      	     };

	if (couter >=2 && counter <= 4)
		event == 73 ? pmcfg1 |= mask[counter - 2] :
			      pmcfg1 &= ~mask[counter - 2];


Frank

>  
> -	pmcfg1 &= ~FIELD_PREP(PMCFG1_ID_MASK, 0x3FFFF);
> -	pmcfg1 |= FIELD_PREP(PMCFG1_ID_MASK, axi_mask);
> -	writel(pmcfg1, pmu->base + PMCFG1);
> +	pmcfg1 &= ~FIELD_PREP(MX93_PMCFG1_ID_MASK, 0x3FFFF);
> +	pmcfg1 |= FIELD_PREP(MX93_PMCFG1_ID_MASK, axi_mask);
> +	writel_relaxed(pmcfg1, pmu->base + PMCFG1);
>  
>  	pmcfg2 = readl_relaxed(pmu->base + PMCFG2);
> -	pmcfg2 &= ~FIELD_PREP(PMCFG2_ID, 0x3FFFF);
> -	pmcfg2 |= FIELD_PREP(PMCFG2_ID, axi_id);
> -	writel(pmcfg2, pmu->base + PMCFG2);
> +	pmcfg2 &= ~FIELD_PREP(MX93_PMCFG2_ID, 0x3FFFF);
> +	pmcfg2 |= FIELD_PREP(MX93_PMCFG2_ID, axi_id);
> +	writel_relaxed(pmcfg2, pmu->base + PMCFG2);
>  }
>  
>  static void ddr_perf_event_update(struct perf_event *event)
> @@ -514,7 +547,7 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
>  		ddr_perf_event_start(event, flags);
>  
>  	/* read trans, write trans, read beat */
> -	ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> +	imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
>  
>  	return 0;
>  }
> -- 
> 2.34.1
>
Frank Li March 7, 2024, 5:05 p.m. UTC | #4
On Thu, Mar 07, 2024 at 05:57:29PM +0800, Xu Yang wrote:
> i.MX95 has a DDR PMU which is almostly same as i.MX93, it now supports
> read beat and write beat filter capabilities. This will add support for
> i.MX95 and enhance the driver to support specific filter handling for it.
> 
> Usage:
> 
> For read beat:
> ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt2,axi_mask=ID_MASK,axi_id=ID/
> ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt1,axi_mask=ID_MASK,axi_id=ID/
> ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt0,axi_mask=ID_MASK,axi_id=ID/
> eg: For edma2: perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt0,axi_mask=0x00f,axi_id=0x00c/
> 
> For write beat:
> ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_wr_beat_filt,axi_mask=ID_MASK,axi_id=ID/
> eg: For edma2: perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_wr_beat_filt,axi_mask=0x00f,axi_id=0x00c/
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v2:
>  - put soc spefific axi filter events to drvdata according
>    to franks suggestions.
>  - adjust pmcfg axi_id and axi_mask config
> Changes in v3:
>  - no changes
> Changes in v4:
>  - only contain imx95 parts
> Changes in v5:
>  - improve imx95_ddr_perf_monitor_config()
>  - use write_relaxed to pair read_relaxed
> Changes in v6:
>  - no changes
> ---
>  drivers/perf/fsl_imx9_ddr_perf.c | 93 ++++++++++++++++++++++++++++++--
>  1 file changed, 90 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> index 52234b97d0cb..a91267e2f5d8 100644
> --- a/drivers/perf/fsl_imx9_ddr_perf.c
> +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> @@ -17,9 +17,19 @@
>  #define MX93_PMCFG1_RD_BT_FILT_EN	BIT(29)
>  #define MX93_PMCFG1_ID_MASK		GENMASK(17, 0)
>  
> +#define MX95_PMCFG1_WR_BEAT_FILT_EN	BIT(31)
> +#define MX95_PMCFG1_RD_BEAT_FILT_EN	BIT(30)
> +
>  #define PMCFG2				0x04
>  #define MX93_PMCFG2_ID			GENMASK(17, 0)
>  
> +#define PMCFG3				0x08
> +#define PMCFG4				0x0C
> +#define PMCFG5				0x10
> +#define PMCFG6				0x14
> +#define MX95_PMCFG_ID_MASK		GENMASK(9, 0)
> +#define MX95_PMCFG_ID			GENMASK(25, 16)
> +
>  /* Global control register affects all counters and takes priority over local control registers */
>  #define PMGC0		0x40
>  /* Global control register bits */
> @@ -77,13 +87,23 @@ static const struct imx_ddr_devtype_data imx93_devtype_data = {
>  	.identifier = "imx93",
>  };
>  
> +static const struct imx_ddr_devtype_data imx95_devtype_data = {
> +	.identifier = "imx95",
> +};
> +
>  static inline bool is_imx93(struct ddr_pmu *pmu)
>  {
>  	return pmu->devtype_data == &imx93_devtype_data;
>  }
>  
> +static inline bool is_imx95(struct ddr_pmu *pmu)
> +{
> +	return pmu->devtype_data == &imx95_devtype_data;
> +}
> +
>  static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
> -	{.compatible = "fsl,imx93-ddr-pmu", .data = &imx93_devtype_data},
> +	{ .compatible = "fsl,imx93-ddr-pmu", .data = &imx93_devtype_data },
> +	{ .compatible = "fsl,imx95-ddr-pmu", .data = &imx95_devtype_data },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, imx_ddr_pmu_dt_ids);
> @@ -192,6 +212,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_ld_wiq_7, ID(2, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_empty, ID(2, 72)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_trans_filt, ID(2, 73)),	/* imx93 specific*/
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_beat_filt, ID(2, 73)),	/* imx95 specific*/
>  
>  	/* counter3 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_0, ID(3, 64)),
> @@ -204,6 +225,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_7, ID(3, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_full, ID(3, 72)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_trans_filt, ID(3, 73)),	/* imx93 specific*/
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt2, ID(3, 73)),	/* imx95 specific*/
>  
>  	/* counter4 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_0, ID(4, 64)),
> @@ -216,6 +238,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_7, ID(4, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_ld_rdq2_rmw, ID(4, 72)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt, ID(4, 73)),	/* imx93 specific*/
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt1, ID(4, 73)),	/* imx95 specific*/
>  
>  	/* counter5 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_0, ID(5, 64)),
> @@ -227,6 +250,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_6, ID(5, 70)),
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_7, ID(5, 71)),
>  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_ld_rdq1, ID(5, 72)),
> +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt0, ID(5, 73)),	/* imx95 specific*/
>  
>  	/* counter6 specific events */
>  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_end_0, ID(6, 64)),
> @@ -263,6 +287,13 @@ ddr_perf_events_attrs_is_visible(struct kobject *kobj,
>  		!is_imx93(ddr_pmu))
>  		return 0;
>  
> +	if ((!strcmp(attr->name, "eddrtq_pm_wr_beat_filt") ||
> +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt2") ||
> +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt1") ||
> +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt0")) &&
> +		!is_imx95(ddr_pmu))
> +		return 0;
> +
>  	return attr->mode;
>  }
>  
> @@ -434,6 +465,57 @@ static void imx93_ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
>  	writel_relaxed(pmcfg2, pmu->base + PMCFG2);
>  }
>  
> +static void imx95_ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> +					  int counter, int axi_id, int axi_mask)
> +{
> +	u32 pmcfg1, pmcfg, offset = 0;
> +
> +	pmcfg1 = readl_relaxed(pmu->base + PMCFG1);
> +
> +	if (event == 73) {
> +		switch (counter) {
> +		case 2:
> +			pmcfg1 |= MX95_PMCFG1_WR_BEAT_FILT_EN;
> +			offset = PMCFG3;
> +			break;
> +		case 3:
> +			pmcfg1 |= MX95_PMCFG1_RD_BEAT_FILT_EN;
> +			offset = PMCFG4;
> +			break;
> +		case 4:
> +			pmcfg1 |= MX95_PMCFG1_RD_BEAT_FILT_EN;
> +			offset = PMCFG5;
> +			break;
> +		case 5:
> +			pmcfg1 |= MX95_PMCFG1_RD_BEAT_FILT_EN;
> +			offset = PMCFG6;
> +			break;
> +		}
> +	} else {
> +		switch (counter) {
> +		case 2:
> +			pmcfg1 &= ~MX95_PMCFG1_WR_BEAT_FILT_EN;
> +			break;
> +		case 3:
> +		case 4:
> +		case 5:
> +			pmcfg1 &= ~MX95_PMCFG1_RD_BEAT_FILT_EN;
> +			break;
> +		}
> +	}

Look like only if event = 73, FILTER need be set.

How about

	pmcfg1 &= ~(MX95_PMCFG1_WR_BEAT_FILT_EN | MX95_PMCFG1_RD_BEAT_FILT_EN);
	if (event == 73)
		switch()
			...

So you need "else" branch.


> +
> +	writel_relaxed(pmcfg1, pmu->base + PMCFG1);
> +
> +	if (offset) {
> +		pmcfg = readl_relaxed(pmu->base + offset);
> +		pmcfg &= ~(FIELD_PREP(MX95_PMCFG_ID_MASK, 0x3FF) |
> +			   FIELD_PREP(MX95_PMCFG_ID, 0x3FF));
> +		pmcfg |= (FIELD_PREP(MX95_PMCFG_ID_MASK, axi_mask) |
> +			  FIELD_PREP(MX95_PMCFG_ID, axi_id));
> +		writel_relaxed(pmcfg, pmu->base + offset);
> +	}
> +}
> +
>  static void ddr_perf_event_update(struct perf_event *event)
>  {
>  	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> @@ -543,8 +625,13 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
>  	hwc->idx = counter;
>  	hwc->state |= PERF_HES_STOPPED;
>  
> -	/* read trans, write trans, read beat */
> -	imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> +	if (is_imx93(pmu))
> +		/* read trans, write trans, read beat */
> +		imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> +
> +	if (is_imx95(pmu))
> +		/* write beat, read beat2, read beat1, read beat */
> +		imx95_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
>  
>  	if (flags & PERF_EF_START)
>  		ddr_perf_event_start(event, flags);
> -- 
> 2.34.1
>
Frank Li March 7, 2024, 10:45 p.m. UTC | #5
On Thu, Mar 07, 2024 at 12:05:42PM -0500, Frank Li wrote:
> On Thu, Mar 07, 2024 at 05:57:29PM +0800, Xu Yang wrote:
> > i.MX95 has a DDR PMU which is almostly same as i.MX93, it now supports
> > read beat and write beat filter capabilities. This will add support for
> > i.MX95 and enhance the driver to support specific filter handling for it.
> > 
> > Usage:
> > 
> > For read beat:
> > ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt2,axi_mask=ID_MASK,axi_id=ID/
> > ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt1,axi_mask=ID_MASK,axi_id=ID/
> > ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt0,axi_mask=ID_MASK,axi_id=ID/
> > eg: For edma2: perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_rd_beat_filt0,axi_mask=0x00f,axi_id=0x00c/
> > 
> > For write beat:
> > ~# perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_wr_beat_filt,axi_mask=ID_MASK,axi_id=ID/
> > eg: For edma2: perf stat -a -I 1000 -e imx9_ddr0/eddrtq_pm_wr_beat_filt,axi_mask=0x00f,axi_id=0x00c/
> > 
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > 
> > ---
> > Changes in v2:
> >  - put soc spefific axi filter events to drvdata according
> >    to franks suggestions.
> >  - adjust pmcfg axi_id and axi_mask config
> > Changes in v3:
> >  - no changes
> > Changes in v4:
> >  - only contain imx95 parts
> > Changes in v5:
> >  - improve imx95_ddr_perf_monitor_config()
> >  - use write_relaxed to pair read_relaxed
> > Changes in v6:
> >  - no changes
> > ---
> >  drivers/perf/fsl_imx9_ddr_perf.c | 93 ++++++++++++++++++++++++++++++--
> >  1 file changed, 90 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> > index 52234b97d0cb..a91267e2f5d8 100644
> > --- a/drivers/perf/fsl_imx9_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> > @@ -17,9 +17,19 @@
> >  #define MX93_PMCFG1_RD_BT_FILT_EN	BIT(29)
> >  #define MX93_PMCFG1_ID_MASK		GENMASK(17, 0)
> >  
> > +#define MX95_PMCFG1_WR_BEAT_FILT_EN	BIT(31)
> > +#define MX95_PMCFG1_RD_BEAT_FILT_EN	BIT(30)
> > +
> >  #define PMCFG2				0x04
> >  #define MX93_PMCFG2_ID			GENMASK(17, 0)
> >  
> > +#define PMCFG3				0x08
> > +#define PMCFG4				0x0C
> > +#define PMCFG5				0x10
> > +#define PMCFG6				0x14
> > +#define MX95_PMCFG_ID_MASK		GENMASK(9, 0)
> > +#define MX95_PMCFG_ID			GENMASK(25, 16)
> > +
> >  /* Global control register affects all counters and takes priority over local control registers */
> >  #define PMGC0		0x40
> >  /* Global control register bits */
> > @@ -77,13 +87,23 @@ static const struct imx_ddr_devtype_data imx93_devtype_data = {
> >  	.identifier = "imx93",
> >  };
> >  
> > +static const struct imx_ddr_devtype_data imx95_devtype_data = {
> > +	.identifier = "imx95",
> > +};
> > +
> >  static inline bool is_imx93(struct ddr_pmu *pmu)
> >  {
> >  	return pmu->devtype_data == &imx93_devtype_data;
> >  }
> >  
> > +static inline bool is_imx95(struct ddr_pmu *pmu)
> > +{
> > +	return pmu->devtype_data == &imx95_devtype_data;
> > +}
> > +
> >  static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
> > -	{.compatible = "fsl,imx93-ddr-pmu", .data = &imx93_devtype_data},
> > +	{ .compatible = "fsl,imx93-ddr-pmu", .data = &imx93_devtype_data },
> > +	{ .compatible = "fsl,imx95-ddr-pmu", .data = &imx95_devtype_data },
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, imx_ddr_pmu_dt_ids);
> > @@ -192,6 +212,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_ld_wiq_7, ID(2, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_empty, ID(2, 72)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_trans_filt, ID(2, 73)),	/* imx93 specific*/
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_beat_filt, ID(2, 73)),	/* imx95 specific*/
> >  
> >  	/* counter3 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_0, ID(3, 64)),
> > @@ -204,6 +225,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_7, ID(3, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_full, ID(3, 72)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_trans_filt, ID(3, 73)),	/* imx93 specific*/
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt2, ID(3, 73)),	/* imx95 specific*/
> >  
> >  	/* counter4 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_0, ID(4, 64)),
> > @@ -216,6 +238,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_7, ID(4, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_ld_rdq2_rmw, ID(4, 72)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt, ID(4, 73)),	/* imx93 specific*/
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt1, ID(4, 73)),	/* imx95 specific*/
> >  
> >  	/* counter5 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_0, ID(5, 64)),
> > @@ -227,6 +250,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_6, ID(5, 70)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_7, ID(5, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_ld_rdq1, ID(5, 72)),
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt0, ID(5, 73)),	/* imx95 specific*/
> >  
> >  	/* counter6 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_end_0, ID(6, 64)),
> > @@ -263,6 +287,13 @@ ddr_perf_events_attrs_is_visible(struct kobject *kobj,
> >  		!is_imx93(ddr_pmu))
> >  		return 0;
> >  
> > +	if ((!strcmp(attr->name, "eddrtq_pm_wr_beat_filt") ||
> > +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt2") ||
> > +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt1") ||
> > +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt0")) &&
> > +		!is_imx95(ddr_pmu))
> > +		return 0;
> > +
> >  	return attr->mode;
> >  }
> >  
> > @@ -434,6 +465,57 @@ static void imx93_ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> >  	writel_relaxed(pmcfg2, pmu->base + PMCFG2);
> >  }
> >  
> > +static void imx95_ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> > +					  int counter, int axi_id, int axi_mask)
> > +{
> > +	u32 pmcfg1, pmcfg, offset = 0;
> > +
> > +	pmcfg1 = readl_relaxed(pmu->base + PMCFG1);
> > +
> > +	if (event == 73) {
> > +		switch (counter) {
> > +		case 2:
> > +			pmcfg1 |= MX95_PMCFG1_WR_BEAT_FILT_EN;
> > +			offset = PMCFG3;
> > +			break;
> > +		case 3:
> > +			pmcfg1 |= MX95_PMCFG1_RD_BEAT_FILT_EN;
> > +			offset = PMCFG4;
> > +			break;
> > +		case 4:
> > +			pmcfg1 |= MX95_PMCFG1_RD_BEAT_FILT_EN;
> > +			offset = PMCFG5;
> > +			break;
> > +		case 5:
> > +			pmcfg1 |= MX95_PMCFG1_RD_BEAT_FILT_EN;
> > +			offset = PMCFG6;
> > +			break;
> > +		}
> > +	} else {
> > +		switch (counter) {
> > +		case 2:
> > +			pmcfg1 &= ~MX95_PMCFG1_WR_BEAT_FILT_EN;
> > +			break;
> > +		case 3:
> > +		case 4:
> > +		case 5:
> > +			pmcfg1 &= ~MX95_PMCFG1_RD_BEAT_FILT_EN;
> > +			break;
> > +		}
> > +	}
> 
> Look like only if event = 73, FILTER need be set.
> 
> How about
> 
> 	pmcfg1 &= ~(MX95_PMCFG1_WR_BEAT_FILT_EN | MX95_PMCFG1_RD_BEAT_FILT_EN);
> 	if (event == 73)
> 		switch()
> 			...
> 
> So you need "else" branch.

Forget this one. My code have problem.

Frank

> 
> 
> > +
> > +	writel_relaxed(pmcfg1, pmu->base + PMCFG1);
> > +
> > +	if (offset) {
> > +		pmcfg = readl_relaxed(pmu->base + offset);
> > +		pmcfg &= ~(FIELD_PREP(MX95_PMCFG_ID_MASK, 0x3FF) |
> > +			   FIELD_PREP(MX95_PMCFG_ID, 0x3FF));
> > +		pmcfg |= (FIELD_PREP(MX95_PMCFG_ID_MASK, axi_mask) |
> > +			  FIELD_PREP(MX95_PMCFG_ID, axi_id));
> > +		writel_relaxed(pmcfg, pmu->base + offset);
> > +	}
> > +}
> > +
> >  static void ddr_perf_event_update(struct perf_event *event)
> >  {
> >  	struct ddr_pmu *pmu = to_ddr_pmu(event->pmu);
> > @@ -543,8 +625,13 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> >  	hwc->idx = counter;
> >  	hwc->state |= PERF_HES_STOPPED;
> >  
> > -	/* read trans, write trans, read beat */
> > -	imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> > +	if (is_imx93(pmu))
> > +		/* read trans, write trans, read beat */
> > +		imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> > +
> > +	if (is_imx95(pmu))
> > +		/* write beat, read beat2, read beat1, read beat */
> > +		imx95_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> >  
> >  	if (flags & PERF_EF_START)
> >  		ddr_perf_event_start(event, flags);
> > -- 
> > 2.34.1
> >
Xu Yang March 8, 2024, 3:30 a.m. UTC | #6
Hi John,

> 
> On 07/03/2024 09:57, Xu Yang wrote:
> > Add JSON metrics for i.MX95 DDR Performance Monitor.
> >
> > Reviewed-by: Ian Rogers<irogers@google.com>
> > Reviewed-by: Frank Li<Frank.Li@nxp.com>
> > Signed-off-by: Xu Yang<xu.yang_2@nxp.com>
> 
> FWIW, Please note that I gave a RB tag here:

Sorry, I forgot this. Will add this RB tag in next version.

Thanks,
Xu Yang
Xu Yang March 8, 2024, 3:36 a.m. UTC | #7
> 
> On Thu, Mar 07, 2024 at 05:57:25PM +0800, Xu Yang wrote:
> > The user can set event and counter in cmdline and the driver need to parse
> > it using 'config' attr value. This will add macro definitions to avoid
> > hard-code in driver.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >
> > ---
> > Changes in v4:
> >  - new patch
> > Changes in v5:
> >  - move this patch earlier
> > Changes in v6:
> >  - no changes
> > ---
> >  drivers/perf/fsl_imx9_ddr_perf.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> > index 9685645bfe04..d1c566e661d8 100644
> > --- a/drivers/perf/fsl_imx9_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> > @@ -42,6 +42,11 @@
> >  #define NUM_COUNTERS		11
> >  #define CYCLES_COUNTER		0
> >
> > +#define CONFIG_EVENT_MASK	0x00FF
> > +#define CONFIG_EVENT_OFFSET	0
> 
> Needn't need OFFSET if use FIELD_*

Okay, I will use FIELD_*.

> 
> > +#define CONFIG_COUNTER_MASK	0xFF00
> > +#define CONFIG_COUNTER_OFFSET	8
> 
> The same
> 
> > +
> >  #define to_ddr_pmu(p)		container_of(p, struct ddr_pmu, pmu)
> >
> >  #define DDR_PERF_DEV_NAME	"imx9_ddr"
> > @@ -339,8 +344,10 @@ static void ddr_perf_counter_local_config(struct ddr_pmu *pmu, int config,
> >  				    int counter, bool enable)
> >  {
> >  	u32 ctrl_a;
> > +	int event;
> >
> >  	ctrl_a = readl_relaxed(pmu->base + PMLCA(counter));
> > +	event = (config & CONFIG_EVENT_MASK) >> CONFIG_EVENT_OFFSET;
> 
> FIELD_GET(CONFIG_EVENT_MASK, config);
> 
> same below all code about config.

Okay.

Thanks,
Xu Yang

> 
> >
> >  	if (enable) {
> >  		ctrl_a |= PMLCA_FC;
> > @@ -352,7 +359,7 @@ static void ddr_perf_counter_local_config(struct ddr_pmu *pmu, int config,
> >  		ctrl_a &= ~PMLCA_FC;
> >  		ctrl_a |= PMLCA_CE;
> >  		ctrl_a &= ~FIELD_PREP(PMLCA_EVENT, 0x7F);
> > -		ctrl_a |= FIELD_PREP(PMLCA_EVENT, (config & 0x000000FF));
> > +		ctrl_a |= FIELD_PREP(PMLCA_EVENT, event);
> >  		writel(ctrl_a, pmu->base + PMLCA(counter));
> >  	} else {
> >  		/* Freeze counter. */
> > @@ -366,8 +373,8 @@ static void ddr_perf_monitor_config(struct ddr_pmu *pmu, int cfg, int cfg1, int
> >  	u32 pmcfg1, pmcfg2;
> >  	int event, counter;
> >
> > -	event = cfg & 0x000000FF;
> > -	counter = (cfg & 0x0000FF00) >> 8;
> > +	event = (cfg & CONFIG_EVENT_MASK) >> CONFIG_EVENT_OFFSET;
> > +	counter = (cfg & CONFIG_COUNTER_MASK) >> CONFIG_COUNTER_OFFSET;
> >
> >  	pmcfg1 = readl_relaxed(pmu->base + PMCFG1);
> >
> > @@ -469,7 +476,7 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> >  	int cfg2 = event->attr.config2;
> >  	int counter;
> >
> > -	counter = (cfg & 0x0000FF00) >> 8;
> > +	counter = (cfg & CONFIG_COUNTER_MASK) >> CONFIG_COUNTER_OFFSET;
> >
> >  	pmu->events[counter] = event;
> >  	pmu->active_events++;
> > --
> > 2.34.1
> >
Xu Yang March 8, 2024, 7:32 a.m. UTC | #8
> 
> On Thu, Mar 07, 2024 at 05:57:27PM +0800, Xu Yang wrote:
> > This driver is initinally used to support imx93 Soc and now it's time to
> > add support for imx95 Soc. However, some macro definitions and events are
> > different on these two Socs. For preparing imx95 supports, this will
> > refactor driver for imx93.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >
> > ---
> > Changes in v4:
> >  - new patch
> > Changes in v5:
> >  - use is_visible to hide unwanted attributes as suggested by Will
> > Changes in v6:
> >  - improve imx93_ddr_perf_monitor_config()
> > ---
> >  drivers/perf/fsl_imx9_ddr_perf.c | 99 +++++++++++++++++++++-----------
> >  1 file changed, 66 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> > index 8d85b4d98544..4e8a3a4349c5 100644
> > --- a/drivers/perf/fsl_imx9_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> > @@ -11,14 +11,14 @@
> >  #include <linux/perf_event.h>
> >
> >  /* Performance monitor configuration */
> > -#define PMCFG1  			0x00
> > -#define PMCFG1_RD_TRANS_FILT_EN 	BIT(31)
> > -#define PMCFG1_WR_TRANS_FILT_EN 	BIT(30)
> > -#define PMCFG1_RD_BT_FILT_EN 		BIT(29)
> > -#define PMCFG1_ID_MASK  		GENMASK(17, 0)
> > +#define PMCFG1				0x00
> > +#define MX93_PMCFG1_RD_TRANS_FILT_EN	BIT(31)
> > +#define MX93_PMCFG1_WR_TRANS_FILT_EN	BIT(30)
> > +#define MX93_PMCFG1_RD_BT_FILT_EN	BIT(29)
> > +#define MX93_PMCFG1_ID_MASK		GENMASK(17, 0)
> >
> > -#define PMCFG2  			0x04
> > -#define PMCFG2_ID			GENMASK(17, 0)
> > +#define PMCFG2				0x04
> > +#define MX93_PMCFG2_ID			GENMASK(17, 0)
> >
> >  /* Global control register affects all counters and takes priority over local control registers */
> >  #define PMGC0		0x40
> > @@ -77,6 +77,11 @@ static const struct imx_ddr_devtype_data imx93_devtype_data = {
> >  	.identifier = "imx93",
> >  };
> >
> > +static inline bool is_imx93(struct ddr_pmu *pmu)
> > +{
> > +	return pmu->devtype_data == &imx93_devtype_data;
> > +}
> > +
> >  static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
> >  	{.compatible = "fsl,imx93-ddr-pmu", .data = &imx93_devtype_data},
> >  	{ /* sentinel */ }
> > @@ -186,7 +191,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_ld_wiq_6, ID(2, 70)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_ld_wiq_7, ID(2, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_empty, ID(2, 72)),
> > -	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_trans_filt, ID(2, 73)),
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_trans_filt, ID(2, 73)),	/* imx93 specific*/
> >
> >  	/* counter3 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_0, ID(3, 64)),
> > @@ -198,7 +203,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_6, ID(3, 70)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_collision_7, ID(3, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_full, ID(3, 72)),
> > -	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_trans_filt, ID(3, 73)),
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_wr_trans_filt, ID(3, 73)),	/* imx93 specific*/
> >
> >  	/* counter4 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_0, ID(4, 64)),
> > @@ -210,7 +215,7 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_6, ID(4, 70)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_row_open_7, ID(4, 71)),
> >  	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pmon_ld_rdq2_rmw, ID(4, 72)),
> > -	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt, ID(4, 73)),
> > +	IMX9_DDR_PMU_EVENT_ATTR(eddrtq_pm_rd_beat_filt, ID(4, 73)),	/* imx93 specific*/
> >
> >  	/* counter5 specific events */
> >  	IMX9_DDR_PMU_EVENT_ATTR(ddrc_qx_valid_start_0, ID(5, 64)),
> > @@ -245,9 +250,26 @@ static struct attribute *ddr_perf_events_attrs[] = {
> >  	NULL,
> >  };
> >
> > +static umode_t
> > +ddr_perf_events_attrs_is_visible(struct kobject *kobj,
> > +				       struct attribute *attr, int unused)
> > +{
> > +	struct pmu *pmu = dev_get_drvdata(kobj_to_dev(kobj));
> > +	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> > +
> > +	if ((!strcmp(attr->name, "eddrtq_pm_rd_trans_filt") ||
> > +		!strcmp(attr->name, "eddrtq_pm_wr_trans_filt") ||
> > +		!strcmp(attr->name, "eddrtq_pm_rd_beat_filt")) &&
> > +		!is_imx93(ddr_pmu))
> > +		return 0;
> > +
> > +	return attr->mode;
> > +}
> > +
> >  static const struct attribute_group ddr_perf_events_attr_group = {
> >  	.name = "events",
> >  	.attrs = ddr_perf_events_attrs,
> > +	.is_visible = ddr_perf_events_attrs_is_visible,
> >  };
> >
> >  PMU_FORMAT_ATTR(event, "config:0-15");
> > @@ -369,36 +391,47 @@ static void ddr_perf_counter_local_config(struct ddr_pmu *pmu, int config,
> >  	}
> >  }
> >
> > -static void ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> > -				    int counter, int axi_id, int axi_mask)
> > +static void imx93_ddr_perf_monitor_config(struct ddr_pmu *pmu, int event,
> > +					  int counter, int axi_id, int axi_mask)
> >  {
> >  	u32 pmcfg1, pmcfg2;
> >
> >  	pmcfg1 = readl_relaxed(pmu->base + PMCFG1);
> >
> > -	if (counter == 2 && event == 73)
> > -		pmcfg1 |= PMCFG1_RD_TRANS_FILT_EN;
> > -	else if (counter == 2 && event != 73)
> > -		pmcfg1 &= ~PMCFG1_RD_TRANS_FILT_EN;
> > -
> > -	if (counter == 3 && event == 73)
> > -		pmcfg1 |= PMCFG1_WR_TRANS_FILT_EN;
> > -	else if (counter == 3 && event != 73)
> > -		pmcfg1 &= ~PMCFG1_WR_TRANS_FILT_EN;
> > -
> > -	if (counter == 4 && event == 73)
> > -		pmcfg1 |= PMCFG1_RD_BT_FILT_EN;
> > -	else if (counter == 4 && event != 73)
> > -		pmcfg1 &= ~PMCFG1_RD_BT_FILT_EN;
> > +	if (event == 73) {
> > +		switch (counter) {
> > +		case 2:
> > +			pmcfg1 |= MX93_PMCFG1_RD_TRANS_FILT_EN;
> > +			break;
> > +		case 3:
> > +			pmcfg1 |= MX93_PMCFG1_WR_TRANS_FILT_EN;
> > +			break;
> > +		case 4:
> > +			pmcfg1 |= MX93_PMCFG1_RD_BT_FILT_EN;
> > +			break;
> > +		}
> > +	} else {
> > +		switch (counter) {
> > +		case 2:
> > +			pmcfg1 &= ~MX93_PMCFG1_RD_TRANS_FILT_EN;
> > +			break;
> > +		case 3:
> > +			pmcfg1 &= ~MX93_PMCFG1_WR_TRANS_FILT_EN;
> > +			break;
> > +		case 4:
> > +			pmcfg1 &= ~MX93_PMCFG1_RD_BT_FILT_EN;
> > +			break;
> > +		}
> > +	}
> 
> how about
> 	u32 mask[] = { 	MX93_PMCFG1_RD_TRANS_FILT_EN,
> 			MX93_PMCFG1_WR_TRANS_FILT_EN,
> 			MX93_PMCFG1_RD_BT_FILT_EN
> 	      	     };
> 
> 	if (couter >=2 && counter <= 4)
> 		event == 73 ? pmcfg1 |= mask[counter - 2] :
> 			      pmcfg1 &= ~mask[counter - 2];

It seems ok for me. Will try this.

Thanks,
Xu Yang

> 
> 
> Frank
> 
> >
> > -	pmcfg1 &= ~FIELD_PREP(PMCFG1_ID_MASK, 0x3FFFF);
> > -	pmcfg1 |= FIELD_PREP(PMCFG1_ID_MASK, axi_mask);
> > -	writel(pmcfg1, pmu->base + PMCFG1);
> > +	pmcfg1 &= ~FIELD_PREP(MX93_PMCFG1_ID_MASK, 0x3FFFF);
> > +	pmcfg1 |= FIELD_PREP(MX93_PMCFG1_ID_MASK, axi_mask);
> > +	writel_relaxed(pmcfg1, pmu->base + PMCFG1);
> >
> >  	pmcfg2 = readl_relaxed(pmu->base + PMCFG2);
> > -	pmcfg2 &= ~FIELD_PREP(PMCFG2_ID, 0x3FFFF);
> > -	pmcfg2 |= FIELD_PREP(PMCFG2_ID, axi_id);
> > -	writel(pmcfg2, pmu->base + PMCFG2);
> > +	pmcfg2 &= ~FIELD_PREP(MX93_PMCFG2_ID, 0x3FFFF);
> > +	pmcfg2 |= FIELD_PREP(MX93_PMCFG2_ID, axi_id);
> > +	writel_relaxed(pmcfg2, pmu->base + PMCFG2);
> >  }
> >
> >  static void ddr_perf_event_update(struct perf_event *event)
> > @@ -514,7 +547,7 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> >  		ddr_perf_event_start(event, flags);
> >
> >  	/* read trans, write trans, read beat */
> > -	ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> > +	imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> >
> >  	return 0;
> >  }
> > --
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/perf/fsl-imx-ddr.yaml b/Documentation/devicetree/bindings/perf/fsl-imx-ddr.yaml
index 6c96a4204e5d..37e8b98f2cdc 100644
--- a/Documentation/devicetree/bindings/perf/fsl-imx-ddr.yaml
+++ b/Documentation/devicetree/bindings/perf/fsl-imx-ddr.yaml
@@ -30,6 +30,9 @@  properties:
       - items:
           - const: fsl,imx8dxl-ddr-pmu
           - const: fsl,imx8-ddr-pmu
+      - items:
+          - const: fsl,imx95-ddr-pmu
+          - const: fsl,imx93-ddr-pmu
 
   reg:
     maxItems: 1