Hi Simon, On Tue, Oct 15, 2024 at 09:27:14AM -0600, Simon Glass wrote: > Hi Dmitry, > > On Tue, 15 Oct 2024 at 08:42, Dmitry Rokosov > <ddroko...@salutedevices.com> wrote: > > > > Hi Mattijs, Simon, > > > > On Tue, Oct 15, 2024 at 02:10:10PM +0200, Mattijs Korpershoek wrote: > > > Hi Simon, Dmitry > > > > > > On lun., oct. 14, 2024 at 15:06, Simon Glass <s...@chromium.org> wrote: > > > > > > > Hi Dmitry, > > > > > > > > On Mon, 14 Oct 2024 at 14:38, Dmitry Rokosov > > > > <ddroko...@salutedevices.com> wrote: > > > >> > > > >> Hello Mattijs, > > > >> > > > >> On Sat, Oct 12, 2024 at 10:49:08AM +0200, Mattijs Korpershoek wrote: > > > >> > Hi Dmitry, > > > >> > > > > >> > On ven., oct. 11, 2024 at 21:00, Dmitry Rokosov > > > >> > <ddroko...@salutedevices.com> wrote: > > > >> > > > > >> > > On Fri, Oct 11, 2024 at 04:20:39PM +0200, Mattijs Korpershoek > > > >> > > wrote: > > > >> > >> On ven., oct. 11, 2024 at 15:30, "Mattijs Korpershoek via > > > >> > >> groups.io" <mkorpershoek=baylibre....@groups.io> wrote: > > > >> > >> > > > >> > >> > Hi Dmitry, > > > >> > >> > > > > >> > >> > Thank you for the patch. > > > >> > >> > > > > >> > >> > On mar., oct. 08, 2024 at 23:18, Dmitry Rokosov > > > >> > >> > <ddroko...@salutedevices.com> wrote: > > > >> > >> > > > > >> > >> >> To enhance code organization, it is beneficial to consolidate > > > >> > >> >> all A/B > > > >> > >> >> BCB management routines into a single super-command. > > > >> > >> >> The 'bcb' command is an excellent candidate for this purpose. > > > >> > >> >> > > > >> > >> >> This patch integrates the separate 'ab_select' command into > > > >> > >> >> the 'bcb' > > > >> > >> >> group as the 'ab_select' subcommand, maintaining the same > > > >> > >> >> parameter list > > > >> > >> >> for consistency. > > > >> > >> >> > > > >> > >> >> Signed-off-by: Dmitry Rokosov <ddroko...@salutedevices.com> > > > >> > >> >> --- > > > >> > >> >> MAINTAINERS | 1 - > > > >> > >> >> cmd/Kconfig | 15 +------ > > > >> > >> >> cmd/Makefile | 1 - > > > >> > >> >> cmd/ab_select.c | 66 > > > >> > >> >> ------------------------------- > > > >> > >> >> cmd/bcb.c | 63 > > > >> > >> >> +++++++++++++++++++++++++++++ > > > >> > >> >> configs/am57xx_hs_evm_usb_defconfig | 1 - > > > >> > >> >> configs/khadas-vim3_android_ab_defconfig | 1 - > > > >> > >> >> configs/khadas-vim3l_android_ab_defconfig | 1 - > > > >> > >> >> configs/sandbox64_defconfig | 4 +- > > > >> > >> >> configs/sandbox_defconfig | 4 +- > > > >> > >> >> doc/android/ab.rst | 12 +++--- > > > >> > >> >> include/configs/khadas-vim3_android.h | 2 +- > > > >> > >> >> include/configs/khadas-vim3l_android.h | 2 +- > > > >> > >> >> include/configs/meson64_android.h | 4 +- > > > >> > >> >> include/configs/ti_omap5_common.h | 4 +- > > > >> > >> >> test/py/tests/test_android/test_ab.py | 8 ++-- > > > >> > >> >> 16 files changed, 85 insertions(+), 104 deletions(-) > > > >> > >> >> > > > >> > >> >> diff --git a/MAINTAINERS b/MAINTAINERS > > > >> > >> >> index > > > >> > >> >> 7aefda93d017f07d616f0f6d191129914fbeb484..668ccec9ae6df47192b1af668e3fdbeb1dfa15ea > > > >> > >> >> 100644 > > > >> > >> >> --- a/MAINTAINERS > > > >> > >> >> +++ b/MAINTAINERS > > > >> > >> >> @@ -65,7 +65,6 @@ R: Sam Protsenko > > > >> > >> >> <semen.protse...@linaro.org> > > > >> > >> >> S: Maintained > > > >> > >> >> T: git > > > >> > >> >> https://source.denx.de/u-boot/custodians/u-boot-dfu.git > > > >> > >> >> F: boot/android_ab.c > > > >> > >> >> -F: cmd/ab_select.c > > > >> > >> >> F: doc/android/ab.rst > > > >> > >> >> F: include/android_ab.h > > > >> > >> >> F: test/py/tests/test_android/test_ab.py > > > >> > >> >> diff --git a/cmd/Kconfig b/cmd/Kconfig > > > >> > >> >> index > > > >> > >> >> dd33266cec70a2b134b7244acae1b7f098b921e8..11e8d363dc9b137723a86a240412d82dd0dbccc5 > > > >> > >> >> 100644 > > > >> > >> >> --- a/cmd/Kconfig > > > >> > >> >> +++ b/cmd/Kconfig > > > >> > >> >> @@ -1067,6 +1067,7 @@ config CMD_ADC > > > >> > >> >> config CMD_BCB > > > >> > >> >> bool "bcb" > > > >> > >> >> depends on PARTITIONS > > > >> > >> >> + depends on ANDROID_AB > > > >> > >> > > > > >> > >> > When building with khadas-vim3_android_defconfig, we can see > > > >> > >> > that CMD_BCB is no > > > >> > >> > longer part of that build: > > > >> > >> > > > > >> > >> > $ grep CMD_BCB .config > > > >> > >> > <empty> > > > >> > >> > > > > >> > >> > However, if we look at include/configs/meson64_android.h, we > > > >> > >> > can see > > > >> > >> > that the "bcb" command is not only used for checking the _slot > > > >> > >> > suffix. > > > >> > >> > > > > >> > >> > It's also used for checking the bootloader reason. For example, > > > >> > >> > in > > > >> > >> > BOOTENV_DEV_FASTBOOT, we call: > > > >> > >> > > > > >> > >> > "if bcb test command = bootonce-bootloader; then " \ > > > >> > >> > > > > >> > >> > Since CMD_BCB is no longer part of the .config (due to this > > > >> > >> > dependency), > > > >> > >> > the boot script now shows errors: > > > >> > >> > > > > >> > >> > """ > > > >> > >> > U-Boot 2024.10-00796-g969325278805 (Oct 11 2024 - 14:46:00 > > > >> > >> > +0200) khadas-vim3 > > > >> > >> > > > > >> > >> > Model: Khadas VIM3 > > > >> > >> > SoC: Amlogic Meson G12B (A311D) Revision 29:b (10:2) > > > >> > >> > DRAM: 2 GiB (effective 3.8 GiB) > > > >> > >> > Core: 411 devices, 36 uclasses, devicetree: separate > > > >> > >> > MMC: mmc@ffe03000: 0, mmc@ffe05000: 1, mmc@ffe07000: 2 > > > >> > >> > Loading Environment from MMC... fs uses incompatible features: > > > >> > >> > 00020000, ignoring > > > >> > >> > Reading from MMC(2)... *** Warning - bad CRC, using default > > > >> > >> > environment > > > >> > >> > > > > >> > >> > In: usbkbd,serial > > > >> > >> > Out: vidconsole,serial > > > >> > >> > Err: vidconsole,serial > > > >> > >> > Net: eth0: ethernet@ff3f0000 > > > >> > >> > > > > >> > >> > Hit any key to stop autoboot: 0 > > > >> > >> > Verify GPT: success! > > > >> > >> > Unknown command 'bcb' - try 'help' > > > >> > >> > Warning: BCB is corrupted or does not exist > > > >> > >> > dev: pinctrl@14 > > > >> > >> > dev: pinctrl@40 > > > >> > >> > gpio: pin 88 (gpio 88) value is 1 > > > >> > >> > Unknown command 'bcb' - try 'help' > > > >> > >> > Warning: BCB is corrupted or does not exist > > > >> > >> > Loading Android boot partition... > > > >> > >> > switch to partitions #0, OK > > > >> > >> > mmc2(part 0) is current device > > > >> > >> > """ > > > >> > >> > > > > >> > >> > I know we should not be using a boot script, nor non A/B > > > >> > >> > configs but > > > >> > >> > it's a bummer that this series breaks an upstream > > > >> > >> > defconfig (khadas-vim3_android_defconfig) > > > >> > >> > > > > >> > >> > My recommendation: > > > >> > >> > > > > >> > >> > Make BCB_CMD_AB_SELECT implementation dependant on ANDROID_AB. > > > >> > >> > This way, users can use CMD_BCB with and without ANDROID_AB > > > >> > >> > being enabled. > > > >> > >> > > > > >> > >> > We could do: > > > >> > >> > When ANDROID_AB=y, implement bcb ab_select subcommand > > > >> > >> > When ANDROID_AB=n, command is not accessible. > > > >> > >> > > > > >> > >> > I'll send you a diff shortly for this. > > > >> > >> > > > >> > >> Here is an illustration on how that would work: > > > >> > >> > > > >> > >> diff --git a/cmd/Kconfig b/cmd/Kconfig > > > >> > >> index 861c31e26408..e1a4a97b042d 100644 > > > >> > >> --- a/cmd/Kconfig > > > >> > >> +++ b/cmd/Kconfig > > > >> > >> @@ -1055,7 +1055,6 @@ config CMD_ADC > > > >> > >> config CMD_BCB > > > >> > >> bool "bcb" > > > >> > >> depends on PARTITIONS > > > >> > >> - depends on ANDROID_AB > > > >> > >> help > > > >> > >> Read/modify/write the fields of Bootloader Control Block, > > > >> > >> usually > > > >> > >> stored on the flash "misc" partition with its structure > > > >> > >> defined in: > > > >> > >> diff --git a/cmd/bcb.c b/cmd/bcb.c > > > >> > >> index 4fd32186ae65..4fe634f14cc5 100644 > > > >> > >> --- a/cmd/bcb.c > > > >> > >> +++ b/cmd/bcb.c > > > >> > >> @@ -438,6 +438,9 @@ static int do_bcb_ab_select(struct cmd_tbl > > > >> > >> *cmdtp, int flag, int argc, > > > >> > >> char slot[2]; > > > >> > >> bool dec_tries = true; > > > >> > >> > > > >> > >> + if (!CONFIG_IS_ENABLED(AB_SELECT)) > > > >> > >> + return CMD_RET_SUCCESS; > > > >> > >> + > > > >> > >> for (int i = 4; i < argc; i++) { > > > >> > >> if (!strcmp(argv[i], "--no-dec")) > > > >> > >> dec_tries = false; > > > >> > >> @@ -474,6 +477,9 @@ static int do_bcb_ab_dump(struct cmd_tbl > > > >> > >> *cmdtp, int flag, int argc, > > > >> > >> struct blk_desc *dev_desc; > > > >> > >> struct disk_partition part_info; > > > >> > >> > > > >> > >> + if (!CONFIG_IS_ENABLED(AB_SELECT)) > > > >> > >> + return CMD_RET_SUCCESS; > > > >> > >> + > > > >> > >> if (part_get_info_by_dev_and_name_or_num(argv[1], argv[2], > > > >> > >> &dev_desc, &part_info, > > > >> > >> false) < 0) { > > > >> > >> > > > >> > > > > > >> > > We also need to include an #ifdef directive for the > > > >> > > ab_select_slot() > > > >> > > function usage; otherwise, the code will not compile successfully. > > > >> > > > > >> > Are you sure? Per my understanding, it's possible that the compiler > > > >> > optimizes this out because CONFIG_IS_ENABLED(AB_SELECT) > > > >> > is known as build time. > > > >> > > > > >> > When I tried this diff with khadas-vim3_android_defconfig I did not > > > >> > see > > > >> > any build errors. I will try again early next week. > > > >> > > > > >> > > > >> As I recall, the IS_ENABLED() mechanism serves as a runtime checker to > > > >> determine whether specific CONFIG_* options are enabled. Consequently, > > > >> all code paths under this mechanism are always compiled. I attempted to > > > >> disable CONFIG_ANDROID_AB for the sandbox_defconfig, but it resulted in > > > >> the expected linker error. > > > >> > > > >> /tmp/ccAvYrKL.ltrans25.ltrans.o: In function `do_bcb_ab_select': > > > >> <artificial>:(.text+0x6d5d): undefined reference to `ab_select_slot' > > > >> collect2: error: ld returned 1 exit status > > > >> Makefile:1813: recipe for target 'u-boot' failed > > > >> make: *** [u-boot] Error 1 > > > >> > > > >> I have already prepared a new version using #ifdef directives. I will > > > >> send it shortly. > > > > > > > > Something else is going on here, since we do this all the time and > > > > rely on it. So long as the code is behind an if() the dead code should > > > > be eliminated. > > > > > > > > > > I have passed my diff (using CONFIG_IS_ENABLED) through the U-Boot CI: > > > https://source.denx.de/u-boot/custodians/u-boot-dfu/-/pipelines/22700 > > > > > > See the branch: > > > https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commits/dmitry/ab-dump-v3 > > > > > > There are some test errors (sandbox test) but the "world build" stage > > > finished sucessfully. > > > > > > I have also tested locally using the CI container: > > > $ cd ~/work/upstream/u-boot > > > $ git clean -xdf > > > $ make mproper > > > $ docker run -v $PWD:$PWD -it > > > trini/u-boot-gitlab-ci-runner:jammy-20240227-14Mar2024 /bin/bash > > > > > > # In container > > > uboot@0ba059e8b7af/$ cd /home/mkorpershoek/work/upstream/u-boot > > > uboot@0ba059e8b7af:/home/mkorpershoek/work/upstream/u-boot$ pip install > > > -r test/py/requirements.txt > > > uboot@0ba059e8b7af:/home/mkorpershoek/work/upstream/u-boot$ > > > ./test/py/test.py --bd sandbox --build -k test_ut > > > > > > No build errors either. > > > > > > Dmitry, can you clarify what compiler/build commands you've used to see > > > that error? > > > > > > For reference, here is what buildman has in the CI container: > > > > > > uboot@0ba059e8b7af:/home/mkorpershoek/work/upstream/u-boot$ > > > ./tools/buildman/buildman --list-tool-chains > > > List of available toolchains (17): > > > aarch64 : /opt/gcc-13.2.0-nolibc/aarch64-linux/bin/aarch64-linux-gcc > > > arc : /opt/gcc-13.2.0-nolibc/arc-linux/bin/arc-linux-gcc > > > arm : > > > /opt/gcc-13.2.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc > > > c89 : /usr/bin/c89-gcc > > > c99 : /usr/bin/c99-gcc > > > i386 : /opt/gcc-13.2.0-nolibc/i386-linux/bin/i386-linux-gcc > > > m68k : /opt/gcc-13.2.0-nolibc/m68k-linux/bin/m68k-linux-gcc > > > microblaze: > > > /opt/gcc-13.2.0-nolibc/microblaze-linux/bin/microblaze-linux-gcc > > > mips : /opt/gcc-13.2.0-nolibc/mips-linux/bin/mips-linux-gcc > > > nios2 : /opt/gcc-13.2.0-nolibc/nios2-linux/bin/nios2-linux-gcc > > > powerpc : /opt/gcc-13.2.0-nolibc/powerpc-linux/bin/powerpc-linux-gcc > > > riscv32 : /opt/gcc-13.2.0-nolibc/riscv32-linux/bin/riscv32-linux-gcc > > > riscv64 : /opt/gcc-13.2.0-nolibc/riscv64-linux/bin/riscv64-linux-gcc > > > sandbox : /usr/bin/cgcc > > > sh2 : /opt/gcc-13.2.0-nolibc/sh2-linux/bin/sh2-linux-gcc > > > x86_64 : /usr/bin/x86_64-linux-gnu-gcc > > > xtensa : /opt/2020.07/xtensa-dc233c-elf/bin/xtensa-dc233c-elf-gcc > > > > > > I think we should use CONFIG_IS_ENABLED if possible > > > > What do you think if we replace > > > > ``` > > #ifdef CONFIG_ANDROID_AB > > ``` > > > > with > > > > ``` > > #if IS_ENABLED(CONFIG_ANDROID_AB) > > That seems OK to some extent...but see comments: > > > ``` > > > > Like below: > > > > ``` > > git --no-pager diff . > > diff --git a/cmd/bcb.c b/cmd/bcb.c > > index a0eff55c2ed4..82ea4f04659b 100644 > > --- a/cmd/bcb.c > > +++ b/cmd/bcb.c > > @@ -24,7 +24,7 @@ enum bcb_cmd { > > BCB_CMD_FIELD_TEST, > > BCB_CMD_FIELD_DUMP, > > BCB_CMD_STORE, > > -#ifdef CONFIG_ANDROID_AB > > +#if IS_ENABLED(CONFIG_ANDROID_AB) > > BCB_CMD_AB_SELECT, > > BCB_CMD_AB_DUMP, > > #endif > > We try to avoid this sort of thing, as it makes it impossible to use > if() instead of #ifdef(). For the same reason, header files should > always be included, not be behind #ifdefs > > > @@ -57,7 +57,7 @@ static int bcb_cmd_get(char *cmd) > > return BCB_CMD_STORE; > > if (!strcmp(cmd, "dump")) > > return BCB_CMD_FIELD_DUMP; > > -#ifdef CONFIG_ANDROID_AB > > +#if IS_ENABLED(CONFIG_ANDROID_AB) > > if (!strcmp(cmd, "ab_select")) > > if (IS_ENABLED(CONFIG_ANDROID_AB) && !strcmp(cmd, "ab_select")) > > > return BCB_CMD_AB_SELECT; > > if (!strcmp(cmd, "ab_dump")) > > @@ -96,7 +96,7 @@ static int bcb_is_misused(int argc, char *const argv[]) > > if (argc != 2) > > goto err; > > break; > > -#ifdef CONFIG_ANDROID_AB > > +#if IS_ENABLED(CONFIG_ANDROID_AB) > > case BCB_CMD_AB_SELECT: > > if (argc != 4 && argc != 5) > > goto err; > > @@ -435,7 +435,7 @@ void bcb_reset(void) > > __bcb_reset(); > > } > > > > -#ifdef CONFIG_ANDROID_AB > > +#if IS_ENABLED(CONFIG_ANDROID_AB) > > static int do_bcb_ab_select(struct cmd_tbl *cmdtp, int flag, int argc, > > char * const argv[]) > > { > > @@ -504,7 +504,7 @@ static struct cmd_tbl cmd_bcb_sub[] = { > > U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""), > > U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""), > > U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", > > ""), > > -#ifdef CONFIG_ANDROID_AB > > +#if IS_ENABLED(CONFIG_ANDROID_AB) > > U_BOOT_CMD_MKENT(ab_select, CONFIG_SYS_MAXARGS, 1, > > do_bcb_ab_select, "", ""), > > U_BOOT_CMD_MKENT(ab_dump, CONFIG_SYS_MAXARGS, 1, > > @@ -549,7 +549,7 @@ U_BOOT_CMD( > > "bcb dump <field> - dump BCB <field>\n" > > "bcb store - store BCB back to <interface>\n" > > "\n" > > -#ifdef CONFIG_ANDROID_AB > > +#if IS_ENABLED(CONFIG_ANDROID_AB) > > "bcb ab_select -\n" > > " Select the slot used to boot from and register the boot > > attempt.\n" > > " <slot_var_name> <interface> <dev[:part|#part_name]> > > [--no-dec]\n" > > ``` > > Long help should use U_BOOT_LONGHELP, we normally handle this with > #ifdef - see cmd/bootflow.c for example
Thank you for pointing me to U_BOOT_LONGHELP. I have redesigned cmd/bcb.c to utilize the U_BOOT_LONGHELP(), U_BOOT_CMD_WITH_SUBCMDS(), and U_BOOT_SUBCMD_MKENT() functions. As a result, a significant amount of command management code has been removed from cmd/bcb.c, making it look much more compact. To be honest, I think I missed the minargs parameter in U_BOOT_CMD_WITH_SUBCMDS(). Including minargs would enable us to avoid checking argc for a minimum value in the each subcmd callback and returning CMD_RET_USAGE in case of any errors. Anyway, I will prepare v5 version with refactored cmd/bcb.c to U_BOOT_LONGHELP usage. -- Thank you, Dmitry
