[PATCH v1 01/11] coresight: etm4x: Control the trace unit in CPU suspend

[James Clark]

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 at 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().

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

>   	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);
>   }
>