[PATCH v1 0/4] coresight: ctcu: Enable byte-cntr function for TMC ETR

[PATCH v1 0/4] coresight: ctcu: Enable byte-cntr function for TMC ETR

[Jie Gan]

From: Jie Gan <jie.gan@oss.qualcomm.com>

The byte-cntr function provided by the CTCU device is used to transfer data
from the ETR buffer to the userspace. An interrupt is tiggered if the data
size exceeds the threshold set in the BYTECNTRVAL register. The interrupt
handler counts the number of triggered interruptions and the read function
will read the data from the ETR buffer if the IRQ count is greater than 0.
Each successful read process will decrement the IRQ count by 1.

The byte cntr function will start when the device node is opened for reading,
and the IRQ count will reset when the byte cntr function has stopped. When
the file node is opened, the w_offset of the ETR buffer will be read and
stored in byte_cntr_data, serving as the original r_offset (indicating
where reading starts) for the byte counter function.

The work queue for the read operation will wake up once when ETR is stopped,
ensuring that the remaining data in the ETR buffer has been flushed based on
the w_offset read at the time of stopping.

The following shell commands write threshold to BYTECNTRVAL registers.

Only enable byte-cntr for ETR0:
echo 0x10000 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val

Enable byte-cntr for both ETR0 and ETR1(support both hex and decimal values):
echo 0x10000 4096 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val

Setting the BYTECNTRVAL registers to 0 disables the byte-cntr function.
Disable byte-cntr for ETR0:
echo 0 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val

Disable byte-cntr for both ETR0 and ETR1:
echo 0 0 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val

There is a minimum threshold to prevent generating too many interrupts.
The minimum threshold is 4096 bytes. The write process will fail if user try
to set the BYTECNTRVAL registers to a value less than 4096 bytes(except
for 0).

Finally, the user can read data from the ETR buffer through the byte-cntr file
nodes located under /dev, for example reads data from the ETR0 buffer:
cat /dev/byte-cntr0

Way to enable and start byte-cntr for ETR0:
echo 0x10000 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
echo 1 > /sys/bus/coresight/devices/etm0/enable_source
cat /dev/byte-cntr0

Jie Gan (4):
  coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer
  dt-bindings: arm: Add an interrupt property for Coresight CTCU
  coresight: ctcu: Enable byte-cntr for TMC ETR devices
  arm64: dts: qcom: sa8775p: Add interrupts to CTCU device

 .../bindings/arm/qcom,coresight-ctcu.yaml     |  17 +
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         |   5 +
 drivers/hwtracing/coresight/Makefile          |   2 +-
 .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
 .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
 drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
 .../hwtracing/coresight/coresight-tmc-etr.c   |  45 ++-
 drivers/hwtracing/coresight/coresight-tmc.h   |   3 +
 8 files changed, 556 insertions(+), 10 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c

-- 
2.34.1

[PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Jie Gan]

The new functions calculate and return the offset to the write pointer of
the ETR buffer based on whether the memory mode is SG, flat or reserved.
The functions have the RWP offset can directly read data from ETR buffer,
enabling the transfer of data to any required location.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
 2 files changed, 41 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index eda7cdad0e2b..ec636ab1fd75 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
 }
 EXPORT_SYMBOL_GPL(tmc_free_sg_table);
 
+static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata *drvdata)
+{
+	dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
+	u64 rwp;
+
+	rwp = tmc_read_rwp(drvdata);
+	return rwp - paddr;
+}
+
+static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
+{
+	struct etr_buf *etr_buf = drvdata->sysfs_buf;
+	struct etr_sg_table *etr_table = etr_buf->private;
+	struct tmc_sg_table *table = etr_table->sg_table;
+	long w_offset;
+	u64 rwp;
+
+	rwp = tmc_read_rwp(drvdata);
+	w_offset = tmc_sg_get_data_page_offset(table, rwp);
+
+	return w_offset;
+}
+
+/*
+ * Retrieve the offset to the write pointer of the ETR buffer based on whether
+ * the memory mode is SG, flat or reserved.
+ */
+long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
+{
+	struct etr_buf *etr_buf = drvdata->sysfs_buf;
+
+	if (etr_buf->mode == ETR_MODE_ETR_SG)
+		return tmc_sg_get_rwp_offset(drvdata);
+	else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
+		return tmc_flat_resrv_get_rwp_offset(drvdata);
+	else
+		return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
+
 /*
  * Alloc pages for the table. Since this will be used by the device,
  * allocate the pages closer to the device (i.e, dev_to_node(dev)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index b48bc9a01cc0..baedb4dcfc3f 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
 struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
 				   enum cs_mode mode, void *data);
 extern const struct attribute_group coresight_etr_group;
+long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
 
 #endif
-- 
2.34.1

[PATCH v1 2/4] dt-bindings: arm: Add an interrupt property for Coresight CTCU

[Jie Gan]

Add an interrupt property to CTCU device. The interrupt will be triggered
when the data size in the ETR buffer exceeds the threshlod of the
BYTECNTRVAL register. Programming a threshold in the BYTECNTRVAL register
of CTCU device will enable the interrupt.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 .../bindings/arm/qcom,coresight-ctcu.yaml       | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml
index 843b52eaf872..ea05ad8f3dd3 100644
--- a/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml
@@ -39,6 +39,16 @@ properties:
     items:
       - const: apb
 
+  interrupts:
+    items:
+      - description: Byte cntr interrupt for etr0
+      - description: Byte cntr interrupt for etr1
+
+  interrupt-names:
+    items:
+      - const: etr0
+      - const: etr1
+
   in-ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
@@ -56,6 +66,8 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
     ctcu@1001000 {
         compatible = "qcom,sa8775p-ctcu";
         reg = <0x1001000 0x1000>;
@@ -63,6 +75,11 @@ examples:
         clocks = <&aoss_qmp>;
         clock-names = "apb";
 
+        interrupts = <GIC_SPI 270 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 262 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "etr0",
+                          "etr1";
+
         in-ports {
             #address-cells = <1>;
             #size-cells = <0>;
-- 
2.34.1

[PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices

[Jie Gan]

The byte-cntr function provided by the CTCU device is used to transfer data
from the ETR buffer to the userspace. An interrupt is triggered if the data
size exceeds the threshold set in the BYTECNTRVAL register. The interrupt
handler counts the number of triggered interruptions and the read function
will read the data from the ETR buffer if the IRQ count is greater than 0.
Each successful read process will decrement the IRQ count by 1.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 drivers/hwtracing/coresight/Makefile          |   2 +-
 .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
 .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
 drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
 .../hwtracing/coresight/coresight-tmc-etr.c   |   5 +-
 drivers/hwtracing/coresight/coresight-tmc.h   |   2 +
 6 files changed, 493 insertions(+), 10 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 8e62c3150aeb..c90a06768a18 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -52,4 +52,4 @@ coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
 obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
 obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
 obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
-coresight-ctcu-y := coresight-ctcu-core.o
+coresight-ctcu-y := coresight-ctcu-core.o coresight-ctcu-byte-cntr.o
diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
new file mode 100644
index 000000000000..0d16385663f5
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/coresight.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/fs.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/property.h>
+#include <linux/uaccess.h>
+
+#include "coresight-ctcu.h"
+#include "coresight-priv.h"
+#include "coresight-tmc.h"
+
+#define BYTE_CNTR_CLASS_STR "byte-cntr-class"
+
+static struct class *byte_cntr_class;
+static dev_t byte_cntr_base;
+
+static irqreturn_t byte_cntr_handler(int irq, void *data)
+{
+	struct ctcu_byte_cntr *byte_cntr_data = (struct ctcu_byte_cntr *)data;
+
+	atomic_inc(&byte_cntr_data->irq_cnt);
+	wake_up(&byte_cntr_data->wq);
+
+	return IRQ_HANDLED;
+}
+
+/* Read the data from ETR's DDR buffer. */
+static void __ctcu_byte_cntr_read_etr_bytes(struct ctcu_byte_cntr *byte_cntr_data,
+					    size_t *len, char **bufp)
+{
+	struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
+	size_t actual, bytes = byte_cntr_data->thresh_val;
+	struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
+	long r_offset;
+
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	r_offset = byte_cntr_data->r_offset;
+	if (*len >= bytes)
+		*len = bytes;
+	else if ((r_offset % bytes) + *len > bytes)
+		*len = bytes - (r_offset % bytes);
+
+	actual = tmc_etr_buf_get_data(etr_buf, r_offset, *len, bufp);
+	*len = actual;
+	if (actual == bytes || (actual + r_offset) % bytes == 0)
+		atomic_dec(&byte_cntr_data->irq_cnt);
+}
+
+/* Flush the remaining data in the ETR buffer after the byte cntr has stopped. */
+static ssize_t ctcu_flush_etr_buffer(struct ctcu_byte_cntr *byte_cntr_data, size_t len,
+				     char **bufpp)
+{
+	struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
+	struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
+	ssize_t read_len = 0, remaining_len;
+	long r_offset, w_offset;
+
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	r_offset = byte_cntr_data->r_offset;
+	w_offset = byte_cntr_data->w_offset;
+	if (w_offset < r_offset)
+		remaining_len = tmcdrvdata->size + w_offset - r_offset;
+	else
+		remaining_len = w_offset - r_offset;
+
+	if (remaining_len > len)
+		remaining_len = len;
+
+	if (remaining_len > 0)
+		read_len = tmc_etr_buf_get_data(etr_buf, r_offset, remaining_len, bufpp);
+
+	return read_len;
+}
+
+static ssize_t ctcu_byte_cntr_copy_data(char __user *data, size_t len, char *bufp,
+					struct ctcu_byte_cntr *byte_cntr_data,
+					struct tmc_drvdata *tmcdrvdata)
+{
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	if (copy_to_user(data, bufp, len))
+		return -EFAULT;
+
+	byte_cntr_data->total_size += len;
+	if (byte_cntr_data->r_offset + len >= tmcdrvdata->size)
+		byte_cntr_data->r_offset = 0;
+	else
+		byte_cntr_data->r_offset += len;
+
+	return len;
+}
+
+/* The read function for /dev/byte-cntr%d */
+static ssize_t ctcu_byte_cntr_read_etr_bytes(struct file *fp, char __user *data,
+					     size_t len, loff_t *ppos)
+{
+	struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
+	struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
+	char *bufp = NULL;
+	ssize_t read_len;
+
+	if (!data)
+		return -EINVAL;
+
+	/*
+	 * Flush the remaining data in the ETR buffer based on the write
+	 * offset of the ETR buffer when the byte cntr function has stopped.
+	 */
+	if (!byte_cntr_data->read_active || !byte_cntr_data->enable) {
+		read_len = ctcu_flush_etr_buffer(byte_cntr_data, len, &bufp);
+		if (read_len > 0)
+			return ctcu_byte_cntr_copy_data(data, read_len, bufp,
+							byte_cntr_data, tmcdrvdata);
+
+		return -EINVAL;
+	}
+
+	if (!atomic_read(&byte_cntr_data->irq_cnt)) {
+		if (wait_event_interruptible(byte_cntr_data->wq,
+					     atomic_read(&byte_cntr_data->irq_cnt) > 0 ||
+					     !byte_cntr_data->enable))
+			return -ERESTARTSYS;
+	}
+
+	__ctcu_byte_cntr_read_etr_bytes(byte_cntr_data, &len, &bufp);
+
+	return ctcu_byte_cntr_copy_data(data, len, bufp, byte_cntr_data, tmcdrvdata);
+}
+
+/* Start the byte-cntr function when the path is enabled. */
+void ctcu_byte_cntr_start(struct coresight_device *csdev, struct coresight_path *path)
+{
+	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct coresight_device *sink = coresight_get_sink(path);
+	struct ctcu_byte_cntr *byte_cntr_data;
+	int port_num;
+
+	if (!sink)
+		return;
+
+	port_num = ctcu_get_active_port(sink, csdev);
+	if (port_num < 0)
+		return;
+
+	byte_cntr_data = &drvdata->byte_cntr_data[port_num];
+	/* Don't start byte-cntr function when threshold is not set. */
+	if (!byte_cntr_data->thresh_val)
+		return;
+
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	atomic_set(&byte_cntr_data->irq_cnt, 0);
+	byte_cntr_data->sink = sink;
+	byte_cntr_data->enable = true;
+}
+
+/* Stop the byte-cntr function when the path is disabled. */
+void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct coresight_path *path)
+{
+	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct coresight_device *sink = coresight_get_sink(path);
+	struct ctcu_byte_cntr *byte_cntr_data;
+	struct tmc_drvdata *tmcdrvdata;
+	int port_num;
+
+	if (!sink)
+		return;
+
+	port_num = ctcu_get_active_port(sink, csdev);
+	if (port_num < 0)
+		return;
+
+	byte_cntr_data = &drvdata->byte_cntr_data[port_num];
+	tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
+
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	/* Store the w_offset of the ETR buffer when stopping. */
+	byte_cntr_data->w_offset = tmc_get_rwp_offset(tmcdrvdata);
+	atomic_set(&byte_cntr_data->irq_cnt, 0);
+	byte_cntr_data->read_active = false;
+	byte_cntr_data->enable = false;
+	/*
+	 * Wakeup once to force the read function to read the remaining
+	 * data of the ETR buffer.
+	 */
+	wake_up(&byte_cntr_data->wq);
+}
+
+static int ctcu_byte_cntr_release(struct inode *in, struct file *fp)
+{
+	struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
+	struct device *dev = &byte_cntr_data->sink->dev;
+
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	atomic_set(&byte_cntr_data->irq_cnt, 0);
+	byte_cntr_data->read_active = false;
+	disable_irq_wake(byte_cntr_data->byte_cntr_irq);
+	dev_dbg(dev, "send data total size: %llu bytes, r_offset: %ld w_offset: %ld\n",
+		byte_cntr_data->total_size, byte_cntr_data->r_offset,
+		byte_cntr_data->w_offset);
+
+	return 0;
+}
+
+static int ctcu_byte_cntr_open(struct inode *in, struct file *fp)
+{
+	struct ctcu_byte_cntr *byte_cntr_data = container_of(in->i_cdev,
+							     struct ctcu_byte_cntr, c_dev);
+	struct tmc_drvdata *tmcdrvdata;
+
+	if (byte_cntr_data->read_active)
+		return -EBUSY;
+
+	if (!byte_cntr_data->thresh_val || !byte_cntr_data->sink ||
+	    (coresight_get_mode(byte_cntr_data->sink) !=  CS_MODE_SYSFS))
+		return -EINVAL;
+
+	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
+	enable_irq_wake(byte_cntr_data->byte_cntr_irq);
+	fp->private_data = byte_cntr_data;
+	nonseekable_open(in, fp);
+	tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
+	/*
+	 * The original r_offset is the w_offset of the ETR buffer at the
+	 * start of the byte-cntr.
+	 */
+	byte_cntr_data->r_offset = tmc_get_rwp_offset(tmcdrvdata);
+	byte_cntr_data->total_size = 0;
+	byte_cntr_data->read_active = true;
+	byte_cntr_data->enable = true;
+
+	return 0;
+}
+
+static const struct file_operations byte_cntr_fops = {
+	.owner		= THIS_MODULE,
+	.open		= ctcu_byte_cntr_open,
+	.read		= ctcu_byte_cntr_read_etr_bytes,
+	.release	= ctcu_byte_cntr_release,
+};
+
+static int ctcu_byte_cntr_register_chardev(struct ctcu_byte_cntr *byte_cntr_data,
+					   int port_num)
+{
+	struct device *device;
+	dev_t devt;
+	int ret;
+
+	cdev_init(&byte_cntr_data->c_dev, &byte_cntr_fops);
+	devt = MKDEV(MAJOR(byte_cntr_base), MINOR(byte_cntr_base) + port_num);
+	ret = cdev_add(&byte_cntr_data->c_dev, devt, 1);
+	if (ret < 0)
+		return -ENOMEM;
+
+	device = device_create(byte_cntr_class, NULL, devt, byte_cntr_data,
+			       byte_cntr_data->name);
+
+	if (IS_ERR(device))
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void ctcu_byte_cntr_unregister_chardev(struct ctcu_drvdata *drvdata)
+{
+	struct ctcu_byte_cntr *byte_cntr_data;
+	int i;
+
+	for (i = 0; i < ETR_MAX_NUM; i++) {
+		byte_cntr_data = &drvdata->byte_cntr_data[i];
+		device_destroy(byte_cntr_class, byte_cntr_data->c_dev.dev);
+	}
+}
+
+int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata *drvdata, int etr_num)
+{
+	struct ctcu_byte_cntr *byte_cntr_data;
+	struct device_node *nd = dev->of_node;
+	int byte_cntr_irq, ret, i;
+
+	ret = alloc_chrdev_region(&byte_cntr_base, 0, ETR_MAX_NUM, BYTE_CNTR_CLASS_STR);
+	if (ret < 0)
+		return -ENOMEM;
+
+	byte_cntr_class = class_create(BYTE_CNTR_CLASS_STR);
+	if (IS_ERR(byte_cntr_class)) {
+		unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < etr_num; i++) {
+		byte_cntr_data = &drvdata->byte_cntr_data[i];
+		byte_cntr_irq = of_irq_get_byname(nd, byte_cntr_data->irq_name);
+		if (byte_cntr_irq < 0) {
+			ret = byte_cntr_irq;
+			goto err_exit;
+		}
+
+		ret = devm_request_irq(dev, byte_cntr_irq, byte_cntr_handler,
+				       IRQF_TRIGGER_RISING | IRQF_SHARED,
+				       dev_name(dev), byte_cntr_data);
+		if (ret) {
+			dev_err(dev, "Failed to register IRQ for %s\n",
+				byte_cntr_data->name);
+			goto err_exit;
+		}
+
+		ret = ctcu_byte_cntr_register_chardev(byte_cntr_data, i);
+		if (ret) {
+			dev_err(dev, "Failed to register chardev for %s\n",
+				byte_cntr_data->name);
+			goto err_exit;
+		}
+
+		byte_cntr_data->byte_cntr_irq = byte_cntr_irq;
+		atomic_set(&byte_cntr_data->irq_cnt, 0);
+		init_waitqueue_head(&byte_cntr_data->wq);
+	}
+
+	return 0;
+
+err_exit:
+	ctcu_byte_cntr_unregister_chardev(drvdata);
+	class_destroy(byte_cntr_class);
+	unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
+	return ret;
+}
+
+void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata)
+{
+	ctcu_byte_cntr_unregister_chardev(drvdata);
+	class_destroy(byte_cntr_class);
+	unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
+}
diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
index da35d8b4d579..5782655a5f39 100644
--- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
+++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
@@ -46,16 +46,24 @@ DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
 #define CTCU_ATID_REG_BIT(traceid)	(traceid % 32)
 #define CTCU_ATID_REG_SIZE		0x10
 #define CTCU_ETR0_ATID0			0xf8
+#define CTCU_ETR0_IRQCTRL		0x6c
 #define CTCU_ETR1_ATID0			0x108
+#define CTCU_ETR1_IRQCTRL		0x70
 
 static const struct ctcu_etr_config sa8775p_etr_cfgs[] = {
 	{
-		.atid_offset	= CTCU_ETR0_ATID0,
-		.port_num	= 0,
+		.atid_offset		= CTCU_ETR0_ATID0,
+		.irq_ctrl_offset	= CTCU_ETR0_IRQCTRL,
+		.irq_name		= "etr0",
+		.byte_cntr_name		= "byte-cntr0",
+		.port_num		= 0,
 	},
 	{
-		.atid_offset	= CTCU_ETR1_ATID0,
-		.port_num	= 1,
+		.atid_offset		= CTCU_ETR1_ATID0,
+		.irq_ctrl_offset	= CTCU_ETR1_IRQCTRL,
+		.irq_name		= "etr1",
+		.byte_cntr_name		= "byte-cntr1",
+		.port_num		= 1,
 	},
 };
 
@@ -64,6 +72,69 @@ static const struct ctcu_config sa8775p_cfgs = {
 	.num_etr_config	= ARRAY_SIZE(sa8775p_etr_cfgs),
 };
 
