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

[James Clark <james.clark@linaro.org>]

On 07/10/2026 17:24, Leo Yan wrote:
> 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.
> 

Shouldn't we also use TRCEXTOUT0 instead of 1? Isn't 1 for devices that 
have two external outputs, but some devices might only have 1 output so 
only have TRCEXTOUT0?

>>   /* 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.
> 

At least with ETE the ARM says:

  D4.6.12  External Outputs

  SRBKWBThe   TRCIDR0.NUMEVENT field shows how many ETEEvents are
              for the particular implementation

  0x4011, TRCEXTOUT1, Trace unit external output 1


  D14 PMU Event Descriptions

  The counter counts each event signaled by the trace unit on external
  event 1.

  It is IMPLEMENTATION DEFINED whether this event is available as an
  external input to the ETE.

  PMCEID0_EL0[49] reads as 1 if this event is implemented and 0
  otherwise.

The number of outputs and the PMU event are both discoverable. It 
specifically says that only the external input is implementation 
defined, implying that if it's available it's always connected as an output.

We could leave the MIDR list to only support errata when the external 
output isn't connected to the PMU event.



>> +
>> +	/* 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