Message-ID: <37cb0da5-fcca-41e0-95d8-280fd95541c9@linaro.org>
Date: Mon, 24 Feb 2025 09:53:16 +0000
From: James Clark <james.clark@...aro.org>
To: Jie Gan <quic_jiegan@...cinc.com>,
 Suzuki K Poulose <suzuki.poulose@....com>
Cc: Tingwei Zhang <quic_tingweiz@...cinc.com>,
 Jinlong Mao <quic_jinlmao@...cinc.com>, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com, Mike Leach
 <mike.leach@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH v12 6/7] Coresight: Add Coresight TMC Control Unit driver



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@...aro.org>
>>> Signed-off-by: Jie Gan <quic_jiegan@...cinc.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.

Powered by blists - more mailing lists