Re: [PATCH v1 5/5] coresight: etm4x: save/restore state across CPU low power states

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Wed, 26 Jun 2019 at 04:21, Mike Leach <mike.leach@linaro.org> wrote:
>
> Hi,
>
> Sorry, a bit late on this set as it didn't appear in the Coresight
> mailing list as expected per suzukis suggestion.
>
> On Tue, 25 Jun 2019 at 20:57, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > Hi,
> >
> > On Tue, 25 Jun 2019 at 04:07, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> > >
> > > Hi Mathieu,
> > >
> > > On 18/06/2019 23:55, Mathieu Poirier wrote:
> > > > On Tue, Jun 18, 2019 at 01:54:33PM +0100, Andrew Murray wrote:
> > > >> Some hardware will ignore bit TRCPDCR.PU which is used to signal
> > > >> to hardware that power should not be removed from the trace unit.
> > > >> Let's mitigate against this by saving and restoring the trace
> > > >> unit state when the CPU enters low power states.
> > > >>
> > > >> To provide the benefit to both self-hosted and external debuggers
> > > >> we save/restore the entire state which includes etmv4_config data
> > > >> and dynamic data such as inflight counter values, sequencer
> > > >> states, etc.
> > > >>
> > > >> To reduce CPU suspend/resume latency the state is only saved or
> > > >> restored if coresight is in use as determined by the claimset
> > > >> registers.
> > > >>
> > > >> To aid debug of CPU suspend/resume a disable_pm_save parameter
> > > >> is provided to disable this feature.
> > > >>
> > > >> Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > >
> > >
> > > >> +static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> > > >> +                          void *v)
> > > >> +{
> > > >> +    struct etmv4_drvdata *drvdata = container_of(nb,
> > > >> +                                    struct etmv4_drvdata, nb);
> > > >> +
> > > >> +    if (disable_pm_save)
> > > >> +            return NOTIFY_OK;
> > > >> +
> > > >> +    switch (cmd) {
> > > >> +    case CPU_PM_ENTER:
> > > >> +            /* save the state if coresight is in use */
> > > >> +            if (coresight_is_claimed_any(drvdata->base))
> > > >
> > > > claimed_any()? At this point if coresight_is_claimed_self_hosted() == false an
> > > > external agent is competing with the framework and we should abdicate.
> > >
> > > I think claimed_any() is correct check. As per PSCI, ARM DEN 0022D, section
> > > 6.8.1 Debug and Trace save and restore,  the OS software is
> > > in charge of save/restoring the context of Debug/Trace. The claim tags
> > > are a mechanism to indicate who is consuming the components. Also, given
> > > the OS software doesn't have a reliable way to communicate back to the
> > > the External debugger about its decision to power down the CPU, that
> > > makes sense to save/restore it.
> >
> > What I understand from section 6.8.1 is that supervisory and OS power
> > management SW are responsible to save the debug context when operating
> > in their respective mode, which reflects my comment above.
> >
> > I also see that two options are available to an external agent, i.e
> > either use the DBGNOPWRDWN and DBGPWRUPREQ bits to request powerdown
> > emulation or use the "OS Unlock Catch" debug event (which probably
> > relates to the lost of context bit) to restore the debug context.
> > From where I stand there is no provision for OS power management code
> > to take care of the debug context of an external agent.  Am I missing
> > something here?
> >
>
> OS lock is precisely the provision designed for an OS to handle
> save/restore on behalf of an external debug agent. OS lock blocks the
> external debugger from accessing the coresight when it is powered but
> being updated by the OS
>
> A scenario may be:-
> a) external debug halts core(s) & programs Coresight subsystem -
> likely extracting trace via TPIU.
> b) external debug agent restarts cores - linux (continues) running /
> booting - collecting the trace we want.
> c) Some event happens and the external debug agent regains control.
> (breakpoint / halt request).
>
> During b) cores may be powering up and down. When this happens we need
> the state to be saved and restored so that trace continues. (assuming
> that the various debug power requests above are either not supported
> in the fw/hardware or not asserted by the external agent).
> The external debug agent cannot safely manipulate coresight during
> this period - it can never know if a register is going to be available
> - a classic race condition.
>
> Irrespective of whoever "owns" the ETM programming - if the CPUidle
> notification is required due to implementation issues, then in both
> cases the save and restore is required.
>
> For the external agent owner I agree that everything needs to be saved
>  - but for self hosted, just the dynamic values should be read back,
> much of the remainder of the information is already held in the driver
> in etmv4_config. This should help reduce at least the power down
> latency.
>
>

Many thanks for shedding light on the expectations of external agents.

Andrew, from Mike's explanation the original implementation of
checking any of the claimtag bits to trigger a save/restore was valid.
It also means that contrary to one of my previous comment, context
save restore should always be performed regardless of whether the
framework is being used for self hosted debugging or not.

Thanks,
Mathieu

>
>
> Regards
>
> Mike
>
>
> > >
> > > Cheers
> > > Suzuki
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel