Re: [PATCH v15 13/35] firmware: arm_scmi: Add Telemetry SHMTI areas enumeration

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

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

> Add Telemetry support to enumerate SHMTI areas.

I'd be tempted to leave the local data structures like snaps to
be introduced only when code actually uses them.

> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v14 --> v15
>  - fix prologue/epilogue endianity fields
>  - rework SHMTI initialization and allocation
>  - add snapshot suppport data structures
>  - added alignment checks on SHMTII length size
>  - fixed error string formatting
>  - warn if less than expected SHMTIs are enumerated
>  - improve memory area sanity checks
> v13 --> v14
>  - splitted out of the old Basic Telemetry patch, contains the
>    SHMTI areas enumeration routines
> ---
>  drivers/firmware/arm_scmi/telemetry.c | 312 ++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h         |  59 +++++
>  2 files changed, 371 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/telemetry.c b/drivers/firmware/arm_scmi/telemetry.c
> index e57534c93b26..d9f1a3e3468e 100644
> --- a/drivers/firmware/arm_scmi/telemetry.c
> +++ b/drivers/firmware/arm_scmi/telemetry.c
> @@ -16,6 +16,7 @@
>  #include <linux/limits.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/sizes.h>
>  #include <linux/slab.h>
>  #include <linux/sprintf.h>
>  #include <linux/string.h>

> +struct scmi_msg_resp_telemetry_shmti_list {
> +	__le32 num_shmti;

Add a comment that this is not actually the number of shmti. 
Not confusing spec name at all.   Maybe just put a pair of
extraction macros right after it like you've had for
some of the other similar cases and that will act as documentation
that not all is what it seems.

> +	struct scmi_shmti_desc desc[];
> +};
> +
>  struct de_desc_fastchannel {
>  	__le32 addr_low;
>  	__le32 addr_high;
> @@ -107,6 +121,72 @@ struct scmi_msg_resp_telemetry_update_intervals {
>  	__le32 intervals[];
>  };
>  
> +/* TDCF */
> +
> +#define _I(__a)		(ioread32((void __iomem *)(__a)))

Is that really worth doing?  I think I'd rather see some long lines
with that expanded out.

> +
> +struct prlg {
> +	__le32 sign_start;
> +#define SIGNATURE_START	0x5442474E	/* TBGN */
> +	__le32 match_start;
> +	__le32 num_qwords;
> +	__le32 hdr_meta_1;
> +#define TDCF_REVISION_GET(x)	(le32_get_bits((x)->hdr_meta_1, GENMASK(7, 0)))

Outer brackets needed?

> +};
> +
> +struct eplg {
> +	__le32 match_end;
> +	__le32 sign_end;
> +#define SIGNATURE_END	0x54454E44	/* TEND */
> +};
> +
> +#define TDCF_EPLG_SZ	(sizeof(struct eplg))
> +
> +struct tdcf {
> +	struct prlg prlg;
> +	unsigned char payld[];
> +};
> +
> +#define SHMTI_MIN_SIZE	(sizeof(struct tdcf) + TDCF_EPLG_SZ)
> +/* An arbitrary upper bound to avoid crazy allocations driven by buggy FW */
> +#define SHMTI_MAX_SIZE	SZ_64K
> +
> +#define TDCF_START_SIGNATURE(x)	(_I(&((x)->prlg.sign_start)))
> +#define TDCF_END_SIGNATURE(e)	(_I(&((e)->sign_end)))


>  	/* Mutex to protect access to @free_des */
> @@ -878,6 +960,211 @@ scmi_telemetry_enumerate_common_intervals(struct telemetry_info *ti)
>  					ti->info.intervals);
>  }
>  
> +static int iter_shmti_update_state(struct scmi_iterator_state *st,
> +				   const void *response, void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_shmti_list *r = response;
> +
> +	st->num_returned = le32_get_bits(r->num_shmti, GENMASK(15, 0));
> +	st->num_remaining = le32_get_bits(r->num_shmti, GENMASK(31, 16));
I'd move those two into defines above, just to document that num_shmti is more
complex than it seems at first glance. Also for general consistency.
> +
> +	if (st->rx_len < (sizeof(*r) + sizeof(r->desc[0]) * st->num_returned))
> +		return -EINVAL;
> +
> +	return 0;
> +}

