[PATCH v5 11/11] coresight: etm4x: Reuse normal enable and disable logic in CPU idle

[Suzuki K Poulose]

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:
> 
> 1) 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.
> 
> 2) 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.
> 
> 3) 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.
> 
> 4) 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 at linaro.org>
> Reviewed-by: Yeoreum Yun <yeoreum.yun at arm.com>
> Signed-off-by: Leo Yan <leo.yan at 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