[PATCH v4 00/13] coresight: Update timestamp attribute to be an interval instead of bool

[PATCH v4 00/13] coresight: Update timestamp attribute to be an interval instead of bool

[James Clark]

Do some cleanups then expand the timestamp format attribute from 1 bit
to 4 bits for ETMv4 in Perf mode. The current interval is too high for
most use cases, and particularly on the FVP the number of timestamps
generated is excessive. This change not only still allows disabling or
enabling timestamps, but also allows the interval to be configured.

The old bit is kept deprecated and undocumented for now. There are known
broken versions of Perf that don't read the format attribute positions
from sysfs and instead hard code the timestamp bit. We can leave the old
bit in the driver until we need the bit for another feature or enough
time has passed that these old Perfs are unlikely to be used.

The interval option is added as an event format attribute, rather than a
Coresight config because it's something that the driver is already
configuring automatically in Perf mode using any unused counter, so it's
not possible to modify this with a config.

Applies to coresight/next

Signed-off-by: James Clark <james.clark@linaro.org>
---
Changes in v4:
- Add #defines for true and false resources ETM_RES_SEL_TRUE/FALSE
- Reword comment about finding a counter to say if there are no
  resources there are no counters.
- Extend existing timestamp format attribute instead of adding a new one
- Refactor all the config definitions and parsing to use
  GEN_PMU_FORMAT_ATTR()/ATTR_CFG_GET_FLD() so we can see where the
  unused bits are.
- Link to v3: https://lore.kernel.org/r/20251002-james-cs-syncfreq-v3-0-fe5df2bf91d1@linaro.org

Changes in v3:
- Move the format attr definitions to coresight-etm-perf.h we can
  compile on arm32 without #ifdefs - (Leo)
- Convert the new #ifdefs to a single one in an is_visible() function so
  that the code is cleaner - (Leo)
- Drop the change to remove the holes in struct etmv4_config as they
  were grouped by function - (Mike)
- Link to v2: https://lore.kernel.org/r/20250814-james-cs-syncfreq-v2-0-c76fcb87696d@linaro.org

Changes in v2:
- Only show the attribute for ETMv4 to improve usability and fix the
  arm32 build error. Wrapping everything in
  IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) isn't ideal, but the -perf.c
  file is shared between ETMv3 and ETMv4, and there is already precedent
  for doing it this way.
- Link to v1: https://lore.kernel.org/r/20250811-james-cs-syncfreq-v1-0-b001cd6e3404@linaro.org

---
James Clark (13):
      coresight: Change syncfreq to be a u8
      coresight: Repack struct etmv4_drvdata
      coresight: Refactor etm4_config_timestamp_event()
      coresight: Hide unused ETMv3 format attributes
      coresight: Define format attributes with GEN_PMU_FORMAT_ATTR()
      coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD()
      coresight: Don't reject unrecognized ETMv3 format attributes
      coresight: Interpret perf config with ATTR_CFG_GET_FLD()
      coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD()
      coresight: Remove misleading definitions
      coresight: Extend width of timestamp format attribute
      coresight: Allow setting the timestamp interval
      coresight: docs: Document etm4x timestamp interval option

 Documentation/trace/coresight/coresight.rst        |  15 +-
 drivers/hwtracing/coresight/coresight-etm-perf.c   |  59 +++++---
 drivers/hwtracing/coresight/coresight-etm-perf.h   |  39 +++++
 drivers/hwtracing/coresight/coresight-etm3x-core.c |  36 ++---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 164 +++++++++++++--------
 drivers/hwtracing/coresight/coresight-etm4x.h      |  62 +++++---
 include/linux/coresight-pmu.h                      |  24 ---
 7 files changed, 247 insertions(+), 152 deletions(-)
---
base-commit: efdccf6a511891db037e08f1351e72eaa101021e
change-id: 20250724-james-cs-syncfreq-7c2257a38ed3

Best regards,
-- 
James Clark <james.clark@linaro.org>

[PATCH v4 01/13] coresight: Change syncfreq to be a u8

[James Clark]

TRCSYNCPR.PERIOD is the only functional part of TRCSYNCPR and it only
has 5 valid bits so it can be stored in a u8.

