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

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

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@arm.com>

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