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

[James Clark]

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 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 ?
> 

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