Re: [PATCH v14 07/32] firmware: arm_scmi: Add Telemetry basic attributes enumeration

[Cristian Marussi]

On Mon, Oct 19, 2026 at 02:27:19PM -0700, Jonathan Cameron wrote:
> On Fri, 16 Oct 2026 19:55:28 +0100
> Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> 
> > Add SCMIv4.0 Telemetry basic support for minimal initialization and
> > protocol attributes enumeration: expose telemetry instance descriptor
> > via a simple information retrieval protocol operation.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> 
> A few minor comments but nothing I care strongly about.
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
> 
> >  #endif /* _SCMI_PROTOCOLS_H */
> > diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> > new file mode 100644
> > index 000000000000..32f6f8d1106d
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/telemetry.c
> > @@ -0,0 +1,148 @@
> 
> > +
> > +enum scmi_telemetry_protocol_cmd {
> 
> Perhaps add a comment on enum scmi_common_cmd containing additional values
> that apply to telemetry.

I'll do.

> 
> > +	TELEMETRY_LIST_SHMTI = 0x3,
> 
> > +
> > +struct telemetry_info {
> > +	bool streaming_mode;
> 
> I'd bring fields of internal tracking structures in only when
> they are used (even if that is just setting them).
> 

I agree....but it has become a bit overwhelming to split this series
while taking care to introduce stuff only at the very last point in time
when it is used...I will try to improve anyway

> > +	unsigned int num_shmti;
> > +	unsigned int num_des_tstamp;
> > +	unsigned int default_blk_ts_rate;
> > +	const struct scmi_protocol_handle *ph;
> > +	struct scmi_telemetry_info info;
> > +};
> > +
> > +static int
> > +scmi_telemetry_protocol_attributes_get(struct telemetry_info *ti)
> > +{
> > +	struct scmi_msg_resp_telemetry_protocol_attributes *resp;
> > +	const struct scmi_protocol_handle *ph = ti->ph;
> > +	struct scmi_xfer *t;
> > +	int ret;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> > +				      sizeof(*resp), &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	resp = t->rx.buf;
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> > +		__le32 attr = resp->attributes_1;
> > +		__be32 de_impl_version[SCMI_TLM_DE_IMPL_NUM_DWORDS];
> > +
> 
> > +		for (int i = 0; i < SCMI_TLM_DE_IMPL_NUM_DWORDS; i++)
> > +			de_impl_version[i] =
> > +				cpu_to_be32(le32_to_cpu(resp->de_implem_rev_dword[i]));
> 
> That dance is a swap as data is stored le but UUID fields are be.
> Maybe going via cpu endian is fine as would be silly to have le32_to_be32()
> so it perhaps has documentation value.  Fair enough - I argued myself around
> with agreeing with how you did it here, but left the discussion for anyone
> else who has similar thoughts.

Yes indeed it is awkward but I did not really know how else to do it while
making it clear what was going on...by the spec the UUIDs are BEs in memory
(i.e. when laid out in the SHMTI/TDCF areas) and they are also BE in uuid_t
when 'at rest',  BUT when they are carried in a message payload they have
to be LE since all the SCMI messages payloads are LEs...

> 
> > +		import_uuid(&ti->info.base.primary_revision, (const __u8 *)&de_impl_version);
> > +		ti->info.single_read_support = SUPPORTS_SINGLE_READ(attr);
> > +		ti->info.continuous_update_support = SUPPORTS_CONTINUOS_UPDATE(attr);
> > +		ti->info.per_group_config_support = SUPPORTS_PER_GROUP_CONFIG(attr);
> > +		ti->info.reset_support = SUPPORTS_RESET(attr);
> > +		ti->info.fc_support = SUPPORTS_FC(attr);
> > +		ti->num_shmti = le32_get_bits(attr, GENMASK(15, 0));
> > +		ti->default_blk_ts_rate = le32_to_cpu(resp->default_blk_ts_rate);
> > +	}
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> 
> > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > index 5ab73b1ab9aa..121c83e1c6d8 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> 
> ...
> 
> 
> > +/**
> > + * struct scmi_telemetry_proto_ops - represents the various operations provided
> > + *	by SCMI Telemetry Protocol
> > + *
> > + * @info_get: get the general Telemetry information.
> > + */
> > +struct scmi_telemetry_proto_ops {
> info> +	const struct scmi_telemetry_info __must_check *(*info_get)
> > +		(const struct scmi_protocol_handle *ph);
> 
> Why __must_check?  Be a bit crazy if no one did anything with the
> info they just requested, so that seems overkill and doesn't help with
> the long line.
> 

... indeed this is silly...it cannot even be NULL...I will drop it..

Thanks !
Cristian