CoreSight January 2025

coresight@lists.linaro.org

8 participants
43 discussions

Re: [PATCH v10 04/10] arm64/sysreg: Add a comment that the sysreg file should be sorted

by James Clark

On 14/01/2025 6:16 pm, Rob Herring wrote: > On Mon, Jan 13, 2025 at 10:49 AM Marc Zyngier <maz(a)kernel.org> wrote: >> >> On Mon, 13 Jan 2025 15:43:39 +0000, >> James Clark <james.clark(a)linaro.org> wrote: >>> >>> >>> >>> On 12/01/2025 12:49 pm, Marc Zyngier wrote: >>>> On Tue, 07 Jan 2025 11:32:41 +0000, >>>> James Clark <james.clark(a)linaro.org> wrote: >>>>> >>>>> From: James Clark <james.clark(a)arm.com> >>>>> >>>>> There are a few entries particularly at the end of the file that aren't >>>>> in order. To avoid confusion, add a comment that might help new entries >>>>> to be added in the right place. >>>>> >>>>> Reviewed-by: Mark Brown <broonie(a)kernel.org> >>>>> Signed-off-by: James Clark <james.clark(a)arm.com> >>>>> Signed-off-by: James Clark <james.clark(a)linaro.org> >>>>> --- >>>>> arch/arm64/tools/sysreg | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg >>>>> index b081b54d6d22..4ba167089e2a 100644 >>>>> --- a/arch/arm64/tools/sysreg >>>>> +++ b/arch/arm64/tools/sysreg >>>>> @@ -48,6 +48,8 @@ >>>>> # feature that introduces them (eg, FEAT_LS64_ACCDATA introduces enumeration >>>>> # item ACCDATA) though it may be more taseful to do something else. >>>>> +# Please try to keep entries in this file sorted by sysreg >>>>> encoding. >>>>> + >>>>> Sysreg OSDTRRX_EL1 2 0 0 0 2 >>>>> Res0 63:32 >>>>> Field 31:0 DTRRX >>>> >>>> "Do as I say, don't do as I do". >>>> >>>> I don't think this makes any sense if we don't actually sort the file >>>> the first place. >>>> >>>> M. >>>> >>> >>> I think it's ok if it avoids review comments that new entries should >>> be sorted. Or maybe we do the opposite and the comment should say this >>> file is allowed to be unsorted... >> >> The better option would be to add the comment *and* sort the file. >> Leading by example has some value, it seems. > > IME, it's better if documentation just states what the tools enforce. > > Can't we add something like this to the header generation: > > $ grep '^Sysreg\s' arch/arm64/tools/sysreg | sort -n -k3 -k4 -k5 -k6 -k7 -c > sort: -:22: disorder: Sysreg ID_MMFR4_EL1 3 0 0 > 2 6 > > Rob Actually I updated gen-sysreg.awk to fail the build if it's not sorted, was just about to post it. I don't know if a build failure or a warning is preferred but I can do either. James

3 months, 3 weeks

Re: [PATCH FOR-NEXT] perf tools: Expose quiet/verbose variables in Makefile.perf

by Arnaldo Carvalho de Melo

On Tue, Jan 14, 2025 at 11:35:44AM -0800, Charlie Jenkins wrote: > The variables to make builds silent/verbose live inside > tools/build/Makefile.build. Move those variables to the top-level > Makefile.perf to be generally available. > > Committer testing: Re-tested, now the lines below continue to appear, and also the 'make -C tools/perf build-test' that was failiong on the first patch submission is now ok as well, so I re issue my: Tested-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> At this point this will go thru Namhyung, that will process patches while I'm in vacation for perf-tools-next as well, thanks! - Arnaldo > See the SYSCALL lines, now they are consistent with the other > operations in other lines: > SYSTBL /tmp/build/perf-tools-next/arch/x86/include/generated/asm/syscalls_32.h > SYSTBL /tmp/build/perf-tools-next/arch/x86/include/generated/asm/syscalls_64.h > GEN /tmp/build/perf-tools-next/common-cmds.h > GEN /tmp/build/perf-tools-next/arch/arm64/include/generated/asm/sysreg-defs.h > PERF_VERSION = 6.13.rc2.g3d94bb6ed1d0 > GEN perf-archive > MKDIR /tmp/build/perf-tools-next/jvmti/ > MKDIR /tmp/build/perf-tools-next/jvmti/ > MKDIR /tmp/build/perf-tools-next/jvmti/ > MKDIR /tmp/build/perf-tools-next/jvmti/ > GEN perf-iostat > CC /tmp/build/perf-tools-next/jvmti/libjvmti.o > > Reported-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> > Signed-off-by: Charlie Jenkins <charlie(a)rivosinc.com> > Tested-by: Arnaldo Carvalho de Melo <acme(a)redhat.com> > --- > tools/build/Makefile.build | 20 ----------------- > tools/perf/Makefile.perf | 37 ++++++++++++++++++++++++++++++- > tools/perf/tests/shell/coresight/Makefile | 2 +- > 3 files changed, 37 insertions(+), 22 deletions(-) > > diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build > index 5fb3fb3d97e0fd114e245805809e4fc926b4343e..e710ed67a1b49d9fda11db02821bbd8d36066b44 100644 > --- a/tools/build/Makefile.build > +++ b/tools/build/Makefile.build > @@ -12,26 +12,6 @@ > PHONY := __build > __build: > > -ifeq ($(V),1) > - quiet = > - Q = > -else > - quiet=quiet_ > - Q=@ > -endif > - > -# If the user is running make -s (silent mode), suppress echoing of commands > -# make-4.0 (and later) keep single letter options in the 1st word of MAKEFLAGS. > -ifeq ($(filter 3.%,$(MAKE_VERSION)),) > -short-opts := $(firstword -$(MAKEFLAGS)) > -else > -short-opts := $(filter-out --%,$(MAKEFLAGS)) > -endif > - > -ifneq ($(findstring s,$(short-opts)),) > - quiet=silent_ > -endif > - > build-dir := $(srctree)/tools/build > > # Define $(fixdep) for dep-cmd function > diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf > index a449d0015536442273a9268b37be34e4757f577a..55d6ce9ea52fb2a57b8632cc6d0ddc501e29cbfc 100644 > --- a/tools/perf/Makefile.perf > +++ b/tools/perf/Makefile.perf > @@ -161,12 +161,47 @@ export VPATH > SOURCE := $(shell ln -sf $(srctree)/tools/perf $(OUTPUT)/source) > endif > > +# Beautify output > +# --------------------------------------------------------------------------- > +# > +# Most of build commands in Kbuild start with "cmd_". You can optionally define > +# "quiet_cmd_*". If defined, the short log is printed. Otherwise, no log from > +# that command is printed by default. > +# > +# e.g.) > +# quiet_cmd_depmod = DEPMOD $(MODLIB) > +# cmd_depmod = $(srctree)/scripts/depmod.sh $(DEPMOD) $(KERNELRELEASE) > +# > +# A simple variant is to prefix commands with $(Q) - that's useful > +# for commands that shall be hidden in non-verbose mode. > +# > +# $(Q)$(MAKE) $(build)=scripts/basic > +# > +# To put more focus on warnings, be less verbose as default > +# Use 'make V=1' to see the full commands > + > ifeq ($(V),1) > + quiet = > Q = > else > - Q = @ > + quiet=quiet_ > + Q=@ > endif > > +# If the user is running make -s (silent mode), suppress echoing of commands > +# make-4.0 (and later) keep single letter options in the 1st word of MAKEFLAGS. > +ifeq ($(filter 3.%,$(MAKE_VERSION)),) > +short-opts := $(firstword -$(MAKEFLAGS)) > +else > +short-opts := $(filter-out --%,$(MAKEFLAGS)) > +endif > + > +ifneq ($(findstring s,$(short-opts)),) > + quiet=silent_ > +endif > + > +export quiet Q > + > # Do not use make's built-in rules > # (this improves performance and avoids hard-to-debug behaviour); > MAKEFLAGS += -r > diff --git a/tools/perf/tests/shell/coresight/Makefile b/tools/perf/tests/shell/coresight/Makefile > index b070e779703e9fbd70f88c826172b2150ee3d302..fa08fd9a5991dd889583edc1afe8304e72278f64 100644 > --- a/tools/perf/tests/shell/coresight/Makefile > +++ b/tools/perf/tests/shell/coresight/Makefile > @@ -24,6 +24,6 @@ CLEANDIRS = $(SUBDIRS:%=clean-%) > > clean: $(CLEANDIRS) > $(CLEANDIRS): > - $(call QUIET_CLEAN, test-$(@:clean-%=%)) $(Q)$(MAKE) -C $(@:clean-%=%) clean >/dev/null > + $(call QUIET_CLEAN, test-$(@:clean-%=%)) $(MAKE) -C $(@:clean-%=%) clean >/dev/null > > .PHONY: all clean $(SUBDIRS) $(CLEANDIRS) $(INSTALLDIRS) > > --- > base-commit: e9cbc854d8b148e3491291fb615e94261970fb54 > change-id: 20250114-perf_make_test-1141d4ad8877 > -- > - Charlie

3 months, 3 weeks

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

