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@xxxxxxxxxxxxxx>
> ---
> 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
>
