| 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: <a96c2e49-217d-4c90-b32a-ad8eb439a4ec@quicinc.com>
Date: Tue, 14 Jan 2025 09:50:51 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: James Clark <james.clark@...aro.org>
CC: 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>,
Tingwei Zhang <quic_tingweiz@...cinc.com>,
<linux-arm-msm@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
Suzuki K Poulose <suzuki.poulose@....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 v8 4/5] Coresight: Add Coresight TMC Control Unit driver
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@...cinc.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
Powered by blists - more mailing lists