Thank you Mattijs for taking time to verify it, uploaded v2 with "mcc" thing fixed
On Thu, Nov 9, 2023 at 2:06 AM Mattijs Korpershoek < mkorpershoek@baylibre.com> wrote:
On jeu., nov. 09, 2023 at 10:40, Mattijs Korpershoek < mkorpershoek@baylibre.com> wrote:
Hi Dmitrii,
Thank you for your patch.
Thank you as well for taking the time to upstream things from: https://android.googlesource.com/platform/external/u-boot/
On jeu., nov. 09, 2023 at 00:36, Dmitrii Merkurev dimorinny@google.com
wrote:
Currently BCB command-line, C APIs and implementation only support MMC interface. Extend it to allow various block device interfaces.
Signed-off-by: Dmitrii Merkurev dimorinny@google.com Cc: Eugeniu Rosca erosca@de.adit-jv.com Cc: Ying-Chun Liu (PaulLiu) paul.liu@linaro.org Cc: Simon Glass sjg@chromium.org Cc: Mattijs Korpershoek mkorpershoek@baylibre.com Cc: Sean Anderson sean.anderson@seco.com Cc: Cody Schuffelen schuffelen@google.com
This breaks vim3/vim3l android boot flow because both rely on the usage of the bcb command in their U-Boot environment:
Error: 1572575691 110528528: read failed (-19) Warning: BCB is corrupted or does not exist
The following diff fixes it:
diff --git a/include/configs/meson64_android.h
b/include/configs/meson64_android.h
index c0e977abb01f..442233e827d8 100644 --- a/include/configs/meson64_android.h +++ b/include/configs/meson64_android.h @@ -164,7 +164,7 @@ "fi; " \ "fi;" \ "if test "${run_fastboot}" -eq 0; then " \
"if bcb load "__stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
"if bcb load mmc "__stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
CONTROL_PARTITION "; then " \ "if bcb test command =bootonce-bootloader; then " \
"echo BCB: Bootloader boot...; "\
@@ -195,7 +195,7 @@ "echo Recovery button is pressed;" \ "setenv run_recovery 1;" \ "fi; " \
"if bcb load "__stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
"if bcb load mmc "__stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
CONTROL_PARTITION "; then " \ "if bcb test command = boot-recovery; then " \ "echo BCB: Recovery boot...; " \diff --git a/include/configs/ti_omap5_common.h
b/include/configs/ti_omap5_common.h
index 4e5aa74147d4..f571744adaf8 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -168,7 +168,7 @@ "mmc dev $mmcdev; " \ "mmc rescan; " \ AB_SELECT_SLOT \
"if bcb load "__stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
"if bcb load mmc "__stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
CONTROL_PARTITION "; then " \ "setenv ardaddr -; " \ "if bcb test command = bootonce-bootloader; then" \
Can you consider including this as part of this patch ?
cmd/Kconfig | 1 - cmd/bcb.c | 70 ++++++++++++++++++++++-------------- doc/android/bcb.rst | 34 +++++++++--------- drivers/fastboot/fb_common.c | 2 +- include/bcb.h | 5 +-- 5 files changed, 65 insertions(+), 47 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index df6d71c103..ce2435bbb8 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -981,7 +981,6 @@ config CMD_ADC
config CMD_BCB bool "bcb"
- depends on MMC depends on PARTITIONS help Read/modify/write the fields of Bootloader Control Block, usually
diff --git a/cmd/bcb.c b/cmd/bcb.c index 02d0c70d87..5d8c232663 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -25,6 +25,7 @@ enum bcb_cmd { BCB_CMD_STORE, };
+static enum uclass_id bcb_uclass_id = UCLASS_INVALID; static int bcb_dev = -1; static int bcb_part = -1; static struct bootloader_message bcb __aligned(ARCH_DMA_MINALIGN) = {
{ 0 } };
@@ -53,6 +54,9 @@ static int bcb_is_misused(int argc, char *const
argv[])
switch (cmd) { case BCB_CMD_LOAD:
if (argc != 3 && argc != 4)goto err;break;@@ -115,7 +119,7 @@ static int bcb_field_get(char *name, char **fieldp,
int *sizep)
return 0;}
-static int __bcb_load(int devnum, const char *partp) +static int __bcb_load(const char *iface, int devnum, const char *partp) { struct blk_desc *desc; struct disk_partition info; @@ -123,14 +127,14 @@ static int __bcb_load(int devnum, const char
*partp)
char *endp; int part, ret;
- desc = blk_get_devnum_by_uclass_id(UCLASS_MMC, devnum);
desc = blk_get_dev(iface, devnum); if (!desc) { ret = -ENODEV; goto err_read_fail; }
/*
* always select the USER mmc hwpart in case another
* always select the first hwpart in case another * blk operation selected a different hwpart */@@ -161,18 +165,20 @@ static int __bcb_load(int devnum, const char
*partp)
goto err_read_fail; }
- bcb_uclass_id = desc->uclass_id; bcb_dev = desc->devnum; bcb_part = part;
- debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part);
- debug("%s: Loaded from %s %d:%d\n", __func__, iface, bcb_dev,
bcb_part);
return CMD_RET_SUCCESS;err_read_fail:
- printf("Error: mmc %d:%s read failed (%d)\n", devnum, partp, ret);
- printf("Error: %s %d:%s read failed (%d)\n", iface, devnum, partp,
ret);
goto err;err_too_small:
- printf("Error: mmc %d:%s too small!", devnum, partp);
- printf("Error: %s %d:%s too small!", iface, devnum, partp); goto err;
err:
- bcb_uclass_id = UCLASS_INVALID; bcb_dev = -1; bcb_part = -1;
@@ -182,15 +188,23 @@ err: static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) {
- int devnum; char *endp;
- int devnum = simple_strtoul(argv[1], &endp, 0);
- char *iface = "mcc";
Should this be mmc instead of mcc ?
Just to clarify: having "mmc" instead of "mcc" also fixes the bcb reading on vim3.
if (argc == 4) {
iface = argv[1];argc--;argv++;}
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]);
- return __bcb_load(iface, devnum, argv[2]);
}
static int __bcb_set(char *fieldp, const char *valp) @@ -298,7 +312,7 @@ static int __bcb_store(void) u64 cnt; int ret;
- desc = blk_get_devnum_by_uclass_id(UCLASS_MMC, bcb_dev);
- desc = blk_get_devnum_by_uclass_id(bcb_uclass_id, bcb_dev); if (!desc) { ret = -ENODEV; goto err;
@@ -317,7 +331,7 @@ static int __bcb_store(void)
return CMD_RET_SUCCESS;err:
- printf("Error: mmc %d:%d write failed (%d)\n", bcb_dev, bcb_part,
ret);
- printf("Error: %d %d:%d write failed (%d)\n", bcb_uclass_id,
bcb_dev, bcb_part, ret);
return CMD_RET_FAILURE;} @@ -328,11 +342,11 @@ 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, const char
*reasonp)
+int bcb_write_reboot_reason(const char *iface, int devnum, char
*partp, const char *reasonp)
{ int ret;
- ret = __bcb_load(devnum, partp);
- ret = __bcb_load(iface, devnum, partp); if (ret != CMD_RET_SUCCESS) return ret;
@@ -385,21 +399,23 @@ static int do_bcb(struct cmd_tbl *cmdtp, int
flag, int argc, char *const argv[])
U_BOOT_CMD( bcb, CONFIG_SYS_MAXARGS, 1, do_bcb, "Load/set/clear/test/dump/store Android BCB fields",
- "load <dev> <part> - load BCB from mmc <dev>:<part>\n"
- "bcb set <field> <val> - set BCB <field> to <val>\n"
- "bcb clear [<field>] - clear BCB <field> or all fields\n"
- "bcb test <field> <op> <val> - test BCB <field> against <val>\n"
- "bcb dump <field> - dump BCB <field>\n"
- "bcb store - store BCB back to mmc\n"
- "load <interface> <dev> <part> - load BCB from <interface>
<dev>:<part>\n"
- "load <dev> <part> - load BCB from mmc
<dev>:<part>\n"
- "bcb set <field> <val> - set BCB <field> to <val>\n"
- "bcb clear [<field>] - clear BCB <field> or all
fields\n"
- "bcb test <field> <op> <val> - test BCB <field> against
<val>\n"
- "bcb dump <field> - dump BCB <field>\n"
- "bcb store - store BCB back to <interface>\n" "\n" "Legend:\n"
- "<dev> - MMC device index containing the BCB partition\n"
- "<part> - MMC partition index or name containing the BCB\n"
- "<field> - one of {command,status,recovery,stage,reserved}\n"
- "<op> - the binary operator used in 'bcb test':\n"
- " '=' returns true if <val> matches the string stored in
<field>\n"
- " '~' returns true if <val> matches a subset of <field>'s
string\n"
- "<val> - string/text provided as input to bcb {set,test}\n"
- " NOTE: any ':' character in <val> will be replaced by
line feed\n"
- " during 'bcb set' and used as separator by upper
layers\n"
- "<interface> - storage device interface (virtio, mmc, etc)\n"
- "<dev> - storage device index containing the BCB partition\n"
- "<part> - partition index or name containing the BCB\n"
- "<field> - one of {command,status,recovery,stage,reserved}\n"
- "<op> - the binary operator used in 'bcb test':\n"
- " '=' returns true if <val> matches the string stored
in <field>\n"
- " '~' returns true if <val> matches a subset of
<field>'s string\n"
- "<val> - string/text provided as input to bcb {set,test}\n"
- " NOTE: any ':' character in <val> will be replaced
by line feed\n"
- " during 'bcb set' and used as separator by upper
layers\n"
); diff --git a/doc/android/bcb.rst b/doc/android/bcb.rst index 8861608300..2226517d39 100644 --- a/doc/android/bcb.rst +++ b/doc/android/bcb.rst @@ -41,23 +41,25 @@ requirements enumerated above. Below is the
command's help message::
bcb - Load/set/clear/test/dump/store Android BCB fields Usage:
- bcb load <dev> <part> - load BCB from mmc <dev>:<part>
- bcb set <field> <val> - set BCB <field> to <val>
- bcb clear [<field>] - clear BCB <field> or all fields
- bcb test <field> <op> <val> - test BCB <field> against <val>
- bcb dump <field> - dump BCB <field>
- bcb store - store BCB back to mmc
- bcb load <interface> <dev> <part> - load BCB from <interface>
<dev>:<part>
load <dev> <part> - load BCB from mmc <dev>:<part>
bcb set <field> <val> - set BCB <field> to <val>
bcb clear [<field>] - clear BCB <field> or all fields
bcb test <field> <op> <val> - test BCB <field> against <val>
bcb dump <field> - dump BCB <field>
bcb store - store BCB back to <interface>
Legend:
- <dev> - MMC device index containing the BCB partition
- <part> - MMC partition index or name containing the BCB
- <field> - one of {command,status,recovery,stage,reserved}
- <op> - the binary operator used in 'bcb test':
'=' returns true if <val> matches the string stored in<field> >> - '~' returns true if <val> matches a subset of <field>'s string >> - <val> - string/text provided as input to bcb {set,test} >> - NOTE: any ':' character in <val> will be replaced by line feed >> - during 'bcb set' and used as separator by upper layers >> + <interface> - storage device interface (virtio, mmc, etc) >> + <dev> - storage device index containing the BCB partition >> + <part> - partition index or name containing the BCB >> + <field> - one of {command,status,recovery,stage,reserved} >> + <op> - the binary operator used in 'bcb test': >> + '=' returns true if <val> matches the string stored in <field> >> + '~' returns true if <val> matches a subset of <field>'s string >> + <val> - string/text provided as input to bcb {set,test} >> + NOTE: any ':' character in <val> will be replaced by line feed >> + during 'bcb set' and used as separator by upper layers >> >> >> 'bcb'. Example of getting reboot reason >> @@ -91,7 +93,7 @@ The following Kconfig options must be enabled:: >> >> CONFIG_PARTITIONS=y >> CONFIG_MMC=y >> - CONFIG_BCB=y >> + CONFIG_CMD_BCB=y >> >> .. [1] https://android.googlesource.com/platform/bootable/recovery >> .. [2] https://source.android.com/devices/bootloader >> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c >> index 4e9d9b719c..2a6608b28c 100644 >> --- a/drivers/fastboot/fb_common.c >> +++ b/drivers/fastboot/fb_common.c >> @@ -105,7 +105,7 @@ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) >> if (reason >= FASTBOOT_REBOOT_REASONS_COUNT) >> return -EINVAL; >> >> - return bcb_write_reboot_reason(mmc_dev, "misc", boot_cmds[reason]); >> + return bcb_write_reboot_reason("mmc", mmc_dev, "misc", boot_cmds[reason]); >> } >> >> /** >> diff --git a/include/bcb.h b/include/bcb.h >> index 5edb17aa47..a6326523c4 100644 >> --- a/include/bcb.h >> +++ b/include/bcb.h >> @@ -9,10 +9,11 @@ >> #define __BCB_H__ >> >> #if IS_ENABLED(CONFIG_CMD_BCB) >> -int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp); >> +int bcb_write_reboot_reason(const char *iface, int devnum, char *partp, const char *reasonp); >> #else >> #include <linux/errno.h> >> -static inline int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) >> +static inline int bcb_write_reboot_reason(const char *iface, int devnum, >> + char *partp, const char *reasonp) >> { >> return -EOPNOTSUPP; >> } >> -- >> 2.42.0.869.gea05f2083d-goog