+static ssize_t byte_cntr_val_show(struct device *dev, struct device_attribute *attr,
+				  char *buf)
+{
+	struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	int i, len = 0;
+
+	for (i = 0; i < ETR_MAX_NUM; i++) {
+		if (drvdata->byte_cntr_data[i].irq_ctrl_offset)
+			len += scnprintf(buf + len, PAGE_SIZE - len, "%u ",
+					 drvdata->byte_cntr_data[i].thresh_val);
+	}
+
+	len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
+
+	return len;
+}
+
+static ssize_t byte_cntr_val_store(struct device *dev, struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	u32 thresh_vals[ETR_MAX_NUM] = { 0 };
+	u32 irq_ctrl_offset;
+	int num, i;
+
+	num = sscanf(buf, "%i %i", &thresh_vals[0], &thresh_vals[1]);
+	if (num <= 0 || num > ETR_MAX_NUM)
+		return -EINVAL;
+
+	/* Threshold 0 disables the interruption. */
+	guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
+	for (i = 0; i < num; i++) {
+		/* A small threshold will result in a large number of interruptions */
+		if (thresh_vals[i] && thresh_vals[i] < 4096)
+			return -EINVAL;
+
+		if (drvdata->byte_cntr_data[i].irq_ctrl_offset) {
+			drvdata->byte_cntr_data[i].thresh_val = thresh_vals[i];
+			irq_ctrl_offset = drvdata->byte_cntr_data[i].irq_ctrl_offset;
+			CS_UNLOCK(drvdata->base);
+			writel_relaxed(thresh_vals[i], drvdata->base + irq_ctrl_offset);
+			CS_LOCK(drvdata->base);
+		}
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(byte_cntr_val);
+
+static struct attribute *ctcu_attrs[] = {
+	&dev_attr_byte_cntr_val.attr,
+	NULL,
+};
+
+static struct attribute_group ctcu_attr_grp = {
+	.attrs = ctcu_attrs,
+};
+
+static const struct attribute_group *ctcu_attr_grps[] = {
+	&ctcu_attr_grp,
+	NULL,
+};
+
 static void ctcu_program_atid_register(struct ctcu_drvdata *drvdata, u32 reg_offset,
 				       u8 bit, bool enable)
 {
@@ -122,7 +193,7 @@ static int __ctcu_set_etr_traceid(struct coresight_device *csdev, u8 traceid, in
  * Searching the sink device from helper's view in case there are multiple helper devices
  * connected to the sink device.
  */
-static int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper)
+int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper)
 {
 	struct coresight_platform_data *pdata = helper->pdata;
 	int i;
@@ -160,6 +231,8 @@ static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, void *
 {
 	struct coresight_path *path = (struct coresight_path *)data;
 
+	ctcu_byte_cntr_start(csdev, path);
+
 	return ctcu_set_etr_traceid(csdev, path, true);
 }
 
@@ -167,6 +240,8 @@ static int ctcu_disable(struct coresight_device *csdev, void *data)
 {
 	struct coresight_path *path = (struct coresight_path *)data;
 
+	ctcu_byte_cntr_stop(csdev, path);
+
 	return ctcu_set_etr_traceid(csdev, path, false);
 }
 
@@ -188,7 +263,7 @@ static int ctcu_probe(struct platform_device *pdev)
 	const struct ctcu_config *cfgs;
 	struct ctcu_drvdata *drvdata;
 	void __iomem *base;
-	int i;
+	int ret, i;
 
 	desc.name = coresight_alloc_device_name(&ctcu_devs, dev);
 	if (!desc.name)
@@ -217,7 +292,14 @@ static int ctcu_probe(struct platform_device *pdev)
 			for (i = 0; i < cfgs->num_etr_config; i++) {
 				etr_cfg = &cfgs->etr_cfgs[i];
 				drvdata->atid_offset[i] = etr_cfg->atid_offset;
+				drvdata->byte_cntr_data[i].irq_name = etr_cfg->irq_name;
+				drvdata->byte_cntr_data[i].name = etr_cfg->byte_cntr_name;
+				drvdata->byte_cntr_data[i].irq_ctrl_offset =
+					etr_cfg->irq_ctrl_offset;
 			}
+			ret = ctcu_byte_cntr_init(dev, drvdata, cfgs->num_etr_config);
+			if (ret < 0)
+				dev_warn(dev, "Byte cntr init failed\n");
 		}
 	}
 
@@ -229,6 +311,7 @@ static int ctcu_probe(struct platform_device *pdev)
 	desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU;
 	desc.pdata = pdata;
 	desc.dev = dev;
+	desc.groups = ctcu_attr_grps;
 	desc.ops = &ctcu_ops;
 	desc.access = CSDEV_ACCESS_IOMEM(base);
 
@@ -247,6 +330,7 @@ static void ctcu_remove(struct platform_device *pdev)
 {
 	struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
 
+	ctcu_byte_cntr_remove(drvdata);
 	coresight_unregister(drvdata->csdev);
 }
 
diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/hwtracing/coresight/coresight-ctcu.h
index e9594c38dd91..e38535c91090 100644
--- a/drivers/hwtracing/coresight/coresight-ctcu.h
+++ b/drivers/hwtracing/coresight/coresight-ctcu.h
@@ -5,6 +5,8 @@
 
 #ifndef _CORESIGHT_CTCU_H
 #define _CORESIGHT_CTCU_H
+#include <linux/cdev.h>
+
 #include "coresight-trace-id.h"
 
 /* Maximum number of supported ETR devices for a single CTCU. */
@@ -13,10 +15,16 @@
 /**
  * struct ctcu_etr_config
  * @atid_offset:	offset to the ATID0 Register.
- * @port_num:		in-port number of CTCU device that connected to ETR.
+ * @irq_ctrl_offset:	offset to the BYTECNTRVAL register.
+ * @irq_name:		IRQ name in dt node.
+ * @byte_cntr_name:	name of the byte cntr device node.
+ * @port_num:		in-port number of the CTCU device that connected to ETR.
  */
 struct ctcu_etr_config {
 	const u32 atid_offset;
+	const u32 irq_ctrl_offset;
+	const char *irq_name;
+	const char *byte_cntr_name;
 	const u32 port_num;
 };
 
@@ -25,15 +33,64 @@ struct ctcu_config {
 	int num_etr_config;
 };
 
+/**
+ * struct ctcu_byte_cntr
+ * @c_dev:		cdev for byte_cntr.
+ * @sink		csdev of sink device.
+ * @enable:		indicates that byte_cntr function is enabled or not.
+ * @read_active:	indicates that byte-cntr node is opened or not.
+ * @thresh_val:		threshold to trigger a interruption.
+ * @total_size		total size of transferred data.
+ * @byte_cntr_irq:	IRQ number.
+ * @irq_cnt:		IRQ count.
+ * @wq:			workqueue of reading ETR data.
+ * @read_work:		work of reading ETR data.
+ * @spin_lock:		spinlock of byte cntr data.
+ * @r_offset:		offset of the pointer where reading begins.
+ * @w_offset:		offset of the write pointer in the ETR buffer when
+ *			the byte cntr is stopped.
+ * @irq_ctrl_offset:	offset to the BYTECNTRVAL Register.
+ * @name:		the name of byte cntr device node.
+ * @irq_name:		IRQ name in DT.
+ */
+struct ctcu_byte_cntr {
+	struct cdev		c_dev;
+	struct coresight_device	*sink;
+	bool			enable;
+	bool			read_active;
+	u32			thresh_val;
+	u64			total_size;
+	int			byte_cntr_irq;
+	atomic_t		irq_cnt;
+	wait_queue_head_t	wq;
+	struct work_struct	read_work;
+	raw_spinlock_t		spin_lock;
+	long			r_offset;
+	long			w_offset;
+	u32			irq_ctrl_offset;
+	const char		*name;
+	const char		*irq_name;
+};
+
 struct ctcu_drvdata {
 	void __iomem		*base;
 	struct clk		*apb_clk;
 	struct device		*dev;
 	struct coresight_device	*csdev;
+	struct ctcu_byte_cntr   byte_cntr_data[ETR_MAX_NUM];
 	raw_spinlock_t		spin_lock;
 	u32			atid_offset[ETR_MAX_NUM];
 	/* refcnt for each traceid of each sink */
 	u8			traceid_refcnt[ETR_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
 };
 
+/* Generic functions */
+int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper);
+
+/* Byte-cntr functions */
+void ctcu_byte_cntr_start(struct coresight_device *csdev, struct coresight_path *path);
+void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct coresight_path *path);
+int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata *drvdata, int port_num);
+void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata);
+
 #endif
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index ec636ab1fd75..5dc94e890927 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1040,14 +1040,15 @@ static void tmc_free_etr_buf(struct etr_buf *etr_buf)
  * Returns: The size of the linear data available @pos, with *bufpp
  * updated to point to the buffer.
  */
-static ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
-				    u64 offset, size_t len, char **bufpp)
+ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
+			     u64 offset, size_t len, char **bufpp)
 {
 	/* Adjust the length to limit this transaction to end of buffer */
 	len = (len < (etr_buf->size - offset)) ? len : etr_buf->size - offset;
 
 	return etr_buf->ops->get_data(etr_buf, (u64)offset, len, bufpp);
 }
+EXPORT_SYMBOL_GPL(tmc_etr_buf_get_data);
 
 static inline s64
 tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 offset)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index baedb4dcfc3f..2fc77fd4ea25 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -443,5 +443,7 @@ struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
 				   enum cs_mode mode, void *data);
 extern const struct attribute_group coresight_etr_group;
 long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
+ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf, u64 offset, size_t len,
+			     char **bufpp);
 
 #endif
-- 
2.34.1

[PATCH v1 4/4] arm64: dts: qcom: sa8775p: Add interrupts to CTCU device

[Jie Gan]

Add interrupts to enable byte-cntr function for TMC ETR devices.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
Dependency:
prerequisite-message-id: 20250303032931.2500935-11-quic_jiegan@quicinc.com
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 93ca37843990..091ae73774fa 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -2425,6 +2425,11 @@ ctcu@4001000 {
 			clocks = <&aoss_qmp>;
 			clock-names = "apb";
 
+			interrupts = <GIC_SPI 270 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 262 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "etr0",
+					  "etr1";
+
 			in-ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
-- 
2.34.1

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Krzysztof Kozlowski]

On 10/03/2025 10:04, Jie Gan wrote:
> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
> +{
> +	struct etr_buf *etr_buf = drvdata->sysfs_buf;
> +	struct etr_sg_table *etr_table = etr_buf->private;
> +	struct tmc_sg_table *table = etr_table->sg_table;
> +	long w_offset;
> +	u64 rwp;
> +
> +	rwp = tmc_read_rwp(drvdata);
> +	w_offset = tmc_sg_get_data_page_offset(table, rwp);
> +
> +	return w_offset;
> +}
> +
> +/*
> + * Retrieve the offset to the write pointer of the ETR buffer based on whether
> + * the memory mode is SG, flat or reserved.
> + */
> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)

You need kerneldoc for exports.


Best regards,
Krzysztof

Re: [PATCH v1 4/4] arm64: dts: qcom: sa8775p: Add interrupts to CTCU device

[Krzysztof Kozlowski]

On 10/03/2025 10:04, Jie Gan wrote:
> Add interrupts to enable byte-cntr function for TMC ETR devices.
> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
> Dependency:
> prerequisite-message-id: 20250303032931.2500935-11-quic_jiegan@quicinc.com
Which too generated such changelog? Why this cannot be lore link?

Best regards,
Krzysztof

Re: [PATCH v1 4/4] arm64: dts: qcom: sa8775p: Add interrupts to CTCU device

[Jie Gan]

On 3/10/2025 5:08 PM, Krzysztof Kozlowski wrote:
> On 10/03/2025 10:04, Jie Gan wrote:
>> Add interrupts to enable byte-cntr function for TMC ETR devices.
>>
>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>> ---
>> Dependency:
>> prerequisite-message-id: 20250303032931.2500935-11-quic_jiegan@quicinc.com
> Which too generated such changelog? Why this cannot be lore link?
> 
> Best regards,
> Krzysztof

Hi Krzysztof,

It was entered manually. It's my fault, will fix in next version.

Thanks,
Jie

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Jie Gan]

On 3/10/2025 5:07 PM, Krzysztof Kozlowski wrote:
> On 10/03/2025 10:04, Jie Gan wrote:
>> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
>> +{
>> +	struct etr_buf *etr_buf = drvdata->sysfs_buf;
>> +	struct etr_sg_table *etr_table = etr_buf->private;
>> +	struct tmc_sg_table *table = etr_table->sg_table;
>> +	long w_offset;
>> +	u64 rwp;
>> +
>> +	rwp = tmc_read_rwp(drvdata);
>> +	w_offset = tmc_sg_get_data_page_offset(table, rwp);
>> +
>> +	return w_offset;
>> +}
>> +
>> +/*
>> + * Retrieve the offset to the write pointer of the ETR buffer based on whether
>> + * the memory mode is SG, flat or reserved.
>> + */
>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
> 
> You need kerneldoc for exports.

Hi Krzysztof,

Sorry for the insufficient description for an export function. Will fix 
it in next version.

Thanks,
Jie

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Mike Leach]

Hi,

