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

[Mike Leach <mike.leach@linaro.org>]

OK I see it now. The real problem is that the logic to determine if we
are to set a counter based timestamp is confusingly spread across two
functions and two patches.

On Thu, 27 Nov 2025 at 15:48, Mike Leach <mike.leach@linaro.org> wrote:
>
> Hi James
>
> On Wed, 26 Nov 2025 at 10:57, 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. 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 c7bf73c8f2d7..0129b0502726 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)

Should pass ts_level in here

> >  {
> >         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));
> > +
>
> I could be missing something here - but if we have a perf command line:
>
> perf -e cs_etm/timestamp=0/
>
> is this bit not changing that to timestamp=1 regardless? The docs
> (patch 13) indicate timestamp=0 to be timestamps off.
>
> This command is used in test_arm_coresight.sh when testing the
> combination of options on the CS system.
>
> Mike
>
> > +       /*
> > +        * Disable counter generated timestamps when timestamp == MAX. Leave
> > +        * only SYNC timestamps.
> > +        */
> > +       if (ts_level == ATTR_CFG_GET_FLD(&max_timestamp, timestamp))
> > +               return 0;

All the attr manipulation logic should be where this function is
called not in here.

Mike

> >
> >         /* 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
> >
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK