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

["Lothar Waßmann" <LW@KARO-electronics.de>]

Hi,

On Wed, 05 Mar 2025 21:28:19 +0800 Alice Guo (OSS) wrote:
> From: Alice Guo <alice.guo@nxp.com>
> 
> Clock driver based on SCMI clock management protocol in Linux checks
> clock state, parent and rate control permissions. To be consistent with
> the kernel driver, add this check here.
> 
> When using common clock framework (CCF), use the clock signal ID to get
> the clock registered by clk_register() in scmi_clk_probe(), and then
> obatin the struct clk_scmi variable with container_of().
> 
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/clk_scmi.c   | 173 +++++++++++++++++++++++++++++++++++++++++++----
>  include/scmi_protocols.h |  26 ++++++-
>  2 files changed, 186 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
> index 84333cdd0c..d120c1eba0 100644
> --- a/drivers/clk/clk_scmi.c
> +++ b/drivers/clk/clk_scmi.c
> @@ -12,6 +12,56 @@
>  #include <asm/types.h>
>  #include <linux/clk-provider.h>
>  
> +struct clk_scmi {
> +	struct clk clk;
> +	u32 ctrl_flags;
> +};
> +
> +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);
> +	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;
> +}
> +
>  static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
>  {
>  	struct scmi_clk_protocol_attr_out out;
> @@ -32,7 +82,8 @@ static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
>  	return 0;
>  }
>  
> -static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)
> +static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name,
> +				 u32 *attr)
>  {
>  	struct scmi_clk_attribute_in in = {
>  		.clock_id = clkid,
> @@ -53,6 +104,7 @@ static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)
>  		return ret;
>  
>  	*name = strdup(out.clock_name);
> +	*attr = out.attributes;
>  
>  	return 0;
>  }
> @@ -78,12 +130,48 @@ static int scmi_clk_gate(struct clk *clk, int enable)
>  
>  static int scmi_clk_enable(struct clk *clk)
>  {
> -	return scmi_clk_gate(clk, 1);
> +	struct clk_scmi *clkscmi;
> +	struct clk *c;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(CLK_CCF))
> +		return scmi_clk_gate(clk, 1);
> +
> +	ret = clk_get_by_id(clk->id, &c);
> +	if (ret)
> +		return ret;
> +
> +	clkscmi = container_of(c, struct clk_scmi, clk);
> +
> +	if (clkscmi->ctrl_flags & SUPPORT_CLK_STAT_CONTROL)
> +		return scmi_clk_gate(clk, 1);
> +
> +	/* Following Linux drivers/clk/clk-scmi.c, directly return 0 if agent has no permission. */
> +	log_debug("%s: SCMI CLOCK: the clock cannot be enabled by the agent.\n", __func__);
> +	return 0;
>  }
>  
>  static int scmi_clk_disable(struct clk *clk)
>  {
> -	return scmi_clk_gate(clk, 0);
> +	struct clk_scmi *clkscmi;
> +	struct clk *c;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(CLK_CCF))
> +		return scmi_clk_gate(clk, 0);
> +
> +	ret = clk_get_by_id(clk->id, &c);
> +	if (ret)
> +		return ret;
> +
> +	clkscmi = container_of(c, struct clk_scmi, clk);
> +
> +	if (clkscmi->ctrl_flags & SUPPORT_CLK_STAT_CONTROL)
> +		return scmi_clk_gate(clk, 0);
> +
> +	/* Following Linux drivers/clk/clk-scmi.c, directly return 0 if agent has no permission. */
> +	log_debug("%s: SCMI CLOCK: the clock cannot be disabled by the agent.\n", __func__);
> +	return 0;
>  }
>  
>  static ulong scmi_clk_get_rate(struct clk *clk)
> @@ -108,7 +196,7 @@ static ulong scmi_clk_get_rate(struct clk *clk)
>  	return (ulong)(((u64)out.rate_msb << 32) | out.rate_lsb);
>  }
>  
> -static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
> +static ulong __scmi_clk_set_rate(struct clk *clk, ulong rate)
>  {
>  	struct scmi_clk_rate_set_in in = {
>  		.clock_id = clk->id,
> @@ -133,9 +221,32 @@ static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
>  	return scmi_clk_get_rate(clk);
>  }
>  
> +static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
> +{
> +	struct clk_scmi *clkscmi;
> +	struct clk *c;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(CLK_CCF))
> +		return __scmi_clk_set_rate(clk, rate);
> +
> +	ret = clk_get_by_id(clk->id, &c);
> +	if (ret)
> +		return ret;
> +
> +	clkscmi = container_of(c, struct clk_scmi, clk);
> +
> +	if (clkscmi->ctrl_flags & SUPPORT_CLK_RATE_CONTROL)
> +		return __scmi_clk_set_rate(clk, rate);
> +
> +	/* Following Linux drivers/clk/clk-scmi.c, directly return 0 if agent has no permission. */
> +	log_debug("%s: SCMI CLOCK: the clock rate cannot be changed by the agent.\n", __func__);
> +	return 0;
> +}
> +
>  static int scmi_clk_probe(struct udevice *dev)
>  {
> -	struct clk *clk;
> +	struct clk_scmi *clk_scmi;
>  	size_t num_clocks, i;
>  	int ret;
>  
> @@ -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);
>  				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;
>  }
>  
> -static int scmi_clk_set_parent(struct clk *clk, struct clk *parent)
> +static int __scmi_clk_set_parent(struct clk *clk, struct clk *parent)
>  {
>  	struct scmi_clk_parent_set_in in = {
>  		.clock_id = clk->id,
> @@ -197,6 +323,29 @@ static int scmi_clk_set_parent(struct clk *clk, struct clk *parent)
>  	return scmi_to_linux_errno(out.status);
>  }
>  
> +static int scmi_clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	struct clk_scmi *clkscmi;
> +	struct clk *c;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(CLK_CCF))
> +		return -ENOTSUPP;
> +
> +	ret = clk_get_by_id(clk->id, &c);
> +	if (ret)
> +		return ret;
> +
> +	clkscmi = container_of(c, struct clk_scmi, clk);
> +
> +	if (clkscmi->ctrl_flags & SUPPORT_CLK_PARENT_CONTROL)
> +		return __scmi_clk_set_parent(clk, parent);
> +
> +	/* Following Linux drivers/clk/clk-scmi.c, directly return 0 if agent has no permission. */
> +	log_debug("%s: SCMI CLOCK: the clock's parent cannot be changed by the agent.\n", __func__);
> +	return 0;
> +}
> +
>  static const struct clk_ops scmi_clk_ops = {
>  	.enable = scmi_clk_enable,
>  	.disable = scmi_clk_disable,
> diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> index 0d8c177025..2684f4177d 100644
> --- a/include/scmi_protocols.h
> +++ b/include/scmi_protocols.h
> @@ -731,13 +731,15 @@ int scmi_pwd_name_get(struct udevice *dev, u32 domain_id, u8 **name);
>  /*
>   * SCMI Clock Protocol
>   */
> +#define CLOCK_PROTOCOL_VERSION_3_0	0x30000
>  
>  enum scmi_clock_message_id {
>  	SCMI_CLOCK_ATTRIBUTES = 0x3,
>  	SCMI_CLOCK_RATE_SET = 0x5,
>  	SCMI_CLOCK_RATE_GET = 0x6,
>  	SCMI_CLOCK_CONFIG_SET = 0x7,
> -	SCMI_CLOCK_PARENT_SET = 0xD
> +	SCMI_CLOCK_PARENT_SET = 0xD,
> +	SCMI_CLOCK_GET_PERMISSIONS = 0xF
>
add a comma to facilitate extending the enum definition


Lothar Waßmann