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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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 ...
}


Suzuki