On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@quicinc.com> wrote:
>
> The new functions calculate and return the offset to the write pointer of
> the ETR buffer based on whether the memory mode is SG, flat or reserved.
> The functions have the RWP offset can directly read data from ETR buffer,
> enabling the transfer of data to any required location.
>
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>  .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
>  2 files changed, 41 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index eda7cdad0e2b..ec636ab1fd75 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
>  }
>  EXPORT_SYMBOL_GPL(tmc_free_sg_table);
>
> +static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata *drvdata)
> +{
> +       dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
> +       u64 rwp;
> +

It is not valid to read RWP if the TMC is running. It must be in the
stopped or disabled state - see the specifications for TMC /ETR

It is likely that CSUNLOCK / CSLOCK are needed here too,  along with
the spinlock that protects drvdata

See the code in coresight_tmc_etr.c :-

e.g. in

tmc_update_etr_buffer()

...
<take spinlock>
...
CS_UNLOCK(drvdata->base);
tmc_flush_and_stop(drvdata); // this ensures tmc is stopped and
flushed to memory - essential to ensure full formatted frame is in
memory.
tmc_sync_etr_buf(drvdata); // this function reads rwp.
CS_LOCK(drvdata->base);
<release spinlokc>

This type of program flow is common to both sysfs and perf handling of
TMC buffers.

> +       rwp = tmc_read_rwp(drvdata);
> +       return rwp - paddr;
> +}
> +
> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
> +{
> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
> +       struct etr_sg_table *etr_table = etr_buf->private;
> +       struct tmc_sg_table *table = etr_table->sg_table;
> +       long w_offset;
> +       u64 rwp;
> +

Same comments as above

> +       rwp = tmc_read_rwp(drvdata);
> +       w_offset = tmc_sg_get_data_page_offset(table, rwp);
> +
> +       return w_offset;
> +}
> +
> +/*
> + * Retrieve the offset to the write pointer of the ETR buffer based on whether
> + * the memory mode is SG, flat or reserved.
> + */
> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
> +{
> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
> +

As this is an exported function, please ensure that the inputs are
valid - check the pointers

Code to ensure TMC is flushed and stopped could be inserted here.

Regards

Mike

> +       if (etr_buf->mode == ETR_MODE_ETR_SG)
> +               return tmc_sg_get_rwp_offset(drvdata);
> +       else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
> +               return tmc_flat_resrv_get_rwp_offset(drvdata);
> +       else
> +               return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
> +
>  /*
>   * Alloc pages for the table. Since this will be used by the device,
>   * allocate the pages closer to the device (i.e, dev_to_node(dev)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index b48bc9a01cc0..baedb4dcfc3f 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
>  struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>                                    enum cs_mode mode, void *data);
>  extern const struct attribute_group coresight_etr_group;
> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>
>  #endif
> --
> 2.34.1
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Jie Gan]

On 3/12/2025 12:49 AM, Mike Leach wrote:
> Hi,
> 
> On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@quicinc.com> wrote:
>>
>> The new functions calculate and return the offset to the write pointer of
>> the ETR buffer based on whether the memory mode is SG, flat or reserved.
>> The functions have the RWP offset can directly read data from ETR buffer,
>> enabling the transfer of data to any required location.
>>
>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>> ---
>>   .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
>>   2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index eda7cdad0e2b..ec636ab1fd75 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
>>   }
>>   EXPORT_SYMBOL_GPL(tmc_free_sg_table);
>>
>> +static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata *drvdata)
>> +{
>> +       dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
>> +       u64 rwp;
>> +
> 
> It is not valid to read RWP if the TMC is running. It must be in the
> stopped or disabled state - see the specifications for TMC /ETR
> 
> It is likely that CSUNLOCK / CSLOCK are needed here too,  along with
> the spinlock that protects drvdata
> 
> See the code in coresight_tmc_etr.c :-
> 
> e.g. in
> 
> tmc_update_etr_buffer()
> 
> ...
> <take spinlock>
> ...
> CS_UNLOCK(drvdata->base);
> tmc_flush_and_stop(drvdata); // this ensures tmc is stopped and
> flushed to memory - essential to ensure full formatted frame is in
> memory.
> tmc_sync_etr_buf(drvdata); // this function reads rwp.
> CS_LOCK(drvdata->base);
> <release spinlokc>
> 
> This type of program flow is common to both sysfs and perf handling of
> TMC buffers.

Hi Mike,

I am fully understood your point here.

The function is designed this way to read the w_offset (which may not be 
entirely accurate because the etr buffer is not synced) when the 
byte-cntr devnode is opened, aiming to reduce the length of redundant 
trace data. In this case, we cannot ensure the TMC is stopped or 
disabled. The byte-cntr only requires an offset to know where it can 
start before the expected trace data gets into ETR buffer.

The w_offset is also read when the byte-cntr function stops, which 
occurs after the TMC is disabled.

Maybe this is not a good idea and I should read r_offset upon open?
The primary goal for byte-cntr is trying to transfer useful trace data 
from the ETR buffer to the userspace, if we start from r_offset, a large 
number of redundant trace data which the user does not expect will be 
transferred simultaneously.


> 
>> +       rwp = tmc_read_rwp(drvdata);
>> +       return rwp - paddr;
>> +}
>> +
>> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
>> +{
>> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
>> +       struct etr_sg_table *etr_table = etr_buf->private;
>> +       struct tmc_sg_table *table = etr_table->sg_table;
>> +       long w_offset;
>> +       u64 rwp;
>> +
> 
> Same comments as above
> 
>> +       rwp = tmc_read_rwp(drvdata);
>> +       w_offset = tmc_sg_get_data_page_offset(table, rwp);
>> +
>> +       return w_offset;
>> +}
>> +
>> +/*
>> + * Retrieve the offset to the write pointer of the ETR buffer based on whether
>> + * the memory mode is SG, flat or reserved.
>> + */
>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
>> +{
>> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
>> +
> 
> As this is an exported function, please ensure that the inputs are
> valid - check the pointers

Sure, will do.

Thanks,
Jie

> 
> Code to ensure TMC is flushed and stopped could be inserted here.
> 
> Regards
> 
> Mike
> 
>> +       if (etr_buf->mode == ETR_MODE_ETR_SG)
>> +               return tmc_sg_get_rwp_offset(drvdata);
>> +       else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
>> +               return tmc_flat_resrv_get_rwp_offset(drvdata);
>> +       else
>> +               return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
>> +
>>   /*
>>    * Alloc pages for the table. Since this will be used by the device,
>>    * allocate the pages closer to the device (i.e, dev_to_node(dev)
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
>> index b48bc9a01cc0..baedb4dcfc3f 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>> @@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
>>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>>                                     enum cs_mode mode, void *data);
>>   extern const struct attribute_group coresight_etr_group;
>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>>
>>   #endif
>> --
>> 2.34.1
>>
> 
> 

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Mike Leach]

Hi Jie,

On Wed, 12 Mar 2025 at 01:21, Jie Gan <quic_jiegan@quicinc.com> wrote:
>
>
>
> On 3/12/2025 12:49 AM, Mike Leach wrote:
> > Hi,
> >
> > On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@quicinc.com> wrote:
> >>
> >> The new functions calculate and return the offset to the write pointer of
> >> the ETR buffer based on whether the memory mode is SG, flat or reserved.
> >> The functions have the RWP offset can directly read data from ETR buffer,
> >> enabling the transfer of data to any required location.
> >>
> >> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> >> ---
> >>   .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++++++++
> >>   drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
> >>   2 files changed, 41 insertions(+)
> >>
> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >> index eda7cdad0e2b..ec636ab1fd75 100644
> >> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >> @@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
> >>   }
> >>   EXPORT_SYMBOL_GPL(tmc_free_sg_table);
> >>
> >> +static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata *drvdata)
> >> +{
> >> +       dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
> >> +       u64 rwp;
> >> +
> >
> > It is not valid to read RWP if the TMC is running. It must be in the
> > stopped or disabled state - see the specifications for TMC /ETR
> >
> > It is likely that CSUNLOCK / CSLOCK are needed here too,  along with
> > the spinlock that protects drvdata
> >
> > See the code in coresight_tmc_etr.c :-
> >
> > e.g. in
> >
> > tmc_update_etr_buffer()
> >
> > ...
> > <take spinlock>
> > ...
> > CS_UNLOCK(drvdata->base);
> > tmc_flush_and_stop(drvdata); // this ensures tmc is stopped and
> > flushed to memory - essential to ensure full formatted frame is in
> > memory.
> > tmc_sync_etr_buf(drvdata); // this function reads rwp.
> > CS_LOCK(drvdata->base);
> > <release spinlokc>
> >
> > This type of program flow is common to both sysfs and perf handling of
> > TMC buffers.
>
> Hi Mike,
>
> I am fully understood your point here.
>
> The function is designed this way to read the w_offset (which may not be
> entirely accurate because the etr buffer is not synced) when the

Why would you ever base memory access on a pointer that is not
entirely accurate?

The manuals for TMC/ETR all state that reads to both RWP and RRP when
the ETR is running return unknown values. These cannot be used to
access the buffer, or determine how much of the buffer has been used
on a running ETR.

The ETR specification specifically states that it is not permitted to
read the buffer data while the ETR is running, when configured in
circular buffer mode - which is the mode used in the TMC-ETR linux
drivers.

Reading the buffer while ETR is running is only permitted if
configured in Software FIFO mode 2 - were the ETR will stop on full
and stall incoming trace until some data is read out, signalled to the
ETR via the RURP.

I also note that you are reading back the etr_buf data without doing
any dma_sync operations that the perf and sysfs methods in the driver
do, after stopping the tmc.

> byte-cntr devnode is opened, aiming to reduce the length of redundant
> trace data. In this case, we cannot ensure the TMC is stopped or
> disabled.

The specification requires that you must ensure the TMC is stopped to
read these registers.


>The byte-cntr only requires an offset to know where it can
> start before the expected trace data gets into ETR buffer.
>
> The w_offset is also read when the byte-cntr function stops, which
> occurs after the TMC is disabled.
>
> Maybe this is not a good idea and I should read r_offset upon open?
> The primary goal for byte-cntr is trying to transfer useful trace data
> from the ETR buffer to the userspace, if we start from r_offset, a large
> number of redundant trace data which the user does not expect will be
> transferred simultaneously.
>
>

It is difficult to justify adding code to a driver that does not
correspond to the specification of the hardware device.

Regards

Mike

> >
> >> +       rwp = tmc_read_rwp(drvdata);
> >> +       return rwp - paddr;
> >> +}
> >> +
> >> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
> >> +{
> >> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
> >> +       struct etr_sg_table *etr_table = etr_buf->private;
> >> +       struct tmc_sg_table *table = etr_table->sg_table;
> >> +       long w_offset;
> >> +       u64 rwp;
> >> +
> >
> > Same comments as above
> >
> >> +       rwp = tmc_read_rwp(drvdata);
> >> +       w_offset = tmc_sg_get_data_page_offset(table, rwp);
> >> +
> >> +       return w_offset;
> >> +}
> >> +
> >> +/*
> >> + * Retrieve the offset to the write pointer of the ETR buffer based on whether
> >> + * the memory mode is SG, flat or reserved.
> >> + */
> >> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
> >> +{
> >> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
> >> +
> >
> > As this is an exported function, please ensure that the inputs are
> > valid - check the pointers
>
> Sure, will do.
>
> Thanks,
> Jie
>
> >
> > Code to ensure TMC is flushed and stopped could be inserted here.
> >
> > Regards
> >
> > Mike
> >
> >> +       if (etr_buf->mode == ETR_MODE_ETR_SG)
> >> +               return tmc_sg_get_rwp_offset(drvdata);
> >> +       else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
> >> +               return tmc_flat_resrv_get_rwp_offset(drvdata);
> >> +       else
> >> +               return -EINVAL;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
> >> +
> >>   /*
> >>    * Alloc pages for the table. Since this will be used by the device,
> >>    * allocate the pages closer to the device (i.e, dev_to_node(dev)
> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> >> index b48bc9a01cc0..baedb4dcfc3f 100644
> >> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> >> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> >> @@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
> >>   struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
> >>                                     enum cs_mode mode, void *data);
> >>   extern const struct attribute_group coresight_etr_group;
> >> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
> >>
> >>   #endif
> >> --
> >> 2.34.1
> >>
> >
> >
>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v1 0/4] coresight: ctcu: Enable byte-cntr function for TMC ETR

[Mike Leach]

Hi,

On Mon, 10 Mar 2025 at 09:05, Jie Gan <quic_jiegan@quicinc.com> wrote:
>
> From: Jie Gan <jie.gan@oss.qualcomm.com>
>
> The byte-cntr function provided by the CTCU device is used to transfer data
> from the ETR buffer to the userspace. An interrupt is tiggered if the data
> size exceeds the threshold set in the BYTECNTRVAL register. The interrupt
> handler counts the number of triggered interruptions and the read function
> will read the data from the ETR buffer if the IRQ count is greater than 0.
> Each successful read process will decrement the IRQ count by 1.
>
> The byte cntr function will start when the device node is opened for reading,
> and the IRQ count will reset when the byte cntr function has stopped. When
> the file node is opened, the w_offset of the ETR buffer will be read and
> stored in byte_cntr_data, serving as the original r_offset (indicating
> where reading starts) for the byte counter function.
>
> The work queue for the read operation will wake up once when ETR is stopped,
> ensuring that the remaining data in the ETR buffer has been flushed based on
> the w_offset read at the time of stopping.
>
> The following shell commands write threshold to BYTECNTRVAL registers.
>
> Only enable byte-cntr for ETR0:
> echo 0x10000 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>
> Enable byte-cntr for both ETR0 and ETR1(support both hex and decimal values):
> echo 0x10000 4096 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>
> Setting the BYTECNTRVAL registers to 0 disables the byte-cntr function.
> Disable byte-cntr for ETR0:
> echo 0 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>
> Disable byte-cntr for both ETR0 and ETR1:
> echo 0 0 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>
> There is a minimum threshold to prevent generating too many interrupts.
> The minimum threshold is 4096 bytes. The write process will fail if user try
> to set the BYTECNTRVAL registers to a value less than 4096 bytes(except
> for 0).
>
> Finally, the user can read data from the ETR buffer through the byte-cntr file
> nodes located under /dev, for example reads data from the ETR0 buffer:
> cat /dev/byte-cntr0
>
> Way to enable and start byte-cntr for ETR0:
> echo 0x10000 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
> echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
> echo 1 > /sys/bus/coresight/devices/etm0/enable_source
> cat /dev/byte-cntr0
>

There is a significant issue with attempting to drain an ETR buffer
while it is live in the way you appear to be doing.

You have no way of knowing if the TMC hardware write pointer wraps and
overtakes the point where you are currently reading. This could cause
data corruption as TMC writes as you are reading, or contention for
the buffer that affects the TMC write.

Even if those two events do not occur, then the trace capture sequence
is corrupted.

Take a simple example - suppose we split the buffer into 4 blocks of
trace, which are filled by the ETR

buffer = 1, 2, 3, 4

Now you suppose you have read 1 & 2 into your userspace buffer / file.

file = 1, 2

If there is now some system event that prevents your userspace code
from running for a while, then it is possible that the ETR continues,
wraps and the buffer is now

buffer = 5, 6, 7, 4

Your next two reads will be 7, 4

file = 1, 2, 7, 4

This trace is now corrupt and will cause decode errors. There is no
way for the decoder to determine that the interface between blocks 2 &
7 is not correct. If you are fortunate then this issue will cause an
actual explicit decode error, if you are less fortunate then decode
will continue but in fact be inaccurate, with no obvious way to detect
the inaccuracy.

We encountered this problem early in the development of the perf data
collection. Even though perf was stopping the trace to copy the
hardware buffer, it would concatenate unrelated trace blocks into the
perf userspace buffer, which initially caused decoding errors. This is
now mitigated in perf by marking boundaries and recording indexes of
the boundaries, so the tool can reset the decoder at the start of non
contiguous blocks.

If you do not stop the TMC when draining the ETR buffer, you have no
way of determining if this has occurred.

Clearly using large buffers, split into smaller blocks can mitigate
the possibility of a wrap in this way - but never eliminate it,
especially given the extreme rate that trace data can be generated.

Regards

Mike


> Jie Gan (4):
>   coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer
>   dt-bindings: arm: Add an interrupt property for Coresight CTCU
>   coresight: ctcu: Enable byte-cntr for TMC ETR devices
>   arm64: dts: qcom: sa8775p: Add interrupts to CTCU device
>
>  .../bindings/arm/qcom,coresight-ctcu.yaml     |  17 +
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi         |   5 +
>  drivers/hwtracing/coresight/Makefile          |   2 +-
>  .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
>  .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
>  drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
>  .../hwtracing/coresight/coresight-tmc-etr.c   |  45 ++-
>  drivers/hwtracing/coresight/coresight-tmc.h   |   3 +
>  8 files changed, 556 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>
> --
> 2.34.1
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Jie Gan]

On 3/12/2025 8:54 PM, Mike Leach wrote:
> Hi Jie,
> 
> On Wed, 12 Mar 2025 at 01:21, Jie Gan <quic_jiegan@quicinc.com> wrote:
>>
>>
>>
>> On 3/12/2025 12:49 AM, Mike Leach wrote:
>>> Hi,
>>>
>>> On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@quicinc.com> wrote:
>>>>
>>>> The new functions calculate and return the offset to the write pointer of
>>>> the ETR buffer based on whether the memory mode is SG, flat or reserved.
>>>> The functions have the RWP offset can directly read data from ETR buffer,
>>>> enabling the transfer of data to any required location.
>>>>
>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>> ---
>>>>    .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++++++++
>>>>    drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
>>>>    2 files changed, 41 insertions(+)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>> index eda7cdad0e2b..ec636ab1fd75 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>> @@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(tmc_free_sg_table);
>>>>
>>>> +static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata *drvdata)
>>>> +{
>>>> +       dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
>>>> +       u64 rwp;
>>>> +
>>>
>>> It is not valid to read RWP if the TMC is running. It must be in the
>>> stopped or disabled state - see the specifications for TMC /ETR
>>>
>>> It is likely that CSUNLOCK / CSLOCK are needed here too,  along with
>>> the spinlock that protects drvdata
>>>
>>> See the code in coresight_tmc_etr.c :-
>>>
>>> e.g. in
>>>
>>> tmc_update_etr_buffer()
>>>
>>> ...
>>> <take spinlock>
>>> ...
>>> CS_UNLOCK(drvdata->base);
>>> tmc_flush_and_stop(drvdata); // this ensures tmc is stopped and
>>> flushed to memory - essential to ensure full formatted frame is in
>>> memory.
>>> tmc_sync_etr_buf(drvdata); // this function reads rwp.
>>> CS_LOCK(drvdata->base);
>>> <release spinlokc>
>>>
>>> This type of program flow is common to both sysfs and perf handling of
>>> TMC buffers.
>>
>> Hi Mike,
>>
>> I am fully understood your point here.
>>
>> The function is designed this way to read the w_offset (which may not be
>> entirely accurate because the etr buffer is not synced) when the
> 
> Why would you ever base memory access on a pointer that is not
> entirely accurate?
> 
> The manuals for TMC/ETR all state that reads to both RWP and RRP when
> the ETR is running return unknown values. These cannot be used to
> access the buffer, or determine how much of the buffer has been used
> on a running ETR.
> 
> The ETR specification specifically states that it is not permitted to
> read the buffer data while the ETR is running, when configured in
> circular buffer mode - which is the mode used in the TMC-ETR linux
> drivers.
> 
> Reading the buffer while ETR is running is only permitted if
> configured in Software FIFO mode 2 - were the ETR will stop on full
> and stall incoming trace until some data is read out, signalled to the
> ETR via the RURP.
> 

Hi Mike,

I appreciate for your patient explanation.

I was wrong about read data from etr_buffer. I must follow the 
specification to design a method to reading buffer while ETR is running.

How about the following method:

1. The byte-cntr interrupt handler will count the IRQ triggered number 
when byte-cntr file node is opened.
2. Read the buffer after the ETR is stopped(full or stopped manually) 
according to the counted number. we got the etr->offset, etr->size and 
the counted number, so we can calculate the offset where starts to read.
3. Restart the ETR to keep counting the number of IRQ triggers.

Thanks,
Jie

> I also note that you are reading back the etr_buf data without doing
> any dma_sync operations that the perf and sysfs methods in the driver
> do, after stopping the tmc.
> 
>> byte-cntr devnode is opened, aiming to reduce the length of redundant
>> trace data. In this case, we cannot ensure the TMC is stopped or
>> disabled.
> 
> The specification requires that you must ensure the TMC is stopped to
> read these registers.
> 
> 
>> The byte-cntr only requires an offset to know where it can
>> start before the expected trace data gets into ETR buffer.
>>
>> The w_offset is also read when the byte-cntr function stops, which
>> occurs after the TMC is disabled.
>>
>> Maybe this is not a good idea and I should read r_offset upon open?
>> The primary goal for byte-cntr is trying to transfer useful trace data
>> from the ETR buffer to the userspace, if we start from r_offset, a large
>> number of redundant trace data which the user does not expect will be
>> transferred simultaneously.
>>
>>
> 
> It is difficult to justify adding code to a driver that does not
> correspond to the specification of the hardware device.
> 
> Regards
> 
> Mike
> 
>>>
>>>> +       rwp = tmc_read_rwp(drvdata);
>>>> +       return rwp - paddr;
>>>> +}
>>>> +
>>>> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
>>>> +{
>>>> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
>>>> +       struct etr_sg_table *etr_table = etr_buf->private;
>>>> +       struct tmc_sg_table *table = etr_table->sg_table;
>>>> +       long w_offset;
>>>> +       u64 rwp;
>>>> +
>>>
>>> Same comments as above
>>>
>>>> +       rwp = tmc_read_rwp(drvdata);
>>>> +       w_offset = tmc_sg_get_data_page_offset(table, rwp);
>>>> +
>>>> +       return w_offset;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Retrieve the offset to the write pointer of the ETR buffer based on whether
>>>> + * the memory mode is SG, flat or reserved.
>>>> + */
>>>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
>>>> +{
>>>> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
>>>> +
>>>
>>> As this is an exported function, please ensure that the inputs are
>>> valid - check the pointers
>>
>> Sure, will do.
>>
>> Thanks,
>> Jie
>>
>>>
>>> Code to ensure TMC is flushed and stopped could be inserted here.
>>>
>>> Regards
>>>
>>> Mike
>>>
>>>> +       if (etr_buf->mode == ETR_MODE_ETR_SG)
>>>> +               return tmc_sg_get_rwp_offset(drvdata);
>>>> +       else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
>>>> +               return tmc_flat_resrv_get_rwp_offset(drvdata);
>>>> +       else
>>>> +               return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
>>>> +
>>>>    /*
>>>>     * Alloc pages for the table. Since this will be used by the device,
>>>>     * allocate the pages closer to the device (i.e, dev_to_node(dev)
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
>>>> index b48bc9a01cc0..baedb4dcfc3f 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>>> @@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
>>>>    struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>>>>                                      enum cs_mode mode, void *data);
>>>>    extern const struct attribute_group coresight_etr_group;
>>>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>>>>
>>>>    #endif
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
>>
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

Re: [PATCH v1 0/4] coresight: ctcu: Enable byte-cntr function for TMC ETR

[Jie Gan]

On 3/12/2025 9:22 PM, Mike Leach wrote:
> Hi,
> 
> On Mon, 10 Mar 2025 at 09:05, Jie Gan <quic_jiegan@quicinc.com> wrote:
>>
>> From: Jie Gan <jie.gan@oss.qualcomm.com>
>>
>> The byte-cntr function provided by the CTCU device is used to transfer data
>> from the ETR buffer to the userspace. An interrupt is tiggered if the data
>> size exceeds the threshold set in the BYTECNTRVAL register. The interrupt
>> handler counts the number of triggered interruptions and the read function
>> will read the data from the ETR buffer if the IRQ count is greater than 0.
>> Each successful read process will decrement the IRQ count by 1.
>>
>> The byte cntr function will start when the device node is opened for reading,
>> and the IRQ count will reset when the byte cntr function has stopped. When
>> the file node is opened, the w_offset of the ETR buffer will be read and
>> stored in byte_cntr_data, serving as the original r_offset (indicating
>> where reading starts) for the byte counter function.
>>
>> The work queue for the read operation will wake up once when ETR is stopped,
>> ensuring that the remaining data in the ETR buffer has been flushed based on
>> the w_offset read at the time of stopping.
>>
>> The following shell commands write threshold to BYTECNTRVAL registers.
>>
>> Only enable byte-cntr for ETR0:
>> echo 0x10000 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>>
>> Enable byte-cntr for both ETR0 and ETR1(support both hex and decimal values):
>> echo 0x10000 4096 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>>
>> Setting the BYTECNTRVAL registers to 0 disables the byte-cntr function.
>> Disable byte-cntr for ETR0:
>> echo 0 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>>
>> Disable byte-cntr for both ETR0 and ETR1:
>> echo 0 0 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>>
>> There is a minimum threshold to prevent generating too many interrupts.
>> The minimum threshold is 4096 bytes. The write process will fail if user try
>> to set the BYTECNTRVAL registers to a value less than 4096 bytes(except
>> for 0).
>>
>> Finally, the user can read data from the ETR buffer through the byte-cntr file
>> nodes located under /dev, for example reads data from the ETR0 buffer:
>> cat /dev/byte-cntr0
>>
>> Way to enable and start byte-cntr for ETR0:
>> echo 0x10000 > /sys/devices/platform/soc@0/4001000.ctcu/ctcu0/byte_cntr_val
>> echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
>> echo 1 > /sys/bus/coresight/devices/etm0/enable_source
>> cat /dev/byte-cntr0
>>
> 
> There is a significant issue with attempting to drain an ETR buffer
> while it is live in the way you appear to be doing.
> 
> You have no way of knowing if the TMC hardware write pointer wraps and
> overtakes the point where you are currently reading. This could cause
> data corruption as TMC writes as you are reading, or contention for
> the buffer that affects the TMC write.
> 
> Even if those two events do not occur, then the trace capture sequence
> is corrupted.
> 
> Take a simple example - suppose we split the buffer into 4 blocks of
> trace, which are filled by the ETR
> 
> buffer = 1, 2, 3, 4
> 
> Now you suppose you have read 1 & 2 into your userspace buffer / file.
> 
> file = 1, 2
> 
> If there is now some system event that prevents your userspace code
> from running for a while, then it is possible that the ETR continues,
> wraps and the buffer is now
> 
> buffer = 5, 6, 7, 4
> 
> Your next two reads will be 7, 4
> 
> file = 1, 2, 7, 4
> 
> This trace is now corrupt and will cause decode errors. There is no
> way for the decoder to determine that the interface between blocks 2 &
> 7 is not correct. If you are fortunate then this issue will cause an
> actual explicit decode error, if you are less fortunate then decode
> will continue but in fact be inaccurate, with no obvious way to detect
> the inaccuracy.
> 
> We encountered this problem early in the development of the perf data
> collection. Even though perf was stopping the trace to copy the
> hardware buffer, it would concatenate unrelated trace blocks into the
> perf userspace buffer, which initially caused decoding errors. This is
> now mitigated in perf by marking boundaries and recording indexes of
> the boundaries, so the tool can reset the decoder at the start of non
> contiguous blocks.
> 
> If you do not stop the TMC when draining the ETR buffer, you have no
> way of determining if this has occurred.
> 
> Clearly using large buffers, split into smaller blocks can mitigate
> the possibility of a wrap in this way - but never eliminate it,
> especially given the extreme rate that trace data can be generated.
> 

Hi Mike,

Thanks for detailed explanation. It's clear and makes sense to me.

I will look for another reasonable solution.

Thanks,
Jie

> Regards
> 
> Mike
> 
> 
>> Jie Gan (4):
>>    coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer
>>    dt-bindings: arm: Add an interrupt property for Coresight CTCU
>>    coresight: ctcu: Enable byte-cntr for TMC ETR devices
>>    arm64: dts: qcom: sa8775p: Add interrupts to CTCU device
>>
>>   .../bindings/arm/qcom,coresight-ctcu.yaml     |  17 +
>>   arch/arm64/boot/dts/qcom/sa8775p.dtsi         |   5 +
>>   drivers/hwtracing/coresight/Makefile          |   2 +-
>>   .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
>>   .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
>>   drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
>>   .../hwtracing/coresight/coresight-tmc-etr.c   |  45 ++-
>>   drivers/hwtracing/coresight/coresight-tmc.h   |   3 +
>>   8 files changed, 556 insertions(+), 10 deletions(-)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>
>> --
>> 2.34.1
>>
> 
> 

Re: [PATCH v1 1/4] coresight: tmc: Introduce new APIs to get the RWP offset of ETR buffer

[Jie Gan]

On 3/13/2025 9:32 AM, Jie Gan wrote:
> 
> 
> On 3/12/2025 8:54 PM, Mike Leach wrote:
>> Hi Jie,
>>
>> On Wed, 12 Mar 2025 at 01:21, Jie Gan <quic_jiegan@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 3/12/2025 12:49 AM, Mike Leach wrote:
>>>> Hi,
>>>>
>>>> On Mon, 10 Mar 2025 at 09:04, Jie Gan <quic_jiegan@quicinc.com> wrote:
>>>>>
>>>>> The new functions calculate and return the offset to the write 
>>>>> pointer of
>>>>> the ETR buffer based on whether the memory mode is SG, flat or 
>>>>> reserved.
>>>>> The functions have the RWP offset can directly read data from ETR 
>>>>> buffer,
>>>>> enabling the transfer of data to any required location.
>>>>>
>>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>>> ---
>>>>>    .../hwtracing/coresight/coresight-tmc-etr.c   | 40 +++++++++++++ 
>>>>> ++++++
>>>>>    drivers/hwtracing/coresight/coresight-tmc.h   |  1 +
>>>>>    2 files changed, 41 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ 
>>>>> drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>> index eda7cdad0e2b..ec636ab1fd75 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>> @@ -267,6 +267,46 @@ void tmc_free_sg_table(struct tmc_sg_table 
>>>>> *sg_table)
>>>>>    }
>>>>>    EXPORT_SYMBOL_GPL(tmc_free_sg_table);
>>>>>
>>>>> +static long tmc_flat_resrv_get_rwp_offset(struct tmc_drvdata 
>>>>> *drvdata)
>>>>> +{
>>>>> +       dma_addr_t paddr = drvdata->sysfs_buf->hwaddr;
>>>>> +       u64 rwp;
>>>>> +
>>>>
>>>> It is not valid to read RWP if the TMC is running. It must be in the
>>>> stopped or disabled state - see the specifications for TMC /ETR
>>>>
>>>> It is likely that CSUNLOCK / CSLOCK are needed here too,  along with
>>>> the spinlock that protects drvdata
>>>>
>>>> See the code in coresight_tmc_etr.c :-
>>>>
>>>> e.g. in
>>>>
>>>> tmc_update_etr_buffer()
>>>>
>>>> ...
>>>> <take spinlock>
>>>> ...
>>>> CS_UNLOCK(drvdata->base);
>>>> tmc_flush_and_stop(drvdata); // this ensures tmc is stopped and
>>>> flushed to memory - essential to ensure full formatted frame is in
>>>> memory.
>>>> tmc_sync_etr_buf(drvdata); // this function reads rwp.
>>>> CS_LOCK(drvdata->base);
>>>> <release spinlokc>
>>>>
>>>> This type of program flow is common to both sysfs and perf handling of
>>>> TMC buffers.
>>>
>>> Hi Mike,
>>>
>>> I am fully understood your point here.
>>>
>>> The function is designed this way to read the w_offset (which may not be
>>> entirely accurate because the etr buffer is not synced) when the
>>
>> Why would you ever base memory access on a pointer that is not
>> entirely accurate?
>>
>> The manuals for TMC/ETR all state that reads to both RWP and RRP when
>> the ETR is running return unknown values. These cannot be used to
>> access the buffer, or determine how much of the buffer has been used
>> on a running ETR.
>>
>> The ETR specification specifically states that it is not permitted to
>> read the buffer data while the ETR is running, when configured in
>> circular buffer mode - which is the mode used in the TMC-ETR linux
>> drivers.
>>
>> Reading the buffer while ETR is running is only permitted if
>> configured in Software FIFO mode 2 - were the ETR will stop on full
>> and stall incoming trace until some data is read out, signalled to the
>> ETR via the RURP.
>>
> 
> Hi Mike,
> 
> I appreciate for your patient explanation.
> 
> I was wrong about read data from etr_buffer. I must follow the 
> specification to design a method to reading buffer while ETR is running.
> 
> How about the following method:
> 
> 1. The byte-cntr interrupt handler will count the IRQ triggered number 
> when byte-cntr file node is opened.
> 2. Read the buffer after the ETR is stopped(full or stopped manually) 
> according to the counted number. we got the etr->offset, etr->size and 
> the counted number, so we can calculate the offset where starts to read.
> 3. Restart the ETR to keep counting the number of IRQ triggers.
> 
> Thanks,
> Jie

Hi Mike,

Please ignore my previous proposal.

Let me clarify the purpose of the byte-cntr function to ensure there are 
no misunderstandings from my previous comments.

First, we definitely need to change the method for reading the rwp 
pointer from the RWP register.The new method ensures that we obtain a 
valid rwp value.

code snippet:
long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
{
	struct etr_buf *etr_buf;
	unsigned long flags;
	long w_offset;

	if (WARN_ON(!drvdata->sysfs_buf))
		return -EINVAL;

	spin_lock_irqsave(&drvdata->spinlock, flags);
	etr_buf = drvdata->sysfs_buf;
	if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
		__tmc_etr_disable_hw(drvdata);

	if (etr_buf->mode == ETR_MODE_ETR_SG)
		w_offset = tmc_sg_get_rwp_offset(drvdata);
	else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == ETR_MODE_RESRV)
		w_offset = tmc_flat_resrv_get_rwp_offset(drvdata);
	else
		w_offset = -EINVAL;

	if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
		__tmc_etr_enable_hw(drvdata);

	spin_unlock_irqrestore(&drvdata->spinlock, flags);

	return w_offset;
}

The rwp offset is necessary because the length of the trace data 
transferred by the byte-cntr depends on the IRQ count number and the rwp 
value is read (as a benchmark to indicate where we start from) when the 
interrupt is enabled.

With a valid rwp value, we can copy trace data from the ETR buffer (The 
trace data in the ETR buffer can still be read using tmc_read) based on 
the IRQ count number.

We need the byte-cntr function to continue transferring data from the 
ETR buffer to userspace (because the etr buffer has some limitations to 
store trace data) while the ETR is running but we just utilize the rwp 
value as a benchmark to copy trace data from etr buffer, is this acceptable?

Please correct me if I am wrong.

Thanks,
Jie

> 
>> I also note that you are reading back the etr_buf data without doing
>> any dma_sync operations that the perf and sysfs methods in the driver
>> do, after stopping the tmc.
>>
>>> byte-cntr devnode is opened, aiming to reduce the length of redundant
>>> trace data. In this case, we cannot ensure the TMC is stopped or
>>> disabled.
>>
>> The specification requires that you must ensure the TMC is stopped to
>> read these registers.
>>
>>
>>> The byte-cntr only requires an offset to know where it can
>>> start before the expected trace data gets into ETR buffer.
>>>
>>> The w_offset is also read when the byte-cntr function stops, which
>>> occurs after the TMC is disabled.
>>>
>>> Maybe this is not a good idea and I should read r_offset upon open?
>>> The primary goal for byte-cntr is trying to transfer useful trace data
>>> from the ETR buffer to the userspace, if we start from r_offset, a large
>>> number of redundant trace data which the user does not expect will be
>>> transferred simultaneously.
>>>
>>>
>>
>> It is difficult to justify adding code to a driver that does not
>> correspond to the specification of the hardware device.
>>
>> Regards
>>
>> Mike
>>
>>>>
>>>>> +       rwp = tmc_read_rwp(drvdata);
>>>>> +       return rwp - paddr;
>>>>> +}
>>>>> +
>>>>> +static long tmc_sg_get_rwp_offset(struct tmc_drvdata *drvdata)
>>>>> +{
>>>>> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
>>>>> +       struct etr_sg_table *etr_table = etr_buf->private;
>>>>> +       struct tmc_sg_table *table = etr_table->sg_table;
>>>>> +       long w_offset;
>>>>> +       u64 rwp;
>>>>> +
>>>>
>>>> Same comments as above
>>>>
>>>>> +       rwp = tmc_read_rwp(drvdata);
>>>>> +       w_offset = tmc_sg_get_data_page_offset(table, rwp);
>>>>> +
>>>>> +       return w_offset;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Retrieve the offset to the write pointer of the ETR buffer 
>>>>> based on whether
>>>>> + * the memory mode is SG, flat or reserved.
>>>>> + */
>>>>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata)
>>>>> +{
>>>>> +       struct etr_buf *etr_buf = drvdata->sysfs_buf;
>>>>> +
>>>>
>>>> As this is an exported function, please ensure that the inputs are
>>>> valid - check the pointers
>>>
>>> Sure, will do.
>>>
>>> Thanks,
>>> Jie
>>>
>>>>
>>>> Code to ensure TMC is flushed and stopped could be inserted here.
>>>>
>>>> Regards
>>>>
>>>> Mike
>>>>
>>>>> +       if (etr_buf->mode == ETR_MODE_ETR_SG)
>>>>> +               return tmc_sg_get_rwp_offset(drvdata);
>>>>> +       else if (etr_buf->mode == ETR_MODE_FLAT || etr_buf->mode == 
>>>>> ETR_MODE_RESRV)
>>>>> +               return tmc_flat_resrv_get_rwp_offset(drvdata);
>>>>> +       else
>>>>> +               return -EINVAL;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(tmc_get_rwp_offset);
>>>>> +
>>>>>    /*
>>>>>     * Alloc pages for the table. Since this will be used by the 
>>>>> device,
>>>>>     * allocate the pages closer to the device (i.e, dev_to_node(dev)
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/ 
>>>>> hwtracing/coresight/coresight-tmc.h
>>>>> index b48bc9a01cc0..baedb4dcfc3f 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>>>> @@ -442,5 +442,6 @@ void tmc_etr_remove_catu_ops(void);
>>>>>    struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>>>>>                                      enum cs_mode mode, void *data);
>>>>>    extern const struct attribute_group coresight_etr_group;
>>>>> +long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>>>>>
>>>>>    #endif
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>
>>>>
>>>
>>
>>
>> -- 
>> Mike Leach
>> Principal Engineer, ARM Ltd.
>> Manchester Design Centre. UK
> 
> 

Re: [PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices

[Suzuki K Poulose]

On 10/03/2025 09:04, Jie Gan wrote:
> The byte-cntr function provided by the CTCU device is used to transfer data
> from the ETR buffer to the userspace. An interrupt is triggered if the data

Why do we need a new device to transfer the data to userspace ?

> size exceeds the threshold set in the BYTECNTRVAL register. The interrupt
> handler counts the number of triggered interruptions and the read function
> will read the data from the ETR buffer if the IRQ count is greater than 0.
> Each successful read process will decrement the IRQ count by 1.

Having an interrupt is good, but see below.

> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>   drivers/hwtracing/coresight/Makefile          |   2 +-
>   .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
>   .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
>   drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
>   .../hwtracing/coresight/coresight-tmc-etr.c   |   5 +-
>   drivers/hwtracing/coresight/coresight-tmc.h   |   2 +
>   6 files changed, 493 insertions(+), 10 deletions(-)
>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
> 
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index 8e62c3150aeb..c90a06768a18 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -52,4 +52,4 @@ coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
>   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>   obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
> -coresight-ctcu-y := coresight-ctcu-core.o
> +coresight-ctcu-y := coresight-ctcu-core.o coresight-ctcu-byte-cntr.o
> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
> new file mode 100644
> index 000000000000..0d16385663f5
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/coresight.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/fs.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_irq.h>
> +#include <linux/property.h>
> +#include <linux/uaccess.h>
> +
> +#include "coresight-ctcu.h"
> +#include "coresight-priv.h"
> +#include "coresight-tmc.h"
> +
> +#define BYTE_CNTR_CLASS_STR "byte-cntr-class"
> +
> +static struct class *byte_cntr_class;
> +static dev_t byte_cntr_base;
> +
> +static irqreturn_t byte_cntr_handler(int irq, void *data)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data = (struct ctcu_byte_cntr *)data;
> +
> +	atomic_inc(&byte_cntr_data->irq_cnt);
> +	wake_up(&byte_cntr_data->wq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Read the data from ETR's DDR buffer. */
> +static void __ctcu_byte_cntr_read_etr_bytes(struct ctcu_byte_cntr *byte_cntr_data,
> +					    size_t *len, char **bufp)
> +{
> +	struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
> +	size_t actual, bytes = byte_cntr_data->thresh_val;
> +	struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
> +	long r_offset;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	r_offset = byte_cntr_data->r_offset;
> +	if (*len >= bytes)
> +		*len = bytes;
> +	else if ((r_offset % bytes) + *len > bytes)
> +		*len = bytes - (r_offset % bytes);
> +
> +	actual = tmc_etr_buf_get_data(etr_buf, r_offset, *len, bufp);
> +	*len = actual;
> +	if (actual == bytes || (actual + r_offset) % bytes == 0)
> +		atomic_dec(&byte_cntr_data->irq_cnt);
> +}
> +

How can you safely read the ETR data while it is running ? You should
stop the ETR and then provide the data. You could potentially do things
like split the ETR buffer into two halves and switch the half used by
the ETR on interrupt and expose this to user.


> +/* Flush the remaining data in the ETR buffer after the byte cntr has stopped. */
> +static ssize_t ctcu_flush_etr_buffer(struct ctcu_byte_cntr *byte_cntr_data, size_t len,
> +				     char **bufpp)
> +{
> +	struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
> +	struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
> +	ssize_t read_len = 0, remaining_len;
> +	long r_offset, w_offset;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	r_offset = byte_cntr_data->r_offset;
> +	w_offset = byte_cntr_data->w_offset;
> +	if (w_offset < r_offset)
> +		remaining_len = tmcdrvdata->size + w_offset - r_offset;
> +	else
> +		remaining_len = w_offset - r_offset;
> +
> +	if (remaining_len > len)
> +		remaining_len = len;
> +
> +	if (remaining_len > 0)
> +		read_len = tmc_etr_buf_get_data(etr_buf, r_offset, remaining_len, bufpp);
> +
> +	return read_len;
> +}
> +
> +static ssize_t ctcu_byte_cntr_copy_data(char __user *data, size_t len, char *bufp,
> +					struct ctcu_byte_cntr *byte_cntr_data,
> +					struct tmc_drvdata *tmcdrvdata)
> +{
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	if (copy_to_user(data, bufp, len))
> +		return -EFAULT;
> +
> +	byte_cntr_data->total_size += len;
> +	if (byte_cntr_data->r_offset + len >= tmcdrvdata->size)
> +		byte_cntr_data->r_offset = 0;
> +	else
> +		byte_cntr_data->r_offset += len;
> +
> +	return len;
> +}
> +
> +/* The read function for /dev/byte-cntr%d */

WHY ? WHY can't the data be exported by the existing /dev/tmc_etr* ?

> +static ssize_t ctcu_byte_cntr_read_etr_bytes(struct file *fp, char __user *data,
> +					     size_t len, loff_t *ppos)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
> +	struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
> +	char *bufp = NULL;
> +	ssize_t read_len;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	/*
> +	 * Flush the remaining data in the ETR buffer based on the write
> +	 * offset of the ETR buffer when the byte cntr function has stopped.
> +	 */
> +	if (!byte_cntr_data->read_active || !byte_cntr_data->enable) {
> +		read_len = ctcu_flush_etr_buffer(byte_cntr_data, len, &bufp);
> +		if (read_len > 0)
> +			return ctcu_byte_cntr_copy_data(data, read_len, bufp,
> +							byte_cntr_data, tmcdrvdata);
> +
> +		return -EINVAL;
> +	}
> +
> +	if (!atomic_read(&byte_cntr_data->irq_cnt)) {
> +		if (wait_event_interruptible(byte_cntr_data->wq,
> +					     atomic_read(&byte_cntr_data->irq_cnt) > 0 ||
> +					     !byte_cntr_data->enable))
> +			return -ERESTARTSYS;
> +	}
> +
> +	__ctcu_byte_cntr_read_etr_bytes(byte_cntr_data, &len, &bufp);
> +
> +	return ctcu_byte_cntr_copy_data(data, len, bufp, byte_cntr_data, tmcdrvdata);
> +}

Could we not plug this into TMC-ETR read ?

NAK for a special file approach. As Mike mentioned you need to make sure 
we are doing this safely.

Suzuki



> +
> +/* Start the byte-cntr function when the path is enabled. */
> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct coresight_path *path)
> +{
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct coresight_device *sink = coresight_get_sink(path);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	int port_num;
> +
> +	if (!sink)
> +		return;
> +
> +	port_num = ctcu_get_active_port(sink, csdev);
> +	if (port_num < 0)
> +		return;
> +
> +	byte_cntr_data = &drvdata->byte_cntr_data[port_num];
> +	/* Don't start byte-cntr function when threshold is not set. */
> +	if (!byte_cntr_data->thresh_val)
> +		return;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	atomic_set(&byte_cntr_data->irq_cnt, 0);
> +	byte_cntr_data->sink = sink;
> +	byte_cntr_data->enable = true;
> +}
> +
> +/* Stop the byte-cntr function when the path is disabled. */
> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct coresight_path *path)
> +{
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	struct coresight_device *sink = coresight_get_sink(path);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	struct tmc_drvdata *tmcdrvdata;
> +	int port_num;
> +
> +	if (!sink)
> +		return;
> +
> +	port_num = ctcu_get_active_port(sink, csdev);
> +	if (port_num < 0)
> +		return;
> +
> +	byte_cntr_data = &drvdata->byte_cntr_data[port_num];
> +	tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	/* Store the w_offset of the ETR buffer when stopping. */
> +	byte_cntr_data->w_offset = tmc_get_rwp_offset(tmcdrvdata);
> +	atomic_set(&byte_cntr_data->irq_cnt, 0);
> +	byte_cntr_data->read_active = false;
> +	byte_cntr_data->enable = false;
> +	/*
> +	 * Wakeup once to force the read function to read the remaining
> +	 * data of the ETR buffer.
> +	 */
> +	wake_up(&byte_cntr_data->wq);
> +}
> +
> +static int ctcu_byte_cntr_release(struct inode *in, struct file *fp)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
> +	struct device *dev = &byte_cntr_data->sink->dev;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	atomic_set(&byte_cntr_data->irq_cnt, 0);
> +	byte_cntr_data->read_active = false;
> +	disable_irq_wake(byte_cntr_data->byte_cntr_irq);
> +	dev_dbg(dev, "send data total size: %llu bytes, r_offset: %ld w_offset: %ld\n",
> +		byte_cntr_data->total_size, byte_cntr_data->r_offset,
> +		byte_cntr_data->w_offset);
> +
> +	return 0;
> +}
> +
> +static int ctcu_byte_cntr_open(struct inode *in, struct file *fp)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data = container_of(in->i_cdev,
> +							     struct ctcu_byte_cntr, c_dev);
> +	struct tmc_drvdata *tmcdrvdata;
> +
> +	if (byte_cntr_data->read_active)
> +		return -EBUSY;
> +
> +	if (!byte_cntr_data->thresh_val || !byte_cntr_data->sink ||
> +	    (coresight_get_mode(byte_cntr_data->sink) !=  CS_MODE_SYSFS))
> +		return -EINVAL;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	enable_irq_wake(byte_cntr_data->byte_cntr_irq);
> +	fp->private_data = byte_cntr_data;
> +	nonseekable_open(in, fp);
> +	tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
> +	/*
> +	 * The original r_offset is the w_offset of the ETR buffer at the
> +	 * start of the byte-cntr.
> +	 */
> +	byte_cntr_data->r_offset = tmc_get_rwp_offset(tmcdrvdata);
> +	byte_cntr_data->total_size = 0;
> +	byte_cntr_data->read_active = true;
> +	byte_cntr_data->enable = true;
> +
> +	return 0;
> +}
> +
> +static const struct file_operations byte_cntr_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= ctcu_byte_cntr_open,
> +	.read		= ctcu_byte_cntr_read_etr_bytes,
> +	.release	= ctcu_byte_cntr_release,
> +};
> +
> +static int ctcu_byte_cntr_register_chardev(struct ctcu_byte_cntr *byte_cntr_data,
> +					   int port_num)
> +{
> +	struct device *device;
> +	dev_t devt;
> +	int ret;
> +
> +	cdev_init(&byte_cntr_data->c_dev, &byte_cntr_fops);
> +	devt = MKDEV(MAJOR(byte_cntr_base), MINOR(byte_cntr_base) + port_num);
> +	ret = cdev_add(&byte_cntr_data->c_dev, devt, 1);
> +	if (ret < 0)
> +		return -ENOMEM;
> +
> +	device = device_create(byte_cntr_class, NULL, devt, byte_cntr_data,
> +			       byte_cntr_data->name);
> +
> +	if (IS_ERR(device))
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void ctcu_byte_cntr_unregister_chardev(struct ctcu_drvdata *drvdata)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	int i;
> +
> +	for (i = 0; i < ETR_MAX_NUM; i++) {
> +		byte_cntr_data = &drvdata->byte_cntr_data[i];
> +		device_destroy(byte_cntr_class, byte_cntr_data->c_dev.dev);
> +	}
> +}
> +
> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata *drvdata, int etr_num)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	struct device_node *nd = dev->of_node;
> +	int byte_cntr_irq, ret, i;
> +
> +	ret = alloc_chrdev_region(&byte_cntr_base, 0, ETR_MAX_NUM, BYTE_CNTR_CLASS_STR);
> +	if (ret < 0)
> +		return -ENOMEM;
> +
> +	byte_cntr_class = class_create(BYTE_CNTR_CLASS_STR);
> +	if (IS_ERR(byte_cntr_class)) {
> +		unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < etr_num; i++) {
> +		byte_cntr_data = &drvdata->byte_cntr_data[i];
> +		byte_cntr_irq = of_irq_get_byname(nd, byte_cntr_data->irq_name);
> +		if (byte_cntr_irq < 0) {
> +			ret = byte_cntr_irq;
> +			goto err_exit;
> +		}
> +
> +		ret = devm_request_irq(dev, byte_cntr_irq, byte_cntr_handler,
> +				       IRQF_TRIGGER_RISING | IRQF_SHARED,
> +				       dev_name(dev), byte_cntr_data);
> +		if (ret) {
> +			dev_err(dev, "Failed to register IRQ for %s\n",
> +				byte_cntr_data->name);
> +			goto err_exit;
> +		}
> +
> +		ret = ctcu_byte_cntr_register_chardev(byte_cntr_data, i);
> +		if (ret) {
> +			dev_err(dev, "Failed to register chardev for %s\n",
> +				byte_cntr_data->name);
> +			goto err_exit;
> +		}
> +
> +		byte_cntr_data->byte_cntr_irq = byte_cntr_irq;
> +		atomic_set(&byte_cntr_data->irq_cnt, 0);
> +		init_waitqueue_head(&byte_cntr_data->wq);
> +	}
> +
> +	return 0;
> +
> +err_exit:
> +	ctcu_byte_cntr_unregister_chardev(drvdata);
> +	class_destroy(byte_cntr_class);
> +	unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
> +	return ret;
> +}
> +
> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata)
> +{
> +	ctcu_byte_cntr_unregister_chardev(drvdata);
> +	class_destroy(byte_cntr_class);
> +	unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
> +}
> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index da35d8b4d579..5782655a5f39 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> @@ -46,16 +46,24 @@ DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
>   #define CTCU_ATID_REG_BIT(traceid)	(traceid % 32)
>   #define CTCU_ATID_REG_SIZE		0x10
>   #define CTCU_ETR0_ATID0			0xf8
> +#define CTCU_ETR0_IRQCTRL		0x6c
>   #define CTCU_ETR1_ATID0			0x108
> +#define CTCU_ETR1_IRQCTRL		0x70
>   
>   static const struct ctcu_etr_config sa8775p_etr_cfgs[] = {
>   	{
> -		.atid_offset	= CTCU_ETR0_ATID0,
> -		.port_num	= 0,
> +		.atid_offset		= CTCU_ETR0_ATID0,
> +		.irq_ctrl_offset	= CTCU_ETR0_IRQCTRL,
> +		.irq_name		= "etr0",
> +		.byte_cntr_name		= "byte-cntr0",
> +		.port_num		= 0,
>   	},
>   	{
> -		.atid_offset	= CTCU_ETR1_ATID0,
> -		.port_num	= 1,
> +		.atid_offset		= CTCU_ETR1_ATID0,
> +		.irq_ctrl_offset	= CTCU_ETR1_IRQCTRL,
> +		.irq_name		= "etr1",
> +		.byte_cntr_name		= "byte-cntr1",
> +		.port_num		= 1,
>   	},
>   };
>   
> @@ -64,6 +72,69 @@ static const struct ctcu_config sa8775p_cfgs = {
>   	.num_etr_config	= ARRAY_SIZE(sa8775p_etr_cfgs),
>   };
>   
> +static ssize_t byte_cntr_val_show(struct device *dev, struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	int i, len = 0;
> +
> +	for (i = 0; i < ETR_MAX_NUM; i++) {
> +		if (drvdata->byte_cntr_data[i].irq_ctrl_offset)
> +			len += scnprintf(buf + len, PAGE_SIZE - len, "%u ",
> +					 drvdata->byte_cntr_data[i].thresh_val);
> +	}
> +
> +	len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
> +
> +	return len;
> +}
> +
> +static ssize_t byte_cntr_val_store(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t size)
> +{
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	u32 thresh_vals[ETR_MAX_NUM] = { 0 };
> +	u32 irq_ctrl_offset;
> +	int num, i;
> +
> +	num = sscanf(buf, "%i %i", &thresh_vals[0], &thresh_vals[1]);
> +	if (num <= 0 || num > ETR_MAX_NUM)
> +		return -EINVAL;
> +
> +	/* Threshold 0 disables the interruption. */
> +	guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
> +	for (i = 0; i < num; i++) {
> +		/* A small threshold will result in a large number of interruptions */
> +		if (thresh_vals[i] && thresh_vals[i] < 4096)
> +			return -EINVAL;
> +
> +		if (drvdata->byte_cntr_data[i].irq_ctrl_offset) {
> +			drvdata->byte_cntr_data[i].thresh_val = thresh_vals[i];
> +			irq_ctrl_offset = drvdata->byte_cntr_data[i].irq_ctrl_offset;
> +			CS_UNLOCK(drvdata->base);
> +			writel_relaxed(thresh_vals[i], drvdata->base + irq_ctrl_offset);
> +			CS_LOCK(drvdata->base);
> +		}
> +	}
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(byte_cntr_val);
> +
> +static struct attribute *ctcu_attrs[] = {
> +	&dev_attr_byte_cntr_val.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group ctcu_attr_grp = {
> +	.attrs = ctcu_attrs,
> +};
> +
> +static const struct attribute_group *ctcu_attr_grps[] = {
> +	&ctcu_attr_grp,
> +	NULL,
> +};
> +
>   static void ctcu_program_atid_register(struct ctcu_drvdata *drvdata, u32 reg_offset,
>   				       u8 bit, bool enable)
>   {
> @@ -122,7 +193,7 @@ static int __ctcu_set_etr_traceid(struct coresight_device *csdev, u8 traceid, in
>    * Searching the sink device from helper's view in case there are multiple helper devices
>    * connected to the sink device.
>    */
> -static int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper)
> +int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper)
>   {
>   	struct coresight_platform_data *pdata = helper->pdata;
>   	int i;
> @@ -160,6 +231,8 @@ static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, void *
>   {
>   	struct coresight_path *path = (struct coresight_path *)data;
>   
> +	ctcu_byte_cntr_start(csdev, path);
> +
>   	return ctcu_set_etr_traceid(csdev, path, true);
>   }
>   
> @@ -167,6 +240,8 @@ static int ctcu_disable(struct coresight_device *csdev, void *data)
>   {
>   	struct coresight_path *path = (struct coresight_path *)data;
>   
> +	ctcu_byte_cntr_stop(csdev, path);
> +
>   	return ctcu_set_etr_traceid(csdev, path, false);
>   }
>   
> @@ -188,7 +263,7 @@ static int ctcu_probe(struct platform_device *pdev)
>   	const struct ctcu_config *cfgs;
>   	struct ctcu_drvdata *drvdata;
>   	void __iomem *base;
> -	int i;
> +	int ret, i;
>   
>   	desc.name = coresight_alloc_device_name(&ctcu_devs, dev);
>   	if (!desc.name)
> @@ -217,7 +292,14 @@ static int ctcu_probe(struct platform_device *pdev)
>   			for (i = 0; i < cfgs->num_etr_config; i++) {
>   				etr_cfg = &cfgs->etr_cfgs[i];
>   				drvdata->atid_offset[i] = etr_cfg->atid_offset;
> +				drvdata->byte_cntr_data[i].irq_name = etr_cfg->irq_name;
> +				drvdata->byte_cntr_data[i].name = etr_cfg->byte_cntr_name;
> +				drvdata->byte_cntr_data[i].irq_ctrl_offset =
> +					etr_cfg->irq_ctrl_offset;
>   			}
> +			ret = ctcu_byte_cntr_init(dev, drvdata, cfgs->num_etr_config);
> +			if (ret < 0)
> +				dev_warn(dev, "Byte cntr init failed\n");
>   		}
>   	}
>   
> @@ -229,6 +311,7 @@ static int ctcu_probe(struct platform_device *pdev)
>   	desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU;
>   	desc.pdata = pdata;
>   	desc.dev = dev;
> +	desc.groups = ctcu_attr_grps;
>   	desc.ops = &ctcu_ops;
>   	desc.access = CSDEV_ACCESS_IOMEM(base);
>   
> @@ -247,6 +330,7 @@ static void ctcu_remove(struct platform_device *pdev)
>   {
>   	struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
>   
> +	ctcu_byte_cntr_remove(drvdata);
>   	coresight_unregister(drvdata->csdev);
>   }
>   
> diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/hwtracing/coresight/coresight-ctcu.h
> index e9594c38dd91..e38535c91090 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu.h
> +++ b/drivers/hwtracing/coresight/coresight-ctcu.h
> @@ -5,6 +5,8 @@
>   
>   #ifndef _CORESIGHT_CTCU_H
>   #define _CORESIGHT_CTCU_H
> +#include <linux/cdev.h>
> +
>   #include "coresight-trace-id.h"
>   
>   /* Maximum number of supported ETR devices for a single CTCU. */
> @@ -13,10 +15,16 @@
>   /**
>    * struct ctcu_etr_config
>    * @atid_offset:	offset to the ATID0 Register.
> - * @port_num:		in-port number of CTCU device that connected to ETR.
> + * @irq_ctrl_offset:	offset to the BYTECNTRVAL register.
> + * @irq_name:		IRQ name in dt node.
> + * @byte_cntr_name:	name of the byte cntr device node.
> + * @port_num:		in-port number of the CTCU device that connected to ETR.
>    */
>   struct ctcu_etr_config {
>   	const u32 atid_offset;
> +	const u32 irq_ctrl_offset;
> +	const char *irq_name;
> +	const char *byte_cntr_name;
>   	const u32 port_num;
>   };
>   
> @@ -25,15 +33,64 @@ struct ctcu_config {
>   	int num_etr_config;
>   };
>   
> +/**
> + * struct ctcu_byte_cntr
> + * @c_dev:		cdev for byte_cntr.
> + * @sink		csdev of sink device.
> + * @enable:		indicates that byte_cntr function is enabled or not.
> + * @read_active:	indicates that byte-cntr node is opened or not.
> + * @thresh_val:		threshold to trigger a interruption.
> + * @total_size		total size of transferred data.
> + * @byte_cntr_irq:	IRQ number.
> + * @irq_cnt:		IRQ count.
> + * @wq:			workqueue of reading ETR data.
> + * @read_work:		work of reading ETR data.
> + * @spin_lock:		spinlock of byte cntr data.
> + * @r_offset:		offset of the pointer where reading begins.
> + * @w_offset:		offset of the write pointer in the ETR buffer when
> + *			the byte cntr is stopped.
> + * @irq_ctrl_offset:	offset to the BYTECNTRVAL Register.
> + * @name:		the name of byte cntr device node.
> + * @irq_name:		IRQ name in DT.
> + */
> +struct ctcu_byte_cntr {
> +	struct cdev		c_dev;
> +	struct coresight_device	*sink;
> +	bool			enable;
> +	bool			read_active;
> +	u32			thresh_val;
> +	u64			total_size;
> +	int			byte_cntr_irq;
> +	atomic_t		irq_cnt;
> +	wait_queue_head_t	wq;
> +	struct work_struct	read_work;
> +	raw_spinlock_t		spin_lock;
> +	long			r_offset;
> +	long			w_offset;
> +	u32			irq_ctrl_offset;
> +	const char		*name;
> +	const char		*irq_name;
> +};
> +
>   struct ctcu_drvdata {
>   	void __iomem		*base;
>   	struct clk		*apb_clk;
>   	struct device		*dev;
>   	struct coresight_device	*csdev;
> +	struct ctcu_byte_cntr   byte_cntr_data[ETR_MAX_NUM];
>   	raw_spinlock_t		spin_lock;
>   	u32			atid_offset[ETR_MAX_NUM];
>   	/* refcnt for each traceid of each sink */
>   	u8			traceid_refcnt[ETR_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
>   };
>   
> +/* Generic functions */
> +int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper);
> +
> +/* Byte-cntr functions */
> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct coresight_path *path);
> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct coresight_path *path);
> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata *drvdata, int port_num);
> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata);
> +
>   #endif
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index ec636ab1fd75..5dc94e890927 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1040,14 +1040,15 @@ static void tmc_free_etr_buf(struct etr_buf *etr_buf)
>    * Returns: The size of the linear data available @pos, with *bufpp
>    * updated to point to the buffer.
>    */
> -static ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
> -				    u64 offset, size_t len, char **bufpp)
> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
> +			     u64 offset, size_t len, char **bufpp)
>   {
>   	/* Adjust the length to limit this transaction to end of buffer */
>   	len = (len < (etr_buf->size - offset)) ? len : etr_buf->size - offset;
>   
>   	return etr_buf->ops->get_data(etr_buf, (u64)offset, len, bufpp);
>   }
> +EXPORT_SYMBOL_GPL(tmc_etr_buf_get_data);
>   
>   static inline s64
>   tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 offset)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index baedb4dcfc3f..2fc77fd4ea25 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -443,5 +443,7 @@ struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>   				   enum cs_mode mode, void *data);
>   extern const struct attribute_group coresight_etr_group;
>   long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf, u64 offset, size_t len,
> +			     char **bufpp);
>   
>   #endif

