On 2/1/21 11:17 PM, Mathieu Poirier wrote:

Hi Anshuman,

I have started reviewing this set. As it is quite voluminous comments will come over serveral days. I will let you know when I am done.

On Wed, Jan 27, 2021 at 02:25:25PM +0530, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

When there are multiple sinks on the system, in the absence of a specified sink, it is quite possible that a default sink for an ETM could be different from that of another ETM. However we do not support having multiple sinks for an event yet. This patch allows the event to use the default sinks on the ETMs where they are scheduled as long as the sinks are of the same type.

e.g, if we have 1x1 topology with per-CPU ETRs, the event can use the per-CPU ETR for the session. However, if the sinks are of different type, e.g TMC-ETR on one and a custom sink on another, the event will only trace on the first detected sink.

I found the above changelog very confusing - I read it several times and still couldn't get all of it. In the end this patch prevents sinks of different types from being used for session, and this is what the text should reflect.

Sorry about that. Your inference is correct, but it is only a side effect of the primary motive. How about the following :

"When a sink is not specified by the user, the etm perf driver finds a suitable sink automatically based on the first ETM, where this event could be scheduled. Then we allocate the sink buffer based on the selected sink. This is fine for a CPU bound event as the "sink" is always guaranteed to be reachable from the ETM (as this is the only ETM where the event is going to be scheduled). However, if we have a task bound event, the event could be scheduled on any of the ETMs on the system. In this case, currently we automatically select a sink and exclude any ETMs that are not reachable from the selected sink. This is problematic for 1x1 configurations as we end up in tracing the event only on the "first" ETM, as the default sink is local to the first ETM and unreachable from the rest. However, we could allow the other ETMs to trace if they all have a sink that is compatible with the "selected" sink and can use the sink buffer. This can be easily done by verifying that they are all driven by the same driver and matches the same subtype."

...

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Tested-by: Linu Cherian lcherian@marvell.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm-perf.c | 48 +++++++++++++++++++----- 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index bdc34ca..eb9e7e9 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -204,6 +204,13 @@ static void etm_free_aux(void *data) schedule_work(&event_data->work); } +static bool sinks_match(struct coresight_device *a, struct coresight_device *b) +{

• if (!a || !b)

• return false;
  

• return (sink_ops(a) == sink_ops(b));

Yes

I think we can tighten this by verifying the dev->sub_type matches too.

...

+}

• static void *etm_setup_aux(struct perf_event *event, void **pages, int nr_pages, bool overwrite) {

@@ -212,6 +219,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, cpumask_t *mask; struct coresight_device *sink = NULL;

     struct coresight_device *user_sink = NULL;

...

struct etm_event_data *event_data = NULL;

• bool sink_forced = false;

event_data = alloc_event_data(cpu); if (!event_data) @@ -222,6 +230,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, if (event->attr.config2) { id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id);

             user_sink = coresight_get_sink_by_id(id);

...

• sink_forced = true;
  

  }

mask = &event_data->mask; @@ -235,7 +244,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, */ for_each_cpu(cpu, mask) { struct list_head *path;

• struct coresight_device *csdev;
  

             struct coresight_device *last_sink = NULL;

...

• struct coresight_device *csdev, *new_sink;
  

csdev = per_cpu(csdev_src, cpu); /* @@ -249,21 +258,35 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, } /*

•  * No sink provided - look for a default sink for one of the
  

•  * devices. At present we only support topology where all CPUs
  

•  * use the same sink [N:1], so only need to find one sink. The
  

•  * coresight_build_path later will remove any CPU that does not
  

•  * attach to the sink, or if we have not found a sink.
  

•  * No sink provided - look for a default sink for all the devices.
  

•  * We only support multiple sinks, only if all the default sinks
  

•  * are of the same type, so that the sink buffer can be shared
  

•  * as the event moves around. We don't trace on a CPU if it can't
  

s/can't/can't./

...

•  *
  

Extra line

OK

...

 */

• if (!sink)
  

• 	sink = coresight_find_default_sink(csdev);
  

• if (!sink_forced) {
  

• 	new_sink = coresight_find_default_sink(csdev);
  

• 	if (!new_sink) {
  

• 		cpumask_clear_cpu(cpu, mask);
  

• 		continue;
  

• 	}
  

• 	/* Skip checks for the first sink */
  

• 	if (!sink) {
  

• 	       sink = new_sink;
  

• 	} else if (!sinks_match(new_sink, sink)) {
  

• 		cpumask_clear_cpu(cpu, mask);
  

• 		continue;
  

• 	}
  

• } else {
  

• 	new_sink = sink;
  

• }
  

             if (!user_sink) {
                     /* find default sink for this CPU */
                     sink = coresight_find_default_sink(csdev);
                     if (!sink) {
                             cpumask_clear_cpu(cpu, mask);
                             continue;
                     }

                     /* Chech new sink with last sink */
                     if (last_sink && !sink_match(last_sink, sink)) {
                             cpumask_clear_cpu(cpu, mask);
                             continue;
                     }

                     last_sink = sink;
             } else {
                     sink = user_sink;
             }

Agreed, it is much better readable.

Suzuki

On 2/2/21 4:33 PM, Mike Leach wrote:

Hi,

On Tue, 2 Feb 2021 at 09:42, Suzuki K Poulose suzuki.poulose@arm.com wrote:

...

On 2/1/21 11:17 PM, Mathieu Poirier wrote:

...

Hi Anshuman,

I have started reviewing this set. As it is quite voluminous comments will come over serveral days. I will let you know when I am done.

On Wed, Jan 27, 2021 at 02:25:25PM +0530, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

When there are multiple sinks on the system, in the absence of a specified sink, it is quite possible that a default sink for an ETM could be different from that of another ETM. However we do not support having multiple sinks for an event yet. This patch allows the event to use the default sinks on the ETMs where they are scheduled as long as the sinks are of the same type.

e.g, if we have 1x1 topology with per-CPU ETRs, the event can use the per-CPU ETR for the session. However, if the sinks are of different type, e.g TMC-ETR on one and a custom sink on another, the event will only trace on the first detected sink.

I found the above changelog very confusing - I read it several times and still couldn't get all of it. In the end this patch prevents sinks of different types from being used for session, and this is what the text should reflect.

Sorry about that. Your inference is correct, but it is only a side effect of the primary motive. How about the following :

"When a sink is not specified by the user, the etm perf driver finds a suitable sink automatically based on the first ETM, where this event could be scheduled. Then we allocate the sink buffer based on the selected sink. This is fine for a CPU bound event as the "sink" is always guaranteed to be reachable from the ETM (as this is the only ETM where the event is going to be scheduled). However, if we have a task bound event, the event could be scheduled on any of the ETMs on the system. In this case, currently we automatically select a sink and exclude any ETMs that are not reachable from the selected sink. This is problematic for 1x1 configurations as we end up in tracing the event only on the "first" ETM, as the default sink is local to the first ETM and unreachable from the rest. However, we could allow the other ETMs to trace if they all have a sink that is compatible with the "selected" sink and can use the sink buffer. This can be easily done by verifying that they are all driven by the same driver and matches the same subtype."

Not sure that the logic here makes total sense - I can't see _why_ multiple sinks need to be of the same type.

Because we have a single "sink_config" (read, single sink specific buffer) for an event. i.e, we do the sink_ops->alloc_buffer() only once and rightly so. This allocates any buffers that is used by a given sink. e.g, for ETR it allocates an etr_perf_buffer. Now if we wanted the same event to run on an ETM with TRBE, the TRBE doesn't have any buffer set up to collect the trace and cant make any sense of etr_perf_buffer. However, if there is another ETM with a different ETR, the second ETR can make sense of the sink_config (etr_perf_buffer) and trace the event. Please remember that this only applies to task bound events where the event can be scheduled on different ETMs.

1. This patch is designed to allow multiple sinks to be used in a 1:1

topology system - but there is no specific restriction here - and N:M should work on the same basis

Yes, this should work in any topology.

2. This implies that multiple sinks will work within the coresight

infrastucture.

I am afraid I don't understand the context here.

3. The sink interface -> struct coresight_ops_sink allows sinks to be

abstracted - therefore whichever sink is chosen the coresight infrastructure calls the operations for the given sink.

Correct. The patch is trying to ensure that a private data setup by one driver is not interpreted by another driver as its own private data. (the private data being sink_config)

4. Each individual sink, will have its own hardware buffer - copied

into the perf buffers at some appropriate point.

Correct. Supporting multiple types of sinks for a single event is complex and not worth the benefit of the extra complexity. Moreover we don't expect sane systems to have such a configuration.

Thus if the users specifies a selected sink - we need to eliminate any source that cannot reach it.

Yes, we do that now.

If not we need to find the relevant default sink for the source, which might be a shared ETR, or per CPU TRBE / ETR, and the abstraction logic ought to handle getting the captured data to the correct place.

The abstraction logic works fine, but the per-event private data is something that makes this complex.

If it doesn't then we are on shaky ground with any multiple sink solution.

On the face of it - type is irrelevant. If I am missing something - this patch needs a better explanation.

I hope the explanation above makes it clear. Please let me know otherwise.

...

...

...

+}

• static void *etm_setup_aux(struct perf_event *event, void **pages, int nr_pages, bool overwrite) {

@@ -212,6 +219,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, cpumask_t *mask; struct coresight_device *sink = NULL;

      struct coresight_device *user_sink = NULL;

...

  struct etm_event_data *event_data = NULL;

• bool sink_forced = false;

  event_data = alloc_event_data(cpu); if (!event_data)

@@ -222,6 +230,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, if (event->attr.config2) { id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id);

              user_sink = coresight_get_sink_by_id(id);

...

•        sink_forced = true;
  

The comment for this block uses the term "selected sink", and the functions use _default_sink . This may read better if we rename the bool to "selected_sink" rather than "sink_forced"

I have removed the bool and replaced with the user_sink as Mathieu suggested.

Thanks for the review Suzuki

On Tue, Feb 02, 2021 at 09:42:34AM +0000, Suzuki K Poulose wrote:

On 2/1/21 11:17 PM, Mathieu Poirier wrote:

...

Hi Anshuman,

I have started reviewing this set. As it is quite voluminous comments will come over serveral days. I will let you know when I am done.

On Wed, Jan 27, 2021 at 02:25:25PM +0530, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

When there are multiple sinks on the system, in the absence of a specified sink, it is quite possible that a default sink for an ETM could be different from that of another ETM. However we do not support having multiple sinks for an event yet. This patch allows the event to use the default sinks on the ETMs where they are scheduled as long as the sinks are of the same type.

e.g, if we have 1x1 topology with per-CPU ETRs, the event can use the per-CPU ETR for the session. However, if the sinks are of different type, e.g TMC-ETR on one and a custom sink on another, the event will only trace on the first detected sink.

I found the above changelog very confusing - I read it several times and still couldn't get all of it. In the end this patch prevents sinks of different types from being used for session, and this is what the text should reflect.

Sorry about that. Your inference is correct, but it is only a side effect of the primary motive. How about the following :

"When a sink is not specified by the user, the etm perf driver finds a suitable sink automatically based on the first ETM, where this event could be scheduled. Then we allocate the sink buffer based on the selected sink. This is fine for a CPU bound event as the "sink" is always guaranteed to be reachable from the ETM (as this is the only ETM where the event is going to be scheduled). However, if we have a task bound event, the event could be scheduled on any of the ETMs on the system. In this case, currently we automatically select a sink and exclude any ETMs that are not reachable from the selected sink. This is problematic for 1x1 configurations as we end up in tracing the event only on the "first" ETM, as the default sink is local to the first ETM and unreachable from the rest. However, we could allow the other ETMs to trace if they all have a sink that is compatible with the "selected" sink and can use the sink buffer. This can be easily done by verifying that they are all driven by the same driver and matches the same subtype."

Much better, thanks for the rework.

...

...

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Tested-by: Linu Cherian lcherian@marvell.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm-perf.c | 48 +++++++++++++++++++----- 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index bdc34ca..eb9e7e9 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -204,6 +204,13 @@ static void etm_free_aux(void *data) schedule_work(&event_data->work); } +static bool sinks_match(struct coresight_device *a, struct coresight_device *b) +{

• if (!a || !b)

• return false;
  

• return (sink_ops(a) == sink_ops(b));

Yes

I think we can tighten this by verifying the dev->sub_type matches too.

We could do that but I'm not sure we need to. I remember spending a few minutes yesterday thinking about ways to make the test more stringent but in the end I thought what you had was sufficient, at least for now. I'll leave that one to you - proceed as you see fit.

...

...

+}

