Re: [PATCH V1] soc: qcom: smp2p: Introduce tracepoint support
On Tue, Apr 30, 2024 at 04:18:27PM -0700, Chris Lew wrote: > 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? +1 > > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile [..] > > 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 > > #include > > +#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. > Perhaps we can use __print_flags() in TP_printk() for that, it will attempt to resolve the bits and if it fails include the numeric value. > > } > > } [..] > > diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.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. > And __print_symbolic() for this one. Regards, Bjorn
Re: [PATCH V1] soc: qcom: smp2p: Introduce tracepoint support
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
Signed-off-by: Deepak Kumar Singh
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
#include
+#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(>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 ..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
+
+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),
+
[PATCH V1] soc: qcom: smp2p: Introduce tracepoint support
Introduce tracepoint support for smp2p providing useful logging
for communication between clients.
Signed-off-by: Sudeepgoud Patil
Signed-off-by: Deepak Kumar Singh
---
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
#include
+#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);
}
}
@@ -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(>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 ..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
+
+TRACE_EVENT(smp2p_ssr_ack,
+ TP_PROTO(unsigned int remote_pid),
+ 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),
+