Re: [PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices

[Jie Gan]

On 3/14/2025 12:24 AM, Suzuki K Poulose wrote:
> On 10/03/2025 09:04, Jie Gan wrote:
>> The byte-cntr function provided by the CTCU device is used to transfer 
>> data
>> from the ETR buffer to the userspace. An interrupt is triggered if the 
>> data
> 
> Why do we need a new device to transfer the data to userspace ?

Hi Suzuki Mike,

The purpose of the byte-cntr is for live debugging, allowing trace data 
to be read while the ETR is running and preventing the required trace 
data from being overwritten before output.

I realized that I forgot to introduce the function of the BYTECNTRCTL 
register.

There is a big concern that reading the etr_buf while the ETR is running 
is unsafe because the etr_buf is not synced, am right?

As I mentioned, we need program a value to the BYTECNTRCTL register. The 
hardware will count the bytes of the trace data that are routed to 
RAM(etr_buf).

The interrupt will be trigger once the counted bytes exceed the 
programmed value. Based on this concept, we can confirm this part of 
trace data is already in the etr_buf and is safe to read. So, it's safe 
for the read function only read this part of trace data in one 
shot(unlike tmc_read, the whole synced etr_buf).

That's how we expect the byte-cntr to work when reading the trace data 
from the etr_buf while the etr is running.

This also explains why we need a separate dev node to manage the read 
function for byte-cntr.

Thanks,
Jie


> 
>> size exceeds the threshold set in the BYTECNTRVAL register. The interrupt
>> handler counts the number of triggered interruptions and the read 
>> function
>> will read the data from the ETR buffer if the IRQ count is greater 
>> than 0.
>> Each successful read process will decrement the IRQ count by 1.
> 
> Having an interrupt is good, but see below.
> 
>>
>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Makefile          |   2 +-
>>   .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
>>   .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
>>   drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
>>   .../hwtracing/coresight/coresight-tmc-etr.c   |   5 +-
>>   drivers/hwtracing/coresight/coresight-tmc.h   |   2 +
>>   6 files changed, 493 insertions(+), 10 deletions(-)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte- 
>> cntr.c
>>
>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/ 
>> coresight/Makefile
>> index 8e62c3150aeb..c90a06768a18 100644
>> --- a/drivers/hwtracing/coresight/Makefile
>> +++ b/drivers/hwtracing/coresight/Makefile
>> @@ -52,4 +52,4 @@ coresight-cti-y := coresight-cti-core.o    
>> coresight-cti-platform.o \
>>   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>>   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>>   obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
>> -coresight-ctcu-y := coresight-ctcu-core.o
>> +coresight-ctcu-y := coresight-ctcu-core.o coresight-ctcu-byte-cntr.o
>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c b/ 
>> drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>> new file mode 100644
>> index 000000000000..0d16385663f5
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include <linux/coresight.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/fs.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/property.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include "coresight-ctcu.h"
>> +#include "coresight-priv.h"
>> +#include "coresight-tmc.h"
>> +
>> +#define BYTE_CNTR_CLASS_STR "byte-cntr-class"
>> +
>> +static struct class *byte_cntr_class;
>> +static dev_t byte_cntr_base;
>> +
>> +static irqreturn_t byte_cntr_handler(int irq, void *data)
>> +{
>> +    struct ctcu_byte_cntr *byte_cntr_data = (struct ctcu_byte_cntr 
>> *)data;
>> +
>> +    atomic_inc(&byte_cntr_data->irq_cnt);
>> +    wake_up(&byte_cntr_data->wq);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +/* Read the data from ETR's DDR buffer. */
>> +static void __ctcu_byte_cntr_read_etr_bytes(struct ctcu_byte_cntr 
>> *byte_cntr_data,
>> +                        size_t *len, char **bufp)
>> +{
>> +    struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data- 
>> >sink->dev.parent);
>> +    size_t actual, bytes = byte_cntr_data->thresh_val;
>> +    struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
>> +    long r_offset;
>> +
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    r_offset = byte_cntr_data->r_offset;
>> +    if (*len >= bytes)
>> +        *len = bytes;
>> +    else if ((r_offset % bytes) + *len > bytes)
>> +        *len = bytes - (r_offset % bytes);
>> +
>> +    actual = tmc_etr_buf_get_data(etr_buf, r_offset, *len, bufp);
>> +    *len = actual;
>> +    if (actual == bytes || (actual + r_offset) % bytes == 0)
>> +        atomic_dec(&byte_cntr_data->irq_cnt);
>> +}
>> +
> 
> How can you safely read the ETR data while it is running ? You should
> stop the ETR and then provide the data. You could potentially do things
> like split the ETR buffer into two halves and switch the half used by
> the ETR on interrupt and expose this to user.
> 
> 
>> +/* Flush the remaining data in the ETR buffer after the byte cntr has 
>> stopped. */
>> +static ssize_t ctcu_flush_etr_buffer(struct ctcu_byte_cntr 
>> *byte_cntr_data, size_t len,
>> +                     char **bufpp)
>> +{
>> +    struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data- 
>> >sink->dev.parent);
>> +    struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
>> +    ssize_t read_len = 0, remaining_len;
>> +    long r_offset, w_offset;
>> +
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    r_offset = byte_cntr_data->r_offset;
>> +    w_offset = byte_cntr_data->w_offset;
>> +    if (w_offset < r_offset)
>> +        remaining_len = tmcdrvdata->size + w_offset - r_offset;
>> +    else
>> +        remaining_len = w_offset - r_offset;
>> +
>> +    if (remaining_len > len)
>> +        remaining_len = len;
>> +
>> +    if (remaining_len > 0)
>> +        read_len = tmc_etr_buf_get_data(etr_buf, r_offset, 
>> remaining_len, bufpp);
>> +
>> +    return read_len;
>> +}
>> +
>> +static ssize_t ctcu_byte_cntr_copy_data(char __user *data, size_t 
>> len, char *bufp,
>> +                    struct ctcu_byte_cntr *byte_cntr_data,
>> +                    struct tmc_drvdata *tmcdrvdata)
>> +{
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    if (copy_to_user(data, bufp, len))
>> +        return -EFAULT;
>> +
>> +    byte_cntr_data->total_size += len;
>> +    if (byte_cntr_data->r_offset + len >= tmcdrvdata->size)
>> +        byte_cntr_data->r_offset = 0;
>> +    else
>> +        byte_cntr_data->r_offset += len;
>> +
>> +    return len;
>> +}
>> +
>> +/* The read function for /dev/byte-cntr%d */
> 
> WHY ? WHY can't the data be exported by the existing /dev/tmc_etr* ?
> 
>> +static ssize_t ctcu_byte_cntr_read_etr_bytes(struct file *fp, char 
>> __user *data,
>> +                         size_t len, loff_t *ppos)
>> +{
>> +    struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
>> +    struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data- 
>> >sink->dev.parent);
>> +    char *bufp = NULL;
>> +    ssize_t read_len;
>> +
>> +    if (!data)
>> +        return -EINVAL;
>> +
>> +    /*
>> +     * Flush the remaining data in the ETR buffer based on the write
>> +     * offset of the ETR buffer when the byte cntr function has stopped.
>> +     */
>> +    if (!byte_cntr_data->read_active || !byte_cntr_data->enable) {
>> +        read_len = ctcu_flush_etr_buffer(byte_cntr_data, len, &bufp);
>> +        if (read_len > 0)
>> +            return ctcu_byte_cntr_copy_data(data, read_len, bufp,
>> +                            byte_cntr_data, tmcdrvdata);
>> +
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!atomic_read(&byte_cntr_data->irq_cnt)) {
>> +        if (wait_event_interruptible(byte_cntr_data->wq,
>> +                         atomic_read(&byte_cntr_data->irq_cnt) > 0 ||
>> +                         !byte_cntr_data->enable))
>> +            return -ERESTARTSYS;
>> +    }
>> +
>> +    __ctcu_byte_cntr_read_etr_bytes(byte_cntr_data, &len, &bufp);
>> +
>> +    return ctcu_byte_cntr_copy_data(data, len, bufp, byte_cntr_data, 
>> tmcdrvdata);
>> +}
> 
> Could we not plug this into TMC-ETR read ?
> 
> NAK for a special file approach. As Mike mentioned you need to make sure 
> we are doing this safely.
> 
> Suzuki
> 
> 
> 
>> +
>> +/* Start the byte-cntr function when the path is enabled. */
>> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct 
>> coresight_path *path)
>> +{
>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    struct coresight_device *sink = coresight_get_sink(path);
>> +    struct ctcu_byte_cntr *byte_cntr_data;
>> +    int port_num;
>> +
>> +    if (!sink)
>> +        return;
>> +
>> +    port_num = ctcu_get_active_port(sink, csdev);
>> +    if (port_num < 0)
>> +        return;
>> +
>> +    byte_cntr_data = &drvdata->byte_cntr_data[port_num];
>> +    /* Don't start byte-cntr function when threshold is not set. */
>> +    if (!byte_cntr_data->thresh_val)
>> +        return;
>> +
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>> +    byte_cntr_data->sink = sink;
>> +    byte_cntr_data->enable = true;
>> +}
>> +
>> +/* Stop the byte-cntr function when the path is disabled. */
>> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct 
>> coresight_path *path)
>> +{
>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    struct coresight_device *sink = coresight_get_sink(path);
>> +    struct ctcu_byte_cntr *byte_cntr_data;
>> +    struct tmc_drvdata *tmcdrvdata;
>> +    int port_num;
>> +
>> +    if (!sink)
>> +        return;
>> +
>> +    port_num = ctcu_get_active_port(sink, csdev);
>> +    if (port_num < 0)
>> +        return;
>> +
>> +    byte_cntr_data = &drvdata->byte_cntr_data[port_num];
>> +    tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
>> +
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    /* Store the w_offset of the ETR buffer when stopping. */
>> +    byte_cntr_data->w_offset = tmc_get_rwp_offset(tmcdrvdata);
>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>> +    byte_cntr_data->read_active = false;
>> +    byte_cntr_data->enable = false;
>> +    /*
>> +     * Wakeup once to force the read function to read the remaining
>> +     * data of the ETR buffer.
>> +     */
>> +    wake_up(&byte_cntr_data->wq);
>> +}
>> +
>> +static int ctcu_byte_cntr_release(struct inode *in, struct file *fp)
>> +{
>> +    struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
>> +    struct device *dev = &byte_cntr_data->sink->dev;
>> +
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>> +    byte_cntr_data->read_active = false;
>> +    disable_irq_wake(byte_cntr_data->byte_cntr_irq);
>> +    dev_dbg(dev, "send data total size: %llu bytes, r_offset: %ld 
>> w_offset: %ld\n",
>> +        byte_cntr_data->total_size, byte_cntr_data->r_offset,
>> +        byte_cntr_data->w_offset);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ctcu_byte_cntr_open(struct inode *in, struct file *fp)
>> +{
>> +    struct ctcu_byte_cntr *byte_cntr_data = container_of(in->i_cdev,
>> +                                 struct ctcu_byte_cntr, c_dev);
>> +    struct tmc_drvdata *tmcdrvdata;
>> +
>> +    if (byte_cntr_data->read_active)
>> +        return -EBUSY;
>> +
>> +    if (!byte_cntr_data->thresh_val || !byte_cntr_data->sink ||
>> +        (coresight_get_mode(byte_cntr_data->sink) !=  CS_MODE_SYSFS))
>> +        return -EINVAL;
>> +
>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>> +    enable_irq_wake(byte_cntr_data->byte_cntr_irq);
>> +    fp->private_data = byte_cntr_data;
>> +    nonseekable_open(in, fp);
>> +    tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
>> +    /*
>> +     * The original r_offset is the w_offset of the ETR buffer at the
>> +     * start of the byte-cntr.
>> +     */
>> +    byte_cntr_data->r_offset = tmc_get_rwp_offset(tmcdrvdata);
>> +    byte_cntr_data->total_size = 0;
>> +    byte_cntr_data->read_active = true;
>> +    byte_cntr_data->enable = true;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct file_operations byte_cntr_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .open        = ctcu_byte_cntr_open,
>> +    .read        = ctcu_byte_cntr_read_etr_bytes,
>> +    .release    = ctcu_byte_cntr_release,
>> +};
>> +
>> +static int ctcu_byte_cntr_register_chardev(struct ctcu_byte_cntr 
>> *byte_cntr_data,
>> +                       int port_num)
>> +{
>> +    struct device *device;
>> +    dev_t devt;
>> +    int ret;
>> +
>> +    cdev_init(&byte_cntr_data->c_dev, &byte_cntr_fops);
>> +    devt = MKDEV(MAJOR(byte_cntr_base), MINOR(byte_cntr_base) + 
>> port_num);
>> +    ret = cdev_add(&byte_cntr_data->c_dev, devt, 1);
>> +    if (ret < 0)
>> +        return -ENOMEM;
>> +
>> +    device = device_create(byte_cntr_class, NULL, devt, byte_cntr_data,
>> +                   byte_cntr_data->name);
>> +
>> +    if (IS_ERR(device))
>> +        return -ENOMEM;
>> +
>> +    return 0;
>> +}
>> +
>> +static void ctcu_byte_cntr_unregister_chardev(struct ctcu_drvdata 
>> *drvdata)
>> +{
>> +    struct ctcu_byte_cntr *byte_cntr_data;
>> +    int i;
>> +
>> +    for (i = 0; i < ETR_MAX_NUM; i++) {
>> +        byte_cntr_data = &drvdata->byte_cntr_data[i];
>> +        device_destroy(byte_cntr_class, byte_cntr_data->c_dev.dev);
>> +    }
>> +}
>> +
>> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata 
>> *drvdata, int etr_num)
>> +{
>> +    struct ctcu_byte_cntr *byte_cntr_data;
>> +    struct device_node *nd = dev->of_node;
>> +    int byte_cntr_irq, ret, i;
>> +
>> +    ret = alloc_chrdev_region(&byte_cntr_base, 0, ETR_MAX_NUM, 
>> BYTE_CNTR_CLASS_STR);
>> +    if (ret < 0)
>> +        return -ENOMEM;
>> +
>> +    byte_cntr_class = class_create(BYTE_CNTR_CLASS_STR);
>> +    if (IS_ERR(byte_cntr_class)) {
>> +        unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    for (i = 0; i < etr_num; i++) {
>> +        byte_cntr_data = &drvdata->byte_cntr_data[i];
>> +        byte_cntr_irq = of_irq_get_byname(nd, byte_cntr_data->irq_name);
>> +        if (byte_cntr_irq < 0) {
>> +            ret = byte_cntr_irq;
>> +            goto err_exit;
>> +        }
>> +
>> +        ret = devm_request_irq(dev, byte_cntr_irq, byte_cntr_handler,
>> +                       IRQF_TRIGGER_RISING | IRQF_SHARED,
>> +                       dev_name(dev), byte_cntr_data);
>> +        if (ret) {
>> +            dev_err(dev, "Failed to register IRQ for %s\n",
>> +                byte_cntr_data->name);
>> +            goto err_exit;
>> +        }
>> +
>> +        ret = ctcu_byte_cntr_register_chardev(byte_cntr_data, i);
>> +        if (ret) {
>> +            dev_err(dev, "Failed to register chardev for %s\n",
>> +                byte_cntr_data->name);
>> +            goto err_exit;
>> +        }
>> +
>> +        byte_cntr_data->byte_cntr_irq = byte_cntr_irq;
>> +        atomic_set(&byte_cntr_data->irq_cnt, 0);
>> +        init_waitqueue_head(&byte_cntr_data->wq);
>> +    }
>> +
>> +    return 0;
>> +
>> +err_exit:
>> +    ctcu_byte_cntr_unregister_chardev(drvdata);
>> +    class_destroy(byte_cntr_class);
>> +    unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>> +    return ret;
>> +}
>> +
>> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata)
>> +{
>> +    ctcu_byte_cntr_unregister_chardev(drvdata);
>> +    class_destroy(byte_cntr_class);
>> +    unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>> +}
>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/ 
>> drivers/hwtracing/coresight/coresight-ctcu-core.c
>> index da35d8b4d579..5782655a5f39 100644
>> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
>> @@ -46,16 +46,24 @@ DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
>>   #define CTCU_ATID_REG_BIT(traceid)    (traceid % 32)
>>   #define CTCU_ATID_REG_SIZE        0x10
>>   #define CTCU_ETR0_ATID0            0xf8
>> +#define CTCU_ETR0_IRQCTRL        0x6c
>>   #define CTCU_ETR1_ATID0            0x108
>> +#define CTCU_ETR1_IRQCTRL        0x70
>>   static const struct ctcu_etr_config sa8775p_etr_cfgs[] = {
>>       {
>> -        .atid_offset    = CTCU_ETR0_ATID0,
>> -        .port_num    = 0,
>> +        .atid_offset        = CTCU_ETR0_ATID0,
>> +        .irq_ctrl_offset    = CTCU_ETR0_IRQCTRL,
>> +        .irq_name        = "etr0",
>> +        .byte_cntr_name        = "byte-cntr0",
>> +        .port_num        = 0,
>>       },
>>       {
>> -        .atid_offset    = CTCU_ETR1_ATID0,
>> -        .port_num    = 1,
>> +        .atid_offset        = CTCU_ETR1_ATID0,
>> +        .irq_ctrl_offset    = CTCU_ETR1_IRQCTRL,
>> +        .irq_name        = "etr1",
>> +        .byte_cntr_name        = "byte-cntr1",
>> +        .port_num        = 1,
>>       },
>>   };
>> @@ -64,6 +72,69 @@ static const struct ctcu_config sa8775p_cfgs = {
>>       .num_etr_config    = ARRAY_SIZE(sa8775p_etr_cfgs),
>>   };
>> +static ssize_t byte_cntr_val_show(struct device *dev, struct 
>> device_attribute *attr,
>> +                  char *buf)
>> +{
>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    int i, len = 0;
>> +
>> +    for (i = 0; i < ETR_MAX_NUM; i++) {
>> +        if (drvdata->byte_cntr_data[i].irq_ctrl_offset)
>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "%u ",
>> +                     drvdata->byte_cntr_data[i].thresh_val);
>> +    }
>> +
>> +    len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
>> +
>> +    return len;
>> +}
>> +
>> +static ssize_t byte_cntr_val_store(struct device *dev, struct 
>> device_attribute *attr,
>> +                   const char *buf, size_t size)
>> +{
>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    u32 thresh_vals[ETR_MAX_NUM] = { 0 };
>> +    u32 irq_ctrl_offset;
>> +    int num, i;
>> +
>> +    num = sscanf(buf, "%i %i", &thresh_vals[0], &thresh_vals[1]);
>> +    if (num <= 0 || num > ETR_MAX_NUM)
>> +        return -EINVAL;
>> +
>> +    /* Threshold 0 disables the interruption. */
>> +    guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
>> +    for (i = 0; i < num; i++) {
>> +        /* A small threshold will result in a large number of 
>> interruptions */
>> +        if (thresh_vals[i] && thresh_vals[i] < 4096)
>> +            return -EINVAL;
>> +
>> +        if (drvdata->byte_cntr_data[i].irq_ctrl_offset) {
>> +            drvdata->byte_cntr_data[i].thresh_val = thresh_vals[i];
>> +            irq_ctrl_offset = drvdata- 
>> >byte_cntr_data[i].irq_ctrl_offset;
>> +            CS_UNLOCK(drvdata->base);
>> +            writel_relaxed(thresh_vals[i], drvdata->base + 
>> irq_ctrl_offset);
>> +            CS_LOCK(drvdata->base);
>> +        }
>> +    }
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(byte_cntr_val);
>> +
>> +static struct attribute *ctcu_attrs[] = {
>> +    &dev_attr_byte_cntr_val.attr,
>> +    NULL,
>> +};
>> +
>> +static struct attribute_group ctcu_attr_grp = {
>> +    .attrs = ctcu_attrs,
>> +};
>> +
>> +static const struct attribute_group *ctcu_attr_grps[] = {
>> +    &ctcu_attr_grp,
>> +    NULL,
>> +};
>> +
>>   static void ctcu_program_atid_register(struct ctcu_drvdata *drvdata, 
>> u32 reg_offset,
>>                          u8 bit, bool enable)
>>   {
>> @@ -122,7 +193,7 @@ static int __ctcu_set_etr_traceid(struct 
>> coresight_device *csdev, u8 traceid, in
>>    * Searching the sink device from helper's view in case there are 
>> multiple helper devices
>>    * connected to the sink device.
>>    */
>> -static int ctcu_get_active_port(struct coresight_device *sink, struct 
>> coresight_device *helper)
>> +int ctcu_get_active_port(struct coresight_device *sink, struct 
>> coresight_device *helper)
>>   {
>>       struct coresight_platform_data *pdata = helper->pdata;
>>       int i;
>> @@ -160,6 +231,8 @@ static int ctcu_enable(struct coresight_device 
>> *csdev, enum cs_mode mode, void *
>>   {
>>       struct coresight_path *path = (struct coresight_path *)data;
>> +    ctcu_byte_cntr_start(csdev, path);
>> +
>>       return ctcu_set_etr_traceid(csdev, path, true);
>>   }
>> @@ -167,6 +240,8 @@ static int ctcu_disable(struct coresight_device 
>> *csdev, void *data)
>>   {
>>       struct coresight_path *path = (struct coresight_path *)data;
>> +    ctcu_byte_cntr_stop(csdev, path);
>> +
>>       return ctcu_set_etr_traceid(csdev, path, false);
>>   }
>> @@ -188,7 +263,7 @@ static int ctcu_probe(struct platform_device *pdev)
>>       const struct ctcu_config *cfgs;
>>       struct ctcu_drvdata *drvdata;
>>       void __iomem *base;
>> -    int i;
>> +    int ret, i;
>>       desc.name = coresight_alloc_device_name(&ctcu_devs, dev);
>>       if (!desc.name)
>> @@ -217,7 +292,14 @@ static int ctcu_probe(struct platform_device *pdev)
>>               for (i = 0; i < cfgs->num_etr_config; i++) {
>>                   etr_cfg = &cfgs->etr_cfgs[i];
>>                   drvdata->atid_offset[i] = etr_cfg->atid_offset;
>> +                drvdata->byte_cntr_data[i].irq_name = etr_cfg->irq_name;
>> +                drvdata->byte_cntr_data[i].name = etr_cfg- 
>> >byte_cntr_name;
>> +                drvdata->byte_cntr_data[i].irq_ctrl_offset =
>> +                    etr_cfg->irq_ctrl_offset;
>>               }
>> +            ret = ctcu_byte_cntr_init(dev, drvdata, cfgs- 
>> >num_etr_config);
>> +            if (ret < 0)
>> +                dev_warn(dev, "Byte cntr init failed\n");
>>           }
>>       }
>> @@ -229,6 +311,7 @@ static int ctcu_probe(struct platform_device *pdev)
>>       desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU;
>>       desc.pdata = pdata;
>>       desc.dev = dev;
>> +    desc.groups = ctcu_attr_grps;
>>       desc.ops = &ctcu_ops;
>>       desc.access = CSDEV_ACCESS_IOMEM(base);
>> @@ -247,6 +330,7 @@ static void ctcu_remove(struct platform_device *pdev)
>>   {
>>       struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
>> +    ctcu_byte_cntr_remove(drvdata);
>>       coresight_unregister(drvdata->csdev);
>>   }
>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/ 
>> hwtracing/coresight/coresight-ctcu.h
>> index e9594c38dd91..e38535c91090 100644
>> --- a/drivers/hwtracing/coresight/coresight-ctcu.h
>> +++ b/drivers/hwtracing/coresight/coresight-ctcu.h
>> @@ -5,6 +5,8 @@
>>   #ifndef _CORESIGHT_CTCU_H
>>   #define _CORESIGHT_CTCU_H
>> +#include <linux/cdev.h>
>> +
>>   #include "coresight-trace-id.h"
>>   /* Maximum number of supported ETR devices for a single CTCU. */
>> @@ -13,10 +15,16 @@
>>   /**
>>    * struct ctcu_etr_config
>>    * @atid_offset:    offset to the ATID0 Register.
>> - * @port_num:        in-port number of CTCU device that connected to 
>> ETR.
>> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL register.
>> + * @irq_name:        IRQ name in dt node.
>> + * @byte_cntr_name:    name of the byte cntr device node.
>> + * @port_num:        in-port number of the CTCU device that connected 
>> to ETR.
>>    */
>>   struct ctcu_etr_config {
>>       const u32 atid_offset;
>> +    const u32 irq_ctrl_offset;
>> +    const char *irq_name;
>> +    const char *byte_cntr_name;
>>       const u32 port_num;
>>   };
>> @@ -25,15 +33,64 @@ struct ctcu_config {
>>       int num_etr_config;
>>   };
>> +/**
>> + * struct ctcu_byte_cntr
>> + * @c_dev:        cdev for byte_cntr.
>> + * @sink        csdev of sink device.
>> + * @enable:        indicates that byte_cntr function is enabled or not.
>> + * @read_active:    indicates that byte-cntr node is opened or not.
>> + * @thresh_val:        threshold to trigger a interruption.
>> + * @total_size        total size of transferred data.
>> + * @byte_cntr_irq:    IRQ number.
>> + * @irq_cnt:        IRQ count.
>> + * @wq:            workqueue of reading ETR data.
>> + * @read_work:        work of reading ETR data.
>> + * @spin_lock:        spinlock of byte cntr data.
>> + * @r_offset:        offset of the pointer where reading begins.
>> + * @w_offset:        offset of the write pointer in the ETR buffer when
>> + *            the byte cntr is stopped.
>> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL Register.
>> + * @name:        the name of byte cntr device node.
>> + * @irq_name:        IRQ name in DT.
>> + */
>> +struct ctcu_byte_cntr {
>> +    struct cdev        c_dev;
>> +    struct coresight_device    *sink;
>> +    bool            enable;
>> +    bool            read_active;
>> +    u32            thresh_val;
>> +    u64            total_size;
>> +    int            byte_cntr_irq;
>> +    atomic_t        irq_cnt;
>> +    wait_queue_head_t    wq;
>> +    struct work_struct    read_work;
>> +    raw_spinlock_t        spin_lock;
>> +    long            r_offset;
>> +    long            w_offset;
>> +    u32            irq_ctrl_offset;
>> +    const char        *name;
>> +    const char        *irq_name;
>> +};
>> +
>>   struct ctcu_drvdata {
>>       void __iomem        *base;
>>       struct clk        *apb_clk;
>>       struct device        *dev;
>>       struct coresight_device    *csdev;
>> +    struct ctcu_byte_cntr   byte_cntr_data[ETR_MAX_NUM];
>>       raw_spinlock_t        spin_lock;
>>       u32            atid_offset[ETR_MAX_NUM];
>>       /* refcnt for each traceid of each sink */
>>       u8            traceid_refcnt[ETR_MAX_NUM] 
>> [CORESIGHT_TRACE_ID_RES_TOP];
>>   };
>> +/* Generic functions */
>> +int ctcu_get_active_port(struct coresight_device *sink, struct 
>> coresight_device *helper);
>> +
>> +/* Byte-cntr functions */
>> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct 
>> coresight_path *path);
>> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct 
>> coresight_path *path);
>> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata 
>> *drvdata, int port_num);
>> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata);
>> +
>>   #endif
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ 
>> drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index ec636ab1fd75..5dc94e890927 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -1040,14 +1040,15 @@ static void tmc_free_etr_buf(struct etr_buf 
>> *etr_buf)
>>    * Returns: The size of the linear data available @pos, with *bufpp
>>    * updated to point to the buffer.
>>    */
>> -static ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
>> -                    u64 offset, size_t len, char **bufpp)
>> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
>> +                 u64 offset, size_t len, char **bufpp)
>>   {
>>       /* Adjust the length to limit this transaction to end of buffer */
>>       len = (len < (etr_buf->size - offset)) ? len : etr_buf->size - 
>> offset;
>>       return etr_buf->ops->get_data(etr_buf, (u64)offset, len, bufpp);
>>   }
>> +EXPORT_SYMBOL_GPL(tmc_etr_buf_get_data);
>>   static inline s64
>>   tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 offset)
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/ 
>> hwtracing/coresight/coresight-tmc.h
>> index baedb4dcfc3f..2fc77fd4ea25 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>> @@ -443,5 +443,7 @@ struct etr_buf *tmc_etr_get_buffer(struct 
>> coresight_device *csdev,
>>                      enum cs_mode mode, void *data);
>>   extern const struct attribute_group coresight_etr_group;
>>   long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf, u64 offset, 
>> size_t len,
>> +                 char **bufpp);
>>   #endif
> 

