Message-ID: <31a9a84e-c22d-446e-9082-12b74a1c82e0@quicinc.com>
Date: Fri, 17 Jan 2025 10:31:57 +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/16/2025 6:17 PM, James Clark wrote:
> 
> 
> On 16/01/2025 3:01 am, Jie Gan wrote:
>>
>>
>> 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;
> 
> What's the perf handle for? Seems like this change for the CTCU only 
> requires access to the trace_id which is added below.
In perf mode, the handle as the "sink_data" has been transmitted to 
coresight_enable_path function.

etm_event_start:
struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
struct perf_output_handle *handle = &ctxt->handle;

...

if (coresight_enable_path(cs_path, CS_MODE_PERF, handle))

...

Then, the handle as the "sink_data", it will be transmitted to 
coresight_enable_helpers and coresight_enable_sink.

As I mentioned before, I modified following codes in coresight_enable_path:
coresight_enable_helpers(csdev, mode, sink_data) ->
coresight_enable_helpers(csdev, mode, cs_path)

So, I need to consider how to correctly to handle the "handle" 
parameter! That's why I added it to the coresight_path, ensure the 
'handle' is correctly referenced wherever necessary.

> 
>>      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;
>>
> 
> Looks good.
> 
>> 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?
>>
> 
> Can it not be done the same way as Perf, at least for consistency? If 
> the enable helper function already gets access to the path, and the path 
> already has the trace ID, why is any search required?
Hi, James

For sysfs mode, We need to retrieve the trace_id once within the enable 
function, then cache it to coresight_path. (As discussed before, there 
is a special case of the source device is TPDM!)

Consider the CTCU is the helper device of the TMC ETR device, not all 
path need the trace_id(other sink, etf), so I put the retrieving logic 
in the CTCU driver(within CTCU enable).

For the disable function, we just use the cached trace_id in 
coresight_path to disable the trace_id filter functionality.

Or I was thinking another solution in coresight_build_path, We assume 
only the sysfs mode needs to retrieve the trace_id when construct path. 
So we can call source_ops(csdev)->trace_id once, hopefully the source 
device is not TPDM! If not, we will try to locate the TPDA device, then 
call link_ops(csdev)->trace_id to read the trace_id. And for perf mode, 
we still use the solution I mentioned before.

How do you think about it, which one do you prefer? Or any other proposal?

Thanks,
Jie

Powered by blists - more mailing lists