[PATCH 2/4] coresight: Add support for multiple output ports on the funnel
Suzuki K Poulose
suzuki.poulose at arm.com
Thu Apr 18 01:59:41 PDT 2024
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 at arm.com> 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 at arm.com> 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 at xxxxxxx {
>>>>>>>>> ... ... ... ...
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> funnel_XXX: funnel at xxxxxxx {
>>>>>>>>> ... ... ... ...
>>>>>>>>> out-ports {
>>>>>>>>> ... ... ... ...
>>>>>>>>> port at x {
>>>>>>>>> ... ... ... ...
>>>>>>>>> label = "xxxxxxx.tpdm"; <-- To label the source
>>>>>>>>> }; corresponding to the output
>>>>>>>>> ... ... ... ... connection "port at 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.
>
> 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 at 0x1234000 {
> compatible = "compound-funnel"
> regs = < 0x1234000 0x1000>
> sub-funnel at 0 {
> in-ports {
> [some port definitions]
> }
> out-ports {
> [some port definitions]
> }
> }
> sub-funnel at 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.
>
>
> 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 at 0 {
>>>> ...
>>>> <hard-wired-to*> = &component_output_port0;
>>>> };
>>>> ...
>>>> };
>>>>
>>>> output_ports {
>>>> componentne_output_port0: port at 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 at quicinc.com>
>>>>>>>>> ---
>>>>>>>>> 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;
>>>>>>>>
>>>>>>
>>>>
>>>
>>>
>>
>
>
More information about the linux-arm-kernel
mailing list