Re: [PATCH 1/2] coresight: etm4x: Report whether the PMU counts external output 1

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

Hi Amir,

Thanks for the patch. I have a few initial comments below. We may have
further feedback after our internal review.

On Wed, Sep 30, 2026 at 11:37:43PM -0700, Amir Ayupov wrote:

[...]

>  #define CS_CFG_MATCH_CLASS_SRC_ALL	0x0001	/* match any source */
>  #define CS_CFG_MATCH_CLASS_SRC_ETM4	0x0002	/* match any ETMv4 device */
> +/* ETM external output 1 is countable by the PMU as TRCEXTOUT1 */
> +#define CS_CFG_MATCH_CAP_PMU_EXTOUT1	0x0004

Do we need to tie this capability to TRCEXTOUT1? For example, Neoverse
V2 exposes TRCEXTOUT0 through TRCEXTOUT3 as PMU events.

>  /* flags defining device instance matching - used in config match desc data. */
>  #define CS_CFG_MATCH_INST_ANY		0x80000000 /* any instance of a class */
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> index e1a59b4345052..2847d2d7f7bed 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c
> @@ -174,9 +174,14 @@ static int etm4_cfg_load_feature(struct coresight_device *csdev,
>  
>  int etm4_cscfg_register(struct coresight_device *csdev)
>  {
> +	struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>  	struct cscfg_csdev_feat_ops ops;
> +	u32 match_flags = CS_CFG_ETM4_MATCH_FLAGS;
>  
>  	ops.load_feat = &etm4_cfg_load_feature;
>  
> -	return cscfg_register_csdev(csdev, CS_CFG_ETM4_MATCH_FLAGS, &ops);
> +	if (drvdata->pmu_extout1)
> +		match_flags |= CS_CFG_MATCH_CAP_PMU_EXTOUT1;
> +
> +	return cscfg_register_csdev(csdev, match_flags, &ops);

The current matching logic succeeds if a device and feature share any
flag bit. If the feature sets both CS_CFG_MATCH_CLASS_SRC_ETM4 and
CS_CFG_MATCH_CAP_PMU_EXTOUT1, it will still load on an ETM4 device that
lacks the capability.

Could we check the capability separately when loading the feature,
perhaps in cscfg_load_feat_csdev(), and skip it on unsupported devices?

> +static bool etm4_pmu_has_extout1(struct etmv4_drvdata *drvdata)
> +{
> +	int pmuver = read_pmuver();
> +
> +	if (!is_midr_in_range_list(etm4_pmu_extout1_cpus))
> +		return false;

Based on specific CPU variant, we should already have identified
TRCEXTOUT has supported.

So either we only base on MIDR list or we can figure out a reliable
way to detect the feature dynamically.

> +
> +	/* PMCEID0_EL0[63:32] describe events 0x4000-0x401f from PMUv3p1 */
> +	if (!pmuv3_implemented(pmuver) || pmuver < ID_AA64DFR0_EL1_PMUVer_V3P1)
> +		return false;
> +	if (!(read_pmceid0() & BIT_ULL(32 + ARMV8_PMUV3_PERFCTR_TRCEXTOUT1 -
> +				       ARMV8_PMUV3_EXT_COMMON_EVENT_BASE)))
> +		return false;
> +
> +	/* nr_event is TRCIDR0.NUMEVENT, the number of events minus one */
> +	return drvdata->nr_event >= 1;
> +}

[...]

> @@ -1116,6 +1116,13 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
>  
>  		if (err)
>  			cscfg_config_desc_put(config_desc);
> +	} else {
> +		/*
> +		 * The configuration is active but was not loaded on this
> +		 * device, for example because the device lacks a capability
> +		 * its features require. Fail rather than trace without it.
> +		 */
> +		err = -EINVAL;
>  	}

This fixes a pre-existing issue. It is worther to put it in a separate
patch with fixes tag:

An early return would also avoid the normal enable path's indentation:

	if (!config_csdev_active)
		return -EINVAL;

	err = cscfg_csdev_enable_config(config_csdev_active, preset);
	...

Thanks,
Leo