Re: [PATCH v15 21/35] firmware: arm_scmi: Add Telemetry boot-on support

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

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

> Add the initialization and discovery logic needed to detect when the
> platform SCMI server is configured with telemetry enabled at boot and
> perform all the needed resource enumerations to keep the kernel telemetry
> subsystem state aligned with the platform boot-on configurations.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Minor things inline.

Whilst reading this I'm kind of on the lookout for things that maybe
could be pushed out to a future patch set just to cut down on the
rather large reviews needed on current set.

I'll address that separately when I get to end of the series, but
maybe this is one such thing that could wait.

Jonathan

> ---
>  drivers/firmware/arm_scmi/telemetry.c | 287 +++++++++++++++++++++++++-
>  1 file changed, 280 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index 918067e225d5..2322348f98c9 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c

> +
> +static int
> +iter_enabled_list_process_response(const struct scmi_protocol_handle *ph,
> +				   const void *response,
> +				   struct scmi_iterator_state *st, void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_de_enabled_list *r = response;
> +	const struct scmi_enabled_de_desc *desc;
> +	struct telemetry_info *ti = priv;
> +	struct telemetry_de *tde;
> +	u32 de_id;
> +	int ret;
> +
> +	desc = &r->array[st->loop_idx];
> +	de_id = le32_to_cpu(desc->id);
> +	if (scmi_telemetry_tde_lookup(ti, de_id)) {
> +		dev_err(ph->dev,
> +			"Found INVALID DE with DUPLICATED ID:0x%08X\n", de_id);
> +		return -EINVAL;
> +	}
> +
> +	tde = scmi_telemetry_tde_get(ti, de_id);
> +	if (IS_ERR(tde))
> +		return PTR_ERR(tde);
> +
> +	guard(mutex)(&tde->mtx);
> +	tde->de.info->id = de_id;
> +	tde->de.enabled = true;
> +	tde->de.tstamp_enabled = le32_to_cpu(desc->mode) == DE_ENABLED_WITH_TSTAMP;
> +	/*
> +	 * Guessing that, if a DE is reported as enabled with timestamp, it has
> +	 * to support timestamp: NOTE THAT the timestamp type (tde->ts_type) can
> +	 * ONLY be discovered later by  DE_DESCRIPTION.

Extra pace before DE_DESCRIPTION that probably wants to go.

> +	 */
> +	tde->de.tstamp_support = tde->de.tstamp_enabled;
> +
> +	ret = scmi_telemetry_tde_register(ti, tde);
> +	if (ret) {
> +		scmi_telemetry_free_tde_put(ti, tde);
> +		return ret;
> +	}
> +
> +	scmi_telemetry_de_state_update(ti, ENA_STATE, NULL, true);
> +	if (tde->de.tstamp_enabled)
> +		scmi_telemetry_de_state_update(ti, ENA_TSTAMP, NULL, true);
> +
> +	dev_dbg(ph->dev, "Registered new ENABLED DE with ID:0x%08X\n",
> +		tde->de.info->id);
> +
> +	return 0;
> +}


>  static int
>  scmi_telemetry_de_groups_init(struct telemetry_info *ti)
>  {
> @@ -1163,8 +1417,13 @@ scmi_telemetry_de_groups_init(struct telemetry_info *ti)
>  		}
>  	}
>  
> -	/* Expose all groups once all fully initialized */
> -	rinfo->num_groups = num_groups;

This reflects on patch 11, but using num_groups here was rather inconsistent
with all the use of ti->info.base.num_groups.

They are equal as it was a santity check earlier but I'd make a change
back then to use the ti->info.base.num_groups to assign this.
Then more obvious here that you have the write change to this loop.

> +	for (int i = 0; i < ti->info.base.num_groups; i++) {
> +		/* Expose groups once successfully initialized */
> +		if (!scmi_telemetry_group_config_lookup(ti, &rinfo->grps[i]))
> +			rinfo->num_groups++;
> +		else
> +			trace_scmi_tlm_access(0, "GROUP_LOOKUP_FAILED", 0, 0);
> +	}
>  
>  	return 0;
>  }