[PATCH v1 0/3] add sysfs nodes to configure TPDA's registers

[PATCH v1 0/3] add sysfs nodes to configure TPDA's registers

[Jie Gan]

Patchset 1 introduces configuration of the cross-trigger registers with
appropriate values to enable proper generation of cross-trigger packets.

Patchset 2 introduces a function to configure the TPDA_SYNCR register,
which determines the frequency of ASYNC packet generation. These packets
assist userspace tools in accurately identifying each valid packet.

Patchset 3 introduces a sysfs node to initiate a flush request for the
specific port, forcing the data to synchronize and be transmitted to the
sink device.

Tao Zhang (3):
  coresight: tpda: Add sysfs node for tpda cross-trigger configuration
  coresight: tpda: add function to configure TPDA_SYNCR register
  coresight: tpda: add sysfs node to flush specific port

 .../testing/sysfs-bus-coresight-devices-tpda  |  50 +++
 drivers/hwtracing/coresight/coresight-tpda.c  | 301 ++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpda.h  |  29 ++
 3 files changed, 380 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda

-- 
2.34.1

[PATCH v1 1/3] coresight: tpda: Add sysfs node for tpda cross-trigger configuration

[Jie Gan]

From: Tao Zhang <tao.zhang@oss.qualcomm.com>

Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
These registers define the characteristics of cross-trigger packets,
including generation frequency and flag values.

Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
---
 .../testing/sysfs-bus-coresight-devices-tpda  |  43 ++++
 drivers/hwtracing/coresight/coresight-tpda.c  | 241 ++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpda.h  |  27 ++
 3 files changed, 311 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
new file mode 100644
index 000000000000..e827396a0fa1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
@@ -0,0 +1,43 @@
+What:		/sys/bus/coresight/devices/<tpda-name>/trig_async_enable
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Enable/disable cross trigger synchronization sequence interface.
+
+What:		/sys/bus/coresight/devices/<tpda-name>/trig_flag_ts_enable
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Enable/disable cross trigger FLAG packet request interface.
+
+What:		/sys/bus/coresight/devices/<tpda-name>/trig_freq_enable
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Enable/disable cross trigger FREQ packet request interface.
+
+What:		/sys/bus/coresight/devices/<tpda-name>/freq_ts_enable
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Enable/disable the timestamp for all FREQ packets.
+
+What:		/sys/bus/coresight/devices/<tpda-name>/global_flush_req
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Set/unset global (all ports) flush request bit. The bit remains set until a
+		global flush request sequence completes.
+
+What:		/sys/bus/coresight/devices/<tpda-name>/cmbchan_mode
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Configure the CMB/MCMB channel mode for all enabled ports.
+		Value 0 means raw channel mapping mode. Value 1 means channel pair marking mode.
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 4e93fa5bace4..cc254d53b8ec 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -156,9 +156,37 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
 	u32 val;
 
 	val = readl_relaxed(drvdata->base + TPDA_CR);
+	val &= ~TPDA_CR_MID;
 	val &= ~TPDA_CR_ATID;
 	val |= FIELD_PREP(TPDA_CR_ATID, drvdata->atid);
+	if (drvdata->trig_async)
+		val = val | TPDA_CR_SRIE;
+	else
+		val = val & ~TPDA_CR_SRIE;
+	if (drvdata->trig_flag_ts)
+		val = val | TPDA_CR_FLRIE;
+	else
+		val = val & ~TPDA_CR_FLRIE;
+	if (drvdata->trig_freq)
+		val = val | TPDA_CR_FRIE;
+	else
+		val = val & ~TPDA_CR_FRIE;
+	if (drvdata->freq_ts)
+		val = val | TPDA_CR_FREQTS;
+	else
+		val = val & ~TPDA_CR_FREQTS;
+	if (drvdata->cmbchan_mode)
+		val = val | TPDA_CR_CMBCHANMODE;
+	else
+		val = val & ~TPDA_CR_CMBCHANMODE;
 	writel_relaxed(val, drvdata->base + TPDA_CR);
+
+	/*
+	 * If FLRIE bit is set, set the master and channel
+	 * id as zero
+	 */
+	if (drvdata->trig_flag_ts)
+		writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
 }
 
 static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
@@ -274,6 +302,217 @@ static const struct coresight_ops tpda_cs_ops = {
 	.link_ops	= &tpda_link_ops,
 };
 
+static ssize_t trig_async_enable_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_async);
+}
+
+static ssize_t trig_async_enable_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf,
+				       size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (val)
+		drvdata->trig_async = true;
+	else
+		drvdata->trig_async = false;
+
+	return size;
+}
+static DEVICE_ATTR_RW(trig_async_enable);
+
+static ssize_t trig_flag_ts_enable_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_flag_ts);
+}
+
+static ssize_t trig_flag_ts_enable_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf,
+					 size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (val)
+		drvdata->trig_flag_ts = true;
+	else
+		drvdata->trig_flag_ts = false;
+
+	return size;
+}
+static DEVICE_ATTR_RW(trig_flag_ts_enable);
+
+static ssize_t trig_freq_enable_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_freq);
+}
+
+static ssize_t trig_freq_enable_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf,
+				      size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (val)
+		drvdata->trig_freq = true;
+	else
+		drvdata->trig_freq = false;
+
+	return size;
+}
+static DEVICE_ATTR_RW(trig_freq_enable);
+
+static ssize_t freq_ts_enable_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->freq_ts);
+}
+
+static ssize_t freq_ts_enable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf,
+				    size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (val)
+		drvdata->freq_ts = true;
+	else
+		drvdata->freq_ts = false;
+
+	return size;
+}
+static DEVICE_ATTR_RW(freq_ts_enable);
+
+static ssize_t global_flush_req_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (!drvdata->csdev->refcnt)
+		return -EPERM;
+
+	val = readl_relaxed(drvdata->base + TPDA_CR);
+	return sysfs_emit(buf, "%lx\n", val);
+}
+
+static ssize_t global_flush_req_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf,
+				      size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (!drvdata->csdev->refcnt)
+		return -EPERM;
+
+	if (val) {
+		CS_UNLOCK(drvdata->base);
+		val = readl_relaxed(drvdata->base + TPDA_CR);
+		val = val | BIT(0);
+		writel_relaxed(val, drvdata->base + TPDA_CR);
+		CS_LOCK(drvdata->base);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(global_flush_req);
+
+static ssize_t cmbchan_mode_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->cmbchan_mode);
+}
+
+static ssize_t cmbchan_mode_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	bool val;
+
+	if (kstrtobool(buf, &val))
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (val)
+		drvdata->cmbchan_mode = true;
+	else
+		drvdata->cmbchan_mode = false;
+
+	return size;
+}
+static DEVICE_ATTR_RW(cmbchan_mode);
+
+static struct attribute *tpda_attrs[] = {
+	&dev_attr_trig_async_enable.attr,
+	&dev_attr_trig_flag_ts_enable.attr,
+	&dev_attr_trig_freq_enable.attr,
+	&dev_attr_freq_ts_enable.attr,
+	&dev_attr_global_flush_req.attr,
+	&dev_attr_cmbchan_mode.attr,
+	NULL,
+};
+
+static struct attribute_group tpda_attr_grp = {
+	.attrs = tpda_attrs,
+};
+
+static const struct attribute_group *tpda_attr_grps[] = {
+	&tpda_attr_grp,
+	NULL,
+};
+
 static int tpda_init_default_data(struct tpda_drvdata *drvdata)
 {
 	int atid;
@@ -289,6 +528,7 @@ static int tpda_init_default_data(struct tpda_drvdata *drvdata)
 		return atid;
 
 	drvdata->atid = atid;
+	drvdata->freq_ts = true;
 	return 0;
 }
 
@@ -332,6 +572,7 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id)
 	desc.ops = &tpda_cs_ops;
 	desc.pdata = adev->dev.platform_data;
 	desc.dev = &adev->dev;
+	desc.groups = tpda_attr_grps;
 	desc.access = CSDEV_ACCESS_IOMEM(base);
 	drvdata->csdev = coresight_register(&desc);
 	if (IS_ERR(drvdata->csdev))
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index c6af3d2da3ef..b651372d4c88 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.h
+++ b/drivers/hwtracing/coresight/coresight-tpda.h
@@ -8,17 +8,34 @@
 
 #define TPDA_CR			(0x000)
 #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
+#define TPDA_FPID_CR		(0x084)
+
+/* Cross trigger FREQ packets timestamp bit */
+#define TPDA_CR_FREQTS		BIT(2)
+/* Cross trigger FREQ packet request bit */
+#define TPDA_CR_FRIE		BIT(3)
+/* Cross trigger FLAG packet request interface bit */
+#define TPDA_CR_FLRIE		BIT(4)
+/* Cross trigger synchronization bit */
+#define TPDA_CR_SRIE		BIT(5)
+/* Packetize CMB/MCMB traffic bit */
+#define TPDA_CR_CMBCHANMODE	BIT(20)
+
 /* Aggregator port enable bit */
 #define TPDA_Pn_CR_ENA		BIT(0)
 /* Aggregator port CMB data set element size bit */
 #define TPDA_Pn_CR_CMBSIZE		GENMASK(7, 6)
 /* Aggregator port DSB data set element size bit */
 #define TPDA_Pn_CR_DSBSIZE		BIT(8)
+/* Mode control bit */
+#define TPDA_MODE_CTRL			BIT(12)
 
 #define TPDA_MAX_INPORTS	32
 
 /* Bits 6 ~ 12 is for atid value */
 #define TPDA_CR_ATID		GENMASK(12, 6)
+/* Bits 13 ~ 19 is for mid value */
+#define TPDA_CR_MID		GENMASK(19, 13)
 
 /**
  * struct tpda_drvdata - specifics associated to an TPDA component
@@ -29,6 +46,11 @@
  * @enable:     enable status of the component.
  * @dsb_esize   Record the DSB element size.
  * @cmb_esize   Record the CMB element size.
+ * @trig_async:	Enable/disable cross trigger synchronization sequence interface.
+ * @trig_flag_ts: Enable/disable cross trigger FLAG packet request interface.
+ * @trig_freq:	Enable/disable cross trigger FREQ packet request interface.
+ * @freq_ts:	Enable/disable the timestamp for all FREQ packets.
+ * @cmbchan_mode: Configure the CMB/MCMB channel mode.
  */
 struct tpda_drvdata {
 	void __iomem		*base;
@@ -38,6 +60,11 @@ struct tpda_drvdata {
 	u8			atid;
 	u32			dsb_esize;
 	u32			cmb_esize;
+	bool			trig_async;
+	bool			trig_flag_ts;
+	bool			trig_freq;
+	bool			freq_ts;
+	bool			cmbchan_mode;
 };
 
 #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
-- 
2.34.1

[PATCH v1 2/3] coresight: tpda: add function to configure TPDA_SYNCR register

[Jie Gan]

From: Tao Zhang <tao.zhang@oss.qualcomm.com>

The TPDA_SYNCR register defines the frequency at which TPDA generates
ASYNC packets, enabling userspace tools to accurately parse each valid
packet.

Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
---
 drivers/hwtracing/coresight/coresight-tpda.c | 15 +++++++++++++++
 drivers/hwtracing/coresight/coresight-tpda.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index cc254d53b8ec..9e623732d1e7 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -189,6 +189,18 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
 		writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
 }
 
