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
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
-
Leo Yan