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

[Jinlong Mao]

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 at quicinc.com>
>>> Signed-off-by: Mao Jinlong <quic_jinlmao at quicinc.com>
>>> ---
>>>   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.
>
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 at lists.linaro.org
>> To unsubscribe send an email to coresight-leave at lists.linaro.org
> _______________________________________________
> CoreSight mailing list -- coresight at lists.linaro.org
> To unsubscribe send an email to coresight-leave at lists.linaro.org