Re: [PATCH v8 4/5] Coresight: Add Coresight TMC Control Unit driver

[Jie Gan <quic_jiegan@quicinc.com>]

On 1/13/2025 8:05 PM, James Clark wrote:
> 
> 
> On 26/12/2024 1:10 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          |   8 +
>>   drivers/hwtracing/coresight/Makefile         |   1 +
>>   drivers/hwtracing/coresight/coresight-ctcu.c | 273 +++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-ctcu.h |  21 ++
>>   include/linux/coresight.h                    |   3 +-
>>   5 files changed, 305 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..152eab0b9b2a 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -133,6 +133,14 @@ 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"
>> +    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.
>> +
>>   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..7650dbe9a41e
>> --- /dev/null
>> +++ b/drivers/hwtracing/coresight/coresight-ctcu.c
>> @@ -0,0 +1,273 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024 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"
>> +#include "coresight-trace-id.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 uint32_t atid_offset;
>> +    const uint32_t 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)
>> +{
>> +    uint32_t atid_offset, reg_offset, val;
>> +    struct ctcu_drvdata *drvdata;
>> +    int bit;
>> +
>> +    if (!IS_VALID_CS_TRACE_ID(traceid))
>> +        return -EINVAL;
> 
> Minor point, but this was already done in the calling function.
Thanks for comment. Totally agree with you, it's redundant codes here.
I will remove it in next version.

> 
>> +
>> +    drvdata = dev_get_drvdata(csdev->dev.parent);
>> +    if (IS_ERR_OR_NULL(drvdata))
>> +        return -EINVAL;
>> +
>> +    atid_offset = drvdata->atid_offset[port_num];
>> +    if (atid_offset == 0)
>> +        return -EINVAL;
>> +
>> +    guard(raw_spinlock_irqsave)(&drvdata->spin_lock);
>> +    CS_UNLOCK(drvdata->base);
>> +
>> +    bit = CTCU_ATID_REG_BIT(traceid);
>> +    reg_offset = CTCU_ATID_REG_OFFSET(traceid, atid_offset);
> 
> The locks only need to be around the read/write below. bit and 
> reg_offset are all local and shouldn't be affected. Doesn't really make 
> a difference but makes the code a bit more readable.
Yes, agree with you. It makes sense and is easier to read. Will move the 
CS_UNLOCK to the proper position.

> 
>> +    if (reg_offset - atid_offset > CTCU_ATID_REG_SIZE) {
>> +        CS_LOCK(drvdata);
>> +        return -EINVAL;
>> +    }
>> +
>> +    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;
>> +}
>> +
[...]

Thanks,
Jie