> +
> +static int iter_shmti_process_response(const struct scmi_protocol_handle *ph,
> +				       const void *response,
> +				       struct scmi_iterator_state *st,
> +				       void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_shmti_list *r = response;
> +	struct telemetry_info *ti = priv;
> +	struct telemetry_shmti *shmti;
> +	const struct scmi_shmti_desc *desc;
> +	void __iomem *addr;
> +	phys_addr_t phys_addr;
> +	size_t len;
> +	int ret;
> +
> +	desc = &r->desc[st->loop_idx];
> +	shmti = &ti->shmti[st->desc_index + st->loop_idx];
> +
> +	shmti->info.sid = le32_to_cpu(desc->id);
> +	shmti->flags = le32_to_cpu(desc->flags);
> +
> +	ret = scmi_telemetry_area_check(ph->dev, "SHMTI",
> +					le32_to_cpu(desc->addr_low),
> +					le32_to_cpu(desc->addr_high),

Same as before. I'd do the build of phys_addr and len outside the
call. Keep it to checking validity as it's name suggests.

> +					le32_to_cpu(desc->length),
> +					&phys_addr, &len);
> +	if (ret) {
> +		dev_err(ph->dev, "Malformed SHMTI ID:%u. Abort.\n",
> +			shmti->info.sid);
> +		return ret;
> +	}
> +
> +	if (len < SHMTI_MIN_SIZE || len > SHMTI_MAX_SIZE) {
> +		dev_err(ph->dev,
> +			FW_BUG "Invalid length for SHMTI ID:%u len:%zu. Abort.\n",
> +			shmti->info.sid, len);
> +		return -EINVAL;
> +	}
> +
> +	/* SHMTI base phys_addr SHOULD be 64bit aligned by the SCMI spec */
> +	if (!IS_ALIGNED(phys_addr, 8))
> +		dev_warn(ph->dev,
> +			 "SHMTI ID:%u is NOT 64bit aligned (paddr:0x%llX)\n",
> +			 shmti->info.sid, phys_addr);
> +
> +	addr = devm_ioremap(ph->dev, phys_addr, len);
> +	if (!addr)
> +		return -EADDRNOTAVAIL;
> +
> +	shmti->base = addr;
> +	shmti->info.len = len;
> +	shmti->info.phys = phys_addr;
> +	shmti->info.offset = shmti->info.phys & ~PAGE_MASK;
> +
> +	ret = scmi_telemetry_shmti_validate(ph->dev, shmti);
> +	if (ret)
> +		return ret;
> +
> +	ti->num_shmti++;
> +
> +	return 0;
> +}


> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f38ca2f8099b..b7b8bd77b37d 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -830,6 +830,57 @@ struct scmi_pinctrl_proto_ops {
>  	int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
>  };
>  

> +
> +enum scmi_telemetry_compo_type {
> +	SCMI_TLM_COMPO_TYPE_USPECIFIED,
> +	SCMI_TLM_COMPO_TYPE_CPU,
> +	SCMI_TLM_COMPO_TYPE_CLUSTER,
> +	SCMI_TLM_COMPO_TYPE_GPU,
> +	SCMI_TLM_COMPO_TYPE_NPU,
> +	SCMI_TLM_COMPO_TYPE_INTERCONNECT,
> +	SCMI_TLM_COMPO_TYPE_MEM_CNTRL,
> +	SCMI_TLM_COMPO_TYPE_L1_CACHE,
> +	SCMI_TLM_COMPO_TYPE_L2_CACHE,
> +	SCMI_TLM_COMPO_TYPE_L3_CACHE,
> +	SCMI_TLM_COMPO_TYPE_LL_CACHE,
> +	SCMI_TLM_COMPO_TYPE_SYS_CACHE,
> +	SCMI_TLM_COMPO_TYPE_DISP_CNTRL,
> +	SCMI_TLM_COMPO_TYPE_IPU,
> +	SCMI_TLM_COMPO_TYPE_CHIPLET,
> +	SCMI_TLM_COMPO_TYPE_PACKAGE,
> +	SCMI_TLM_COMPO_TYPE_SOC,
> +	SCMI_TLM_COMPO_TYPE_SYSTEM,
> +	SCMI_TLM_COMPO_TYPE_SMCU,
> +	SCMI_TLM_COMPO_TYPE_ACCEL,
> +	SCMI_TLM_COMPO_TYPE_BATTERY,
> +	SCMI_TLM_COMPO_TYPE_CHARGER,
> +	SCMI_TLM_COMPO_TYPE_PMIC,
> +	SCMI_TLM_COMPO_TYPE_BOARD,
> +	SCMI_TLM_COMPO_TYPE_MEMORY,
> +	SCMI_TLM_COMPO_TYPE_PERIPH,
> +	SCMI_TLM_COMPO_TYPE_PERIPH_SUBC,
> +	SCMI_TLM_COMPO_TYPE_LID,
> +	SCMI_TLM_COMPO_TYPE_DISPLAY,

Die 0x1d is in the version I'm looking at. Perhaps a comment
for specific version of spec used.  I'd also
be tempted to assign every one of these as it makes
it easy to eyeball an particular one vs the spec.

> +	SCMI_TLM_COMPO_TYPE_RESERVED_START = 0x1d,
> +	SCMI_TLM_COMPO_TYPE_RESERVED_END = 0xdf,
> +	SCMI_TLM_COMPO_TYPE_OEM_START = 0xe0,
> +	SCMI_TLM_COMPO_TYPE_OEM_END = 0xff,
> +};