Re: [PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices

[Suzuki K Poulose]

On 14/03/2025 01:59, Jie Gan wrote:
> 
> 
> On 3/14/2025 12:24 AM, Suzuki K Poulose wrote:
>> On 10/03/2025 09:04, Jie Gan wrote:
>>> The byte-cntr function provided by the CTCU device is used to 
>>> transfer data
>>> from the ETR buffer to the userspace. An interrupt is triggered if 
>>> the data
>>
>> Why do we need a new device to transfer the data to userspace ?
> 
> Hi Suzuki Mike,
> 
> The purpose of the byte-cntr is for live debugging, allowing trace data 
> to be read while the ETR is running and preventing the required trace 
> data from being overwritten before output.
> 
> I realized that I forgot to introduce the function of the BYTECNTRCTL 
> register.
> 
> There is a big concern that reading the etr_buf while the ETR is running 
> is unsafe because the etr_buf is not synced, am right?

Unsafe to read the buffer, written to by the ETR.

> 
> As I mentioned, we need program a value to the BYTECNTRCTL register. The 
> hardware will count the bytes of the trace data that are routed to 
> RAM(etr_buf).
> 
> The interrupt will be trigger once the counted bytes exceed the 
> programmed value. Based on this concept, we can confirm this part of 
> trace data is already in the etr_buf and is safe to read. So, it's safe 
> for the read function only read this part of trace data in one 
> shot(unlike tmc_read, the whole synced etr_buf).

Since the ETR is used in CIRCULAR buffer mode, what is the guarantee
that it is not overwritten while you are reading ?

> 
> That's how we expect the byte-cntr to work when reading the trace data 
> from the etr_buf while the etr is running.

We understood the functionality of byte-counter, but we think :

1. What you are doing is not safe. ETR can overwrite while you may be 
reading.
2. Adding another "character" file to expose the "same" data.

What we would like to see :

1. Solve the problem of ETR writing into the etr_buf, while you are
copying it. This could be via:

a. Stop the ETR while you are copying the buffer
  OR
b. Split the "buffer" available for ETR to two or more parts, limiting
    the ETR to only allowing writing to one part. On interrupt, disable
    the interrupt, and modify the ETR to use the next part. Now you can
    safely expose the "finished" part to the user.

2. Don't add another file. Use the existing one.

> 
> This also explains why we need a separate dev node to manage the read 
> function for byte-cntr.

No, you don't.


Suzuki


> 
> Thanks,
> Jie
> 
> 
>>
>>> size exceeds the threshold set in the BYTECNTRVAL register. The 
>>> interrupt
>>> handler counts the number of triggered interruptions and the read 
>>> function
>>> will read the data from the ETR buffer if the IRQ count is greater 
>>> than 0.
>>> Each successful read process will decrement the IRQ count by 1.
>>
>> Having an interrupt is good, but see below.
>>
>>>
>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/Makefile          |   2 +-
>>>   .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++++++
>>>   .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
>>>   drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
>>>   .../hwtracing/coresight/coresight-tmc-etr.c   |   5 +-
>>>   drivers/hwtracing/coresight/coresight-tmc.h   |   2 +
>>>   6 files changed, 493 insertions(+), 10 deletions(-)
>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu-byte- 
>>> cntr.c
>>>
>>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/ 
>>> hwtracing/ coresight/Makefile
>>> index 8e62c3150aeb..c90a06768a18 100644
>>> --- a/drivers/hwtracing/coresight/Makefile
>>> +++ b/drivers/hwtracing/coresight/Makefile
>>> @@ -52,4 +52,4 @@ coresight-cti-y := coresight-cti-core.o coresight- 
>>> cti-platform.o \
>>>   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>>>   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>>>   obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
>>> -coresight-ctcu-y := coresight-ctcu-core.o
>>> +coresight-ctcu-y := coresight-ctcu-core.o coresight-ctcu-byte-cntr.o
>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c 
>>> b/ drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>> new file mode 100644
>>> index 000000000000..0d16385663f5
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>> @@ -0,0 +1,339 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights 
>>> reserved.
>>> + */
>>> +
>>> +#include <linux/coresight.h>
>>> +#include <linux/device.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/property.h>
>>> +#include <linux/uaccess.h>
>>> +
>>> +#include "coresight-ctcu.h"
>>> +#include "coresight-priv.h"
>>> +#include "coresight-tmc.h"
>>> +
>>> +#define BYTE_CNTR_CLASS_STR "byte-cntr-class"
>>> +
>>> +static struct class *byte_cntr_class;
>>> +static dev_t byte_cntr_base;
>>> +
>>> +static irqreturn_t byte_cntr_handler(int irq, void *data)
>>> +{
>>> +    struct ctcu_byte_cntr *byte_cntr_data = (struct ctcu_byte_cntr 
>>> *)data;
>>> +
>>> +    atomic_inc(&byte_cntr_data->irq_cnt);
>>> +    wake_up(&byte_cntr_data->wq);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* Read the data from ETR's DDR buffer. */
>>> +static void __ctcu_byte_cntr_read_etr_bytes(struct ctcu_byte_cntr 
>>> *byte_cntr_data,
>>> +                        size_t *len, char **bufp)
>>> +{
>>> +    struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data- 
>>> >sink->dev.parent);
>>> +    size_t actual, bytes = byte_cntr_data->thresh_val;
>>> +    struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
>>> +    long r_offset;
>>> +
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    r_offset = byte_cntr_data->r_offset;
>>> +    if (*len >= bytes)
>>> +        *len = bytes;
>>> +    else if ((r_offset % bytes) + *len > bytes)
>>> +        *len = bytes - (r_offset % bytes);
>>> +
>>> +    actual = tmc_etr_buf_get_data(etr_buf, r_offset, *len, bufp);
>>> +    *len = actual;
>>> +    if (actual == bytes || (actual + r_offset) % bytes == 0)
>>> +        atomic_dec(&byte_cntr_data->irq_cnt);
>>> +}
>>> +
>>
>> How can you safely read the ETR data while it is running ? You should
>> stop the ETR and then provide the data. You could potentially do things
>> like split the ETR buffer into two halves and switch the half used by
>> the ETR on interrupt and expose this to user.
>>
>>
>>> +/* Flush the remaining data in the ETR buffer after the byte cntr 
>>> has stopped. */
>>> +static ssize_t ctcu_flush_etr_buffer(struct ctcu_byte_cntr 
>>> *byte_cntr_data, size_t len,
>>> +                     char **bufpp)
>>> +{
>>> +    struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data- 
>>> >sink->dev.parent);
>>> +    struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
>>> +    ssize_t read_len = 0, remaining_len;
>>> +    long r_offset, w_offset;
>>> +
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    r_offset = byte_cntr_data->r_offset;
>>> +    w_offset = byte_cntr_data->w_offset;
>>> +    if (w_offset < r_offset)
>>> +        remaining_len = tmcdrvdata->size + w_offset - r_offset;
>>> +    else
>>> +        remaining_len = w_offset - r_offset;
>>> +
>>> +    if (remaining_len > len)
>>> +        remaining_len = len;
>>> +
>>> +    if (remaining_len > 0)
>>> +        read_len = tmc_etr_buf_get_data(etr_buf, r_offset, 
>>> remaining_len, bufpp);
>>> +
>>> +    return read_len;
>>> +}
>>> +
>>> +static ssize_t ctcu_byte_cntr_copy_data(char __user *data, size_t 
>>> len, char *bufp,
>>> +                    struct ctcu_byte_cntr *byte_cntr_data,
>>> +                    struct tmc_drvdata *tmcdrvdata)
>>> +{
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    if (copy_to_user(data, bufp, len))
>>> +        return -EFAULT;
>>> +
>>> +    byte_cntr_data->total_size += len;
>>> +    if (byte_cntr_data->r_offset + len >= tmcdrvdata->size)
>>> +        byte_cntr_data->r_offset = 0;
>>> +    else
>>> +        byte_cntr_data->r_offset += len;
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +/* The read function for /dev/byte-cntr%d */
>>
>> WHY ? WHY can't the data be exported by the existing /dev/tmc_etr* ?
>>
>>> +static ssize_t ctcu_byte_cntr_read_etr_bytes(struct file *fp, char 
>>> __user *data,
>>> +                         size_t len, loff_t *ppos)
>>> +{
>>> +    struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
>>> +    struct tmc_drvdata *tmcdrvdata = dev_get_drvdata(byte_cntr_data- 
>>> >sink->dev.parent);
>>> +    char *bufp = NULL;
>>> +    ssize_t read_len;
>>> +
>>> +    if (!data)
>>> +        return -EINVAL;
>>> +
>>> +    /*
>>> +     * Flush the remaining data in the ETR buffer based on the write
>>> +     * offset of the ETR buffer when the byte cntr function has 
>>> stopped.
>>> +     */
>>> +    if (!byte_cntr_data->read_active || !byte_cntr_data->enable) {
>>> +        read_len = ctcu_flush_etr_buffer(byte_cntr_data, len, &bufp);
>>> +        if (read_len > 0)
>>> +            return ctcu_byte_cntr_copy_data(data, read_len, bufp,
>>> +                            byte_cntr_data, tmcdrvdata);
>>> +
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (!atomic_read(&byte_cntr_data->irq_cnt)) {
>>> +        if (wait_event_interruptible(byte_cntr_data->wq,
>>> +                         atomic_read(&byte_cntr_data->irq_cnt) > 0 ||
>>> +                         !byte_cntr_data->enable))
>>> +            return -ERESTARTSYS;
>>> +    }
>>> +
>>> +    __ctcu_byte_cntr_read_etr_bytes(byte_cntr_data, &len, &bufp);
>>> +
>>> +    return ctcu_byte_cntr_copy_data(data, len, bufp, byte_cntr_data, 
>>> tmcdrvdata);
>>> +}
>>
>> Could we not plug this into TMC-ETR read ?
>>
>> NAK for a special file approach. As Mike mentioned you need to make 
>> sure we are doing this safely.
>>
>> Suzuki
>>
>>
>>
>>> +
>>> +/* Start the byte-cntr function when the path is enabled. */
>>> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct 
>>> coresight_path *path)
>>> +{
>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +    struct coresight_device *sink = coresight_get_sink(path);
>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>> +    int port_num;
>>> +
>>> +    if (!sink)
>>> +        return;
>>> +
>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>> +    if (port_num < 0)
>>> +        return;
>>> +
>>> +    byte_cntr_data = &drvdata->byte_cntr_data[port_num];
>>> +    /* Don't start byte-cntr function when threshold is not set. */
>>> +    if (!byte_cntr_data->thresh_val)
>>> +        return;
>>> +
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>>> +    byte_cntr_data->sink = sink;
>>> +    byte_cntr_data->enable = true;
>>> +}
>>> +
>>> +/* Stop the byte-cntr function when the path is disabled. */
>>> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct 
>>> coresight_path *path)
>>> +{
>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +    struct coresight_device *sink = coresight_get_sink(path);
>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>> +    struct tmc_drvdata *tmcdrvdata;
>>> +    int port_num;
>>> +
>>> +    if (!sink)
>>> +        return;
>>> +
>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>> +    if (port_num < 0)
>>> +        return;
>>> +
>>> +    byte_cntr_data = &drvdata->byte_cntr_data[port_num];
>>> +    tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
>>> +
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    /* Store the w_offset of the ETR buffer when stopping. */
>>> +    byte_cntr_data->w_offset = tmc_get_rwp_offset(tmcdrvdata);
>>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>>> +    byte_cntr_data->read_active = false;
>>> +    byte_cntr_data->enable = false;
>>> +    /*
>>> +     * Wakeup once to force the read function to read the remaining
>>> +     * data of the ETR buffer.
>>> +     */
>>> +    wake_up(&byte_cntr_data->wq);
>>> +}
>>> +
>>> +static int ctcu_byte_cntr_release(struct inode *in, struct file *fp)
>>> +{
>>> +    struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
>>> +    struct device *dev = &byte_cntr_data->sink->dev;
>>> +
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>>> +    byte_cntr_data->read_active = false;
>>> +    disable_irq_wake(byte_cntr_data->byte_cntr_irq);
>>> +    dev_dbg(dev, "send data total size: %llu bytes, r_offset: %ld 
>>> w_offset: %ld\n",
>>> +        byte_cntr_data->total_size, byte_cntr_data->r_offset,
>>> +        byte_cntr_data->w_offset);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ctcu_byte_cntr_open(struct inode *in, struct file *fp)
>>> +{
>>> +    struct ctcu_byte_cntr *byte_cntr_data = container_of(in->i_cdev,
>>> +                                 struct ctcu_byte_cntr, c_dev);
>>> +    struct tmc_drvdata *tmcdrvdata;
>>> +
>>> +    if (byte_cntr_data->read_active)
>>> +        return -EBUSY;
>>> +
>>> +    if (!byte_cntr_data->thresh_val || !byte_cntr_data->sink ||
>>> +        (coresight_get_mode(byte_cntr_data->sink) !=  CS_MODE_SYSFS))
>>> +        return -EINVAL;
>>> +
>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>> +    enable_irq_wake(byte_cntr_data->byte_cntr_irq);
>>> +    fp->private_data = byte_cntr_data;
>>> +    nonseekable_open(in, fp);
>>> +    tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
>>> +    /*
>>> +     * The original r_offset is the w_offset of the ETR buffer at the
>>> +     * start of the byte-cntr.
>>> +     */
>>> +    byte_cntr_data->r_offset = tmc_get_rwp_offset(tmcdrvdata);
>>> +    byte_cntr_data->total_size = 0;
>>> +    byte_cntr_data->read_active = true;
>>> +    byte_cntr_data->enable = true;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct file_operations byte_cntr_fops = {
>>> +    .owner        = THIS_MODULE,
>>> +    .open        = ctcu_byte_cntr_open,
>>> +    .read        = ctcu_byte_cntr_read_etr_bytes,
>>> +    .release    = ctcu_byte_cntr_release,
>>> +};
>>> +
>>> +static int ctcu_byte_cntr_register_chardev(struct ctcu_byte_cntr 
>>> *byte_cntr_data,
>>> +                       int port_num)
>>> +{
>>> +    struct device *device;
>>> +    dev_t devt;
>>> +    int ret;
>>> +
>>> +    cdev_init(&byte_cntr_data->c_dev, &byte_cntr_fops);
>>> +    devt = MKDEV(MAJOR(byte_cntr_base), MINOR(byte_cntr_base) + 
>>> port_num);
>>> +    ret = cdev_add(&byte_cntr_data->c_dev, devt, 1);
>>> +    if (ret < 0)
>>> +        return -ENOMEM;
>>> +
>>> +    device = device_create(byte_cntr_class, NULL, devt, byte_cntr_data,
>>> +                   byte_cntr_data->name);
>>> +
>>> +    if (IS_ERR(device))
>>> +        return -ENOMEM;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void ctcu_byte_cntr_unregister_chardev(struct ctcu_drvdata 
>>> *drvdata)
>>> +{
>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ETR_MAX_NUM; i++) {
>>> +        byte_cntr_data = &drvdata->byte_cntr_data[i];
>>> +        device_destroy(byte_cntr_class, byte_cntr_data->c_dev.dev);
>>> +    }
>>> +}
>>> +
>>> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata 
>>> *drvdata, int etr_num)
>>> +{
>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>> +    struct device_node *nd = dev->of_node;
>>> +    int byte_cntr_irq, ret, i;
>>> +
>>> +    ret = alloc_chrdev_region(&byte_cntr_base, 0, ETR_MAX_NUM, 
>>> BYTE_CNTR_CLASS_STR);
>>> +    if (ret < 0)
>>> +        return -ENOMEM;
>>> +
>>> +    byte_cntr_class = class_create(BYTE_CNTR_CLASS_STR);
>>> +    if (IS_ERR(byte_cntr_class)) {
>>> +        unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    for (i = 0; i < etr_num; i++) {
>>> +        byte_cntr_data = &drvdata->byte_cntr_data[i];
>>> +        byte_cntr_irq = of_irq_get_byname(nd, byte_cntr_data- 
>>> >irq_name);
>>> +        if (byte_cntr_irq < 0) {
>>> +            ret = byte_cntr_irq;
>>> +            goto err_exit;
>>> +        }
>>> +
>>> +        ret = devm_request_irq(dev, byte_cntr_irq, byte_cntr_handler,
>>> +                       IRQF_TRIGGER_RISING | IRQF_SHARED,
>>> +                       dev_name(dev), byte_cntr_data);
>>> +        if (ret) {
>>> +            dev_err(dev, "Failed to register IRQ for %s\n",
>>> +                byte_cntr_data->name);
>>> +            goto err_exit;
>>> +        }
>>> +
>>> +        ret = ctcu_byte_cntr_register_chardev(byte_cntr_data, i);
>>> +        if (ret) {
>>> +            dev_err(dev, "Failed to register chardev for %s\n",
>>> +                byte_cntr_data->name);
>>> +            goto err_exit;
>>> +        }
>>> +
>>> +        byte_cntr_data->byte_cntr_irq = byte_cntr_irq;
>>> +        atomic_set(&byte_cntr_data->irq_cnt, 0);
>>> +        init_waitqueue_head(&byte_cntr_data->wq);
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err_exit:
>>> +    ctcu_byte_cntr_unregister_chardev(drvdata);
>>> +    class_destroy(byte_cntr_class);
>>> +    unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>>> +    return ret;
>>> +}
>>> +
>>> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata)
>>> +{
>>> +    ctcu_byte_cntr_unregister_chardev(drvdata);
>>> +    class_destroy(byte_cntr_class);
>>> +    unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>>> +}
>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/ 
>>> drivers/hwtracing/coresight/coresight-ctcu-core.c
>>> index da35d8b4d579..5782655a5f39 100644
>>> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
>>> @@ -46,16 +46,24 @@ DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
>>>   #define CTCU_ATID_REG_BIT(traceid)    (traceid % 32)
>>>   #define CTCU_ATID_REG_SIZE        0x10
>>>   #define CTCU_ETR0_ATID0            0xf8
>>> +#define CTCU_ETR0_IRQCTRL        0x6c
>>>   #define CTCU_ETR1_ATID0            0x108
>>> +#define CTCU_ETR1_IRQCTRL        0x70
>>>   static const struct ctcu_etr_config sa8775p_etr_cfgs[] = {
>>>       {
>>> -        .atid_offset    = CTCU_ETR0_ATID0,
>>> -        .port_num    = 0,
>>> +        .atid_offset        = CTCU_ETR0_ATID0,
>>> +        .irq_ctrl_offset    = CTCU_ETR0_IRQCTRL,
>>> +        .irq_name        = "etr0",
>>> +        .byte_cntr_name        = "byte-cntr0",
>>> +        .port_num        = 0,
>>>       },
>>>       {
>>> -        .atid_offset    = CTCU_ETR1_ATID0,
>>> -        .port_num    = 1,
>>> +        .atid_offset        = CTCU_ETR1_ATID0,
>>> +        .irq_ctrl_offset    = CTCU_ETR1_IRQCTRL,
>>> +        .irq_name        = "etr1",
>>> +        .byte_cntr_name        = "byte-cntr1",
>>> +        .port_num        = 1,
>>>       },
>>>   };
>>> @@ -64,6 +72,69 @@ static const struct ctcu_config sa8775p_cfgs = {
>>>       .num_etr_config    = ARRAY_SIZE(sa8775p_etr_cfgs),
>>>   };
>>> +static ssize_t byte_cntr_val_show(struct device *dev, struct 
>>> device_attribute *attr,
>>> +                  char *buf)
>>> +{
>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    int i, len = 0;
>>> +
>>> +    for (i = 0; i < ETR_MAX_NUM; i++) {
>>> +        if (drvdata->byte_cntr_data[i].irq_ctrl_offset)
>>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "%u ",
>>> +                     drvdata->byte_cntr_data[i].thresh_val);
>>> +    }
>>> +
>>> +    len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +static ssize_t byte_cntr_val_store(struct device *dev, struct 
>>> device_attribute *attr,
>>> +                   const char *buf, size_t size)
>>> +{
>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    u32 thresh_vals[ETR_MAX_NUM] = { 0 };
>>> +    u32 irq_ctrl_offset;
>>> +    int num, i;
>>> +
>>> +    num = sscanf(buf, "%i %i", &thresh_vals[0], &thresh_vals[1]);
>>> +    if (num <= 0 || num > ETR_MAX_NUM)
>>> +        return -EINVAL;
>>> +
>>> +    /* Threshold 0 disables the interruption. */
>>> +    guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
>>> +    for (i = 0; i < num; i++) {
>>> +        /* A small threshold will result in a large number of 
>>> interruptions */
>>> +        if (thresh_vals[i] && thresh_vals[i] < 4096)
>>> +            return -EINVAL;
>>> +
>>> +        if (drvdata->byte_cntr_data[i].irq_ctrl_offset) {
>>> +            drvdata->byte_cntr_data[i].thresh_val = thresh_vals[i];
>>> +            irq_ctrl_offset = drvdata- 
>>> >byte_cntr_data[i].irq_ctrl_offset;
>>> +            CS_UNLOCK(drvdata->base);
>>> +            writel_relaxed(thresh_vals[i], drvdata->base + 
>>> irq_ctrl_offset);
>>> +            CS_LOCK(drvdata->base);
>>> +        }
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(byte_cntr_val);
>>> +
>>> +static struct attribute *ctcu_attrs[] = {
>>> +    &dev_attr_byte_cntr_val.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static struct attribute_group ctcu_attr_grp = {
>>> +    .attrs = ctcu_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *ctcu_attr_grps[] = {
>>> +    &ctcu_attr_grp,
>>> +    NULL,
>>> +};
>>> +
>>>   static void ctcu_program_atid_register(struct ctcu_drvdata 
>>> *drvdata, u32 reg_offset,
>>>                          u8 bit, bool enable)
>>>   {
>>> @@ -122,7 +193,7 @@ static int __ctcu_set_etr_traceid(struct 
>>> coresight_device *csdev, u8 traceid, in
>>>    * Searching the sink device from helper's view in case there are 
>>> multiple helper devices
>>>    * connected to the sink device.
>>>    */
>>> -static int ctcu_get_active_port(struct coresight_device *sink, 
>>> struct coresight_device *helper)
>>> +int ctcu_get_active_port(struct coresight_device *sink, struct 
>>> coresight_device *helper)
>>>   {
>>>       struct coresight_platform_data *pdata = helper->pdata;
>>>       int i;
>>> @@ -160,6 +231,8 @@ static int ctcu_enable(struct coresight_device 
>>> *csdev, enum cs_mode mode, void *
>>>   {
>>>       struct coresight_path *path = (struct coresight_path *)data;
>>> +    ctcu_byte_cntr_start(csdev, path);
>>> +
>>>       return ctcu_set_etr_traceid(csdev, path, true);
>>>   }
>>> @@ -167,6 +240,8 @@ static int ctcu_disable(struct coresight_device 
>>> *csdev, void *data)
>>>   {
>>>       struct coresight_path *path = (struct coresight_path *)data;
>>> +    ctcu_byte_cntr_stop(csdev, path);
>>> +
>>>       return ctcu_set_etr_traceid(csdev, path, false);
>>>   }
>>> @@ -188,7 +263,7 @@ static int ctcu_probe(struct platform_device *pdev)
>>>       const struct ctcu_config *cfgs;
>>>       struct ctcu_drvdata *drvdata;
>>>       void __iomem *base;
>>> -    int i;
>>> +    int ret, i;
>>>       desc.name = coresight_alloc_device_name(&ctcu_devs, dev);
>>>       if (!desc.name)
>>> @@ -217,7 +292,14 @@ static int ctcu_probe(struct platform_device *pdev)
>>>               for (i = 0; i < cfgs->num_etr_config; i++) {
>>>                   etr_cfg = &cfgs->etr_cfgs[i];
>>>                   drvdata->atid_offset[i] = etr_cfg->atid_offset;
>>> +                drvdata->byte_cntr_data[i].irq_name = etr_cfg- 
>>> >irq_name;
>>> +                drvdata->byte_cntr_data[i].name = etr_cfg- 
>>> >byte_cntr_name;
>>> +                drvdata->byte_cntr_data[i].irq_ctrl_offset =
>>> +                    etr_cfg->irq_ctrl_offset;
>>>               }
>>> +            ret = ctcu_byte_cntr_init(dev, drvdata, cfgs- 
>>> >num_etr_config);
>>> +            if (ret < 0)
>>> +                dev_warn(dev, "Byte cntr init failed\n");
>>>           }
>>>       }
>>> @@ -229,6 +311,7 @@ static int ctcu_probe(struct platform_device *pdev)
>>>       desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU;
>>>       desc.pdata = pdata;
>>>       desc.dev = dev;
>>> +    desc.groups = ctcu_attr_grps;
>>>       desc.ops = &ctcu_ops;
>>>       desc.access = CSDEV_ACCESS_IOMEM(base);
>>> @@ -247,6 +330,7 @@ static void ctcu_remove(struct platform_device 
>>> *pdev)
>>>   {
>>>       struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
>>> +    ctcu_byte_cntr_remove(drvdata);
>>>       coresight_unregister(drvdata->csdev);
>>>   }
>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/ 
>>> hwtracing/coresight/coresight-ctcu.h
>>> index e9594c38dd91..e38535c91090 100644
>>> --- a/drivers/hwtracing/coresight/coresight-ctcu.h
>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu.h
>>> @@ -5,6 +5,8 @@
>>>   #ifndef _CORESIGHT_CTCU_H
>>>   #define _CORESIGHT_CTCU_H
>>> +#include <linux/cdev.h>
>>> +
>>>   #include "coresight-trace-id.h"
>>>   /* Maximum number of supported ETR devices for a single CTCU. */
>>> @@ -13,10 +15,16 @@
>>>   /**
>>>    * struct ctcu_etr_config
>>>    * @atid_offset:    offset to the ATID0 Register.
>>> - * @port_num:        in-port number of CTCU device that connected to 
>>> ETR.
>>> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL register.
>>> + * @irq_name:        IRQ name in dt node.
>>> + * @byte_cntr_name:    name of the byte cntr device node.
>>> + * @port_num:        in-port number of the CTCU device that 
>>> connected to ETR.
>>>    */
>>>   struct ctcu_etr_config {
>>>       const u32 atid_offset;
>>> +    const u32 irq_ctrl_offset;
>>> +    const char *irq_name;
>>> +    const char *byte_cntr_name;
>>>       const u32 port_num;
>>>   };
>>> @@ -25,15 +33,64 @@ struct ctcu_config {
>>>       int num_etr_config;
>>>   };
>>> +/**
>>> + * struct ctcu_byte_cntr
>>> + * @c_dev:        cdev for byte_cntr.
>>> + * @sink        csdev of sink device.
>>> + * @enable:        indicates that byte_cntr function is enabled or not.
>>> + * @read_active:    indicates that byte-cntr node is opened or not.
>>> + * @thresh_val:        threshold to trigger a interruption.
>>> + * @total_size        total size of transferred data.
>>> + * @byte_cntr_irq:    IRQ number.
>>> + * @irq_cnt:        IRQ count.
>>> + * @wq:            workqueue of reading ETR data.
>>> + * @read_work:        work of reading ETR data.
>>> + * @spin_lock:        spinlock of byte cntr data.
>>> + * @r_offset:        offset of the pointer where reading begins.
>>> + * @w_offset:        offset of the write pointer in the ETR buffer when
>>> + *            the byte cntr is stopped.
>>> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL Register.
>>> + * @name:        the name of byte cntr device node.
>>> + * @irq_name:        IRQ name in DT.
>>> + */
>>> +struct ctcu_byte_cntr {
>>> +    struct cdev        c_dev;
>>> +    struct coresight_device    *sink;
>>> +    bool            enable;
>>> +    bool            read_active;
>>> +    u32            thresh_val;
>>> +    u64            total_size;
>>> +    int            byte_cntr_irq;
>>> +    atomic_t        irq_cnt;
>>> +    wait_queue_head_t    wq;
>>> +    struct work_struct    read_work;
>>> +    raw_spinlock_t        spin_lock;
>>> +    long            r_offset;
>>> +    long            w_offset;
>>> +    u32            irq_ctrl_offset;
>>> +    const char        *name;
>>> +    const char        *irq_name;
>>> +};
>>> +
>>>   struct ctcu_drvdata {
>>>       void __iomem        *base;
>>>       struct clk        *apb_clk;
>>>       struct device        *dev;
>>>       struct coresight_device    *csdev;
>>> +    struct ctcu_byte_cntr   byte_cntr_data[ETR_MAX_NUM];
>>>       raw_spinlock_t        spin_lock;
>>>       u32            atid_offset[ETR_MAX_NUM];
>>>       /* refcnt for each traceid of each sink */
>>>       u8            traceid_refcnt[ETR_MAX_NUM] 
>>> [CORESIGHT_TRACE_ID_RES_TOP];
>>>   };
>>> +/* Generic functions */
>>> +int ctcu_get_active_port(struct coresight_device *sink, struct 
>>> coresight_device *helper);
>>> +
>>> +/* Byte-cntr functions */
>>> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct 
>>> coresight_path *path);
>>> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct 
>>> coresight_path *path);
>>> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata 
>>> *drvdata, int port_num);
>>> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata);
>>> +
>>>   #endif
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ 
>>> drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> index ec636ab1fd75..5dc94e890927 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> @@ -1040,14 +1040,15 @@ static void tmc_free_etr_buf(struct etr_buf 
>>> *etr_buf)
>>>    * Returns: The size of the linear data available @pos, with *bufpp
>>>    * updated to point to the buffer.
>>>    */
>>> -static ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
>>> -                    u64 offset, size_t len, char **bufpp)
>>> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
>>> +                 u64 offset, size_t len, char **bufpp)
>>>   {
>>>       /* Adjust the length to limit this transaction to end of buffer */
>>>       len = (len < (etr_buf->size - offset)) ? len : etr_buf->size - 
>>> offset;
>>>       return etr_buf->ops->get_data(etr_buf, (u64)offset, len, bufpp);
>>>   }
>>> +EXPORT_SYMBOL_GPL(tmc_etr_buf_get_data);
>>>   static inline s64
>>>   tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 offset)
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/ 
>>> hwtracing/coresight/coresight-tmc.h
>>> index baedb4dcfc3f..2fc77fd4ea25 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>> @@ -443,5 +443,7 @@ struct etr_buf *tmc_etr_get_buffer(struct 
>>> coresight_device *csdev,
>>>                      enum cs_mode mode, void *data);
>>>   extern const struct attribute_group coresight_etr_group;
>>>   long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>>> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf, u64 offset, 
>>> size_t len,
>>> +                 char **bufpp);
>>>   #endif
>>
> 

