[PATCH v2 01/13] clk: scmi: add compatibility with clock protocol 2.0

[Patrick DELAUNAY]

Hi,

On 5/27/25 15:27, Patrice Chotard wrote:
> From: Valentin Caron <valentin.caron at foss.st.com>
>
> Since clock protocol 2.0, SCMI specification add an option field
> "clock_enable_delay" to CLOCK_ATTRIBUTES command.
>
> scmi_read_resp_from_smt() will return an error ("Buffer too small") as
> the message length coming from the SCMI server is not the same as expected.


For me it is more a workaround for current SCMI implementation in U-Boot

normally the message in SCMI protocol are backward compatible to avoid 
this kind of issue

=> the added properties are always optional, with default NULL value


And the scmi_read_resp_from_smt() should

- truncated the answer at the provided buffer size when the buffer is 
larger than expected

and no more response "Buffer too small"

- fill the buffer with zero if the answer if lower than buffer size


but this modification need more modifications and tests....


>
> So implement a condition to SCMI clock protocol version to change the
> length of the expected message.
>
> Signed-off-by: Valentin Caron <valentin.caron at foss.st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard at foss.st.com>
> Cc: Lukasz Majewski <lukma at denx.de>
> Cc: Sean Anderson <seanga2 at gmail.com>
> ---
>
> (no changes since v1)
>
>   drivers/clk/clk_scmi.c   | 54 +++++++++++++++++++++++++++++-----------
>   include/scmi_protocols.h | 15 +++++++++++
>   2 files changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
> index af69850cdd8..e323a948780 100644
> --- a/drivers/clk/clk_scmi.c
> +++ b/drivers/clk/clk_scmi.c
> @@ -84,26 +84,47 @@ static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
>   static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name,
>   				 u32 *attr)
>   {
> +	struct scmi_clock_priv *priv = dev_get_priv(dev);
>   	struct scmi_clk_attribute_in in = {
>   		.clock_id = clkid,
>   	};
> -	struct scmi_clk_attribute_out out;
> -	struct scmi_msg msg = {
> -		.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> -		.message_id = SCMI_CLOCK_ATTRIBUTES,
> -		.in_msg = (u8 *)&in,
> -		.in_msg_sz = sizeof(in),
> -		.out_msg = (u8 *)&out,
> -		.out_msg_sz = sizeof(out),
> -	};
>   	int ret;
>   
> -	ret = devm_scmi_process_msg(dev, &msg);
> -	if (ret)
> -		return ret;
> -
> -	*name = strdup(out.clock_name);
> -	*attr = out.attributes;
> +	if (priv->version >= 0x20000) {
> +		struct scmi_clk_attribute_out_v2 out;
> +		struct scmi_msg msg = {
> +			.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> +			.message_id = SCMI_CLOCK_ATTRIBUTES,
> +			.in_msg = (u8 *)&in,
> +			.in_msg_sz = sizeof(in),
> +			.out_msg = (u8 *)&out,
> +			.out_msg_sz = sizeof(out),
> +		};
> +
> +		ret = devm_scmi_process_msg(dev, &msg);
> +		if (ret)
> +			return ret;
> +
> +		*name = strdup(out.clock_name);
> +		*attr = out.attributes;
> +	} else {
> +		struct scmi_clk_attribute_out out;
> +		struct scmi_msg msg = {
> +			.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> +			.message_id = SCMI_CLOCK_ATTRIBUTES,
> +			.in_msg = (u8 *)&in,
> +			.in_msg_sz = sizeof(in),
> +			.out_msg = (u8 *)&out,
> +			.out_msg_sz = sizeof(out),
> +		};
> +
> +		ret = devm_scmi_process_msg(dev, &msg);
> +		if (ret)
> +			return ret;
> +
> +		*name = strdup(out.clock_name);
> +		*attr = out.attributes;
> +	}
>   
>   	return 0;
>   }
> @@ -257,6 +278,9 @@ static int scmi_clk_probe(struct udevice *dev)
>   	if (!CONFIG_IS_ENABLED(CLK_CCF))
>   		return 0;
>   
> +	ret = scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_CLOCK,
> +					    &priv->version);
> +
>   	/* register CCF children: CLK UCLASS, no probed again */
>   	if (device_get_uclass_id(dev->parent) == UCLASS_CLK)
>   		return 0;Even
> diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> index 9046de7e3e7..762a1032c37 100644
> --- a/include/scmi_protocols.h
> +++ b/include/scmi_protocols.h
> @@ -782,6 +782,21 @@ struct scmi_clk_attribute_out {
>   	char clock_name[SCMI_CLOCK_NAME_LENGTH_MAX];
>   };
>   
> +/**
> + * struct scmi_clk_get_nb_out_v2 - Response payload for SCMI_CLOCK_ATTRIBUTES command
> + * Clock management Protocol 2.0
> + * @status:	SCMI command status
> + * @attributes:	clock attributes
> + * @clock_name:	name of the clock
> + * @clock_enable_delay: delay incurred by the platform to enable the clock
> + */
> +struct scmi_clk_attribute_out_v2 {
> +	s32 status;
> +	u32 attributes;
> +	char clock_name[SCMI_CLOCK_NAME_LENGTH_MAX];
> +	u32 clock_enable_delay;
> +};
> +

see previous remark

=> So no need to create a new struct for each version of message

if SCMI correctly handle answer size for backward compatibility


>   /**
>    * struct scmi_clk_state_in - Message payload for CLOCK_CONFIG_SET command
>    * @clock_id:	SCMI clock ID


Even with previous remarks,

the patch is functional today as short term solution for clk message v2


Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>

Thanks
Patrick