Message-ID: <49f6619d-f042-42b9-9359-b7d78783a32e@linaro.org>
Date: Thu, 16 Jan 2025 10:17:30 +0000
From: James Clark <james.clark@...aro.org>
To: Jie Gan <quic_jiegan@...cinc.com>
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 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.

>      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?

> Thanks,
> Jie
> 
> [...]
> 

Powered by blists - more mailing lists