[PATCH v3 10/31] coresight: etm4x: Reuse normal enable and disable logic in CPU idle

[Leo Yan]

Hi Mike,

On Wed, Oct 01, 2025 at 09:48:06AM +0100, Mike Leach wrote:

> Hi Leo,
> 
> Overall I like the idea, but there are some minor issues that need addressing.

Thanks for review!

[...]

> > 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.
> >
> 
> Not sure I agree - in the original save restore the claim tag state
> was retained - so if an external claim was asserted it would not be
> lost.
> here if we do not retain the claim tag value, then an external state
> can be lost.

As described in PSCI (DEN0022F.b 1.3), section 6.8.1.3 "Saving and
restoring trace context", when ETMCLAIM[0] is set to 1 by the external
debugger, "The supervisory software will perform a save and restore
sequence of the trace context". So the supervisory software
(TF-a or EL2 hypervisor) should save and restore context (including
the claim bit) for the external debugger.

It does not make much sense to use the ETM driver to save and restore
only the claim bit while ignoring other registers. For a robust
solution, the supervisory software would be better place for the job.

> The debugger cannot access the device during idle/oslok, but it can
> detect that the device is powered down/oslocked and this potential is
> a change of behaviour from the external debug perspective, where a
> race to claim the resource now exists over powerdown were one did not
> before.

If we consider the supervisory software, then no race condition occurs.
As the supervisory software must run prior to the Linux kernel during
resume, it will safely restore claim bit for the external debugger.

Thoughts?

[...]

> > -       if (drvdata->nrseqstate) {
> > -               state->trcseqrstevr = etm4x_read32(csa, TRCSEQRSTEVR);
> > -               state->trcseqstr = etm4x_read32(csa, TRCSEQSTR);
> 
> The TRCSEQSTR changes in use, but is not read back be the standard
> enable/disable flow. This does not matter in a normal enable/disable
> case, but in a save/restore we need to retain the current state.
> Probably should be added to the normal disable flow to read back this
> as we do with counters etc.

Good point.  I will update in next spin.

Thanks,
Leo