Hi Quentin,
On 10/9/2026 10:34 AM, Quentin Schulz wrote:
> Hi Jonas,
>
> On 8/4/26 11:44 PM, Jonas Karlman wrote:
>> The rockchip_get_cru() helper is used by the sysreset_rockchip to get
>> the SoC specific CRU base address. The helper may trigger a full clock
>> driver probe to get the fixed CRU base address from the clock driver
>> private data, something that can take several ms.
>>
>
> OK, but isn't probing the clock driver actually making sure the
> assigned-clocks, assigned-clock-parents and assigned-clock-rates
> properties are respected? There's *some* side effect. Considering the
> current user of that function, they aren't doing anything with clocks so
> I guess this is fine on that side. But I'm getting a bit worried, is
> there anything actually probing the clock controller driver if we don't
> have this anymore? I guess it's fine right now because we only have an
> SPL for RK35xx and thus have a Device Tree, but what happens when we
> have an open TPL?
Currently probing of clock driver is very nondeterministic, hence the
series "rockchip: Always init clocks in SPL build for RK35xx" to help
ensure clock driver is probed in a deterministic way/stage.
For TPL the clock driver may or may not be required, the DRAM clocks are
typically not handled by the clock driver so the DRAM init code may not
even need to probe the clock driver.
For DEBUG_UART in bare minimum TPL for RK35xx I have been playing with,
I use the macros defined in cru_<soc>.h directly in debug_uart_init().
Something like that could be possible in size constrained TPL, or the
clock driver could be extended to handle clocks needed for DRAM init.
Snip of board_debug_uart_init() for RK3572 that ensure that the UART
clock use correct parent and divider, full at [1]:
/* sclk_uart0_sel=osc sclk_uart0_div=0 */
writel(FIELD_PREP_WM16(CLK_UART_SEL_MASK, CLK_UART_SEL_24M) |
FIELD_PREP_WM16(CLK_UART_DIV_MASK, 0),
RK3572_CLKSEL_CON(71));
[1]
https://git.u-boot-project.org/u-boot/contributors/kwiboo/u-boot/-/blob/rk3562/arch/arm/mach-rockchip/rk3572/rk3572.c#L105
When it comes to CRU assigned-clocks, they are typically stripped from
xPL DT and will instead be applied at U-Boot proper phase. Currently
they are even applied 2-3 times due to clk_set_defaults() being called
multiple times for clock driver and also for the reset driver. Something
I will be addressing in a separate series.
>
>> Change to return the fixed RK3588 specific CRU base address directly to
>> simplify and remove the unneeded complexity of using runtime lookup in
>> SoC specific code.
>>
>> Signed-off-by: Jonas Karlman <[email protected]>
>> ---
>> arch/arm/mach-rockchip/rk3588/clk_rk3588.c | 15 +--------------
>> 1 file changed, 1 insertion(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/mach-rockchip/rk3588/clk_rk3588.c
>> b/arch/arm/mach-rockchip/rk3588/clk_rk3588.c
>> index 250ec423bd24..9b112e45f1de 100644
>> --- a/arch/arm/mach-rockchip/rk3588/clk_rk3588.c
>> +++ b/arch/arm/mach-rockchip/rk3588/clk_rk3588.c
>> @@ -4,10 +4,7 @@
>> */
>>
>> #include <dm.h>
>> -#include <syscon.h>
>> -#include <asm/arch-rockchip/clock.h>
>> #include <asm/arch-rockchip/cru_rk3588.h>
>> -#include <linux/err.h>
>>
>> int rockchip_get_clk(struct udevice **devp)
>> {
>
> There's no user of that function anymore after this patch, so just
> squash the patch removing it later in this series with this patch here.
This is a public function declared in asm/arch-rockchip/clock.h, so
dropping it in a separate commit seem more appropriate, to avoid doing
two separate things in same commit ;-)
Will send a v2 addressing your other review comments.
Regards,
Jonas
>
> Thanks!
> Quentin
