[PATCH v9 5/6] Coresight: Add Coresight TMC Control Unit driver
Jie Gan
quic_jiegan at quicinc.com
Tue Jan 28 16:46:44 PST 2025
On 1/28/2025 7:55 PM, James Clark wrote:
>
>
> On 24/01/2025 7:25 am, Jie Gan wrote:
>> The Coresight TMC Control Unit hosts miscellaneous configuration
>> registers
>> which control various features related to TMC ETR sink.
>>
>> Based on the trace ID, which is programmed in the related CTCU ATID
>> register of a specific ETR, trace data with that trace ID gets into
>> the ETR buffer, while other trace data gets dropped.
>>
>> Enabling source device sets one bit of the ATID register based on
>> source device's trace ID.
>> Disabling source device resets the bit according to the source
>> device's trace ID.
>>
>> Signed-off-by: Jie Gan <quic_jiegan at quicinc.com>
>> ---
>> drivers/hwtracing/coresight/Kconfig | 12 +
>> drivers/hwtracing/coresight/Makefile | 1 +
>> drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++++++
>> drivers/hwtracing/coresight/coresight-ctcu.h | 30 ++
>> include/linux/coresight.h | 3 +-
>> 5 files changed, 321 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>> create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
> >
>
> [...]
>
>> +/*
>> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
>> + *
>> + * Returns 0 indicates success. None-zero result means failure.
>> + */
>> +static int ctcu_set_etr_traceid(struct coresight_device *csdev,
>> struct coresight_path *cs_path,
>> + bool enable)
>> +{
>> + struct coresight_device *sink = coresight_get_sink(cs_path->path);
>> + struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> + u8 trace_id = cs_path->trace_id;
>> + int port_num;
>> +
>> + if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) ||
>> IS_ERR_OR_NULL(drvdata)) {
>> + dev_err(&csdev->dev, "Invalid parameters\n");
>> + return -EINVAL;
>> + }
>> +
>> + port_num = ctcu_get_active_port(sink, csdev);
>> + if (port_num < 0)
>> + return -EINVAL;
>> +
>> + /*
>> + * Skip the disable session if more than one TPDM device that
>> + * connected to the same TPDA device has been enabled.
>> + */
>> + if (enable)
>> + atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
>> + else {
>> + if (atomic_dec_return(&drvdata->traceid_refcnt[port_num]
>> [trace_id]) > 0) {
>> + dev_dbg(&csdev->dev, "Skip the disable session\n");
>> + return 0;
>> + }
>> + }
>> +
>> + dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
>> +
>> + return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
>
> Hi Jie,
>
> Using atomic_dec_return() here doesn't prevent __ctcu_set_etr_traceid()
> from running concurrent enable and disables. Once you pass the
> atomic_dec_return() a second call to enable it will mess it up.
>
> I think you need a spinlock around the whole thing and then the
> refcounts don't need to be atomics.
>
Hi, James
Thanks for comment. I may not fully tested my codes here. What I was
thinking is there's no way the refcnt could become a negative number
under current framework. So I just added spinlock in
__ctcu_set_etr_traceid() to ensure concurrent sessions correctly
manipulate the register.
As the trace_id related to the bit of the ATID register, I think the
concurrent processes are working fine with spinlock around read/write
register.
I may not fully got your point here. Please help me to correct it.
Thanks,
Jie
More information about the linux-arm-kernel
mailing list