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

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, Jun 20, 2019 at 12:41:17PM +0100, Andrew Murray wrote:
> 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?).
>

Do we have numbers ? It's always helpful to have lowest latencies possible
for wakeup and adding extra notifiers will always add some latencies,
so it's not 0. We always want to reduce there notifiers and hopefully
move save/restore to hardware/firmware in future.

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

Sorry but even then I prefer it not to be default and force extra work
to the people who add support and constantly be reminded that it's
broken and they are deviating from default behaviour in the kernel
which may come and latency penalty.

Making it default may hide the problem if Linux is used for some validation.

Also we hardly have 3-4 platforms in upstream that support coresight,
and many are broken except Juno. But that doesn't imply all others
are broken and we just can't derive that unless we have more information.

--
Regards,
Sudeep

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