On 14/01/2025 2:51 am, Jie Gan wrote: > > > On 1/13/2025 8:02 PM, James Clark wrote: >> >> >> On 26/12/2024 1:10 am, Jie Gan wrote: >>> Add 'trace_id' function pointer in ops. It's responsible for >>> retrieving the device's trace ID. >>> >>> Add 'struct cs_sink_data' to store the data that is needed by >>> coresight_enable_path/coresight_disable_path. The structure >>> will be transmitted to the helper and sink device to enable >>> related funcationalities. >>> >> >> The new cs_sink_data struct is quite specific to this change. Can we >> start passing the path around to enable/disable functions, that will >> allow devices to gather anything they want in the future. Because we >> already have coresight_get_sink(path), coresight_get_source(path) etc. >> >> And see below, but for this case we can also change the path struct to >> contain the trace ID. Then all the new functions, allocations and >> searches for the trace ID are unecessary. The CTCU will have access to >> the path, and by the time its enable function is called the trace ID >> is already assigned. >> >> It's also easier to understand at which point a trace ID is allocated, >> rather than adding the trace_id() callbacks from everywhere which >> could potentially either read or allocate. I suppose that's "safer" >> because maybe it's not allocated, but I can't see what case it would >> happen in reverse. >> > Thank you for comment. I will try this solution. > The biggest challenge for the patch is how to correctly read trace_id > from source device and passthrough it to helper device as the source > device always the last one to enable. I believe your proposed solution > is better than mine and has minimal impact on the basic framework, but I > think we still need read_trace in source_ops and link_ops. Then we can > read the trace_id in coresight_build_path function and save it to the > coresight_path to avoid redundant searching? > > >>> Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> >>> --- >>> drivers/hwtracing/coresight/coresight-core.c | 59 +++++++++++++++---- >>> drivers/hwtracing/coresight/coresight-etb10.c | 3 +- >>> .../hwtracing/coresight/coresight-etm-perf.c | 37 ++++++++++-- >>> .../coresight/coresight-etm3x-core.c | 30 ++++++++++ >>> .../coresight/coresight-etm4x-core.c | 29 +++++++++ >>> drivers/hwtracing/coresight/coresight-priv.h | 13 +++- >>> drivers/hwtracing/coresight/coresight-stm.c | 22 +++++++ >>> drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++- >>> .../hwtracing/coresight/coresight-tmc-etf.c | 3 +- >>> .../hwtracing/coresight/coresight-tmc-etr.c | 6 +- >>> drivers/hwtracing/coresight/coresight-tpda.c | 20 +++++++ >>> drivers/hwtracing/coresight/coresight-trbe.c | 4 +- >>> drivers/hwtracing/coresight/ultrasoc-smb.c | 3 +- >>> include/linux/coresight.h | 6 ++ >>> 14 files changed, 234 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ >>> hwtracing/coresight/coresight-core.c >>> index 0a9380350fb5..2e560b425fd4 100644 >>> --- a/drivers/hwtracing/coresight/coresight-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-core.c >>> @@ -23,6 +23,7 @@ >>> #include "coresight-etm-perf.h" >>> #include "coresight-priv.h" >>> #include "coresight-syscfg.h" >>> +#include "coresight-trace-id.h" >>> /* >>> * Mutex used to lock all sysfs enable and disable actions and >>> loading and >>> @@ -331,12 +332,12 @@ static int coresight_enable_helper(struct >>> coresight_device *csdev, >>> return helper_ops(csdev)->enable(csdev, mode, data); >>> } >>> -static void coresight_disable_helper(struct coresight_device *csdev) >>> +static void coresight_disable_helper(struct coresight_device *csdev, >>> void *data) >>> { >>> - helper_ops(csdev)->disable(csdev, NULL); >>> + helper_ops(csdev)->disable(csdev, data); >>> } >>> -static void coresight_disable_helpers(struct coresight_device *csdev) >>> +static void coresight_disable_helpers(struct coresight_device >>> *csdev, void *data) >>> { >>> int i; >>> struct coresight_device *helper; >>> @@ -344,7 +345,7 @@ static void coresight_disable_helpers(struct >>> coresight_device *csdev) >>> for (i = 0; i < csdev->pdata->nr_outconns; ++i) { >>> helper = csdev->pdata->out_conns[i]->dest_dev; >>> if (helper && coresight_is_helper(helper)) >>> - coresight_disable_helper(helper); >>> + coresight_disable_helper(helper, data); >>> } >>> } >>> @@ -361,7 +362,7 @@ static void coresight_disable_helpers(struct >>> coresight_device *csdev) >>> void coresight_disable_source(struct coresight_device *csdev, void >>> *data) >>> { >>> source_ops(csdev)->disable(csdev, data); >>> - coresight_disable_helpers(csdev); >>> + coresight_disable_helpers(csdev, NULL); >>> } >>> EXPORT_SYMBOL_GPL(coresight_disable_source); >>> @@ -371,7 +372,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source); >>> * disabled. >>> */ >>> static void coresight_disable_path_from(struct list_head *path, >>> - struct coresight_node *nd) >>> + struct coresight_node *nd, >>> + void *sink_data) >>> { >>> u32 type; >>> struct coresight_device *csdev, *parent, *child; >>> @@ -417,13 +419,13 @@ static void coresight_disable_path_from(struct >>> list_head *path, >>> } >>> /* Disable all helpers adjacent along the path last */ >>> - coresight_disable_helpers(csdev); >>> + coresight_disable_helpers(csdev, sink_data); >>> } >>> } >>> -void coresight_disable_path(struct list_head *path) >>> +void coresight_disable_path(struct list_head *path, void *sink_data) >>> { >>> - coresight_disable_path_from(path, NULL); >>> + coresight_disable_path_from(path, NULL, sink_data); >>> } >>> EXPORT_SYMBOL_GPL(coresight_disable_path); >>> @@ -505,10 +507,47 @@ int coresight_enable_path(struct list_head >>> *path, enum cs_mode mode, >>> out: >>> return ret; >>> err: >>> - coresight_disable_path_from(path, nd); >>> + coresight_disable_path_from(path, nd, sink_data); >>> goto out; >>> } >>> +int coresight_read_traceid(struct list_head *path, enum cs_mode mode, >>> + struct coresight_trace_id_map *id_map) >>> +{ >>> + int trace_id, type; >>> + struct coresight_device *csdev; >>> + struct coresight_node *nd; >>> + >>> + list_for_each_entry(nd, path, link) { >> >> What do you think about also changing the path to this: >> >> struct coresight_path { >> struct list_head *path, >> u8 trace_id >> }; >> > That's better, I can simplify the coresight_read_traceid function > without traverse the path. > > But we still need to check the type of the coresight device, because the > TPDM does not have traceid and we use the trace_id from the TPDA device > that the TPDM connected. That's why I added trace_id to link_ops. > But if any device that allocates a trace ID saves it into the path, then as long as any other device that needs the ID is enabled after that it just reads it from the path directly. Assuming we pass the path to every enable and disable function. We wouldn't need coresight_read_traceid() if it always happens that way around, which I think it currently does? > >> That would avoid having to traverse the path on every enable and would >> remove this function. You could also cache the trace ID in the CTCU >> for a similar benefit, but it wouldn't remove the need to call this at >> least once. >> >> The expensive part should be the create path part, after that enable >> and disable should be cheap because they happen on schedule for Perf >> mode. We should be avoiding allocations and searches. >> > I can move the read trace_id logic to build_path function, I believe > it's better than current solution. > >>> + csdev = nd->csdev; >>> + type = csdev->type; >>> + >>> + switch (type) { >>> + case CORESIGHT_DEV_TYPE_SOURCE: >>> + if (source_ops(csdev)->trace_id != NULL) { >>> + trace_id = source_ops(csdev)->trace_id(csdev, >>> + mode, >>> + id_map); >>> + if (IS_VALID_CS_TRACE_ID(trace_id)) >>> + goto out; >>> + } >>> + break; >>> + case CORESIGHT_DEV_TYPE_LINK: >>> + if (link_ops(csdev)->trace_id != NULL) { >>> + trace_id = link_ops(csdev)->trace_id(csdev); >>> + if (IS_VALID_CS_TRACE_ID(trace_id)) >>> + goto out; >>> + } >>> + break; >>> + default: >>> + break; >>> + } >>> + } >>> + return -EINVAL; >>> +out: >>> + return trace_id; >>> +} >>> + >>> struct coresight_device *coresight_get_sink(struct list_head *path) >>> { >>> struct coresight_device *csdev; >>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/ >>> hwtracing/coresight/coresight-etb10.c >>> index aea9ac9c4bd0..904b5531c256 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etb10.c >>> +++ b/drivers/hwtracing/coresight/coresight-etb10.c >>> @@ -173,7 +173,8 @@ static int etb_enable_perf(struct >>> coresight_device *csdev, void *data) >>> pid_t pid; >>> unsigned long flags; >>> struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >>> - struct perf_output_handle *handle = data; >>> + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; >>> + struct perf_output_handle *handle = sink_data->handle; >>> struct cs_buffers *buf = etm_perf_sink_config(handle); >>> spin_lock_irqsave(&drvdata->spinlock, flags); >>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/ >>> drivers/hwtracing/coresight/coresight-etm-perf.c >>> index ad6a8f4b70b6..e676edd42ddc 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c >>> @@ -459,6 +459,7 @@ static void etm_event_start(struct perf_event >>> *event, int flags) >>> struct perf_output_handle *handle = &ctxt->handle; >>> struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); >>> struct list_head *path; >>> + struct cs_sink_data *sink_data = NULL; >>> u64 hw_id; >>> u8 trace_id; >>> @@ -498,9 +499,20 @@ static void etm_event_start(struct perf_event >>> *event, int flags) >>> if (WARN_ON_ONCE(!sink)) >>> goto fail_end_stop; >>> + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); >> >> kzalloc can't be called from here. Check dmesg for the warning. That's >> another reason to do this change on the path. Because the path is >> allocated on etm_setup_aux() where allocations are allowed. >> > Will fix it. > > >>> + if (!sink_data) >>> + goto fail_end_stop; >>> + >>> + sink_data->sink = sink; >>> + sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF, >>> + &sink->perf_sink_id_map); >>> + sink_data->handle = handle; >>> + >>> /* Nothing will happen without a path */ >>> - if (coresight_enable_path(path, CS_MODE_PERF, handle)) >>> + if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) { >>> + kfree(sink_data); >>> goto fail_end_stop; >>> + } >>> /* Finally enable the tracer */ >>> if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF, >>> @@ -526,6 +538,7 @@ static void etm_event_start(struct perf_event >>> *event, int flags) >>> perf_report_aux_output_id(event, hw_id); >>> } >>> + kfree(sink_data); >>> out: >>> /* Tell the perf core the event is alive */ >>> event->hw.state = 0; >>> @@ -534,7 +547,8 @@ static void etm_event_start(struct perf_event >>> *event, int flags) >>> return; >>> fail_disable_path: >>> - coresight_disable_path(path); >>> + coresight_disable_path(path, sink_data); >>> + kfree(sink_data); >>> fail_end_stop: >>> /* >>> * Check if the handle is still associated with the event, >>> @@ -559,6 +573,7 @@ static void etm_event_stop(struct perf_event >>> *event, int mode) >>> struct perf_output_handle *handle = &ctxt->handle; >>> struct etm_event_data *event_data; >>> struct list_head *path; >>> + struct cs_sink_data *sink_data = NULL; >>> /* >>> * If we still have access to the event_data via handle, >>> @@ -603,6 +618,10 @@ static void etm_event_stop(struct perf_event >>> *event, int mode) >>> if (!sink) >>> return; >>> + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); >> >> Same problem as above. >> >>> + if (!sink_data) >>> + return; >>> + >>> /* stop tracer */ >>> coresight_disable_source(csdev, event); >>> @@ -616,12 +635,16 @@ static void etm_event_stop(struct perf_event >>> *event, int mode) >>> * have to do anything here. >>> */ >>> if (handle->event && (mode & PERF_EF_UPDATE)) { >>> - if (WARN_ON_ONCE(handle->event != event)) >>> + if (WARN_ON_ONCE(handle->event != event)) { >>> + kfree(sink_data); >>> return; >>> + } >>> /* update trace information */ >>> - if (!sink_ops(sink)->update_buffer) >>> + if (!sink_ops(sink)->update_buffer) { >>> + kfree(sink_data); >>> return; >>> + } >>> size = sink_ops(sink)->update_buffer(sink, handle, >>> event_data->snk_config); >>> @@ -642,8 +665,12 @@ static void etm_event_stop(struct perf_event >>> *event, int mode) >>> WARN_ON(size); >>> } >>> + sink_data->sink = sink; >>> + sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF, >>> + &sink->perf_sink_id_map); >>> /* Disabling the path make its elements available to other >>> sessions */ >>> - coresight_disable_path(path); >>> + coresight_disable_path(path, sink_data); >>> + kfree(sink_data); >>> } >>> static int etm_event_add(struct perf_event *event, int mode) >>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/ >>> drivers/hwtracing/coresight/coresight-etm3x-core.c >>> index c103f4c70f5d..0f095fab7e0a 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c >>> @@ -697,10 +697,40 @@ static void etm_disable(struct coresight_device >>> *csdev, >>> coresight_set_mode(csdev, CS_MODE_DISABLED); >>> } >>> +static int etm_trace_id(struct coresight_device *csdev, >>> + enum cs_mode mode, >>> + struct coresight_trace_id_map *id_map) >>> +{ >>> + int trace_id; >>> + struct etm_drvdata *drvdata; >>> + >>> + if (csdev == NULL) >>> + return -EINVAL; >>> + >>> + drvdata = dev_get_drvdata(csdev->dev.parent); >>> + switch (mode) { >>> + case CS_MODE_SYSFS: >>> + trace_id = etm_read_alloc_trace_id(drvdata); >>> + break; >>> + case CS_MODE_PERF: >>> + trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, >>> id_map); >>> + if (IS_VALID_CS_TRACE_ID(trace_id)) >>> + drvdata->traceid = (u8)trace_id; >>> + break; >>> + default: >>> + trace_id = -EINVAL; >>> + break; >>> + } >>> + >>> + return trace_id; >>> +} >>> + >>> + >>> static const struct coresight_ops_source etm_source_ops = { >>> .cpu_id = etm_cpu_id, >>> .enable = etm_enable, >>> .disable = etm_disable, >>> + .trace_id = etm_trace_id, >>> }; >>> static const struct coresight_ops etm_cs_ops = { >>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ >>> drivers/hwtracing/coresight/coresight-etm4x-core.c >>> index dd8c74f893db..bac9ea371688 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >>> @@ -1025,10 +1025,39 @@ static void etm4_disable(struct >>> coresight_device *csdev, >>> coresight_set_mode(csdev, CS_MODE_DISABLED); >>> } >>> +static int etm4_trace_id(struct coresight_device *csdev, >>> + enum cs_mode mode, >>> + struct coresight_trace_id_map *id_map) >>> +{ >>> + int trace_id; >>> + struct etmv4_drvdata *drvdata; >>> + >>> + if (csdev == NULL) >>> + return -EINVAL; >>> + >>> + drvdata = dev_get_drvdata(csdev->dev.parent); >>> + switch (mode) { >>> + case CS_MODE_SYSFS: >>> + trace_id = etm4_read_alloc_trace_id(drvdata); >>> + break; >>> + case CS_MODE_PERF: >>> + trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, >>> id_map); >>> + if (IS_VALID_CS_TRACE_ID(trace_id)) >>> + drvdata->trcid = (u8)trace_id; >>> + break; >>> + default: >>> + trace_id = -EINVAL; >>> + break; >>> + } >>> + >>> + return trace_id; >>> +} >>> + >>> static const struct coresight_ops_source etm4_source_ops = { >>> .cpu_id = etm4_cpu_id, >>> .enable = etm4_enable, >>> .disable = etm4_disable, >>> + .trace_id = etm4_trace_id, >>> }; >>> static const struct coresight_ops etm4_cs_ops = { >>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/ >>> hwtracing/coresight/coresight-priv.h >>> index 05f891ca6b5c..6a95a3ba9339 100644 >>> --- a/drivers/hwtracing/coresight/coresight-priv.h >>> +++ b/drivers/hwtracing/coresight/coresight-priv.h >>> @@ -105,6 +105,15 @@ struct cs_buffers { >>> void **data_pages; >>> }; >>> +/** >>> + * struct cs_sink_data - data used by coresight_enable_path/ >>> coresight_disable_path >>> + */ >>> +struct cs_sink_data { >>> + struct perf_output_handle *handle; >>> + struct coresight_device *sink; >>> + u8 traceid; >>> +}; >>> + >>> static inline void coresight_insert_barrier_packet(void *buf) >>> { >>> if (buf) >>> @@ -129,9 +138,11 @@ static inline void CS_UNLOCK(void __iomem *addr) >>> } while (0); >>> } >>> -void coresight_disable_path(struct list_head *path); >>> +void coresight_disable_path(struct list_head *path, void *sink_data); >>> int coresight_enable_path(struct list_head *path, enum cs_mode mode, >>> void *sink_data); >>> +int coresight_read_traceid(struct list_head *path, enum cs_mode mode, >>> + struct coresight_trace_id_map *id_map); >>> struct coresight_device *coresight_get_sink(struct list_head *path); >>> struct coresight_device *coresight_get_sink_by_id(u32 id); >>> struct coresight_device * >>> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/ >>> hwtracing/coresight/coresight-stm.c >>> index b581a30a1cd9..5709eda77d9b 100644 >>> --- a/drivers/hwtracing/coresight/coresight-stm.c >>> +++ b/drivers/hwtracing/coresight/coresight-stm.c >>> @@ -281,9 +281,31 @@ static void stm_disable(struct coresight_device >>> *csdev, >>> } >>> } >>> +static int stm_trace_id(struct coresight_device *csdev, >>> + enum cs_mode mode, >>> + struct coresight_trace_id_map *id_map) >>> +{ >>> + int trace_id; >>> + struct stm_drvdata *drvdata; >>> + >>> + if (csdev == NULL) >>> + return -EINVAL; >>> + >>> + drvdata = dev_get_drvdata(csdev->dev.parent); >>> + trace_id = drvdata->traceid; >>> + if (!IS_VALID_CS_TRACE_ID(trace_id)) { >> >> Trace ID is static and always set for STM so this isn't necessary, >> just return drvdata->traceid which would already be valid otherwise >> the device wouldn't be registered. >> > Thanks for comment. > Will fix in next version > > >>> + trace_id = coresight_trace_id_get_system_id(); >>> + if (IS_VALID_CS_TRACE_ID(trace_id)) >>> + drvdata->traceid = (u8)trace_id; >>> + } >>> + >>> + return trace_id; >>> +} >>> + >>> static const struct coresight_ops_source stm_source_ops = { >>> .enable = stm_enable, >>> .disable = stm_disable, >>> + .trace_id = stm_trace_id, >>> }; >>> static const struct coresight_ops stm_cs_ops = { >>> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/ >>> hwtracing/coresight/coresight-sysfs.c >>> index a01c9e54e2ed..8816bccba374 100644 >>> --- a/drivers/hwtracing/coresight/coresight-sysfs.c >>> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c >>> @@ -168,6 +168,7 @@ int coresight_enable_sysfs(struct >>> coresight_device *csdev) >>> int cpu, ret = 0; >>> struct coresight_device *sink; >>> struct list_head *path; >>> + struct cs_sink_data *sink_data; >>> enum coresight_dev_subtype_source subtype; >>> u32 hash; >>> @@ -209,7 +210,14 @@ int coresight_enable_sysfs(struct >>> coresight_device *csdev) >>> goto out; >>> } >>> - ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL); >>> + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); >>> + if (!sink_data) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + sink_data->traceid = coresight_read_traceid(path, CS_MODE_SYSFS, >>> NULL); >>> + sink_data->sink = sink; >>> + ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data); >>> if (ret) >>> goto err_path; >>> @@ -246,15 +254,17 @@ int coresight_enable_sysfs(struct >>> coresight_device *csdev) >>> break; >>> } >>> + kfree(sink_data); >>> out: >>> mutex_unlock(&coresight_mutex); >>> return ret; >>> err_source: >>> - coresight_disable_path(path); >>> + coresight_disable_path(path, sink_data); >>> err_path: >>> coresight_release_path(path); >>> + kfree(sink_data); >>> goto out; >>> } >>> EXPORT_SYMBOL_GPL(coresight_enable_sysfs); >>> @@ -263,6 +273,7 @@ void coresight_disable_sysfs(struct >>> coresight_device *csdev) >>> { >>> int cpu, ret; >>> struct list_head *path = NULL; >>> + struct cs_sink_data *sink_data = NULL; >>> u32 hash; >>> mutex_lock(&coresight_mutex); >>> @@ -274,6 +285,10 @@ void coresight_disable_sysfs(struct >>> coresight_device *csdev) >>> if (!coresight_disable_source_sysfs(csdev, NULL)) >>> goto out; >>> + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); >>> + if (!sink_data) >>> + goto out; >>> + >>> switch (csdev->subtype.source_subtype) { >>> case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: >>> cpu = source_ops(csdev)->cpu_id(csdev); >>> @@ -297,8 +312,11 @@ void coresight_disable_sysfs(struct >>> coresight_device *csdev) >>> break; >>> } >>> - coresight_disable_path(path); >>> + sink_data->sink = coresight_find_activated_sysfs_sink(csdev); >>> + sink_data->traceid = coresight_read_traceid(path, CS_MODE_SYSFS, >>> NULL); >>> + coresight_disable_path(path, sink_data); >>> coresight_release_path(path); >>> + kfree(sink_data); >>> out: >>> mutex_unlock(&coresight_mutex); >>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/ >>> drivers/hwtracing/coresight/coresight-tmc-etf.c >>> index d4f641cd9de6..7dc536eba3e2 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c >>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c >>> @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct >>> coresight_device *csdev, void *data) >>> pid_t pid; >>> unsigned long flags; >>> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >>> - struct perf_output_handle *handle = data; >>> + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; >>> + struct perf_output_handle *handle = sink_data->handle; >>> struct cs_buffers *buf = etm_perf_sink_config(handle); >>> spin_lock_irqsave(&drvdata->spinlock, flags); >>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/ >>> drivers/hwtracing/coresight/coresight-tmc-etr.c >>> index a48bb85d0e7f..90a4058c9959 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >>> @@ -1254,7 +1254,8 @@ static int tmc_enable_etr_sink_sysfs(struct >>> coresight_device *csdev) >>> struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, >>> enum cs_mode mode, void *data) >>> { >>> - struct perf_output_handle *handle = data; >>> + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; >>> + struct perf_output_handle *handle = sink_data->handle; >>> struct etr_perf_buffer *etr_perf; >>> switch (mode) { >>> @@ -1648,7 +1649,8 @@ static int tmc_enable_etr_sink_perf(struct >>> coresight_device *csdev, void *data) >>> pid_t pid; >>> unsigned long flags; >>> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); >>> - struct perf_output_handle *handle = data; >>> + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; >>> + struct perf_output_handle *handle = sink_data->handle; >>> struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); >>> spin_lock_irqsave(&drvdata->spinlock, flags); >>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/ >>> hwtracing/coresight/coresight-tpda.c >>> index 189a4abc2561..05622333f2d2 100644 >>> --- a/drivers/hwtracing/coresight/coresight-tpda.c >>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c >>> @@ -241,9 +241,29 @@ static void tpda_disable(struct coresight_device >>> *csdev, >>> dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port); >>> } >>> +static int tpda_trace_id(struct coresight_device *csdev) >>> +{ >>> + int trace_id; >>> + struct tpda_drvdata *drvdata; >>> + >>> + if (csdev == NULL) >>> + return -EINVAL; >>> + >>> + drvdata = dev_get_drvdata(csdev->dev.parent); >>> + trace_id = drvdata->atid; >>> + if (!IS_VALID_CS_TRACE_ID(trace_id)) { >> >> Same comment as STM. This is always set so no ned to try to assign >> another one. >> > Will fix. > > [...] > > Thanks, > Jie >

