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!

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

Hi James

On Thu, 2 Oct 2025 at 11:10, James Clark james.clark@linaro.org wrote:

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

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

Only show this attribute for ETM4x because timestamps aren't configured in the same way for ETM3x. The attribute is only ever read in coresight-etm4x-core.c.

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.c | 16 +++++++++++++++- drivers/hwtracing/coresight/coresight-etm-perf.h | 7 +++++++ drivers/hwtracing/coresight/coresight-etm4x-core.c | 21 ++++++++++++--------- 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index f677c08233ba..0c1b990fc56e 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> @@ -69,7 +70,8 @@ PMU_FORMAT_ATTR(sinkid, "config2:0-31"); /* config ID - set if a system configuration is selected */ PMU_FORMAT_ATTR(configid, "config2:32-63"); PMU_FORMAT_ATTR(cc_threshold, "config3:0-11");

+/* Interval = (2 ^ ts_level) */ +GEN_PMU_FORMAT_ATTR(ts_level);

/*

• contextid always traces the "PID". The PID is in CONTEXTIDR_EL1

@@ -103,11 +105,23 @@ static struct attribute *etm_config_formats_attr[] = { &format_attr_configid.attr, &format_attr_branch_broadcast.attr, &format_attr_cc_threshold.attr,

•   &format_attr_ts_level.attr,
    NULL,
  

};

+static umode_t etm_format_attr_is_visible(struct kobject *kobj,

•                                     struct attribute *attr, int unused)
  

+{

•   if (attr == &format_attr_ts_level.attr &&
  

•       !IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X))
  

•           return 0;
  

•   return attr->mode;
  

+}

static const struct attribute_group etm_pmu_format_group = { .name = "format",

•   .is_visible = etm_format_attr_is_visible,
    .attrs  = etm_config_formats_attr,
  

};

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 5febbcdb8696..d2664ffb33e5 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"

@@ -20,6 +21,12 @@ struct cscfg_config_desc; */ #define ETM_ADDR_CMP_MAX 8

+#define ATTR_CFG_FLD_ts_level_CFG config3 +#define ATTR_CFG_FLD_ts_level_LO 12 +#define ATTR_CFG_FLD_ts_level_HI 15 +#define ATTR_CFG_FLD_ts_level_MASK GENMASK(ATTR_CFG_FLD_ts_level_HI, \

•                                           ATTR_CFG_FLD_ts_level_LO)
  

/**

• struct etm_filter - single instruction range or start/stop configuration.
• @start_addr: The address to start tracing on.

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 920d092ef862..034844f52bb2 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -28,6 +28,7 @@ #include <linux/amba/bus.h> #include <linux/seq_file.h> #include <linux/uaccess.h> +#include <linux/perf/arm_pmu.h> #include <linux/perf_event.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> @@ -616,7 +617,7 @@ static void etm4_enable_hw_smp_call(void *info)

• +--------------+

•     |
  

• +------v-------+

• • | Counter x | (reload to 1 on underflow)

• • | Counter x | (reload to 2 ^ ts_level on underflow)
  • +--------------+

  •     |
    

  • +------v--------------+

@@ -627,11 +628,17 @@ static void etm4_enable_hw_smp_call(void *info)

• | Timestamp Generator | (timestamp on resource y)
• +----------------------+

*/ -static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata) +static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata,

•                                  struct perf_event_attr *attr)
  

{ int ctridx; int rselector; struct etmv4_config *config = &drvdata->config;

•   u8 ts_level = ATTR_CFG_GET_FLD(attr, ts_level);
  

•   /* Disable when ts_level == MAX */
  

•   if (ts_level == FIELD_GET(ATTR_CFG_FLD_ts_level_MASK, UINT_MAX))
  

•           return 0;
  
    /* No point in trying if we don't have at least one counter */
    if (!drvdata->nr_cntr)
  

@@ -667,12 +674,8 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata) return -ENOSPC; }

•   /*
  

•    * Initialise original and reload counter value to the smallest
  

•    * possible value in order to get as much precision as we can.
  

•    */
  

•   config->cntr_val[ctridx] = 1;
  

•   config->cntrldvr[ctridx] = 1;
  

•   /* Initialise original and reload counter value. */
  

•   config->cntr_val[ctridx] = config->cntrldvr[ctridx] = 1 << ts_level;
  
    /*
     * Trace Counter Control Register TRCCNTCTLRn
  

@@ -762,7 +765,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