Re: [PATCH v1 3/3] coresight: tpda: add sysfs node to flush specific port
From: Jie Gan
Date: Tue Aug 26 2025 - 20:50:59 EST
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@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxx>
Co-developed-by: Jie Gan <jie.gan@xxxxxxxxxxxxxxxx>
Signed-off-by: Jie Gan <jie.gan@xxxxxxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxxxxxx>, Tao Zhang <tao.zhang@xxxxxx
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@xxxxxxxxxxxxxxxx>, Tao Zhang
<tao.zhang@xxxxxxxxxxxxxxxx>, Jie Gan <jie.gan@xxxxxxxxxxxxxxxx>
+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)