3 months, 3 weeks

Re: [PATCH] coresight: catu: Fix number of pages while using 64k pages

by Suzuki K Poulose

On 10/01/2025 21:32, Ilkka Koskinen wrote: > > Hi Suzuko, > > On Fri, 10 Jan 2025, Suzuki K Poulose wrote: >> On 09/01/2025 21:53, Ilkka Koskinen wrote: >>> Trying to record a trace on kernel with 64k pages resulted in -ENOMEM. >>> This happens due to a bug in calculating the number of table pages, >>> which >>> returns zero. Fix the issue by rounding up. >>> >>> $ perf record --kcore -e cs_etm/@tmc_etr55,cycacc,branch_broadcast/k >>> --per-thread taskset --cpu-list 1 dd if=/dev/zero of=/dev/null >>> failed to mmap with 12 (Cannot allocate memory) >>> >> >> Needs a Fixes tag. >> >> Fixes : 8ed536b1e283 ("coresight: catu: Add support for scatter gather >> tables") > > That's a good point > >> >>> Signed-off-by: Ilkka Koskinen <ilkka(a)os.amperecomputing.com> >>> --- >>> drivers/hwtracing/coresight/coresight-catu.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/ >>> hwtracing/coresight/coresight-catu.c >>> index 275cc0d9f505..3378bb77e6b4 100644 >>> --- a/drivers/hwtracing/coresight/coresight-catu.c >>> +++ b/drivers/hwtracing/coresight/coresight-catu.c >>> @@ -269,7 +269,7 @@ catu_init_sg_table(struct device *catu_dev, int >>> node, >>> * Each table can address upto 1MB and we can have >>> * CATU_PAGES_PER_SYSPAGE tables in a system page. >>> */ >>> - nr_tpages = DIV_ROUND_UP(size, SZ_1M) / CATU_PAGES_PER_SYSPAGE; >>> + nr_tpages = DIV_ROUND_UP(size, CATU_PAGES_PER_SYSPAGE * SZ_1M); >>> catu_table = tmc_alloc_sg_table(catu_dev, node, nr_tpages, >>> size >> PAGE_SHIFT, pages); >>> if (IS_ERR(catu_table)) >> >> Looks good to me, I will queue this later for v6.15. >> >> Suzuki > > Sounds great. Just to confirm, are you ok to add the fixes line or would > you prefer me to submit v2 with it? No, not required. I have picked this up for v6.15, and will push it out once v6.14-rc is out. Suzuki > > Cheers, Ilkka

