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 <sjg@...> wrote:
Hi Dmitry,
On Mon, 14 Oct 2024 at 14:38, Dmitry Rokosov
<ddrokosov@...> 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 <ddrokosov@...> 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@...> wrote:
Hi Dmitry,
Thank you for the patch.
On mar., oct. 08, 2024 at 23:18, Dmitry Rokosov <ddrokosov@...> 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 <ddrokosov@...>
---
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.protsenko@...>
S: Maintained
T: 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:
See the branch:
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)
```
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
@@ -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"))
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"
```
--
Thank you,
Dmitry
