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;
div = DIV_ROUND_UP(prate, rate);
if (!div || ((div - 1) << __ffs(mask)) & ~mask)
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;
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
instead get that from a function, so would look something like:
if ((div = get_divisor(rk3506_pll_get_rate(priv, PLL_V1PLL),
rate, CCLK_SDMMC_DIV_MASK, 1)))
sel = CCLK_SDMMC_SEL_V1PLL;
Please let me know what you think of using something similar to above.
Regards,
Jonas
>
> This here doesn't change the logic anyway so this can be done in a
> follow-up patch, thus:
>
> Reviewed-by: Quentin Schulz <[email protected]>
>
> Thanks!
> Quentin
