| 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: <2c5c1c8e-a10b-48fb-a0ec-c8164b4f3908@quicinc.com>
Date: Mon, 24 Feb 2025 18:33:53 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: James Clark <james.clark@...aro.org>,
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 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@...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.
>>
>
> 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
>>
>
>
>
>
>
Powered by blists - more mailing lists