diff mbox series

[V2,2/2] soc: qcom: smp2p: Introduce tracepoint support

Message ID 20240611123351.3813190-3-quic_sudeepgo@quicinc.com (mailing list archive)
State Changes Requested
Headers show
Series Add tracepoint support and remote name mapping to smp2p | expand

Commit Message

Sudeepgoud Patil June 11, 2024, 12:33 p.m. UTC 
This commit introduces tracepoint support for smp2p,
enabling logging of communication between local and remote processors.
The tracepoints include information about the remote processor ID,
remote subsystem name, negotiation details, supported features,
bit change notifications, and ssr activity.
These tracepoints are valuable for debugging issues between subsystems.

Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
---
 drivers/soc/qcom/Makefile      |   1 +
 drivers/soc/qcom/smp2p.c       |  12 ++++
 drivers/soc/qcom/trace-smp2p.h | 116 +++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/soc/qcom/trace-smp2p.h

--

Comments

Chris Lew June 11, 2024, 11:05 p.m. UTC | #1 
On 6/11/2024 5:33 AM, Sudeepgoud Patil wrote:
> This commit introduces tracepoint support for smp2p,
> enabling logging of communication between local and remote processors.
> The tracepoints include information about the remote processor ID,
> remote subsystem name, negotiation details, supported features,
> bit change notifications, and ssr activity.
> These tracepoints are valuable for debugging issues between subsystems.
> 
> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
> ---
...
> diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h
> new file mode 100644
> index 000000000000..833782460b57
> --- /dev/null
> +++ b/drivers/soc/qcom/trace-smp2p.h
> @@ -0,0 +1,116 @@
> +/* 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>
> +
> +#define SMP2P_FEATURE_SSR_ACK 0x1

Now that I see it, redefining the the feature flag here seems a bit out 
of place. I'm not sure if it's worth kicking off a header file for this 
single define though.

> +
> +TRACE_EVENT(smp2p_ssr_ack,
> +	TP_PROTO(unsigned int remote_pid, char *irq_devname),
> +	TP_ARGS(remote_pid, irq_devname),
> +	TP_STRUCT__entry(
> +		__field(u32, remote_pid)
> +		__string(irq_devname, irq_devname)
> +	),
> +	TP_fast_assign(
> +		__entry->remote_pid = remote_pid;
> +		__assign_str(irq_devname, irq_devname);
> +	),
> +	TP_printk("%d: %s: SSR detected, doing SSR Handshake",
> +		__entry->remote_pid,
> +		__get_str(irq_devname)
> +	)
> +);
> +

I don't think we need to pass remote_pid into all of the traces if we 
have a unique name "irq_devname" to identify the remote now. We could 
remove remote_pid from all the trace event arguments.

We can probably drop the "doing SSR Handshake" part of this print. I 
think it can be assumed that we're doing the handshake once we've 
detected SSR.
Deepak Kumar Singh June 12, 2024, 9:12 a.m. UTC | #2 
On 6/12/2024 4:35 AM, Chris Lew wrote:
> 
> 
> On 6/11/2024 5:33 AM, Sudeepgoud Patil wrote:
>> This commit introduces tracepoint support for smp2p,
>> enabling logging of communication between local and remote processors.
>> The tracepoints include information about the remote processor ID,
>> remote subsystem name, negotiation details, supported features,
>> bit change notifications, and ssr activity.
>> These tracepoints are valuable for debugging issues between subsystems.
>>
>> Signed-off-by: Sudeepgoud Patil <quic_sudeepgo@quicinc.com>
>> ---
> ...
>> diff --git a/drivers/soc/qcom/trace-smp2p.h 
>> b/drivers/soc/qcom/trace-smp2p.h
>> new file mode 100644
>> index 000000000000..833782460b57
>> --- /dev/null
>> +++ b/drivers/soc/qcom/trace-smp2p.h
>> @@ -0,0 +1,116 @@
>> +/* 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>
>> +
>> +#define SMP2P_FEATURE_SSR_ACK 0x1
> 
> Now that I see it, redefining the the feature flag here seems a bit out 
> of place. I'm not sure if it's worth kicking off a header file for this 
> single define though.
> 
I think it is ok to have this define in smp2p.c, as that is the only 
place where it is being used.
>> +
>> +TRACE_EVENT(smp2p_ssr_ack,
>> +    TP_PROTO(unsigned int remote_pid, char *irq_devname),
>> +    TP_ARGS(remote_pid, irq_devname),
>> +    TP_STRUCT__entry(
>> +        __field(u32, remote_pid)
>> +        __string(irq_devname, irq_devname)
>> +    ),
>> +    TP_fast_assign(
>> +        __entry->remote_pid = remote_pid;
>> +        __assign_str(irq_devname, irq_devname);
>> +    ),
>> +    TP_printk("%d: %s: SSR detected, doing SSR Handshake",
>> +        __entry->remote_pid,
>> +        __get_str(irq_devname)
>> +    )
>> +);
>> +
> 
> I don't think we need to pass remote_pid into all of the traces if we 
> have a unique name "irq_devname" to identify the remote now. We could 
> remove remote_pid from all the trace event arguments.
> 
> We can probably drop the "doing SSR Handshake" part of this print. I 
> think it can be assumed that we're doing the handshake once we've 
> detected SSR.
diff mbox series

Patch

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 a77fee048b38..6eab8ff55691 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
@@ -193,6 +196,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->irq_devname);
 	smp2p->ssr_ack = !smp2p->ssr_ack;
 
 	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);
@@ -215,6 +219,8 @@  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->irq_devname,
+				out->features);
 	}
 }
 
@@ -253,6 +259,9 @@  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, smp2p->irq_devname,
+				entry->name, status, val);
+
 		/* No changes of this entry? */
 		if (!status)
 			continue;