• static void *etm_setup_aux(struct perf_event *event, void **pages, int nr_pages, bool overwrite) {

@@ -212,6 +219,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, cpumask_t *mask; struct coresight_device *sink = NULL;

     struct coresight_device *user_sink = NULL;

...

struct etm_event_data *event_data = NULL;

• bool sink_forced = false; event_data = alloc_event_data(cpu); if (!event_data)

@@ -222,6 +230,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, if (event->attr.config2) { id = (u32)event->attr.config2; sink = coresight_get_sink_by_id(id);

             user_sink = coresight_get_sink_by_id(id);

...

• sink_forced = true;
  

  } mask = &event_data->mask;

@@ -235,7 +244,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, */ for_each_cpu(cpu, mask) { struct list_head *path;

• struct coresight_device *csdev;
  

             struct coresight_device *last_sink = NULL;

...

• struct coresight_device *csdev, *new_sink;
  

  csdev = per_cpu(csdev_src, cpu); /*

@@ -249,21 +258,35 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, } /*

•  * No sink provided - look for a default sink for one of the
  

•  * devices. At present we only support topology where all CPUs
  

•  * use the same sink [N:1], so only need to find one sink. The
  

•  * coresight_build_path later will remove any CPU that does not
  

•  * attach to the sink, or if we have not found a sink.
  

•  * No sink provided - look for a default sink for all the devices.
  

•  * We only support multiple sinks, only if all the default sinks
  

•  * are of the same type, so that the sink buffer can be shared
  

•  * as the event moves around. We don't trace on a CPU if it can't
  

s/can't/can't./

...

•  *
  

Extra line

OK

...

...

 */

• if (!sink)
  

• 	sink = coresight_find_default_sink(csdev);
  

• if (!sink_forced) {
  

• 	new_sink = coresight_find_default_sink(csdev);
  

• 	if (!new_sink) {
  

• 		cpumask_clear_cpu(cpu, mask);
  

• 		continue;
  

• 	}
  

• 	/* Skip checks for the first sink */
  

• 	if (!sink) {
  

• 	       sink = new_sink;
  

• 	} else if (!sinks_match(new_sink, sink)) {
  

• 		cpumask_clear_cpu(cpu, mask);
  

• 		continue;
  

• 	}
  

• } else {
  

• 	new_sink = sink;
  

• }
  

             if (!user_sink) {
                     /* find default sink for this CPU */
                     sink = coresight_find_default_sink(csdev);
                     if (!sink) {
                             cpumask_clear_cpu(cpu, mask);
                             continue;
                     }

                     /* Chech new sink with last sink */
                     if (last_sink && !sink_match(last_sink, sink)) {
                             cpumask_clear_cpu(cpu, mask);
                             continue;
                     }

                     last_sink = sink;
             } else {
                     sink = user_sink;
             }

Agreed, it is much better readable.

Suzuki

On Wed, Jan 27, 2021 at 02:25:26PM +0530, Anshuman Khandual wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

If a graph node is not found for a given node, of_get_next_endpoint() will emit the following error message :

OF: graph: no port node found in /<node_name>

If the given component doesn't have any explicit connections (e.g, ETE) we could simply ignore the graph parsing.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-platform.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 3629b78..c594f45 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -90,6 +90,12 @@ static void of_coresight_get_ports_legacy(const struct device_node *node, struct of_endpoint endpoint; int in = 0, out = 0;

• /*

• * Avoid warnings in of_graph_get_next_endpoint()
  

• * if the device doesn't have any graph connections
  

• */
  

• if (!of_graph_is_present(node))

• return;
  

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

do { ep = of_graph_get_next_endpoint(node, ep); if (!ep) -- 2.7.4

Hi Mike

On 2/2/21 11:10 AM, Mike Leach wrote:

Hi Ansuman,

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual anshuman.khandual@arm.com wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

If a graph node is not found for a given node, of_get_next_endpoint() will emit the following error message :

OF: graph: no port node found in /<node_name>

If the given component doesn't have any explicit connections (e.g, ETE) we could simply ignore the graph parsing.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-platform.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 3629b78..c594f45 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -90,6 +90,12 @@ static void of_coresight_get_ports_legacy(const struct device_node *node, struct of_endpoint endpoint; int in = 0, out = 0;

•   /*
  

•    * Avoid warnings in of_graph_get_next_endpoint()
  

•    * if the device doesn't have any graph connections
  

•    */
  

•   if (!of_graph_is_present(node))
  

•     return;
  

The problem here is that you are masking genuine errors.

If the graph is not described for a component, where it is mandatory, it won't be usable by the driver and as such using the devices will fail.

e.g, if an ETM misses the bindings, tracing will fail. (in either mode).

The solution is to either call this only if the device type is one that ports are not required - i.e. ETE, or upgrade the .dts bindings

The proposed change is too invasive and is not worth the benefit that it brings.

The side effect of this patch is, if someone makes a mistake in the bindings they don't see the "warning" in the dmesg. But will definitely hit the issue when trying to use the system.

i.e, Functionally there is no change.

On the other hand issuing a warning message for ETE is confusing for a well behaved user.

for the rest of the ETM devices to yaml so that the ports requirement is checked and validated there.

This is a step that we must take, but in a separate series. And I don't think this will solve handling non-compliant DTs *immediately*, as there could be : a) DTS that are not upstream (Quite common for CoreSight) b) People are getting used to the schema and running schema checks.

So, personally I vote for :

1) Merge this patch in as is 2) Convert the bindings to Yaml in a separate series.

Suzuki

On Wed, Jan 27, 2021 at 02:25:27PM +0530, Anshuman Khandual wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

ETE may not implement the OS lock and instead could rely on the PE OS Lock for the trace unit access. This is indicated by the TRCOLSR.OSM == 0b100. Add support for handling the PE OS lock

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm4x-core.c | 50 ++++++++++++++++++---- drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++ 2 files changed, 56 insertions(+), 9 deletions(-)

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 473ab74..9edf8be 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -114,30 +114,59 @@ void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit) } } -static void etm4_os_unlock_csa(struct etmv4_drvdata *drvdata, struct csdev_access *csa) +static void etm_detect_os_lock(struct etmv4_drvdata *drvdata,

• 	       struct csdev_access *csa)
  

{

• /* Writing 0 to TRCOSLAR unlocks the trace registers */
• etm4x_relaxed_write32(csa, 0x0, TRCOSLAR);
• drvdata->os_unlock = true;

• u32 oslsr = etm4x_relaxed_read32(csa, TRCOSLSR);
• drvdata->os_lock_model = ETM_OSLSR_OSLM(oslsr);

+}

+static void etm_write_os_lock(struct etmv4_drvdata *drvdata,

• 	      struct csdev_access *csa, u32 val)
  

+{

• val = !!val;
• switch (drvdata->os_lock_model) {
• case ETM_OSLOCK_PRESENT:

• etm4x_relaxed_write32(csa, val, TRCOSLAR);
  

• break;
  

• case ETM_OSLOCK_PE:

• write_sysreg_s(val, SYS_OSLAR_EL1);
  

• break;
  

• default:

• pr_warn_once("CPU%d: Unsupported Trace OSLock model: %x\n",
  

• 	     smp_processor_id(), drvdata->os_lock_model);
  

• fallthrough;
  

• case ETM_OSLOCK_NI:

• return;
  

• } isb();

} +static inline void etm4_os_unlock_csa(struct etmv4_drvdata *drvdata,

• 		      struct csdev_access *csa)
  

+{

• WARN_ON(drvdata->cpu != smp_processor_id());
• /* Writing 0 to OS Lock unlocks the trace unit registers */
• etm_write_os_lock(drvdata, csa, 0x0);
• drvdata->os_unlock = true;

+}

static void etm4_os_unlock(struct etmv4_drvdata *drvdata) { if (!WARN_ON(!drvdata->csdev)) etm4_os_unlock_csa(drvdata, &drvdata->csdev->access);

} static void etm4_os_lock(struct etmv4_drvdata *drvdata) { if (WARN_ON(!drvdata->csdev)) return;

• /* Writing 0x1 to TRCOSLAR locks the trace registers */
• etm4x_relaxed_write32(&drvdata->csdev->access, 0x1, TRCOSLAR);

• /* Writing 0x1 to OS Lock locks the trace registers */
• etm_write_os_lock(drvdata, &drvdata->csdev->access, 0x1); drvdata->os_unlock = false;

• isb();

} static void etm4_cs_lock(struct etmv4_drvdata *drvdata, @@ -906,6 +935,9 @@ static void etm4_init_arch_data(void *info) if (!etm4_init_csdev_access(drvdata, csa)) return;

• /* Detect the support for OS Lock before we actuall use it */
• etm_detect_os_lock(drvdata, csa);
• /* Make sure all registers are accessible */ etm4_os_unlock_csa(drvdata, csa); etm4_cs_unlock(drvdata, csa);

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 0af6057..0e86eba 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -506,6 +506,20 @@ ETM_MODE_EXCL_USER) /*

• • TRCOSLSR.OSLM advertises the OS Lock model.
• • OSLM[2:0] = TRCOSLSR[4:3,0]
• • 0b000 - Trace OS Lock is not implemented.
• • 0b010 - Trace OS Lock is implemented.
• • 0b100 - Trace OS Lock is not implemented, unit is controlled by PE OS Lock.
• */

+#define ETM_OSLOCK_NI 0b000 +#define ETM_OSLOCK_PRESENT 0b010 +#define ETM_OSLOCK_PE 0b100

+#define ETM_OSLSR_OSLM(oslsr) ((((oslsr) & GENMASK(4, 3)) >> 2) | (oslsr & 0x1))

+/*

• TRCDEVARCH Bit field definitions
• Bits[31:21] - ARCHITECT = Always Arm Ltd.

•            * Bits[31:28] = 0x4
  

@@ -897,6 +911,7 @@ struct etmv4_drvdata { u8 s_ex_level; u8 ns_ex_level; u8 q_support;

• u8 os_lock_model; bool sticky_enable; bool boot_enable; bool os_unlock;

-- 2.7.4

Reviewed-by: mike.leach mike.leach@linaro.org

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual anshuman.khandual@arm.com wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

ETE may not implement the OS lock and instead could rely on the PE OS Lock for the trace unit access. This is indicated by the TRCOLSR.OSM == 0b100. Add support for handling the PE OS lock

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm4x-core.c | 50 ++++++++++++++++++---- drivers/hwtracing/coresight/coresight-etm4x.h | 15 +++++++ 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 473ab74..9edf8be 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -114,30 +114,59 @@ void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit) } }

-static void etm4_os_unlock_csa(struct etmv4_drvdata *drvdata, struct csdev_access *csa) +static void etm_detect_os_lock(struct etmv4_drvdata *drvdata,

•                          struct csdev_access *csa)
  

{

•   /* Writing 0 to TRCOSLAR unlocks the trace registers */
  

•   etm4x_relaxed_write32(csa, 0x0, TRCOSLAR);
  

•   drvdata->os_unlock = true;
  

•   u32 oslsr = etm4x_relaxed_read32(csa, TRCOSLSR);
  

•   drvdata->os_lock_model = ETM_OSLSR_OSLM(oslsr);
  

+}

+static void etm_write_os_lock(struct etmv4_drvdata *drvdata,

•                         struct csdev_access *csa, u32 val)
  

+{

•   val = !!val;
  

•   switch (drvdata->os_lock_model) {
  

•   case ETM_OSLOCK_PRESENT:
  

•           etm4x_relaxed_write32(csa, val, TRCOSLAR);
  

•           break;
  

•   case ETM_OSLOCK_PE:
  

•           write_sysreg_s(val, SYS_OSLAR_EL1);
  

•           break;
  

•   default:
  

•           pr_warn_once("CPU%d: Unsupported Trace OSLock model: %x\n",
  

•                        smp_processor_id(), drvdata->os_lock_model);
  

•           fallthrough;
  

•   case ETM_OSLOCK_NI:
  

•           return;
  

•   }
    isb();
  

}

+static inline void etm4_os_unlock_csa(struct etmv4_drvdata *drvdata,

•                                 struct csdev_access *csa)
  

+{

•   WARN_ON(drvdata->cpu != smp_processor_id());
  

•   /* Writing 0 to OS Lock unlocks the trace unit registers */
  

•   etm_write_os_lock(drvdata, csa, 0x0);
  

•   drvdata->os_unlock = true;
  

+}

static void etm4_os_unlock(struct etmv4_drvdata *drvdata) { if (!WARN_ON(!drvdata->csdev)) etm4_os_unlock_csa(drvdata, &drvdata->csdev->access);

}

static void etm4_os_lock(struct etmv4_drvdata *drvdata) { if (WARN_ON(!drvdata->csdev)) return;

•   /* Writing 0x1 to TRCOSLAR locks the trace registers */
  

•   etm4x_relaxed_write32(&drvdata->csdev->access, 0x1, TRCOSLAR);
  

•   /* Writing 0x1 to OS Lock locks the trace registers */
  

•   etm_write_os_lock(drvdata, &drvdata->csdev->access, 0x1);
    drvdata->os_unlock = false;
  

•   isb();
  

}

static void etm4_cs_lock(struct etmv4_drvdata *drvdata, @@ -906,6 +935,9 @@ static void etm4_init_arch_data(void *info) if (!etm4_init_csdev_access(drvdata, csa)) return;

•   /* Detect the support for OS Lock before we actuall use it */
  

•   etm_detect_os_lock(drvdata, csa);
  

•   /* Make sure all registers are accessible */
    etm4_os_unlock_csa(drvdata, csa);
    etm4_cs_unlock(drvdata, csa);
  

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 0af6057..0e86eba 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -506,6 +506,20 @@ ETM_MODE_EXCL_USER)

/*

• • TRCOSLSR.OSLM advertises the OS Lock model.
• • OSLM[2:0] = TRCOSLSR[4:3,0]

• • 0b000 - Trace OS Lock is not implemented.
    

• • 0b010 - Trace OS Lock is implemented.
    

• • 0b100 - Trace OS Lock is not implemented, unit is controlled by PE OS Lock.
    

• */

+#define ETM_OSLOCK_NI 0b000 +#define ETM_OSLOCK_PRESENT 0b010 +#define ETM_OSLOCK_PE 0b100

+#define ETM_OSLSR_OSLM(oslsr) ((((oslsr) & GENMASK(4, 3)) >> 2) | (oslsr & 0x1))