Re: [PATCH v1 3/4] coresight: ctcu: Enable byte-cntr for TMC ETR devices

[Jie Gan]

On 3/15/2025 1:33 AM, Suzuki K Poulose wrote:
> On 14/03/2025 01:59, Jie Gan wrote:
>>
>>
>> On 3/14/2025 12:24 AM, Suzuki K Poulose wrote:
>>> On 10/03/2025 09:04, Jie Gan wrote:
>>>> The byte-cntr function provided by the CTCU device is used to 
>>>> transfer data
>>>> from the ETR buffer to the userspace. An interrupt is triggered if 
>>>> the data
>>>
>>> Why do we need a new device to transfer the data to userspace ?
>>
>> Hi Suzuki Mike,
>>
>> The purpose of the byte-cntr is for live debugging, allowing trace 
>> data to be read while the ETR is running and preventing the required 
>> trace data from being overwritten before output.
>>
>> I realized that I forgot to introduce the function of the BYTECNTRCTL 
>> register.
>>
>> There is a big concern that reading the etr_buf while the ETR is 
>> running is unsafe because the etr_buf is not synced, am right?
> 
> Unsafe to read the buffer, written to by the ETR.

Hi Suzuki,

I have confirmed the behavior of BYTECNTRVAL register internally.

The portion of trace data is safe to read from the etr_buf when the 
interrupt is triggered.
For example, if the threshold for the BYTECNTRVAL register is 0x10000.
This portion of trace data(0x10000) is confirmed to be already in the 
buffer and ready to read when the interrupt is triggered.

