Re: [PATCH 2/5] perf/arm-cmn: Refactor event filter programming

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

On Tue, 30 Jun 2026, Robin Murphy wrote:

> We're soon going to need to cope with events having multiple filters,
> plus the filter fields themselves moving around, wherein any more inline
> if/else logic will struggle to scale. Add a more general abstraction for
> the node-specific filter controls, and rejig the pmu_event_sel filter
> programming around it in a more extensible manner.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Looks good to me

Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>

Cheers, Ilkka

> ---
> drivers/perf/arm-cmn.c | 139 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 101 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 2a8a67da72c3..14c267d2e2f9 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -278,8 +278,8 @@ enum cmn_node_type {
> };
>
> enum cmn_filter_select {
> -	SEL_NONE = -1,
> -	SEL_OCCUP1ID,
> +	SEL_NONE,
> +	SEL_OCCUP1_ID,
> 	SEL_CLASS_OCCUP_ID,
> 	SEL_CBUSY_SNTHROTTLE_SEL,
> 	SEL_HBT_LBT_SEL,
> @@ -599,6 +599,57 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id)
> static void arm_cmn_debugfs_init(struct arm_cmn *cmn, int id) {}
> #endif
>
> +enum cmn_filter_type {
> +	FILT_NONE,
> +	FILT_OCCUP1_ID,
> +	FILT_HNF_700,
> +	FILT_HNS,
> +};
> +#define CMN_FILTER(_sel)	[SEL_##_sel] = CMN__PMU_##_sel
> +
> +static const u64 arm_cmn_filters[][SEL_MAX] = {
> +	[FILT_NONE] = {},
> +	/* DVM etc. */
> +	[FILT_OCCUP1_ID] = {
> +		CMN_FILTER(OCCUP1_ID)
> +	},
> +	/* Newer HN-F */
> +	[FILT_HNF_700] = {
> +		CMN_FILTER(OCCUP1_ID),
> +		CMN_FILTER(CLASS_OCCUP_ID),
> +		CMN_FILTER(CBUSY_SNTHROTTLE_SEL)
> +	},
> +	/* HN-S */
> +	[FILT_HNS] = {
> +		CMN_FILTER(OCCUP1_ID),
> +		CMN_FILTER(CLASS_OCCUP_ID),
> +		CMN_FILTER(CBUSY_SNTHROTTLE_SEL),
> +		CMN_FILTER(HBT_LBT_SEL),
> +		CMN_FILTER(SN_HOME_SEL)
> +	},
> +};
> +
> +static enum cmn_filter_type arm_cmn_filter(enum cmn_node_type node,
> +					   enum cmn_model model)
> +{
> +	switch (node) {
> +	default:
> +		return FILT_NONE;
> +	case CMN_TYPE_DVM:
> +	case CMN_TYPE_CXRA:
> +	case CMN_TYPE_CXHA:
> +	case CMN_TYPE_CCRA:
> +	case CMN_TYPE_CCHA:
> +		return FILT_OCCUP1_ID;
> +	case CMN_TYPE_HNF:
> +		if (model < CMN700)
> +			return FILT_OCCUP1_ID;
> +		return FILT_HNF_700;
> +	case CMN_TYPE_HNS:
> +		return FILT_HNS;
> +	};
> +}
> +
> struct arm_cmn_hw_event {
> 	struct arm_cmn_node *dn;
> 	union {
> @@ -708,7 +759,7 @@ static ssize_t arm_cmn_event_show(struct device *dev,
> 				  "type=0x%x,eventid=0x%x,wp_dev_sel=?,wp_chn_sel=?,wp_grp=?,wp_val=?,wp_mask=?\n",
> 				  eattr->type, eattr->eventid);
>
> -	if (eattr->fsel > SEL_NONE)
> +	if (eattr->fsel)
> 		return sysfs_emit(buf, "type=0x%x,eventid=0x%x,filter=0x%x\n",
> 				  eattr->type, eattr->eventid, eattr->filter);
>
> @@ -832,16 +883,16 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
> #define CMN_EVENT_DVM(_model, _name, _event)			\
> 	_CMN_EVENT_DVM(_model, _name, _event, 0, SEL_NONE)
> #define CMN_EVENT_DVM_OCC(_model, _name, _event)			\
> -	_CMN_EVENT_DVM(_model, _name##_all, _event, 0, SEL_OCCUP1ID),	\
> -	_CMN_EVENT_DVM(_model, _name##_dvmop, _event, 1, SEL_OCCUP1ID),	\
> -	_CMN_EVENT_DVM(_model, _name##_dvmsync, _event, 2, SEL_OCCUP1ID)
> +	_CMN_EVENT_DVM(_model, _name##_all, _event, 0, SEL_OCCUP1_ID),	\
> +	_CMN_EVENT_DVM(_model, _name##_dvmop, _event, 1, SEL_OCCUP1_ID),	\
> +	_CMN_EVENT_DVM(_model, _name##_dvmsync, _event, 2, SEL_OCCUP1_ID)
>
> #define CMN_EVENT_HN_OCC(_model, _name, _type, _event)		\
> -	_CMN_EVENT_ATTR(_model, _name##_all, _type, _event, 0, SEL_OCCUP1ID), \
> -	_CMN_EVENT_ATTR(_model, _name##_read, _type, _event, 1, SEL_OCCUP1ID), \
> -	_CMN_EVENT_ATTR(_model, _name##_write, _type, _event, 2, SEL_OCCUP1ID), \
> -	_CMN_EVENT_ATTR(_model, _name##_atomic, _type, _event, 3, SEL_OCCUP1ID), \
> -	_CMN_EVENT_ATTR(_model, _name##_stash, _type, _event, 4, SEL_OCCUP1ID)
> +	_CMN_EVENT_ATTR(_model, _name##_all, _type, _event, 0, SEL_OCCUP1_ID), \
> +	_CMN_EVENT_ATTR(_model, _name##_read, _type, _event, 1, SEL_OCCUP1_ID), \
> +	_CMN_EVENT_ATTR(_model, _name##_write, _type, _event, 2, SEL_OCCUP1_ID), \
> +	_CMN_EVENT_ATTR(_model, _name##_atomic, _type, _event, 3, SEL_OCCUP1_ID), \
> +	_CMN_EVENT_ATTR(_model, _name##_stash, _type, _event, 4, SEL_OCCUP1_ID)
> #define CMN_EVENT_HN_CLS(_model, _name, _type, _event)			\
> 	_CMN_EVENT_ATTR(_model, _name##_class0, _type, _event, 0, SEL_CLASS_OCCUP_ID), \
> 	_CMN_EVENT_ATTR(_model, _name##_class1, _type, _event, 1, SEL_CLASS_OCCUP_ID), \
> @@ -865,9 +916,9 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
>
> #define CMN_EVENT_HNS_OCC(_name, _event)				\
> 	CMN_EVENT_HN_OCC(CMN_ANY, hns_##_name, CMN_TYPE_HNS, _event),	\
> -	_CMN_EVENT_ATTR(CMN_ANY, hns_##_name##_rxsnp, CMN_TYPE_HNS, _event, 5, SEL_OCCUP1ID), \
> -	_CMN_EVENT_ATTR(CMN_ANY, hns_##_name##_lbt, CMN_TYPE_HNS, _event, 6, SEL_OCCUP1ID), \
> -	_CMN_EVENT_ATTR(CMN_ANY, hns_##_name##_hbt, CMN_TYPE_HNS, _event, 7, SEL_OCCUP1ID)
> +	_CMN_EVENT_ATTR(CMN_ANY, hns_##_name##_rxsnp, CMN_TYPE_HNS, _event, 5, SEL_OCCUP1_ID), \
> +	_CMN_EVENT_ATTR(CMN_ANY, hns_##_name##_lbt, CMN_TYPE_HNS, _event, 6, SEL_OCCUP1_ID), \
> +	_CMN_EVENT_ATTR(CMN_ANY, hns_##_name##_hbt, CMN_TYPE_HNS, _event, 7, SEL_OCCUP1_ID)
> #define CMN_EVENT_HNS_CLS( _name, _event)				\
> 	CMN_EVENT_HN_CLS(CMN_ANY, hns_##_name, CMN_TYPE_HNS, _event)
> #define CMN_EVENT_HNS_SNT(_name, _event)				\
> @@ -1547,27 +1598,20 @@ static void arm_cmn_event_read(struct perf_event *event)
> }
>
> static int arm_cmn_set_event_sel_hi(struct arm_cmn_node *dn,
> -				    enum cmn_filter_select fsel, u8 filter)
> +				    enum cmn_filter_select fsel, u8 val)
> {
> -	u64 reg;
> -
> -	if (fsel == SEL_NONE)
> -		return 0;
> -
> 	if (!dn->filter[fsel].count) {
> -		dn->filter[fsel].val = filter;
> -		reg = FIELD_PREP(CMN__PMU_CBUSY_SNTHROTTLE_SEL,
> -				 dn->filter[SEL_CBUSY_SNTHROTTLE_SEL].val) |
> -		      FIELD_PREP(CMN__PMU_SN_HOME_SEL,
> -				 dn->filter[SEL_SN_HOME_SEL].val) |
> -		      FIELD_PREP(CMN__PMU_HBT_LBT_SEL,
> -				 dn->filter[SEL_HBT_LBT_SEL].val) |
> -		      FIELD_PREP(CMN__PMU_CLASS_OCCUP_ID,
> -				 dn->filter[SEL_CLASS_OCCUP_ID].val) |
> -		      FIELD_PREP(CMN__PMU_OCCUP1_ID,
> -				 dn->filter[SEL_OCCUP1ID].val);
> +		const u64 *filter = arm_cmn_filters[dn->filter[SEL_NONE].val];
> +		u64 reg = 0;
> +
> +		dn->filter[fsel].val = val;
> +		for (int i = SEL_OCCUP1_ID; i < SEL_MAX; i++) {
> +			if (filter[i])
> +				reg |= field_prep(filter[i], dn->filter[i].val);
> +		}
> +
> 		writel_relaxed(reg >> 32, dn->pmu_base + CMN_PMU_EVENT_SEL + 4);
> -	} else if (dn->filter[fsel].val != filter) {
> +	} else if (dn->filter[fsel].val != val) {
> 		return -EBUSY;
> 	}
> 	dn->filter[fsel].count++;
> @@ -1696,7 +1740,7 @@ static void arm_cmn_val_add_event(struct arm_cmn *cmn, struct arm_cmn_val *val,
>
> 		val->dtm_count[dtm]++;
>
> -		if (sel > SEL_NONE)
> +		if (sel)
> 			val->filter[dtm][sel] = CMN_EVENT_FILTER(event) + 1;
>
> 		if (type != CMN_TYPE_WP)
> @@ -1748,7 +1792,7 @@ static int arm_cmn_validate_group(struct arm_cmn *cmn, struct perf_event *event)
> 		if (val->dtm_count[dtm] == CMN_DTM_NUM_COUNTERS)
> 			goto done;
>
> -		if (sel > SEL_NONE && val->filter[dtm][sel] &&
> +		if (sel && val->filter[dtm][sel] &&
> 		    val->filter[dtm][sel] != CMN_EVENT_FILTER(event) + 1)
> 			goto done;
>
> @@ -1894,7 +1938,7 @@ static void arm_cmn_event_clear(struct arm_cmn *cmn, struct perf_event *event,
> 			dtm->wp_event[wp_idx] = -1;
> 		}
>
> -		if (hw->filter_sel > SEL_NONE)
> +		if (hw->filter_sel)
> 			hw->dn[i].filter[hw->filter_sel].count--;
>
> 		dtm->pmu_config_low &= ~CMN__PMEVCNT_PAIRED(dtm_idx);
> @@ -1906,6 +1950,17 @@ static void arm_cmn_event_clear(struct arm_cmn *cmn, struct perf_event *event,
> 		cmn->dtc[j].counters[idx] = NULL;
> }
>
> +static int arm_cmn_set_event_filter(struct arm_cmn_node *dn, struct perf_event *event)
> +{
> +	enum cmn_filter_select fsel = to_cmn_hw(event)->filter_sel;
> +	int ret = 0;
> +
> +	if (fsel)
> +		ret = arm_cmn_set_event_sel_hi(dn, fsel, CMN_EVENT_FILTER(event));
> +
> +	return ret;
> +}
> +
> static int arm_cmn_event_add(struct perf_event *event, int flags)
> {
> 	struct arm_cmn *cmn = to_cmn(event->pmu);
> @@ -1981,7 +2036,7 @@ static int arm_cmn_event_add(struct perf_event *event, int flags)
> 			input_sel = CMN__PMEVCNT0_INPUT_SEL_DEV + dtm_idx +
> 				    (nid.port << 4) + (nid.dev << 2);
>
> -			if (arm_cmn_set_event_sel_hi(dn, hw->filter_sel, CMN_EVENT_FILTER(event)))
> +			if (arm_cmn_set_event_filter(dn, event))
> 				goto free_dtms;
> 		}
>
> @@ -2311,6 +2366,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 	void __iomem *cfg_region, __iomem *xp_region;
> 	struct arm_cmn_node cfg, *dn;
> 	struct arm_cmn_dtm *dtm;
> +	enum cmn_model model;
> 	enum cmn_part part;
> 	u16 child_count, child_poff;
> 	u64 reg;
> @@ -2342,12 +2398,14 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 			 "Firmware binding mismatch: expected part number 0x%x, found 0x%x\n",
> 			 cmn->part, part);
> 	cmn->part = part;
> -	if (!arm_cmn_model(cmn))
> -		dev_warn(cmn->dev, "Unknown part number: 0x%x\n", part);
>
> 	reg = readl_relaxed(cfg_region + CMN_CFGM_PERIPH_ID_23);
> 	cmn->rev = FIELD_GET(CMN_CFGM_PID2_REVISION, reg);
>
> +	model = arm_cmn_model(cmn);
> +	if (!model)
> +		dev_warn(cmn->dev, "Unknown part number: 0x%x\n", part);
> +
> 	/*
> 	 * With the device isolation feature, if firmware has neglected to enable
> 	 * an XP port then we risk locking up if we try to access anything behind
> @@ -2500,6 +2558,11 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
> 				dev_err(cmn->dev, "Node ID invalid for supported CMN versions: %d\n", dn->logid);
> 				return -ENODEV;
> 			}
> +			/*
> +			 * We can utilise the "wasted" filter array slot to store
> +			 * the index for referencing the filter encodings later.
> +			 */
> +			dn->filter[SEL_NONE].val = arm_cmn_filter(dn->type, model);
>
> 			switch (dn->type) {
> 			case CMN_TYPE_DTC:
> -- 
> 2.54.0.dirty
>
>