Re: [PATCH 4/8] cmd: reset: dispatch named reset modes via the reboot-mode framework

Hi Balaji,

On 2026-08-11T05:18:35, Balaji Selvanathan
<[email protected]> wrote:
> cmd: reset: dispatch named reset modes via the reboot-mode framework
>
> Teach the "reset" command to trigger the reset modes registered with the
> reboot-mode framework in addition to the existing cold and warm resets:
>
>   reset          cold reset (unchanged)
>   reset -w       warm reset (unchanged)
>   reset -<mode>  reset into a mode declared in the device tree
>   reset -l       list the available reset modes
>
> For example "reset -edl" enters Qualcomm EDL/download mode when the psci
> device tree node carries a "reboot-mode" subnode with
> "mode-edl = <0x80000000 0x00000001>". The command never parses the magic
> values itself; it strips the leading '-' and hands the mode name to
> reboot_mode_request(), so the values stay in the device tree and are not
> hardcoded per SoC. An unknown mode prints the registered modes rather than
> silently falling through to a cold reset.

I'm going to argue that '-edl' is a bit strange. It looks like three
separate flags (-e, -d, -l) and may end up being confusing.

Perhaps we should allocate a proper flag for download mode, e.g. -d ?

Otherwise, I suggest using a flag to specify the mode, e.g. '-m <mode>'

>
> Signed-off-by: Balaji Selvanathan <[email protected]>
>
> cmd/boot.c                         |  6 +++++-
>  drivers/sysreset/sysreset-uclass.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 2 deletions(-)

> diff --git a/cmd/boot.c b/cmd/boot.c
> @@ -60,7 +60,11 @@ U_BOOT_CMD(
>       reset, 2, 0,    do_reset,
>       "Perform RESET of the CPU",
>       "- cold boot without level specifier\n"
> -     "reset -w - warm reset if implemented"
> +     "reset -w - warm reset if implemented\n"
> +#if IS_ENABLED(CONFIG_DM_REBOOT_MODE)
> +     "reset -<mode> - reset into a mode declared in the device tree\n"
> +     "reset -l - list the available reset modes\n"
> +#endif
>  );

The help text is gated on CONFIG_DM_REBOOT_MODE, but the dispatch code
below only exists under CONFIG_SYSRESET_CMD_RESET. Boards that use an
arch do_reset() (e.g. arch/arm/lib/reset.c) will advertise 'reset
-<mode>' in help but silently cold-reset when it is used. Either gate
the help on both, or move the dispatch into a common location (perhaps
a new do_reset() wrapper in cmd/boot.c that calls into the reboot-mode
helpers before falling through to the arch/sysreset implementation).

> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
> @@ -13,6 +13,7 @@
>  #include <hang.h>
>  #include <log.h>
>  #include <regmap.h>
> +#include <reboot-mode/reboot-mode.h>
>  #include <spl.h>

We normally put subdirs at the end of the sorting order.

> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
> @@ -125,8 +126,32 @@ int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> -     if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') {
> +     if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w' &&
> +         !argv[1][2]) {
>               reset_type = SYSRESET_WARM;
> +     } else if (CONFIG_IS_ENABLED(DM_REBOOT_MODE) && argc == 2 &&
> +                argv[1][0] == '-') {

Please use IS_ENABLED() to match the help text in cmd/boot.c -
CONFIG_IS_ENABLED() picks up the SPL/TPL variant and there's no reason
for that in a command handler.

> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
> @@ -125,8 +126,32 @@ int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +             printf("resetting into \"%s\" mode ...\n", name);
> +             mdelay(100);
> +
> +             /* Does not return on success. */
> +             reboot_mode_request(name);
> +
> +             printf("Unknown reset mode \"%s\"\n", name);
> +             reboot_mode_list();
> +             return CMD_RET_USAGE;

Two things. First, please look the mode up before printing 'resetting
into ...' and delaying - as written, an unknown mode causes a bogus
"resetting into" line, a 100ms pause and then the error, which reads
oddly on the console. Second, an unknown mode is a runtime failure,
not a usage error, so CMD_RET_FAILURE is a better fit; CMD_RET_USAGE
will splat the full command help on top of the list you just printed.

Please add a blank line before the final return in functions

> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
> @@ -125,8 +126,32 @@ int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +             if (!strcmp(name, "l"))
> +                     return reboot_mode_list() ? CMD_RET_FAILURE :
> +                                                 CMD_RET_SUCCESS;

'-l' collides with any mode literally named 'l'. Very unlikely in
practice, but worth a comment explaining that '-l' is reserved, and
perhaps the mode-name parser in patch 1 should reject a name of 'l' or
a name starting with '-' so the collision is caught at bind time
rather than at 'reset -l'.

Regards,
Simon