Re: [PATCH v2] clk: meson: Fix the determine rate error in clk_regmap_divider_ro_ops

[Chuan Liu <chuan.liu@amlogic.com>]

On 11/12/2024 9:24 PM, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
>
> On 12/11/2024 13:57, Chuan Liu via B4 Relay wrote:
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> The rate determined by calling clk_regmap_divider_ro_ops with
>> clk_regmap_div_determine_rate is not RO, which will result in the
>> unexpected modification of the frequency of its children when setting
>> the rate of a clock that references clk_regmap_divider_ro_ops.
>>
>> Fiexs: ea11dda9e091 ("clk: meson: add regmap clocks")
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>> Background: During the execution of clk_set_rate(), the function
>> clk_core_round_rate_nolock() is called to calculate the matching rate
>> and save it to 'core->new_rate'. At the same time, it recalculates and
>> updates its 'child->newrate'. Finally, clk_change_rate() is called to
>> set all 'new_rates'.
>> ---
>> Changes in v2:
>> - Remove the CLK_DIVIDER_READ_ONLY judgment logic in
>> clk_regmap_div_determine_rate().
>> - Add clk_regmap_div_ro_determine_rate().
>> - Link to v1: 
>> https://lore.kernel.org/r/20241111-fix_childclk_of_roclk_has_been_tampered_with-v1-1-f8c1b6ffdcb0@amlogic.com
>> ---
>>   drivers/clk/meson/clk-regmap.c | 36 
>> ++++++++++++++++++++----------------
>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-regmap.c 
>> b/drivers/clk/meson/clk-regmap.c
>> index 07f7e441b916..edf65ca92c7a 100644
>> --- a/drivers/clk/meson/clk-regmap.c
>> +++ b/drivers/clk/meson/clk-regmap.c
>> @@ -80,21 +80,6 @@ static int clk_regmap_div_determine_rate(struct 
>> clk_hw *hw,
>>   {
>>       struct clk_regmap *clk = to_clk_regmap(hw);
>>       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>> -     unsigned int val;
>> -     int ret;
>> -
>> -     /* if read only, just return current value */
>> -     if (div->flags & CLK_DIVIDER_READ_ONLY) {
>
> You're breaking current code by no more checking this flag,
> the new clk_regmap_div_ro_determine_rate() is fine, but you should call
> it from here if CLK_DIVIDER_READ_ONLY is set.

My idea is that the newly added clk_regmap_div_ro_determine_rate()
implements the functionality of handling CLK_DIVIDER_READ_ONLY in
clk_regmap_div_determine_rate(). If we still keep the logic for
handling CLK_DIVIDER_READ_ONLY here, it will make
clk_regmap_div_determine_rate() ambiguous and easily confused.

>
> Neil
>
>> -             ret = regmap_read(clk->map, div->offset, &val);
>> -             if (ret)
>> -                     return ret;
>> -
>> -             val >>= div->shift;
>> -             val &= clk_div_mask(div->width);
>> -
>> -             return divider_ro_determine_rate(hw, req, div->table,
>> -                                              div->width, 
>> div->flags, val);
>> -     }
>>
>>       return divider_determine_rate(hw, req, div->table, div->width,
>>                                     div->flags);
>> @@ -127,9 +112,28 @@ const struct clk_ops clk_regmap_divider_ops = {
>>   };
>>   EXPORT_SYMBOL_NS_GPL(clk_regmap_divider_ops, CLK_MESON);
>>
>> +static int clk_regmap_div_ro_determine_rate(struct clk_hw *hw,
>> +                                         struct clk_rate_request *req)
>> +{
>> +     struct clk_regmap *clk = to_clk_regmap(hw);
>> +     struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>> +     unsigned int val;
>> +     int ret;
>> +
>> +     ret = regmap_read(clk->map, div->offset, &val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     val >>= div->shift;
>> +     val &= clk_div_mask(div->width);
>> +
>> +     return divider_ro_determine_rate(hw, req, div->table, div->width,
>> +                                      div->flags, val);
>> +}
>> +
>>   const struct clk_ops clk_regmap_divider_ro_ops = {
>>       .recalc_rate = clk_regmap_div_recalc_rate,
>> -     .determine_rate = clk_regmap_div_determine_rate,
>> +     .determine_rate = clk_regmap_div_ro_determine_rate,
>>   };
>>   EXPORT_SYMBOL_NS_GPL(clk_regmap_divider_ro_ops, CLK_MESON);
>>
>>
>> ---
>> base-commit: 664988eb47dd2d6ae1d9e4188ec91832562f8f26
>> change-id: 
>> 20241111-fix_childclk_of_roclk_has_been_tampered_with-61dbcc623746
>>
>> Best regards,
>