Re: [PATCH v6 3/4] clk: keystone: sci-clk: add restore_context() operation

[Thomas Richard <thomas.richard@bootlin.com>]

On 5/5/26 1:42 PM, Nishanth Menon wrote:
> On 14:21-20260427, Thomas Richard (TI) wrote:
>> Implement the restore_context() operation to restore the clock rate and the
>> clock parent state. The clock rate is saved in sci_clk struct during
>> set_rate() and recalc_rate() operations. The parent index is saved in
>> sci_clk struct during set_parent() operation. During clock registration,
>> the core retrieves each clock’s parent using get_parent() operation to
>> ensure the internal clock tree reflects the actual hardware state,
>> including any configurations made by the bootloader. So we also save the
>> parent index in get_parent().
>>
>> Reviewed-by: Dhruva Gole <d-gole@ti.com>
>> Reviewed-by: Kendall Willis <k-willis@ti.com>
>> Signed-off-by: Thomas Richard (TI) <thomas.richard@bootlin.com>
>> ---
>>  drivers/clk/keystone/sci-clk.c | 44 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index 9d5071223f4c..b090edf5f82e 100644
>> --- a/drivers/clk/keystone/sci-clk.c
>> +++ b/drivers/clk/keystone/sci-clk.c
>> @@ -47,6 +47,8 @@ struct sci_clk_provider {
>>   * @node:	 Link for handling clocks probed via DT
>>   * @cached_req:	 Cached requested freq for determine rate calls
>>   * @cached_res:	 Cached result freq for determine rate calls
>> + * @parent_id:	 Parent index for this clock
>> + * @rate:	 Clock rate
>>   */
>>  struct sci_clk {
>>  	struct clk_hw hw;
>> @@ -58,6 +60,8 @@ struct sci_clk {
>>  	struct list_head node;
>>  	unsigned long cached_req;
>>  	unsigned long cached_res;
>> +	u8 parent_id;
>> +	unsigned long rate;
>>  };
>>  
>>  #define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
>> @@ -150,6 +154,8 @@ static unsigned long sci_clk_recalc_rate(struct clk_hw *hw,
>>  		return 0;
>>  	}
>>  
>> +	clk->rate = freq;
>> +
>>  	return freq;
>>  }
>>  
>> @@ -210,10 +216,15 @@ static int sci_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>>  			    unsigned long parent_rate)
>>  {
>>  	struct sci_clk *clk = to_sci_clk(hw);
>> +	int ret;
>>  
>> -	return clk->provider->ops->set_freq(clk->provider->sci, clk->dev_id,
>> -					    clk->clk_id, rate / 10 * 9, rate,
>> -					    rate / 10 * 11);
>> +	ret = clk->provider->ops->set_freq(clk->provider->sci, clk->dev_id,
>> +					   clk->clk_id, rate / 10 * 9, rate,
>> +					   rate / 10 * 11);
>> +	if (!ret)
>> +		clk->rate = rate;
>> +
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -237,9 +248,9 @@ static u8 sci_clk_get_parent(struct clk_hw *hw)
>>  		return 0;
> 
> What happens if get_parent() fails during clock registration? Looking at
> sci_clk_get_parent(), if the firmware call fails, parent_id will be a
> valid value of 0? and in restore, we'd attempt to restore it back? I
> think that is wrong.

Okay. I'll use an integer for parent_id, so I can also store
get_parent()'s return code in case of error. And in restore_context() I
call set_parent() only if num_parents > 1 and parent_id >= 0.

Best Regards,
Thomas