[PATCH v2 2/2] rockchip: rock5b-rk3588: Add support for ROCK 5B+

Mon Aug 11 23:17:01 CEST 2025

Hi Quentin,

On 8/11/2025 11:24 AM, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 8/1/25 7:09 PM, Jonas Karlman wrote:
>> Include FDTs for both ROCK 5B and 5B+ in the FIT and add board selection
>> code to load the 5B+ FDT when the DRAM type is LPDDR5 and ADC channel 5
>> value is close to 4095.
>>
>>    U-Boot 2025.07 (Jul 14 2025 - 21:28:20 +0000)
>>
>>    Model: Radxa ROCK 5B+
>>    SoC:   RK3588
>>    DRAM:  8 GiB
>>
>> Features tested on a ROCK 5B+ v1.2:
>> - SD-card boot
>> - eMMC boot
>> - SPI flash boot
>> - PCIe/NVMe
>> - Ethernet
>> - USB/TCPM
>>
>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>> ---
>> Changes in v2:
>> - Add DRAM type check in addition to SARADC check
>> ---
>>   arch/arm/dts/rk3588-rock-5b-plus-u-boot.dtsi |  3 +
>>   arch/arm/dts/rk3588-rock-5b-u-boot.dtsi      |  5 ++
>>   board/radxa/rock5b-rk3588/Kconfig            |  5 ++
>>   board/radxa/rock5b-rk3588/MAINTAINERS        |  3 +-
>>   board/radxa/rock5b-rk3588/rock5b-rk3588.c    | 63 ++++++++++++++++++++
>>   configs/rock5b-rk3588_defconfig              |  1 +
>>   doc/board/rockchip/rockchip.rst              |  2 +-
>>   7 files changed, 79 insertions(+), 3 deletions(-)
>>   create mode 100644 arch/arm/dts/rk3588-rock-5b-plus-u-boot.dtsi
>>
>> diff --git a/arch/arm/dts/rk3588-rock-5b-plus-u-boot.dtsi b/arch/arm/dts/rk3588-rock-5b-plus-u-boot.dtsi
>> new file mode 100644
>> index 000000000000..c07696c83913
>> --- /dev/null
>> +++ b/arch/arm/dts/rk3588-rock-5b-plus-u-boot.dtsi
>> @@ -0,0 +1,3 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +
>> +#include "rk3588-rock-5b-u-boot.dtsi"
>> diff --git a/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi b/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>> index d51fbf51cb88..e07b549c767f 100644
>> --- a/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>> +++ b/arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>> @@ -46,6 +46,11 @@
>>   	};
>>   };
>>   
>> +&saradc {
>> +	bootph-pre-ram;
>> +	vdd-microvolts = <1800000>;
>> +};
>> +
>>   &sdhci {
>>   	cap-mmc-highspeed;
>>   	mmc-hs200-1_8v;
>> diff --git a/board/radxa/rock5b-rk3588/Kconfig b/board/radxa/rock5b-rk3588/Kconfig
>> index 41dfe2402b12..98d630117836 100644
>> --- a/board/radxa/rock5b-rk3588/Kconfig
>> +++ b/board/radxa/rock5b-rk3588/Kconfig
>> @@ -9,4 +9,9 @@ config SYS_VENDOR
>>   config SYS_CONFIG_NAME
>>   	default "rock5b-rk3588"
>>   
>> +config BOARD_SPECIFIC_OPTIONS # dummy
>> +	def_bool y
>> +	select ADC
>> +	select SPL_ADC
>> +
>>   endif
>> diff --git a/board/radxa/rock5b-rk3588/MAINTAINERS b/board/radxa/rock5b-rk3588/MAINTAINERS
>> index 4460c9971a96..c8a43769105e 100644
>> --- a/board/radxa/rock5b-rk3588/MAINTAINERS
>> +++ b/board/radxa/rock5b-rk3588/MAINTAINERS
>> @@ -5,5 +5,4 @@ S:	Maintained
>>   F:	board/radxa/rock5b-rk3588
>>   F:	include/configs/rock5b-rk3588.h
>>   F:	configs/rock5b-rk3588_defconfig
>> -F:	arch/arm/dts/rk3588-rock-5b.dts
>> -F:	arch/arm/dts/rk3588-rock-5b-u-boot.dtsi
>> +F:	arch/arm/dts/rk3588-rock-5b*
>> diff --git a/board/radxa/rock5b-rk3588/rock5b-rk3588.c b/board/radxa/rock5b-rk3588/rock5b-rk3588.c
>> index fc2f69db2241..6bf4497ce3ae 100644
>> --- a/board/radxa/rock5b-rk3588/rock5b-rk3588.c
>> +++ b/board/radxa/rock5b-rk3588/rock5b-rk3588.c
>> @@ -3,8 +3,71 @@
>>    * Copyright (c) 2023-2024 Collabora Ltd.
>>    */
>>   
>> +#include <adc.h>
>> +#include <env.h>
>>   #include <fdtdec.h>
>>   #include <fdt_support.h>
>> +#include <asm/arch-rockchip/sdram.h>
>> +#include <linux/errno.h>
>> +
>> +#define PMU1GRF_BASE		0xfd58a000
>> +#define OS_REG2_REG		0x208
>> +
> 
> I assume the register address and meaning is stable across all RK3588, 
> so maybe we should abstract that somewhere in the SoC file 
> (arch/arm/mach-rockchip/rk3588) so that the caller doesn't need to 
> specify them?

Correct, these are stable for the SoC and could be defined in soc
related header and is something I have very slowly been working on in my
optimize branch at [1] for a future series.

Right now we have some code using syscon_get_first_range() to get a base
addr from DT, and newer code that instead use a define. See e.g.
sdram_rk3588.c vs sdram_rk3576.c, getting the base from DT adds several
ms delay and something that seem pointless when we know the stable addr
at compile time.

So for now I just kept this here with the expectation that this should
be changed in a future series.

[1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commit/3a11ab7fba8db133374e60edf8ab2b46195cf684

> 
>> +#define HW_ID_CHANNEL		5
>> +
>> +struct board_model {
>> +	unsigned int dram;
>> +	unsigned int low;
>> +	unsigned int high;
>> +	const char *fdtfile;
>> +};
>> +
>> +static const struct board_model board_models[] = {
>> +	{ LPDDR5, 4005, 4185, "rockchip/rk3588-rock-5b-plus.dtb" },
>> +};
>> +
>> +static const struct board_model *get_board_model(void)
>> +{
>> +	unsigned int val, dram_type;
>> +	int i, ret;
> 
> i could be an unsigned number type as well. (and it should since we're 
> using it to access items in an array)

Sure, will change in v3.

> 
>> +
>> +	dram_type = rockchip_sdram_type(PMU1GRF_BASE + OS_REG2_REG);
>> +
>> +	ret = adc_channel_single_shot("adc at fec10000", HW_ID_CHANNEL, &val);
>> +	if (ret)
>> +		return NULL;
>> +
> 
> While checking whether adc_channel_single_shot() is the right function 
> to call, I stumbled upon the call in arch/arm/mach-rockchip/boot_mode.c 
> which i believe wouldn't work for Rockchip SoCs whose DTSI uses adc@ as 
> node name instead of saradc at . That would be rk3328, rk3588, rk3576, 
> rk3528 and rv1126.

Correct, the saradc@ in boot_mode.c is typically used for "recovery" or
"maskrom" detection. However, this uses a hardcoded saradc channel that
may not necessarily be used for that purpose, especially on newer SoCs.

I suggest we do not try to change that to cover more SoCs without first
adding a way to define what channel to use for the button detection,
possible in a /config or a Kconfig option.

> 
> I guess there are different ways to fix this. We could have each SoC 
> define a constant string with the node name of the SARADC or have an 
> additional entry in /aliases for saradc in the DT? Maybe some other way 
> could be used as well, just throwing ideas :)
> 
> Unrelated to this patch though, so feel free to ignore.

As noted above, I think it is something to fix/consider but it may have
unintended consequences to blindly try and fix boot_mode.c for all RK
SoCs without having some sort of board option to specify when and how
the feature should be used.

Regards,
Jonas

> 
> Cheers,
> Quentin