CoreSight

coresight@lists.linaro.org

6 participants
2370 discussions

[PATCH] arm64: dts: hi3660: Add property for fixing CPUIdle

by Leo Yan

During CPU low power modes, ETM components will lose their context. Add the "arm,coresight-loses-context-with-cpu" property to ETM nodes to save and restore ETM context for CPU idle states. Signed-off-by: Leo Yan <leo.yan(a)arm.com> --- arch/arm64/boot/dts/hisilicon/hi3660-coresight.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-coresight.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660-coresight.dtsi index 79a55a0fa2f1..4c6a075908d1 100644 --- a/arch/arm64/boot/dts/hisilicon/hi3660-coresight.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi3660-coresight.dtsi @@ -17,6 +17,7 @@ etm@ecc40000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu0>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -34,6 +35,7 @@ etm@ecd40000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu1>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -51,6 +53,7 @@ etm@ece40000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu2>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -68,6 +71,7 @@ etm@ecf40000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu3>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -160,6 +164,7 @@ etm@ed440000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu4>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -177,6 +182,7 @@ etm@ed540000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu5>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -194,6 +200,7 @@ etm@ed640000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu6>; + arm,coresight-loses-context-with-cpu; out-ports { port { @@ -211,6 +218,7 @@ etm@ed740000 { clocks = <&crg_ctrl HI3660_PCLK>; clock-names = "apb_pclk"; cpu = <&cpu7>; + arm,coresight-loses-context-with-cpu; out-ports { port { -- 2.34.1

3 months, 1 week

Re: [PATCH v2 3/5] coresight-tnoc: add nodes to configure flush

by Leo Yan

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

3 months, 1 week

Re: [PATCH v2] coresight: add verification process for coresight_etm_get_trace_id

by Suzuki K Poulose

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>

3 months, 1 week

Re: [PATCH v6 0/9] coresight: change some driver' spinlock type to raw_spinlock_t

by Suzuki K Poulose

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>

3 months, 1 week

Re: [PATCH v2 2/5] coresight: add coresight Trace NOC driver

by Leo Yan

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().

3 months, 1 week

Re: [PATCH] coresight: add verification process for coresight_etm_get_trace_id

by Suzuki K Poulose

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);

3 months, 1 week

[PATCH 1/1 next] perf libunwind: Fixup conversion perf_sample->user_regs to a pointer

by Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <acme(a)redhat.com> The dc6d2bc2d893a878 ("perf sample: Make user_regs and intr_regs optional") misses the changes to a file, resulting in this problem: $ make LIBUNWIND=1 -C tools/perf O=/tmp/build/perf-tools-next install-bin <SNIP> CC /tmp/build/perf-tools-next/util/unwind-libunwind-local.o CC /tmp/build/perf-tools-next/util/unwind-libunwind.o <SNIP> util/unwind-libunwind-local.c: In function ‘access_mem’: util/unwind-libunwind-local.c:582:56: error: ‘ui->sample->user_regs’ is a pointer; did you mean to use ‘->’? 582 | if (__write || !stack || !ui->sample->user_regs.regs) { | ^ | -> util/unwind-libunwind-local.c:587:38: error: passing argument 2 of ‘perf_reg_value’ from incompatible pointer type [-Wincompatible-pointer-types] 587 | ret = perf_reg_value(&start, &ui->sample->user_regs, | ^~~~~~~~~~~~~~~~~~~~~~ | | | struct regs_dump ** <SNIP> ⬢ [acme@toolbox perf-tools-next]$ git bisect bad dc6d2bc2d893a878e7b58578ff01b4738708deb4 is the first bad commit commit dc6d2bc2d893a878e7b58578ff01b4738708deb4 (HEAD) Author: Ian Rogers <irogers(a)google.com> Date: Mon Jan 13 11:43:45 2025 -0800 perf sample: Make user_regs and intr_regs optional Detected using: make -C tools/perf build-test Fixes: dc6d2bc2d893a878 ("perf sample: Make user_regs and intr_regs optional") Cc: Adrian Hunter <adrian.hunter(a)intel.com> Cc: Alexander Shishkin <alexander.shishkin(a)linux.intel.com> Cc: Andi Kleen <ak(a)linux.intel.com> Cc: Charlie Jenkins <charlie(a)rivosinc.com> Cc: Ian Rogers <irogers(a)google.com> Cc: Ingo Molnar <mingo(a)redhat.com> Cc: James Clark <james.clark(a)linaro.org> Cc: John Garry <john.g.garry(a)oracle.com> Cc: Kan Liang <kan.liang(a)linux.intel.com> Cc: Leo Yan <leo.yan(a)linux.dev> Cc: Mark Rutland <mark.rutland(a)arm.com> Cc: Michael Petlan <mpetlan(a)redhat.com> Cc: Namhyung Kim <namhyung(a)kernel.org> Cc: Peter Zijlstra <peterz(a)infradead.org> Cc: Tavian Barnes <tavianator(a)tavianator.com> Cc: Veronika Molnarova <vmolnaro(a)redhat.com> Cc: coresight(a)lists.linaro.org Cc: linux-arm-kernel(a)lists.infradead.org Signed-off-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> --- tools/perf/util/unwind-libunwind-local.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c index 16c2b03831f39afc..cc0e41f2c2f2ef10 100644 --- a/tools/perf/util/unwind-libunwind-local.c +++ b/tools/perf/util/unwind-libunwind-local.c @@ -579,12 +579,12 @@ static int access_mem(unw_addr_space_t __maybe_unused as, int ret; /* Don't support write, probably not needed. */ - if (__write || !stack || !ui->sample->user_regs.regs) { + if (__write || !stack || !ui->sample->user_regs->regs) { *valp = 0; return 0; } - ret = perf_reg_value(&start, &ui->sample->user_regs, + ret = perf_reg_value(&start, perf_sample__user_regs(ui->sample), perf_arch_reg_sp(arch)); if (ret) return ret; @@ -628,7 +628,7 @@ static int access_reg(unw_addr_space_t __maybe_unused as, return 0; } - if (!ui->sample->user_regs.regs) { + if (!ui->sample->user_regs->regs) { *valp = 0; return 0; } @@ -637,7 +637,7 @@ static int access_reg(unw_addr_space_t __maybe_unused as, if (id < 0) return -EINVAL; - ret = perf_reg_value(&val, &ui->sample->user_regs, id); + ret = perf_reg_value(&val, perf_sample__user_regs(ui->sample), id); if (ret) { if (!ui->best_effort) pr_err("unwind: can't read reg %d\n", regnum); @@ -741,7 +741,7 @@ static int get_entries(struct unwind_info *ui, unwind_entry_cb_t cb, unw_cursor_t c; int ret, i = 0; - ret = perf_reg_value(&val, &ui->sample->user_regs, + ret = perf_reg_value(&val, perf_sample__user_regs(ui->sample), perf_arch_reg_ip(arch)); if (ret) return ret; @@ -808,7 +808,7 @@ static int _unwind__get_entries(unwind_entry_cb_t cb, void *arg, .best_effort = best_effort }; - if (!data->user_regs.regs) + if (!data->user_regs->regs) return -EINVAL; if (max_stack <= 0) -- 2.47.0

3 months, 2 weeks

Re: [PATCH v3 7/7] coresight-tgu: add reset node to initialize

by Mike Leach

Hi, On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai(a)quicinc.com> wrote: > > From: Songwei Chai <quic_songchai(a)quicinc.com> > > Add reset node to initialize the value of > priority/condition_decode/condition_select/timer/counter nodes > > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com> > Signed-off-by: songchai <quic_songchai(a)quicinc.com> > --- > .../testing/sysfs-bus-coresight-devices-tgu | 7 ++ > drivers/hwtracing/coresight/coresight-tgu.c | 79 +++++++++++++++++++ > 2 files changed, 86 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > index d88d05fbff43..8fb5afd7c655 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > @@ -42,3 +42,10 @@ KernelVersion 6.15 > Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > Description: > (RW) Set/Get the counter value with specific step for TGU. > + > +What: /sys/bus/coresight/devices/<tgu-name>/reset_tgu > +Date: February 2025 > +KernelVersion 6.15 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > +Description: > + (Write) Reset the dataset for TGU. Document the value needed to initiate the reset. > diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c > index 693d632fb079..b36ced761c0d 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.c > +++ b/drivers/hwtracing/coresight/coresight-tgu.c > @@ -343,6 +343,84 @@ static ssize_t enable_tgu_store(struct device *dev, > } > static DEVICE_ATTR_RW(enable_tgu); > > +/* reset_tgu_store - Reset Trace and Gating Unit (TGU) configuration. */ > +static ssize_t reset_tgu_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t size) > +{ > + unsigned long value; > + struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent); > + int i, j, ret; > + > + if (kstrtoul(buf, 0, &value)) > + return -EINVAL; > + Check "value" here and bail out with an error code if 0. > + if (!drvdata->enable) { > + ret = pm_runtime_get_sync(drvdata->dev); > + if (ret < 0) { > + pm_runtime_put(drvdata->dev); > + return ret; > + } > + } > + > + spin_lock(&drvdata->spinlock); > + CS_UNLOCK(drvdata->base); > + > + if (value) { drop this line > + tgu_writel(drvdata, 0, TGU_CONTROL); > + > + if (drvdata->value_table->priority) > + memset(drvdata->value_table->priority, 0, > + MAX_PRIORITY * drvdata->max_step * > + drvdata->max_reg * sizeof(unsigned int)); > + > + if (drvdata->value_table->condition_decode) > + memset(drvdata->value_table->condition_decode, 0, > + drvdata->max_condition_decode * drvdata->max_step * > + sizeof(unsigned int)); > + > + /* Initialize all condition registers to NOT(value=0x1000000) */ > + for (i = 0; i < drvdata->max_step; i++) { > + for (j = 0; j < drvdata->max_condition_decode; j++) { > + drvdata->value_table > + ->condition_decode[calculate_array_location( > + drvdata, i, TGU_CONDITION_DECODE, j)] = > + 0x1000000; > + } > + } > + > + if (drvdata->value_table->condition_select) > + memset(drvdata->value_table->condition_select, 0, > + drvdata->max_condition_select * drvdata->max_step * > + sizeof(unsigned int)); > + > + if (drvdata->value_table->timer) > + memset(drvdata->value_table->timer, 0, > + (drvdata->max_step) * > + (drvdata->max_timer_counter) * > + sizeof(unsigned int)); > + > + if (drvdata->value_table->counter) > + memset(drvdata->value_table->counter, 0, > + (drvdata->max_step) * > + (drvdata->max_timer_counter) * > + sizeof(unsigned int)); > + > + dev_dbg(dev, "Coresight-TGU reset complete\n"); > + } else { > + dev_dbg(dev, "Coresight-TGU invalid input\n"); not needed if early exit on input errror > + } > + > + CS_LOCK(drvdata->base); > + > + drvdata->enable = false; > + spin_unlock(&drvdata->spinlock); > + pm_runtime_put(drvdata->dev); > + > + return size; > +} > +static DEVICE_ATTR_WO(reset_tgu); > + > static const struct coresight_ops_helper tgu_helper_ops = { > .enable = tgu_enable, > .disable = tgu_disable, > @@ -354,6 +432,7 @@ static const struct coresight_ops tgu_ops = { > > static struct attribute *tgu_common_attrs[] = { > &dev_attr_enable_tgu.attr, > + &dev_attr_reset_tgu.attr, > NULL, > }; > > Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

3 months, 2 weeks

Re: [PATCH v3 3/7] coresight-tgu: Add signal priority support

by Mike Leach

Hi On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai(a)quicinc.com> wrote: > > From: Songwei Chai <quic_songchai(a)quicinc.com> > > Like circuit of a Logic analyzer, in TGU, the requirement could be > configured in each step and the trigger will be created once the > requirements are met. Add priority functionality here to sort the > signals into different priorities. The signal which is wanted could > be configured in each step's priority node, the larger number means > the higher priority and the signal with higher priority will be sensed > more preferentially. > > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com> > Signed-off-by: songchai <quic_songchai(a)quicinc.com> > --- > .../testing/sysfs-bus-coresight-devices-tgu | 7 + > drivers/hwtracing/coresight/coresight-tgu.c | 139 ++++++++++++++++++ > drivers/hwtracing/coresight/coresight-tgu.h | 110 ++++++++++++++ > 3 files changed, 256 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > index 741bc9fd9df5..af7332153833 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > @@ -7,3 +7,10 @@ Description: > Accepts only one of the 2 values - 0 or 1. > 0 : disable TGU. > 1 : enable TGU. > + > +What: /sys/bus/coresight/devices/<tgu-name>/step[0:7]_priority[0:3]/reg[0:17] > +Date: February 2025 > +KernelVersion 6.15 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > +Description: > + (RW) Set/Get the sensed siganal with specific step and priority for TGU. sp/siganal/signal > diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c > index da4c04ac1097..f28761619ebe 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.c > +++ b/drivers/hwtracing/coresight/coresight-tgu.c > @@ -17,9 +17,92 @@ > > DEFINE_CORESIGHT_DEVLIST(tgu_devs, "tgu"); > > +static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index, > + int operation_index, int reg_index) > +{ > + int ret = -EINVAL; > + > + ret = operation_index * (drvdata->max_step) * > + (drvdata->max_reg) + step_index * (drvdata->max_reg) > + + reg_index; > + > + return ret; > +} > + > +static ssize_t tgu_dataset_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct tgu_attribute *tgu_attr = > + container_of(attr, struct tgu_attribute, attr); > + > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->priority[calculate_array_location( > + drvdata, tgu_attr->step_index, > + tgu_attr->operation_index, tgu_attr->reg_num)]); > + > +} > + > +static ssize_t tgu_dataset_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t size) > +{ > + unsigned long val; > + ssize_t ret = -EINVAL; > + > + struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev->parent); > + struct tgu_attribute *tgu_attr = > + container_of(attr, struct tgu_attribute, attr); > + > + if (kstrtoul(buf, 0, &val)) > + return ret; > + > + guard(spinlock)(&tgu_drvdata->spinlock); > + tgu_drvdata->value_table->priority[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + > + return ret; ret is unneeded - directly return either size or -EINVAL. > +} > + > +static umode_t tgu_node_visible(struct kobject *kobject, struct attribute *attr, > + int n) > +{ > + struct device *dev = kobj_to_dev(kobject); > + struct tgu_drvdata *drvdata = dev_get_drvdata(dev->parent); > + int ret = 0; > + > + struct device_attribute *dev_attr = > + container_of(attr, struct device_attribute, attr); > + struct tgu_attribute *tgu_attr = > + container_of(dev_attr, struct tgu_attribute, attr); > + > + if (tgu_attr->step_index < drvdata->max_step) { > + ret = (tgu_attr->reg_num < drvdata->max_reg) ? > + attr->mode : 0; > + return ret; > + } > + return SYSFS_GROUP_INVISIBLE; > +} default ret as SYSFS_GROUP_INVISIBLE, and returnret at end > + > static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata) > { > + int i, j, k; > + > CS_UNLOCK(drvdata->base); > + > + for (i = 0; i < drvdata->max_step; i++) { > + for (j = 0; j < MAX_PRIORITY; j++) { > + for (k = 0; k < drvdata->max_reg; k++) { > + tgu_writel(drvdata, > + drvdata->value_table->priority > + [calculate_array_location(drvdata, i, j, k)], > + PRIORITY_REG_STEP(i, j, k)); > + } > + } > + } > + > /* Enable TGU to program the triggers */ > tgu_writel(drvdata, 1, TGU_CONTROL); > CS_LOCK(drvdata->base); > @@ -130,6 +213,38 @@ static const struct attribute_group tgu_common_grp = { > > static const struct attribute_group *tgu_attr_groups[] = { > &tgu_common_grp, > + PRIORITY_ATTRIBUTE_GROUP_INIT(0, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(0, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(0, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(0, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(1, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(1, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(1, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(1, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(2, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(2, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(2, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(2, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(3, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(3, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(3, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(3, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(4, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(4, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(4, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(4, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(5, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(5, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(5, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(5, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(6, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(6, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(6, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(6, 3), > + PRIORITY_ATTRIBUTE_GROUP_INIT(7, 0), > + PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1), > + PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2), > + PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3), > NULL, > }; > > @@ -164,6 +279,30 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > > spin_lock_init(&drvdata->spinlock); > > + ret = of_property_read_u32(adev->dev.of_node, "tgu-regs", > + &drvdata->max_reg); > + if (ret) > + return -EINVAL; > + > + ret = of_property_read_u32(adev->dev.of_node, "tgu-steps", > + &drvdata->max_step); > + if (ret) > + return -EINVAL; > + > + drvdata->value_table = > + devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL); > + if (!drvdata->value_table) > + return -ENOMEM; > + > + drvdata->value_table->priority = devm_kzalloc( > + dev, > + MAX_PRIORITY * drvdata->max_reg * drvdata->max_step * > + sizeof(*(drvdata->value_table->priority)), > + GFP_KERNEL); > + > + if (!drvdata->value_table->priority) > + return -ENOMEM; > + > drvdata->enable = false; > desc.type = CORESIGHT_DEV_TYPE_HELPER; > desc.pdata = adev->dev.platform_data; > diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h > index 380686f94130..6e5d465117df 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.h > +++ b/drivers/hwtracing/coresight/coresight-tgu.h > @@ -13,6 +13,110 @@ > #define tgu_writel(drvdata, val, off) __raw_writel((val), drvdata->base + off) > #define tgu_readl(drvdata, off) __raw_readl(drvdata->base + off) > > +/* > + * TGU configuration space Step configuration > + * offset table space layout > + * x-------------------------x$ x-------------x$ > + * | |$ | |$ > + * | | | reserve |$ > + * | | | |$ > + * |coresight management | |-------------|base+n*0x1D8+0x1F4$ > + * | registe | |---> |prioroty[3] |$ > + * | | | |-------------|base+n*0x1D8+0x194$ > + * | | | |prioroty[2] |$ > + * |-------------------------| | |-------------|base+n*0x1D8+0x134$ > + * | | | |prioroty[1] |$ > + * | step[7] | | |-------------|base+n*0x1D8+0xD4$ > + * |-------------------------|->base+0x40+7*0x1D8 | |prioroty[0] |$ > + * | | | |-------------|base+n*0x1D8+0x74$ > + * | ... | | | condition |$ > + * | | | | select |$ > + * |-------------------------|->base+0x40+1*0x1D8 | |-------------|base+n*0x1D8+0x60$ > + * | | | | condition |$ > + * | step[0] |--------------------> | decode |$ > + * |-------------------------|-> base+0x40 |-------------|base+n*0x1D8+0x50$ > + * | | | |$ > + * | Control and status space| |Timer/Counter|$ > + * | space | | |$ > + * x-------------------------x->base x-------------x base+n*0x1D8+0x40$ > + * > + */ > + > +/* Calculate compare step addresses */ > +#define PRIORITY_REG_STEP(step, priority, reg)\ > + (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step) > + use #defines + explanation instead of arbitrary magic numbers > +#define tgu_dataset_ro(name, step_index, type, reg_num) \ > + (&((struct tgu_attribute[]){ { \ > + __ATTR(name, 0444, tgu_dataset_show, NULL), \ > + step_index, \ > + type, \ > + reg_num, \ > + } })[0].attr.attr) > + This define unused in this patch, Drop till it is used. > +#define tgu_dataset_rw(name, step_index, type, reg_num) \ > + (&((struct tgu_attribute[]){ { \ > + __ATTR(name, 0644, tgu_dataset_show, tgu_dataset_store), \ > + step_index, \ > + type, \ > + reg_num, \ > + } })[0].attr.attr) > + > +#define STEP_PRIORITY(step_index, reg_num, priority) \ > + tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \ > + reg_num) > + > +#define STEP_PRIORITY_LIST(step_index, priority) \ > + {STEP_PRIORITY(step_index, 0, priority), \ > + STEP_PRIORITY(step_index, 1, priority), \ > + STEP_PRIORITY(step_index, 2, priority), \ > + STEP_PRIORITY(step_index, 3, priority), \ > + STEP_PRIORITY(step_index, 4, priority), \ > + STEP_PRIORITY(step_index, 5, priority), \ > + STEP_PRIORITY(step_index, 6, priority), \ > + STEP_PRIORITY(step_index, 7, priority), \ > + STEP_PRIORITY(step_index, 8, priority), \ > + STEP_PRIORITY(step_index, 9, priority), \ > + STEP_PRIORITY(step_index, 10, priority), \ > + STEP_PRIORITY(step_index, 11, priority), \ > + STEP_PRIORITY(step_index, 12, priority), \ > + STEP_PRIORITY(step_index, 13, priority), \ > + STEP_PRIORITY(step_index, 14, priority), \ > + STEP_PRIORITY(step_index, 15, priority), \ > + STEP_PRIORITY(step_index, 16, priority), \ > + STEP_PRIORITY(step_index, 17, priority), \ > + NULL \ > + } > + > +#define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\ > + (&(const struct attribute_group){\ > + .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, priority),\ > + .is_visible = tgu_node_visible,\ > + .name = "step" #step "_priority" #priority \ > + }) > + > +enum operation_index { > + TGU_PRIORITY0, > + TGU_PRIORITY1, > + TGU_PRIORITY2, > + TGU_PRIORITY3 > + > +}; > + > +/* Maximum priority that TGU supports */ > +#define MAX_PRIORITY 4 > + > +struct tgu_attribute { > + struct device_attribute attr; > + u32 step_index; > + enum operation_index operation_index; > + u32 reg_num; > +}; > + > +struct value_table { > + unsigned int *priority; > +}; > + > /** > * struct tgu_drvdata - Data structure for a TGU (Trigger Generator Unit) device > * @base: Memory-mapped base address of the TGU device > @@ -20,6 +124,9 @@ > * @csdev: Pointer to the associated coresight device > * @spinlock: Spinlock for handling concurrent access > * @enable: Flag indicating whether the TGU device is enabled > + * @value_table: Store given value based on relevant parameters. > + * @max_reg: Maximum number of registers > + * @max_step: Maximum step size > * > * This structure defines the data associated with a TGU device, including its base > * address, device pointers, clock, spinlock for synchronization, trigger data pointers, > @@ -31,6 +138,9 @@ struct tgu_drvdata { > struct coresight_device *csdev; > spinlock_t spinlock; > bool enable; > + struct value_table *value_table; > + int max_reg; > + int max_step; > }; > > #endif Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

3 months, 2 weeks

Re: [PATCH v3 4/7] coresight-tgu: Add TGU decode support

by Mike Leach

Hi, On Thu, 27 Feb 2025 at 09:27, songchai <quic_songchai(a)quicinc.com> wrote: > > From: Songwei Chai <quic_songchai(a)quicinc.com> > > Decoding is when all the potential pieces for creating a trigger > are brought together for a given step. Example - there may be a > counter keeping track of some occurrences and a priority-group that > is being used to detect a pattern on the sense inputs. These 2 > inputs to condition_decode must be programmed, for a given step, > to establish the condition for the trigger, or movement to another > step. > > Signed-off-by: Songwei Chai <quic_songchai(a)quicinc.com> > Signed-off-by: songchai <quic_songchai(a)quicinc.com> > --- > .../testing/sysfs-bus-coresight-devices-tgu | 7 ++ > drivers/hwtracing/coresight/coresight-tgu.c | 113 ++++++++++++++++-- > drivers/hwtracing/coresight/coresight-tgu.h | 29 ++++- > 3 files changed, 136 insertions(+), 13 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > index af7332153833..dd6cc1184d52 100644 > --- a/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > +++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-tgu > @@ -14,3 +14,10 @@ KernelVersion 6.15 > Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > Description: > (RW) Set/Get the sensed siganal with specific step and priority for TGU. > + > +What: /sys/bus/coresight/devices/<tgu-name>/step[0:7]_condition_decode/reg[0:3] > +Date: February 2025 > +KernelVersion 6.15 > +Contact: Jinlong Mao (QUIC) <quic_jinlmao(a)quicinc.com>, Sam Chai (QUIC) <quic_songchai(a)quicinc.com> > +Description: > + (RW) Set/Get the decode mode with specific step for TGU. > diff --git a/drivers/hwtracing/coresight/coresight-tgu.c b/drivers/hwtracing/coresight/coresight-tgu.c > index f28761619ebe..5eebf5eecbbb 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.c > +++ b/drivers/hwtracing/coresight/coresight-tgu.c > @@ -22,9 +22,21 @@ static int calculate_array_location(struct tgu_drvdata *drvdata, int step_index, > { > int ret = -EINVAL; > > - ret = operation_index * (drvdata->max_step) * > - (drvdata->max_reg) + step_index * (drvdata->max_reg) > - + reg_index; > + switch (operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + ret = operation_index * (drvdata->max_step) * > + (drvdata->max_reg) + step_index * (drvdata->max_reg) > + + reg_index; > + break; > + case TGU_CONDITION_DECODE: > + ret = step_index * (drvdata->max_condition_decode) + reg_index; > + break; > + default: > + break; > + } > > return ret; > } > @@ -36,10 +48,23 @@ static ssize_t tgu_dataset_show(struct device *dev, > struct tgu_attribute *tgu_attr = > container_of(attr, struct tgu_attribute, attr); > > - return sysfs_emit(buf, "0x%x\n", > - drvdata->value_table->priority[calculate_array_location( > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->priority[calculate_array_location( calculate_array_location() can return -EINVAL - you could be referencing array value drvdata->value_table->priority[-EINVAL] here. Test the return before referencing the array. Same for all following occurrences. > drvdata, tgu_attr->step_index, > tgu_attr->operation_index, tgu_attr->reg_num)]); > + case TGU_CONDITION_DECODE: > + return sysfs_emit(buf, "0x%x\n", > + drvdata->value_table->condition_decode[calculate_array_location( > + drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)]); > + missing default - did this code compile without warnings? > + } > + return -EINVAL; > > } > > @@ -58,11 +83,25 @@ static ssize_t tgu_dataset_store(struct device *dev, > return ret; > > guard(spinlock)(&tgu_drvdata->spinlock); > - tgu_drvdata->value_table->priority[calculate_array_location( > - tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > - tgu_attr->reg_num)] = val; > - ret = size; > - > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + tgu_drvdata->value_table->priority[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + break; > + case TGU_CONDITION_DECODE: > + tgu_drvdata->value_table->condition_decode[calculate_array_location( > + tgu_drvdata, tgu_attr->step_index, tgu_attr->operation_index, > + tgu_attr->reg_num)] = val; > + ret = size; > + break; > + default: > + break; > + } > return ret; > } > > @@ -79,8 +118,23 @@ static umode_t tgu_node_visible(struct kobject *kobject, struct attribute *attr, > container_of(dev_attr, struct tgu_attribute, attr); > > if (tgu_attr->step_index < drvdata->max_step) { > - ret = (tgu_attr->reg_num < drvdata->max_reg) ? > - attr->mode : 0; > + switch (tgu_attr->operation_index) { > + case TGU_PRIORITY0: > + case TGU_PRIORITY1: > + case TGU_PRIORITY2: > + case TGU_PRIORITY3: > + ret = (tgu_attr->reg_num < drvdata->max_reg) ? > + attr->mode : 0; > + break; > + case TGU_CONDITION_DECODE: > + ret = (tgu_attr->reg_num < > + drvdata->max_condition_decode) ? > + attr->mode : 0; > + break; > + default: > + break; > + } > + > return ret; > } > return SYSFS_GROUP_INVISIBLE; > @@ -103,6 +157,17 @@ static void tgu_write_all_hw_regs(struct tgu_drvdata *drvdata) > } > } > > + for (i = 0; i < drvdata->max_step; i++) { > + for (j = 0; j < drvdata->max_condition_decode; j++) { > + tgu_writel(drvdata, > + drvdata->value_table > + ->condition_decode[calculate_array_location( > + drvdata, i, > + TGU_CONDITION_DECODE, j)], > + CONDITION_DECODE_STEP(i, j)); > + } > + } > + > /* Enable TGU to program the triggers */ > tgu_writel(drvdata, 1, TGU_CONTROL); > CS_LOCK(drvdata->base); > @@ -245,6 +310,14 @@ static const struct attribute_group *tgu_attr_groups[] = { > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 1), > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 2), > PRIORITY_ATTRIBUTE_GROUP_INIT(7, 3), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(0), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(1), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(2), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(3), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(4), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6), > + CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7), > NULL, > }; > > @@ -289,6 +362,13 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > if (ret) > return -EINVAL; > > + ret = of_property_read_u32(adev->dev.of_node, "tgu-conditions", > + &drvdata->max_condition); > + if (ret) > + return -EINVAL; > + > + drvdata->max_condition_decode = drvdata->max_condition; > + > drvdata->value_table = > devm_kzalloc(dev, sizeof(*drvdata->value_table), GFP_KERNEL); > if (!drvdata->value_table) > @@ -303,6 +383,15 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id) > if (!drvdata->value_table->priority) > return -ENOMEM; > > + drvdata->value_table->condition_decode = devm_kzalloc( > + dev, > + drvdata->max_condition_decode * drvdata->max_step * > + sizeof(*(drvdata->value_table->condition_decode)), > + GFP_KERNEL); > + > + if (!drvdata->value_table->condition_decode) > + return -ENOMEM; > + > drvdata->enable = false; > desc.type = CORESIGHT_DEV_TYPE_HELPER; > desc.pdata = adev->dev.platform_data; > diff --git a/drivers/hwtracing/coresight/coresight-tgu.h b/drivers/hwtracing/coresight/coresight-tgu.h > index 6e5d465117df..c2a9ce38b44f 100644 > --- a/drivers/hwtracing/coresight/coresight-tgu.h > +++ b/drivers/hwtracing/coresight/coresight-tgu.h > @@ -46,6 +46,9 @@ > #define PRIORITY_REG_STEP(step, priority, reg)\ > (0x0074 + 0x60 * priority + 0x4 * reg + 0x1D8 * step) > > +#define CONDITION_DECODE_STEP(step, decode) \ > + (0x0050 + 0x4 * decode + 0x1D8 * step) > + use #define constants with explanations of what they are rather than arbitrary magic numbers. > #define tgu_dataset_ro(name, step_index, type, reg_num) \ > (&((struct tgu_attribute[]){ { \ > __ATTR(name, 0444, tgu_dataset_show, NULL), \ > @@ -66,6 +69,9 @@ > tgu_dataset_rw(reg##reg_num, step_index, TGU_PRIORITY##priority, \ > reg_num) > > +#define STEP_DECODE(step_index, reg_num) \ > + tgu_dataset_rw(reg##reg_num, step_index, TGU_CONDITION_DECODE, reg_num) > + > #define STEP_PRIORITY_LIST(step_index, priority) \ > {STEP_PRIORITY(step_index, 0, priority), \ > STEP_PRIORITY(step_index, 1, priority), \ > @@ -88,6 +94,14 @@ > NULL \ > } > > +#define STEP_DECODE_LIST(n) \ > + {STEP_DECODE(n, 0), \ > + STEP_DECODE(n, 1), \ > + STEP_DECODE(n, 2), \ > + STEP_DECODE(n, 3), \ > + NULL \ > + } > + > #define PRIORITY_ATTRIBUTE_GROUP_INIT(step, priority)\ > (&(const struct attribute_group){\ > .attrs = (struct attribute*[])STEP_PRIORITY_LIST(step, priority),\ > @@ -95,11 +109,19 @@ > .name = "step" #step "_priority" #priority \ > }) > > +#define CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(step)\ > + (&(const struct attribute_group){\ > + .attrs = (struct attribute*[])STEP_DECODE_LIST(step),\ > + .is_visible = tgu_node_visible,\ > + .name = "step" #step "_condition_decode" \ > + }) > + > enum operation_index { > TGU_PRIORITY0, > TGU_PRIORITY1, > TGU_PRIORITY2, > - TGU_PRIORITY3 > + TGU_PRIORITY3, > + TGU_CONDITION_DECODE > > }; > > @@ -115,6 +137,7 @@ struct tgu_attribute { > > struct value_table { > unsigned int *priority; > + unsigned int *condition_decode; > }; > > /** > @@ -127,6 +150,8 @@ struct value_table { > * @value_table: Store given value based on relevant parameters. > * @max_reg: Maximum number of registers > * @max_step: Maximum step size > + * @max_condition: Maximum number of condition > + * @max_condition_decode: Maximum number of condition_decode > * > * This structure defines the data associated with a TGU device, including its base > * address, device pointers, clock, spinlock for synchronization, trigger data pointers, > @@ -141,6 +166,8 @@ struct tgu_drvdata { > struct value_table *value_table; > int max_reg; > int max_step; > + int max_condition; > + int max_condition_decode; > }; > > #endif > Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

3 months, 2 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight