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

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

Hello Brian,

On 4/9/26 12:30 AM, Brian Masney wrote:
> Hi Thomas,
> 
> On Tue, Apr 07, 2026 at 04:25:08PM +0200, 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() operation. 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>
>> Signed-off-by: Thomas Richard (TI) <thomas.richard@bootlin.com>
>> ---
>>  drivers/clk/keystone/sci-clk.c | 42 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 34 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index 9d5071223f4c..428050a05de3 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)
>> @@ -210,10 +214,16 @@ 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;
>> +
>> +	ret = clk->provider->ops->set_freq(clk->provider->sci, clk->dev_id,
>> +					   clk->clk_id, rate / 10 * 9, rate,
>> +					   rate / 10 * 11);
>>  
>> -	return 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;
>>  }
> 
> Should the computed rate from sci_clk_recalc_rate() be saved as well?

Yes you're right. As recal_rate() is called for each clock at
registration, I'll be able to restore rate for all configured clocks
including the ones which were configured by the bootloader but not by Linux.

> 
>>  
>>  /**
>> @@ -237,9 +247,9 @@ static u8 sci_clk_get_parent(struct clk_hw *hw)
>>  		return 0;
>>  	}
>>  
>> -	parent_id = parent_id - clk->clk_id - 1;
>> +	clk->parent_id = (u8)(parent_id - clk->clk_id - 1);
>>  
>> -	return (u8)parent_id;
>> +	return clk->parent_id;
>>  }
>>  
>>  /**
>> @@ -252,12 +262,27 @@ static u8 sci_clk_get_parent(struct clk_hw *hw)
>>  static int sci_clk_set_parent(struct clk_hw *hw, u8 index)
>>  {
>>  	struct sci_clk *clk = to_sci_clk(hw);
>> +	int ret;
>>  
>>  	clk->cached_req = 0;
>>  
>> -	return clk->provider->ops->set_parent(clk->provider->sci, clk->dev_id,
>> -					      clk->clk_id,
>> -					      index + 1 + clk->clk_id);
>> +	ret = clk->provider->ops->set_parent(clk->provider->sci, clk->dev_id,
>> +					     clk->clk_id,
>> +					     index + 1 + clk->clk_id);
>> +	if (!ret)
>> +		clk->parent_id = index;
>> +
>> +	return ret;
>> +}
>> +
>> +static void sci_clk_restore_context(struct clk_hw *hw)
>> +{
>> +	struct sci_clk *clk = to_sci_clk(hw);
>> +
>> +	sci_clk_set_parent(hw, clk->parent_id);
> 
> Are all of these clocks muxes?

Not all.
I will check if the clock has more than one parent before to call
set_parent().

Best Regards,
Thomas