* [PATCH v5 01/13] coresight: Change syncfreq to be a u8
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-18 16:27 ` [PATCH v5 02/13] coresight: Repack struct etmv4_drvdata James Clark
` (11 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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>
Reviewed-by: Leo Yan <leo.yan@arm.com>
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 012c52fd1933..0287d19ce12e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -825,7 +825,6 @@ struct etmv4_config {
u32 eventctrl1;
u32 stall_ctrl;
u32 ts_ctrl;
- u32 syncfreq;
u32 ccctlr;
u32 bb_ctrl;
u32 vinst_ctrl;
@@ -833,6 +832,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v5 02/13] coresight: Repack struct etmv4_drvdata
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
2025-11-18 16:27 ` [PATCH v5 01/13] coresight: Change syncfreq to be a u8 James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-18 16:27 ` [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event() James Clark
` (10 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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 | 36 +++++++++++++--------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 0287d19ce12e..d178d79d9827 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -1016,27 +1016,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;
struct etmv4_save_state *save_state;
- bool skip_power_up;
- bool paused;
DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
};
--
2.34.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
2025-11-18 16:27 ` [PATCH v5 01/13] coresight: Change syncfreq to be a u8 James Clark
2025-11-18 16:27 ` [PATCH v5 02/13] coresight: Repack struct etmv4_drvdata James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-20 13:04 ` Mike Leach
2025-11-18 16:27 ` [PATCH v5 04/13] coresight: Hide unused ETMv3 format attributes James Clark
` (9 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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 560975b70474..5d21af346799 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -642,18 +642,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++)
@@ -663,15 +678,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])
@@ -680,13 +699,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.
@@ -694,26 +709,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 d178d79d9827..b319335e9bc3 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
@@ -824,7 +837,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;
@@ -837,11 +850,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-18 16:27 ` [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event() James Clark
@ 2025-11-20 13:04 ` Mike Leach
2025-11-20 13:52 ` James Clark
0 siblings, 1 reply; 36+ messages in thread
From: Mike Leach @ 2025-11-20 13:04 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
Hi James
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> wrote:
>
> 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 560975b70474..5d21af346799 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -642,18 +642,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++)
> @@ -663,15 +678,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])
> @@ -680,13 +699,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.
> @@ -694,26 +709,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
These field descriptions should match the terminology in the spec -
and this is not field in this register as defined in the spec - nor
are the following really. The event format is generic - so need a
event select macro, which is then applied to any register that needs
it
> + * 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);
>
So if we define generic event generators:-
#define ETM4_SEL_RS_PAIR BIT(7)
#defiine ETM4_RS_SEL_EVENT_SINGLE(rs_sel_idx) (GENMASK(4:0) & rs_sel_idx)
#define ETM4_RS_SEL_EVENT_PAIR(rs_sel_pair_idx) ((GENMASK(3:0) &
rs_sel_pair_idx) | ETM4_SEL_RS_PAIR)
these are then reuseable for all registers that need the 8 bit event
selectors, beyond just this register.
Thus we now accurately define the fields in the TRCCNTCTLRn
#define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
and use
FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK,
ETM4_RS_SEL_EVENT_SINGLE(ETM_RES_SEL_FALSE))
etc.
> - 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);
>
FIELD_PREP(TRCTSCTLR_EVENT_MASK, ETM4_RS_SEL_EVENT_SINGLE(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 d178d79d9827..b319335e9bc3 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)
per spec this should be
TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
> +#define TRCCNTCTLRn_CNTTYPE_MASK BIT(7)
> +#define TRCCNTCTLRn_CNTSEL_MASK GENMASK(4, 0)
per spec this should be
TRCCNTCTLRn_CNTEVENT_MASK GENMASK(7, 0)
> +
> +#define TRCTSCTLR_TYPE BIT(7)
> +#define TRCTSCTLR_SEL_MASK GENMASK(4, 0)
TRCTSCTLR_EVENT_MASK GENMASK(7: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
> @@ -824,7 +837,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;
> @@ -837,11 +850,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
>
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-20 13:04 ` Mike Leach
@ 2025-11-20 13:52 ` James Clark
2025-11-20 14:18 ` Leo Yan
2025-11-20 14:26 ` Mike Leach
0 siblings, 2 replies; 36+ messages in thread
From: James Clark @ 2025-11-20 13:52 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 20/11/2025 1:04 pm, Mike Leach wrote:
> Hi James
>
> On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> wrote:
>>
>> 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 560975b70474..5d21af346799 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -642,18 +642,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++)
>> @@ -663,15 +678,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])
>> @@ -680,13 +699,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.
>> @@ -694,26 +709,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
>
> These field descriptions should match the terminology in the spec -
> and this is not field in this register as defined in the spec - nor
> are the following really. The event format is generic - so need a
> event select macro, which is then applied to any register that needs
> it
>
>> + * 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)
>> + */
Hmmm not sure where they came from. I think I got stuck trying to decide
on which format to use because the ETM spec and Arm ARM are slightly
different. I tried to settle on the Arm ARM (because this code is also
for ETE, it's newer, and there was existing code that matched its style)
but didn't copy that properly either. It should be:
CNTCHAIN
RLDSELF
RLDEVENT_TYPE
RLDEVENT_SEL
CNTEVENT_TYPE
CNTEVENT_SEL
Some are correct but some need updating.
>> + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
>> + FIELD_PREP(TRCCNTCTLRn_RLDSEL_MASK, ETM_RES_SEL_FALSE) |
>> + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, ETM_RES_SEL_TRUE);
>>
>
> So if we define generic event generators:-
>
> #define ETM4_SEL_RS_PAIR BIT(7)
> #defiine ETM4_RS_SEL_EVENT_SINGLE(rs_sel_idx) (GENMASK(4:0) & rs_sel_idx)
> #define ETM4_RS_SEL_EVENT_PAIR(rs_sel_pair_idx) ((GENMASK(3:0) &
> rs_sel_pair_idx) | ETM4_SEL_RS_PAIR)
>
> these are then reuseable for all registers that need the 8 bit event
> selectors, beyond just this register.
>
> Thus we now accurately define the fields in the TRCCNTCTLRn
>
> #define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
>
> and use
>
> FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK,
> ETM4_RS_SEL_EVENT_SINGLE(ETM_RES_SEL_FALSE))
>
> etc.
>
>
I'm not sure I agree with that, the Arm ARM has CNTEVENT_TYPE as a
regular bit in the TRCCNTCTLRn register so it should be set like any
other. Hiding it as a subfield of "EVENT" when it always exists and
always does the same thing was maybe seen as a bad decision which is why
it was updated?
Also IMO, beyond using labels instead of raw numbers, the code should
just show what's programmed into the register.
ETM4_RS_SEL_EVENT_SINGLE() would be one more macro to jump through to
see what's actually happening.
>> - 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);
>>
>
> FIELD_PREP(TRCTSCTLR_EVENT_MASK, ETM4_RS_SEL_EVENT_SINGLE(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 d178d79d9827..b319335e9bc3 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)
> per spec this should be
> TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
>
>> +#define TRCCNTCTLRn_CNTTYPE_MASK BIT(7)
>> +#define TRCCNTCTLRn_CNTSEL_MASK GENMASK(4, 0)
>
> per spec this should be
> TRCCNTCTLRn_CNTEVENT_MASK GENMASK(7, 0)
>
>
>> +
>> +#define TRCTSCTLR_TYPE BIT(7)
>> +#define TRCTSCTLR_SEL_MASK GENMASK(4, 0)
>
> TRCTSCTLR_EVENT_MASK GENMASK(7: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
>> @@ -824,7 +837,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;
>> @@ -837,11 +850,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
>>
>
> Regards
>
> Mike
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-20 13:52 ` James Clark
@ 2025-11-20 14:18 ` Leo Yan
2025-11-20 14:25 ` Leo Yan
2025-11-20 14:26 ` Mike Leach
1 sibling, 1 reply; 36+ messages in thread
From: Leo Yan @ 2025-11-20 14:18 UTC (permalink / raw)
To: James Clark
Cc: Mike Leach, Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Thu, Nov 20, 2025 at 01:52:03PM +0000, James Clark wrote:
[...]
> > > + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
> > > + FIELD_PREP(TRCCNTCTLRn_RLDSEL_MASK, ETM_RES_SEL_FALSE) |
> > > + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, ETM_RES_SEL_TRUE);
> > >
> >
> > So if we define generic event generators:-
> >
> > #define ETM4_SEL_RS_PAIR BIT(7)
> > #defiine ETM4_RS_SEL_EVENT_SINGLE(rs_sel_idx) (GENMASK(4:0) & rs_sel_idx)
> > #define ETM4_RS_SEL_EVENT_PAIR(rs_sel_pair_idx) ((GENMASK(3:0) &
> > rs_sel_pair_idx) | ETM4_SEL_RS_PAIR)
> >
> > these are then reuseable for all registers that need the 8 bit event
> > selectors, beyond just this register.
> >
> > Thus we now accurately define the fields in the TRCCNTCTLRn
> >
> > #define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
> >
> > and use
> >
> > FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK,
> > ETM4_RS_SEL_EVENT_SINGLE(ETM_RES_SEL_FALSE))
> >
> > etc.
> >
> >
>
> I'm not sure I agree with that, the Arm ARM has CNTEVENT_TYPE as a regular
> bit in the TRCCNTCTLRn register so it should be set like any other. Hiding
> it as a subfield of "EVENT" when it always exists and always does the same
> thing was maybe seen as a bad decision which is why it was updated?
>
> Also IMO, beyond using labels instead of raw numbers, the code should just
> show what's programmed into the register. ETM4_RS_SEL_EVENT_SINGLE() would
> be one more macro to jump through to see what's actually happening.
Maybe define a general macro but with extra checking:
#define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
#define ETM4_RS_SEL_EVENT(paired, sel) ({ \
if (paired) \
assert(!(sel & ~GENMASK(3, 0))); \
else \
assert(!(sel & ~GENMASK(4, 0))); \
FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK, \
((paird << 7) | sel)); \
})
Thanks,
Leo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-20 14:18 ` Leo Yan
@ 2025-11-20 14:25 ` Leo Yan
2025-11-20 14:39 ` Mike Leach
0 siblings, 1 reply; 36+ messages in thread
From: Leo Yan @ 2025-11-20 14:25 UTC (permalink / raw)
To: James Clark, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Thu, Nov 20, 2025 at 02:18:21PM +0000, Coresight ML wrote:
[...]
> Maybe define a general macro but with extra checking:
>
> #define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
>
> #define ETM4_RS_SEL_EVENT(paired, sel) ({ \
> if (paired) \
> assert(!(sel & ~GENMASK(3, 0))); \
> else \
> assert(!(sel & ~GENMASK(4, 0))); \
> FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK, \
> ((paird << 7) | sel)); \
> })
It'd be better to use BUILD_BUG_ON() instead of assert().
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-20 14:25 ` Leo Yan
@ 2025-11-20 14:39 ` Mike Leach
0 siblings, 0 replies; 36+ messages in thread
From: Mike Leach @ 2025-11-20 14:39 UTC (permalink / raw)
To: Leo Yan
Cc: James Clark, Alexander Shishkin, Jonathan Corbet, Randy Dunlap,
coresight, linux-arm-kernel, linux-kernel, linux-doc
Hi Leo,
On Thu, 20 Nov 2025 at 14:25, Leo Yan <leo.yan@arm.com> wrote:
>
> On Thu, Nov 20, 2025 at 02:18:21PM +0000, Coresight ML wrote:
>
> [...]
>
> > Maybe define a general macro but with extra checking:
> >
> > #define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
> >
> > #define ETM4_RS_SEL_EVENT(paired, sel) ({ \
> > if (paired) \
> > assert(!(sel & ~GENMASK(3, 0))); \
> > else \
> > assert(!(sel & ~GENMASK(4, 0))); \
> > FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK, \
> > ((paird << 7) | sel)); \
> > })
>
> It'd be better to use BUILD_BUG_ON() instead of assert().
That is a decent option - except I would have ETM4_RS_SEL_EVENT(mask,
paired, sel) - so it can be used for every register that has the form
<REGNAME>_<EVENTFIEILD>_MASK
It is slightly vulnerable though if someone passes something that is
not 1/0 for paired - which is why I preferred the two separate macros.
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-20 13:52 ` James Clark
2025-11-20 14:18 ` Leo Yan
@ 2025-11-20 14:26 ` Mike Leach
2025-11-20 14:42 ` James Clark
1 sibling, 1 reply; 36+ messages in thread
From: Mike Leach @ 2025-11-20 14:26 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
HI James
On Thu, 20 Nov 2025 at 13:52, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 20/11/2025 1:04 pm, Mike Leach wrote:
> > Hi James
> >
> > On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> wrote:
> >>
> >> 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 560975b70474..5d21af346799 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >> @@ -642,18 +642,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++)
> >> @@ -663,15 +678,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])
> >> @@ -680,13 +699,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.
> >> @@ -694,26 +709,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
> >
> > These field descriptions should match the terminology in the spec -
> > and this is not field in this register as defined in the spec - nor
> > are the following really. The event format is generic - so need a
> > event select macro, which is then applied to any register that needs
> > it
> >
> >> + * 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)
> >> + */
>
> Hmmm not sure where they came from. I think I got stuck trying to decide
> on which format to use because the ETM spec and Arm ARM are slightly
> different. I tried to settle on the Arm ARM (because this code is also
> for ETE, it's newer, and there was existing code that matched its style)
> but didn't copy that properly either. It should be:
>
> CNTCHAIN
> RLDSELF
> RLDEVENT_TYPE
> RLDEVENT_SEL
> CNTEVENT_TYPE
> CNTEVENT_SEL
>
> Some are correct but some need updating.
>
> >> + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
> >> + FIELD_PREP(TRCCNTCTLRn_RLDSEL_MASK, ETM_RES_SEL_FALSE) |
> >> + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, ETM_RES_SEL_TRUE);
> >>
> >
> > So if we define generic event generators:-
> >
> > #define ETM4_SEL_RS_PAIR BIT(7)
> > #defiine ETM4_RS_SEL_EVENT_SINGLE(rs_sel_idx) (GENMASK(4:0) & rs_sel_idx)
> > #define ETM4_RS_SEL_EVENT_PAIR(rs_sel_pair_idx) ((GENMASK(3:0) &
> > rs_sel_pair_idx) | ETM4_SEL_RS_PAIR)
> >
> > these are then reuseable for all registers that need the 8 bit event
> > selectors, beyond just this register.
> >
> > Thus we now accurately define the fields in the TRCCNTCTLRn
> >
> > #define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
> >
> > and use
> >
> > FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK,
> > ETM4_RS_SEL_EVENT_SINGLE(ETM_RES_SEL_FALSE))
> >
> > etc.
> >
> >
>
> I'm not sure I agree with that, the Arm ARM has CNTEVENT_TYPE as a
> regular bit in the TRCCNTCTLRn register so it should be set like any
Given that this is the ETMv4 driver I was looking at the ETMv4
specification that defines an 8-bit event field. This might have
changed for ETE
> other. Hiding it as a subfield of "EVENT" when it always exists and
> always does the same thing was maybe seen as a bad decision which is why
> it was updated?
>
I think it is more a side effect of generating documentation from
computer readable register definitions - which in my view results in
far poorer documentation from a human readable/comprehension
perspective.
It is a common event format, and naming a bit that always appears and
does the same thing with multiple different names is not more
understandable.
> Also IMO, beyond using labels instead of raw numbers, the code should
> just show what's programmed into the register.
> ETM4_RS_SEL_EVENT_SINGLE() would be one more macro to jump through to
> see what's actually happening.
Except that if the macro is used consistently throughout the code, and
you understand the event field format - then you only need to look it
up once to understand what is happening everywhere it appears in the
code.
Moreover, "TRCCNTCTLRn_RLDSEL_MASK GENMASK(12, 8)" is only actually
valid if TRCCNTCTLRn_RLDTYPE is set to 0. If this bit is set to 1 then
the correct mask is (11, 8) - you can have 0-31 single resources
selected, but only 0-15 pairs.
Regards
Mike
>
> >> - 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);
> >>
> >
> > FIELD_PREP(TRCTSCTLR_EVENT_MASK, ETM4_RS_SEL_EVENT_SINGLE(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 d178d79d9827..b319335e9bc3 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)
> > per spec this should be
> > TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
> >
> >> +#define TRCCNTCTLRn_CNTTYPE_MASK BIT(7)
> >> +#define TRCCNTCTLRn_CNTSEL_MASK GENMASK(4, 0)
> >
> > per spec this should be
> > TRCCNTCTLRn_CNTEVENT_MASK GENMASK(7, 0)
> >
> >
> >> +
> >> +#define TRCTSCTLR_TYPE BIT(7)
> >> +#define TRCTSCTLR_SEL_MASK GENMASK(4, 0)
> >
> > TRCTSCTLR_EVENT_MASK GENMASK(7: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
> >> @@ -824,7 +837,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;
> >> @@ -837,11 +850,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
> >>
> >
> > Regards
> >
> > Mike
> >
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event()
2025-11-20 14:26 ` Mike Leach
@ 2025-11-20 14:42 ` James Clark
0 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-20 14:42 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 20/11/2025 2:26 pm, Mike Leach wrote:
> HI James
>
> On Thu, 20 Nov 2025 at 13:52, James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 20/11/2025 1:04 pm, Mike Leach wrote:
>>> Hi James
>>>
>>> On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> wrote:
>>>>
>>>> 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 560975b70474..5d21af346799 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> @@ -642,18 +642,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++)
>>>> @@ -663,15 +678,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])
>>>> @@ -680,13 +699,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.
>>>> @@ -694,26 +709,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
>>>
>>> These field descriptions should match the terminology in the spec -
>>> and this is not field in this register as defined in the spec - nor
>>> are the following really. The event format is generic - so need a
>>> event select macro, which is then applied to any register that needs
>>> it
>>>
>>>> + * 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)
>>>> + */
>>
>> Hmmm not sure where they came from. I think I got stuck trying to decide
>> on which format to use because the ETM spec and Arm ARM are slightly
>> different. I tried to settle on the Arm ARM (because this code is also
>> for ETE, it's newer, and there was existing code that matched its style)
>> but didn't copy that properly either. It should be:
>>
>> CNTCHAIN
>> RLDSELF
>> RLDEVENT_TYPE
>> RLDEVENT_SEL
>> CNTEVENT_TYPE
>> CNTEVENT_SEL
>>
>> Some are correct but some need updating.
>>
>>>> + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
>>>> + FIELD_PREP(TRCCNTCTLRn_RLDSEL_MASK, ETM_RES_SEL_FALSE) |
>>>> + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, ETM_RES_SEL_TRUE);
>>>>
>>>
>>> So if we define generic event generators:-
>>>
>>> #define ETM4_SEL_RS_PAIR BIT(7)
>>> #defiine ETM4_RS_SEL_EVENT_SINGLE(rs_sel_idx) (GENMASK(4:0) & rs_sel_idx)
>>> #define ETM4_RS_SEL_EVENT_PAIR(rs_sel_pair_idx) ((GENMASK(3:0) &
>>> rs_sel_pair_idx) | ETM4_SEL_RS_PAIR)
>>>
>>> these are then reuseable for all registers that need the 8 bit event
>>> selectors, beyond just this register.
>>>
>>> Thus we now accurately define the fields in the TRCCNTCTLRn
>>>
>>> #define TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
>>>
>>> and use
>>>
>>> FIELD_PREP(TRCCNTCTLRn_RLDEVENT_MASK,
>>> ETM4_RS_SEL_EVENT_SINGLE(ETM_RES_SEL_FALSE))
>>>
>>> etc.
>>>
>>>
>>
>> I'm not sure I agree with that, the Arm ARM has CNTEVENT_TYPE as a
>> regular bit in the TRCCNTCTLRn register so it should be set like any
>
> Given that this is the ETMv4 driver I was looking at the ETMv4
> specification that defines an 8-bit event field. This might have
> changed for ETE
>
>> other. Hiding it as a subfield of "EVENT" when it always exists and
>> always does the same thing was maybe seen as a bad decision which is why
>> it was updated?
>>
>
> I think it is more a side effect of generating documentation from
> computer readable register definitions - which in my view results in
> far poorer documentation from a human readable/comprehension
> perspective.
If that's wrong then it makes sense to get it fixed up in the Arm ARM
first. And then once that's fixed we can update the driver. We're moving
towards the sysreg-defs.h style, and even that can be auto generated
with json so will give us the registers defined in this format.
I don't think the Arm ARM can ever be "wrong" because there's some other
document that used to define something slightly differently.
> It is a common event format, and naming a bit that always appears and
> does the same thing with multiple different names is not more
> understandable.
>
>
>> Also IMO, beyond using labels instead of raw numbers, the code should
>> just show what's programmed into the register.
>> ETM4_RS_SEL_EVENT_SINGLE() would be one more macro to jump through to
>> see what's actually happening.
>
> Except that if the macro is used consistently throughout the code, and
> you understand the event field format - then you only need to look it
> up once to understand what is happening everywhere it appears in the
> code.
>
> Moreover, "TRCCNTCTLRn_RLDSEL_MASK GENMASK(12, 8)" is only actually
> valid if TRCCNTCTLRn_RLDTYPE is set to 0. If this bit is set to 1 then
> the correct mask is (11, 8) - you can have 0-31 single resources
> selected, but only 0-15 pairs.
>
I'm wondering if this review is taking into context what the commit
message says this change is doing. I'm trying to remove magic numbers
from existing code in a single function, rather than re-write anything
or make any new frameworks.
Of course we can always do more and start to validate and assert like
Leo suggested, but that can be on top of this and is more of a
functional change. I don't think it necessarily needs to block this
change as it is now. Only the slightly off field names in the block
comment make sense to update.
This has also been on the list for quite a while now and I'm starting to
wonder if style changes at this stage are the most efficient way to go
about tidying up.
> Regards
>
> Mike
>>
>>>> - 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);
>>>>
>>>
>>> FIELD_PREP(TRCTSCTLR_EVENT_MASK, ETM4_RS_SEL_EVENT_SINGLE(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 d178d79d9827..b319335e9bc3 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)
>>> per spec this should be
>>> TRCCNTCTLRn_RLDEVENT_MASK GENMASK(15, 8)
>>>
>>>> +#define TRCCNTCTLRn_CNTTYPE_MASK BIT(7)
>>>> +#define TRCCNTCTLRn_CNTSEL_MASK GENMASK(4, 0)
>>>
>>> per spec this should be
>>> TRCCNTCTLRn_CNTEVENT_MASK GENMASK(7, 0)
>>>
>>>
>>>> +
>>>> +#define TRCTSCTLR_TYPE BIT(7)
>>>> +#define TRCTSCTLR_SEL_MASK GENMASK(4, 0)
>>>
>>> TRCTSCTLR_EVENT_MASK GENMASK(7: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
>>>> @@ -824,7 +837,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;
>>>> @@ -837,11 +850,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
>>>>
>>>
>>> Regards
>>>
>>> Mike
>>>
>>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 04/13] coresight: Hide unused ETMv3 format attributes
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (2 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 03/13] coresight: Refactor etm4_config_timestamp_event() James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-20 11:21 ` Mike Leach
2025-11-18 16:27 ` [PATCH v5 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR() James Clark
` (8 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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 | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 17afa0f4cdee..faebd7822a77 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -106,8 +106,30 @@ 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 uses these attributes for programming itself (see
+ * ETM3X_SUPPORTED_OPTIONS). Sink ID is also supported for selecting a
+ * sink, but not used for configuring the ETM.
+ */
+ if (attr == &format_attr_cycacc.attr ||
+ attr == &format_attr_timestamp.attr ||
+ attr == &format_attr_retstack.attr ||
+ attr == &format_attr_sinkid.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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 04/13] coresight: Hide unused ETMv3 format attributes
2025-11-18 16:27 ` [PATCH v5 04/13] coresight: Hide unused ETMv3 format attributes James Clark
@ 2025-11-20 11:21 ` Mike Leach
0 siblings, 0 replies; 36+ messages in thread
From: Mike Leach @ 2025-11-20 11:21 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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 | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 17afa0f4cdee..faebd7822a77 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -106,8 +106,30 @@ 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 uses these attributes for programming itself (see
> + * ETM3X_SUPPORTED_OPTIONS). Sink ID is also supported for selecting a
> + * sink, but not used for configuring the ETM.
> + */
> + if (attr == &format_attr_cycacc.attr ||
> + attr == &format_attr_timestamp.attr ||
> + attr == &format_attr_retstack.attr ||
> + attr == &format_attr_sinkid.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
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR()
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (3 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 04/13] coresight: Hide unused ETMv3 format attributes James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-20 15:59 ` Mike Leach
2025-11-18 16:27 ` [PATCH v5 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD() James Clark
` (7 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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.
Reviewed-by: Leo Yan <leo.yan@arm.com>
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 faebd7822a77..28f1bfc4579f 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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR()
2025-11-18 16:27 ` [PATCH v5 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR() James Clark
@ 2025-11-20 15:59 ` Mike Leach
0 siblings, 0 replies; 36+ messages in thread
From: Mike Leach @ 2025-11-20 15:59 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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.
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 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 faebd7822a77..28f1bfc4579f 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
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD()
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (4 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 05/13] coresight: Define format attributes with GEN_PMU_FORMAT_ATTR() James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-20 16:08 ` Mike Leach
2025-11-18 16:27 ` [PATCH v5 07/13] coresight: Don't reject unrecognized ETMv3 format attributes James Clark
` (6 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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.
Reviewed-by: Leo Yan <leo.yan@arm.com>
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD()
2025-11-18 16:27 ` [PATCH v5 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD() James Clark
@ 2025-11-20 16:08 ` Mike Leach
0 siblings, 0 replies; 36+ messages in thread
From: Mike Leach @ 2025-11-20 16:08 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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.
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 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
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 07/13] coresight: Don't reject unrecognized ETMv3 format attributes
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (5 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 06/13] coresight: Interpret ETMv3 config with ATTR_CFG_GET_FLD() James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-20 14:48 ` Mike Leach
2025-11-18 16:27 ` [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD() James Clark
` (5 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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.
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 07/13] coresight: Don't reject unrecognized ETMv3 format attributes
2025-11-18 16:27 ` [PATCH v5 07/13] coresight: Don't reject unrecognized ETMv3 format attributes James Clark
@ 2025-11-20 14:48 ` Mike Leach
0 siblings, 0 replies; 36+ messages in thread
From: Mike Leach @ 2025-11-20 14:48 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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.
>
> 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
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (6 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 07/13] coresight: Don't reject unrecognized ETMv3 format attributes James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-19 9:32 ` Mike Leach
2025-11-18 16:27 ` [PATCH v5 09/13] coresight: Interpret ETMv4 " James Clark
` (4 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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 | 26 +++++++++++++++---------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 28f1bfc4579f..1b9ae832a576 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -80,12 +80,19 @@ static ssize_t format_attr_contextid_show(struct device *dev,
struct device_attribute *attr,
char *page)
{
- int pid_fmt = ETM_OPT_CTXTID;
+ /*
+ * is_kernel_in_hyp_mode() isn't defined in arm32 so avoid calling it
+ * even though is_visible() prevents this function from being called.
+ */
+#if IS_ENABLED(CONFIG_ARM64)
+ if (is_kernel_in_hyp_mode())
+ return contextid2_show(dev, attr, page);
-#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
- pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
+ return contextid1_show(dev, attr, page);
+#else
+ WARN_ONCE(1, "ETM contextid not supported on arm32\n");
+ return 0;
#endif
- return sprintf(page, "config:%d\n", pid_fmt);
}
static struct device_attribute format_attr_contextid =
@@ -337,7 +344,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;
@@ -350,13 +357,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-18 16:27 ` [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD() James Clark
@ 2025-11-19 9:32 ` Mike Leach
2025-11-19 11:26 ` James Clark
0 siblings, 1 reply; 36+ messages in thread
From: Mike Leach @ 2025-11-19 9:32 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
Hi James
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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 | 26 +++++++++++++++---------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 28f1bfc4579f..1b9ae832a576 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -80,12 +80,19 @@ static ssize_t format_attr_contextid_show(struct device *dev,
> struct device_attribute *attr,
> char *page)
> {
> - int pid_fmt = ETM_OPT_CTXTID;
> + /*
> + * is_kernel_in_hyp_mode() isn't defined in arm32 so avoid calling it
> + * even though is_visible() prevents this function from being called.
> + */
> +#if IS_ENABLED(CONFIG_ARM64)
> + if (is_kernel_in_hyp_mode())
> + return contextid2_show(dev, attr, page);
>
> -#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> - pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
> + return contextid1_show(dev, attr, page);
> +#else
> + WARN_ONCE(1, "ETM contextid not supported on arm32\n");
> + return 0;
Context ID is supported in aarch32 - and traced in etmv3 / ptm and etmv4.
Mike
> #endif
> - return sprintf(page, "config:%d\n", pid_fmt);
> }
>
> static struct device_attribute format_attr_contextid =
> @@ -337,7 +344,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;
> @@ -350,13 +357,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
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 9:32 ` Mike Leach
@ 2025-11-19 11:26 ` James Clark
2025-11-19 11:45 ` Mike Leach
0 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-19 11:26 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 19/11/2025 9:32 am, Mike Leach wrote:
> Hi James
>
> On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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 | 26 +++++++++++++++---------
>> 1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 28f1bfc4579f..1b9ae832a576 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -80,12 +80,19 @@ static ssize_t format_attr_contextid_show(struct device *dev,
>> struct device_attribute *attr,
>> char *page)
>> {
>> - int pid_fmt = ETM_OPT_CTXTID;
>> + /*
>> + * is_kernel_in_hyp_mode() isn't defined in arm32 so avoid calling it
>> + * even though is_visible() prevents this function from being called.
>> + */
>> +#if IS_ENABLED(CONFIG_ARM64)
>> + if (is_kernel_in_hyp_mode())
>> + return contextid2_show(dev, attr, page);
>>
>> -#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
>> - pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
>> + return contextid1_show(dev, attr, page);
>> +#else
>> + WARN_ONCE(1, "ETM contextid not supported on arm32\n");
>> + return 0;
>
> Context ID is supported in aarch32 - and traced in etmv3 / ptm and etmv4.
>
> Mike
>
Not in Perf mode unless I'm missing something:
#define ETM3X_SUPPORTED_OPTIONS (ETMCR_CYC_ACC | \
ETMCR_TIMESTAMP_EN | \
ETMCR_RETURN_STACK)
Only these options are currently accepted. I suppose the comment is
describing what the current behavior is, whether that is what we want is
another thing.
But if we do start supporting context ID in ETMv3 we can update that
comment.
>> #endif
>> - return sprintf(page, "config:%d\n", pid_fmt);
>> }
>>
>> static struct device_attribute format_attr_contextid =
>> @@ -337,7 +344,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;
>> @@ -350,13 +357,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
>>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 11:26 ` James Clark
@ 2025-11-19 11:45 ` Mike Leach
2025-11-19 12:00 ` James Clark
0 siblings, 1 reply; 36+ messages in thread
From: Mike Leach @ 2025-11-19 11:45 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
Hi James
On Wed, 19 Nov 2025 at 11:26, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 19/11/2025 9:32 am, Mike Leach wrote:
> > Hi James
> >
> > On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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 | 26 +++++++++++++++---------
> >> 1 file changed, 16 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> index 28f1bfc4579f..1b9ae832a576 100644
> >> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> >> @@ -80,12 +80,19 @@ static ssize_t format_attr_contextid_show(struct device *dev,
> >> struct device_attribute *attr,
> >> char *page)
> >> {
> >> - int pid_fmt = ETM_OPT_CTXTID;
> >> + /*
> >> + * is_kernel_in_hyp_mode() isn't defined in arm32 so avoid calling it
> >> + * even though is_visible() prevents this function from being called.
> >> + */
> >> +#if IS_ENABLED(CONFIG_ARM64)
> >> + if (is_kernel_in_hyp_mode())
> >> + return contextid2_show(dev, attr, page);
> >>
> >> -#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> >> - pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
> >> + return contextid1_show(dev, attr, page);
> >> +#else
> >> + WARN_ONCE(1, "ETM contextid not supported on arm32\n");
> >> + return 0;
> >
> > Context ID is supported in aarch32 - and traced in etmv3 / ptm and etmv4.
> >
> > Mike
> >
>
> Not in Perf mode unless I'm missing something:
>
> #define ETM3X_SUPPORTED_OPTIONS (ETMCR_CYC_ACC | \
> ETMCR_TIMESTAMP_EN | \
> ETMCR_RETURN_STACK)
>
> Only these options are currently accepted. I suppose the comment is
> describing what the current behavior is, whether that is what we want is
> another thing.
>
> But if we do start supporting context ID in ETMv3 we can update that
> comment.
>
Unlikely - but the comment seems to conflate core architecture and etm
architecture.
A core with aarch32 can be traced in etm4 - i.e etm4 supports A64, A32
and T32 instruction sets.
If this set gets another version it might be worth saying "not
ETMv3/PTM" rather than not A32.
Mike
> >> #endif
> >> - return sprintf(page, "config:%d\n", pid_fmt);
> >> }
> >>
> >> static struct device_attribute format_attr_contextid =
> >> @@ -337,7 +344,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;
> >> @@ -350,13 +357,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
> >>
> >
> >
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 11:45 ` Mike Leach
@ 2025-11-19 12:00 ` James Clark
2025-11-19 12:36 ` Leo Yan
0 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-19 12:00 UTC (permalink / raw)
To: Mike Leach, Leo Yan
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 19/11/2025 11:45 am, Mike Leach wrote:
> Hi James
>
> On Wed, 19 Nov 2025 at 11:26, James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 19/11/2025 9:32 am, Mike Leach wrote:
>>> Hi James
>>>
>>> On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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 | 26 +++++++++++++++---------
>>>> 1 file changed, 16 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> index 28f1bfc4579f..1b9ae832a576 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> @@ -80,12 +80,19 @@ static ssize_t format_attr_contextid_show(struct device *dev,
>>>> struct device_attribute *attr,
>>>> char *page)
>>>> {
>>>> - int pid_fmt = ETM_OPT_CTXTID;
>>>> + /*
>>>> + * is_kernel_in_hyp_mode() isn't defined in arm32 so avoid calling it
>>>> + * even though is_visible() prevents this function from being called.
>>>> + */
>>>> +#if IS_ENABLED(CONFIG_ARM64)
>>>> + if (is_kernel_in_hyp_mode())
>>>> + return contextid2_show(dev, attr, page);
>>>>
>>>> -#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
>>>> - pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
>>>> + return contextid1_show(dev, attr, page);
>>>> +#else
>>>> + WARN_ONCE(1, "ETM contextid not supported on arm32\n");
>>>> + return 0;
>>>
>>> Context ID is supported in aarch32 - and traced in etmv3 / ptm and etmv4.
>>>
>>> Mike
>>>
>>
>> Not in Perf mode unless I'm missing something:
>>
>> #define ETM3X_SUPPORTED_OPTIONS (ETMCR_CYC_ACC | \
>> ETMCR_TIMESTAMP_EN | \
>> ETMCR_RETURN_STACK)
>>
>> Only these options are currently accepted. I suppose the comment is
>> describing what the current behavior is, whether that is what we want is
>> another thing.
>>
>> But if we do start supporting context ID in ETMv3 we can update that
>> comment.
>>
>
> Unlikely - but the comment seems to conflate core architecture and etm
> architecture.
> A core with aarch32 can be traced in etm4 - i.e etm4 supports A64, A32
> and T32 instruction sets.
> If this set gets another version it might be worth saying "not
> ETMv3/PTM" rather than not A32.
>
> Mike
>
>
Oh I see what you mean, yes that would be slightly better. But then to
make the code match the warning it might also make sense to change
CONFIG_ARM64 back to CONFIG_CORESIGHT_SOURCE_ETM4X, which Leo suggested
to change. Maybe I can just delete the warning text, practically this
warning can never be hit.
It would have been nicer to avoid any conditional compilation or
warnings and comments, because we're hiding contextid on ETMv3 anyway.
It's unfortunate that the compiler doesn't allow us to ignore
is_kernel_in_hyp_mode() being undefined even with short circuit evaluation.
>>>> #endif
>>>> - return sprintf(page, "config:%d\n", pid_fmt);
>>>> }
>>>>
>>>> static struct device_attribute format_attr_contextid =
>>>> @@ -337,7 +344,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;
>>>> @@ -350,13 +357,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
>>>>
>>>
>>>
>>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 12:00 ` James Clark
@ 2025-11-19 12:36 ` Leo Yan
2025-11-19 13:55 ` James Clark
0 siblings, 1 reply; 36+ messages in thread
From: Leo Yan @ 2025-11-19 12:36 UTC (permalink / raw)
To: James Clark
Cc: Mike Leach, Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Wed, Nov 19, 2025 at 12:00:30PM +0000, James Clark wrote:
[...]
> ... But then to make
> the code match the warning it might also make sense to change CONFIG_ARM64
> back to CONFIG_CORESIGHT_SOURCE_ETM4X, which Leo suggested to change. Maybe
> I can just delete the warning text, practically this warning can never be
> hit.
Armv8 CPUs can runs in aarch32 mode, strictly speaking, we should also
can run ETMv4 driver in aarch32 mode as well. Then CONFIG_ARM64 is the
right choice, this can remind us that `is_kernel_in_hyp_mode()` is
always stick to aarch64 mode.
static ssize_t format_attr_contextid_show(struct device *dev,
struct device_attribute *attr,
char *page)
{
#if IS_ENABLED(CONFIG_ARM64)
if (is_kernel_in_hyp_mode())
return contextid2_show(dev, attr, page);
#endif
return contextid1_show(dev, attr, page);
}
Thanks,
Leo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 12:36 ` Leo Yan
@ 2025-11-19 13:55 ` James Clark
2025-11-19 14:37 ` Leo Yan
0 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-19 13:55 UTC (permalink / raw)
To: Leo Yan
Cc: Mike Leach, Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 19/11/2025 12:36 pm, Leo Yan wrote:
> On Wed, Nov 19, 2025 at 12:00:30PM +0000, James Clark wrote:
>
> [...]
>
>> ... But then to make
>> the code match the warning it might also make sense to change CONFIG_ARM64
>> back to CONFIG_CORESIGHT_SOURCE_ETM4X, which Leo suggested to change. Maybe
>> I can just delete the warning text, practically this warning can never be
>> hit.
>
> Armv8 CPUs can runs in aarch32 mode, strictly speaking, we should also
> can run ETMv4 driver in aarch32 mode as well. Then CONFIG_ARM64 is the
> right choice, this can remind us that `is_kernel_in_hyp_mode()` is
> always stick to aarch64 mode.
>
For the avoidance of confusion, in this case CONFIG_ARM64 and
CONFIG_CORESIGHT_SOURCE_ETM4X are 100% equivalent and there's no
functional difference. Yes maybe 32 bit userspace can be traced from
ETMv4, but that's not really related with how this code is compiled.
> static ssize_t format_attr_contextid_show(struct device *dev,
> struct device_attribute *attr,
> char *page)
> {
> #if IS_ENABLED(CONFIG_ARM64)
> if (is_kernel_in_hyp_mode())
> return contextid2_show(dev, attr, page);
> #endif
>
> return contextid1_show(dev, attr, page);
Not having an #else implies that the contextid1_show() part is valid
when !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON
because it's dead code.
Personally I would drop the is_visible(). It makes sense for dynamically
hidden things, but these are all compile time. IMO it's cleaner to just
not include them to begin with, rather than include and then hide them.
Then the extra condition in format_attr_contextid_show() isn't needed
because the function doesn't exist:
GEN_PMU_FORMAT_ATTR(cycacc);
GEN_PMU_FORMAT_ATTR(timestamp);
GEN_PMU_FORMAT_ATTR(retstack);
GEN_PMU_FORMAT_ATTR(sinkid);
#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
/* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
GEN_PMU_FORMAT_ATTR(contextid1);
/* contextid2 enables tracing CONTEXTIDR_EL2 for ETMv4 */
GEN_PMU_FORMAT_ATTR(contextid2);
GEN_PMU_FORMAT_ATTR(branch_broadcast);
/* preset - if sink ID is used as a configuration selector */
GEN_PMU_FORMAT_ATTR(preset);
/* config ID - set if a system configuration is selected */
GEN_PMU_FORMAT_ATTR(configid);
GEN_PMU_FORMAT_ATTR(cc_threshold);
/*
* contextid always traces the "PID". The PID is in CONTEXTIDR_EL1
* when the kernel is running at EL1; when the kernel is at EL2,
* the PID is in CONTEXTIDR_EL2.
*/
static ssize_t format_attr_contextid_show(struct device *dev,
struct device_attribute *attr,
char *page)
{
if (is_kernel_in_hyp_mode())
return contextid2_show(dev, attr, page);
return contextid1_show(dev, attr, page);
}
static struct device_attribute format_attr_contextid =
__ATTR(contextid, 0444, format_attr_contextid_show, NULL);
#endif
/*
* ETMv3 only uses the first 3 attributes for programming itself (see
* ETM3X_SUPPORTED_OPTIONS). Sink ID is also supported for selecting a
* sink in both, but not used for configuring the ETM. The remaining
* attributes are ETMv4 specific.
*/
static struct attribute *etm_config_formats_attr[] = {
&format_attr_cycacc.attr,
&format_attr_timestamp.attr,
&format_attr_retstack.attr,
&format_attr_sinkid.attr,
#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
&format_attr_contextid.attr,
&format_attr_contextid1.attr,
&format_attr_contextid2.attr,
&format_attr_preset.attr,
&format_attr_configid.attr,
&format_attr_branch_broadcast.attr,
&format_attr_cc_threshold.attr,
#endif
NULL,
};
> }
>
> Thanks,
> Leo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 13:55 ` James Clark
@ 2025-11-19 14:37 ` Leo Yan
2025-11-19 15:15 ` James Clark
0 siblings, 1 reply; 36+ messages in thread
From: Leo Yan @ 2025-11-19 14:37 UTC (permalink / raw)
To: James Clark
Cc: Mike Leach, Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Wed, Nov 19, 2025 at 01:55:15PM +0000, James Clark wrote:
[...]
> > static ssize_t format_attr_contextid_show(struct device *dev,
> > struct device_attribute *attr,
> > char *page)
> > {
> > #if IS_ENABLED(CONFIG_ARM64)
> > if (is_kernel_in_hyp_mode())
> > return contextid2_show(dev, attr, page);
> > #endif
> >
> > return contextid1_show(dev, attr, page);
>
> Not having an #else implies that the contextid1_show() part is valid when
> !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON because
> it's dead code.
Based on ETMv3/v4 spec, would contextid1 always be valid ? (Though we do
not support context ID for ETMv3 yet).
> Personally I would drop the is_visible(). It makes sense for dynamically
> hidden things, but these are all compile time. IMO it's cleaner to just not
> include them to begin with, rather than include and then hide them. Then the
> extra condition in format_attr_contextid_show() isn't needed because the
> function doesn't exist:
This is fine for me, though in general I think the dynamic approach is
readable and extendable than the compile-time approach.
Thanks,
Leo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()
2025-11-19 14:37 ` Leo Yan
@ 2025-11-19 15:15 ` James Clark
0 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-19 15:15 UTC (permalink / raw)
To: Leo Yan, Mike Leach
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 19/11/2025 2:37 pm, Leo Yan wrote:
> On Wed, Nov 19, 2025 at 01:55:15PM +0000, James Clark wrote:
>
> [...]
>
>>> static ssize_t format_attr_contextid_show(struct device *dev,
>>> struct device_attribute *attr,
>>> char *page)
>>> {
>>> #if IS_ENABLED(CONFIG_ARM64)
>>> if (is_kernel_in_hyp_mode())
>>> return contextid2_show(dev, attr, page);
>>> #endif
>>>
>>> return contextid1_show(dev, attr, page);
>>
>> Not having an #else implies that the contextid1_show() part is valid when
>> !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON because
>> it's dead code.
>
> Based on ETMv3/v4 spec, would contextid1 always be valid ? (Though we do
> not support context ID for ETMv3 yet).
>
It's not currently supported for ETMv3 in perf mode, which is the
relevant thing here. So format_attr_contextid_show() never gets called
for ETMv3 (equivalent to !CONFIG_ARM64).
Based on the spec it may be supported, but that's a different discussion
and I doubt anyone wants it so it's unlikely to be added.
>> Personally I would drop the is_visible(). It makes sense for dynamically
>> hidden things, but these are all compile time. IMO it's cleaner to just not
>> include them to begin with, rather than include and then hide them. Then the
>> extra condition in format_attr_contextid_show() isn't needed because the
>> function doesn't exist:
>
> This is fine for me, though in general I think the dynamic approach is
> readable and extendable than the compile-time approach.
>
> Thanks,
> Leo
I agree in a perfect world, but it seems to have caused confusion and
wasn't that clean because is_kernel_in_hyp_mode() only exists for arm64.
I'll send a new version without it.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 09/13] coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD()
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (7 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD() James Clark
@ 2025-11-18 16:27 ` James Clark
2025-11-20 16:10 ` Mike Leach
2025-11-18 16:28 ` [PATCH v5 10/13] coresight: Remove misleading definitions James Clark
` (3 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:27 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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.
Reviewed-by: Leo Yan <leo.yan@arm.com>
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 5d21af346799..c7208ffc9432 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>
@@ -780,17 +781,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
@@ -810,17 +811,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;
@@ -831,26 +832,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;
+ cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
+ if (cfg_hash) {
+ 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
@@ -859,7 +856,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;
}
}
@@ -1083,11 +1080,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 09/13] coresight: Interpret ETMv4 config with ATTR_CFG_GET_FLD()
2025-11-18 16:27 ` [PATCH v5 09/13] coresight: Interpret ETMv4 " James Clark
@ 2025-11-20 16:10 ` Mike Leach
0 siblings, 0 replies; 36+ messages in thread
From: Mike Leach @ 2025-11-20 16:10 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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.
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 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 5d21af346799..c7208ffc9432 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>
> @@ -780,17 +781,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
> @@ -810,17 +811,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;
> @@ -831,26 +832,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;
> + cfg_hash = ATTR_CFG_GET_FLD(attr, configid);
> + if (cfg_hash) {
> + 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
> @@ -859,7 +856,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;
> }
> }
>
> @@ -1083,11 +1080,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
>
Reviewed-by: Mike Leach <mike.leach@linaro.org>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 10/13] coresight: Remove misleading definitions
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (8 preceding siblings ...)
2025-11-18 16:27 ` [PATCH v5 09/13] coresight: Interpret ETMv4 " James Clark
@ 2025-11-18 16:28 ` James Clark
2025-11-20 11:21 ` Mike Leach
2025-11-18 16:28 ` [PATCH v5 11/13] coresight: Extend width of timestamp format attribute James Clark
` (2 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: James Clark @ 2025-11-18 16:28 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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.
Reviewed-by: Leo Yan <leo.yan@arm.com>
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v5 10/13] coresight: Remove misleading definitions
2025-11-18 16:28 ` [PATCH v5 10/13] coresight: Remove misleading definitions James Clark
@ 2025-11-20 11:21 ` Mike Leach
2025-11-20 11:53 ` James Clark
0 siblings, 1 reply; 36+ messages in thread
From: Mike Leach @ 2025-11-20 11:21 UTC (permalink / raw)
To: James Clark
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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.
>
The perf version is used to reconstruct the control registers for etm3
/ etm4 to put into the trace metadata headers in the perf.data file
for the decoder to be intitialised correctly.
perf_event_attr::config uses the ETM_OPT_* values, used directly for
etm3 since they match the bit pattern in the etm3/ptm config register,
and remapped for etm4 from ETM_OPT_* to equivalent ETM4_CFG_BIT* for
the etm4 config register.
The reason we do this re-construction - rather than read registers as
we do for the other metadata - is that the register value is not set
at the point we are recording the metadata - it does not actually get
set until the etm is enabled, later in the perf process.
On this basis it would seem that any changing of the attribute bit
ordering has potential to break perf decode. Probably safe to remove
from this version of the file though, but overall, changing bit
meanings in the underlying variable may possibly need a fix in perf
and potentially a breaking change for earlier versions of the tools.
That said, for this particular version of the header, since it appears
to no longer need the values due to earlier changes.
Reviewed-by: Mike Leach <mike.leach@linaro.org>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 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
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v5 10/13] coresight: Remove misleading definitions
2025-11-20 11:21 ` Mike Leach
@ 2025-11-20 11:53 ` James Clark
0 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-20 11:53 UTC (permalink / raw)
To: Mike Leach
Cc: Suzuki K Poulose, Alexander Shishkin, Jonathan Corbet, Leo Yan,
Randy Dunlap, coresight, linux-arm-kernel, linux-kernel,
linux-doc
On 20/11/2025 11:21 am, Mike Leach wrote:
> On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> 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.
>>
>
> The perf version is used to reconstruct the control registers for etm3
> / etm4 to put into the trace metadata headers in the perf.data file
> for the decoder to be intitialised correctly.
>
> perf_event_attr::config uses the ETM_OPT_* values, used directly for
> etm3 since they match the bit pattern in the etm3/ptm config register,
> and remapped for etm4 from ETM_OPT_* to equivalent ETM4_CFG_BIT* for
> the etm4 config register.
That was one of the parts that confused me. Just because the format bit
positions happened to match the register config for ETM3, didn't mean
that it had to avoid reading the positions from sysfs. And it wasn't
consistent between ETM3 and ETM4 either.
That's decoupled now anyway, so although they still match there's no
reason it can't change in the future.
>
> The reason we do this re-construction - rather than read registers as
> we do for the other metadata - is that the register value is not set
> at the point we are recording the metadata - it does not actually get
> set until the etm is enabled, later in the perf process.
>
> On this basis it would seem that any changing of the attribute bit
> ordering has potential to break perf decode. Probably safe to remove
Yes so we should keep the positions the same until enough time has
passed after the Perf fixes go in. Although other than the timestamp one
I can't see any of the format bits needing to be moved.
The timestamp bit now won't be passed correctly to OpenCSD until Perf is
fixed, but I saw it wasn't used for decode anyway, so that should be fine?
> from this version of the file though, but overall, changing bit
> meanings in the underlying variable may possibly need a fix in perf
> and potentially a breaking change for earlier versions of the tools.
>
> That said, for this particular version of the header, since it appears
> to no longer need the values due to earlier changes.
>
The reconstruction of the registers will remain in Perf, so nothing will
change there apart from not hard coding the format bit positions.
I plan to define the register bits in Perf in cs-etm.c in the same style
as we do in the kernel. ETM_OPT_* and ETM4_CFG_* weren't searchable
because the the bits are already defined in the kernel in a different
style. So I'll define them to match like this:
+/* ETMv4 CONFIGR register bits */
+#define TRCCONFIGR_BB BIT(3)
+#define TRCCONFIGR_CCI BIT(4)
+#define TRCCONFIGR_CID BIT(6)
+#define TRCCONFIGR_VMID BIT(7)
+#define TRCCONFIGR_TS BIT(11)
+#define TRCCONFIGR_RS BIT(12)
+#define TRCCONFIGR_VMIDOPT BIT(15)
+
+/* ETMv3 ETMCR register bits */
+#define ETMCR_CYC_ACC BIT(12)
+#define ETMCR_TIMESTAMP_EN BIT(28)
+#define ETMCR_RETURN_STACK BIT(29)
+
Then use the format strings to check which ones are set:
if (!evsel__get_config_val(cs_etm_pmu, evsel, "cycacc", &val) && val)
trcconfigr |= TRCCONFIGR_CCI;
if (!evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &val) && val)
trcconfigr |= TRCCONFIGR_CID;
if (!evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val) && val)
trcconfigr |= TRCCONFIGR_TS;
if (!evsel__get_config_val(cs_etm_pmu, evsel, "retstack", &val) && val)
trcconfigr |= TRCCONFIGR_RS;
if (!evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &val) && val)
trcconfigr |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT;
if (!evsel__get_config_val(cs_etm_pmu, evsel, "branch_broadcast", &val)
&& val)
trcconfigr |= TRCCONFIGR_BB;
This also highlighted another Perf bug that evsel__set_config_if_unset()
only operates on config and not config1, config2 etc. That's also used
for setting up the event so I'll fix that too.
Seems like the API goes through a lot of effort to publish the bit
positions in sysfs, but we've selectively not used them which wasn't
quite right.
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
>
>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>> 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
>>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v5 11/13] coresight: Extend width of timestamp format attribute
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (9 preceding siblings ...)
2025-11-18 16:28 ` [PATCH v5 10/13] coresight: Remove misleading definitions James Clark
@ 2025-11-18 16:28 ` James Clark
2025-11-18 16:28 ` [PATCH v5 12/13] coresight: Allow setting the timestamp interval James Clark
2025-11-18 16:28 ` [PATCH v5 13/13] coresight: docs: Document etm4x timestamp interval option James Clark
12 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-18 16:28 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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.
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 c7208ffc9432..cfd6d2b7bc50 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>
@@ -791,7 +792,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v5 12/13] coresight: Allow setting the timestamp interval
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (10 preceding siblings ...)
2025-11-18 16:28 ` [PATCH v5 11/13] coresight: Extend width of timestamp format attribute James Clark
@ 2025-11-18 16:28 ` James Clark
2025-11-18 16:28 ` [PATCH v5 13/13] coresight: docs: Document etm4x timestamp interval option James Clark
12 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-18 16:28 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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 cfd6d2b7bc50..a91981a651e7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -651,7 +651,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--------------+
@@ -662,11 +662,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)
@@ -704,12 +718,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
@@ -799,7 +809,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v5 13/13] coresight: docs: Document etm4x timestamp interval option
2025-11-18 16:27 [PATCH v5 00/13] coresight: Update timestamp attribute to be an interval instead of bool James Clark
` (11 preceding siblings ...)
2025-11-18 16:28 ` [PATCH v5 12/13] coresight: Allow setting the timestamp interval James Clark
@ 2025-11-18 16:28 ` James Clark
12 siblings, 0 replies; 36+ messages in thread
From: James Clark @ 2025-11-18 16:28 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, Alexander Shishkin, Jonathan Corbet,
Leo Yan, Randy Dunlap
Cc: coresight, linux-arm-kernel, linux-kernel, linux-doc, 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 | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/Documentation/trace/coresight/coresight.rst b/Documentation/trace/coresight/coresight.rst
index 806699871b80..d461de4e067e 100644
--- a/Documentation/trace/coresight/coresight.rst
+++ b/Documentation/trace/coresight/coresight.rst
@@ -613,8 +613,20 @@ 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 = minimum interval .. 15 = maximum 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 ^ (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 maximum interval (15) will disable the counter generated
+ timestamps, freeing the counter resource, leaving only ones emitted when
+ a SYNC packet is generated. The sync interval is controlled with
+ TRCSYNCPR.PERIOD which is every 4096 bytes of trace by default.
+
* - 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
^ permalink raw reply related [flat|nested] 36+ messages in thread