[PATCH v3 0/3] source filtering for multi-port output

[PATCH v3 0/3] source filtering for multi-port output

[Tao Zhang]

In our hardware design, by combining a funnel and a replicator, it
implement a hardware device with one-to-one correspondence between
output ports and input ports. The programming usage on this device
is the same as funnel. The software uses a funnel and a static
replicator to implement the driver of this device. Since original
funnels only support a single output connection and original
replicator only support a single input connection, the code needs
to be modified to support this new feature. The following is a
typical topology diagram of multi-port output mechanism.
|----------|     |---------|     |----------|   |---------|
|  TPDM 0  |     | Source0 |     | Source 1 |   | TPDM 1  |
|----------|     |---------|     |----------|   |---------|
      |                |                |             |
      |                |                |             |
      |      --------- |                |             |
      |      |                          |             |
      |      |                          |             |
      |      |                          |             |
   \-------------/ ----------------------             |
    \  Funnel 0 /  |                                  |
     -----------   |     ------------------------------
          |        |     |
          |        |     |
        \------------------/
         \    Funnel 1    /     ----|
          \--------------/          |
                  |                 |----> Combine a funnel and a
                  |                 |      static replicator
          /-----------------\       |
         /    replicator 0   \  ----|
        /---------------------\
             |     |      |
             |     |      |-----------|
             |     |---------|        |
             |               |TPDM0   |TPDM1
             |            \-----------------/
             |             \   TPDA 0      /
             |              \-------------/
             |                    |
             |                    |
             |Source0/1           |
          \-------------------------------/
           \           Funnel 2          /
            \---------------------------/


Changes in V3:
1. Rename the function "coresight_source_filter" to
"coresight_block_source". And refine this function.
-- Suzuki K Poulose
2. Rename the parameters of the function
"coresight_find_out_connection" to avoid confusion.
-- Suzuki K Poulose
3. Get the source of path in "coresight_enable_path" and
"coresight_disable_path".
-- Suzuki K Poulose
4. Fix filter source device before skip the port in
"coresight_orphan_match".
-- Suzuki K Poulose
5. Make sure the device still orphan if whter is a filter
source firmware node but the filter source device is null.
-- Suzuki K Poulose
6. Walk through the entire coresight bus and fixup the
"filter_src_dev" if the source is being removed.
-- Suzuki K Poulose
7. Refine the commit description of patch#2.
-- Suzuki K Poulose
8. Call "fwnode_handle_put" to "drop" the refcount for the
device firmware node.
-- Suzuki K Poulose
9. Fix the warning reported by kernel test robot.
-- kernel test robot.
10. Use the source device directly if the port has a
hardcoded filter in "tpda_get_element_size".
-- Suzuki K Poulose

Changes in V2:
1. Change the reference for endpoint property in dt-binding.
-- Krzysztof Kozlowski
2. Change the property name "filter_src" to "filter-src".
-- Krzysztof Kozlowski
3. Fix the errors in running 'make dt_binding_check'.
-- Rob Herring
4. Pass in the source parameter instead of path.
-- Suzuki K Poulose
5. Reset the "filter_src_dev" if the "src" csdev is being removed.
-- Suzuki K Poulose
6. Add a warning if the "filter_src_dev" is of not the
type DEV_TYPE_SOURCE.
-- Suzuki K Poulose
7. Optimize the procedure for handling all possible cases.
-- Suzuki K Poulose

Changes in V1:
1. Add a static replicator connect to a funnel to implement the
correspondence between the output ports and the input ports on
funnels.
-- Suzuki K Poulose
2. Add filter_src_dev and filter_src_dev phandle to
"coresight_connection" struct, and populate them if there is one.
-- Suzuki K Poulose
3. To look at the phandle and then fixup/remove the filter_src
device in fixup/remove connections.
-- Suzuki K Poulose
4. When TPDA reads DSB/CMB element size, it is implemented by
looking up filter src device in the connections.
-- Suzuki K Poulose

Tao Zhang (3):
  dt-bindings: arm: qcom,coresight-static-replicator: Add property for
    source filtering
  coresight: Add support for trace filtering by source
  coresight-tpda: Optimize the function of reading element size

 .../arm/arm,coresight-static-replicator.yaml  |  19 ++-
 drivers/hwtracing/coresight/coresight-core.c  | 136 +++++++++++++++---
 .../hwtracing/coresight/coresight-platform.c  |  18 +++
 drivers/hwtracing/coresight/coresight-tpda.c  |  11 +-
 include/linux/coresight.h                     |   5 +
 5 files changed, 164 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Tao Zhang]

The is some "magic" hard coded filtering in the replicators,
which only passes through trace from a particular "source". Add
a new property "filter-src" to label a phandle to the coresight
trace source device matching the hard coded filtering for the port.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 .../arm/arm,coresight-static-replicator.yaml  | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml
index 1892a091ac35..0d258c79eb94 100644
--- a/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml
+++ b/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml
@@ -45,7 +45,22 @@ properties:
     patternProperties:
       '^port@[01]$':
         description: Output connections to CoreSight Trace bus
-        $ref: /schemas/graph.yaml#/properties/port
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+
+        properties:
+          endpoint:
+            $ref: /schemas/graph.yaml#/$defs/endpoint-base
+            unevaluatedProperties: false
+
+            properties:
+              filter-src:
+                $ref: /schemas/types.yaml#/definitions/phandle
+                description:
+                  phandle to the coresight trace source device matching the
+                  hard coded filtering for this port
+
+              remote-endpoint: true
 
 required:
   - compatible
@@ -72,6 +87,7 @@ examples:
                 reg = <0>;
                 replicator_out_port0: endpoint {
                     remote-endpoint = <&etb_in_port>;
+                    filter-src = <&tpdm_video>;
                 };
             };
 
@@ -79,6 +95,7 @@ examples:
                 reg = <1>;
                 replicator_out_port1: endpoint {
                     remote-endpoint = <&tpiu_in_port>;
+                    filter-src = <&tpdm_mdss>;
                 };
             };
         };
-- 
2.17.1

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Suzuki K Poulose]

On 21/08/2024 04:13, Tao Zhang wrote:
> The is some "magic" hard coded filtering in the replicators,
> which only passes through trace from a particular "source". Add
> a new property "filter-src" to label a phandle to the coresight
> trace source device matching the hard coded filtering for the port.

Minor nit: Please do not use abbreviate "source" in the bindings.
I am not an expert on other changes below and will leave it to
Rob/Krzysztof to comment.

Rob, Krzysztof,

We need someway to "link" (add a phandle) from a "port". The patch below
is extending "standard" port to add a phandle. Please let us know if
there is a better way.

e.g.:

filters = list of tuples of port, phandle. ?

e.g.:

filters = < 0, <&tpdm_video>,
             1, <&tpdm_mdss>
	   >


Thanks

Suzuki


> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   .../arm/arm,coresight-static-replicator.yaml  | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml b/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml
> index 1892a091ac35..0d258c79eb94 100644
> --- a/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml
> +++ b/Documentation/devicetree/bindings/arm/arm,coresight-static-replicator.yaml
> @@ -45,7 +45,22 @@ properties:
>       patternProperties:
>         '^port@[01]$':
>           description: Output connections to CoreSight Trace bus
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +            unevaluatedProperties: false
> +
> +            properties:
> +              filter-src:
> +                $ref: /schemas/types.yaml#/definitions/phandle
> +                description:
> +                  phandle to the coresight trace source device matching the
> +                  hard coded filtering for this port
> +
> +              remote-endpoint: true
>   
>   required:
>     - compatible
> @@ -72,6 +87,7 @@ examples:
>                   reg = <0>;
>                   replicator_out_port0: endpoint {
>                       remote-endpoint = <&etb_in_port>;
> +                    filter-src = <&tpdm_video>;
>                   };
>               };
>   
> @@ -79,6 +95,7 @@ examples:
>                   reg = <1>;
>                   replicator_out_port1: endpoint {
>                       remote-endpoint = <&tpiu_in_port>;
> +                    filter-src = <&tpdm_mdss>;
>                   };
>               };
>           };

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Suzuki K Poulose]

On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>> On 21/08/2024 04:13, Tao Zhang wrote:
>>> The is some "magic" hard coded filtering in the replicators,
>>> which only passes through trace from a particular "source". Add
>>> a new property "filter-src" to label a phandle to the coresight
>>> trace source device matching the hard coded filtering for the port.
>>
>> Minor nit: Please do not use abbreviate "source" in the bindings.
>> I am not an expert on other changes below and will leave it to
>> Rob/Krzysztof to comment.
>>
>> Rob, Krzysztof,
>>
>> We need someway to "link" (add a phandle) from a "port". The patch below
>> is extending "standard" port to add a phandle. Please let us know if
>> there is a better way.
>>
>> e.g.:
>>
>> filters = list of tuples of port, phandle. ?
>>
>> e.g.:
>>
>> filters = < 0, <&tpdm_video>,
>>              1, <&tpdm_mdss>
>> 	   >
>>
> 
> Current solution feels like band-aid - what if next time you need some
> second filter? Or "wall"? Or whatever? Next property?



> 
> Isn't filter just one endpoint in the graph?
> 
> A <--> filter <--> B

To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
multi output ports).

For clearer example:

A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1) <--> B1
A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2) <--> B2
A2 <--> .. <--> ../                  p2  \ --> Unfiltered        <--> B0



> Instead of
> 
> A <----through-filter----> B?

The problem is we need to know the components in the path from A0 to X
through, (Not just A0 and L). And also we need to know "which port (p0 
vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out of the
link "L".

So ideally, we need a way to tie p0 -> A1, p1 -> A2.

would we need something else in the future ? I don't know for sure.
People could design their own things ;-). But this was the first time
ever in the last 12yrs since we supported coresight in the kernel.
(there is always a first time).

Fundamentally, the "ports" cannot have additional properties today.
Not sure if there are other usecases (I don't see why). So, we have
to manually extend like above, which I think is not nice.

Happy to proceed with anything that seems acceptable for you folks.

Suzuki



> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Suzuki K Poulose]

On 22/08/2024 11:34, Suzuki K Poulose wrote:
> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>> The is some "magic" hard coded filtering in the replicators,
>>>> which only passes through trace from a particular "source". Add
>>>> a new property "filter-src" to label a phandle to the coresight
>>>> trace source device matching the hard coded filtering for the port.
>>>
>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>> I am not an expert on other changes below and will leave it to
>>> Rob/Krzysztof to comment.
>>>
>>> Rob, Krzysztof,
>>>
>>> We need someway to "link" (add a phandle) from a "port". The patch below
>>> is extending "standard" port to add a phandle. Please let us know if
>>> there is a better way.
>>>
>>> e.g.:
>>>
>>> filters = list of tuples of port, phandle. ?
>>>
>>> e.g.:
>>>
>>> filters = < 0, <&tpdm_video>,
>>>              1, <&tpdm_mdss>
>>>        >
>>>
>>
>> Current solution feels like band-aid - what if next time you need some
>> second filter? Or "wall"? Or whatever? Next property?
> 
> 
> 
>>
>> Isn't filter just one endpoint in the graph?
>>
>> A <--> filter <--> B
> 
> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
> multi output ports).
> 
> For clearer example:
> 
> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1) <--> B1
> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2) <--> B2
> A2 <--> .. <--> ../                  p2  \ --> Unfiltered        <--> B0
> 
> 
> 
>> Instead of
>>
>> A <----through-filter----> B?
> 
> The problem is we need to know the components in the path from A0 to X
> through, (Not just A0 and L). And also we need to know "which port (p0 
> vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out of the
> link "L".
> 
> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
> 
> would we need something else in the future ? I don't know for sure.
> People could design their own things ;-). But this was the first time
> ever in the last 12yrs since we supported coresight in the kernel.
> (there is always a first time).
> 
> Fundamentally, the "ports" cannot have additional properties today.
> Not sure if there are other usecases (I don't see why). So, we have
> to manually extend like above, which I think is not nice.

Replying to the other thread [0], made me realize that the above is not
true. Indeed it is possible to add properties for endpoints, e.g:

e.g.: media/video-interfaces.yaml

So extending the endpoint node is indeed acceptable (unlike I thought).
May be the we it is achieved in this patch is making it look otherwise.

Suzuki
[0] https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com



> 
> Happy to proceed with anything that seems acceptable for you folks.
> 
> Suzuki
> 
> 
> 
>>
>> Best regards,
>> Krzysztof
>>
> 

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Suzuki K Poulose]

