Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute support

[Sudeep Holla]

On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
> 
> There are clocks:
>  system critical, not allow linux to disable, change rate
>  allow linux to get rate, because some periphals will use the frequency
>  to configure periphals.
> 
>  So introduce an attribute to indicated FIXED clock
> 
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
>  drivers/clk/clk-scmi.c            | 6 ++++++
>  drivers/firmware/arm_scmi/clock.c | 5 ++++-
>  include/linux/scmi_protocol.h     | 1 +
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 8cbe24789c24..a539a35bd45a 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
>  	.determine_rate = scmi_clk_determine_rate,
>  };
>  
> +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> +	.recalc_rate = scmi_clk_recalc_rate,
> +};
> +
>  static const struct clk_ops scmi_atomic_clk_ops = {
>  	.recalc_rate = scmi_clk_recalc_rate,
>  	.round_rate = scmi_clk_round_rate,
> @@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  		if (is_atomic &&
>  		    sclk->info->enable_latency <= atomic_threshold)
>  			scmi_ops = &scmi_atomic_clk_ops;
> +		else if (sclk->info->rate_fixed)
> +			scmi_ops = &scmi_fixed_rate_clk_ops;
>  		else
>  			scmi_ops = &scmi_clk_ops;
>  
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index ddaef34cd88b..8c52db539e54 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -46,6 +46,7 @@ struct scmi_msg_resp_clock_attributes {
>  #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x)	((x) & BIT(30))
>  #define SUPPORTS_EXTENDED_NAMES(x)		((x) & BIT(29))
>  #define SUPPORTS_PARENT_CLOCK(x)		((x) & BIT(28))
> +#define SUPPORTS_FIXED_RATE_CLOCK(x)		((x) & BIT(27))

I don't see this in the specification, am I missing something ?

And why do we need it. Can't this be discrete clock with only one clock
rate ? Or step clock with both lowest and highest the same and step being 0.
At-least I don't see the need to change the spec for this and hence no need
to assign any attribute bit-field to represent the same.

--
Regards,
Sudeep