3 months, 3 weeks

Re: [PATCH v10 06/10] arm64/sysreg/tools: Move TRFCR definitions to sysreg

by James Clark

On 12/01/2025 1:58 pm, Marc Zyngier wrote: > On Tue, 07 Jan 2025 11:32:43 +0000, > James Clark <james.clark(a)linaro.org> wrote: >> >> From: James Clark <james.clark(a)arm.com> >> >> Convert TRFCR to automatic generation. Add separate definitions for ELx >> and EL2 as TRFCR_EL1 doesn't have CX. This also mirrors the previous >> definition so no code change is required. >> >> Also add TRFCR_EL12 which will start to be used in a later commit. >> >> Unfortunately, to avoid breaking the Perf build with duplicate >> definition errors, the tools copy of the sysreg.h header needs to be >> updated at the same time rather than the usual second commit. This is >> because the generated version of sysreg >> (arch/arm64/include/generated/asm/sysreg-defs.h), is currently shared >> and tools/ does not have its own copy. >> >> Reviewed-by: Mark Brown <broonie(a)kernel.org> >> Signed-off-by: James Clark <james.clark(a)arm.com> >> Signed-off-by: James Clark <james.clark(a)linaro.org> > > I've added the following patch to the series, dropping the TCFCR_ELx > construct and fixing the code that makes use of it. > > M. > > From fc77862e06ca1c15e44b0c915da193bd8ed855bc Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz(a)kernel.org> > Date: Sun, 12 Jan 2025 13:08:59 +0000 > Subject: [PATCH] arm64/sysreg: Get rid of TRFCR_ELx SysregFields > > There is no such thing as TRFCR_ELx in the architecture. > What we have is TRFCR_EL1, for which TRFCR_EL12 is an accessor. > > Rename TRFCR_ELx_* to TRFCR_EL1_*, and fix the bit of code using > these names. > > Similarly, TRFCR_EL12 is redefined as a mapping to TRFCR_EL1. > I saw your 'mapping' patch but didn't pull it in for whatever reason. LGTM. Reviewed-by: James Clark <james.clark(a)linaro.org> > Signed-off-by: Marc Zyngier <maz(a)kernel.org> > Cc: James Clark <james.clark(a)linaro.org> > Cc: Suzuki K Poulose <suzuki.poulose(a)arm.com> > Cc: Mark Brown <broonie(a)kernel.org> > Cc: Will Deacon <will(a)kernel.org> > Cc: Catalin Marinas <catalin.marinas(a)arm.com> > --- > arch/arm64/tools/sysreg | 8 ++------ > .../hwtracing/coresight/coresight-etm4x-core.c | 16 ++++++++-------- > .../hwtracing/coresight/coresight-etm4x-sysfs.c | 10 +++++----- > drivers/hwtracing/coresight/coresight-trbe.c | 2 +- > 4 files changed, 16 insertions(+), 20 deletions(-) > > diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg > index 4c2c9c6767c93..ac5202e1df862 100644 > --- a/arch/arm64/tools/sysreg > +++ b/arch/arm64/tools/sysreg > @@ -1999,7 +1999,7 @@ Field 17:16 ZEN > Res0 15:0 > EndSysreg > > -SysregFields TRFCR_ELx > +Sysreg TRFCR_EL1 3 0 1 2 1 > Res0 63:7 > UnsignedEnum 6:5 TS > 0b0001 VIRTUAL > @@ -2011,10 +2011,6 @@ Field 1 ExTRE > Field 0 E0TRE > EndSysregFields > > -Sysreg TRFCR_EL1 3 0 1 2 1 > -Fields TRFCR_ELx > -EndSysreg > - > Sysreg SMPRI_EL1 3 0 1 2 4 > Res0 63:4 > Field 3:0 PRIORITY > @@ -2991,7 +2987,7 @@ Mapping ZCR_EL1 > EndSysreg > > Sysreg TRFCR_EL12 3 5 1 2 1 > -Fields TRFCR_ELx > +Mapping TRFCR_EL1 > EndSysreg > > Sysreg SMCR_EL12 3 5 1 2 6 > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index fbc4aa3785279..2c1a60577728e 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -275,7 +275,7 @@ static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata) > if (!drvdata->trfcr) > return; > > - trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE); > + trfcr = drvdata->trfcr & ~(TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); > > write_trfcr(trfcr); > kvm_tracing_set_el1_configuration(trfcr); > @@ -286,9 +286,9 @@ static u64 etm4x_get_kern_user_filter(struct etmv4_drvdata *drvdata) > u64 trfcr = drvdata->trfcr; > > if (drvdata->config.mode & ETM_MODE_EXCL_KERN) > - trfcr &= ~TRFCR_ELx_ExTRE; > + trfcr &= ~TRFCR_EL1_ExTRE; > if (drvdata->config.mode & ETM_MODE_EXCL_USER) > - trfcr &= ~TRFCR_ELx_E0TRE; > + trfcr &= ~TRFCR_EL1_E0TRE; > > return trfcr; > } > @@ -312,7 +312,7 @@ static void etm4x_allow_trace(struct etmv4_drvdata *drvdata) > return; > > if (drvdata->config.mode & ETM_MODE_EXCL_HOST) > - trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE); > + trfcr = drvdata->trfcr & ~(TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); > else > trfcr = etm4x_get_kern_user_filter(drvdata); > > @@ -320,7 +320,7 @@ static void etm4x_allow_trace(struct etmv4_drvdata *drvdata) > > /* Set filters for guests and pass to KVM */ > if (drvdata->config.mode & ETM_MODE_EXCL_GUEST) > - guest_trfcr = drvdata->trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE); > + guest_trfcr = drvdata->trfcr & ~(TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); > else > guest_trfcr = etm4x_get_kern_user_filter(drvdata); > > @@ -1176,9 +1176,9 @@ static void cpu_detect_trace_filtering(struct etmv4_drvdata *drvdata) > * tracing at the kernel EL and EL0, forcing to use the > * virtual time as the timestamp. > */ > - trfcr = (TRFCR_ELx_TS_VIRTUAL | > - TRFCR_ELx_ExTRE | > - TRFCR_ELx_E0TRE); > + trfcr = (TRFCR_EL1_TS_VIRTUAL | > + TRFCR_EL1_ExTRE | > + TRFCR_EL1_E0TRE); > > /* If we are running at EL2, allow tracing the CONTEXTIDR_EL2. */ > if (is_kernel_in_hyp_mode()) > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > index a9f19629f3f84..c767f8ae4cf1d 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > @@ -2319,11 +2319,11 @@ static ssize_t ts_source_show(struct device *dev, > goto out; > } > > - switch (drvdata->trfcr & TRFCR_ELx_TS_MASK) { > - case TRFCR_ELx_TS_VIRTUAL: > - case TRFCR_ELx_TS_GUEST_PHYSICAL: > - case TRFCR_ELx_TS_PHYSICAL: > - val = FIELD_GET(TRFCR_ELx_TS_MASK, drvdata->trfcr); > + switch (drvdata->trfcr & TRFCR_EL1_TS_MASK) { > + case TRFCR_EL1_TS_VIRTUAL: > + case TRFCR_EL1_TS_GUEST_PHYSICAL: > + case TRFCR_EL1_TS_PHYSICAL: > + val = FIELD_GET(TRFCR_EL1_TS_MASK, drvdata->trfcr); > break; > default: > val = -1; > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c > index d6eb0d525a4d6..fff67aac84181 100644 > --- a/drivers/hwtracing/coresight/coresight-trbe.c > +++ b/drivers/hwtracing/coresight/coresight-trbe.c > @@ -1118,7 +1118,7 @@ static u64 cpu_prohibit_trace(void) > u64 trfcr = read_trfcr(); > > /* Prohibit tracing at EL0 & the kernel EL */ > - write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE)); > + write_trfcr(trfcr & ~(TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE)); > /* Return the original value of the TRFCR */ > return trfcr; > }

