| Message ID | 20240109115310.1917139-1-caleb.connolly@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] boot: add support for button commands | expand |
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 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 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!
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