diff mbox series

[v2,2/3] firmware: scm: Add tracepoints to scm driver for pas calls

Message ID 1605563084-30385-3-git-send-email-rishabhb@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Add events to trace remoteproc lifecycle | expand

Commit Message

Rishabh Bhatnagar Nov. 16, 2020, 9:44 p.m. UTC 
Add trace events to the qcom_scm driver to trace pas calls.
These events can help us analyze the time impact for each scm
operation and can also serve as standard checkpoints in code.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
---
 drivers/firmware/qcom_scm.c     |  9 +++++++++
 include/trace/events/qcom_scm.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 include/trace/events/qcom_scm.h

Comments

Bjorn Andersson Dec. 10, 2020, 5:12 p.m. UTC | #1 
On Mon 16 Nov 15:44 CST 2020, Rishabh Bhatnagar wrote:

> Add trace events to the qcom_scm driver to trace pas calls.
> These events can help us analyze the time impact for each scm
> operation and can also serve as standard checkpoints in code.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>  drivers/firmware/qcom_scm.c     |  9 +++++++++
>  include/trace/events/qcom_scm.h | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100644 include/trace/events/qcom_scm.h
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 7be48c1..5bc9b65 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -19,6 +19,9 @@
>  
>  #include "qcom_scm.h"
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/qcom_scm.h>
> +
>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>  module_param(download_mode, bool, 0);
>  
> @@ -442,6 +445,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>  	};
>  	struct qcom_scm_res res;
>  
> +	trace_qcom_scm_call("Start scm_pas_init_image");

Please don't trace human readable strings into the trace buffer. Also,
aren't you curious about at least which @peripheral this was?

>  	/*
>  	 * During the scm call memory protection will be enabled for the meta
>  	 * data blob, so make sure it's physically contiguous, 4K aligned and
> @@ -467,6 +471,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>  
>  free_metadata:
>  	dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> +	trace_qcom_scm_call("Complete scm_pas_init_image");

Wouldn't it be useful to know if this call succeeded?

>  
>  	return ret ? : res.result[0];
>  }
> @@ -495,6 +500,7 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
>  	};
>  	struct qcom_scm_res res;
>  
> +	trace_qcom_scm_call("Start scm_pas_mem_setup");

Wouldn't it be useful to know the @peripheral, @addr and @size here?

>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -502,6 +508,7 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	qcom_scm_clk_disable();
>  
> +	trace_qcom_scm_call("Complete scm_pas_mem_setup");

Ditto.

>  	return ret ? : res.result[0];
>  }
>  EXPORT_SYMBOL(qcom_scm_pas_mem_setup);
> @@ -525,6 +532,7 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
>  	};
>  	struct qcom_scm_res res;
>  
> +	trace_qcom_scm_call("Start auth_and_reset");

Ditto.

>  	ret = qcom_scm_clk_enable();
>  	if (ret)
>  		return ret;
> @@ -532,6 +540,7 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	qcom_scm_clk_disable();
>  
> +	trace_qcom_scm_call("Complete  auth_and_reset");

Ditto.

>  	return ret ? : res.result[0];
>  }
>  EXPORT_SYMBOL(qcom_scm_pas_auth_and_reset);
> diff --git a/include/trace/events/qcom_scm.h b/include/trace/events/qcom_scm.h
> new file mode 100644
> index 0000000..d918332
> --- /dev/null
> +++ b/include/trace/events/qcom_scm.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 qcom_scm
> +
> +#if !defined(_TRACE_QCOM_SCM_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_QCOM_SCM_H
> +
> +#include <linux/types.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(qcom_scm_call,
> +
> +	TP_PROTO(const char *event),
> +
> +	TP_ARGS(event),
> +
> +	TP_STRUCT__entry(
> +		__string(event, event)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(event, event);
> +	),
> +
> +	TP_printk("qcom_scm_call event:%s", __get_str(event))

We already know that this is the qcom_scm_call trace event, so no need
to include that in the trace data.

Regards,
Bjorn

> +);
> +
> +#endif
> +#include <trace/define_trace.h>
> +
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
diff mbox series

Patch

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 7be48c1..5bc9b65 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -19,6 +19,9 @@ 
 
 #include "qcom_scm.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/qcom_scm.h>
+
 static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
 module_param(download_mode, bool, 0);
 
@@ -442,6 +445,7 @@  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
 	};
 	struct qcom_scm_res res;
 
+	trace_qcom_scm_call("Start scm_pas_init_image");
 	/*
 	 * During the scm call memory protection will be enabled for the meta
 	 * data blob, so make sure it's physically contiguous, 4K aligned and
@@ -467,6 +471,7 @@  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
 
 free_metadata:
 	dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
+	trace_qcom_scm_call("Complete scm_pas_init_image");
 
 	return ret ? : res.result[0];
 }
@@ -495,6 +500,7 @@  int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
 	};
 	struct qcom_scm_res res;
 
+	trace_qcom_scm_call("Start scm_pas_mem_setup");
 	ret = qcom_scm_clk_enable();
 	if (ret)
 		return ret;
@@ -502,6 +508,7 @@  int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_clk_disable();
 
+	trace_qcom_scm_call("Complete scm_pas_mem_setup");
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL(qcom_scm_pas_mem_setup);
@@ -525,6 +532,7 @@  int qcom_scm_pas_auth_and_reset(u32 peripheral)
 	};
 	struct qcom_scm_res res;
 
+	trace_qcom_scm_call("Start auth_and_reset");
 	ret = qcom_scm_clk_enable();
 	if (ret)
 		return ret;
@@ -532,6 +540,7 @@  int qcom_scm_pas_auth_and_reset(u32 peripheral)
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_clk_disable();
 
+	trace_qcom_scm_call("Complete  auth_and_reset");
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL(qcom_scm_pas_auth_and_reset);
diff --git a/include/trace/events/qcom_scm.h b/include/trace/events/qcom_scm.h
new file mode 100644
index 0000000..d918332
--- /dev/null
+++ b/include/trace/events/qcom_scm.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 qcom_scm
+
+#if !defined(_TRACE_QCOM_SCM_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_QCOM_SCM_H
+
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(qcom_scm_call,
+
+	TP_PROTO(const char *event),
+
+	TP_ARGS(event),
+
+	TP_STRUCT__entry(
+		__string(event, event)
+	),
+
+	TP_fast_assign(
+		__assign_str(event, event);
+	),
+
+	TP_printk("qcom_scm_call event:%s", __get_str(event))
+);
+
+#endif
+#include <trace/define_trace.h>
+