Message-ID: <5c0827ce-cf25-43d0-a160-5f99e82f582c@quicinc.com>
Date: Thu, 16 Jan 2025 11:01:03 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: James Clark <james.clark@...aro.org>
CC: Jinlong Mao <quic_jinlmao@...cinc.com>, <coresight@...ts.linaro.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        Tingwei Zhang <quic_tingweiz@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Mike Leach
	<mike.leach@...aro.org>,
        Alexander Shishkin
	<alexander.shishkin@...ux.intel.com>,
        Maxime Coquelin
	<mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the
 trace ID



On 1/15/2025 8:29 PM, James Clark wrote:
> 
> 
> On 15/01/2025 1:44 am, Jie Gan wrote:
>>
>>
>> On 1/14/2025 6:07 PM, James Clark wrote:
>>>
>>>
>>> On 14/01/2025 2:51 am, Jie Gan wrote:
>>>>
>>>>
>>>> On 1/13/2025 8:02 PM, James Clark wrote:
>>>>>
>>>>>
>>>>> On 26/12/2024 1:10 am, Jie Gan wrote:
>>>>>> Add 'trace_id' function pointer in ops. It's responsible for
>>>>>> retrieving the device's trace ID.
>>>>>>
>>>>>> Add 'struct cs_sink_data' to store the data that is needed by
>>>>>> coresight_enable_path/coresight_disable_path. The structure
>>>>>> will be transmitted to the helper and sink device to enable
>>>>>> related funcationalities.
>>>>>>
>>>>>
>>>>> The new cs_sink_data struct is quite specific to this change. Can 
>>>>> we start passing the path around to enable/disable functions, that 
>>>>> will allow devices to gather anything they want in the future. 
>>>>> Because we already have coresight_get_sink(path), 
>>>>> coresight_get_source(path) etc.
>>>>>
>>>>> And see below, but for this case we can also change the path struct 
>>>>> to contain the trace ID. Then all the new functions, allocations 
>>>>> and searches for the trace ID are unecessary. The CTCU will have 
>>>>> access to the path, and by the time its enable function is called 
>>>>> the trace ID is already assigned.
>>>>>
>>>>> It's also easier to understand at which point a trace ID is 
>>>>> allocated, rather than adding the trace_id() callbacks from 
>>>>> everywhere which could potentially either read or allocate. I 
>>>>> suppose that's "safer" because maybe it's not allocated, but I 
>>>>> can't see what case it would happen in reverse.
>>>>>
>>>> Thank you for comment. I will try this solution.
>>>> The biggest challenge for the patch is how to correctly read 
>>>> trace_id from source device and passthrough it to helper device as 
>>>> the source device always the last one to enable. I believe your 
>>>> proposed solution is better than mine and has minimal impact on the 
>>>> basic framework, but I think we still need read_trace in source_ops 
>>>> and link_ops. Then we can read the trace_id in coresight_build_path 
>>>> function and save it to the coresight_path to avoid redundant 
>>>> searching?
>>>>
>>>>
>>>>>> Signed-off-by: Jie Gan <quic_jiegan@...cinc.com>
>>>>>> ---
>>>>>>   drivers/hwtracing/coresight/coresight-core.c  | 59 +++++++++++++ 
>>>>>> + +----
>>>>>>   drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
>>>>>>   .../hwtracing/coresight/coresight-etm-perf.c  | 37 ++++++++++--
>>>>>>   .../coresight/coresight-etm3x-core.c          | 30 ++++++++++
>>>>>>   .../coresight/coresight-etm4x-core.c          | 29 +++++++++
>>>>>>   drivers/hwtracing/coresight/coresight-priv.h  | 13 +++-
>>>>>>   drivers/hwtracing/coresight/coresight-stm.c   | 22 +++++++
>>>>>>   drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++-
>>>>>>   .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
>>>>>>   .../hwtracing/coresight/coresight-tmc-etr.c   |  6 +-
>>>>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 20 +++++++
>>>>>>   drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
>>>>>>   drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
>>>>>>   include/linux/coresight.h                     |  6 ++
>>>>>>   14 files changed, 234 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/ 
>>>>>> drivers/ hwtracing/coresight/coresight-core.c
>>>>>> index 0a9380350fb5..2e560b425fd4 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>   #include "coresight-etm-perf.h"
>>>>>>   #include "coresight-priv.h"
>>>>>>   #include "coresight-syscfg.h"
>>>>>> +#include "coresight-trace-id.h"
>>>>>>   /*
>>>>>>    * Mutex used to lock all sysfs enable and disable actions and 
>>>>>> loading and
>>>>>> @@ -331,12 +332,12 @@ static int coresight_enable_helper(struct 
>>>>>> coresight_device *csdev,
>>>>>>       return helper_ops(csdev)->enable(csdev, mode, data);
>>>>>>   }
>>>>>> -static void coresight_disable_helper(struct coresight_device *csdev)
>>>>>> +static void coresight_disable_helper(struct coresight_device 
>>>>>> *csdev, void *data)
>>>>>>   {
>>>>>> -    helper_ops(csdev)->disable(csdev, NULL);
>>>>>> +    helper_ops(csdev)->disable(csdev, data);
>>>>>>   }
>>>>>> -static void coresight_disable_helpers(struct coresight_device 
>>>>>> *csdev)
>>>>>> +static void coresight_disable_helpers(struct coresight_device 
>>>>>> *csdev, void *data)
>>>>>>   {
>>>>>>       int i;
>>>>>>       struct coresight_device *helper;
>>>>>> @@ -344,7 +345,7 @@ static void coresight_disable_helpers(struct 
>>>>>> coresight_device *csdev)
>>>>>>       for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
>>>>>>           helper = csdev->pdata->out_conns[i]->dest_dev;
>>>>>>           if (helper && coresight_is_helper(helper))
>>>>>> -            coresight_disable_helper(helper);
>>>>>> +            coresight_disable_helper(helper, data);
>>>>>>       }
>>>>>>   }
>>>>>> @@ -361,7 +362,7 @@ static void coresight_disable_helpers(struct 
>>>>>> coresight_device *csdev)
>>>>>>   void coresight_disable_source(struct coresight_device *csdev, 
>>>>>> void *data)
>>>>>>   {
>>>>>>       source_ops(csdev)->disable(csdev, data);
>>>>>> -    coresight_disable_helpers(csdev);
>>>>>> +    coresight_disable_helpers(csdev, NULL);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL_GPL(coresight_disable_source);
>>>>>> @@ -371,7 +372,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
>>>>>>    * disabled.
>>>>>>    */
>>>>>>   static void coresight_disable_path_from(struct list_head *path,
>>>>>> -                    struct coresight_node *nd)
>>>>>> +                    struct coresight_node *nd,
>>>>>> +                    void *sink_data)
>>>>>>   {
>>>>>>       u32 type;
>>>>>>       struct coresight_device *csdev, *parent, *child;
>>>>>> @@ -417,13 +419,13 @@ static void 
>>>>>> coresight_disable_path_from(struct list_head *path,
>>>>>>           }
>>>>>>           /* Disable all helpers adjacent along the path last */
>>>>>> -        coresight_disable_helpers(csdev);
>>>>>> +        coresight_disable_helpers(csdev, sink_data);
>>>>>>       }
>>>>>>   }
>>>>>> -void coresight_disable_path(struct list_head *path)
>>>>>> +void coresight_disable_path(struct list_head *path, void *sink_data)
>>>>>>   {
>>>>>> -    coresight_disable_path_from(path, NULL);
>>>>>> +    coresight_disable_path_from(path, NULL, sink_data);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL_GPL(coresight_disable_path);
>>>>>> @@ -505,10 +507,47 @@ int coresight_enable_path(struct list_head 
>>>>>> *path, enum cs_mode mode,
>>>>>>   out:
>>>>>>       return ret;
>>>>>>   err:
>>>>>> -    coresight_disable_path_from(path, nd);
>>>>>> +    coresight_disable_path_from(path, nd, sink_data);
>>>>>>       goto out;
>>>>>>   }
>>>>>> +int coresight_read_traceid(struct list_head *path, enum cs_mode 
>>>>>> mode,
>>>>>> +               struct coresight_trace_id_map *id_map)
>>>>>> +{
>>>>>> +    int trace_id, type;
>>>>>> +    struct coresight_device *csdev;
>>>>>> +    struct coresight_node *nd;
>>>>>> +
>>>>>> +    list_for_each_entry(nd, path, link) {
>>>>>
>>>>> What do you think about also changing the path to this:
>>>>>
>>>>>   struct coresight_path {
>>>>>     struct list_head *path,
>>>>>     u8 trace_id
>>>>>   };
>>>>>
>>>> That's better, I can simplify the coresight_read_traceid function 
>>>> without traverse the path.
>>>>
>>>> But we still need to check the type of the coresight device, because 
>>>> the TPDM does not have traceid and we use the trace_id from the TPDA 
>>>> device that the TPDM connected. That's why I added trace_id to 
>>>> link_ops.
>>>>
>>>
>>> But if any device that allocates a trace ID saves it into the path, 
>>> then as long as any other device that needs the ID is enabled after 
>>> that it just reads it from the path directly. Assuming we pass the 
>>> path to every enable and disable function.
>>>
>>> We wouldn't need coresight_read_traceid() if it always happens that 
>>> way around, which I think it currently does?
>>>
>> I got your point here. You are right. If we passed path to the helper 
>> device, just use coresight_get_source to obtain the source device, 
>> then call the source_ops->trace_id to obtain the trace_id. So we 
>> definitely dont need a standalone function, coresight_read_traceid().
>>
>> Besides, I still need a function to retrive the trace_id of the TPDA 
>> device if the source device is TPDM, right?
>>
>>
>> Thanks,
>> Jie
>>
> 
> Yes, and that would require a search as the TPDA not always at one end 
> of the path like coresight_get_source() and coresight_get_sink(). Which 
> is why I was thinking it might be good to save the trace ID in the path 
> struct to avoid it.
> 
As you proposed, I created coresight_path structure as below:
struct coresight_path {
	struct perf_output_handle       *handle;
	struct list_head                *path;
	u8				trace_id;
};

In coresight_enable_path, I modified the parameters that transmitted to 
helper device:
struct coresight_path *cs_path;

coresight_enable_helpers(csdev, mode, sink_data) ->
coresight_enable_helpers(csdev, mode, cs_path)

The cs_path will be constructed and initialized in coresight_build_path 
function.

For perf mode, the trace_id is collected within etm_setup_aux and stored 
in cs_path->trace_id to avoid extra cost of retrieving the trace_id;

For sysfs mode, I let the CTCU device to retrieving the trace_id with 
path. The TPDA device is located close to the TPDM, making the cost of 
searching for the TPDA device acceptable.

The cs_path will be stored in the same manner as the previous path.

How do you think about this solution?

Thanks,
Jie

[...]

Powered by blists - more mailing lists