On Thu, Mar 06, 2025 at 04:39:27PM +0800, Yuanfang Zhang wrote:
[...]
> >> +static ssize_t flush_req_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf,
> >> + size_t size)
> >> +{
...
> >> + reg = readl_relaxed(drvdata->base + TRACE_NOC_CTRL);
> >> + reg = reg | TRACE_NOC_CTRL_FLUSHREQ;
> >> + writel_relaxed(reg, drvdata->base + TRACE_NOC_CTRL);
> >
> > How can userspace determine when to trigger a flush?
> It can be triggered under any circumstances.
> >
> > Generally, a driver kicks off a flush operation for a hardware before
> > reading data from buffer or when disable a link path. I don't know the
> > hardware mechanism of TNOC, but seems to me, it does not make sense to
> > let the userspace to trigger a hardware flush, given the userspace has
> > no knowledge for device's state.
>
> TNOC supports the aforementioned flush operation, and it also adds this
> flush functionality, allowing users to set the flush themselves.
I am still not convinced for providing knobs to allow userspace to
directly control hardware.
A low level driver should have sufficient information to know when and
how it triggers a flush. E.g., CoreSight ETF (coresight-tmc-etf.c) can
act as a link, in this case, it calls the tmc_flush_and_stop() function
to flush its buffer when it is stopped. A flushing is triggered when a
session is terminated (either is a perf session or a Sysfs session).
Why not TNOC driver do the flushing same as other drivers? It can flush
the data before a hardware link is to be disabled. I don't think flush
operations are required at any time.
Seems to me, exposing APIs to userspace for flushing operations also
will introduce potential security risk. A malicious software might
attack system with triggering tons of flushing in short time.
> > Furthermore, based on my understanding for patch 02 and 03, the working
> > flow is also concerned me. IIUC, you want to use the driver to create
> > a linkage and then use userspace program to poll state and trigger
> > flushing. Could you explain why use this way for managing the device?
> >
> TNOC support flush just like other links. This interface simply provides
> customers with an additional option to trigger the flush.
This is not true for Arm CoreSight components. My understanding is Arm
CoreSight drivers never provides an API to userspace to manually trigger
flush operations.
Thanks,
Leo
On Mon, 10 Mar 2025 18:27:24 +0800, Jie Gan wrote:
> The coresight_etm_get_trace_id function is a global function. The
> verification process for 'csdev' is required prior to its usage.
>
>
Applied, thanks!
[1/1] coresight: add verification process for coresight_etm_get_trace_id
https://git.kernel.org/coresight/c/ab37128a
Best regards,
--
Suzuki K Poulose <suzuki.poulose(a)arm.com>
On Thu, 06 Mar 2025 12:11:01 +0000, Yeoreum Yun wrote:
> In some coresight drivers, drvdata->spinlock can be held during __schedule()
> by perf_event_task_sched_out()/in().
>
> Since drvdata->spinlock type is spinlock_t and
> perf_event_task_sched_out()/in() is called after acquiring rq_lock,
> which is raw_spinlock_t (an unsleepable lock),
> this poses an issue in PREEMPT_RT kernel where spinlock_t is sleepable.
>
> [...]
Applied, thanks!
[1/9] coresight: change coresight_device lock type to raw_spinlock_t
https://git.kernel.org/coresight/c/26f060c1
[2/9] coresight-etm4x: change etmv4_drvdata spinlock type to raw_spinlock_t
https://git.kernel.org/coresight/c/743c5a97
[3/9] coresight: change coresight_trace_id_map's lock type to raw_spinlock_t
https://git.kernel.org/coresight/c/4cf364ca
[4/9] coresight-cti: change cti_drvdata spinlock's type to raw_spinlock_t
https://git.kernel.org/coresight/c/e3044065
[5/9] coresight-etb10: change etb_drvdata spinlock's type to raw_spinlock_t
https://git.kernel.org/coresight/c/6b80c0ab
[6/9] coresight-funnel: change funnel_drvdata spinlock's type to raw_spinlock_t
https://git.kernel.org/coresight/c/56eb02f0
[7/9] coresight-replicator: change replicator_drvdata spinlock's type to raw_spinlock_t
https://git.kernel.org/coresight/c/982d0a0e
[8/9] coresight-tmc: change tmc_drvdata spinlock's type to raw_spinlock_t
https://git.kernel.org/coresight/c/db11f75b
[9/9] coresight/ultrasoc: change smb_drv_data spinlock's type to raw_spinlock_t
https://git.kernel.org/coresight/c/d11eb31d
Best regards,
--
Suzuki K Poulose <suzuki.poulose(a)arm.com>
On Thu, Mar 06, 2025 at 04:22:20PM +0800, Yuanfang Zhang wrote:
[...]
> >> +static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
> >> +{
> >> + int atid;
> >> +
> >> + atid = coresight_trace_id_get_system_id();
> >> + if (atid < 0)
> >> + return atid;
> >> +
> >> + drvdata->atid = atid;
> >> +
> >> + drvdata->freq_type = FREQ_TS;
> >
> > I don't see anywhere uses FREQ. Please remove the unused definitions
> > and related code.
>
> it is used in trace_noc_enable_hw().
I understood some macros and definitions are used by seqential patches.
A good practice is code should be added only when they are used. This
can allow every patch in neat way and easier for review.
Thanks,
Leo
> >
> >> + drvdata->flag_type = FLAG;
> >
> > FLAG_TS is not used in the driver as well. Remove it.
> it is used in trace_noc_enable_hw().
On 10/03/2025 02:23, Jie Gan wrote:
> The coresight_etm_get_trace_id function is a global function. The
> verification process for 'csdev' is required prior to its usage.
>
> Fixes: c367a89dec26 ("Coresight: Add trace_id function to retrieving the trace ID")
> Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index bd0a7edd38c9..5a7cd2376e2d 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1616,9 +1616,12 @@ EXPORT_SYMBOL_GPL(coresight_remove_driver);
> int coresight_etm_get_trace_id(struct coresight_device *csdev, enum cs_mode mode,
> struct coresight_device *sink)
> {
> - int trace_id;
> - int cpu = source_ops(csdev)->cpu_id(csdev);
> + int cpu, trace_id;
> +
> + if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE && !source_ops(csdev)->cpu_id)
That must be :
csdev->type != CORESIGHT_DEV_TYPE_SOURCE || !source_ops(csdev)->cpu_id)
Suzuki
> + return -EINVAL;
>
> + cpu = source_ops(csdev)->cpu_id(csdev);
> switch (mode) {
> case CS_MODE_SYSFS:
> trace_id = coresight_trace_id_get_cpu_id(cpu);
Hi,
On Tue, 4 Mar 2025 at 08:05, Krzysztof Kozlowski <krzk(a)kernel.org> wrote:
>
> On Thu, Feb 27, 2025 at 05:26:34PM +0800, songchai wrote:
> > From: Songwei Chai <quic_songchai(a)quicinc.com>
> >
> > The Trigger Generation Unit (TGU) is designed to detect patterns or
> > sequences within a specific region of the System on Chip (SoC). Once
> > configured and activated, it monitors sense inputs and can detect a
> > pre-programmed state or sequence across clock cycles, subsequently
> > producing a trigger.
> >
> > TGU configuration space
> > offset table
> > x-------------------------x
> > | |
> > | |
> > | | Step configuration
> > | | space layout
> > | coresight management | x-------------x
> > | registers | |---> | |
> > | | | | reserve |
> > | | | | |
> > |-------------------------| | |-------------|
> > | | | | priority[3] |
> > | step[7] |<-- | |-------------|
> > |-------------------------| | | | priority[2] |
> > | | | | |-------------|
> > | ... | |Steps region | | priority[1] |
> > | | | | |-------------|
> > |-------------------------| | | | priority[0] |
> > | |<-- | |-------------|
> > | step[0] |--------------------> | |
> > |-------------------------| | condition |
> > | | | |
> > | control and status | x-------------x
> > | space | | |
> > x-------------------------x |Timer/Counter|
> > | |
> > x-------------x
> > TGU Configuration in Hardware
> >
> > The TGU provides a step region for user configuration, similar
> > to a flow chart. Each step region consists of three register clusters:
> >
> > 1.Priority Region: Sets the required signals with priority.
> > 2.Condition Region: Defines specific requirements (e.g., signal A
> > reaches three times) and the subsequent action once the requirement is
> > met.
> > 3.Timer/Counter (Optional): Provides timing or counting functionality.
> >
> > Add a new coresight-tgu.yaml file to describe the bindings required to
> > define the TGU in the device trees.
> >
> > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com>
> > Signed-off-by: songchai <quic_songchai(a)quicinc.com>
>
> Don't duplicate yourself.
>
> Anyway, this is marked as v3, I cannot find previous versions, no
> changelog, no references.
>
> What happened here in this binding?
>
> > ---
> > .../bindings/arm/qcom,coresight-tgu.yaml | 135 ++++++++++++++++++
> > 1 file changed, 135 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
> > new file mode 100644
> > index 000000000000..a41ac68a4fe7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-tgu.yaml
> > @@ -0,0 +1,135 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +# Copyright (c) 2023-2025 Qualcomm Innovation Center, Inc. All rights reserved.
>
> 2023 and 2024? Where was it published in these years?
>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/qcom,coresight-tgu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Trigger Generation Unit - TGU
> > +
> > +description: |
> > + The Trigger Generation Unit (TGU) is a Data Engine which can be utilized
> > + to sense a plurality of signals and create a trigger into the CTI or
> > + generate interrupts to processors. The TGU is like the trigger circuit
> > + of a Logic Analyzer. The corresponding trigger logic can be realized by
> > + configuring the conditions for each step after sensing the signal.
> > + Once setup and enabled, it will observe sense inputs and based upon
> > + the activity of those inputs, even over clock cycles, may detect a
> > + preprogrammed state/sequence and then produce a trigger or interrupt.
> > +
> > + The primary use case of the TGU is to detect patterns or sequences on a
> > + given set of signals within some region of the SoC.
> > +
> > +maintainers:
> > + - Mao Jinlong <quic_jinlmao(a)quicinc.com>
> > + - Sam Chai <quic_songchai(a)quicinc.com>
> > +
> > +# Need a custom select here or 'arm,primecell' will match on lots of nodes
> > +select:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,coresight-tgu
> > + required:
> > + - compatible
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - const: qcom,coresight-tgu
> > + - const: arm,primecell
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > + clock-names:
> > + items:
> > + - const: apb_pclk
> > +
> > + qcom,tgu-steps:
> > + description:
> > + The trigger logic is realized by configuring each step after sensing
> > + the signal. The parameter here is used to describe the maximum of steps
> > + that could be configured in the current TGU.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 1
> > + maximum: 8
> > +
Hardware features are usually defined by ID registers in coresight
devices. e.g. ETM has a number of ID registers that describe the
number of comparators / counters etc.
Does this device not have similar registers? Is there not a unique ID
for each hardware variant - hardware discoverablility is an
architecture requirement for coresight devices?
> > + qcom,tgu-regs:
> > + description:
> > + There are some "groups" register clusters in each step, which are used to
> > + configure the signal that we want to detect. Meanwhile, each group has its
> > + own priority, and the priority increases with number of groups. For example,
> > + group3 has a higher priority than group2, the signal configured in group3
> > + will be sensed more preferentially than the signal which is configured in group2.
> > + The parameter here is used to describe the signal number that each group
> > + could be configured.
>
> And all groups are indexed by number? Or do they have names?
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 1
> > + maximum: 18
> > +
> > + qcom,tgu-conditions:
> > + description:
> > + A condition sets a specific requirement for a step and defines the subsequent
> > + action once the requirement is met. For example, in step two, if signal A is
> > + detected three times, the process jumps back to step one. The parameter describes
> > + the register number for each functionality, whether it is setting a specific
> > + requirement or defining a subsequent action.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 1
> > + maximum: 4
> > +
> > + qcom,tgu-timer-counters:
> > + description:
> > + TGU has timer and counter which are used to set some requirement on each step.
>
> Wrap according to Linux coding style, so at 80.
>
> > + For example, we could use counter to create a trigger into CTI once TGU senses
> > + the target signal three times.This parameter is used to describe the number of
> > + Timers/Counters in TGU.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0
>
> Drop
>
> > + maximum: 2
> > +
> > + in-ports:
> > + $ref: /schemas/graph.yaml#/properties/ports
> > + additionalProperties: false
> > +
> > + properties:
> > + port:
> > + description: AXI Slave connected to another Coresight component
>
> So this TGU can be connected to anything in coresight graph, no
> restrictions?
>
Coresight uses APB for register access and ATB for moving trace from
source to sink. The only use of AXI is on the ETR/CATU output saving
trace data into system memory.
> > + $ref: /schemas/graph.yaml#/properties/port
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
>
> Most likely you miss also: in-ports
>
>
> Best regards,
> Krzysztof
>
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK