Re: [PATCH v7 02/10] Coresight: Add coresight TPDM source driver

[Suzuki K Poulose]

On 01/06/2022 10:21, Jinlong Mao wrote:
> Hi Suzuki,
>
> On 5/24/2022 3:00 PM, Jinlong Mao wrote:
> > Hi Suzuki,
> >
> > Thank you for the review.
> >
> > On 5/23/2022 4:57 PM, Suzuki K Poulose wrote:
> > > Hi
> > >
> > > On 09/05/2022 14:39, Mao Jinlong wrote:
> > > > Add driver to support Coresight device TPDM (Trace, Profiling and
> > > > Diagnostics Monitor). TPDM is a monitor to collect data from
> > > > different datasets. This change is to add probe/enable/disable
> > > > functions for tpdm source.
> > > >
> > > > Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
> > > > Signed-off-by: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> > > > ---
> > > >   drivers/hwtracing/coresight/Kconfig          |  13 ++
> > > >   drivers/hwtracing/coresight/Makefile         |   1 +
> > > >   drivers/hwtracing/coresight/coresight-core.c |   5 +-
> > > >   drivers/hwtracing/coresight/coresight-tpdm.c | 146 
> > > > +++++++++++++++++++
> > > >   drivers/hwtracing/coresight/coresight-tpdm.h |  26 ++++
> > > >   include/linux/coresight.h                    |   1 +
> > > >   6 files changed, 191 insertions(+), 1 deletion(-)
> > > >   create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.c
> > > >   create mode 100644 drivers/hwtracing/coresight/coresight-tpdm.h
>
> > > > +/**
> > > > + * struct tpdm_drvdata - specifics associated to an TPDM component
> > > > + * @base:       memory mapped base address for this component.
> > > > + * @dev:        The device entity associated to this component.
> > > > + * @csdev:      component vitals needed by the framework.
> > > > + * @lock:       lock for the enable value.
> > > > + * @enable:     enable status of the component.
> > > > + */
> > > > +
> > > > +struct tpdm_drvdata {
> > > > +    void __iomem        *base;
> > > > +    struct device        *dev;
> > > > +    struct coresight_device    *csdev;
> > > > +    struct mutex        lock;
> > >
> > > Why mutex lock ? Couldn't this be a spinlock ?
> > 1. There is no irq for TPDM
> > 2. As there are 7 dataset types, there will be some FOR loop to configure
> > tpdm registers which may cause some time.

How long does it take to configure ? Is it too long enough to trigger
RCU stalls ? as long as we don't do any sleeping/blocking operations
we should be fine with a spinlock.

Suzuki

> >
> I think we can use mutex lock here. Do you have any more comments for 
> this ?

> Thanks
> Jinlong Mao
> > > > +    bool            enable;
> > > > +};
> > > > +
> > > > +#endif  /* _CORESIGHT_CORESIGHT_TPDM_H */
> > > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > > > index 247147c11231..a9efac55029d 100644
> > > > --- a/include/linux/coresight.h
> > > > +++ b/include/linux/coresight.h
> > > > @@ -61,6 +61,7 @@ enum coresight_dev_subtype_source {
> > > >       CORESIGHT_DEV_SUBTYPE_SOURCE_PROC,
> > > >       CORESIGHT_DEV_SUBTYPE_SOURCE_BUS,
> > > >       CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE,
> > > > +    CORESIGHT_DEV_SUBTYPE_SOURCE_DATA_ONLY,
> > >
> > > super minor nit: I find the choice of name a bit odd.
> > > We could simply make it something like :
> > >
> > >     CORESIGHT_DEV_SUBTYPE_SOURCE_OTHERS:
> > >
> > > Suzuki
> > I will check and update.
> > > >   };
> > > >     enum coresight_dev_subtype_helper {
> > >
> > > _______________________________________________
> > > CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx
> > > To unsubscribe send an email to coresight-leave@xxxxxxxxxxxxxxxx
> > _______________________________________________
> > CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx
> > To unsubscribe send an email to coresight-leave@xxxxxxxxxxxxxxxx