On 03/11/2025 16:49, Leo Yan wrote:
The etm4_enable_trace_unit() function is almost identical to the restore flow, with a few differences listed below:
TRCQCTLR register
The TRCQCTLR register is saved and restored during CPU idle, but it is never touched in the normal flow. Given the Q element is not enabled (TRCCONFIGR.QE bits), it is acceptable to omit saving and restoring this register during idle.
TRCSSCSRn.STATUS bit
The restore flow does not explicitly clear the TRCSSCSRn.STATUS bit but instead directly loads the saved value. In contrast, the normal flow clears this bit to 0 when re-enabling single-shot control.
Therefore, the restore function passes the restart_ss argument as false to etm4_enable_hw() to avoid re-enabling single-shot mode.
Claim Tag Handling
The claim tag is acquired and released in normal flow, on the other hand, the claim protocol is not applied in CPU idle flow - it simply restores the saved value.
The claim bits serve two purpose:
Exclusive access between the kernel driver and an external debugger. During CPU idle, the kernel driver locks the OS Lock, ensuring that the external debugger cannot access the trace unit. Therefore, it is safe to release the claim tag during idle.
Notification to supervisory software to save/restore context for external debuggers. The kernel driver does not touch the external debugger's claim bit (ETMCLAIM[0]).
Based on this, it is safe to acquire and release claim tag in the idle sequence.
OS Lock Behavior
The OS Lock should be locked during CPU idle states. This differs from the normal flow, which unlock it. This special handling remains in the CPU save path.
This commit reuses the normal enable and disable logic in the CPU idle path. The only addition is locking the OS Lock upon entering idle to ensure exclusive access.
The save context in the driver data is no longer needed and has been removed.
Tested-by: James Clark james.clark@linaro.org Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 201 +-------------------- drivers/hwtracing/coresight/coresight-etm4x.h | 57 ------ 2 files changed, 5 insertions(+), 253 deletions(-)
nice diff stat !
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 662492ddd296d9974406ddffad14e7ccb92edbae..d7da5f3b97b3b18d5d6eee0c20a3bdca0c4bec1a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1845,8 +1845,7 @@ static int etm4_dying_cpu(unsigned int cpu) static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) {
- int i, ret = 0;
- struct etmv4_save_state *state;
- int ret = 0; struct coresight_device *csdev = drvdata->csdev; struct csdev_access *csa; struct device *etm_dev;
@@ -1876,93 +1875,11 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) ret = -EBUSY; goto out; }
- etm4_cs_lock(drvdata, csa);
- etm4_disable_hw(drvdata);
- etm4_cs_unlock(drvdata, csa);
This looks a bit hacky. Here are the options :
1. Leave it as above, but clearly explain in a comment why we do this. 2. The other option is to restructure etm4_disable_hw into etm4_cs_unlock() __etm4_disable_hw() etm4_cs_lock()
And use __etm4_disable_hw() from here.
/* wait for TRCSTATR.IDLE to go up */ if (etm4x_wait_status(csa, TRCSTATR_IDLE_BIT, 1)) { dev_err(etm_dev, @@ -1972,14 +1889,6 @@ static int __etm4_cpu_save(struct etmv4_drvdata *drvdata) goto out;
This goto can be removed too.
Suzuki