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