+/*

• TRCDEVARCH Bit field definitions
• Bits[31:21] - ARCHITECT = Always Arm Ltd.

•            * Bits[31:28] = 0x4
  

@@ -897,6 +911,7 @@ struct etmv4_drvdata { u8 s_ex_level; u8 ns_ex_level; u8 q_support;

•   u8                              os_lock_model;
    bool                            sticky_enable;
    bool                            boot_enable;
    bool                            os_unlock;
  

-- 2.7.4

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On Wed, Jan 27, 2021 at 02:25:28PM +0530, Anshuman Khandual wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

Add support for handling the system registers for Embedded Trace Extensions (ETE). ETE shares most of the registers with ETMv4 except for some and also adds some new registers. Re-arrange the ETMv4x list to share the common definitions and add the ETE sysreg support.

Cc: Mike Leach mike.leach@linaro.org Cc: Mathieu Poirier mathieu.poirier@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm4x-core.c | 32 +++++++++++++ drivers/hwtracing/coresight/coresight-etm4x.h | 52 ++++++++++++++++++---- 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 9edf8be..9e92d2a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -114,6 +114,38 @@ void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit) } } +u64 ete_sysreg_read(u32 offset, bool _relaxed, bool _64bit) +{

• u64 res = 0;
• switch (offset) {
• ETE_READ_CASES(res)
• default :

• WARN_ONCE(1, "ete: trying to read unsupported register @%x\n",
  

• 	 offset);
  

Alignment

• }
• if (!_relaxed)

• __iormb(res);	/* Imitate the !relaxed I/O helpers */
  

• return res;

+}

+void ete_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit) +{

• if (!_relaxed)

• __iowmb();	/* Imitate the !relaxed I/O helpers */
  

• if (!_64bit)

• val &= GENMASK(31, 0);
  

• switch (offset) {
• ETE_WRITE_CASES(val)
• default :

• WARN_ONCE(1, "ete: trying to write to unsupported register @%x\n",
  

• 	offset);
  

Alignment

• }

+}

The etm4x_sysreg_xyz() equivalent of these use a pr_warn_ratelimited() rather than a WARN_ONE().

With that:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

static void etm_detect_os_lock(struct etmv4_drvdata *drvdata, struct csdev_access *csa) { diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 0e86eba..ca24ac5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -29,6 +29,7 @@ #define TRCAUXCTLR 0x018 #define TRCEVENTCTL0R 0x020 #define TRCEVENTCTL1R 0x024 +#define TRCRSR 0x028 #define TRCSTALLCTLR 0x02C #define TRCTSCTLR 0x030 #define TRCSYNCPR 0x034 @@ -49,6 +50,7 @@ #define TRCSEQRSTEVR 0x118 #define TRCSEQSTR 0x11C #define TRCEXTINSELR 0x120 +#define TRCEXTINSELRn(n) (0x120 + (n * 4)) /* n = 0-3 */ #define TRCCNTRLDVRn(n) (0x140 + (n * 4)) /* n = 0-3 */ #define TRCCNTCTLRn(n) (0x150 + (n * 4)) /* n = 0-3 */ #define TRCCNTVRn(n) (0x160 + (n * 4)) /* n = 0-3 */ @@ -160,10 +162,22 @@ #define CASE_NOP(__unused, x) \ case (x): /* fall through */ +#define ETE_ONLY_SYSREG_LIST(op, val) \

• CASE_##op((val), TRCRSR) \
• CASE_##op((val), TRCEXTINSELRn(1)) \
• CASE_##op((val), TRCEXTINSELRn(2)) \
• CASE_##op((val), TRCEXTINSELRn(3))

/* List of registers accessible via System instructions */ -#define ETM_SYSREG_LIST(op, val) \

• CASE_##op((val), TRCPRGCTLR) \

+#define ETM4x_ONLY_SYSREG_LIST(op, val) \ CASE_##op((val), TRCPROCSELR) \

• CASE_##op((val), TRCVDCTLR) \
• CASE_##op((val), TRCVDSACCTLR) \
• CASE_##op((val), TRCVDARCCTLR) \
• CASE_##op((val), TRCOSLAR)

+#define ETM_COMMON_SYSREG_LIST(op, val) \

• CASE_##op((val), TRCPRGCTLR) \ CASE_##op((val), TRCSTATR) \ CASE_##op((val), TRCCONFIGR) \ CASE_##op((val), TRCAUXCTLR) \

@@ -180,9 +194,6 @@ CASE_##op((val), TRCVIIECTLR) \ CASE_##op((val), TRCVISSCTLR) \ CASE_##op((val), TRCVIPCSSCTLR) \

• CASE_##op((val), TRCVDCTLR) \
• CASE_##op((val), TRCVDSACCTLR) \
• CASE_##op((val), TRCVDARCCTLR) \ CASE_##op((val), TRCSEQEVRn(0)) \ CASE_##op((val), TRCSEQEVRn(1)) \ CASE_##op((val), TRCSEQEVRn(2)) \

@@ -277,7 +288,6 @@ CASE_##op((val), TRCSSPCICRn(5)) \ CASE_##op((val), TRCSSPCICRn(6)) \ CASE_##op((val), TRCSSPCICRn(7)) \

• CASE_##op((val), TRCOSLAR) \ CASE_##op((val), TRCOSLSR) \ CASE_##op((val), TRCACVRn(0)) \ CASE_##op((val), TRCACVRn(1)) \

@@ -369,12 +379,36 @@ CASE_##op((val), TRCPIDR2) \ CASE_##op((val), TRCPIDR3) -#define ETM4x_READ_SYSREG_CASES(res) ETM_SYSREG_LIST(READ, (res)) -#define ETM4x_WRITE_SYSREG_CASES(val) ETM_SYSREG_LIST(WRITE, (val)) +#define ETM4x_READ_SYSREG_CASES(res) \

• ETM_COMMON_SYSREG_LIST(READ, (res)) \
• ETM4x_ONLY_SYSREG_LIST(READ, (res))

+#define ETM4x_WRITE_SYSREG_CASES(val) \

• ETM_COMMON_SYSREG_LIST(WRITE, (val)) \
• ETM4x_ONLY_SYSREG_LIST(WRITE, (val))

+#define ETM_COMMON_SYSREG_LIST_CASES \

• ETM_COMMON_SYSREG_LIST(NOP, __unused)

+#define ETM4x_SYSREG_LIST_CASES \

• ETM_COMMON_SYSREG_LIST_CASES \
• ETM4x_ONLY_SYSREG_LIST(NOP, __unused)

-#define ETM4x_SYSREG_LIST_CASES ETM_SYSREG_LIST(NOP, __unused) #define ETM4x_MMAP_LIST_CASES ETM_MMAP_LIST(NOP, __unused) +/* ETE only supports system register access */ +#define ETE_READ_CASES(res) \

• ETM_COMMON_SYSREG_LIST(READ, (res)) \
• ETE_ONLY_SYSREG_LIST(READ, (res))

+#define ETE_WRITE_CASES(val) \

• ETM_COMMON_SYSREG_LIST(WRITE, (val)) \
• ETE_ONLY_SYSREG_LIST(WRITE, (val))

+#define ETE_ONLY_SYSREG_LIST_CASES \

• ETM_COMMON_SYSREG_LIST_CASES \
• ETE_ONLY_SYSREG_LIST(NOP, __unused)

#define read_etm4x_sysreg_offset(offset, _64bit) \ ({ \ u64 __val; \ -- 2.7.4

Reviewed-by: Mike Leach mike.leach@linaro.org

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual anshuman.khandual@arm.com wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

Add support for handling the system registers for Embedded Trace Extensions (ETE). ETE shares most of the registers with ETMv4 except for some and also adds some new registers. Re-arrange the ETMv4x list to share the common definitions and add the ETE sysreg support.

Cc: Mike Leach mike.leach@linaro.org Cc: Mathieu Poirier mathieu.poirier@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm4x-core.c | 32 +++++++++++++ drivers/hwtracing/coresight/coresight-etm4x.h | 52 ++++++++++++++++++---- 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 9edf8be..9e92d2a 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -114,6 +114,38 @@ void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit) } }

+u64 ete_sysreg_read(u32 offset, bool _relaxed, bool _64bit) +{

•   u64 res = 0;
  

•   switch (offset) {
  

•   ETE_READ_CASES(res)
  

•   default :
  

•           WARN_ONCE(1, "ete: trying to read unsupported register @%x\n",
  

•                    offset);
  

•   }
  

•   if (!_relaxed)
  

•           __iormb(res);   /* Imitate the !relaxed I/O helpers */
  

•   return res;
  

+}

+void ete_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit) +{

•   if (!_relaxed)
  

•           __iowmb();      /* Imitate the !relaxed I/O helpers */
  

•   if (!_64bit)
  

•           val &= GENMASK(31, 0);
  

•   switch (offset) {
  

•   ETE_WRITE_CASES(val)
  

•   default :
  

•           WARN_ONCE(1, "ete: trying to write to unsupported register @%x\n",
  

•                   offset);
  

•   }
  

+}

static void etm_detect_os_lock(struct etmv4_drvdata *drvdata, struct csdev_access *csa) { diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 0e86eba..ca24ac5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -29,6 +29,7 @@ #define TRCAUXCTLR 0x018 #define TRCEVENTCTL0R 0x020 #define TRCEVENTCTL1R 0x024 +#define TRCRSR 0x028 #define TRCSTALLCTLR 0x02C #define TRCTSCTLR 0x030 #define TRCSYNCPR 0x034 @@ -49,6 +50,7 @@ #define TRCSEQRSTEVR 0x118 #define TRCSEQSTR 0x11C #define TRCEXTINSELR 0x120 +#define TRCEXTINSELRn(n) (0x120 + (n * 4)) /* n = 0-3 */ #define TRCCNTRLDVRn(n) (0x140 + (n * 4)) /* n = 0-3 */ #define TRCCNTCTLRn(n) (0x150 + (n * 4)) /* n = 0-3 */ #define TRCCNTVRn(n) (0x160 + (n * 4)) /* n = 0-3 */ @@ -160,10 +162,22 @@ #define CASE_NOP(__unused, x) \ case (x): /* fall through */

+#define ETE_ONLY_SYSREG_LIST(op, val) \

•   CASE_##op((val), TRCRSR)                \
  

•   CASE_##op((val), TRCEXTINSELRn(1))      \
  

•   CASE_##op((val), TRCEXTINSELRn(2))      \
  

•   CASE_##op((val), TRCEXTINSELRn(3))
  

/* List of registers accessible via System instructions */ -#define ETM_SYSREG_LIST(op, val) \

•   CASE_##op((val), TRCPRGCTLR)            \
  

+#define ETM4x_ONLY_SYSREG_LIST(op, val) \ CASE_##op((val), TRCPROCSELR) \

•   CASE_##op((val), TRCVDCTLR)             \
  

•   CASE_##op((val), TRCVDSACCTLR)          \
  

•   CASE_##op((val), TRCVDARCCTLR)          \
  

•   CASE_##op((val), TRCOSLAR)
  

+#define ETM_COMMON_SYSREG_LIST(op, val) \

•   CASE_##op((val), TRCPRGCTLR)            \
    CASE_##op((val), TRCSTATR)              \
    CASE_##op((val), TRCCONFIGR)            \
    CASE_##op((val), TRCAUXCTLR)            \
  

@@ -180,9 +194,6 @@ CASE_##op((val), TRCVIIECTLR) \ CASE_##op((val), TRCVISSCTLR) \ CASE_##op((val), TRCVIPCSSCTLR) \

•   CASE_##op((val), TRCVDCTLR)             \
  

•   CASE_##op((val), TRCVDSACCTLR)          \
  

•   CASE_##op((val), TRCVDARCCTLR)          \
    CASE_##op((val), TRCSEQEVRn(0))         \
    CASE_##op((val), TRCSEQEVRn(1))         \
    CASE_##op((val), TRCSEQEVRn(2))         \
  

@@ -277,7 +288,6 @@ CASE_##op((val), TRCSSPCICRn(5)) \ CASE_##op((val), TRCSSPCICRn(6)) \ CASE_##op((val), TRCSSPCICRn(7)) \

•   CASE_##op((val), TRCOSLAR)              \
    CASE_##op((val), TRCOSLSR)              \
    CASE_##op((val), TRCACVRn(0))           \
    CASE_##op((val), TRCACVRn(1))           \
  

@@ -369,12 +379,36 @@ CASE_##op((val), TRCPIDR2) \ CASE_##op((val), TRCPIDR3)

-#define ETM4x_READ_SYSREG_CASES(res) ETM_SYSREG_LIST(READ, (res)) -#define ETM4x_WRITE_SYSREG_CASES(val) ETM_SYSREG_LIST(WRITE, (val)) +#define ETM4x_READ_SYSREG_CASES(res) \

•   ETM_COMMON_SYSREG_LIST(READ, (res))     \
  

•   ETM4x_ONLY_SYSREG_LIST(READ, (res))
  

+#define ETM4x_WRITE_SYSREG_CASES(val) \

•   ETM_COMMON_SYSREG_LIST(WRITE, (val))    \
  

•   ETM4x_ONLY_SYSREG_LIST(WRITE, (val))
  

+#define ETM_COMMON_SYSREG_LIST_CASES \

•   ETM_COMMON_SYSREG_LIST(NOP, __unused)
  

+#define ETM4x_SYSREG_LIST_CASES \

•   ETM_COMMON_SYSREG_LIST_CASES            \
  

•   ETM4x_ONLY_SYSREG_LIST(NOP, __unused)
  
  

-#define ETM4x_SYSREG_LIST_CASES ETM_SYSREG_LIST(NOP, __unused) #define ETM4x_MMAP_LIST_CASES ETM_MMAP_LIST(NOP, __unused)

+/* ETE only supports system register access */ +#define ETE_READ_CASES(res) \

•   ETM_COMMON_SYSREG_LIST(READ, (res))     \
  

•   ETE_ONLY_SYSREG_LIST(READ, (res))
  

+#define ETE_WRITE_CASES(val) \

•   ETM_COMMON_SYSREG_LIST(WRITE, (val))    \
  

•   ETE_ONLY_SYSREG_LIST(WRITE, (val))
  

+#define ETE_ONLY_SYSREG_LIST_CASES \

•   ETM_COMMON_SYSREG_LIST_CASES            \
  

•   ETE_ONLY_SYSREG_LIST(NOP, __unused)
  

#define read_etm4x_sysreg_offset(offset, _64bit) \ ({ \ u64 __val; \ -- 2.7.4

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On Wed, Jan 27, 2021 at 02:25:29PM +0530, Anshuman Khandual wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

Add ETE as one of the supported device types we support with ETM4x driver. The devices are named following the existing convention as ete<N>.

ETE mandates that the trace resource status register is programmed before the tracing is turned on. For the moment simply write to it indicating TraceActive.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/Kconfig | 10 ++-- drivers/hwtracing/coresight/coresight-etm4x-core.c | 56 +++++++++++++++++----- .../hwtracing/coresight/coresight-etm4x-sysfs.c | 19 ++++++-- drivers/hwtracing/coresight/coresight-etm4x.h | 16 ++++++- 4 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 7b44ba2..f154ae7 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -97,15 +97,15 @@ config CORESIGHT_SOURCE_ETM3X module will be called coresight-etm3x. config CORESIGHT_SOURCE_ETM4X

• tristate "CoreSight Embedded Trace Macrocell 4.x driver"

• tristate "CoreSight ETMv4.x / ETE driver" depends on ARM64 select CORESIGHT_LINKS_AND_SINKS select PID_IN_CONTEXTIDR help

•  This driver provides support for the ETM4.x tracer module, tracing the
  

•  instructions that a processor is executing. This is primarily useful
  

•  for instruction level tracing. Depending on the implemented version
  

•  data tracing may also be available.
  

•  This driver provides support for the CoreSight Embedded Trace Macrocell
  

•  version 4.x and the Embedded Trace Extensions (ETE). Both are CPU tracer
  

•  modules, tracing the instructions that a processor is executing. This is
  

•  primarily useful for instruction level tracing.
  

To compile this driver as a module, choose M here: the module will be called coresight-etm4x. diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 9e92d2a..b40e3c2 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -431,6 +431,13 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, trcpdcr | TRCPDCR_PU, TRCPDCR); }

