On jeu., nov. 09, 2023 at 10:40, Mattijs Korpershoek <mkorpersh...@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 <dimori...@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 <dimori...@google.com> >> Cc: Eugeniu Rosca <ero...@de.adit-jv.com> >> Cc: Ying-Chun Liu (PaulLiu) <paul....@linaro.org> >> Cc: Simon Glass <s...@chromium.org> >> Cc: Mattijs Korpershoek <mkorpersh...@baylibre.com> >> Cc: Sean Anderson <sean.ander...@seco.com> >> Cc: Cody Schuffelen <schuffe...@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; >> case BCB_CMD_FIELD_SET: >> if (argc != 3) >> goto err; >> @@ -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 >> */ >> ret = blk_dselect_hwpart(desc, 0); >> @@ -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
