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

Hi Quentin,

On 8/24/2026 4:54 PM, Quentin Schulz wrote:
> 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.

"A number is divisible by another number if it can be divided into equal
parts with no remainder."

Basic idea is to keep existing pattern, we first check if rate can be
divided exact without any reminder, and fallback to do best effort to
get closest rate. Divisible was a word that seemed to match this meaning.

> 
>> 	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.

Correct, we want to ensure that div-1 fit inside the field, i.e. same as:

  if (!div || !FIELD_FIT(mask, div - 1))

However, FIELD_FIT only works with constants, so above was a quick and
dirty unpack of FIELD_FIT to test the idea. Hopefully there is a way to
instead make use of FIELD_FIT and not a custom re-implementation.

> 
>> 		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?

As mentioned above, final param was just to signal if we need exact
match / no reminder.

> 
> 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.

Yeah, I was considering goto and extra functions, but wanted to keep
the logic as small and simple as possible.

I have evolved my idea to take advantage of macros to help make the code
more readable:

	if (RATE_TO_DIV(div, 32000, rate, CLK_SARADC_DIV_MASK, 1))
		sel = CLK_SARADC_SEL_32K;
	else if (RATE_TO_DIV(div, 400000, rate, CLK_SARADC_DIV_MASK, 1))
		sel = CLK_SARADC_SEL_400K;
	else if (RATE_TO_DIV_OSC(div, rate, CLK_SARADC_DIV_MASK, 0))
		sel = CLK_SARADC_SEL_24M;
	else
		return -EINVAL;

or

	if (RATE_TO_DIV_PLL(div, PLL_V0PLL, rate, CLK_I2C0_DIV_MASK, 1))
		sel = CLK_I2C_SEL_V0PLL;
	else if (RATE_TO_DIV_PLL(div, PLL_V1PLL, rate, CLK_I2C0_DIV_MASK, 1))
		sel = CLK_I2C_SEL_V1PLL;
	else if (RATE_TO_DIV_PLL(div, PLL_GPLL, rate, CLK_I2C0_DIV_MASK, 0))
		sel = CLK_I2C_SEL_GPLL;
	else
		return -EINVAL;

It still assigns div (first parameter) inside the if, but with the
benefit that it keeps the code and code flow as simple as possible.

See rk3506 [1] or rk3538 [2] clock driver in my rk3562 branch for a full
working example of an entire clock driver using above pattern.

[1] https://git.u-boot-project.org/u-boot/contributors/kwiboo/u-boot/-/blob/rk3562/drivers/clk/rockchip/clk_rk3506.c#L500-519
[2] https://git.u-boot-project.org/u-boot/contributors/kwiboo/u-boot/-/blob/rk3562/drivers/clk/rockchip/clk_rk3538.c

> 
>> 	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.

In the clock drivers at [1] and [2] I have changed to fetch all PLL
rates via <soc>_pll_get_rate(), the calls is mostly hidden behind new
macros to keep code cleaner and easier to follow, e.g.:

	con = readl(RK3506_CLKSEL_CON(49));
	sel = FIELD_GET(CCLK_SDMMC_SEL_MASK, con);
	div = FIELD_GET(CCLK_SDMMC_DIV_MASK, con);

	if (sel == CCLK_SDMMC_SEL_24M)
		return DIV_TO_RATE_OSC(div);
	else if (sel == CCLK_SDMMC_SEL_GPLL)
		return DIV_TO_RATE_PLL(PLL_GPLL, div);
	else if (sel == CCLK_SDMMC_SEL_V0PLL)
		return DIV_TO_RATE_PLL(PLL_V0PLL, div);
	else if (sel == CCLK_SDMMC_SEL_V1PLL)
		return DIV_TO_RATE_PLL(PLL_V1PLL, div);
	else
		return -EINVAL;

With something like this we no longer need to pre-cache PLL rates at
probe time, instead PLL rates will be fetched when needed.

The rk3562 branch contains wip commit diffs that step by step transition
current rk3506 clock driver to use above proposed patterns.

Please take a look at the full rk3506 driver to see what you think :-)

Regards,
Jonas

> 
> Cheers,
> Quentin