Re: [PATCH v15 12/35] firmware: arm_scmi: Add Telemetry update intervals enumeration

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon,  9 Nov 2026 09:11:58 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Add support to enumerate available Telemetry update intervals.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v14 --> v15
>  - removed unused dev reference in interval parsing
>  - added missing initialization of intrvs->grp_id
> v13 --> v14
>  - splitted out of the old Basic Telemetry patch, contains the
>    update_intervals enumeration routines
> ---
>  drivers/firmware/arm_scmi/telemetry.c | 198 ++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h         |  21 +++
>  2 files changed, 219 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index 9944506652f5..e57534c93b26 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c
> @@ -92,6 +92,21 @@ struct scmi_msg_resp_telemetry_de_description {
>  	struct scmi_de_desc desc[];
>  };
>  
> +struct scmi_msg_telemetry_update_intervals {
> +	__le32 index;
> +	__le32 group_identifier;
> +#define	ALL_DES_NO_GROUP	0x0
> +#define SPECIFIC_GROUP_DES	0x1
> +#define ALL_DES_ANY_GROUP	0x2

For previous defines like this they have followed the value they are in.
So I think these belong after flags.  (That fits with common practice for
this sort of thing which is why I looked).

> +	__le32 flags;
> +};
> +
> +struct scmi_msg_resp_telemetry_update_intervals {
> +	__le32 flags;
> +#define INTERVALS_DISCRETE(x)	(le32_get_bits((x), BIT(12)) == 0)
> +	__le32 intervals[];
> +};
> +
>  #define IS_TDE_TSTAMP_TYPE(__t, __ttype)	((__t)->ts_type == (__ttype))
>  #define IS_TDE_TS_NONE(_t)	(IS_TDE_TSTAMP_TYPE((_t), TSTAMP_NONE))
>  #define IS_TDE_TS_LINE(_t)	(IS_TDE_TSTAMP_TYPE((_t), TSTAMP_LINE))
> @@ -693,6 +708,176 @@ static int scmi_telemetry_de_descriptors_get(struct telemetry_info *ti)
>  	return scmi_telemetry_de_groups_init(ti);
>  }
>  
> +struct scmi_tlm_ivl_priv {
> +	struct scmi_telemetry_intervals **intrvs;
> +	unsigned int grp_id;
> +	unsigned int flags;

Given these are fixed size, maybe u32 for each of them just to
make sure that matches up.

> +};
> +
> +static void iter_intervals_prepare_message(void *message,
> +					   unsigned int desc_index,
> +					   const void *priv)
> +{
> +	struct scmi_msg_telemetry_update_intervals *msg = message;
> +	const struct scmi_tlm_ivl_priv *p = priv;
> +
> +	msg->index = cpu_to_le32(desc_index);
> +	msg->group_identifier = cpu_to_le32(p->grp_id);
> +	msg->flags = cpu_to_le32(FIELD_PREP(GENMASK(3, 0), p->flags));

Need the mask?  It's valid but I'd expect the protection on sanity
to be at the point of p->flags being written rather than here.

> +}
> +
> +static int iter_intervals_update_state(struct scmi_iterator_state *st,
> +				       const void *response, void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_update_intervals *r = response;
> +
> +	st->num_returned = le32_get_bits(r->flags, GENMASK(11, 0));
> +	st->num_remaining = le32_get_bits(r->flags, GENMASK(31, 16));
> +
> +	if (st->rx_len < (sizeof(*r) + sizeof(r->intervals[0]) * st->num_returned))
> +		return -EINVAL;
> +
> +	/*
> +	 * total intervals is not declared previously anywhere so we
> +	 * assume it's returned+remaining on first call.
> +	 */
> +	if (!st->max_resources) {
> +		struct scmi_tlm_ivl_priv *p = priv;
> +		struct scmi_telemetry_intervals *intrvs;
> +		bool discrete;
> +		int inum;
> +
> +		discrete = INTERVALS_DISCRETE(r->flags);
> +		/* Check consistency on first call */
> +		if (!discrete && (st->num_returned != 3 || st->num_remaining != 0))
> +			return -EINVAL;
> +
> +		inum = st->num_returned + st->num_remaining;
> +		intrvs = kzalloc_flex(*intrvs, update_intervals, inum);
> +		if (!intrvs)
> +			return -ENOMEM;
> +
> +		intrvs->num_intervals = inum;
> +		intrvs->discrete = discrete;
> +		intrvs->grp_id = p->grp_id;
> +		st->max_resources = intrvs->num_intervals;
> +
> +		*p->intrvs = intrvs;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +iter_intervals_process_response(const struct scmi_protocol_handle *ph,
> +				const void *response,
> +				struct scmi_iterator_state *st, void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_update_intervals *r = response;
> +	struct scmi_tlm_ivl_priv *p = priv;
> +	struct scmi_telemetry_intervals *intrvs = *p->intrvs;
> +	unsigned int idx = st->loop_idx;
> +
> +	intrvs->update_intervals[st->desc_index + idx] =
> +		le32_to_cpu(r->intervals[idx]);

I'd go a bit long and put that on one line.

> +
> +	return 0;
> +}

> +static int
> +scmi_telemetry_enumerate_common_intervals(struct telemetry_info *ti)
> +{
> +	unsigned int flags;
> +	int ret;
> +
> +	flags = !ti->info.per_group_config_support ?
> +		ALL_DES_ANY_GROUP : ALL_DES_NO_GROUP;

Why not flip the condition and reverse the values?
This thing is weird enough I'd add a comment or rename them
to be more helpful. ALL_DES_THAT_HAVE_NO_GROUP for example.


> +
> +	ret = scmi_tlm_enumerate_update_intervals(ti, &ti->info.intervals,
> +						  SCMI_TLM_GRP_INVALID, flags);
> +	if (ret)
> +		return ret;
> +
> +	/* A copy for UAPI access... */
> +	ti->info.base.num_intervals = ti->info.intervals->num_intervals;
> +
> +	/* Delegate freeing of allocated intervals to unbind time */
> +	return devm_add_action_or_reset(ti->ph->dev,
> +					scmi_telemetry_intervals_free,
> +					ti->info.intervals);
> +}

>  /**