+static void tpda_enable_post_port(struct tpda_drvdata *drvdata)
+{
+	uint32_t val;
+
+	val = readl_relaxed(drvdata->base + TPDA_SYNCR);
+	/* Clear the mode */
+	val = val & ~TPDA_MODE_CTRL;
+	/* Program the counter value */
+	val = val | 0xFFF;
+	writel_relaxed(val, drvdata->base + TPDA_SYNCR);
+}
+
 static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
 {
 	u32 val;
@@ -227,6 +239,9 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
 		tpda_enable_pre_port(drvdata);
 
 	ret = tpda_enable_port(drvdata, port);
+	if (!drvdata->csdev->refcnt)
+		tpda_enable_post_port(drvdata);
+
 	CS_LOCK(drvdata->base);
 
 	return ret;
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index b651372d4c88..00d146960d81 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.h
+++ b/drivers/hwtracing/coresight/coresight-tpda.h
@@ -9,6 +9,7 @@
 #define TPDA_CR			(0x000)
 #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
 #define TPDA_FPID_CR		(0x084)
+#define TPDA_SYNCR		(0x08C)
 
 /* Cross trigger FREQ packets timestamp bit */
 #define TPDA_CR_FREQTS		BIT(2)
-- 
2.34.1

[PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[Jie Gan]

From: Tao Zhang <tao.zhang@oss.qualcomm.com>

Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
for port i, forcing the data to synchronize and be transmitted to the
sink device.

Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
---
 .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
 drivers/hwtracing/coresight/coresight-tpda.c  | 45 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
 3 files changed, 53 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
index e827396a0fa1..8803158ba42f 100644
--- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
@@ -41,3 +41,10 @@ Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
 Description:
 		(RW) Configure the CMB/MCMB channel mode for all enabled ports.
 		Value 0 means raw channel mapping mode. Value 1 means channel pair marking mode.
+
+What:		/sys/bus/coresight/devices/<tpda-name>/port_flush_req
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
+Description:
+		(RW) Configure the bit i to requests a flush operation of port i on the TPDA.
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 9e623732d1e7..c5f169facc51 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(cmbchan_mode);
 
+static ssize_t port_flush_req_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (!drvdata->csdev->refcnt)
+		return -EPERM;
+
+	val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
+	return sysfs_emit(buf, "%lx\n", val);
+}
+
+static ssize_t port_flush_req_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf,
+				    size_t size)
+{
+	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	/* The valid value ranges from 0 to 127 */
+	if (val > 127)
+		return -EINVAL;
+
+	guard(spinlock)(&drvdata->spinlock);
+	if (!drvdata->csdev->refcnt)
+		return -EPERM;
+
+	if (val) {
+		CS_UNLOCK(drvdata->base);
+		writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
+		CS_LOCK(drvdata->base);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(port_flush_req);
+
 static struct attribute *tpda_attrs[] = {
 	&dev_attr_trig_async_enable.attr,
 	&dev_attr_trig_flag_ts_enable.attr,
@@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
 	&dev_attr_freq_ts_enable.attr,
 	&dev_attr_global_flush_req.attr,
 	&dev_attr_cmbchan_mode.attr,
+	&dev_attr_port_flush_req.attr,
 	NULL,
 };
 
diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
index 00d146960d81..55a18d718357 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.h
+++ b/drivers/hwtracing/coresight/coresight-tpda.h
@@ -10,6 +10,7 @@
 #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
 #define TPDA_FPID_CR		(0x084)
 #define TPDA_SYNCR		(0x08C)
+#define TPDA_FLUSH_CR		(0x090)
 
 /* Cross trigger FREQ packets timestamp bit */
 #define TPDA_CR_FREQTS		BIT(2)
-- 
2.34.1

Re: [PATCH v1 1/3] coresight: tpda: Add sysfs node for tpda cross-trigger configuration

[James Clark]

On 26/08/2025 8:01 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
> 
> Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
> These registers define the characteristics of cross-trigger packets,
> including generation frequency and flag values.
> 
> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
> ---
>   .../testing/sysfs-bus-coresight-devices-tpda  |  43 ++++
>   drivers/hwtracing/coresight/coresight-tpda.c  | 241 ++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpda.h  |  27 ++
>   3 files changed, 311 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> new file mode 100644
> index 000000000000..e827396a0fa1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> @@ -0,0 +1,43 @@
> +What:		/sys/bus/coresight/devices/<tpda-name>/trig_async_enable
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Enable/disable cross trigger synchronization sequence interface.
> +
> +What:		/sys/bus/coresight/devices/<tpda-name>/trig_flag_ts_enable
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Enable/disable cross trigger FLAG packet request interface.
> +
> +What:		/sys/bus/coresight/devices/<tpda-name>/trig_freq_enable
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Enable/disable cross trigger FREQ packet request interface.
> +
> +What:		/sys/bus/coresight/devices/<tpda-name>/freq_ts_enable
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Enable/disable the timestamp for all FREQ packets.
> +
> +What:		/sys/bus/coresight/devices/<tpda-name>/global_flush_req
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Set/unset global (all ports) flush request bit. The bit remains set until a

I don't think you can unset? global_flush_req_store() only does 
something for set.

> +		global flush request sequence completes.
> +
> +What:		/sys/bus/coresight/devices/<tpda-name>/cmbchan_mode
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Configure the CMB/MCMB channel mode for all enabled ports.
> +		Value 0 means raw channel mapping mode. Value 1 means channel pair marking mode.
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 4e93fa5bace4..cc254d53b8ec 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -156,9 +156,37 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   	u32 val;
>   
>   	val = readl_relaxed(drvdata->base + TPDA_CR);
> +	val &= ~TPDA_CR_MID;
>   	val &= ~TPDA_CR_ATID;
>   	val |= FIELD_PREP(TPDA_CR_ATID, drvdata->atid);
> +	if (drvdata->trig_async)
> +		val = val | TPDA_CR_SRIE;
> +	else
> +		val = val & ~TPDA_CR_SRIE;

val |=
val &=

> +	if (drvdata->trig_flag_ts)
> +		val = val | TPDA_CR_FLRIE;
> +	else
> +		val = val & ~TPDA_CR_FLRIE;
> +	if (drvdata->trig_freq)
> +		val = val | TPDA_CR_FRIE;
> +	else
> +		val = val & ~TPDA_CR_FRIE;
> +	if (drvdata->freq_ts)
> +		val = val | TPDA_CR_FREQTS;
> +	else
> +		val = val & ~TPDA_CR_FREQTS;
> +	if (drvdata->cmbchan_mode)
> +		val = val | TPDA_CR_CMBCHANMODE;
> +	else
> +		val = val & ~TPDA_CR_CMBCHANMODE;
>   	writel_relaxed(val, drvdata->base + TPDA_CR);
> +
> +	/*
> +	 * If FLRIE bit is set, set the master and channel
> +	 * id as zero
> +	 */
> +	if (drvdata->trig_flag_ts)
> +		writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>   }
>   
>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
> @@ -274,6 +302,217 @@ static const struct coresight_ops tpda_cs_ops = {
>   	.link_ops	= &tpda_link_ops,
>   };
>   
> +static ssize_t trig_async_enable_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_async);
> +}
> +
> +static ssize_t trig_async_enable_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf,
> +				       size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (val)
> +		drvdata->trig_async = true;
> +	else
> +		drvdata->trig_async = false;
> +

drvdata->trig_async = !!val

same with all the following ones too

> +	return size;
> +}
> +static DEVICE_ATTR_RW(trig_async_enable);
> +
> +static ssize_t trig_flag_ts_enable_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_flag_ts);
> +}
> +
> +static ssize_t trig_flag_ts_enable_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf,
> +					 size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (val)
> +		drvdata->trig_flag_ts = true;
> +	else
> +		drvdata->trig_flag_ts = false;
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(trig_flag_ts_enable);
> +
> +static ssize_t trig_freq_enable_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_freq);
> +}
> +
> +static ssize_t trig_freq_enable_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf,
> +				      size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (val)
> +		drvdata->trig_freq = true;
> +	else
> +		drvdata->trig_freq = false;
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(trig_freq_enable);
> +
> +static ssize_t freq_ts_enable_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->freq_ts);
> +}
> +
> +static ssize_t freq_ts_enable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf,
> +				    size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (val)
> +		drvdata->freq_ts = true;
> +	else
> +		drvdata->freq_ts = false;
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(freq_ts_enable);
> +
> +static ssize_t global_flush_req_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (!drvdata->csdev->refcnt)
> +		return -EPERM;

-EPERM doesn't seem right, maybe EBUSY or EINVAL?

Also don't you need CS_UNLOCK() for reading? I'm not 100% sure but I 
found one example of it in debug_init_arch_data().

> +
> +	val = readl_relaxed(drvdata->base + TPDA_CR);
> +	return sysfs_emit(buf, "%lx\n", val);
> +}
> +
> +static ssize_t global_flush_req_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf,
> +				      size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +

if (!val)
   return size;

Check this first, no point in taking the spinlock or checking the 
refcount if you aren't going to do anything.

> +	guard(spinlock)(&drvdata->spinlock);
> +	if (!drvdata->csdev->refcnt)
> +		return -EPERM;

ditto for -EPERM

