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

From: Sudeep Holla <[email-masked]>
Date: Wed, 19 Jun 2019 12:07:49 +0100

On Wed, Jun 19, 2019 at 11:38:12AM +0100, Suzuki K Poulose wrote:
> Cc: Al Grant, Mike Leach
>
> Hi Sudeep,
>
> On 18/06/2019 14:21, Sudeep Holla 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.
> >
> > So, how or can we identify or discover such system ? DT/ACPI ?
> >
>
> I don't think there is a mechanism at the moment to identify such
> systems. But if we really need to know this information, we could
> always think about it.
>

I prefer that as we shouldn't systems that are not broken.

> > > Let's mitigate against this by saving and restoring the trace
> > > unit state when the CPU enters low power states.
> > >
> >
> > I prefer to do this conditionally. It's unnecessary on systems which
> > don't ignore the TRCPDCR.PU and I really don't like them to be penalised
> > while we want to add this support for *broken* systems.
>
> It is conditional. i.e, you may disable the operation using a kernel/module
> parameter, which I think should be mentioned in the description here.
>

Why should the user of coresight need to know if the corresponding
hardware module is broken or not. I prefer the firmware tell OS.

> >
> > This is generally most useful to debug CPU suspend/resume exercising
> > the code path completely with emulated CPU power on/off as most of the
> > systems have the trace unit and the CPUs in the same power domain.
>
> I understand, which is specifically why this comes with an option to handle
> such cases.
>

OK

> >
> > Just curious if this reported on any platforms ?
> >
>
> I have heard people complaining about this, but not sure about the exact
> platform(s) affected.
>

One we add mechanism in place, platform need to advertise that it's
broken in firmware(DT/ACPI). Or just have a blacklist if we don't
want to add anything extra to the firmware(DT/ACPI) ?

> > I wounder if we can use TRCPDSR(Power Down Status Register) to check the
> > status. I know on Juno, it doesn't loose context rather the power down
> > is emulated and saving/restoring may not be needed at all. Have you
> > tested on Juno with and without these patches and seen any difference ?
>
> The problem is trace unit looses power the moment CPU goes to low power mode
> and if we try to read this register, it could cause unexpected side-effects.
>

No I meant before CPU loose power i.e. in CPU_ENTER case. However I do
remember you/Andrew mentioning that even that may be bogus on broken
systems, so firmware is only way to avoid penalising all platforms IMO.

Or other option is to stop the coresight tracing session like we do
for PMUs or not entering idle when there's any active coresight session
in progress on such platforms.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel