All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] stm: class: Add MIPI OST protocol support
@ 2023-02-08 12:40 ` Mao Jinlong
  0 siblings, 0 replies; 10+ messages in thread
From: Mao Jinlong @ 2023-02-08 12:40 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: Mao Jinlong, linux-kernel, linux-arm-msm, linux-stm32,
	linux-arm-kernel, Tingwei Zhang, Yuanfang Zhang, Tao Zhang,
	Hao Zhang

Add MIPI OST protocol support for stm to format the traces.
Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
support").

Signed-off-by: Tingwei Zhang <quic_tingweiz@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/stm/Kconfig  | 14 +++++
 drivers/hwtracing/stm/Makefile |  2 +
 drivers/hwtracing/stm/p_ost.c  | 95 ++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 drivers/hwtracing/stm/p_ost.c

diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index aad594fe79cc..0b52901125f7 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -41,6 +41,20 @@ config STM_PROTO_SYS_T
 
 	  If you don't know what this is, say N.
 
+config STM_PROTO_OST
+	tristate "MIPI OST STM framing protocol driver"
+	default CONFIG_STM
+	help
+	  This is an implementation of MIPI OST protocol to be used
+	  over the STP transport. In addition to the data payload, it
+	  also carries additional metadata for entity, better
+	  means of trace source identification, etc.
+
+	  The receiving side must be able to decode this protocol in
+	  addition to the MIPI STP, in order to extract the data.
+
+	  If you don't know what this is, say N.
+
 config STM_DUMMY
 	tristate "Dummy STM driver"
 	help
diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
index 1692fcd29277..715fc721891e 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -5,9 +5,11 @@ stm_core-y		:= core.o policy.o
 
 obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
 obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o
+obj-$(CONFIG_STM_PROTO_OST) += stm_p_ost.o
 
 stm_p_basic-y		:= p_basic.o
 stm_p_sys-t-y		:= p_sys-t.o
+stm_p_ost-y		:= p_ost.o
 
 obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
 
diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
new file mode 100644
index 000000000000..2ca1a3fda57f
--- /dev/null
+++ b/drivers/hwtracing/stm/p_ost.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
+ * ("stm class: Add MIPI SyS-T protocol support").
+ *
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2020 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, Intel Corporation.
+ *
+ * MIPI OST framing protocol for STM devices.
+ */
+
+#include <linux/configfs.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/stm.h>
+#include <linux/sched/clock.h>
+#include "stm.h"
+
+#define OST_TOKEN_STARTSIMPLE		(0x10)
+#define OST_VERSION_MIPI1		(0x10 << 8)
+#define OST_ENTITY_FTRACE		(0x01 << 16)
+#define OST_CONTROL_PROTOCOL		(0x0 << 24)
+
+#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
+			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
+
+#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
+#define STM_HEADER_MAGIC		(0x5953)
+
+static ssize_t notrace ost_write(struct stm_data *data,
+		struct stm_output *output, unsigned int chan,
+		const char *buf, size_t count)
+{
+	unsigned int c = output->channel + chan;
+	unsigned int m = output->master;
+	const unsigned char nil = 0;
+	u32 header = DATA_HEADER;
+	u8 trc_hdr[24];
+	ssize_t sz;
+
+	/*
+	 * STP framing rules for OST frames:
+	 *   * the first packet of the OST frame is marked;
+	 *   * the last packet is a FLAG.
+	 */
+	/* Message layout: HEADER / DATA / TAIL */
+	/* HEADER */
+
+	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
+			  4, (u8 *)&header);
+	if (sz <= 0)
+		return sz;
+	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
+	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
+	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
+	*(uint64_t *)(trc_hdr + 8) = sched_clock();
+	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
+	sz = stm_data_write(data, m, c, false, trc_hdr, sizeof(trc_hdr));
+	if (sz <= 0)
+		return sz;
+
+	/* DATA */
+	sz = stm_data_write(data, m, c, false, buf, count);
+
+	/* TAIL */
+	if (sz > 0)
+		data->packet(data, m, c, STP_PACKET_FLAG,
+			STP_PACKET_TIMESTAMPED, 0, &nil);
+
+	return sz;
+}
+
+static const struct stm_protocol_driver ost_pdrv = {
+	.owner			= THIS_MODULE,
+	.name			= "p_ost",
+	.write			= ost_write,
+};
+
+static int ost_stm_init(void)
+{
+	return stm_register_protocol(&ost_pdrv);
+}
+
+static void ost_stm_exit(void)
+{
+	stm_unregister_protocol(&ost_pdrv);
+}
+
+module_init(ost_stm_init);
+module_exit(ost_stm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MIPI Open System Trace STM framing protocol driver");
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] stm: class: Add MIPI OST protocol support
@ 2023-02-08 12:40 ` Mao Jinlong
  0 siblings, 0 replies; 10+ messages in thread
From: Mao Jinlong @ 2023-02-08 12:40 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: Mao Jinlong, linux-kernel, linux-arm-msm, linux-stm32,
	linux-arm-kernel, Tingwei Zhang, Yuanfang Zhang, Tao Zhang,
	Hao Zhang

Add MIPI OST protocol support for stm to format the traces.
Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
support").

Signed-off-by: Tingwei Zhang <quic_tingweiz@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/stm/Kconfig  | 14 +++++
 drivers/hwtracing/stm/Makefile |  2 +
 drivers/hwtracing/stm/p_ost.c  | 95 ++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 drivers/hwtracing/stm/p_ost.c

diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index aad594fe79cc..0b52901125f7 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -41,6 +41,20 @@ config STM_PROTO_SYS_T
 
 	  If you don't know what this is, say N.
 
+config STM_PROTO_OST
+	tristate "MIPI OST STM framing protocol driver"
+	default CONFIG_STM
+	help
+	  This is an implementation of MIPI OST protocol to be used
+	  over the STP transport. In addition to the data payload, it
+	  also carries additional metadata for entity, better
+	  means of trace source identification, etc.
+
+	  The receiving side must be able to decode this protocol in
+	  addition to the MIPI STP, in order to extract the data.
+
+	  If you don't know what this is, say N.
+
 config STM_DUMMY
 	tristate "Dummy STM driver"
 	help
diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
index 1692fcd29277..715fc721891e 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -5,9 +5,11 @@ stm_core-y		:= core.o policy.o
 
 obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
 obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o
+obj-$(CONFIG_STM_PROTO_OST) += stm_p_ost.o
 
 stm_p_basic-y		:= p_basic.o
 stm_p_sys-t-y		:= p_sys-t.o
+stm_p_ost-y		:= p_ost.o
 
 obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
 
diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
new file mode 100644
index 000000000000..2ca1a3fda57f
--- /dev/null
+++ b/drivers/hwtracing/stm/p_ost.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
+ * ("stm class: Add MIPI SyS-T protocol support").
+ *
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2020 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, Intel Corporation.
+ *
+ * MIPI OST framing protocol for STM devices.
+ */
+
+#include <linux/configfs.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/stm.h>
+#include <linux/sched/clock.h>
+#include "stm.h"
+
+#define OST_TOKEN_STARTSIMPLE		(0x10)
+#define OST_VERSION_MIPI1		(0x10 << 8)
+#define OST_ENTITY_FTRACE		(0x01 << 16)
+#define OST_CONTROL_PROTOCOL		(0x0 << 24)
+
+#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
+			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
+
+#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
+#define STM_HEADER_MAGIC		(0x5953)
+
+static ssize_t notrace ost_write(struct stm_data *data,
+		struct stm_output *output, unsigned int chan,
+		const char *buf, size_t count)
+{
+	unsigned int c = output->channel + chan;
+	unsigned int m = output->master;
+	const unsigned char nil = 0;
+	u32 header = DATA_HEADER;
+	u8 trc_hdr[24];
+	ssize_t sz;
+
+	/*
+	 * STP framing rules for OST frames:
+	 *   * the first packet of the OST frame is marked;
+	 *   * the last packet is a FLAG.
+	 */
+	/* Message layout: HEADER / DATA / TAIL */
+	/* HEADER */
+
+	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
+			  4, (u8 *)&header);
+	if (sz <= 0)
+		return sz;
+	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
+	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
+	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
+	*(uint64_t *)(trc_hdr + 8) = sched_clock();
+	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
+	sz = stm_data_write(data, m, c, false, trc_hdr, sizeof(trc_hdr));
+	if (sz <= 0)
+		return sz;
+
+	/* DATA */
+	sz = stm_data_write(data, m, c, false, buf, count);
+
+	/* TAIL */
+	if (sz > 0)
+		data->packet(data, m, c, STP_PACKET_FLAG,
+			STP_PACKET_TIMESTAMPED, 0, &nil);
+
+	return sz;
+}
+
+static const struct stm_protocol_driver ost_pdrv = {
+	.owner			= THIS_MODULE,
+	.name			= "p_ost",
+	.write			= ost_write,
+};
+
+static int ost_stm_init(void)
+{
+	return stm_register_protocol(&ost_pdrv);
+}
+
+static void ost_stm_exit(void)
+{
+	stm_unregister_protocol(&ost_pdrv);
+}
+
+module_init(ost_stm_init);
+module_exit(ost_stm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MIPI Open System Trace STM framing protocol driver");
-- 
2.39.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
  2023-02-08 12:40 ` Mao Jinlong
@ 2023-03-02 12:29   ` Jinlong Mao
  -1 siblings, 0 replies; 10+ messages in thread
From: Jinlong Mao @ 2023-03-02 12:29 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: linux-kernel, linux-arm-msm, linux-stm32, linux-arm-kernel,
	Tingwei Zhang, Yuanfang Zhang, Tao Zhang, Hao Zhang

Hi Reviewers,

Could you please help to review this patch ?

Thanks
Jinlong Mao

On 2/8/2023 8:40 PM, Mao Jinlong wrote:
> Add MIPI OST protocol support for stm to format the traces.
> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
> support").
>
> Signed-off-by: Tingwei Zhang <quic_tingweiz@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   drivers/hwtracing/stm/Kconfig  | 14 +++++
>   drivers/hwtracing/stm/Makefile |  2 +
>   drivers/hwtracing/stm/p_ost.c  | 95 ++++++++++++++++++++++++++++++++++
>   3 files changed, 111 insertions(+)
>   create mode 100644 drivers/hwtracing/stm/p_ost.c
>
> diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
> index aad594fe79cc..0b52901125f7 100644
> --- a/drivers/hwtracing/stm/Kconfig
> +++ b/drivers/hwtracing/stm/Kconfig
> @@ -41,6 +41,20 @@ config STM_PROTO_SYS_T
>   
>   	  If you don't know what this is, say N.
>   
> +config STM_PROTO_OST
> +	tristate "MIPI OST STM framing protocol driver"
> +	default CONFIG_STM
> +	help
> +	  This is an implementation of MIPI OST protocol to be used
> +	  over the STP transport. In addition to the data payload, it
> +	  also carries additional metadata for entity, better
> +	  means of trace source identification, etc.
> +
> +	  The receiving side must be able to decode this protocol in
> +	  addition to the MIPI STP, in order to extract the data.
> +
> +	  If you don't know what this is, say N.
> +
>   config STM_DUMMY
>   	tristate "Dummy STM driver"
>   	help
> diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
> index 1692fcd29277..715fc721891e 100644
> --- a/drivers/hwtracing/stm/Makefile
> +++ b/drivers/hwtracing/stm/Makefile
> @@ -5,9 +5,11 @@ stm_core-y		:= core.o policy.o
>   
>   obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
>   obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o
> +obj-$(CONFIG_STM_PROTO_OST) += stm_p_ost.o
>   
>   stm_p_basic-y		:= p_basic.o
>   stm_p_sys-t-y		:= p_sys-t.o
> +stm_p_ost-y		:= p_ost.o
>   
>   obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
>   
> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
> new file mode 100644
> index 000000000000..2ca1a3fda57f
> --- /dev/null
> +++ b/drivers/hwtracing/stm/p_ost.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
> + * ("stm class: Add MIPI SyS-T protocol support").
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2018, Intel Corporation.
> + *
> + * MIPI OST framing protocol for STM devices.
> + */
> +
> +#include <linux/configfs.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/stm.h>
> +#include <linux/sched/clock.h>
> +#include "stm.h"
> +
> +#define OST_TOKEN_STARTSIMPLE		(0x10)
> +#define OST_VERSION_MIPI1		(0x10 << 8)
> +#define OST_ENTITY_FTRACE		(0x01 << 16)
> +#define OST_CONTROL_PROTOCOL		(0x0 << 24)
> +
> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
> +			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
> +
> +#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
> +#define STM_HEADER_MAGIC		(0x5953)
> +
> +static ssize_t notrace ost_write(struct stm_data *data,
> +		struct stm_output *output, unsigned int chan,
> +		const char *buf, size_t count)
> +{
> +	unsigned int c = output->channel + chan;
> +	unsigned int m = output->master;
> +	const unsigned char nil = 0;
> +	u32 header = DATA_HEADER;
> +	u8 trc_hdr[24];
> +	ssize_t sz;
> +
> +	/*
> +	 * STP framing rules for OST frames:
> +	 *   * the first packet of the OST frame is marked;
> +	 *   * the last packet is a FLAG.
> +	 */
> +	/* Message layout: HEADER / DATA / TAIL */
> +	/* HEADER */
> +
> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
> +			  4, (u8 *)&header);
> +	if (sz <= 0)
> +		return sz;
> +	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
> +	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
> +	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
> +	*(uint64_t *)(trc_hdr + 8) = sched_clock();
> +	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
> +	sz = stm_data_write(data, m, c, false, trc_hdr, sizeof(trc_hdr));
> +	if (sz <= 0)
> +		return sz;
> +
> +	/* DATA */
> +	sz = stm_data_write(data, m, c, false, buf, count);
> +
> +	/* TAIL */
> +	if (sz > 0)
> +		data->packet(data, m, c, STP_PACKET_FLAG,
> +			STP_PACKET_TIMESTAMPED, 0, &nil);
> +
> +	return sz;
> +}
> +
> +static const struct stm_protocol_driver ost_pdrv = {
> +	.owner			= THIS_MODULE,
> +	.name			= "p_ost",
> +	.write			= ost_write,
> +};
> +
> +static int ost_stm_init(void)
> +{
> +	return stm_register_protocol(&ost_pdrv);
> +}
> +
> +static void ost_stm_exit(void)
> +{
> +	stm_unregister_protocol(&ost_pdrv);
> +}
> +
> +module_init(ost_stm_init);
> +module_exit(ost_stm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("MIPI Open System Trace STM framing protocol driver");

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
@ 2023-03-02 12:29   ` Jinlong Mao
  0 siblings, 0 replies; 10+ messages in thread
From: Jinlong Mao @ 2023-03-02 12:29 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: linux-kernel, linux-arm-msm, linux-stm32, linux-arm-kernel,
	Tingwei Zhang, Yuanfang Zhang, Tao Zhang, Hao Zhang

Hi Reviewers,

Could you please help to review this patch ?

Thanks
Jinlong Mao

On 2/8/2023 8:40 PM, Mao Jinlong wrote:
> Add MIPI OST protocol support for stm to format the traces.
> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
> support").
>
> Signed-off-by: Tingwei Zhang <quic_tingweiz@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   drivers/hwtracing/stm/Kconfig  | 14 +++++
>   drivers/hwtracing/stm/Makefile |  2 +
>   drivers/hwtracing/stm/p_ost.c  | 95 ++++++++++++++++++++++++++++++++++
>   3 files changed, 111 insertions(+)
>   create mode 100644 drivers/hwtracing/stm/p_ost.c
>
> diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
> index aad594fe79cc..0b52901125f7 100644
> --- a/drivers/hwtracing/stm/Kconfig
> +++ b/drivers/hwtracing/stm/Kconfig
> @@ -41,6 +41,20 @@ config STM_PROTO_SYS_T
>   
>   	  If you don't know what this is, say N.
>   
> +config STM_PROTO_OST
> +	tristate "MIPI OST STM framing protocol driver"
> +	default CONFIG_STM
> +	help
> +	  This is an implementation of MIPI OST protocol to be used
> +	  over the STP transport. In addition to the data payload, it
> +	  also carries additional metadata for entity, better
> +	  means of trace source identification, etc.
> +
> +	  The receiving side must be able to decode this protocol in
> +	  addition to the MIPI STP, in order to extract the data.
> +
> +	  If you don't know what this is, say N.
> +
>   config STM_DUMMY
>   	tristate "Dummy STM driver"
>   	help
> diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
> index 1692fcd29277..715fc721891e 100644
> --- a/drivers/hwtracing/stm/Makefile
> +++ b/drivers/hwtracing/stm/Makefile
> @@ -5,9 +5,11 @@ stm_core-y		:= core.o policy.o
>   
>   obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
>   obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o
> +obj-$(CONFIG_STM_PROTO_OST) += stm_p_ost.o
>   
>   stm_p_basic-y		:= p_basic.o
>   stm_p_sys-t-y		:= p_sys-t.o
> +stm_p_ost-y		:= p_ost.o
>   
>   obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
>   
> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
> new file mode 100644
> index 000000000000..2ca1a3fda57f
> --- /dev/null
> +++ b/drivers/hwtracing/stm/p_ost.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
> + * ("stm class: Add MIPI SyS-T protocol support").
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2018, Intel Corporation.
> + *
> + * MIPI OST framing protocol for STM devices.
> + */
> +
> +#include <linux/configfs.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/stm.h>
> +#include <linux/sched/clock.h>
> +#include "stm.h"
> +
> +#define OST_TOKEN_STARTSIMPLE		(0x10)
> +#define OST_VERSION_MIPI1		(0x10 << 8)
> +#define OST_ENTITY_FTRACE		(0x01 << 16)
> +#define OST_CONTROL_PROTOCOL		(0x0 << 24)
> +
> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
> +			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
> +
> +#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
> +#define STM_HEADER_MAGIC		(0x5953)
> +
> +static ssize_t notrace ost_write(struct stm_data *data,
> +		struct stm_output *output, unsigned int chan,
> +		const char *buf, size_t count)
> +{
> +	unsigned int c = output->channel + chan;
> +	unsigned int m = output->master;
> +	const unsigned char nil = 0;
> +	u32 header = DATA_HEADER;
> +	u8 trc_hdr[24];
> +	ssize_t sz;
> +
> +	/*
> +	 * STP framing rules for OST frames:
> +	 *   * the first packet of the OST frame is marked;
> +	 *   * the last packet is a FLAG.
> +	 */
> +	/* Message layout: HEADER / DATA / TAIL */
> +	/* HEADER */
> +
> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
> +			  4, (u8 *)&header);
> +	if (sz <= 0)
> +		return sz;
> +	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
> +	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
> +	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
> +	*(uint64_t *)(trc_hdr + 8) = sched_clock();
> +	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
> +	sz = stm_data_write(data, m, c, false, trc_hdr, sizeof(trc_hdr));
> +	if (sz <= 0)
> +		return sz;
> +
> +	/* DATA */
> +	sz = stm_data_write(data, m, c, false, buf, count);
> +
> +	/* TAIL */
> +	if (sz > 0)
> +		data->packet(data, m, c, STP_PACKET_FLAG,
> +			STP_PACKET_TIMESTAMPED, 0, &nil);
> +
> +	return sz;
> +}
> +
> +static const struct stm_protocol_driver ost_pdrv = {
> +	.owner			= THIS_MODULE,
> +	.name			= "p_ost",
> +	.write			= ost_write,
> +};
> +
> +static int ost_stm_init(void)
> +{
> +	return stm_register_protocol(&ost_pdrv);
> +}
> +
> +static void ost_stm_exit(void)
> +{
> +	stm_unregister_protocol(&ost_pdrv);
> +}
> +
> +module_init(ost_stm_init);
> +module_exit(ost_stm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("MIPI Open System Trace STM framing protocol driver");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
  2023-02-08 12:40 ` Mao Jinlong
@ 2023-03-02 18:05   ` Alexander Shishkin
  -1 siblings, 0 replies; 10+ messages in thread
From: Alexander Shishkin @ 2023-03-02 18:05 UTC (permalink / raw)
  To: Mao Jinlong, Maxime Coquelin, Alexandre Torgue
  Cc: Mao Jinlong, linux-kernel, linux-arm-msm, linux-stm32,
	linux-arm-kernel, Tingwei Zhang, Yuanfang Zhang, Tao Zhang,
	Hao Zhang, alexander.shishkin

Mao Jinlong <quic_jinlmao@quicinc.com> writes:

> Add MIPI OST protocol support for stm to format the traces.

Missing an explanation of what OST is, what it's used for, how it is
different from the SyS-T and others.

> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of

You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.

> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
> support").

Why is this significant?

> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
> new file mode 100644
> index 000000000000..2ca1a3fda57f
> --- /dev/null
> +++ b/drivers/hwtracing/stm/p_ost.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
> + * ("stm class: Add MIPI SyS-T protocol support").

Same as in the commit message.

[...]

> +#define OST_TOKEN_STARTSIMPLE		(0x10)
> +#define OST_VERSION_MIPI1		(0x10 << 8)
> +#define OST_ENTITY_FTRACE		(0x01 << 16)
> +#define OST_CONTROL_PROTOCOL		(0x0 << 24)

These could use an explanation.

> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
> +			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)

Does this mean that everything is ftrace? Because it's not.

> +
> +#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
> +#define STM_HEADER_MAGIC		(0x5953)
> +
> +static ssize_t notrace ost_write(struct stm_data *data,
> +		struct stm_output *output, unsigned int chan,
> +		const char *buf, size_t count)
> +{
> +	unsigned int c = output->channel + chan;
> +	unsigned int m = output->master;
> +	const unsigned char nil = 0;
> +	u32 header = DATA_HEADER;
> +	u8 trc_hdr[24];
> +	ssize_t sz;
> +
> +	/*
> +	 * STP framing rules for OST frames:
> +	 *   * the first packet of the OST frame is marked;
> +	 *   * the last packet is a FLAG.

Which in your case is also timestamped.

> +	 */
> +	/* Message layout: HEADER / DATA / TAIL */
> +	/* HEADER */
> +
> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
> +			  4, (u8 *)&header);

The /* HEADER */ comment applies to the above line, so it should
probably be directly before it.

> +	if (sz <= 0)
> +		return sz;
> +	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
> +	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
> +	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
> +	*(uint64_t *)(trc_hdr + 8) = sched_clock();

Why sched_clock()? It should, among other things, be called with
interrupts disabled, which is not the case here.

> +	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());

Is there a reason why trc_hdr is not a struct?

Thanks,
--
Alex

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
@ 2023-03-02 18:05   ` Alexander Shishkin
  0 siblings, 0 replies; 10+ messages in thread
From: Alexander Shishkin @ 2023-03-02 18:05 UTC (permalink / raw)
  To: Mao Jinlong, Maxime Coquelin, Alexandre Torgue
  Cc: Mao Jinlong, linux-kernel, linux-arm-msm, linux-stm32,
	linux-arm-kernel, Tingwei Zhang, Yuanfang Zhang, Tao Zhang,
	Hao Zhang, alexander.shishkin

Mao Jinlong <quic_jinlmao@quicinc.com> writes:

> Add MIPI OST protocol support for stm to format the traces.

Missing an explanation of what OST is, what it's used for, how it is
different from the SyS-T and others.

> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of

You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.

> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
> support").

Why is this significant?

> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
> new file mode 100644
> index 000000000000..2ca1a3fda57f
> --- /dev/null
> +++ b/drivers/hwtracing/stm/p_ost.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
> + * ("stm class: Add MIPI SyS-T protocol support").

Same as in the commit message.

[...]

> +#define OST_TOKEN_STARTSIMPLE		(0x10)
> +#define OST_VERSION_MIPI1		(0x10 << 8)
> +#define OST_ENTITY_FTRACE		(0x01 << 16)
> +#define OST_CONTROL_PROTOCOL		(0x0 << 24)

These could use an explanation.

> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
> +			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)

Does this mean that everything is ftrace? Because it's not.

> +
> +#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
> +#define STM_HEADER_MAGIC		(0x5953)
> +
> +static ssize_t notrace ost_write(struct stm_data *data,
> +		struct stm_output *output, unsigned int chan,
> +		const char *buf, size_t count)
> +{
> +	unsigned int c = output->channel + chan;
> +	unsigned int m = output->master;
> +	const unsigned char nil = 0;
> +	u32 header = DATA_HEADER;
> +	u8 trc_hdr[24];
> +	ssize_t sz;
> +
> +	/*
> +	 * STP framing rules for OST frames:
> +	 *   * the first packet of the OST frame is marked;
> +	 *   * the last packet is a FLAG.

Which in your case is also timestamped.

> +	 */
> +	/* Message layout: HEADER / DATA / TAIL */
> +	/* HEADER */
> +
> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
> +			  4, (u8 *)&header);

The /* HEADER */ comment applies to the above line, so it should
probably be directly before it.

> +	if (sz <= 0)
> +		return sz;
> +	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
> +	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
> +	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
> +	*(uint64_t *)(trc_hdr + 8) = sched_clock();

Why sched_clock()? It should, among other things, be called with
interrupts disabled, which is not the case here.

> +	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());

Is there a reason why trc_hdr is not a struct?

Thanks,
--
Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
  2023-03-02 18:05   ` Alexander Shishkin
@ 2023-03-07 13:26     ` Jinlong Mao
  -1 siblings, 0 replies; 10+ messages in thread
From: Jinlong Mao @ 2023-03-07 13:26 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: linux-kernel, linux-arm-msm, linux-stm32, linux-arm-kernel,
	Tingwei Zhang, Yuanfang Zhang, Tao Zhang, Hao Zhang

Hi Alexande,

On 3/3/2023 2:05 AM, Alexander Shishkin wrote:
> Mao Jinlong <quic_jinlmao@quicinc.com> writes:
>
>> Add MIPI OST protocol support for stm to format the traces.
> Missing an explanation of what OST is, what it's used for, how it is
> different from the SyS-T and others.
I will updae the explanation in next version.
>
>> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
> You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.

The driver refers to code structure of p_sys-t driver. So, add this 
comments after
internal review.

>
>> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
>> support").
> Why is this significant?
>
>> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
>> new file mode 100644
>> index 000000000000..2ca1a3fda57f
>> --- /dev/null
>> +++ b/drivers/hwtracing/stm/p_ost.c
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
>> + * ("stm class: Add MIPI SyS-T protocol support").
> Same as in the commit message.
>
> [...]
>
>> +#define OST_TOKEN_STARTSIMPLE		(0x10)
>> +#define OST_VERSION_MIPI1		(0x10 << 8)
>> +#define OST_ENTITY_FTRACE		(0x01 << 16)
>> +#define OST_CONTROL_PROTOCOL		(0x0 << 24)
> These could use an explanation.
I will add the explanation.
>> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
>> +			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
> Does this mean that everything is ftrace? Because it's not.
Only ftrace is supported in p_ost now. Other header type will be added 
later.
>
>> +
>> +#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
>> +#define STM_HEADER_MAGIC		(0x5953)
>> +
>> +static ssize_t notrace ost_write(struct stm_data *data,
>> +		struct stm_output *output, unsigned int chan,
>> +		const char *buf, size_t count)
>> +{
>> +	unsigned int c = output->channel + chan;
>> +	unsigned int m = output->master;
>> +	const unsigned char nil = 0;
>> +	u32 header = DATA_HEADER;
>> +	u8 trc_hdr[24];
>> +	ssize_t sz;
>> +
>> +	/*
>> +	 * STP framing rules for OST frames:
>> +	 *   * the first packet of the OST frame is marked;
>> +	 *   * the last packet is a FLAG.
> Which in your case is also timestamped.
I will add the comments.
>
>> +	 */
>> +	/* Message layout: HEADER / DATA / TAIL */
>> +	/* HEADER */
>> +
>> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
>> +			  4, (u8 *)&header);
> The /* HEADER */ comment applies to the above line, so it should
> probably be directly before it.
Got it.
>
>> +	if (sz <= 0)
>> +		return sz;
>> +	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
>> +	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
>> +	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
>> +	*(uint64_t *)(trc_hdr + 8) = sched_clock();
> Why sched_clock()? It should, among other things, be called with
> interrupts disabled, which is not the case here.
I will check. If it is not necessary here, I will remove it.
>
>> +	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
> Is there a reason why trc_hdr is not a struct?
No particular reason here.
>
> Thanks,
> --
> Alex

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
@ 2023-03-07 13:26     ` Jinlong Mao
  0 siblings, 0 replies; 10+ messages in thread
From: Jinlong Mao @ 2023-03-07 13:26 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: linux-kernel, linux-arm-msm, linux-stm32, linux-arm-kernel,
	Tingwei Zhang, Yuanfang Zhang, Tao Zhang, Hao Zhang

Hi Alexande,

On 3/3/2023 2:05 AM, Alexander Shishkin wrote:
> Mao Jinlong <quic_jinlmao@quicinc.com> writes:
>
>> Add MIPI OST protocol support for stm to format the traces.
> Missing an explanation of what OST is, what it's used for, how it is
> different from the SyS-T and others.
I will updae the explanation in next version.
>
>> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
> You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.

The driver refers to code structure of p_sys-t driver. So, add this 
comments after
internal review.

>
>> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
>> support").
> Why is this significant?
>
>> diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c
>> new file mode 100644
>> index 000000000000..2ca1a3fda57f
>> --- /dev/null
>> +++ b/drivers/hwtracing/stm/p_ost.c
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit d69d5e83110f
>> + * ("stm class: Add MIPI SyS-T protocol support").
> Same as in the commit message.
>
> [...]
>
>> +#define OST_TOKEN_STARTSIMPLE		(0x10)
>> +#define OST_VERSION_MIPI1		(0x10 << 8)
>> +#define OST_ENTITY_FTRACE		(0x01 << 16)
>> +#define OST_CONTROL_PROTOCOL		(0x0 << 24)
> These could use an explanation.
I will add the explanation.
>> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
>> +			OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
> Does this mean that everything is ftrace? Because it's not.
Only ftrace is supported in p_ost now. Other header type will be added 
later.
>
>> +
>> +#define STM_MAKE_VERSION(ma, mi)	((ma << 8) | mi)
>> +#define STM_HEADER_MAGIC		(0x5953)
>> +
>> +static ssize_t notrace ost_write(struct stm_data *data,
>> +		struct stm_output *output, unsigned int chan,
>> +		const char *buf, size_t count)
>> +{
>> +	unsigned int c = output->channel + chan;
>> +	unsigned int m = output->master;
>> +	const unsigned char nil = 0;
>> +	u32 header = DATA_HEADER;
>> +	u8 trc_hdr[24];
>> +	ssize_t sz;
>> +
>> +	/*
>> +	 * STP framing rules for OST frames:
>> +	 *   * the first packet of the OST frame is marked;
>> +	 *   * the last packet is a FLAG.
> Which in your case is also timestamped.
I will add the comments.
>
>> +	 */
>> +	/* Message layout: HEADER / DATA / TAIL */
>> +	/* HEADER */
>> +
>> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
>> +			  4, (u8 *)&header);
> The /* HEADER */ comment applies to the above line, so it should
> probably be directly before it.
Got it.
>
>> +	if (sz <= 0)
>> +		return sz;
>> +	*(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
>> +	*(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
>> +	*(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
>> +	*(uint64_t *)(trc_hdr + 8) = sched_clock();
> Why sched_clock()? It should, among other things, be called with
> interrupts disabled, which is not the case here.
I will check. If it is not necessary here, I will remove it.
>
>> +	*(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
> Is there a reason why trc_hdr is not a struct?
No particular reason here.
>
> Thanks,
> --
> Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
  2023-03-07 13:26     ` Jinlong Mao
@ 2023-03-07 13:28       ` Jinlong Mao
  -1 siblings, 0 replies; 10+ messages in thread
From: Jinlong Mao @ 2023-03-07 13:28 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: linux-kernel, linux-arm-msm, linux-stm32, linux-arm-kernel,
	Tingwei Zhang, Yuanfang Zhang, Tao Zhang, Hao Zhang

On 3/7/2023 9:26 PM, Jinlong Mao wrote:

> Hi Alexander,
Sorry, correct the typo.
>
> On 3/3/2023 2:05 AM, Alexander Shishkin wrote:
>> Mao Jinlong <quic_jinlmao@quicinc.com> writes:
>>
>>> Add MIPI OST protocol support for stm to format the traces.
>> Missing an explanation of what OST is, what it's used for, how it is
>> different from the SyS-T and others.
> I will updae the explanation in next version.
>>
>>> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
>> You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.
>
> The driver refers to code structure of p_sys-t driver. So, add this 
> comments after
> internal review.
>
>>
>>> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
>>> support").
>> Why is this significant?
>>
>>> diff --git a/drivers/hwtracing/stm/p_ost.c 
>>> b/drivers/hwtracing/stm/p_ost.c
>>> new file mode 100644
>>> index 000000000000..2ca1a3fda57f
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/stm/p_ost.c
>>> @@ -0,0 +1,95 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit 
>>> d69d5e83110f
>>> + * ("stm class: Add MIPI SyS-T protocol support").
>> Same as in the commit message.
>>
>> [...]
>>
>>> +#define OST_TOKEN_STARTSIMPLE (0x10)
>>> +#define OST_VERSION_MIPI1        (0x10 << 8)
>>> +#define OST_ENTITY_FTRACE        (0x01 << 16)
>>> +#define OST_CONTROL_PROTOCOL        (0x0 << 24)
>> These could use an explanation.
> I will add the explanation.
>>> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
>>> +            OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
>> Does this mean that everything is ftrace? Because it's not.
> Only ftrace is supported in p_ost now. Other header type will be added 
> later.
>>
>>> +
>>> +#define STM_MAKE_VERSION(ma, mi)    ((ma << 8) | mi)
>>> +#define STM_HEADER_MAGIC        (0x5953)
>>> +
>>> +static ssize_t notrace ost_write(struct stm_data *data,
>>> +        struct stm_output *output, unsigned int chan,
>>> +        const char *buf, size_t count)
>>> +{
>>> +    unsigned int c = output->channel + chan;
>>> +    unsigned int m = output->master;
>>> +    const unsigned char nil = 0;
>>> +    u32 header = DATA_HEADER;
>>> +    u8 trc_hdr[24];
>>> +    ssize_t sz;
>>> +
>>> +    /*
>>> +     * STP framing rules for OST frames:
>>> +     *   * the first packet of the OST frame is marked;
>>> +     *   * the last packet is a FLAG.
>> Which in your case is also timestamped.
> I will add the comments.
>>
>>> +     */
>>> +    /* Message layout: HEADER / DATA / TAIL */
>>> +    /* HEADER */
>>> +
>>> +    sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
>>> +              4, (u8 *)&header);
>> The /* HEADER */ comment applies to the above line, so it should
>> probably be directly before it.
> Got it.
>>
>>> +    if (sz <= 0)
>>> +        return sz;
>>> +    *(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
>>> +    *(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
>>> +    *(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
>>> +    *(uint64_t *)(trc_hdr + 8) = sched_clock();
>> Why sched_clock()? It should, among other things, be called with
>> interrupts disabled, which is not the case here.
> I will check. If it is not necessary here, I will remove it.
>>
>>> +    *(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
>> Is there a reason why trc_hdr is not a struct?
> No particular reason here.
>>
>> Thanks,
>> -- 
>> Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] stm: class: Add MIPI OST protocol support
@ 2023-03-07 13:28       ` Jinlong Mao
  0 siblings, 0 replies; 10+ messages in thread
From: Jinlong Mao @ 2023-03-07 13:28 UTC (permalink / raw)
  To: Alexander Shishkin, Maxime Coquelin, Alexandre Torgue
  Cc: linux-kernel, linux-arm-msm, linux-stm32, linux-arm-kernel,
	Tingwei Zhang, Yuanfang Zhang, Tao Zhang, Hao Zhang

On 3/7/2023 9:26 PM, Jinlong Mao wrote:

> Hi Alexander,
Sorry, correct the typo.
>
> On 3/3/2023 2:05 AM, Alexander Shishkin wrote:
>> Mao Jinlong <quic_jinlmao@quicinc.com> writes:
>>
>>> Add MIPI OST protocol support for stm to format the traces.
>> Missing an explanation of what OST is, what it's used for, how it is
>> different from the SyS-T and others.
> I will updae the explanation in next version.
>>
>>> Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
>> You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.
>
> The driver refers to code structure of p_sys-t driver. So, add this 
> comments after
> internal review.
>
>>
>>> commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
>>> support").
>> Why is this significant?
>>
>>> diff --git a/drivers/hwtracing/stm/p_ost.c 
>>> b/drivers/hwtracing/stm/p_ost.c
>>> new file mode 100644
>>> index 000000000000..2ca1a3fda57f
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/stm/p_ost.c
>>> @@ -0,0 +1,95 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit 
>>> d69d5e83110f
>>> + * ("stm class: Add MIPI SyS-T protocol support").
>> Same as in the commit message.
>>
>> [...]
>>
>>> +#define OST_TOKEN_STARTSIMPLE (0x10)
>>> +#define OST_VERSION_MIPI1        (0x10 << 8)
>>> +#define OST_ENTITY_FTRACE        (0x01 << 16)
>>> +#define OST_CONTROL_PROTOCOL        (0x0 << 24)
>> These could use an explanation.
> I will add the explanation.
>>> +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
>>> +            OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
>> Does this mean that everything is ftrace? Because it's not.
> Only ftrace is supported in p_ost now. Other header type will be added 
> later.
>>
>>> +
>>> +#define STM_MAKE_VERSION(ma, mi)    ((ma << 8) | mi)
>>> +#define STM_HEADER_MAGIC        (0x5953)
>>> +
>>> +static ssize_t notrace ost_write(struct stm_data *data,
>>> +        struct stm_output *output, unsigned int chan,
>>> +        const char *buf, size_t count)
>>> +{
>>> +    unsigned int c = output->channel + chan;
>>> +    unsigned int m = output->master;
>>> +    const unsigned char nil = 0;
>>> +    u32 header = DATA_HEADER;
>>> +    u8 trc_hdr[24];
>>> +    ssize_t sz;
>>> +
>>> +    /*
>>> +     * STP framing rules for OST frames:
>>> +     *   * the first packet of the OST frame is marked;
>>> +     *   * the last packet is a FLAG.
>> Which in your case is also timestamped.
> I will add the comments.
>>
>>> +     */
>>> +    /* Message layout: HEADER / DATA / TAIL */
>>> +    /* HEADER */
>>> +
>>> +    sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
>>> +              4, (u8 *)&header);
>> The /* HEADER */ comment applies to the above line, so it should
>> probably be directly before it.
> Got it.
>>
>>> +    if (sz <= 0)
>>> +        return sz;
>>> +    *(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
>>> +    *(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
>>> +    *(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
>>> +    *(uint64_t *)(trc_hdr + 8) = sched_clock();
>> Why sched_clock()? It should, among other things, be called with
>> interrupts disabled, which is not the case here.
> I will check. If it is not necessary here, I will remove it.
>>
>>> +    *(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
>> Is there a reason why trc_hdr is not a struct?
> No particular reason here.
>>
>> Thanks,
>> -- 
>> Alex

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-07 13:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-08 12:40 [PATCH] stm: class: Add MIPI OST protocol support Mao Jinlong
2023-02-08 12:40 ` Mao Jinlong
2023-03-02 12:29 ` Jinlong Mao
2023-03-02 12:29   ` Jinlong Mao
2023-03-02 18:05 ` Alexander Shishkin
2023-03-02 18:05   ` Alexander Shishkin
2023-03-07 13:26   ` Jinlong Mao
2023-03-07 13:26     ` Jinlong Mao
2023-03-07 13:28     ` Jinlong Mao
2023-03-07 13:28       ` Jinlong Mao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.