| Message ID | 20240821031348.6837-1-quic_taozha@quicinc.com |
|---|---|
| Headers | show |
| Series | source filtering for multi-port output | expand |
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
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
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
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(-)