I also did following verification about 30 times:

1. Configure the buffer size of the etr as large as possible to avoid 
overwrite for the experiment, e.g, I configured 0x1000000.

2. Read the trace data via byte-cntr with current solution from etr_buf 
and save it in a file: file_byte_cntr.bin.

3. Stop the trace(stop source and etr devices), read the trace data of 
the whole etr_buf, and save it in a file: file_etr.bin

-rw-r--r--    1 root     root      13749968 Apr 28 17:45 file_byte_cntr.bin
-rw-r--r--    1 root     root      13765328 Apr 28 17:46 file_etr.bin

The size of file_byte_cntr.bin should be less than file_etr.bin and the 
data in file_byte_cntr.bin should exactly match a part of the data in 
file_etr.bin continuously from a random point in the file_etr.bin to the 
end of the file.

The byte-cntr read starts at a random point of the tracing, so some of 
the data from the beginning is useless when we parse the data.

> 
>>
>> As I mentioned, we need program a value to the BYTECNTRCTL register. 
>> The hardware will count the bytes of the trace data that are routed to 
>> RAM(etr_buf).
>>
>> The interrupt will be trigger once the counted bytes exceed the 
>> programmed value. Based on this concept, we can confirm this part of 
>> trace data is already in the etr_buf and is safe to read. So, it's 
>> safe for the read function only read this part of trace data in one 
>> shot(unlike tmc_read, the whole synced etr_buf).
> 
> Since the ETR is used in CIRCULAR buffer mode, what is the guarantee
> that it is not overwritten while you are reading ?

We cannot 100% guarantee there is no overwrite happened while reading 
the buffer. But by using a large buffer_size and ensuring the threshold 
for the byte-cntr is less than the buffer_size, we can mitigate this 
issue(this behavior aims to enture that no overwrite occurs for the 
portion of data before the the read function completes.)

The byte-cntr should read the trace data from the etr_buf as soon as 
possible when the interrupt is triggered(via IRQ count and a workqueue).

> 
>>
>> That's how we expect the byte-cntr to work when reading the trace data 
>> from the etr_buf while the etr is running.
> 
> We understood the functionality of byte-counter, but we think :
> 
> 1. What you are doing is not safe. ETR can overwrite while you may be 
> reading.

I agree with you. We need to ensure that the solution is safe for 
reading the buffer.

> 2. Adding another "character" file to expose the "same" data.

Discarded the new "char" file. I integrated the read function of 
byte-cntr with current ETR's file_operation.

The new behavior likes:
1. when BYTECNTRCTL register is configured, the read behavior for 
tmc_read functions as byte-cntr behavior.

2. when BYTECNTRCTL register is disabled, the read behavior for tmc_read 
functions as its original behavior.

I will send this solution with V2 if it is acceptable.

> 
> What we would like to see :
> 
> 1. Solve the problem of ETR writing into the etr_buf, while you are
> copying it. This could be via:
> 
> a. Stop the ETR while you are copying the buffer
>   OR
> b. Split the "buffer" available for ETR to two or more parts, limiting
>     the ETR to only allowing writing to one part. On interrupt, disable
>     the interrupt, and modify the ETR to use the next part. Now you can
>     safely expose the "finished" part to the user.
> 
> 2. Don't add another file. Use the existing one.

Sure, please refer to my earlier comment in this message.

Thanks,
Jie

