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

[Mike Leach]

Hi Leo

On Wed, 1 Oct 2025 at 11:25, Leo Yan <leo.yan at arm.com> wrote:
>
> 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 the firmware is PSCI compliant there is no need to save/restore the
context within the kernel driver if the external claim bit is set?

Mike

> [...]
>
> > > -       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


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK