Re: [PATCH 05/10] firmware: arm_scmi: Add Telemetry protocol support
flat view
From: Jonathan Cameron <hidden>
Date: 2025-10-17 15:37:30
Also in:
arm-scmi, lkml
On Thu, 25 Sep 2025 21:35:49 +0100 Cristian Marussi [off-list ref] wrote:
Add basic support for SCMI V4.0-alpha_0 Telemetry protocol including SHMTI, FastChannels, Notifications and Single Sample Reads collection methods. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Hi, This is very much in the superficial drive by category as reviews go. A few things noted but I've not looked at the code in enough detail. Jonathan
quoted hunk ↗ jump to hunk
diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c new file mode 100644 index 000000000000..f03000c173c2 --- /dev/null +++ b/drivers/firmware/arm_scmi/telemetry.c@@ -0,0 +1,2117 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * System Control and Management Interface (SCMI) Telemetry Protocol + * + * Copyright (C) 2025 ARM Ltd. + *
My favorite trivial comment applies. What does this blank line add to readability? I'd drop it.
+ */
quoted hunk ↗ jump to hunk
+ +struct scmi_de_desc { + __le32 id; + __le32 grp_id; + __le32 data_sz; + __le32 attr_1; +#define IS_NAME_SUPPORTED(d) ((d)->attr_1 & BIT(31)) +#define IS_FC_SUPPORTED(d) ((d)->attr_1 & BIT(30)) +#define GET_DE_TYPE(d) (le32_get_bits((d)->attr_1, GENMASK(29, 22))) +#define IS_PERSISTENT(d) ((d)->attr_1 & BIT(21)) +#define GET_DE_UNIT_EXP(d) \ + ({ \ + int __signed_exp = \ + le32_get_bits((d)->attr_1, GENMASK(20, 13)); \ + \ + if (__signed_exp & BIT(7)) \ + __signed_exp |= GENMASK(31, 8); \ + __signed_exp; \ + }) +#define GET_DE_UNIT(d) (le32_get_bits((d)->attr_1, GENMASK(12, 5))) + +#define GET_DE_TSTAMP_EXP(d) \ + ({ \ + int __signed_exp = \ + FIELD_GET(GENMASK(4, 1), (d)->attr_1); \ + \ + if (__signed_exp & BIT(3)) \ + __signed_exp |= GENMASK(31, 4); \ + __signed_exp; \
See below for sign_extend32() using code to replace these.
quoted hunk ↗ jump to hunk
+ +struct scmi_msg_resp_telemetry_reading_complete { + __le32 num_dwords; + __le32 dwords[];
__counted_by(num_word);
quoted hunk ↗ jump to hunk
+}; + +/* TDCF */ + +#define TO_CPU_64(h, l) (((u64)le32_to_cpu((h)) << 32) | le32_to_cpu((l)))
Some of this stuff sounds very generic and isn't at all. Personally I think I'd just drop this one as it may be better to see the implementation wherever it is used.
quoted hunk ↗ jump to hunk
+static int scmi_telemetry_tdcf_line_parse(struct telemetry_info *ti, + struct payload __iomem *payld, + struct telemetry_shmti *shmti, + bool update) +{ + int used_qwords; + + used_qwords = (USE_LINE_TS(payld) && TS_VALID(payld)) ? + QWORDS_TS_LINE_DATA_PAYLD : QWORDS_LINE_DATA_PAYLD; + + /*Invalid lines are not an error, could simply be disabled DEs */
Check for inconsistent comment syntax etc.
+ if (DATA_INVALID(payld)) + return used_qwords;
quoted hunk ↗ jump to hunk
+ +static int scmi_telemetry_shmti_scan(struct telemetry_info *ti, + unsigned int shmti_id, u64 ts, + bool update) +{ + struct telemetry_shmti *shmti = &ti->shmti[shmti_id]; + struct tdcf __iomem *tdcf = shmti->base; + int retries = SCMI_TLM_TDCF_MAX_RETRIES; + u64 startm = 0, endm = 0xffffffffffffffff;
No one likes counting fs. Use a GENMASK probably.
+ void *eplg = SHMTI_EPLG(shmti);
quoted hunk ↗ jump to hunk
+static void +scmi_telemetry_msg_payld_process(struct telemetry_info *ti, + unsigned int num_dwords, unsigned int *dwords,
I'd kind of expect something called dwords to have a fixed size. u32, u64 or whatever.
quoted hunk ↗ jump to hunk
+ ktime_t timestamp) +{ + u32 next = 0; + + while (next < num_dwords) { + struct payload *payld = (struct payload *)&dwords[next]; + struct scmi_telemetry_de *de; + struct telemetry_de *tde; + u32 de_id; + + next += USE_LINE_TS(payld) ? + TS_LINE_DATA_PAYLD_WORDS : LINE_DATA_PAYLD_WORDS; + + if (DATA_INVALID(payld)) { + dev_err(ti->dev, "MSG - Received INVALID DATA line\n"); + continue; + } + + de_id = le32_to_cpu(payld->id); + de = xa_load(&ti->xa_des, de_id); + if (!de || !de->enabled) { + dev_err(ti->dev, + "MSG - Received INVALID DE - ID:%u enabled:%d\n", + de_id, de ? (de->enabled ? 'Y' : 'N') : 'X'); + continue; + } + + tde = to_tde(de); + guard(mutex)(&tde->mtx); + tde->cached = true; + tde->last_val = LINE_DATA_GET(&payld->tsl); + //TODO BLK_TS in notification payloads + if (USE_LINE_TS(payld) && TS_VALID(payld)) + tde->last_ts = LINE_TSTAMP_GET(&payld->tsl); + else + tde->last_ts = 0; + } +}
quoted hunk ↗ jump to hunk
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index 59527193d6dd..6c6db95d0089 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h
...
+#define SCMI_TLM_GET_UPDATE_INTERVAL_SECS(x) \ + (le32_get_bits((x), GENMASK(20, 5)))
Why is this one little endian specific and the next just uses assumption of CPU Endian?
quoted hunk ↗ jump to hunk
+#define SCMI_TLM_GET_UPDATE_INTERVAL_EXP(x) \ + ({ \ + int __signed_exp = FIELD_GET(GENMASK(4, 0), (x)); \ + \ + if (__signed_exp & BIT(4)) \ + __signed_exp |= GENMASK(31, 5); \
sign_extend32() from bitops.h should work here and is much more self explanatory. That would then make this something like #define SCMI_TLM_GET_UPDATE_INTERVAL_EXP(x) \ sign_extend32(x, 4); or you can mask it first if you like but I don't think it makes any difference in practice.
quoted hunk ↗ jump to hunk
+ __signed_exp; \ + }) + +#define SCMI_TLM_BUILD_UPDATE_INTERVAL(s, e) \ + (FIELD_PREP(GENMASK(20, 5), (s)) | FIELD_PREP(GENMASK(4, 0), (e)))
quoted hunk ↗ jump to hunk
+ +struct scmi_telemetry_update_report { + ktime_t timestamp; + unsigned int agent_id; + int status; + unsigned int num_dwords; + unsigned int dwords[];
More places where __counted_by is appropriate. I'll not comment on any others and just assume you'll add them wherever appropriate.
+};