• /*

• * ETE mandates that the TRCRSR is written to before
  

• * enabling it.
  

• */
  

• if (etm4x_is_ete(drvdata))

• etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
  

• /* Enable the trace unit */ etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);

@@ -864,13 +871,24 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata, * ETMs implementing sysreg access must implement TRCDEVARCH. */ devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);

• if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH)

• switch (devarch & ETM_DEVARCH_ID_MASK) {
• case ETM_DEVARCH_ETMv4x_ARCH:

• *csa = (struct csdev_access) {
  

• 	.io_mem	= false,
  

• 	.read	= etm4x_sysreg_read,
  

• 	.write	= etm4x_sysreg_write,
  

• };
  

• break;
  

• case ETM_DEVARCH_ETE_ARCH:

• *csa = (struct csdev_access) {
  

• 	.io_mem	= false,
  

• 	.read	= ete_sysreg_read,
  

• 	.write	= ete_sysreg_write,
  

• };
  

• break;
  

• default: return false;

• *csa = (struct csdev_access) {

• .io_mem	= false,
  

• .read	= etm4x_sysreg_read,
  

• .write	= etm4x_sysreg_write,
  

• };

• }

drvdata->arch = etm_devarch_to_arch(devarch); return true; @@ -1808,6 +1826,8 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) struct etmv4_drvdata *drvdata; struct coresight_desc desc = { 0 }; struct etm4_init_arg init_arg = { 0 };

• u8 major, minor;
• char *type_name;

drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); if (!drvdata) @@ -1834,10 +1854,6 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) if (drvdata->cpu < 0) return drvdata->cpu;

• desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
• if (!desc.name)

• return -ENOMEM;
  

• init_arg.drvdata = drvdata; init_arg.csa = &desc.access; init_arg.pid = etm_pid;

@@ -1853,6 +1869,20 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) if (!desc.access.io_mem || fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up")) drvdata->skip_power_up = true;

Add a space here...

• major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
• minor = ETM_ARCH_MINOR_VERSION(drvdata->arch);

And here too. Othersiwe it makes a big blob in the middle of the function.

• if (etm4x_is_ete(drvdata)) {

• type_name = "ete";
  

• /* ETE v1 has major version == 5. Adjust this for logging.*/
  

• major -= 4;
  

I don't have the documentation for the ETE but I would not adjust @major. I would simply leave it to what the HW gives us since regardless of the name, the major revision of the IP block is 5.

• } else {

• type_name = "etm";
  

• }
• desc.name = devm_kasprintf(dev, GFP_KERNEL,

• 		   "%s%d", type_name, drvdata->cpu);
  

• if (!desc.name)

• return -ENOMEM;
  

etm4_init_trace_id(drvdata); etm4_set_default(&drvdata->config); @@ -1881,9 +1911,8 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) etmdrvdata[drvdata->cpu] = drvdata;

• dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n",

•  drvdata->cpu, ETM_ARCH_MAJOR_VERSION(drvdata->arch),
  

•  ETM_ARCH_MINOR_VERSION(drvdata->arch));
  

• dev_info(&drvdata->csdev->dev, "CPU%d: %s v%d.%d initialized\n",

•  drvdata->cpu, type_name, major, minor);
  

if (boot_enable) { coresight_enable(drvdata->csdev); @@ -2027,6 +2056,7 @@ static struct amba_driver etm4x_amba_driver = { static const struct of_device_id etm4_sysreg_match[] = { { .compatible = "arm,coresight-etm4x-sysreg" },

• { .compatible = "arm,embedded-trace-extension" }, {}

}; diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index b646d53..1c490bc 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -2374,12 +2374,20 @@ static inline bool etm4x_register_implemented(struct etmv4_drvdata *drvdata, u32 offset) { switch (offset) {

• ETM4x_SYSREG_LIST_CASES

• ETM_COMMON_SYSREG_LIST_CASES /*

•  * Registers accessible via system instructions are always
  

•  * implemented.
  

•  * Common registers to ETE & ETM4x accessible via system
  

•  * instructions are always implemented.
  

  */ return true;
• ETM4x_ONLY_SYSREG_LIST_CASES

• /*
  

•  * We only support etm4x and ete. So if the device is not
  

•  * ETE, it must be ETMv4x.
  

•  */
  

• return !etm4x_is_ete(drvdata);
  

• ETM4x_MMAP_LIST_CASES /*
  • Registers accessible only via memory-mapped registers

@@ -2389,8 +2397,13 @@ etm4x_register_implemented(struct etmv4_drvdata *drvdata, u32 offset) * coresight_register() and the csdev is not initialized * until that is done. So rely on the drvdata->base to * detect if we have a memory mapped access.

•  * Also ETE doesn't implement memory mapped access, thus
  

•  * it is sufficient to check that we are using mmio.
  

  */ return !!drvdata->base;
• ETE_ONLY_SYSREG_LIST_CASES

• return etm4x_is_ete(drvdata);
  

  }

return false; diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index ca24ac5..8b90de5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -128,6 +128,8 @@ #define TRCCIDR2 0xFF8 #define TRCCIDR3 0xFFC +#define TRCRSR_TA BIT(12)

/*

• System instructions to access ETM registers.
• See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions

@@ -390,6 +392,9 @@ #define ETM_COMMON_SYSREG_LIST_CASES \ ETM_COMMON_SYSREG_LIST(NOP, __unused) +#define ETM4x_ONLY_SYSREG_LIST_CASES \

• ETM4x_ONLY_SYSREG_LIST(NOP, __unused)

#define ETM4x_SYSREG_LIST_CASES \ ETM_COMMON_SYSREG_LIST_CASES \ ETM4x_ONLY_SYSREG_LIST(NOP, __unused) @@ -406,7 +411,6 @@ ETE_ONLY_SYSREG_LIST(WRITE, (val)) #define ETE_ONLY_SYSREG_LIST_CASES \

• ETM_COMMON_SYSREG_LIST_CASES \

This goes in patch 04.

With the above:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

ETE_ONLY_SYSREG_LIST(NOP, __unused) #define read_etm4x_sysreg_offset(offset, _64bit) \ @@ -589,11 +593,14 @@ ((ETM_DEVARCH_MAKE_ARCHID_ARCH_VER(major)) | ETM_DEVARCH_ARCHID_ARCH_PART(0xA13)) #define ETM_DEVARCH_ARCHID_ETMv4x ETM_DEVARCH_MAKE_ARCHID(0x4) +#define ETM_DEVARCH_ARCHID_ETE ETM_DEVARCH_MAKE_ARCHID(0x5) #define ETM_DEVARCH_ID_MASK \ (ETM_DEVARCH_ARCHITECT_MASK | ETM_DEVARCH_ARCHID_MASK | ETM_DEVARCH_PRESENT) #define ETM_DEVARCH_ETMv4x_ARCH \ (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETMv4x | ETM_DEVARCH_PRESENT) +#define ETM_DEVARCH_ETE_ARCH \

• (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT)

#define TRCSTATR_IDLE_BIT 0 #define TRCSTATR_PMSTABLE_BIT 1 @@ -683,6 +690,8 @@ #define ETM_ARCH_MINOR_VERSION(arch) ((arch) & 0xfU) #define ETM_ARCH_V4 ETM_ARCH_VERSION(4, 0) +#define ETM_ARCH_ETE ETM_ARCH_VERSION(5, 0)

/* Interpretation of resource numbers change at ETM v4.3 architecture */ #define ETM_ARCH_V4_3 ETM_ARCH_VERSION(4, 3) @@ -989,4 +998,9 @@ void etm4_config_trace_mode(struct etmv4_config *config); u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit); void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit);

+static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata) +{

• return drvdata->arch >= ETM_ARCH_ETE;

+}

#endif

2.7.4

HI,

On Tue, 2 Feb 2021 at 18:56, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Wed, Jan 27, 2021 at 02:25:29PM +0530, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

Add ETE as one of the supported device types we support with ETM4x driver. The devices are named following the existing convention as ete<N>.

ETE mandates that the trace resource status register is programmed before the tracing is turned on. For the moment simply write to it indicating TraceActive.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/Kconfig | 10 ++-- drivers/hwtracing/coresight/coresight-etm4x-core.c | 56 +++++++++++++++++----- .../hwtracing/coresight/coresight-etm4x-sysfs.c | 19 ++++++-- drivers/hwtracing/coresight/coresight-etm4x.h | 16 ++++++- 4 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 7b44ba2..f154ae7 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -97,15 +97,15 @@ config CORESIGHT_SOURCE_ETM3X module will be called coresight-etm3x.

config CORESIGHT_SOURCE_ETM4X

• tristate "CoreSight Embedded Trace Macrocell 4.x driver"
  

• tristate "CoreSight ETMv4.x / ETE driver"
  depends on ARM64
  select CORESIGHT_LINKS_AND_SINKS
  select PID_IN_CONTEXTIDR
  help
  

•   This driver provides support for the ETM4.x tracer module, tracing the
  

•   instructions that a processor is executing. This is primarily useful
  

•   for instruction level tracing. Depending on the implemented version
  

•   data tracing may also be available.
  

•   This driver provides support for the CoreSight Embedded Trace Macrocell
  

•   version 4.x and the Embedded Trace Extensions (ETE). Both are CPU tracer
  

•   modules, tracing the instructions that a processor is executing. This is
  

•   primarily useful for instruction level tracing.
  
    To compile this driver as a module, choose M here: the
    module will be called coresight-etm4x.
  

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 9e92d2a..b40e3c2 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -431,6 +431,13 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata) etm4x_relaxed_write32(csa, trcpdcr | TRCPDCR_PU, TRCPDCR); }

• /*
  

•  * ETE mandates that the TRCRSR is written to before
  

•  * enabling it.
  

•  */
  

• if (etm4x_is_ete(drvdata))
  

•         etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
  

• /* Enable the trace unit */
  etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
  
  

@@ -864,13 +871,24 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata, * ETMs implementing sysreg access must implement TRCDEVARCH. */ devarch = read_etm4x_sysreg_const_offset(TRCDEVARCH);

• if ((devarch & ETM_DEVARCH_ID_MASK) != ETM_DEVARCH_ETMv4x_ARCH)
  