3 months, 3 weeks

Re: [PATCH v9 2/7] tools: arm64: Update sysreg.h header files

by James Clark

On 13/01/2025 3:28 pm, Marc Zyngier wrote: > On Mon, 13 Jan 2025 15:07:21 +0000, > Mark Brown <broonie(a)kernel.org> wrote: >> >> [1 <text/plain; us-ascii (quoted-printable)>] >> On Mon, Jan 06, 2025 at 02:24:37PM +0000, James Clark wrote: >>> From: James Clark <james.clark(a)arm.com> >>> >>> Created with the following: >>> >>> cp include/linux/kasan-tags.h tools/include/linux/ >>> cp arch/arm64/include/asm/sysreg.h tools/arch/arm64/include/asm/ >>> >>> Update the tools copy of sysreg.h so that the next commit to add a new >>> register doesn't have unrelated changes in it. Because the new version >>> of sysreg.h includes kasan-tags.h, that file also now needs to be copied >>> into tools. >> >> This breaks the build of the KVM selftests: >> >> aarch64/aarch32_id_regs.c:150:16: error: use of undeclared identifier 'ID_AA64PFR0_EL1_ELx_64BIT_ONLY' >> 150 | return el0 == ID_AA64PFR0_EL1_ELx_64BIT_ONLY; >> | ^ >> 1 warning and 1 error generated. >> >> since it's pulled over the changes in 056600ff7330 ("arm64/cpufeature: >> Replace custom macros with fields from ID_AA64PFR0_EL1") but not updated >> the test to reflect this. > > Thanks for the heads up. I've stashed a fix on top, which hopefully > will be in next tomorrow. > > M. > Oops sorry about that. I only tested the Perf build and hadn't done the self tests for a few versions. Thanks for making the fix.

3 months, 3 weeks

Re: [PATCH v10 04/10] arm64/sysreg: Add a comment that the sysreg file should be sorted

by James Clark

