CoreSight December 2025

coresight@lists.linaro.org

6 participants
41 discussions

Re: [PATCH v2 01/12] perf parse-events: Refactor get_config_terms() to remove macros

by James Clark

On 09/12/2025 20:09, Ian Rogers wrote: > On Tue, Dec 9, 2025 at 9:55 AM James Clark <james.clark(a)linaro.org> wrote: >> >> On 09/12/2025 15:58, Ian Rogers wrote: >>> On Tue, Dec 9, 2025 at 4:48 AM James Clark <james.clark(a)linaro.org> wrote: >>>> >>>> On 08/12/2025 2:22 pm, James Clark wrote: >>>>> The ADD_CONFIG_TERM() macros build the __type argument out of a partial >>>>> EVSEL__CONFIG_TERM_x enum name. This means that they can't be called >>>>> from a function where __type is a variable and it's also impossible to >>>>> grep the codebase to find usages of these enums as they're never typed >>>>> in full. >>>>> >>>>> Fix this by removing the macros and replacing them with an >>>>> add_config_term() function. It seems the main reason these existed in >>>>> the first place was to avoid type punning and to write to a specific >>>>> field in the union, but the same thing can be achieved with a single >>>>> write to a u64 'val' field. >>>>> >>>>> Signed-off-by: James Clark <james.clark(a)linaro.org> >>>>> --- >>>>> tools/perf/util/evsel_config.h | 1 + >>>>> tools/perf/util/parse-events.c | 146 ++++++++++++++++++++++++----------------- >>>>> 2 files changed, 86 insertions(+), 61 deletions(-) >>>>> >>>>> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h >>>>> index bcd3a978f0c4..685fd8d5c4a8 100644 >>>>> --- a/tools/perf/util/evsel_config.h >>>>> +++ b/tools/perf/util/evsel_config.h >>>>> @@ -50,6 +50,7 @@ struct evsel_config_term { >>>>> u64 cfg_chg; >>>>> char *str; >>>>> int cpu; >>>>> + u64 val; >>>>> } val; >>>>> bool weak; >>>>> }; >>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >>>>> index 17c1c36a7bf9..d5b009b4ebab 100644 >>>>> --- a/tools/perf/util/parse-events.c >>>>> +++ b/tools/perf/util/parse-events.c >>>>> @@ -1116,105 +1116,107 @@ static int config_attr(struct perf_event_attr *attr, >>>>> return 0; >>>>> } >>>>> >>>>> -static int get_config_terms(const struct parse_events_terms *head_config, >>>>> - struct list_head *head_terms) >>>>> +static struct evsel_config_term *add_config_term(enum evsel_term_type type, >>>>> + struct list_head *head_terms, >>>>> + bool weak) >>>>> { >>>>> -#define ADD_CONFIG_TERM(__type, __weak) \ >>>>> - struct evsel_config_term *__t; \ >>>>> - \ >>>>> - __t = zalloc(sizeof(*__t)); \ >>>>> - if (!__t) \ >>>>> - return -ENOMEM; \ >>>>> - \ >>>>> - INIT_LIST_HEAD(&__t->list); \ >>>>> - __t->type = EVSEL__CONFIG_TERM_ ## __type; \ >>>>> - __t->weak = __weak; \ >>>>> - list_add_tail(&__t->list, head_terms) >>>>> - >>>>> -#define ADD_CONFIG_TERM_VAL(__type, __name, __val, __weak) \ >>>>> -do { \ >>>>> - ADD_CONFIG_TERM(__type, __weak); \ >>>>> - __t->val.__name = __val; \ >>>>> -} while (0) >>>>> + struct evsel_config_term *t; >>>>> >>>>> -#define ADD_CONFIG_TERM_STR(__type, __val, __weak) \ >>>>> -do { \ >>>>> - ADD_CONFIG_TERM(__type, __weak); \ >>>>> - __t->val.str = strdup(__val); \ >>>>> - if (!__t->val.str) { \ >>>>> - zfree(&__t); \ >>>>> - return -ENOMEM; \ >>>>> - } \ >>>>> - __t->free_str = true; \ >>>>> -} while (0) >>>>> + t = zalloc(sizeof(*t)); >>>>> + if (!t) >>>>> + return NULL; >>>>> + >>>>> + INIT_LIST_HEAD(&t->list); >>>>> + t->type = type; >>>>> + t->weak = weak; >>>>> + list_add_tail(&t->list, head_terms); >>>>> >>>>> + return t; >>>>> +} >>>>> + >>>>> +static int get_config_terms(const struct parse_events_terms *head_config, >>>>> + struct list_head *head_terms) >>>>> +{ >>>>> struct parse_events_term *term; >>>>> >>>>> list_for_each_entry(term, &head_config->terms, list) { >>>>> + struct evsel_config_term *new_term; >>>>> + enum evsel_term_type new_type; >>>>> + char *str = NULL; >>>>> + u64 val; >>>>> + >>>>> switch (term->type_term) { >>>>> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD: >>>>> - ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_PERIOD; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ: >>>>> - ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_FREQ; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_TIME: >>>>> - ADD_CONFIG_TERM_VAL(TIME, time, term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_TIME; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH: >>>>> - ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_CALLGRAPH; >>>>> + str = term->val.str; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: >>>>> - ADD_CONFIG_TERM_STR(BRANCH, term->val.str, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_BRANCH; >>>>> + str = term->val.str; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_STACKSIZE: >>>>> - ADD_CONFIG_TERM_VAL(STACK_USER, stack_user, >>>>> - term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_STACK_USER; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_INHERIT: >>>>> - ADD_CONFIG_TERM_VAL(INHERIT, inherit, >>>>> - term->val.num ? 1 : 0, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_INHERIT; >>>>> + val = term->val.num ? 1 : 0; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_NOINHERIT: >>>>> - ADD_CONFIG_TERM_VAL(INHERIT, inherit, >>>>> - term->val.num ? 0 : 1, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_INHERIT; >>>>> + val = term->val.num ? 0 : 1; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_MAX_STACK: >>>>> - ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack, >>>>> - term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_MAX_STACK; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS: >>>>> - ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events, >>>>> - term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_MAX_EVENTS; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_OVERWRITE: >>>>> - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, >>>>> - term->val.num ? 1 : 0, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_OVERWRITE; >>>>> + val = term->val.num ? 1 : 0; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE: >>>>> - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, >>>>> - term->val.num ? 0 : 1, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_OVERWRITE; >>>>> + val = term->val.num ? 0 : 1; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_DRV_CFG: >>>>> - ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_DRV_CFG; >>>>> + str = term->val.str; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_PERCORE: >>>>> - ADD_CONFIG_TERM_VAL(PERCORE, percore, >>>>> - term->val.num ? true : false, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_PERCORE; >>>>> + val = term->val.num ? true : false; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT: >>>>> - ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output, >>>>> - term->val.num ? 1 : 0, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_AUX_OUTPUT; >>>>> + val = term->val.num ? 1 : 0; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_AUX_ACTION: >>>>> - ADD_CONFIG_TERM_STR(AUX_ACTION, term->val.str, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_AUX_ACTION; >>>>> + str = term->val.str; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE: >>>>> - ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, >>>>> - term->val.num, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE; >>>>> + val = term->val.num; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_RATIO_TO_PREV: >>>>> - ADD_CONFIG_TERM_STR(RATIO_TO_PREV, term->val.str, term->weak); >>>>> + new_type = EVSEL__CONFIG_TERM_RATIO_TO_PREV; >>>>> + str = term->val.str; >>>>> break; >>>>> case PARSE_EVENTS__TERM_TYPE_USER: >>>>> case PARSE_EVENTS__TERM_TYPE_CONFIG: >>>>> @@ -1229,7 +1231,23 @@ do { \ >>>>> case PARSE_EVENTS__TERM_TYPE_RAW: >>>>> case PARSE_EVENTS__TERM_TYPE_CPU: >>>>> default: >>>>> - break; >>>>> + /* Don't add a new term for these ones */ >>>>> + continue; >>>>> + } >>>>> + >>>>> + new_term = add_config_term(new_type, head_terms, term->weak); >>>>> + if (!new_term) >>>>> + return -ENOMEM; >>>>> + >>>>> + if (str) { >>>>> + new_term->val.str = strdup(str); >>>>> + if (!new_term->val.str) { >>>>> + zfree(&new_term); >>>>> + return -ENOMEM; >>>>> + } >>>>> + new_term->free_str = true; >>>>> + } else { >>>> >>>> This will incorrectly hit the else if term->val.str is NULL. Not sure if >>>> that can happen but will fix anyway. >>>> >>>>> + new_term->val.val = val; >>>> >>>> There's an uninitialized variable warning for val here on release >>>> builds. Will fix too >>> >>> I'm not sure how I feel about the change, but did you check that it is >>> ubsan clean? I worry it is introducing writes to "val" and then reads >>> from say "period" in the evsel_config_term val union. I believe ubsan >>> will flag this as: >>> https://en.cppreference.com/w/cpp/language/union.html >>> "It is undefined behavior to read from the member of the union that >>> wasn't most recently written." >>> >>> Thanks, >>> Ian >>> >> >> Maybe for C++, but not for C [1]: >> >> Accessing my_union.i after most recently writing to the other member, >> my_union.d, is an allowed form of type-punning in C, provided that the >> member read is not larger than the one whose value was set >> >> Type punning with unions is widely used in the kernel and there are a >> couple of instances in Perf. There are tons of discussions about it on >> the mailing lists too. I can check it with ubsan though. > > Thanks. I'm mostly worried about ubsan being clean. One of those > mailing list discussions, or sadly outside the usual trolls lack of > discussion, is my trying to remove a prevalent source of type punning: > https://lore.kernel.org/lkml/20251016205126.2882625-1-irogers@google.com/ > > Ian > No ubsan issues on any Perf tests from this, but I did forget a free in the new test that I saw with asan so I'll send a new version. >> [1]: https://en.wikipedia.org/wiki/Type_punning#C_and_C++ >> >>>>> } >>>>> } >>>>> return 0; >>>>> @@ -1290,10 +1308,16 @@ static int get_config_chgs(struct perf_pmu *pmu, struct parse_events_terms *head >>>>> } >>>>> } >>>>> >>>>> - if (bits) >>>>> - ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits, false); >>>>> + if (bits) { >>>>> + struct evsel_config_term *new_term; >>>>> + >>>>> + new_term = add_config_term(EVSEL__CONFIG_TERM_CFG_CHG, >>>>> + head_terms, false); >>>>> + if (!new_term) >>>>> + return -ENOMEM; >>>>> + new_term->val.cfg_chg = bits; >>>>> + } >>>>> >>>>> -#undef ADD_CONFIG_TERM >>>>> return 0; >>>>> } >>>>> >>>>> >>>> >>

1 week, 2 days

Re: [PATCH v2 01/12] perf parse-events: Refactor get_config_terms() to remove macros

by James Clark

On 09/12/2025 15:58, Ian Rogers wrote: > On Tue, Dec 9, 2025 at 4:48 AM James Clark <james.clark(a)linaro.org> wrote: >> >> On 08/12/2025 2:22 pm, James Clark wrote: >>> The ADD_CONFIG_TERM() macros build the __type argument out of a partial >>> EVSEL__CONFIG_TERM_x enum name. This means that they can't be called >>> from a function where __type is a variable and it's also impossible to >>> grep the codebase to find usages of these enums as they're never typed >>> in full. >>> >>> Fix this by removing the macros and replacing them with an >>> add_config_term() function. It seems the main reason these existed in >>> the first place was to avoid type punning and to write to a specific >>> field in the union, but the same thing can be achieved with a single >>> write to a u64 'val' field. >>> >>> Signed-off-by: James Clark <james.clark(a)linaro.org> >>> --- >>> tools/perf/util/evsel_config.h | 1 + >>> tools/perf/util/parse-events.c | 146 ++++++++++++++++++++++++----------------- >>> 2 files changed, 86 insertions(+), 61 deletions(-) >>> >>> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h >>> index bcd3a978f0c4..685fd8d5c4a8 100644 >>> --- a/tools/perf/util/evsel_config.h >>> +++ b/tools/perf/util/evsel_config.h >>> @@ -50,6 +50,7 @@ struct evsel_config_term { >>> u64 cfg_chg; >>> char *str; >>> int cpu; >>> + u64 val; >>> } val; >>> bool weak; >>> }; >>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >>> index 17c1c36a7bf9..d5b009b4ebab 100644 >>> --- a/tools/perf/util/parse-events.c >>> +++ b/tools/perf/util/parse-events.c >>> @@ -1116,105 +1116,107 @@ static int config_attr(struct perf_event_attr *attr, >>> return 0; >>> } >>> >>> -static int get_config_terms(const struct parse_events_terms *head_config, >>> - struct list_head *head_terms) >>> +static struct evsel_config_term *add_config_term(enum evsel_term_type type, >>> + struct list_head *head_terms, >>> + bool weak) >>> { >>> -#define ADD_CONFIG_TERM(__type, __weak) \ >>> - struct evsel_config_term *__t; \ >>> - \ >>> - __t = zalloc(sizeof(*__t)); \ >>> - if (!__t) \ >>> - return -ENOMEM; \ >>> - \ >>> - INIT_LIST_HEAD(&__t->list); \ >>> - __t->type = EVSEL__CONFIG_TERM_ ## __type; \ >>> - __t->weak = __weak; \ >>> - list_add_tail(&__t->list, head_terms) >>> - >>> -#define ADD_CONFIG_TERM_VAL(__type, __name, __val, __weak) \ >>> -do { \ >>> - ADD_CONFIG_TERM(__type, __weak); \ >>> - __t->val.__name = __val; \ >>> -} while (0) >>> + struct evsel_config_term *t; >>> >>> -#define ADD_CONFIG_TERM_STR(__type, __val, __weak) \ >>> -do { \ >>> - ADD_CONFIG_TERM(__type, __weak); \ >>> - __t->val.str = strdup(__val); \ >>> - if (!__t->val.str) { \ >>> - zfree(&__t); \ >>> - return -ENOMEM; \ >>> - } \ >>> - __t->free_str = true; \ >>> -} while (0) >>> + t = zalloc(sizeof(*t)); >>> + if (!t) >>> + return NULL; >>> + >>> + INIT_LIST_HEAD(&t->list); >>> + t->type = type; >>> + t->weak = weak; >>> + list_add_tail(&t->list, head_terms); >>> >>> + return t; >>> +} >>> + >>> +static int get_config_terms(const struct parse_events_terms *head_config, >>> + struct list_head *head_terms) >>> +{ >>> struct parse_events_term *term; >>> >>> list_for_each_entry(term, &head_config->terms, list) { >>> + struct evsel_config_term *new_term; >>> + enum evsel_term_type new_type; >>> + char *str = NULL; >>> + u64 val; >>> + >>> switch (term->type_term) { >>> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD: >>> - ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_PERIOD; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ: >>> - ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_FREQ; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_TIME: >>> - ADD_CONFIG_TERM_VAL(TIME, time, term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_TIME; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH: >>> - ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_CALLGRAPH; >>> + str = term->val.str; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: >>> - ADD_CONFIG_TERM_STR(BRANCH, term->val.str, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_BRANCH; >>> + str = term->val.str; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_STACKSIZE: >>> - ADD_CONFIG_TERM_VAL(STACK_USER, stack_user, >>> - term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_STACK_USER; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_INHERIT: >>> - ADD_CONFIG_TERM_VAL(INHERIT, inherit, >>> - term->val.num ? 1 : 0, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_INHERIT; >>> + val = term->val.num ? 1 : 0; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_NOINHERIT: >>> - ADD_CONFIG_TERM_VAL(INHERIT, inherit, >>> - term->val.num ? 0 : 1, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_INHERIT; >>> + val = term->val.num ? 0 : 1; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_MAX_STACK: >>> - ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack, >>> - term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_MAX_STACK; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS: >>> - ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events, >>> - term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_MAX_EVENTS; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_OVERWRITE: >>> - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, >>> - term->val.num ? 1 : 0, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_OVERWRITE; >>> + val = term->val.num ? 1 : 0; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE: >>> - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite, >>> - term->val.num ? 0 : 1, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_OVERWRITE; >>> + val = term->val.num ? 0 : 1; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_DRV_CFG: >>> - ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_DRV_CFG; >>> + str = term->val.str; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_PERCORE: >>> - ADD_CONFIG_TERM_VAL(PERCORE, percore, >>> - term->val.num ? true : false, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_PERCORE; >>> + val = term->val.num ? true : false; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT: >>> - ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output, >>> - term->val.num ? 1 : 0, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_AUX_OUTPUT; >>> + val = term->val.num ? 1 : 0; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_AUX_ACTION: >>> - ADD_CONFIG_TERM_STR(AUX_ACTION, term->val.str, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_AUX_ACTION; >>> + str = term->val.str; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE: >>> - ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, >>> - term->val.num, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE; >>> + val = term->val.num; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_RATIO_TO_PREV: >>> - ADD_CONFIG_TERM_STR(RATIO_TO_PREV, term->val.str, term->weak); >>> + new_type = EVSEL__CONFIG_TERM_RATIO_TO_PREV; >>> + str = term->val.str; >>> break; >>> case PARSE_EVENTS__TERM_TYPE_USER: >>> case PARSE_EVENTS__TERM_TYPE_CONFIG: >>> @@ -1229,7 +1231,23 @@ do { \ >>> case PARSE_EVENTS__TERM_TYPE_RAW: >>> case PARSE_EVENTS__TERM_TYPE_CPU: >>> default: >>> - break; >>> + /* Don't add a new term for these ones */ >>> + continue; >>> + } >>> + >>> + new_term = add_config_term(new_type, head_terms, term->weak); >>> + if (!new_term) >>> + return -ENOMEM; >>> + >>> + if (str) { >>> + new_term->val.str = strdup(str); >>> + if (!new_term->val.str) { >>> + zfree(&new_term); >>> + return -ENOMEM; >>> + } >>> + new_term->free_str = true; >>> + } else { >> >> This will incorrectly hit the else if term->val.str is NULL. Not sure if >> that can happen but will fix anyway. >> >>> + new_term->val.val = val; >> >> There's an uninitialized variable warning for val here on release >> builds. Will fix too > > I'm not sure how I feel about the change, but did you check that it is > ubsan clean? I worry it is introducing writes to "val" and then reads > from say "period" in the evsel_config_term val union. I believe ubsan > will flag this as: > https://en.cppreference.com/w/cpp/language/union.html > "It is undefined behavior to read from the member of the union that > wasn't most recently written." > > Thanks, > Ian > Maybe for C++, but not for C [1]: Accessing my_union.i after most recently writing to the other member, my_union.d, is an allowed form of type-punning in C, provided that the member read is not larger than the one whose value was set Type punning with unions is widely used in the kernel and there are a couple of instances in Perf. There are tons of discussions about it on the mailing lists too. I can check it with ubsan though. [1]: https://en.wikipedia.org/wiki/Type_punning#C_and_C++ >>> } >>> } >>> return 0; >>> @@ -1290,10 +1308,16 @@ static int get_config_chgs(struct perf_pmu *pmu, struct parse_events_terms *head >>> } >>> } >>> >>> - if (bits) >>> - ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits, false); >>> + if (bits) { >>> + struct evsel_config_term *new_term; >>> + >>> + new_term = add_config_term(EVSEL__CONFIG_TERM_CFG_CHG, >>> + head_terms, false); >>> + if (!new_term) >>> + return -ENOMEM; >>> + new_term->val.cfg_chg = bits; >>> + } >>> >>> -#undef ADD_CONFIG_TERM >>> return 0; >>> } >>> >>> >>

1 week, 4 days

Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

by Leo Yan

On Tue, Dec 09, 2025 at 08:51:38PM +0800, Yingchao Deng (Consultant) wrote: [...] > > void cti_write_single_reg(struct cti_drvdata *drvdata, > > int offset, u32 value) > > { > > CS_UNLOCK(drvdata->base); > > writel_relaxed(value, cti_reg_addr(drvdata, offset)); > > CS_LOCK(drvdata->base); > > } > > However, since we also need to handle cti_reg_addr_with_nr, it will be > necessary to add an additional parameter "nr" to cti_write_single_reg? I expect the argument "offset" has already containted the nr in bits[31..28], so don't need to pass "nr" parameter to cti_write_single_reg(). You will change inen_store() / outen_store(), e.g.,: cti_write_single_reg(drvdata, CTI_REG_SET_NR(CTIINEN, index), value); Just remind, this might be a separate refactor for common code and you need to write a patch for this, then is followed by QCOM CTI support patch. Thanks, Leo

1 week, 5 days

Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

by Leo Yan

On Tue, Dec 02, 2025 at 02:42:21PM +0800, Yingchao Deng wrote: > The QCOM extended CTI is a heavily parameterized version of ARM’s CSCTI. > It allows a debugger to send to trigger events to a processor or to send > a trigger event to one or more processors when a trigger event occurs > on another processor on the same SoC, or even between SoCs. Qualcomm CTI > implementation differs from the standard CTI in the following aspects: > > 1. The number of supported triggers is extended to 128. > 2. Several register offsets differ from the CoreSight specification. I apologize for my late review of this series. For easier maintenance later, I have several comments for register access. [...] > +static const u32 cti_normal_offset[] = { > + [INDEX_CTIINTACK] = CTIINTACK, > + [INDEX_CTIAPPSET] = CTIAPPSET, > + [INDEX_CTIAPPCLEAR] = CTIAPPCLEAR, > + [INDEX_CTIAPPPULSE] = CTIAPPPULSE, > + [INDEX_CTIINEN] = CTIINEN(0), > + [INDEX_CTIOUTEN] = CTIOUTEN(0), I prefer to update the these two macros to CTIINENn and CTIOUTENn, as later we will not use CTIINEN(n) and CTIOUTEN(n) anymore. > + [INDEX_CTITRIGINSTATUS] = CTITRIGINSTATUS, > + [INDEX_CTITRIGOUTSTATUS] = CTITRIGOUTSTATUS, > + [INDEX_CTICHINSTATUS] = CTICHINSTATUS, > + [INDEX_CTICHOUTSTATUS] = CTICHOUTSTATUS, > + [INDEX_CTIGATE] = CTIGATE, > + [INDEX_ASICCTL] = ASICCTL, > + [INDEX_ITCHINACK] = ITCHINACK, > + [INDEX_ITTRIGINACK] = ITTRIGINACK, > + [INDEX_ITCHOUT] = ITCHOUT, > + [INDEX_ITTRIGOUT] = ITTRIGOUT, > + [INDEX_ITCHOUTACK] = ITCHOUTACK, > + [INDEX_ITTRIGOUTACK] = ITTRIGOUTACK, > + [INDEX_ITCHIN] = ITCHIN, > + [INDEX_ITTRIGIN] = ITTRIGIN, > + [INDEX_ITCTRL] = CORESIGHT_ITCTRL, > +}; > + > +static const u32 cti_extended_offset[] = { > + [INDEX_CTIINTACK] = QCOM_CTIINTACK, > + [INDEX_CTIAPPSET] = QCOM_CTIAPPSET, > + [INDEX_CTIAPPCLEAR] = QCOM_CTIAPPCLEAR, > + [INDEX_CTIAPPPULSE] = QCOM_CTIAPPPULSE, > + [INDEX_CTIINEN] = QCOM_CTIINEN, > + [INDEX_CTIOUTEN] = QCOM_CTIOUTEN, > + [INDEX_CTITRIGINSTATUS] = QCOM_CTITRIGINSTATUS, > + [INDEX_CTITRIGOUTSTATUS] = QCOM_CTITRIGOUTSTATUS, > + [INDEX_CTICHINSTATUS] = QCOM_CTICHINSTATUS, > + [INDEX_CTICHOUTSTATUS] = QCOM_CTICHOUTSTATUS, > + [INDEX_CTIGATE] = QCOM_CTIGATE, > + [INDEX_ASICCTL] = QCOM_ASICCTL, > + [INDEX_ITCHINACK] = QCOM_ITCHINACK, > + [INDEX_ITTRIGINACK] = QCOM_ITTRIGINACK, > + [INDEX_ITCHOUT] = QCOM_ITCHOUT, > + [INDEX_ITTRIGOUT] = QCOM_ITTRIGOUT, > + [INDEX_ITCHOUTACK] = QCOM_ITCHOUTACK, > + [INDEX_ITTRIGOUTACK] = QCOM_ITTRIGOUTACK, > + [INDEX_ITCHIN] = QCOM_ITCHIN, > + [INDEX_ITTRIGIN] = QCOM_ITTRIGIN, > + [INDEX_ITCTRL] = CORESIGHT_ITCTRL, > +}; I saw CTI registers are within 4KiB (0x1000), we can don't convert standard regiserts and only convert to QCOM register based on the standard ones. So you can drop the cti_normal_offset strucuture and only have a cti_reg_qcom_offset[] struct: static const u32 cti_extended_offset[] = { [CTIINTACK] = QCOM_CTIINTACK, [CTIAPPSET] = QCOM_CTIAPPSET, [CTIAPPCLEAR] = QCOM_CTIAPPCLEAR, [CTIAPPPULSE] = QCOM_CTIAPPPULSE, [CTIINEN] = QCOM_CTIINEN, ... }; Then you could create two helpers for register address: static void __iomem *cti_reg_addr_with_nr(struct cti_drvdata *drvdata, u32 reg, u32 nr) { /* convert to qcom specific offset */ if (unlikely(drvdata->is_qcom_cti)) reg = cti_extended_offset[reg]; return drvdata->base + reg + sizeof(u32) * nr; } static void __iomem *cti_reg_addr(struct cti_drvdata *drvdata, u32 reg) { return cti_reg_addr_with_nr(drvdata, reg, 0); } > /* > * CTI devices can be associated with a PE, or be connected to CoreSight > @@ -70,15 +119,16 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata) > > /* write the CTI trigger registers */ > for (i = 0; i < config->nr_trig_max; i++) { > - writel_relaxed(config->ctiinen[i], drvdata->base + CTIINEN(i)); > + writel_relaxed(config->ctiinen[i], > + drvdata->base + cti_offset(drvdata, INDEX_CTIINEN, i)); writel_relaxed(config->ctiinen[i], cti_reg_addr_with_nr(drvdata, CTIINENn, i)); And apply for the same cases below. > /* other regs */ > - writel_relaxed(config->ctigate, drvdata->base + CTIGATE); > - writel_relaxed(config->asicctl, drvdata->base + ASICCTL); > - writel_relaxed(config->ctiappset, drvdata->base + CTIAPPSET); > + writel_relaxed(config->ctigate, drvdata->base + cti_offset(drvdata, INDEX_CTIGATE, 0)); writel_relaxed(config->ctigate, cti_reg_addr(drvdata, CTIGATE)); And apply for the same cases below. [...] > @@ -394,8 +447,8 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, > > /* update the local register values */ > chan_bitmask = BIT(channel_idx); > - reg_offset = (direction == CTI_TRIG_IN ? CTIINEN(trigger_idx) : > - CTIOUTEN(trigger_idx)); > + reg_offset = (direction == CTI_TRIG_IN ? cti_offset(drvdata, INDEX_CTIINEN, trigger_idx) : > + cti_offset(drvdata, INDEX_CTIOUTEN, trigger_idx)); For readable, we can improve a bit with code alignment: reg_offset = (direction == CTI_TRIG_IN) ? cti_reg_addr_with_nr(drvdata, CTIINENn, trigger_idx) : cti_reg_addr_with_nr(drvdata, CTIOUTENn, trigger_idx); [...] > @@ -981,9 +1035,28 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) > drvdata->csdev_release = drvdata->csdev->dev.release; > drvdata->csdev->dev.release = cti_device_release; > > + /* check architect value*/ > + devarch = readl_relaxed(drvdata->base + CORESIGHT_DEVARCH); > + if (CTI_DEVARCH_ARCHITECT(devarch) == ARCHITECT_QCOM) { > + drvdata->subtype = QCOM_CTI; > + drvdata->offsets = cti_extended_offset; As a result, we can only set the is_qcom_cti flag: drvdata->is_qcom_cti = true; > + /* > + * QCOM CTI does not implement Claimtag functionality as > + * per CoreSight specification, but its CLAIMSET register > + * is incorrectly initialized to 0xF. This can mislead > + * tools or drivers into thinking the component is claimed. > + * > + * Reset CLAIMSET to 0 to reflect that no claims are active. > + */ > + writel_relaxed(0, drvdata->base + CORESIGHT_CLAIMSET); I am confused for this. If QCOM CTI does not implement claim tag, then what is the designed register at the offset CORESIGHT_CLAIMSET? Should you bypass all claim tag related operations for QCOM CTI case? (I don't see you touch anything for claim and declaim tags). > + } else { > + drvdata->subtype = ARM_STD_CTI; > + drvdata->offsets = cti_normal_offset; > + } > + > /* all done - dec pm refcount */ > pm_runtime_put(&adev->dev); > - dev_info(&drvdata->csdev->dev, "CTI initialized\n"); > + dev_info(&drvdata->csdev->dev, "CTI initialized; subtype=%d\n", drvdata->subtype); dev_info(&drvdata->csdev->dev, "%s CTI initialized\n", drvdata->is_qcom_cti ? "QCOM" : ""); > return 0; > > pm_release: > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > index a9df77215141..12a495382999 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > @@ -172,9 +172,8 @@ static struct attribute *coresight_cti_attrs[] = { > > /* register based attributes */ > > -/* Read registers with power check only (no enable check). */ > -static ssize_t coresight_cti_reg_show(struct device *dev, > - struct device_attribute *attr, char *buf) > +static ssize_t coresight_cti_mgmt_reg_show(struct device *dev, > + struct device_attribute *attr, char *buf) > { > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr); > @@ -189,6 +188,40 @@ static ssize_t coresight_cti_reg_show(struct device *dev, > return sysfs_emit(buf, "0x%x\n", val); > } > > +/* Read registers with power check only (no enable check). */ > +static ssize_t coresight_cti_reg_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr); > + u32 idx, val = 0; > + > + pm_runtime_get_sync(dev->parent); > + raw_spin_lock(&drvdata->spinlock); > + idx = drvdata->config.ext_reg_sel; > + if (drvdata->config.hw_powered) { > + switch (cti_attr->off) { > + case INDEX_CTITRIGINSTATUS: > + case INDEX_CTITRIGOUTSTATUS: > + case INDEX_ITTRIGINACK: > + case INDEX_ITTRIGOUT: > + case INDEX_ITTRIGOUTACK: > + case INDEX_ITTRIGIN: > + val = readl_relaxed(drvdata->base + > + cti_offset(drvdata, cti_attr->off, idx)); > + break; > + > + default: > + val = readl_relaxed(drvdata->base + cti_offset(drvdata, cti_attr->off, 0)); > + break; > + } > + } > + > + raw_spin_unlock(&drvdata->spinlock); > + pm_runtime_put_sync(dev->parent); > + return sysfs_emit(buf, "0x%x\n", val); > +} > + > /* Write registers with power check only (no enable check). */ > static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, > struct device_attribute *attr, > @@ -197,19 +230,39 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr); > unsigned long val = 0; > + u32 idx; > > if (kstrtoul(buf, 0, &val)) > return -EINVAL; > > pm_runtime_get_sync(dev->parent); > raw_spin_lock(&drvdata->spinlock); > - if (drvdata->config.hw_powered) > - cti_write_single_reg(drvdata, cti_attr->off, val); > + idx = drvdata->config.ext_reg_sel; > + if (drvdata->config.hw_powered) { > + switch (cti_attr->off) { > + case INDEX_ITTRIGINACK: > + case INDEX_ITTRIGOUT: > + cti_write_single_reg(drvdata, cti_offset(drvdata, cti_attr->off, idx), val); > + break; > + > + default: > + cti_write_single_reg(drvdata, cti_offset(drvdata, cti_attr->off, 0), val); > + break; > + } > + } For both coresight_cti_reg_show() and coresight_cti_reg_store(), can we always use "cti_attr->off" as the offset for regitser access? I mean we don't need the extra config.ext_reg_sel, eventually any register we can calculate a offset for it. > raw_spin_unlock(&drvdata->spinlock); > pm_runtime_put_sync(dev->parent); > return size; > } > > +#define coresight_cti_mgmt_reg(name, offset) \ > + (&((struct cs_off_attribute[]) { \ > + { \ > + __ATTR(name, 0444, coresight_cti_mgmt_reg_show, NULL), \ > + offset \ > + } \ > + })[0].attr.attr) > + > #define coresight_cti_reg(name, offset) \ > (&((struct cs_off_attribute[]) { \ > { \ > @@ -237,17 +290,17 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, > > /* coresight management registers */ > static struct attribute *coresight_cti_mgmt_attrs[] = { > - coresight_cti_reg(devaff0, CTIDEVAFF0), > - coresight_cti_reg(devaff1, CTIDEVAFF1), > - coresight_cti_reg(authstatus, CORESIGHT_AUTHSTATUS), > - coresight_cti_reg(devarch, CORESIGHT_DEVARCH), > - coresight_cti_reg(devid, CORESIGHT_DEVID), > - coresight_cti_reg(devtype, CORESIGHT_DEVTYPE), > - coresight_cti_reg(pidr0, CORESIGHT_PERIPHIDR0), > - coresight_cti_reg(pidr1, CORESIGHT_PERIPHIDR1), > - coresight_cti_reg(pidr2, CORESIGHT_PERIPHIDR2), > - coresight_cti_reg(pidr3, CORESIGHT_PERIPHIDR3), > - coresight_cti_reg(pidr4, CORESIGHT_PERIPHIDR4), > + coresight_cti_mgmt_reg(devaff0, CTIDEVAFF0), > + coresight_cti_mgmt_reg(devaff1, CTIDEVAFF1), > + coresight_cti_mgmt_reg(authstatus, CORESIGHT_AUTHSTATUS), > + coresight_cti_mgmt_reg(devarch, CORESIGHT_DEVARCH), > + coresight_cti_mgmt_reg(devid, CORESIGHT_DEVID), > + coresight_cti_mgmt_reg(devtype, CORESIGHT_DEVTYPE), > + coresight_cti_mgmt_reg(pidr0, CORESIGHT_PERIPHIDR0), > + coresight_cti_mgmt_reg(pidr1, CORESIGHT_PERIPHIDR1), > + coresight_cti_mgmt_reg(pidr2, CORESIGHT_PERIPHIDR2), > + coresight_cti_mgmt_reg(pidr3, CORESIGHT_PERIPHIDR3), > + coresight_cti_mgmt_reg(pidr4, CORESIGHT_PERIPHIDR4), I don't see any benefit for updating from coresight_cti_reg() to coresight_cti_mgmt_reg(). If really want to do this, should remove the macro coresight_cti_reg()? > NULL, > }; > > @@ -258,13 +311,15 @@ static struct attribute *coresight_cti_mgmt_attrs[] = { > * If inaccessible & pcached_val not NULL then show cached value. > */ > static ssize_t cti_reg32_show(struct device *dev, char *buf, > - u32 *pcached_val, int reg_offset) > + u32 *pcached_val, int index) We don't need to change anything for this. The passed "reg_offset" should be always a final offset, no matter for standard CTI or QCOM case, the driver directly uses the offset for register access. [...] > +/* > + * QCOM CTI supports up to 128 triggers, there are 6 registers need to be > + * expanded to up to 4 instances, and ext_reg_sel can be used to indicate > + * which one is in use. > + * CTITRIGINSTATUS, CTITRIGOUTSTATUS, > + * ITTRIGIN, ITTRIGOUT, > + * ITTRIGINACK, ITTRIGOUTACK. > + */ > +static ssize_t ext_reg_sel_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + u32 val; > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + raw_spin_lock(&drvdata->spinlock); > + val = drvdata->config.ext_reg_sel; > + raw_spin_unlock(&drvdata->spinlock); > + return sprintf(buf, "%d\n", val); > +} > + > +static ssize_t ext_reg_sel_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + unsigned long val; > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (kstrtoul(buf, 0, &val)) > + return -EINVAL; > + if (val > ((drvdata->config.nr_trig_max + 31) / 32 - 1)) > + return -EINVAL; > + > + raw_spin_lock(&drvdata->spinlock); > + drvdata->config.ext_reg_sel = val; > + raw_spin_unlock(&drvdata->spinlock); > + return size; > +} As said, I don't think the trigger register is any different from other register access. So the existed APIs would be sufficient. As a result, we don't need to add two above functions. Thanks, Leo

1 week, 5 days

[PATCH v2 00/12] perf cs-etm/arm-spe: Remove hard coded config fields

by James Clark

The specific config field that an event format attribute is in is consistently hard coded, even though the API is supposed to be that the driver publishes the config field name. To stop this pattern from being copy pasted and causing problems in the future, replace them all with calls to a new helper that returns the value that a user set. This reveals some issues in evsel__set_config_if_unset(). It doesn't work with sparse bitfields, which are an unused but documented feature. And it also only writes to the attr.config field. To fix it we need to start tracking user changes for all config fields and then use existing helper functions that support sparse bitfields. Some other refactoring was also required and a test was added. Signed-off-by: James Clark <james.clark(a)linaro.org> --- Changes in v2: - Remove macros in get_config_chgs() and some other refactoring. - Support sparse bitfields in evsel__set_config_if_unset(). - Always track user changes instead of only when 'pmu->perf_event_attr_init_default' is set. - Add a test. - Don't bail out in cs-etm.c if any format fields are missing (Leo). - Rename 'guess' to 'synth' (Mike). - Link to v1: https://lore.kernel.org/r/20251201-james-perf-config-bits-v1-0-22ecbbf8007c… --- James Clark (12): perf parse-events: Refactor get_config_terms() to remove macros perf evsel: Support sparse fields in evsel__set_config_if_unset() perf parse-events: Track all user changed config bits perf evsel: apply evsel__set_config_if_unset() to all config fields perf evsel: Add a helper to get the value of a config field perf parse-events: Always track user config changes perf tests: Test evsel__set_config_if_unset() and config change tracking perf cs-etm: Make a helper to find the Coresight evsel perf cs-etm: Don't use hard coded config bits when setting up ETMCR perf cs-etm: Don't use hard coded config bits when setting up TRCCONFIGR perf cs-etm: Don't hard code config attribute when configuring the event perf arm-spe: Don't hard code config attribute tools/perf/arch/arm/util/cs-etm.c | 193 +++++++++++++++------------ tools/perf/arch/arm64/util/arm-spe.c | 15 ++- tools/perf/tests/pmu.c | 90 +++++++++++++ tools/perf/util/evsel.c | 6 +- tools/perf/util/evsel.h | 2 + tools/perf/util/evsel_config.h | 7 +- tools/perf/util/parse-events.c | 248 ++++++++++++++++++++--------------- tools/perf/util/pmu.c | 112 ++++++++++++++-- 8 files changed, 459 insertions(+), 214 deletions(-) --- base-commit: 2eeb09fe1c5173b659929f92fee4461796ca8c14 change-id: 20251112-james-perf-config-bits-bee7106f0f00 Best regards, -- James Clark <james.clark(a)linaro.org>

