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

[Andrew Murray <andrew.murray@arm.com>]

On Wed, Jun 26, 2019 at 10:57:17AM -0600, Mathieu Poirier wrote:
> 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.

Indeed, thanks for the helpful feedback.

> 
> 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 for clarifying this for me. I'll share a v2 shortly.

Thanks,

Andrew Murray

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