Reviewed-by: Mike Leach <mike.leach@linaro.org>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 8d4a1f0f1e52..56a359184c6f 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -826,7 +826,6 @@ struct etmv4_config {
 	u32				eventctrl1;
 	u32				stall_ctrl;
 	u32				ts_ctrl;
-	u32				syncfreq;
 	u32				ccctlr;
 	u32				bb_ctrl;
 	u32				vinst_ctrl;
@@ -834,6 +833,7 @@ struct etmv4_config {
 	u32				vissctlr;
 	u32				vipcssctlr;
 	u8				seq_idx;
+	u8				syncfreq;
 	u32				seq_ctrl[ETM_MAX_SEQ_STATES];
 	u32				seq_rst;
 	u32				seq_state;

-- 
2.34.1

[PATCH v4 02/13] coresight: Repack struct etmv4_drvdata

[James Clark]

Fix holes and convert the long list of bools to single bits to save
some space because there's one of these for each ETM.

Reviewed-by: Leo Yan <leo.yan@arm.com>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x.h | 37 ++++++++++++++-------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 56a359184c6f..8f546764908c 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -962,26 +962,27 @@ struct etmv4_drvdata {
 	u8				ns_ex_level;
 	u8				q_support;
 	u8				os_lock_model;
-	bool				sticky_enable;
-	bool				boot_enable;
-	bool				os_unlock;
-	bool				instrp0;
-	bool				q_filt;
-	bool				trcbb;
-	bool				trccond;
-	bool				retstack;
-	bool				trccci;
-	bool				trc_error;
-	bool				syncpr;
-	bool				stallctl;
-	bool				sysstall;
-	bool				nooverflow;
-	bool				atbtrig;
-	bool				lpoverride;
+	bool				sticky_enable : 1;
+	bool				boot_enable : 1;
+	bool				os_unlock : 1;
+	bool				instrp0 : 1;
+	bool				q_filt : 1;
+	bool				trcbb : 1;
+	bool				trccond : 1;
+	bool				retstack : 1;
+	bool				trccci : 1;
+	bool				trc_error : 1;
+	bool				syncpr : 1;
+	bool				stallctl : 1;
+	bool				sysstall : 1;
+	bool				nooverflow : 1;
+	bool				atbtrig : 1;
+	bool				lpoverride : 1;
+	bool				skip_power_up : 1;
+	bool				paused : 1;
 	u64				trfcr;
 	struct etmv4_config		config;
-	bool				skip_power_up;
-	bool				paused;
+
 	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
 };
 

-- 
2.34.1

[PATCH v4 03/13] coresight: Refactor etm4_config_timestamp_event()

[James Clark]

Remove some of the magic numbers and try to clarify some of the
documentation so it's clearer how this sets up the timestamp interval.

Return errors directly instead of jumping to out and returning ret,
nothing needs to be cleaned up at the end and it only obscures the flow
and return value.

Reviewed-by: Leo Yan <leo.yan@arm.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 96 ++++++++++++++--------
 drivers/hwtracing/coresight/coresight-etm4x.h      | 23 ++++--
 2 files changed, 81 insertions(+), 38 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 1c17d5472920..380a7840adb8 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -651,18 +651,33 @@ static void etm4_enable_sysfs_smp_call(void *info)
  * TRCRSCTLR1 (always true) used to get the counter to decrement.  From
  * there a resource selector is configured with the counter and the
  * timestamp control register to use the resource selector to trigger the
- * event that will insert a timestamp packet in the stream.
+ * event that will insert a timestamp packet in the stream:
+ *
+ *  +--------------+
+ *  | Resource 1   |   fixed "always-true" resource
+ *  +--------------+
+ *         |
+ *  +------v-------+
+ *  | Counter x    |   (reload to 1 on underflow)
+ *  +--------------+
+ *         |
+ *  +------v--------------+
+ *  | Resource Selector y |   (trigger on counter x == 0)
+ *  +---------------------+
+ *         |
+ *  +------v---------------+
+ *  | Timestamp Generator  |  (timestamp on resource y)
+ *  +----------------------+
  */
 static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
 {
-	int ctridx, ret = -EINVAL;
-	int counter, rselector;
-	u32 val = 0;
+	int ctridx;
+	int rselector;
 	struct etmv4_config *config = &drvdata->config;
 
 	/* No point in trying if we don't have at least one counter */
 	if (!drvdata->nr_cntr)
-		goto out;
+		return -EINVAL;
 
 	/* Find a counter that hasn't been initialised */
 	for (ctridx = 0; ctridx < drvdata->nr_cntr; ctridx++)
@@ -672,15 +687,19 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
 	/* All the counters have been configured already, bail out */
 	if (ctridx == drvdata->nr_cntr) {
 		pr_debug("%s: no available counter found\n", __func__);
-		ret = -ENOSPC;
-		goto out;
+		return -ENOSPC;
 	}
 
 	/*
-	 * Searching for an available resource selector to use, starting at
-	 * '2' since every implementation has at least 2 resource selector.
-	 * ETMIDR4 gives the number of resource selector _pairs_,
-	 * hence multiply by 2.
+	 * Searching for an available resource selector to use, starting at '2'
+	 * since resource 0 is the fixed 'always returns false' resource and 1
+	 * is the fixed 'always returns true' resource. See IHI0064H_b '7.3.64
+	 * TRCRSCTLRn, Resource Selection Control Registers, n=2-31'. If there
+	 * are no resources, there would also be no counters so wouldn't get
+	 * here.
+	 *
+	 * ETMIDR4 gives the number of resource selector _pairs_, hence multiply
+	 * by 2.
 	 */
 	for (rselector = 2; rselector < drvdata->nr_resource * 2; rselector++)
 		if (!config->res_ctrl[rselector])
@@ -689,13 +708,9 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
 	if (rselector == drvdata->nr_resource * 2) {
 		pr_debug("%s: no available resource selector found\n",
 			 __func__);
-		ret = -ENOSPC;
-		goto out;
+		return -ENOSPC;
 	}
 
-	/* Remember what counter we used */
-	counter = 1 << ctridx;
-
 	/*
 	 * Initialise original and reload counter value to the smallest
 	 * possible value in order to get as much precision as we can.
@@ -703,26 +718,41 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata)
 	config->cntr_val[ctridx] = 1;
 	config->cntrldvr[ctridx] = 1;
 
-	/* Set the trace counter control register */
-	val =  0x1 << 16	|  /* Bit 16, reload counter automatically */
-	       0x0 << 7		|  /* Select single resource selector */
-	       0x1;		   /* Resource selector 1, i.e always true */
-
-	config->cntr_ctrl[ctridx] = val;
-
-	val = 0x2 << 16		| /* Group 0b0010 - Counter and sequencers */
-	      counter << 0;	  /* Counter to use */
-
-	config->res_ctrl[rselector] = val;
+	/*
+	 * Trace Counter Control Register TRCCNTCTLRn
+	 *
+	 * CNTCHAIN = 0, don't reload on the previous counter
+	 * RLDSELF = true, reload counter automatically on underflow
+	 * RLDTYPE = 0, one reload input resource
+	 * RLDSEL = RES_SEL_FALSE (0), reload on false resource (never reload)
+	 * CNTTYPE = 0, one count input resource
+	 * CNTSEL = RES_SEL_TRUE (1), count fixed 'always true' resource (always decrement)
+	 */
+	config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
+				    FIELD_PREP(TRCCNTCTLRn_RLDSEL_MASK, ETM_RES_SEL_FALSE) |
+				    FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, ETM_RES_SEL_TRUE);
 
-	val = 0x0 << 7		| /* Select single resource selector */
-	      rselector;	  /* Resource selector */
+	/*
+	 * Resource Selection Control Register TRCRSCTLRn
+	 *
+	 * PAIRINV = 0, INV = 0, don't invert
+	 * GROUP = 2, SELECT = ctridx, trigger when counter 'ctridx' reaches 0
+	 *
+	 * Multiple counters can be selected, and each bit signifies a counter,
+	 * so set bit 'ctridx' to select our counter.
+	 */
+	config->res_ctrl[rselector] = FIELD_PREP(TRCRSCTLRn_GROUP_MASK, 2) |
+				      FIELD_PREP(TRCRSCTLRn_SELECT_MASK, 1 << ctridx);
 
-	config->ts_ctrl = val;
+	/*
+	 * Global Timestamp Control Register TRCTSCTLR
+	 *
+	 * TYPE = 0, one input resource
+	 * SEL = rselector, generate timestamp on resource 'rselector'
+	 */
+	config->ts_ctrl = FIELD_PREP(TRCTSCTLR_SEL_MASK, rselector);
 
-	ret = 0;
-out:
-	return ret;
+	return 0;
 }
 
 static int etm4_parse_event_config(struct coresight_device *csdev,
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 8f546764908c..0f83a29320e6 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -225,6 +225,19 @@
 #define TRCRSCTLRn_GROUP_MASK			GENMASK(19, 16)
 #define TRCRSCTLRn_SELECT_MASK			GENMASK(15, 0)
 
+#define TRCCNTCTLRn_CNTCHAIN			BIT(17)
+#define TRCCNTCTLRn_RLDSELF			BIT(16)
+#define TRCCNTCTLRn_RLDTYPE			BIT(15)
+#define TRCCNTCTLRn_RLDSEL_MASK			GENMASK(12, 8)
+#define TRCCNTCTLRn_CNTTYPE_MASK		BIT(7)
+#define TRCCNTCTLRn_CNTSEL_MASK			GENMASK(4, 0)
+
+#define TRCTSCTLR_TYPE				BIT(7)
+#define TRCTSCTLR_SEL_MASK			GENMASK(4, 0)
+
+#define ETM_RES_SEL_FALSE 0 /* Fixed function 'always false' resource selector */
+#define ETM_RES_SEL_TRUE  1 /* Fixed function 'always true' resource selector */
+
 /*
  * System instructions to access ETM registers.
  * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
@@ -825,7 +838,7 @@ struct etmv4_config {
 	u32				eventctrl0;
 	u32				eventctrl1;
 	u32				stall_ctrl;
-	u32				ts_ctrl;
+	u32				ts_ctrl; /* TRCTSCTLR */
 	u32				ccctlr;
 	u32				bb_ctrl;
 	u32				vinst_ctrl;
@@ -838,11 +851,11 @@ struct etmv4_config {
 	u32				seq_rst;
 	u32				seq_state;
 	u8				cntr_idx;
-	u32				cntrldvr[ETMv4_MAX_CNTR];
-	u32				cntr_ctrl[ETMv4_MAX_CNTR];
-	u32				cntr_val[ETMv4_MAX_CNTR];
+	u32				cntrldvr[ETMv4_MAX_CNTR]; /* TRCCNTRLDVRn */
+	u32				cntr_ctrl[ETMv4_MAX_CNTR];  /* TRCCNTCTLRn */
+	u32				cntr_val[ETMv4_MAX_CNTR]; /* TRCCNTVRn */
 	u8				res_idx;
-	u32				res_ctrl[ETM_MAX_RES_SEL];
+	u32				res_ctrl[ETM_MAX_RES_SEL]; /* TRCRSCTLRn */
 	u8				ss_idx;
 	u32				ss_ctrl[ETM_MAX_SS_CMP];
 	u32				ss_status[ETM_MAX_SS_CMP];

-- 
2.34.1

[PATCH v4 04/13] coresight: Hide unused ETMv3 format attributes

[James Clark]

ETMv3 only has a few attributes, and setting unused ones results in an
error, so hide them to begin with.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 17afa0f4cdee..91132abca244 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -106,8 +106,27 @@ static struct attribute *etm_config_formats_attr[] = {
 	NULL,
 };
 
+static umode_t etm_format_attr_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int unused)
+{
+	/* ETM4 has all attributes */
+	if (IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
+		return attr->mode;
+
+	/* ETM3 only has these attributes */
+	if (attr == &format_attr_cycacc.attr ||
+	    attr == &format_attr_timestamp.attr ||
+	    attr == &format_attr_retstack.attr ||
+	    attr == &format_attr_sinkid.attr ||
+	    attr == &format_attr_configid.attr)
+		return attr->mode;
+
+	return 0;
+}
+
 static const struct attribute_group etm_pmu_format_group = {
 	.name   = "format",
+	.is_visible = etm_format_attr_is_visible,
 	.attrs  = etm_config_formats_attr,
 };
 

-- 
2.34.1

[PATCH v4 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR()

[James Clark]

This allows us to define and consume them in a unified way in later
commits.

A lot of the existing code has open coded bit shifts or direct usage of
whole config values which is error prone and hides which bits are in use
and which are free.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 22 ++++++++---------
 drivers/hwtracing/coresight/coresight-etm-perf.h | 31 ++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 91132abca244..d19e2568a0d1 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -13,6 +13,7 @@
 #include <linux/mm.h>
 #include <linux/init.h>
 #include <linux/perf_event.h>
+#include <linux/perf/arm_pmu.h>
 #include <linux/percpu-defs.h>
 #include <linux/slab.h>
 #include <linux/stringhash.h>
@@ -54,22 +55,21 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
  * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config';
  * now take them as general formats and apply on all ETMs.
  */
-PMU_FORMAT_ATTR(branch_broadcast, "config:"__stringify(ETM_OPT_BRANCH_BROADCAST));
-PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
+GEN_PMU_FORMAT_ATTR(branch_broadcast);
+GEN_PMU_FORMAT_ATTR(cycacc);
 /* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
-PMU_FORMAT_ATTR(contextid1,	"config:" __stringify(ETM_OPT_CTXTID));
+GEN_PMU_FORMAT_ATTR(contextid1);
 /* contextid2 enables tracing CONTEXTIDR_EL2 for ETMv4 */
-PMU_FORMAT_ATTR(contextid2,	"config:" __stringify(ETM_OPT_CTXTID2));
-PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
-PMU_FORMAT_ATTR(retstack,	"config:" __stringify(ETM_OPT_RETSTK));
+GEN_PMU_FORMAT_ATTR(contextid2);
+GEN_PMU_FORMAT_ATTR(timestamp);
+GEN_PMU_FORMAT_ATTR(retstack);
 /* preset - if sink ID is used as a configuration selector */
-PMU_FORMAT_ATTR(preset,		"config:0-3");
+GEN_PMU_FORMAT_ATTR(preset);
 /* Sink ID - same for all ETMs */
-PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
+GEN_PMU_FORMAT_ATTR(sinkid);
 /* config ID - set if a system configuration is selected */
-PMU_FORMAT_ATTR(configid,	"config2:32-63");
-PMU_FORMAT_ATTR(cc_threshold,	"config3:0-11");
-
+GEN_PMU_FORMAT_ATTR(configid);
+GEN_PMU_FORMAT_ATTR(cc_threshold);
 
 /*
  * contextid always traces the "PID".  The PID is in CONTEXTIDR_EL1
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 5febbcdb8696..c794087a0e99 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -20,6 +20,37 @@ struct cscfg_config_desc;
  */
 #define ETM_ADDR_CMP_MAX	8
 
+#define ATTR_CFG_FLD_preset_CFG			config
+#define ATTR_CFG_FLD_preset_LO			0
+#define ATTR_CFG_FLD_preset_HI			3
+#define ATTR_CFG_FLD_branch_broadcast_CFG	config
+#define ATTR_CFG_FLD_branch_broadcast_LO	8
+#define ATTR_CFG_FLD_branch_broadcast_HI	8
+#define ATTR_CFG_FLD_cycacc_CFG			config
+#define ATTR_CFG_FLD_cycacc_LO			12
+#define ATTR_CFG_FLD_cycacc_HI			12
+#define ATTR_CFG_FLD_contextid1_CFG		config
+#define ATTR_CFG_FLD_contextid1_LO		14
+#define ATTR_CFG_FLD_contextid1_HI		14
+#define ATTR_CFG_FLD_contextid2_CFG		config
+#define ATTR_CFG_FLD_contextid2_LO		15
+#define ATTR_CFG_FLD_contextid2_HI		15
+#define ATTR_CFG_FLD_timestamp_CFG		config
+#define ATTR_CFG_FLD_timestamp_LO		28
+#define ATTR_CFG_FLD_timestamp_HI		28
+#define ATTR_CFG_FLD_retstack_CFG		config
+#define ATTR_CFG_FLD_retstack_LO		29
+#define ATTR_CFG_FLD_retstack_HI		29
+#define ATTR_CFG_FLD_sinkid_CFG			config2
+#define ATTR_CFG_FLD_sinkid_LO			0
+#define ATTR_CFG_FLD_sinkid_HI			31
+#define ATTR_CFG_FLD_configid_CFG		config2
+#define ATTR_CFG_FLD_configid_LO		32
+#define ATTR_CFG_FLD_configid_HI		63
+#define ATTR_CFG_FLD_cc_threshold_CFG		config3
+#define ATTR_CFG_FLD_cc_threshold_LO		0
+#define ATTR_CFG_FLD_cc_threshold_HI		11
+
 /**
  * struct etm_filter - single instruction range or start/stop configuration.
  * @start_addr:	The address to start tracing on.

-- 
2.34.1

[PATCH v4 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD()

[James Clark]

Currently we're programming attr->config directly into ETMCR after some
validation. This obscures which fields are being used, and also makes it
impossible to move fields around or use other configN fields in the
future.

Improve it by only reading the fields that are valid and then setting
the appropriate ETMCR bits based on each one.

The ETMCR_CTXID_SIZE part can be removed as it was never a valid option
because it's not in ETM3X_SUPPORTED_OPTIONS.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm3x-core.c | 24 ++++++++++++----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index a5e809589d3e..4511fc2f8d72 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -28,6 +28,7 @@
 #include <linux/uaccess.h>
 #include <linux/clk.h>
 #include <linux/perf_event.h>
+#include <linux/perf/arm_pmu.h>
 #include <asm/sections.h>
 
 #include "coresight-etm.h"
@@ -339,21 +340,22 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
 	if (attr->config & ~ETM3X_SUPPORTED_OPTIONS)
 		return -EINVAL;
 
-	config->ctrl = attr->config;
+	config->ctrl = 0;
 
-	/* Don't trace contextID when runs in non-root PID namespace */
-	if (!task_is_in_init_pid_ns(current))
-		config->ctrl &= ~ETMCR_CTXID_SIZE;
+	if (ATTR_CFG_GET_FLD(attr, cycacc))
+		config->ctrl |= ETMCR_CYC_ACC;
+
+	if (ATTR_CFG_GET_FLD(attr, timestamp))
+		config->ctrl |= ETMCR_TIMESTAMP_EN;
 
 	/*
-	 * Possible to have cores with PTM (supports ret stack) and ETM
-	 * (never has ret stack) on the same SoC. So if we have a request
-	 * for return stack that can't be honoured on this core then
-	 * clear the bit - trace will still continue normally
+	 * Possible to have cores with PTM (supports ret stack) and ETM (never
+	 * has ret stack) on the same SoC. So only enable when it can be honored
+	 * - trace will still continue normally otherwise.
 	 */
-	if ((config->ctrl & ETMCR_RETURN_STACK) &&
-	    !(drvdata->etmccer & ETMCCER_RETSTACK))
-		config->ctrl &= ~ETMCR_RETURN_STACK;
+	if (ATTR_CFG_GET_FLD(attr, retstack) &&
+	    (drvdata->etmccer & ETMCCER_RETSTACK))
+		config->ctrl |= ETMCR_RETURN_STACK;
 
 	return 0;
 }

-- 
2.34.1

[PATCH v4 07/13] coresight: Don't reject unrecognized ETMv3 format attributes

[James Clark]

config isn't the only field, there are also config1, config2, etc.
Rejecting unrecognized attributes is therefore inconsistent as it wasn't
done for all fields. It was only necessary when we were directly
programming attr->config into ETMCR and didn't hide the unsupported
fields, but now it's not needed so remove it.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm3x-core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index 4511fc2f8d72..584d653eda81 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -333,13 +333,6 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
 	if (config->mode)
 		etm_config_trace_mode(config);
 
-	/*
-	 * At this time only cycle accurate, return stack  and timestamp
-	 * options are available.
-	 */
-	if (attr->config & ~ETM3X_SUPPORTED_OPTIONS)
-		return -EINVAL;
-
 	config->ctrl = 0;
 
 	if (ATTR_CFG_GET_FLD(attr, cycacc))

-- 
2.34.1

[PATCH v4 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()

[James Clark]

The "config:" string construction in format_attr_contextid_show() can be
removed because it either showed the existing context1 or context2
formats which have already been generated, so can be called themselves.

The other conversions are straightforward replacements.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index d19e2568a0d1..7272e758aebf 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -80,12 +80,11 @@ static ssize_t format_attr_contextid_show(struct device *dev,
 					  struct device_attribute *attr,
 					  char *page)
 {
-	int pid_fmt = ETM_OPT_CTXTID;
-
 #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
-	pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
+	if (is_kernel_in_hyp_mode())
+		return contextid2_show(dev, attr, page);
 #endif
-	return sprintf(page, "config:%d\n", pid_fmt);
+	return contextid1_show(dev, attr, page);
 }
 
 static struct device_attribute format_attr_contextid =
@@ -334,7 +333,7 @@ static bool sinks_compatible(struct coresight_device *a,
 static void *etm_setup_aux(struct perf_event *event, void **pages,
 			   int nr_pages, bool overwrite)
 {
-	u32 id, cfg_hash;
+	u32 sink_hash, cfg_hash;
 	int cpu = event->cpu;
 	cpumask_t *mask;
 	struct coresight_device *sink = NULL;
@@ -347,13 +346,12 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
 	INIT_WORK(&event_data->work, free_event_data);
 
 	/* First get the selected sink from user space. */
-	if (event->attr.config2 & GENMASK_ULL(31, 0)) {
-		id = (u32)event->attr.config2;
-		sink = user_sink = coresight_get_sink_by_id(id);
-	}
+	sink_hash = ATTR_CFG_GET_FLD(&event->attr, sinkid);
+	if (sink_hash)
+		sink = user_sink = coresight_get_sink_by_id(sink_hash);
 
 	/* check if user wants a coresight configuration selected */
-	cfg_hash = (u32)((event->attr.config2 & GENMASK_ULL(63, 32)) >> 32);
+	cfg_hash = ATTR_CFG_GET_FLD(&event->attr, configid);
 	if (cfg_hash) {
 		if (cscfg_activate_config(cfg_hash))
 			goto err;

-- 
2.34.1

[PATCH v4 09/13] coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD()

[James Clark]

Remove hard coded bitfield extractions and shifts and replace with
ATTR_CFG_GET_FLD().

ETM4_CFG_BIT_BB was defined to give the register bit positions to
userspace, TRCCONFIGR_BB should be used in the kernel so replace it.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 44 ++++++++++------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 380a7840adb8..1aa0357a5ce7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -29,6 +29,7 @@
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
 #include <linux/perf_event.h>
+#include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -789,17 +790,17 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 		goto out;
 
 	/* Go from generic option to ETMv4 specifics */
-	if (attr->config & BIT(ETM_OPT_CYCACC)) {
+	if (ATTR_CFG_GET_FLD(attr, cycacc)) {
 		config->cfg |= TRCCONFIGR_CCI;
 		/* TRM: Must program this for cycacc to work */
-		cc_threshold = attr->config3 & ETM_CYC_THRESHOLD_MASK;
+		cc_threshold = ATTR_CFG_GET_FLD(attr, cc_threshold);
 		if (!cc_threshold)
 			cc_threshold = ETM_CYC_THRESHOLD_DEFAULT;
 		if (cc_threshold < drvdata->ccitmin)
 			cc_threshold = drvdata->ccitmin;
 		config->ccctlr = cc_threshold;
 	}
-	if (attr->config & BIT(ETM_OPT_TS)) {
+	if (ATTR_CFG_GET_FLD(attr, timestamp)) {
 		/*
 		 * Configure timestamps to be emitted at regular intervals in
 		 * order to correlate instructions executed on different CPUs
@@ -819,17 +820,17 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 	}
 
 	/* Only trace contextID when runs in root PID namespace */
-	if ((attr->config & BIT(ETM_OPT_CTXTID)) &&
+	if (ATTR_CFG_GET_FLD(attr, contextid1) &&
 	    task_is_in_init_pid_ns(current))
 		/* bit[6], Context ID tracing bit */
 		config->cfg |= TRCCONFIGR_CID;
 
 	/*
-	 * If set bit ETM_OPT_CTXTID2 in perf config, this asks to trace VMID
-	 * for recording CONTEXTIDR_EL2.  Do not enable VMID tracing if the
-	 * kernel is not running in EL2.
+	 * If set bit contextid2 in perf config, this asks to trace VMID for
+	 * recording CONTEXTIDR_EL2.  Do not enable VMID tracing if the kernel
+	 * is not running in EL2.
 	 */
-	if (attr->config & BIT(ETM_OPT_CTXTID2)) {
+	if (ATTR_CFG_GET_FLD(attr, contextid2)) {
 		if (!is_kernel_in_hyp_mode()) {
 			ret = -EINVAL;
 			goto out;
@@ -840,26 +841,22 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 	}
 
 	/* return stack - enable if selected and supported */
-	if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
+	if (ATTR_CFG_GET_FLD(attr, retstack) && drvdata->retstack)
 		/* bit[12], Return stack enable bit */
 		config->cfg |= TRCCONFIGR_RS;
 
 	/*
-	 * Set any selected configuration and preset.
-	 *
-	 * This extracts the values of PMU_FORMAT_ATTR(configid) and PMU_FORMAT_ATTR(preset)
-	 * in the perf attributes defined in coresight-etm-perf.c.
-	 * configid uses bits 63:32 of attr->config2, preset uses bits 3:0 of attr->config.
-	 * A zero configid means no configuration active, preset = 0 means no preset selected.
+	 * Set any selected configuration and preset. A zero configid means no
+	 * configuration active, preset = 0 means no preset selected.
 	 */
-	if (attr->config2 & GENMASK_ULL(63, 32)) {
-		cfg_hash = (u32)(attr->config2 >> 32);
-		preset = attr->config & 0xF;
+	if (ATTR_CFG_GET_FLD(attr, configid)) {
+		cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
+		preset = ATTR_CFG_GET_FLD(attr, preset);
 		ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
 	}
 
 	/* branch broadcast - enable if selected and supported */
-	if (attr->config & BIT(ETM_OPT_BRANCH_BROADCAST)) {
+	if (ATTR_CFG_GET_FLD(attr, branch_broadcast)) {
 		if (!drvdata->trcbb) {
 			/*
 			 * Missing BB support could cause silent decode errors
@@ -868,7 +865,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 			ret = -EINVAL;
 			goto out;
 		} else {
-			config->cfg |= BIT(ETM4_CFG_BIT_BB);
+			config->cfg |= TRCCONFIGR_BB;
 		}
 	}
 
@@ -1104,11 +1101,8 @@ static int etm4_disable_perf(struct coresight_device *csdev,
 		return -EINVAL;
 
 	etm4_disable_hw(drvdata);
-	/*
-	 * The config_id occupies bits 63:32 of the config2 perf event attr
-	 * field. If this is non-zero then we will have enabled a config.
-	 */
-	if (attr->config2 & GENMASK_ULL(63, 32))
+	/* If configid is non-zero then we will have enabled a config. */
+	if (ATTR_CFG_GET_FLD(attr, configid))
 		cscfg_csdev_disable_active_config(csdev);
 
 	/*

-- 
2.34.1

[PATCH v4 10/13] coresight: Remove misleading definitions

[James Clark]

ETM_OPT_* definitions duplicate the PMU format attributes that have
always been published in sysfs. Hardcoding them here makes it misleading
as to what the 'real' PMU API is and prevents attributes from being
rearranged in the future.

ETM4_CFG_BIT_* definitions just define what the Arm Architecture is
which is not the responsibility of the kernel to do and doesn't scale to
other registers or versions of ETM. It's not an actual software ABI/API
and these definitions here mislead that it is.

Any tools using the first ones would be broken anyway as they won't work
when attributes are moved, so removing them is the right thing to do and
will prompt a fix. Tools using the second ones can trivially redefine
them locally.

Perf also has its own copy of the headers so both of these things can be
fixed up at a later date.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 include/linux/coresight-pmu.h | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
index 89b0ac0014b0..2e179abe472a 100644
--- a/include/linux/coresight-pmu.h
+++ b/include/linux/coresight-pmu.h
@@ -21,30 +21,6 @@
  */
 #define CORESIGHT_LEGACY_CPU_TRACE_ID(cpu)  (0x10 + (cpu * 2))
 
-/*
- * Below are the definition of bit offsets for perf option, and works as
- * arbitrary values for all ETM versions.
- *
- * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
- * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
- * directly use below macros as config bits.
- */
-#define ETM_OPT_BRANCH_BROADCAST 8
-#define ETM_OPT_CYCACC		12
-#define ETM_OPT_CTXTID		14
-#define ETM_OPT_CTXTID2		15
-#define ETM_OPT_TS		28
-#define ETM_OPT_RETSTK		29
-
-/* ETMv4 CONFIGR programming bits for the ETM OPTs */
-#define ETM4_CFG_BIT_BB         3
-#define ETM4_CFG_BIT_CYCACC	4
-#define ETM4_CFG_BIT_CTXTID	6
-#define ETM4_CFG_BIT_VMID	7
-#define ETM4_CFG_BIT_TS		11
-#define ETM4_CFG_BIT_RETSTK	12
-#define ETM4_CFG_BIT_VMID_OPT	15
-
 /*
  * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload.
  * Used to associate a CPU with the CoreSight Trace ID.

-- 
2.34.1

[PATCH v4 11/13] coresight: Extend width of timestamp format attribute

[James Clark]

'timestamp' is currently 1 bit wide for on/off. To enable setting
different intervals in a later commit, extend it to 4 bits wide. Keep
the old bit position for backward compatibility but don't publish in the
format/ folder. It will be removed from the documentation and can be
removed completely after enough time has passed.

ETM3x doesn't support different intervals, so validate that the value is
either 0 or 1.

Tools that read the bit positions from the format/ folder will continue
to work as before, setting either 0 or 1 for off/on. Tools that
incorrectly didn't do this and set the ETM_OPT_TS bit directly will also
continue to work because that old bit is still checked.

This avoids adding a second timestamp attribute for setting the
interval. This would be awkward to use because tools would have to be
updated to ensure that the timestamps are always enabled when an
interval is set, and the driver would have to validate that both options
are provided together. All this does is implement the semantics of a
single enum but spread over multiple fields.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.h   | 13 ++++++++++---
 drivers/hwtracing/coresight/coresight-etm3x-core.c |  9 ++++++++-
 drivers/hwtracing/coresight/coresight-etm4x-core.c |  4 +++-
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index c794087a0e99..24d929428633 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -23,6 +23,9 @@ struct cscfg_config_desc;
 #define ATTR_CFG_FLD_preset_CFG			config
 #define ATTR_CFG_FLD_preset_LO			0
 #define ATTR_CFG_FLD_preset_HI			3
+#define ATTR_CFG_FLD_timestamp_CFG		config
+#define ATTR_CFG_FLD_timestamp_LO		4
+#define ATTR_CFG_FLD_timestamp_HI		7
 #define ATTR_CFG_FLD_branch_broadcast_CFG	config
 #define ATTR_CFG_FLD_branch_broadcast_LO	8
 #define ATTR_CFG_FLD_branch_broadcast_HI	8
@@ -35,9 +38,13 @@ struct cscfg_config_desc;
 #define ATTR_CFG_FLD_contextid2_CFG		config
 #define ATTR_CFG_FLD_contextid2_LO		15
 #define ATTR_CFG_FLD_contextid2_HI		15
-#define ATTR_CFG_FLD_timestamp_CFG		config
-#define ATTR_CFG_FLD_timestamp_LO		28
-#define ATTR_CFG_FLD_timestamp_HI		28
+/*
+ * Old position of 'timestamp' and not published in sysfs. Remove at a later
+ * date if necessary.
+ */
+#define ATTR_CFG_FLD_deprecated_timestamp_CFG	config
+#define ATTR_CFG_FLD_deprecated_timestamp_LO	28
+#define ATTR_CFG_FLD_deprecated_timestamp_HI	28
 #define ATTR_CFG_FLD_retstack_CFG		config
 #define ATTR_CFG_FLD_retstack_LO		29
 #define ATTR_CFG_FLD_retstack_HI		29
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
index 584d653eda81..d4c04e563bf6 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -338,9 +338,16 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
 	if (ATTR_CFG_GET_FLD(attr, cycacc))
 		config->ctrl |= ETMCR_CYC_ACC;
 
-	if (ATTR_CFG_GET_FLD(attr, timestamp))
+	if (ATTR_CFG_GET_FLD(attr, deprecated_timestamp) ||
+	    ATTR_CFG_GET_FLD(attr, timestamp))
 		config->ctrl |= ETMCR_TIMESTAMP_EN;
 
+	if (ATTR_CFG_GET_FLD(attr, timestamp) > 1) {
+		dev_dbg(&drvdata->csdev->dev,
+			"timestamp format attribute should be 0 (off) or 1 (on)\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * Possible to have cores with PTM (supports ret stack) and ETM (never
 	 * has ret stack) on the same SoC. So only enable when it can be honored
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 1aa0357a5ce7..d4e294cd48ae 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/perf/arm_pmu.h>
 #include <linux/platform_device.h>
@@ -800,7 +801,8 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
 			cc_threshold = drvdata->ccitmin;
 		config->ccctlr = cc_threshold;
 	}
-	if (ATTR_CFG_GET_FLD(attr, timestamp)) {
+	if (ATTR_CFG_GET_FLD(attr, deprecated_timestamp) ||
+	    ATTR_CFG_GET_FLD(attr, timestamp)) {
 		/*
 		 * Configure timestamps to be emitted at regular intervals in
 		 * order to correlate instructions executed on different CPUs

-- 
2.34.1

[PATCH v4 12/13] coresight: Allow setting the timestamp interval

[James Clark]

Timestamps are currently emitted at the maximum rate possible, which is
much too frequent for most use cases. Set the interval using the value
from the timestamp field. Granular control is not required, so save
space in the config by interpreting it as 2 ^ timestamp. And then 4
bits (0 - 15) is enough to set the interval to be larger than the
existing SYNC timestamp interval.

No sysfs mode support is needed for this attribute because counter
generated timestamps are only configured for Perf mode.

Reviewed-by: Leo Yan <leo.yan@arm.com>
Tested-by: Leo Yan <leo.yan@arm.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.h   |  1 +
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 28 +++++++++++++++-------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 24d929428633..128f80bb1443 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -7,6 +7,7 @@
 #ifndef _CORESIGHT_ETM_PERF_H
 #define _CORESIGHT_ETM_PERF_H
 
+#include <linux/bits.h>
 #include <linux/percpu-defs.h>
 #include "coresight-priv.h"
 
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index d4e294cd48ae..cb6f08510dc0 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -660,7 +660,7 @@ static void etm4_enable_sysfs_smp_call(void *info)
  *  +--------------+
  *         |
  *  +------v-------+
- *  | Counter x    |   (reload to 1 on underflow)
+ *  | Counter x    |   (reload to 2 ^ timestamp on underflow)
  *  +--------------+
  *         |
  *  +------v--------------+
@@ -671,11 +671,25 @@ static void etm4_enable_sysfs_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;
+	struct perf_event_attr max_timestamp = {
+		.ATTR_CFG_FLD_timestamp_CFG = U64_MAX,
+	};
+
+	/* timestamp may be 0 if deprecated_timestamp is used, so make min 1 */
+	u8 ts_level = max(1, ATTR_CFG_GET_FLD(attr, timestamp));
+
+	/*
+	 * Disable counter generated timestamps when timestamp == MAX. Leave
+	 * only SYNC timestamps.
+	 */
+	if (ts_level == ATTR_CFG_GET_FLD(&max_timestamp, timestamp))
+		return 0;
 
 	/* No point in trying if we don't have at least one counter */
 	if (!drvdata->nr_cntr)
@@ -713,12 +727,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 - 1);
 
 	/*
 	 * Trace Counter Control Register TRCCNTCTLRn
@@ -808,7 +818,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

-- 
2.34.1

[PATCH v4 13/13] coresight: docs: Document etm4x timestamp interval option

[James Clark]

Document how the new field is used, maximum value and the interaction
with SYNC timestamps.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 Documentation/trace/coresight/coresight.rst | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
index 806699871b80..80b5ed09d69b 100644
--- a/Documentation/trace/coresight/coresight.rst
+++ b/Documentation/trace/coresight/coresight.rst
@@ -613,8 +613,19 @@ They are also listed in the folder /sys/bus/event_source/devices/cs_etm/format/
      - Session local version of the system wide setting: :ref:`ETM_MODE_RETURNSTACK
        <coresight-return-stack>`
    * - timestamp
-     - Session local version of the system wide setting: :ref:`ETMv4_MODE_TIMESTAMP
-       <coresight-timestamp>`
+     - Controls generation and interval of timestamps.
+
+       0 = off, 1 = maximum interval .. 15 = minimum interval.
+
+       Values 1 - 14 use a counter that decrements every cycle to generate a
+       timestamp on underflow. The reload value for the counter is 2 raised to
+       the power of timestamp interval - 1. If the value is 1 then the reload
+       value is 1, if the value is 11 then the reload value is 1024 etc.
+
+       Setting the minimum interval (15) will disable the counter generated
+       timestamps, freeing the counter resource, leaving only ones emitted when
+       a SYNC packet is generated for every 4096 bytes of trace.
+
    * - cc_threshold
      - Cycle count threshold value. If nothing is provided here or the provided value is 0, then the
        default value i.e 0x100 will be used. If provided value is less than minimum cycles threshold

-- 
2.34.1

Re: [PATCH v4 13/13] coresight: docs: Document etm4x timestamp interval option

[Randy Dunlap]

On 11/12/25 7:22 AM, James Clark wrote:
> Document how the new field is used, maximum value and the interaction
> with SYNC timestamps.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  Documentation/trace/coresight/coresight.rst | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
> index 806699871b80..80b5ed09d69b 100644
> --- a/Documentation/trace/coresight/coresight.rst
> +++ b/Documentation/trace/coresight/coresight.rst
> @@ -613,8 +613,19 @@ They are also listed in the folder /sys/bus/event_source/devices/cs_etm/format/
>       - Session local version of the system wide setting: :ref:`ETM_MODE_RETURNSTACK
>         <coresight-return-stack>`
>     * - timestamp
> -     - Session local version of the system wide setting: :ref:`ETMv4_MODE_TIMESTAMP
> -       <coresight-timestamp>`
> +     - Controls generation and interval of timestamps.
> +
> +       0 = off, 1 = maximum interval .. 15 = minimum interval.
> +
> +       Values 1 - 14 use a counter that decrements every cycle to generate a
> +       timestamp on underflow. The reload value for the counter is 2 raised to
> +       the power of timestamp interval - 1. If the value is 1 then the reload
> +       value is 1, if the value is 11 then the reload value is 1024 etc.

	  value = 11: 2^11-1 = 2047

Maybe add some parens?


-- 
~Randy

Re: [PATCH v4 13/13] coresight: docs: Document etm4x timestamp interval option

[James Clark]

On 14/11/2025 5:02 am, Randy Dunlap wrote:
> 
> 
> On 11/12/25 7:22 AM, James Clark wrote:
>> Document how the new field is used, maximum value and the interaction
>> with SYNC timestamps.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   Documentation/trace/coresight/coresight.rst | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
>> index 806699871b80..80b5ed09d69b 100644
>> --- a/Documentation/trace/coresight/coresight.rst
>> +++ b/Documentation/trace/coresight/coresight.rst
>> @@ -613,8 +613,19 @@ They are also listed in the folder /sys/bus/event_source/devices/cs_etm/format/
>>        - Session local version of the system wide setting: :ref:`ETM_MODE_RETURNSTACK
>>          <coresight-return-stack>`
>>      * - timestamp
>> -     - Session local version of the system wide setting: :ref:`ETMv4_MODE_TIMESTAMP
>> -       <coresight-timestamp>`
>> +     - Controls generation and interval of timestamps.
>> +
>> +       0 = off, 1 = maximum interval .. 15 = minimum interval.
>> +
>> +       Values 1 - 14 use a counter that decrements every cycle to generate a
>> +       timestamp on underflow. The reload value for the counter is 2 raised to
>> +       the power of timestamp interval - 1. If the value is 1 then the reload
>> +       value is 1, if the value is 11 then the reload value is 1024 etc.
> 
> 	  value = 11: 2^11-1 = 2047
> 
> Maybe add some parens?
> 
> 

Will do. I had them but I removed them because it was in a sentence so 
it made it look like a side note. I think I'll change from words to symbols:

   The reload value for the counter is 2 ^ (interval - 1).

Thanks
James

Re: [PATCH v4 01/13] coresight: Change syncfreq to be a u8

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:07PM +0000, James Clark wrote:
> TRCSYNCPR.PERIOD is the only functional part of TRCSYNCPR and it only
> has 5 valid bits so it can be stored in a u8.
> 
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Leo Yan <leo.yan@arm.com>

> ---
>  drivers/hwtracing/coresight/coresight-etm4x.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 8d4a1f0f1e52..56a359184c6f 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -826,7 +826,6 @@ struct etmv4_config {
>  	u32				eventctrl1;
>  	u32				stall_ctrl;
>  	u32				ts_ctrl;
> -	u32				syncfreq;
>  	u32				ccctlr;
>  	u32				bb_ctrl;
>  	u32				vinst_ctrl;
> @@ -834,6 +833,7 @@ struct etmv4_config {
>  	u32				vissctlr;
>  	u32				vipcssctlr;
>  	u8				seq_idx;
> +	u8				syncfreq;
>  	u32				seq_ctrl[ETM_MAX_SEQ_STATES];
>  	u32				seq_rst;
>  	u32				seq_state;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 04/13] coresight: Hide unused ETMv3 format attributes

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:10PM +0000, James Clark wrote:
> ETMv3 only has a few attributes, and setting unused ones results in an
> error, so hide them to begin with.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 17afa0f4cdee..91132abca244 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -106,8 +106,27 @@ static struct attribute *etm_config_formats_attr[] = {
>  	NULL,
>  };
>  
> +static umode_t etm_format_attr_is_visible(struct kobject *kobj,
> +					  struct attribute *attr, int unused)
> +{
> +	/* ETM4 has all attributes */
> +	if (IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
> +		return attr->mode;
> +
> +	/* ETM3 only has these attributes */
> +	if (attr == &format_attr_cycacc.attr ||
> +	    attr == &format_attr_timestamp.attr ||
> +	    attr == &format_attr_retstack.attr ||
> +	    attr == &format_attr_sinkid.attr ||
> +	    attr == &format_attr_configid.attr)

Do we support configid for ETM3?

> +		return attr->mode;

It is good to give a bit information why only these attributes
supported, e.g.,

  /*
   * ETM3 only support subset attributes (see ETM3X_SUPPORTED_OPTIONS),
   * keep 'sinkid' attr for specifying sink ID.
   */

Two side topics but not issues caused this patch:

- Since CTXTID/CTXTID2 is only used for ETM4x, we can remove "#if
  IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)" from the
  format_attr_contextid_show() function.

- etm_parse_event_config() does not check attr->config3, thus user
  sets 'cc_threshold' it will slip without any error report.

Thanks,
Leo

> +
> +	return 0;
> +}
> +
>  static const struct attribute_group etm_pmu_format_group = {
>  	.name   = "format",
> +	.is_visible = etm_format_attr_is_visible,
>  	.attrs  = etm_config_formats_attr,
>  };
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR()

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:11PM +0000, James Clark wrote:
> This allows us to define and consume them in a unified way in later
> commits.
> 
> A lot of the existing code has open coded bit shifts or direct usage of
> whole config values which is error prone and hides which bits are in use
> and which are free.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Leo Yan <leo.yan@arm.com>

> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 22 ++++++++---------
>  drivers/hwtracing/coresight/coresight-etm-perf.h | 31 ++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 91132abca244..d19e2568a0d1 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -13,6 +13,7 @@
>  #include <linux/mm.h>
>  #include <linux/init.h>
>  #include <linux/perf_event.h>
> +#include <linux/perf/arm_pmu.h>
>  #include <linux/percpu-defs.h>
>  #include <linux/slab.h>
>  #include <linux/stringhash.h>
> @@ -54,22 +55,21 @@ static DEFINE_PER_CPU(struct coresight_device *, csdev_src);
>   * The PMU formats were orignally for ETMv3.5/PTM's ETMCR 'config';
>   * now take them as general formats and apply on all ETMs.
>   */
> -PMU_FORMAT_ATTR(branch_broadcast, "config:"__stringify(ETM_OPT_BRANCH_BROADCAST));
> -PMU_FORMAT_ATTR(cycacc,		"config:" __stringify(ETM_OPT_CYCACC));
> +GEN_PMU_FORMAT_ATTR(branch_broadcast);
> +GEN_PMU_FORMAT_ATTR(cycacc);
>  /* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
> -PMU_FORMAT_ATTR(contextid1,	"config:" __stringify(ETM_OPT_CTXTID));
> +GEN_PMU_FORMAT_ATTR(contextid1);
>  /* contextid2 enables tracing CONTEXTIDR_EL2 for ETMv4 */
> -PMU_FORMAT_ATTR(contextid2,	"config:" __stringify(ETM_OPT_CTXTID2));
> -PMU_FORMAT_ATTR(timestamp,	"config:" __stringify(ETM_OPT_TS));
> -PMU_FORMAT_ATTR(retstack,	"config:" __stringify(ETM_OPT_RETSTK));
> +GEN_PMU_FORMAT_ATTR(contextid2);
> +GEN_PMU_FORMAT_ATTR(timestamp);
> +GEN_PMU_FORMAT_ATTR(retstack);
>  /* preset - if sink ID is used as a configuration selector */
> -PMU_FORMAT_ATTR(preset,		"config:0-3");
> +GEN_PMU_FORMAT_ATTR(preset);
>  /* Sink ID - same for all ETMs */
> -PMU_FORMAT_ATTR(sinkid,		"config2:0-31");
> +GEN_PMU_FORMAT_ATTR(sinkid);
>  /* config ID - set if a system configuration is selected */
> -PMU_FORMAT_ATTR(configid,	"config2:32-63");
> -PMU_FORMAT_ATTR(cc_threshold,	"config3:0-11");
> -
> +GEN_PMU_FORMAT_ATTR(configid);
> +GEN_PMU_FORMAT_ATTR(cc_threshold);
>  
>  /*
>   * contextid always traces the "PID".  The PID is in CONTEXTIDR_EL1
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
> index 5febbcdb8696..c794087a0e99 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
> @@ -20,6 +20,37 @@ struct cscfg_config_desc;
>   */
>  #define ETM_ADDR_CMP_MAX	8
>  
> +#define ATTR_CFG_FLD_preset_CFG			config
> +#define ATTR_CFG_FLD_preset_LO			0
> +#define ATTR_CFG_FLD_preset_HI			3
> +#define ATTR_CFG_FLD_branch_broadcast_CFG	config
> +#define ATTR_CFG_FLD_branch_broadcast_LO	8
> +#define ATTR_CFG_FLD_branch_broadcast_HI	8
> +#define ATTR_CFG_FLD_cycacc_CFG			config
> +#define ATTR_CFG_FLD_cycacc_LO			12
> +#define ATTR_CFG_FLD_cycacc_HI			12
> +#define ATTR_CFG_FLD_contextid1_CFG		config
> +#define ATTR_CFG_FLD_contextid1_LO		14
> +#define ATTR_CFG_FLD_contextid1_HI		14
> +#define ATTR_CFG_FLD_contextid2_CFG		config
> +#define ATTR_CFG_FLD_contextid2_LO		15
> +#define ATTR_CFG_FLD_contextid2_HI		15
> +#define ATTR_CFG_FLD_timestamp_CFG		config
> +#define ATTR_CFG_FLD_timestamp_LO		28
> +#define ATTR_CFG_FLD_timestamp_HI		28
> +#define ATTR_CFG_FLD_retstack_CFG		config
> +#define ATTR_CFG_FLD_retstack_LO		29
> +#define ATTR_CFG_FLD_retstack_HI		29
> +#define ATTR_CFG_FLD_sinkid_CFG			config2
> +#define ATTR_CFG_FLD_sinkid_LO			0
> +#define ATTR_CFG_FLD_sinkid_HI			31
> +#define ATTR_CFG_FLD_configid_CFG		config2
> +#define ATTR_CFG_FLD_configid_LO		32
> +#define ATTR_CFG_FLD_configid_HI		63
> +#define ATTR_CFG_FLD_cc_threshold_CFG		config3
> +#define ATTR_CFG_FLD_cc_threshold_LO		0
> +#define ATTR_CFG_FLD_cc_threshold_HI		11
> +
>  /**
>   * struct etm_filter - single instruction range or start/stop configuration.
>   * @start_addr:	The address to start tracing on.
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD()

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:12PM +0000, James Clark wrote:
> Currently we're programming attr->config directly into ETMCR after some
> validation. This obscures which fields are being used, and also makes it
> impossible to move fields around or use other configN fields in the
> future.
> 
> Improve it by only reading the fields that are valid and then setting
> the appropriate ETMCR bits based on each one.
> 
> The ETMCR_CTXID_SIZE part can be removed as it was never a valid option
> because it's not in ETM3X_SUPPORTED_OPTIONS.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>

LGTM:

Reviewed-by: Leo Yan <leo.yan@arm.com>

> ---
>  drivers/hwtracing/coresight/coresight-etm3x-core.c | 24 ++++++++++++----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index a5e809589d3e..4511fc2f8d72 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -28,6 +28,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/clk.h>
>  #include <linux/perf_event.h>
> +#include <linux/perf/arm_pmu.h>
>  #include <asm/sections.h>
>  
>  #include "coresight-etm.h"
> @@ -339,21 +340,22 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
>  	if (attr->config & ~ETM3X_SUPPORTED_OPTIONS)
>  		return -EINVAL;
>  
> -	config->ctrl = attr->config;
> +	config->ctrl = 0;
>  
> -	/* Don't trace contextID when runs in non-root PID namespace */
> -	if (!task_is_in_init_pid_ns(current))
> -		config->ctrl &= ~ETMCR_CTXID_SIZE;
> +	if (ATTR_CFG_GET_FLD(attr, cycacc))
> +		config->ctrl |= ETMCR_CYC_ACC;
> +
> +	if (ATTR_CFG_GET_FLD(attr, timestamp))
> +		config->ctrl |= ETMCR_TIMESTAMP_EN;
>  
>  	/*
> -	 * Possible to have cores with PTM (supports ret stack) and ETM
> -	 * (never has ret stack) on the same SoC. So if we have a request
> -	 * for return stack that can't be honoured on this core then
> -	 * clear the bit - trace will still continue normally
> +	 * Possible to have cores with PTM (supports ret stack) and ETM (never
> +	 * has ret stack) on the same SoC. So only enable when it can be honored
> +	 * - trace will still continue normally otherwise.
>  	 */
> -	if ((config->ctrl & ETMCR_RETURN_STACK) &&
> -	    !(drvdata->etmccer & ETMCCER_RETSTACK))
> -		config->ctrl &= ~ETMCR_RETURN_STACK;
> +	if (ATTR_CFG_GET_FLD(attr, retstack) &&
> +	    (drvdata->etmccer & ETMCCER_RETSTACK))
> +		config->ctrl |= ETMCR_RETURN_STACK;
>  
>  	return 0;
>  }
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 07/13] coresight: Don't reject unrecognized ETMv3 format attributes

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:13PM +0000, James Clark wrote:
> config isn't the only field, there are also config1, config2, etc.
> Rejecting unrecognized attributes is therefore inconsistent as it wasn't
> done for all fields. It was only necessary when we were directly
> programming attr->config into ETMCR and didn't hide the unsupported
> fields, but now it's not needed so remove it.

It is fine for not validating all configs (please ignore my comment for
checking all configs in my reply for patch 04).

I am wandering if need to remove ETM3X_SUPPORTED_OPTIONS totally. I
saw etm_enable_hw() uses it to clear config bits, so it makes sense to
keep it.

Reviewed-by: Leo Yan <leo.yan@arm.com>

> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm3x-core.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 4511fc2f8d72..584d653eda81 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -333,13 +333,6 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
>  	if (config->mode)
>  		etm_config_trace_mode(config);
>  
> -	/*
> -	 * At this time only cycle accurate, return stack  and timestamp
> -	 * options are available.
> -	 */
> -	if (attr->config & ~ETM3X_SUPPORTED_OPTIONS)
> -		return -EINVAL;
> -
>  	config->ctrl = 0;
>  
>  	if (ATTR_CFG_GET_FLD(attr, cycacc))
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 07/13] coresight: Don't reject unrecognized ETMv3 format attributes

[James Clark]

On 14/11/2025 3:18 pm, Leo Yan wrote:
> On Wed, Nov 12, 2025 at 03:22:13PM +0000, James Clark wrote:
>> config isn't the only field, there are also config1, config2, etc.
>> Rejecting unrecognized attributes is therefore inconsistent as it wasn't
>> done for all fields. It was only necessary when we were directly
>> programming attr->config into ETMCR and didn't hide the unsupported
>> fields, but now it's not needed so remove it.
> 
> It is fine for not validating all configs (please ignore my comment for
> checking all configs in my reply for patch 04).
> 
> I am wandering if need to remove ETM3X_SUPPORTED_OPTIONS totally. I
> saw etm_enable_hw() uses it to clear config bits, so it makes sense to
> keep it.
> 
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 

I tried to remove it, but as you saw it's used to clear the old config. 
It wasn't obvious to me if there was some other state in ETMCR that 
needed to be preserved, so only selected bits are cleared. I assumed 
there was a reason it wasn't just overwritten with config->ctrl to begin 
with as that would have been much simpler.

There wasn't a comment and I didn't want to spend much more time digging 
so I just left it. I think it's harmless to leave for now.

>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm3x-core.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> index 4511fc2f8d72..584d653eda81 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> @@ -333,13 +333,6 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
>>   	if (config->mode)
>>   		etm_config_trace_mode(config);
>>   
>> -	/*
>> -	 * At this time only cycle accurate, return stack  and timestamp
>> -	 * options are available.
>> -	 */
>> -	if (attr->config & ~ETM3X_SUPPORTED_OPTIONS)
>> -		return -EINVAL;
>> -
>>   	config->ctrl = 0;
>>   
>>   	if (ATTR_CFG_GET_FLD(attr, cycacc))
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v4 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:14PM +0000, James Clark wrote:
> The "config:" string construction in format_attr_contextid_show() can be
> removed because it either showed the existing context1 or context2
> formats which have already been generated, so can be called themselves.
> 
> The other conversions are straightforward replacements.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index d19e2568a0d1..7272e758aebf 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -80,12 +80,11 @@ static ssize_t format_attr_contextid_show(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *page)
>  {
> -	int pid_fmt = ETM_OPT_CTXTID;
> -
>  #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> -	pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
> +	if (is_kernel_in_hyp_mode())
> +		return contextid2_show(dev, attr, page);
>  #endif

As said, this function now is only used for ETM4, we can remove "#if
IS_ENABLED(...)".

Otherwise, LGTM:

Reviewed-by: Leo Yan <leo.yan@arm.com>

Re: [PATCH v4 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()

[James Clark]

On 14/11/2025 3:30 pm, Leo Yan wrote:
> On Wed, Nov 12, 2025 at 03:22:14PM +0000, James Clark wrote:
>> The "config:" string construction in format_attr_contextid_show() can be
>> removed because it either showed the existing context1 or context2
>> formats which have already been generated, so can be called themselves.
>>
>> The other conversions are straightforward replacements.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm-perf.c | 18 ++++++++----------
>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index d19e2568a0d1..7272e758aebf 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -80,12 +80,11 @@ static ssize_t format_attr_contextid_show(struct device *dev,
>>   					  struct device_attribute *attr,
>>   					  char *page)
>>   {
>> -	int pid_fmt = ETM_OPT_CTXTID;
>> -
>>   #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
>> -	pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
>> +	if (is_kernel_in_hyp_mode())
>> +		return contextid2_show(dev, attr, page);
>>   #endif
> 
> As said, this function now is only used for ETM4, we can remove "#if
> IS_ENABLED(...)".
> 
> Otherwise, LGTM:
> 
> Reviewed-by: Leo Yan <leo.yan@arm.com>

Unfortunately it's still needed to make the build work. 
is_kernel_in_hyp_mode() results in an undefined symbol when building for 
arm32 so it needs to be ifdef'd out. I can add a comment though.

Re: [PATCH v4 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()

[Leo Yan]

On Fri, Nov 14, 2025 at 03:34:32PM +0000, James Clark wrote:

[...]

> > > @@ -80,12 +80,11 @@ static ssize_t format_attr_contextid_show(struct device *dev,
> > >   					  struct device_attribute *attr,
> > >   					  char *page)
> > >   {
> > > -	int pid_fmt = ETM_OPT_CTXTID;
> > > -
> > >   #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> > > -	pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
> > > +	if (is_kernel_in_hyp_mode())
> > > +		return contextid2_show(dev, attr, page);
> > >   #endif
> > 
> > As said, this function now is only used for ETM4, we can remove "#if
> > IS_ENABLED(...)".
> > 
> > Otherwise, LGTM:
> > 
> > Reviewed-by: Leo Yan <leo.yan@arm.com>
> 
> Unfortunately it's still needed to make the build work.
> is_kernel_in_hyp_mode() results in an undefined symbol when building for
> arm32 so it needs to be ifdef'd out. I can add a comment though.

Maybe "#ifdef CONFIG_ARM64" is more suitable?

Re: [PATCH v4 09/13] coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD()

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:15PM +0000, James Clark wrote:
> Remove hard coded bitfield extractions and shifts and replace with
> ATTR_CFG_GET_FLD().
> 
> ETM4_CFG_BIT_BB was defined to give the register bit positions to
> userspace, TRCCONFIGR_BB should be used in the kernel so replace it.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 44 ++++++++++------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 380a7840adb8..1aa0357a5ce7 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -29,6 +29,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/uaccess.h>
>  #include <linux/perf_event.h>
> +#include <linux/perf/arm_pmu.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
> @@ -789,17 +790,17 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>  		goto out;
>  
>  	/* Go from generic option to ETMv4 specifics */
> -	if (attr->config & BIT(ETM_OPT_CYCACC)) {
> +	if (ATTR_CFG_GET_FLD(attr, cycacc)) {
>  		config->cfg |= TRCCONFIGR_CCI;
>  		/* TRM: Must program this for cycacc to work */
> -		cc_threshold = attr->config3 & ETM_CYC_THRESHOLD_MASK;
> +		cc_threshold = ATTR_CFG_GET_FLD(attr, cc_threshold);
>  		if (!cc_threshold)
>  			cc_threshold = ETM_CYC_THRESHOLD_DEFAULT;
>  		if (cc_threshold < drvdata->ccitmin)
>  			cc_threshold = drvdata->ccitmin;
>  		config->ccctlr = cc_threshold;
>  	}
> -	if (attr->config & BIT(ETM_OPT_TS)) {
> +	if (ATTR_CFG_GET_FLD(attr, timestamp)) {
>  		/*
>  		 * Configure timestamps to be emitted at regular intervals in
>  		 * order to correlate instructions executed on different CPUs
> @@ -819,17 +820,17 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>  	}
>  
>  	/* Only trace contextID when runs in root PID namespace */
> -	if ((attr->config & BIT(ETM_OPT_CTXTID)) &&
> +	if (ATTR_CFG_GET_FLD(attr, contextid1) &&
>  	    task_is_in_init_pid_ns(current))
>  		/* bit[6], Context ID tracing bit */
>  		config->cfg |= TRCCONFIGR_CID;
>  
>  	/*
> -	 * If set bit ETM_OPT_CTXTID2 in perf config, this asks to trace VMID
> -	 * for recording CONTEXTIDR_EL2.  Do not enable VMID tracing if the
> -	 * kernel is not running in EL2.
> +	 * If set bit contextid2 in perf config, this asks to trace VMID for
> +	 * recording CONTEXTIDR_EL2.  Do not enable VMID tracing if the kernel
> +	 * is not running in EL2.
>  	 */
> -	if (attr->config & BIT(ETM_OPT_CTXTID2)) {
> +	if (ATTR_CFG_GET_FLD(attr, contextid2)) {
>  		if (!is_kernel_in_hyp_mode()) {
>  			ret = -EINVAL;
>  			goto out;
> @@ -840,26 +841,22 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>  	}
>  
>  	/* return stack - enable if selected and supported */
> -	if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack)
> +	if (ATTR_CFG_GET_FLD(attr, retstack) && drvdata->retstack)
>  		/* bit[12], Return stack enable bit */
>  		config->cfg |= TRCCONFIGR_RS;
>  
>  	/*
> -	 * Set any selected configuration and preset.
> -	 *
> -	 * This extracts the values of PMU_FORMAT_ATTR(configid) and PMU_FORMAT_ATTR(preset)
> -	 * in the perf attributes defined in coresight-etm-perf.c.
> -	 * configid uses bits 63:32 of attr->config2, preset uses bits 3:0 of attr->config.
> -	 * A zero configid means no configuration active, preset = 0 means no preset selected.
> +	 * Set any selected configuration and preset. A zero configid means no
> +	 * configuration active, preset = 0 means no preset selected.
>  	 */
> -	if (attr->config2 & GENMASK_ULL(63, 32)) {
> -		cfg_hash = (u32)(attr->config2 >> 32);
> -		preset = attr->config & 0xF;
> +	if (ATTR_CFG_GET_FLD(attr, configid)) {
> +		cfg_hash = ATTR_CFG_GET_FLD(attr, configid);

Nitpick:


    cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
    if (cfg_hash) {

Then we can avoid a redundant ATTR_CFG_GET_FLD().

Otherwise:

Reviewed-by: Leo Yan <leo.yan@arm.com>

> +		preset = ATTR_CFG_GET_FLD(attr, preset);
>  		ret = cscfg_csdev_enable_active_config(csdev, cfg_hash, preset);
>  	}
>  
>  	/* branch broadcast - enable if selected and supported */
> -	if (attr->config & BIT(ETM_OPT_BRANCH_BROADCAST)) {
> +	if (ATTR_CFG_GET_FLD(attr, branch_broadcast)) {
>  		if (!drvdata->trcbb) {
>  			/*
>  			 * Missing BB support could cause silent decode errors
> @@ -868,7 +865,7 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>  			ret = -EINVAL;
>  			goto out;
>  		} else {
> -			config->cfg |= BIT(ETM4_CFG_BIT_BB);
> +			config->cfg |= TRCCONFIGR_BB;
>  		}
>  	}
>  
> @@ -1104,11 +1101,8 @@ static int etm4_disable_perf(struct coresight_device *csdev,
>  		return -EINVAL;
>  
>  	etm4_disable_hw(drvdata);
> -	/*
> -	 * The config_id occupies bits 63:32 of the config2 perf event attr
> -	 * field. If this is non-zero then we will have enabled a config.
> -	 */
> -	if (attr->config2 & GENMASK_ULL(63, 32))
> +	/* If configid is non-zero then we will have enabled a config. */
> +	if (ATTR_CFG_GET_FLD(attr, configid))
>  		cscfg_csdev_disable_active_config(csdev);
>  
>  	/*
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 10/13] coresight: Remove misleading definitions

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:16PM +0000, James Clark wrote:
> ETM_OPT_* definitions duplicate the PMU format attributes that have
> always been published in sysfs. Hardcoding them here makes it misleading
> as to what the 'real' PMU API is and prevents attributes from being
> rearranged in the future.
> 
> ETM4_CFG_BIT_* definitions just define what the Arm Architecture is
> which is not the responsibility of the kernel to do and doesn't scale to
> other registers or versions of ETM. It's not an actual software ABI/API
> and these definitions here mislead that it is.
> 
> Any tools using the first ones would be broken anyway as they won't work
> when attributes are moved, so removing them is the right thing to do and
> will prompt a fix. Tools using the second ones can trivially redefine
> them locally.
> 
> Perf also has its own copy of the headers so both of these things can be
> fixed up at a later date.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>

Good cleanup for me!

Reviewed-by: Leo Yan <leo.yan@arm.com>

> ---
>  include/linux/coresight-pmu.h | 24 ------------------------
>  1 file changed, 24 deletions(-)
> 
> diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h
> index 89b0ac0014b0..2e179abe472a 100644
> --- a/include/linux/coresight-pmu.h
> +++ b/include/linux/coresight-pmu.h
> @@ -21,30 +21,6 @@
>   */
>  #define CORESIGHT_LEGACY_CPU_TRACE_ID(cpu)  (0x10 + (cpu * 2))
>  
> -/*
> - * Below are the definition of bit offsets for perf option, and works as
> - * arbitrary values for all ETM versions.
> - *
> - * Most of them are orignally from ETMv3.5/PTM's ETMCR config, therefore,
> - * ETMv3.5/PTM doesn't define ETMCR config bits with prefix "ETM3_" and
> - * directly use below macros as config bits.
> - */
> -#define ETM_OPT_BRANCH_BROADCAST 8
> -#define ETM_OPT_CYCACC		12
> -#define ETM_OPT_CTXTID		14
> -#define ETM_OPT_CTXTID2		15
> -#define ETM_OPT_TS		28
> -#define ETM_OPT_RETSTK		29
> -
> -/* ETMv4 CONFIGR programming bits for the ETM OPTs */
> -#define ETM4_CFG_BIT_BB         3
> -#define ETM4_CFG_BIT_CYCACC	4
> -#define ETM4_CFG_BIT_CTXTID	6
> -#define ETM4_CFG_BIT_VMID	7
> -#define ETM4_CFG_BIT_TS		11
> -#define ETM4_CFG_BIT_RETSTK	12
> -#define ETM4_CFG_BIT_VMID_OPT	15
> -
>  /*
>   * Interpretation of the PERF_RECORD_AUX_OUTPUT_HW_ID payload.
>   * Used to associate a CPU with the CoreSight Trace ID.
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 11/13] coresight: Extend width of timestamp format attribute

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:17PM +0000, James Clark wrote:
> 'timestamp' is currently 1 bit wide for on/off. To enable setting
> different intervals in a later commit, extend it to 4 bits wide. Keep
> the old bit position for backward compatibility but don't publish in the
> format/ folder. It will be removed from the documentation and can be
> removed completely after enough time has passed.
> 
> ETM3x doesn't support different intervals, so validate that the value is
> either 0 or 1.
> 
> Tools that read the bit positions from the format/ folder will continue
> to work as before, setting either 0 or 1 for off/on. Tools that
> incorrectly didn't do this and set the ETM_OPT_TS bit directly will also
> continue to work because that old bit is still checked.
> 
> This avoids adding a second timestamp attribute for setting the
> interval. This would be awkward to use because tools would have to be
> updated to ensure that the timestamps are always enabled when an
> interval is set, and the driver would have to validate that both options
> are provided together. All this does is implement the semantics of a
> single enum but spread over multiple fields.

Just a bit thoughts.  My understanding is that the PMU format would
clearly represent the PMU characteristics.  I imagine that after reading
the ETM specification, someone should be able to easily map to the PMU
formats (enable field and counter field separately).

Alternatively, this patch provides a user-friendly interface that allows
all settings (enable + counter) in one go.  Users can benefit from it
without knowing the underlying hardware mechanism, but might need to
digest its semantics.  As this will be well documented, I am fine for
current approach:

Reviewed-by: Leo Yan <leo.yan@arm.com>


> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.h   | 13 ++++++++++---
>  drivers/hwtracing/coresight/coresight-etm3x-core.c |  9 ++++++++-
>  drivers/hwtracing/coresight/coresight-etm4x-core.c |  4 +++-
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
> index c794087a0e99..24d929428633 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
> @@ -23,6 +23,9 @@ struct cscfg_config_desc;
>  #define ATTR_CFG_FLD_preset_CFG			config
>  #define ATTR_CFG_FLD_preset_LO			0
>  #define ATTR_CFG_FLD_preset_HI			3
> +#define ATTR_CFG_FLD_timestamp_CFG		config
> +#define ATTR_CFG_FLD_timestamp_LO		4
> +#define ATTR_CFG_FLD_timestamp_HI		7
>  #define ATTR_CFG_FLD_branch_broadcast_CFG	config
>  #define ATTR_CFG_FLD_branch_broadcast_LO	8
>  #define ATTR_CFG_FLD_branch_broadcast_HI	8
> @@ -35,9 +38,13 @@ struct cscfg_config_desc;
>  #define ATTR_CFG_FLD_contextid2_CFG		config
>  #define ATTR_CFG_FLD_contextid2_LO		15
>  #define ATTR_CFG_FLD_contextid2_HI		15
> -#define ATTR_CFG_FLD_timestamp_CFG		config
> -#define ATTR_CFG_FLD_timestamp_LO		28
> -#define ATTR_CFG_FLD_timestamp_HI		28
> +/*
> + * Old position of 'timestamp' and not published in sysfs. Remove at a later
> + * date if necessary.
> + */
> +#define ATTR_CFG_FLD_deprecated_timestamp_CFG	config
> +#define ATTR_CFG_FLD_deprecated_timestamp_LO	28
> +#define ATTR_CFG_FLD_deprecated_timestamp_HI	28
>  #define ATTR_CFG_FLD_retstack_CFG		config
>  #define ATTR_CFG_FLD_retstack_LO		29
>  #define ATTR_CFG_FLD_retstack_HI		29
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 584d653eda81..d4c04e563bf6 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -338,9 +338,16 @@ static int etm_parse_event_config(struct etm_drvdata *drvdata,
>  	if (ATTR_CFG_GET_FLD(attr, cycacc))
>  		config->ctrl |= ETMCR_CYC_ACC;
>  
> -	if (ATTR_CFG_GET_FLD(attr, timestamp))
> +	if (ATTR_CFG_GET_FLD(attr, deprecated_timestamp) ||
> +	    ATTR_CFG_GET_FLD(attr, timestamp))
>  		config->ctrl |= ETMCR_TIMESTAMP_EN;
>  
> +	if (ATTR_CFG_GET_FLD(attr, timestamp) > 1) {
> +		dev_dbg(&drvdata->csdev->dev,
> +			"timestamp format attribute should be 0 (off) or 1 (on)\n");
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Possible to have cores with PTM (supports ret stack) and ETM (never
>  	 * has ret stack) on the same SoC. So only enable when it can be honored
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 1aa0357a5ce7..d4e294cd48ae 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/perf/arm_pmu.h>
>  #include <linux/platform_device.h>
> @@ -800,7 +801,8 @@ static int etm4_parse_event_config(struct coresight_device *csdev,
>  			cc_threshold = drvdata->ccitmin;
>  		config->ccctlr = cc_threshold;
>  	}
> -	if (ATTR_CFG_GET_FLD(attr, timestamp)) {
> +	if (ATTR_CFG_GET_FLD(attr, deprecated_timestamp) ||
> +	    ATTR_CFG_GET_FLD(attr, timestamp)) {
>  		/*
>  		 * Configure timestamps to be emitted at regular intervals in
>  		 * order to correlate instructions executed on different CPUs
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 13/13] coresight: docs: Document etm4x timestamp interval option

[Leo Yan]

On Wed, Nov 12, 2025 at 03:22:19PM +0000, James Clark wrote:
> Document how the new field is used, maximum value and the interaction
> with SYNC timestamps.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  Documentation/trace/coresight/coresight.rst | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
> index 806699871b80..80b5ed09d69b 100644
> --- a/Documentation/trace/coresight/coresight.rst
> +++ b/Documentation/trace/coresight/coresight.rst
> @@ -613,8 +613,19 @@ They are also listed in the folder /sys/bus/event_source/devices/cs_etm/format/
>       - Session local version of the system wide setting: :ref:`ETM_MODE_RETURNSTACK
>         <coresight-return-stack>`
>     * - timestamp
> -     - Session local version of the system wide setting: :ref:`ETMv4_MODE_TIMESTAMP
> -       <coresight-timestamp>`
> +     - Controls generation and interval of timestamps.
> +
> +       0 = off, 1 = maximum interval .. 15 = minimum interval.

I am struggling to understand the "maximum" and "minimum" usage here.

Seems to me, you are saying:

    1 = maximum frequency .. 15 = minimum frequency

> +
> +       Values 1 - 14 use a counter that decrements every cycle to generate a
> +       timestamp on underflow. The reload value for the counter is 2 raised to
> +       the power of timestamp interval - 1. If the value is 1 then the reload
> +       value is 1, if the value is 11 then the reload value is 1024 etc.

I saw your replying to Randy, yeah, the formula would be much clear.

> +
> +       Setting the minimum interval (15) will disable the counter generated
> +       timestamps, freeing the counter resource, leaving only ones emitted when
> +       a SYNC packet is generated for every 4096 bytes of trace.

"every 4096 bytes" is not always true.

Isn't this defined in TRCSYNCPR.PERIOD?  Maybe just say "... for every
period of trace defined in TRCSYNCPR.PERIOD", or even more general
"... for every period of trace".

Thanks,
Leo

> +
>     * - cc_threshold
>       - Cycle count threshold value. If nothing is provided here or the provided value is 0, then the
>         default value i.e 0x100 will be used. If provided value is less than minimum cycles threshold
> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 04/13] coresight: Hide unused ETMv3 format attributes

[James Clark]

On 14/11/2025 2:55 pm, Leo Yan wrote:
> On Wed, Nov 12, 2025 at 03:22:10PM +0000, James Clark wrote:
>> ETMv3 only has a few attributes, and setting unused ones results in an
>> error, so hide them to begin with.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm-perf.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 17afa0f4cdee..91132abca244 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -106,8 +106,27 @@ static struct attribute *etm_config_formats_attr[] = {
>>   	NULL,
>>   };
>>   
>> +static umode_t etm_format_attr_is_visible(struct kobject *kobj,
>> +					  struct attribute *attr, int unused)
>> +{
>> +	/* ETM4 has all attributes */
>> +	if (IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
>> +		return attr->mode;
>> +
>> +	/* ETM3 only has these attributes */
>> +	if (attr == &format_attr_cycacc.attr ||
>> +	    attr == &format_attr_timestamp.attr ||
>> +	    attr == &format_attr_retstack.attr ||
>> +	    attr == &format_attr_sinkid.attr ||
>> +	    attr == &format_attr_configid.attr)
> 
> Do we support configid for ETM3?
> 

They're "activated" in etm_setup_aux() so this would be for both ETM3 
and 4. But they're only subsequently "enabled" for ETM4. So no, they're 
not supported. I'll hide this attribute too.

I can leave it for a later fix if we want to stop activating them, but I 
don't think it does any harm.

>> +		return attr->mode;
> 
> It is good to give a bit information why only these attributes
> supported, e.g.,
> 
>    /*
>     * ETM3 only support subset attributes (see ETM3X_SUPPORTED_OPTIONS),
>     * keep 'sinkid' attr for specifying sink ID.
>     */
> 
> Two side topics but not issues caused this patch:
> 
> - Since CTXTID/CTXTID2 is only used for ETM4x, we can remove "#if
>    IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)" from the
>    format_attr_contextid_show() function.

Will change to CONFIG_ARM64 as mentioned on the other thread.

> 
> - etm_parse_event_config() does not check attr->config3, thus user
>    sets 'cc_threshold' it will slip without any error report.

Will ignore this comment as mentioned on patch 7, we're not doing any 
errors for unsupported arguments anymore.

> 
> Thanks,
> Leo
> 
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct attribute_group etm_pmu_format_group = {
>>   	.name   = "format",
>> +	.is_visible = etm_format_attr_is_visible,
>>   	.attrs  = etm_config_formats_attr,
>>   };
>>   
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v4 13/13] coresight: docs: Document etm4x timestamp interval option

[James Clark]

On 14/11/2025 6:05 pm, Leo Yan wrote:
> On Wed, Nov 12, 2025 at 03:22:19PM +0000, James Clark wrote:
>> Document how the new field is used, maximum value and the interaction
>> with SYNC timestamps.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   Documentation/trace/coresight/coresight.rst | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
>> index 806699871b80..80b5ed09d69b 100644
>> --- a/Documentation/trace/coresight/coresight.rst
>> +++ b/Documentation/trace/coresight/coresight.rst
>> @@ -613,8 +613,19 @@ They are also listed in the folder /sys/bus/event_source/devices/cs_etm/format/
>>        - Session local version of the system wide setting: :ref:`ETM_MODE_RETURNSTACK
>>          <coresight-return-stack>`
>>      * - timestamp
>> -     - Session local version of the system wide setting: :ref:`ETMv4_MODE_TIMESTAMP
>> -       <coresight-timestamp>`
>> +     - Controls generation and interval of timestamps.
>> +
>> +       0 = off, 1 = maximum interval .. 15 = minimum interval.
> 
> I am struggling to understand the "maximum" and "minimum" usage here.
> 
> Seems to me, you are saying:
> 
>      1 = maximum frequency .. 15 = minimum frequency
> 

Yes the documentation was the wrong way around. Will flip it.

>> +
>> +       Values 1 - 14 use a counter that decrements every cycle to generate a
>> +       timestamp on underflow. The reload value for the counter is 2 raised to
>> +       the power of timestamp interval - 1. If the value is 1 then the reload
>> +       value is 1, if the value is 11 then the reload value is 1024 etc.
> 
> I saw your replying to Randy, yeah, the formula would be much clear.
> 
>> +
>> +       Setting the minimum interval (15) will disable the counter generated
>> +       timestamps, freeing the counter resource, leaving only ones emitted when
>> +       a SYNC packet is generated for every 4096 bytes of trace.
> 
> "every 4096 bytes" is not always true.
> 
> Isn't this defined in TRCSYNCPR.PERIOD?  Maybe just say "... for every
> period of trace defined in TRCSYNCPR.PERIOD", or even more general
> "... for every period of trace".
> 
> Thanks,
> Leo
> 

It would only not be true when using a custom config. I can say that 
it's TRCSYNCPR.PERIOD but that the default is 4096 bytes.

>> +
>>      * - cc_threshold
>>        - Cycle count threshold value. If nothing is provided here or the provided value is 0, then the
>>          default value i.e 0x100 will be used. If provided value is less than minimum cycles threshold
>>
>> -- 
>> 2.34.1
>>