[PATCH] clk: update clk_clean_rate_cache to use private clk struct

[Patrick DELAUNAY]

Hi Sean

On 2/1/22 16:14, Sean Anderson wrote:
> On 1/31/22 10:18 AM, Patrick Delaunay wrote:
>> In clk_clean_rate_cache, clk->rate should update the private clock
>> struct, in particular when CCF is activated, to save the cached
>> rate value.
>>
>> When clk_get_parent_rate is called, the cached information
>> is read from pclk->rate, with pclk = clk_get_parent(clk).
>>
>> As the cached is read from private clk data, the update should
>> be done also on it.
>>
>> Fixes: 6b7fd3128f7 ("clk: fix set_rate to clean up cached rates for 
>> the hierarchy")
>> Signed-off-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>> ---
>>
>>   drivers/clk/clk-uclass.c | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index d245b672fa..62393b73e1 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -576,6 +576,21 @@ ulong clk_round_rate(struct clk *clk, ulong rate)
>>       return ops->round_rate(clk, rate);
>>   }
>>   +static void clk_get_priv(struct clk *clk, struct clk **clkp)
>> +{
>> +    *clkp = NULL;
>> +
>> +    /* get private clock struct associated to the provided clock */
>> +    if (CONFIG_IS_ENABLED(CLK_CCF)) {
>> +        /* Take id 0 as a non-valid clk, such as dummy */
>> +        if (clk->id)
>> +            clk_get_by_id(clk->id, clkp);
>> +    } else {
>> +        *clkp = dev_get_clk_ptr(clk->dev);
>> +    }
>
> Does this even do anything without CCF? AFAIK those clocks are the only
> ones which mirror the clock structure in udevices and keep their "real"
> struct clock as their private data. IMO this whole system is broken
> because most drivers do not/should not do this.

I take some time to follow the clk struct management in clk U-Class with

(the clk struct is in private data of U-Class) or without CCF

(the clock provider don't manage private data = clk struct is allocated by

clk API caller).


In particular, I check the clock identifier and how to be sure with CCF 
that the

identifier is unique for all clock provider (cf  clk_get_by_id) and the 
FIXME

comment in clk_register() about dev_set_uclass_priv.


But yes , in  this in this function I think, I need to change the else 
case to


+    } else {

+        *clkp = clk;
+    }


to avoid issue with other drivers without CCF.


>
> --Sean
>
>> +}
>> +
>> +/* clean cache, called with private clock struct */
>>   static void clk_clean_rate_cache(struct clk *clk)
>>   {
>>       struct udevice *child_dev;
>> @@ -595,6 +610,7 @@ static void clk_clean_rate_cache(struct clk *clk)
>>   ulong clk_set_rate(struct clk *clk, ulong rate)
>>   {
>>       const struct clk_ops *ops;
>> +    struct clk *clkp;
>>         debug("%s(clk=%p, rate=%lu)\n", __func__, clk, rate);
>>       if (!clk_valid(clk))
>> @@ -604,8 +620,10 @@ ulong clk_set_rate(struct clk *clk, ulong rate)
>>       if (!ops->set_rate)
>>           return -ENOSYS;
>>   +    /* get private clock struct used for cache */
>> +    clk_get_priv(clk, &clkp);
>>       /* Clean up cached rates for us and all child clocks */
>> -    clk_clean_rate_cache(clk);
>> +    clk_clean_rate_cache(clkp);
>>         return ops->set_rate(clk, rate);
>>   }
>>
>

Regards


Patrick