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

[Amir Ayupov <aaupov@fb.com>]

Hi Leo,

Thank you for the review. See responses inline:

On Wed, 7 Oct 2026 17:24:06 +0100 Leo Yan <leo.yan@arm.com> 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.
> 

No, don't need to tie to 1, can use any one from 1 to 3.

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

ack, will do in v2.

> > +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.
> 

Agreed, fixed in v2.
> > +
> > +	/* 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

Will split out in v2.

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