Re: [PATCH V3 2/2] clk: scmi: support state_ctrl_forbidden

[Peng Fan <peng.fan@oss.nxp.com>]

在 1/19/2024 2:27 AM, Cristian Marussi 写道:
> On Mon, Jan 15, 2024 at 02:02:03PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> Some clocks may exported to linux, while those clocks are not allowed
>> to configure by Linux. For example:
>>
>> SYS_CLK1-----
>>               \
>> 	     --MUX--->MMC1_CLK
>>               /
>> SYS_CLK2-----
>>
>> MMC1 needs set parent, so SYS_CLK1 and SYS_CLK2 are exported to Linux,
>> then the clk propagation will touch SYS_CLK1 or SYS_CLK2.
>> So we need bypass the failure for SYS_CLK1 or SYS_CLK2 when enable
>> the clock of MMC1.
>>
> 
> Hi,
> 
> so this looks good to me and apparently (as noted) the CLK framework is OK
> with a driver swallowing the -EACCESS when a clock is immutable, BUT at the
> end of the day do we even need to try this SCMI call and hide the failure in
> case of immutable clocks ?
> 
> I mean, what if we just dont provide any callback for enable/disable...I can
> see plenty of drivers not providing those callbacks ?
> Maybe this is probably more of a question for Stephen...
> 
> IOW what about doing something like below...does it make any difference
> in your setup ? works fine in my emulated env

It should be fine to use your changes. Do you expect me to use your 
patch or make it as a follow up patch?

> 
> (Note that last snippet in clk_gate_restore_context() is probably a fix
>   that needs to be added anyway by looking at the code in clk.c)

This API seems only used by TI gate driver, this change should be in a
standalone change go through clk tree. So I would your changes
as a standalone optimization follow up patch, while not included
in my patchset.

THanks,
Peng.

> 
> Thanks,
> Cristian
> 
> --->8----
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 5327e0547741..a669a2f2f78b 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -121,11 +121,7 @@ static int scmi_clk_enable(struct clk_hw *hw)
>   	struct scmi_clk *clk = to_scmi_clk(hw);
>   	int ret;
>   
> -	ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
> -	if (ret == -EACCES && clk->info->state_ctrl_forbidden)
> -		return 0;
> -
> -	return ret;
> +	return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
>   }
>   
>   static void scmi_clk_disable(struct clk_hw *hw)
> @@ -140,11 +136,7 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw)
>   	struct scmi_clk *clk = to_scmi_clk(hw);
>   	int ret;
>   
> -	ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
> -	if (ret == -EACCES && clk->info->state_ctrl_forbidden)
> -		return 0;
> -
> -	return ret;
> +	return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
>   }
>   
>   static void scmi_clk_atomic_disable(struct clk_hw *hw)
> @@ -204,6 +196,15 @@ static const struct clk_ops scmi_atomic_clk_ops = {
>   	.determine_rate = scmi_clk_determine_rate,
>   };
>   
> +static const struct clk_ops scmi_no_state_ctrl_clk_ops = {
> +	.recalc_rate = scmi_clk_recalc_rate,
> +	.round_rate = scmi_clk_round_rate,
> +	.set_rate = scmi_clk_set_rate,
> +	.set_parent = scmi_clk_set_parent,
> +	.get_parent = scmi_clk_get_parent,
> +	.determine_rate = scmi_clk_determine_rate,
> +};
> +
>   static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
>   			     const struct clk_ops *scmi_ops)
>   {
> @@ -300,8 +301,10 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>   		 * specify (or support) an enable_latency associated with a
>   		 * clock, we default to use atomic operations mode.
>   		 */
> -		if (is_atomic &&
> -		    sclk->info->enable_latency <= atomic_threshold)
> +		if (sclk->info->state_ctrl_forbidden)
> +			scmi_ops = &scmi_no_state_ctrl_clk_ops;
> +		else if (is_atomic &&
> +			 sclk->info->enable_latency <= atomic_threshold)
>   			scmi_ops = &scmi_atomic_clk_ops;
>   		else
>   			scmi_ops = &scmi_clk_ops;
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f0940af485a5..79b90a8099d7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1200,9 +1200,11 @@ void clk_gate_restore_context(struct clk_hw *hw)
>   	struct clk_core *core = hw->core;
>   
>   	if (core->enable_count)
> -		core->ops->enable(hw);
> +		if (core->ops->enable)
> +			core->ops->enable(hw);
>   	else
> -		core->ops->disable(hw);
> +		if (core->ops->disable)
> +			core->ops->disable(hw);
>   }
>   EXPORT_SYMBOL_GPL(clk_gate_restore_context);
> ---8<---
> 
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>
>> V3:
>>   Add check in atomic enable
>>
>> V2:
>>   New. Take Cristian's suggestion
>>
>>   drivers/clk/clk-scmi.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
>> index 8cbe24789c24..5327e0547741 100644
>> --- a/drivers/clk/clk-scmi.c
>> +++ b/drivers/clk/clk-scmi.c
>> @@ -119,8 +119,13 @@ static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *r
>>   static int scmi_clk_enable(struct clk_hw *hw)
>>   {
>>   	struct scmi_clk *clk = to_scmi_clk(hw);
>> +	int ret;
>> +
>> +	ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
>> +	if (ret == -EACCES && clk->info->state_ctrl_forbidden)
>> +		return 0;
>>   
>> -	return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
>> +	return ret;
>>   }
>>   
>>   static void scmi_clk_disable(struct clk_hw *hw)
>> @@ -133,8 +138,13 @@ static void scmi_clk_disable(struct clk_hw *hw)
>>   static int scmi_clk_atomic_enable(struct clk_hw *hw)
>>   {
>>   	struct scmi_clk *clk = to_scmi_clk(hw);
>> +	int ret;
>> +
>> +	ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
>> +	if (ret == -EACCES && clk->info->state_ctrl_forbidden)
>> +		return 0;
>>   
>> -	return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
>> +	return ret;
>>   }
>>   
>>   static void scmi_clk_atomic_disable(struct clk_hw *hw)
>> -- 
>> 2.37.1
>>