Re: [PATCH 2/4] coresight: Add support for multiple output ports on the funnel

Tingwei Zhang <quic_tingweiz@xxxxxxxxxxx> · Thu, 18 Apr 2024 20:01:02 +0800

Hi Mike and Suzuki

On 4/18/2024 5:25 PM, Mike Leach wrote:
Hi Suzuki

-----Original Message-----
From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
Sent: Thursday, April 18, 2024 10:00 AM
To: Mike Leach <mike.leach@xxxxxxxxxx>
Cc: Tao Zhang <quic_taozha@xxxxxxxxxxx>; Mathieu Poirier
<mathieu.poirier@xxxxxxxxxx>; Alexander Shishkin
<alexander.shishkin@xxxxxxxxxxxxxxx>; Konrad Dybcio
<konradybcio@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof
Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Jinlong Mao
<quic_jinlmao@xxxxxxxxxxx>; Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx>; coresight@xxxxxxxxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
devicetree@xxxxxxxxxxxxxxx; Tingwei Zhang <quic_tingweiz@xxxxxxxxxxx>;
Yuanfang Zhang <quic_yuanfang@xxxxxxxxxxx>; Trilok Soni
<quic_tsoni@xxxxxxxxxxx>; Song Chai <quic_songchai@xxxxxxxxxxx>; linux-arm-
msm@xxxxxxxxxxxxxxx; andersson@xxxxxxxxxx
Subject: Re: [PATCH 2/4] coresight: Add support for multiple output ports on the
funnel

Hi Mike

On 18/04/2024 09:48, Mike Leach wrote:
Hi Suzuki

On Wed, 17 Apr 2024 at 10:21, Suzuki K Poulose <suzuki.poulose@xxxxxxx>
wrote:

Hi Mike

On 16/04/2024 15:19, Mike Leach wrote:
Hi,

On Mon, 15 Apr 2024 at 14:24, Suzuki K Poulose <suzuki.poulose@xxxxxxx>
wrote:

On 09/04/2024 14:22, Tao Zhang wrote:

On 4/9/2024 3:13 PM, Suzuki K Poulose wrote:
Hi

On 29/03/2024 09:27, Tao Zhang wrote:

On 3/22/2024 12:41 AM, Suzuki K Poulose wrote:
On 21/03/2024 08:32, Tao Zhang wrote:
Funnel devices are now capable of supporting multiple-inputs
and multiple-outputs configuration with in built hardware
filtering for TPDM devices. Add software support to this
function. Output port is selected according to the source in the trace
path.

The source of the input port on funnels will be marked in the
device tree.
e.g.
tpdm@xxxxxxx {
        ... ... ... ...
};

funnel_XXX: funnel@xxxxxxx {
        ... ... ... ...
        out-ports {
            ... ... ... ...
            port@x {
                ... ... ... ...
                label = "xxxxxxx.tpdm"; <-- To label the source
            };                           corresponding to the output
        ... ... ... ...                  connection "port@x". And this
        };                               is a hardware static
connections.
        ... ... ... ...                  Here needs to refer to hardware
};                                   design.

Then driver will parse the source label marked in the device
tree, and save it to the coresight path. When the function
needs to know the source label, it could obtain it from coresight path
parameter.
Finally,
the output port knows which source it corresponds to, and it
also knows which input port it corresponds to.

Why do we need labels ? We have connection information for all
devices (both in and out), so, why do we need this label to find a device
?

Because our funnel's design has multi-output ports, the data
stream will not

know which output port should pass in building the data trace path.
This source

label can make the data stream find the right output port to go.


And also, I thought TPDM is a source device, why does a funnel
output port link to a source ?

No, this label doesn't mean this funnel output port link to a
source, it just let

the output port know its data source.


Are these funnels programmable ? Or, are they static ? If they
are static, do these need to be described in the DT ? If they
are simply acting as a "LINK" (or HWFIFO ?)

These funnels are static, and we will add the "label" to the DT
to describe the

multi-output ports for these funnels.

I think there is still a bit of confusion. By "Dynamic" I mean,
the "dynamic funnel" (explicit port enablement via MMIO) vs
"static funnel" (no programming, always ON).