Krzysztof

On 22/08/2024 12:50, Suzuki K Poulose wrote:
> On 22/08/2024 11:34, Suzuki K Poulose wrote:
>> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>>> The is some "magic" hard coded filtering in the replicators,
>>>>> which only passes through trace from a particular "source". Add
>>>>> a new property "filter-src" to label a phandle to the coresight
>>>>> trace source device matching the hard coded filtering for the port.
>>>>
>>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>>> I am not an expert on other changes below and will leave it to
>>>> Rob/Krzysztof to comment.
>>>>
>>>> Rob, Krzysztof,
>>>>
>>>> We need someway to "link" (add a phandle) from a "port". The patch 
>>>> below
>>>> is extending "standard" port to add a phandle. Please let us know if
>>>> there is a better way.
>>>>
>>>> e.g.:
>>>>
>>>> filters = list of tuples of port, phandle. ?
>>>>
>>>> e.g.:
>>>>
>>>> filters = < 0, <&tpdm_video>,
>>>>              1, <&tpdm_mdss>
>>>>        >
>>>>
>>>
>>> Current solution feels like band-aid - what if next time you need some
>>> second filter? Or "wall"? Or whatever? Next property?
>>
>>
>>
>>>
>>> Isn't filter just one endpoint in the graph?
>>>
>>> A <--> filter <--> B
>>
>> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
>> multi output ports).
>>
>> For clearer example:
>>
>> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1) <--> B1
>> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2) <--> B2
>> A2 <--> .. <--> ../                  p2  \ --> Unfiltered        <--> B0
>>
>>
>>
>>> Instead of
>>>
>>> A <----through-filter----> B?
>>
>> The problem is we need to know the components in the path from A0 to X
>> through, (Not just A0 and L). And also we need to know "which port (p0 
>> vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out of the
>> link "L".
>>
>> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
>>
>> would we need something else in the future ? I don't know for sure.
>> People could design their own things ;-). But this was the first time
>> ever in the last 12yrs since we supported coresight in the kernel.
>> (there is always a first time).
>>
>> Fundamentally, the "ports" cannot have additional properties today.
>> Not sure if there are other usecases (I don't see why). So, we have
>> to manually extend like above, which I think is not nice.
> 
> Replying to the other thread [0], made me realize that the above is not
> true. Indeed it is possible to add properties for endpoints, e.g:
> 
> e.g.: media/video-interfaces.yaml
> 
> So extending the endpoint node is indeed acceptable (unlike I thought).
> May be the we it is achieved in this patch is making it look otherwise.
> 
> Suzuki
> [0] https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com

Please could you let us know if it is acceptable to extend "endpoint"
node to have an optional property ?

Suzuki


> 
> 
> 
>>
>> Happy to proceed with anything that seems acceptable for you folks.
>>
>> Suzuki
>>
>>
>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
> 

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Tao Zhang]

On 10/9/2024 6:52 PM, Suzuki K Poulose wrote:
> Krzysztof
>
> On 22/08/2024 12:50, Suzuki K Poulose wrote:
>> On 22/08/2024 11:34, Suzuki K Poulose wrote:
>>> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>>>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>>>> The is some "magic" hard coded filtering in the replicators,
>>>>>> which only passes through trace from a particular "source". Add
>>>>>> a new property "filter-src" to label a phandle to the coresight
>>>>>> trace source device matching the hard coded filtering for the port.
>>>>>
>>>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>>>> I am not an expert on other changes below and will leave it to
>>>>> Rob/Krzysztof to comment.
>>>>>
>>>>> Rob, Krzysztof,
>>>>>
>>>>> We need someway to "link" (add a phandle) from a "port". The patch 
>>>>> below
>>>>> is extending "standard" port to add a phandle. Please let us know if
>>>>> there is a better way.
>>>>>
>>>>> e.g.:
>>>>>
>>>>> filters = list of tuples of port, phandle. ?
>>>>>
>>>>> e.g.:
>>>>>
>>>>> filters = < 0, <&tpdm_video>,
>>>>>              1, <&tpdm_mdss>
>>>>>        >
>>>>>
>>>>
>>>> Current solution feels like band-aid - what if next time you need some
>>>> second filter? Or "wall"? Or whatever? Next property?
>>>
>>>
>>>
>>>>
>>>> Isn't filter just one endpoint in the graph?
>>>>
>>>> A <--> filter <--> B
>>>
>>> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
>>> multi output ports).
>>>
>>> For clearer example:
>>>
>>> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1) 
>>> <--> B1
>>> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2) 
>>> <--> B2
>>> A2 <--> .. <--> ../                  p2  \ --> Unfiltered        
>>> <--> B0
>>>
>>>
>>>
>>>> Instead of
>>>>
>>>> A <----through-filter----> B?
>>>
>>> The problem is we need to know the components in the path from A0 to X
>>> through, (Not just A0 and L). And also we need to know "which port 
>>> (p0 vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out 
>>> of the
>>> link "L".
>>>
>>> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
>>>
>>> would we need something else in the future ? I don't know for sure.
>>> People could design their own things ;-). But this was the first time
>>> ever in the last 12yrs since we supported coresight in the kernel.
>>> (there is always a first time).
>>>
>>> Fundamentally, the "ports" cannot have additional properties today.
>>> Not sure if there are other usecases (I don't see why). So, we have
>>> to manually extend like above, which I think is not nice.
>>
>> Replying to the other thread [0], made me realize that the above is not
>> true. Indeed it is possible to add properties for endpoints, e.g:
>>
>> e.g.: media/video-interfaces.yaml
>>
>> So extending the endpoint node is indeed acceptable (unlike I thought).
>> May be the we it is achieved in this patch is making it look otherwise.
>>
>> Suzuki
>> [0] 
>> https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com
>
> Please could you let us know if it is acceptable to extend "endpoint"
> node to have an optional property ?

Hi Krzysztof,


Kindly reminder, could you help comment on this?


Best,

Tao

>
> Suzuki
>
>
>>
>>
>>
>>>
>>> Happy to proceed with anything that seems acceptable for you folks.
>>>
>>> Suzuki
>>>
>>>
>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Krzysztof Kozlowski]

On 17/10/2024 09:23, Tao Zhang wrote:
> 
> On 10/9/2024 6:52 PM, Suzuki K Poulose wrote:
>> Krzysztof
>>
>> On 22/08/2024 12:50, Suzuki K Poulose wrote:
>>> On 22/08/2024 11:34, Suzuki K Poulose wrote:
>>>> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>>>>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>>>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>>>>> The is some "magic" hard coded filtering in the replicators,
>>>>>>> which only passes through trace from a particular "source". Add
>>>>>>> a new property "filter-src" to label a phandle to the coresight
>>>>>>> trace source device matching the hard coded filtering for the port.
>>>>>>
>>>>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>>>>> I am not an expert on other changes below and will leave it to
>>>>>> Rob/Krzysztof to comment.
>>>>>>
>>>>>> Rob, Krzysztof,
>>>>>>
>>>>>> We need someway to "link" (add a phandle) from a "port". The patch 
>>>>>> below
>>>>>> is extending "standard" port to add a phandle. Please let us know if
>>>>>> there is a better way.
>>>>>>
>>>>>> e.g.:
>>>>>>
>>>>>> filters = list of tuples of port, phandle. ?
>>>>>>
>>>>>> e.g.:
>>>>>>
>>>>>> filters = < 0, <&tpdm_video>,
>>>>>>              1, <&tpdm_mdss>
>>>>>>        >
>>>>>>
>>>>>
>>>>> Current solution feels like band-aid - what if next time you need some
>>>>> second filter? Or "wall"? Or whatever? Next property?
>>>>
>>>>
>>>>
>>>>>
>>>>> Isn't filter just one endpoint in the graph?
>>>>>
>>>>> A <--> filter <--> B
>>>>
>>>> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
>>>> multi output ports).
>>>>
>>>> For clearer example:
>>>>
>>>> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1) 
>>>> <--> B1
>>>> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2) 
>>>> <--> B2
>>>> A2 <--> .. <--> ../                  p2  \ --> Unfiltered        
>>>> <--> B0
>>>>
>>>>
>>>>
>>>>> Instead of
>>>>>
>>>>> A <----through-filter----> B?
>>>>
>>>> The problem is we need to know the components in the path from A0 to X
>>>> through, (Not just A0 and L). And also we need to know "which port 
>>>> (p0 vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out 
>>>> of the
>>>> link "L".
>>>>
>>>> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
>>>>
>>>> would we need something else in the future ? I don't know for sure.
>>>> People could design their own things ;-). But this was the first time
>>>> ever in the last 12yrs since we supported coresight in the kernel.
>>>> (there is always a first time).
>>>>
>>>> Fundamentally, the "ports" cannot have additional properties today.
>>>> Not sure if there are other usecases (I don't see why). So, we have
>>>> to manually extend like above, which I think is not nice.
>>>
>>> Replying to the other thread [0], made me realize that the above is not
>>> true. Indeed it is possible to add properties for endpoints, e.g:
>>>
>>> e.g.: media/video-interfaces.yaml
>>>
>>> So extending the endpoint node is indeed acceptable (unlike I thought).
>>> May be the we it is achieved in this patch is making it look otherwise.
>>>
>>> Suzuki
>>> [0] 
>>> https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com
>>
>> Please could you let us know if it is acceptable to extend "endpoint"
>> node to have an optional property ?
> 
> Hi Krzysztof,
> 
> 
> Kindly reminder, could you help comment on this?

I don't have any smart ideas and with earlier explanation sounds ok.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Suzuki K Poulose]

On 18/10/2024 11:05, Krzysztof Kozlowski wrote:
> On 17/10/2024 09:23, Tao Zhang wrote:
>>
>> On 10/9/2024 6:52 PM, Suzuki K Poulose wrote:
>>> Krzysztof
>>>
>>> On 22/08/2024 12:50, Suzuki K Poulose wrote:
>>>> On 22/08/2024 11:34, Suzuki K Poulose wrote:
>>>>> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>>>>>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>>>>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>>>>>> The is some "magic" hard coded filtering in the replicators,
>>>>>>>> which only passes through trace from a particular "source". Add
>>>>>>>> a new property "filter-src" to label a phandle to the coresight
>>>>>>>> trace source device matching the hard coded filtering for the port.
>>>>>>>
>>>>>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>>>>>> I am not an expert on other changes below and will leave it to
>>>>>>> Rob/Krzysztof to comment.
>>>>>>>
>>>>>>> Rob, Krzysztof,
>>>>>>>
>>>>>>> We need someway to "link" (add a phandle) from a "port". The patch
>>>>>>> below
>>>>>>> is extending "standard" port to add a phandle. Please let us know if
>>>>>>> there is a better way.
>>>>>>>
>>>>>>> e.g.:
>>>>>>>
>>>>>>> filters = list of tuples of port, phandle. ?
>>>>>>>
>>>>>>> e.g.:
>>>>>>>
>>>>>>> filters = < 0, <&tpdm_video>,
>>>>>>>               1, <&tpdm_mdss>
>>>>>>>         >
>>>>>>>
>>>>>>
>>>>>> Current solution feels like band-aid - what if next time you need some
>>>>>> second filter? Or "wall"? Or whatever? Next property?
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Isn't filter just one endpoint in the graph?
>>>>>>
>>>>>> A <--> filter <--> B
>>>>>
>>>>> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
>>>>> multi output ports).
>>>>>
>>>>> For clearer example:
>>>>>
>>>>> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1)
>>>>> <--> B1
>>>>> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2)
>>>>> <--> B2
>>>>> A2 <--> .. <--> ../                  p2  \ --> Unfiltered
>>>>> <--> B0
>>>>>
>>>>>
>>>>>
>>>>>> Instead of
>>>>>>
>>>>>> A <----through-filter----> B?
>>>>>
>>>>> The problem is we need to know the components in the path from A0 to X
>>>>> through, (Not just A0 and L). And also we need to know "which port
>>>>> (p0 vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out
>>>>> of the
>>>>> link "L".
>>>>>
>>>>> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
>>>>>
>>>>> would we need something else in the future ? I don't know for sure.
>>>>> People could design their own things ;-). But this was the first time
>>>>> ever in the last 12yrs since we supported coresight in the kernel.
>>>>> (there is always a first time).
>>>>>
>>>>> Fundamentally, the "ports" cannot have additional properties today.
>>>>> Not sure if there are other usecases (I don't see why). So, we have
>>>>> to manually extend like above, which I think is not nice.
>>>>
>>>> Replying to the other thread [0], made me realize that the above is not
>>>> true. Indeed it is possible to add properties for endpoints, e.g:
>>>>
>>>> e.g.: media/video-interfaces.yaml
>>>>
>>>> So extending the endpoint node is indeed acceptable (unlike I thought).
>>>> May be the we it is achieved in this patch is making it look otherwise.
>>>>
>>>> Suzuki
>>>> [0]
>>>> https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com
>>>
>>> Please could you let us know if it is acceptable to extend "endpoint"
>>> node to have an optional property ?
>>
>> Hi Krzysztof,
>>
>>
>> Kindly reminder, could you help comment on this?
> 
> I don't have any smart ideas and with earlier explanation sounds ok.

Just to confirm, are you OK with adding a property to the "endpoint"
node that will indicate a phandle that the device allows on this
endpoint ?

Kind regards
Suzuki


> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Krzysztof Kozlowski]

