Re: [PATCH V2] coresight: Make sysFS functional on topologies with per core sink

[Linu Cherian <linuc.decode@gmail.com>]

Hi Anshuman,

On Mon, Aug 3, 2020 at 3:07 PM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> Hello Linu,
>
> Please do mention the tree on which this patch applies on. The coresight
> API functions being changed here are not even present on v5.8 but instead
> are part of linux-next (atleast next-20200731).

Will be more explicit next time, though i have mentioned the relevant
tree below.

>
> On 08/01/2020 02:39 PM, Linu Cherian wrote:
> > Coresight driver assumes sink is common across all the ETMs,
>
> Does 'common' just implies reachable here. Should not all the sinks on
> the system be reachable from all the sources.

Common implies, a topology where a sink is common to all ETMs,
a global ETR for example. Please see below for details.

>
> > and tries to build a path between ETM and the first enabled
> > sink found using bus based search. This breaks sysFS usage
> > on implementations that has multiple per core sinks in
> > enabled state.
>
> Could you please explain how it breaks the sysfs usage ? Why those per
> core sinks were not found through existing bus based search.
>

On an implementation with per core ETR, and when a user tries to enable
multiple sinks using the syfs interface, only the first sink on the bus would
get enabled at hardware level.
This is because coresight_get_enabled_sink would always select the
first user enabled
sink on the coresight bus, which works fine on implementations with
global ETR but
not for per core ETR/ETF.

So for example, when an user enables ETM 0 and ETR0 , followed by ETM
1 and ETR 1,
then ETR1 hardware block will never get enabled.


> >
> > For this,
> > - coresight_find_sink API is updated with an additional flag
> >   so that it is able to return an enabled sink
> > - coresight_get_enabled_sink API is updated to do a
> >   connection based search, when a source reference is given.
> >
> > Change-Id: I6cc91ddb3ef8936a8f41a5f7c7c455b0ece9d85d
> > Signed-off-by: Linu Cherian <lcherian@marvell.com>
> > ---
> > Applies on https://git.linaro.org/kernel/coresight.git/log/?h=next
> >
> > Changes in V2:
> > - Fixed few typos in commit message
> > - Rephrased commit message
> >
> >  .../hwtracing/coresight/coresight-etm-perf.c  |  2 +-
> >  drivers/hwtracing/coresight/coresight-priv.h  |  5 +-
> >  drivers/hwtracing/coresight/coresight.c       | 51 +++++++++++++++++--
> >  3 files changed, 51 insertions(+), 7 deletions(-)
> >



> > +/**
> > + * coresight_find_enabled_sink: Find the suitable enabled sink
> > + *
> > + * @csdev: starting source to find a connected sink.
> > + *
> > + * Walks connections graph looking for a suitable sink to enable for the
> > + * supplied source. Uses CoreSight device subtypes and distance from source
> > + * to select the best sink.
> > + *
> > + * Used in cases where the CoreSight user (sysfs) has selected a sink.
> > + */
> > +struct coresight_device *
> > +coresight_find_enabled_sink(struct coresight_device *csdev)
> > +{
> > +     int depth = 0;
> > +
> > +     /* look for the enabled sink */
> > +     return coresight_find_sink(csdev, &depth, true);
> > +}
> > +
>
> Why this wrapper is required ? Could not coresight_find_sink() be used
> directly in the only caller i.e coresight_get_enabled_sink(). Besides,
> their names are really very similar and might lead to confusion.

Thought that a wrapper avoids exposing the internals like "depth" in
the search API
to its users.

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