On Wed, 15 May 2019 at 09:30, Mike Leach mike.leach@linaro.org wrote:
Hi Mathieu,
On Tue, 14 May 2019 at 17:41, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Wed, May 01, 2019 at 09:49:37AM +0100, Mike Leach wrote:
The CoreSight subsystem enables a path of devices from source to sink. Any CTI devices associated with the path devices must be enabled at the same time.
This patch adds an associated device element to the main coresight device structure, and uses this to create associations between the CTI and other devices based on the device tree data. The assoc dev element is used to enable CTI in conjunction with the path elements. CTI are reference counted so where a single CTI is associated with multiple elements on the path, it will be enabled on the first associated device enable, and disabled with the last associated device disable.
Signed-off-by: Mike Leach mike.leach@linaro.org
drivers/hwtracing/coresight/coresight-cti.c | 90 ++++++++++++++++++++ drivers/hwtracing/coresight/coresight-priv.h | 3 + drivers/hwtracing/coresight/coresight.c | 39 ++++++++- include/linux/coresight.h | 4 + 4 files changed, 135 insertions(+), 1 deletion(-)
[snip]
static int coresight_enable_sink(struct coresight_device *csdev, u32 mode, void *data) { @@ -227,6 +252,9 @@ static int coresight_enable_sink(struct coresight_device *csdev, */ if (sink_ops(csdev)->enable) { ret = sink_ops(csdev)->enable(csdev, mode, data);
if (ret)return ret;ret = coresight_control_assoc_ectdev(csdev, 1); if (ret) return ret;In case of error the sink is left enabled.
good point - I'll have to recheck this - though if an error is seen in the "enable path" code in the coresight enable path process, does the "disable path" not unwind everything anyway?
The sink is not - see comment here [1]. Sources are also special since they aren't handled by the core [2]. I think handling of CTI devices should look atomic to the framework, i.e both the device and CTI companion are enabled or both have failed.
[1]. https://elixir.bootlin.com/linux/latest/source/drivers/hwtracing/coresight/c... [2]. https://elixir.bootlin.com/linux/latest/source/drivers/hwtracing/coresight/c...
Also, wouldn't it be best to enable the ect device first and then the sink? Otherwise if the sink wrongly emits ECT signals when enabled we'll catch the condition all the time. In the case where the ECT is enabled after the sink we will never know if the wrongly emitted signals will be caught, leading to intermittent behavior.
May not have a choice - CTIs can be connected to multiple devices, so any given CTI - especially ones not bound to cores can be enabled by association with any of these devices. The CTI enable process has a refcount - enable on first enable request, disable on last disable request.
When enabling a CTI that is connected to a data device, is it possible to enable only the in/out ports for that device and enable the ports for other data devices when those are enabled? If the HW supports it function cti_write_all_hw_regs() would have to be redesigned.
I would tend to favour a sink / associated device first - CTI second model as programming / enable of the associated device should in general be done in such a way as to not have triggers active on enable. If enabling a sink triggers some output signals - that are typically FULL or ACQCOMP, then that is either a sink programming error, or we may have not seen a valid signal the last time we were enabled.
Exactly - and we won't know about these conditions if the associated device is enabled first and the CTI second. Let me know if I'm really missing the point.
Thanks, Mathieu
csdev->enable = true;@@ -242,6 +270,7 @@ static void coresight_disable_sink(struct coresight_device *csdev) if (atomic_dec_return(csdev->refcnt) == 0) { if (sink_ops(csdev)->disable) { sink_ops(csdev)->disable(csdev);
coresight_control_assoc_ectdev(csdev, 0); csdev->enable = false; } }@@ -275,6 +304,8 @@ static int coresight_enable_link(struct coresight_device *csdev, if (atomic_inc_return(&csdev->refcnt[refport]) == 1) { if (link_ops(csdev)->enable) { ret = link_ops(csdev)->enable(csdev, inport, outport);
if (!ret)ret = coresight_control_assoc_ectdev(csdev, 1); if (ret) { atomic_dec(&csdev->refcnt[refport]); return ret;Same comment as above.
@@ -314,8 +345,10 @@ static void coresight_disable_link(struct coresight_device *csdev, }
if (atomic_dec_return(&csdev->refcnt[refport]) == 0) {
if (link_ops(csdev)->disable)
if (link_ops(csdev)->disable) { link_ops(csdev)->disable(csdev, inport, outport);coresight_control_assoc_ectdev(csdev, 0);} } for (i = 0; i < nr_conns; i++)@@ -338,6 +371,8 @@ static int coresight_enable_source(struct coresight_device *csdev, u32 mode) if (!csdev->enable) { if (source_ops(csdev)->enable) { ret = source_ops(csdev)->enable(csdev, NULL, mode);
if (!ret)ret = coresight_control_assoc_ectdev(csdev, 1); if (ret) return ret;Same comment as above.
}@@ -362,6 +397,7 @@ static bool coresight_disable_source(struct coresight_device *csdev) if (atomic_dec_return(csdev->refcnt) == 0) { if (source_ops(csdev)->disable) source_ops(csdev)->disable(csdev, NULL);
coresight_control_assoc_ectdev(csdev, 0); csdev->enable = false; } return !csdev->enable;@@ -1238,6 +1274,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
coresight_fixup_device_conns(csdev); coresight_fixup_orphan_conns(csdev);
cti_add_assoc_to_csdev(csdev); mutex_unlock(&coresight_mutex);diff --git a/include/linux/coresight.h b/include/linux/coresight.h index d1c1ed17d2ca..50be479c35d1 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -167,6 +167,8 @@ struct coresight_connection {
activated but not yet enabled. Enabling for a _sink_appens when a source has been selected for that it.- @ea: Device attribute for sink representation under PMU directory.
- @ect_dev: Associated cross trigger device. Not part of the trace data
*/
path or connections.struct coresight_device { struct coresight_connection *conns; @@ -182,6 +184,8 @@ struct coresight_device { /* sink specific fields */ bool activated; /* true only if a sink is part of a path */ struct dev_ext_attribute *ea;
/* cross trigger handling */struct coresight_device *ect_dev;I am still not sure how to treat ECT devices, i.e should they become a special helper device or be considered a separate class of device. They are a separate class since they deal with signals rather than data, but they could easily re-use the infrastructure put in place for data devices. I haven't taken a position on this yet so just keep going with the current scheme. I expect the decision will get easier to take as this set is refined over iterations.
I considered them separate from the data devices as the topology is completely different. The trace data devices have an end to end, one way flow of data. This has limit programmability in the way that data flows - i.e. you can choose your sink, but not much else, and downstream components cannot affect upstream components. ECT is a star topology, with highly programmable flexibility - with the correct configuration an ETM trace event can trigger a sink to stop collecting trace, or a sink full event can stop generation. When you add that to the possibility of the hooks into STM hardware signal triggers, and none CS hardware such as ELA, system profilers & clocks (see the Juno .dtsi for connections such as these) it becomes clear that the functionality does not fit easily into the existing data flow infrastructure.
My initial development tried to re-use more of the existing infrastructure but it just got increasingly complex & difficult.
};
#define to_coresight_device(d) container_of(d, struct coresight_device, dev)
2.20.1
Regards
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK