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