[PATCH v2] boot: add support for button commands
With the relatively new button API in U-Boot, it's now much easier to model the common usecase of mapping arbitrary actions to different buttons during boot - for example entering fastboot mode, setting some additional kernel cmdline arguments, or booting with a custom recovery ramdisk, to name a few.
Historically, this functionality has been implemented in board code, making it fixed for a given U-Boot binary and requiring the code be duplicated and modified for every board.
Implement a generic abstraction to run an arbitrary command during boot when a specific button is pressed. The button -> command mapping is configured via environment variables with the following format:
button_cmd_N_name=<button label> button_cmd_N=<command to run>
Where N is the mapping number starting from 0. For example:
button_cmd_0_name=vol_down button_cmd_0=fastboot usb 0
This will cause the device to enter fastboot mode if volume down is held during boot.
After we enter the cli loop the button commands are no longer valid, this allows the buttons to additionally be used for navigating a boot menu.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # Tegra30 Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- Changes since v1: * make get_button_cmd() static. * use #if IS_ENABLED(CONFIG_BUTTON_CMD) instead of #ifdef CONFIG_BUTTON_CMD. * Kconfig fixes --- boot/Kconfig | 15 +++++++ common/Makefile | 1 + common/button_cmd.c | 83 +++++++++++++++++++++++++++++++++++++++ common/main.c | 3 ++ doc/usage/environment.rst | 4 ++ include/button.h | 9 +++++ 6 files changed, 115 insertions(+) create mode 100644 common/button_cmd.c
diff --git a/boot/Kconfig b/boot/Kconfig index fbc49c5bca47..882835731ea9 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -20,6 +20,21 @@ config TIMESTAMP loaded that does not, the message 'Wrong FIT format: no timestamp' is shown.
+config BUTTON_CMD + bool "Support for running a command if a button is held during boot" + depends on CMDLINE + depends on BUTTON + help + For many embedded devices it's useful to enter a special flashing mode + such as fastboot mode when a button is held during boot. This option + allows arbitrary commands to be assigned to specific buttons. These will + be run after "preboot" if the button is held. Configuration is done via + the environment variables "button_cmd_N_name" and "button_cmd_N" where n is + the button number (starting from 0). e.g: + + "button_cmd_0_name=vol_down" + "button_cmd_0=fastboot usb 0" + menuconfig FIT bool "Flattened Image Tree (FIT)" select HASH diff --git a/common/Makefile b/common/Makefile index cdeadf72026c..53105a6ce12a 100644 --- a/common/Makefile +++ b/common/Makefile @@ -10,6 +10,7 @@ obj-y += main.o obj-y += exports.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o +obj-$(CONFIG_BUTTON_CMD) += button_cmd.o
# # boards obj-y += board_f.o diff --git a/common/button_cmd.c b/common/button_cmd.c new file mode 100644 index 000000000000..b6a8434d6f29 --- /dev/null +++ b/common/button_cmd.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2023 Linaro Ltd. + * Author: Caleb Connolly caleb.connolly@linaro.org + */ + +#include <button.h> +#include <command.h> +#include <env.h> +#include <log.h> +#include <vsprintf.h> + +/* Some sane limit "just in case" */ +#define MAX_BTN_CMDS 32 + +struct button_cmd { + bool pressed; + const char *btn_name; + const char *cmd; +}; + +/* + * Button commands are set via environment variables, e.g.: + * button_cmd_N_name=Volume Up + * button_cmd_N=fastboot usb 0 + * + * This function will retrieve the command for the given button N + * and populate the cmd struct with the command string and pressed + * state of the button. + * + * Returns 1 if a command was found, 0 otherwise. + */ +static int get_button_cmd(int n, struct button_cmd *cmd) +{ + const char *cmd_str; + struct udevice *btn; + char buf[24]; + + snprintf(buf, sizeof(buf), "button_cmd_%d_name", n); + cmd->btn_name = env_get(buf); + if (!cmd->btn_name) + return 0; + + button_get_by_label(cmd->btn_name, &btn); + if (!btn) { + log_err("No button labelled '%s'\n", cmd->btn_name); + return 0; + } + + cmd->pressed = button_get_state(btn) == BUTTON_ON; + /* If the button isn't pressed then cmd->cmd will be unused so don't waste + * cycles reading it + */ + if (!cmd->pressed) + return 1; + + snprintf(buf, sizeof(buf), "button_cmd_%d", n); + cmd_str = env_get(buf); + if (!cmd_str) { + log_err("No command set for button '%s'\n", cmd->btn_name); + return 0; + } + + cmd->cmd = cmd_str; + + return 1; +} + +void process_button_cmds(void) +{ + struct button_cmd cmd = {0}; + int i = 0; + + while (get_button_cmd(i++, &cmd) && i < MAX_BTN_CMDS) { + if (!cmd.pressed) + continue; + + log_info("BTN '%s'> %s\n", cmd.btn_name, cmd.cmd); + run_command(cmd.cmd, CMD_FLAG_ENV); + /* Don't run commands for multiple buttons */ + return; + } +} diff --git a/common/main.c b/common/main.c index 7c70de2e59a8..717e8b7e8bd2 100644 --- a/common/main.c +++ b/common/main.c @@ -8,6 +8,7 @@
#include <common.h> #include <autoboot.h> +#include <button.h> #include <bootstage.h> #include <cli.h> #include <command.h> @@ -61,6 +62,8 @@ void main_loop(void) efi_launch_capsules(); }
+ process_button_cmds(); + s = bootdelay_process(); if (cli_process_fdt(&s)) cli_secure_boot_cmd(s); diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst index c57b717caaf3..ce5a9627f025 100644 --- a/doc/usage/environment.rst +++ b/doc/usage/environment.rst @@ -190,6 +190,10 @@ bootm_size bootstopkeysha256, bootdelaykey, bootstopkey See README.autoboot
+button_cmd_0, button_cmd_0_name ... button_cmd_N, button_cmd_N_name + Used to map commands to run when a button is held during boot. + See CONFIG_BUTTON_CMD. + updatefile Location of the software update file on a TFTP server, used by the automatic software update feature. Please refer to diff --git a/include/button.h b/include/button.h index 207f4a0f4dbd..8d38e521324d 100644 --- a/include/button.h +++ b/include/button.h @@ -74,4 +74,13 @@ enum button_state_t button_get_state(struct udevice *dev); */ int button_get_code(struct udevice *dev);
+#if IS_ENABLED(CONFIG_BUTTON_CMD) +/* Process button command mappings specified in the environment, + * running the commands for buttons which are pressed + */ +void process_button_cmds(void); +#else +static inline void process_button_cmds(void) {} +#endif /* CONFIG_BUTTON_CMD */ + #endif
On 2024-01-09 12:51, Caleb Connolly wrote:
With the relatively new button API in U-Boot, it's now much easier to model the common usecase of mapping arbitrary actions to different buttons during boot - for example entering fastboot mode, setting some additional kernel cmdline arguments, or booting with a custom recovery ramdisk, to name a few.
Historically, this functionality has been implemented in board code, making it fixed for a given U-Boot binary and requiring the code be duplicated and modified for every board.
Implement a generic abstraction to run an arbitrary command during boot when a specific button is pressed. The button -> command mapping is configured via environment variables with the following format:
button_cmd_N_name=<button label> button_cmd_N=<command to run>
Where N is the mapping number starting from 0. For example:
button_cmd_0_name=vol_down button_cmd_0=fastboot usb 0
This will cause the device to enter fastboot mode if volume down is held during boot.
This is simply awesome, but I see one possible issue -- the need to have proper environment variables defined for a particular board or device, to make the buttons work as expected. Obviously, those environment variables can be absent or can become missing for numerous reasons.
I think that we should have an additional mechanism in place that defines the buttons and the associated commands even if no environment variables are found. Like a set of fallback defaults for a particular board or device, built into the U-Boot image. For example, Rockchip boards have those defaults pretty well defined.
After we enter the cli loop the button commands are no longer valid, this allows the buttons to additionally be used for navigating a boot menu.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # Tegra30 Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
Changes since v1:
- make get_button_cmd() static.
- use #if IS_ENABLED(CONFIG_BUTTON_CMD) instead of #ifdef CONFIG_BUTTON_CMD.
- Kconfig fixes
boot/Kconfig | 15 +++++++ common/Makefile | 1 + common/button_cmd.c | 83 +++++++++++++++++++++++++++++++++++++++ common/main.c | 3 ++ doc/usage/environment.rst | 4 ++ include/button.h | 9 +++++ 6 files changed, 115 insertions(+) create mode 100644 common/button_cmd.c
diff --git a/boot/Kconfig b/boot/Kconfig index fbc49c5bca47..882835731ea9 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -20,6 +20,21 @@ config TIMESTAMP loaded that does not, the message 'Wrong FIT format: no timestamp' is shown.
+config BUTTON_CMD
- bool "Support for running a command if a button is held during boot"
- depends on CMDLINE
- depends on BUTTON
- help
For many embedded devices it's useful to enter a special flashingmode
such as fastboot mode when a button is held during boot. Thisoption
allows arbitrary commands to be assigned to specific buttons. Thesewill
be run after "preboot" if the button is held. Configuration is donevia
the environment variables "button_cmd_N_name" and "button_cmd_N"where n is
the button number (starting from 0). e.g:"button_cmd_0_name=vol_down""button_cmd_0=fastboot usb 0"menuconfig FIT bool "Flattened Image Tree (FIT)" select HASH diff --git a/common/Makefile b/common/Makefile index cdeadf72026c..53105a6ce12a 100644 --- a/common/Makefile +++ b/common/Makefile @@ -10,6 +10,7 @@ obj-y += main.o obj-y += exports.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o +obj-$(CONFIG_BUTTON_CMD) += button_cmd.o
# # boards obj-y += board_f.o diff --git a/common/button_cmd.c b/common/button_cmd.c new file mode 100644 index 000000000000..b6a8434d6f29 --- /dev/null +++ b/common/button_cmd.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (c) 2023 Linaro Ltd.
- Author: Caleb Connolly caleb.connolly@linaro.org
- */
+#include <button.h> +#include <command.h> +#include <env.h> +#include <log.h> +#include <vsprintf.h>
+/* Some sane limit "just in case" */ +#define MAX_BTN_CMDS 32
+struct button_cmd {
- bool pressed;
- const char *btn_name;
- const char *cmd;
+};
+/*
- Button commands are set via environment variables, e.g.:
- button_cmd_N_name=Volume Up
- button_cmd_N=fastboot usb 0
- This function will retrieve the command for the given button N
- and populate the cmd struct with the command string and pressed
- state of the button.
- Returns 1 if a command was found, 0 otherwise.
- */
+static int get_button_cmd(int n, struct button_cmd *cmd) +{
- const char *cmd_str;
- struct udevice *btn;
- char buf[24];
- snprintf(buf, sizeof(buf), "button_cmd_%d_name", n);
- cmd->btn_name = env_get(buf);
- if (!cmd->btn_name)
return 0;- button_get_by_label(cmd->btn_name, &btn);
- if (!btn) {
log_err("No button labelled '%s'\n", cmd->btn_name);return 0;- }
- cmd->pressed = button_get_state(btn) == BUTTON_ON;
- /* If the button isn't pressed then cmd->cmd will be unused so don't
waste
* cycles reading it*/- if (!cmd->pressed)
return 1;- snprintf(buf, sizeof(buf), "button_cmd_%d", n);
- cmd_str = env_get(buf);
- if (!cmd_str) {
log_err("No command set for button '%s'\n", cmd->btn_name);return 0;- }
- cmd->cmd = cmd_str;
- return 1;
+}
+void process_button_cmds(void) +{
- struct button_cmd cmd = {0};
- int i = 0;
- while (get_button_cmd(i++, &cmd) && i < MAX_BTN_CMDS) {
if (!cmd.pressed)continue;log_info("BTN '%s'> %s\n", cmd.btn_name, cmd.cmd);run_command(cmd.cmd, CMD_FLAG_ENV);/* Don't run commands for multiple buttons */return;- }
+} diff --git a/common/main.c b/common/main.c index 7c70de2e59a8..717e8b7e8bd2 100644 --- a/common/main.c +++ b/common/main.c @@ -8,6 +8,7 @@
#include <common.h> #include <autoboot.h> +#include <button.h> #include <bootstage.h> #include <cli.h> #include <command.h> @@ -61,6 +62,8 @@ void main_loop(void) efi_launch_capsules(); }
- process_button_cmds();
- s = bootdelay_process(); if (cli_process_fdt(&s)) cli_secure_boot_cmd(s);
diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst index c57b717caaf3..ce5a9627f025 100644 --- a/doc/usage/environment.rst +++ b/doc/usage/environment.rst @@ -190,6 +190,10 @@ bootm_size bootstopkeysha256, bootdelaykey, bootstopkey See README.autoboot
+button_cmd_0, button_cmd_0_name ... button_cmd_N, button_cmd_N_name
- Used to map commands to run when a button is held during boot.
- See CONFIG_BUTTON_CMD.
updatefile Location of the software update file on a TFTP server, used by the automatic software update feature. Please refer to diff --git a/include/button.h b/include/button.h index 207f4a0f4dbd..8d38e521324d 100644 --- a/include/button.h +++ b/include/button.h @@ -74,4 +74,13 @@ enum button_state_t button_get_state(struct udevice *dev); */ int button_get_code(struct udevice *dev);
+#if IS_ENABLED(CONFIG_BUTTON_CMD) +/* Process button command mappings specified in the environment,
- running the commands for buttons which are pressed
- */
+void process_button_cmds(void); +#else +static inline void process_button_cmds(void) {} +#endif /* CONFIG_BUTTON_CMD */
#endif
On 10/01/2024 18:08, Dragan Simic wrote:
On 2024-01-09 12:51, Caleb Connolly wrote:
With the relatively new button API in U-Boot, it's now much easier to model the common usecase of mapping arbitrary actions to different buttons during boot - for example entering fastboot mode, setting some additional kernel cmdline arguments, or booting with a custom recovery ramdisk, to name a few.
Historically, this functionality has been implemented in board code, making it fixed for a given U-Boot binary and requiring the code be duplicated and modified for every board.
Implement a generic abstraction to run an arbitrary command during boot when a specific button is pressed. The button -> command mapping is configured via environment variables with the following format:
button_cmd_N_name=<button label> button_cmd_N=<command to run>
Where N is the mapping number starting from 0. For example:
button_cmd_0_name=vol_down button_cmd_0=fastboot usb 0
This will cause the device to enter fastboot mode if volume down is held during boot.
This is simply awesome, but I see one possible issue -- the need to have proper environment variables defined for a particular board or device, to make the buttons work as expected. Obviously, those environment variables can be absent or can become missing for numerous reasons.
Is CFG_EXTRA_ENV_SETTINGS not persistent enough?
I think that we should have an additional mechanism in place that defines the buttons and the associated commands even if no environment variables are found. Like a set of fallback defaults for a particular board or device, built into the U-Boot image. For example, Rockchip boards have those defaults pretty well defined.
A programmatic API for register button/cmd mapping from board_late_init() (for example) sounds sensible to me.
Is this really an issue that invalidates the implementation proposed here though? It feels much more like a nice-to-have addition that maybe we could leave out for now?
It also has a MUCH wider scope imo - should board override env or vice versa? What about triggering default AND custom actions for one button press? What if a board wants to register a callback function instead of running a command?) - these are questions I don't want to answer with this patch.
After we enter the cli loop the button commands are no longer valid, this allows the buttons to additionally be used for navigating a boot menu.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # Tegra30 Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
Changes since v1: * make get_button_cmd() static. * use #if IS_ENABLED(CONFIG_BUTTON_CMD) instead of #ifdef CONFIG_BUTTON_CMD. * Kconfig fixes
boot/Kconfig | 15 +++++++ common/Makefile | 1 + common/button_cmd.c | 83 +++++++++++++++++++++++++++++++++++++++ common/main.c | 3 ++ doc/usage/environment.rst | 4 ++ include/button.h | 9 +++++ 6 files changed, 115 insertions(+) create mode 100644 common/button_cmd.c
diff --git a/boot/Kconfig b/boot/Kconfig index fbc49c5bca47..882835731ea9 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -20,6 +20,21 @@ config TIMESTAMP loaded that does not, the message 'Wrong FIT format: no timestamp' is shown.
+config BUTTON_CMD + bool "Support for running a command if a button is held during boot" + depends on CMDLINE + depends on BUTTON + help + For many embedded devices it's useful to enter a special flashing mode + such as fastboot mode when a button is held during boot. This option + allows arbitrary commands to be assigned to specific buttons. These will + be run after "preboot" if the button is held. Configuration is done via + the environment variables "button_cmd_N_name" and "button_cmd_N" where n is + the button number (starting from 0). e.g:
+ "button_cmd_0_name=vol_down" + "button_cmd_0=fastboot usb 0"
menuconfig FIT bool "Flattened Image Tree (FIT)" select HASH diff --git a/common/Makefile b/common/Makefile index cdeadf72026c..53105a6ce12a 100644 --- a/common/Makefile +++ b/common/Makefile @@ -10,6 +10,7 @@ obj-y += main.o obj-y += exports.o obj-$(CONFIG_HUSH_PARSER) += cli_hush.o obj-$(CONFIG_AUTOBOOT) += autoboot.o +obj-$(CONFIG_BUTTON_CMD) += button_cmd.o
# # boards obj-y += board_f.o diff --git a/common/button_cmd.c b/common/button_cmd.c new file mode 100644 index 000000000000..b6a8434d6f29 --- /dev/null +++ b/common/button_cmd.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (c) 2023 Linaro Ltd.
- * Author: Caleb Connolly caleb.connolly@linaro.org
- */
+#include <button.h> +#include <command.h> +#include <env.h> +#include <log.h> +#include <vsprintf.h>
+/* Some sane limit "just in case" */ +#define MAX_BTN_CMDS 32
+struct button_cmd { + bool pressed; + const char *btn_name; + const char *cmd; +};
+/*
- Button commands are set via environment variables, e.g.:
- button_cmd_N_name=Volume Up
- button_cmd_N=fastboot usb 0
- This function will retrieve the command for the given button N
- and populate the cmd struct with the command string and pressed
- state of the button.
- Returns 1 if a command was found, 0 otherwise.
- */
+static int get_button_cmd(int n, struct button_cmd *cmd) +{ + const char *cmd_str; + struct udevice *btn; + char buf[24];
+ snprintf(buf, sizeof(buf), "button_cmd_%d_name", n); + cmd->btn_name = env_get(buf); + if (!cmd->btn_name) + return 0;
+ button_get_by_label(cmd->btn_name, &btn); + if (!btn) { + log_err("No button labelled '%s'\n", cmd->btn_name); + return 0; + }
+ cmd->pressed = button_get_state(btn) == BUTTON_ON; + /* If the button isn't pressed then cmd->cmd will be unused so don't waste + * cycles reading it + */ + if (!cmd->pressed) + return 1;
+ snprintf(buf, sizeof(buf), "button_cmd_%d", n); + cmd_str = env_get(buf); + if (!cmd_str) { + log_err("No command set for button '%s'\n", cmd->btn_name); + return 0; + }
+ cmd->cmd = cmd_str;
+ return 1; +}
+void process_button_cmds(void) +{ + struct button_cmd cmd = {0}; + int i = 0;
+ while (get_button_cmd(i++, &cmd) && i < MAX_BTN_CMDS) { + if (!cmd.pressed) + continue;
+ log_info("BTN '%s'> %s\n", cmd.btn_name, cmd.cmd); + run_command(cmd.cmd, CMD_FLAG_ENV); + /* Don't run commands for multiple buttons */ + return; + } +} diff --git a/common/main.c b/common/main.c index 7c70de2e59a8..717e8b7e8bd2 100644 --- a/common/main.c +++ b/common/main.c @@ -8,6 +8,7 @@
#include <common.h> #include <autoboot.h> +#include <button.h> #include <bootstage.h> #include <cli.h> #include <command.h> @@ -61,6 +62,8 @@ void main_loop(void) efi_launch_capsules(); }
+ process_button_cmds();
s = bootdelay_process(); if (cli_process_fdt(&s)) cli_secure_boot_cmd(s); diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst index c57b717caaf3..ce5a9627f025 100644 --- a/doc/usage/environment.rst +++ b/doc/usage/environment.rst @@ -190,6 +190,10 @@ bootm_size bootstopkeysha256, bootdelaykey, bootstopkey See README.autoboot
+button_cmd_0, button_cmd_0_name ... button_cmd_N, button_cmd_N_name + Used to map commands to run when a button is held during boot. + See CONFIG_BUTTON_CMD.
updatefile Location of the software update file on a TFTP server, used by the automatic software update feature. Please refer to diff --git a/include/button.h b/include/button.h index 207f4a0f4dbd..8d38e521324d 100644 --- a/include/button.h +++ b/include/button.h @@ -74,4 +74,13 @@ enum button_state_t button_get_state(struct udevice *dev); */ int button_get_code(struct udevice *dev);
+#if IS_ENABLED(CONFIG_BUTTON_CMD) +/* Process button command mappings specified in the environment,
- running the commands for buttons which are pressed
- */
+void process_button_cmds(void); +#else +static inline void process_button_cmds(void) {} +#endif /* CONFIG_BUTTON_CMD */
#endif
This is simply awesome, but I see one possible issue -- the need to have proper environment variables defined for a particular board or device, to make the buttons work as expected. Obviously, those environment variables can be absent or can become missing for numerous reasons.
Is CFG_EXTRA_ENV_SETTINGS not persistent enough?
IMHO no. Because a user might accidentially mess up the environment variables.
I think that we should have an additional mechanism in place that defines the buttons and the associated commands even if no environment variables are found. Like a set of fallback defaults for a particular board or device, built into the U-Boot image. For example, Rockchip boards have those defaults pretty well defined.
A programmatic API for register button/cmd mapping from board_late_init() (for example) sounds sensible to me.
I don't know if it has to be that complex, or if it will be enough to just have some compile-time constants like CONFIG_BUTTON_CMD_N.
Is this really an issue that invalidates the implementation proposed here though? It feels much more like a nice-to-have addition that maybe we could leave out for now?
Agreed. Looks like one can add it to get_button_cmd() later.
It also has a MUCH wider scope imo - should board override env or vice versa? What about triggering default AND custom actions for one button press? What if a board wants to register a callback function instead of running a command?) - these are questions I don't want to answer with this patch.
One use case I have is restoring the default environment *in any case*; regardless what the state of the board is. In this case, the environment must not override the button settings. This can be a compiled-in command, which always takes precedence.
If you want to be able to overwrite a button command, then I guess you can already do that with the environment setting. Provide sane defaults via CFG_EXTRA_ENV_SETTINGS and a user can then overwrite it.
In summary, the registered (compiled-in) command should always take precedence. If one wants to supply a default command which can be changed later, that can go via the (compiled-in) default environment.
-michael
Hello Caleb and Michael,
On 2024-01-11 10:38, Michael Walle wrote:
This is simply awesome, but I see one possible issue -- the need to have proper environment variables defined for a particular board or device, to make the buttons work as expected. Obviously, those environment variables can be absent or can become missing for numerous reasons.
Is CFG_EXTRA_ENV_SETTINGS not persistent enough?
IMHO no. Because a user might accidentially mess up the environment variables.
I apologize for my delayed response.
Using CONFIG_EXTRA_ENV_SETTINGS should be good enough to provide the fallback defaults. However, the users can still mess the things up, but again, they can do that already in many places.
I think that we should have an additional mechanism in place that defines the buttons and the associated commands even if no environment variables are found. Like a set of fallback defaults for a particular board or device, built into the U-Boot image. For example, Rockchip boards have those defaults pretty well defined.
A programmatic API for register button/cmd mapping from board_late_init() (for example) sounds sensible to me.
I don't know if it has to be that complex, or if it will be enough to just have some compile-time constants like CONFIG_BUTTON_CMD_N.
Having compile-time constants is also a viable option, IMHO. We should just need some fallback mechanism, and the simpler the better.
Is this really an issue that invalidates the implementation proposed here though? It feels much more like a nice-to-have addition that maybe we could leave out for now?
Agreed. Looks like one can add it to get_button_cmd() later.
I also agree, but it should be added eventually.
It also has a MUCH wider scope imo - should board override env or vice versa? What about triggering default AND custom actions for one button press? What if a board wants to register a callback function instead of running a command?) - these are questions I don't want to answer with this patch.
Yes, all this opens quite a few questions. IMHO, we should simply have some fallback mechanism, for which using CONFIG_EXTRA_ENV_SETTINGS seems fine to me, and leave everything else to as it already is. Sure, the users could mess it up, but that can already happen in many places.
One use case I have is restoring the default environment *in any case*; regardless what the state of the board is. In this case, the environment must not override the button settings. This can be a compiled-in command, which always takes precedence.
To me, having an option to restore the default environment isn't a bad idea, but not automatically and always. Perhaps some other mechanism that triggers the restoration should be devised.
If you want to be able to overwrite a button command, then I guess you can already do that with the environment setting. Provide sane defaults via CFG_EXTRA_ENV_SETTINGS and a user can then overwrite it.
I agree.
In summary, the registered (compiled-in) command should always take precedence. If one wants to supply a default command which can be changed later, that can go via the (compiled-in) default environment.
Sorry, this is a bit confusing to me. Didn't you write above that the users should be able to change the associated commands through the environment variables?
Hi,
Using CONFIG_EXTRA_ENV_SETTINGS should be good enough to provide the fallback defaults. However, the users can still mess the things up, but again, they can do that already in many places.
I disagree. In my case that is a last resort recovery. And it should work in any case. Even if the user has messed up anything (except from erasing the bootloader in the SPI flash ;)).
In summary, the registered (compiled-in) command should always take precedence. If one wants to supply a default command which can be changed later, that can go via the (compiled-in) default environment.
Sorry, this is a bit confusing to me. Didn't you write above that the users should be able to change the associated commands through the environment variables?
I had two kinds of button commands in mind: immutable ones and mutable ones. The first can be achieved with compiled-in commands, the second with a default environment and environment variables.
Also, whether a command is a mutable one or not is the decision of the developer (or the one who's compiling/configuring u-boot), not the user.
-michael
On 2024-01-18 08:59, Michael Walle wrote:
Using CONFIG_EXTRA_ENV_SETTINGS should be good enough to provide the fallback defaults. However, the users can still mess the things up, but again, they can do that already in many places.
I disagree. In my case that is a last resort recovery. And it should work in any case. Even if the user has messed up anything (except from erasing the bootloader in the SPI flash ;)).
Maybe the solution could be another compile-time option to "lock down" the built-in defaults provided through CONFIG_EXTRA_ENV_SETTINGS? If that new option is selected, changes to the environment would make no changes to the built-in defaults, i.e. those parts of the environment would actually be ignored.
In summary, the registered (compiled-in) command should always take precedence. If one wants to supply a default command which can be changed later, that can go via the (compiled-in) default environment.
Sorry, this is a bit confusing to me. Didn't you write above that the users should be able to change the associated commands through the environment variables?
I had two kinds of button commands in mind: immutable ones and mutable ones. The first can be achieved with compiled-in commands, the second with a default environment and environment variables.
Also, whether a command is a mutable one or not is the decision of the developer (or the one who's compiling/configuring u-boot), not the user.
I believe that the additional compile-time option, which I proposed above, could be extended to specify which of the built-in default button-command associations are immutable, and which are allowed to be modified through the environment variables.
Using CONFIG_EXTRA_ENV_SETTINGS should be good enough to provide the fallback defaults. However, the users can still mess the things up, but again, they can do that already in many places.
I disagree. In my case that is a last resort recovery. And it should work in any case. Even if the user has messed up anything (except from erasing the bootloader in the SPI flash ;)).
Maybe the solution could be another compile-time option to "lock down" the built-in defaults provided through CONFIG_EXTRA_ENV_SETTINGS? If that new option is selected, changes to the environment would make no changes to the built-in defaults, i.e. those parts of the environment would actually be ignored.
Not sure locking down the whole environment is a good idea.
In summary, the registered (compiled-in) command should always take precedence. If one wants to supply a default command which can be changed later, that can go via the (compiled-in) default environment.
Sorry, this is a bit confusing to me. Didn't you write above that the users should be able to change the associated commands through the environment variables?
I had two kinds of button commands in mind: immutable ones and mutable ones. The first can be achieved with compiled-in commands, the second with a default environment and environment variables.
Also, whether a command is a mutable one or not is the decision of the developer (or the one who's compiling/configuring u-boot), not the user.
I believe that the additional compile-time option, which I proposed above, could be extended to specify which of the built-in default button-command associations are immutable, and which are allowed to be modified through the environment variables.
IIRC there is already a mechanism for that. Environment hooks or something like that. But I'm not sure that has other implications and qualify as simple and lightweight for this use-case.
Anyway, we digress. I just wanted to make you aware of another use-case, which btw. is already done today in the lsxl board for example.
-michael
On 2024-01-18 16:11, Michael Walle wrote:
Using CONFIG_EXTRA_ENV_SETTINGS should be good enough to provide the fallback defaults. However, the users can still mess the things up, but again, they can do that already in many places.
I disagree. In my case that is a last resort recovery. And it should work in any case. Even if the user has messed up anything (except from erasing the bootloader in the SPI flash ;)).
Maybe the solution could be another compile-time option to "lock down" the built-in defaults provided through CONFIG_EXTRA_ENV_SETTINGS? If that new option is selected, changes to the environment would make no changes to the built-in defaults, i.e. those parts of the environment would actually be ignored.
Not sure locking down the whole environment is a good idea.
Not the entire environment, just the default button-command associations supplied through CONFIG_EXTRA_ENV_SETTINGS. I'm sorry if I didn't write it clearly before.
In summary, the registered (compiled-in) command should always take precedence. If one wants to supply a default command which can be changed later, that can go via the (compiled-in) default environment.
Sorry, this is a bit confusing to me. Didn't you write above that the users should be able to change the associated commands through the environment variables?
I had two kinds of button commands in mind: immutable ones and mutable ones. The first can be achieved with compiled-in commands, the second with a default environment and environment variables.
Also, whether a command is a mutable one or not is the decision of the developer (or the one who's compiling/configuring u-boot), not the user.
I believe that the additional compile-time option, which I proposed above, could be extended to specify which of the built-in default button-command associations are immutable, and which are allowed to be modified through the environment variables.
IIRC there is already a mechanism for that. Environment hooks or something like that. But I'm not sure that has other implications and qualify as simple and lightweight for this use-case.
Anyway, we digress. I just wanted to make you aware of another use-case, which btw. is already done today in the lsxl board for example.
On Tue, Jan 09, 2024 at 11:51:09AM +0000, Caleb Connolly wrote:
With the relatively new button API in U-Boot, it's now much easier to model the common usecase of mapping arbitrary actions to different buttons during boot - for example entering fastboot mode, setting some additional kernel cmdline arguments, or booting with a custom recovery ramdisk, to name a few.
Historically, this functionality has been implemented in board code, making it fixed for a given U-Boot binary and requiring the code be duplicated and modified for every board.
Implement a generic abstraction to run an arbitrary command during boot when a specific button is pressed. The button -> command mapping is configured via environment variables with the following format:
button_cmd_N_name=<button label> button_cmd_N=<command to run>
Where N is the mapping number starting from 0. For example:
button_cmd_0_name=vol_down button_cmd_0=fastboot usb 0
This will cause the device to enter fastboot mode if volume down is held during boot.
After we enter the cli loop the button commands are no longer valid, this allows the buttons to additionally be used for navigating a boot menu.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # Tegra30 Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
Applied to u-boot/master, thanks!
participants (4)
-
Caleb Connolly -
Dragan Simic -
Michael Walle -
Tom Rini