Re: [PATCH v4 10/15] coresight: Add 'in_idle' argument to enable/disable path functions

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

On Mon, Nov 10, 2025 at 11:48:45AM +0000, James Clark wrote:

[...]

> > >   static void coresight_disable_path_from(struct coresight_path *path,
> > > -                    struct coresight_node *nd)
> > > +                    struct coresight_node *nd,
> > > +                    bool in_idle)
> > >   {
> > >       u32 type;
> > >       struct coresight_device *csdev, *parent, *child;
> > > @@ -498,6 +499,10 @@ static void coresight_disable_path_from(struct
> > > coresight_path *path,
> > >                           CORESIGHT_DEV_TYPE_SINK :
> > >                           CORESIGHT_DEV_TYPE_LINK;
> > > +        /* To reduce latency, CPU idle does not touch the sink */
> > > +        if (in_idle && type == CORESIGHT_DEV_TYPE_SINK)
> > > +            continue;
> > > +
> > >           switch (type) {
> > >           case CORESIGHT_DEV_TYPE_SINK:
> > >               coresight_disable_sink(csdev);
> > > @@ -527,7 +532,7 @@ static void coresight_disable_path_from(struct
> > > coresight_path *path,
> > >   void coresight_disable_path(struct coresight_path *path)
> > >   {
> > > -    coresight_disable_path_from(path, NULL);
> > > +    coresight_disable_path_from(path, NULL, false);
> > 
> > Something like a 'reason' enum would be more readable than a boolean. We
> > might have more than two options in the future anyway.

I will add a "path->in_idle" flag instead.

> > >   }
> > >   EXPORT_SYMBOL_GPL(coresight_disable_path);
> > > @@ -550,8 +555,9 @@ static int coresight_enable_helpers(struct
> > > coresight_device *csdev,
> > >       return 0;
> > >   }
> > > -int coresight_enable_path(struct coresight_path *path, enum cs_mode
> > > mode,
> > > -              void *sink_data)
> > > +static int coresight_enable_path_internal(struct coresight_path *path,
> > > +                      enum cs_mode mode, void *sink_data,
> > > +                      bool in_idle)
> > >   {
> > >       int ret = 0;
> > >       u32 type;
> > > @@ -564,10 +570,6 @@ int coresight_enable_path(struct coresight_path
> > > *path, enum cs_mode mode,
> > >           csdev = nd->csdev;
> > >           type = csdev->type;
> > > -        /* Enable all helpers adjacent to the path first */
> > > -        ret = coresight_enable_helpers(csdev, mode, path);
> > > -        if (ret)
> > > -            goto err_disable_path;
> > >           /*
> > >            * ETF devices are tricky... They can be a link or a sink,
> > >            * depending on how they are configured.  If an ETF has been
> > > @@ -579,6 +581,15 @@ int coresight_enable_path(struct coresight_path
> > > *path, enum cs_mode mode,
> > >                           CORESIGHT_DEV_TYPE_SINK :
> > >                           CORESIGHT_DEV_TYPE_LINK;
> > > +        /* To reduce latency, CPU idle does not touch the sink */
> > > +        if (in_idle && type == CORESIGHT_DEV_TYPE_SINK)
> > > +            continue;
> > 
> > I can imagine that not being wanted for all sink types in the future, so
> > we could let the sink decide this. But it's probably fine to leave in
> > the core for now and it would be easy to change later.
> 
> I got to patch 12 and it adds a special condition in the core to do
> something different for TRBE. We already have pm_is_needed() so shouldn't
> the sinks be deciding this for themselves?

pm_is_needed() is related to pm_save_disable()/pm_restore_enable()
pairs. OTOH, coresight_enable_path() invokes enable()/disable()
callbacks.  I don't want to mix up two different things.

But I will check if can use pm_is_needed() for sink device in
cpu_pm_save()/cpu_pm_restore().

Thanks,
Leo