This patch series adds support for the Qualcomm CoreSight Interconnect TNOC
(Trace Network On Chip) block, which acts as a CoreSight graph link forwarding
trace data from subsystems to the Aggregator TNOC. Unlike the Aggregator TNOC,
this block does not support aggregation or ATID assignment.
Signed-off-by: Yuanfang Zhang <yuanfang.zhang(a)oss.qualcomm.com>
---
Changes in v3:
- Add detail for changes in V2.
- Remove '#address-cells' and '#size-cells' properties from in-ports field.
- Fix comment indentation for packet description.
- Link to v2: https://lore.kernel.org/r/20250819-itnoc-v2-0-2d0e6be44e2f@oss.qualcomm.com
Changes in v2:
- Removed the trailing '|' after the description in qcom,coresight-itnoc.yaml.
- Dropped the 'select' section from the YAML file.
- Updated node name to use a more generic naming convention.
- Removed the 'items' property from the compatible field.
- Deleted the description for the reg property.
- Dropped clock-names and adjusted the order of clock-names and clocks.
- Moved additionalProperties to follow the $ref of out-ports.
- Change "atid" type from u32 to int, set it as "-EOPNOTSUPP" for non-AMBA device.
- Link to v1: https://lore.kernel.org/r/20250815-itnoc-v1-0-62c8e4f7ad32@oss.qualcomm.com
---
Yuanfang Zhang (3):
dt-bindings: arm: qcom: Add Coresight Interconnect TNOC
coresight-tnoc: add platform driver to support Interconnect TNOC
coresight-tnoc: Add runtime PM support for Interconnect TNOC
.../bindings/arm/qcom,coresight-itnoc.yaml | 90 +++++++++++++
drivers/hwtracing/coresight/coresight-tnoc.c | 139 +++++++++++++++++++--
2 files changed, 216 insertions(+), 13 deletions(-)
---
base-commit: 2b52cf338d39d684a1c6af298e8204902c026aca
change-id: 20250815-itnoc-460273d1b80c
Best regards,
--
Yuanfang Zhang <yuanfang.zhang(a)oss.qualcomm.com>
On 27/08/2025 11:55 am, Jie Gan wrote:
> Patchset 1 introduces configuration of the cross-trigger registers with
> appropriate values to enable proper generation of cross-trigger packets.
>
> Patchset 2 introduces a logic 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.
>
> Changes in V3:
> 1. Optimizing codes according to James's comment.
> Link to V2 - https://lore.kernel.org/all/20250827042042.6786-1-jie.gan@oss.qualcomm.com/
>
> Changes in V2:
> 1. Refactoring the code based on James's comment for optimization.
> Link to V1 - https://lore.kernel.org/all/20250826070150.5603-1-jie.gan@oss.qualcomm.com/
>
> Tao Zhang (3):
> coresight: tpda: add sysfs nodes for tpda cross-trigger configuration
> coresight: tpda: add logic 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 | 278 ++++++++++++++++++
> drivers/hwtracing/coresight/coresight-tpda.h | 33 ++-
> 3 files changed, 360 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>
Reviewed-by: James Clark <james.clark(a)linaro.org>
On 27/08/2025 10:48 am, Jie Gan wrote:
>
>
> On 8/27/2025 5:17 PM, James Clark wrote:
>>
>>
>> On 27/08/2025 5:20 am, Jie Gan wrote:
>>> From: Tao Zhang <tao.zhang(a)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(a)oss.qualcomm.com>
>>> Co-developed-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
>>> ---
>>> .../testing/sysfs-bus-coresight-devices-tpda | 7 ++++
>>> drivers/hwtracing/coresight/coresight-tpda.c | 42 +++++++++++++++++++
>>> drivers/hwtracing/coresight/coresight-tpda.h | 2 +
>>> 3 files changed, 51 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-
>>> tpda b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tpda
>>> index fb651aebeb31..2cf2dcfc13c8 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(a)oss.qualcomm.com>, Tao Zhang <tao.zhang(a)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(a)oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang(a)oss.qualcomm.com>, Jie Gan <jie.gan(a)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 430f76c559f2..8b1fe128881d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
>>> @@ -487,6 +487,47 @@ 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;
>>> +
>>> + if (!drvdata->csdev->refcnt)
>>> + return -EINVAL;
>>> +
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + CS_UNLOCK(drvdata->base);
>>> + val = readl_relaxed(drvdata->base + TPDA_FLUSH_CR);
>>> + CS_LOCK(drvdata->base);
>>> + return sysfs_emit(buf, "%lx\n", val);
>>
>> Still missing the 0x prefix
>
> Will re-check rest of the codes and add prefix. Sorry I missed it during
> the review process.
>
>>
>>> +}
>>> +
>>> +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;
>>> +
>>> + if (!drvdata->csdev->refcnt || !val)
>>> + return -EINVAL;
>>> +
>>> + val |= FIELD_PREP(TPDA_MAX_INPORTS_MASK, val);
>>
>> Using FIELD_PREP() now that it's the full width of the register makes
>> less sense. Especially when there is no corresponding FIELD_FIT()
>> check, which is fine because everything always fits. But if you
>> didn't know the mask was the full width you'd wonder if the check is
>> missing.
>>
>> I would just write val directly to TPDA_FLUSH_CR so it's simpler.
>>
>> It should also have been val = FIELD_PREP(...)
>
> Yeah, it should have been val = FIELD_PREP(...) here... sorry for the
> mistake here..
>
> I was thinking the unsigned long here could be 64 or 32 bits and we only
> need the value of the lower 32 bits. So that's why I am using val =
> FIELD_PREP(...) here. We shouldn't write a value greater than UINT32_MAX
> to the register.
>
> Thanks,
> Jie
>
writel_relaxed() is always 32 bits though so it is a bit confusing if
you truncate the user value without an error. Also a reason to use u32
instead of unsigned long types.
Are you trying to support arm and arm64 with tpda? Or just arm64? For it
to be consistent you can use kstrtou32(), or use kstrtoull() and then
FIELD_FIT() to error on truncation. kstrtou32() is probably the cleanest.
>>
>>> + guard(spinlock)(&drvdata->spinlock);
>>> + 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,
>>> @@ -494,6 +535,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 8e1b66115ad1..56d3ad293e46 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)
>>> @@ -35,6 +36,7 @@
>>> #define TPDA_SYNCR_MAX_COUNTER_VAL (0xFFF)
>>> #define TPDA_MAX_INPORTS 32
>>> +#define TPDA_MAX_INPORTS_MASK GENMASK(31, 0)
>>> /* Bits 6 ~ 12 is for atid value */
>>> #define TPDA_CR_ATID GENMASK(12, 6)
>>
>>
>
On 27/08/2025 5:20 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang(a)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(a)oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
> ---
> drivers/hwtracing/coresight/coresight-tpda.c | 7 +++++++
> drivers/hwtracing/coresight/coresight-tpda.h | 6 ++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
> index 647ab49a98d7..430f76c559f2 100644
> --- a/drivers/hwtracing/coresight/coresight-tpda.c
> +++ b/drivers/hwtracing/coresight/coresight-tpda.c
> @@ -187,6 +187,13 @@ static void tpda_enable_pre_port(struct tpda_drvdata *drvdata)
> */
> if (drvdata->trig_flag_ts)
> writel_relaxed(0x0, drvdata->base + TPDA_FPID_CR);
> +
> + /* Program the counter value for TPDA_SYNCR */
> + val = readl_relaxed(drvdata->base + TPDA_SYNCR);
> + /* Clear the mode */
> + val &= ~TPDA_SYNCR_MODE_CTRL;
> + val |= FIELD_PREP(TPDA_SYNCR_COUNTER_MASK, TPDA_SYNCR_MAX_COUNTER_VAL);
Just use the mask directly if you want to set all the bits. This makes
it seem like the MAX_COUNTER_VAL is something different.
val |= TPDA_SYNCR_COUNTER_MASK
> + writel_relaxed(val, drvdata->base + TPDA_SYNCR);
> }
>
> static int tpda_enable_port(struct tpda_drvdata *drvdata, int port)
> diff --git a/drivers/hwtracing/coresight/coresight-tpda.h b/drivers/hwtracing/coresight/coresight-tpda.h
> index 0be625fb52fd..8e1b66115ad1 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)
> @@ -27,6 +28,11 @@
> #define TPDA_Pn_CR_CMBSIZE GENMASK(7, 6)
> /* Aggregator port DSB data set element size bit */
> #define TPDA_Pn_CR_DSBSIZE BIT(8)
> +/* TPDA_SYNCR mode control bit */
> +#define TPDA_SYNCR_MODE_CTRL BIT(12)
> +/* TPDA_SYNCR counter mask */
> +#define TPDA_SYNCR_COUNTER_MASK GENMASK(11, 0)
> +#define TPDA_SYNCR_MAX_COUNTER_VAL (0xFFF)
No need to define a numeric value that's the same as the mask. It also
opens the possibility of making a mistake.
>
> #define TPDA_MAX_INPORTS 32
>
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(a)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(a)oss.qualcomm.com>
>>>>> Co-developed-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
>>>>> Signed-off-by: Jie Gan <jie.gan(a)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(a)oss.qualcomm.com>, Tao Zhang <tao.zhang(a)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(a)oss.qualcomm.com>, Tao Zhang
>>>>> <tao.zhang(a)oss.qualcomm.com>, Jie Gan <jie.gan(a)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)
>>>>
>>>>
>>>
>>
>>
>
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(a)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(a)oss.qualcomm.com>
>>> Co-developed-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
>>> Signed-off-by: Jie Gan <jie.gan(a)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(a)oss.qualcomm.com>, Tao Zhang <tao.zhang(a)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(a)oss.qualcomm.com>, Tao Zhang
>>> <tao.zhang(a)oss.qualcomm.com>, Jie Gan <jie.gan(a)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)
>>
>>
>
On 26/08/2025 8:01 am, Jie Gan wrote:
> From: Tao Zhang <tao.zhang(a)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(a)oss.qualcomm.com>
> Co-developed-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan(a)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)