Re: [PATCH v2 05/17] firmware: arm_scmi: Add Telemetry protocol support

[Cristian Marussi <cristian.marussi@arm.com>]

On Fri, Jan 23, 2026 at 11:00:46AM +0000, Elif Topuz wrote:
> 
> Hi Cristian,

HI Elif,

a long overdue reply from me !

Thanks for having a look at this series...

> 
> On 14/01/2026 11:46, Cristian Marussi wrote:
> > Add basic support for SCMI V4.0 Telemetry protocol including SHMTI,
> > FastChannels, Notifications and Single Sample Reads collection methods.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > v1 --> v2
> >  - Add proper ioread accessors for TDCF areas
> >  - Rework resource allocation logic and lifecycle
> >  - Introduce new resources accessors and res_get() operation to
> >    implement lazy enumeration
> >  - Support boot-on telemetry:
> >    + Add DE_ENABLED_LIST cmd support
> >    + Add CONFIG_GET cmd support
> >    + Add TDCF_SCAN for best effort enumeration
> >    + Harden driver against out-of-spec FW with out of spec cmds
> >  - Use FCs list
> >  - Rework de_info_lookup to a moer general de_lookup
> >  - Fixed reset to use CONFIG_GET and DE_ENABLED_LIST to gather initial
> >    state
> >  - Using sign_extend32 helper
> >  - Added counted_by marker where appropriate
> > ---
> >  drivers/firmware/arm_scmi/Makefile    |    2 +-
> >  drivers/firmware/arm_scmi/driver.c    |    2 +
> >  drivers/firmware/arm_scmi/protocols.h |    1 +
> >  drivers/firmware/arm_scmi/telemetry.c | 2671 +++++++++++++++++++++++++
> >  include/linux/scmi_protocol.h         |  188 +-
> >  5 files changed, 2862 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/firmware/arm_scmi/telemetry.c
> > 

[snip]

> > +	/* Build composing DES string */
> > +	for (int i = 0; i < ti->info.base.num_groups; i++) {
> > +		struct scmi_telemetry_group *grp = &rinfo->grps[i];
> > +		size_t bufsize = grp->info->num_des * 11 + 1;
> > +		char *buf = grp->des_str;
> > +
> > +		for (int j = 0; j < grp->info->num_des; j++) {
> > +			char term = j != (grp->info->num_des - 1) ? ' ' : '\0';
> > +			int len;
> > +
> > +			len = scnprintf(buf, bufsize, "0x%04X%c",
> 
> I think it should be 0x%08X%c to print 8 bytes.
> 

Good catch ! I will fix..

> > +					rinfo->des[grp->des[j]]->info->id, term);
> > +
> > +			buf += len;
> > +			bufsize -= len;
> > +		}
> > +	}
> > +

[snip]

> > +
> > +static int scmi_telemetry_tdcf_line_parse(struct telemetry_info *ti,
> > +					  struct payload __iomem *payld,
> > +					  struct telemetry_shmti *shmti,
> > +					  enum scan_mode mode)
> > +{
> > +	int used_qwords;
> > +
> > +	used_qwords = (USE_LINE_TS(payld) && TS_VALID(payld)) ?
> > +		QWORDS_TS_LINE_DATA_PAYLD : QWORDS_LINE_DATA_PAYLD;
> 
> If I understand correctly from the "chat with ATG" comments below, when
> timestamp is disabled (after DE is enabled with timestamp), physically ts line
> is still there (use_line_ts = 1 and ts_valid = 0). That's why I think we need to
> drop TS_VALID(payld) check. Otherwise qwords will be QWORDS_LINE_DATA_PAYLD
> although ts line exists.
> 
> Also, for the alpha version of the spec, the combination of use_line_ts = 1 and
> ts_valid = 0 doesn't seem to be possible as I understand. But I think beta
> version allows us?

Yes indeed ALPHA_0 definition of TS Line fields was ambiguos but now in
BETA the field is more explicitly called as LineExtension bit field...so
the code in the upcoming V3 will change accordingly.

> 
> > +
> > +	/* Invalid lines are not an error, could simply be disabled DEs */
> > +	if (DATA_INVALID(payld))
> > +		return used_qwords;
> > +
> > +	if (!IS_BLK_TS(payld))
> > +		scmi_telemetry_tdcf_data_parse(ti, payld, shmti, mode);
> > +	else
> > +		scmi_telemetry_tdcf_blkts_parse(ti, payld, shmti);
> > +
> > +	return used_qwords;
> > +}
> > +
> > +static int scmi_telemetry_shmti_scan(struct telemetry_info *ti,
> > +				     unsigned int shmti_id, u64 ts,
> > +				     enum scan_mode mode)
> 
> ts is not used in the function.
> 

Yes indeed I have removed it in V3.

> > +{
> > +	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 = TDCF_BAD_END_SEQ;
> > +	void *eplg = SHMTI_EPLG(shmti);
> > +
> > +	if (!tdcf)
> > +		return -ENODEV;
> > +

[snip]

> > +
> > +		de_id = le32_to_cpu(payld->id);
> > +		de = xa_load(&ti->xa_des, de_id);
> > +		if (!de || !de->enabled) {
> > +			dev_err(ti->ph->dev,
> > +				"MSG - Received INVALID DE - ID:%u  enabled:%d\n",
> 
> enabled:%c?
> 

Yep

> > +				de_id, de ? (de->enabled ? 'Y' : 'N') : 'X');
> > +			continue;

[snip]

> >  struct scmi_power_state_changed_report {
> > @@ -1114,4 +1292,12 @@ struct scmi_powercap_meas_changed_report {
> >  	unsigned int	domain_id;
> >  	unsigned int	power;
> >  };
> > +
> > +struct scmi_telemetry_update_report {
> > +	ktime_t		timestamp;
> > +	unsigned int	agent_id;
> > +	int		status;
> > +	unsigned int	num_dwords;
> > +	unsigned int	dwords[];
> > +};
> >  #endif /* _LINUX_SCMI_PROTOCOL_H */
> 
> Thanks,
> Elif

Thanks for the review !
Cristian

PS: (nitpick) you should trim your review replies to remove all the long
bunch of code that you are not comenting on  (leaving a [snip] placeholder
as I did above)... it helps finding your comments in the sea of code...