Received-SPF: pass (google.com: domain of linux-kernel+bounces-164793-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <172ab19b-4643-6ac0-53ab-d2d1e217171c@quicinc.com>
Date: Tue, 30 Apr 2024 16:18:27 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH V1] soc: qcom: smp2p: Introduce tracepoint support
To: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>, <quic_bjorande@quicinc.com>,
        <andersson@kernel.org>, <mathieu.poirier@linaro.org>
CC: <linux-kernel@vger.kernel.org>, <quic_deesin@quicinc.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-remoteproc@vger.kernel.org>,
        "Konrad
 Dybcio" <konrad.dybcio@linaro.org>
References: <20240429075528.1723133-1-quic_sudeepgo@quicinc.com>
Content-Language: en-US
From: Chris Lew <quic_clew@quicinc.com>
In-Reply-To: <20240429075528.1723133-1-quic_sudeepgo@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 4/29/2024 12:55 AM, Sudeepgoud Patil wrote:
> Introduce tracepoint support for smp2p providing useful logging
> for communication between clients.
> 

Let's add some more description as to why these tracepoint are useful. 
Do they help us track latency? debugging information for us? for clients?

> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>

As Elliot mentioned in the internal review, your Signed-off-by should be 
last. I would suggest removing Deepak's Signed-off-by and letting him 
reply with Reviewed-by since he was the main reviewer internally.

> ---
>   drivers/soc/qcom/Makefile      |  1 +
>   drivers/soc/qcom/smp2p.c       | 10 ++++
>   drivers/soc/qcom/trace-smp2p.h | 99 ++++++++++++++++++++++++++++++++++
>   3 files changed, 110 insertions(+)
>   create mode 100644 drivers/soc/qcom/trace-smp2p.h
> 
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index ca0bece0dfff..30c1bf645501 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -23,6 +23,7 @@ qcom_rpmh-y			+= rpmh.o
>   obj-$(CONFIG_QCOM_SMD_RPM)	+= rpm-proc.o smd-rpm.o
>   obj-$(CONFIG_QCOM_SMEM) +=	smem.o
>   obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> +CFLAGS_smp2p.o := -I$(src)
>   obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
>   obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>   obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index a21241cbeec7..dde8513641ae 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -20,6 +20,9 @@
>   #include <linux/soc/qcom/smem_state.h>
>   #include <linux/spinlock.h>
>   
> +#define CREATE_TRACE_POINTS
> +#include "trace-smp2p.h"
> +
>   /*
>    * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
>    * of a single 32-bit value between two processors.  Each value has a single
> @@ -191,6 +194,7 @@ static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p)
>   	struct smp2p_smem_item *out = smp2p->out;
>   	u32 val;
>   
> +	trace_smp2p_ssr_ack(smp2p->remote_pid);
>   	smp2p->ssr_ack = !smp2p->ssr_ack;
>   
>   	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);
> @@ -213,6 +217,7 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
>   			smp2p->ssr_ack_enabled = true;
>   
>   		smp2p->negotiation_done = true;
> +		trace_smp2p_negotiate(smp2p->remote_pid, smp2p->ssr_ack_enabled);

since this tracepoint is for negotiating, maybe we should capture all of 
the features (out->features) instead of just the ssr_ack feature.

>   	}
>   }
>   
> @@ -251,6 +256,8 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>   		status = val ^ entry->last_value;
>   		entry->last_value = val;
>   
> +		trace_smp2p_notify_in(smp2p->remote_pid, entry->name, status, val);
> +
>   		/* No changes of this entry? */
>   		if (!status)
>   			continue;
> @@ -406,6 +413,9 @@ static int smp2p_update_bits(void *data, u32 mask, u32 value)
>   	writel(val, entry->value);
>   	spin_unlock_irqrestore(&entry->lock, flags);
>   
> +	trace_smp2p_update_bits(entry->smp2p->remote_pid,
> +		entry->name, orig, val);
> +
>   	if (val != orig)
>   		qcom_smp2p_kick(entry->smp2p);
>   
> diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h
> new file mode 100644
> index 000000000000..c61afab23f0c
> --- /dev/null
> +++ b/drivers/soc/qcom/trace-smp2p.h
> @@ -0,0 +1,99 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM qcom_smp2p
> +
> +#if !defined(__QCOM_SMP2P_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
> +#define __QCOM_SMP2P_TRACE_H__
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(smp2p_ssr_ack,
> +	TP_PROTO(unsigned int remote_pid),

Is there any way we can map the remote pid's to a string? I feel like 
the tracepoints would be more useful if they called out modem, adsp, etc 
instead of an integer value.

> +	TP_ARGS(remote_pid),
> +	TP_STRUCT__entry(
> +		__field(u32, remote_pid)
> +	),
> +	TP_fast_assign(
> +		__entry->remote_pid = remote_pid;
> +	),
> +	TP_printk("%d: SSR detected, doing SSR Handshake",
> +		__entry->remote_pid
> +	)
> +);
> +
> +TRACE_EVENT(smp2p_negotiate,
> +	TP_PROTO(unsigned int remote_pid, bool ssr_ack_enabled),
> +	TP_ARGS(remote_pid, ssr_ack_enabled),
> +	TP_STRUCT__entry(
> +		__field(u32, remote_pid)
> +		__field(bool, ssr_ack_enabled)
> +	),
> +	TP_fast_assign(
> +		__entry->remote_pid = remote_pid;
> +		__entry->ssr_ack_enabled = ssr_ack_enabled;
> +	),
> +	TP_printk("%d: state=open ssr_ack=%d",
> +		__entry->remote_pid,
> +		__entry->ssr_ack_enabled
> +	)
> +);
> +
> +TRACE_EVENT(smp2p_notify_in,
> +	TP_PROTO(unsigned int remote_pid, const char *name, unsigned long status, u32 val),
> +	TP_ARGS(remote_pid, name, status, val),
> +	TP_STRUCT__entry(
> +		__field(u32, remote_pid)
> +		__string(name, name)
> +		__field(unsigned long, status)
> +		__field(u32, val)
> +	),
> +	TP_fast_assign(
> +		__entry->remote_pid = remote_pid;
> +		__assign_str(name, name);
> +		__entry->status = status;
> +		__entry->val = val;
> +	),
> +	TP_printk("%d: %s: status:0x%0lx val:0x%0x",
> +		__entry->remote_pid,
> +		__get_str(name),
> +		__entry->status,
> +		__entry->val
> +	)
> +);
> +
> +TRACE_EVENT(smp2p_update_bits,
> +	TP_PROTO(unsigned int remote_pid, const char *name, u32 orig, u32 val),
> +	TP_ARGS(remote_pid, name, orig, val),
> +	TP_STRUCT__entry(
> +		__field(u32, remote_pid)
> +		__string(name, name)
> +		__field(u32, orig)
> +		__field(u32, val)
> +	),
> +	TP_fast_assign(
> +		__entry->remote_pid = remote_pid;
> +		__assign_str(name, name);
> +		__entry->orig = orig;
> +		__entry->val = val;
> +	),
> +	TP_printk("%d: %s: orig:0x%0x new:0x%0x",
> +		__entry->remote_pid,
> +		__get_str(name),
> +		__entry->orig,
> +		__entry->val
> +	)
> +);
> +
> +#endif /* __QCOM_SMP2P_TRACE_H__ */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH .
> +
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE trace-smp2p
> +
> +#include <trace/define_trace.h>