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

Thomas Richard
2026-05-05T15:14:48Z

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