Message-ID: <20251021172224.GO281971@e132581.arm.com>
Date: Tue, 21 Oct 2025 18:22:24 +0100
From: Leo Yan <leo.yan@....com>
To: James Clark <james.clark@...aro.org>
Cc: Mike Leach <mike.leach@...aro.org>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jonathan Corbet <corbet@....net>, coresight@...ts.linaro.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 4/5] coresight: Add format attribute for setting the
 timestamp interval

On Wed, Oct 15, 2025 at 04:19:04PM +0100, James Clark wrote:

[...]

> > > -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;
> > > 
> > 
> > Returning 0 from this function _enables_ the timestamps
> > 
> 
> Returning 0 just means that etm4_parse_event_config() doesn't exit with an
> error. For ts_level == MAX we want to disable timestamps generated by the
> counter, but we still want the minimum periodic timestamps.
> 
> To disable all timestamps you'd need to have attr->config & BIT(ETM_OPT_TS)
> == false. This is set by the "timestamp" format flag which I tried to
> explain that in the docs change.
> 
> I could also change the comment to say "/* Disable counter generated
> timestamps with ts_level == MAX */"
> 
> It's unfortunate that there are now two format options for timestamps. Maybe
> instead of adding a second option we can change "timestamp" from a 1 bit
> field to 4 bits, with the following meanings:
> 
>  0:     No counter timestamps or SYNC timestamps
>  1-14:  Counter timestamps = 2 ^ x. Plus SYNC timestamps
>  15:    Only SYNC timestamps

I am just wandering how can extend "timestamp" from 1 bit to 4 bits.

  #define ETM_OPT_TS              28
  #define ETM_OPT_RETSTK          29

  PMU_FORMAT_ATTR(timestamp,      "config:" __stringify(ETM_OPT_TS));
  PMU_FORMAT_ATTR(retstack,       "config:" __stringify(ETM_OPT_RETSTK));

"retstack" has occupied a higher bit, we cannot naturelly extend
"timestamp" field?

Even we can extend "timestamp" format to 4 bits, it will be mess when
run the updated perf on old kernels.  Let's see an example:

  perf record -e cs_etm/timestamp=0/ -- test
  perf record -e cs_etm/timestamp=2/ -- test

Because the lowest bit is cleared for both timestamp=0 and timestamp=2,
the old kernel support only one bit always treats these two setting as
timestamp disabled, or the perf tool needs to do extra checking for
old kernel.

> Now we basically have the same meanings except you also have to set the
> timestamp bit. Seems a bit pointless.

> Previous versions of Perf were hard coding the timestamp format bit rather
> than reading it out of
> "/sys/bus/event_source/devices/cs_etm/format/timestamp" though:
> 
> -       /* All good, let the kernel know */
> -       evsel->core.attr.config |= (1 << ETM_OPT_TS);
> 
> For that reason we'd have to leave that one where it is for backwards
> compatibility. If it's set it would be equivalent to the new wider timestamp
> field == 1.

Are you suggesting the timestamp field to be extended to two
non-consecutive fields?

For me, this is even worse than current two discrete formats. The reason
is it is complex in implementation, and it is not directive for usage
(users need to digest the field for three different semantics: on/off,
counter, and SYNC mode only).

Thanks,
Leo

> I don't know if there's any precedent for changing the bitfield that backs a
> format field, but presumably that's the point of publishing them in files
> rather than a header.


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

Powered by blists - more mailing lists