[PATCH v9 5/6] Coresight: Add Coresight TMC Control Unit driver

[Jie Gan]

On 1/29/2025 6:35 PM, James Clark wrote:
> 
> 
> On 29/01/2025 12:46 am, Jie Gan wrote:
>>
>>
>> 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
>>
>>
> 
> No it can't become negative, but the refcount can be a different state 
> to the one that was actually written:
> 
> 
>    CPU0                             CPU1
>    ----                             ----
>    ctcu_set_etr_traceid(enable)
>                                     ctcu_set_etr_traceid(disable)
>    atomic_inc()
>    recount == 1
>                                     atomic_dec()
>                                     recount == 0
> 
>                                     __ctcu_set_etr_traceid(disable)
>                                     Lock and write disable state to
>                                     device
> 
>    __ctcu_set_etr_traceid(enable)
>    Lock and write enable state to
>    device
> 
> 
> As you can see this leaves the device in an enabled state but the 
> refcount is 0.
Yes, you are right. I didnt consider this scenario. We definitely need 
spinlock here.

> 
> This is also quite large if you use atomic types:
> 
>   /* refcnt for each traceid of each sink */
>   atomic_t traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
> 
> Presumably you can't have the refcount for each ID be higher than the 
> max number of TPDMs connected? If you make the locked area a bit wider 
> you don't need atomic types and also solve the above problem. So you 
> could do u8, or DECLARE_BITMAP() and bitmap_read() etc to read 3 bit 
> values. Or however wide it needs to be.
The original purpose of using atomic here is trying to narrow the locked 
area.

I think u8 is ok here.
u8 traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP] will cost 
224 bytes, I think it's acceptable here.

Thanks,
Jie

>