Hi Dmitry, On mar., oct. 15, 2024 at 16:26, Dmitry Rokosov <ddroko...@salutedevices.com> wrote:
> Hi Mattijs, > > 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 > > I just run build on the my x86 Ubuntu machine. > > $ cd uboot > $ make mproper > $ make sandbox_defconfig > $ make -j$(nproc) I tried these commands: Here is the successfull build output: https://paste.debian.net/1332378/ I use: $ ~/work/upstream/u-boot-dfu/ dmitry/ab-dump-v3* gcc --version gcc (GCC) 14.2.1 20240912 (Red Hat 14.2.1-3) Copyright (C) 2024 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. $ ~/work/upstream/u-boot-dfu/ dmitry/ab-dump-v3* ld --version GNU ld version 2.41-37.fc40 Copyright (C) 2023 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or (at your option) a later version. This program has absolutely no warranty. What toolchains do you use? > > That's all. > > I've already sent the v4 patch with #ifdef. I can prepare the v5 patch > using the IS_ENABLED() macro and will aim to send it today. > > But I have one question: > > Do we really want to display the ab_select and ab_dump subcommands to > users if these commands are just stubs? Perhaps we should consider > adding #ifdef directives to the subcommand arrays. That's a valid concern. I don't think we should display the ab_select and ab_dump commands to the users but I still want to have IS_ENABLED wherever possible to keep the code simple. > > -- > Thank you, > Dmitry