So, coming to your example, do we need to "explicitly" enable
trace flow for an "input" and/or an "output" port in your "funnel" ?

Sorry for my misunderstanding in the previous mails. Our funnels
are programmable just like the common dynamic funnels.

In our solution, we just make funnels have multiple output ports
connected to different devices or ports. When we use it, we still

enable the input port through programming. Our solution is to know
which input port the expected data comes from based on the

source label corresponding to the output port. This way we can
build the expected trace path. In other respects, it is used the
same

as common dynamic funnels.


Ok. So, to summarise :

1. This is not a standard Funnel, but a trace link with multiple-input
       and multiple-output, with inputs hardwired to an outline at
       integration.
2. The programming model is same as that of a "standard funnel".

Now, we do have enough information in the coresight_connections to
traverse input/output ports. But we need additional logic to
"hardwire" the ports to each other and necessary logic to handle
the

There are two options here :

1. Treat this as a new component and have its own driver, with
       additional logic to handle the input/output wiring.

2. Drive it using the funnel driver, with a a new compatible and
       add additional logic to handle the input/output wiring.

My inclination is towards (2), we need to see how this works out.

We need to irrespective of the options, we need special handling
for hardwired ports in 1) building path 2) walking back the path
(in TPDA driver)

We also need some "DT" information to bind a given input port to an
output port. We must not use "any device" labels to hack this up,
like the approach in this series.


Given that the internal connections are static for the given device,
then the compatible will imply these connections in just the same
way as the arm,coresight-funnel implies that all inputs are
connected to the single output.

I am sorry, I couldn't follow the last part. We have two or more
output ports and we need a way to identify, which input port is
hardwired to
output-port0 and output-port1. Given we need special handling for
these anyway, I would like to avoid hard coding the input-output connection.
i.e., we do not want to assume that input-0  is always => output-0.


If we regard the current component as having compatible
"qcom,coresight-compound-funnel-v1", then this identifies the
relationship between the in-ports and out-ports.
So the new driver / extension to the funnel driver that handles this
compatible with know the static mapping between input and output so
program it.

Ok, but like I said, having one compatible may not be enough to know the "static"
mapping for all possible device instances on different SoCs.


The compatible name would have to change if the mapping changed.
Using the compatible is simpler, but less flexible


If however you want a more generic approach to handle future different
versions of the component, then of course a method in DT of mapping
in-ports to out-ports is useful.

If did wonder if something along the lines of:-

compound-funnel@0x1234000 {
       compatible = "compound-funnel"
       regs = < 0x1234000 0x1000>
        sub-funnel@0 {
                   in-ports {
                          [some port definitions]
                    }
                   out-ports {
                         [some port definitions]
                    }
         }
        sub-funnel@1 {
                   in-ports {
                          [some port definitions]
                    }
                   out-ports {
                         [some port definitions]
                   }
          }
}

That would work, with "two" different coresight devices for each "embedded
funnel". And that also confuses the user with the topology.

I wasn't suggesting two different coresight devices, but finding a way to iterate the sub-nodes to create a single device with the inputs mapped to outputs. Which may or may not be easily do-able.

As to topology - no more confusing than a "funnel" with multiple outputs, or phandle links between inputs and outputs. It does visually represent what the device really is - multiple effective funnels controlled by a single set of registers.


Let me provide the hardware topology here to facilitate the discussion.

|----------|     |---------|     |----------|   |---------|
|  TPDM 0  |     | Source0 |     | Source 1 |   | TPDM 1  |
|----------|     |---------|     |----------|   |---------|
     |                |                |             |
     |                |                |             |
     |      --------- |                |             |
     |      |                          |             |
     |      |                          |             |
     |      |                          |             |
  \-------------/ ----------------------             |
   \  Funnel 0 /  |                                  |
    -----------   |  ---------------------------------
         |        |  |
         |        |  |
       \ -------------/
        \  Funnel 1  /
         -----------/
            |  |---------------------
            |  |------------         |
            |               |TPDM0   |TPDM1
            |            \ ----------------/
            |             \   TPDA 0      /
            |              --------------/
            |                    |
            |                    |
            |Source0/1           |
         \-------------------------------/
          \     Funnel 2                /
            ---------------------------

To describe this topology in device tree, we need to indicate input 
port0 of FUNNEL0 is static link to output port0 of FUNNEL0 which links 
to input port0 of TPDA0. When code builds the path, it can get the 
static link information from topology and select correct path. As Suzuki 
has suggested, we can describe the topology like below.

funnel0 {
	...
	in-ports {
		port@0 {
			funnel0_in0: endpoint {
				remote-endpoint = <&tpdm0_output>;
				<hard-wired-to*>=
				  <&funnel1_out0>;
			}
		}
	}
}

funnel1 {
	...
	out-ports {
		port@0 {
			funnel1_out0: endpoint {
				remote-endpoint= <&tpdm0_in0>;
				<hard-wired-to*>=
					<&funnel0_in0>;
			}
		}
	}
}


Mike




might be made to work? not sure about the implications of having more
that one set of in-ports and out-ports in a component in the device
tree & would need specific handling in the driver to iterate
sub-funnels.




Irrespective of if a new driver is used, or an extension to the
current funnel driver to handle a new compatible - the mapping
between input and output ports are created based on the compatible..

As we are building a path from source to sink, what is then needed
is a method in the generic path building code, to recognise these
amppings and filter the output ports that are searched based on the
input port in use.

Agreed. We could mark this as a property of the
port/coresight_connection.


On standard components, where the mapping is not present, then the
code will continue as it does now, for these compound funnels, the
mappings will be present and the output filtering will occur.

Agreed

This removes the need for the labels / extra connection attributes
on devices other than the funnel, and also removes the need to
specify the internal connections as part of the device tree.

I am still not clear how we map the input-output ports. Rest is what
exactly I had in mind. So, once we sort out the port mapping we could
proceed to the prototyping.


given we iterate by output port index into an array of out ports, and
have an array of in-ports by index, a third mapping array, same size
as in-ports, determining the associated out port index should suffice.
Mapping array should be optional - if not there, path discovery works
as previously

We could simply add a "(sticky)flag" to the
"coresight_connection".src_port/dest_port and extend the array to include the
sticky_port for src/dest port and use that flag to force the path traversal.

Suzuki




Regards

Mike

Kind regards
Suzuki



Regards

Mike

Rob/Krzysztof,

Do you have any recommendations for describing the 'hard wired
ports' ?

e.g:

component {
       input_ports {
          component_input_port0: port@0 {
              ...
              <hard-wired-to*> = &component_output_port0;
          };
          ...
      };

      output_ports {
        componentne_output_port0: port@0 {
            ...
            <hard-wired-to> = &component_input_port0;
        };
        ...
      };

};

*Need a better suitable property than "hard-wired-to".


Suzuki




Best,

Tao




"If they are simply acting as a "LINK" (or HWFIFO ?) " I'm not
sure what's the meaning

i.e, Like TMC-ETF in HWFIFO mode. In this mode, the TMC-ETF is
acting like a cache for easing ATB data load, by providing h/w buffering.
(In your case, it may not be providing any buffering, it doesn't
matter either way, as it is not visible to the driver).

Suzuki


of this. Could you describe it in detail?


Best,

Tao


Suzuki


Signed-off-by: Tao Zhang <quic_taozha@xxxxxxxxxxx>
---
     drivers/hwtracing/coresight/coresight-core.c  | 81
++++++++++++++++---
     .../hwtracing/coresight/coresight-platform.c  |  5 ++
     include/linux/coresight.h                     |  2 +
     3 files changed, 75 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c