> +
> +	if (val) {
> +		CS_UNLOCK(drvdata->base);
> +		val = readl_relaxed(drvdata->base + TPDA_CR);
> +		val = val | BIT(0);
> +		writel_relaxed(val, drvdata->base + TPDA_CR);
> +		CS_LOCK(drvdata->base);
> +	}
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(global_flush_req);
> +
> +static ssize_t cmbchan_mode_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +
> +	return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->cmbchan_mode);
> +}
> +
> +static ssize_t cmbchan_mode_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf,
> +				  size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	bool val;
> +
> +	if (kstrtobool(buf, &val))
> +		return -EINVAL;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (val)
> +		drvdata->cmbchan_mode = true;
> +	else
> +		drvdata->cmbchan_mode = false;
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(cmbchan_mode);
> +
> +static struct attribute *tpda_attrs[] = {
> +	&dev_attr_trig_async_enable.attr,
> +	&dev_attr_trig_flag_ts_enable.attr,
> +	&dev_attr_trig_freq_enable.attr,
> +	&dev_attr_freq_ts_enable.attr,
> +	&dev_attr_global_flush_req.attr,
> +	&dev_attr_cmbchan_mode.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group tpda_attr_grp = {
> +	.attrs = tpda_attrs,
> +};
> +
> +static const struct attribute_group *tpda_attr_grps[] = {
> +	&tpda_attr_grp,
> +	NULL,
> +};
> +
>   static int tpda_init_default_data(struct tpda_drvdata *drvdata)
>   {
>   	int atid;
> @@ -289,6 +528,7 @@ static int tpda_init_default_data(struct tpda_drvdata *drvdata)
>   		return atid;
>   
>   	drvdata->atid = atid;
> +	drvdata->freq_ts = true;
>   	return 0;
>   }
>   
> @@ -332,6 +572,7 @@ static int tpda_probe(struct amba_device *adev, const struct amba_id *id)
>   	desc.ops = &tpda_cs_ops;
>   	desc.pdata = adev->dev.platform_data;
>   	desc.dev = &adev->dev;
> +	desc.groups = tpda_attr_grps;
>   	desc.access = CSDEV_ACCESS_IOMEM(base);
>   	drvdata->csdev = coresight_register(&desc);
>   	if (IS_ERR(drvdata->csdev))
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index c6af3d2da3ef..b651372d4c88 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -8,17 +8,34 @@
>   
>   #define TPDA_CR			(0x000)
>   #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
> +#define TPDA_FPID_CR		(0x084)
> +
> +/* Cross trigger FREQ packets timestamp bit */
> +#define TPDA_CR_FREQTS		BIT(2)
> +/* Cross trigger FREQ packet request bit */
> +#define TPDA_CR_FRIE		BIT(3)
> +/* Cross trigger FLAG packet request interface bit */
> +#define TPDA_CR_FLRIE		BIT(4)
> +/* Cross trigger synchronization bit */
> +#define TPDA_CR_SRIE		BIT(5)
> +/* Packetize CMB/MCMB traffic bit */
> +#define TPDA_CR_CMBCHANMODE	BIT(20)
> +
>   /* Aggregator port enable bit */
>   #define TPDA_Pn_CR_ENA		BIT(0)
>   /* Aggregator port CMB data set element size bit */
>   #define TPDA_Pn_CR_CMBSIZE		GENMASK(7, 6)
>   /* Aggregator port DSB data set element size bit */
>   #define TPDA_Pn_CR_DSBSIZE		BIT(8)
> +/* Mode control bit */
> +#define TPDA_MODE_CTRL			BIT(12)
>   
>   #define TPDA_MAX_INPORTS	32
>   
>   /* Bits 6 ~ 12 is for atid value */
>   #define TPDA_CR_ATID		GENMASK(12, 6)
> +/* Bits 13 ~ 19 is for mid value */
> +#define TPDA_CR_MID		GENMASK(19, 13)
>   
>   /**
>    * struct tpda_drvdata - specifics associated to an TPDA component
> @@ -29,6 +46,11 @@
>    * @enable:     enable status of the component.
>    * @dsb_esize   Record the DSB element size.
>    * @cmb_esize   Record the CMB element size.
> + * @trig_async:	Enable/disable cross trigger synchronization sequence interface.
> + * @trig_flag_ts: Enable/disable cross trigger FLAG packet request interface.
> + * @trig_freq:	Enable/disable cross trigger FREQ packet request interface.
> + * @freq_ts:	Enable/disable the timestamp for all FREQ packets.
> + * @cmbchan_mode: Configure the CMB/MCMB channel mode.
>    */
>   struct tpda_drvdata {
>   	void __iomem		*base;
> @@ -38,6 +60,11 @@ struct tpda_drvdata {
>   	u8			atid;
>   	u32			dsb_esize;
>   	u32			cmb_esize;
> +	bool			trig_async;
> +	bool			trig_flag_ts;
> +	bool			trig_freq;
> +	bool			freq_ts;
> +	bool			cmbchan_mode;
>   };
>   
>   #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */

Re: [PATCH v1 1/3] coresight: tpda: Add sysfs node for tpda cross-trigger configuration

[Jie Gan]

On 8/26/2025 5:00 PM, James Clark wrote:
> 
> 
> On 26/08/2025 8:01 am, Jie Gan wrote:
>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>
>> Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
>> These registers define the characteristics of cross-trigger packets,
>> including generation frequency and flag values.
>>
>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> ---
>>   .../testing/sysfs-bus-coresight-devices-tpda  |  43 ++++
>>   drivers/hwtracing/coresight/coresight-tpda.c  | 241 ++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpda.h  |  27 ++
>>   3 files changed, 311 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight- 
>> devices-tpda
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>> new file mode 100644
>> index 000000000000..e827396a0fa1
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>> @@ -0,0 +1,43 @@
>> +What:        /sys/bus/coresight/devices/<tpda-name>/trig_async_enable
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Enable/disable cross trigger synchronization sequence 
>> interface.
>> +
>> +What:        /sys/bus/coresight/devices/<tpda-name>/trig_flag_ts_enable
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Enable/disable cross trigger FLAG packet request interface.
>> +
>> +What:        /sys/bus/coresight/devices/<tpda-name>/trig_freq_enable
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Enable/disable cross trigger FREQ packet request interface.
>> +
>> +What:        /sys/bus/coresight/devices/<tpda-name>/freq_ts_enable
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Enable/disable the timestamp for all FREQ packets.
>> +
>> +What:        /sys/bus/coresight/devices/<tpda-name>/global_flush_req
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Set/unset global (all ports) flush request bit. The bit 
>> remains set until a
> 
> I don't think you can unset? global_flush_req_store() only does 
> something for set.
> 
>> +        global flush request sequence completes.
>> +
>> +What:        /sys/bus/coresight/devices/<tpda-name>/cmbchan_mode
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Configure the CMB/MCMB channel mode for all enabled ports.
>> +        Value 0 means raw channel mapping mode. Value 1 means channel 
>> pair marking mode.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>> hwtracing/coresight/coresight-tpda.c
>> index 4e93fa5bace4..cc254d53b8ec 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -156,9 +156,37 @@ static void tpda_enable_pre_port(struct 
>> tpda_drvdata *drvdata)
>>       u32 val;
>>       val = readl_relaxed(drvdata->base + TPDA_CR);
>> +    val &= ~TPDA_CR_MID;
>>       val &= ~TPDA_CR_ATID;
>>       val |= FIELD_PREP(TPDA_CR_ATID, drvdata->atid);
>> +    if (drvdata->trig_async)
>> +        val = val | TPDA_CR_SRIE;
>> +    else
>> +        val = val & ~TPDA_CR_SRIE;
> 
> val |=
> val &=
> 

Will update in next version.

>> +    if (drvdata->trig_flag_ts)
>> +        val = val | TPDA_CR_FLRIE;
>> +    else
>> +        val = val & ~TPDA_CR_FLRIE;
>> +    if (drvdata->trig_freq)
>> +        val = val | TPDA_CR_FRIE;
>> +    else
>> +        val = val & ~TPDA_CR_FRIE;
>> +    if (drvdata->freq_ts)
>> +        val = val | TPDA_CR_FREQTS;
>> +    else
>> +        val = val & ~TPDA_CR_FREQTS;
>> +    if (drvdata->cmbchan_mode)
>> +        val = val | TPDA_CR_CMBCHANMODE;
>> +    else
>> +        val = val & ~TPDA_CR_CMBCHANMODE;
>>       writel_relaxed(val, drvdata->base + TPDA_CR);
>> +
>> +    /*
>> +     * If FLRIE bit is set, set the master and channel
>> +     * id as zero
>> +     */
>> +    if (drvdata->trig_flag_ts)
>> +        writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>>   }
>>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>> @@ -274,6 +302,217 @@ static const struct coresight_ops tpda_cs_ops = {
>>       .link_ops    = &tpda_link_ops,
>>   };
>> +static ssize_t trig_async_enable_show(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_async);
>> +}
>> +
>> +static ssize_t trig_async_enable_store(struct device *dev,
>> +                       struct device_attribute *attr,
>> +                       const char *buf,
>> +                       size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->trig_async = true;
>> +    else
>> +        drvdata->trig_async = false;
>> +
> 
> drvdata->trig_async = !!val
> 
> same with all the following ones too
> 

Will address all codes.

>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(trig_async_enable);
>> +
>> +static ssize_t trig_flag_ts_enable_show(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_flag_ts);
>> +}
>> +
>> +static ssize_t trig_flag_ts_enable_store(struct device *dev,
>> +                     struct device_attribute *attr,
>> +                     const char *buf,
>> +                     size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->trig_flag_ts = true;
>> +    else
>> +        drvdata->trig_flag_ts = false;
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(trig_flag_ts_enable);
>> +
>> +static ssize_t trig_freq_enable_show(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_freq);
>> +}
>> +
>> +static ssize_t trig_freq_enable_store(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      const char *buf,
>> +                      size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->trig_freq = true;
>> +    else
>> +        drvdata->trig_freq = false;
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(trig_freq_enable);
>> +
>> +static ssize_t freq_ts_enable_show(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->freq_ts);
>> +}
>> +
>> +static ssize_t freq_ts_enable_store(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    const char *buf,
>> +                    size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->freq_ts = true;
>> +    else
>> +        drvdata->freq_ts = false;
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(freq_ts_enable);
>> +
>> +static ssize_t global_flush_req_show(struct device *dev,
>> +                     struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (!drvdata->csdev->refcnt)
>> +        return -EPERM;
> 
> -EPERM doesn't seem right, maybe EBUSY or EINVAL?

I think EINVAL is better just because the TPDA is not enabled yet. Will fix.

> 
> Also don't you need CS_UNLOCK() for reading? I'm not 100% sure but I 
> found one example of it in debug_init_arch_data().
> 

Sorry about that, I shouldnt miss the CS_UNLOCK&&CS_LOCK pairs.

>> +
>> +    val = readl_relaxed(drvdata->base + TPDA_CR);
>> +    return sysfs_emit(buf, "%lx\n", val);
>> +}
>> +
>> +static ssize_t global_flush_req_store(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      const char *buf,
>> +                      size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
> 
> if (!val)
>    return size;
> 
> Check this first, no point in taking the spinlock or checking the 
> refcount if you aren't going to do anything.

Will fix it.

> 
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (!drvdata->csdev->refcnt)
>> +        return -EPERM;
> 
> ditto for -EPERM

Will fix it.

Thanks,
Jie

> 
>> +
>> +    if (val) {
>> +        CS_UNLOCK(drvdata->base);
>> +        val = readl_relaxed(drvdata->base + TPDA_CR);
>> +        val = val | BIT(0);
>> +        writel_relaxed(val, drvdata->base + TPDA_CR);
>> +        CS_LOCK(drvdata->base);
>> +    }
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(global_flush_req);
>> +
>> +static ssize_t cmbchan_mode_show(struct device *dev,
>> +                 struct device_attribute *attr,
>> +                 char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->cmbchan_mode);
>> +}
>> +
>> +static ssize_t cmbchan_mode_store(struct device *dev,
>> +                  struct device_attribute *attr,
>> +                  const char *buf,
>> +                  size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    bool val;
>> +
>> +    if (kstrtobool(buf, &val))
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (val)
>> +        drvdata->cmbchan_mode = true;
>> +    else
>> +        drvdata->cmbchan_mode = false;
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(cmbchan_mode);
>> +
>> +static struct attribute *tpda_attrs[] = {
>> +    &dev_attr_trig_async_enable.attr,
>> +    &dev_attr_trig_flag_ts_enable.attr,
>> +    &dev_attr_trig_freq_enable.attr,
>> +    &dev_attr_freq_ts_enable.attr,
>> +    &dev_attr_global_flush_req.attr,
>> +    &dev_attr_cmbchan_mode.attr,
>> +    NULL,
>> +};
>> +
>> +static struct attribute_group tpda_attr_grp = {
>> +    .attrs = tpda_attrs,
>> +};
>> +
>> +static const struct attribute_group *tpda_attr_grps[] = {
>> +    &tpda_attr_grp,
>> +    NULL,
>> +};
>> +
>>   static int tpda_init_default_data(struct tpda_drvdata *drvdata)
>>   {
>>       int atid;
>> @@ -289,6 +528,7 @@ static int tpda_init_default_data(struct 
>> tpda_drvdata *drvdata)
>>           return atid;
>>       drvdata->atid = atid;
>> +    drvdata->freq_ts = true;
>>       return 0;
>>   }
>> @@ -332,6 +572,7 @@ static int tpda_probe(struct amba_device *adev, 
>> const struct amba_id *id)
>>       desc.ops = &tpda_cs_ops;
>>       desc.pdata = adev->dev.platform_data;
>>       desc.dev = &adev->dev;
>> +    desc.groups = tpda_attr_grps;
>>       desc.access = CSDEV_ACCESS_IOMEM(base);
>>       drvdata->csdev = coresight_register(&desc);
>>       if (IS_ERR(drvdata->csdev))
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>> hwtracing/coresight/coresight-tpda.h
>> index c6af3d2da3ef..b651372d4c88 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>> @@ -8,17 +8,34 @@
>>   #define TPDA_CR            (0x000)
>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>> +#define TPDA_FPID_CR        (0x084)
>> +
>> +/* Cross trigger FREQ packets timestamp bit */
>> +#define TPDA_CR_FREQTS        BIT(2)
>> +/* Cross trigger FREQ packet request bit */
>> +#define TPDA_CR_FRIE        BIT(3)
>> +/* Cross trigger FLAG packet request interface bit */
>> +#define TPDA_CR_FLRIE        BIT(4)
>> +/* Cross trigger synchronization bit */
>> +#define TPDA_CR_SRIE        BIT(5)
>> +/* Packetize CMB/MCMB traffic bit */
>> +#define TPDA_CR_CMBCHANMODE    BIT(20)
>> +
>>   /* Aggregator port enable bit */
>>   #define TPDA_Pn_CR_ENA        BIT(0)
>>   /* Aggregator port CMB data set element size bit */
>>   #define TPDA_Pn_CR_CMBSIZE        GENMASK(7, 6)
>>   /* Aggregator port DSB data set element size bit */
>>   #define TPDA_Pn_CR_DSBSIZE        BIT(8)
>> +/* Mode control bit */
>> +#define TPDA_MODE_CTRL            BIT(12)
>>   #define TPDA_MAX_INPORTS    32
>>   /* Bits 6 ~ 12 is for atid value */
>>   #define TPDA_CR_ATID        GENMASK(12, 6)
>> +/* Bits 13 ~ 19 is for mid value */
>> +#define TPDA_CR_MID        GENMASK(19, 13)
>>   /**
>>    * struct tpda_drvdata - specifics associated to an TPDA component
>> @@ -29,6 +46,11 @@
>>    * @enable:     enable status of the component.
>>    * @dsb_esize   Record the DSB element size.
>>    * @cmb_esize   Record the CMB element size.
>> + * @trig_async:    Enable/disable cross trigger synchronization 
>> sequence interface.
>> + * @trig_flag_ts: Enable/disable cross trigger FLAG packet request 
>> interface.
>> + * @trig_freq:    Enable/disable cross trigger FREQ packet request 
>> interface.
>> + * @freq_ts:    Enable/disable the timestamp for all FREQ packets.
>> + * @cmbchan_mode: Configure the CMB/MCMB channel mode.
>>    */
>>   struct tpda_drvdata {
>>       void __iomem        *base;
>> @@ -38,6 +60,11 @@ struct tpda_drvdata {
>>       u8            atid;
>>       u32            dsb_esize;
>>       u32            cmb_esize;
>> +    bool            trig_async;
>> +    bool            trig_flag_ts;
>> +    bool            trig_freq;
>> +    bool            freq_ts;
>> +    bool            cmbchan_mode;
>>   };
>>   #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
> 

Re: [PATCH v1 1/3] coresight: tpda: Add sysfs node for tpda cross-trigger configuration

[Jie Gan]

On 8/26/2025 5:09 PM, Jie Gan wrote:
> 
> 
> On 8/26/2025 5:00 PM, James Clark wrote:
>>
>>
>> On 26/08/2025 8:01 am, Jie Gan wrote:
>>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>
>>> Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
>>> These registers define the characteristics of cross-trigger packets,
>>> including generation frequency and flag values.
>>>
>>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>> ---
>>>   .../testing/sysfs-bus-coresight-devices-tpda  |  43 ++++
>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 241 ++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-tpda.h  |  27 ++
>>>   3 files changed, 311 insertions(+)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight- 
>>> devices-tpda
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> new file mode 100644
>>> index 000000000000..e827396a0fa1
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> @@ -0,0 +1,43 @@
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/trig_async_enable
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Enable/disable cross trigger synchronization sequence 
>>> interface.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/trig_flag_ts_enable
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Enable/disable cross trigger FLAG packet request 
>>> interface.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/trig_freq_enable
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Enable/disable cross trigger FREQ packet request 
>>> interface.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/freq_ts_enable
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Enable/disable the timestamp for all FREQ packets.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/global_flush_req
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Set/unset global (all ports) flush request bit. The bit 
>>> remains set until a
>>
>> I don't think you can unset? global_flush_req_store() only does 
>> something for set.

Sorry for just missed this comment.

I think we dont need do the unset. The unset will done by register 
itself after the flush work has done. I will update the description to 
correct this point.

Thanks,
Jie


>>
>>> +        global flush request sequence completes.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/cmbchan_mode
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Configure the CMB/MCMB channel mode for all enabled ports.
>>> +        Value 0 means raw channel mapping mode. Value 1 means 
>>> channel pair marking mode.
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>>> hwtracing/coresight/coresight-tpda.c
>>> index 4e93fa5bace4..cc254d53b8ec 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -156,9 +156,37 @@ static void tpda_enable_pre_port(struct 
>>> tpda_drvdata *drvdata)
>>>       u32 val;
>>>       val = readl_relaxed(drvdata->base + TPDA_CR);
>>> +    val &= ~TPDA_CR_MID;
>>>       val &= ~TPDA_CR_ATID;
>>>       val |= FIELD_PREP(TPDA_CR_ATID, drvdata->atid);
>>> +    if (drvdata->trig_async)
>>> +        val = val | TPDA_CR_SRIE;
>>> +    else
>>> +        val = val & ~TPDA_CR_SRIE;
>>
>> val |=
>> val &=
>>
> 
> Will update in next version.
> 
>>> +    if (drvdata->trig_flag_ts)
>>> +        val = val | TPDA_CR_FLRIE;
>>> +    else
>>> +        val = val & ~TPDA_CR_FLRIE;
>>> +    if (drvdata->trig_freq)
>>> +        val = val | TPDA_CR_FRIE;
>>> +    else
>>> +        val = val & ~TPDA_CR_FRIE;
>>> +    if (drvdata->freq_ts)
>>> +        val = val | TPDA_CR_FREQTS;
>>> +    else
>>> +        val = val & ~TPDA_CR_FREQTS;
>>> +    if (drvdata->cmbchan_mode)
>>> +        val = val | TPDA_CR_CMBCHANMODE;
>>> +    else
>>> +        val = val & ~TPDA_CR_CMBCHANMODE;
>>>       writel_relaxed(val, drvdata->base + TPDA_CR);
>>> +
>>> +    /*
>>> +     * If FLRIE bit is set, set the master and channel
>>> +     * id as zero
>>> +     */
>>> +    if (drvdata->trig_flag_ts)
>>> +        writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>>>   }
>>>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>>> @@ -274,6 +302,217 @@ static const struct coresight_ops tpda_cs_ops = {
>>>       .link_ops    = &tpda_link_ops,
>>>   };
>>> +static ssize_t trig_async_enable_show(struct device *dev,
>>> +                      struct device_attribute *attr,
>>> +                      char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_async);
>>> +}
>>> +
>>> +static ssize_t trig_async_enable_store(struct device *dev,
>>> +                       struct device_attribute *attr,
>>> +                       const char *buf,
>>> +                       size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if (kstrtoul(buf, 0, &val))
>>> +        return -EINVAL;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->trig_async = true;
>>> +    else
>>> +        drvdata->trig_async = false;
>>> +
>>
>> drvdata->trig_async = !!val
>>
>> same with all the following ones too
>>
> 
> Will address all codes.
> 
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(trig_async_enable);
>>> +
>>> +static ssize_t trig_flag_ts_enable_show(struct device *dev,
>>> +                    struct device_attribute *attr,
>>> +                    char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata- 
>>> >trig_flag_ts);
>>> +}
>>> +
>>> +static ssize_t trig_flag_ts_enable_store(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     const char *buf,
>>> +                     size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if (kstrtoul(buf, 0, &val))
>>> +        return -EINVAL;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->trig_flag_ts = true;
>>> +    else
>>> +        drvdata->trig_flag_ts = false;
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(trig_flag_ts_enable);
>>> +
>>> +static ssize_t trig_freq_enable_show(struct device *dev,
>>> +                      struct device_attribute *attr,
>>> +                      char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->trig_freq);
>>> +}
>>> +
>>> +static ssize_t trig_freq_enable_store(struct device *dev,
>>> +                      struct device_attribute *attr,
>>> +                      const char *buf,
>>> +                      size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if (kstrtoul(buf, 0, &val))
>>> +        return -EINVAL;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->trig_freq = true;
>>> +    else
>>> +        drvdata->trig_freq = false;
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(trig_freq_enable);
>>> +
>>> +static ssize_t freq_ts_enable_show(struct device *dev,
>>> +                   struct device_attribute *attr,
>>> +                   char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata->freq_ts);
>>> +}
>>> +
>>> +static ssize_t freq_ts_enable_store(struct device *dev,
>>> +                    struct device_attribute *attr,
>>> +                    const char *buf,
>>> +                    size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if (kstrtoul(buf, 0, &val))
>>> +        return -EINVAL;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->freq_ts = true;
>>> +    else
>>> +        drvdata->freq_ts = false;
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(freq_ts_enable);
>>> +
>>> +static ssize_t global_flush_req_show(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (!drvdata->csdev->refcnt)
>>> +        return -EPERM;
>>
>> -EPERM doesn't seem right, maybe EBUSY or EINVAL?
> 
> I think EINVAL is better just because the TPDA is not enabled yet. Will 
> fix.
> 
>>
>> Also don't you need CS_UNLOCK() for reading? I'm not 100% sure but I 
>> found one example of it in debug_init_arch_data().
>>
> 
> Sorry about that, I shouldnt miss the CS_UNLOCK&&CS_LOCK pairs.
> 
>>> +
>>> +    val = readl_relaxed(drvdata->base + TPDA_CR);
>>> +    return sysfs_emit(buf, "%lx\n", val);
>>> +}
>>> +
>>> +static ssize_t global_flush_req_store(struct device *dev,
>>> +                      struct device_attribute *attr,
>>> +                      const char *buf,
>>> +                      size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if (kstrtoul(buf, 0, &val))
>>> +        return -EINVAL;
>>> +
>>
>> if (!val)
>>    return size;
>>
>> Check this first, no point in taking the spinlock or checking the 
>> refcount if you aren't going to do anything.
> 
> Will fix it.
> 
>>
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (!drvdata->csdev->refcnt)
>>> +        return -EPERM;
>>
>> ditto for -EPERM
> 
> Will fix it.
> 
> Thanks,
> Jie
> 
>>
>>> +
>>> +    if (val) {
>>> +        CS_UNLOCK(drvdata->base);
>>> +        val = readl_relaxed(drvdata->base + TPDA_CR);
>>> +        val = val | BIT(0);
>>> +        writel_relaxed(val, drvdata->base + TPDA_CR);
>>> +        CS_LOCK(drvdata->base);
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(global_flush_req);
>>> +
>>> +static ssize_t cmbchan_mode_show(struct device *dev,
>>> +                 struct device_attribute *attr,
>>> +                 char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", (unsigned int)drvdata- 
>>> >cmbchan_mode);
>>> +}
>>> +
>>> +static ssize_t cmbchan_mode_store(struct device *dev,
>>> +                  struct device_attribute *attr,
>>> +                  const char *buf,
>>> +                  size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    bool val;
>>> +
>>> +    if (kstrtobool(buf, &val))
>>> +        return -EINVAL;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (val)
>>> +        drvdata->cmbchan_mode = true;
>>> +    else
>>> +        drvdata->cmbchan_mode = false;
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(cmbchan_mode);
>>> +
>>> +static struct attribute *tpda_attrs[] = {
>>> +    &dev_attr_trig_async_enable.attr,
>>> +    &dev_attr_trig_flag_ts_enable.attr,
>>> +    &dev_attr_trig_freq_enable.attr,
>>> +    &dev_attr_freq_ts_enable.attr,
>>> +    &dev_attr_global_flush_req.attr,
>>> +    &dev_attr_cmbchan_mode.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static struct attribute_group tpda_attr_grp = {
>>> +    .attrs = tpda_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *tpda_attr_grps[] = {
>>> +    &tpda_attr_grp,
>>> +    NULL,
>>> +};
>>> +
>>>   static int tpda_init_default_data(struct tpda_drvdata *drvdata)
>>>   {
>>>       int atid;
>>> @@ -289,6 +528,7 @@ static int tpda_init_default_data(struct 
>>> tpda_drvdata *drvdata)
>>>           return atid;
>>>       drvdata->atid = atid;
>>> +    drvdata->freq_ts = true;
>>>       return 0;
>>>   }
>>> @@ -332,6 +572,7 @@ static int tpda_probe(struct amba_device *adev, 
>>> const struct amba_id *id)
>>>       desc.ops = &tpda_cs_ops;
>>>       desc.pdata = adev->dev.platform_data;
>>>       desc.dev = &adev->dev;
>>> +    desc.groups = tpda_attr_grps;
>>>       desc.access = CSDEV_ACCESS_IOMEM(base);
>>>       drvdata->csdev = coresight_register(&desc);
>>>       if (IS_ERR(drvdata->csdev))
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>>> hwtracing/coresight/coresight-tpda.h
>>> index c6af3d2da3ef..b651372d4c88 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>> @@ -8,17 +8,34 @@
>>>   #define TPDA_CR            (0x000)
>>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>> +#define TPDA_FPID_CR        (0x084)
>>> +
>>> +/* Cross trigger FREQ packets timestamp bit */
>>> +#define TPDA_CR_FREQTS        BIT(2)
>>> +/* Cross trigger FREQ packet request bit */
>>> +#define TPDA_CR_FRIE        BIT(3)
>>> +/* Cross trigger FLAG packet request interface bit */
>>> +#define TPDA_CR_FLRIE        BIT(4)
>>> +/* Cross trigger synchronization bit */
>>> +#define TPDA_CR_SRIE        BIT(5)
>>> +/* Packetize CMB/MCMB traffic bit */
>>> +#define TPDA_CR_CMBCHANMODE    BIT(20)
>>> +
>>>   /* Aggregator port enable bit */
>>>   #define TPDA_Pn_CR_ENA        BIT(0)
>>>   /* Aggregator port CMB data set element size bit */
>>>   #define TPDA_Pn_CR_CMBSIZE        GENMASK(7, 6)
>>>   /* Aggregator port DSB data set element size bit */
>>>   #define TPDA_Pn_CR_DSBSIZE        BIT(8)
>>> +/* Mode control bit */
>>> +#define TPDA_MODE_CTRL            BIT(12)
>>>   #define TPDA_MAX_INPORTS    32
>>>   /* Bits 6 ~ 12 is for atid value */
>>>   #define TPDA_CR_ATID        GENMASK(12, 6)
>>> +/* Bits 13 ~ 19 is for mid value */
>>> +#define TPDA_CR_MID        GENMASK(19, 13)
>>>   /**
>>>    * struct tpda_drvdata - specifics associated to an TPDA component
>>> @@ -29,6 +46,11 @@
>>>    * @enable:     enable status of the component.
>>>    * @dsb_esize   Record the DSB element size.
>>>    * @cmb_esize   Record the CMB element size.
>>> + * @trig_async:    Enable/disable cross trigger synchronization 
>>> sequence interface.
>>> + * @trig_flag_ts: Enable/disable cross trigger FLAG packet request 
>>> interface.
>>> + * @trig_freq:    Enable/disable cross trigger FREQ packet request 
>>> interface.
>>> + * @freq_ts:    Enable/disable the timestamp for all FREQ packets.
>>> + * @cmbchan_mode: Configure the CMB/MCMB channel mode.
>>>    */
>>>   struct tpda_drvdata {
>>>       void __iomem        *base;
>>> @@ -38,6 +60,11 @@ struct tpda_drvdata {
>>>       u8            atid;
>>>       u32            dsb_esize;
>>>       u32            cmb_esize;
>>> +    bool            trig_async;
>>> +    bool            trig_flag_ts;
>>> +    bool            trig_freq;
>>> +    bool            freq_ts;
>>> +    bool            cmbchan_mode;
>>>   };
>>>   #endif  /* _CORESIGHT_CORESIGHT_TPDA_H */
>>
> 
> 

Re: [PATCH v1 1/3] coresight: tpda: Add sysfs node for tpda cross-trigger configuration

[James Clark]

On 26/08/2025 8:01 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
> 
> Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
> These registers define the characteristics of cross-trigger packets,
> including generation frequency and flag values.
> 
> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
> ---
>   .../testing/sysfs-bus-coresight-devices-tpda  |  43 ++++
>   drivers/hwtracing/coresight/coresight-tpda.c  | 241 ++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpda.h  |  27 ++
>   3 files changed, 311 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> 
[...]
> +#define TPDA_FPID_CR		(0x084)
> +
> +/* Cross trigger FREQ packets timestamp bit */
> +#define TPDA_CR_FREQTS		BIT(2)
> +/* Cross trigger FREQ packet request bit */
> +#define TPDA_CR_FRIE		BIT(3)
> +/* Cross trigger FLAG packet request interface bit */
> +#define TPDA_CR_FLRIE		BIT(4)
> +/* Cross trigger synchronization bit */
> +#define TPDA_CR_SRIE		BIT(5)
> +/* Packetize CMB/MCMB traffic bit */
> +#define TPDA_CR_CMBCHANMODE	BIT(20)
> +
>   /* Aggregator port enable bit */
>   #define TPDA_Pn_CR_ENA		BIT(0)
>   /* Aggregator port CMB data set element size bit */
>   #define TPDA_Pn_CR_CMBSIZE		GENMASK(7, 6)
>   /* Aggregator port DSB data set element size bit */
>   #define TPDA_Pn_CR_DSBSIZE		BIT(8)
> +/* Mode control bit */
> +#define TPDA_MODE_CTRL			BIT(12)
>   

This one is missing the register name prefix, like TPDA_SYNCR_MODE_CTRL

Re: [PATCH v1 2/3] coresight: tpda: add function to configure TPDA_SYNCR register

[James Clark]

On 26/08/2025 8:01 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
> 
> The TPDA_SYNCR register defines the frequency at which TPDA generates
> ASYNC packets, enabling userspace tools to accurately parse each valid
> packet.
> 
> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
> ---
>   drivers/hwtracing/coresight/coresight-tpda.c | 15 +++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpda.h |  1 +
>   2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index cc254d53b8ec..9e623732d1e7 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -189,6 +189,18 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
>   		writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>   }
>   
> +static void tpda_enable_post_port(struct tpda_drvdata *drvdata)
> +{
> +	uint32_t val;

Minor nit: this is inconsistent with u32 used elsewhere in this file.

> +
> +	val = readl_relaxed(drvdata->base + TPDA_SYNCR);
> +	/* Clear the mode */
> +	val = val & ~TPDA_MODE_CTRL;

&=

> +	/* Program the counter value */
> +	val = val | 0xFFF;

|=

Defining a field would be a bit nicer here. Like:

val |= FIELD_PREP(TPDA_SYNCR_COUNTER, UINT32_MAX);

Assuming you wanted to set all bits, and 0xFFF isn't some specific value.

> +	writel_relaxed(val, drvdata->base + TPDA_SYNCR);
> +}
> +
>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>   {
>   	u32 val;
> @@ -227,6 +239,9 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
>   		tpda_enable_pre_port(drvdata);
>   
>   	ret = tpda_enable_port(drvdata, port);
> +	if (!drvdata->csdev->refcnt)
> +		tpda_enable_post_port(drvdata);

Any reason this can't be done on tpda_enable_pre_port()? It has the same 
logic where it's only done once for the first port.

If it can't be done there you should add a comment saying why it must be 
done after enabling the first port.

> +
>   	CS_LOCK(drvdata->base);
>   
>   	return ret;
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index b651372d4c88..00d146960d81 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -9,6 +9,7 @@
>   #define TPDA_CR			(0x000)
>   #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
>   #define TPDA_FPID_CR		(0x084)
> +#define TPDA_SYNCR		(0x08C)
>   
>   /* Cross trigger FREQ packets timestamp bit */
>   #define TPDA_CR_FREQTS		BIT(2)

Re: [PATCH v1 1/3] coresight: tpda: Add sysfs node for tpda cross-trigger configuration

[Jie Gan]

On 8/26/2025 5:17 PM, James Clark wrote:
> 
> 
> On 26/08/2025 8:01 am, Jie Gan wrote:
>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>
>> Introduce sysfs nodes to configure cross-trigger parameters for TPDA.
>> These registers define the characteristics of cross-trigger packets,
>> including generation frequency and flag values.
>>
>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> ---
>>   .../testing/sysfs-bus-coresight-devices-tpda  |  43 ++++
>>   drivers/hwtracing/coresight/coresight-tpda.c  | 241 ++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpda.h  |  27 ++
>>   3 files changed, 311 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight- 
>> devices-tpda
>>
> [...]
>> +#define TPDA_FPID_CR        (0x084)
>> +
>> +/* Cross trigger FREQ packets timestamp bit */
>> +#define TPDA_CR_FREQTS        BIT(2)
>> +/* Cross trigger FREQ packet request bit */
>> +#define TPDA_CR_FRIE        BIT(3)
>> +/* Cross trigger FLAG packet request interface bit */
>> +#define TPDA_CR_FLRIE        BIT(4)
>> +/* Cross trigger synchronization bit */
>> +#define TPDA_CR_SRIE        BIT(5)
>> +/* Packetize CMB/MCMB traffic bit */
>> +#define TPDA_CR_CMBCHANMODE    BIT(20)
>> +
>>   /* Aggregator port enable bit */
>>   #define TPDA_Pn_CR_ENA        BIT(0)
>>   /* Aggregator port CMB data set element size bit */
>>   #define TPDA_Pn_CR_CMBSIZE        GENMASK(7, 6)
>>   /* Aggregator port DSB data set element size bit */
>>   #define TPDA_Pn_CR_DSBSIZE        BIT(8)
>> +/* Mode control bit */
>> +#define TPDA_MODE_CTRL            BIT(12)
> 
> This one is missing the register name prefix, like TPDA_SYNCR_MODE_CTRL

Yes, you are right. Besides, I made a mistake here. I should put this 
Macro in patchset 2.

Anyway, I will fix it in next version.

Thanks,
Jie

> 
> 
> 
> 

Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[James Clark]

On 26/08/2025 8:01 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
> 
> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
> for port i, forcing the data to synchronize and be transmitted to the
> sink device.
> 
> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
> ---
>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
>   drivers/hwtracing/coresight/coresight-tpda.c  | 45 +++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
>   3 files changed, 53 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> index e827396a0fa1..8803158ba42f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
> @@ -41,3 +41,10 @@ Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>   Description:
>   		(RW) Configure the CMB/MCMB channel mode for all enabled ports.
>   		Value 0 means raw channel mapping mode. Value 1 means channel pair marking mode.
> +
> +What:		/sys/bus/coresight/devices/<tpda-name>/port_flush_req
> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
> +Description:
> +		(RW) Configure the bit i to requests a flush operation of port i on the TPDA.
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 9e623732d1e7..c5f169facc51 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct device *dev,
>   }
>   static DEVICE_ATTR_RW(cmbchan_mode);
>   
> +static ssize_t port_flush_req_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (!drvdata->csdev->refcnt)
> +		return -EPERM;
> +
> +	val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
> +	return sysfs_emit(buf, "%lx\n", val);

Decimal would be better for a port number that goes from 0 - 127. If you 
really want to use hex then don't you need to prefix it with 0x? 
Otherwise you can't tell the difference between decimal 10 and hex 10, 
and it's not documented that it's hex either.

> +}
> +
> +static ssize_t port_flush_req_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf,
> +				    size_t size)
> +{
> +	struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	/* The valid value ranges from 0 to 127 */
> +	if (val > 127)
> +		return -EINVAL;
> +
> +	guard(spinlock)(&drvdata->spinlock);
> +	if (!drvdata->csdev->refcnt)
> +		return -EPERM;
> +
> +	if (val) {

If 0 - 127 are valid don't you want to write 0 too?

> +		CS_UNLOCK(drvdata->base);
> +		writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
> +		CS_LOCK(drvdata->base);
> +	}
> +
> +	return size;
> +}
> +static DEVICE_ATTR_RW(port_flush_req);
> +
>   static struct attribute *tpda_attrs[] = {
>   	&dev_attr_trig_async_enable.attr,
>   	&dev_attr_trig_flag_ts_enable.attr,
> @@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
>   	&dev_attr_freq_ts_enable.attr,
>   	&dev_attr_global_flush_req.attr,
>   	&dev_attr_cmbchan_mode.attr,
> +	&dev_attr_port_flush_req.attr,
>   	NULL,
>   };
>   
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index 00d146960d81..55a18d718357 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.h
> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
> @@ -10,6 +10,7 @@
>   #define TPDA_Pn_CR(n)		(0x004 + (n * 4))
>   #define TPDA_FPID_CR		(0x084)
>   #define TPDA_SYNCR		(0x08C)
> +#define TPDA_FLUSH_CR		(0x090)
>   
>   /* Cross trigger FREQ packets timestamp bit */
>   #define TPDA_CR_FREQTS		BIT(2)

Re: [PATCH v1 2/3] coresight: tpda: add function to configure TPDA_SYNCR register

[Jie Gan]

On 8/26/2025 5:20 PM, James Clark wrote:
> 
> 
> On 26/08/2025 8:01 am, Jie Gan wrote:
>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>
>> The TPDA_SYNCR register defines the frequency at which TPDA generates
>> ASYNC packets, enabling userspace tools to accurately parse each valid
>> packet.
>>
>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tpda.c | 15 +++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpda.h |  1 +
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>> hwtracing/coresight/coresight-tpda.c
>> index cc254d53b8ec..9e623732d1e7 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -189,6 +189,18 @@ static void tpda_enable_pre_port(struct 
>> tpda_drvdata *drvdata)
>>           writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
>>   }
>> +static void tpda_enable_post_port(struct tpda_drvdata *drvdata)
>> +{
>> +    uint32_t val;
> 
> Minor nit: this is inconsistent with u32 used elsewhere in this file.

Will fix it in next version.

> 
>> +
>> +    val = readl_relaxed(drvdata->base + TPDA_SYNCR);
>> +    /* Clear the mode */
>> +    val = val & ~TPDA_MODE_CTRL;
> 
> &=

Will fix.

> 
>> +    /* Program the counter value */
>> +    val = val | 0xFFF;
> 
> |=

Will fix.

> 
> Defining a field would be a bit nicer here. Like:
> 
> val |= FIELD_PREP(TPDA_SYNCR_COUNTER, UINT32_MAX);

That's better, forgot to use the proper Macro. I will re-check all codes 
again to update all possible fixes.

> 
> Assuming you wanted to set all bits, and 0xFFF isn't some specific value.

Yes, this field has 12 bits and we prefer the max value to prevent to 
generate too many ASYNC packets. This field indicates a count value for 
number of bytes. Once the the count reaches the number, a ASYNC packet 
will be generated.

> 
>> +    writel_relaxed(val, drvdata->base + TPDA_SYNCR);
>> +}
>> +
>>   static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
>>   {
>>       u32 val;
>> @@ -227,6 +239,9 @@ static int __tpda_enable(struct tpda_drvdata 
>> *drvdata, int port)
>>           tpda_enable_pre_port(drvdata);
>>       ret = tpda_enable_port(drvdata, port);
>> +    if (!drvdata->csdev->refcnt)
>> +        tpda_enable_post_port(drvdata);
> 
> Any reason this can't be done on tpda_enable_pre_port()? It has the same 
> logic where it's only done once for the first port.
> 
> If it can't be done there you should add a comment saying why it must be 
> done after enabling the first port.

This register only affect the port which already be enabled. That's why 
we add it to enable_post_port. But as you mentioned, we can put the 
logic into enable_pre_port without side effect.

I think it's ok to move the logic to enable_pre_port to simply codes here.

Thanks,
Jie

> 
>> +
>>       CS_LOCK(drvdata->base);
>>       return ret;
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>> hwtracing/coresight/coresight-tpda.h
>> index b651372d4c88..00d146960d81 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>> @@ -9,6 +9,7 @@
>>   #define TPDA_CR            (0x000)
>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>   #define TPDA_FPID_CR        (0x084)
>> +#define TPDA_SYNCR        (0x08C)
>>   /* Cross trigger FREQ packets timestamp bit */
>>   #define TPDA_CR_FREQTS        BIT(2)
> 
> 

Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[Jie Gan]

On 8/26/2025 5:27 PM, James Clark wrote:
> 
> 
> On 26/08/2025 8:01 am, Jie Gan wrote:
>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>
>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>> for port i, forcing the data to synchronize and be transmitted to the
>> sink device.
>>
>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> ---
>>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
>>   drivers/hwtracing/coresight/coresight-tpda.c  | 45 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>> index e827396a0fa1..8803158ba42f 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>> @@ -41,3 +41,10 @@ Contact:    Jinlong Mao 
>> <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>>   Description:
>>           (RW) Configure the CMB/MCMB channel mode for all enabled ports.
>>           Value 0 means raw channel mapping mode. Value 1 means 
>> channel pair marking mode.
>> +
>> +What:        /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>> +Date:        August 2025
>> +KernelVersion:    6.17
>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>> +Description:
>> +        (RW) Configure the bit i to requests a flush operation of 
>> port i on the TPDA.
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>> hwtracing/coresight/coresight-tpda.c
>> index 9e623732d1e7..c5f169facc51 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>> @@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct device 
>> *dev,
>>   }
>>   static DEVICE_ATTR_RW(cmbchan_mode);
>> +static ssize_t port_flush_req_show(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (!drvdata->csdev->refcnt)
>> +        return -EPERM;
>> +
>> +    val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>> +    return sysfs_emit(buf, "%lx\n", val);
> 
> Decimal would be better for a port number that goes from 0 - 127. If you 
> really want to use hex then don't you need to prefix it with 0x? 
> Otherwise you can't tell the difference between decimal 10 and hex 10, 
> and it's not documented that it's hex either.
> 

Got it. I will fix the code here, and update the description in document.

>> +}
>> +
>> +static ssize_t port_flush_req_store(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    const char *buf,
>> +                    size_t size)
>> +{
>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +    unsigned long val;
>> +
>> +    if (kstrtoul(buf, 0, &val))
>> +        return -EINVAL;
>> +
>> +    /* The valid value ranges from 0 to 127 */
>> +    if (val > 127)
>> +        return -EINVAL;
>> +
>> +    guard(spinlock)(&drvdata->spinlock);
>> +    if (!drvdata->csdev->refcnt)
>> +        return -EPERM;
>> +
>> +    if (val) {
> 
> If 0 - 127 are valid don't you want to write 0 too?

It's 1-127 here. 0 may leads to an unexpected issue here.

Thanks,
Jie

> 
>> +        CS_UNLOCK(drvdata->base);
>> +        writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>> +        CS_LOCK(drvdata->base);
>> +    }
>> +
>> +    return size;
>> +}
>> +static DEVICE_ATTR_RW(port_flush_req);
>> +
>>   static struct attribute *tpda_attrs[] = {
>>       &dev_attr_trig_async_enable.attr,
>>       &dev_attr_trig_flag_ts_enable.attr,
>> @@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
>>       &dev_attr_freq_ts_enable.attr,
>>       &dev_attr_global_flush_req.attr,
>>       &dev_attr_cmbchan_mode.attr,
>> +    &dev_attr_port_flush_req.attr,
>>       NULL,
>>   };
>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>> hwtracing/coresight/coresight-tpda.h
>> index 00d146960d81..55a18d718357 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>> @@ -10,6 +10,7 @@
>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>   #define TPDA_FPID_CR        (0x084)
>>   #define TPDA_SYNCR        (0x08C)
>> +#define TPDA_FLUSH_CR        (0x090)
>>   /* Cross trigger FREQ packets timestamp bit */
>>   #define TPDA_CR_FREQTS        BIT(2)
> 
> 

Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[James Clark]

On 26/08/2025 10:39 am, Jie Gan wrote:
> 
> 
> On 8/26/2025 5:27 PM, James Clark wrote:
>>
>>
>> On 26/08/2025 8:01 am, Jie Gan wrote:
>>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>
>>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>>> for port i, forcing the data to synchronize and be transmitted to the
>>> sink device.
>>>
>>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>> ---
>>>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 45 +++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
>>>   3 files changed, 53 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> index e827396a0fa1..8803158ba42f 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> @@ -41,3 +41,10 @@ Contact:    Jinlong Mao 
>>> <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>>>   Description:
>>>           (RW) Configure the CMB/MCMB channel mode for all enabled 
>>> ports.
>>>           Value 0 means raw channel mapping mode. Value 1 means 
>>> channel pair marking mode.
>>> +
>>> +What:        /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>>> +Date:        August 2025
>>> +KernelVersion:    6.17
>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>> +Description:
>>> +        (RW) Configure the bit i to requests a flush operation of 
>>> port i on the TPDA.
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>>> hwtracing/coresight/coresight-tpda.c
>>> index 9e623732d1e7..c5f169facc51 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct device 
>>> *dev,
>>>   }
>>>   static DEVICE_ATTR_RW(cmbchan_mode);
>>> +static ssize_t port_flush_req_show(struct device *dev,
>>> +                   struct device_attribute *attr,
>>> +                   char *buf)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (!drvdata->csdev->refcnt)
>>> +        return -EPERM;
>>> +
>>> +    val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>>> +    return sysfs_emit(buf, "%lx\n", val);
>>
>> Decimal would be better for a port number that goes from 0 - 127. If 
>> you really want to use hex then don't you need to prefix it with 0x? 
>> Otherwise you can't tell the difference between decimal 10 and hex 10, 
>> and it's not documented that it's hex either.
>>
> 
> Got it. I will fix the code here, and update the description in document.
> 
>>> +}
>>> +
>>> +static ssize_t port_flush_req_store(struct device *dev,
>>> +                    struct device_attribute *attr,
>>> +                    const char *buf,
>>> +                    size_t size)
>>> +{
>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>> +    unsigned long val;
>>> +
>>> +    if (kstrtoul(buf, 0, &val))
>>> +        return -EINVAL;
>>> +
>>> +    /* The valid value ranges from 0 to 127 */
>>> +    if (val > 127)
>>> +        return -EINVAL;
>>> +
>>> +    guard(spinlock)(&drvdata->spinlock);
>>> +    if (!drvdata->csdev->refcnt)
>>> +        return -EPERM;
>>> +
>>> +    if (val) {
>>
>> If 0 - 127 are valid don't you want to write 0 too?
> 
> It's 1-127 here. 0 may leads to an unexpected issue here.
> 
> Thanks,
> Jie
> 

Then can't the above be this:

   /* The valid value ranges from 1 to 127 */
   if (val < 1 || val > 127)
     return -EINVAL;

But I'm wondering how you flush port 0?

Isn't the default value 0? So if you never write to port_flush_req then 
you'd flush port 0, but why can't you change it back to 0 after writing 
a different value?

>>
>>> +        CS_UNLOCK(drvdata->base);
>>> +        writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>>> +        CS_LOCK(drvdata->base);
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +static DEVICE_ATTR_RW(port_flush_req);
>>> +
>>>   static struct attribute *tpda_attrs[] = {
>>>       &dev_attr_trig_async_enable.attr,
>>>       &dev_attr_trig_flag_ts_enable.attr,
>>> @@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
>>>       &dev_attr_freq_ts_enable.attr,
>>>       &dev_attr_global_flush_req.attr,
>>>       &dev_attr_cmbchan_mode.attr,
>>> +    &dev_attr_port_flush_req.attr,
>>>       NULL,
>>>   };
>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>>> hwtracing/coresight/coresight-tpda.h
>>> index 00d146960d81..55a18d718357 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>> @@ -10,6 +10,7 @@
>>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>>   #define TPDA_FPID_CR        (0x084)
>>>   #define TPDA_SYNCR        (0x08C)
>>> +#define TPDA_FLUSH_CR        (0x090)
>>>   /* Cross trigger FREQ packets timestamp bit */
>>>   #define TPDA_CR_FREQTS        BIT(2)
>>
>>
> 

Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[Jie Gan]

On 8/26/2025 5:54 PM, James Clark wrote:
> 
> 
> On 26/08/2025 10:39 am, Jie Gan wrote:
>>
>>
>> On 8/26/2025 5:27 PM, James Clark wrote:
>>>
>>>
>>> On 26/08/2025 8:01 am, Jie Gan wrote:
>>>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>>
>>>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>>>> for port i, forcing the data to synchronize and be transmitted to the
>>>> sink device.
>>>>
>>>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>> ---
>>>>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
>>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 45 +++++++++++++++ 
>>>> ++++
>>>>   drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
>>>>   3 files changed, 53 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>> index e827396a0fa1..8803158ba42f 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>> @@ -41,3 +41,10 @@ Contact:    Jinlong Mao 
>>>> <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>>>>   Description:
>>>>           (RW) Configure the CMB/MCMB channel mode for all enabled 
>>>> ports.
>>>>           Value 0 means raw channel mapping mode. Value 1 means 
>>>> channel pair marking mode.
>>>> +
>>>> +What:        /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>>>> +Date:        August 2025
>>>> +KernelVersion:    6.17
>>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>>> +Description:
>>>> +        (RW) Configure the bit i to requests a flush operation of 
>>>> port i on the TPDA.
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ 
>>>> hwtracing/coresight/coresight-tpda.c
>>>> index 9e623732d1e7..c5f169facc51 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>> @@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct device 
>>>> *dev,
>>>>   }
>>>>   static DEVICE_ATTR_RW(cmbchan_mode);
>>>> +static ssize_t port_flush_req_show(struct device *dev,
>>>> +                   struct device_attribute *attr,
>>>> +                   char *buf)
>>>> +{
>>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    unsigned long val;
>>>> +
>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>> +    if (!drvdata->csdev->refcnt)
>>>> +        return -EPERM;
>>>> +
>>>> +    val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>>>> +    return sysfs_emit(buf, "%lx\n", val);
>>>
>>> Decimal would be better for a port number that goes from 0 - 127. If 
>>> you really want to use hex then don't you need to prefix it with 0x? 
>>> Otherwise you can't tell the difference between decimal 10 and hex 
>>> 10, and it's not documented that it's hex either.
>>>
>>
>> Got it. I will fix the code here, and update the description in document.
>>
>>>> +}
>>>> +
>>>> +static ssize_t port_flush_req_store(struct device *dev,
>>>> +                    struct device_attribute *attr,
>>>> +                    const char *buf,
>>>> +                    size_t size)
>>>> +{
>>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>> +    unsigned long val;
>>>> +
>>>> +    if (kstrtoul(buf, 0, &val))
>>>> +        return -EINVAL;
>>>> +
>>>> +    /* The valid value ranges from 0 to 127 */
>>>> +    if (val > 127)
>>>> +        return -EINVAL;
>>>> +
>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>> +    if (!drvdata->csdev->refcnt)
>>>> +        return -EPERM;
>>>> +
>>>> +    if (val) {
>>>
>>> If 0 - 127 are valid don't you want to write 0 too?
>>
>> It's 1-127 here. 0 may leads to an unexpected issue here.
>>
>> Thanks,
>> Jie
>>
> 
> Then can't the above be this:
> 
>    /* The valid value ranges from 1 to 127 */
>    if (val < 1 || val > 127)
>      return -EINVAL;
> 
> But I'm wondering how you flush port 0?
> 

BIT(0) represents port 0 with value 1 and the default value 0 means 
nothing will be triggered here.

> Isn't the default value 0? So if you never write to port_flush_req then 
> you'd flush port 0, but why can't you change it back to 0 after writing 
> a different value?

We can change the value back to 0 but I think we shouldn't do this 
although I haven't suffer issue after I changed it back to 0(for bit).
Because the document mentioned: "Once set, the bit remains set until the 
flush operation on port i completes and the bit then clears to 0". So I 
think we should let the flush operation finish as expected and clear the 
bit by itself? Or may suffer unexpected error when try to interrupt the 
flush operation?

Thanks,
Jie
  >>>
>>>> +        CS_UNLOCK(drvdata->base);
>>>> +        writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>>>> +        CS_LOCK(drvdata->base);
>>>> +    }
>>>> +
>>>> +    return size;
>>>> +}
>>>> +static DEVICE_ATTR_RW(port_flush_req);
>>>> +
>>>>   static struct attribute *tpda_attrs[] = {
>>>>       &dev_attr_trig_async_enable.attr,
>>>>       &dev_attr_trig_flag_ts_enable.attr,
>>>> @@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
>>>>       &dev_attr_freq_ts_enable.attr,
>>>>       &dev_attr_global_flush_req.attr,
>>>>       &dev_attr_cmbchan_mode.attr,
>>>> +    &dev_attr_port_flush_req.attr,
>>>>       NULL,
>>>>   };
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/ 
>>>> hwtracing/coresight/coresight-tpda.h
>>>> index 00d146960d81..55a18d718357 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>>> @@ -10,6 +10,7 @@
>>>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>>>   #define TPDA_FPID_CR        (0x084)
>>>>   #define TPDA_SYNCR        (0x08C)
>>>> +#define TPDA_FLUSH_CR        (0x090)
>>>>   /* Cross trigger FREQ packets timestamp bit */
>>>>   #define TPDA_CR_FREQTS        BIT(2)
>>>
>>>
>>
> 
> 

Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[James Clark]

On 26/08/2025 1:11 pm, Jie Gan wrote:
> 
> 
> On 8/26/2025 5:54 PM, James Clark wrote:
>>
>>
>> On 26/08/2025 10:39 am, Jie Gan wrote:
>>>
>>>
>>> On 8/26/2025 5:27 PM, James Clark wrote:
>>>>
>>>>
>>>> On 26/08/2025 8:01 am, Jie Gan wrote:
>>>>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>>>
>>>>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>>>>> for port i, forcing the data to synchronize and be transmitted to the
>>>>> sink device.
>>>>>
>>>>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>>> ---
>>>>>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
>>>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 45 ++++++++++++++ 
>>>>> + ++++
>>>>>   drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
>>>>>   3 files changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>>>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>>> index e827396a0fa1..8803158ba42f 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>>> @@ -41,3 +41,10 @@ Contact:    Jinlong Mao 
>>>>> <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>>>>>   Description:
>>>>>           (RW) Configure the CMB/MCMB channel mode for all enabled 
>>>>> ports.
>>>>>           Value 0 means raw channel mapping mode. Value 1 means 
>>>>> channel pair marking mode.
>>>>> +
>>>>> +What:        /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>>>>> +Date:        August 2025
>>>>> +KernelVersion:    6.17
>>>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>>>> +Description:
>>>>> +        (RW) Configure the bit i to requests a flush operation of 
>>>>> port i on the TPDA.
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/ 
>>>>> drivers/ hwtracing/coresight/coresight-tpda.c
>>>>> index 9e623732d1e7..c5f169facc51 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>> @@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct 
>>>>> device *dev,
>>>>>   }
>>>>>   static DEVICE_ATTR_RW(cmbchan_mode);
>>>>> +static ssize_t port_flush_req_show(struct device *dev,
>>>>> +                   struct device_attribute *attr,
>>>>> +                   char *buf)
>>>>> +{
>>>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +    unsigned long val;
>>>>> +
>>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>>> +    if (!drvdata->csdev->refcnt)
>>>>> +        return -EPERM;
>>>>> +
>>>>> +    val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>>>>> +    return sysfs_emit(buf, "%lx\n", val);
>>>>
>>>> Decimal would be better for a port number that goes from 0 - 127. If 
>>>> you really want to use hex then don't you need to prefix it with 0x? 
>>>> Otherwise you can't tell the difference between decimal 10 and hex 
>>>> 10, and it's not documented that it's hex either.
>>>>
>>>
>>> Got it. I will fix the code here, and update the description in 
>>> document.
>>>
>>>>> +}
>>>>> +
>>>>> +static ssize_t port_flush_req_store(struct device *dev,
>>>>> +                    struct device_attribute *attr,
>>>>> +                    const char *buf,
>>>>> +                    size_t size)
>>>>> +{
>>>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>> +    unsigned long val;
>>>>> +
>>>>> +    if (kstrtoul(buf, 0, &val))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    /* The valid value ranges from 0 to 127 */
>>>>> +    if (val > 127)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>>> +    if (!drvdata->csdev->refcnt)
>>>>> +        return -EPERM;
>>>>> +
>>>>> +    if (val) {
>>>>
>>>> If 0 - 127 are valid don't you want to write 0 too?
>>>
>>> It's 1-127 here. 0 may leads to an unexpected issue here.
>>>
>>> Thanks,
>>> Jie
>>>
>>
>> Then can't the above be this:
>>
>>    /* The valid value ranges from 1 to 127 */
>>    if (val < 1 || val > 127)
>>      return -EINVAL;
>>
>> But I'm wondering how you flush port 0?
>>
> 
> BIT(0) represents port 0 with value 1 and the default value 0 means 
> nothing will be triggered here.
> 
>> Isn't the default value 0? So if you never write to port_flush_req 
>> then you'd flush port 0, but why can't you change it back to 0 after 
>> writing a different value?
> 
> We can change the value back to 0 but I think we shouldn't do this 
> although I haven't suffer issue after I changed it back to 0(for bit).
> Because the document mentioned: "Once set, the bit remains set until the 
> flush operation on port i completes and the bit then clears to 0". So I 
> think we should let the flush operation finish as expected and clear the 
> bit by itself? Or may suffer unexpected error when try to interrupt the 
> flush operation?
> 
> Thanks,
> Jie

Oh I see, I thought this was a port number, not a bit for each port. 
That changes this and my other comment about changing the output to be 
decimal then. Hex is probably better but it needs the 0x prefix.

I would also treat 0 as EINVAL. It doesn't do anything different to any 
other out of range request so it should be treated the same way.

Then comparing to 127 isn't that obvious either. Something like 
FIELD_FITS() more clearly states that values have to fit into a bitfield 
rather than be less than some value:

   if (!val || !FIELD_FIT(TPDA_FLUSH_CR_PORTNUM, val))
	return -EINVAL;


>   >>>
>>>>> +        CS_UNLOCK(drvdata->base);
>>>>> +        writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>>>>> +        CS_LOCK(drvdata->base);
>>>>> +    }
>>>>> +
>>>>> +    return size;
>>>>> +}
>>>>> +static DEVICE_ATTR_RW(port_flush_req);
>>>>> +
>>>>>   static struct attribute *tpda_attrs[] = {
>>>>>       &dev_attr_trig_async_enable.attr,
>>>>>       &dev_attr_trig_flag_ts_enable.attr,
>>>>> @@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
>>>>>       &dev_attr_freq_ts_enable.attr,
>>>>>       &dev_attr_global_flush_req.attr,
>>>>>       &dev_attr_cmbchan_mode.attr,
>>>>> +    &dev_attr_port_flush_req.attr,
>>>>>       NULL,
>>>>>   };
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/ 
>>>>> drivers/ hwtracing/coresight/coresight-tpda.h
>>>>> index 00d146960d81..55a18d718357 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>>>> @@ -10,6 +10,7 @@
>>>>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>>>>   #define TPDA_FPID_CR        (0x084)
>>>>>   #define TPDA_SYNCR        (0x08C)
>>>>> +#define TPDA_FLUSH_CR        (0x090)
>>>>>   /* Cross trigger FREQ packets timestamp bit */
>>>>>   #define TPDA_CR_FREQTS        BIT(2)
>>>>
>>>>
>>>
>>
>>
> 

Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port

[Jie Gan]

On 8/26/2025 9:29 PM, James Clark wrote:
> 
> 
> On 26/08/2025 1:11 pm, Jie Gan wrote:
>>
>>
>> On 8/26/2025 5:54 PM, James Clark wrote:
>>>
>>>
>>> On 26/08/2025 10:39 am, Jie Gan wrote:
>>>>
>>>>
>>>> On 8/26/2025 5:27 PM, James Clark wrote:
>>>>>
>>>>>
>>>>> On 26/08/2025 8:01 am, Jie Gan wrote:
>>>>>> From: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>>>>
>>>>>> Setting bit i in the TPDA_FLUSH_CR register initiates a flush request
>>>>>> for port i, forcing the data to synchronize and be transmitted to the
>>>>>> sink device.
>>>>>>
>>>>>> Signed-off-by: Tao Zhang <tao.zhang@oss.qualcomm.com>
>>>>>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>>>> ---
>>>>>>   .../testing/sysfs-bus-coresight-devices-tpda  |  7 +++
>>>>>>   drivers/hwtracing/coresight/coresight-tpda.c  | 45 +++++++++++++ 
>>>>>> + + ++++
>>>>>>   drivers/hwtracing/coresight/coresight-tpda.h  |  1 +
>>>>>>   3 files changed, 53 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight- 
>>>>>> devices- tpda b/Documentation/ABI/testing/sysfs-bus-coresight- 
>>>>>> devices-tpda
>>>>>> index e827396a0fa1..8803158ba42f 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>>>>> @@ -41,3 +41,10 @@ Contact:    Jinlong Mao 
>>>>>> <jinlong.mao@oss.qualcomm.com>, Tao Zhang <tao.zhang@oss.qu
>>>>>>   Description:
>>>>>>           (RW) Configure the CMB/MCMB channel mode for all enabled 
>>>>>> ports.
>>>>>>           Value 0 means raw channel mapping mode. Value 1 means 
>>>>>> channel pair marking mode.
>>>>>> +
>>>>>> +What:        /sys/bus/coresight/devices/<tpda-name>/port_flush_req
>>>>>> +Date:        August 2025
>>>>>> +KernelVersion:    6.17
>>>>>> +Contact:    Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Tao Zhang 
>>>>>> <tao.zhang@oss.qualcomm.com>, Jie Gan <jie.gan@oss.qualcomm.com>
>>>>>> +Description:
>>>>>> +        (RW) Configure the bit i to requests a flush operation of 
>>>>>> port i on the TPDA.
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/ 
>>>>>> drivers/ hwtracing/coresight/coresight-tpda.c
>>>>>> index 9e623732d1e7..c5f169facc51 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>>>>> @@ -509,6 +509,50 @@ static ssize_t cmbchan_mode_store(struct 
>>>>>> device *dev,
>>>>>>   }
>>>>>>   static DEVICE_ATTR_RW(cmbchan_mode);
>>>>>> +static ssize_t port_flush_req_show(struct device *dev,
>>>>>> +                   struct device_attribute *attr,
>>>>>> +                   char *buf)
>>>>>> +{
>>>>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +    unsigned long val;
>>>>>> +
>>>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>>>> +    if (!drvdata->csdev->refcnt)
>>>>>> +        return -EPERM;
>>>>>> +
>>>>>> +    val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>>>>>> +    return sysfs_emit(buf, "%lx\n", val);
>>>>>
>>>>> Decimal would be better for a port number that goes from 0 - 127. 
>>>>> If you really want to use hex then don't you need to prefix it with 
>>>>> 0x? Otherwise you can't tell the difference between decimal 10 and 
>>>>> hex 10, and it's not documented that it's hex either.
>>>>>
>>>>
>>>> Got it. I will fix the code here, and update the description in 
>>>> document.
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t port_flush_req_store(struct device *dev,
>>>>>> +                    struct device_attribute *attr,
>>>>>> +                    const char *buf,
>>>>>> +                    size_t size)
>>>>>> +{
>>>>>> +    struct tpda_drvdata *drvdata = dev_get_drvdata(dev->parent);
>>>>>> +    unsigned long val;
>>>>>> +
>>>>>> +    if (kstrtoul(buf, 0, &val))
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    /* The valid value ranges from 0 to 127 */
>>>>>> +    if (val > 127)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    guard(spinlock)(&drvdata->spinlock);
>>>>>> +    if (!drvdata->csdev->refcnt)
>>>>>> +        return -EPERM;
>>>>>> +
>>>>>> +    if (val) {
>>>>>
>>>>> If 0 - 127 are valid don't you want to write 0 too?
>>>>
>>>> It's 1-127 here. 0 may leads to an unexpected issue here.
>>>>
>>>> Thanks,
>>>> Jie
>>>>
>>>
>>> Then can't the above be this:
>>>
>>>    /* The valid value ranges from 1 to 127 */
>>>    if (val < 1 || val > 127)
>>>      return -EINVAL;
>>>
>>> But I'm wondering how you flush port 0?
>>>
>>
>> BIT(0) represents port 0 with value 1 and the default value 0 means 
>> nothing will be triggered here.
>>
>>> Isn't the default value 0? So if you never write to port_flush_req 
>>> then you'd flush port 0, but why can't you change it back to 0 after 
>>> writing a different value?
>>
>> We can change the value back to 0 but I think we shouldn't do this 
>> although I haven't suffer issue after I changed it back to 0(for bit).
>> Because the document mentioned: "Once set, the bit remains set until 
>> the flush operation on port i completes and the bit then clears to 0". 
>> So I think we should let the flush operation finish as expected and 
>> clear the bit by itself? Or may suffer unexpected error when try to 
>> interrupt the flush operation?
>>
>> Thanks,
>> Jie
> 
> Oh I see, I thought this was a port number, not a bit for each port. 
> That changes this and my other comment about changing the output to be 
> decimal then. Hex is probably better but it needs the 0x prefix.
> 
> I would also treat 0 as EINVAL. It doesn't do anything different to any 
> other out of range request so it should be treated the same way.
> 
> Then comparing to 127 isn't that obvious either. Something like 
> FIELD_FITS() more clearly states that values have to fit into a bitfield 
> rather than be less than some value:
> 
>    if (!val || !FIELD_FIT(TPDA_FLUSH_CR_PORTNUM, val))
>      return -EINVAL;

I found I made a mistake here for value range. 0-127 is for port 0 to 
port 6. But the TPDA device could support up to 32 ports, means u32 here.

So the mask here, the TPDA_FLUSH_CR_PORTNUM, should be designed for 32 
bits, like 0xffffffff.

Thanks,
Jie

> 
> 
>>   >>>
>>>>>> +        CS_UNLOCK(drvdata->base);
>>>>>> +        writel_relaxed(val, drvdata->base + TPDA_FLUSH_CR);
>>>>>> +        CS_LOCK(drvdata->base);
>>>>>> +    }
>>>>>> +
>>>>>> +    return size;
>>>>>> +}
>>>>>> +static DEVICE_ATTR_RW(port_flush_req);
>>>>>> +
>>>>>>   static struct attribute *tpda_attrs[] = {
>>>>>>       &dev_attr_trig_async_enable.attr,
>>>>>>       &dev_attr_trig_flag_ts_enable.attr,
>>>>>> @@ -516,6 +560,7 @@ static struct attribute *tpda_attrs[] = {
>>>>>>       &dev_attr_freq_ts_enable.attr,
>>>>>>       &dev_attr_global_flush_req.attr,
>>>>>>       &dev_attr_cmbchan_mode.attr,
>>>>>> +    &dev_attr_port_flush_req.attr,
>>>>>>       NULL,
>>>>>>   };
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/ 
>>>>>> drivers/ hwtracing/coresight/coresight-tpda.h
>>>>>> index 00d146960d81..55a18d718357 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-tpda.h
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.h
>>>>>> @@ -10,6 +10,7 @@
>>>>>>   #define TPDA_Pn_CR(n)        (0x004 + (n * 4))
>>>>>>   #define TPDA_FPID_CR        (0x084)
>>>>>>   #define TPDA_SYNCR        (0x08C)
>>>>>> +#define TPDA_FLUSH_CR        (0x090)
>>>>>>   /* Cross trigger FREQ packets timestamp bit */
>>>>>>   #define TPDA_CR_FREQTS        BIT(2)
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>