Re: [PATCH 5/6] clk: rockchip: rk3506: Use FIELD_PREP_WM16 macro

Hi Jonas,

On 8/14/26 10:16 AM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 8/12/2026 2:42 PM, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 8/3/26 9:09 PM, Jonas Karlman wrote:
>>> Change to use writel() together with the FIELD_PREP_WM16() macro
>>> instead of using the rk_clrsetreg() macro to avoid having to define the
>>> mask as a parameter to both rk_clrsetreg() and FIELD_PREP(). Also change
>>> to use u32 variables consistently.
>>>
>>> No change in behavior is expected due to this code style change.
>>>
>>> Signed-off-by: Jonas Karlman <[email protected]>
>>> ---
>>>    drivers/clk/rockchip/clk_rk3506.c | 195 ++++++++++++++----------------
>>>    1 file changed, 90 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk_rk3506.c b/drivers/clk/rockchip/clk_rk3506.c
>>> index e156bf19a6b4..7ccab313e6ec 100644
>>> --- a/drivers/clk/rockchip/clk_rk3506.c
>>> +++ b/drivers/clk/rockchip/clk_rk3506.c
> 
> [snip]
> 
>>> @@ -770,7 +759,7 @@ static ulong rk3506_fspi_get_rate(struct rk3506_clk_priv *priv)
>>>    
>>>    static ulong rk3506_fspi_set_rate(struct rk3506_clk_priv *priv, ulong rate)
>>>    {
>>> -	int div, sel;
>>> +	u32 div, sel;
>>>    
>>>    	if (OSC_HZ % rate == 0) {
>>>    		sel = SCLK_FSPI_SEL_24M;
>>> @@ -787,10 +776,9 @@ static ulong rk3506_fspi_set_rate(struct rk3506_clk_priv *priv, ulong rate)
>>>    	}
>>>    	assert(div - 1 <= 31);
>>>    
>>
>> If somehow div = 0, then this will result in an underflow. Because it's
>> an unsigned type, it'll wrap around and start at U32_MAX so assert()
>> should catch it. However, assert() is a noop when DEBUG constant isn't
>> defined, so this actually does nothing.
>>
>> We should switch those assert to something that actually does something
>> otherwise we could still very well attempt to write a divider that's too
>> big and have weird behaviors instead of outright fail when we know we
>> cannot do something that's requested.
> 
> I was looking into trying to use assert(FIELD_FIT(...)) to validate, but
> as you mention this does not help for non-DEBUG builds.
> 
> However, I have been playing around with something like following,
> still not sure I fully like it, please advise.
> 
> static inline u32 get_divider(ulong prate, ulong rate, u32 mask, bool divisible)
> {
> 	u32 div;
> 
> 	if (divisible && prate % rate)
> 		return 0;
> 

What is "divisible" supposed to represent? I don't understand from 
reading the code.

> 	div = DIV_ROUND_UP(prate, rate);
> 	if (!div || ((div - 1) << __ffs(mask)) & ~mask)

I scratch my head looking at this so not sure it would stand the test of 
time. Would:

((div - 1) & mask) == (div - 1)

work better instead? If your value masked (I'm assuming you want to use 
the bitfield mask here, so all bits are set in the bitfield) doesn't 
equal your value, then the mask cannot contain the value.

> 		return 0;
> 
> 	return div;
> }
> 
> static ulong rk3506_saradc_set_rate(struct rk3506_clk_priv *priv, ulong clk_id,
> 				    ulong rate)
> {
> 	u32 div, sel;
> 
> 	if ((div = get_divider(32000, rate, CLK_SARADC_DIV_MASK, 1)))
> 		sel = CLK_SARADC_SEL_32K;
> 	else if ((div = get_divider(400000, rate, CLK_SARADC_DIV_MASK, 1)))
> 		sel = CLK_SARADC_SEL_400K;
> 	else if ((div = get_divider(OSC_HZ, rate, CLK_SARADC_DIV_MASK, 0)))
> 		sel = CLK_SARADC_SEL_24M;

Can you really not divide from x24m as a parent?

Also, please do not assign a variable within an if condition, I find it 
very difficult to remember that checking an if condition can have side 
effects. I can suggest:

div = get_divider(32000, rate, CLK_SARADC_DIV_MASK, 1);
if (div) {
     sel = CLK_SARADC_SEL_32K;
     goto div_found;
}

div = get_divider(400000, rate, CLK_SARADC_DIV_MASK, 1);
if (div) {
     sel = CLK_SARADC_SEL_400K;
     goto div_found;
}
[...]

return -EINVAL;

div_found:
writel(FIELD_PREP_WM16(CLK_SARADC_SEL_MASK, sel) |
[...]
return rk3506_saradc_get_rate(priv, clk_id);

Or use another function for

the writel+return currently at the end of your function and call that 
instead of a goto.

> 	else
> 		return -EINVAL;
> 
> 	writel(FIELD_PREP_WM16(CLK_SARADC_SEL_MASK, sel) |
> 	       FIELD_PREP_WM16(CLK_SARADC_DIV_MASK, div - 1),
> 	       RK3506_CLKSEL_CON(54));
> 
> 	return rk3506_saradc_get_rate(priv, clk_id);
> }
> 
> Here we use a helper function, could possible also be re-made into a
> macro so we actually can use FIELD_FIT() instead of bitwise ops.
> 
> Basic idea is that we assign div and test if the rate is divisible and
> fit inside mask at the same time, and if not we move on to next possible
> parent/divider.
> 
> With above and using the 1 MHz rate I would get something like following,
> provided I also added some log_debug() calls.
> 
>    => adc list
>    invalid divider 24 for prate 24000000, rate 1000000, mask f
>    unsupported clk: id=193 rate=1000000 ret=-22
>    - adc@ff4e8000 status: -22
> 
> In the future I would also like to drop some priv->pll_hz uses and

We should report what's the configured frequency for a PLL from the 
registers. I don't know if we can reliably cache it though (hopefully 
TF-A/OP-TEE doesn't change the ones we're interested in :) ). I don't 
know how much we would spare in boot time by avoiding reading the 
registers often and used a cache version instead.

Cheers,
Quentin