[PATCH 1/2] cmd: bcb: support various block device interfaces for BCB command
Mattijs Korpershoek
mkorpershoek at baylibre.com
Thu Nov 9 11:06:12 CET 2023
On jeu., nov. 09, 2023 at 10:40, Mattijs Korpershoek <mkorpershoek at 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 at 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 at google.com>
>> Cc: Eugeniu Rosca <erosca at de.adit-jv.com>
>> Cc: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> Cc: Sean Anderson <sean.anderson at seco.com>
>> Cc: Cody Schuffelen <schuffelen at 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
More information about the U-Boot
mailing list