Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 10 Dec 2020 11:12:29 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Rishabh Bhatnagar <rishabhb@codeaurora.org>
Cc:     linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        tsoni@codeaurora.org, psodagud@codeaurora.org,
        sidgup@codeaurora.org
Subject: Re: [PATCH v2 2/3] firmware: scm: Add tracepoints to scm driver for
 pas calls
Message-ID: <X9JW/ZQVqp2kIivE@builder.lan>
References: <1605563084-30385-1-git-send-email-rishabhb@codeaurora.org>
 <1605563084-30385-3-git-send-email-rishabhb@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1605563084-30385-3-git-send-email-rishabhb@codeaurora.org>
Precedence: bulk

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
>