[PATCH 0/2] Add trace events to q6v5_pas and mdt_loader driver

[PATCH 0/2] Add trace events to q6v5_pas and mdt_loader driver

[Rishabh Bhatnagar]

Create and insert trace points in mdt_loader and qcom_q6v5_pas
drivers. These tracepoints will be used to analyze the time taken
at each step during bootup/shutdown of the remoteproc. Also 
provide information about location and size of firmware segments
being loaded.

Rishabh Bhatnagar (2):
  soc: qcom: Add tracepoints to mdt loader
  remoteproc: qcom: Add trace events for q6v5_pas driver

 drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++++
 drivers/soc/qcom/mdt_loader.c      |  8 ++++++
 include/trace/events/mdt_loader.h  | 57 ++++++++++++++++++++++++++++++++++++++
 include/trace/events/q6v5_pas.h    | 34 +++++++++++++++++++++++
 4 files changed, 110 insertions(+)
 create mode 100644 include/trace/events/mdt_loader.h
 create mode 100644 include/trace/events/q6v5_pas.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/2] soc: qcom: Add tracepoints to mdt loader

[Rishabh Bhatnagar]

Add trace events to the mdt loader driver. These events
can help us trace the region where we are loading the
segments and the time it takes to initialize the image
and setup the memory region.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/soc/qcom/mdt_loader.c     |  8 ++++++
 include/trace/events/mdt_loader.h | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 include/trace/events/mdt_loader.h

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193..df69e23 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -17,6 +17,9 @@
 #include <linux/slab.h>
 #include <linux/soc/qcom/mdt_loader.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/mdt_loader.h>
+
 static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
 {
 	if (phdr->p_type != PT_LOAD)
@@ -169,6 +172,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 			goto out;
 		}
 
+		trace_memory_setup("pas_init_image", fw_name);
 		ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len);
 
 		kfree(metadata);