• switch (devarch & ETM_DEVARCH_ID_MASK) {
  

• case ETM_DEVARCH_ETMv4x_ARCH:
  

•         *csa = (struct csdev_access) {
  

•                 .io_mem = false,
  

•                 .read   = etm4x_sysreg_read,
  

•                 .write  = etm4x_sysreg_write,
  

•         };
  

•         break;
  

• case ETM_DEVARCH_ETE_ARCH:
  

•         *csa = (struct csdev_access) {
  

•                 .io_mem = false,
  

•                 .read   = ete_sysreg_read,
  

•                 .write  = ete_sysreg_write,
  

•         };
  

•         break;
  

• default:
          return false;
  

• *csa = (struct csdev_access) {
  

•         .io_mem = false,
  

•         .read   = etm4x_sysreg_read,
  

•         .write  = etm4x_sysreg_write,
  

• };
  

• }
  
  drvdata->arch = etm_devarch_to_arch(devarch);
  return true;
  

@@ -1808,6 +1826,8 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) struct etmv4_drvdata *drvdata; struct coresight_desc desc = { 0 }; struct etm4_init_arg init_arg = { 0 };

• u8 major, minor;
  

• char *type_name;
  
  drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
  if (!drvdata)
  

@@ -1834,10 +1854,6 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) if (drvdata->cpu < 0) return drvdata->cpu;

• desc.name = devm_kasprintf(dev, GFP_KERNEL, "etm%d", drvdata->cpu);
  

• if (!desc.name)
  

•         return -ENOMEM;
  

• init_arg.drvdata = drvdata;
  init_arg.csa = &desc.access;
  init_arg.pid = etm_pid;
  

@@ -1853,6 +1869,20 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid) if (!desc.access.io_mem || fwnode_property_present(dev_fwnode(dev), "qcom,skip-power-up")) drvdata->skip_power_up = true;

Add a space here...

...

• major = ETM_ARCH_MAJOR_VERSION(drvdata->arch);
  

• minor = ETM_ARCH_MINOR_VERSION(drvdata->arch);
  

And here too. Othersiwe it makes a big blob in the middle of the function.

...

• if (etm4x_is_ete(drvdata)) {
  

•         type_name = "ete";
  

•         /* ETE v1 has major version == 5. Adjust this for logging.*/
  

•         major -= 4;
  

I don't have the documentation for the ETE but I would not adjust @major. I would simply leave it to what the HW gives us since regardless of the name, the major revision of the IP block is 5.

For consistency this adjustment should be retained. All the prior drivers reported the protocol version related to this. For ETM3.x this value was 0x2 - logging printed "ETMv3.x", for PTM this value was 0x3 - logging printed "PTM 1.x". Only for ETMv4 is this value the same as the protocol version.

Mike

...

• } else {
  

•         type_name = "etm";
  

• }
  

• desc.name = devm_kasprintf(dev, GFP_KERNEL,
  

•                            "%s%d", type_name, drvdata->cpu);
  

• if (!desc.name)
  

•         return -ENOMEM;
  
  etm4_init_trace_id(drvdata);
  etm4_set_default(&drvdata->config);
  

@@ -1881,9 +1911,8 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)

  etmdrvdata[drvdata->cpu] = drvdata;

• dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n",
  

•          drvdata->cpu, ETM_ARCH_MAJOR_VERSION(drvdata->arch),
  

•          ETM_ARCH_MINOR_VERSION(drvdata->arch));
  

• dev_info(&drvdata->csdev->dev, "CPU%d: %s v%d.%d initialized\n",
  

•          drvdata->cpu, type_name, major, minor);
  
  if (boot_enable) {
          coresight_enable(drvdata->csdev);
  

@@ -2027,6 +2056,7 @@ static struct amba_driver etm4x_amba_driver = {

static const struct of_device_id etm4_sysreg_match[] = { { .compatible = "arm,coresight-etm4x-sysreg" },

• { .compatible   = "arm,embedded-trace-extension" },
  {}
  

};

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index b646d53..1c490bc 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -2374,12 +2374,20 @@ static inline bool etm4x_register_implemented(struct etmv4_drvdata *drvdata, u32 offset) { switch (offset) {

• ETM4x_SYSREG_LIST_CASES
  

• ETM_COMMON_SYSREG_LIST_CASES
          /*
  

•          * Registers accessible via system instructions are always
  

•          * implemented.
  

•          * Common registers to ETE & ETM4x accessible via system
  

•          * instructions are always implemented.
           */
          return true;
  

• ETM4x_ONLY_SYSREG_LIST_CASES
  

•         /*
  

•          * We only support etm4x and ete. So if the device is not
  

•          * ETE, it must be ETMv4x.
  

•          */
  

•         return !etm4x_is_ete(drvdata);
  

• ETM4x_MMAP_LIST_CASES
          /*
           * Registers accessible only via memory-mapped registers
  

@@ -2389,8 +2397,13 @@ etm4x_register_implemented(struct etmv4_drvdata *drvdata, u32 offset) * coresight_register() and the csdev is not initialized * until that is done. So rely on the drvdata->base to * detect if we have a memory mapped access.

•          * Also ETE doesn't implement memory mapped access, thus
  

•          * it is sufficient to check that we are using mmio.
           */
          return !!drvdata->base;
  

• ETE_ONLY_SYSREG_LIST_CASES
  

•         return etm4x_is_ete(drvdata);
  }
  
  return false;
  

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index ca24ac5..8b90de5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -128,6 +128,8 @@ #define TRCCIDR2 0xFF8 #define TRCCIDR3 0xFFC

+#define TRCRSR_TA BIT(12)

/*

• System instructions to access ETM registers.
• See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions

@@ -390,6 +392,9 @@ #define ETM_COMMON_SYSREG_LIST_CASES \ ETM_COMMON_SYSREG_LIST(NOP, __unused)

+#define ETM4x_ONLY_SYSREG_LIST_CASES \

• ETM4x_ONLY_SYSREG_LIST(NOP, __unused)
  

#define ETM4x_SYSREG_LIST_CASES \ ETM_COMMON_SYSREG_LIST_CASES \ ETM4x_ONLY_SYSREG_LIST(NOP, __unused) @@ -406,7 +411,6 @@ ETE_ONLY_SYSREG_LIST(WRITE, (val))

#define ETE_ONLY_SYSREG_LIST_CASES \

• ETM_COMMON_SYSREG_LIST_CASES            \
  

This goes in patch 04.

With the above:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

...

  ETE_ONLY_SYSREG_LIST(NOP, __unused)

#define read_etm4x_sysreg_offset(offset, _64bit) \ @@ -589,11 +593,14 @@ ((ETM_DEVARCH_MAKE_ARCHID_ARCH_VER(major)) | ETM_DEVARCH_ARCHID_ARCH_PART(0xA13))

#define ETM_DEVARCH_ARCHID_ETMv4x ETM_DEVARCH_MAKE_ARCHID(0x4) +#define ETM_DEVARCH_ARCHID_ETE ETM_DEVARCH_MAKE_ARCHID(0x5)

#define ETM_DEVARCH_ID_MASK \ (ETM_DEVARCH_ARCHITECT_MASK | ETM_DEVARCH_ARCHID_MASK | ETM_DEVARCH_PRESENT) #define ETM_DEVARCH_ETMv4x_ARCH \ (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETMv4x | ETM_DEVARCH_PRESENT) +#define ETM_DEVARCH_ETE_ARCH \

• (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT)
  
  

#define TRCSTATR_IDLE_BIT 0 #define TRCSTATR_PMSTABLE_BIT 1 @@ -683,6 +690,8 @@ #define ETM_ARCH_MINOR_VERSION(arch) ((arch) & 0xfU)

#define ETM_ARCH_V4 ETM_ARCH_VERSION(4, 0) +#define ETM_ARCH_ETE ETM_ARCH_VERSION(5, 0)

/* Interpretation of resource numbers change at ETM v4.3 architecture */ #define ETM_ARCH_V4_3 ETM_ARCH_VERSION(4, 3)

@@ -989,4 +998,9 @@ void etm4_config_trace_mode(struct etmv4_config *config);

u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit); void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit);

+static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata) +{

• return drvdata->arch >= ETM_ARCH_ETE;
  

+}

#endif

2.7.4

Hi Rob

On 2/9/21 7:00 PM, Rob Herring wrote:

On Wed, Jan 27, 2021 at 02:25:30PM +0530, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

Document the device tree bindings for Embedded Trace Extensions. ETE can be connected to legacy coresight components and thus could optionally contain a connection graph as described by the CoreSight bindings.

Cc: devicetree@vger.kernel.org Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Rob Herring robh@kernel.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Fixed all DT yaml semantics problems

  Documentation/devicetree/bindings/arm/ete.yaml | 74 ++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/ete.yaml

diff --git a/Documentation/devicetree/bindings/arm/ete.yaml b/Documentation/devicetree/bindings/arm/ete.yaml new file mode 100644 index 0000000..edc1fe2 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/ete.yaml @@ -0,0 +1,74 @@ +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause +# Copyright 2021, Arm Ltd +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/arm/ete.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

+title: ARM Embedded Trace Extensions

+maintainers:

• • Suzuki K Poulose suzuki.poulose@arm.com
• • Mathieu Poirier mathieu.poirier@linaro.org

+description: |

• Arm Embedded Trace Extension(ETE) is a per CPU trace component that
• allows tracing the CPU execution. It overlaps with the CoreSight ETMv4
• architecture and has extended support for future architecture changes.
• The trace generated by the ETE could be stored via legacy CoreSight
• components (e.g, TMC-ETR) or other means (e.g, using a per CPU buffer
• Arm Trace Buffer Extension (TRBE)). Since the ETE can be connected to
• legacy CoreSight components, a node must be listed per instance, along
• with any optional connection graph as per the coresight bindings.
• See bindings/arm/coresight.txt.

+properties:

• $nodename:
• pattern: "^ete([0-9a-f]+)$"
• compatible:
• items:

•  - const: arm,embedded-trace-extension
  

• cpu:

We've already established 'cpus' for this purpose.

Please see : https://lkml.kernel.org/r/9417218b-6eda-373b-a2cb-869089ffc7cd@arm.com for my response in the previous version to this and the one with out-ports.

...

• description: |

•  Handle to the cpu this ETE is bound to.
  

• $ref: /schemas/types.yaml#/definitions/phandle
• out-ports:
• type: object

Replace with: $ref: /schemas/graph.yaml#/properties/ports

So, just to confirm again : The CoreSight graph bindings expect the input ports and output ports grouped under in-ports{} and out-ports{} respectively to avoid having to specify the direction of the ports in the individual "port" nodes. i.e

in-ports {

property: ports OR property: port

required: OneOf: ports port }

