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

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 <[email protected]>
> ---
> 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