On 12/01/2025 12:49 pm, Marc Zyngier wrote: > On Tue, 07 Jan 2025 11:32:41 +0000, > James Clark <james.clark(a)linaro.org> wrote: >> >> From: James Clark <james.clark(a)arm.com> >> >> There are a few entries particularly at the end of the file that aren't >> in order. To avoid confusion, add a comment that might help new entries >> to be added in the right place. >> >> Reviewed-by: Mark Brown <broonie(a)kernel.org> >> Signed-off-by: James Clark <james.clark(a)arm.com> >> Signed-off-by: James Clark <james.clark(a)linaro.org> >> --- >> arch/arm64/tools/sysreg | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg >> index b081b54d6d22..4ba167089e2a 100644 >> --- a/arch/arm64/tools/sysreg >> +++ b/arch/arm64/tools/sysreg >> @@ -48,6 +48,8 @@ >> # feature that introduces them (eg, FEAT_LS64_ACCDATA introduces enumeration >> # item ACCDATA) though it may be more taseful to do something else. >> >> +# Please try to keep entries in this file sorted by sysreg encoding. >> + >> Sysreg OSDTRRX_EL1 2 0 0 0 2 >> Res0 63:32 >> Field 31:0 DTRRX > > "Do as I say, don't do as I do". > > I don't think this makes any sense if we don't actually sort the file > the first place. > > M. > I think it's ok if it avoids review comments that new entries should be sorted. Or maybe we do the opposite and the comment should say this file is allowed to be unsorted...

3 months, 3 weeks

Re: [PATCH v8 4/5] Coresight: Add Coresight TMC Control Unit driver

