Re: [PATCH v2 5/6] coresight: Add format attribute for setting the timestamp interval

[James Clark]

On 30/09/2025 4:14 pm, Leo Yan wrote:
> On Thu, Aug 14, 2025 at 11:49:56AM +0100, James Clark wrote:
>
> [...]
>
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -25,6 +25,11 @@
> >   #include "coresight-syscfg.h"
> >   #include "coresight-trace-id.h"
> >   
> > +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> > +#include <linux/perf/arm_pmu.h>
> > +#include "coresight-etm4x.h"
> > +#endif
> > +
> >   static struct pmu etm_pmu;
> >   static bool etm_perf_up;
> >   
> > @@ -69,7 +74,10 @@ PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
> >   /* config ID - set if a system configuration is selected */
> >   PMU_FORMAT_ATTR(configid,	"config2:32-63");
> >   PMU_FORMAT_ATTR(cc_threshold,	"config3:0-11");
> > -
> > +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> > +/* Interval = (2 ^ ts_level) */
> > +GEN_PMU_FORMAT_ATTR(ts_level);
> > +#endif
> >   
> >   /*
> >    * contextid always traces the "PID".  The PID is in CONTEXTIDR_EL1
> > @@ -103,6 +111,9 @@ static struct attribute *etm_config_formats_attr[] = {
> >   	&format_attr_configid.attr,
> >   	&format_attr_branch_broadcast.attr,
> >   	&format_attr_cc_threshold.attr,
> > +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> > +	&format_attr_ts_level.attr,
> > +#endif
>
> By using .visible() callback for attrs, we can improve a bit code
> without spreading "#ifdef IS_ENABLED()" in this file. E.g.,
>
>     static umode_t format_attr_is_visible(struct kobject *kobj,
>                                     struct attribute *attr, int n)
>     {
>          struct device *dev = kobj_to_dev(kobj);
>
>          if (attr == &format_attr_ts_level.attr &&
> 	    !IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
>                  return 0;
>
>          return attr->mode;
>     }
>
> Otherwise, LGTM:
>
> Reviewed-by: Leo Yan <leo.yan@xxxxxxx>

Unfortunately that won't work because you'd have to always include 
coresight-etm4x.h. This file is compiled for both arm32 and arm64 so it 
would break the arm32 build.

I could define the TTR_CFG_FLD_ts_level_* stuff somewhere else but then 
it becomes messier than just doing the #ifdefs here.

> >   	NULL,
> >   };
> >   
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 1a2d02bdcb88..42277c201d4f 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -28,6 +28,7 @@
> >   #include <linux/amba/bus.h>
> >   #include <linux/seq_file.h>
> >   #include <linux/uaccess.h>
> > +#include <linux/perf/arm_pmu.h>
> >   #include <linux/perf_event.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/pm_runtime.h>
> > @@ -615,7 +616,7 @@ static void etm4_enable_hw_smp_call(void *info)
> >    *  +--------------+
> >    *         |
> >    *  +------v-------+
> > - *  | Counter x    |   (reload to 1 on underflow)
> > + *  | Counter x    |   (reload to 2 ^ ts_level on underflow)
> >    *  +--------------+
> >    *         |
> >    *  +------v--------------+
> > @@ -626,11 +627,17 @@ static void etm4_enable_hw_smp_call(void *info)
> >    *  | Timestamp Generator  |  (timestamp on resource y)
> >    *  +----------------------+
> >    */
> > -static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
> > +static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata,
> > +				       struct perf_event_attr *attr)
> >   {
> >   	int ctridx;
> >   	int rselector;
> >   	struct etmv4_config *config = &drvdata->config;
> > +	u8 ts_level = ATTR_CFG_GET_FLD(attr, ts_level);
> > +
> > +	/* Disable when ts_level == MAX */
> > +	if (ts_level == FIELD_GET(ATTR_CFG_FLD_ts_level_MASK, UINT_MAX))
> > +		return 0;
> >   
> >   	/* No point in trying if we don't have at least one counter */
> >   	if (!drvdata->nr_cntr)
> > @@ -666,12 +673,8 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
> >   		return -ENOSPC;
> >   	}
> >   
> > -	/*
> > -	 * Initialise original and reload counter value to the smallest
> > -	 * possible value in order to get as much precision as we can.
> > -	 */
> > -	config->cntr_val[ctridx] = 1;
> > -	config->cntrldvr[ctridx] = 1;
> > +	/* Initialise original and reload counter value. */
> > +	config->cntr_val[ctridx] = config->cntrldvr[ctridx] = 1 << ts_level;
> >   
> >   	/*
> >   	 * Trace Counter Control Register TRCCNTCTLRn
> > @@ -761,7 +764,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
> >   		 * order to correlate instructions executed on different CPUs
> >   		 * (CPU-wide trace scenarios).
> >   		 */
> > -		ret = etm4_config_timestamp_event(drvdata);
> > +		ret = etm4_config_timestamp_event(drvdata, attr);
> >   
> >   		/*
> >   		 * No need to go further if timestamp intervals can't
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > index aaa6633b2d67..54558de158fa 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> > @@ -598,6 +598,12 @@
> >   #define ETM_CNTR_MAX_VAL		0xFFFF
> >   #define ETM_TRACEID_MASK		0x3f
> >   
> > +#define ATTR_CFG_FLD_ts_level_CFG	config3
> > +#define ATTR_CFG_FLD_ts_level_LO	12
> > +#define ATTR_CFG_FLD_ts_level_HI	15
> > +#define ATTR_CFG_FLD_ts_level_MASK	GENMASK(ATTR_CFG_FLD_ts_level_HI, \
> > +						ATTR_CFG_FLD_ts_level_LO)
> > +
> >   /* ETMv4 programming modes */
> >   #define ETM_MODE_EXCLUDE		BIT(0)
> >   #define ETM_MODE_LOAD			BIT(1)
> >
> > --
> > 2.34.1