> 
>>
>> This also explains why we need a separate dev node to manage the read 
>> function for byte-cntr.
> 
> No, you don't.
> 
> 
> Suzuki
> 
> 
>>
>> Thanks,
>> Jie
>>
>>
>>>
>>>> size exceeds the threshold set in the BYTECNTRVAL register. The 
>>>> interrupt
>>>> handler counts the number of triggered interruptions and the read 
>>>> function
>>>> will read the data from the ETR buffer if the IRQ count is greater 
>>>> than 0.
>>>> Each successful read process will decrement the IRQ count by 1.
>>>
>>> Having an interrupt is good, but see below.
>>>
>>>>
>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/Makefile          |   2 +-
>>>>   .../coresight/coresight-ctcu-byte-cntr.c      | 339 ++++++++++++++ 
>>>> ++++
>>>>   .../hwtracing/coresight/coresight-ctcu-core.c |  96 ++++-
>>>>   drivers/hwtracing/coresight/coresight-ctcu.h  |  59 ++-
>>>>   .../hwtracing/coresight/coresight-tmc-etr.c   |   5 +-
>>>>   drivers/hwtracing/coresight/coresight-tmc.h   |   2 +
>>>>   6 files changed, 493 insertions(+), 10 deletions(-)
>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu- 
>>>> byte- cntr.c
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/ 
>>>> hwtracing/ coresight/Makefile
>>>> index 8e62c3150aeb..c90a06768a18 100644
>>>> --- a/drivers/hwtracing/coresight/Makefile
>>>> +++ b/drivers/hwtracing/coresight/Makefile
>>>> @@ -52,4 +52,4 @@ coresight-cti-y := coresight-cti-core.o coresight- 
>>>> cti-platform.o \
>>>>   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>>>>   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>>>>   obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
>>>> -coresight-ctcu-y := coresight-ctcu-core.o
>>>> +coresight-ctcu-y := coresight-ctcu-core.o coresight-ctcu-byte-cntr.o
>>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c 
>>>> b/ drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>>> new file mode 100644
>>>> index 000000000000..0d16385663f5
>>>> --- /dev/null
>>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>>> @@ -0,0 +1,339 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights 
>>>> reserved.
>>>> + */
>>>> +
>>>> +#include <linux/coresight.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/dma-mapping.h>
>>>> +#include <linux/fs.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/property.h>
>>>> +#include <linux/uaccess.h>
>>>> +
>>>> +#include "coresight-ctcu.h"
>>>> +#include "coresight-priv.h"
>>>> +#include "coresight-tmc.h"
>>>> +
>>>> +#define BYTE_CNTR_CLASS_STR "byte-cntr-class"
>>>> +
>>>> +static struct class *byte_cntr_class;
>>>> +static dev_t byte_cntr_base;
>>>> +
>>>> +static irqreturn_t byte_cntr_handler(int irq, void *data)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data = (struct ctcu_byte_cntr 
>>>> *)data;
>>>> +
>>>> +    atomic_inc(&byte_cntr_data->irq_cnt);
>>>> +    wake_up(&byte_cntr_data->wq);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +/* Read the data from ETR's DDR buffer. */
>>>> +static void __ctcu_byte_cntr_read_etr_bytes(struct ctcu_byte_cntr 
>>>> *byte_cntr_data,
>>>> +                        size_t *len, char **bufp)
>>>> +{
>>>> +    struct tmc_drvdata *tmcdrvdata = 
>>>> dev_get_drvdata(byte_cntr_data- >sink->dev.parent);
>>>> +    size_t actual, bytes = byte_cntr_data->thresh_val;
>>>> +    struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
>>>> +    long r_offset;
>>>> +
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    r_offset = byte_cntr_data->r_offset;
>>>> +    if (*len >= bytes)
>>>> +        *len = bytes;
>>>> +    else if ((r_offset % bytes) + *len > bytes)
>>>> +        *len = bytes - (r_offset % bytes);
>>>> +
>>>> +    actual = tmc_etr_buf_get_data(etr_buf, r_offset, *len, bufp);
>>>> +    *len = actual;
>>>> +    if (actual == bytes || (actual + r_offset) % bytes == 0)
>>>> +        atomic_dec(&byte_cntr_data->irq_cnt);
>>>> +}
>>>> +
>>>
>>> How can you safely read the ETR data while it is running ? You should
>>> stop the ETR and then provide the data. You could potentially do things
>>> like split the ETR buffer into two halves and switch the half used by
>>> the ETR on interrupt and expose this to user.
>>>
>>>
>>>> +/* Flush the remaining data in the ETR buffer after the byte cntr 
>>>> has stopped. */
>>>> +static ssize_t ctcu_flush_etr_buffer(struct ctcu_byte_cntr 
>>>> *byte_cntr_data, size_t len,
>>>> +                     char **bufpp)
>>>> +{
>>>> +    struct tmc_drvdata *tmcdrvdata = 
>>>> dev_get_drvdata(byte_cntr_data- >sink->dev.parent);
>>>> +    struct etr_buf *etr_buf = tmcdrvdata->sysfs_buf;
>>>> +    ssize_t read_len = 0, remaining_len;
>>>> +    long r_offset, w_offset;
>>>> +
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    r_offset = byte_cntr_data->r_offset;
>>>> +    w_offset = byte_cntr_data->w_offset;
>>>> +    if (w_offset < r_offset)
>>>> +        remaining_len = tmcdrvdata->size + w_offset - r_offset;
>>>> +    else
>>>> +        remaining_len = w_offset - r_offset;
>>>> +
>>>> +    if (remaining_len > len)
>>>> +        remaining_len = len;
>>>> +
>>>> +    if (remaining_len > 0)
>>>> +        read_len = tmc_etr_buf_get_data(etr_buf, r_offset, 
>>>> remaining_len, bufpp);
>>>> +
>>>> +    return read_len;
>>>> +}
>>>> +
>>>> +static ssize_t ctcu_byte_cntr_copy_data(char __user *data, size_t 
>>>> len, char *bufp,
>>>> +                    struct ctcu_byte_cntr *byte_cntr_data,
>>>> +                    struct tmc_drvdata *tmcdrvdata)
>>>> +{
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    if (copy_to_user(data, bufp, len))
>>>> +        return -EFAULT;
>>>> +
>>>> +    byte_cntr_data->total_size += len;
>>>> +    if (byte_cntr_data->r_offset + len >= tmcdrvdata->size)
>>>> +        byte_cntr_data->r_offset = 0;
>>>> +    else
>>>> +        byte_cntr_data->r_offset += len;
>>>> +
>>>> +    return len;
>>>> +}
>>>> +
>>>> +/* The read function for /dev/byte-cntr%d */
>>>
>>> WHY ? WHY can't the data be exported by the existing /dev/tmc_etr* ?
>>>
>>>> +static ssize_t ctcu_byte_cntr_read_etr_bytes(struct file *fp, char 
>>>> __user *data,
>>>> +                         size_t len, loff_t *ppos)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
>>>> +    struct tmc_drvdata *tmcdrvdata = 
>>>> dev_get_drvdata(byte_cntr_data- >sink->dev.parent);
>>>> +    char *bufp = NULL;
>>>> +    ssize_t read_len;
>>>> +
>>>> +    if (!data)
>>>> +        return -EINVAL;
>>>> +
>>>> +    /*
>>>> +     * Flush the remaining data in the ETR buffer based on the write
>>>> +     * offset of the ETR buffer when the byte cntr function has 
>>>> stopped.
>>>> +     */
>>>> +    if (!byte_cntr_data->read_active || !byte_cntr_data->enable) {
>>>> +        read_len = ctcu_flush_etr_buffer(byte_cntr_data, len, &bufp);
>>>> +        if (read_len > 0)
>>>> +            return ctcu_byte_cntr_copy_data(data, read_len, bufp,
>>>> +                            byte_cntr_data, tmcdrvdata);
>>>> +
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (!atomic_read(&byte_cntr_data->irq_cnt)) {
>>>> +        if (wait_event_interruptible(byte_cntr_data->wq,
>>>> +                         atomic_read(&byte_cntr_data->irq_cnt) > 0 ||
>>>> +                         !byte_cntr_data->enable))
>>>> +            return -ERESTARTSYS;
>>>> +    }
>>>> +
>>>> +    __ctcu_byte_cntr_read_etr_bytes(byte_cntr_data, &len, &bufp);
>>>> +
>>>> +    return ctcu_byte_cntr_copy_data(data, len, bufp, 
>>>> byte_cntr_data, tmcdrvdata);
>>>> +}
>>>
>>> Could we not plug this into TMC-ETR read ?
>>>
>>> NAK for a special file approach. As Mike mentioned you need to make 
>>> sure we are doing this safely.
>>>
>>> Suzuki
>>>
>>>
>>>
>>>> +
>>>> +/* Start the byte-cntr function when the path is enabled. */
>>>> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct 
>>>> coresight_path *path)
>>>> +{
>>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>>> +    struct coresight_device *sink = coresight_get_sink(path);
>>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>>> +    int port_num;
>>>> +
>>>> +    if (!sink)
>>>> +        return;
>>>> +
>>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>>> +    if (port_num < 0)
>>>> +        return;
>>>> +
>>>> +    byte_cntr_data = &drvdata->byte_cntr_data[port_num];
>>>> +    /* Don't start byte-cntr function when threshold is not set. */
>>>> +    if (!byte_cntr_data->thresh_val)
>>>> +        return;
>>>> +
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>>>> +    byte_cntr_data->sink = sink;
>>>> +    byte_cntr_data->enable = true;
>>>> +}
>>>> +
>>>> +/* Stop the byte-cntr function when the path is disabled. */
>>>> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct 
>>>> coresight_path *path)
>>>> +{
>>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>>> +    struct coresight_device *sink = coresight_get_sink(path);
>>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>>> +    struct tmc_drvdata *tmcdrvdata;
>>>> +    int port_num;
>>>> +
>>>> +    if (!sink)
>>>> +        return;
>>>> +
>>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>>> +    if (port_num < 0)
>>>> +        return;
>>>> +
>>>> +    byte_cntr_data = &drvdata->byte_cntr_data[port_num];
>>>> +    tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
>>>> +
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    /* Store the w_offset of the ETR buffer when stopping. */
>>>> +    byte_cntr_data->w_offset = tmc_get_rwp_offset(tmcdrvdata);
>>>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>>>> +    byte_cntr_data->read_active = false;
>>>> +    byte_cntr_data->enable = false;
>>>> +    /*
>>>> +     * Wakeup once to force the read function to read the remaining
>>>> +     * data of the ETR buffer.
>>>> +     */
>>>> +    wake_up(&byte_cntr_data->wq);
>>>> +}
>>>> +
>>>> +static int ctcu_byte_cntr_release(struct inode *in, struct file *fp)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data = fp->private_data;
>>>> +    struct device *dev = &byte_cntr_data->sink->dev;
>>>> +
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    atomic_set(&byte_cntr_data->irq_cnt, 0);
>>>> +    byte_cntr_data->read_active = false;
>>>> +    disable_irq_wake(byte_cntr_data->byte_cntr_irq);
>>>> +    dev_dbg(dev, "send data total size: %llu bytes, r_offset: %ld 
>>>> w_offset: %ld\n",
>>>> +        byte_cntr_data->total_size, byte_cntr_data->r_offset,
>>>> +        byte_cntr_data->w_offset);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ctcu_byte_cntr_open(struct inode *in, struct file *fp)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data = container_of(in->i_cdev,
>>>> +                                 struct ctcu_byte_cntr, c_dev);
>>>> +    struct tmc_drvdata *tmcdrvdata;
>>>> +
>>>> +    if (byte_cntr_data->read_active)
>>>> +        return -EBUSY;
>>>> +
>>>> +    if (!byte_cntr_data->thresh_val || !byte_cntr_data->sink ||
>>>> +        (coresight_get_mode(byte_cntr_data->sink) !=  CS_MODE_SYSFS))
>>>> +        return -EINVAL;
>>>> +
>>>> +    guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
>>>> +    enable_irq_wake(byte_cntr_data->byte_cntr_irq);
>>>> +    fp->private_data = byte_cntr_data;
>>>> +    nonseekable_open(in, fp);
>>>> +    tmcdrvdata = dev_get_drvdata(byte_cntr_data->sink->dev.parent);
>>>> +    /*
>>>> +     * The original r_offset is the w_offset of the ETR buffer at the
>>>> +     * start of the byte-cntr.
>>>> +     */
>>>> +    byte_cntr_data->r_offset = tmc_get_rwp_offset(tmcdrvdata);
>>>> +    byte_cntr_data->total_size = 0;
>>>> +    byte_cntr_data->read_active = true;
>>>> +    byte_cntr_data->enable = true;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct file_operations byte_cntr_fops = {
>>>> +    .owner        = THIS_MODULE,
>>>> +    .open        = ctcu_byte_cntr_open,
>>>> +    .read        = ctcu_byte_cntr_read_etr_bytes,
>>>> +    .release    = ctcu_byte_cntr_release,
>>>> +};
>>>> +
>>>> +static int ctcu_byte_cntr_register_chardev(struct ctcu_byte_cntr 
>>>> *byte_cntr_data,
>>>> +                       int port_num)
>>>> +{
>>>> +    struct device *device;
>>>> +    dev_t devt;
>>>> +    int ret;
>>>> +
>>>> +    cdev_init(&byte_cntr_data->c_dev, &byte_cntr_fops);
>>>> +    devt = MKDEV(MAJOR(byte_cntr_base), MINOR(byte_cntr_base) + 
>>>> port_num);
>>>> +    ret = cdev_add(&byte_cntr_data->c_dev, devt, 1);
>>>> +    if (ret < 0)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    device = device_create(byte_cntr_class, NULL, devt, 
>>>> byte_cntr_data,
>>>> +                   byte_cntr_data->name);
>>>> +
>>>> +    if (IS_ERR(device))
>>>> +        return -ENOMEM;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void ctcu_byte_cntr_unregister_chardev(struct ctcu_drvdata 
>>>> *drvdata)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ETR_MAX_NUM; i++) {
>>>> +        byte_cntr_data = &drvdata->byte_cntr_data[i];
>>>> +        device_destroy(byte_cntr_class, byte_cntr_data->c_dev.dev);
>>>> +    }
>>>> +}
>>>> +
>>>> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata 
>>>> *drvdata, int etr_num)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>>> +    struct device_node *nd = dev->of_node;
>>>> +    int byte_cntr_irq, ret, i;
>>>> +
>>>> +    ret = alloc_chrdev_region(&byte_cntr_base, 0, ETR_MAX_NUM, 
>>>> BYTE_CNTR_CLASS_STR);
>>>> +    if (ret < 0)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    byte_cntr_class = class_create(BYTE_CNTR_CLASS_STR);
>>>> +    if (IS_ERR(byte_cntr_class)) {
>>>> +        unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < etr_num; i++) {
>>>> +        byte_cntr_data = &drvdata->byte_cntr_data[i];
>>>> +        byte_cntr_irq = of_irq_get_byname(nd, byte_cntr_data- 
>>>> >irq_name);
>>>> +        if (byte_cntr_irq < 0) {
>>>> +            ret = byte_cntr_irq;
>>>> +            goto err_exit;
>>>> +        }
>>>> +
>>>> +        ret = devm_request_irq(dev, byte_cntr_irq, byte_cntr_handler,
>>>> +                       IRQF_TRIGGER_RISING | IRQF_SHARED,
>>>> +                       dev_name(dev), byte_cntr_data);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "Failed to register IRQ for %s\n",
>>>> +                byte_cntr_data->name);
>>>> +            goto err_exit;
>>>> +        }
>>>> +
>>>> +        ret = ctcu_byte_cntr_register_chardev(byte_cntr_data, i);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "Failed to register chardev for %s\n",
>>>> +                byte_cntr_data->name);
>>>> +            goto err_exit;
>>>> +        }
>>>> +
>>>> +        byte_cntr_data->byte_cntr_irq = byte_cntr_irq;
>>>> +        atomic_set(&byte_cntr_data->irq_cnt, 0);
>>>> +        init_waitqueue_head(&byte_cntr_data->wq);
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_exit:
>>>> +    ctcu_byte_cntr_unregister_chardev(drvdata);
>>>> +    class_destroy(byte_cntr_class);
>>>> +    unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata)
>>>> +{
>>>> +    ctcu_byte_cntr_unregister_chardev(drvdata);
>>>> +    class_destroy(byte_cntr_class);
>>>> +    unregister_chrdev_region(byte_cntr_base, ETR_MAX_NUM);
>>>> +}
>>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/ 
>>>> drivers/hwtracing/coresight/coresight-ctcu-core.c
>>>> index da35d8b4d579..5782655a5f39 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
>>>> @@ -46,16 +46,24 @@ DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
>>>>   #define CTCU_ATID_REG_BIT(traceid)    (traceid % 32)
>>>>   #define CTCU_ATID_REG_SIZE        0x10
>>>>   #define CTCU_ETR0_ATID0            0xf8
>>>> +#define CTCU_ETR0_IRQCTRL        0x6c
>>>>   #define CTCU_ETR1_ATID0            0x108
>>>> +#define CTCU_ETR1_IRQCTRL        0x70
>>>>   static const struct ctcu_etr_config sa8775p_etr_cfgs[] = {
>>>>       {
>>>> -        .atid_offset    = CTCU_ETR0_ATID0,
>>>> -        .port_num    = 0,
>>>> +        .atid_offset        = CTCU_ETR0_ATID0,
>>>> +        .irq_ctrl_offset    = CTCU_ETR0_IRQCTRL,
>>>> +        .irq_name        = "etr0",
>>>> +        .byte_cntr_name        = "byte-cntr0",
>>>> +        .port_num        = 0,
>>>>       },
>>>>       {
>>>> -        .atid_offset    = CTCU_ETR1_ATID0,
>>>> -        .port_num    = 1,
>>>> +        .atid_offset        = CTCU_ETR1_ATID0,
>>>> +        .irq_ctrl_offset    = CTCU_ETR1_IRQCTRL,
>>>> +        .irq_name        = "etr1",
>>>> +        .byte_cntr_name        = "byte-cntr1",
>>>> +        .port_num        = 1,
>>>>       },
>>>>   };
>>>> @@ -64,6 +72,69 @@ static const struct ctcu_config sa8775p_cfgs = {
>>>>       .num_etr_config    = ARRAY_SIZE(sa8775p_etr_cfgs),
>>>>   };
>>>> +static ssize_t byte_cntr_val_show(struct device *dev, struct 
>>>> device_attribute *attr,
>>>> +                  char *buf)
>>>> +{
>>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    int i, len = 0;
>>>> +
>>>> +    for (i = 0; i < ETR_MAX_NUM; i++) {
>>>> +        if (drvdata->byte_cntr_data[i].irq_ctrl_offset)
>>>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "%u ",
>>>> +                     drvdata->byte_cntr_data[i].thresh_val);
>>>> +    }
>>>> +
>>>> +    len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
>>>> +
>>>> +    return len;
>>>> +}
>>>> +
>>>> +static ssize_t byte_cntr_val_store(struct device *dev, struct 
>>>> device_attribute *attr,
>>>> +                   const char *buf, size_t size)
>>>> +{
>>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    u32 thresh_vals[ETR_MAX_NUM] = { 0 };
>>>> +    u32 irq_ctrl_offset;
>>>> +    int num, i;
>>>> +
>>>> +    num = sscanf(buf, "%i %i", &thresh_vals[0], &thresh_vals[1]);
>>>> +    if (num <= 0 || num > ETR_MAX_NUM)
>>>> +        return -EINVAL;
>>>> +
>>>> +    /* Threshold 0 disables the interruption. */
>>>> +    guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
>>>> +    for (i = 0; i < num; i++) {
>>>> +        /* A small threshold will result in a large number of 
>>>> interruptions */
>>>> +        if (thresh_vals[i] && thresh_vals[i] < 4096)
>>>> +            return -EINVAL;
>>>> +
>>>> +        if (drvdata->byte_cntr_data[i].irq_ctrl_offset) {
>>>> +            drvdata->byte_cntr_data[i].thresh_val = thresh_vals[i];
>>>> +            irq_ctrl_offset = drvdata- 
>>>> >byte_cntr_data[i].irq_ctrl_offset;
>>>> +            CS_UNLOCK(drvdata->base);
>>>> +            writel_relaxed(thresh_vals[i], drvdata->base + 
>>>> irq_ctrl_offset);
>>>> +            CS_LOCK(drvdata->base);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(byte_cntr_val);
>>>> +
>>>> +static struct attribute *ctcu_attrs[] = {
>>>> +    &dev_attr_byte_cntr_val.attr,
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +static struct attribute_group ctcu_attr_grp = {
>>>> +    .attrs = ctcu_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group *ctcu_attr_grps[] = {
>>>> +    &ctcu_attr_grp,
>>>> +    NULL,
>>>> +};
>>>> +
>>>>   static void ctcu_program_atid_register(struct ctcu_drvdata 
>>>> *drvdata, u32 reg_offset,
>>>>                          u8 bit, bool enable)
>>>>   {
>>>> @@ -122,7 +193,7 @@ static int __ctcu_set_etr_traceid(struct 
>>>> coresight_device *csdev, u8 traceid, in
>>>>    * Searching the sink device from helper's view in case there are 
>>>> multiple helper devices
>>>>    * connected to the sink device.
>>>>    */
>>>> -static int ctcu_get_active_port(struct coresight_device *sink, 
>>>> struct coresight_device *helper)
>>>> +int ctcu_get_active_port(struct coresight_device *sink, struct 
>>>> coresight_device *helper)
>>>>   {
>>>>       struct coresight_platform_data *pdata = helper->pdata;
>>>>       int i;
>>>> @@ -160,6 +231,8 @@ static int ctcu_enable(struct coresight_device 
>>>> *csdev, enum cs_mode mode, void *
>>>>   {
>>>>       struct coresight_path *path = (struct coresight_path *)data;
>>>> +    ctcu_byte_cntr_start(csdev, path);
>>>> +
>>>>       return ctcu_set_etr_traceid(csdev, path, true);
>>>>   }
>>>> @@ -167,6 +240,8 @@ static int ctcu_disable(struct coresight_device 
>>>> *csdev, void *data)
>>>>   {
>>>>       struct coresight_path *path = (struct coresight_path *)data;
>>>> +    ctcu_byte_cntr_stop(csdev, path);
>>>> +
>>>>       return ctcu_set_etr_traceid(csdev, path, false);
>>>>   }
>>>> @@ -188,7 +263,7 @@ static int ctcu_probe(struct platform_device *pdev)
>>>>       const struct ctcu_config *cfgs;
>>>>       struct ctcu_drvdata *drvdata;
>>>>       void __iomem *base;
>>>> -    int i;
>>>> +    int ret, i;
>>>>       desc.name = coresight_alloc_device_name(&ctcu_devs, dev);
>>>>       if (!desc.name)
>>>> @@ -217,7 +292,14 @@ static int ctcu_probe(struct platform_device 
>>>> *pdev)
>>>>               for (i = 0; i < cfgs->num_etr_config; i++) {
>>>>                   etr_cfg = &cfgs->etr_cfgs[i];
>>>>                   drvdata->atid_offset[i] = etr_cfg->atid_offset;
>>>> +                drvdata->byte_cntr_data[i].irq_name = etr_cfg- 
>>>> >irq_name;
>>>> +                drvdata->byte_cntr_data[i].name = etr_cfg- 
>>>> >byte_cntr_name;
>>>> +                drvdata->byte_cntr_data[i].irq_ctrl_offset =
>>>> +                    etr_cfg->irq_ctrl_offset;
>>>>               }
>>>> +            ret = ctcu_byte_cntr_init(dev, drvdata, cfgs- 
>>>> >num_etr_config);
>>>> +            if (ret < 0)
>>>> +                dev_warn(dev, "Byte cntr init failed\n");
>>>>           }
>>>>       }
>>>> @@ -229,6 +311,7 @@ static int ctcu_probe(struct platform_device *pdev)
>>>>       desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU;
>>>>       desc.pdata = pdata;
>>>>       desc.dev = dev;
>>>> +    desc.groups = ctcu_attr_grps;
>>>>       desc.ops = &ctcu_ops;
>>>>       desc.access = CSDEV_ACCESS_IOMEM(base);
>>>> @@ -247,6 +330,7 @@ static void ctcu_remove(struct platform_device 
>>>> *pdev)
>>>>   {
>>>>       struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
>>>> +    ctcu_byte_cntr_remove(drvdata);
>>>>       coresight_unregister(drvdata->csdev);
>>>>   }
>>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/ 
>>>> hwtracing/coresight/coresight-ctcu.h
>>>> index e9594c38dd91..e38535c91090 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-ctcu.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu.h
>>>> @@ -5,6 +5,8 @@
>>>>   #ifndef _CORESIGHT_CTCU_H
>>>>   #define _CORESIGHT_CTCU_H
>>>> +#include <linux/cdev.h>
>>>> +
>>>>   #include "coresight-trace-id.h"
>>>>   /* Maximum number of supported ETR devices for a single CTCU. */
>>>> @@ -13,10 +15,16 @@
>>>>   /**
>>>>    * struct ctcu_etr_config
>>>>    * @atid_offset:    offset to the ATID0 Register.
>>>> - * @port_num:        in-port number of CTCU device that connected 
>>>> to ETR.
>>>> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL register.
>>>> + * @irq_name:        IRQ name in dt node.
>>>> + * @byte_cntr_name:    name of the byte cntr device node.
>>>> + * @port_num:        in-port number of the CTCU device that 
>>>> connected to ETR.
>>>>    */
>>>>   struct ctcu_etr_config {
>>>>       const u32 atid_offset;
>>>> +    const u32 irq_ctrl_offset;
>>>> +    const char *irq_name;
>>>> +    const char *byte_cntr_name;
>>>>       const u32 port_num;
>>>>   };
>>>> @@ -25,15 +33,64 @@ struct ctcu_config {
>>>>       int num_etr_config;
>>>>   };
>>>> +/**
>>>> + * struct ctcu_byte_cntr
>>>> + * @c_dev:        cdev for byte_cntr.
>>>> + * @sink        csdev of sink device.
>>>> + * @enable:        indicates that byte_cntr function is enabled or 
>>>> not.
>>>> + * @read_active:    indicates that byte-cntr node is opened or not.
>>>> + * @thresh_val:        threshold to trigger a interruption.
>>>> + * @total_size        total size of transferred data.
>>>> + * @byte_cntr_irq:    IRQ number.
>>>> + * @irq_cnt:        IRQ count.
>>>> + * @wq:            workqueue of reading ETR data.
>>>> + * @read_work:        work of reading ETR data.
>>>> + * @spin_lock:        spinlock of byte cntr data.
>>>> + * @r_offset:        offset of the pointer where reading begins.
>>>> + * @w_offset:        offset of the write pointer in the ETR buffer 
>>>> when
>>>> + *            the byte cntr is stopped.
>>>> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL Register.
>>>> + * @name:        the name of byte cntr device node.
>>>> + * @irq_name:        IRQ name in DT.
>>>> + */
>>>> +struct ctcu_byte_cntr {
>>>> +    struct cdev        c_dev;
>>>> +    struct coresight_device    *sink;
>>>> +    bool            enable;
>>>> +    bool            read_active;
>>>> +    u32            thresh_val;
>>>> +    u64            total_size;
>>>> +    int            byte_cntr_irq;
>>>> +    atomic_t        irq_cnt;
>>>> +    wait_queue_head_t    wq;
>>>> +    struct work_struct    read_work;
>>>> +    raw_spinlock_t        spin_lock;
>>>> +    long            r_offset;
>>>> +    long            w_offset;
>>>> +    u32            irq_ctrl_offset;
>>>> +    const char        *name;
>>>> +    const char        *irq_name;
>>>> +};
>>>> +
>>>>   struct ctcu_drvdata {
>>>>       void __iomem        *base;
>>>>       struct clk        *apb_clk;
>>>>       struct device        *dev;
>>>>       struct coresight_device    *csdev;
>>>> +    struct ctcu_byte_cntr   byte_cntr_data[ETR_MAX_NUM];
>>>>       raw_spinlock_t        spin_lock;
>>>>       u32            atid_offset[ETR_MAX_NUM];
>>>>       /* refcnt for each traceid of each sink */
>>>>       u8            traceid_refcnt[ETR_MAX_NUM] 
>>>> [CORESIGHT_TRACE_ID_RES_TOP];
>>>>   };
>>>> +/* Generic functions */
>>>> +int ctcu_get_active_port(struct coresight_device *sink, struct 
>>>> coresight_device *helper);
>>>> +
>>>> +/* Byte-cntr functions */
>>>> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct 
>>>> coresight_path *path);
>>>> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct 
>>>> coresight_path *path);
>>>> +int ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata 
>>>> *drvdata, int port_num);
>>>> +void ctcu_byte_cntr_remove(struct ctcu_drvdata *drvdata);
>>>> +
>>>>   #endif
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ 
>>>> drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>> index ec636ab1fd75..5dc94e890927 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>> @@ -1040,14 +1040,15 @@ static void tmc_free_etr_buf(struct etr_buf 
>>>> *etr_buf)
>>>>    * Returns: The size of the linear data available @pos, with *bufpp
>>>>    * updated to point to the buffer.
>>>>    */
>>>> -static ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
>>>> -                    u64 offset, size_t len, char **bufpp)
>>>> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf,
>>>> +                 u64 offset, size_t len, char **bufpp)
>>>>   {
>>>>       /* Adjust the length to limit this transaction to end of 
>>>> buffer */
>>>>       len = (len < (etr_buf->size - offset)) ? len : etr_buf->size - 
>>>> offset;
>>>>       return etr_buf->ops->get_data(etr_buf, (u64)offset, len, bufpp);
>>>>   }
>>>> +EXPORT_SYMBOL_GPL(tmc_etr_buf_get_data);
>>>>   static inline s64
>>>>   tmc_etr_buf_insert_barrier_packet(struct etr_buf *etr_buf, u64 
>>>> offset)
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/ 
>>>> hwtracing/coresight/coresight-tmc.h
>>>> index baedb4dcfc3f..2fc77fd4ea25 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>>> @@ -443,5 +443,7 @@ struct etr_buf *tmc_etr_get_buffer(struct 
>>>> coresight_device *csdev,
>>>>                      enum cs_mode mode, void *data);
>>>>   extern const struct attribute_group coresight_etr_group;
>>>>   long tmc_get_rwp_offset(struct tmc_drvdata *drvdata);
>>>> +ssize_t tmc_etr_buf_get_data(struct etr_buf *etr_buf, u64 offset, 
>>>> size_t len,
>>>> +                 char **bufpp);
>>>>   #endif
>>>
>>
>