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 < mkorpersh...@baylibre.com> wrote: > 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 >
