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
