Re: [PATCH v14 6/7] coresight: ctcu: enable byte-cntr for TMC ETR devices

[Jie Gan <jie.gan@oss.qualcomm.com>]

On 3/10/2026 5:15 PM, Suzuki K Poulose wrote:
> On 10/03/2026 03:01, Jie Gan wrote:
>>
>>
>> On 3/9/2026 8:43 PM, Suzuki K Poulose wrote:
>>> On 09/03/2026 09:47, Jie Gan wrote:
>>>> The byte-cntr function provided by the CTCU device is used to 
>>>> transfer data
>>>> from the ETR buffer to the userspace. An interrupt is triggered if 
>>>> the data
>>>> size exceeds the threshold set in the BYTECNTRVAL register. The 
>>>> interrupt
>>>> handler counts the number of triggered interruptions and the read 
>>>> function
>>>> will read the data from the synced ETR buffer.
>>>>
>>>> Switching the sysfs_buf when current buffer is full or the timeout is
>>>> triggered and resets rrp and rwp registers after switched the buffer.
>>>> The synced buffer will become available for reading after the switch.
>>>>
>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>>>> ---
>>>>   .../ABI/testing/sysfs-bus-coresight-devices-ctcu   |   8 +
>>>>   drivers/hwtracing/coresight/Makefile               |   2 +-
>>>>   .../hwtracing/coresight/coresight-ctcu-byte-cntr.c | 351 +++++++++ 
>>>> + + ++++++++++
>>>>   drivers/hwtracing/coresight/coresight-ctcu-core.c  | 103 +++++-
>>>>   drivers/hwtracing/coresight/coresight-ctcu.h       |  76 ++++-
>>>>   drivers/hwtracing/coresight/coresight-tmc-core.c   |   8 +-
>>>>   drivers/hwtracing/coresight/coresight-tmc-etr.c    |  18 ++
>>>>   drivers/hwtracing/coresight/coresight-tmc.h        |   4 +
>>>>   8 files changed, 555 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices- 
>>>> ctcu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-ctcu
>>>> new file mode 100644
>>>> index 000000000000..6ff1708fb944
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-ctcu
>>>> @@ -0,0 +1,8 @@
>>>> +What:           /sys/bus/coresight/devices/<ctcu-name>/ 
>>>> irq_threshold[0:1]
>>>> +Date:           March 2026
>>>> +KernelVersion:  7.1
>>>> +Contact:        Tingwei Zhang <tingwei.zhang@oss.qualcomm.com>; 
>>>> Jinlong Mao <jinlong.mao@oss.qualcomm.com>; Jie Gan 
>>>> <jie.gan@oss.qualcomm.com>
>>>> +Description:
>>>> +        (RW) Configure the byte-cntr IRQ register for the specified 
>>>> ETR device
>>>> +        based on its port number. An interrupt is generated when 
>>>> the data size
>>>> +        exceeds the value set in the IRQ register.
>>>> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/ 
>>>> hwtracing/ coresight/Makefile
>>>> index ab16d06783a5..821a1b06b20c 100644
>>>> --- a/drivers/hwtracing/coresight/Makefile
>>>> +++ b/drivers/hwtracing/coresight/Makefile
>>>> @@ -55,5 +55,5 @@ coresight-cti-y := coresight-cti-core.o coresight- 
>>>> cti-platform.o \
>>>>   obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
>>>>   obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
>>>>   obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
>>>> -coresight-ctcu-y := coresight-ctcu-core.o
>>>> +coresight-ctcu-y := coresight-ctcu-core.o coresight-ctcu-byte-cntr.o
>>>>   obj-$(CONFIG_CORESIGHT_KUNIT_TESTS) += coresight-kunit-tests.o
>>>> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c 
>>>> b/ drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>>> new file mode 100644
>>>> index 000000000000..0bf738d6c283
>>>> --- /dev/null
>>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
>>>> @@ -0,0 +1,351 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>>>> + */
>>>> +
>>>> +#include <linux/coresight.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/fs.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/uaccess.h>
>>>> +
>>>> +#include "coresight-ctcu.h"
>>>> +#include "coresight-priv.h"
>>>> +#include "coresight-tmc.h"
>>>> +
>>>> +static irqreturn_t byte_cntr_handler(int irq, void *data)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data = (struct ctcu_byte_cntr 
>>>> *)data;
>>>> +
>>>> +    atomic_inc(&byte_cntr_data->irq_cnt);
>>>> +    wake_up(&byte_cntr_data->wq);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static void ctcu_reset_sysfs_buf(struct tmc_drvdata *drvdata)
>>>
>>> minor nit: This has nothing to do with the CTCU. For what it is worth,
>>> it must be called, tmc_etr_reset_sysf_buf(). But more on this below,
>>> and even do we need it, further below.
>>>
>>>> +{
>>>> +    u32 sts;
>>>> +
>>>> +    CS_UNLOCK(drvdata->base);
>>>> +    tmc_write_rrp(drvdata, drvdata->sysfs_buf->hwaddr);
>>>> +    tmc_write_rwp(drvdata, drvdata->sysfs_buf->hwaddr);
>>>> +    sts = readl_relaxed(drvdata->base + TMC_STS) & ~TMC_STS_FULL;
>>>> +    writel_relaxed(sts, drvdata->base + TMC_STS);
>>>> +    CS_LOCK(drvdata->base);
>>>
>>> Could we not keep this function in the tmc-etr.c and invoke from here ?
>>>
>>
>> Sure, will move the function tmc-etr.c
>>
>>>> +}
>>>> +
>>>> +static void ctcu_cfg_byte_cntr_reg(struct tmc_drvdata *drvdata, u32 
>>>> val, u32 offset)
>>>> +{
>>>> +    struct ctcu_drvdata *ctcu_drvdata;
>>>> +    struct coresight_device *helper;
>>>> +
>>>> +    helper = tmc_etr_get_ctcu_device(drvdata);
>>>> +    if (!helper)
>>>> +        return;
>>>> +
>>>> +    ctcu_drvdata = dev_get_drvdata(helper->dev.parent);
>>>> +    /* A one value for IRQCTRL register represents 8 bytes */
>>>> +    ctcu_program_register(ctcu_drvdata, val / 8, offset);
>>>> +}
>>>> +
>>>> +static struct ctcu_byte_cntr *ctcu_get_byte_cntr_data(struct 
>>>> tmc_drvdata *drvdata)
>>>> +{
>>>> +    struct ctcu_byte_cntr *byte_cntr_data;
>>>> +    struct ctcu_drvdata *ctcu_drvdata;
>>>> +    struct coresight_device *helper;
>>>> +    int port;
>>>> +
>>>> +    helper = tmc_etr_get_ctcu_device(drvdata);
>>>> +    if (!helper)
>>>> +        return NULL;
>>>> +
>>>
>>>
>>>
>>>> +    port = coresight_get_in_port(drvdata->csdev, helper);
>>>> +    if (port < 0)
>>>> +        return NULL;
>>>> +
>>>
>>> Please validate that the port_num you get is valid for the CTCU ? 
>>> That applies to all uses of this construct.
>>>
>>
>> Will validate it before using.
>>
>>>> +    ctcu_drvdata = dev_get_drvdata(helper->dev.parent);
>>>> +    byte_cntr_data = &ctcu_drvdata->byte_cntr_data[port];
>>>> +    return byte_cntr_data;
>>>
>>>
>>>
>>> nit:
>>>      return  &ctcu_drvdata->byte_cntr_data[port]; ?
>>>
>>> Also, why not make this into a helper, as we seem to use this other 
>>> places too ?
>>>
>>
>> Didnt get the point here. We may run more than one ETR devices 
>> concurrently. So we should get the proper byte_cntr_data according to 
>> the port number at runtime.
>>
> 
> 
> static struct ctcu_byte_cntr *ctcu_byte_cntr(struct coresight_device 
> *cctcu_dev, struct coresight_device *tmc_etr, ) {
> 
>      port = coresight_get_in_port()..
>      // Verify the port in this helper and everyone uses this.
>      if (//!validate_port//)
>          return NULL
>      return ...
> }

Got the point now. I have missed the instance 2/instance 3 you have 
mentioned in previous message.

Thanks,
Jie

> 
> 
> Suzuki
> 
>