Re: [PATCH v8 07/19] clk: scmi: check the clock state/parent/rate control permissions

[Marek Vasut <marex@denx.de>]

On 3/21/25 8:15 AM, Alice Guo (OSS) wrote:

[...]

> +static int scmi_clk_get_permissions(struct udevice *dev, int clkid, u32 *perm)
> +{
> +	u32 version;
> +	int ret;
> +
> +	struct scmi_clk_get_permissions_in in = {
> +		.clock_id = clkid,
> +	};
> +	struct scmi_clk_get_permissions_out out;
> +	struct scmi_msg msg = {
> +		.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> +		.message_id = SCMI_CLOCK_GET_PERMISSIONS,
> +		.in_msg = (u8 *)&in,
> +		.in_msg_sz = sizeof(in),
> +		.out_msg = (u8 *)&out,
> +		.out_msg_sz = sizeof(out),
> +	};
> +
> +	ret = scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_CLOCK, &version);

Would it be possible to query the version in .probe() once and cache the 
value in private data ?

> +	if (ret) {
> +		log_debug("%s: get SCMI clock management protocol version failed\n", __func__);
> +		return ret;
> +	}
> +
> +	if (version < CLOCK_PROTOCOL_VERSION_3_0) {
> +		log_debug("%s: SCMI clock management protocol version is less than 3.0.\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_scmi_process_msg(dev, &msg);
> +	if (ret) {
> +		log_debug("%s: get SCMI clock management protocol permissions failed\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = scmi_to_linux_errno(out.status);
> +	if (ret < 0) {
> +		log_debug("%s: the status code of getting permissions: %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	*perm = out.permissions;
> +	return 0;
> +}

[...]

> @@ -156,29 +267,44 @@ static int scmi_clk_probe(struct udevice *dev)
>   
>   	for (i = 0; i < num_clocks; i++) {
>   		char *clock_name;
> +		u32 attributes;
>   
> -		if (!scmi_clk_get_attibute(dev, i, &clock_name)) {
> -			clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> -			if (!clk || !clock_name)
> +		if (!scmi_clk_get_attibute(dev, i, &clock_name, &attributes)) {
> +			clk_scmi = kzalloc(sizeof(*clk_scmi), GFP_KERNEL);
> +			if (!clk_scmi || !clock_name)
>   				ret = -ENOMEM;
>   			else
> -				ret = clk_register(clk, dev->driver->name,
> +				ret = clk_register(&clk_scmi->clk, dev->driver->name,
>   						   clock_name, dev->name);
>   
>   			if (ret) {
> -				free(clk);
> +				free(clk_scmi);
>   				free(clock_name);
> +				free(&attributes);

Do you really need to free() an u32 attributes here ?

>   				return ret;
>   			}
>   
> -			clk_dm(i, clk);
> +			clk_dm(i, &clk_scmi->clk);
> +
> +			if (CLK_HAS_RESTRICTIONS(attributes)) {
> +				u32 perm;
> +
> +				ret = scmi_clk_get_permissions(dev, i, &perm);
> +				if (ret < 0)
> +					clk_scmi->ctrl_flags = 0;
> +				else
> +					clk_scmi->ctrl_flags = perm;
> +			} else {
> +				clk_scmi->ctrl_flags = SUPPORT_CLK_STAT_CONTROL | SUPPORT_CLK_PARENT_CONTROL |
> +						       SUPPORT_CLK_RATE_CONTROL;
> +			}
>   		}
>   	}
>   
>   	return 0;
>   }

[...]

>   enum scmi_clock_message_id {
>   	SCMI_CLOCK_ATTRIBUTES = 0x3,
> @@ -738,6 +739,7 @@ enum scmi_clock_message_id {
>   	SCMI_CLOCK_RATE_GET = 0x6,
>   	SCMI_CLOCK_CONFIG_SET = 0x7,
>   	SCMI_CLOCK_PARENT_SET = 0xD,
> +	SCMI_CLOCK_GET_PERMISSIONS = 0xF,

Better call it SCMI_CLOCK_PERMISSIONS_GET to be consistent with the rest 
of these fields , unless this is some SCMI specification name ?

[...]