[RESEND 0/7] Refactor generic fastboot_set_reboot_flag implementation
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
New function called bcb_set_reboot_reason should be exposed by BCB commands, so that all of the details could be put there instead of dealing with it in fastboot implementation directly. After that, fastboot_set_reboot_flag could simply pass correct reboot reason string to the BCB implementation. If CMD_BCB is disabled then the whole operation would return error code, which is no different behaviour than the current implementation.
Eugeniu Rosca (5): cmd: bcb: Extract '__bcb_load' from 'do_bcb_load' for internal needs cmd: bcb: Extract '__bcb_set' from 'do_bcb_set' for internal needs cmd: bcb: Extract '__bcb_store' from 'do_bcb_store' for internal needs cmd: bcb: Expose 'bcb_write_reboot_reason' to external callers cmd: bcb: Add support for processing const string literals in bcb_set()
Roman Kovalivskyi (2): Revert "fastboot: Add default fastboot_set_reboot_flag implementation" fastboot: Implement generic fastboot_set_reboot_flag
cmd/bcb.c | 88 +++++++++++++++++++++++++++------- drivers/fastboot/Kconfig | 12 ----- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ----------------- drivers/fastboot/fb_common.c | 12 ++++- include/bcb.h | 20 ++++++++ include/fastboot.h | 9 ---- 7 files changed, 101 insertions(+), 84 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c create mode 100644 include/bcb.h
From: Eugeniu Rosca erosca@de.adit-jv.com
Enriching the functionality of U-Boot 'bcb' may assume using the existing sub-commands as building blocks for the next ones.
A clean way to achive the above is to expose a number of static routines, each mapped to an existing user command (e.g. load/set/store), with a user/caller-friendly prototype (i.e. do not force the caller to wrap an integer into a string).
This first patch makes '__bcb_load' available for internal needs.
No functional change, except for a tiny update in error handling.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index e03218066bf2..2ed8b801a3e2 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -110,8 +110,7 @@ static int bcb_field_get(char *name, char **fieldp, int *sizep) return 0; }
-static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int __bcb_load(int devnum, const char *partp) { struct blk_desc *desc; struct disk_partition info; @@ -119,17 +118,19 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, char *endp; int part, ret;
- ret = blk_get_device_by_str("mmc", argv[1], &desc); - if (ret < 0) + desc = blk_get_devnum_by_type(IF_TYPE_MMC, devnum); + if (!desc) { + ret = -ENODEV; goto err_read_fail; + }
- part = simple_strtoul(argv[2], &endp, 0); + part = simple_strtoul(partp, &endp, 0); if (*endp == '\0') { ret = part_get_info(desc, part, &info); if (ret) goto err_read_fail; } else { - part = part_get_info_by_name(desc, argv[2], &info); + part = part_get_info_by_name(desc, partp, &info); if (part < 0) { ret = part; goto err_read_fail; @@ -151,10 +152,10 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc,
return CMD_RET_SUCCESS; err_read_fail: - printf("Error: mmc %s:%s read failed (%d)\n", argv[1], argv[2], ret); + printf("Error: mmc %d:%s read failed (%d)\n", devnum, partp, ret); goto err; err_too_small: - printf("Error: mmc %s:%s too small!", argv[1], argv[2]); + printf("Error: mmc %d:%s too small!", devnum, partp); goto err; err: bcb_dev = -1; @@ -163,6 +164,20 @@ err: return CMD_RET_FAILURE; }
+static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, + char * const argv[]) +{ + char *endp; + int devnum = simple_strtoul(argv[1], &endp, 0); + + if (*endp != '\0') { + printf("Error: Device id '%s' not a number\n", argv[1]); + return CMD_RET_FAILURE; + } + + return __bcb_load(devnum, argv[2]); +} + static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
From: Eugeniu Rosca erosca@de.adit-jv.com
Enriching the functionality of U-Boot 'bcb' may assume using the existing sub-commands as building blocks for the next ones.
A clean way to achive the above is to expose a number of static routines, each mapped to an existing user command (e.g. load/set/store), with a user/caller-friendly prototype (i.e. do not force the caller to wrap an integer into a string).
This second patch makes '__bcb_set' available for internal needs.
No functional change intended.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index 2ed8b801a3e2..113f04ffe6b2 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -178,22 +178,21 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_load(devnum, argv[2]); }
-static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int __bcb_set(char *fieldp, char *valp) { int size, len; char *field, *str, *found;
- if (bcb_field_get(argv[1], &field, &size)) + if (bcb_field_get(fieldp, &field, &size)) return CMD_RET_FAILURE;
- len = strlen(argv[2]); + len = strlen(valp); if (len >= size) { printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n", - argv[2], len, size, argv[1]); + valp, len, size, fieldp); return CMD_RET_FAILURE; } - str = argv[2]; + str = valp;
field[0] = '\0'; while ((found = strsep(&str, ":"))) { @@ -205,6 +204,12 @@ static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
+static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, + char * const argv[]) +{ + return __bcb_set(argv[1], argv[2]); +} + static int do_bcb_clear(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
From: Eugeniu Rosca erosca@de.adit-jv.com
Enriching the functionality of U-Boot 'bcb' may assume using the existing sub-commands as building blocks for the next ones.
A clean way to achive the above is to expose a number of static routines, each mapped to an existing user command (e.g. load/set/store), with a user/caller-friendly prototype (i.e. do not force the caller to wrap an integer into a string).
This third patch makes '__bcb_store' available for internal needs.
No functional change intended.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index 113f04ffe6b2..b9cd20ea3d56 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -270,8 +270,7 @@ static int do_bcb_dump(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
-static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int __bcb_store(void) { struct blk_desc *desc; struct disk_partition info; @@ -302,6 +301,12 @@ err: return CMD_RET_FAILURE; }
+static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, + char * const argv[]) +{ + return __bcb_store(); +} + static struct cmd_tbl cmd_bcb_sub[] = { U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""), U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),
From: Eugeniu Rosca erosca@de.adit-jv.com
Fastboot is evolving and beginning with commit [1], the upstream implementation expects bootloaders to offer support for: - reboot-recovery - reboot-fastboot
The most natural way to achieve the above is through a set of pre-defined "reboot reason" strings, written into / read from the BCB "command" field, e.g.: - bootonce-bootloader [2] - boot-fastboot [3] - boot-recovery [4]
Expose the first 'bcb' API meant to be called by e.g. fastboot stack, to allow updating the BCB reboot reason via the BCB 'command' field.
[1] https://android.googlesource.com/platform/system/core/+/dea91b4b5354af2 ("Add fastbootd.") [2] https://android.googlesource.com/platform/bootable/recovery/+/cba7fa88d8b9 ("Add 'reboot bootloader' to bootloader_message.") [3] https://android.googlesource.com/platform/bootable/recovery/+/eee4e260f9f6 ("recovery: Add "boot-fastboot" command to BCB.") [4] https://android.googlesource.com/platform/system/core/+/5e98b633a748695f ("init: Write the reason in BCB on "reboot recovery"")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 20 ++++++++++++++++++++ include/bcb.h | 20 ++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 include/bcb.h
diff --git a/cmd/bcb.c b/cmd/bcb.c index b9cd20ea3d56..5da3526142ad 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -6,6 +6,7 @@ */
#include <android_bootloader_message.h> +#include <bcb.h> #include <command.h> #include <common.h> #include <log.h> @@ -307,6 +308,25 @@ static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_store(); }
+int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +{ + int ret; + + ret = __bcb_load(devnum, partp); + if (ret != CMD_RET_SUCCESS) + return ret; + + ret = __bcb_set("command", reasonp); + if (ret != CMD_RET_SUCCESS) + return ret; + + ret = __bcb_store(); + if (ret != CMD_RET_SUCCESS) + return ret; + + return 0; +} + static struct cmd_tbl cmd_bcb_sub[] = { U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""), U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""), diff --git a/include/bcb.h b/include/bcb.h new file mode 100644 index 000000000000..9b662d1203a6 --- /dev/null +++ b/include/bcb.h @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2020 Eugeniu Rosca rosca.eugeniu@gmail.com + * + * Android Bootloader Control Block Header + */ + +#ifndef __BCB_H__ +#define __BCB_H__ + +#if CONFIG_IS_ENABLED(CMD_BCB) +int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp); +#else +static inline int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +{ + return -EOPNOTSUPP; +} +#endif + +#endif /* __BCB_H__ */
From: Eugeniu Rosca erosca@de.adit-jv.com
On request/suggestion from Simon Glass back in May 22 2019 [1], the 'strsep' mechanism implemented in bcb_set() was set to work directly with user-provided argv strings, to avoid duplicating memory and for the sake of simpler implementation.
However, since we recently exposed bcb_write_reboot_reason() API to be called by U-Boot fastboot, the idea is to be able to pass const string literals to this new BCB API, carrying the reboot reason.
Since 'strsep' (just like its older/superseded sibling 'strtok') modifies the input string passed as parameter, BCB command in its current state would attempt to perform in-place modifications in a readonly string, which might lead to unexpected results.
Fix the above with the cost of one dynamic memory allocation ('strdup'). This will also ensure no compiler warnings when passing string literals to bcb_write_reboot_reason().
[1] http://u-boot.10912.n7.nabble.com/PATCH-v2-0-2-Add-bcb-command-to-read-modif...
Cc: Simon Glass sjg@chromium.org Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 17 ++++++++++++----- include/bcb.h | 4 ++-- 2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index 5da3526142ad..6b6f1e9a2f10 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -11,6 +11,7 @@ #include <common.h> #include <log.h> #include <part.h> +#include <malloc.h>
enum bcb_cmd { BCB_CMD_LOAD, @@ -179,10 +180,10 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_load(devnum, argv[2]); }
-static int __bcb_set(char *fieldp, char *valp) +static int __bcb_set(char *fieldp, const char *valp) { int size, len; - char *field, *str, *found; + char *field, *str, *found, *tmp;
if (bcb_field_get(fieldp, &field, &size)) return CMD_RET_FAILURE; @@ -193,14 +194,20 @@ static int __bcb_set(char *fieldp, char *valp) valp, len, size, fieldp); return CMD_RET_FAILURE; } - str = valp; + str = strdup(valp); + if (!str) { + printf("Error: Out of memory while strdup\n"); + return CMD_RET_FAILURE; + }
+ tmp = str; field[0] = '\0'; - while ((found = strsep(&str, ":"))) { + while ((found = strsep(&tmp, ":"))) { if (field[0] != '\0') strcat(field, "\n"); strcat(field, found); } + free(str);
return CMD_RET_SUCCESS; } @@ -308,7 +315,7 @@ static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_store(); }
-int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) { int ret;
diff --git a/include/bcb.h b/include/bcb.h index 9b662d1203a6..9ef4928642ca 100644 --- a/include/bcb.h +++ b/include/bcb.h @@ -9,9 +9,9 @@ #define __BCB_H__
#if CONFIG_IS_ENABLED(CMD_BCB) -int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp); +int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp); #else -static inline int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +static inline int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) { return -EOPNOTSUPP; }
This reverts commit 0ebf9842e56c5b8cb7cb1f990bb452cc14af6225.
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
Next commit introduces new generic implementation of fastboot_set_reboot_flag.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- drivers/fastboot/Kconfig | 12 ---------- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ---------------------------------- include/fastboot.h | 9 ------- 4 files changed, 65 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig index 4352ba67a713..d4436dfc9173 100644 --- a/drivers/fastboot/Kconfig +++ b/drivers/fastboot/Kconfig @@ -165,18 +165,6 @@ config FASTBOOT_CMD_OEM_FORMAT relies on the env variable partitions to contain the list of partitions as required by the gpt command.
-config FASTBOOT_USE_BCB_SET_REBOOT_FLAG - bool "Use BCB by fastboot to set boot reason" - depends on CMD_BCB && !ARCH_MESON && !ARCH_ROCKCHIP && !TARGET_KC1 && \ - !TARGET_SNIPER && !TARGET_AM57XX_EVM && !TARGET_DRA7XX_EVM - default y - help - Fastboot could implement setting of reboot reason in a generic fashion - via BCB commands. BCB commands are able to write reboot reason into - command field of boot control block. In general case it is sufficient - implementation if your platform supports BCB commands and doesn't - require any specific reboot reason handling. - endif # FASTBOOT
endmenu diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile index 2b2c390fe4de..048af5aa8234 100644 --- a/drivers/fastboot/Makefile +++ b/drivers/fastboot/Makefile @@ -5,4 +5,3 @@ obj-y += fb_getvar.o obj-y += fb_command.o obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o -obj-$(CONFIG_FASTBOOT_USE_BCB_SET_REBOOT_FLAG) += fb_bcb_impl.o diff --git a/drivers/fastboot/fb_bcb_impl.c b/drivers/fastboot/fb_bcb_impl.c deleted file mode 100644 index 89ec3601b6f6..000000000000 --- a/drivers/fastboot/fb_bcb_impl.c +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * Copyright 2020 GlobalLogic. - * Roman Kovalivskyi roman.kovalivskyi@globallogic.com - */ - -#include <common.h> -#include <fastboot.h> - -/** - * fastboot_set_reboot_flag() - Set flag to indicate reboot-bootloader - * - * Set flag which indicates that we should reboot into the bootloader - * following the reboot that fastboot executes after this function. - * - * This function should be overridden in your board file with one - * which sets whatever flag your board specific Android bootloader flow - * requires in order to re-enter the bootloader. - */ -int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) -{ - char cmd[64]; - - if (reason >= FASTBOOT_REBOOT_REASONS_COUNT) - return -EINVAL; - - snprintf(cmd, sizeof(cmd), "bcb load %d misc", - CONFIG_FASTBOOT_FLASH_MMC_DEV); - - if (run_command(cmd, 0)) - return -ENODEV; - - snprintf(cmd, sizeof(cmd), "bcb set command %s", - fastboot_boot_cmds[reason]); - - if (run_command(cmd, 0)) - return -ENOEXEC; - - if (run_command("bcb store", 0)) - return -EIO; - - return 0; -} diff --git a/include/fastboot.h b/include/fastboot.h index 8e9ee80907df..b86b508e69fd 100644 --- a/include/fastboot.h +++ b/include/fastboot.h @@ -52,15 +52,6 @@ enum fastboot_reboot_reason { FASTBOOT_REBOOT_REASONS_COUNT };
-/** - * BCB boot commands - */ -static const char * const fastboot_boot_cmds[] = { - [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader", - [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot", - [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery" -}; - /** * fastboot_response() - Writes a response of the form "$tag$reason". *
Hi Roman,
This reverts commit 0ebf9842e56c5b8cb7cb1f990bb452cc14af6225.
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
Next commit introduces new generic implementation of fastboot_set_reboot_flag.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com
drivers/fastboot/Kconfig | 12 ---------- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ---------------------------------- include/fastboot.h | 9 ------- 4 files changed, 65 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig index 4352ba67a713..d4436dfc9173 100644 --- a/drivers/fastboot/Kconfig +++ b/drivers/fastboot/Kconfig @@ -165,18 +165,6 @@ config FASTBOOT_CMD_OEM_FORMAT relies on the env variable partitions to contain the list of partitions as required by the gpt command.
-config FASTBOOT_USE_BCB_SET_REBOOT_FLAG
- bool "Use BCB by fastboot to set boot reason"
- depends on CMD_BCB && !ARCH_MESON && !ARCH_ROCKCHIP &&
!TARGET_KC1 && \
!TARGET_SNIPER && !TARGET_AM57XX_EVM && !TARGET_DRA7XX_EVM- default y
- help
Fastboot could implement setting of reboot reason in ageneric fashion
via BCB commands. BCB commands are able to write rebootreason into
command field of boot control block. In general case it issufficient
implementation if your platform supports BCB commands anddoesn't
require any specific reboot reason handling.endif # FASTBOOT
endmenu diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile index 2b2c390fe4de..048af5aa8234 100644 --- a/drivers/fastboot/Makefile +++ b/drivers/fastboot/Makefile @@ -5,4 +5,3 @@ obj-y += fb_getvar.o obj-y += fb_command.o obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o -obj-$(CONFIG_FASTBOOT_USE_BCB_SET_REBOOT_FLAG) += fb_bcb_impl.o diff --git a/drivers/fastboot/fb_bcb_impl.c b/drivers/fastboot/fb_bcb_impl.c deleted file mode 100644 index 89ec3601b6f6..000000000000 --- a/drivers/fastboot/fb_bcb_impl.c +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/*
- Copyright 2020 GlobalLogic.
- Roman Kovalivskyi roman.kovalivskyi@globallogic.com
- */
-#include <common.h> -#include <fastboot.h>
-/**
- fastboot_set_reboot_flag() - Set flag to indicate
reboot-bootloader
- Set flag which indicates that we should reboot into the bootloader
- following the reboot that fastboot executes after this function.
- This function should be overridden in your board file with one
- which sets whatever flag your board specific Android bootloader
flow
- requires in order to re-enter the bootloader.
- */
-int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) -{
- char cmd[64];
- if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
return -EINVAL;- snprintf(cmd, sizeof(cmd), "bcb load %d misc",
CONFIG_FASTBOOT_FLASH_MMC_DEV);- if (run_command(cmd, 0))
return -ENODEV;- snprintf(cmd, sizeof(cmd), "bcb set command %s",
fastboot_boot_cmds[reason]);- if (run_command(cmd, 0))
return -ENOEXEC;- if (run_command("bcb store", 0))
return -EIO;- return 0;
-} diff --git a/include/fastboot.h b/include/fastboot.h index 8e9ee80907df..b86b508e69fd 100644 --- a/include/fastboot.h +++ b/include/fastboot.h @@ -52,15 +52,6 @@ enum fastboot_reboot_reason { FASTBOOT_REBOOT_REASONS_COUNT };
-/**
- BCB boot commands
- */
-static const char * const fastboot_boot_cmds[] = {
- [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader",
- [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot",
- [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery"
-};
/**
- fastboot_response() - Writes a response of the form "$tag$reason".
If this patch is still needed - please rebase it on newest master (after the incoming PR) as it causes build breaks.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz,
On 1/23/21 5:08 PM, Lukasz Majewski wrote:
Hi Roman,
This reverts commit 0ebf9842e56c5b8cb7cb1f990bb452cc14af6225.
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
Next commit introduces new generic implementation of fastboot_set_reboot_flag.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com
drivers/fastboot/Kconfig | 12 ---------- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ---------------------------------- include/fastboot.h | 9 ------- 4 files changed, 65 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig index 4352ba67a713..d4436dfc9173 100644 --- a/drivers/fastboot/Kconfig +++ b/drivers/fastboot/Kconfig @@ -165,18 +165,6 @@ config FASTBOOT_CMD_OEM_FORMAT relies on the env variable partitions to contain the list of partitions as required by the gpt command.
-config FASTBOOT_USE_BCB_SET_REBOOT_FLAG
- bool "Use BCB by fastboot to set boot reason"
- depends on CMD_BCB && !ARCH_MESON && !ARCH_ROCKCHIP &&
!TARGET_KC1 && \
!TARGET_SNIPER && !TARGET_AM57XX_EVM && !TARGET_DRA7XX_EVM- default y
- help
Fastboot could implement setting of reboot reason in ageneric fashion
via BCB commands. BCB commands are able to write rebootreason into
command field of boot control block. In general case it issufficient
implementation if your platform supports BCB commands anddoesn't
require any specific reboot reason handling.endif # FASTBOOT
endmenu
diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile index 2b2c390fe4de..048af5aa8234 100644 --- a/drivers/fastboot/Makefile +++ b/drivers/fastboot/Makefile @@ -5,4 +5,3 @@ obj-y += fb_getvar.o obj-y += fb_command.o obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o -obj-$(CONFIG_FASTBOOT_USE_BCB_SET_REBOOT_FLAG) += fb_bcb_impl.o diff --git a/drivers/fastboot/fb_bcb_impl.c b/drivers/fastboot/fb_bcb_impl.c deleted file mode 100644 index 89ec3601b6f6..000000000000 --- a/drivers/fastboot/fb_bcb_impl.c +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/*
- Copyright 2020 GlobalLogic.
- Roman Kovalivskyi roman.kovalivskyi@globallogic.com
- */
-#include <common.h> -#include <fastboot.h>
-/**
- fastboot_set_reboot_flag() - Set flag to indicate
reboot-bootloader
- Set flag which indicates that we should reboot into the bootloader
- following the reboot that fastboot executes after this function.
- This function should be overridden in your board file with one
- which sets whatever flag your board specific Android bootloader
flow
- requires in order to re-enter the bootloader.
- */
-int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) -{
- char cmd[64];
- if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
return -EINVAL;- snprintf(cmd, sizeof(cmd), "bcb load %d misc",
CONFIG_FASTBOOT_FLASH_MMC_DEV);- if (run_command(cmd, 0))
return -ENODEV;- snprintf(cmd, sizeof(cmd), "bcb set command %s",
fastboot_boot_cmds[reason]);- if (run_command(cmd, 0))
return -ENOEXEC;- if (run_command("bcb store", 0))
return -EIO;- return 0;
-} diff --git a/include/fastboot.h b/include/fastboot.h index 8e9ee80907df..b86b508e69fd 100644 --- a/include/fastboot.h +++ b/include/fastboot.h @@ -52,15 +52,6 @@ enum fastboot_reboot_reason { FASTBOOT_REBOOT_REASONS_COUNT };
-/**
- BCB boot commands
- */
-static const char * const fastboot_boot_cmds[] = {
- [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader",
- [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot",
- [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery"
-};
- /**
- fastboot_response() - Writes a response of the form "$tag$reason".
If this patch is still needed - please rebase it on newest master (after the incoming PR) as it causes build breaks.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Could you please share some info on how do you build it (used defconfig, error messages, used compiler, etc.)? I cannot reproduce those issues on my side.
I've tried to rebase my patches onto the current master and rebase was done cleanly, there are no conflicts. I've tried building it with rcar3_ulcb_defconfig and build was successful.
Used defconfig: rcar3_ulcb_defconfig Used arch: ARM Used compiler: gcc 7.1 for aarch64-linux-gnu Used CFLAGS: -fgnu89-inline
Best regards, Roman Kovalivskyi
On Mon, Jan 25, 2021 at 07:16:37PM +0200, Roman Kovalivskyi wrote:
Hi Lukasz,
On 1/23/21 5:08 PM, Lukasz Majewski wrote:
Hi Roman,
This reverts commit 0ebf9842e56c5b8cb7cb1f990bb452cc14af6225.
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
Next commit introduces new generic implementation of fastboot_set_reboot_flag.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com
drivers/fastboot/Kconfig | 12 ---------- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ---------------------------------- include/fastboot.h | 9 ------- 4 files changed, 65 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig index 4352ba67a713..d4436dfc9173 100644 --- a/drivers/fastboot/Kconfig +++ b/drivers/fastboot/Kconfig @@ -165,18 +165,6 @@ config FASTBOOT_CMD_OEM_FORMAT relies on the env variable partitions to contain the list of partitions as required by the gpt command. -config FASTBOOT_USE_BCB_SET_REBOOT_FLAG
- bool "Use BCB by fastboot to set boot reason"
- depends on CMD_BCB && !ARCH_MESON && !ARCH_ROCKCHIP &&
!TARGET_KC1 && \
!TARGET_SNIPER && !TARGET_AM57XX_EVM && !TARGET_DRA7XX_EVM- default y
- help
Fastboot could implement setting of reboot reason in ageneric fashion
via BCB commands. BCB commands are able to write rebootreason into
command field of boot control block. In general case it issufficient
implementation if your platform supports BCB commands anddoesn't
require any specific reboot reason handling.- endif # FASTBOOT endmenu
diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile index 2b2c390fe4de..048af5aa8234 100644 --- a/drivers/fastboot/Makefile +++ b/drivers/fastboot/Makefile @@ -5,4 +5,3 @@ obj-y += fb_getvar.o obj-y += fb_command.o obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o -obj-$(CONFIG_FASTBOOT_USE_BCB_SET_REBOOT_FLAG) += fb_bcb_impl.o diff --git a/drivers/fastboot/fb_bcb_impl.c b/drivers/fastboot/fb_bcb_impl.c deleted file mode 100644 index 89ec3601b6f6..000000000000 --- a/drivers/fastboot/fb_bcb_impl.c +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/*
- Copyright 2020 GlobalLogic.
- Roman Kovalivskyi roman.kovalivskyi@globallogic.com
- */
-#include <common.h> -#include <fastboot.h>
-/**
- fastboot_set_reboot_flag() - Set flag to indicate
reboot-bootloader
- Set flag which indicates that we should reboot into the bootloader
- following the reboot that fastboot executes after this function.
- This function should be overridden in your board file with one
- which sets whatever flag your board specific Android bootloader
flow
- requires in order to re-enter the bootloader.
- */
-int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) -{
- char cmd[64];
- if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
return -EINVAL;- snprintf(cmd, sizeof(cmd), "bcb load %d misc",
CONFIG_FASTBOOT_FLASH_MMC_DEV);- if (run_command(cmd, 0))
return -ENODEV;- snprintf(cmd, sizeof(cmd), "bcb set command %s",
fastboot_boot_cmds[reason]);- if (run_command(cmd, 0))
return -ENOEXEC;- if (run_command("bcb store", 0))
return -EIO;- return 0;
-} diff --git a/include/fastboot.h b/include/fastboot.h index 8e9ee80907df..b86b508e69fd 100644 --- a/include/fastboot.h +++ b/include/fastboot.h @@ -52,15 +52,6 @@ enum fastboot_reboot_reason { FASTBOOT_REBOOT_REASONS_COUNT }; -/**
- BCB boot commands
- */
-static const char * const fastboot_boot_cmds[] = {
- [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader",
- [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot",
- [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery"
-};
- /**
- fastboot_response() - Writes a response of the form "$tag$reason".
If this patch is still needed - please rebase it on newest master (after the incoming PR) as it causes build breaks.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Could you please share some info on how do you build it (used defconfig, error messages, used compiler, etc.)? I cannot reproduce those issues on my side.
I've tried to rebase my patches onto the current master and rebase was done cleanly, there are no conflicts. I've tried building it with rcar3_ulcb_defconfig and build was successful.
Used defconfig: rcar3_ulcb_defconfig Used arch: ARM Used compiler: gcc 7.1 for aarch64-linux-gnu Used CFLAGS: -fgnu89-inline
You likely want to submit a GitHub PR against https://github.com/u-boot/u-boot so that you can trigger a world build via Azure and see what fails to build from that.
On 1/25/21 6:16 PM, Roman Kovalivskyi wrote:
[...]
Could you please share some info on how do you build it (used defconfig, error messages, used compiler, etc.)? I cannot reproduce those issues on my side.
I've tried to rebase my patches onto the current master and rebase was done cleanly, there are no conflicts. I've tried building it with rcar3_ulcb_defconfig and build was successful.
Used defconfig: rcar3_ulcb_defconfig Used arch: ARM Used compiler: gcc 7.1 for aarch64-linux-gnu Used CFLAGS: -fgnu89-inline
Mainline U-Boot for rcar3 has no USB gadget support though ?
It is possible to implement fastboot_set_reboot_flag in a generic way if BCB commands are turned on for a target. Using bcb_set_reboot_reason allows to do this by simply passing string with correct reboot reason that should be handled during next boot process.
If BCB are turned off, then bcb_set_reboot_reason would simply return error, so it won't introduce any new behaviour for such targets.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- drivers/fastboot/fb_common.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c index 736ce1cd024f..005dccf3c967 100644 --- a/drivers/fastboot/fb_common.c +++ b/drivers/fastboot/fb_common.c @@ -10,6 +10,7 @@ * Rob Herring robh@kernel.org */
+#include <bcb.h> #include <common.h> #include <command.h> #include <env.h> @@ -90,7 +91,16 @@ void fastboot_okay(const char *reason, char *response) */ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) { - return -ENOSYS; + static const char * const boot_cmds[] = { + [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader", + [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot", + [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery" + }; + + if (reason >= FASTBOOT_REBOOT_REASONS_COUNT) + return -EINVAL; + + return bcb_write_reboot_reason(CONFIG_FASTBOOT_FLASH_MMC_DEV, "misc", boot_cmds[reason]); }
/**
Hi Roman,
It is possible to implement fastboot_set_reboot_flag in a generic way if BCB commands are turned on for a target. Using bcb_set_reboot_reason allows to do this by simply passing string with correct reboot reason that should be handled during next boot process.
If BCB are turned off, then bcb_set_reboot_reason would simply return error, so it won't introduce any new behaviour for such targets.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com
drivers/fastboot/fb_common.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c index 736ce1cd024f..005dccf3c967 100644 --- a/drivers/fastboot/fb_common.c +++ b/drivers/fastboot/fb_common.c @@ -10,6 +10,7 @@
- Rob Herring robh@kernel.org
*/
+#include <bcb.h> #include <common.h> #include <command.h> #include <env.h> @@ -90,7 +91,16 @@ void fastboot_okay(const char *reason, char *response) */ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) {
- return -ENOSYS;
- static const char * const boot_cmds[] = {
[FASTBOOT_REBOOT_REASON_BOOTLOADER] ="bootonce-bootloader",
[FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot",[FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery"- };
- if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
return -EINVAL;- return
bcb_write_reboot_reason(CONFIG_FASTBOOT_FLASH_MMC_DEV, "misc", boot_cmds[reason]); } /**
This patch causes build breaks when I run the CI on azzure. If it is still needed, please rebase it and resend.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Dear U-Boot maintainers,
On Fri, Oct 23, 2020 at 11:52:18AM +0300, Roman Kovalivskyi wrote:
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
New function called bcb_set_reboot_reason should be exposed by BCB commands, so that all of the details could be put there instead of dealing with it in fastboot implementation directly. After that, fastboot_set_reboot_flag could simply pass correct reboot reason string to the BCB implementation. If CMD_BCB is disabled then the whole operation would return error code, which is no different behaviour than the current implementation.
Eugeniu Rosca (5): cmd: bcb: Extract '__bcb_load' from 'do_bcb_load' for internal needs cmd: bcb: Extract '__bcb_set' from 'do_bcb_set' for internal needs cmd: bcb: Extract '__bcb_store' from 'do_bcb_store' for internal needs cmd: bcb: Expose 'bcb_write_reboot_reason' to external callers cmd: bcb: Add support for processing const string literals in bcb_set()
Roman Kovalivskyi (2): Revert "fastboot: Add default fastboot_set_reboot_flag implementation" fastboot: Implement generic fastboot_set_reboot_flag
cmd/bcb.c | 88 +++++++++++++++++++++++++++------- drivers/fastboot/Kconfig | 12 ----- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ----------------- drivers/fastboot/fb_common.c | 12 ++++- include/bcb.h | 20 ++++++++ include/fastboot.h | 9 ---- 7 files changed, 101 insertions(+), 84 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c create mode 100644 include/bcb.h
Any chance the patches will be applied?
On Thu, Dec 10, 2020 at 08:30:36AM +0100, Eugeniu Rosca wrote:
Dear U-Boot maintainers,
On Fri, Oct 23, 2020 at 11:52:18AM +0300, Roman Kovalivskyi wrote:
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
New function called bcb_set_reboot_reason should be exposed by BCB commands, so that all of the details could be put there instead of dealing with it in fastboot implementation directly. After that, fastboot_set_reboot_flag could simply pass correct reboot reason string to the BCB implementation. If CMD_BCB is disabled then the whole operation would return error code, which is no different behaviour than the current implementation.
Eugeniu Rosca (5): cmd: bcb: Extract '__bcb_load' from 'do_bcb_load' for internal needs cmd: bcb: Extract '__bcb_set' from 'do_bcb_set' for internal needs cmd: bcb: Extract '__bcb_store' from 'do_bcb_store' for internal needs cmd: bcb: Expose 'bcb_write_reboot_reason' to external callers cmd: bcb: Add support for processing const string literals in bcb_set()
Roman Kovalivskyi (2): Revert "fastboot: Add default fastboot_set_reboot_flag implementation" fastboot: Implement generic fastboot_set_reboot_flag
cmd/bcb.c | 88 +++++++++++++++++++++++++++------- drivers/fastboot/Kconfig | 12 ----- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ----------------- drivers/fastboot/fb_common.c | 12 ++++- include/bcb.h | 20 ++++++++ include/fastboot.h | 9 ---- 7 files changed, 101 insertions(+), 84 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c create mode 100644 include/bcb.h
Any chance the patches will be applied?
I'll try and review at least the generic fastboot changes soon and if there's no problems, move this in to -next. Thanks.
Dear Tom,
On Thu, Dec 10, 2020 at 11:54:44AM -0500, Tom Rini wrote:
On Thu, Dec 10, 2020 at 08:30:36AM +0100, Eugeniu Rosca wrote:
Dear U-Boot maintainers,
On Fri, Oct 23, 2020 at 11:52:18AM +0300, Roman Kovalivskyi wrote:
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
New function called bcb_set_reboot_reason should be exposed by BCB commands, so that all of the details could be put there instead of dealing with it in fastboot implementation directly. After that, fastboot_set_reboot_flag could simply pass correct reboot reason string to the BCB implementation. If CMD_BCB is disabled then the whole operation would return error code, which is no different behaviour than the current implementation.
Eugeniu Rosca (5): cmd: bcb: Extract '__bcb_load' from 'do_bcb_load' for internal needs cmd: bcb: Extract '__bcb_set' from 'do_bcb_set' for internal needs cmd: bcb: Extract '__bcb_store' from 'do_bcb_store' for internal needs cmd: bcb: Expose 'bcb_write_reboot_reason' to external callers cmd: bcb: Add support for processing const string literals in bcb_set()
Roman Kovalivskyi (2): Revert "fastboot: Add default fastboot_set_reboot_flag implementation" fastboot: Implement generic fastboot_set_reboot_flag
cmd/bcb.c | 88 +++++++++++++++++++++++++++------- drivers/fastboot/Kconfig | 12 ----- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ----------------- drivers/fastboot/fb_common.c | 12 ++++- include/bcb.h | 20 ++++++++ include/fastboot.h | 9 ---- 7 files changed, 101 insertions(+), 84 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c create mode 100644 include/bcb.h
Any chance the patches will be applied?
I'll try and review at least the generic fastboot changes soon and if there's no problems, move this in to -next. Thanks.
Any suggestions how to bring this series back on track? Thanks in advance.
participants (5)
-
Eugeniu Rosca -
Lukasz Majewski -
Marek Vasut -
Roman Kovalivskyi -
Tom Rini