1 week, 5 days

Re: [PATCH v6 2/2] coresight: cti: Add Qualcomm extended CTI support

by Leo Yan

Hi Yingchao, On Tue, Dec 09, 2025 at 04:16:28PM +0800, Yingchao Deng wrote: > Hi Leo & Mike > > Based on Leo’s suggestions, I created a new patch, but there are three points that do not fully align with his recommendations: > > 1. The helper function for returning the register address now returns only the offset, because returning the full address would conflict with cti_write_single_reg. No need to change each callsite for cti_write_single_reg(). You could update cti_write_single_reg() instead: void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value) { CS_UNLOCK(drvdata->base); writel_relaxed(value, cti_reg_addr(drvdata, offset)); CS_LOCK(drvdata->base); } > 2. For registers such as triginstatus1...3, I defined additional macros CTITRIGINSTATUS1...3. This is because CTITRIGINSTATUS + 0x4 equals CTITRIGOUTSTATUS, and to avoid conflicts with existing macros, I chose numbers starting from 0x1000 for the new definitions. To avoid the register naming pollution, please don't define the common names but only used for Qcom registers. AFAIK, you even don't need to define these registers. These registers are only used for sysfs knobs, we can define an extra "nr" field (e.g., bits[31..28] for indexing these registers, something like: #define CIT_REG_NR_SHIFT 28 #define CIT_REG_NR_MASK GENMASK(31, 28) #define CTI_REG_GET_NR(reg) FIELD_GET(CIT_REG_NR_MASK, (reg)) #define CTI_REG_SET_NR(reg, nr) ((reg) | FIELD_PREP(CIT_REG_NR_MASK, (nr)) static struct attribute *coresight_cti_regs_attrs[] = { ... coresight_cti_reg(triginstatus, CTITRIGINSTATUS), coresight_cti_reg(triginstatus1, CTI_REG_SET_NR(CTITRIGINSTATUS, 1)), coresight_cti_reg(triginstatus2, CTI_REG_SET_NR(CTITRIGINSTATUS, 2)), coresight_cti_reg(triginstatus3, CTI_REG_SET_NR(CTITRIGINSTATUS, 3)), ... Then, you just need to decode "nr" fields in cti_qcom_reg_off(). > 3. Regarding the visibility of attributes for triginstatus1...3, since coresight_cti_reg produces an anonymous variable that cannot be directly referenced, I used coresight_cti_regs_attrs[i] to obtain the attribute corresponding to triginstatus1. Okay, I get the meaning for "an anonymous variable" - there have no field naming when define attr with the macro coresight_cti_reg(). but you could comparing the attr string? if (!strcmp(attr->name, "triginstatus1") || !strcmp(attr->name, "triginstatus2") || !strcmp(attr->name, "triginstatus3")) ... Thanks, Leo

1 week, 5 days

Re: [PATCH v8 0/7] Provides support for Trigger Generation Unit

by Suzuki K Poulose

On 05/12/2025 01:13, Songwei Chai wrote: > > > On 12/3/2025 11:48 PM, Suzuki K Poulose wrote: >> Cc: Greg >> >> On 03/12/2025 09:00, Songwei Chai wrote: >>> We propose creating a new qcom directory under drivers/hwtracing >>> to host this TGU driver, as well as additional Qualcomm-specific >>> hwtracing drivers that we plan to submit in the coming months. >>> This structure will help organize vendor-specific implementations >>> and facilitate future development and maintenance. >>> >>> Feedback from the community on this proposal is highly appreciated. >>> >>> - Why we are proposing this: >>> >>> TGU has the ability to monitor signal conditions and trigger debug- >>> related >>> actions, serving as a programmable hardware component that enhances >>> system >>> trace and debug capabilities. Placing it under drivers/hwtracing >>> aligns with >>> its function as a trace generation utility. >>> >>> We previously attempted to push this driver to drivers/hwtracing/ >>> coresight, >>> but did not receive support from the maintainers of the CoreSight >>> subsystem. >>> The reason provided was: “This component is primarily a part of the >>> Qualcomm proprietary QPMDA subsystem, and is capable of operating >>> independently from the CoreSight hardware trace generation system.” >>> >>> Chat history : https://lore.kernel.org/all/ >>> CAJ9a7ViKxHThyZfFFDV_FkNRimk4uo1NrMtQ-kcaj1qO4ZcGnA(a)mail.gmail.com/ >>> >>> Given this, we have been considering whether it would be appropriate >>> to create a dedicated drivers/hwtracing/qcom directory for >>> Qualcomm-related hwtracing drivers. This would follow the precedent set >>> by Intel, which maintains its own directory at drivers/hwtracing/ >>> intel_th. >>> We believe this structure would significantly facilitate >>> future submissions of related Qualcomm drivers. >>> >>> - Maintenance of drivers/hwtracing/qcom: >>> >> >> Fine by, me. >> >> >>> Bjorn, who maintains linux-arm-msm, will be the maintainer of this >>> directory — we’ve discussed this with him and he’s aware that his task >>> list may grow accordingly. Additionally, Qualcomm engineers familiar >>> with >>> the debug hardware — such as [Tingwei Zhang, Jinlong Mao, Songwei Chai], >>> will be available to review incoming patches and support ongoing >>> development. >>> >>> - Detail for TGU: >>> >>> This component can be utilized to sense a plurality of signals and >>> create a trigger into the CTI or generate interrupts to processors >>> once the input signal meets the conditions. We can treat the TGU’s >>> workflow as a flowsheet, it has some “steps” regions for customization. >>> In each step region, we can set the signals that we want with priority >>> in priority_group, set the conditions in each step via condition_decode, >>> and set the resultant action by condition_select. Meanwhile, >>> some TGUs (not all) also provide timer/counter functionality. >>> Based on the characteristics described above, we consider the TGU as a >>> helper in the CoreSight subsystem. Its master device is the TPDM, which >>> can transmit signals from other subsystems, and we reuse the existing >>> ports mechanism to link the TPDM to the connected TGU. >> >> Please remove the coresight_device and other dependencies. You may use >> generic bits, CS_LOCK/UNLOCK etc. But including coresight-priv.h is >> not something I would prefer. It brings in unnecessary dependencies >> between two subsystems and I don't see any reason for using -priv.h. >> It is named as it is, for a reason, coresight private definitions. >> >> >>> >>> Here is a detailed example to explain how to use the TGU: >>> >>> In this example, the TGU is configured to use 2 conditions, 2 steps, and >>> the timer. The goal is to look for one of two patterns which are >>> generated >>> from TPDM, giving priority to one, and then generate a trigger once the >>> timer reaches a certain value. In other words, two conditions are used >>> for the first step to look for the two patterns, where the one with the >>> highest priority is used in the first condition. Then, in the second >>> step, >>> the timer is enabled and set to be compared to the given value at each >>> clock cycle. These steps are better shown below. >>> |-----------------| >>> | | >>> | TPDM | >>> | | >>> |-----------------| >>> | >>> | >>> --- --- --- --- --- --- --- --- --- --- --- --- --- --- --- --- >>> ------ >>> | | >>> | >>> | | |--------------------| | >>> | |---- ---> | | Go to next >>> steps | | >>> | | | |--- ---> | Enable >>> timer | | >>> | | v | | >>> | | >>> | | |-----------------| | |--------------------| | >>> | | | | Yes | | >>> | >>> | | | inputs==0xB | ----->| | >>> <-------- | >>> | | | | | | No | | >>> | No | |-----------------| | v | | >>> | | | | |-----------------| | | >>> | | | | | | | | >>> | | | | | timer>=3 >>> |-- | >>> | | v | | | | >>> | | |-----------------| | |-----------------| | >>> | | | | Yes | | >>> | >>> | |--- | inputs==0xA | ----->| | >>> Yes | >>> | | | | >>> | >>> | |-----------------| v | >>> | |-----------------| | >>> | | | | >>> | | Trigger | | >>> | | | | >>> | |-----------------| | >>> | TGU | >>> | >>> |--- --- --- --- --- --- --- --- --- --- --- --- --- --- |--- >>> --- -- | >>> | >>> v >>> |-----------------| >>> |The controllers | >>> |which will use | >>> |triggers further | >>> |-----------------| >>> >>> steps: >>> 1. Reset TGU /*it will disable tgu and reset dataset*/ >>> - echo 1 > /sys/bus/coresight/devices/<tgu-name>/reset_tgu >>> >>> 2. Set the pattern match for priority0 to 0xA = 0b1010 and for >>> priority 1 to 0xB = 0b1011. >>> - echo 0x11113232 > /sys/bus/coresight/devices/<tgu-name>/ >>> step0_priority0/reg0 >>> - echo 0x11113233 > /sys/bus/coresight/devices/<tgu-name>/ >>> step0_priority1/reg0 >> >> Why do they need to be coresight devices and appear on the coresight >> bus if they are not coresight devices ? As I understand they are simply >> devices with some sysfs knobs. >> >> Otherwise, happy with the proposal >> >> >> Suzuki > > Hi Suzuki, > > Would it be a good idea to register the TGU device as a platform device > so that it will appear under "/sys/devices/platform/" and the code could > also be decoupled from coresight-priv.h It is already an AMBA device, why do you need this as a platform device ? You are welcome to add a sysfs group "tgu" under the device. Suzuki > > BRs, > Songwei> >> >> >> >>> >>> Note: >>> Bit distribution diagram for each priority register >>> |-------------------------------------------------------------------| >>> | Bits | Field Nam | Description | >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 29:28 | SEL_BIT7_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 25:24 | SEL_BIT6_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 21:20 | SEL_BIT5_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 17:16 | SEL_BIT4_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 13:12 | SEL_BIT3_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 9:8 | SEL_BIT2_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 5:4 | SEL_BIT1_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> | | | 00 = bypass for OR >>> output | >>> | 1:0 | SEL_BIT0_TYPE2 | 01 = bypass for AND >>> output | >>> | | | 10 = sense input '0' is >>> true| >>> | | | 11 = sense input '1' is >>> true| >>> |-------------------------------------------------------------------| >>> These bits are used to identify the signals we want to sense, with >>> a maximum signal number of 140. For example, to sense the signal >>> 0xA (binary 1010), we set the value of bits 0 to 13 to 3232, which >>> represents 1010. The remaining bits are set to 1, as we want to use >>> AND gate to summarize all the signals we want to sense here. For >>> rising or falling edge detection of any input to the priority, set >>> the remaining bits to 0 to use an OR gate. >>> >>> 3. look for the pattern for priority_i i=0,1. >>> - echo 0x3 > /sys/bus/coresight/devices/<tgu-name>/ >>> step0_condition_decode/reg0 >>> - echo 0x30 > /sys/bus/coresight/devices/<tgu-name>/ >>> step0_condition_decode/reg1 >>> >>> |-------------------------------------------------------------------------------| >>> | Bits | Field Nam | >>> Description | >>> |-------------------------------------------------------------------------------| >>> | | |For each decoded >>> condition, this | >>> | 24 | NOT |inverts the output. If >>> the condition | >>> | | |decodes to true, and >>> the NOT field | >>> | | |is '1', then the output >>> is NOT true. | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from the associated| >>> | 21 | BC0_COMP_ACTIVE |comparator will be >>> actively included in| >>> | | |the decoding of this >>> particular | >>> | | | >>> condition. | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from the associated| >>> | | |comparator will need to >>> be 1 to affect | >>> | 20 | BC0_COMP_HIGH |the decoding of this >>> condition. | >>> | | |Conversely, a '0' here >>> requires a '0' | >>> | | |from the >>> comparator | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from the associated| >>> | 17 | |comparator will be >>> actively included in| >>> | | TC0_COMP_ACTIVE |the decoding of this >>> particular | >>> | | | >>> condition. | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from the associated| >>> | | |comparator will need to >>> be 1 to affect | >>> | 16 | TC0_COMP_HIGH |the decoding of this >>> particular | >>> | | |condition.Conversely, a >>> 0 here | >>> | | |requires a '0' from the >>> comparator | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from Priority_n | >>> | | |OR logic will be >>> actively | >>> | 4n+3 | Priority_n_OR_ACTIVE|included in the >>> decoding of | >>> | | (n=0,1,2,3) |this particular >>> condition. | >>> | | | >>> | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from Priority_n | >>> | | |will need to be '1' to >>> affect the | >>> | 4n+2 | Priority_n_OR_HIGH |decoding of this >>> particular | >>> | | (n=0,1,2,3) |condition. Conversely, >>> a '0' here | >>> | | |requires a '0' from >>> Priority_n OR logic| >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from Priority_n | >>> | | |AND logic will be >>> actively | >>> | 4n+1 |Priority_n_AND_ACTIVE|included in the >>> decoding of this | >>> | | (n=0,1,2,3) |particular >>> condition. | >>> | | | >>> | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from Priority_n | >>> | | |AND logic will need to >>> be '1' to | >>> | 4n | Priority_n_AND_HIGH |affect the decoding of >>> this | >>> | | (n=0,1,2,3) |particular condition. >>> Conversely, | >>> | | |a '0' here requires a >>> '0' from | >>> | | |Priority_n AND >>> logic. | >>> |-------------------------------------------------------------------------------| >>> Since we use `priority_0` and `priority_1` with an AND output in >>> step 2, we set `0x3` >>> and `0x30` here to activate them. >>> >>> 4. Set NEXT_STEP = 1 and TC0_ENABLE = 1 so that when the conditions >>> are met then the next step will be step 1 and the timer will >>> be enabled. >>> - echo 0x20008 > /sys/bus/coresight/devices/<tgu-name>/ >>> step0_condition_select/reg0 >>> - echo 0x20008 > /sys/bus/coresight/devices/<tgu-name>/ >>> step0_condition_select/reg1 >>> >>> |-----------------------------------------------------------------------------| >>> | Bits | Field Nam | >>> Description | >>> |-----------------------------------------------------------------------------| >>> | | |This field defines the >>> next step the | >>> | 18:17 | NEXT_STEP |TGU will 'goto' for the >>> associated | >>> | | |Condition and >>> Step. | >>> |-----------------------------------------------------------------------------| >>> | | |For each possible output >>> trigger | >>> | 13 | TRIGGER |available, set a '1' if >>> you want | >>> | | |the trigger to go active >>> for the | >>> | | |associated condition and >>> Step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will cause BC0 to >>> increment if the| >>> | 9 | BC0_INC |associated Condition is >>> decoded for | >>> | | |this >>> step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will cause BC0 to >>> decrement if the| >>> | 8 | BC0_DEC |associated Condition is >>> decoded for | >>> | | |this >>> step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will clear BC0 count >>> value to 0 if| >>> | 7 | BC0_CLEAR |the associated Condition >>> is decoded | >>> | | |for this >>> step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will cause TC0 to >>> increment until | >>> | 3 | TC0_ENABLE |paused or cleared if the >>> associated | >>> | | |Condition is decoded for >>> this step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will cause TC0 to >>> pause until | >>> | 2 | TC0_PAUSE |enabled if the associated >>> Condition | >>> | | |is decoded for this >>> step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will clear TC0 count >>> value to 0 | >>> | 1 | TC0_CLEAR |if the associated >>> Condition is | >>> | | |decoded for this >>> step. | >>> |-----------------------------------------------------------------------------| >>> | | |This will set the done >>> signal to the | >>> | 0 | DONE |TGU FSM if the associated >>> Condition | >>> | | |is decoded for this >>> step. | >>> |-----------------------------------------------------------------------------| >>> Based on the distribution diagram, we set `0x20008` for >>> `priority0` and `priority1` to >>> achieve "jump to step 1 and enable TC0" once the signal is sensed. >>> >>> 5. activate the timer comparison for this step. >>> - echo 0x30000 > /sys/bus/coresight/devices/<tgu-name>/ >>> step1_condition_decode/reg0 >>> >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from the associated| >>> | 17 | |comparator will be >>> actively included in| >>> | | TC0_COMP_ACTIVE |the decoding of this >>> particular | >>> | | | >>> condition. | >>> |-------------------------------------------------------------------------------| >>> | | |When '1' the output >>> from the associated| >>> | | |comparator will need to >>> be 1 to affect | >>> | 16 | TC0_COMP_HIGH |the decoding of this >>> particular | >>> | | |condition.Conversely, a >>> 0 here | >>> | | |requires a '0' from the >>> comparator | >>> |-------------------------------------------------------------------------------| >>> Accroding to the decode distribution diagram , we give 0x30000 >>> here to set 16th&17th bit >>> to enable timer comparison. >>> >>> 6. Set the NEXT_STEP = 0 and TC0_PAUSE = 1 and TC0_CLEAR = 1 >>> once the timer >>> has reached the given value. >>> - echo 0x6 > /sys/bus/coresight/devices/<tgu-name>/ >>> step1_condition_select/reg0 >>> >>> 7. Enable Trigger 0 for TGU when the condition 0 is met in step1, >>> i.e. when the timer reaches 3. >>> - echo 0x2000 > /sys/bus/coresight/devices/<tgu-name>/ >>> step1_condition_select/default >>> >>> Note: >>> 1. 'default' register allows for establishing the resultant >>> action for >>> the default condition >>> >>> 2. Trigger:For each possible output trigger available from >>> the Design document, there are three triggers: interrupts, CTI, >>> and Cross-TGU mapping.All three triggers can occur, but >>> the choice of which trigger to use depends on the user's >>> needs. >>> >>> 8. Compare the timer to 3 in step 1. >>> - echo 0x3 > /sys/bus/coresight/devices/<tgu-name>/step1_timer/reg0 >>> >>> 9. enale tgu >>> - echo 1 > /sys/bus/coresight/devices/<tgu-name>/enable_tgu >>> --- >>> Link to V7: https://lore.kernel.org/all/20251104064043.88972-1- >>> songwei.chai(a)oss.qualcomm.com/ >>> >>> Changes in V8: >>> - Add "select" section in bindings. >>> - Update publish date in "sysfs-bus-coresight-devices-tgu". >>> --- >>> Link to V6: https://lore.kernel.org/all/20250709104114.22240-1- >>> songchai(a)qti.qualcomm.com/ >>> >>> Changes in V7: >>> - Move the TGU code location from 'drivers/hwtracing/coresight/' to >>> 'drivers/hwtracing/qcom/'. >>> - Rename the spinlock used in the code from 'spinlock' to 'lock'. >>> - Perform the 'calculate_array_location' separately, instead of doing >>> it within the function. >>> - Update the sender email address. >>> --- >>> Link to V5: https://lore.kernel.org/all/20250529081949.26493-1- >>> quic_songchai(a)quicinc.com/ >>> >>> Changes in V6: >>> - Replace spinlock with guard(spinlock) in tgu_enable. >>> - Remove redundant blank line. >>> - Update publish date and contact member's name in "sysfs-bus- >>> coresight-devices-tgu". >>> --- >>> Link to V4: https://patchwork.kernel.org/project/linux-arm-msm/ >>> cover/20250423101054.954066-1-quic_songchai(a)quicinc.com/ >>> >>> Changes in V5: >>> - Update publish date and kernel_version in "sysfs-bus-coresight- >>> devices-tgu" >>> --- >>> Link to V3: https://lore.kernel.org/all/20250227092640.2666894-1- >>> quic_songchai(a)quicinc.com/ >>> >>> Changes in V4: >>> - Add changlog in coverletter. >>> - Correct 'year' in Copyright in patch1. >>> - Correct port mechansim description in patch1. >>> - Remove 'tgu-steps','tgu-regs','tgu-conditions','tgu-timer-counters' >>> from dt-binding >>> and set them through reading DEVID register as per Mike's suggestion. >>> - Modify tgu_disable func to make it have single return point in >>> patch2 as per >>> Mike's suggestion. >>> - Use sysfs_emit in enable_tgu_show func in ptach2. >>> - Remove redundant judgement in enable_tgu_store in patch2. >>> - Correct typo in description in patch3. >>> - Set default ret as SYSFS_GROUP_INVISIBLE, and returnret at end in >>> pacth3 as >>> per Mike's suggestion. >>> - Remove tgu_dataset_ro definition in patch3 >>> - Use #define constants with explanations of what they are rather than >>> arbitrary magic numbers in patch3 and patch4. >>> - Check -EINVAL before using 'calculate_array_location()' in array in >>> patch4. >>> - Add 'default' in 'tgu_dataset_show''s switch part in patch4. >>> - Document the value needed to initiate the reset in pacth7. >>> - Check "value" in 'reset_tgu_store' and bail out with an error code >>> if 0 in patch7. >>> - Remove dev_dbg in 'reset_tgu_store' in patch7. >>> --- >>> Link to V2: https://lore.kernel.org/all/20241010073917.16023-1- >>> quic_songchai(a)quicinc.com/ >>> >>> Changes in V3: >>> - Correct typo and format in dt-binding in patch1 >>> - Rebase to the latest kernel version >>> --- >>> Link to V1: https://lore.kernel.org/all/20240830092311.14400-1- >>> quic_songchai(a)quicinc.com/ >>> >>> Changes in V2: >>> - Use real name instead of login name, >>> - Correct typo and format in dt-binding and code. >>> - Bring order in tgu_prob(declarations with and without >>> assignments) as per >>> Krzysztof's suggestion. >>> - Add module device table in patch2. >>> - Set const for tgu_common_grp and tgu_ids in patch2. >>> - Initialize 'data' in tgu_ids to fix the warning in pacth2. >>> --- >>> >>> Songwei Chai (7): >>> dt-bindings: arm: Add support for Qualcomm TGU trace >>> qcom-tgu: Add TGU driver >>> qcom-tgu: Add signal priority support >>> qcom-tgu: Add TGU decode support >>> qcom-tgu: Add support to configure next action >>> qcom-tgu: Add timer/counter functionality for TGU >>> qcom-tgu: Add reset node to initialize >>> >>> .../testing/sysfs-bus-coresight-devices-tgu | 51 ++ >>> .../devicetree/bindings/arm/qcom,tgu.yaml | 92 +++ >>> drivers/Makefile | 1 + >>> drivers/hwtracing/Kconfig | 2 + >>> drivers/hwtracing/qcom/Kconfig | 18 + >>> drivers/hwtracing/qcom/Makefile | 3 + >>> drivers/hwtracing/qcom/tgu.c | 737 ++++++++++++++++++ >>> drivers/hwtracing/qcom/tgu.h | 252 ++++++ >>> 8 files changed, 1156 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight- >>> devices-tgu >>> create mode 100644 Documentation/devicetree/bindings/arm/qcom,tgu.yaml >>> create mode 100644 drivers/hwtracing/qcom/Kconfig >>> create mode 100644 drivers/hwtracing/qcom/Makefile >>> create mode 100644 drivers/hwtracing/qcom/tgu.c >>> create mode 100644 drivers/hwtracing/qcom/tgu.h >>> >> >

2 weeks, 2 days

[PATCH 0/7] perf cs-etm/arm-spe: Remove hard coded config fields

by James Clark

2 weeks, 3 days

Re: [PATCH v6 1/2] coresight: cti: Convert trigger usage fields to dynamic bitmaps and arrays

by Mike Leach

You are missing the review-by on this patch I sent for v5 On Tue, 2 Dec 2025 at 06:43, Yingchao Deng <yingchao.deng(a)oss.qualcomm.com> wrote: > > Replace the fixed-size u32 fields in the cti_config and cti_trig_grp > structure with dynamically allocated bitmaps and arrays. This allows > memory to be allocated based on the actual number of triggers during probe > time, reducing memory footprint and improving scalability for platforms > with varying trigger counts. > Additionally, repack struct cti_config to reduce its size from 80 bytes to > 72 bytes. > > Signed-off-by: Yingchao Deng <yingchao.deng(a)oss.qualcomm.com> > --- > drivers/hwtracing/coresight/coresight-cti-core.c | 58 ++++++++++++++++------ > .../hwtracing/coresight/coresight-cti-platform.c | 16 +++--- > drivers/hwtracing/coresight/coresight-cti-sysfs.c | 10 ++-- > drivers/hwtracing/coresight/coresight-cti.h | 17 ++++--- > 4 files changed, 65 insertions(+), 36 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c > index bfbc365bb2ef..f9970e40dd59 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-core.c > +++ b/drivers/hwtracing/coresight/coresight-cti-core.c > @@ -214,8 +214,8 @@ void cti_write_intack(struct device *dev, u32 ackval) > /* DEVID[19:16] - number of CTM channels */ > #define CTI_DEVID_CTMCHANNELS(devid_val) ((int) BMVAL(devid_val, 16, 19)) > > -static void cti_set_default_config(struct device *dev, > - struct cti_drvdata *drvdata) > +static int cti_set_default_config(struct device *dev, > + struct cti_drvdata *drvdata) > { > struct cti_config *config = &drvdata->config; > u32 devid; > @@ -234,12 +234,33 @@ static void cti_set_default_config(struct device *dev, > config->nr_trig_max = CTIINOUTEN_MAX; > } > > + config->trig_in_use = devm_bitmap_zalloc(dev, config->nr_trig_max, GFP_KERNEL); > + if (!config->trig_in_use) > + return -ENOMEM; > + > + config->trig_out_use = devm_bitmap_zalloc(dev, config->nr_trig_max, GFP_KERNEL); > + if (!config->trig_out_use) > + return -ENOMEM; > + > + config->trig_out_filter = devm_bitmap_zalloc(dev, config->nr_trig_max, GFP_KERNEL); > + if (!config->trig_out_filter) > + return -ENOMEM; > + > + config->ctiinen = devm_kcalloc(dev, config->nr_trig_max, sizeof(u32), GFP_KERNEL); > + if (!config->ctiinen) > + return -ENOMEM; > + > + config->ctiouten = devm_kcalloc(dev, config->nr_trig_max, sizeof(u32), GFP_KERNEL); > + if (!config->ctiouten) > + return -ENOMEM; > + > config->nr_ctm_channels = CTI_DEVID_CTMCHANNELS(devid); > > /* Most regs default to 0 as zalloc'ed except...*/ > config->trig_filter_enable = true; > config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0); > config->enable_req_count = 0; > + return 0; > } > > /* > @@ -270,8 +291,10 @@ int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata, > cti_dev->nr_trig_con++; > > /* add connection usage bit info to overall info */ > - drvdata->config.trig_in_use |= tc->con_in->used_mask; > - drvdata->config.trig_out_use |= tc->con_out->used_mask; > + bitmap_or(drvdata->config.trig_in_use, drvdata->config.trig_in_use, > + tc->con_in->used_mask, drvdata->config.nr_trig_max); > + bitmap_or(drvdata->config.trig_out_use, drvdata->config.trig_out_use, > + tc->con_out->used_mask, drvdata->config.nr_trig_max); > > return 0; > } > @@ -293,12 +316,20 @@ struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs, > if (!in) > return NULL; > > + in->used_mask = devm_bitmap_alloc(dev, in_sigs, GFP_KERNEL); > + if (!in->used_mask) > + return NULL; > + > out = devm_kzalloc(dev, > offsetof(struct cti_trig_grp, sig_types[out_sigs]), > GFP_KERNEL); > if (!out) > return NULL; > > + out->used_mask = devm_bitmap_alloc(dev, out_sigs, GFP_KERNEL); > + if (!out->used_mask) > + return NULL; > + > tc->con_in = in; > tc->con_out = out; > tc->con_in->nr_sigs = in_sigs; > @@ -314,7 +345,6 @@ int cti_add_default_connection(struct device *dev, struct cti_drvdata *drvdata) > { > int ret = 0; > int n_trigs = drvdata->config.nr_trig_max; > - u32 n_trig_mask = GENMASK(n_trigs - 1, 0); > struct cti_trig_con *tc = NULL; > > /* > @@ -325,8 +355,9 @@ int cti_add_default_connection(struct device *dev, struct cti_drvdata *drvdata) > if (!tc) > return -ENOMEM; > > - tc->con_in->used_mask = n_trig_mask; > - tc->con_out->used_mask = n_trig_mask; > + bitmap_fill(tc->con_in->used_mask, n_trigs); > + bitmap_fill(tc->con_out->used_mask, n_trigs); > + > ret = cti_add_connection_entry(dev, drvdata, tc, NULL, "default"); > return ret; > } > @@ -339,7 +370,6 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, > { > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *config = &drvdata->config; > - u32 trig_bitmask; > u32 chan_bitmask; > u32 reg_value; > int reg_offset; > @@ -349,18 +379,16 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, > (trigger_idx >= config->nr_trig_max)) > return -EINVAL; > > - trig_bitmask = BIT(trigger_idx); > - > /* ensure registered triggers and not out filtered */ > if (direction == CTI_TRIG_IN) { > - if (!(trig_bitmask & config->trig_in_use)) > + if (!(test_bit(trigger_idx, config->trig_in_use))) > return -EINVAL; > } else { > - if (!(trig_bitmask & config->trig_out_use)) > + if (!(test_bit(trigger_idx, config->trig_out_use))) > return -EINVAL; > > if ((config->trig_filter_enable) && > - (config->trig_out_filter & trig_bitmask)) > + test_bit(trigger_idx, config->trig_out_filter)) > return -EINVAL; > } > > @@ -892,7 +920,9 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) > raw_spin_lock_init(&drvdata->spinlock); > > /* initialise CTI driver config values */ > - cti_set_default_config(dev, drvdata); > + ret = cti_set_default_config(dev, drvdata); > + if (ret) > + return ret; > > pdata = coresight_cti_get_platform_data(dev); > if (IS_ERR(pdata)) { > diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c > index d0ae10bf6128..4bef860a0484 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-platform.c > +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c > @@ -136,8 +136,8 @@ static int cti_plat_create_v8_etm_connection(struct device *dev, > goto create_v8_etm_out; > > /* build connection data */ > - tc->con_in->used_mask = 0xF0; /* sigs <4,5,6,7> */ > - tc->con_out->used_mask = 0xF0; /* sigs <4,5,6,7> */ > + bitmap_set(tc->con_in->used_mask, 4, 4); /* sigs <4,5,6,7> */ > + bitmap_set(tc->con_out->used_mask, 4, 4); /* sigs <4,5,6,7> */ > > /* > * The EXTOUT type signals from the ETM are connected to a set of input > @@ -194,10 +194,10 @@ static int cti_plat_create_v8_connections(struct device *dev, > goto of_create_v8_out; > > /* Set the v8 PE CTI connection data */ > - tc->con_in->used_mask = 0x3; /* sigs <0 1> */ > + bitmap_set(tc->con_in->used_mask, 0, 2); /* sigs <0 1> */ > tc->con_in->sig_types[0] = PE_DBGTRIGGER; > tc->con_in->sig_types[1] = PE_PMUIRQ; > - tc->con_out->used_mask = 0x7; /* sigs <0 1 2 > */ > + bitmap_set(tc->con_out->used_mask, 0, 3); /* sigs <0 1 2 > */ > tc->con_out->sig_types[0] = PE_EDBGREQ; > tc->con_out->sig_types[1] = PE_DBGRESTART; > tc->con_out->sig_types[2] = PE_CTIIRQ; > @@ -213,7 +213,7 @@ static int cti_plat_create_v8_connections(struct device *dev, > goto of_create_v8_out; > > /* filter pe_edbgreq - PE trigout sig <0> */ > - drvdata->config.trig_out_filter |= 0x1; > + set_bit(0, drvdata->config.trig_out_filter); > > of_create_v8_out: > return ret; > @@ -257,7 +257,7 @@ static int cti_plat_read_trig_group(struct cti_trig_grp *tgrp, > if (!err) { > /* set the signal usage mask */ > for (idx = 0; idx < tgrp->nr_sigs; idx++) > - tgrp->used_mask |= BIT(values[idx]); > + set_bit(values[idx], tgrp->used_mask); > } > > kfree(values); > @@ -331,7 +331,9 @@ static int cti_plat_process_filter_sigs(struct cti_drvdata *drvdata, > > err = cti_plat_read_trig_group(tg, fwnode, CTI_DT_FILTER_OUT_SIGS); > if (!err) > - drvdata->config.trig_out_filter |= tg->used_mask; > + bitmap_or(drvdata->config.trig_out_filter, > + drvdata->config.trig_out_filter, > + tg->used_mask, drvdata->config.nr_trig_max); > > kfree(tg); > return err; > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > index 572b80ee96fb..a9df77215141 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > @@ -711,10 +711,8 @@ static ssize_t trigout_filtered_show(struct device *dev, > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *cfg = &drvdata->config; > int size = 0, nr_trig_max = cfg->nr_trig_max; > - unsigned long mask = cfg->trig_out_filter; > > - if (mask) > - size = bitmap_print_to_pagebuf(true, buf, &mask, nr_trig_max); > + size = bitmap_print_to_pagebuf(true, buf, cfg->trig_out_filter, nr_trig_max); > return size; > } > static DEVICE_ATTR_RO(trigout_filtered); > @@ -926,9 +924,8 @@ static ssize_t trigin_sig_show(struct device *dev, > struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var; > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *cfg = &drvdata->config; > - unsigned long mask = con->con_in->used_mask; > > - return bitmap_print_to_pagebuf(true, buf, &mask, cfg->nr_trig_max); > + return bitmap_print_to_pagebuf(true, buf, con->con_in->used_mask, cfg->nr_trig_max); > } > > static ssize_t trigout_sig_show(struct device *dev, > @@ -940,9 +937,8 @@ static ssize_t trigout_sig_show(struct device *dev, > struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var; > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *cfg = &drvdata->config; > - unsigned long mask = con->con_out->used_mask; > > - return bitmap_print_to_pagebuf(true, buf, &mask, cfg->nr_trig_max); > + return bitmap_print_to_pagebuf(true, buf, con->con_out->used_mask, cfg->nr_trig_max); > } > > /* convert a sig type id to a name */ > diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h > index 4f89091ee93f..e7b88b07cffe 100644 > --- a/drivers/hwtracing/coresight/coresight-cti.h > +++ b/drivers/hwtracing/coresight/coresight-cti.h > @@ -68,7 +68,7 @@ struct fwnode_handle; > */ > struct cti_trig_grp { > int nr_sigs; > - u32 used_mask; > + unsigned long *used_mask; > int sig_types[]; > }; > > @@ -146,20 +146,21 @@ struct cti_config { > bool hw_enabled; > bool hw_powered; > > - /* registered triggers and filtering */ > - u32 trig_in_use; > - u32 trig_out_use; > - u32 trig_out_filter; > bool trig_filter_enable; > u8 xtrig_rchan_sel; > > /* cti cross trig programmable regs */ > - u32 ctiappset; > u8 ctiinout_sel; > - u32 ctiinen[CTIINOUTEN_MAX]; > - u32 ctiouten[CTIINOUTEN_MAX]; > + u32 ctiappset; > u32 ctigate; > u32 asicctl; > + u32 *ctiinen; > + u32 *ctiouten; > + > + /* registered triggers and filtering */ > + unsigned long *trig_in_use; > + unsigned long *trig_out_use; > + unsigned long *trig_out_filter; > }; > > /** > > -- > 2.43.0 > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

2 weeks, 3 days

Re: [PATCH v6 6/9] dt-bindings: arm: add an interrupt property for Coresight CTCU

by Suzuki K Poulose

On 04/12/2025 02:53, Jie Gan wrote: > > > On 12/4/2025 2:14 AM, Suzuki K Poulose wrote: >> On 08/09/2025 03:01, Jie Gan wrote: >>> Add an interrupt property to CTCU device. The interrupt will be >>> triggered >>> when the data size in the ETR buffer exceeds the threshold of the >>> BYTECNTRVAL register. Programming a threshold in the BYTECNTRVAL >>> register >>> of CTCU device will enable the interrupt. >>> >>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> >>> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com> >>> --- >>> .../devicetree/bindings/arm/qcom,coresight-ctcu.yaml | 17 ++++++ >>> + ++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/qcom,coresight- >>> ctcu.yaml b/Documentation/devicetree/bindings/arm/qcom,coresight- >>> ctcu.yaml >>> index 843b52eaf872..ea05ad8f3dd3 100644 >>> --- a/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml >>> +++ b/Documentation/devicetree/bindings/arm/qcom,coresight-ctcu.yaml >>> @@ -39,6 +39,16 @@ properties: >>> items: >>> - const: apb >>> + interrupts: >>> + items: >>> + - description: Byte cntr interrupt for etr0 >>> + - description: Byte cntr interrupt for etr1 >>> + >>> + interrupt-names: >>> + items: >>> + - const: etr0 >>> + - const: etr1 >> > > Hi Suzuki, > >> Why are they named "etr0" "etr1" ? That would be confusing, isn't it, >> especially with the Linux driver naming things randomly for the TMC-ETRs. >> > > Yes, it will cause misunderstandings since the "etr0" here may not the > right device we are expecting. > >> >> What we want is the "port" number corresponding to the "TMC-ETR" being >> monitored ? >> >> Have you explored other options, "port-0", "port-1" ? >> > > I think it's much better. Will update in next version. I am not sure if there exists a better scheme for identifying or numbering the interrupts. Happy to listen to the DT experts. Rob, Krzysztof, thoughts ? Suzuki > > Thanks, > Jie > >> Suzuki >> >>> + >>> in-ports: >>> $ref: /schemas/graph.yaml#/properties/ports >>> @@ -56,6 +66,8 @@ additionalProperties: false >>> examples: >>> - | >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + >>> ctcu@1001000 { >>> compatible = "qcom,sa8775p-ctcu"; >>> reg = <0x1001000 0x1000>; >>> @@ -63,6 +75,11 @@ examples: >>> clocks = <&aoss_qmp>; >>> clock-names = "apb"; >>> + interrupts = <GIC_SPI 270 IRQ_TYPE_EDGE_RISING>, >>> + <GIC_SPI 262 IRQ_TYPE_EDGE_RISING>; >>> + interrupt-names = "etr0", >>> + "etr1"; >>> + >>> in-ports { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> >> >> >

2 weeks, 3 days

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight December 2025