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

[Suzuki K Poulose]

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

May be this is due to AUX pause case ? The mode could still be PERF, but
  the ETM/ETE is disabled ?

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

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