Re: [PATCH 4/6] clk: rockchip: rk3506: Fix CLK_SARADC set rate issues

Hi Simon,

On 8/8/2026 7:39 PM, Simon Glass wrote:
> Hi Jonas,
> 
> On 2026-08-03T19:09:38, Jonas Karlman <[email protected]> wrote:
>> clk: rockchip: rk3506: Fix CLK_SARADC set rate issues
>>
>> The set_rate ops for the CLK_SARADC clock in RK3506 clock driver
>> force use of 32 KHz or 400 KHz rates when any requested rate is
>> divisible with 32 KHz or 400 KHz.
>>
>> Adjust logic to better match the 32 KHz, 400 KHz or 1.5-24 GHz rate
>> limitation of the CLK_SARADC clock in RK3506.
> 
> Shouldn't that should be 1.5-24 MHz, not GHz?
> 
>>
>> Signed-off-by: Jonas Karlman <[email protected]>
>>
>> drivers/clk/rockchip/clk_rk3506.c | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
>> diff --git a/drivers/clk/rockchip/clk_rk3506.c b/drivers/clk/rockchip/clk_rk3506.c
>> @@ -440,22 +440,19 @@ static ulong rk3506_sdmmc_set_rate(struct rk3506_clk_priv *priv, ulong clk_id,
>>  static ulong rk3506_saradc_get_rate(struct rk3506_clk_priv *priv, ulong clk_id)
>>  {
>>       u32 con, div, sel;
>> -     ulong prate;
> 
>>
>>       con = readl(RK3506_CLKSEL_CON(54));
>>       sel = FIELD_GET(CLK_SARADC_SEL_MASK, con);
>>       div = FIELD_GET(CLK_SARADC_DIV_MASK, con);
>>
>>       if (sel == CLK_SARADC_SEL_24M)
>> -             prate = OSC_HZ;
>> +             return DIV_TO_RATE(OSC_HZ, div);
>>       else if (sel == CLK_SARADC_SEL_400K)
>> -             prate = 400000;
>> +             return 400000;
>>       else if (sel == CLK_SARADC_SEL_32K)
>> -             prate = 32000;
>> +             return 32000;
> 
> The commit message only mentions set_rate, but this changes get_rate
> to ignore the divider on the 32K/400K paths. Please mention this and
> explain why - I assume the hardware divider is only wired to the 24
> MHz mux input, but that isn't obvious from the code. Also div is now
> read but unused on those two paths.

The divider is configured to 1 in set_rate path, so this change is a
simplification to ensure the ops for CLK_SARADC works as expected had
set_rate been run.

The commit message already mentions that logic is adjusted to better
match the limitation of the CLK_SARADC clock.

> 
>> diff --git a/drivers/clk/rockchip/clk_rk3506.c b/drivers/clk/rockchip/clk_rk3506.c
>> @@ -463,10 +460,10 @@ static ulong rk3506_saradc_set_rate(struct rk3506_clk_priv *priv, ulong clk_id,
>>  {
>>       u32 div, sel;
> 
>>
>> -     if (32000 % rate == 0) {
>> +     if (rate <= 32768) {
>>               sel = CLK_SARADC_SEL_32K;
>>               div = 1;
> 
> The threshold 32768 is odd given that get_rate reports this source as
> 32000 - please pick one value and use it consistently, or add a
> comment. I suspect the real source is 32.768 kHz, in which case
> get_rate should return 32768.

I would also think it should be 32768, however TRM only state 32K and
vendor U-Boot use 32000.

Due to this uncertainty and how the fallback use a very narrow divider
field, 4-bits, it is more appropriate to check for <= 32768 or rate
may have fallback to 1.5-24 MHz depending on bits [3:0] of the
DIV_ROUND_UP(24000000, rate)-1 when close to 32 KHz is requested.

I still think this change fits under adjust logic to fit clock
limitations.

Regards,
Jonas

> 
> Regards,
> Simon