On 18/10/2024 12:08, Suzuki K Poulose wrote:
> On 18/10/2024 11:05, Krzysztof Kozlowski wrote:
>> On 17/10/2024 09:23, Tao Zhang wrote:
>>>
>>> On 10/9/2024 6:52 PM, Suzuki K Poulose wrote:
>>>> Krzysztof
>>>>
>>>> On 22/08/2024 12:50, Suzuki K Poulose wrote:
>>>>> On 22/08/2024 11:34, Suzuki K Poulose wrote:
>>>>>> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>>>>>>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>>>>>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>>>>>>> The is some "magic" hard coded filtering in the replicators,
>>>>>>>>> which only passes through trace from a particular "source". Add
>>>>>>>>> a new property "filter-src" to label a phandle to the coresight
>>>>>>>>> trace source device matching the hard coded filtering for the port.
>>>>>>>>
>>>>>>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>>>>>>> I am not an expert on other changes below and will leave it to
>>>>>>>> Rob/Krzysztof to comment.
>>>>>>>>
>>>>>>>> Rob, Krzysztof,
>>>>>>>>
>>>>>>>> We need someway to "link" (add a phandle) from a "port". The patch
>>>>>>>> below
>>>>>>>> is extending "standard" port to add a phandle. Please let us know if
>>>>>>>> there is a better way.
>>>>>>>>
>>>>>>>> e.g.:
>>>>>>>>
>>>>>>>> filters = list of tuples of port, phandle. ?
>>>>>>>>
>>>>>>>> e.g.:
>>>>>>>>
>>>>>>>> filters = < 0, <&tpdm_video>,
>>>>>>>>               1, <&tpdm_mdss>
>>>>>>>>         >
>>>>>>>>
>>>>>>>
>>>>>>> Current solution feels like band-aid - what if next time you need some
>>>>>>> second filter? Or "wall"? Or whatever? Next property?
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Isn't filter just one endpoint in the graph?
>>>>>>>
>>>>>>> A <--> filter <--> B
>>>>>>
>>>>>> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
>>>>>> multi output ports).
>>>>>>
>>>>>> For clearer example:
>>>>>>
>>>>>> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1)
>>>>>> <--> B1
>>>>>> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2)
>>>>>> <--> B2
>>>>>> A2 <--> .. <--> ../                  p2  \ --> Unfiltered
>>>>>> <--> B0
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Instead of
>>>>>>>
>>>>>>> A <----through-filter----> B?
>>>>>>
>>>>>> The problem is we need to know the components in the path from A0 to X
>>>>>> through, (Not just A0 and L). And also we need to know "which port
>>>>>> (p0 vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out
>>>>>> of the
>>>>>> link "L".
>>>>>>
>>>>>> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
>>>>>>
>>>>>> would we need something else in the future ? I don't know for sure.
>>>>>> People could design their own things ;-). But this was the first time
>>>>>> ever in the last 12yrs since we supported coresight in the kernel.
>>>>>> (there is always a first time).
>>>>>>
>>>>>> Fundamentally, the "ports" cannot have additional properties today.
>>>>>> Not sure if there are other usecases (I don't see why). So, we have
>>>>>> to manually extend like above, which I think is not nice.
>>>>>
>>>>> Replying to the other thread [0], made me realize that the above is not
>>>>> true. Indeed it is possible to add properties for endpoints, e.g:
>>>>>
>>>>> e.g.: media/video-interfaces.yaml
>>>>>
>>>>> So extending the endpoint node is indeed acceptable (unlike I thought).
>>>>> May be the we it is achieved in this patch is making it look otherwise.
>>>>>
>>>>> Suzuki
>>>>> [0]
>>>>> https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com
>>>>
>>>> Please could you let us know if it is acceptable to extend "endpoint"
>>>> node to have an optional property ?
>>>
>>> Hi Krzysztof,
>>>
>>>
>>> Kindly reminder, could you help comment on this?
>>
>> I don't have any smart ideas and with earlier explanation sounds ok.
> 
> Just to confirm, are you OK with adding a property to the "endpoint"
> node that will indicate a phandle that the device allows on this
> endpoint ?

You mean the filter property in endpoint? if so, then yes.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: arm: qcom,coresight-static-replicator: Add property for source filtering

[Suzuki K Poulose]

On 18/10/2024 11:31, Krzysztof Kozlowski wrote:
> On 18/10/2024 12:08, Suzuki K Poulose wrote:
>> On 18/10/2024 11:05, Krzysztof Kozlowski wrote:
>>> On 17/10/2024 09:23, Tao Zhang wrote:
>>>>
>>>> On 10/9/2024 6:52 PM, Suzuki K Poulose wrote:
>>>>> Krzysztof
>>>>>
>>>>> On 22/08/2024 12:50, Suzuki K Poulose wrote:
>>>>>> On 22/08/2024 11:34, Suzuki K Poulose wrote:
>>>>>>> On 22/08/2024 08:08, Krzysztof Kozlowski wrote:
>>>>>>>> On Wed, Aug 21, 2024 at 11:38:55AM +0100, Suzuki K Poulose wrote:
>>>>>>>>> On 21/08/2024 04:13, Tao Zhang wrote:
>>>>>>>>>> The is some "magic" hard coded filtering in the replicators,
>>>>>>>>>> which only passes through trace from a particular "source". Add
>>>>>>>>>> a new property "filter-src" to label a phandle to the coresight
>>>>>>>>>> trace source device matching the hard coded filtering for the port.
>>>>>>>>>
>>>>>>>>> Minor nit: Please do not use abbreviate "source" in the bindings.
>>>>>>>>> I am not an expert on other changes below and will leave it to
>>>>>>>>> Rob/Krzysztof to comment.
>>>>>>>>>
>>>>>>>>> Rob, Krzysztof,
>>>>>>>>>
>>>>>>>>> We need someway to "link" (add a phandle) from a "port". The patch
>>>>>>>>> below
>>>>>>>>> is extending "standard" port to add a phandle. Please let us know if
>>>>>>>>> there is a better way.
>>>>>>>>>
>>>>>>>>> e.g.:
>>>>>>>>>
>>>>>>>>> filters = list of tuples of port, phandle. ?
>>>>>>>>>
>>>>>>>>> e.g.:
>>>>>>>>>
>>>>>>>>> filters = < 0, <&tpdm_video>,
>>>>>>>>>                1, <&tpdm_mdss>
>>>>>>>>>          >
>>>>>>>>>
>>>>>>>>
>>>>>>>> Current solution feels like band-aid - what if next time you need some
>>>>>>>> second filter? Or "wall"? Or whatever? Next property?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Isn't filter just one endpoint in the graph?
>>>>>>>>
>>>>>>>> A <--> filter <--> B
>>>>>>>
>>>>>>> To be more precise, "Filter" is a "port (p0, p1, p2 below)" (among a
>>>>>>> multi output ports).
>>>>>>>
>>>>>>> For clearer example:
>>>>>>>
>>>>>>> A0 <--> .. <--> ..\                  p0  / --> Filtered for (A1)
>>>>>>> <--> B1
>>>>>>> A1 <--> .. <--> .. - < L(filters>    p1  - --> Filtered for (A2)
>>>>>>> <--> B2
>>>>>>> A2 <--> .. <--> ../                  p2  \ --> Unfiltered
>>>>>>> <--> B0
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Instead of
>>>>>>>>
>>>>>>>> A <----through-filter----> B?
>>>>>>>
>>>>>>> The problem is we need to know the components in the path from A0 to X
>>>>>>> through, (Not just A0 and L). And also we need to know "which port
>>>>>>> (p0 vs p1 vs p2)" does the traffic take from a source (A0/A1/A2) out
>>>>>>> of the
>>>>>>> link "L".
>>>>>>>
>>>>>>> So ideally, we need a way to tie p0 -> A1, p1 -> A2.
>>>>>>>
>>>>>>> would we need something else in the future ? I don't know for sure.
>>>>>>> People could design their own things ;-). But this was the first time
>>>>>>> ever in the last 12yrs since we supported coresight in the kernel.
>>>>>>> (there is always a first time).
>>>>>>>
>>>>>>> Fundamentally, the "ports" cannot have additional properties today.
>>>>>>> Not sure if there are other usecases (I don't see why). So, we have
>>>>>>> to manually extend like above, which I think is not nice.
>>>>>>
>>>>>> Replying to the other thread [0], made me realize that the above is not
>>>>>> true. Indeed it is possible to add properties for endpoints, e.g:
>>>>>>
>>>>>> e.g.: media/video-interfaces.yaml
>>>>>>
>>>>>> So extending the endpoint node is indeed acceptable (unlike I thought).
>>>>>> May be the we it is achieved in this patch is making it look otherwise.
>>>>>>
>>>>>> Suzuki
>>>>>> [0]
>>>>>> https://lkml.kernel.org/r/4b51d5a9-3706-4630-83c1-01b01354d9a4@arm.com
>>>>>
>>>>> Please could you let us know if it is acceptable to extend "endpoint"
>>>>> node to have an optional property ?
>>>>
>>>> Hi Krzysztof,
>>>>
>>>>
>>>> Kindly reminder, could you help comment on this?
>>>
>>> I don't have any smart ideas and with earlier explanation sounds ok.
>>
>> Just to confirm, are you OK with adding a property to the "endpoint"
>> node that will indicate a phandle that the device allows on this
>> endpoint ?
> 
> You mean the filter property in endpoint? if so, then yes.

Thanks for confirming !

Cheers
Suzuki


> 
> Best regards,
> Krzysztof
> 

[PATCH v3 2/3] coresight: Add support for trace filtering by source

[Tao Zhang]

Some replicators have hard coded filtering of "trace" data, based on the
source device. This is different from the trace filtering based on
TraceID, available in the standard programmable replicators. e.g.,
Qualcomm replicators have filtering based on custom trace protocol
format and is not programmable.

The source device could be connected to the replicator via intermediate
components (e.g., a funnel). Thus we need platform information from
the firmware tables to decide the source device corresponding to a
given output port from the replicator. Given this affects "trace
path building" and traversing the path back from the sink to source,
add the concept of "filtering by source" to the generic coresight
connection.

The specified source will be marked like below in the Devicetree.
test-replicator {
    ... ... ... ...
    out-ports {
        ... ... ... ...
        port@0 {
            reg = <0>;
            xxx: endpoint {
                remote-endpoint = <&xxx>;
                filter_src = <&xxx>; <-- To specify the source to
            };                           be filtered out here.
        };

        port@1 {
            reg = <1>;
            yyy: endpoint {
                remote-endpoint = <&yyy>;
                filter_src = <&yyy>; <-- To specify the source to
            };                           be filtered out here.
        };
    };
};

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-core.c  | 136 +++++++++++++++---
 .../hwtracing/coresight/coresight-platform.c  |  18 +++
 include/linux/coresight.h                     |   5 +
 3 files changed, 136 insertions(+), 23 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 9fc6f6b863e0..d09266a4dd1c 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -75,22 +75,57 @@ struct coresight_device *coresight_get_percpu_sink(int cpu)
 }
 EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
 
+static struct coresight_device *coresight_get_source(struct list_head *path)
+{
+	struct coresight_device *csdev;
+
+	if (!path)
+		return NULL;
+
+	csdev = list_first_entry(path, struct coresight_node, link)->csdev;
+	if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
+		return NULL;
+
+	return csdev;
+}
+
+/**
+ * coresight_block_source - checks whether the connection matches the source
+ * of path if connection is binded to specific source.
+ * @src:	The source device of the trace path
+ * @conn:	The connection of one outport
+ *
+ * Return false if the connection doesn't have a source binded or source of the
+ * path matches the source binds to connection.
+ */
+static bool coresight_block_source(struct coresight_device *src,
+				    struct coresight_connection *conn)
+{
+	if (conn->filter_src_fwnode && (conn->filter_src_dev != src))
+		return true;
+	else
+		return false;
+}
+
 static struct coresight_connection *
-coresight_find_out_connection(struct coresight_device *src_dev,
-			      struct coresight_device *dest_dev)
+coresight_find_out_connection(struct coresight_device *csdev,
+			      struct coresight_device *out_dev,
+			      struct coresight_device *trace_src)
 {
 	int i;
 	struct coresight_connection *conn;
 
-	for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
-		conn = src_dev->pdata->out_conns[i];
-		if (conn->dest_dev == dest_dev)
+	for (i = 0; i < csdev->pdata->nr_outconns; i++) {
+		conn = csdev->pdata->out_conns[i];
+		if (coresight_block_source(trace_src, conn))
+			continue;
+		if (conn->dest_dev == out_dev)
 			return conn;
 	}
 
-	dev_err(&src_dev->dev,
-		"couldn't find output connection, src_dev: %s, dest_dev: %s\n",
-		dev_name(&src_dev->dev), dev_name(&dest_dev->dev));
+	dev_err(&csdev->dev,
+		"couldn't find output connection, csdev: %s, out_dev: %s\n",
+		dev_name(&csdev->dev), dev_name(&out_dev->dev));
 
 	return ERR_PTR(-ENODEV);
 }
@@ -251,7 +286,8 @@ static void coresight_disable_sink(struct coresight_device *csdev)
 
 static int coresight_enable_link(struct coresight_device *csdev,
 				 struct coresight_device *parent,
-				 struct coresight_device *child)
+				 struct coresight_device *child,
+				 struct coresight_device *source)
 {
 	int link_subtype;
 	struct coresight_connection *inconn, *outconn;
@@ -259,8 +295,8 @@ static int coresight_enable_link(struct coresight_device *csdev,
 	if (!parent || !child)
 		return -EINVAL;
 
-	inconn = coresight_find_out_connection(parent, csdev);
-	outconn = coresight_find_out_connection(csdev, child);
+	inconn = coresight_find_out_connection(parent, csdev, source);
+	outconn = coresight_find_out_connection(csdev, child, source);
 	link_subtype = csdev->subtype.link_subtype;
 
 	if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && IS_ERR(inconn))
@@ -273,15 +309,16 @@ static int coresight_enable_link(struct coresight_device *csdev,
 
 static void coresight_disable_link(struct coresight_device *csdev,
 				   struct coresight_device *parent,
-				   struct coresight_device *child)
+				   struct coresight_device *child,
+				   struct coresight_device *source)
 {
 	struct coresight_connection *inconn, *outconn;
 
 	if (!parent || !child)
 		return;
 
-	inconn = coresight_find_out_connection(parent, csdev);
-	outconn = coresight_find_out_connection(csdev, child);
+	inconn = coresight_find_out_connection(parent, csdev, source);
+	outconn = coresight_find_out_connection(csdev, child, source);
 
 	link_ops(csdev)->disable(csdev, inconn, outconn);
 }
@@ -337,7 +374,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,
+					struct coresight_device *source)
 {
 	u32 type;
 	struct coresight_device *csdev, *parent, *child;
@@ -375,7 +413,7 @@ static void coresight_disable_path_from(struct list_head *path,
 		case CORESIGHT_DEV_TYPE_LINK:
 			parent = list_prev_entry(nd, link)->csdev;
 			child = list_next_entry(nd, link)->csdev;
-			coresight_disable_link(csdev, parent, child);
+			coresight_disable_link(csdev, parent, child, source);
 			break;
 		default:
 			break;
@@ -388,7 +426,10 @@ static void coresight_disable_path_from(struct list_head *path,
 
 void coresight_disable_path(struct list_head *path)
 {
-	coresight_disable_path_from(path, NULL);
+	struct coresight_device *source;
+
+	source = coresight_get_source(path);
+	coresight_disable_path_from(path, NULL, source);
 }
 EXPORT_SYMBOL_GPL(coresight_disable_path);
 
@@ -418,7 +459,9 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
 	u32 type;
 	struct coresight_node *nd;
 	struct coresight_device *csdev, *parent, *child;
+	struct coresight_device *source;
 
+	source = coresight_get_source(path);
 	list_for_each_entry_reverse(nd, path, link) {
 		csdev = nd->csdev;
 		type = csdev->type;
@@ -456,7 +499,7 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
 		case CORESIGHT_DEV_TYPE_LINK:
 			parent = list_prev_entry(nd, link)->csdev;
 			child = list_next_entry(nd, link)->csdev;
-			ret = coresight_enable_link(csdev, parent, child);
+			ret = coresight_enable_link(csdev, parent, child, source);
 			if (ret)
 				goto err;
 			break;
@@ -468,7 +511,7 @@ 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, source);
 	goto out;
 }
 
@@ -619,6 +662,7 @@ static void coresight_drop_device(struct coresight_device *csdev)
  * @csdev:	The device to start from.
  * @sink:	The final sink we want in this path.
  * @path:	The list to add devices to.
+ * @source:	The trace source device of the path.
  *
  * The tree of Coresight device is traversed until @sink is found.
  * From there the sink is added to the list along with all the devices that led
@@ -627,7 +671,8 @@ static void coresight_drop_device(struct coresight_device *csdev)
  */
 static int _coresight_build_path(struct coresight_device *csdev,
 				 struct coresight_device *sink,
-				 struct list_head *path)
+				 struct list_head *path,
+				 struct coresight_device *source)
 {
 	int i, ret;
 	bool found = false;
@@ -639,7 +684,7 @@ static int _coresight_build_path(struct coresight_device *csdev,
 
 	if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) &&
 	    sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) {
-		if (_coresight_build_path(sink, sink, path) == 0) {
+		if (_coresight_build_path(sink, sink, path, source) == 0) {
 			found = true;
 			goto out;
 		}
@@ -650,8 +695,13 @@ static int _coresight_build_path(struct coresight_device *csdev,
 		struct coresight_device *child_dev;
 
 		child_dev = csdev->pdata->out_conns[i]->dest_dev;
+
+		if (csdev->pdata->out_conns[i]->filter_src_dev
+		    && (csdev->pdata->out_conns[i]->filter_src_dev != source))
+			continue;
+
 		if (child_dev &&
-		    _coresight_build_path(child_dev, sink, path) == 0) {
+		    _coresight_build_path(child_dev, sink, path, source) == 0) {
 			found = true;
 			break;
 		}
@@ -696,7 +746,7 @@ struct list_head *coresight_build_path(struct coresight_device *source,
 
 	INIT_LIST_HEAD(path);
 
-	rc = _coresight_build_path(source, sink, path);
+	rc = _coresight_build_path(source, sink, path, source);
 	if (rc) {
 		kfree(path);
 		return ERR_PTR(rc);
@@ -924,6 +974,16 @@ static int coresight_orphan_match(struct device *dev, void *data)
 	for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
 		conn = src_csdev->pdata->out_conns[i];
 
+		/* Fix filter source device before skip the port */
+		if ((conn->filter_src_fwnode) && dst_csdev
+			&& (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
+			if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
+				conn->filter_src_dev = dst_csdev;
+			else
+				dev_warn(&conn->filter_src_dev->dev,
+				  "Filter source is not a source device\n");
+		}
+
 		/* Skip the port if it's already connected. */
 		if (conn->dest_dev)
 			continue;
@@ -953,6 +1013,14 @@ static int coresight_orphan_match(struct device *dev, void *data)
 			ret = coresight_add_in_conn(conn);
 			if (ret)
 				return ret;
+
+			/*
+			 * Make sure the device still orphan if there is a
+			 * filter source firmware node but the filter source
+			 * device is null.
+			 */
+			if (conn->filter_src_fwnode && !conn->filter_src_dev)
+				still_orphan = true;
 		} else {
 			/* This component still has an orphan */
 			still_orphan = true;
@@ -974,18 +1042,40 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)
 			 csdev, coresight_orphan_match);
 }
 
+static int coresight_clear_filter_source(struct device *dev, void *data)
+{
+	int i;
+	struct coresight_device *source = data;
+	struct coresight_device *csdev = to_coresight_device(dev);
+
+	for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
+		if (csdev->pdata->out_conns[i]->filter_src_dev == source)
+			csdev->pdata->out_conns[i]->filter_src_dev = NULL;
+	}
+	return 0;
+}
+
 /* coresight_remove_conns - Remove other device's references to this device */
 static void coresight_remove_conns(struct coresight_device *csdev)
 {
 	int i, j;
 	struct coresight_connection *conn;
 
+	if (csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
+		bus_for_each_dev(&coresight_bustype, NULL, csdev,
+				 coresight_clear_filter_source);
+
 	/*
 	 * Remove the input connection references from the destination device
 	 * for each output connection.
 	 */
 	for (i = 0; i < csdev->pdata->nr_outconns; i++) {
 		conn = csdev->pdata->out_conns[i];
+		if (conn->filter_src_fwnode) {
+			conn->filter_src_dev = NULL;
+			fwnode_handle_put(conn->filter_src_fwnode);
+		}
+
 		if (!conn->dest_dev)
 			continue;
 
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 64e171eaad82..6ef4b26f04bb 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -243,6 +243,24 @@ static int of_coresight_parse_endpoint(struct device *dev,
 		conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
 		conn.dest_port = rendpoint.port;
 
+		/*
+		 * Get the firmware node of the filter source through the
+		 * reference. This could be used to filter the source in
+		 * building path.
+		 */
+		conn.filter_src_fwnode =
+			fwnode_find_reference(&ep->fwnode, "filter-src", 0);
+		if (IS_ERR(conn.filter_src_fwnode))
+			conn.filter_src_fwnode = NULL;
+		else {
+			conn.filter_src_dev =
+			 coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
+			if (conn.filter_src_dev && (conn.filter_src_dev->type
+			    != CORESIGHT_DEV_TYPE_SOURCE))
+				dev_warn(&conn.filter_src_dev->dev,
+				  "Filter source is not a source device\n");
+		}
+
 		new_conn = coresight_add_out_conn(dev, pdata, &conn);
 		if (IS_ERR_VALUE(new_conn)) {
 			fwnode_handle_put(conn.dest_fwnode);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f09ace92176e..91a689b4514b 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -172,6 +172,9 @@ struct coresight_desc {
  * @dest_dev:	a @coresight_device representation of the component
 		connected to @src_port. NULL until the device is created
  * @link: Representation of the connection as a sysfs link.
+ * @filter_src_fwnode: filter source component's fwnode handle.
+ * @filter_src_dev: a @coresight_device representation of the component that
+		needs to be filtered.
  *
  * The full connection structure looks like this, where in_conns store
  * references to same connection as the source device's out_conns.
@@ -200,6 +203,8 @@ struct coresight_connection {
 	struct coresight_device *dest_dev;
 	struct coresight_sysfs_link *link;
 	struct coresight_device *src_dev;
+	struct fwnode_handle *filter_src_fwnode;
+	struct coresight_device *filter_src_dev;
 	atomic_t src_refcnt;
 	atomic_t dest_refcnt;
 };
-- 
2.17.1

Re: [PATCH v3 2/3] coresight: Add support for trace filtering by source

[Suzuki K Poulose]

Hi Tao

On 21/08/2024 04:13, Tao Zhang wrote:
> Some replicators have hard coded filtering of "trace" data, based on the
> source device. This is different from the trace filtering based on
> TraceID, available in the standard programmable replicators. e.g.,
> Qualcomm replicators have filtering based on custom trace protocol
> format and is not programmable.
> 
> The source device could be connected to the replicator via intermediate
> components (e.g., a funnel). Thus we need platform information from
> the firmware tables to decide the source device corresponding to a
> given output port from the replicator. Given this affects "trace
> path building" and traversing the path back from the sink to source,
> add the concept of "filtering by source" to the generic coresight
> connection.
> 

This looks good, except for some minor comments below.

> The specified source will be marked like below in the Devicetree.
> test-replicator {
>      ... ... ... ...
>      out-ports {
>          ... ... ... ...
>          port@0 {
>              reg = <0>;
>              xxx: endpoint {
>                  remote-endpoint = <&xxx>;
>                  filter_src = <&xxx>; <-- To specify the source to
>              };                           be filtered out here.
>          };
> 
>          port@1 {
>              reg = <1>;
>              yyy: endpoint {
>                  remote-endpoint = <&yyy>;
>                  filter_src = <&yyy>; <-- To specify the source to
>              };                           be filtered out here.
>          };
>      };
> };
> 
> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c  | 136 +++++++++++++++---
>   .../hwtracing/coresight/coresight-platform.c  |  18 +++
>   include/linux/coresight.h                     |   5 +
>   3 files changed, 136 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 9fc6f6b863e0..d09266a4dd1c 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -75,22 +75,57 @@ struct coresight_device *coresight_get_percpu_sink(int cpu)
>   }
>   EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
>   
> +static struct coresight_device *coresight_get_source(struct list_head *path)
> +{
> +	struct coresight_device *csdev;
> +
> +	if (!path)
> +		return NULL;
> +
> +	csdev = list_first_entry(path, struct coresight_node, link)->csdev;
> +	if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
> +		return NULL;
> +
> +	return csdev;
> +}
> +
> +/**
> + * coresight_block_source - checks whether the connection matches the source
> + * of path if connection is binded to specific source.
> + * @src:	The source device of the trace path
> + * @conn:	The connection of one outport
> + *
> + * Return false if the connection doesn't have a source binded or source of the
> + * path matches the source binds to connection.
> + */
> +static bool coresight_block_source(struct coresight_device *src,
> +				    struct coresight_connection *conn)
> +{
> +	if (conn->filter_src_fwnode && (conn->filter_src_dev != src))
> +		return true;
> +	else
> +		return false;
> +}
> +
>   static struct coresight_connection *
> -coresight_find_out_connection(struct coresight_device *src_dev,
> -			      struct coresight_device *dest_dev)
> +coresight_find_out_connection(struct coresight_device *csdev,
> +			      struct coresight_device *out_dev,
> +			      struct coresight_device *trace_src)
>   {
>   	int i;
>   	struct coresight_connection *conn;
>   
> -	for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
> -		conn = src_dev->pdata->out_conns[i];
> -		if (conn->dest_dev == dest_dev)
> +	for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> +		conn = csdev->pdata->out_conns[i];
> +		if (coresight_block_source(trace_src, conn))
> +			continue;
> +		if (conn->dest_dev == out_dev)
>   			return conn;
>   	}
>   
> -	dev_err(&src_dev->dev,
> -		"couldn't find output connection, src_dev: %s, dest_dev: %s\n",
> -		dev_name(&src_dev->dev), dev_name(&dest_dev->dev));
> +	dev_err(&csdev->dev,
> +		"couldn't find output connection, csdev: %s, out_dev: %s\n",
> +		dev_name(&csdev->dev), dev_name(&out_dev->dev));
>   
>   	return ERR_PTR(-ENODEV);
>   }
> @@ -251,7 +286,8 @@ static void coresight_disable_sink(struct coresight_device *csdev)
>   
>   static int coresight_enable_link(struct coresight_device *csdev,
>   				 struct coresight_device *parent,
> -				 struct coresight_device *child)
> +				 struct coresight_device *child,
> +				 struct coresight_device *source)
>   {
>   	int link_subtype;
>   	struct coresight_connection *inconn, *outconn;
> @@ -259,8 +295,8 @@ static int coresight_enable_link(struct coresight_device *csdev,
>   	if (!parent || !child)
>   		return -EINVAL;
>   
> -	inconn = coresight_find_out_connection(parent, csdev);
> -	outconn = coresight_find_out_connection(csdev, child);
> +	inconn = coresight_find_out_connection(parent, csdev, source);
> +	outconn = coresight_find_out_connection(csdev, child, source);
>   	link_subtype = csdev->subtype.link_subtype;
>   
>   	if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && IS_ERR(inconn))
> @@ -273,15 +309,16 @@ static int coresight_enable_link(struct coresight_device *csdev,
>   
>   static void coresight_disable_link(struct coresight_device *csdev,
>   				   struct coresight_device *parent,
> -				   struct coresight_device *child)
> +				   struct coresight_device *child,
> +				   struct coresight_device *source)
>   {
>   	struct coresight_connection *inconn, *outconn;
>   
>   	if (!parent || !child)
>   		return;
>   
> -	inconn = coresight_find_out_connection(parent, csdev);
> -	outconn = coresight_find_out_connection(csdev, child);
> +	inconn = coresight_find_out_connection(parent, csdev, source);
> +	outconn = coresight_find_out_connection(csdev, child, source);
>   
>   	link_ops(csdev)->disable(csdev, inconn, outconn);
>   }
> @@ -337,7 +374,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,
> +					struct coresight_device *source)

Apologies, I may not have been clear enough. But we concluded that the
path here is suitable for coresight_get_source(path) and as such we
don't need to explicitly pass the source.

>   {
>   	u32 type;
>   	struct coresight_device *csdev, *parent, *child;
> @@ -375,7 +413,7 @@ static void coresight_disable_path_from(struct list_head *path,
>   		case CORESIGHT_DEV_TYPE_LINK:
>   			parent = list_prev_entry(nd, link)->csdev;
>   			child = list_next_entry(nd, link)->csdev;
> -			coresight_disable_link(csdev, parent, child);
> +			coresight_disable_link(csdev, parent, child, source);
>   			break;
>   		default:
>   			break;
> @@ -388,7 +426,10 @@ static void coresight_disable_path_from(struct list_head *path,
>   
>   void coresight_disable_path(struct list_head *path)
>   {
> -	coresight_disable_path_from(path, NULL);
> +	struct coresight_device *source;
> +
> +	source = coresight_get_source(path);
> +	coresight_disable_path_from(path, NULL, source);
>   }
>   EXPORT_SYMBOL_GPL(coresight_disable_path);
>   
> @@ -418,7 +459,9 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>   	u32 type;
>   	struct coresight_node *nd;
>   	struct coresight_device *csdev, *parent, *child;
> +	struct coresight_device *source;
>   
> +	source = coresight_get_source(path);
>   	list_for_each_entry_reverse(nd, path, link) {
>   		csdev = nd->csdev;
>   		type = csdev->type;
> @@ -456,7 +499,7 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>   		case CORESIGHT_DEV_TYPE_LINK:
>   			parent = list_prev_entry(nd, link)->csdev;
>   			child = list_next_entry(nd, link)->csdev;
> -			ret = coresight_enable_link(csdev, parent, child);
> +			ret = coresight_enable_link(csdev, parent, child, source);
>   			if (ret)
>   				goto err;
>   			break;
> @@ -468,7 +511,7 @@ 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, source);
>   	goto out;
>   }
>   
> @@ -619,6 +662,7 @@ static void coresight_drop_device(struct coresight_device *csdev)
>    * @csdev:	The device to start from.
>    * @sink:	The final sink we want in this path.
>    * @path:	The list to add devices to.
> + * @source:	The trace source device of the path.
>    *
>    * The tree of Coresight device is traversed until @sink is found.
>    * From there the sink is added to the list along with all the devices that led
> @@ -627,7 +671,8 @@ static void coresight_drop_device(struct coresight_device *csdev)
>    */
>   static int _coresight_build_path(struct coresight_device *csdev,
>   				 struct coresight_device *sink,
> -				 struct list_head *path)
> +				 struct list_head *path,
> +				 struct coresight_device *source)

minor nit: Please could we reorder the parameter order :

_coresight_build_path(csdev, source, sink, path) ?

That makes it much better to read : build a path from "source" to 
"sink", from "csdev"

>   {
>   	int i, ret;
>   	bool found = false;
> @@ -639,7 +684,7 @@ static int _coresight_build_path(struct coresight_device *csdev,
>   
>   	if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) &&
>   	    sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) {
> -		if (_coresight_build_path(sink, sink, path) == 0) {
> +		if (_coresight_build_path(sink, sink, path, source) == 0) {
>   			found = true;
>   			goto out;
>   		}
> @@ -650,8 +695,13 @@ static int _coresight_build_path(struct coresight_device *csdev,
>   		struct coresight_device *child_dev;
>   
>   		child_dev = csdev->pdata->out_conns[i]->dest_dev;
> +
> +		if (csdev->pdata->out_conns[i]->filter_src_dev
> +		    && (csdev->pdata->out_conns[i]->filter_src_dev != source))
> +			continue;

Please reuse coresight_block_source(). i.e.,

if (coresight_block_source(source, csdev->pdata->cout_conns[i]))
	continue;

> +
>   		if (child_dev &&
> -		    _coresight_build_path(child_dev, sink, path) == 0) {
> +		    _coresight_build_path(child_dev, sink, path, source) == 0) {
>   			found = true;
>   			break;
>   		}
> @@ -696,7 +746,7 @@ struct list_head *coresight_build_path(struct coresight_device *source,
>   
>   	INIT_LIST_HEAD(path);
>   
> -	rc = _coresight_build_path(source, sink, path);
> +	rc = _coresight_build_path(source, sink, path, source);
>   	if (rc) {
>   		kfree(path);
>   		return ERR_PTR(rc);
> @@ -924,6 +974,16 @@ static int coresight_orphan_match(struct device *dev, void *data)
>   	for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
>   		conn = src_csdev->pdata->out_conns[i];
>   
> +		/* Fix filter source device before skip the port */
> +		if ((conn->filter_src_fwnode) && dst_csdev

minor nit: unnecessary () around conn->filter_src_fwnode

> +			&& (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
> +			if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
> +				conn->filter_src_dev = dst_csdev;
> +			else
> +				dev_warn(&conn->filter_src_dev->dev,
> +				  "Filter source is not a source device\n");
> +		}

We could set the still_orphan here instead of down below.

	/* Fixup filter source link */
	if (conn->filter_src_fwnode && !conn->filter_src_dev) {
		if (dst_csdev &&
		    conn->filter_src_fwnode == dst_csdev->dev.fwnode &&
		    !WARN_ON_ONCE(dst_csdev->type != CORESIGHT_DEV_TYPE_SOURCE) {
			conn->filter_src_dev = dst_csdev;
		else
			still_orphan = true;
	}

minor nit: Also I think it is high time we add a helper to check if a
devices is SOURCE, something like we did for is_device_tpdm(). e.g.,


static inline bool coresight_is_device_source(struct coresight_device 
*csdev)
{
	return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE);
}


> +
>   		/* Skip the port if it's already connected. */
>   		if (conn->dest_dev)
>   			continue;
> @@ -953,6 +1013,14 @@ static int coresight_orphan_match(struct device *dev, void *data)
>   			ret = coresight_add_in_conn(conn);
>   			if (ret)
>   				return ret;
> +
> +			/*
> +			 * Make sure the device still orphan if there is a
> +			 * filter source firmware node but the filter source
> +			 * device is null.
> +			 */
> +			if (conn->filter_src_fwnode && !conn->filter_src_dev)
> +				still_orphan = true;

Could we not move this to the place where we fixup the filter_src_dev ? 
See above.

>   		} else {
>   			/* This component still has an orphan */
>   			still_orphan = true;
> @@ -974,18 +1042,40 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)
>   			 csdev, coresight_orphan_match);
>   }
>   
> +static int coresight_clear_filter_source(struct device *dev, void *data)
> +{
> +	int i;
> +	struct coresight_device *source = data;
> +	struct coresight_device *csdev = to_coresight_device(dev);
> +
> +	for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
> +		if (csdev->pdata->out_conns[i]->filter_src_dev == source)
> +			csdev->pdata->out_conns[i]->filter_src_dev = NULL;
> +	}
> +	return 0;
> +}
> +
>   /* coresight_remove_conns - Remove other device's references to this device */
>   static void coresight_remove_conns(struct coresight_device *csdev)
>   {
>   	int i, j;
>   	struct coresight_connection *conn;
>   
> +	if (csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
> +		bus_for_each_dev(&coresight_bustype, NULL, csdev,
> +				 coresight_clear_filter_source);
> +
>   	/*
>   	 * Remove the input connection references from the destination device
>   	 * for each output connection.
>   	 */
>   	for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>   		conn = csdev->pdata->out_conns[i];
> +		if (conn->filter_src_fwnode) {
> +			conn->filter_src_dev = NULL;
> +			fwnode_handle_put(conn->filter_src_fwnode);
> +		}
> +
>   		if (!conn->dest_dev)
>   			continue;
>   
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 64e171eaad82..6ef4b26f04bb 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -243,6 +243,24 @@ static int of_coresight_parse_endpoint(struct device *dev,
>   		conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
>   		conn.dest_port = rendpoint.port;
>   
> +		/*
> +		 * Get the firmware node of the filter source through the
> +		 * reference. This could be used to filter the source in
> +		 * building path.
> +		 */
> +		conn.filter_src_fwnode =
> +			fwnode_find_reference(&ep->fwnode, "filter-src", 0);
> +		if (IS_ERR(conn.filter_src_fwnode))
> +			conn.filter_src_fwnode = NULL;
> +		else {
> +			conn.filter_src_dev =
> +			 coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
> +			if (conn.filter_src_dev && (conn.filter_src_dev->type
> +			    != CORESIGHT_DEV_TYPE_SOURCE))
> +				dev_warn(&conn.filter_src_dev->dev,
> +				  "Filter source is not a source device\n");
> +		}
> +
>   		new_conn = coresight_add_out_conn(dev, pdata, &conn);
>   		if (IS_ERR_VALUE(new_conn)) {
>   			fwnode_handle_put(conn.dest_fwnode);
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index f09ace92176e..91a689b4514b 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -172,6 +172,9 @@ struct coresight_desc {
>    * @dest_dev:	a @coresight_device representation of the component
>   		connected to @src_port. NULL until the device is created
>    * @link: Representation of the connection as a sysfs link.
> + * @filter_src_fwnode: filter source component's fwnode handle.
> + * @filter_src_dev: a @coresight_device representation of the component that
> +		needs to be filtered.
>    *
>    * The full connection structure looks like this, where in_conns store
>    * references to same connection as the source device's out_conns.
> @@ -200,6 +203,8 @@ struct coresight_connection {
>   	struct coresight_device *dest_dev;
>   	struct coresight_sysfs_link *link;
>   	struct coresight_device *src_dev;
> +	struct fwnode_handle *filter_src_fwnode;
> +	struct coresight_device *filter_src_dev;
>   	atomic_t src_refcnt;
>   	atomic_t dest_refcnt;
>   };


Suzuki

Re: [PATCH v3 2/3] coresight: Add support for trace filtering by source

[Tao Zhang]

On 8/21/2024 11:23 PM, Suzuki K Poulose wrote:
> Hi Tao
>
> On 21/08/2024 04:13, Tao Zhang wrote:
>> Some replicators have hard coded filtering of "trace" data, based on the
>> source device. This is different from the trace filtering based on
>> TraceID, available in the standard programmable replicators. e.g.,
>> Qualcomm replicators have filtering based on custom trace protocol
>> format and is not programmable.
>>
>> The source device could be connected to the replicator via intermediate
>> components (e.g., a funnel). Thus we need platform information from
>> the firmware tables to decide the source device corresponding to a
>> given output port from the replicator. Given this affects "trace
>> path building" and traversing the path back from the sink to source,
>> add the concept of "filtering by source" to the generic coresight
>> connection.
>>
>
> This looks good, except for some minor comments below.
OK, I will remove the comments below in the next version.
>
>> The specified source will be marked like below in the Devicetree.
>> test-replicator {
>>      ... ... ... ...
>>      out-ports {
>>          ... ... ... ...
>>          port@0 {
>>              reg = <0>;
>>              xxx: endpoint {
>>                  remote-endpoint = <&xxx>;
>>                  filter_src = <&xxx>; <-- To specify the source to
>>              };                           be filtered out here.
>>          };
>>
>>          port@1 {
>>              reg = <1>;
>>              yyy: endpoint {
>>                  remote-endpoint = <&yyy>;
>>                  filter_src = <&yyy>; <-- To specify the source to
>>              };                           be filtered out here.
>>          };
>>      };
>> };
>>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c  | 136 +++++++++++++++---
>>   .../hwtracing/coresight/coresight-platform.c  |  18 +++
>>   include/linux/coresight.h                     |   5 +
>>   3 files changed, 136 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 9fc6f6b863e0..d09266a4dd1c 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -75,22 +75,57 @@ struct coresight_device 
>> *coresight_get_percpu_sink(int cpu)
>>   }
>>   EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
>>   +static struct coresight_device *coresight_get_source(struct 
>> list_head *path)
>> +{
>> +    struct coresight_device *csdev;
>> +
>> +    if (!path)
>> +        return NULL;
>> +
>> +    csdev = list_first_entry(path, struct coresight_node, link)->csdev;
>> +    if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
>> +        return NULL;
>> +
>> +    return csdev;
>> +}
>> +
>> +/**
>> + * coresight_block_source - checks whether the connection matches 
>> the source
>> + * of path if connection is binded to specific source.
>> + * @src:    The source device of the trace path
>> + * @conn:    The connection of one outport
>> + *
>> + * Return false if the connection doesn't have a source binded or 
>> source of the
>> + * path matches the source binds to connection.
>> + */
>> +static bool coresight_block_source(struct coresight_device *src,
>> +                    struct coresight_connection *conn)
>> +{
>> +    if (conn->filter_src_fwnode && (conn->filter_src_dev != src))
>> +        return true;
>> +    else
>> +        return false;
>> +}
>> +
>>   static struct coresight_connection *
>> -coresight_find_out_connection(struct coresight_device *src_dev,
>> -                  struct coresight_device *dest_dev)
>> +coresight_find_out_connection(struct coresight_device *csdev,
>> +                  struct coresight_device *out_dev,
>> +                  struct coresight_device *trace_src)
>>   {
>>       int i;
>>       struct coresight_connection *conn;
>>   -    for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
>> -        conn = src_dev->pdata->out_conns[i];
>> -        if (conn->dest_dev == dest_dev)
>> +    for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> +        conn = csdev->pdata->out_conns[i];
>> +        if (coresight_block_source(trace_src, conn))
>> +            continue;
>> +        if (conn->dest_dev == out_dev)
>>               return conn;
>>       }
>>   -    dev_err(&src_dev->dev,
>> -        "couldn't find output connection, src_dev: %s, dest_dev: %s\n",
>> -        dev_name(&src_dev->dev), dev_name(&dest_dev->dev));
>> +    dev_err(&csdev->dev,
>> +        "couldn't find output connection, csdev: %s, out_dev: %s\n",
>> +        dev_name(&csdev->dev), dev_name(&out_dev->dev));
>>         return ERR_PTR(-ENODEV);
>>   }
>> @@ -251,7 +286,8 @@ static void coresight_disable_sink(struct 
>> coresight_device *csdev)
>>     static int coresight_enable_link(struct coresight_device *csdev,
>>                    struct coresight_device *parent,
>> -                 struct coresight_device *child)
>> +                 struct coresight_device *child,
>> +                 struct coresight_device *source)
>>   {
>>       int link_subtype;
>>       struct coresight_connection *inconn, *outconn;
>> @@ -259,8 +295,8 @@ static int coresight_enable_link(struct 
>> coresight_device *csdev,
>>       if (!parent || !child)
>>           return -EINVAL;
>>   -    inconn = coresight_find_out_connection(parent, csdev);
>> -    outconn = coresight_find_out_connection(csdev, child);
>> +    inconn = coresight_find_out_connection(parent, csdev, source);
>> +    outconn = coresight_find_out_connection(csdev, child, source);
>>       link_subtype = csdev->subtype.link_subtype;
>>         if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && 
>> IS_ERR(inconn))
>> @@ -273,15 +309,16 @@ static int coresight_enable_link(struct 
>> coresight_device *csdev,
>>     static void coresight_disable_link(struct coresight_device *csdev,
>>                      struct coresight_device *parent,
>> -                   struct coresight_device *child)
>> +                   struct coresight_device *child,
>> +                   struct coresight_device *source)
>>   {
>>       struct coresight_connection *inconn, *outconn;
>>         if (!parent || !child)
>>           return;
>>   -    inconn = coresight_find_out_connection(parent, csdev);
>> -    outconn = coresight_find_out_connection(csdev, child);
>> +    inconn = coresight_find_out_connection(parent, csdev, source);
>> +    outconn = coresight_find_out_connection(csdev, child, source);
>>         link_ops(csdev)->disable(csdev, inconn, outconn);
>>   }
>> @@ -337,7 +374,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,
>> +                    struct coresight_device *source)
>
> Apologies, I may not have been clear enough. But we concluded that the
> path here is suitable for coresight_get_source(path) and as such we
> don't need to explicitly pass the source.

Do you mean we don't need to explicitly pass the source on 
"coresight_disable_path_from",

we can pass the "source" to "coresight_disable_link" by the following way?

coresight_disable_link(csdev, parent, child, coresight_get_source(path));

>
>>   {
>>       u32 type;
>>       struct coresight_device *csdev, *parent, *child;
>> @@ -375,7 +413,7 @@ static void coresight_disable_path_from(struct 
>> list_head *path,
>>           case CORESIGHT_DEV_TYPE_LINK:
>>               parent = list_prev_entry(nd, link)->csdev;
>>               child = list_next_entry(nd, link)->csdev;
>> -            coresight_disable_link(csdev, parent, child);
>> +            coresight_disable_link(csdev, parent, child, source);
>>               break;
>>           default:
>>               break;
>> @@ -388,7 +426,10 @@ static void coresight_disable_path_from(struct 
>> list_head *path,
>>     void coresight_disable_path(struct list_head *path)
>>   {
>> -    coresight_disable_path_from(path, NULL);
>> +    struct coresight_device *source;
>> +
>> +    source = coresight_get_source(path);
>> +    coresight_disable_path_from(path, NULL, source);
>>   }
>>   EXPORT_SYMBOL_GPL(coresight_disable_path);
>>   @@ -418,7 +459,9 @@ int coresight_enable_path(struct list_head 
>> *path, enum cs_mode mode,
>>       u32 type;
>>       struct coresight_node *nd;
>>       struct coresight_device *csdev, *parent, *child;
>> +    struct coresight_device *source;
>>   +    source = coresight_get_source(path);
>>       list_for_each_entry_reverse(nd, path, link) {
>>           csdev = nd->csdev;
>>           type = csdev->type;
>> @@ -456,7 +499,7 @@ int coresight_enable_path(struct list_head *path, 
>> enum cs_mode mode,
>>           case CORESIGHT_DEV_TYPE_LINK:
>>               parent = list_prev_entry(nd, link)->csdev;
>>               child = list_next_entry(nd, link)->csdev;
>> -            ret = coresight_enable_link(csdev, parent, child);
>> +            ret = coresight_enable_link(csdev, parent, child, source);
>>               if (ret)
>>                   goto err;
>>               break;
>> @@ -468,7 +511,7 @@ 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, source);
>>       goto out;
>>   }
>>   @@ -619,6 +662,7 @@ static void coresight_drop_device(struct 
>> coresight_device *csdev)
>>    * @csdev:    The device to start from.
>>    * @sink:    The final sink we want in this path.
>>    * @path:    The list to add devices to.
>> + * @source:    The trace source device of the path.
>>    *
>>    * The tree of Coresight device is traversed until @sink is found.
>>    * From there the sink is added to the list along with all the 
>> devices that led
>> @@ -627,7 +671,8 @@ static void coresight_drop_device(struct 
>> coresight_device *csdev)
>>    */
>>   static int _coresight_build_path(struct coresight_device *csdev,
>>                    struct coresight_device *sink,
>> -                 struct list_head *path)
>> +                 struct list_head *path,
>> +                 struct coresight_device *source)
>
> minor nit: Please could we reorder the parameter order :
>
> _coresight_build_path(csdev, source, sink, path) ?
>
> That makes it much better to read : build a path from "source" to 
> "sink", from "csdev"
All right, I will update in the next version.
>
>>   {
>>       int i, ret;
>>       bool found = false;
>> @@ -639,7 +684,7 @@ static int _coresight_build_path(struct 
>> coresight_device *csdev,
>>         if (coresight_is_percpu_source(csdev) && 
>> coresight_is_percpu_sink(sink) &&
>>           sink == per_cpu(csdev_sink, 
>> source_ops(csdev)->cpu_id(csdev))) {
>> -        if (_coresight_build_path(sink, sink, path) == 0) {
>> +        if (_coresight_build_path(sink, sink, path, source) == 0) {
>>               found = true;
>>               goto out;
>>           }
>> @@ -650,8 +695,13 @@ static int _coresight_build_path(struct 
>> coresight_device *csdev,
>>           struct coresight_device *child_dev;
>>             child_dev = csdev->pdata->out_conns[i]->dest_dev;
>> +
>> +        if (csdev->pdata->out_conns[i]->filter_src_dev
>> +            && (csdev->pdata->out_conns[i]->filter_src_dev != source))
>> +            continue;
>
> Please reuse coresight_block_source(). i.e.,
>
> if (coresight_block_source(source, csdev->pdata->cout_conns[i]))
>     continue;
OK, I will update in the next version.
>
>> +
>>           if (child_dev &&
>> -            _coresight_build_path(child_dev, sink, path) == 0) {
>> +            _coresight_build_path(child_dev, sink, path, source) == 
>> 0) {
>>               found = true;
>>               break;
>>           }
>> @@ -696,7 +746,7 @@ struct list_head *coresight_build_path(struct 
>> coresight_device *source,
>>         INIT_LIST_HEAD(path);
>>   -    rc = _coresight_build_path(source, sink, path);
>> +    rc = _coresight_build_path(source, sink, path, source);
>>       if (rc) {
>>           kfree(path);
>>           return ERR_PTR(rc);
>> @@ -924,6 +974,16 @@ static int coresight_orphan_match(struct device 
>> *dev, void *data)
>>       for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
>>           conn = src_csdev->pdata->out_conns[i];
>>   +        /* Fix filter source device before skip the port */
>> +        if ((conn->filter_src_fwnode) && dst_csdev
>
> minor nit: unnecessary () around conn->filter_src_fwnode
OK, I will update in the next version.
>
>> +            && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
>> +            if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
>> +                conn->filter_src_dev = dst_csdev;
>> +            else
>> +                dev_warn(&conn->filter_src_dev->dev,
>> +                  "Filter source is not a source device\n");
>> +        }
>
> We could set the still_orphan here instead of down below.
>
>     /* Fixup filter source link */
>     if (conn->filter_src_fwnode && !conn->filter_src_dev) {
>         if (dst_csdev &&
>             conn->filter_src_fwnode == dst_csdev->dev.fwnode &&
>             !WARN_ON_ONCE(dst_csdev->type != CORESIGHT_DEV_TYPE_SOURCE) {
>             conn->filter_src_dev = dst_csdev;
>         else
>             still_orphan = true;
>     }
>
> minor nit: Also I think it is high time we add a helper to check if a
> devices is SOURCE, something like we did for is_device_tpdm(). e.g.,
>
>
> static inline bool coresight_is_device_source(struct coresight_device 
> *csdev)
> {
>     return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE);
> }
Could you help review the following approach?

I will add the helper to coresight-priv.h

static inline bool coresight_is_device_source(struct coresight_device 
*csdev)
{
     return (csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE));
}

Then, calling the help to check if a device is SOURCE in 
coresight-platform.c and coresight-core.c

coresight-platform.c

... ... ... ...

         else {
             conn.filter_src_dev =
  coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
             if (conn.filter_src_dev &&
                 !coresight_is_device_source(conn.filter_src_dev))
                 dev_warn(&conn.filter_src_dev->dev,
                   "Filter source is not a source device\n");
         }

coresight-core.c

... ... ... ...

         if (conn->filter_src_fwnode && !conn->filter_src_dev) {
             if (dst_csdev && (conn->filter_src_fwnode == 
dst_csdev->dev.fwnode)
                 && !WARN_ON_ONCE(!coresight_is_device_source(dst_csdev)))
                 conn->filter_src_dev = dst_csdev;
             else
                 still_orphan = true;
         }

>
>
>> +
>>           /* Skip the port if it's already connected. */
>>           if (conn->dest_dev)
>>               continue;
>> @@ -953,6 +1013,14 @@ static int coresight_orphan_match(struct device 
>> *dev, void *data)
>>               ret = coresight_add_in_conn(conn);
>>               if (ret)
>>                   return ret;
>> +
>> +            /*
>> +             * Make sure the device still orphan if there is a
>> +             * filter source firmware node but the filter source
>> +             * device is null.
>> +             */
>> +            if (conn->filter_src_fwnode && !conn->filter_src_dev)
>> +                still_orphan = true;
>
> Could we not move this to the place where we fixup the filter_src_dev 
> ? See above.
OK, I will update in the next version.
>
>>           } else {
>>               /* This component still has an orphan */
>>               still_orphan = true;
>> @@ -974,18 +1042,40 @@ static int coresight_fixup_orphan_conns(struct 
>> coresight_device *csdev)
>>                csdev, coresight_orphan_match);
>>   }
>>   +static int coresight_clear_filter_source(struct device *dev, void 
>> *data)
>> +{
>> +    int i;
>> +    struct coresight_device *source = data;
>> +    struct coresight_device *csdev = to_coresight_device(dev);
>> +
>> +    for (i = 0; i < csdev->pdata->nr_outconns; ++i) {
>> +        if (csdev->pdata->out_conns[i]->filter_src_dev == source)
>> +            csdev->pdata->out_conns[i]->filter_src_dev = NULL;
>> +    }
>> +    return 0;
>> +}
>> +
>>   /* coresight_remove_conns - Remove other device's references to 
>> this device */
>>   static void coresight_remove_conns(struct coresight_device *csdev)
>>   {
>>       int i, j;
>>       struct coresight_connection *conn;
>>   +    if (csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
>> +        bus_for_each_dev(&coresight_bustype, NULL, csdev,
>> +                 coresight_clear_filter_source);
>> +
>>       /*
>>        * Remove the input connection references from the destination 
>> device
>>        * for each output connection.
>>        */
>>       for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>>           conn = csdev->pdata->out_conns[i];
>> +        if (conn->filter_src_fwnode) {
>> +            conn->filter_src_dev = NULL;
>> +            fwnode_handle_put(conn->filter_src_fwnode);
>> +        }
>> +
>>           if (!conn->dest_dev)
>>               continue;
>>   diff --git a/drivers/hwtracing/coresight/coresight-platform.c 
>> b/drivers/hwtracing/coresight/coresight-platform.c
>> index 64e171eaad82..6ef4b26f04bb 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -243,6 +243,24 @@ static int of_coresight_parse_endpoint(struct 
>> device *dev,
>>           conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
>>           conn.dest_port = rendpoint.port;
>>   +        /*
>> +         * Get the firmware node of the filter source through the
>> +         * reference. This could be used to filter the source in
>> +         * building path.
>> +         */
>> +        conn.filter_src_fwnode =
>> +            fwnode_find_reference(&ep->fwnode, "filter-src", 0);
>> +        if (IS_ERR(conn.filter_src_fwnode))
>> +            conn.filter_src_fwnode = NULL;
>> +        else {
>> +            conn.filter_src_dev =
>> + coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
>> +            if (conn.filter_src_dev && (conn.filter_src_dev->type
>> +                != CORESIGHT_DEV_TYPE_SOURCE))
>> +                dev_warn(&conn.filter_src_dev->dev,
>> +                  "Filter source is not a source device\n");
>> +        }
>> +
>>           new_conn = coresight_add_out_conn(dev, pdata, &conn);
>>           if (IS_ERR_VALUE(new_conn)) {
>>               fwnode_handle_put(conn.dest_fwnode);
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index f09ace92176e..91a689b4514b 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -172,6 +172,9 @@ struct coresight_desc {
>>    * @dest_dev:    a @coresight_device representation of the component
>>           connected to @src_port. NULL until the device is created
>>    * @link: Representation of the connection as a sysfs link.
>> + * @filter_src_fwnode: filter source component's fwnode handle.
>> + * @filter_src_dev: a @coresight_device representation of the 
>> component that
>> +        needs to be filtered.
>>    *
>>    * The full connection structure looks like this, where in_conns store
>>    * references to same connection as the source device's out_conns.
>> @@ -200,6 +203,8 @@ struct coresight_connection {
>>       struct coresight_device *dest_dev;
>>       struct coresight_sysfs_link *link;
>>       struct coresight_device *src_dev;
>> +    struct fwnode_handle *filter_src_fwnode;
>> +    struct coresight_device *filter_src_dev;
>>       atomic_t src_refcnt;
>>       atomic_t dest_refcnt;
>>   };
>
>
> Suzuki

Re: [PATCH v3 2/3] coresight: Add support for trace filtering by source

[Suzuki K Poulose]

Hi Tao

On 22/08/2024 17:44, Tao Zhang wrote:
> 
> On 8/21/2024 11:23 PM, Suzuki K Poulose wrote:
>> Hi Tao
>>
>> On 21/08/2024 04:13, Tao Zhang wrote:
>>> Some replicators have hard coded filtering of "trace" data, based on the
>>> source device. This is different from the trace filtering based on
>>> TraceID, available in the standard programmable replicators. e.g.,
>>> Qualcomm replicators have filtering based on custom trace protocol
>>> format and is not programmable.
>>>
>>> The source device could be connected to the replicator via intermediate
>>> components (e.g., a funnel). Thus we need platform information from
>>> the firmware tables to decide the source device corresponding to a
>>> given output port from the replicator. Given this affects "trace
>>> path building" and traversing the path back from the sink to source,
>>> add the concept of "filtering by source" to the generic coresight
>>> connection.
>>>
>>
>> This looks good, except for some minor comments below.
> OK, I will remove the comments below in the next version.
>>
>>> The specified source will be marked like below in the Devicetree.
>>> test-replicator {
>>>      ... ... ... ...
>>>      out-ports {
>>>          ... ... ... ...
>>>          port@0 {
>>>              reg = <0>;
>>>              xxx: endpoint {
>>>                  remote-endpoint = <&xxx>;
>>>                  filter_src = <&xxx>; <-- To specify the source to
>>>              };                           be filtered out here.
>>>          };
>>>
>>>          port@1 {
>>>              reg = <1>;
>>>              yyy: endpoint {
>>>                  remote-endpoint = <&yyy>;
>>>                  filter_src = <&yyy>; <-- To specify the source to
>>>              };                           be filtered out here.
>>>          };
>>>      };
>>> };
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-core.c  | 136 +++++++++++++++---
>>>   .../hwtracing/coresight/coresight-platform.c  |  18 +++
>>>   include/linux/coresight.h                     |   5 +
>>>   3 files changed, 136 insertions(+), 23 deletions(-)

...

>>> @@ -337,7 +374,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,
>>> +                    struct coresight_device *source)
>>
>> Apologies, I may not have been clear enough. But we concluded that the
>> path here is suitable for coresight_get_source(path) and as such we
>> don't need to explicitly pass the source.
> 
> Do you mean we don't need to explicitly pass the source on 
> "coresight_disable_path_from",
> 
> we can pass the "source" to "coresight_disable_link" by the following way?
> 
> coresight_disable_link(csdev, parent, child, coresight_get_source(path));

Correct.

> 
>>
>>>   {
>>>       u32 type;
>>>       struct coresight_device *csdev, *parent, *child;
>>> @@ -375,7 +413,7 @@ static void coresight_disable_path_from(struct 
>>> list_head *path,
>>>           case CORESIGHT_DEV_TYPE_LINK:
>>>               parent = list_prev_entry(nd, link)->csdev;
>>>               child = list_next_entry(nd, link)->csdev;
>>> -            coresight_disable_link(csdev, parent, child);
>>> +            coresight_disable_link(csdev, parent, child, source);
>>>               break;
>>>           default:
>>>               break;
>>> @@ -388,7 +426,10 @@ static void coresight_disable_path_from(struct 
>>> list_head *path,
>>>     void coresight_disable_path(struct list_head *path)
>>>   {
>>> -    coresight_disable_path_from(path, NULL);
>>> +    struct coresight_device *source;
>>> +
>>> +    source = coresight_get_source(path);
>>> +    coresight_disable_path_from(path, NULL, source);
>>>   }
>>>   EXPORT_SYMBOL_GPL(coresight_disable_path);
>>>   @@ -418,7 +459,9 @@ int coresight_enable_path(struct list_head 
>>> *path, enum cs_mode mode,
>>>       u32 type;
>>>       struct coresight_node *nd;
>>>       struct coresight_device *csdev, *parent, *child;
>>> +    struct coresight_device *source;
>>>   +    source = coresight_get_source(path);
>>>       list_for_each_entry_reverse(nd, path, link) {
>>>           csdev = nd->csdev;
>>>           type = csdev->type;
>>> @@ -456,7 +499,7 @@ int coresight_enable_path(struct list_head *path, 
>>> enum cs_mode mode,
>>>           case CORESIGHT_DEV_TYPE_LINK:
>>>               parent = list_prev_entry(nd, link)->csdev;
>>>               child = list_next_entry(nd, link)->csdev;
>>> -            ret = coresight_enable_link(csdev, parent, child);
>>> +            ret = coresight_enable_link(csdev, parent, child, source);
>>>               if (ret)
>>>                   goto err;
>>>               break;
>>> @@ -468,7 +511,7 @@ 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, source);
>>>       goto out;
>>>   }
>>>   @@ -619,6 +662,7 @@ static void coresight_drop_device(struct 
>>> coresight_device *csdev)
>>>    * @csdev:    The device to start from.
>>>    * @sink:    The final sink we want in this path.
>>>    * @path:    The list to add devices to.
>>> + * @source:    The trace source device of the path.
>>>    *
>>>    * The tree of Coresight device is traversed until @sink is found.
>>>    * From there the sink is added to the list along with all the 
>>> devices that led
>>> @@ -627,7 +671,8 @@ static void coresight_drop_device(struct 
>>> coresight_device *csdev)
>>>    */
>>>   static int _coresight_build_path(struct coresight_device *csdev,
>>>                    struct coresight_device *sink,
>>> -                 struct list_head *path)
>>> +                 struct list_head *path,
>>> +                 struct coresight_device *source)
>>
>> minor nit: Please could we reorder the parameter order :
>>
>> _coresight_build_path(csdev, source, sink, path) ?
>>
>> That makes it much better to read : build a path from "source" to 
>> "sink", from "csdev"
> All right, I will update in the next version.
>>
>>>   {
>>>       int i, ret;
>>>       bool found = false;
>>> @@ -639,7 +684,7 @@ static int _coresight_build_path(struct 
>>> coresight_device *csdev,
>>>         if (coresight_is_percpu_source(csdev) && 
>>> coresight_is_percpu_sink(sink) &&
>>>           sink == per_cpu(csdev_sink, 
>>> source_ops(csdev)->cpu_id(csdev))) {
>>> -        if (_coresight_build_path(sink, sink, path) == 0) {
>>> +        if (_coresight_build_path(sink, sink, path, source) == 0) {
>>>               found = true;
>>>               goto out;
>>>           }
>>> @@ -650,8 +695,13 @@ static int _coresight_build_path(struct 
>>> coresight_device *csdev,
>>>           struct coresight_device *child_dev;
>>>             child_dev = csdev->pdata->out_conns[i]->dest_dev;
>>> +
>>> +        if (csdev->pdata->out_conns[i]->filter_src_dev
>>> +            && (csdev->pdata->out_conns[i]->filter_src_dev != source))
>>> +            continue;
>>
>> Please reuse coresight_block_source(). i.e.,
>>
>> if (coresight_block_source(source, csdev->pdata->cout_conns[i]))
>>     continue;
> OK, I will update in the next version.
>>
>>> +
>>>           if (child_dev &&
>>> -            _coresight_build_path(child_dev, sink, path) == 0) {
>>> +            _coresight_build_path(child_dev, sink, path, source) == 
>>> 0) {
>>>               found = true;
>>>               break;
>>>           }
>>> @@ -696,7 +746,7 @@ struct list_head *coresight_build_path(struct 
>>> coresight_device *source,
>>>         INIT_LIST_HEAD(path);
>>>   -    rc = _coresight_build_path(source, sink, path);
>>> +    rc = _coresight_build_path(source, sink, path, source);
>>>       if (rc) {
>>>           kfree(path);
>>>           return ERR_PTR(rc);
>>> @@ -924,6 +974,16 @@ static int coresight_orphan_match(struct device 
>>> *dev, void *data)
>>>       for (i = 0; i < src_csdev->pdata->nr_outconns; i++) {
>>>           conn = src_csdev->pdata->out_conns[i];
>>>   +        /* Fix filter source device before skip the port */
>>> +        if ((conn->filter_src_fwnode) && dst_csdev
>>
>> minor nit: unnecessary () around conn->filter_src_fwnode
> OK, I will update in the next version.
>>
>>> +            && (conn->filter_src_fwnode == dst_csdev->dev.fwnode)) {
>>> +            if (dst_csdev->type == CORESIGHT_DEV_TYPE_SOURCE)
>>> +                conn->filter_src_dev = dst_csdev;
>>> +            else
>>> +                dev_warn(&conn->filter_src_dev->dev,
>>> +                  "Filter source is not a source device\n");
>>> +        }
>>
>> We could set the still_orphan here instead of down below.
>>
>>     /* Fixup filter source link */
>>     if (conn->filter_src_fwnode && !conn->filter_src_dev) {
>>         if (dst_csdev &&
>>             conn->filter_src_fwnode == dst_csdev->dev.fwnode &&
>>             !WARN_ON_ONCE(dst_csdev->type != CORESIGHT_DEV_TYPE_SOURCE) {
>>             conn->filter_src_dev = dst_csdev;
>>         else
>>             still_orphan = true;
>>     }
>>
>> minor nit: Also I think it is high time we add a helper to check if a
>> devices is SOURCE, something like we did for is_device_tpdm(). e.g.,
>>
>>
>> static inline bool coresight_is_device_source(struct coresight_device 
>> *csdev)
>> {
>>     return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE);
>> }
> Could you help review the following approach?
> 
> I will add the helper to coresight-priv.h
> 
> static inline bool coresight_is_device_source(struct coresight_device 
> *csdev)
> {
>      return (csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE));

minor nit:

	return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE);

> }
> 
> Then, calling the help to check if a device is SOURCE in 
> coresight-platform.c and coresight-core.c

and evert where you compare for SOURCE.

> 
> coresight-platform.c
> 
> ... ... ... ...
> 
>          else {
>              conn.filter_src_dev =
>   coresight_find_csdev_by_fwnode(conn.filter_src_fwnode);
>              if (conn.filter_src_dev &&
>                  !coresight_is_device_source(conn.filter_src_dev))
>                  dev_warn(&conn.filter_src_dev->dev,
>                    "Filter source is not a source device\n");
>          }
> 
> coresight-core.c
> 
> ... ... ... ...
> 
>          if (conn->filter_src_fwnode && !conn->filter_src_dev) {
>              if (dst_csdev && (conn->filter_src_fwnode == 
> dst_csdev->dev.fwnode)
>                  && !WARN_ON_ONCE(!coresight_is_device_source(dst_csdev)))
>                  conn->filter_src_dev = dst_csdev;
>              else
>                  still_orphan = true;
>          }
> 

Correct.

Thanks for your patience. Please wait until we sort out the device tree
bindings

Suzuki

[PATCH v3 3/3] coresight-tpda: Optimize the function of reading element size

[Tao Zhang]

Since the new funnel device supports multi-port output scenarios,
there may be more than one TPDM connected to one TPDA. In this
way, when reading the element size of the TPDM, TPDA driver needs
to find the expected TPDM corresponding to the filter source.
When TPDA finds a TPDM or a filter source from a input connection,
it will read the Devicetree to get the expected TPDM's element
size.

Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
---
 drivers/hwtracing/coresight/coresight-tpda.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index bfca103f9f84..750323c35c5b 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -110,6 +110,16 @@ static int tpda_get_element_size(struct tpda_drvdata *drvdata,
 		    csdev->pdata->in_conns[i]->dest_port != inport)
 			continue;
 
+		/*
+		 * If this port has a hardcoded filter, use the source
+		 * device directly.
+		 */
+		if (csdev->pdata->in_conns[i]->filter_src_fwnode) {
+			in = csdev->pdata->in_conns[i]->filter_src_dev;
+			if (!in)
+				continue;
+		}
+
 		if (coresight_device_is_tpdm(in)) {
 			if (drvdata->dsb_esize || drvdata->cmb_esize)
 				return -EEXIST;
@@ -124,7 +134,6 @@ static int tpda_get_element_size(struct tpda_drvdata *drvdata,
 		}
 	}
 
-
 	return rc;
 }
 
-- 
2.17.1