Re: [PATCH v10 06/24] firmware: arm_scmi: Add basic Telemetry support
flat view
From: Jonathan Cameron <hidden>
Date: 2026-08-24 21:05:06
Also in:
arm-scmi, linux-doc, lkml
On Sun, 16 Aug 2026 00:25:46 +0100 Cristian Marussi [off-list ref] wrote:
Add SCMIv4.0 Telemetry basic support to enable initialization and resources enumeration: add all the telemetry messages definitions and parsing logic but only a few simple state gathering protocol operations. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Obviously this is huge. I'm not going to look through it all today, but some quick comments form the part I did look at. Jonathan ...
quoted hunk ↗ jump to hunk
+static void scmi_telemetry_resources_free(void *arg) +{ + struct telemetry_info *ti = arg; + struct scmi_telemetry_res_info *rinfo = ti->rinfo; + + /* Ensure rinfo is no more accessible upfront */ + smp_store_release(&ti->rinfo, NULL); + + for (int i = 0; i < rinfo->num_des; i++) { + struct telemetry_de *tde = to_tde(rinfo->des[i]); + + scmi_telemetry_free_tde_put(ti, tde);
scmi_telemetry_free_tde_put(ti, to_tde(rinfo->des[i])); Seems resonable to me and avoids the need for the local variable.
quoted hunk ↗ jump to hunk
+ } + xa_destroy(&ti->xa_des); + kfree(ti->tdes); + kfree(rinfo->des); + kfree(rinfo->dei_store); + scmi_telemetry_groups_free(rinfo); + kfree(rinfo->grps); + kfree(rinfo->grps_store); + + kfree(rinfo); +}
quoted hunk ↗ jump to hunk
+ +/** + * scmi_telemetry_resources_enumerate - Enumeration helper + * @ti: A reference to the telemetry info descriptor for this instance + * + * This helper is configured to be called once on the first enumeration + * attempt, when triggered by invoking ti->res_get() from somewhere else. + * Once run it substitues itself in ti->res_get() with the simple accessor + * __scmi_telemetry_resources_get, which returns a descriptor to the resources + * that were possibly discovered. + * + * Note that, while it attempts to fully enumerate Data Events and Groups, it + * does NOT fail when such enumerations fail, instead it simply gives up with + * the end result that only a partially populated, but consistent, resources + * descriptor will be returned; in such a case the incomplete descriptor will + * be marked as NOT fully_enumerated: this design enables the kernel to deal + * with badly implemented out-of-spec firmware support while keep on providing + * a minimal sane, albeit possibly incomplete, set of telemetry respources. + * + * Return: A reference to a fully or partially populated resources descriptor + */ +static struct scmi_telemetry_res_info * +scmi_telemetry_resources_enumerate(struct telemetry_info *ti) +{ + struct scmi_telemetry_res_info *rinfo; + struct device *dev = ti->ph->dev; + int ret; + + /* Ensure local rinfo is initialized */ + rinfo = smp_load_acquire(&ti->rinfo); + + /* + * Ensure this init function can be called only once and + * handles properly concurrent calls. + */ + if (atomic_cmpxchg(&ti->rinfo_initializing, 0, 1)) { + if (!completion_done(&ti->rinfo_initdone))
What's the logic here? This waits only if others are already waiting. Why?
+ wait_for_completion(&ti->rinfo_initdone); + goto out;
return 0;
quoted hunk ↗ jump to hunk
+ } + + ret = scmi_telemetry_de_descriptors_get(ti); + if (ret) { + dev_err(dev, FW_BUG "Cannot fully enumerate DEs resources. Degraded system.\n"); + goto done; + } + + ret = scmi_telemetry_enumerate_groups_intervals(ti); + if (ret) { + dev_err(dev, FW_BUG "Cannot fully enumerate group intervals. Degraded system.\n"); + goto done; + } + + /* Enumeration was fully successful, ensure this is visbile */ + smp_store_release(&rinfo->fully_enumerated, true); +done: + /* Disable initialization permanently */ + smp_store_mb(ti->res_get, __scmi_telemetry_resources_get); + complete_all(&ti->rinfo_initdone); + +out:
labels that just result in returns rarely add to readability of code. I'd just return early instead.
quoted hunk ↗ jump to hunk
+ return rinfo; +} + +/** + * scmi_telemetry_instance_init - Instance initializer + * @ti: A reference to the telemetry info descriptor for this instance + * + * Note that this allocates and initialize all the resources possibly needed + * and then setups the @scmi_telemetry_resources_enumerate helper as the
sets up
quoted hunk ↗ jump to hunk
+ * default method for the first call to ti->res_get(): this mechanism enables + * the possibility of optionally implementing deferred enumeration policies + * which optionally delay the discovery phase and related SCMI message exchanges + * to a later point in time. + * + * Return: 0 on Success, errno otherwise + */ +static int scmi_telemetry_instance_init(struct telemetry_info *ti) +{
...
quoted hunk ↗ jump to hunk
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index 5ab73b1ab9aa..2850b018da0d 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h@@ -2,16 +2,18 @@ /* * SCMI Message Protocol driver header * - * Copyright (C) 2018-2021 ARM Ltd. + * Copyright (C) 2018-2026 ARM Ltd. */ #ifndef _LINUX_SCMI_PROTOCOL_H #define _LINUX_SCMI_PROTOCOL_H #include <linux/bitfield.h> +#include <linux/bitops.h> #include <linux/device.h> #include <linux/notifier.h> #include <linux/types.h> +#include <linux/uuid.h> #define SCMI_MAX_STR_SIZE 64 #define SCMI_SHORT_NAME_MAX_SIZE 16@@ -824,6 +826,184 @@ struct scmi_pinctrl_proto_ops { int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin); }; +enum scmi_telemetry_de_type {
There is quite a bit of stuff here. Seems only related to telemetry so maybe a more specific header makes sense for just scmi_telemetry? The same applies for some of the other stuff already in this header like the sensors protocol elements.
quoted hunk ↗ jump to hunk
+ SCMI_TLM_DE_TYPE_USPECIFIED, + SCMI_TLM_DE_TYPE_ACCUMUL_IDLE_RESIDENCY, + SCMI_TLM_DE_TYPE_ACCUMUL_IDLE_COUNTS, + SCMI_TLM_DE_TYPE_ACCUMUL_OTHERS, + SCMI_TLM_DE_TYPE_INSTA_IDLE_STATE, + SCMI_TLM_DE_TYPE_INSTA_OTHERS, + SCMI_TLM_DE_TYPE_AVERAGE, + SCMI_TLM_DE_TYPE_STATUS, + SCMI_TLM_DE_TYPE_RESERVED_START, + SCMI_TLM_DE_TYPE_RESERVED_END = 0xef, + SCMI_TLM_DE_TYPE_OEM_START = 0xf0, + SCMI_TLM_DE_TYPE_OEM_END = 0xff, +}; + +enum scmi_telemetry_compo_type { + SCMI_TLM_COMPO_TYPE_USPECIFIED, + SCMI_TLM_COMPO_TYPE_CPU, + SCMI_TLM_COMPO_TYPE_CLUSTER, + SCMI_TLM_COMPO_TYPE_GPU, + SCMI_TLM_COMPO_TYPE_NPU, + SCMI_TLM_COMPO_TYPE_INTERCONNECT, + SCMI_TLM_COMPO_TYPE_MEM_CNTRL, + SCMI_TLM_COMPO_TYPE_L1_CACHE, + SCMI_TLM_COMPO_TYPE_L2_CACHE, + SCMI_TLM_COMPO_TYPE_L3_CACHE, + SCMI_TLM_COMPO_TYPE_LL_CACHE, + SCMI_TLM_COMPO_TYPE_SYS_CACHE, + SCMI_TLM_COMPO_TYPE_DISP_CNTRL, + SCMI_TLM_COMPO_TYPE_IPU, + SCMI_TLM_COMPO_TYPE_CHIPLET, + SCMI_TLM_COMPO_TYPE_PACKAGE, + SCMI_TLM_COMPO_TYPE_SOC, + SCMI_TLM_COMPO_TYPE_SYSTEM, + SCMI_TLM_COMPO_TYPE_SMCU, + SCMI_TLM_COMPO_TYPE_ACCEL, + SCMI_TLM_COMPO_TYPE_BATTERY, + SCMI_TLM_COMPO_TYPE_CHARGER, + SCMI_TLM_COMPO_TYPE_PMIC, + SCMI_TLM_COMPO_TYPE_BOARD, + SCMI_TLM_COMPO_TYPE_MEMORY, + SCMI_TLM_COMPO_TYPE_PERIPH, + SCMI_TLM_COMPO_TYPE_PERIPH_SUBC, + SCMI_TLM_COMPO_TYPE_LID, + SCMI_TLM_COMPO_TYPE_DISPLAY, + SCMI_TLM_COMPO_TYPE_RESERVED_START = 0x1d, + SCMI_TLM_COMPO_TYPE_RESERVED_END = 0xdf, + SCMI_TLM_COMPO_TYPE_OEM_START = 0xe0, + SCMI_TLM_COMPO_TYPE_OEM_END = 0xff, +}; + +#define SCMI_TLM_GET_UPDATE_INTERVAL_SECS(x) (FIELD_GET(GENMASK(20, 5), (x))) +#define SCMI_TLM_GET_UPDATE_INTERVAL_EXP(x) (sign_extend32((x), 4)) + +#define SCMI_TLM_GET_UPDATE_INTERVAL(x) (FIELD_GET(GENMASK(20, 0), (x))) +#define SCMI_TLM_BUILD_UPDATE_INTERVAL(s, e) \ + (FIELD_PREP(GENMASK(20, 5), (s)) | FIELD_PREP(GENMASK(4, 0), (e))) + +enum scmi_telemetry_collection { + SCMI_TLM_ONDEMAND, + SCMI_TLM_NOTIFICATION, + SCMI_TLM_SINGLE_READ, +}; + +#define SCMI_TLM_GRP_INVALID 0xFFFFFFFF + +struct scmi_telemetry_intervals { + unsigned int grp_id; + bool discrete; + unsigned int num_intervals; +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_LOW 0 +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_HIGH 1 +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_STEP 2 + unsigned int update_intervals[] __counted_by(num_intervals); +}; + +struct scmi_telemetry_grp_info { + unsigned int grp_id; + unsigned int num_des; + unsigned int num_intervals; +}; + +struct scmi_telemetry_group { + bool enabled; + bool tstamp_enabled; + unsigned int *des; + char *des_str; + struct scmi_telemetry_grp_info *info; + unsigned int active_update_interval; + struct scmi_telemetry_intervals *intervals; + enum scmi_telemetry_collection current_mode; +}; + +struct scmi_telemetry_de_info { + unsigned int id; + unsigned int grp_id; + unsigned int data_sz; + unsigned int type; + unsigned int unit; + int unit_exp; + unsigned int ts_rate; + unsigned int instance_id; + unsigned int compo_instance_id; + unsigned int compo_type; + bool persistent; + char name[16]; +}; + +struct scmi_telemetry_de { + bool tstamp_support; + bool fc_support; + bool name_support; + struct scmi_telemetry_de_info *info; + struct scmi_telemetry_group *grp; + bool enabled; + bool tstamp_enabled; +}; + +struct scmi_telemetry_res_info { + bool fully_enumerated; + unsigned int num_des; + struct scmi_telemetry_de **des; + struct scmi_telemetry_de_info *dei_store; + unsigned int num_groups; + struct scmi_telemetry_group *grps; + struct scmi_telemetry_grp_info *grps_store; +}; + +struct scmi_telemetry_base_info { + unsigned int version; + uuid_t primary_revision; + unsigned int num_des; + unsigned int num_groups; + unsigned int num_intervals; + unsigned int num_shmtis; +}; + +struct scmi_telemetry_shmti_info { + unsigned int sid; + unsigned int len; + unsigned long offset; + phys_addr_t phys; +}; + +struct scmi_telemetry_info { + bool single_read_support; + bool continuos_update_support; + bool per_group_config_support; + bool reset_support; + bool fc_support; + struct scmi_telemetry_base_info base; + unsigned int active_update_interval; + struct scmi_telemetry_intervals *intervals; + struct scmi_telemetry_shmti_info **shmtis; + unsigned int num_uuids; + uuid_t **uuids;
I'm going to guess a __counted_by_ptr marking makes sense here. (I haven't checked!) I won't bother calling out other cases, but looks like there may well be some above.
quoted hunk ↗ jump to hunk
+ bool enabled; + bool notif_enabled; + enum scmi_telemetry_collection current_mode; +};