out-ports {

# same as above }

So thats why I added out-ports as a new object, where the ports/port could be a child node.

Ideally the definition of out-ports /in-ports should go to a common schema for CoreSight bindings, when we move to Yaml for the existing bindings, which will follow in a separate series, later.

...

• description: |

•  Output connections from the ETE to legacy CoreSight trace bus.
  

• properties:

•  port:
  

•    $ref: /schemas/graph.yaml#/properties/port
  

Actually, if only 1 port ever, you can drop 'out-ports' and just have 'port'. Not sure though if the coresight stuff depends on 'out-ports'.

Cheers Suzuki

On 2/18/21 6:33 PM, Rob Herring wrote:

On Wed, Feb 10, 2021 at 12:33:44PM +0000, Suzuki K Poulose wrote:

...

Hi Rob

On 2/9/21 7:00 PM, Rob Herring wrote:

...

On Wed, Jan 27, 2021 at 02:25:30PM +0530, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

Document the device tree bindings for Embedded Trace Extensions. ETE can be connected to legacy coresight components and thus could optionally contain a connection graph as described by the CoreSight bindings.

Cc: devicetree@vger.kernel.org Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Rob Herring robh@kernel.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Fixed all DT yaml semantics problems

  Documentation/devicetree/bindings/arm/ete.yaml | 74 ++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/ete.yaml

diff --git a/Documentation/devicetree/bindings/arm/ete.yaml b/Documentation/devicetree/bindings/arm/ete.yaml new file mode 100644 index 0000000..edc1fe2 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/ete.yaml @@ -0,0 +1,74 @@ +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause +# Copyright 2021, Arm Ltd +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/arm/ete.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

+title: ARM Embedded Trace Extensions

+maintainers:

• • Suzuki K Poulose suzuki.poulose@arm.com
• • Mathieu Poirier mathieu.poirier@linaro.org

+description: |

• Arm Embedded Trace Extension(ETE) is a per CPU trace component that
• allows tracing the CPU execution. It overlaps with the CoreSight ETMv4
• architecture and has extended support for future architecture changes.
• The trace generated by the ETE could be stored via legacy CoreSight
• components (e.g, TMC-ETR) or other means (e.g, using a per CPU buffer
• Arm Trace Buffer Extension (TRBE)). Since the ETE can be connected to
• legacy CoreSight components, a node must be listed per instance, along
• with any optional connection graph as per the coresight bindings.
• See bindings/arm/coresight.txt.

+properties:

• $nodename:
• pattern: "^ete([0-9a-f]+)$"
• compatible:
• items:

•  - const: arm,embedded-trace-extension
  

• cpu:

We've already established 'cpus' for this purpose.

Please see : https://lkml.kernel.org/r/9417218b-6eda-373b-a2cb-869089ffc7cd@arm.com for my response in the previous version to this and the one with out-ports.

Okay, fair enough.

...

...

...

• description: |

•  Handle to the cpu this ETE is bound to.
  

• $ref: /schemas/types.yaml#/definitions/phandle
• out-ports:
• type: object

Replace with: $ref: /schemas/graph.yaml#/properties/ports

So, just to confirm again : The CoreSight graph bindings expect the input ports and output ports grouped under in-ports{} and out-ports{} respectively to avoid having to specify the direction of the ports in the individual "port" nodes. i.e

in-ports {

property: ports OR property: port

required: OneOf: ports port

No, 'ports' as a child of in-ports is not correct. There should only be 'port(@[0-9a-f]+)?' nodes. That's why you need the above $ref added. The $ref doesn't define the node name is 'ports', but what a 'ports' or 'foo-ports' contains.

Sorry, it is my bad. We don't expect ports{} under in-ports. So your suggestion is the accurate one. I will respin.

...

}

out-ports {

# same as above }

So thats why I added out-ports as a new object, where the ports/port could be a child node.

Ideally the definition of out-ports /in-ports should go to a common schema for CoreSight bindings, when we move to Yaml for the existing bindings, which will follow in a separate series, later.

Yes, maybe, but I'm not sure something common is going to help here. You'll still have to describe what each 'port' node does in each device specific binding.

For CoreSight components the end-point of the ports are system specific. i.e, it could be anything on the other end. There is no fixed end-point connection.

e.g, ETM could be connected to a Replicator or a Funnel. Same as here above for ETE. Thus the driver must parse the endpoints and make the connection path from devices to other devices.

Anyways, will come to that in a different series.

I will fix the in-ports/out-ports for the next version.

Thanks for your guidance.

Cheers Suzuki

Rob

On Wed, Jan 27, 2021 at 02:25:31PM +0530, Anshuman Khandual wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

The context associated with an ETM for a given perf event includes :

• handle -> the perf output handle for the AUX buffer.
• the path for the trace components
• the buffer config for the sink.

The path and the buffer config are part of the "aux_priv" data (etm_event_data) setup by the setup_aux() callback, and made available via perf_get_aux(handle).

Now with a sink supporting IRQ, the sink could "end" an output handle when the buffer reaches the programmed limit and would try to restart a handle. This could fail if there is not enough space left the AUX buffer (e.g, the userspace has not consumed the data). This leaves the "handle" disconnected from the "event" and also the "perf_get_aux()" cleared. This all happens within the sink driver, without the etm_perf driver being aware. Now when the event is actually stopped, etm_event_stop() will need to access the "event_data". But since the handle is not valid anymore, we loose the information to stop the "trace" path. So, we need a reliable way to access the etm_event_data even when the handle may not be active.

This patch replaces the per_cpu handle array with a per_cpu context for the ETM, which tracks the "handle" as well as the "etm_event_data". The context notes the etm_event_data at etm_event_start() and clears it at etm_event_stop(). This makes sure that we don't access a stale "etm_event_data" as we are guaranteed that it is not freed by free_aux() as long as the event is active and tracing, also provides us with access to the critical information needed to wind up a session even in the absence of an active output_handle.

This is not an issue for the legacy sinks as none of them supports an IRQ and is centrally handled by the etm-perf.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Anshuman Khandual anshuman.khandual@arm.com Cc: Leo Yan leo.yan@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm-perf.c | 45 +++++++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index eb9e7e9..a3977b0 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -24,7 +24,26 @@ static struct pmu etm_pmu; static bool etm_perf_up; -static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle); +/*

• • An ETM context for a running event includes the perf aux handle
• • and aux_data. For ETM, the aux_data (etm_event_data), consists of
• • the trace path and the sink configuration. The event data is accessible
• • via perf_get_aux(handle). However, a sink could "end" a perf output
• • handle via the IRQ handler. And if the "sink" encounters a failure
• • to "begin" another session (e.g due to lack of space in the buffer),
• • the handle will be cleared. Thus, the event_data may not be accessible
• • from the handle when we get to the etm_event_stop(), which is required
• • for stopping the trace path. The event_data is guaranteed to stay alive
• • until "free_aux()", which cannot happen as long as the event is active on
• • the ETM. Thus the event_data for the session must be part of the ETM context
• • to make sure we can disable the trace path.
• */

+struct etm_ctxt {

• struct perf_output_handle handle;
• struct etm_event_data *event_data;

+};

+static DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt); static DEFINE_PER_CPU(struct coresight_device *, csdev_src); /* ETMv3.5/PTM's ETMCR is 'config' */ @@ -332,7 +351,8 @@ static void etm_event_start(struct perf_event *event, int flags) { int cpu = smp_processor_id(); struct etm_event_data *event_data;

• struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);

• struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
• struct perf_output_handle *handle = &ctxt->handle; struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); struct list_head *path;

if (!csdev) goto fail;

/* * Something went wrong if an event data is already associated * with a context. */ if (WARN_ONE(ctxt->event_data)) goto fail;

@@ -374,6 +394,8 @@ static void etm_event_start(struct perf_event *event, int flags) if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF)) goto fail_disable_path;

• /* Save the event_data for this ETM */
• ctxt->event_data = event_data;

out: return; @@ -392,13 +414,20 @@ static void etm_event_stop(struct perf_event *event, int mode) int cpu = smp_processor_id(); unsigned long size; struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);

• struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);
• struct etm_event_data *event_data = perf_get_aux(handle);

• struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
• struct perf_output_handle *handle = &ctxt->handle;

struct etm_event_data *event_data = perf_get_aux(handle);

struct list_head *path;

if (WARN_ON(event_data && event_data != ctxt->event_data)) return;

event_data = ctxt->event_data; /* Clear the event_data as this ETM is stopping the trace. */ ctxt->event_data = NULL;

With the above:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

if (event->hw.state == PERF_HES_STOPPED) return;

• /* We must have a valid event_data for a running event */
• if (WARN_ON(!event_data))

• return;
  

• if (!csdev) return;

@@ -416,7 +445,13 @@ static void etm_event_stop(struct perf_event *event, int mode) /* tell the core */ event->hw.state = PERF_HES_STOPPED;

• if (mode & PERF_EF_UPDATE) {

• /*

• * If the handle is not bound to an event anymore
  

• * (e.g, the sink driver was unable to restart the
  

• * handle due to lack of buffer space), we don't
  

• * have to do anything here.
  

• */
  

• if (handle->event && (mode & PERF_EF_UPDATE)) { if (WARN_ON_ONCE(handle->event != event)) return;

2.7.4

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual anshuman.khandual@arm.com wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

The context associated with an ETM for a given perf event includes :

• handle -> the perf output handle for the AUX buffer.
• the path for the trace components
• the buffer config for the sink.

The path and the buffer config are part of the "aux_priv" data (etm_event_data) setup by the setup_aux() callback, and made available via perf_get_aux(handle).

Now with a sink supporting IRQ, the sink could "end" an output handle when the buffer reaches the programmed limit and would try to restart a handle. This could fail if there is not enough space left the AUX buffer (e.g, the userspace has not consumed the data). This leaves the "handle" disconnected from the "event" and also the "perf_get_aux()" cleared. This all happens within the sink driver, without the etm_perf driver being aware. Now when the event is actually stopped, etm_event_stop() will need to access the "event_data". But since the handle is not valid anymore, we loose the information to stop the "trace" path. So, we need a reliable way to access the etm_event_data even when the handle may not be active.

This patch replaces the per_cpu handle array with a per_cpu context for the ETM, which tracks the "handle" as well as the "etm_event_data". The context notes the etm_event_data at etm_event_start() and clears it at etm_event_stop(). This makes sure that we don't access a stale "etm_event_data" as we are guaranteed that it is not freed by free_aux() as long as the event is active and tracing, also provides us with access to the critical information needed to wind up a session even in the absence of an active output_handle.

This is not an issue for the legacy sinks as none of them supports an IRQ and is centrally handled by the etm-perf.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Anshuman Khandual anshuman.khandual@arm.com Cc: Leo Yan leo.yan@linaro.org Cc: Mike Leach mike.leach@linaro.org Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

drivers/hwtracing/coresight/coresight-etm-perf.c | 45 +++++++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index eb9e7e9..a3977b0 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -24,7 +24,26 @@ static struct pmu etm_pmu; static bool etm_perf_up;

-static DEFINE_PER_CPU(struct perf_output_handle, ctx_handle); +/*

• • An ETM context for a running event includes the perf aux handle
• • and aux_data. For ETM, the aux_data (etm_event_data), consists of
• • the trace path and the sink configuration. The event data is accessible
• • via perf_get_aux(handle). However, a sink could "end" a perf output
• • handle via the IRQ handler. And if the "sink" encounters a failure
• • to "begin" another session (e.g due to lack of space in the buffer),
• • the handle will be cleared. Thus, the event_data may not be accessible
• • from the handle when we get to the etm_event_stop(), which is required
• • for stopping the trace path. The event_data is guaranteed to stay alive
• • until "free_aux()", which cannot happen as long as the event is active on
• • the ETM. Thus the event_data for the session must be part of the ETM context
• • to make sure we can disable the trace path.
• */

+struct etm_ctxt {

•   struct perf_output_handle handle;
  

•   struct etm_event_data *event_data;
  

+};

+static DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt); static DEFINE_PER_CPU(struct coresight_device *, csdev_src);

/* ETMv3.5/PTM's ETMCR is 'config' */ @@ -332,7 +351,8 @@ static void etm_event_start(struct perf_event *event, int flags) { int cpu = smp_processor_id(); struct etm_event_data *event_data;

•   struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);
  

•   struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
  

•   struct perf_output_handle *handle = &ctxt->handle;
    struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);
    struct list_head *path;
  
  

@@ -374,6 +394,8 @@ static void etm_event_start(struct perf_event *event, int flags) if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF)) goto fail_disable_path;

•   /* Save the event_data for this ETM */
  

•   ctxt->event_data = event_data;
  

out: return;

@@ -392,13 +414,20 @@ static void etm_event_stop(struct perf_event *event, int mode) int cpu = smp_processor_id(); unsigned long size; struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu);

•   struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle);
  

•   struct etm_event_data *event_data = perf_get_aux(handle);
  

•   struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
  

•   struct perf_output_handle *handle = &ctxt->handle;
  

•   struct etm_event_data *event_data = ctxt->event_data;
    struct list_head *path;
  
  

•   /* Clear the event_data as this ETM is stopping the trace. */
  

•   ctxt->event_data = NULL;
    if (event->hw.state == PERF_HES_STOPPED)
            return;
  
  

•   /* We must have a valid event_data for a running event */
  

•   if (WARN_ON(!event_data))
  

•           return;
  

•   if (!csdev)
            return;
  
  

@@ -416,7 +445,13 @@ static void etm_event_stop(struct perf_event *event, int mode) /* tell the core */ event->hw.state = PERF_HES_STOPPED;

•   if (mode & PERF_EF_UPDATE) {
  

•   /*
  

•    * If the handle is not bound to an event anymore
  

•    * (e.g, the sink driver was unable to restart the
  

•    * handle due to lack of buffer space), we don't
  

•    * have to do anything here.
  

•    */
  

•   if (handle->event && (mode & PERF_EF_UPDATE)) {
            if (WARN_ON_ONCE(handle->event != event))
                    return;
  
  

-- 2.7.4

Reviewed-by: Mike Leach mike.leach@linaro.org

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On 1/27/21 8:55 AM, Anshuman Khandual wrote:

Add support for dedicated sinks that are bound to individual CPUs. (e.g, TRBE). To allow quicker access to the sink for a given CPU bound source, keep a percpu array of the sink devices. Also, add support for building a path to the CPU local sink from the ETM.

This adds a new percpu sink type CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM. This new sink type is exclusively available and can only work with percpu source type device CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC.

This defines a percpu structure that accommodates a single coresight_device which can be used to store an initialized instance from a sink driver. As these sinks are exclusively linked and dependent on corresponding percpu sources devices, they should also be the default sink device during a perf session.

Outwards device connections are scanned while establishing paths between a source and a sink device. But such connections are not present for certain percpu source and sink devices which are exclusively linked and dependent. Build the path directly and skip connection scanning for such devices.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Updated coresight_find_default_sink()

  drivers/hwtracing/coresight/coresight-core.c | 16 ++++++++++++++-- include/linux/coresight.h | 12 ++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 0062c89..4795e28 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -23,6 +23,7 @@ #include "coresight-priv.h" static DEFINE_MUTEX(coresight_mutex); +DEFINE_PER_CPU(struct coresight_device *, csdev_sink); /**

• struct coresight_node - elements of a path, from source to sink

@@ -784,6 +785,13 @@ static int _coresight_build_path(struct coresight_device *csdev, if (csdev == sink) goto out;

• if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) &&

•    sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) {
  

• _coresight_build_path(sink, sink, path);
  

• found = true;
  

• goto out;
  

• }
• /* Not a sink - recursively explore each port found on this element */ for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child_dev;

@@ -999,8 +1007,12 @@ coresight_find_default_sink(struct coresight_device *csdev) int depth = 0; /* look for a default sink if we have not found for this device */

• if (!csdev->def_sink)

• csdev->def_sink = coresight_find_sink(csdev, &depth);
  

• if (!csdev->def_sink) {

• if (coresight_is_percpu_source(csdev))
  

• 	csdev->def_sink = per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev));
  

• if (!csdev->def_sink)
  

• 	csdev->def_sink = coresight_find_sink(csdev, &depth);
  

• } return csdev->def_sink; }

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 976ec26..bc3a5ca 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -50,6 +50,7 @@ enum coresight_dev_subtype_sink { CORESIGHT_DEV_SUBTYPE_SINK_PORT, CORESIGHT_DEV_SUBTYPE_SINK_BUFFER, CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,

• CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM, };

