The Coresight TMC Control Unit hosts miscellaneous configuration registers
which control various features related to TMC ETR sink.

Based on the trace ID, which is programmed in the related CTCU ATID
register of a specific ETR, trace data with that trace ID gets into
the ETR buffer, while other trace data gets dropped.

Enabling source device sets one bit of the ATID register based on
source device's trace ID.
Disabling source device resets the bit according to the source
device's trace ID.

Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
---
 drivers/hwtracing/coresight/Kconfig          |  12 +
 drivers/hwtracing/coresight/Makefile         |   1 +
 drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
 include/linux/coresight.h                    |   3 +-
 5 files changed, 321 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
 create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 06f0a7594169..ecd7086a5b83 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -133,6 +133,18 @@ config CORESIGHT_STM
 	  To compile this driver as a module, choose M here: the
 	  module will be called coresight-stm.
 
+config CORESIGHT_CTCU
+	tristate "CoreSight TMC Control Unit driver"
+	depends on CORESIGHT_LINK_AND_SINK_TMC
+	help
+	  This driver provides support for CoreSight TMC Control Unit
+	  that hosts miscellaneous configuration registers. This is
+	  primarily used for controlling the behaviors of the TMC
+	  ETR device.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-ctcu.
+
 config CORESIGHT_CPU_DEBUG
 	tristate "CoreSight CPU Debug driver"
 	depends on ARM || ARM64
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 4ba478211b31..1b7869910a12 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -51,3 +51,4 @@ coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
 		   coresight-cti-sysfs.o
 obj-$(CONFIG_ULTRASOC_SMB) += ultrasoc-smb.o
 obj-$(CONFIG_CORESIGHT_DUMMY) += coresight-dummy.o
+obj-$(CONFIG_CORESIGHT_CTCU) += coresight-ctcu.o
diff --git a/drivers/hwtracing/coresight/coresight-ctcu.c b/drivers/hwtracing/coresight/coresight-ctcu.c
new file mode 100644
index 000000000000..1b7413dcf325
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-ctcu.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/coresight.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "coresight-ctcu.h"
+#include "coresight-priv.h"
+
+DEFINE_CORESIGHT_DEVLIST(ctcu_devs, "ctcu");
+
+#define ctcu_writel(drvdata, val, offset)	__raw_writel((val), drvdata->base + offset)
+#define ctcu_readl(drvdata, offset)		__raw_readl(drvdata->base + offset)
+
+/*
+ * The TMC Coresight Control Unit uses four ATID registers to control the data
+ * filter function based on the trace ID for each TMC ETR sink. The length of
+ * each ATID register is 32 bits. Therefore, the ETR has a related field in
+ * CTCU that is 128 bits long. Each trace ID is represented by one bit in that
+ * filed.
+ * e.g. ETR0ATID0 layout, set bit 5 for traceid 5
+ *                                           bit5
+ * ------------------------------------------------------
+ * |   |28|   |24|   |20|   |16|   |12|   |8|  1|4|   |0|
+ * ------------------------------------------------------
+ *
+ * e.g. ETR0:
+ * 127                     0 from ATID_offset for ETR0ATID0
+ * -------------------------
+ * |ATID3|ATID2|ATID1|ATID0|
+ */
+#define CTCU_ATID_REG_OFFSET(traceid, atid_offset) \
+		((traceid / 32) * 4 + atid_offset)
+
+#define CTCU_ATID_REG_BIT(traceid)	(traceid % 32)
+#define CTCU_ATID_REG_SIZE		0x10
+
+struct ctcu_atid_config {
+	const u32 atid_offset;
+	const u32 port_num;
+};
+
+struct ctcu_config {
+	const struct ctcu_atid_config *atid_config;
+	int num_atid_config;
+};
+
+static const struct ctcu_atid_config sa8775p_atid_cfgs[] = {
+	{0xf8,  0},
+	{0x108, 1},
+};
+
+static const struct ctcu_config sa8775p_cfgs = {
+	.atid_config		= sa8775p_atid_cfgs,
+	.num_atid_config	= ARRAY_SIZE(sa8775p_atid_cfgs),
+};
+
+/*
+ * __ctcu_set_etr_traceid: Set bit in the ATID register based on trace ID when enable is true.
+ * Reset the bit of the ATID register based on trace ID when enable is false.
+ *
+ * @csdev:	coresight_device struct related to the device
+ * @traceid:	trace ID of the source tracer.
+ * @enable:	True for set bit and false for reset bit.
+ *
+ * Returns 0 indicates success. Non-zero result means failure.
+ */
+static int __ctcu_set_etr_traceid(struct coresight_device *csdev, u8 traceid, int port_num,
+				  bool enable)
+{
+	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	u32 atid_offset, reg_offset, val;
+	int bit;
+
+	atid_offset = drvdata->atid_offset[port_num];
+	if (atid_offset == 0)
+		return -EINVAL;
+
+	bit = CTCU_ATID_REG_BIT(traceid);
+	reg_offset = CTCU_ATID_REG_OFFSET(traceid, atid_offset);
+	if (reg_offset - atid_offset > CTCU_ATID_REG_SIZE) {
+		return -EINVAL;
+	}
+
+	guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
+	CS_UNLOCK(drvdata->base);
+	val = ctcu_readl(drvdata, reg_offset);
+	if (enable)
+		val = val | BIT(bit);
+	else
+		val = val & ~BIT(bit);
+
+	ctcu_writel(drvdata, val, reg_offset);
+	CS_LOCK(drvdata->base);
+
+	return 0;
+}
+
+static int ctcu_get_active_port(struct coresight_device *sink, struct coresight_device *helper)
+{
+	int i;
+
+	for (i = 0; i < sink->pdata->nr_outconns; ++i) {
+		if (sink->pdata->out_conns[i]->dest_dev)
+			return sink->pdata->out_conns[i]->dest_port;
+	}
+
+	return -EINVAL;
+}
+
+/*
+ * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
+ *
+ * Returns 0 indicates success. None-zero result means failure.
+ */
+static int ctcu_set_etr_traceid(struct coresight_device *csdev, struct coresight_path *cs_path,
+				bool enable)
+{
+	struct coresight_device *sink = coresight_get_sink(cs_path->path);
+	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	u8 trace_id = cs_path->trace_id;
+	int port_num;
+
+	if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || IS_ERR_OR_NULL(drvdata)) {
+		dev_err(&csdev->dev, "Invalid parameters\n");
+		return -EINVAL;
+	}
+
+	port_num = ctcu_get_active_port(sink, csdev);
+	if (port_num < 0)
+		return -EINVAL;
+
+	/*
+	 * Skip the disable session if more than one TPDM device that
+	 * connected to the same TPDA device has been enabled.
+	 */
+	if (enable)
+		atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
+	else {
+		if (atomic_dec_return(&drvdata->traceid_refcnt[port_num][trace_id]) > 0) {
+			dev_dbg(&csdev->dev, "Skip the disable session\n");
+			return 0;
+		}
+	}
+
+	dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
+
+	return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
+}
+
+static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode,
+		       void *data)
+{
+	struct coresight_path *cs_path = (struct coresight_path *)data;
+
+	return ctcu_set_etr_traceid(csdev, cs_path, true);
+}
+
+static int ctcu_disable(struct coresight_device *csdev, void *data)
+{
+	struct coresight_path *cs_path = (struct coresight_path *)data;
+
+	return ctcu_set_etr_traceid(csdev, cs_path, false);
+}
+
+static const struct coresight_ops_helper ctcu_helper_ops = {
+	.enable = ctcu_enable,
+	.disable = ctcu_disable,
+};
+
+static const struct coresight_ops ctcu_ops = {
+	.helper_ops = &ctcu_helper_ops,
+};
+
+static int ctcu_probe(struct platform_device *pdev)
+{
+	int i;
+	void __iomem *base;
+	struct device *dev = &pdev->dev;
+	struct coresight_platform_data *pdata;
+	struct ctcu_drvdata *drvdata;
+	struct coresight_desc desc = { 0 };
+	const struct ctcu_config *cfgs;
+	const struct ctcu_atid_config *atid_cfg;
+
+	desc.name = coresight_alloc_device_name(&ctcu_devs, dev);
+	if (!desc.name)
+		return -ENOMEM;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	pdata = coresight_get_platform_data(dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+	dev->platform_data = pdata;
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (!base)
+		return -ENOMEM;
+
+	drvdata->apb_clk = coresight_get_enable_apb_pclk(dev);
+	if (IS_ERR(drvdata->apb_clk))
+		return -ENODEV;
+
+	cfgs = of_device_get_match_data(dev);
+	if (cfgs) {
+		if (cfgs->num_atid_config <= ATID_MAX_NUM) {
+			for (i = 0; i < cfgs->num_atid_config; i++) {
+				atid_cfg = &cfgs->atid_config[i];
+				drvdata->atid_offset[i] = atid_cfg->atid_offset;
+			}
+		}
+	}
+
+	drvdata->base = base;
+	drvdata->dev = dev;
+	platform_set_drvdata(pdev, drvdata);
+
+	desc.type = CORESIGHT_DEV_TYPE_HELPER;
+	desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU;
+	desc.pdata = pdata;
+	desc.dev = dev;
+	desc.ops = &ctcu_ops;
+
+	drvdata->csdev = coresight_register(&desc);
+	if (IS_ERR(drvdata->csdev)) {
+		if (!IS_ERR_OR_NULL(drvdata->apb_clk))
+			clk_put(drvdata->apb_clk);
+
+		return PTR_ERR(drvdata->csdev);
+	}
+
+	return 0;
+}
+
+static void ctcu_remove(struct platform_device *pdev)
+{
+	struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
+
+	coresight_unregister(drvdata->csdev);
+	if (!IS_ERR_OR_NULL(drvdata->apb_clk))
+		clk_put(drvdata->apb_clk);
+}
+
+static const struct of_device_id ctcu_match[] = {
+	{.compatible = "qcom,sa8775p-ctcu", .data = &sa8775p_cfgs},
+	{}
+};
+
+static struct platform_driver ctcu_driver = {
+	.probe          = ctcu_probe,
+	.remove         = ctcu_remove,
+	.driver         = {
+		.name   = "coresight-ctcu",
+		.of_match_table = ctcu_match,
+		.suppress_bind_attrs = true,
+	},
+};
+module_platform_driver(ctcu_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CoreSight TMC Control Unit driver");
diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/hwtracing/coresight/coresight-ctcu.h
new file mode 100644
index 000000000000..9885cc7b2042
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-ctcu.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _CORESIGHT_CTCU_H
+#define _CORESIGHT_CTCU_H
+#include "coresight-trace-id.h"
+
+/* Maximum number of supported sink devices for a single CTCU in current projects. */
+#define ATID_MAX_NUM 	2
+
+struct ctcu_traceid_entry {
+	struct hlist_node       hlist;
+	atomic_t                refcnt[ATID_MAX_NUM];
+	u8                      trace_id;
+};
+
+struct ctcu_drvdata {
+	void __iomem		*base;
+	struct clk		*apb_clk;
+	phys_addr_t		pbase;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	raw_spinlock_t		spin_lock;
+	u32			atid_offset[ATID_MAX_NUM];
+	/* refcnt for each traceid of each sink */
+	atomic_t		traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
+};
+#endif
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 82fbcc70a21c..87f9baa7fefe 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -71,7 +71,8 @@ enum coresight_dev_subtype_source {
 
 enum coresight_dev_subtype_helper {
 	CORESIGHT_DEV_SUBTYPE_HELPER_CATU,
-	CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI
+	CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI,
+	CORESIGHT_DEV_SUBTYPE_HELPER_CTCU,
 };
 
 /**
-- 
2.34.1

On 24/01/2025 7:25 am, Jie Gan wrote:
> The Coresight TMC Control Unit hosts miscellaneous configuration registers
> which control various features related to TMC ETR sink.
> 
> Based on the trace ID, which is programmed in the related CTCU ATID
> register of a specific ETR, trace data with that trace ID gets into
> the ETR buffer, while other trace data gets dropped.
> 
> Enabling source device sets one bit of the ATID register based on
> source device's trace ID.
> Disabling source device resets the bit according to the source
> device's trace ID.
> 
> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
> ---
>   drivers/hwtracing/coresight/Kconfig          |  12 +
>   drivers/hwtracing/coresight/Makefile         |   1 +
>   drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
>   include/linux/coresight.h                    |   3 +-
>   5 files changed, 321 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
 >

[...]

> +/*
> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
> + *
> + * Returns 0 indicates success. None-zero result means failure.
> + */
> +static int ctcu_set_etr_traceid(struct coresight_device *csdev, struct coresight_path *cs_path,
> +				bool enable)
> +{
> +	struct coresight_device *sink = coresight_get_sink(cs_path->path);
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +	u8 trace_id = cs_path->trace_id;
> +	int port_num;
> +
> +	if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || IS_ERR_OR_NULL(drvdata)) {
> +		dev_err(&csdev->dev, "Invalid parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	port_num = ctcu_get_active_port(sink, csdev);
> +	if (port_num < 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Skip the disable session if more than one TPDM device that
> +	 * connected to the same TPDA device has been enabled.
> +	 */
> +	if (enable)
> +		atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
> +	else {
> +		if (atomic_dec_return(&drvdata->traceid_refcnt[port_num][trace_id]) > 0) {
> +			dev_dbg(&csdev->dev, "Skip the disable session\n");
> +			return 0;
> +		}
> +	}
> +
> +	dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
> +
> +	return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);

Hi Jie,

Using atomic_dec_return() here doesn't prevent __ctcu_set_etr_traceid() 
from running concurrent enable and disables. Once you pass the 
atomic_dec_return() a second call to enable it will mess it up.

I think you need a spinlock around the whole thing and then the 
refcounts don't need to be atomics.

On 1/28/2025 7:55 PM, James Clark wrote:
> 
> 
> On 24/01/2025 7:25 am, Jie Gan wrote:
>> The Coresight TMC Control Unit hosts miscellaneous configuration 
>> registers
>> which control various features related to TMC ETR sink.
>>
>> Based on the trace ID, which is programmed in the related CTCU ATID
>> register of a specific ETR, trace data with that trace ID gets into
>> the ETR buffer, while other trace data gets dropped.
>>
>> Enabling source device sets one bit of the ATID register based on
>> source device's trace ID.
>> Disabling source device resets the bit according to the source
>> device's trace ID.
>>
>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/Kconfig          |  12 +
>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>   drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
>>   include/linux/coresight.h                    |   3 +-
>>   5 files changed, 321 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>  >
> 
> [...]
> 
>> +/*
>> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
>> + *
>> + * Returns 0 indicates success. None-zero result means failure.
>> + */
>> +static int ctcu_set_etr_traceid(struct coresight_device *csdev, 
>> struct coresight_path *cs_path,
>> +                bool enable)
>> +{
>> +    struct coresight_device *sink = coresight_get_sink(cs_path->path);
>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    u8 trace_id = cs_path->trace_id;
>> +    int port_num;
>> +
>> +    if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || 
>> IS_ERR_OR_NULL(drvdata)) {
>> +        dev_err(&csdev->dev, "Invalid parameters\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    port_num = ctcu_get_active_port(sink, csdev);
>> +    if (port_num < 0)
>> +        return -EINVAL;
>> +
>> +    /*
>> +     * Skip the disable session if more than one TPDM device that
>> +     * connected to the same TPDA device has been enabled.
>> +     */
>> +    if (enable)
>> +        atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
>> +    else {
>> +        if (atomic_dec_return(&drvdata->traceid_refcnt[port_num] 
>> [trace_id]) > 0) {
>> +            dev_dbg(&csdev->dev, "Skip the disable session\n");
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
>> +
>> +    return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
> 
> Hi Jie,
> 
> Using atomic_dec_return() here doesn't prevent __ctcu_set_etr_traceid() 
> from running concurrent enable and disables. Once you pass the 
> atomic_dec_return() a second call to enable it will mess it up.
> 
> I think you need a spinlock around the whole thing and then the 
> refcounts don't need to be atomics.
> 
Hi, James
Thanks for comment. I may not fully tested my codes here. What I was 
thinking is there's no way the refcnt could become a negative number 
under current framework. So I just added spinlock in 
__ctcu_set_etr_traceid() to ensure concurrent sessions correctly 
manipulate the register.

As the trace_id related to the bit of the ATID register, I think the 
concurrent processes are working fine with spinlock around read/write 
register.

I may not fully got your point here. Please help me to correct it.

Thanks,
Jie

On 29/01/2025 12:46 am, Jie Gan wrote:
> 
> 
> On 1/28/2025 7:55 PM, James Clark wrote:
>>
>>
>> On 24/01/2025 7:25 am, Jie Gan wrote:
>>> The Coresight TMC Control Unit hosts miscellaneous configuration 
>>> registers
>>> which control various features related to TMC ETR sink.
>>>
>>> Based on the trace ID, which is programmed in the related CTCU ATID
>>> register of a specific ETR, trace data with that trace ID gets into
>>> the ETR buffer, while other trace data gets dropped.
>>>
>>> Enabling source device sets one bit of the ATID register based on
>>> source device's trace ID.
>>> Disabling source device resets the bit according to the source
>>> device's trace ID.
>>>
>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/Kconfig          |  12 +
>>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>>   drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
>>>   include/linux/coresight.h                    |   3 +-
>>>   5 files changed, 321 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>>  >
>>
>> [...]
>>
>>> +/*
>>> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
>>> + *
>>> + * Returns 0 indicates success. None-zero result means failure.
>>> + */
>>> +static int ctcu_set_etr_traceid(struct coresight_device *csdev, 
>>> struct coresight_path *cs_path,
>>> +                bool enable)
>>> +{
>>> +    struct coresight_device *sink = coresight_get_sink(cs_path->path);
>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>> +    u8 trace_id = cs_path->trace_id;
>>> +    int port_num;
>>> +
>>> +    if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || 
>>> IS_ERR_OR_NULL(drvdata)) {
>>> +        dev_err(&csdev->dev, "Invalid parameters\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>> +    if (port_num < 0)
>>> +        return -EINVAL;
>>> +
>>> +    /*
>>> +     * Skip the disable session if more than one TPDM device that
>>> +     * connected to the same TPDA device has been enabled.
>>> +     */
>>> +    if (enable)
>>> +        atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
>>> +    else {
>>> +        if (atomic_dec_return(&drvdata->traceid_refcnt[port_num] 
>>> [trace_id]) > 0) {
>>> +            dev_dbg(&csdev->dev, "Skip the disable session\n");
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +    dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
>>> +
>>> +    return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
>>
>> Hi Jie,
>>
>> Using atomic_dec_return() here doesn't prevent 
>> __ctcu_set_etr_traceid() from running concurrent enable and disables. 
>> Once you pass the atomic_dec_return() a second call to enable it will 
>> mess it up.
>>
>> I think you need a spinlock around the whole thing and then the 
>> refcounts don't need to be atomics.
>>
> Hi, James
> Thanks for comment. I may not fully tested my codes here. What I was 
> thinking is there's no way the refcnt could become a negative number 
> under current framework. So I just added spinlock in 
> __ctcu_set_etr_traceid() to ensure concurrent sessions correctly 
> manipulate the register.
> 
> As the trace_id related to the bit of the ATID register, I think the 
> concurrent processes are working fine with spinlock around read/write 
> register.
> 
> I may not fully got your point here. Please help me to correct it.
> 
> Thanks,
> Jie
> 
> 

No it can't become negative, but the refcount can be a different state 
to the one that was actually written:


   CPU0                             CPU1
   ----                             ----
   ctcu_set_etr_traceid(enable)
                                    ctcu_set_etr_traceid(disable)
   atomic_inc()
   recount == 1
                                    atomic_dec()
                                    recount == 0

                                    __ctcu_set_etr_traceid(disable)
                                    Lock and write disable state to
                                    device

   __ctcu_set_etr_traceid(enable)
   Lock and write enable state to
   device


As you can see this leaves the device in an enabled state but the 
refcount is 0.

This is also quite large if you use atomic types:

  /* refcnt for each traceid of each sink */
  atomic_t traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];

Presumably you can't have the refcount for each ID be higher than the 
max number of TPDMs connected? If you make the locked area a bit wider 
you don't need atomic types and also solve the above problem. So you 
could do u8, or DECLARE_BITMAP() and bitmap_read() etc to read 3 bit 
values. Or however wide it needs to be.

On 1/29/2025 6:35 PM, James Clark wrote:
> 
> 
> On 29/01/2025 12:46 am, Jie Gan wrote:
>>
>>
>> On 1/28/2025 7:55 PM, James Clark wrote:
>>>
>>>
>>> On 24/01/2025 7:25 am, Jie Gan wrote:
>>>> The Coresight TMC Control Unit hosts miscellaneous configuration 
>>>> registers
>>>> which control various features related to TMC ETR sink.
>>>>
>>>> Based on the trace ID, which is programmed in the related CTCU ATID
>>>> register of a specific ETR, trace data with that trace ID gets into
>>>> the ETR buffer, while other trace data gets dropped.
>>>>
>>>> Enabling source device sets one bit of the ATID register based on
>>>> source device's trace ID.
>>>> Disabling source device resets the bit according to the source
>>>> device's trace ID.
>>>>
>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>> ---
>>>>   drivers/hwtracing/coresight/Kconfig          |  12 +
>>>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>>>   drivers/hwtracing/coresight/coresight-ctcu.c | 276 +++++++++++++++ 
>>>> ++++
>>>>   drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
>>>>   include/linux/coresight.h                    |   3 +-
>>>>   5 files changed, 321 insertions(+), 1 deletion(-)
>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>>>  >
>>>
>>> [...]
>>>
>>>> +/*
>>>> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
>>>> + *
>>>> + * Returns 0 indicates success. None-zero result means failure.
>>>> + */
>>>> +static int ctcu_set_etr_traceid(struct coresight_device *csdev, 
>>>> struct coresight_path *cs_path,
>>>> +                bool enable)
>>>> +{
>>>> +    struct coresight_device *sink = coresight_get_sink(cs_path->path);
>>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>>> +    u8 trace_id = cs_path->trace_id;
>>>> +    int port_num;
>>>> +
>>>> +    if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || 
>>>> IS_ERR_OR_NULL(drvdata)) {
>>>> +        dev_err(&csdev->dev, "Invalid parameters\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>>> +    if (port_num < 0)
>>>> +        return -EINVAL;
>>>> +
>>>> +    /*
>>>> +     * Skip the disable session if more than one TPDM device that
>>>> +     * connected to the same TPDA device has been enabled.
>>>> +     */
>>>> +    if (enable)
>>>> +        atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
>>>> +    else {
>>>> +        if (atomic_dec_return(&drvdata->traceid_refcnt[port_num] 
>>>> [trace_id]) > 0) {
>>>> +            dev_dbg(&csdev->dev, "Skip the disable session\n");
>>>> +            return 0;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
>>>> +
>>>> +    return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
>>>
>>> Hi Jie,
>>>
>>> Using atomic_dec_return() here doesn't prevent 
>>> __ctcu_set_etr_traceid() from running concurrent enable and disables. 
>>> Once you pass the atomic_dec_return() a second call to enable it will 
>>> mess it up.
>>>
>>> I think you need a spinlock around the whole thing and then the 
>>> refcounts don't need to be atomics.
>>>
>> Hi, James
>> Thanks for comment. I may not fully tested my codes here. What I was 
>> thinking is there's no way the refcnt could become a negative number 
>> under current framework. So I just added spinlock in 
>> __ctcu_set_etr_traceid() to ensure concurrent sessions correctly 
>> manipulate the register.
>>
>> As the trace_id related to the bit of the ATID register, I think the 
>> concurrent processes are working fine with spinlock around read/write 
>> register.
>>
>> I may not fully got your point here. Please help me to correct it.
>>
>> Thanks,
>> Jie
>>
>>
> 
> No it can't become negative, but the refcount can be a different state 
> to the one that was actually written:
> 
> 
>    CPU0                             CPU1
>    ----                             ----
>    ctcu_set_etr_traceid(enable)
>                                     ctcu_set_etr_traceid(disable)
>    atomic_inc()
>    recount == 1
>                                     atomic_dec()
>                                     recount == 0
> 
>                                     __ctcu_set_etr_traceid(disable)
>                                     Lock and write disable state to
>                                     device
> 
>    __ctcu_set_etr_traceid(enable)
>    Lock and write enable state to
>    device
> 
> 
> As you can see this leaves the device in an enabled state but the 
> refcount is 0.
Yes, you are right. I didnt consider this scenario. We definitely need 
spinlock here.

> 
> This is also quite large if you use atomic types:
> 
>   /* refcnt for each traceid of each sink */
>   atomic_t traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
> 
> Presumably you can't have the refcount for each ID be higher than the 
> max number of TPDMs connected? If you make the locked area a bit wider 
> you don't need atomic types and also solve the above problem. So you 
> could do u8, or DECLARE_BITMAP() and bitmap_read() etc to read 3 bit 
> values. Or however wide it needs to be.
The original purpose of using atomic here is trying to narrow the locked 
area.

I think u8 is ok here.
u8 traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP] will cost 
224 bytes, I think it's acceptable here.

Thanks,
Jie

> 

On 29/01/2025 1:02 pm, Jie Gan wrote:
> 
> 
> On 1/29/2025 6:35 PM, James Clark wrote:
>>
>>
>> On 29/01/2025 12:46 am, Jie Gan wrote:
>>>
>>>
>>> On 1/28/2025 7:55 PM, James Clark wrote:
>>>>
>>>>
>>>> On 24/01/2025 7:25 am, Jie Gan wrote:
>>>>> The Coresight TMC Control Unit hosts miscellaneous configuration 
>>>>> registers
>>>>> which control various features related to TMC ETR sink.
>>>>>
>>>>> Based on the trace ID, which is programmed in the related CTCU ATID
>>>>> register of a specific ETR, trace data with that trace ID gets into
>>>>> the ETR buffer, while other trace data gets dropped.
>>>>>
>>>>> Enabling source device sets one bit of the ATID register based on
>>>>> source device's trace ID.
>>>>> Disabling source device resets the bit according to the source
>>>>> device's trace ID.
>>>>>
>>>>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>>>>> ---
>>>>>   drivers/hwtracing/coresight/Kconfig          |  12 +
>>>>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>>>>   drivers/hwtracing/coresight/coresight-ctcu.c | 276 ++++++++++++++ 
>>>>> + ++++
>>>>>   drivers/hwtracing/coresight/coresight-ctcu.h |  30 ++
>>>>>   include/linux/coresight.h                    |   3 +-
>>>>>   5 files changed, 321 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.c
>>>>>   create mode 100644 drivers/hwtracing/coresight/coresight-ctcu.h
>>>>  >
>>>>
>>>> [...]
>>>>
>>>>> +/*
>>>>> + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID.
>>>>> + *
>>>>> + * Returns 0 indicates success. None-zero result means failure.
>>>>> + */
>>>>> +static int ctcu_set_etr_traceid(struct coresight_device *csdev, 
>>>>> struct coresight_path *cs_path,
>>>>> +                bool enable)
>>>>> +{
>>>>> +    struct coresight_device *sink = coresight_get_sink(cs_path- 
>>>>> >path);
>>>>> +    struct ctcu_drvdata *drvdata = dev_get_drvdata(csdev- 
>>>>> >dev.parent);
>>>>> +    u8 trace_id = cs_path->trace_id;
>>>>> +    int port_num;
>>>>> +
>>>>> +    if ((sink == NULL) || !IS_VALID_CS_TRACE_ID(trace_id) || 
>>>>> IS_ERR_OR_NULL(drvdata)) {
>>>>> +        dev_err(&csdev->dev, "Invalid parameters\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    port_num = ctcu_get_active_port(sink, csdev);
>>>>> +    if (port_num < 0)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    /*
>>>>> +     * Skip the disable session if more than one TPDM device that
>>>>> +     * connected to the same TPDA device has been enabled.
>>>>> +     */
>>>>> +    if (enable)
>>>>> +        atomic_inc(&drvdata->traceid_refcnt[port_num][trace_id]);
>>>>> +    else {
>>>>> +        if (atomic_dec_return(&drvdata->traceid_refcnt[port_num] 
>>>>> [trace_id]) > 0) {
>>>>> +            dev_dbg(&csdev->dev, "Skip the disable session\n");
>>>>> +            return 0;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    dev_dbg(&csdev->dev, "traceid is %d\n", cs_path->trace_id);
>>>>> +
>>>>> +    return __ctcu_set_etr_traceid(csdev, trace_id, port_num, enable);
>>>>
>>>> Hi Jie,
>>>>
>>>> Using atomic_dec_return() here doesn't prevent 
>>>> __ctcu_set_etr_traceid() from running concurrent enable and 
>>>> disables. Once you pass the atomic_dec_return() a second call to 
>>>> enable it will mess it up.
>>>>
>>>> I think you need a spinlock around the whole thing and then the 
>>>> refcounts don't need to be atomics.
>>>>
>>> Hi, James
>>> Thanks for comment. I may not fully tested my codes here. What I was 
>>> thinking is there's no way the refcnt could become a negative number 
>>> under current framework. So I just added spinlock in 
>>> __ctcu_set_etr_traceid() to ensure concurrent sessions correctly 
>>> manipulate the register.
>>>
>>> As the trace_id related to the bit of the ATID register, I think the 
>>> concurrent processes are working fine with spinlock around read/write 
>>> register.
>>>
>>> I may not fully got your point here. Please help me to correct it.
>>>
>>> Thanks,
>>> Jie
>>>
>>>
>>
>> No it can't become negative, but the refcount can be a different state 
>> to the one that was actually written:
>>
>>
>>    CPU0                             CPU1
>>    ----                             ----
>>    ctcu_set_etr_traceid(enable)
>>                                     ctcu_set_etr_traceid(disable)
>>    atomic_inc()
>>    recount == 1
>>                                     atomic_dec()
>>                                     recount == 0
>>
>>                                     __ctcu_set_etr_traceid(disable)
>>                                     Lock and write disable state to
>>                                     device
>>
>>    __ctcu_set_etr_traceid(enable)
>>    Lock and write enable state to
>>    device
>>
>>
>> As you can see this leaves the device in an enabled state but the 
>> refcount is 0.
> Yes, you are right. I didnt consider this scenario. We definitely need 
> spinlock here.
> 
>>
>> This is also quite large if you use atomic types:
>>
>>   /* refcnt for each traceid of each sink */
>>   atomic_t traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP];
>>
>> Presumably you can't have the refcount for each ID be higher than the 
>> max number of TPDMs connected? If you make the locked area a bit wider 
>> you don't need atomic types and also solve the above problem. So you 
>> could do u8, or DECLARE_BITMAP() and bitmap_read() etc to read 3 bit 
>> values. Or however wide it needs to be.
> The original purpose of using atomic here is trying to narrow the locked 
> area.
> 
> I think u8 is ok here.
> u8 traceid_refcnt[ATID_MAX_NUM][CORESIGHT_TRACE_ID_RES_TOP] will cost 
> 224 bytes, I think it's acceptable here.
> 
> Thanks,
> Jie
> 

Yep u8 sounds ok then