[PATCH 2/4] coresight: Add support for multiple output ports on the funnel
Suzuki K Poulose
suzuki.poulose at arm.com
Wed Apr 17 02:21:08 PDT 2024
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.
>
> 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.
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