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

[Amir Ayupov <aaupov@fb.com>]

On Thu, 8 Oct 2026 09:38:52 +0100 James Clark <james.clark@linaro.org> wrote:

Hi James,

Please see answers inline:

> 
> 
> 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?
> 

IIUC we can't because output 0 is driven by event 0, which stops tracing:
CoreSight Base System Architecture, RMHKBV (second bullet)

> For at least one of the events implemented in the trace unit, when this event occurs, any trace buffer which
> captures trace from this trace unit must be able to detect the event has occurred, and must be able to use this
> event to stop trace capture. One of the following mechanisms must be used:
> • Use of the trigger trace source ID value 0x7D as defined in [12], and use of the ATB Trigger Enable
> function in the trace unit.
> • Use of a dedicated mechanism when Event 0 occurs in the trace unit.


> >>   /* 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.
> 
> 
> 
I searched for TRCEXTOUT in errata notices and couldn't find any, so I'm
dropping the list in v2. Feel free to add it in the future.

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

Sent using hkml (https://github.com/sjp38/hackermail)