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 | 95 ++++++++++++++--------
drivers/hwtracing/coresight/coresight-etm4x.h | 20 +++--
2 files changed, 77 insertions(+), 38 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 020f070bf17d..920d092ef862 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -609,18 +609,33 @@ static void etm4_enable_hw_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++)
@@ -630,15 +645,17 @@ 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'.
+ *
+ * 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])
@@ -647,13 +664,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.
@@ -661,26 +674,42 @@ 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 = 0, reload on resource 0 (fixed always false resource, never
+ * reload)
+ * CNTTYPE = 0, one count input resource
+ * CNTSEL = 1, count on resource 1 (fixed always true resource, always
+ * decrement)
+ */
+ config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
+ FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, 1);
- 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 813e7208ad17..a06338851ef5 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -225,6 +225,16 @@
#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)
+
/*
* System instructions to access ETM registers.
* See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
@@ -824,7 +834,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 +847,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.1Hi James,
On Thu, 2 Oct 2025 at 11:09, 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 | 95 ++++++++++++++--------
> drivers/hwtracing/coresight/coresight-etm4x.h | 20 +++--
> 2 files changed, 77 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 020f070bf17d..920d092ef862 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -609,18 +609,33 @@ static void etm4_enable_hw_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;
As you mention elsewhere - TS will always be output for at least every
4096 bytes - so if we have no counter perhaps we can live with that.
Previously we where trying to get as fast as possible, now we want to
slow them down - so every 4096 looks a decent compromise if the
hardware has no counters.
TRCTSCTLR will be 0 - which selects the false event - so only the
non-event timestamps occur. Returning 0 here will enable the non event
timestamps
What this means is if the user selects timestamps, there will always
be at least some timestamps output.
Alternatively, TRCTSCTLR = 0x1 selects the TRUE event - which might
result once again in the etm attempting to insert timestamps at every
available opportunity. - though never tried this!
>
> /* Find a counter that hasn't been initialised */
> for (ctridx = 0; ctridx < drvdata->nr_cntr; ctridx++)
> @@ -630,15 +645,17 @@ 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.
Well - from ETMv4.3 that statement is not true - there can be no RS
pairs - but no RS pairs forces no counters so should not actually get
here.
Needs comment to reflect this.
> + * 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'.
> + *
> + * 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])
> @@ -647,13 +664,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.
> @@ -661,26 +674,42 @@ 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 = 0, reload on resource 0 (fixed always false resource, never
> + * reload)
> + * CNTTYPE = 0, one count input resource
> + * CNTSEL = 1, count on resource 1 (fixed always true resource, always
> + * decrement)
> + */
> + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
> + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, 1);
>
if we are eliminating magic numbers - the '1' here should be something
like RESOURCE_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 813e7208ad17..a06338851ef5 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -225,6 +225,16 @@
> #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)
> +
> /*
> * System instructions to access ETM registers.
> * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
> @@ -824,7 +834,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 +847,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
On 09/10/2025 5:02 pm, Mike Leach wrote:
> Hi James,
>
> On Thu, 2 Oct 2025 at 11:09, 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 | 95 ++++++++++++++--------
>> drivers/hwtracing/coresight/coresight-etm4x.h | 20 +++--
>> 2 files changed, 77 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 020f070bf17d..920d092ef862 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -609,18 +609,33 @@ static void etm4_enable_hw_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;
>
> As you mention elsewhere - TS will always be output for at least every
> 4096 bytes - so if we have no counter perhaps we can live with that.
> Previously we where trying to get as fast as possible, now we want to
> slow them down - so every 4096 looks a decent compromise if the
> hardware has no counters.
> TRCTSCTLR will be 0 - which selects the false event - so only the
> non-event timestamps occur. Returning 0 here will enable the non event
> timestamps
>
> What this means is if the user selects timestamps, there will always
> be at least some timestamps output.
>
> Alternatively, TRCTSCTLR = 0x1 selects the TRUE event - which might
> result once again in the etm attempting to insert timestamps at every
> available opportunity. - though never tried this!
>
I tested it and it doesn't make a difference to the frequency (on N1SDP)
whether the TRUE resource is used directly or routed through the counter
with reload value = 1. When I first looked at the function I was under
the impression that TRCTSCTLR.SEL couldn't use the TRUE resource
directly, and it had to go through a counter. But looks like that's not
the case so I'm not sure why it wasn't done that way in the first place.
We could change it so that if ts_level == 1 then don't consume a counter
and use the TRUE resource. But I'm not sure if it's worth it because
nobody ever complained, so it could be adding complexity where it's not
needed.
I also wouldn't do it if ts_level > 1 and no counters are available. I
think continuing to return an error is better in that case because we
can't give the user what they asked for.
>>
>> /* Find a counter that hasn't been initialised */
>> for (ctridx = 0; ctridx < drvdata->nr_cntr; ctridx++)
>> @@ -630,15 +645,17 @@ 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.
>
> Well - from ETMv4.3 that statement is not true - there can be no RS
> pairs - but no RS pairs forces no counters so should not actually get
> here.
> Needs comment to reflect this.
>
>
Will insert "...every implementation _with counters_ has at least..."
>> + * 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'.
>> + *
>> + * 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])
>> @@ -647,13 +664,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.
>> @@ -661,26 +674,42 @@ 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 = 0, reload on resource 0 (fixed always false resource, never
>> + * reload)
>> + * CNTTYPE = 0, one count input resource
>> + * CNTSEL = 1, count on resource 1 (fixed always true resource, always
>> + * decrement)
>> + */
>> + config->cntr_ctrl[ctridx] = TRCCNTCTLRn_RLDSELF |
>> + FIELD_PREP(TRCCNTCTLRn_CNTSEL_MASK, 1);
>>
>
> if we are eliminating magic numbers - the '1' here should be something
> like RESOURCE_SEL_TRUE?
>
Given that resource 0 and 1 are fixed function, yes I think a #define
for them could be clearer.
Thanks
James
>> - 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 813e7208ad17..a06338851ef5 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -225,6 +225,16 @@
>> #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)
>> +
>> /*
>> * System instructions to access ETM registers.
>> * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
>> @@ -824,7 +834,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 +847,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
© 2016 - 2026 Red Hat, Inc.