enum coresight_dev_subtype_link { @@ -428,6 +429,17 @@ static inline void csdev_access_write64(struct csdev_access *csa, u64 val, u32 o csa->write(val, offset, false, true); } +static inline bool coresight_is_percpu_source(struct coresight_device *csdev) +{

• return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) &&

•       csdev->subtype.source_subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_PROC;
  

Please add () around the last line. Same below.

+}

+static inline bool coresight_is_percpu_sink(struct coresight_device *csdev) +{

• return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SINK) &&

•       csdev->subtype.sink_subtype == CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
  

+} #else /* !CONFIG_64BIT */ static inline u64 csdev_access_relaxed_read64(struct csdev_access *csa,

With the above :

Tested-by: Suzuki K Poulose suzuki.poulose@arm.com Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

On 2/5/21 12:04 AM, Mathieu Poirier wrote:

On Thu, Jan 28, 2021 at 09:16:34AM +0000, Suzuki K Poulose wrote:

...

On 1/27/21 8:55 AM, Anshuman Khandual wrote:

...

Add support for dedicated sinks that are bound to individual CPUs. (e.g, TRBE). To allow quicker access to the sink for a given CPU bound source, keep a percpu array of the sink devices. Also, add support for building a path to the CPU local sink from the ETM.

This adds a new percpu sink type CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM. This new sink type is exclusively available and can only work with percpu source type device CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC.

This defines a percpu structure that accommodates a single coresight_device which can be used to store an initialized instance from a sink driver. As these sinks are exclusively linked and dependent on corresponding percpu sources devices, they should also be the default sink device during a perf session.

Outwards device connections are scanned while establishing paths between a source and a sink device. But such connections are not present for certain percpu source and sink devices which are exclusively linked and dependent. Build the path directly and skip connection scanning for such devices.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Updated coresight_find_default_sink()

  drivers/hwtracing/coresight/coresight-core.c | 16 ++++++++++++++-- include/linux/coresight.h | 12 ++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 0062c89..4795e28 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -23,6 +23,7 @@ #include "coresight-priv.h" static DEFINE_MUTEX(coresight_mutex); +DEFINE_PER_CPU(struct coresight_device *, csdev_sink); /**

• struct coresight_node - elements of a path, from source to sink

@@ -784,6 +785,13 @@ static int _coresight_build_path(struct coresight_device *csdev, if (csdev == sink) goto out;

• if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) &&

•    sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) {
  

• _coresight_build_path(sink, sink, path);
  

The return value for _coresight_build_path() needs to be checked. Otherwise a failure to allocate a node for the sink will go unoticed and make for a very hard problem to debug.

How about this instead ?

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 4795e28..e93e669 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -787,9 +787,10 @@ 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))) { - _coresight_build_path(sink, sink, path); - found = true; - goto out; + if (_coresight_build_path(sink, sink, path) == 0) { + found = true; + goto out; + } }

/* Not a sink - recursively explore each port found on this element */

...

...

• found = true;
  

• goto out;
  

• }
• /* Not a sink - recursively explore each port found on this element */ for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child_dev;

@@ -999,8 +1007,12 @@ coresight_find_default_sink(struct coresight_device *csdev) int depth = 0; /* look for a default sink if we have not found for this device */

• if (!csdev->def_sink)

• csdev->def_sink = coresight_find_sink(csdev, &depth);
  

• if (!csdev->def_sink) {

• if (coresight_is_percpu_source(csdev))
  

• 	csdev->def_sink = per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev));
  

• if (!csdev->def_sink)
  

• 	csdev->def_sink = coresight_find_sink(csdev, &depth);
  

• } return csdev->def_sink; }

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 976ec26..bc3a5ca 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -50,6 +50,7 @@ enum coresight_dev_subtype_sink { CORESIGHT_DEV_SUBTYPE_SINK_PORT, CORESIGHT_DEV_SUBTYPE_SINK_BUFFER, CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,

• CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,

Do we absolutely need to add a new sink type? It is only used in _coresight_build_path() and that code could be:

if (coresight_is_percpu_source(csdev)) { sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev));

Do you mean if (sink == per_cpu(...)) above ?

        if (sink && sink == csdev) {

How could the sink fetched from the source csdev be the same ?

I would still suggest keeping CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM for logical separation between source and sink, which also improves clarity and readability.

On 1/28/21 2:46 PM, Suzuki K Poulose wrote:

On 1/27/21 8:55 AM, Anshuman Khandual wrote:

...

Add support for dedicated sinks that are bound to individual CPUs. (e.g, TRBE). To allow quicker access to the sink for a given CPU bound source, keep a percpu array of the sink devices. Also, add support for building a path to the CPU local sink from the ETM.

This adds a new percpu sink type CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM. This new sink type is exclusively available and can only work with percpu source type device CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC.

This defines a percpu structure that accommodates a single coresight_device which can be used to store an initialized instance from a sink driver. As these sinks are exclusively linked and dependent on corresponding percpu sources devices, they should also be the default sink device during a perf session.

Outwards device connections are scanned while establishing paths between a source and a sink device. But such connections are not present for certain percpu source and sink devices which are exclusively linked and dependent. Build the path directly and skip connection scanning for such devices.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Updated coresight_find_default_sink()

drivers/hwtracing/coresight/coresight-core.c | 16 ++++++++++++++--   include/linux/coresight.h                    | 12 ++++++++++++   2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 0062c89..4795e28 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -23,6 +23,7 @@   #include "coresight-priv.h"     static DEFINE_MUTEX(coresight_mutex); +DEFINE_PER_CPU(struct coresight_device *, csdev_sink);     /**    * struct coresight_node - elements of a path, from source to sink @@ -784,6 +785,13 @@ static int _coresight_build_path(struct coresight_device *csdev,       if (csdev == sink)           goto out;   +    if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) && +        sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) { +        _coresight_build_path(sink, sink, path); +        found = true; +        goto out; +    }

/* Not a sink - recursively explore each port found on this element */       for (i = 0; i < csdev->pdata->nr_outport; i++) {           struct coresight_device *child_dev; @@ -999,8 +1007,12 @@ coresight_find_default_sink(struct coresight_device *csdev)       int depth = 0;         /* look for a default sink if we have not found for this device */ -    if (!csdev->def_sink) -        csdev->def_sink = coresight_find_sink(csdev, &depth); +    if (!csdev->def_sink) { +        if (coresight_is_percpu_source(csdev)) +            csdev->def_sink = per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev)); +        if (!csdev->def_sink) +            csdev->def_sink = coresight_find_sink(csdev, &depth); +    }       return csdev->def_sink;   }   diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 976ec26..bc3a5ca 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -50,6 +50,7 @@ enum coresight_dev_subtype_sink {       CORESIGHT_DEV_SUBTYPE_SINK_PORT,       CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,       CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM, +    CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,   };     enum coresight_dev_subtype_link { @@ -428,6 +429,17 @@ static inline void csdev_access_write64(struct csdev_access *csa, u64 val, u32 o           csa->write(val, offset, false, true);   }   +static inline bool coresight_is_percpu_source(struct coresight_device *csdev) +{ +    return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) && +           csdev->subtype.source_subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_PROC;

Please add () around the last line. Same below.

Okay, will do.

...

+}

+static inline bool coresight_is_percpu_sink(struct coresight_device *csdev) +{ +    return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SINK) && +           csdev->subtype.sink_subtype == CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;

Okay, will add here as well.

...

+}   #else    /* !CONFIG_64BIT */     static inline u64 csdev_access_relaxed_read64(struct csdev_access *csa,

With the above :

Tested-by: Suzuki K Poulose suzuki.poulose@arm.com Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

On Mon, Feb 15, 2021 at 04:27:26PM +0000, Mike Leach wrote:

HI Anshuman

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual anshuman.khandual@arm.com wrote:

...

Add support for dedicated sinks that are bound to individual CPUs. (e.g, TRBE). To allow quicker access to the sink for a given CPU bound source, keep a percpu array of the sink devices. Also, add support for building a path to the CPU local sink from the ETM.

Really need to tighten up the terminology here - I think what you mean is a PE architecturally defined sink - i.e. one that can be determined by reading the feature registers on the PE, rather than an ETR which cannot. However, the Coresight Base System Architecture specification does recommend a per cpu design using an ETR per CPU - now I assume that this case is not catered for in this patch?

...

This adds a new percpu sink type CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM. This new sink type is exclusively available and can only work with percpu source type device CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC.

CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC - this does not exist.

...

This defines a percpu structure that accommodates a single coresight_device which can be used to store an initialized instance from a sink driver. As these sinks are exclusively linked and dependent on corresponding percpu sources devices, they should also be the default sink device during a perf session.

Outwards device connections are scanned while establishing paths between a source and a sink device. But such connections are not present for certain percpu source and sink devices which are exclusively linked and dependent. Build the path directly and skip connection scanning for such devices.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Updated coresight_find_default_sink()

drivers/hwtracing/coresight/coresight-core.c | 16 ++++++++++++++-- include/linux/coresight.h | 12 ++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 0062c89..4795e28 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -23,6 +23,7 @@ #include "coresight-priv.h"

static DEFINE_MUTEX(coresight_mutex); +DEFINE_PER_CPU(struct coresight_device *, csdev_sink);

If you do indeed mean the architecturally defined sinks then this could be 'csdev_pe_arch_sink' - or something similar to indicate the reliance on the PE architecture, unless per-cpu ETR topologies are also handled here.

I would like to treat systems with one ETR per CPU the same way we do for TRBEs. That way we have two distinct way of working, i.e topologies where the sink is shared and 1:1 topologies. As such moving forward with "csdev_pe_arch_sink" could become misleading when 1:1 ETR topologies are supported.

Mathieu

...

/**

• struct coresight_node - elements of a path, from source to sink

@@ -784,6 +785,13 @@ static int _coresight_build_path(struct coresight_device *csdev, if (csdev == sink) goto out;

•   if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) &&
  

•       sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) {
  

•           _coresight_build_path(sink, sink, path);
  

•           found = true;
  

•           goto out;
  

•   }
  

•   /* Not a sink - recursively explore each port found on this element */
    for (i = 0; i < csdev->pdata->nr_outport; i++) {
            struct coresight_device *child_dev;
  

@@ -999,8 +1007,12 @@ coresight_find_default_sink(struct coresight_device *csdev) int depth = 0;

    /* look for a default sink if we have not found for this device */

•   if (!csdev->def_sink)
  

•           csdev->def_sink = coresight_find_sink(csdev, &depth);
  

•   if (!csdev->def_sink) {
  

•           if (coresight_is_percpu_source(csdev))
  

•                   csdev->def_sink = per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev));
  

•           if (!csdev->def_sink)
  

•                   csdev->def_sink = coresight_find_sink(csdev, &depth);
  

•   }
    return csdev->def_sink;
  

}

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 976ec26..bc3a5ca 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -50,6 +50,7 @@ enum coresight_dev_subtype_sink { CORESIGHT_DEV_SUBTYPE_SINK_PORT, CORESIGHT_DEV_SUBTYPE_SINK_BUFFER, CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,

•   CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
  

If this is needed then could it not be ..._SINK_SYSMEM_PROC - to be consistent with ..._SOURCE_PROC?

...

};

enum coresight_dev_subtype_link { @@ -428,6 +429,17 @@ static inline void csdev_access_write64(struct csdev_access *csa, u64 val, u32 o csa->write(val, offset, false, true); }

+static inline bool coresight_is_percpu_source(struct coresight_device *csdev)

All cpu sources are per cpu - that is ETMv3, ETMv4, PTM, ETE - this might be better as simply coresight_is_cpu_source() as all the aforementioned types will return true.

...

+{

•   return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) &&
  

•          csdev->subtype.source_subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_PROC;
  

+}

+static inline bool coresight_is_percpu_sink(struct coresight_device *csdev) +{

•   return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SINK) &&
  

•          csdev->subtype.sink_subtype == CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
  

+} #else /* !CONFIG_64BIT */

