Re: [PATCH 5/5] perf/arm-cmn: Support CMN S3 r2

[Leo Yan <leo.yan@arm.com>]

On Tue, Jun 30, 2026 at 04:19:20PM +0100, Robin Murphy wrote:

[...]

> @@ -913,7 +956,12 @@ static umode_t arm_cmn_event_attr_is_visible(struct kobject *kobj,
>  	CMN_EVENT_ATTR(_model, _name##_group1_read, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 3), \
>  	CMN_EVENT_ATTR(_model, _name##_group1_write, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 4), \
>  	CMN_EVENT_ATTR(_model, _name##_read, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 5), \
> -	CMN_EVENT_ATTR(_model, _name##_write, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 6)
> +	CMN_EVENT_ATTR(_model, _name##_write, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 6), \
> +	CMN_EVENT_ATTR(CMNS3R2, _name##_ccg_read, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 9), \
> +	CMN_EVENT_ATTR(CMNS3R2, _name##_ccg_write, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 10), \
> +	CMN_EVENT_ATTR(CMNS3R2, _name##_lbt_read, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 11), \
> +	CMN_EVENT_ATTR(CMNS3R2, _name##_lbt_write, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 12), \
> +	CMN_EVENT_ATTR(CMNS3R2, _name##_lbt, _type, _event, SEL_CBUSY_SNTHROTTLE_SEL, 13)

If these CMN_EVENT_ATTR(CMNS3R2, ...) entries are appended to
CMN_EVENT_HN_SNT(), they will extend the CMNS3R2 attributes for other
models as well. For example:

  CMN_EVENT_HNF_SNT(CMN700, sn_throttle, 0x2a),

This would also add the CMNS3R2 attributes to CMN700. Is this intended?

> +#define CMN_EVENT_HNS_OCC(_model, _name, _event)			\
> +	CMN_EVENT_HN_OCC(_model, hns_##_name, CMN_TYPE_HNS, _event),	\
> +	_CMN_EVENT_HNS(_model, _name##_rxsnp, _event, SEL_OCCUP1_ID, 5), \
> +	_CMN_EVENT_HNS(_model, _name##_lbt, _event, SEL_OCCUP1_ID, 6),	\
> +	_CMN_EVENT_HNS(_model, _name##_hbt, _event, SEL_OCCUP1_ID, 7),	\
> +	_CMN_EVENT_HNS(CMNS3R2, _name##_rnf, _event, SEL_OCCUP1_ID, 8),	\
> +	_CMN_EVENT_HNS(CMNS3R2, _name##_rni, _event, SEL_OCCUP1_ID, 9),	\
> +	_CMN_EVENT_HNS(CMNS3R2, _name##_ccglcn, _event, SEL_OCCUP1_ID, 10), \
> +	_CMN_EVENT_HNS(CMNS3R2, _name##_ccgrn, _event, SEL_OCCUP1_ID, 11)

I have a similar question here. My impression is that this is mainly for
the convenience of appending CMNS3R2 specific attributes, but it doesn't
necessarily mean those attributes should be added for every model.

> @@ -1288,65 +1388,72 @@ static struct attribute *arm_cmn_event_attrs[] = {

> +	CMN_EVENT_HNS_HBT_ENHBT(cache_miss,		0x01),

I manually extend the macro and got the result:

  CMN_EVENT_ATTR(CMN700 | CMNS3R01, hns_cache_miss_all, CMN_TYPE_HNS, 0x1, SEL_HBT_LBT_SEL, 0),
  CMN_EVENT_ATTR(CMN700 | CMNS3R01, hns_cache_miss_hbt, CMN_TYPE_HNS, 0x1, SEL_HBT_LBT_SEL, 1),
  CMN_EVENT_ATTR(CMN700 | CMNS3R01, hns_cache_miss_lbt, CMN_TYPE_HNS, 0x1, SEL_HBT_LBT_SEL, 2),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_all, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 0),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_hbt, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 1),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_lbt, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 2),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_rnf, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 3),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_rni, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 4),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_ccglcn, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 5),
  CMN_EVENT_ATTR(CMNS3R2, hns_cache_miss_ccgrn, CMN_TYPE_HNS, 0x1, SEL_ENHANCED_HBT_LBT_SEL, 6),

CMNS3R2 has added items like rnf/rni/ccglcn/ccgrn, not sure if this is
purposed or not ...

> @@ -2431,21 +2538,33 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>  	/*
>  	 * 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
> -	 * it; however we also have no way to tell from Non-Secure whether any
> -	 * given port is disabled or not, so the only way to win is not to play...
> +	 * it; however prior to CMN S3 r2p0 we also have no way to tell from
> +	 * Non-Secure whether any given port is disabled or not, so in that case
> +	 * the only way to win is not to play...
>  	 */
>  	reg = readq_relaxed(cfg_region + CMN_CFGM_INFO_GLOBAL);
> -	if (reg & CMN_INFO_DEVICE_ISO_ENABLE) {
> +	if (reg & CMN_INFO_DEVICE_ISO_ENABLE && model == CMNS3R01) {

As the comment claims "prior to CMN S3 r2p0", would here be:

  if (reg & CMN_INFO_DEVICE_ISO_ENABLE && model < CMNS3R2) {

>  		dev_err(cmn->dev, "Device isolation enabled, not continuing due to risk of lockup\n");
>  		return -ENODEV;
>  	}

Thanks,
Leo