Re: [PATCH v4 1/9] coresight: etm4x: introduce struct etm4_caps

[Leo Yan]

On Mon, Apr 13, 2026 at 03:19:54PM +0100, Yeoreum Yun wrote:
> Introduce struct etm4_caps to describe ETMv4 capabilities
> and move capabilities information into it.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>

LGTM:

Reviewed-by: Leo Yan <leo.yan@xxxxxxx>

FWIW, two comments from Sashiko are valuable for me, please see below.

> ---
>  .../coresight/coresight-etm4x-core.c          | 234 +++++++++---------
>  .../coresight/coresight-etm4x-sysfs.c         | 190 ++++++++------
>  drivers/hwtracing/coresight/coresight-etm4x.h | 176 ++++++-------
>  3 files changed, 328 insertions(+), 272 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index d565a73f0042..6443f3717b37 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -88,8 +88,9 @@ static int etm4_probe_cpu(unsigned int cpu);
>   */
>  static bool etm4x_sspcicrn_present(struct etmv4_drvdata *drvdata, int n)
>  {
> -	return (n < drvdata->nr_ss_cmp) &&
> -	       drvdata->nr_pe &&
> +	const struct etmv4_caps *caps = &drvdata->caps;
> +
> +	return (n < caps->nr_ss_cmp) && caps->nr_pe &&
>  	       (drvdata->config.ss_status[n] & TRCSSCSRn_PC);

As Sashiko suggests:

  "This isn't a regression introduced by this patch, but should this be
   checking caps->nr_pe_cmp instead of caps->nr_pe?"

I confirmed the ETMv4 specification (ARM IHI0064H.b), the comment
above is valid as the we should check caps->nr_pe_cmp instead.

Could you first use a patch to fix the typo and then apply
capabilities afterwards?  This is helpful for porting to stable
kernels.

[...]

> @@ -525,14 +530,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1))
>  		dev_err(etm_dev,
>  			"timeout while waiting for Idle Trace Status\n");
> -	if (drvdata->nr_pe)
> +	if (caps->nr_pe)
>  		etm4x_relaxed_write32(csa, config->pe_sel, TRCPROCSELR);
>  	etm4x_relaxed_write32(csa, config->cfg, TRCCONFIGR);
>  	/* nothing specific implemented */
>  	etm4x_relaxed_write32(csa, 0x0, TRCAUXCTLR);
>  	etm4x_relaxed_write32(csa, config->eventctrl0, TRCEVENTCTL0R);
>  	etm4x_relaxed_write32(csa, config->eventctrl1, TRCEVENTCTL1R);
> -	if (drvdata->stallctl)
> +	if (caps->stallctl)
>  		etm4x_relaxed_write32(csa, config->stall_ctrl, TRCSTALLCTLR);
>  	etm4x_relaxed_write32(csa, config->ts_ctrl, TRCTSCTLR);
>  	etm4x_relaxed_write32(csa, config->syncfreq, TRCSYNCPR);
> @@ -542,17 +547,17 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	etm4x_relaxed_write32(csa, config->vinst_ctrl, TRCVICTLR);
>  	etm4x_relaxed_write32(csa, config->viiectlr, TRCVIIECTLR);
>  	etm4x_relaxed_write32(csa, config->vissctlr, TRCVISSCTLR);
> -	if (drvdata->nr_pe_cmp)
> +	if (caps->nr_pe_cmp)
>  		etm4x_relaxed_write32(csa, config->vipcssctlr, TRCVIPCSSCTLR);
> -	for (i = 0; i < drvdata->nrseqstate - 1; i++)
> +	for (i = 0; i < caps->nrseqstate - 1; i++)
>  		etm4x_relaxed_write32(csa, config->seq_ctrl[i], TRCSEQEVRn(i));

Sashiko's comment:

  "If the hardware does not implement a sequencer, caps->nrseqstate (a u8)
   will be 0. Does 0 - 1 evaluate to -1 as an int, which then gets promoted
   to ULONG_MAX against val (an unsigned long)?"

This is a good catch.  The condition check should be:

  for (i = 0; i < caps->nrseqstate; i++)
       ...;

The issue is irrelevant to your patch, but could you use a patch to fix
"nrseqstate - 1" first and then apply the cap refactoring on it?  This
would be friendly for porting to stable kernel.

Thanks,
Leo