Re: [PATCH v12 6/7] Coresight: Add Coresight TMC Control Unit driver

[Jie Gan <quic_jiegan@quicinc.com>]

On 2/24/2025 6:04 PM, Jie Gan wrote:
> 
> 
> On 2/24/2025 5:53 PM, James Clark wrote:
>>
>>
>> On 24/02/2025 3:32 am, Jie Gan wrote:
>>>
>>>
>>> On 2/21/2025 7:39 PM, Suzuki K Poulose wrote:
>>>> On 17/02/2025 09:30, 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.
>>>>>
>>>>> Reviewed-by: James Clark <james.clark@linaro.org>
>>>>> 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 | 268 ++++++++++++++ 
>>>>> + ++++
>>>>>   drivers/hwtracing/coresight/coresight-ctcu.h |  24 ++
>>>>>   include/linux/coresight.h                    |   3 +-
>>>>>   5 files changed, 307 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..e1460a627c4d
>>>>> --- /dev/null
>>>>> +++ b/drivers/hwtracing/coresight/coresight-ctcu.c
>>>>> @@ -0,0 +1,268 @@
>>>>> +// 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),
>>>>> +};
>>>>> +
>>>>> +static void ctcu_program_atid_register(struct ctcu_drvdata 
>>>>> *drvdata, u32 reg_offset,
>>>>> +                       u8 bit, bool enable)
>>>>> +{
>>>>> +    u32 val;
>>>>> +
>>>>> +    CS_UNLOCK(drvdata->base);
>>>>> +    val = ctcu_readl(drvdata, reg_offset);
>>>>> +    val = enable? (val | BIT(bit)) : (val & ~BIT(bit));
>>>>
>>>> minor nit: If possible do not use the ternary operator like this. It 
>>>> is much better readable as:
>>>>
>>>>      if (enable)
>>>>          val |= BIT(bit);
>>>>      else
>>>>          val &= ~BIT(bit);
>>>>
>>>
>>> Will do this way.
>>>
>>>>> +    ctcu_writel(drvdata, val, reg_offset);
>>>>> +    CS_LOCK(drvdata->base);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * __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.
>>>>> + * @port_num:    port number from TMC ETR sink.
>>>>> + * @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;
>>>>> +    u8 refcnt, 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);
>>>>> +    refcnt = drvdata->traceid_refcnt[port_num][traceid];
>>>>> +    /* Only program the atid register when the refcnt value is 0 
>>>>> or 1 */
>>>>
>>>> A normal trace source won't be enabled more than once (e.g., ETM). 
>>>> The only odd one out is the STM, which may be driven by multiple 
>>>> agents.
>>>> So this refcounting looks necessary.
>>>>
>>>
>>> Besides, for the TPDMs which shared the trace_id of the TPDA also 
>>> need the refcnt. Consider we have TPDM1 and TPDM2 connected to the 
>>> same TPDA device. Once we disable one of the TPDM without checking 
>>> the refcnt, the filter function will be disabled for another TPDM.
>>>
>>>>> +    if (enable && (++refcnt == 1))
>>>>> +        ctcu_program_atid_register(drvdata, reg_offset, bit, enable);
>>>>> +    else if (!enable && (--refcnt == 0))
>>>>> +        ctcu_program_atid_register(drvdata, reg_offset, bit, enable);
>>>>
>>>> minor nit:
>>>>
>>>>      if ((enable && !refcount++) ||
>>>>          (!enable && --refcount))
>>>>          ctcu_program_atid_register(drvdata, reg_offset, bit, enable);
>>>>
>>>>
>>>
>>> I did (enable && (++refcnt == 1)) just because I think we only need 
>>> program the register when refcnt is equal to 1. We dont need 
>>> reprogram the register with same value when refcnt greater than 1. So 
>>> I think it's better for the performance?
>>>
>>>> Also, see my comment the bottom for "refcount" being u8 .
>>>
>>> Sure, will check.
>>>
>>>>
>>>>
>>>>> +
>>>>> +    drvdata->traceid_refcnt[port_num][traceid] = refcnt;
>>>>> +
>>>>> +    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;
>>>>
>>>> Don't we need to make sure it matches the helper ? What if there are 
>>>> multiple helpers ? e.g, a CATU connected to the ETR and CTCU ?
>>>> Or even try the opposite ? i.e. search the helper and find the port
>>>> matching the sink ?
>>
>> Good catch, looks like this should be done the opposite way around.
>>
>>>>
>>>>      struct coresight_platform_data *pdata = helper->pdata;
>>>>
>>>>      for (i = 0; i < pdata->nr_inconns; ++i)
>>>>          if (pdata->in_conns[i]->dest_dev == sink)
>>>>              return pdata->in_conns[i]->src_port;
>>>>
>>>> Not sure if that works with the helper device connection, James ?
>>
>> Yeah connections are always made in both directions.
>>
>>> Can we check the subtype of the helper device? We should only have 
>>> one CTCU helper device for each ETR.
>>>
>>> enum coresight_dev_subtype_helper subtype;
>>>
>>> for (i = 0; i < sink->pdata->nr_outconns; ++i) {
>>>      subtype = sink->pdata->out_conns[i]->dest_dev- 
>>> >subtype.helper_subtype;
>>>      if (subtype == CORESIGHT_DEV_SUBTYPE_HELPER_CTCU)
>>>         return sink->pdata->out_conns[i]->dest_port;
>>>
>>
>> I don't think we need to check the type, just search all the CTCU's 
>> in_conns until you find the sink.
>>
>> As Suzuki says, by looking at the out_conns of the sink you might find 
>> a different helper device. Checking that it really is connected to the 
>> sink is probably more robust that relying on the type anyway.
>>
> 
> Hi James,
> 
> Thanks for explaination, will update per Suzuki's suggestion.
> 
> Jie
> 

Hi Suzuki James,

Just for confirm, the updated codes from opposite way should be:

struct coresight_platform_data *pdata = helper->pdata;

for (i = 0; i < pdata->nr_inconns; ++i)
     if (pdata->in_conns[i]->src_dev == sink)
     	return pdata->in_conns[i]->dest_port;

Just because the direction when create connection is sink->helper, am right?

--------------------------     ---------------------------
    src_dev(sink) src_port| --> |dest_port dest_dev(helper)
--------------------------     ---------------------------
Thanks,
Jie

>>
> 
> 
> 
> 
>