@@ -408,6 +417,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->smp2p->irq_devname,
+		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..833782460b57
--- /dev/null
+++ b/drivers/soc/qcom/trace-smp2p.h
@@ -0,0 +1,116 @@ 
+/* 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>
+
+#define SMP2P_FEATURE_SSR_ACK 0x1
+
+TRACE_EVENT(smp2p_ssr_ack,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname),
+	TP_ARGS(remote_pid, irq_devname),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+	),
+	TP_printk("%d: %s: SSR detected, doing SSR Handshake",
+		__entry->remote_pid,
+		__get_str(irq_devname)
+	)
+);
+
+TRACE_EVENT(smp2p_negotiate,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname, unsigned int features),
+	TP_ARGS(remote_pid, irq_devname, features),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+		__field(u32, out_features)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+		__entry->out_features = features;
+	),
+	TP_printk("%d: %s: state=open out_features=%s",
+		__entry->remote_pid,
+		__get_str(irq_devname),
+		__print_flags(__entry->out_features, "|",
+			{SMP2P_FEATURE_SSR_ACK, "SMP2P_FEATURE_SSR_ACK"})
+	)
+);
+
+TRACE_EVENT(smp2p_notify_in,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname,
+		 const char *name, unsigned long status, u32 val),
+	TP_ARGS(remote_pid, irq_devname, name, status, val),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+		__string(name, name)
+		__field(unsigned long, status)
+		__field(u32, val)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+		__assign_str(name, name);
+		__entry->status = status;
+		__entry->val = val;
+	),
+	TP_printk("%d: %s: %s: status:0x%0lx val:0x%0x",
+		__entry->remote_pid,
+		__get_str(irq_devname),
+		__get_str(name),
+		__entry->status,
+		__entry->val
+	)
+);
+
+TRACE_EVENT(smp2p_update_bits,
+	TP_PROTO(unsigned int remote_pid, char *irq_devname,
+		 const char *name, u32 orig, u32 val),
+	TP_ARGS(remote_pid, irq_devname, name, orig, val),
+	TP_STRUCT__entry(
+		__field(u32, remote_pid)
+		__string(irq_devname, irq_devname)
+		__string(name, name)
+		__field(u32, orig)
+		__field(u32, val)
+	),
+	TP_fast_assign(
+		__entry->remote_pid = remote_pid;
+		__assign_str(irq_devname, irq_devname);
+		__assign_str(name, name);
+		__entry->orig = orig;
+		__entry->val = val;
+	),
+	TP_printk("%d: %s: %s: orig:0x%0x new:0x%0x",
+		__entry->remote_pid,
+		__get_str(irq_devname),
+		__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>