Hi Jonas, On Sat, 8 Aug 2026 at 14:41, Jonas Karlman <[email protected]> wrote: > > 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.
Yes, agreed. Regards, Simon