@@ -196,8 +200,10 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 
 	if (relocate) {
 		if (pas_init) {
+			trace_memory_setup("pas_mem_setup", fw_name);
 			ret = qcom_scm_pas_mem_setup(pas_id, mem_phys,
 						     max_addr - min_addr);
+
 			if (ret) {
 				dev_err(dev, "unable to setup relocation\n");
 				goto out;
@@ -232,6 +238,8 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 
 		ptr = mem_region + offset;
 
+		trace_regions(ptr, phdr->p_filesz, i);
+
 		if (phdr->p_filesz && phdr->p_offset < fw->size) {
 			/* Firmware is large enough to be non-split */
 			if (phdr->p_offset + phdr->p_filesz > fw->size) {
diff --git a/include/trace/events/mdt_loader.h b/include/trace/events/mdt_loader.h
new file mode 100644
index 0000000..6299f65
--- /dev/null
+++ b/include/trace/events/mdt_loader.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM mdt_loader
+
+#if !defined(_TRACE_MDT_LOADER_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_MDT_LOADER_H
+
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(memory_setup,
+
+	TP_PROTO(const char *event, char *fw_name),
+
+	TP_ARGS(event, fw_name),
+
+	TP_STRUCT__entry(
+		__string(event, event)
+		__string(fw_name, fw_name)
+	),
+
+	TP_fast_assign(
+		__assign_str(event, event);
+		__assign_str(fw_name, fw_name);
+	),
+
+	TP_printk("doing %s for %s", __get_str(event), __get_str(fw_name))
+);
+
+TRACE_EVENT(regions,
+
+	TP_PROTO(void *region_start, size_t region_size, int i),
+
+	TP_ARGS(region_start, region_size, i),
+
+	TP_STRUCT__entry(
+		__field(void *, region_start)
+		__field(size_t, region_size)
+		__field(int, index)
+	),
+
+	TP_fast_assign(
+		__entry->region_start = region_start;
+		__entry->region_size = region_size;
+		__entry->index = i;
+	),
+
+	TP_printk("segment %d: region start=%pK size=%zx", __entry->index,
+		  __entry->region_start, __entry->region_size)
+);
+
+#endif
+#include <trace/define_trace.h>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] remoteproc: qcom: Add trace events for q6v5_pas driver

[Rishabh Bhatnagar]

Add tracepoints for q6v5_pas driver. These will help in
analyzing the time taken by each step in remoteproc
bootup/shutdown process and also serve as standard
checkpoints in code.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 11 +++++++++++
 include/trace/events/q6v5_pas.h    | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 include/trace/events/q6v5_pas.h

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23..b3c0a6a 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -29,6 +29,9 @@
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/q6v5_pas.h>
+
 struct adsp_data {
 	int crash_reason_smem;
 	const char *firmware_name;
@@ -121,12 +124,14 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
 	int ret;
 
+	trace_q6v5_pas("setting up memory and loading segments", rproc->name);
 	ret = qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
 			    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
 			    &adsp->mem_reloc);
 	if (ret)
 		return ret;
 
+	trace_q6v5_pas("done loading segments", rproc->name);
 	qcom_pil_info_store(adsp->info_name, adsp->mem_phys, adsp->mem_size);
 
 	return 0;
@@ -137,6 +142,7 @@ static int adsp_start(struct rproc *rproc)
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
 	int ret;
 
+	trace_q6v5_pas("Voting for resources", rproc->name);
 	qcom_q6v5_prepare(&adsp->q6v5);
 
 	ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
@@ -163,12 +169,14 @@ static int adsp_start(struct rproc *rproc)
 	if (ret)
 		goto disable_cx_supply;
 
+	trace_q6v5_pas("Before authenticate and reset", rproc->name);
 	ret = qcom_scm_pas_auth_and_reset(adsp->pas_id);
 	if (ret) {
 		dev_err(adsp->dev,
 			"failed to authenticate image and release reset\n");
 		goto disable_px_supply;
 	}
+	trace_q6v5_pas("After authenticate and reset", rproc->name);
 
 	ret = qcom_q6v5_wait_for_start(&adsp->q6v5, msecs_to_jiffies(5000));
 	if (ret == -ETIMEDOUT) {
@@ -177,6 +185,7 @@ static int adsp_start(struct rproc *rproc)
 		goto disable_px_supply;
 	}
 
+	trace_q6v5_pas("Remoteproc is up", rproc->name);
 	return 0;
 
 disable_px_supply:
@@ -214,6 +223,7 @@ static int adsp_stop(struct rproc *rproc)
 	int handover;
 	int ret;
 
+	trace_q6v5_pas("Request stop", rproc->name);
 	ret = qcom_q6v5_request_stop(&adsp->q6v5);
 	if (ret == -ETIMEDOUT)
 		dev_err(adsp->dev, "timed out on wait\n");
@@ -227,6 +237,7 @@ static int adsp_stop(struct rproc *rproc)
 	if (handover)
 		qcom_pas_handover(&adsp->q6v5);
 
+	trace_q6v5_pas("Remoteproc is down", rproc->name);
 	return ret;
 }
 
diff --git a/include/trace/events/q6v5_pas.h b/include/trace/events/q6v5_pas.h
new file mode 100644
index 0000000..38ee5e2
--- /dev/null
+++ b/include/trace/events/q6v5_pas.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM q6v5_pas
+
+#if !defined(_TRACE_Q6V5_PAS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_Q6V5_PAS_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(q6v5_pas,
+
+	TP_PROTO(const char *event, const char *rproc_name),
+
+	TP_ARGS(event, rproc_name),
+
+	TP_STRUCT__entry(
+		__string(event, event)
+		__string(rproc_name, rproc_name)
+	),
+
+	TP_fast_assign(
+		__assign_str(event, event);
+		__assign_str(rproc_name, rproc_name);
+	),
+
+	TP_printk("event=%s remoteproc:%s", __get_str(event), __get_str(rproc_name))
+);
+
+#endif
+#include <trace/define_trace.h>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] remoteproc: qcom: Add trace events for q6v5_pas driver

[Bjorn Andersson]

On Mon 09 Nov 19:20 CST 2020, Rishabh Bhatnagar wrote:

> Add tracepoints for q6v5_pas driver. These will help in
> analyzing the time taken by each step in remoteproc
> bootup/shutdown process and also serve as standard
> checkpoints in code.
> 

These tracepoints seems quite generic and useful to drivers other than
the Qualcomm PAS driver. Please move them into the framework instead.

> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 11 +++++++++++
>  include/trace/events/q6v5_pas.h    | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>  create mode 100644 include/trace/events/q6v5_pas.h
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 3837f23..b3c0a6a 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -29,6 +29,9 @@
>  #include "qcom_q6v5.h"
>  #include "remoteproc_internal.h"
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/q6v5_pas.h>
> +
>  struct adsp_data {
>  	int crash_reason_smem;
>  	const char *firmware_name;
> @@ -121,12 +124,14 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>  	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>  	int ret;
>  
> +	trace_q6v5_pas("setting up memory and loading segments", rproc->name);
>  	ret = qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
>  			    adsp->mem_region, adsp->mem_phys, adsp->mem_size,
>  			    &adsp->mem_reloc);
>  	if (ret)
>  		return ret;
>  
> +	trace_q6v5_pas("done loading segments", rproc->name);
>  	qcom_pil_info_store(adsp->info_name, adsp->mem_phys, adsp->mem_size);
>  
>  	return 0;
> @@ -137,6 +142,7 @@ static int adsp_start(struct rproc *rproc)
>  	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>  	int ret;
>  
> +	trace_q6v5_pas("Voting for resources", rproc->name);
>  	qcom_q6v5_prepare(&adsp->q6v5);
>  
>  	ret = adsp_pds_enable(adsp, adsp->active_pds, adsp->active_pd_count);
> @@ -163,12 +169,14 @@ static int adsp_start(struct rproc *rproc)
>  	if (ret)
>  		goto disable_cx_supply;
>  
> +	trace_q6v5_pas("Before authenticate and reset", rproc->name);
>  	ret = qcom_scm_pas_auth_and_reset(adsp->pas_id);
>  	if (ret) {
>  		dev_err(adsp->dev,
>  			"failed to authenticate image and release reset\n");
>  		goto disable_px_supply;
>  	}
> +	trace_q6v5_pas("After authenticate and reset", rproc->name);
>  
>  	ret = qcom_q6v5_wait_for_start(&adsp->q6v5, msecs_to_jiffies(5000));
>  	if (ret == -ETIMEDOUT) {
> @@ -177,6 +185,7 @@ static int adsp_start(struct rproc *rproc)
>  		goto disable_px_supply;
>  	}
>  
> +	trace_q6v5_pas("Remoteproc is up", rproc->name);
>  	return 0;
>  
>  disable_px_supply:
> @@ -214,6 +223,7 @@ static int adsp_stop(struct rproc *rproc)
>  	int handover;
>  	int ret;
>  
> +	trace_q6v5_pas("Request stop", rproc->name);
>  	ret = qcom_q6v5_request_stop(&adsp->q6v5);
>  	if (ret == -ETIMEDOUT)
>  		dev_err(adsp->dev, "timed out on wait\n");
> @@ -227,6 +237,7 @@ static int adsp_stop(struct rproc *rproc)
>  	if (handover)
>  		qcom_pas_handover(&adsp->q6v5);
>  
> +	trace_q6v5_pas("Remoteproc is down", rproc->name);
>  	return ret;
>  }
>  
> diff --git a/include/trace/events/q6v5_pas.h b/include/trace/events/q6v5_pas.h
> new file mode 100644
> index 0000000..38ee5e2
> --- /dev/null
> +++ b/include/trace/events/q6v5_pas.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM q6v5_pas
> +
> +#if !defined(_TRACE_Q6V5_PAS_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_Q6V5_PAS_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(q6v5_pas,
> +
> +	TP_PROTO(const char *event, const char *rproc_name),

Rather than distinguishing the trace events by the textual first
parameter, split it into individual trace events for each event.

Regards,
Bjorn

> +
> +	TP_ARGS(event, rproc_name),
> +
> +	TP_STRUCT__entry(
> +		__string(event, event)
> +		__string(rproc_name, rproc_name)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(event, event);
> +		__assign_str(rproc_name, rproc_name);
> +	),
> +
> +	TP_printk("event=%s remoteproc:%s", __get_str(event), __get_str(rproc_name))
> +);
> +
> +#endif
> +#include <trace/define_trace.h>
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 1/2] soc: qcom: Add tracepoints to mdt loader

