Re：Re: [PATCH v3] arm: rockchip: spl: Add hotkey detection support.

[刘垣辰 <valentinliu@icloud.com>]

[-- Attachment #1: Type: text/plain, Size: 5965 bytes --]

Hi Quentin, Thanks for your reviewing and suggestions. I think the config name shouldn't be changed to " SPL_ROCKCHIP_ENTER_MASKROM_ON_KEY ". Because this function is aim to provide a way to enter the MaskROM, Loader and more mode (supported by Rockchip chips directly) in SPL by pressing hotkey (like Ctrl+B, Ctrl+D) in serial. This function is dependent on a working serial port and not dependent a physical button. About merging code to rockchip_dnl_mode_check() and same places: The SPL hotkey support is aim to provide a way to enter MaskROM and more mode without ADC keys support. That's meaning we didn't need a physical button in board to enter MaskROM. About delay 100ms: I have tested this function on my Mekotronics R58X-4G board, this is the shortest time that the hotkey can be detected. (Even though, I still need to press Ctrl+B before SPL running) Sorry for my poor English, I'm trying my best to describe the function of this config in Kconfig. The other suggestions have been accepted in v4 patch. Like checking CONFIG_ROCKCHIP_BOOT_MODE_REG, adding a small comment. etc. Best regards, Valentin Liu 2026年8月20日下午11:38，Quentin Schulz <u-boot@0leil.net> 写道： Hi Valentin, On 8/18/26 6:55 PM, Valentin Liu wrote: Add a configurable Rockchip SPL hotkey feature that checks the serial console during SPL startup. Ctrl+B can be used to enter BootROM download (MASKROM) mode and be widely used. We can add more boot mode support in future. Add CONFIG_SPL_ROCKCHIP_HOTKEY to enable the feature and wait for the serial port to be ready to receive input before checking for hotkeys. Signed-off-by: Valentin Liu <valentinliu@icloud.com> --- Changes for v2: - Simplify the dependencies of SPL_ROCKCHIP_HOTKEY. - Remove the conditions for the newly added includes. --- Changes for v3: - Add a dummy spl_hotkey_init() to avoid undefined reference errors when building without CONFIG_SPL_ROCKCHIP_HOTKEY. arch/arm/mach-rockchip/Kconfig | 13 ++++++++++ arch/arm/mach-rockchip/spl.c | 44 ++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig index 1a2e7847c9e..f2d2b5520ef 100644 --- a/arch/arm/mach-rockchip/Kconfig +++ b/arch/arm/mach-rockchip/Kconfig @@ -743,6 +743,19 @@ config TPL_ROCKCHIP_EARLYRETURN_TO_BROM config SPL_MMC default y if !SPL_ROCKCHIP_BACK_TO_BROM +config SPL_ROCKCHIP_HOTKEY + bool "SPL hotkey support" The symbol name and prompt is not clear enough on what it does. config SPL_ROCKCHIP_ENTER_MASKROM_ON_KEY bool "Enter MaskROM on key press during SPL" maybe? + depends on SPL_DM_RESET && SPL_SERIAL + help + Enable hotkey detection during SPL booting stage. + + When enabled, SPL checks the serial console for a control + character and can execute Rockchip-specific hotkey actions, + such as entering BootROM download mode (MASKROM) with Ctrl+B. + + The hotkey is checked after the SPL console has been + initialized. + Simplify to: """ When enabled, the SPL will check whether Ctrl+B is pressed and enter MaskROM in that case. """ config ROCKCHIP_SPI_IMAGE bool "Build a SPI image for rockchip" help diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index e989c148079..0bcfb42c306 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -13,11 +13,14 @@ #include <log.h> #include <mapmem.h> #include <ram.h> +#include <serial.h> #include <spl.h> +#include <asm/arch-rockchip/boot_mode.h> #include <asm/arch-rockchip/bootrom.h> #include <asm/arch-rockchip/timer.h> #include <asm/global_data.h> #include <asm/io.h> +#include <linux/delay.h> #include <linux/bitops.h> DECLARE_GLOBAL_DATA_PTR; @@ -107,6 +110,44 @@ __weak int arch_cpu_init(void) return 0; } +#if IS_ENABLED(CONFIG_SPL_ROCKCHIP_HOTKEY) Please use CONFIG_IS_ENABLED() instead. +static void rockchip_reset_from_hotkey(const int code) +{ + switch (code) { + case 0x02: Please add a small comment after 0x02: to specify which key combination triggers this code. E.g.: case 0x02: /* Ctrl+B */ + printf("SPL Hotkey: Ctrl+B: BootROM download!\n"); Please be consistent with what we have in arch/arm/mach-rockchip/boot_mode.c, that is: "Ctrl+B pressed, entering download mode..." I don't like it, as it's typically called MaskROM, but it's something we can fix later on and I prefer being consistent with what we currently have. + writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG); We *really* shouldn't be doing this if CONFIG_ROCKCHIP_BOOT_MODE_REG is 0 (the case for most boards). + do_reset(NULL, 0, 0, NULL); + /*NOTREACHED*/ + default: + if (code <= 0x1a) /* 'z' */ + printf("SPL Hotkey: Ctrl+%c\n", code + 'A' - 1); + else + printf("SPL Hotkey: Unknown code: 0x%x, ignore\n", code);> + }> +} + +static void spl_hotkey_init(void) +{ + if (!gd || !(gd->flags & GD_FLG_HAVE_CONSOLE)) + return; + if (gd->flags & GD_FLG_DISABLE_CONSOLE) + return; + + /* Wait for the serial port to be ready to receive data. */ + mdelay(100); + Is it not ready by the time we call this function? How did you come up with 100ms? + if (serial_tstc()) + rockchip_reset_from_hotkey(serial_getc()); + else + printf("SPL Hotkey: No key pressed, continue\n"); We don't need to print on the standard path. If you reaaaaaaally want to have something, then use log_debug/debug instead so it isn't printed by default except if you build with debug logging enabled. +} +#else +static void spl_hotkey_init(void) +{ +} +#endif + void board_init_f(ulong dummy) { int ret; @@ -143,6 +184,9 @@ void board_init_f(ulong dummy) } #endif preloader_console_init(); + + if (IS_ENABLED(CONFIG_SPL_ROCKCHIP_HOTKEY)) + spl_hotkey_init(); Can we merge with the very similar logic we have for an ADC button in arch/arm/mach-rockchip/boot_mode.c instead? I believe it makes more sense to have everything capable of entering MaskROM mode in the same place, with the same logic. I could see an else if() block in rockchip_dnl_mode_check() for example. Cheers, Quentin

[-- Attachment #2.1: Type: text/html, Size: 11338 bytes --]