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

Hi Jonas,

On 8/3/26 9:09 PM, Jonas Karlman wrote:
> 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.
> 

Same typo as previous patch, it's MHz and not GHz. Same remark as 
previous patch, the divider applies to any of the parent clock so the 
1.5-24MHz is kinda misleading.

> 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
> index 457082eea87b..e156bf19a6b4 100644
> --- 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;
>   	else
>   		return -EINVAL;
> -
> -	return DIV_TO_RATE(prate, div);

I'm not sure this is correct. The divider applies to any parent clock 
and this applies the divider only to the 24MHz parent clock.

>   }
>   
>   static ulong rk3506_saradc_set_rate(struct rk3506_clk_priv *priv, ulong clk_id,
> @@ -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;
> -	} else if (400000 % rate == 0) {
> +	} else if (rate == 400000) {
>   		sel = CLK_SARADC_SEL_400K;
>   		div = 1;

This is also not correct.

I think what you want instead is to modify div to not be always 1 but 
rather 32000 / rate and 4000000 / rate.

Cheers,
Quentin