[Bjorn Andersson]

On Mon 09 Nov 19:20 CST 2020, Rishabh Bhatnagar wrote:

> Add trace events to the mdt loader driver. These events
> can help us trace the region where we are loading the
> segments and the time it takes to initialize the image
> and setup the memory region.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  drivers/soc/qcom/mdt_loader.c     |  8 ++++++
>  include/trace/events/mdt_loader.h | 57 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>  create mode 100644 include/trace/events/mdt_loader.h
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 24cd193..df69e23 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -17,6 +17,9 @@
>  #include <linux/slab.h>
>  #include <linux/soc/qcom/mdt_loader.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/mdt_loader.h>
> +
>  static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
>  {
>  	if (phdr->p_type != PT_LOAD)
> @@ -169,6 +172,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  			goto out;
>  		}
>  
> +		trace_memory_setup("pas_init_image", fw_name);

I think it would be favourable if you pushed this into the PAS functions
in the scm driver instead.

>  		ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len);
>  
>  		kfree(metadata);
> @@ -196,8 +200,10 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  
>  	if (relocate) {
>  		if (pas_init) {
> +			trace_memory_setup("pas_mem_setup", fw_name);
>  			ret = qcom_scm_pas_mem_setup(pas_id, mem_phys,
>  						     max_addr - min_addr);
> +
>  			if (ret) {
>  				dev_err(dev, "unable to setup relocation\n");
>  				goto out;
> @@ -232,6 +238,8 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  
>  		ptr = mem_region + offset;
>  
> +		trace_regions(ptr, phdr->p_filesz, i);

"regions" is a very generic name for a trace event, perhaps
trace_qcom_mdt_load_segment() ?

I think it would be quite useful with a trace event indicating which
firmware mdt file (and what .bXX files) we're trying to load.

PS. ptr is a virtual address, there's no point in tracing this - we're
interested in "mem_reloc + offset".

Regards,
Bjorn

> +
>  		if (phdr->p_filesz && phdr->p_offset < fw->size) {
>  			/* Firmware is large enough to be non-split */
>  			if (phdr->p_offset + phdr->p_filesz > fw->size) {
> diff --git a/include/trace/events/mdt_loader.h b/include/trace/events/mdt_loader.h
> new file mode 100644
> index 0000000..6299f65
> --- /dev/null
> +++ b/include/trace/events/mdt_loader.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM mdt_loader
> +
> +#if !defined(_TRACE_MDT_LOADER_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_MDT_LOADER_H
> +
> +#include <linux/types.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(memory_setup,
> +
> +	TP_PROTO(const char *event, char *fw_name),
> +
> +	TP_ARGS(event, fw_name),
> +
> +	TP_STRUCT__entry(
> +		__string(event, event)
> +		__string(fw_name, fw_name)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(event, event);
> +		__assign_str(fw_name, fw_name);
> +	),
> +
> +	TP_printk("doing %s for %s", __get_str(event), __get_str(fw_name))
> +);
> +
> +TRACE_EVENT(regions,
> +
> +	TP_PROTO(void *region_start, size_t region_size, int i),
> +
> +	TP_ARGS(region_start, region_size, i),
> +
> +	TP_STRUCT__entry(
> +		__field(void *, region_start)
> +		__field(size_t, region_size)
> +		__field(int, index)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->region_start = region_start;
> +		__entry->region_size = region_size;
> +		__entry->index = i;
> +	),
> +
> +	TP_printk("segment %d: region start=%pK size=%zx", __entry->index,
> +		  __entry->region_start, __entry->region_size)
> +);
> +
> +#endif
> +#include <trace/define_trace.h>
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>