Introduce tracepoint support for smp2p providing useful logging
for communication between clients.
Signed-off-by: Sudeepgoud Patil <[email protected]>
Signed-off-by: Deepak Kumar Singh <[email protected]>
---
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);
}
}
@@ -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),
+ 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>
--
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 <[email protected]>
> Signed-off-by: Deepak Kumar Singh <[email protected]>
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>
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 <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.
>
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