| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c274175a-ed6d-4ce9-be86-d48f56cafe9d@arm.com>
Date: Mon, 24 Feb 2025 10:22:30 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Jie Gan <quic_jiegan@...cinc.com>, Mike Leach <mike.leach@...aro.org>,
James Clark <james.clark@...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>
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
Subject: Re: [PATCH v12 6/7] Coresight: Add Coresight TMC Control Unit driver
On 24/02/2025 03:32, 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?
The code above is similar to yours. It would "set" only for the first
time, when
enable == 0, refcount == 0 now, but will be incremented to 1.
Suzuki
Powered by blists - more mailing lists