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

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Leo!

> 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@arm.com>
>
> LGTM:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>

Thanks.

>
> 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.

Sashiko finds valid point. And I'm bit of surprised no body find it.
Okay. I'll send it next round.

>
> [...]
>
> > @@ -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.

Okay. I'll send it next round.

Thanks!

--
Sincerely,
Yeoreum Yun