b/drivers/hwtracing/coresight/coresight-core.c
index 5dde597403b3..b1b5e6d9ec7a 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -113,15 +113,63 @@ 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_source_filter - checks whether the connection
+matches
the source
+ * of path if connection is binded to specific source.
+ * @path:    The list of devices
+ * @conn:    The connection of one outport
+ *
+ * Return zero if the connection doesn't have a source binded
+ or
source of the
+ * path matches the source binds to connection.
+ */
+static int coresight_source_filter(struct list_head *path,
+            struct coresight_connection *conn) {
+    int ret = 0;
+    struct coresight_device *source = NULL;
+
+    if (conn->source_label == NULL)
+        return ret;
+
+    source = coresight_get_source(path);
+    if (source == NULL)
+        return ret;
+
+    if (strstr(kobject_get_path(&source->dev.kobj, GFP_KERNEL),
+            conn->source_label))
+        ret = 0;
+    else
+        ret = -1;
+
+    return ret;
+}
+
     static struct coresight_connection *
     coresight_find_out_connection(struct coresight_device *src_dev,
-                  struct coresight_device *dest_dev)
+                  struct coresight_device *dest_dev,
+                  struct list_head *path)
     {
         int i;
         struct coresight_connection *conn;
           for (i = 0; i < src_dev->pdata->nr_outconns; i++) {
             conn = src_dev->pdata->out_conns[i];
+        if (coresight_source_filter(path, conn))
+            continue;
             if (conn->dest_dev == dest_dev)
                 return conn;
         }
@@ -312,7 +360,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 list_head *path)
     {
         int ret = 0;
         int link_subtype;
@@ -321,8 +370,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, path);
+    outconn = coresight_find_out_connection(csdev, child,
+ path);
         link_subtype = csdev->subtype.link_subtype;
           if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG
&&
IS_ERR(inconn))
@@ -341,7 +390,8 @@ 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 list_head *path)
     {
         int i;
         int link_subtype;
@@ -350,8 +400,8 @@ static void coresight_disable_link(struct
coresight_device *csdev,
         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, path);
+    outconn = coresight_find_out_connection(csdev, child,
+ path);
         link_subtype = csdev->subtype.link_subtype;
           if (link_ops(csdev)->disable) { @@ -507,7 +557,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,
+ path);
                 break;
             default:
                 break;
@@ -588,7 +638,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,
+ path);
                 if (ret)
                     goto err;
                 break;
@@ -802,7 +852,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;
@@ -814,7 +865,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;
             }
@@ -825,8 +876,12 @@ 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]->source_label &&
+ !strstr(kobject_get_path(&source->dev.kobj, GFP_KERNEL),
+ csdev->pdata->out_conns[i]->source_label))
+            continue;
             if (child_dev &&
-            _coresight_build_path(child_dev, sink, path) == 0) {
+            _coresight_build_path(child_dev, sink, path,
+ source)
== 0) {
                 found = true;
                 break;
             }
@@ -871,7 +926,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); diff --git
a/drivers/hwtracing/coresight/coresight-platform.c
b/drivers/hwtracing/coresight/coresight-platform.c
index 9d550f5697fa..f553fb20966d 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -205,6 +205,7 @@ static int
of_coresight_parse_endpoint(struct
device *dev,
         struct fwnode_handle *rdev_fwnode;
         struct coresight_connection conn = {};
         struct coresight_connection *new_conn;
+    const char *label;
           do {
             /* Parse the local port details */ @@ -243,6
+244,10 @@ static int of_coresight_parse_endpoint(struct
device *dev,
             conn.dest_fwnode = fwnode_handle_get(rdev_fwnode);
             conn.dest_port = rendpoint.port;
     +        conn.source_label = NULL;
+        if (!of_property_read_string(ep, "label", &label))
+            conn.source_label = label;
+
             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 e8b6e388218c..a9c06ef9bbb2
100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -167,6 +167,7 @@ struct coresight_desc {
      * struct coresight_connection - representation of a
single connection
      * @src_port:    a connection's output port number.
      * @dest_port:    destination's input port number @src_port is
connected to.
+ * @source_label: source component's label.
      * @dest_fwnode: destination component's fwnode handle.
      * @dest_dev:    a @coresight_device representation of the
component
             connected to @src_port. NULL until the device is
created @@ -195,6 +196,7 @@ struct coresight_desc {
     struct coresight_connection {
         int src_port;
         int dest_port;
+    const char *source_label;
         struct fwnode_handle *dest_fwnode;
         struct coresight_device *dest_dev;
         struct coresight_sysfs_link *link;









_______________________________________________
CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx To unsubscribe send an email to
coresight-leave@xxxxxxxxxxxxxxxx
_______________________________________________
CoreSight mailing list -- coresight@xxxxxxxxxxxxxxxx
To unsubscribe send an email to coresight-leave@xxxxxxxxxxxxxxxx

--
Thanks,
Tingwei