static inline u64 csdev_access_relaxed_read64(struct csdev_access *csa,

2.7.4

Regards

Mike

Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On Mon, Feb 15, 2021 at 05:58:37PM +0000, Mike Leach wrote:

Hi Mathieu,

On Mon, 15 Feb 2021 at 16:56, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

On Mon, Feb 15, 2021 at 04:27:26PM +0000, Mike Leach wrote:

...

HI Anshuman

On Wed, 27 Jan 2021 at 08:55, Anshuman Khandual anshuman.khandual@arm.com wrote:

...

Add support for dedicated sinks that are bound to individual CPUs. (e.g, TRBE). To allow quicker access to the sink for a given CPU bound source, keep a percpu array of the sink devices. Also, add support for building a path to the CPU local sink from the ETM.

Really need to tighten up the terminology here - I think what you mean is a PE architecturally defined sink - i.e. one that can be determined by reading the feature registers on the PE, rather than an ETR which cannot. However, the Coresight Base System Architecture specification does recommend a per cpu design using an ETR per CPU - now I assume that this case is not catered for in this patch?

...

This adds a new percpu sink type CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM. This new sink type is exclusively available and can only work with percpu source type device CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC.

CORESIGHT_DEV_SUBTYPE_SOURCE_PERCPU_PROC - this does not exist.

...

This defines a percpu structure that accommodates a single coresight_device which can be used to store an initialized instance from a sink driver. As these sinks are exclusively linked and dependent on corresponding percpu sources devices, they should also be the default sink device during a perf session.

Outwards device connections are scanned while establishing paths between a source and a sink device. But such connections are not present for certain percpu source and sink devices which are exclusively linked and dependent. Build the path directly and skip connection scanning for such devices.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

---

Changes in V3:

• Updated coresight_find_default_sink()

drivers/hwtracing/coresight/coresight-core.c | 16 ++++++++++++++-- include/linux/coresight.h | 12 ++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 0062c89..4795e28 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -23,6 +23,7 @@ #include "coresight-priv.h"

static DEFINE_MUTEX(coresight_mutex); +DEFINE_PER_CPU(struct coresight_device *, csdev_sink);

If you do indeed mean the architecturally defined sinks then this could be 'csdev_pe_arch_sink' - or something similar to indicate the reliance on the PE architecture, unless per-cpu ETR topologies are also handled here.

I would like to treat systems with one ETR per CPU the same way we do for TRBEs. That way we have two distinct way of working, i.e topologies where the sink is shared and 1:1 topologies. As such moving forward with "csdev_pe_arch_sink" could become misleading when 1:1 ETR topologies are supported.

Mathieu

I believe that In terms of connecting source -> sink for 1:1 ETM:ETR, then the existing code will already work via the normal build path and ports declarations. Suzukis changes in coresight-etm-perf to allow multiple sinks of the same type to be active for ETE:TRBE will also work for ETx:ETR. (at least in terms of path building - there may still be other issues that come into play about buffers etc).

The TRBE .dts doesn''t have any ports and is as such outside this framework. This patch appears to be making it detectable when connecting source -> sink where we have ETE:TRBE on a given CPU - as in the subsequent patches, the TRBE driver registers in the per cpu sink array.

So these changes are not really related to 1:1 specifically, but the detectability of PE architected sinks. There is a need for the per cpu array for TRBE as there is no other way of finding them - but not for ETR - which should work just fine without changes I think.

I thought about this further after reading the above... ETRs have ports, they are present in the DTS and are already supported. There is no point in trying to handle them the same way TRBEs are handled in this set. We can look at the (potential) advantage of doing so at a later time if the need arises but not in this set.

Thanks, Mathieu

Regards

Mike

...

...

...

/**

• struct coresight_node - elements of a path, from source to sink

@@ -784,6 +785,13 @@ static int _coresight_build_path(struct coresight_device *csdev, if (csdev == sink) goto out;

•   if (coresight_is_percpu_source(csdev) && coresight_is_percpu_sink(sink) &&
  

•       sink == per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev))) {
  

•           _coresight_build_path(sink, sink, path);
  

•           found = true;
  

•           goto out;
  

•   }
  

•   /* Not a sink - recursively explore each port found on this element */
    for (i = 0; i < csdev->pdata->nr_outport; i++) {
            struct coresight_device *child_dev;
  

@@ -999,8 +1007,12 @@ coresight_find_default_sink(struct coresight_device *csdev) int depth = 0;

    /* look for a default sink if we have not found for this device */

•   if (!csdev->def_sink)
  

•           csdev->def_sink = coresight_find_sink(csdev, &depth);
  

•   if (!csdev->def_sink) {
  

•           if (coresight_is_percpu_source(csdev))
  

•                   csdev->def_sink = per_cpu(csdev_sink, source_ops(csdev)->cpu_id(csdev));
  

•           if (!csdev->def_sink)
  

•                   csdev->def_sink = coresight_find_sink(csdev, &depth);
  

•   }
    return csdev->def_sink;
  

}

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 976ec26..bc3a5ca 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -50,6 +50,7 @@ enum coresight_dev_subtype_sink { CORESIGHT_DEV_SUBTYPE_SINK_PORT, CORESIGHT_DEV_SUBTYPE_SINK_BUFFER, CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM,

•   CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM,
  

If this is needed then could it not be ..._SINK_SYSMEM_PROC - to be consistent with ..._SOURCE_PROC?

...

};

enum coresight_dev_subtype_link { @@ -428,6 +429,17 @@ static inline void csdev_access_write64(struct csdev_access *csa, u64 val, u32 o csa->write(val, offset, false, true); }

+static inline bool coresight_is_percpu_source(struct coresight_device *csdev)

All cpu sources are per cpu - that is ETMv3, ETMv4, PTM, ETE - this might be better as simply coresight_is_cpu_source() as all the aforementioned types will return true.

...

+{

•   return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SOURCE) &&
  

•          csdev->subtype.source_subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_PROC;
  

+}

+static inline bool coresight_is_percpu_sink(struct coresight_device *csdev) +{

•   return csdev && (csdev->type == CORESIGHT_DEV_TYPE_SINK) &&
  

•          csdev->subtype.sink_subtype == CORESIGHT_DEV_SUBTYPE_SINK_PERCPU_SYSMEM;
  

+} #else /* !CONFIG_64BIT */

static inline u64 csdev_access_relaxed_read64(struct csdev_access *csa,

2.7.4

Regards

Mike

Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

On 1/27/21 8:55 AM, Anshuman Khandual wrote:

This adds TRBE related registers and corresponding feature macros.

Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Mike Leach mike.leach@linaro.org Cc: Suzuki K Poulose suzuki.poulose@arm.com

Cc: Catalin Marinas catalin.marinas@arm.com Cc: Mark Rutland mark.rutland@arm.com Cc: Will Deacon will@kernel.org

Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

---

Changes in V3:

• ID_AA64DFR0_TRBE_SHIFT has been moved here from the TRBE driver

• Changed TRBLIMITR_TRIG_MODE_SHIFT as 3

  arch/arm64/include/asm/sysreg.h | 50 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 4acff97..85ae4db 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -329,6 +329,55 @@ /*** End of Statistical Profiling Extension ***/ +/*

• • TRBE Registers
• */

+#define SYS_TRBLIMITR_EL1 sys_reg(3, 0, 9, 11, 0) +#define SYS_TRBPTR_EL1 sys_reg(3, 0, 9, 11, 1) +#define SYS_TRBBASER_EL1 sys_reg(3, 0, 9, 11, 2) +#define SYS_TRBSR_EL1 sys_reg(3, 0, 9, 11, 3) +#define SYS_TRBMAR_EL1 sys_reg(3, 0, 9, 11, 4) +#define SYS_TRBTRG_EL1 sys_reg(3, 0, 9, 11, 6) +#define SYS_TRBIDR_EL1 sys_reg(3, 0, 9, 11, 7)

+#define TRBLIMITR_LIMIT_MASK GENMASK_ULL(51, 0) +#define TRBLIMITR_LIMIT_SHIFT 12 +#define TRBLIMITR_NVM BIT(5) +#define TRBLIMITR_TRIG_MODE_MASK GENMASK(1, 0) +#define TRBLIMITR_TRIG_MODE_SHIFT 3 +#define TRBLIMITR_FILL_MODE_MASK GENMASK(1, 0) +#define TRBLIMITR_FILL_MODE_SHIFT 1 +#define TRBLIMITR_ENABLE BIT(0) +#define TRBPTR_PTR_MASK GENMASK_ULL(63, 0) +#define TRBPTR_PTR_SHIFT 0 +#define TRBBASER_BASE_MASK GENMASK_ULL(51, 0) +#define TRBBASER_BASE_SHIFT 12 +#define TRBSR_EC_MASK GENMASK(5, 0) +#define TRBSR_EC_SHIFT 26 +#define TRBSR_IRQ BIT(22) +#define TRBSR_TRG BIT(21) +#define TRBSR_WRAP BIT(20) +#define TRBSR_ABORT BIT(18) +#define TRBSR_STOP BIT(17) +#define TRBSR_MSS_MASK GENMASK(15, 0) +#define TRBSR_MSS_SHIFT 0 +#define TRBSR_BSC_MASK GENMASK(5, 0) +#define TRBSR_BSC_SHIFT 0 +#define TRBSR_FSC_MASK GENMASK(5, 0) +#define TRBSR_FSC_SHIFT 0 +#define TRBMAR_SHARE_MASK GENMASK(1, 0) +#define TRBMAR_SHARE_SHIFT 8 +#define TRBMAR_OUTER_MASK GENMASK(3, 0) +#define TRBMAR_OUTER_SHIFT 4 +#define TRBMAR_INNER_MASK GENMASK(3, 0) +#define TRBMAR_INNER_SHIFT 0 +#define TRBTRG_TRG_MASK GENMASK(31, 0) +#define TRBTRG_TRG_SHIFT 0 +#define TRBIDR_FLAG BIT(5) +#define TRBIDR_PROG BIT(4) +#define TRBIDR_ALIGN_MASK GENMASK(3, 0) +#define TRBIDR_ALIGN_SHIFT 0

• #define SYS_PMINTENSET_EL1 sys_reg(3, 0, 9, 14, 1) #define SYS_PMINTENCLR_EL1 sys_reg(3, 0, 9, 14, 2)

@@ -831,6 +880,7 @@ #define ID_AA64MMFR2_CNP_SHIFT 0 /* id_aa64dfr0 */ +#define ID_AA64DFR0_TRBE_SHIFT 44 #define ID_AA64DFR0_TRACE_FILT_SHIFT 40 #define ID_AA64DFR0_DOUBLELOCK_SHIFT 36 #define ID_AA64DFR0_PMSVER_SHIFT 32

On 2021-01-27 08:55, Anshuman Khandual wrote:

From: Suzuki K Poulose suzuki.poulose@arm.com

When the kernel is booted at EL2 in a nvhe configuration, enable the TRBE access to the EL1. The EL1 still can't trace EL2, unless EL2 permits explicitly via TRFCR_EL2.E2TRE.

Cc: Will Deacon will@kernel.org Cc: Catalin Marinas catalin.marinas@arm.com Cc: Marc Zyngier maz@kernel.org Cc: Mark Rutland mark.rutland@arm.com cc: Anshuman Khandual anshuman.khandual@arm.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

Acked-by: Marc Zyngier maz@kernel.org

One comment below, though:

---

arch/arm64/include/asm/el2_setup.h | 19 +++++++++++++++++++ arch/arm64/include/asm/kvm_arm.h | 2 ++ 2 files changed, 21 insertions(+)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h index a7f5a1b..05ecce9 100644 --- a/arch/arm64/include/asm/el2_setup.h +++ b/arch/arm64/include/asm/el2_setup.h @@ -72,6 +72,25 @@ .endif

3:

+.ifeqs "\mode", "nvhe"

• /*

• * If the Trace Buffer is available, allow
  

• * the EL1 to own it. Note that EL1 cannot
  

• * trace the EL2, as it is prevented by
  

• * TRFCR_EL2.E2TRE == 0.
  

• */
  

• ubfx x0, x1, #ID_AA64DFR0_TRBE_SHIFT, #4
• cbz x0, 1f
• mrs_s x0, SYS_TRBIDR_EL1
• and x0, x0, TRBIDR_PROG
• cbnz x0, 1f
• mov x0, #(MDCR_EL2_E2TB_EL1_OWN << MDCR_EL2_E2TB_SHIFT)
• orr x2, x2, x0

+.endif

+1:

Note that this will (badly) conflict with the late-VHE patches[1], where this code path has been reworked.

Thanks,

M.

[1] https://lore.kernel.org/r/20210125105019.2946057-1-maz@kernel.org

On 2021-01-28 09:34, Suzuki K Poulose wrote:

On 1/27/21 9:58 AM, Marc Zyngier wrote:

...

On 2021-01-27 08:55, Anshuman Khandual wrote:

...

From: Suzuki K Poulose suzuki.poulose@arm.com

When the kernel is booted at EL2 in a nvhe configuration, enable the TRBE access to the EL1. The EL1 still can't trace EL2, unless EL2 permits explicitly via TRFCR_EL2.E2TRE.

Cc: Will Deacon will@kernel.org Cc: Catalin Marinas catalin.marinas@arm.com Cc: Marc Zyngier maz@kernel.org Cc: Mark Rutland mark.rutland@arm.com cc: Anshuman Khandual anshuman.khandual@arm.com Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com

Acked-by: Marc Zyngier maz@kernel.org

One comment below, though:

...

---

arch/arm64/include/asm/el2_setup.h | 19 +++++++++++++++++++  arch/arm64/include/asm/kvm_arm.h   |  2 ++  2 files changed, 21 insertions(+)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h index a7f5a1b..05ecce9 100644 --- a/arch/arm64/include/asm/el2_setup.h +++ b/arch/arm64/include/asm/el2_setup.h @@ -72,6 +72,25 @@  .endif

3:

+.ifeqs    "\mode", "nvhe" +    /* +     * If the Trace Buffer is available, allow +     * the EL1 to own it. Note that EL1 cannot +     * trace the EL2, as it is prevented by +     * TRFCR_EL2.E2TRE == 0. +     */ +    ubfx    x0, x1, #ID_AA64DFR0_TRBE_SHIFT, #4 +    cbz    x0, 1f

+    mrs_s    x0, SYS_TRBIDR_EL1 +    and    x0, x0, TRBIDR_PROG +    cbnz    x0, 1f +    mov    x0, #(MDCR_EL2_E2TB_EL1_OWN << MDCR_EL2_E2TB_SHIFT) +    orr    x2, x2, x0 +.endif

+1:

Note that this will (badly) conflict with the late-VHE patches[1], where this code path has been reworked.

Thanks for the heads up. We will need to see how things get merged. Ideally this patch and the previous one (TRBE definitions could go via the arm64 tree / kvm tree), in which case we could rebase these two patches on the respective tree.

I think the current plan of action is to go via the arm64 tree, given that there is nothing really KVM specific there. I'll respin the series one last (hopefully!) time on Monday. Let me know if you need a hand with the rebasing.

Thanks,

M.