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

From: Andrew Murray <andrew.murray@arm.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Al Grant <Al.Grant@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Mike Leach <mike.leach@linaro.org>
Subject: Re: [PATCH v1 5/5] coresight: etm4x: save/restore state across CPU low power states
Date: Thu, 20 Jun 2019 12:41:17 +0100	[thread overview]
Message-ID: <20190620114116.GE20984@e119886-lin.cambridge.arm.com> (raw)
In-Reply-To: <CANLsYkw-KhMVgTfyBSF4-uv4wxQBBQfzyvVbAnaFSqHhkgX6Mg@mail.gmail.com>

On Wed, Jun 19, 2019 at 10:22:58AM -0600, Mathieu Poirier wrote:
> On Wed, 19 Jun 2019 at 05:07, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > 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.
> 
> I think using ACPI/DT is the best and simplest solution.

I certainly agree that it feels wrong to have a default level of support
which is targeted at broken systems. However the penalty (latency) for doing so
doesn't seem high - seeing as this only effects users that are actively using
coresight (I assume self hosted mode is only used as a debug tool, rather than to
obtain metrics during normal use?).

Adding some broken tag in ACPI/DT seems like a good solution - assuming it will
get adopted and used in systems. The existing "disable_pm_save" module option
can be renamed to "enable_pm_save" for those that have less control of their
firmware.

Unless of course we think it's unlikely we'll ever see hardware that isn't
broken - I don't have enough knowledge of how likely or not this is.

Another solution might be to enable save/restore by default (as it is now),
and then on resume we read the hardware registers to determine if state was
lost. If it wasn't lost then we can disable the save/restore feature. (Though
is it possible for systems to be partly broken, e.g. working for some CPUs
but not others?). With this approach on good systems you only get penalised
once.

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

I'll update the cover letter and commit messages to reflect that this
option is present. (And likewise for conditionally saving/restoring the
registers only if coresight is in use).

> >
> > > >
> > > > Just curious if this reported on any platforms ?
> > > >
> > >
> > > I have heard people complaining about this, but not sure about the exact
> > > platform(s) affected.
> 
> Are you referring to platforms that ignore the TRCPDCR.PU bit?  If so
> Juno is the only one that does _not_ ignore it, hence the need to find
> another solution.
> 
> > >
> >
> > 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.
> 
> I wouldn't assume that anything is working properly.

Thanks,

Andrew Murray

> 
> >
> > 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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel