Hi Jonas,
On 8/24/26 6:56 PM, Jonas Karlman wrote:
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."
Yes, which is what prate % rate == 0 would do.
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.
You set divisible only for 32K and 400K but not 24M. And after reading
the code 5 times I now understand that the only thing you care about is
that the *last* call to get_divider() sets divisible to false.
If I remember correctly, we're supposed to take the clock configuration
that allows for a frequency that is below the requested one, but as
close as possible. So the logic here is kinda broken, you'd only select
32k or 400k if we can cleanly derive the frequency, and default to 24M
otherwise.
Take me requesting 31KHz. It cannot be cleanly derived from neither
32KHz, nor 400Kz, nor 24MHz. We'll end up trying to get it from 24MHz,
while the max divider is 16 and we clearly won't be able to derive 31KHz
from it with that divider, so we return 0 and we error out. Instead, we
can at least derive something from 400KHz by using divider 13, which
amounts to ~30.77KHz. So I think we should instead just always return
div if it fits inside the register and do maths to figure out which of
the possible parents provide a rate that is lower but closest to the
requested rate.
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.
But I think the logic is broken unfortunately. And I really dislike that
we need to remember the last call to get_divider must have divisible unset.
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.
I think the kernel caches the clock rates by default (unless
CLK_GET_RATE_NOCACHE is set). They are probably doing this for a reason,
hopefully only performance? Gut feeling is this will increase boot time,
but considering you've been hunting this recently, I'm guessing you had
a look at the impact and decided the time increase (if any) wasn't
meaningful?
Cheers,
Quentin
