Re: [PATCH] coresight: trbe: Save/restore state across CPU low power state

[Leo Yan <leo.yan@arm.com>]

Hi all,

On Mon, Apr 28, 2025 at 11:53:26AM +0100, Mike Leach wrote:

[...]

> > > > @@ -362,6 +362,10 @@ enum cs_mode {
> > > >   * @alloc_buffer:    initialises perf's ring buffer for trace collection.
> > > >   * @free_buffer:     release memory allocated in @get_config.
> > > >   * @update_buffer:   update buffer pointers after a trace session.
> > > > + * @percpu_save:     saves state when CPU enters idle state.
> > > > + *                   Only set for percpu sink.
> > > > + * @percpu_restore:  restores state when CPU exits idle state.
> > > > + *                   only set for percpu sink.
> > > >   */
> > > >  struct coresight_ops_sink {
> > > >       int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > > > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > > >       unsigned long (*update_buffer)(struct coresight_device *csdev,
> > > >                             struct perf_output_handle *handle,
> > > >                             void *sink_config);
> > > > +     void (*percpu_save)(struct coresight_device *csdev);
> > > > +     void (*percpu_restore)(struct coresight_device *csdev);
> > >
> > > Again - why this percpu_* prefix ?
> > >
> > > >  };
> > > >
> > > >  /**
> 
> I do not think this is the best approach.
> 
> The TRBE driver has its own power management registration functions,
> so is it not possible for the save and restore should be handled
> there, through a PM notifier, just as the ETM/ETE is?
> 
> Drop the unnecessary DT entry - TRBE is a per cpu architectural device
> - if a TRBE is present, we know it will power down with the PE.
> 
> The CoreSight architecture permits an ETE to drive trace to an
> external sink - so the TRBE might be present but unused, therefore
> hooking into a source driver that may not be driving trace into the
> device does not seem wise..

Sorry I jumped in a bit late (I saw the patch at last week and it is
on my review list).

I would suggest to hold on for this patch.  I am refactoring CPU PM and
hotplug in CoreSight based on the CoreSight path.

The idea is when we do CPU power management for CoreSight devices, we
cannot simply control individual devices.  Alternatively, we need to
control logics based on the linkages on CoreSight path as it can reflect
dependencies crossing the components.  For example, for a CoreSight
path, when running into CPU low power state, we need firstly disable
tracer, then links, and at the end sinks.  When CPU resumes back, we
need to use the reversed ordering for turning on devices.

As a result, except the tracers (ETM / ETE) should be saved and
restored contexts during CPU power states, other components on the
path will be controlled by traversing CoreSight paths.  The benefit is
if a component (e.g., a link or a sink module) is shared by multiple
CoreSight paths, then the component can be disabled or enabled based on
reference counter.

I know I am a bit lagged - as I also need to support the locking on
CoreSight paths.  Please expect in next 1~2 weeks I will send out
patches for public review.

> The TRBE PM can follow the model of the ETE / ETMv4 and save and
> restore if currently in use.

If TRBE PM is registered as a seperate PM notifier, a prominent issue is
it cannot promise the depedency between ETE and TRBE when execute CPU
power management.  E.g., when entering CPU idle states, ETE should be
disabled prior to switch off TRBE, otherwise, it might cause lockup
issue in hardware.  If ETE and TRBE register PM notifier separately,
we cannot ensure the sequence between ETE and TRBE, this is why we need
to do the operations based on CoreSight paths.

Thanks,
Leo