by James Clark

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(a)quicinc.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. > + > + 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. > + 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; > +} > + > +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; > + } > + > + return -EINVAL; > +} > + > +/* > + * ctcu_set_etr_traceid: Retrieve the ATID offset and trace ID. > + * > + * Returns 0 indicates success. None-zero result means failure. > + */ > +static int ctcu_set_etr_traceid(struct coresight_device *csdev, > + struct cs_sink_data *sink_data, > + bool enable) > +{ > + int port_num; > + > + if (!IS_VALID_CS_TRACE_ID(sink_data->traceid) || > + (sink_data->sink == NULL)) { > + dev_err(&csdev->dev, "Invalid parameters\n"); > + return -EINVAL; > + } > + > + port_num = ctcu_get_active_port(sink_data->sink, csdev); > + if (port_num < 0) > + return -EINVAL; > + > + dev_dbg(&csdev->dev, "traceid is %d\n", sink_data->traceid); > + > + return __ctcu_set_etr_traceid(csdev, sink_data->traceid, port_num, enable); > +} > + > +static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode, > + void *data) > +{ > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + > + return ctcu_set_etr_traceid(csdev, sink_data, true); > +} > + > +static int ctcu_disable(struct coresight_device *csdev, void *data) > +{ > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + > + return ctcu_set_etr_traceid(csdev, sink_data, false); > +} > + > +static const struct coresight_ops_helper ctcu_helper_ops = { > + .enable = ctcu_enable, > + .disable = ctcu_disable, > +}; > + > +static const struct coresight_ops ctcu_ops = { > + .helper_ops = &ctcu_helper_ops, > +}; > + > +static int ctcu_probe(struct platform_device *pdev) > +{ > + int i; > + void __iomem *base; > + struct device *dev = &pdev->dev; > + struct coresight_platform_data *pdata; > + struct ctcu_drvdata *drvdata; > + struct coresight_desc desc = { 0 }; > + const struct ctcu_config *cfgs; > + const struct ctcu_atid_config *atid_cfg; > + > + desc.name = coresight_alloc_device_name(&ctcu_devs, dev); > + if (!desc.name) > + return -ENOMEM; > + > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + > + pdata = coresight_get_platform_data(dev); > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + dev->platform_data = pdata; > + > + base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL); > + if (!base) > + return -ENOMEM; > + > + drvdata->apb_clk = coresight_get_enable_apb_pclk(dev); > + if (IS_ERR(drvdata->apb_clk)) > + return -ENODEV; > + > + cfgs = of_device_get_match_data(dev); > + if (cfgs) { > + if (cfgs->num_atid_config <= ATID_MAX_NUM) { > + for (i = 0; i < cfgs->num_atid_config; i++) { > + atid_cfg = &cfgs->atid_config[i]; > + drvdata->atid_offset[i] = atid_cfg->atid_offset; > + } > + } > + } > + > + drvdata->base = base; > + drvdata->dev = dev; > + platform_set_drvdata(pdev, drvdata); > + > + desc.type = CORESIGHT_DEV_TYPE_HELPER; > + desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CTCU; > + desc.pdata = pdata; > + desc.dev = dev; > + desc.ops = &ctcu_ops; > + > + drvdata->csdev = coresight_register(&desc); > + if (IS_ERR(drvdata->csdev)) { > + if (!IS_ERR_OR_NULL(drvdata->apb_clk)) > + clk_put(drvdata->apb_clk); > + > + return PTR_ERR(drvdata->csdev); > + } > + > + return 0; > +} > + > +static void ctcu_remove(struct platform_device *pdev) > +{ > + struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev); > + > + coresight_unregister(drvdata->csdev); > + if (!IS_ERR_OR_NULL(drvdata->apb_clk)) > + clk_put(drvdata->apb_clk); > +} > + > +static const struct of_device_id ctcu_match[] = { > + {.compatible = "qcom,sa8775p-ctcu", .data = &sa8775p_cfgs}, > + {} > +}; > + > +static struct platform_driver ctcu_driver = { > + .probe = ctcu_probe, > + .remove = ctcu_remove, > + .driver = { > + .name = "coresight-ctcu", > + .of_match_table = ctcu_match, > + .suppress_bind_attrs = true, > + }, > +}; > +module_platform_driver(ctcu_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("CoreSight TMC Control Unit driver"); > diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/hwtracing/coresight/coresight-ctcu.h > new file mode 100644 > index 000000000000..fc85f3377872 > --- /dev/null > +++ b/drivers/hwtracing/coresight/coresight-ctcu.h > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef _CORESIGHT_CTCU_H > +#define _CORESIGHT_CTCU_H > + > +#define ATID_MAX_NUM 5 > + > +struct ctcu_drvdata { > + void __iomem *base; > + struct clk *apb_clk; > + phys_addr_t pbase; > + struct device *dev; > + struct coresight_device *csdev; > + raw_spinlock_t spin_lock; > + uint32_t atid_offset[ATID_MAX_NUM]; > +}; > + > +#endif > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 1929f0088c0f..18bb4de1d1e4 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -71,7 +71,8 @@ enum coresight_dev_subtype_source { > > enum coresight_dev_subtype_helper { > CORESIGHT_DEV_SUBTYPE_HELPER_CATU, > - CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI > + CORESIGHT_DEV_SUBTYPE_HELPER_ECT_CTI, > + CORESIGHT_DEV_SUBTYPE_HELPER_CTCU, > }; > > /**

3 months, 3 weeks

Re: [PATCH v8 2/5] Coresight: Add trace_id function to retrieving the trace ID

by James Clark

On 26/12/2024 1:10 am, Jie Gan wrote: > Add 'trace_id' function pointer in ops. It's responsible for > retrieving the device's trace ID. > > Add 'struct cs_sink_data' to store the data that is needed by > coresight_enable_path/coresight_disable_path. The structure > will be transmitted to the helper and sink device to enable > related funcationalities. > The new cs_sink_data struct is quite specific to this change. Can we start passing the path around to enable/disable functions, that will allow devices to gather anything they want in the future. Because we already have coresight_get_sink(path), coresight_get_source(path) etc. And see below, but for this case we can also change the path struct to contain the trace ID. Then all the new functions, allocations and searches for the trace ID are unecessary. The CTCU will have access to the path, and by the time its enable function is called the trace ID is already assigned. It's also easier to understand at which point a trace ID is allocated, rather than adding the trace_id() callbacks from everywhere which could potentially either read or allocate. I suppose that's "safer" because maybe it's not allocated, but I can't see what case it would happen in reverse. > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 59 +++++++++++++++---- > drivers/hwtracing/coresight/coresight-etb10.c | 3 +- > .../hwtracing/coresight/coresight-etm-perf.c | 37 ++++++++++-- > .../coresight/coresight-etm3x-core.c | 30 ++++++++++ > .../coresight/coresight-etm4x-core.c | 29 +++++++++ > drivers/hwtracing/coresight/coresight-priv.h | 13 +++- > drivers/hwtracing/coresight/coresight-stm.c | 22 +++++++ > drivers/hwtracing/coresight/coresight-sysfs.c | 24 +++++++- > .../hwtracing/coresight/coresight-tmc-etf.c | 3 +- > .../hwtracing/coresight/coresight-tmc-etr.c | 6 +- > drivers/hwtracing/coresight/coresight-tpda.c | 20 +++++++ > drivers/hwtracing/coresight/coresight-trbe.c | 4 +- > drivers/hwtracing/coresight/ultrasoc-smb.c | 3 +- > include/linux/coresight.h | 6 ++ > 14 files changed, 234 insertions(+), 25 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 0a9380350fb5..2e560b425fd4 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -23,6 +23,7 @@ > #include "coresight-etm-perf.h" > #include "coresight-priv.h" > #include "coresight-syscfg.h" > +#include "coresight-trace-id.h" > > /* > * Mutex used to lock all sysfs enable and disable actions and loading and > @@ -331,12 +332,12 @@ static int coresight_enable_helper(struct coresight_device *csdev, > return helper_ops(csdev)->enable(csdev, mode, data); > } > > -static void coresight_disable_helper(struct coresight_device *csdev) > +static void coresight_disable_helper(struct coresight_device *csdev, void *data) > { > - helper_ops(csdev)->disable(csdev, NULL); > + helper_ops(csdev)->disable(csdev, data); > } > > -static void coresight_disable_helpers(struct coresight_device *csdev) > +static void coresight_disable_helpers(struct coresight_device *csdev, void *data) > { > int i; > struct coresight_device *helper; > @@ -344,7 +345,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev) > for (i = 0; i < csdev->pdata->nr_outconns; ++i) { > helper = csdev->pdata->out_conns[i]->dest_dev; > if (helper && coresight_is_helper(helper)) > - coresight_disable_helper(helper); > + coresight_disable_helper(helper, data); > } > } > > @@ -361,7 +362,7 @@ static void coresight_disable_helpers(struct coresight_device *csdev) > void coresight_disable_source(struct coresight_device *csdev, void *data) > { > source_ops(csdev)->disable(csdev, data); > - coresight_disable_helpers(csdev); > + coresight_disable_helpers(csdev, NULL); > } > EXPORT_SYMBOL_GPL(coresight_disable_source); > > @@ -371,7 +372,8 @@ EXPORT_SYMBOL_GPL(coresight_disable_source); > * disabled. > */ > static void coresight_disable_path_from(struct list_head *path, > - struct coresight_node *nd) > + struct coresight_node *nd, > + void *sink_data) > { > u32 type; > struct coresight_device *csdev, *parent, *child; > @@ -417,13 +419,13 @@ static void coresight_disable_path_from(struct list_head *path, > } > > /* Disable all helpers adjacent along the path last */ > - coresight_disable_helpers(csdev); > + coresight_disable_helpers(csdev, sink_data); > } > } > > -void coresight_disable_path(struct list_head *path) > +void coresight_disable_path(struct list_head *path, void *sink_data) > { > - coresight_disable_path_from(path, NULL); > + coresight_disable_path_from(path, NULL, sink_data); > } > EXPORT_SYMBOL_GPL(coresight_disable_path); > > @@ -505,10 +507,47 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode, > out: > return ret; > err: > - coresight_disable_path_from(path, nd); > + coresight_disable_path_from(path, nd, sink_data); > goto out; > } > > +int coresight_read_traceid(struct list_head *path, enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id, type; > + struct coresight_device *csdev; > + struct coresight_node *nd; > + > + list_for_each_entry(nd, path, link) { What do you think about also changing the path to this: struct coresight_path { struct list_head *path, u8 trace_id }; That would avoid having to traverse the path on every enable and would remove this function. You could also cache the trace ID in the CTCU for a similar benefit, but it wouldn't remove the need to call this at least once. The expensive part should be the create path part, after that enable and disable should be cheap because they happen on schedule for Perf mode. We should be avoiding allocations and searches. > + csdev = nd->csdev; > + type = csdev->type; > + > + switch (type) { > + case CORESIGHT_DEV_TYPE_SOURCE: > + if (source_ops(csdev)->trace_id != NULL) { > + trace_id = source_ops(csdev)->trace_id(csdev, > + mode, > + id_map); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + goto out; > + } > + break; > + case CORESIGHT_DEV_TYPE_LINK: > + if (link_ops(csdev)->trace_id != NULL) { > + trace_id = link_ops(csdev)->trace_id(csdev); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + goto out; > + } > + break; > + default: > + break; > + } > + } > + return -EINVAL; > +out: > + return trace_id; > +} > + > struct coresight_device *coresight_get_sink(struct list_head *path) > { > struct coresight_device *csdev; > diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c > index aea9ac9c4bd0..904b5531c256 100644 > --- a/drivers/hwtracing/coresight/coresight-etb10.c > +++ b/drivers/hwtracing/coresight/coresight-etb10.c > @@ -173,7 +173,8 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data) > pid_t pid; > unsigned long flags; > struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct cs_buffers *buf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index ad6a8f4b70b6..e676edd42ddc 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -459,6 +459,7 @@ static void etm_event_start(struct perf_event *event, int flags) > struct perf_output_handle *handle = &ctxt->handle; > struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); > struct list_head *path; > + struct cs_sink_data *sink_data = NULL; > u64 hw_id; > u8 trace_id; > > @@ -498,9 +499,20 @@ static void etm_event_start(struct perf_event *event, int flags) > if (WARN_ON_ONCE(!sink)) > goto fail_end_stop; > > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); kzalloc can't be called from here. Check dmesg for the warning. That's another reason to do this change on the path. Because the path is allocated on etm_setup_aux() where allocations are allowed. > + if (!sink_data) > + goto fail_end_stop; > + > + sink_data->sink = sink; > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF, > + &sink->perf_sink_id_map); > + sink_data->handle = handle; > + > /* Nothing will happen without a path */ > - if (coresight_enable_path(path, CS_MODE_PERF, handle)) > + if (coresight_enable_path(path, CS_MODE_PERF, sink_data)) { > + kfree(sink_data); > goto fail_end_stop; > + } > > /* Finally enable the tracer */ > if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF, > @@ -526,6 +538,7 @@ static void etm_event_start(struct perf_event *event, int flags) > perf_report_aux_output_id(event, hw_id); > } > > + kfree(sink_data); > out: > /* Tell the perf core the event is alive */ > event->hw.state = 0; > @@ -534,7 +547,8 @@ static void etm_event_start(struct perf_event *event, int flags) > return; > > fail_disable_path: > - coresight_disable_path(path); > + coresight_disable_path(path, sink_data); > + kfree(sink_data); > fail_end_stop: > /* > * Check if the handle is still associated with the event, > @@ -559,6 +573,7 @@ static void etm_event_stop(struct perf_event *event, int mode) > struct perf_output_handle *handle = &ctxt->handle; > struct etm_event_data *event_data; > struct list_head *path; > + struct cs_sink_data *sink_data = NULL; > > /* > * If we still have access to the event_data via handle, > @@ -603,6 +618,10 @@ static void etm_event_stop(struct perf_event *event, int mode) > if (!sink) > return; > > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); Same problem as above. > + if (!sink_data) > + return; > + > /* stop tracer */ > coresight_disable_source(csdev, event); > > @@ -616,12 +635,16 @@ static void etm_event_stop(struct perf_event *event, int mode) > * have to do anything here. > */ > if (handle->event && (mode & PERF_EF_UPDATE)) { > - if (WARN_ON_ONCE(handle->event != event)) > + if (WARN_ON_ONCE(handle->event != event)) { > + kfree(sink_data); > return; > + } > > /* update trace information */ > - if (!sink_ops(sink)->update_buffer) > + if (!sink_ops(sink)->update_buffer) { > + kfree(sink_data); > return; > + } > > size = sink_ops(sink)->update_buffer(sink, handle, > event_data->snk_config); > @@ -642,8 +665,12 @@ static void etm_event_stop(struct perf_event *event, int mode) > WARN_ON(size); > } > > + sink_data->sink = sink; > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_PERF, > + &sink->perf_sink_id_map); > /* Disabling the path make its elements available to other sessions */ > - coresight_disable_path(path); > + coresight_disable_path(path, sink_data); > + kfree(sink_data); > } > > static int etm_event_add(struct perf_event *event, int mode) > diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c > index c103f4c70f5d..0f095fab7e0a 100644 > --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c > @@ -697,10 +697,40 @@ static void etm_disable(struct coresight_device *csdev, > coresight_set_mode(csdev, CS_MODE_DISABLED); > } > > +static int etm_trace_id(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id; > + struct etm_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + switch (mode) { > + case CS_MODE_SYSFS: > + trace_id = etm_read_alloc_trace_id(drvdata); > + break; > + case CS_MODE_PERF: > + trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, id_map); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->traceid = (u8)trace_id; > + break; > + default: > + trace_id = -EINVAL; > + break; > + } > + > + return trace_id; > +} > + > + > static const struct coresight_ops_source etm_source_ops = { > .cpu_id = etm_cpu_id, > .enable = etm_enable, > .disable = etm_disable, > + .trace_id = etm_trace_id, > }; > > static const struct coresight_ops etm_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c > index dd8c74f893db..bac9ea371688 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c > @@ -1025,10 +1025,39 @@ static void etm4_disable(struct coresight_device *csdev, > coresight_set_mode(csdev, CS_MODE_DISABLED); > } > > +static int etm4_trace_id(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id; > + struct etmv4_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + switch (mode) { > + case CS_MODE_SYSFS: > + trace_id = etm4_read_alloc_trace_id(drvdata); > + break; > + case CS_MODE_PERF: > + trace_id = coresight_trace_id_read_cpu_id_map(drvdata->cpu, id_map); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->trcid = (u8)trace_id; > + break; > + default: > + trace_id = -EINVAL; > + break; > + } > + > + return trace_id; > +} > + > static const struct coresight_ops_source etm4_source_ops = { > .cpu_id = etm4_cpu_id, > .enable = etm4_enable, > .disable = etm4_disable, > + .trace_id = etm4_trace_id, > }; > > static const struct coresight_ops etm4_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h > index 05f891ca6b5c..6a95a3ba9339 100644 > --- a/drivers/hwtracing/coresight/coresight-priv.h > +++ b/drivers/hwtracing/coresight/coresight-priv.h > @@ -105,6 +105,15 @@ struct cs_buffers { > void **data_pages; > }; > > +/** > + * struct cs_sink_data - data used by coresight_enable_path/coresight_disable_path > + */ > +struct cs_sink_data { > + struct perf_output_handle *handle; > + struct coresight_device *sink; > + u8 traceid; > +}; > + > static inline void coresight_insert_barrier_packet(void *buf) > { > if (buf) > @@ -129,9 +138,11 @@ static inline void CS_UNLOCK(void __iomem *addr) > } while (0); > } > > -void coresight_disable_path(struct list_head *path); > +void coresight_disable_path(struct list_head *path, void *sink_data); > int coresight_enable_path(struct list_head *path, enum cs_mode mode, > void *sink_data); > +int coresight_read_traceid(struct list_head *path, enum cs_mode mode, > + struct coresight_trace_id_map *id_map); > struct coresight_device *coresight_get_sink(struct list_head *path); > struct coresight_device *coresight_get_sink_by_id(u32 id); > struct coresight_device * > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c > index b581a30a1cd9..5709eda77d9b 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -281,9 +281,31 @@ static void stm_disable(struct coresight_device *csdev, > } > } > > +static int stm_trace_id(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map) > +{ > + int trace_id; > + struct stm_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + trace_id = drvdata->traceid; > + if (!IS_VALID_CS_TRACE_ID(trace_id)) { Trace ID is static and always set for STM so this isn't necessary, just return drvdata->traceid which would already be valid otherwise the device wouldn't be registered. > + trace_id = coresight_trace_id_get_system_id(); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->traceid = (u8)trace_id; > + } > + > + return trace_id; > +} > + > static const struct coresight_ops_source stm_source_ops = { > .enable = stm_enable, > .disable = stm_disable, > + .trace_id = stm_trace_id, > }; > > static const struct coresight_ops stm_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c > index a01c9e54e2ed..8816bccba374 100644 > --- a/drivers/hwtracing/coresight/coresight-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c > @@ -168,6 +168,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > int cpu, ret = 0; > struct coresight_device *sink; > struct list_head *path; > + struct cs_sink_data *sink_data; > enum coresight_dev_subtype_source subtype; > u32 hash; > > @@ -209,7 +210,14 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > goto out; > } > > - ret = coresight_enable_path(path, CS_MODE_SYSFS, NULL); > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); > + if (!sink_data) { > + ret = -ENOMEM; > + goto out; > + } > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_SYSFS, NULL); > + sink_data->sink = sink; > + ret = coresight_enable_path(path, CS_MODE_SYSFS, sink_data); > if (ret) > goto err_path; > > @@ -246,15 +254,17 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > break; > } > > + kfree(sink_data); > out: > mutex_unlock(&coresight_mutex); > return ret; > > err_source: > - coresight_disable_path(path); > + coresight_disable_path(path, sink_data); > > err_path: > coresight_release_path(path); > + kfree(sink_data); > goto out; > } > EXPORT_SYMBOL_GPL(coresight_enable_sysfs); > @@ -263,6 +273,7 @@ void coresight_disable_sysfs(struct coresight_device *csdev) > { > int cpu, ret; > struct list_head *path = NULL; > + struct cs_sink_data *sink_data = NULL; > u32 hash; > > mutex_lock(&coresight_mutex); > @@ -274,6 +285,10 @@ void coresight_disable_sysfs(struct coresight_device *csdev) > if (!coresight_disable_source_sysfs(csdev, NULL)) > goto out; > > + sink_data = kzalloc(sizeof(*sink_data), GFP_KERNEL); > + if (!sink_data) > + goto out; > + > switch (csdev->subtype.source_subtype) { > case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > cpu = source_ops(csdev)->cpu_id(csdev); > @@ -297,8 +312,11 @@ void coresight_disable_sysfs(struct coresight_device *csdev) > break; > } > > - coresight_disable_path(path); > + sink_data->sink = coresight_find_activated_sysfs_sink(csdev); > + sink_data->traceid = coresight_read_traceid(path, CS_MODE_SYSFS, NULL); > + coresight_disable_path(path, sink_data); > coresight_release_path(path); > + kfree(sink_data); > > out: > mutex_unlock(&coresight_mutex); > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c > index d4f641cd9de6..7dc536eba3e2 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c > @@ -250,7 +250,8 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data) > pid_t pid; > unsigned long flags; > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct cs_buffers *buf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index a48bb85d0e7f..90a4058c9959 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1254,7 +1254,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev, > enum cs_mode mode, void *data) > { > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct etr_perf_buffer *etr_perf; > > switch (mode) { > @@ -1648,7 +1649,8 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) > pid_t pid; > unsigned long flags; > struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct etr_perf_buffer *etr_perf = etm_perf_sink_config(handle); > > spin_lock_irqsave(&drvdata->spinlock, flags); > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c > index 189a4abc2561..05622333f2d2 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -241,9 +241,29 @@ static void tpda_disable(struct coresight_device *csdev, > dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port); > } > > +static int tpda_trace_id(struct coresight_device *csdev) > +{ > + int trace_id; > + struct tpda_drvdata *drvdata; > + > + if (csdev == NULL) > + return -EINVAL; > + > + drvdata = dev_get_drvdata(csdev->dev.parent); > + trace_id = drvdata->atid; > + if (!IS_VALID_CS_TRACE_ID(trace_id)) { Same comment as STM. This is always set so no ned to try to assign another one. > + trace_id = coresight_trace_id_get_system_id(); > + if (IS_VALID_CS_TRACE_ID(trace_id)) > + drvdata->atid = (u8)trace_id; > + } > + > + return trace_id; > +} > + > static const struct coresight_ops_link tpda_link_ops = { > .enable = tpda_enable, > .disable = tpda_disable, > + .trace_id = tpda_trace_id, > }; > > static const struct coresight_ops tpda_cs_ops = { > diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c > index 919804b12a67..6680d3c69906 100644 > --- a/drivers/hwtracing/coresight/coresight-trbe.c > +++ b/drivers/hwtracing/coresight/coresight-trbe.c > @@ -21,6 +21,7 @@ > > #include "coresight-self-hosted-trace.h" > #include "coresight-trbe.h" > +#include "coresight-priv.h" > > #define PERF_IDX2OFF(idx, buf) ((idx) % ((buf)->nr_pages << PAGE_SHIFT)) > > @@ -1012,7 +1013,8 @@ static int arm_trbe_enable(struct coresight_device *csdev, enum cs_mode mode, > { > struct trbe_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); > struct trbe_cpudata *cpudata = dev_get_drvdata(&csdev->dev); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct trbe_buf *buf = etm_perf_sink_config(handle); > > WARN_ON(cpudata->cpu != smp_processor_id()); > diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c > index dc3c9504dd7c..99e0eb33c01f 100644 > --- a/drivers/hwtracing/coresight/ultrasoc-smb.c > +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c > @@ -216,7 +216,8 @@ static void smb_enable_sysfs(struct coresight_device *csdev) > static int smb_enable_perf(struct coresight_device *csdev, void *data) > { > struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent); > - struct perf_output_handle *handle = data; > + struct cs_sink_data *sink_data = (struct cs_sink_data *)data; > + struct perf_output_handle *handle = sink_data->handle; > struct cs_buffers *buf = etm_perf_sink_config(handle); > pid_t pid; > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 157c4bd009a1..1929f0088c0f 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -368,6 +368,7 @@ struct coresight_ops_sink { > * Operations available for links. > * @enable: enables flow between iport and oport. > * @disable: disables flow between iport and oport. > + * @trace_id: Collect the traceid. > */ > struct coresight_ops_link { > int (*enable)(struct coresight_device *csdev, > @@ -376,6 +377,7 @@ struct coresight_ops_link { > void (*disable)(struct coresight_device *csdev, > struct coresight_connection *in, > struct coresight_connection *out); > + int (*trace_id)(struct coresight_device *csdev); > }; > > /** > @@ -385,6 +387,7 @@ struct coresight_ops_link { > * is associated to. > * @enable: enables tracing for a source. > * @disable: disables tracing for a source. > + * @trace_id: collect the traceid. > */ > struct coresight_ops_source { > int (*cpu_id)(struct coresight_device *csdev); > @@ -392,6 +395,9 @@ struct coresight_ops_source { > enum cs_mode mode, struct coresight_trace_id_map *id_map); > void (*disable)(struct coresight_device *csdev, > struct perf_event *event); > + int (*trace_id)(struct coresight_device *csdev, > + enum cs_mode mode, > + struct coresight_trace_id_map *id_map); > }; > > /**

3 months, 3 weeks

Re: [PATCH v8 1/5] Coresight: Add support for new APB clock name

by James Clark

On 26/12/2024 1:10 am, Jie Gan wrote: > Add support for new APB clock-name. If the function fails > to obtain the clock with the name "apb_pclk", it will > attempt to acquire the clock with the name "apb". > > Signed-off-by: Jie Gan <quic_jiegan(a)quicinc.com> > --- > include/linux/coresight.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/linux/coresight.h b/include/linux/coresight.h > index 17276965ff1d..157c4bd009a1 100644 > --- a/include/linux/coresight.h > +++ b/include/linux/coresight.h > @@ -459,8 +459,11 @@ static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev) > int ret; > > pclk = clk_get(dev, "apb_pclk"); > - if (IS_ERR(pclk)) > - return NULL; > + if (IS_ERR(pclk)) { > + pclk = clk_get(dev, "apb"); > + if (IS_ERR(pclk)) > + return NULL; > + } > > ret = clk_prepare_enable(pclk); > if (ret) { Reviewed-by: James Clark <james.clark(a)linaro.org>

3 months, 3 weeks

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight January 2025