On 04/06/2025 10:48 am, Suzuki K Poulose wrote:
On 03/06/2025 14:40, James Clark wrote:
On 16/05/2025 5:07 pm, Leo Yan wrote:
If a trace unit has been enabled, a CPU suspend operation misses to disable the trace unit. After the CPU resumes, it also does not follow the general flow for re-enabling the trace unit. As a result, this may lead to a wrong state machine for the ETM.
This commit stops the trace unit during the CPU suspend flow and re-enables the trace unit in the resume flow.
Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 15 +++++++++++ +++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ drivers/hwtracing/coresight/coresight-etm4x-core.c index 6a5898355a83..b12d59b89a49 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1820,6 +1820,11 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) state = drvdata->save_state; state->trcprgctlr = etm4x_read32(csa, TRCPRGCTLR);
+ /* Stop the trace unit if it is enabled */ + if (state->trcprgctlr)
etm4_cpu_save() looks at coresight_get_mode() to determine enabled state. Seems a bit inconsistent to look directly at the enable bit right after checking coresight_get_mode().
May be this is due to AUX pause case ? The mode could still be PERF, but the ETM/ETE is disabled ?
Disabling unconditionally would be harmless, and that's quite a bit of an edge case so it might be worth the simplification.
Then __etm4_cpu_save() could remove the duplicate wait on TRCSTATR_PMSTABLE_BIT and multiple reads of TRCPRGCTLR. There are other inconsistencies like one does etm4x_relaxed_read32(csa, TRCPRGCTLR) and the other does etm4x_read32(csa, TRCPRGCTLR).
+ etm4_disable_trace_unit(drvdata);
Section 3.4.1 in ARM IHI 0064H.b doesn't say that anything extra needs to be done here, and it implies that restoring to an enabled state should work. It might make sense to mention why it diverges from the published sequence. Or get the sequence updated? I suppose that document doesn't include ETE and I see there is one extra register missing from __etm4_cpu_save() which is written to in etm4_disable_trace_unit().
Plus we end up doing extra things like two waits on TRCSTATR.PMSTABLE.
I guess, we need to stop the tracing before we reliably capture the register states (similar to the disable/pause sequence).
Is that because the counters and comparators are changing? I'm not sure why we need to read everything anyway, etm4_enable_hw() is done entirely from drvdata, implying that everything we need is already saved.
If there are only two things that might be different to drvdata (TRCSSCSRn and TRCCNTVRn) we could only save those in __etm4_cpu_save(). But then the function looks an awful lot like etm4_disable_hw() and I wonder if there isn't a way to share the code to show we're doing basically the same thing.
There are a few other small inconsistencies between disabling and saving like etm4_disable_hw() removes power first and __etm4_cpu_save() does it last. And __etm4_cpu_restore() doesn't follow the correct sequence to write to TRCCLAIMSET.
Could it look something like this:
void __etm4_cpu_save() { etm4_disable_hw(drvdata); drvdata->state_needs_restore = true; }
void __etm4_cpu_restore() { etm4_enable_hw(drvdata, false); drvdata->state_needs_restore = false; }
Where the extra argument for enable stops clearing the comparator status:
void etm4_enable_hw(struct etmv4_drvdata *drvdata, bool clear_cmp) { /* clear status bit on restart if using single-shot */ if (clear_cmp && (config->ss_ctrl[i] || config->ss_pe_cmp[i])) config->ss_status[i] &= ~TRCSSCSRn_STATUS;
Suzuki
if (drvdata->nr_pe) state->trcprocselr = etm4x_read32(csa, TRCPROCSELR); state->trcconfigr = etm4x_read32(csa, TRCCONFIGR); @@ -1951,7 +1956,6 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) etm4_cs_unlock(drvdata, csa); etm4x_relaxed_write32(csa, state->trcclaimset, TRCCLAIMSET); - etm4x_relaxed_write32(csa, state->trcprgctlr, TRCPRGCTLR); if (drvdata->nr_pe) etm4x_relaxed_write32(csa, state->trcprocselr, TRCPROCSELR); etm4x_relaxed_write32(csa, state->trcconfigr, TRCCONFIGR); @@ -2035,6 +2039,15 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata) /* Unlock the OS lock to re-enable trace and external debug access */ etm4_os_unlock(drvdata);
+ /* + * Re-enable the trace unit if it was enabled before suspend. + * Put it after etm4_os_unlock() as unlocking the OS lock is the + * prerequisite for enabling the trace unit. + */ + if (state->trcprgctlr) + etm4_enable_trace_unit(drvdata);
etm4_cs_lock(drvdata, csa); }