[U-Boot] [PATCH v2 0/8] am57xx: Implement Android 10 boot flow

[U-Boot] [PATCH v2 0/8] am57xx: Implement Android 10 boot flow

[Sam Protsenko]

Android 10 brings a lot of new requirements for bootloaders: [1]. This
patch series attempts to implement such a boot process on BeagleBoard
X15 platform. Some common code is added too, which can be reused later
for other platforms.

This patch series denends on next (still not merged) patches:
  1. libavb: Update libavb to current AOSP master [2]
  2. libavb: Fix build warnings after updating the lib [3]
  3. cmd: avb: Support A/B slots [4]
  4. cmd: avb: Fix requested partitions list [5]

Changes in v2:
  - add the unit test for new functionality ('bootimg' command, dtb/dtbo
    fields in Android Boot Image)
  - add "bootimg set_addr" sub-command
  - provide mem mappings so that 'bootimg' command works in sandbox
    (needed for tests to work correctly)
  - rebase on top of most recent master
  - re-sync am57x defconfigs with "make savedefconfig"

[1] https://source.android.com/devices/bootloader
[2] https://patchwork.ozlabs.org/patch/1147825/
[3] https://patchwork.ozlabs.org/patch/1147826/
[4] https://patchwork.ozlabs.org/patch/1144646/
[5] https://patchwork.ozlabs.org/patch/1147731/

Sam Protsenko (8):
  image: android: Add functions for handling dtb field
  image: android: Add routine to get dtbo params
  cmd: bootimg: Add bootimg command
  test/py: android: Add test for bootimg
  configs: am57xx_evm: Enable Android commands
  env: ti: boot: Respect slot_suffix in AVB commands
  env: ti: boot: Boot Android with dynamic partitions
  arm: ti: boot: Use correct dtb and dtbo on Android boot

 cmd/Kconfig                                |   8 +
 cmd/Makefile                               |   1 +
 cmd/bootimg.c                              | 210 ++++++++++++++++
 common/Makefile                            |   2 +-
 common/image-android.c                     | 276 +++++++++++++++++++++
 configs/am57xx_evm_defconfig               |  10 +-
 configs/am57xx_hs_evm_defconfig            |   6 +
 configs/am57xx_hs_evm_usb_defconfig        |   6 +
 configs/sandbox_defconfig                  |   1 +
 include/configs/ti_armv7_common.h          |   7 +
 include/environment/ti/boot.h              | 145 ++++++-----
 include/image.h                            |   6 +
 test/py/tests/test_android/test_bootimg.py | 157 ++++++++++++
 13 files changed, 767 insertions(+), 68 deletions(-)
 create mode 100644 cmd/bootimg.c
 create mode 100644 test/py/tests/test_android/test_bootimg.py

-- 
2.23.0

[U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field

[Sam Protsenko]

Android Boot Image v2 adds "DTB" payload (and corresponding field in the
image header). Provide functions for its handling:

  - android_image_get_dtb_by_index(): Obtain DTB file from "DTB" part of
    boot image, by file index
  - android_image_print_dtb_contents(): Iterate over all DTB files in
    "DTB" part of boot image and print those files info

"DTB" payload might be in one of the following formats:
  1. concatenated DTB files
  2. Android DTBO format

The latter requires "android-image-dt.c" functionality, so this commit
selects that file for building for CONFIG_ANDROID_BOOT_IMAGE option.

Right now this new functionality isn't used, but it can be used further.
As it's required to apply some specific dtbo file(s) from "dtbo"
partition, we can't automate this process inside of "bootm" command. But
we can do next:
  - come up with some new command like "bootimg" to extract dtb file
    from boot image (using functions from this patch)
  - extract desired dtbo files from "dtbo" partition using "dtimg"
    command
  - merge dtbo files into dtb file using "fdt apply" command
  - pass resulting dtb file into bootm command in order to boot the
    Android kernel with Android ramdisk from boot image

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - provide mem mappings for sandbox
  - rebase on top of master

 common/Makefile        |   2 +-
 common/image-android.c | 215 +++++++++++++++++++++++++++++++++++++++++
 include/image.h        |   5 +
 3 files changed, 221 insertions(+), 1 deletion(-)

diff --git a/common/Makefile b/common/Makefile
index 302d8beaf3..7e5f5058b3 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -108,7 +108,7 @@ endif
 
 obj-y += image.o
 obj-$(CONFIG_ANDROID_AB) += android_ab.o
-obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
+obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o image-android-dt.o
 obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
 obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
 obj-$(CONFIG_$(SPL_)MULTI_DTB_FIT) += boot_fit.o common_fit.o
diff --git a/common/image-android.c b/common/image-android.c
index 3564a64221..5e721a27a7 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -6,10 +6,12 @@
 #include <common.h>
 #include <env.h>
 #include <image.h>
+#include <image-android-dt.h>
 #include <android_image.h>
 #include <malloc.h>
 #include <errno.h>
 #include <asm/unaligned.h>
+#include <mapmem.h>
 
 #define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR	0x10008000
 
@@ -195,6 +197,122 @@ int android_image_get_second(const struct andr_img_hdr *hdr,
 	return 0;
 }
 
+/**
+ * android_image_get_dtb_img_addr() - Get the address of DTB area in boot image.
+ * @hdr_addr: Boot image header address
+ * @addr: Will contain the address of DTB area in boot image
+ *
+ * Return: true on success or false on fail.
+ */
+static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong *addr)
+{
+	const struct andr_img_hdr *hdr;
+	ulong dtb_img_addr;
+	bool res = true;
+
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	if (android_image_check_header(hdr)) {
+		printf("Error: Boot Image header is incorrect\n");
+		res = false;
+		goto exit;
+	}
+
+	if (hdr->header_version < 2) {
+		printf("Error: header_version must be >= 2 to get dtb\n");
+		res = false;
+		goto exit;
+	}
+
+	if (hdr->dtb_size == 0) {
+		printf("Error: dtb_size is 0\n");
+		res = false;
+		goto exit;
+	}
+
+	/* Calculate the address of DTB area in boot image */
+	dtb_img_addr = hdr_addr;
+	dtb_img_addr += hdr->page_size;
+	dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
+	dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
+	dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
+	dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
+
+	if (addr)
+		*addr = dtb_img_addr;
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
+/**
+ * android_image_get_dtb_by_index() - Get address and size of file in DTB area.
+ * @hdr_addr: Boot image header address
+ * @index: Index of desired DTB in DTB area (starting from 0)
+ * @addr: If not NULL, will contain address to specified DTB
+ * @size: If not NULL, will contain size of specified DTB
+ *
+ * Get the address and size of DTB file by its index in DTB area of Android
+ * Boot Image in RAM.
+ *
+ * Return: true on success or false on error.
+ */
+bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
+				    u32 *size)
+{
+	const struct andr_img_hdr *hdr;
+	bool res;
+	ulong dtb_img_addr;	/* address of DTB part in boot image */
+	u32 dtb_img_size;	/* size of DTB payload in boot image */
+	ulong dtb_addr;		/* address of DTB file with specified index  */
+	u32 i;			/* index iterator */
+
+	res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr);
+	if (!res)
+		return false;
+
+	/* Check if DTB area of boot image is in DTBO format */
+	if (android_dt_check_header(dtb_img_addr)) {
+		return android_dt_get_fdt_by_index(dtb_img_addr, index, addr,
+						   size);
+	}
+
+	/* Find out the address of DTB with specified index in concat blobs */
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	dtb_img_size = hdr->dtb_size;
+	unmap_sysmem(hdr);
+	i = 0;
+	dtb_addr = dtb_img_addr;
+	while (dtb_addr < dtb_img_addr + dtb_img_size) {
+		const struct fdt_header *fdt;
+		u32 dtb_size;
+
+		fdt = map_sysmem(dtb_addr, sizeof(*fdt));
+		if (fdt_check_header(fdt) != 0) {
+			unmap_sysmem(fdt);
+			printf("Error: Invalid FDT header for index %u\n", i);
+			return false;
+		}
+
+		dtb_size = fdt_totalsize(fdt);
+		unmap_sysmem(fdt);
+
+		if (i == index) {
+			if (size)
+				*size = dtb_size;
+			if (addr)
+				*addr = dtb_addr;
+			return true;
+		}
+
+		dtb_addr += dtb_size;
+		++i;
+	}
+
+	printf("Error: Index is out of bounds (%u/%u)\n", index, i);
+	return false;
+}
+
 #if !defined(CONFIG_SPL_BUILD)
 /**
  * android_print_contents - prints out the contents of the Android format image
@@ -246,4 +364,101 @@ void android_print_contents(const struct andr_img_hdr *hdr)
 		printf("%sdtb addr:             %llx\n", p, hdr->dtb_addr);
 	}
 }
+
+static bool android_image_print_dtb_info(const struct fdt_header *fdt,
+					 u32 index)
+{
+	int root_node_off;
+	u32 fdt_size;
+	const char *model;
+	const char *compatible;
+
+	root_node_off = fdt_path_offset(fdt, "/");
+	if (root_node_off < 0) {
+		printf("Error: Root node not found\n");
+		return false;
+	}
+
+	fdt_size = fdt_totalsize(fdt);
+	compatible = fdt_getprop(fdt, root_node_off, "compatible",
+				 NULL);
+	model = fdt_getprop(fdt, root_node_off, "model", NULL);
+
+	printf(" - DTB #%u:\n", index);
+	printf("           (DTB)size = %d\n", fdt_size);
+	printf("          (DTB)model = %s\n", model ? model : "(unknown)");
+	printf("     (DTB)compatible = %s\n",
+	       compatible ? compatible : "(unknown)");
+
+	return true;
+}
+
+/**
+ * android_image_print_dtb_contents() - Print info for DTB files in DTB area.
+ * @hdr_addr: Boot image header address
+ *
+ * DTB payload in Android Boot Image v2+ can be in one of following formats:
+ *   1. Concatenated DTB files
+ *   2. Android DTBO format (see CONFIG_CMD_DTIMG for details)
+ *
+ * This function does next:
+ *   1. Prints out the format used in DTB area
+ *   2. Iterates over all DTB files in DTB area and prints out the info for
+ *      each file.
+ *
+ * Return: true on success or false on error.
+ */
+bool android_image_print_dtb_contents(ulong hdr_addr)
+{
+	const struct andr_img_hdr *hdr;
+	bool res;
+	ulong dtb_img_addr;	/* address of DTB part in boot image */
+	u32 dtb_img_size;	/* size of DTB payload in boot image */
+	ulong dtb_addr;		/* address of DTB file with specified index  */
+	u32 i;			/* index iterator */
+
+	res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr);
+	if (!res)
+		return false;
+
+	/* Check if DTB area of boot image is in DTBO format */
+	if (android_dt_check_header(dtb_img_addr)) {
+		printf("## DTB area contents (DTBO format):\n");
+		android_dt_print_contents(dtb_img_addr);
+		return true;
+	}
+
+	printf("## DTB area contents (concat format):\n");
+
+	/* Iterate over concatenated DTB files */
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	dtb_img_size = hdr->dtb_size;
+	unmap_sysmem(hdr);
+	i = 0;
+	dtb_addr = dtb_img_addr;
+	while (dtb_addr < dtb_img_addr + dtb_img_size) {
+		const struct fdt_header *fdt;
+		u32 dtb_size;
+
+		fdt = map_sysmem(dtb_addr, sizeof(*fdt));
+		if (fdt_check_header(fdt) != 0) {
+			unmap_sysmem(fdt);
+			printf("Error: Invalid FDT header for index %u\n", i);
+			return false;
+		}
+
+		res = android_image_print_dtb_info(fdt, i);
+		if (!res) {
+			unmap_sysmem(fdt);
+			return false;
+		}
+
+		dtb_size = fdt_totalsize(fdt);
+		unmap_sysmem(fdt);
+		dtb_addr += dtb_size;
+		++i;
+	}
+
+	return true;
+}
 #endif
diff --git a/include/image.h b/include/image.h
index c1065c06f9..042eb89a0f 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1333,10 +1333,15 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
 			      ulong *rd_data, ulong *rd_len);
 int android_image_get_second(const struct andr_img_hdr *hdr,
 			      ulong *second_data, ulong *second_len);
+bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
+				    u32 *size);
 ulong android_image_get_end(const struct andr_img_hdr *hdr);
 ulong android_image_get_kload(const struct andr_img_hdr *hdr);
 ulong android_image_get_kcomp(const struct andr_img_hdr *hdr);
 void android_print_contents(const struct andr_img_hdr *hdr);
+#if !defined(CONFIG_SPL_BUILD)
+bool android_image_print_dtb_contents(ulong hdr_addr);
+#endif
 
 #endif /* CONFIG_ANDROID_BOOT_IMAGE */
 
-- 
2.23.0

[U-Boot] [PATCH v2 2/8] image: android: Add routine to get dtbo params

[Sam Protsenko]

Android Boot Image v1 adds "Recovery DTB" field in image header and
associate payload in boot image itself [1]. Payload should be in
Android DTB/DTBO format [2]. That "Recovery DTB" area should be only
populated for non-A/B devices, and only in recovery image.

Add function to get an address and size of that payload. That function
can be further used e.g. in 'bootimg' command to provide the user a way
to get the address of recovery dtbo from U-Boot shell, which can be
further parsed using 'dtimg' command.

[1] https://source.android.com/devices/bootloader/boot-image-header
[2] https://source.android.com/devices/architecture/dto/partitions

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - provide mem mappings for sandbox
  - rebase on top of master

 common/image-android.c | 61 ++++++++++++++++++++++++++++++++++++++++++
 include/image.h        |  1 +
 2 files changed, 62 insertions(+)

diff --git a/common/image-android.c b/common/image-android.c
index 5e721a27a7..c833e319d4 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -197,6 +197,67 @@ int android_image_get_second(const struct andr_img_hdr *hdr,
 	return 0;
 }
 
+/**
+ * android_image_get_dtbo() - Get address and size of recovery DTBO image.
+ * @hdr_addr: Boot image header address
+ * @addr: If not NULL, will contain address of recovery DTBO image
+ * @size: If not NULL, will contain size of recovery DTBO image
+ *
+ * Get the address and size of DTBO image in "Recovery DTBO" area of Android
+ * Boot Image in RAM. The format of this image is Android DTBO (see
+ * corresponding "DTB/DTBO Partitions" AOSP documentation for details). Once
+ * the address is obtained from this function, one can use 'dtimg' U-Boot
+ * command or android_dt_*() functions to extract desired DTBO file.
+ *
+ * This DTBO (included in boot image) is only needed for non-A/B devices, and it
+ * only can be found in recovery image. On A/B devices we can always rely on
+ * "dtbo" partition. See "Including DTBO in Recovery for Non-A/B Devices" in
+ * AOSP documentation for details.
+ *
+ * Return: true on success or false on error.
+ */
+bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size)
+{
+	const struct andr_img_hdr *hdr;
+	ulong dtbo_img_addr;
+	bool res = true;
+
+	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
+	if (android_image_check_header(hdr)) {
+		printf("Error: Boot Image header is incorrect\n");
+		res = false;
+		goto exit;
+	}
+
+	if (hdr->header_version < 1) {
+		printf("Error: header_version must be >= 1 to get dtbo\n");
+		res = false;
+		goto exit;
+	}
+
+	if (hdr->recovery_dtbo_size == 0) {
+		printf("Error: recovery_dtbo_size is 0\n");
+		res = false;
+		goto exit;
+	}
+
+	/* Calculate the address of DTB area in boot image */
+	dtbo_img_addr = hdr_addr;
+	dtbo_img_addr += hdr->page_size;
+	dtbo_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
+	dtbo_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
+	dtbo_img_addr += ALIGN(hdr->second_size, hdr->page_size);
+
+	if (addr)
+		*addr = dtbo_img_addr;
+	if (size)
+		*size = hdr->recovery_dtbo_size;
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
 /**
  * android_image_get_dtb_img_addr() - Get the address of DTB area in boot image.
  * @hdr_addr: Boot image header address
diff --git a/include/image.h b/include/image.h
index 042eb89a0f..55fe57e9da 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1333,6 +1333,7 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
 			      ulong *rd_data, ulong *rd_len);
 int android_image_get_second(const struct andr_img_hdr *hdr,
 			      ulong *second_data, ulong *second_len);
+bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size);
 bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
 				    u32 *size);
 ulong android_image_get_end(const struct andr_img_hdr *hdr);
-- 
2.23.0

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

This command can be used to extract fields and image payloads from
Android Boot Image. It can be used for example to implement boot flow
where dtb is taken from boot.img (as v2 incorporated dtb inside of
boot.img). Using this command, one can obtain needed dtb file from
boot.img in scripting manner, and then apply needed dtbo's (from "dtbo"
partition) on top of that, providing then the resulting image to bootm
command in order to boot the Android.

Also right now this command has the sub-command to get an address and
size of recovery dtbo from recovery image. It can be further parsed using
'dtimg' command and merged into dtb file (for non-A/B devices only, see
[1,2] for details).

[1] https://source.android.com/devices/bootloader/boot-image-header
[2] https://source.android.com/devices/architecture/dto/partitions

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - add "set_addr" sub-command
  - provide mem mappings for sandbox
  - rebase on top of master

 cmd/Kconfig   |   8 ++
 cmd/Makefile  |   1 +
 cmd/bootimg.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 cmd/bootimg.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 07060c63a7..d89a91cde4 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -356,6 +356,14 @@ config CMD_DTIMG
 	  files should be merged in one dtb further, which needs to be passed to
 	  the kernel, as part of a boot process.
 
+config CMD_BOOTIMG
+	bool "bootimg"
+	depends on ANDROID_BOOT_IMAGE
+	help
+	  Android Boot Image manipulation commands. Allows one to extract
+	  images contained in boot.img, like kernel, ramdisk, dtb, etc, and
+	  obtain corresponding meta-information from boot.img.
+
 config CMD_ELF
 	bool "bootelf, bootvx"
 	default y
diff --git a/cmd/Makefile b/cmd/Makefile
index ac843b4b16..bf15e38f41 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -48,6 +48,7 @@ ifdef CONFIG_POST
 obj-$(CONFIG_CMD_DIAG) += diag.o
 endif
 obj-$(CONFIG_CMD_DTIMG) += dtimg.o
+obj-$(CONFIG_CMD_BOOTIMG) += bootimg.o
 obj-$(CONFIG_CMD_ECHO) += echo.o
 obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
 obj-$(CONFIG_CMD_EEPROM) += eeprom.o
diff --git a/cmd/bootimg.c b/cmd/bootimg.c
new file mode 100644
index 0000000000..a26a74f124
--- /dev/null
+++ b/cmd/bootimg.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2018 Linaro Ltd.
+ * Sam Protsenko <semen.protsenko@linaro.org>
+ */
+
+#include <android_image.h>
+#include <common.h>
+#include <mapmem.h>
+
+#define bootimg_addr() (_bootimg_addr == -1 ? load_addr : _bootimg_addr)
+
+/* Please use bootimg_addr() macro to obtain the boot image address */
+static ulong _bootimg_addr = -1;
+
+static int do_bootimg_set_addr(cmd_tbl_t *cmdtp, int flag, int argc,
+			       char * const argv[])
+{
+	char *endp;
+	ulong img_addr;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	img_addr = simple_strtoul(argv[1], &endp, 16);
+	if (*endp != '\0') {
+		printf("Error: Wrong image address\n");
+		return CMD_RET_FAILURE;
+	}
+
+	_bootimg_addr = img_addr;
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_bootimg_ver(cmd_tbl_t *cmdtp, int flag, int argc,
+			  char * const argv[])
+{
+	const struct andr_img_hdr *hdr;
+	char buf[65];
+	int res = CMD_RET_SUCCESS;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	hdr = map_sysmem(bootimg_addr(), sizeof(*hdr));
+	if (android_image_check_header(hdr)) {
+		printf("Error: Boot Image header is incorrect\n");
+		res = CMD_RET_FAILURE;
+		goto exit;
+	}
+
+	snprintf(buf, sizeof(buf), "%u", hdr->header_version);
+	env_set(argv[1], buf);
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
+static int do_bootimg_get_dtbo(cmd_tbl_t *cmdtp, int flag, int argc,
+			       char * const argv[])
+{
+	char buf[65];
+	ulong addr;
+	u32 size;
+
+	if (argc < 2 || argc > 3)
+		return CMD_RET_USAGE;
+
+	if (!android_image_get_dtbo(bootimg_addr(), &addr, &size))
+		return CMD_RET_FAILURE;
+
+	snprintf(buf, sizeof(buf), "%lx", addr);
+	env_set(argv[1], buf);
+
+	if (argc == 3) {
+		snprintf(buf, sizeof(buf), "%x", size);
+		env_set(argv[2], buf);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_bootimg_dtb_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+			       char * const argv[])
+{
+	if (argc != 1)
+		return CMD_RET_USAGE;
+
+	if (android_image_print_dtb_contents(bootimg_addr()))
+		return CMD_RET_FAILURE;
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_bootimg_dtb_load_addr(cmd_tbl_t *cmdtp, int flag, int argc,
+				    char * const argv[])
+{
+	const struct andr_img_hdr *hdr;
+	char buf[65];
+	int res = CMD_RET_SUCCESS;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	hdr = map_sysmem(bootimg_addr(), sizeof(*hdr));
+	if (android_image_check_header(hdr)) {
+		printf("Error: Boot Image header is incorrect\n");
+		res = CMD_RET_FAILURE;
+		goto exit;
+	}
+
+	if (hdr->header_version < 2) {
+		printf("Error: header_version must be >= 2 for this\n");
+		res = CMD_RET_FAILURE;
+		goto exit;
+	}
+
+	snprintf(buf, sizeof(buf), "%lx", (ulong)hdr->dtb_addr);
+	env_set(argv[1], buf);
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
+static int do_bootimg_get_dtb_file(cmd_tbl_t *cmdtp, int flag, int argc,
+				   char * const argv[])
+{
+	char *endp;
+	char buf[65];
+	u32 index;
+	ulong addr;
+	u32 size;
+
+	if (argc < 3 || argc > 4)
+		return CMD_RET_USAGE;
+
+	index = simple_strtoul(argv[1], &endp, 0);
+	if (*endp != '\0') {
+		printf("Error: Wrong index\n");
+		return CMD_RET_FAILURE;
+	}
+
+	if (!android_image_get_dtb_by_index(bootimg_addr(), index, &addr,
+					    &size))
+		return CMD_RET_FAILURE;
+
+	snprintf(buf, sizeof(buf), "%lx", addr);
+	env_set(argv[2], buf);
+
+	if (argc == 4) {
+		snprintf(buf, sizeof(buf), "%x", size);
+		env_set(argv[3], buf);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static cmd_tbl_t cmd_bootimg_sub[] = {
+	U_BOOT_CMD_MKENT(set_addr, 2, 1, do_bootimg_set_addr, "", ""),
+	U_BOOT_CMD_MKENT(ver, 2, 1, do_bootimg_ver, "", ""),
+	U_BOOT_CMD_MKENT(get_dtbo, 3, 1, do_bootimg_get_dtbo, "", ""),
+	U_BOOT_CMD_MKENT(dtb_dump, 1, 1, do_bootimg_dtb_dump, "", ""),
+	U_BOOT_CMD_MKENT(dtb_load_addr, 2, 1, do_bootimg_dtb_load_addr, "", ""),
+	U_BOOT_CMD_MKENT(get_dtb_file, 4, 1, do_bootimg_get_dtb_file, "", ""),
+};
+
+static int do_bootimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	cmd_tbl_t *cp;
+
+	cp = find_cmd_tbl(argv[1], cmd_bootimg_sub,
+			  ARRAY_SIZE(cmd_bootimg_sub));
+
+	/* Strip off leading 'bootimg' command argument */
+	argc--;
+	argv++;
+
+	if (!cp || argc > cp->maxargs)
+		return CMD_RET_USAGE;
+	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
+		return CMD_RET_SUCCESS;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+U_BOOT_CMD(
+	bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
+	"manipulate Android Boot Image",
+	"set_addr <addr>\n"
+	"    - set the address in RAM where boot image is located\n"
+	"      ($loadaddr is used by default)\n"
+	"bootimg ver <varname>\n"
+	"    - get header version\n"
+	"bootimg get_dtbo <addr_var> [size_var]\n"
+	"    - get address and size (hex) of recovery DTBO area in the image\n"
+	"      <addr_var>: variable name to contain DTBO area address\n"
+	"      [size_var]: variable name to contain DTBO area size\n"
+	"bootimg dtb_dump\n"
+	"    - print info for all files in DTB area\n"
+	"bootimg dtb_load_addr <varname>\n"
+	"    - get load address (hex) of DTB\n"
+	"bootimg get_dtb_file <index> <addr_var> [size_var]\n"
+	"    - get address and size (hex) of DTB file in the image\n"
+	"      <index>: index of desired DTB file in DTB area\n"
+	"      <addr_var>: variable name to contain DTB file address\n"
+	"      [size_var]: variable name to contain DTB file size\n"
+);
-- 
2.23.0

[U-Boot] [PATCH v2 4/8] test/py: android: Add test for bootimg

[Sam Protsenko]

Unit test for 'bootimg' command. Right now it covers dtb/dtbo
functionality in Android Boot Image v2, which was added recently.

Running test:

    $ ./test/py/test.py --bd sandbox --build -k test_bootimg

shows that 1/1 tests passes successfully.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - this test appears in v2

 configs/sandbox_defconfig                  |   1 +
 test/py/tests/test_android/test_bootimg.py | 157 +++++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 test/py/tests/test_android/test_bootimg.py

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 20ebc68997..8829993c6e 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -24,6 +24,7 @@ CONFIG_ANDROID_AB=y
 CONFIG_CMD_CPU=y
 CONFIG_CMD_LICENSE=y
 CONFIG_CMD_BOOTZ=y
+CONFIG_CMD_BOOTIMG=y
 # CONFIG_CMD_ELF is not set
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
diff --git a/test/py/tests/test_android/test_bootimg.py b/test/py/tests/test_android/test_bootimg.py
new file mode 100644
index 0000000000..a4792efcf5
--- /dev/null
+++ b/test/py/tests/test_android/test_bootimg.py
@@ -0,0 +1,157 @@
+# SPDX-License-Identifier:  GPL-2.0+
+# Copyright (c) 2019, Linaro Limited
+# Author: Sam Protsenko <semen.protsenko@linaro.org>
+
+# Test U-Boot's "bootimg" commands.
+
+import os
+import pytest
+import u_boot_utils
+
+"""
+These tests rely on disk image (boot.img), which is automatically created by
+the test from the stored hex dump. This is done to avoid the dependency on the
+most recent mkbootimg tool from AOSP/master. Here is the list of commands which
+was used to generate the boot.img and obtain compressed hex dump from it:
+
+    $ echo '/dts-v1/; / { model = "x1"; compatible = "y1,z1"; };' > test1.dts
+    $ echo '/dts-v1/; / { model = "x2"; compatible = "y2,z2"; };' > test2.dts
+    $ dtc test1.dts > dt1.dtb
+    $ dtc test2.dts > dt2.dtb
+    $ cat dt1.dtb dt2.dtb > dtb.img
+    $ echo 'kernel payload' > kernel
+    $ echo 'ramdisk payload' > ramdisk.img
+    $ mkbootimg --kernel ./kernel --ramdisk ./ramdisk.img  \
+                --cmdline "cmdline test" --dtb ./dtb.img   \
+                --os_version R --os_patch_level 2019-06-05 \
+                --header_version 2 --output boot.img
+    $ gzip -9 boot.img
+    $ xxd -p boot.img.gz > boot.img.gz.hex
+
+Now one can obtain original boot.img from this hex dump like this:
+
+    $ xxd -r -p boot.img.gz.hex boot.img.gz
+    $ gunzip -9 boot.img.gz
+"""
+
+# boot.img.gz hex dump
+img_hex = """1f8b08084844af5d0203626f6f742e696d670073f47309f2f77451e46700
+820606010106301084501f04181819041838181898803c3346060c909c9b
+92939997aa50925a5cc2300a461c3078b2e1793c4b876fd92db97939fb6c
+b7762ffff07d345446c1281805e8a0868d81e117a45e111c0d8dc101b253
+8bf25273140a122b73f21353b8460364148c8251300a46c1281801a02831
+3725b3387bb401300a46c1281805a360148c207081f7df5b20550bc41640
+9c03c41a0c90f17fe85400986d82452b6c3680198a192a0ce17c3610ae34
+d4a9820881a70f3873f35352731892f3730b124b32937252a96bb9119ae5
+463a5546f82c1f05a360148c8251300a462e000085bf67f200200000"""
+# Expected response for "bootimg dtb_dump" command
+dtb_dump_resp="""## DTB area contents (concat format):
+ - DTB #0:
+           (DTB)size = 125
+          (DTB)model = x1
+     (DTB)compatible = y1,z1
+ - DTB #1:
+           (DTB)size = 125
+          (DTB)model = x2
+     (DTB)compatible = y2,z2"""
+# Address in RAM where to load the boot image ('bootimg' looks in $loadaddr)
+loadaddr = 0x1000
+# Expected DTB #1 offset from the boot image start address
+dtb1_offset = 0x187d
+# DTB #1 start address in RAM
+dtb1_addr = loadaddr + dtb1_offset
+
+class BootimgTestDiskImage(object):
+    """Disk image used by bootimg tests."""
+
+    def __init__(self, u_boot_console):
+        """Initialize a new BootimgDiskImage object.
+
+        Args:
+            u_boot_console: A U-Boot console.
+
+        Returns:
+            Nothing.
+        """
+
+        gz_hex = u_boot_console.config.persistent_data_dir + '/boot.img.gz.hex'
+        gz = u_boot_console.config.persistent_data_dir + '/boot.img.gz'
+
+        filename = 'boot.img'
+        persistent = u_boot_console.config.persistent_data_dir + '/' + filename
+        self.path = u_boot_console.config.result_dir  + '/' + filename
+
+        with u_boot_utils.persistent_file_helper(u_boot_console.log, persistent):
+            if os.path.exists(persistent):
+                u_boot_console.log.action('Disk image file ' + persistent +
+                    ' already exists')
+            else:
+                u_boot_console.log.action('Generating ' + persistent)
+
+                f = open(gz_hex, "w")
+                f.write(img_hex)
+                f.close()
+
+                cmd = ('xxd', '-r', '-p', gz_hex, gz)
+                u_boot_utils.run_and_log(u_boot_console, cmd)
+
+                cmd = ('gunzip', '-9', gz)
+                u_boot_utils.run_and_log(u_boot_console, cmd)
+
+        cmd = ('cp', persistent, self.path)
+        u_boot_utils.run_and_log(u_boot_console, cmd)
+
+gtdi = None
+ at pytest.fixture(scope='function')
+def bootimg_disk_image(u_boot_console):
+    """pytest fixture to provide a BootimgTestDiskImage object to tests.
+    This is function-scoped because it uses u_boot_console, which is also
+    function-scoped. However, we don't need to actually do any function-scope
+    work, so this simply returns the same object over and over each time."""
+
+    global gtdi
+    if not gtdi:
+        gtdi = BootimgTestDiskImage(u_boot_console)
+    return gtdi
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('android_boot_image')
+ at pytest.mark.buildconfigspec('cmd_bootimg')
+ at pytest.mark.buildconfigspec('cmd_fdt')
+ at pytest.mark.requiredtool('xxd')
+ at pytest.mark.requiredtool('gunzip')
+def test_bootimg(bootimg_disk_image, u_boot_console):
+    """Test the 'bootimg' command."""
+
+    u_boot_console.log.action('Loading disk image to RAM...')
+    u_boot_console.run_command('setenv loadaddr 0x%x' % (loadaddr))
+    u_boot_console.run_command('host load hostfs - 0x%x %s' % (loadaddr,
+        bootimg_disk_image.path))
+
+    u_boot_console.log.action('Testing \'bootimg ver\' command...')
+    u_boot_console.run_command('bootimg ver v')
+    response = u_boot_console.run_command('env print v')
+    assert response == 'v=2'
+
+    u_boot_console.log.action('Testing \'bootimg get_dtbo\' command...')
+    response = u_boot_console.run_command('bootimg get_dtbo a')
+    assert response == 'Error: recovery_dtbo_size is 0'
+
+    u_boot_console.log.action('Testing \'bootimg dtb_dump\' command...')
+    response = u_boot_console.run_command('bootimg dtb_dump').replace('\r', '')
+    assert response == dtb_dump_resp
+
+    u_boot_console.log.action('Testing \'bootimg dtb_load_addr\' command...')
+    u_boot_console.run_command('bootimg dtb_load_addr a')
+    response = u_boot_console.run_command('env print a')
+    assert response == 'a=11f00000'
+
+    u_boot_console.log.action('Testing \'bootimg get_dtb_file\' command...')
+    u_boot_console.run_command('bootimg get_dtb_file 1 dtb1_start')
+    response = u_boot_console.run_command('env print dtb1_start')
+    correct_str = "dtb1_start=%x" % (dtb1_addr)
+    assert response == correct_str
+    u_boot_console.run_command('fdt addr $dtb1_start')
+    u_boot_console.run_command('fdt get value v / model')
+    response = u_boot_console.run_command('env print v')
+    assert response == 'v=x2'
-- 
2.23.0

[U-Boot] [PATCH v2 5/8] configs: am57xx_evm: Enable Android commands

[Sam Protsenko]

Enable Android commands that will be needed for Android 10 boot flow
implementation, for all AM57x variants. Commands enabled:

  1. 'bootimg':
     - CONFIG_CMD_BOOTIMG=y
  2. 'ab_select':
     - CONFIG_ANDROID_AB=y
     - CONFIG_CMD_AB_SELECT=y
  3. 'avb':
     - CONFIG_LIBAVB=y
     - CONFIG_AVB_VERIFY=y
     - CONFIG_CMD_AVB=y

While at it, resync defconfig files with "make savedefconfig".

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - rebase on top of master
  - re-sync with "make savedefconfig"

 configs/am57xx_evm_defconfig        | 10 ++++++++--
 configs/am57xx_hs_evm_defconfig     |  6 ++++++
 configs/am57xx_hs_evm_usb_defconfig |  6 ++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/configs/am57xx_evm_defconfig b/configs/am57xx_evm_defconfig
index e2f558cde7..ded9ecdcc5 100644
--- a/configs/am57xx_evm_defconfig
+++ b/configs/am57xx_evm_defconfig
@@ -21,6 +21,8 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
 # CONFIG_MISC_INIT_R is not set
 CONFIG_VERSION_VARIABLE=y
 CONFIG_BOARD_EARLY_INIT_F=y
+CONFIG_AVB_VERIFY=y
+CONFIG_ANDROID_AB=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_DMA_SUPPORT=y
@@ -30,11 +32,14 @@ CONFIG_SPL_SPI_LOAD=y
 CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
 CONFIG_SPL_YMODEM_SUPPORT=y
 CONFIG_CMD_DTIMG=y
+CONFIG_CMD_BOOTIMG=y
 CONFIG_CMD_SPL=y
 CONFIG_CMD_BCB=y
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_AB_SELECT=y
 # CONFIG_CMD_PMIC is not set
+CONFIG_CMD_AVB=y
 CONFIG_OF_CONTROL=y
 CONFIG_SPL_OF_CONTROL=y
 CONFIG_DEFAULT_DEVICE_TREE="am572x-idk"
@@ -48,6 +53,8 @@ CONFIG_SPL_REGMAP=y
 CONFIG_SPL_SYSCON=y
 CONFIG_SPL_OF_TRANSLATE=y
 CONFIG_DWC_AHCI=y
+CONFIG_CLK=y
+CONFIG_CLK_CDCE9XX=y
 CONFIG_DFU_MMC=y
 CONFIG_DFU_RAM=y
 CONFIG_DFU_SF=y
@@ -98,5 +105,4 @@ CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments"
 CONFIG_USB_GADGET_VENDOR_NUM=0x0451
 CONFIG_USB_GADGET_PRODUCT_NUM=0xd022
-CONFIG_CLK=y
-CONFIG_CLK_CDCE9XX=y
+CONFIG_LIBAVB=y
diff --git a/configs/am57xx_hs_evm_defconfig b/configs/am57xx_hs_evm_defconfig
index 7b56df8db7..89e5094a74 100644
--- a/configs/am57xx_hs_evm_defconfig
+++ b/configs/am57xx_hs_evm_defconfig
@@ -26,6 +26,8 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
 # CONFIG_MISC_INIT_R is not set
 CONFIG_VERSION_VARIABLE=y
 CONFIG_BOARD_EARLY_INIT_F=y
+CONFIG_AVB_VERIFY=y
+CONFIG_ANDROID_AB=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_DMA_SUPPORT=y
@@ -33,10 +35,13 @@ CONFIG_SPL_DMA_SUPPORT=y
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_SYS_SPI_U_BOOT_OFFS=0x40000
 CONFIG_CMD_DTIMG=y
+CONFIG_CMD_BOOTIMG=y
 CONFIG_CMD_BCB=y
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_AB_SELECT=y
 # CONFIG_CMD_PMIC is not set
+CONFIG_CMD_AVB=y
 CONFIG_OF_CONTROL=y
 CONFIG_SPL_OF_CONTROL=y
 CONFIG_DEFAULT_DEVICE_TREE="am57xx-beagle-x15"
@@ -96,3 +101,4 @@ CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments"
 CONFIG_USB_GADGET_VENDOR_NUM=0x0451
 CONFIG_USB_GADGET_PRODUCT_NUM=0xd022
+CONFIG_LIBAVB=y
diff --git a/configs/am57xx_hs_evm_usb_defconfig b/configs/am57xx_hs_evm_usb_defconfig
index 0b47df6e3c..04dc402a23 100644
--- a/configs/am57xx_hs_evm_usb_defconfig
+++ b/configs/am57xx_hs_evm_usb_defconfig
@@ -27,6 +27,8 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
 # CONFIG_MISC_INIT_R is not set
 CONFIG_VERSION_VARIABLE=y
 CONFIG_BOARD_EARLY_INIT_F=y
+CONFIG_AVB_VERIFY=y
+CONFIG_ANDROID_AB=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_DMA_SUPPORT=y
@@ -38,10 +40,13 @@ CONFIG_SPL_USB_GADGET=y
 CONFIG_SPL_DFU=y
 CONFIG_SPL_YMODEM_SUPPORT=y
 CONFIG_CMD_DTIMG=y
+CONFIG_CMD_BOOTIMG=y
 CONFIG_CMD_BCB=y
 # CONFIG_CMD_FLASH is not set
 # CONFIG_CMD_SETEXPR is not set
+CONFIG_CMD_AB_SELECT=y
 # CONFIG_CMD_PMIC is not set
+CONFIG_CMD_AVB=y
 CONFIG_OF_CONTROL=y
 CONFIG_SPL_OF_CONTROL=y
 CONFIG_DEFAULT_DEVICE_TREE="am57xx-beagle-x15"
@@ -103,3 +108,4 @@ CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments"
 CONFIG_USB_GADGET_VENDOR_NUM=0x0451
 CONFIG_USB_GADGET_PRODUCT_NUM=0xd022
+CONFIG_LIBAVB=y
-- 
2.23.0

[U-Boot] [PATCH v2 6/8] env: ti: boot: Respect slot_suffix in AVB commands

[Sam Protsenko]

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - rebase on top of master

 include/environment/ti/boot.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
index 684a744f31..da99215fbd 100644
--- a/include/environment/ti/boot.h
+++ b/include/environment/ti/boot.h
@@ -63,7 +63,7 @@
 			"else " \
 				"echo AVB verification failed.;" \
 			"exit; fi;"
-#define AVB_VERIFY_CMD "avb_verify=avb init 1; avb verify;\0"
+#define AVB_VERIFY_CMD "avb_verify=avb init 1; avb verify $slot_suffix;\0"
 #else
 #define AVB_VERIFY_CHECK ""
 #define AVB_VERIFY_CMD ""
-- 
2.23.0

[U-Boot] [PATCH v2 7/8] env: ti: boot: Boot Android with dynamic partitions

[Sam Protsenko]

Changes:
  - use boot.img instead of boot_fit.img
  - use .dtb from boot.img v2
  - implement recovery boot
  - always boot ramdisk from boot.img, we can't mount system as root
    now, as system is a logical partition inside of super partition
  - don't add "skip_initramfs" to cmdline anymore
  - to boot into recovery, use boot image from recovery partition
  - prepare partition table:
    - A/B scheme
    - use 'super' partition instead of 'system' and 'vendor'
    - add dtbo partitions
    - introduce metadata partition

Not implemented: reading and applying dtbo files from dtbo partition.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - rebase on top of master

 include/environment/ti/boot.h | 105 +++++++++++++---------------------
 1 file changed, 41 insertions(+), 64 deletions(-)

diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
index da99215fbd..a7644a5874 100644
--- a/include/environment/ti/boot.h
+++ b/include/environment/ti/boot.h
@@ -13,28 +13,6 @@
 #define CONSOLEDEV "ttyS2"
 #endif
 
-#define VBMETA_PART_SIZE		(64 * 1024)
-
-#if defined(CONFIG_LIBAVB)
-#define VBMETA_PART \
-	"name=vbmeta,size=" __stringify(VBMETA_PART_SIZE) \
-	",uuid=${uuid_gpt_vbmeta};"
-#else
-#define VBMETA_PART			""
-#endif
-
-#if defined(CONFIG_CMD_AB_SELECT)
-#define COMMON_PARTS \
-	"name=boot_a,size=20M,uuid=${uuid_gpt_boot_a};" \
-	"name=boot_b,size=20M,uuid=${uuid_gpt_boot_b};" \
-	"name=system_a,size=1024M,uuid=${uuid_gpt_system_a};" \
-	"name=system_b,size=1024M,uuid=${uuid_gpt_system_b};"
-#else
-#define COMMON_PARTS \
-	"name=boot,size=20M,uuid=${uuid_gpt_boot};" \
-	"name=system,size=1024M,uuid=${uuid_gpt_system};"
-#endif
-
 #ifndef PARTS_DEFAULT
 /* Define the default GPT table for eMMC */
 #define PARTS_DEFAULT \
@@ -49,10 +27,15 @@
 	"name=bootloader,size=2048K,uuid=${uuid_gpt_bootloader};" \
 	"name=uboot-env,start=2432K,size=256K,uuid=${uuid_gpt_reserved};" \
 	"name=misc,size=128K,uuid=${uuid_gpt_misc};" \
-	"name=recovery,size=40M,uuid=${uuid_gpt_recovery};" \
-	COMMON_PARTS \
-	"name=vendor,size=256M,uuid=${uuid_gpt_vendor};" \
-	VBMETA_PART \
+	"name=boot_a,size=20M,uuid=${uuid_gpt_boot_a};" \
+	"name=boot_b,size=20M,uuid=${uuid_gpt_boot_b};" \
+	"name=dtbo_a,size=8M,uuid=${uuid_gpt_dtbo_a};" \
+	"name=dtbo_b,size=8M,uuid=${uuid_gpt_dtbo_b};" \
+	"name=vbmeta_a,size=64K,uuid=${uuid_gpt_vbmeta_a};" \
+	"name=vbmeta_b,size=64K,uuid=${uuid_gpt_vbmeta_b};" \
+	"name=recovery,size=64M,uuid=${uuid_gpt_recovery};" \
+	"name=super,size=2560M,uuid=${uuid_gpt_super};" \
+	"name=metadata,size=16M,uuid=${uuid_gpt_metadata};" \
 	"name=userdata,size=-,uuid=${uuid_gpt_userdata}"
 #endif /* PARTS_DEFAULT */
 
@@ -72,7 +55,7 @@
 #define CONTROL_PARTITION "misc"
 
 #if defined(CONFIG_CMD_AB_SELECT)
-#define AB_SELECT \
+#define AB_SELECT_SLOT \
 	"if part number mmc 1 " CONTROL_PARTITION " control_part_number; " \
 	"then " \
 		"echo " CONTROL_PARTITION \
@@ -82,18 +65,12 @@
 		"echo " CONTROL_PARTITION " partition not found;" \
 		"exit;" \
 	"fi;" \
-	"setenv slot_suffix _${slot_name};" \
-	"if part number mmc ${mmcdev} system${slot_suffix} " \
-	"system_part_number; then " \
-		"setenv bootargs_ab " \
-		"ro root=/dev/mmcblk${mmcdev}p${system_part_number} " \
-		"rootwait init=/init skip_initramfs " \
-		"androidboot.slot_suffix=${slot_suffix};" \
-		"echo A/B cmdline addition: ${bootargs_ab};" \
-		"setenv bootargs ${bootargs} ${bootargs_ab};" \
-	"else " \
-		"echo system${slot_suffix} partition not found;" \
-	"fi;"
+	"setenv slot_suffix _${slot_name};"
+
+#define AB_SELECT_ARGS \
+	"setenv bootargs_ab androidboot.slot_suffix=${slot_suffix}; " \
+	"echo A/B cmdline addition: ${bootargs_ab};" \
+	"setenv bootargs ${bootargs} ${bootargs_ab};"
 #else
 #define AB_SELECT ""
 #endif
@@ -121,46 +98,46 @@
 		"setenv mmcroot /dev/mmcblk0p2 rw; " \
 		"run mmcboot;\0" \
 	"emmc_android_boot=" \
+		"setenv mmcdev 1; " \
+		"mmc dev $mmcdev; " \
+		"mmc rescan; " \
+		AB_SELECT_SLOT \
 		"if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \
 		CONTROL_PARTITION "; then " \
+			"setenv ardaddr -; " \
 			"if bcb test command = bootonce-bootloader; then " \
-				"echo BCB: Bootloader boot...; " \
+				"echo Android: Bootloader boot...; " \
 				"bcb clear command; bcb store; " \
 				FASTBOOT_CMD \
+				"exit; " \
 			"elif bcb test command = boot-recovery; then " \
-				"echo BCB: Recovery boot...; " \
-				"echo Warning: recovery is not implemented; " \
-				"echo Performing normal boot for now...; " \
-				"bcb clear command; bcb store; " \
-				"run emmc_android_normal_boot; " \
+				"echo Android: Recovery boot...; " \
+				"setenv ardaddr $loadaddr;" \
+				"setenv apart recovery; " \
 			"else " \
-				"echo BCB: Normal boot requested...; " \
-				"run emmc_android_normal_boot; " \
+				"echo Android: Normal boot...; " \
+				"setenv ardaddr $loadaddr; " \
+				"setenv apart boot${slot_suffix}; " \
 			"fi; " \
 		"else " \
 			"echo Warning: BCB is corrupted or does not exist; " \
-			"echo Performing normal boot...; " \
-			"run emmc_android_normal_boot; " \
-		"fi;\0" \
-	"emmc_android_normal_boot=" \
-		"echo Trying to boot Android from eMMC ...; " \
-		"run update_to_fit; " \
+			"echo Android: Normal boot...; " \
+		"fi; " \
 		"setenv eval_bootargs setenv bootargs $bootargs; " \
 		"run eval_bootargs; " \
-		"setenv mmcdev 1; " \
 		"setenv machid fe6; " \
-		"mmc dev $mmcdev; " \
-		"mmc rescan; " \
 		AVB_VERIFY_CHECK \
-		AB_SELECT \
-		"if part start mmc ${mmcdev} boot${slot_suffix} boot_start; " \
-		"then " \
-			"part size mmc ${mmcdev} boot${slot_suffix} " \
-				"boot_size; " \
-			"mmc read ${loadaddr} ${boot_start} ${boot_size}; " \
-			"bootm ${loadaddr}#${fdtfile}; " \
+		AB_SELECT_ARGS \
+		"if part start mmc $mmcdev $apart boot_start; then " \
+			"part size mmc $mmcdev $apart boot_size; " \
+			"mmc read $loadaddr $boot_start $boot_size; " \
+			"bootimg get_dtb_file 0 dtb_start dtb_size; " \
+			"cp.b $dtb_start $fdtaddr $dtb_size; " \
+			"fdt addr $fdtaddr; " \
+			"bootm $loadaddr $ardaddr $fdtaddr; " \
 		"else " \
-			"echo boot${slot_suffix} partition not found; " \
+			"echo $apart partition not found; " \
+			"exit; " \
 		"fi;\0"
 
 #ifdef CONFIG_OMAP54XX
-- 
2.23.0

[U-Boot] [PATCH v2 8/8] arm: ti: boot: Use correct dtb and dtbo on Android boot

[Sam Protsenko]

Read correct dtb file from boot.img/recovery.img and apply correct dtbo
files from dtbo partition.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - rebase on top of master

 include/configs/ti_armv7_common.h |  7 +++++
 include/environment/ti/boot.h     | 44 ++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/include/configs/ti_armv7_common.h b/include/configs/ti_armv7_common.h
index 6d15304a65..d403613587 100644
--- a/include/configs/ti_armv7_common.h
+++ b/include/configs/ti_armv7_common.h
@@ -37,11 +37,18 @@
  * seen large trees).  We say all of this must be within the first 256MB
  * as that will normally be within the kernel lowmem and thus visible via
  * bootm_size and we only run on platforms with 256MB or more of memory.
+ *
+ * As a temporary storage for DTBO files (which should be applied into DTB
+ * file), we use the location 15.5 MB above the ramdisk. If someone wants to
+ * use ramdisk bigger than 15.5 MB, then DTBO can be loaded and applied to DTB
+ * file before loading the ramdisk, as DTBO location is only used as a temporary
+ * storage, and can be re-used after 'fdt apply' command is done.
  */
 #define DEFAULT_LINUX_BOOT_ENV \
 	"loadaddr=0x82000000\0" \
 	"kernel_addr_r=0x82000000\0" \
 	"fdtaddr=0x88000000\0" \
+	"dtboaddr=0x89000000\0" \
 	"fdt_addr_r=0x88000000\0" \
 	"rdaddr=0x88080000\0" \
 	"ramdisk_addr_r=0x88080000\0" \
diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
index a7644a5874..1b2b923842 100644
--- a/include/environment/ti/boot.h
+++ b/include/environment/ti/boot.h
@@ -75,6 +75,46 @@
 #define AB_SELECT ""
 #endif
 
+/*
+ * Prepares complete device tree file for current board (for Android boot).
+ *
+ * Boot image or recovery image should be loaded into $loadaddr prior to running
+ * these commands. The logic of these commnads is next:
+ *
+ *   1. Read correct DTB file for current SoC/board from boot image in $loadaddr
+ *      to $fdtaddr
+ *   2. Merge all needed DTBO files for current board from 'dtbo' partition
+ *      into read DTB file
+ *   3. User should provide $fdtaddr as 3rd argument to 'bootm'
+ *
+ * XXX: Why passing 0x100000 to 'fdt addr'? Maybe omit it (it's optional)?
+ */
+#define PREPARE_FDT \
+	"echo ---> Preparing FDT...; " \
+	"if test $board_name = am57xx_evm_reva3; then " \
+		"echo \"  -> Reading DTBO partition...\"; " \
+		"part start mmc ${mmcdev} dtbo${slot_suffix} p_dtbo_start; " \
+		"part size mmc ${mmcdev} dtbo${slot_suffix} p_dtbo_size; " \
+		"mmc read ${dtboaddr} ${p_dtbo_start} ${p_dtbo_size}; " \
+		"echo \"  -> Reading DTB file for AM57x EVM RevA3...\"; " \
+		"bootimg get_dtb_file 0 dtb_start dtb_size; " \
+		"cp.b $dtb_start $fdtaddr $dtb_size; " \
+		"fdt addr $fdtaddr 0x100000; " \
+		"echo \"  -> Applying DTBOs for AM57x EVM RevA3...\"; " \
+		"dtimg start ${dtboaddr} 0 dtbo_addr; " \
+		"fdt apply $dtbo_addr; " \
+		"dtimg start ${dtboaddr} 1 dtbo_addr; " \
+		"fdt apply $dtbo_addr; " \
+	"elif test $board_name = beagle_x15_revc; then " \
+		"echo \"  -> Reading DTB file for Beagle X15 RevC...\"; " \
+		"bootimg get_dtb_file 0 dtb_start dtb_size; " \
+		"cp.b $dtb_start $fdtaddr $dtb_size; " \
+		"fdt addr $fdtaddr 0x100000; " \
+	"else " \
+		"echo Error: Android boot is not supported for $board_name; " \
+		"exit; " \
+	"fi; " \
+
 #define FASTBOOT_CMD \
 	"echo Booting into fastboot ...; " \
 	"fastboot " __stringify(CONFIG_FASTBOOT_USB_DEV) "; "
@@ -131,9 +171,7 @@
 		"if part start mmc $mmcdev $apart boot_start; then " \
 			"part size mmc $mmcdev $apart boot_size; " \
 			"mmc read $loadaddr $boot_start $boot_size; " \
-			"bootimg get_dtb_file 0 dtb_start dtb_size; " \
-			"cp.b $dtb_start $fdtaddr $dtb_size; " \
-			"fdt addr $fdtaddr; " \
+			PREPARE_FDT \
 			"bootm $loadaddr $ardaddr $fdtaddr; " \
 		"else " \
 			"echo $apart partition not found; " \
-- 
2.23.0

[U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field

[Eugeniu Rosca]

Hi Sam,

On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
> Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> image header). Provide functions for its handling:

I believe this totally new degree of freedom brought by "Android Boot
Image v2" might unintentionally make some users more unhappy [0], since
it enables yet another way of passing a DTB to the Android kernel.

It looks to me that there are at least three valid design choices for
DTB storage/passing which platform maintainers have to consider:
 A. Android Image second area [1-2]
 B. Dedicated DTB/DTBO partitions on a block device
 C. DTB area in Android Image v2

While there are some major disadvantages for [A][1-2], [B] and [C] look
more or less equivalent and will most likely only differ in the tooling
used to generate and extract the useful/relevant artifacts.

Not to mention that hybrids [B+C] are also possible.
Which solution should we pick as a long-term one?

[0] https://en.wikipedia.org/wiki/The_Paradox_of_Choice
[1] 104816142f9c6a ("parse the second area of android image")
[2] 6a7b406aa8b981 ("fdt: support booting with dtb in Android image")

[..]

>  common/Makefile        |   2 +-
>  common/image-android.c | 215 +++++++++++++++++++++++++++++++++++++++++
>  include/image.h        |   5 +
>  3 files changed, 221 insertions(+), 1 deletion(-)
> 
> diff --git a/common/Makefile b/common/Makefile
> index 302d8beaf3..7e5f5058b3 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -108,7 +108,7 @@ endif
>  
>  obj-y += image.o
>  obj-$(CONFIG_ANDROID_AB) += android_ab.o
> -obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
> +obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o image-android-dt.o

I expect that in a not so distant future, Android users who care
about performance aspects of their system (e.g. in automotive sector,
where boot time is paramount to achieve ~2s to Rear View Camera) will
attempt optimizing U-Boot by compiling out features they don't need.

With this background:
 - Would it make sense to allow users to selectively enable and disable
   support for A/B-capable and non-A/B devices? My guess is that it
   is currently not an important development/review criteria, since
   particularly image-android-dt.c implements functions, some of which
   are A/B-specific, some non-A/B-specific (e.g. android_image_get_dtbo)
   and some are common.
 - I would try to avoid wiring the same compilation unit to Kbuild
   (e.g. image-android-dt.o) via multiple Kconfig symbols
   (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes
   the relationship between the CONFIG symbols unclear. As a user, I
   would like to see a simple mapping between compiled objects and
   Kconfig symbols, eventually reflecting a hierarchy/dependency:

   config ANDROID_BOOT_IMAGE
      select ANDROID_BOOT_IMAGE_DT

   config DTIMG
      select ANDROID_BOOT_IMAGE_DT

[..]

> diff --git a/common/image-android.c b/common/image-android.c
> index 3564a64221..5e721a27a7 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -6,10 +6,12 @@
>  #include <common.h>
>  #include <env.h>
>  #include <image.h>
> +#include <image-android-dt.h>
>  #include <android_image.h>
>  #include <malloc.h>
>  #include <errno.h>
>  #include <asm/unaligned.h>
> +#include <mapmem.h>
>  
>  #define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR	0x10008000
>  
> @@ -195,6 +197,122 @@ int android_image_get_second(const struct andr_img_hdr *hdr,
>  	return 0;
>  }
>  
> +/**
> + * android_image_get_dtb_img_addr() - Get the address of DTB area in boot image.
> + * @hdr_addr: Boot image header address
> + * @addr: Will contain the address of DTB area in boot image
> + *
> + * Return: true on success or false on fail.
> + */
> +static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong *addr)
> +{
> +	const struct andr_img_hdr *hdr;
> +	ulong dtb_img_addr;
> +	bool res = true;
> +
> +	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> +	if (android_image_check_header(hdr)) {
> +		printf("Error: Boot Image header is incorrect\n");
> +		res = false;
> +		goto exit;
> +	}
> +
> +	if (hdr->header_version < 2) {
> +		printf("Error: header_version must be >= 2 to get dtb\n");
> +		res = false;
> +		goto exit;
> +	}
> +
> +	if (hdr->dtb_size == 0) {
> +		printf("Error: dtb_size is 0\n");
> +		res = false;
> +		goto exit;
> +	}
> +
> +	/* Calculate the address of DTB area in boot image */
> +	dtb_img_addr = hdr_addr;
> +	dtb_img_addr += hdr->page_size;
> +	dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
> +	dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +	dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
> +	dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
> +
> +	if (addr)
> +		*addr = dtb_img_addr;

In this recent series, I noticed a consistent preference towards
doing a lot of processing with returning nothing to the caller in
case of an invalid argument. Is this by choice?

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Simon Glass]

Hi Sam,

On Wed, 23 Oct 2019 at 08:34, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> This command can be used to extract fields and image payloads from
> Android Boot Image. It can be used for example to implement boot flow
> where dtb is taken from boot.img (as v2 incorporated dtb inside of
> boot.img). Using this command, one can obtain needed dtb file from
> boot.img in scripting manner, and then apply needed dtbo's (from "dtbo"
> partition) on top of that, providing then the resulting image to bootm
> command in order to boot the Android.
>
> Also right now this command has the sub-command to get an address and
> size of recovery dtbo from recovery image. It can be further parsed using
> 'dtimg' command and merged into dtb file (for non-A/B devices only, see
> [1,2] for details).
>
> [1] https://source.android.com/devices/bootloader/boot-image-header
> [2] https://source.android.com/devices/architecture/dto/partitions
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - add "set_addr" sub-command
>   - provide mem mappings for sandbox
>   - rebase on top of master
>
>  cmd/Kconfig   |   8 ++
>  cmd/Makefile  |   1 +
>  cmd/bootimg.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+)
>  create mode 100644 cmd/bootimg.c

Sorry I still think this name 'bootimg' is confusing. How would anyone
know that it is specific to Android?

How about abootimg?

Regards,
Simon

[U-Boot] [PATCH v2 7/8] env: ti: boot: Boot Android with dynamic partitions

[Tom Rini]

On Wed, Oct 23, 2019 at 05:34:26PM +0300, Sam Protsenko wrote:

> Changes:
>   - use boot.img instead of boot_fit.img
>   - use .dtb from boot.img v2
>   - implement recovery boot
>   - always boot ramdisk from boot.img, we can't mount system as root
>     now, as system is a logical partition inside of super partition
>   - don't add "skip_initramfs" to cmdline anymore
>   - to boot into recovery, use boot image from recovery partition
>   - prepare partition table:
>     - A/B scheme
>     - use 'super' partition instead of 'system' and 'vendor'
>     - add dtbo partitions
>     - introduce metadata partition
> 
> Not implemented: reading and applying dtbo files from dtbo partition.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

This breaks at least "dra7xx_evm" building:
       arm:  +   dra7xx_evm
+(dra7xx_evm) In file included from include/configs/ti_omap5_common.h:57:0,
+(dra7xx_evm)                  from include/configs/dra7xx_evm.h:62,
+(dra7xx_evm)                  from include/config.h:5,
+(dra7xx_evm)                  from include/common.h:23,
+(dra7xx_evm)                  from env/common.c:10:
+(dra7xx_evm) include/environment/ti/boot.h:104:3: error: expected '}' before 'AB_SELECT_SLOT'
+(dra7xx_evm)    AB_SELECT_SLOT \
+(dra7xx_evm)    ^
+(dra7xx_evm) include/configs/ti_omap5_common.h:65:2: note: in expansion of macro 'DEFAULT_COMMON_BOOT_TI_ARGS'
+(dra7xx_evm)   DEFAULT_COMMON_BOOT_TI_ARGS \
+(dra7xx_evm)   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
+(dra7xx_evm) include/env_default.h:111:2: note: in expansion of macro 'CONFIG_EXTRA_ENV_SETTINGS'
+(dra7xx_evm)   CONFIG_EXTRA_ENV_SETTINGS
+(dra7xx_evm)   ^~~~~~~~~~~~~~~~~~~~~~~~~
+(dra7xx_evm) make[2]: *** [env/common.o] Error 1
+(dra7xx_evm) make[1]: *** [env] Error 2
+(dra7xx_evm) make[1]: *** wait: No child processes.  Stop.
+(dra7xx_evm) make: *** [sub-make] Error 2

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191103/2e55e259/attachment.sig>

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

Hi Sam,

On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> +static int do_bootimg_get_dtb_file(cmd_tbl_t *cmdtp, int flag, int argc,
> +				   char * const argv[])
> +{
> +	char *endp;
> +	char buf[65];
> +	u32 index;
> +	ulong addr;
> +	u32 size;
> +
> +	if (argc < 3 || argc > 4)
> +		return CMD_RET_USAGE;
> +
> +	index = simple_strtoul(argv[1], &endp, 0);
> +	if (*endp != '\0') {
> +		printf("Error: Wrong index\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	if (!android_image_get_dtb_by_index(bootimg_addr(), index, &addr,
> +					    &size))

As discussed in https://patchwork.ozlabs.org/patch/958594/#2302310, we
try to enhance the "dtimg" U-Boot command to be able to identify DT
blobs by "id" or "rev" [*] field value.

It's quite challenging in this context to avoid conflicting with your
recently proposed "bootimg" command, since the latter makes use of the
android_image_get_dtb_by_index API, which is subject of modification
and/or renaming.

I am willing to cooperate to entirely avoid or reconcile the conflict
in the best possible way, but for that I need your feedback.

[*] https://source.android.google.cn/devices/architecture/dto/partitions

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field

[Sam Protsenko]

Hi Eugeniu,

On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
> > Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> > image header). Provide functions for its handling:
>
> I believe this totally new degree of freedom brought by "Android Boot
> Image v2" might unintentionally make some users more unhappy [0], since
> it enables yet another way of passing a DTB to the Android kernel.
>
> It looks to me that there are at least three valid design choices for
> DTB storage/passing which platform maintainers have to consider:
>  A. Android Image second area [1-2]
>  B. Dedicated DTB/DTBO partitions on a block device
>  C. DTB area in Android Image v2
>
> While there are some major disadvantages for [A][1-2], [B] and [C] look
> more or less equivalent and will most likely only differ in the tooling
> used to generate and extract the useful/relevant artifacts.
>
> Not to mention that hybrids [B+C] are also possible.
> Which solution should we pick as a long-term one?
>

My opinion is next: we should provide means (commands) to achieve any
of [A,B,C] options. Then user (I mean, U-Boot developer who implements
boot for each particular board) should decide which approach to use.
Also [D] FIT image can be used instead of Android Boot Image. But we
should remember that Google requires *mandatory* for all *new* devices
(starting with Android 10) to stick with [C] approach only. Option [B]
might be harder to handle from AVB verification point of view. Btw,
when we come to "boot_android" command, I think we'll need to
implement only officially recommended boot method. Maybe provide a
param to choose whether to do Android-9 or Android-10 boot scheme.

Anyway, the short answer is: we should provide a bunch of isolated
commands to allow the user to implement any boot method.

Also, this particular patch doesn't change boot behavior (it only adds
functions to handle dtb field), so it should be backward-compatible.

> [0] https://en.wikipedia.org/wiki/The_Paradox_of_Choice
> [1] 104816142f9c6a ("parse the second area of android image")
> [2] 6a7b406aa8b981 ("fdt: support booting with dtb in Android image")
>
> [..]
>
> >  common/Makefile        |   2 +-
> >  common/image-android.c | 215 +++++++++++++++++++++++++++++++++++++++++
> >  include/image.h        |   5 +
> >  3 files changed, 221 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/Makefile b/common/Makefile
> > index 302d8beaf3..7e5f5058b3 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -108,7 +108,7 @@ endif
> >
> >  obj-y += image.o
> >  obj-$(CONFIG_ANDROID_AB) += android_ab.o
> > -obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
> > +obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o image-android-dt.o
>
> I expect that in a not so distant future, Android users who care
> about performance aspects of their system (e.g. in automotive sector,
> where boot time is paramount to achieve ~2s to Rear View Camera) will
> attempt optimizing U-Boot by compiling out features they don't need.
>
> With this background:
>  - Would it make sense to allow users to selectively enable and disable
>    support for A/B-capable and non-A/B devices? My guess is that it
>    is currently not an important development/review criteria, since
>    particularly image-android-dt.c implements functions, some of which
>    are A/B-specific, some non-A/B-specific (e.g. android_image_get_dtbo)
>    and some are common.
>  - I would try to avoid wiring the same compilation unit to Kbuild
>    (e.g. image-android-dt.o) via multiple Kconfig symbols
>    (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes
>    the relationship between the CONFIG symbols unclear. As a user, I
>    would like to see a simple mapping between compiled objects and
>    Kconfig symbols, eventually reflecting a hierarchy/dependency:
>
>    config ANDROID_BOOT_IMAGE
>       select ANDROID_BOOT_IMAGE_DT
>
>    config DTIMG
>       select ANDROID_BOOT_IMAGE_DT
>

I thought about that 4 months ago when implementing this patch, even
experimented with that. But decided to just add image-android-dt.o in
Makefile instead, don't remember for what reason exactly. Frankly,
don't really want to go there again and spend too much time (at least
not in the context of this patch series).

So here is what I suggest: can we approach this one-step-at-a-time?
This patch-set is a major step as it is, and it takes a lot of time to
get it merged. What you suggest makes sense but might have some
implications (even architectural) when trying to implement that. Can
we do that in another series?

> [..]
>
> > diff --git a/common/image-android.c b/common/image-android.c
> > index 3564a64221..5e721a27a7 100644
> > --- a/common/image-android.c
> > +++ b/common/image-android.c
> > @@ -6,10 +6,12 @@
> >  #include <common.h>
> >  #include <env.h>
> >  #include <image.h>
> > +#include <image-android-dt.h>
> >  #include <android_image.h>
> >  #include <malloc.h>
> >  #include <errno.h>
> >  #include <asm/unaligned.h>
> > +#include <mapmem.h>
> >
> >  #define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR    0x10008000
> >
> > @@ -195,6 +197,122 @@ int android_image_get_second(const struct andr_img_hdr *hdr,
> >       return 0;
> >  }
> >
> > +/**
> > + * android_image_get_dtb_img_addr() - Get the address of DTB area in boot image.
> > + * @hdr_addr: Boot image header address
> > + * @addr: Will contain the address of DTB area in boot image
> > + *
> > + * Return: true on success or false on fail.
> > + */
> > +static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong *addr)
> > +{
> > +     const struct andr_img_hdr *hdr;
> > +     ulong dtb_img_addr;
> > +     bool res = true;
> > +
> > +     hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> > +     if (android_image_check_header(hdr)) {
> > +             printf("Error: Boot Image header is incorrect\n");
> > +             res = false;
> > +             goto exit;
> > +     }
> > +
> > +     if (hdr->header_version < 2) {
> > +             printf("Error: header_version must be >= 2 to get dtb\n");
> > +             res = false;
> > +             goto exit;
> > +     }
> > +
> > +     if (hdr->dtb_size == 0) {
> > +             printf("Error: dtb_size is 0\n");
> > +             res = false;
> > +             goto exit;
> > +     }
> > +
> > +     /* Calculate the address of DTB area in boot image */
> > +     dtb_img_addr = hdr_addr;
> > +     dtb_img_addr += hdr->page_size;
> > +     dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
> > +     dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
> > +     dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
> > +     dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
> > +
> > +     if (addr)
> > +             *addr = dtb_img_addr;
>
> In this recent series, I noticed a consistent preference towards
> doing a lot of processing with returning nothing to the caller in
> case of an invalid argument. Is this by choice?
>

In this particular case, this is probably just a leftover from
copy-paste from other DTBO related function, where such a parameter is
optional. Here we can just remove that "if" condition, I guess... Not
sure it's a major issue though. If you see any broken logic or
incorrect behavior, please comment in place.

Otherwise, if there are no major concerns, I suggest to take it as is,
and adding other desired features in other series. Please add your
Reviewed-by tag if you agree.

Thanks!

> --
> Best Regards,
> Eugeniu

[U-Boot] [PATCH v2 7/8] env: ti: boot: Boot Android with dynamic partitions

[Sam Protsenko]

Hi Tom,

On Sun, Nov 3, 2019 at 4:35 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Oct 23, 2019 at 05:34:26PM +0300, Sam Protsenko wrote:
>
> > Changes:
> >   - use boot.img instead of boot_fit.img
> >   - use .dtb from boot.img v2
> >   - implement recovery boot
> >   - always boot ramdisk from boot.img, we can't mount system as root
> >     now, as system is a logical partition inside of super partition
> >   - don't add "skip_initramfs" to cmdline anymore
> >   - to boot into recovery, use boot image from recovery partition
> >   - prepare partition table:
> >     - A/B scheme
> >     - use 'super' partition instead of 'system' and 'vendor'
> >     - add dtbo partitions
> >     - introduce metadata partition
> >
> > Not implemented: reading and applying dtbo files from dtbo partition.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>
> This breaks at least "dra7xx_evm" building:
>        arm:  +   dra7xx_evm
> +(dra7xx_evm) In file included from include/configs/ti_omap5_common.h:57:0,
> +(dra7xx_evm)                  from include/configs/dra7xx_evm.h:62,
> +(dra7xx_evm)                  from include/config.h:5,
> +(dra7xx_evm)                  from include/common.h:23,
> +(dra7xx_evm)                  from env/common.c:10:
> +(dra7xx_evm) include/environment/ti/boot.h:104:3: error: expected '}' before 'AB_SELECT_SLOT'
> +(dra7xx_evm)    AB_SELECT_SLOT \
> +(dra7xx_evm)    ^
> +(dra7xx_evm) include/configs/ti_omap5_common.h:65:2: note: in expansion of macro 'DEFAULT_COMMON_BOOT_TI_ARGS'
> +(dra7xx_evm)   DEFAULT_COMMON_BOOT_TI_ARGS \
> +(dra7xx_evm)   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> +(dra7xx_evm) include/env_default.h:111:2: note: in expansion of macro 'CONFIG_EXTRA_ENV_SETTINGS'
> +(dra7xx_evm)   CONFIG_EXTRA_ENV_SETTINGS
> +(dra7xx_evm)   ^~~~~~~~~~~~~~~~~~~~~~~~~
> +(dra7xx_evm) make[2]: *** [env/common.o] Error 1
> +(dra7xx_evm) make[1]: *** [env] Error 2
> +(dra7xx_evm) make[1]: *** wait: No child processes.  Stop.
> +(dra7xx_evm) make: *** [sub-make] Error 2
>

Thanks, will be fixed in new version. I should run buildman more often :)

> --
> Tom

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

Hi Eugeniu,

On Wed, Nov 27, 2019 at 9:17 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > +static int do_bootimg_get_dtb_file(cmd_tbl_t *cmdtp, int flag, int argc,
> > +                                char * const argv[])
> > +{
> > +     char *endp;
> > +     char buf[65];
> > +     u32 index;
> > +     ulong addr;
> > +     u32 size;
> > +
> > +     if (argc < 3 || argc > 4)
> > +             return CMD_RET_USAGE;
> > +
> > +     index = simple_strtoul(argv[1], &endp, 0);
> > +     if (*endp != '\0') {
> > +             printf("Error: Wrong index\n");
> > +             return CMD_RET_FAILURE;
> > +     }
> > +
> > +     if (!android_image_get_dtb_by_index(bootimg_addr(), index, &addr,
> > +                                         &size))
>
> As discussed in https://patchwork.ozlabs.org/patch/958594/#2302310, we
> try to enhance the "dtimg" U-Boot command to be able to identify DT
> blobs by "id" or "rev" [*] field value.
>
> It's quite challenging in this context to avoid conflicting with your
> recently proposed "bootimg" command, since the latter makes use of the
> android_image_get_dtb_by_index API, which is subject of modification
> and/or renaming.
>
> I am willing to cooperate to entirely avoid or reconcile the conflict
> in the best possible way, but for that I need your feedback.
>

I'd like this patch series to be applied ASAP (probably before DTBO
patches you mention are merged). It's been too long as it is. Once
merged and we are unblocked w.r.t. Android boot stuff, we can then
look into DTBO changes you mention, and if any C API are changed, we
can change those usage overall the U-Boot tree. Hope you agree. I will
help you guys to figure out all DTBO related changes further, but
please let's get this patch series over with...

Thanks!

> [*] https://source.android.google.cn/devices/architecture/dto/partitions
>
> --
> Best Regards,
> Eugeniu

[U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field

[Eugeniu Rosca]

Hi Sam,

On Mon, Dec 02, 2019 at 07:19:53PM +0200, Sam Protsenko wrote:
> On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
> > > Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> > > image header). Provide functions for its handling:
> >
> > I believe this totally new degree of freedom brought by "Android Boot
> > Image v2" might unintentionally make some users more unhappy [0], since
> > it enables yet another way of passing a DTB to the Android kernel.
> >
> > It looks to me that there are at least three valid design choices for
> > DTB storage/passing which platform maintainers have to consider:
> >  A. Android Image second area [1-2]
> >  B. Dedicated DTB/DTBO partitions on a block device
> >  C. DTB area in Android Image v2
> >
> > While there are some major disadvantages for [A][1-2], [B] and [C] look
> > more or less equivalent and will most likely only differ in the tooling
> > used to generate and extract the useful/relevant artifacts.
> >
> > Not to mention that hybrids [B+C] are also possible.
> > Which solution should we pick as a long-term one?
> 
> My opinion is next: we should provide means (commands) to achieve any
> of [A,B,C] options. Then user (I mean, U-Boot developer who implements
> boot for each particular board) should decide which approach to use.

Fine. Thanks. But I hope you also keep in mind that the more design
choices you make available to the users (especially if redundant):
 - the more test coverage you will have to accomplish, which translates
   to more requests from Simon for test development and maintenance
 - the greater the attack surface/vector, i.e. increased likelihood for
   CVEs and the like

> Also [D] FIT image can be used instead of Android Boot Image. But we
> should remember that Google requires *mandatory* for all *new* devices
> (starting with Android 10) to stick with [C] approach only. Option [B]
> might be harder to handle from AVB verification point of view.

That's useful feedback. Thanks.

> Btw,
> when we come to "boot_android" command, I think we'll need to
> implement only officially recommended boot method. Maybe provide a
> param to choose whether to do Android-9 or Android-10 boot scheme.
> 
> Anyway, the short answer is: we should provide a bunch of isolated
> commands to allow the user to implement any boot method.

Agreed with the above warnings.

[..]

> >  - I would try to avoid wiring the same compilation unit to Kbuild
> >    (e.g. image-android-dt.o) via multiple Kconfig symbols
> >    (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes
> >    the relationship between the CONFIG symbols unclear. As a user, I
> >    would like to see a simple mapping between compiled objects and
> >    Kconfig symbols, eventually reflecting a hierarchy/dependency:
> >
> >    config ANDROID_BOOT_IMAGE
> >       select ANDROID_BOOT_IMAGE_DT
> >
> >    config DTIMG
> >       select ANDROID_BOOT_IMAGE_DT
> >
> 
> I thought about that 4 months ago when implementing this patch, even
> experimented with that. But decided to just add image-android-dt.o in
> Makefile instead, don't remember for what reason exactly. Frankly,
> don't really want to go there again and spend too much time (at least
> not in the context of this patch series).
> 
> So here is what I suggest: can we approach this one-step-at-a-time?
> This patch-set is a major step as it is, and it takes a lot of time to
> get it merged. What you suggest makes sense but might have some
> implications (even architectural) when trying to implement that. Can
> we do that in another series?

I totally agree with the following:
 - This series is a major step forward. Many thanks for this effort.
 - Bikeshedding and minor findings can be postponed.

However, for the sake of a non-chaotic git history and the right degree
of intuitiveness in the usage of all newly developed commands, I see
below review points as _major_ and better be fixed before merging:
 - The name of the "bootimg" command intrudes too much into the U-Boot
   global namespace. This has been flagged by Simon [*]
   with no reply from you yet. Do you plan to provide feedback?
 - Kconfig wiring and naming. IMHO the right decisions have to be made
   whenever new files are added to Kbuild. It is like placing the right
   pipe infrastructure into the basement of the building. Why would you
   want to make it twice? Renaming/deleting/relocating Kconfig symbols
   makes comparing [**] the configurations of distinct U-Boot/Linux
   versions pretty painful later on.
 - Agreeing on the _right_ usage scheme/pattern for any newly developed
   U-Boot command is incredibly important right from the start. What
   makes me so confident in stating that and do I have doubts about the
   current "bootimg" usage pattern? Yes, I do. That's because the
   "bootimg" arguments follow more or less the  "dtimg" usage pattern.
   My recent series [***], updating the "dtimg" Android command,
   _struggled_ with finding an intuitive way to get the DTB/DTBO based
   on user-provided "id" and/or "rev" fields, as documented in
   https://source.android.com/devices/architecture/dto/partitions . To
   give an example, making the <index> argument mandatory in
   "bootimg get_dtb_file <index> <addr_var> [size_var]" is probably not
   the best choice simply b/c searching/retrieving DTB/DTBO by absolute
   <index> in the image is just one possible search criteria. Our
   customers would _not_ like to use this criteria for obvious reasons.
   They would like to retrieve DTB/DTBO by "id" and "rev" field values.
   So, how do you interpret <index> in these latter cases if you keep
   it as a required argument? Or should we treat <index> as an optional
   parameter when "id" and/or "rev" are provided? I hope you can get a
   feeling of how important it is to agree on the usage concept for
   "bootimg" right now, not later.

[*] https://patchwork.ozlabs.org/patch/1182212/#2291600
[**] https://patchwork.kernel.org/patch/10230207/#21524437
[***] https://patchwork.ozlabs.org/cover/1202575/
     ("cmd: dtimg: Enhance with --id and --rev options (take #1)")

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field

[Sam Protsenko]

Hi,

On Tue, Dec 3, 2019 at 3:59 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Sam,
>
> On Mon, Dec 02, 2019 at 07:19:53PM +0200, Sam Protsenko wrote:
> > On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > > On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
> > > > Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> > > > image header). Provide functions for its handling:
> > >
> > > I believe this totally new degree of freedom brought by "Android Boot
> > > Image v2" might unintentionally make some users more unhappy [0], since
> > > it enables yet another way of passing a DTB to the Android kernel.
> > >
> > > It looks to me that there are at least three valid design choices for
> > > DTB storage/passing which platform maintainers have to consider:
> > >  A. Android Image second area [1-2]
> > >  B. Dedicated DTB/DTBO partitions on a block device
> > >  C. DTB area in Android Image v2
> > >
> > > While there are some major disadvantages for [A][1-2], [B] and [C] look
> > > more or less equivalent and will most likely only differ in the tooling
> > > used to generate and extract the useful/relevant artifacts.
> > >
> > > Not to mention that hybrids [B+C] are also possible.
> > > Which solution should we pick as a long-term one?
> >
> > My opinion is next: we should provide means (commands) to achieve any
> > of [A,B,C] options. Then user (I mean, U-Boot developer who implements
> > boot for each particular board) should decide which approach to use.
>
> Fine. Thanks. But I hope you also keep in mind that the more design
> choices you make available to the users (especially if redundant):
>  - the more test coverage you will have to accomplish, which translates
>    to more requests from Simon for test development and maintenance
>  - the greater the attack surface/vector, i.e. increased likelihood for
>    CVEs and the like
>
> > Also [D] FIT image can be used instead of Android Boot Image. But we
> > should remember that Google requires *mandatory* for all *new* devices
> > (starting with Android 10) to stick with [C] approach only. Option [B]
> > might be harder to handle from AVB verification point of view.
>
> That's useful feedback. Thanks.
>
> > Btw,
> > when we come to "boot_android" command, I think we'll need to
> > implement only officially recommended boot method. Maybe provide a
> > param to choose whether to do Android-9 or Android-10 boot scheme.
> >
> > Anyway, the short answer is: we should provide a bunch of isolated
> > commands to allow the user to implement any boot method.
>
> Agreed with the above warnings.
>
> [..]
>
> > >  - I would try to avoid wiring the same compilation unit to Kbuild
> > >    (e.g. image-android-dt.o) via multiple Kconfig symbols
> > >    (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes
> > >    the relationship between the CONFIG symbols unclear. As a user, I
> > >    would like to see a simple mapping between compiled objects and
> > >    Kconfig symbols, eventually reflecting a hierarchy/dependency:
> > >
> > >    config ANDROID_BOOT_IMAGE
> > >       select ANDROID_BOOT_IMAGE_DT
> > >
> > >    config DTIMG
> > >       select ANDROID_BOOT_IMAGE_DT
> > >
> >
> > I thought about that 4 months ago when implementing this patch, even
> > experimented with that. But decided to just add image-android-dt.o in
> > Makefile instead, don't remember for what reason exactly. Frankly,
> > don't really want to go there again and spend too much time (at least
> > not in the context of this patch series).
> >
> > So here is what I suggest: can we approach this one-step-at-a-time?
> > This patch-set is a major step as it is, and it takes a lot of time to
> > get it merged. What you suggest makes sense but might have some
> > implications (even architectural) when trying to implement that. Can
> > we do that in another series?
>
> I totally agree with the following:
>  - This series is a major step forward. Many thanks for this effort.
>  - Bikeshedding and minor findings can be postponed.
>
> However, for the sake of a non-chaotic git history and the right degree
> of intuitiveness in the usage of all newly developed commands, I see
> below review points as _major_ and better be fixed before merging:
>  - The name of the "bootimg" command intrudes too much into the U-Boot
>    global namespace. This has been flagged by Simon [*]
>    with no reply from you yet. Do you plan to provide feedback?

Already renamed to 'abootimg' and replied to Simon. Will send v3 soon.

>  - Kconfig wiring and naming. IMHO the right decisions have to be made
>    whenever new files are added to Kbuild. It is like placing the right
>    pipe infrastructure into the basement of the building. Why would you
>    want to make it twice? Renaming/deleting/relocating Kconfig symbols
>    makes comparing [**] the configurations of distinct U-Boot/Linux
>    versions pretty painful later on.

Ok, will look into that tomorrow. And either will improve that, or at
least will provide some proper explanation as to why I don't want to
do so :)

Do I understand correctly that your point is: we should be able to
disable all DTBO related code when using 'bootimg' command (or just
when enabling ANDROID_BOOT_IMAGE)?

>  - Agreeing on the _right_ usage scheme/pattern for any newly developed
>    U-Boot command is incredibly important right from the start. What
>    makes me so confident in stating that and do I have doubts about the
>    current "bootimg" usage pattern? Yes, I do. That's because the
>    "bootimg" arguments follow more or less the  "dtimg" usage pattern.
>    My recent series [***], updating the "dtimg" Android command,
>    _struggled_ with finding an intuitive way to get the DTB/DTBO based
>    on user-provided "id" and/or "rev" fields, as documented in
>    https://source.android.com/devices/architecture/dto/partitions . To
>    give an example, making the <index> argument mandatory in
>    "bootimg get_dtb_file <index> <addr_var> [size_var]" is probably not
>    the best choice simply b/c searching/retrieving DTB/DTBO by absolute
>    <index> in the image is just one possible search criteria. Our
>    customers would _not_ like to use this criteria for obvious reasons.
>    They would like to retrieve DTB/DTBO by "id" and "rev" field values.
>    So, how do you interpret <index> in these latter cases if you keep
>    it as a required argument? Or should we treat <index> as an optional
>    parameter when "id" and/or "rev" are provided? I hope you can get a
>    feeling of how important it is to agree on the usage concept for
>    "bootimg" right now, not later.
>

Agreed. Do you have some particular way of making 'bootimg' more
extensible? I'd like to keep index way as well (because if dtb files
are just concatenated this is probably the only way to specify it).
But I can change 'bootimg' interface to something like:

    bootimg get_dtb_file index <index> <addr_var> [size_var]
    bootimg get_dtb_file id <id> <addr_var> [size_var]
    bootimg get_dtb_file rev <rev> <addr_var> [size_var]

This approach seems to be easy to extend in future. That would be ok
with you, or you see some better way to fix this?

As for 'dtimg' command: after giving it some thought, I think not much
people using it yet. So in this particular case I don't have some
strong preference, and if you think the 'dtimg' interface is ugly, and
it overcomes "don't break interfaces" rule, maybe now is a good time
to rework it (before it gets widely used). Anyway, let's continue
'dtimg' related discussion in dtimg thread, it's kinda off-topic here.

Thanks for detailed review!

> [*] https://patchwork.ozlabs.org/patch/1182212/#2291600
> [**] https://patchwork.kernel.org/patch/10230207/#21524437
> [***] https://patchwork.ozlabs.org/cover/1202575/
>      ("cmd: dtimg: Enhance with --id and --rev options (take #1)")
>
> --
> Best Regards,
> Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

Hi Sam,
Cc: Aleksandr, Roman

As expressed in the attached e-mail, to minimize the headaches extending
the argument list of "bootimg" in future, can we please agree on below?

On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> +U_BOOT_CMD(
> +	bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> +	"manipulate Android Boot Image",
> +	"set_addr <addr>\n"
> +	"    - set the address in RAM where boot image is located\n"
> +	"      ($loadaddr is used by default)\n"
> +	"bootimg ver <varname>\n"

Can we make <varname> optional, with the background provided in [1]?

> +	"    - get header version\n"
> +	"bootimg get_dtbo <addr_var> [size_var]\n"

How about converting <addr_var> to an optional argument too?

> +	"    - get address and size (hex) of recovery DTBO area in the image\n"
> +	"      <addr_var>: variable name to contain DTBO area address\n"
> +	"      [size_var]: variable name to contain DTBO area size\n"
> +	"bootimg dtb_dump\n"
> +	"    - print info for all files in DTB area\n"
> +	"bootimg dtb_load_addr <varname>\n"

Same as above w.r.t. <varname>.

> +	"    - get load address (hex) of DTB\n"
> +	"bootimg get_dtb_file <index> <addr_var> [size_var]\n"

How about converting <addr_var> to an optional argument too?
How do you foresee getting a blob entry based on id=<i> and/or
rev=<r>? Which of below is your favorite option?

 - get_dtb_file <index> [--id=<i>] [--rev=<r>] [addr_var] [size_var]
   where: - in case <i> and/or <r> are provided, <index> tells the
            command to pick the N's entry in the selection filtered by
            <i> and <r>
          - in case neither <i> nor <r> is provided, <index>
            behaves like in the current patch (selects a blob entry
            found at absolute index value ${index} in the image)

 - get_dtb_file [<index>|[--id=<i>|--rev=<r>]] [addr_var] [size_var]
   To make it clear, some example commands would be:
   - get_dtb_file <index>
     => current behavior
   - get_dtb_file --id=<i>
     => get _first_ blob entry matching id value <i>
   - get_dtb_file --rev=<r>
     => get _first_ blob entry matching rev value <r>
   - get_dtb_file --id=<i> --rev=<r>
     => get _first_ blob entry matching id <i> _and_ rev=<r>
   - get_dtb_file <index> --id=<i>
   - get_dtb_file <index> --rev=<r>
     => Wrong usage

 - get_dtb_file  anything else?

I think it is crucial to agree on the above, since the very first
revision of "bootimg"/"abootimg" may impose strict limitations on how
the command can be extended in future.

[just noticed your reply shedding more light on a subset of these
questions, but still sending this out; please skip if already answered]

> +	"    - get address and size (hex) of DTB file in the image\n"
> +	"      <index>: index of desired DTB file in DTB area\n"
> +	"      <addr_var>: variable name to contain DTB file address\n"
> +	"      [size_var]: variable name to contain DTB file size\n"
> +);

[1] https://patchwork.ozlabs.org/patch/1202579/
    ("cmd: dtimg: Make <varname> an optional argument")

-- 
Best Regards,
Eugeniu
-------------- next part --------------
From: Eugeniu Rosca <roscaeugeniu@gmail.com>
Date: Tue, 3 Dec 2019 14:59:46 +0100
To: Sam Protsenko <semen.protsenko@linaro.org>
CC: Eugeniu Rosca <erosca@de.adit-jv.com>, U-Boot Mailing List
	<u-boot@lists.denx.de>, Alistair Strachan <adelva@google.com>, Alex Deymo
	<deymo@google.com>, Eugeniu Rosca <roscaeugeniu@gmail.com>, Praneeth Bajjuri
	<praneeth@ti.com>, Mykhailo Sopiha <mykhailo.sopiha@linaro.org>
Subject: Re: [U-Boot] [PATCH v2 1/8] image: android: Add functions for
 handling dtb field
Message-ID: <20191203135946.GA29671@x230>
Content-Type: text/plain; charset="us-ascii"
In-Reply-To: <CAKaJLVtWbWjHc70yfJn8X6+HYUQE0p5ybgmP=uKYQb-+Wivthg@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
MIME-Version: 1.0

Hi Sam,

On Mon, Dec 02, 2019 at 07:19:53PM +0200, Sam Protsenko wrote:
> On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
> > > Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> > > image header). Provide functions for its handling:
> >
> > I believe this totally new degree of freedom brought by "Android Boot
> > Image v2" might unintentionally make some users more unhappy [0], since
> > it enables yet another way of passing a DTB to the Android kernel.
> >
> > It looks to me that there are at least three valid design choices for
> > DTB storage/passing which platform maintainers have to consider:
> >  A. Android Image second area [1-2]
> >  B. Dedicated DTB/DTBO partitions on a block device
> >  C. DTB area in Android Image v2
> >
> > While there are some major disadvantages for [A][1-2], [B] and [C] look
> > more or less equivalent and will most likely only differ in the tooling
> > used to generate and extract the useful/relevant artifacts.
> >
> > Not to mention that hybrids [B+C] are also possible.
> > Which solution should we pick as a long-term one?
> 
> My opinion is next: we should provide means (commands) to achieve any
> of [A,B,C] options. Then user (I mean, U-Boot developer who implements
> boot for each particular board) should decide which approach to use.

Fine. Thanks. But I hope you also keep in mind that the more design
choices you make available to the users (especially if redundant):
 - the more test coverage you will have to accomplish, which translates
   to more requests from Simon for test development and maintenance
 - the greater the attack surface/vector, i.e. increased likelihood for
   CVEs and the like

> Also [D] FIT image can be used instead of Android Boot Image. But we
> should remember that Google requires *mandatory* for all *new* devices
> (starting with Android 10) to stick with [C] approach only. Option [B]
> might be harder to handle from AVB verification point of view.

That's useful feedback. Thanks.

> Btw,
> when we come to "boot_android" command, I think we'll need to
> implement only officially recommended boot method. Maybe provide a
> param to choose whether to do Android-9 or Android-10 boot scheme.
> 
> Anyway, the short answer is: we should provide a bunch of isolated
> commands to allow the user to implement any boot method.

Agreed with the above warnings.

[..]

> >  - I would try to avoid wiring the same compilation unit to Kbuild
> >    (e.g. image-android-dt.o) via multiple Kconfig symbols
> >    (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes
> >    the relationship between the CONFIG symbols unclear. As a user, I
> >    would like to see a simple mapping between compiled objects and
> >    Kconfig symbols, eventually reflecting a hierarchy/dependency:
> >
> >    config ANDROID_BOOT_IMAGE
> >       select ANDROID_BOOT_IMAGE_DT
> >
> >    config DTIMG
> >       select ANDROID_BOOT_IMAGE_DT
> >
> 
> I thought about that 4 months ago when implementing this patch, even
> experimented with that. But decided to just add image-android-dt.o in
> Makefile instead, don't remember for what reason exactly. Frankly,
> don't really want to go there again and spend too much time (at least
> not in the context of this patch series).
> 
> So here is what I suggest: can we approach this one-step-at-a-time?
> This patch-set is a major step as it is, and it takes a lot of time to
> get it merged. What you suggest makes sense but might have some
> implications (even architectural) when trying to implement that. Can
> we do that in another series?

I totally agree with the following:
 - This series is a major step forward. Many thanks for this effort.
 - Bikeshedding and minor findings can be postponed.

However, for the sake of a non-chaotic git history and the right degree
of intuitiveness in the usage of all newly developed commands, I see
below review points as _major_ and better be fixed before merging:
 - The name of the "bootimg" command intrudes too much into the U-Boot
   global namespace. This has been flagged by Simon [*]
   with no reply from you yet. Do you plan to provide feedback?
 - Kconfig wiring and naming. IMHO the right decisions have to be made
   whenever new files are added to Kbuild. It is like placing the right
   pipe infrastructure into the basement of the building. Why would you
   want to make it twice? Renaming/deleting/relocating Kconfig symbols
   makes comparing [**] the configurations of distinct U-Boot/Linux
   versions pretty painful later on.
 - Agreeing on the _right_ usage scheme/pattern for any newly developed
   U-Boot command is incredibly important right from the start. What
   makes me so confident in stating that and do I have doubts about the
   current "bootimg" usage pattern? Yes, I do. That's because the
   "bootimg" arguments follow more or less the  "dtimg" usage pattern.
   My recent series [***], updating the "dtimg" Android command,
   _struggled_ with finding an intuitive way to get the DTB/DTBO based
   on user-provided "id" and/or "rev" fields, as documented in
   https://source.android.com/devices/architecture/dto/partitions . To
   give an example, making the <index> argument mandatory in
   "bootimg get_dtb_file <index> <addr_var> [size_var]" is probably not
   the best choice simply b/c searching/retrieving DTB/DTBO by absolute
   <index> in the image is just one possible search criteria. Our
   customers would _not_ like to use this criteria for obvious reasons.
   They would like to retrieve DTB/DTBO by "id" and "rev" field values.
   So, how do you interpret <index> in these latter cases if you keep
   it as a required argument? Or should we treat <index> as an optional
   parameter when "id" and/or "rev" are provided? I hope you can get a
   feeling of how important it is to agree on the usage concept for
   "bootimg" right now, not later.

[*] https://patchwork.ozlabs.org/patch/1182212/#2291600
[**] https://patchwork.kernel.org/patch/10230207/#21524437
[***] https://patchwork.ozlabs.org/cover/1202575/
     ("cmd: dtimg: Enhance with --id and --rev options (take #1)")

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

Hi Sam,

On Mon, Dec 02, 2019 at 09:07:15PM +0200, Sam Protsenko wrote:
> I'd like this patch series to be applied ASAP (probably before DTBO
> patches you mention are merged). It's been too long as it is. Once
> merged and we are unblocked w.r.t. Android boot stuff, we can then
> look into DTBO changes you mention, and if any C API are changed, we
> can change those usage overall the U-Boot tree. Hope you agree. I will
> help you guys to figure out all DTBO related changes further, but
> please let's get this patch series over with...
> 
> Thanks!

[to avoid this comment of yours unanswered]

I am ready to provide the Reviewed-by for your Android 10 series,
as soon as the review points summarized in [*] are taken care of.

[*] https://patchwork.ozlabs.org/patch/1182207/#2317118

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

Hello Sam,
Please, see one more suggestion below.

On Tue, Dec 03, 2019 at 08:29:10PM +0100, Eugeniu Rosca wrote:
> Hi Sam,
> Cc: Aleksandr, Roman
> 
> As expressed in the attached e-mail, to minimize the headaches extending
> the argument list of "bootimg" in future, can we please agree on below?
> 
> On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > +U_BOOT_CMD(
> > +	bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> > +	"manipulate Android Boot Image",
> > +	"set_addr <addr>\n"
> > +	"    - set the address in RAM where boot image is located\n"
> > +	"      ($loadaddr is used by default)\n"
> > +	"bootimg ver <varname>\n"
> 
> Can we make <varname> optional, with the background provided in [1]?
> 
> > +	"    - get header version\n"
> > +	"bootimg get_dtbo <addr_var> [size_var]\n"
> 
> How about converting <addr_var> to an optional argument too?
> 
> > +	"    - get address and size (hex) of recovery DTBO area in the image\n"
> > +	"      <addr_var>: variable name to contain DTBO area address\n"
> > +	"      [size_var]: variable name to contain DTBO area size\n"
> > +	"bootimg dtb_dump\n"
> > +	"    - print info for all files in DTB area\n"
> > +	"bootimg dtb_load_addr <varname>\n"
> 
> Same as above w.r.t. <varname>.
> 
> > +	"    - get load address (hex) of DTB\n"
> > +	"bootimg get_dtb_file <index> <addr_var> [size_var]\n"

How about "get_dte" or "get_dtbe" instead of "get_dtb_file" ?
It's shorter and should be easier to remember (dt{b}e = DT{B} Entry).

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

Hi again,

[I would be willing to take this discussion offline, if you consider it
too noisy for ML]

On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> +U_BOOT_CMD(
> +	bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> +	"manipulate Android Boot Image",
> +	"set_addr <addr>\n"
> +	"    - set the address in RAM where boot image is located\n"
> +	"      ($loadaddr is used by default)\n"
> +	"bootimg ver <varname>\n"
> +	"    - get header version\n"
> +	"bootimg get_dtbo <addr_var> [size_var]\n"
> +	"    - get address and size (hex) of recovery DTBO area in the image\n"
> +	"      <addr_var>: variable name to contain DTBO area address\n"
> +	"      [size_var]: variable name to contain DTBO area size\n"
> +	"bootimg dtb_dump\n"
> +	"    - print info for all files in DTB area\n"

I now see that "DTBO area" is used intermixed with "DTB area".
I think it makes sense to use one of the two consistently and drop the
other. Otherwise, users might think there are two distinct areas in
the same Android image.

> +	"bootimg dtb_load_addr <varname>\n"
> +	"    - get load address (hex) of DTB\n"

This is actually not something retrieved from the DTB/DTBO area, but
from the "dtb_addr" field of the Android Image itself, as defined in
https://android.googlesource.com/platform/system/tools/mkbootimg/+/refs/heads/master/include/bootimg/bootimg.h

I think this little bit of information is essential, but not sure how to
make it more transparent to the user, since purely based on the
available help message, I tend to infer that there is connection between
"DTB" in "get load address (hex) of DTB" and the "DTBO area", while
there is no connection whatsoever.

> +	"bootimg get_dtb_file <index> <addr_var> [size_var]\n"
> +	"    - get address and size (hex) of DTB file in the image\n"
> +	"      <index>: index of desired DTB file in DTB area\n"
> +	"      <addr_var>: variable name to contain DTB file address\n"
> +	"      [size_var]: variable name to contain DTB file size\n"

Would it make more sense to use "DTB entry" instead of "DTB file"
since this is the wording used in the Google spec/header?

Another general comment regarding the current sub-commands:
 - set_addr
 - ver
 - get_dtbo
 - dtb_dump
 - dtb_load_addr
 - get_dtb_file

I observe the following (inconsistent) pattern:
 - <do>_<what>
 - <what>
 - <do>_<what>
 - <what>_<do>
 - <what>_<do>_<what>
 - <do>_<what>

Looking at the "fdt" command, I find its sub-commands
somehow better partitioned and easier to digest/remember:

fdt addr [-c]  <addr> [<length>]
fdt apply <addr>
fdt move   <fdt> <newaddr> <length>
fdt resize [<extrasize>]
fdt print  <path> [<prop>]
fdt list   <path> [<prop>]
fdt get value <var> <path> <prop>
fdt get name <var> <path> <index>
fdt get addr <var> <path> <prop>
fdt get size <var> <path> [<prop>]
fdt set    <path> <prop> [<val>]
fdt mknode <path> <node>
fdt rm     <path> [<prop>]
fdt header [get <var> <member>]

Its syntax seems to be:
 <command> <do> <what> [options]

Would it make sense to borrow this naming style/principle?
It could translate to the following for abootimg:

abootimg (current sub-command name enclosed in brackets):
 - addr (set_addr)
 - ver
 - dump dtbo (dtb_dump)
 - get dtbo (get_dtbo)
 - get dtbe (get_dtb_file)
 - get dtla (dtb_load_addr)

> +);

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

Hi,

On Tue, Dec 3, 2019 at 9:29 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
> Cc: Aleksandr, Roman
>
> As expressed in the attached e-mail, to minimize the headaches extending
> the argument list of "bootimg" in future, can we please agree on below?
>
> On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > +U_BOOT_CMD(
> > +     bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> > +     "manipulate Android Boot Image",
> > +     "set_addr <addr>\n"
> > +     "    - set the address in RAM where boot image is located\n"
> > +     "      ($loadaddr is used by default)\n"
> > +     "bootimg ver <varname>\n"
>
> Can we make <varname> optional, with the background provided in [1]?
>

Already done in v3 (will send soon).

> > +     "    - get header version\n"
> > +     "bootimg get_dtbo <addr_var> [size_var]\n"
>
> How about converting <addr_var> to an optional argument too?
>

Already done in v3.

> > +     "    - get address and size (hex) of recovery DTBO area in the image\n"
> > +     "      <addr_var>: variable name to contain DTBO area address\n"
> > +     "      [size_var]: variable name to contain DTBO area size\n"
> > +     "bootimg dtb_dump\n"
> > +     "    - print info for all files in DTB area\n"
> > +     "bootimg dtb_load_addr <varname>\n"
>
> Same as above w.r.t. <varname>.
>

Already done in v3.

> > +     "    - get load address (hex) of DTB\n"
> > +     "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
>
> How about converting <addr_var> to an optional argument too?

Already done in v3.

> How do you foresee getting a blob entry based on id=<i> and/or
> rev=<r>? Which of below is your favorite option?
>
>  - get_dtb_file <index> [--id=<i>] [--rev=<r>] [addr_var] [size_var]
>    where: - in case <i> and/or <r> are provided, <index> tells the
>             command to pick the N's entry in the selection filtered by
>             <i> and <r>
>           - in case neither <i> nor <r> is provided, <index>
>             behaves like in the current patch (selects a blob entry
>             found at absolute index value ${index} in the image)
>
>  - get_dtb_file [<index>|[--id=<i>|--rev=<r>]] [addr_var] [size_var]
>    To make it clear, some example commands would be:
>    - get_dtb_file <index>
>      => current behavior
>    - get_dtb_file --id=<i>
>      => get _first_ blob entry matching id value <i>
>    - get_dtb_file --rev=<r>
>      => get _first_ blob entry matching rev value <r>
>    - get_dtb_file --id=<i> --rev=<r>
>      => get _first_ blob entry matching id <i> _and_ rev=<r>
>    - get_dtb_file <index> --id=<i>
>    - get_dtb_file <index> --rev=<r>
>      => Wrong usage
>
>  - get_dtb_file  anything else?
>

I already came up with next usage:

    abootimg get_dtb_file index <num> [addr_var] [size_var]

It's already done in v3. Once your patch series is merged, I will add
next two sub-commands:

    abootimg get_dtb_file id    <num> [addr_var] [size_var]
    abootimg get_dtb_file rev   <num> [addr_var] [size_var]

This way it's extensible. Also please see my review for your patches,
esp. for patch 4/4, where I discuss similar matter in context of
'dtimg' command.

> I think it is crucial to agree on the above, since the very first
> revision of "bootimg"/"abootimg" may impose strict limitations on how
> the command can be extended in future.
>

All those are addressed in v3 now. Along with renaming: 'bootimg' -> 'abootimg'.

The only thing remains to be addressed is Kconfig/Makefile decisions
you mentioned. Will look into that tomorrow, and either make it as you
pointed out or explain why it's good as it is.

> [just noticed your reply shedding more light on a subset of these
> questions, but still sending this out; please skip if already answered]
>
> > +     "    - get address and size (hex) of DTB file in the image\n"
> > +     "      <index>: index of desired DTB file in DTB area\n"
> > +     "      <addr_var>: variable name to contain DTB file address\n"
> > +     "      [size_var]: variable name to contain DTB file size\n"
> > +);
>
> [1] https://patchwork.ozlabs.org/patch/1202579/
>     ("cmd: dtimg: Make <varname> an optional argument")
>
> --
> Best Regards,
> Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

Hi,

On Wed, Dec 4, 2019 at 7:33 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hello Sam,
> Please, see one more suggestion below.
>
> On Tue, Dec 03, 2019 at 08:29:10PM +0100, Eugeniu Rosca wrote:
> > Hi Sam,
> > Cc: Aleksandr, Roman
> >
> > As expressed in the attached e-mail, to minimize the headaches extending
> > the argument list of "bootimg" in future, can we please agree on below?
> >
> > On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > > +U_BOOT_CMD(
> > > +   bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> > > +   "manipulate Android Boot Image",
> > > +   "set_addr <addr>\n"
> > > +   "    - set the address in RAM where boot image is located\n"
> > > +   "      ($loadaddr is used by default)\n"
> > > +   "bootimg ver <varname>\n"
> >
> > Can we make <varname> optional, with the background provided in [1]?
> >
> > > +   "    - get header version\n"
> > > +   "bootimg get_dtbo <addr_var> [size_var]\n"
> >
> > How about converting <addr_var> to an optional argument too?
> >
> > > +   "    - get address and size (hex) of recovery DTBO area in the image\n"
> > > +   "      <addr_var>: variable name to contain DTBO area address\n"
> > > +   "      [size_var]: variable name to contain DTBO area size\n"
> > > +   "bootimg dtb_dump\n"
> > > +   "    - print info for all files in DTB area\n"
> > > +   "bootimg dtb_load_addr <varname>\n"
> >
> > Same as above w.r.t. <varname>.
> >
> > > +   "    - get load address (hex) of DTB\n"
> > > +   "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
>
> How about "get_dte" or "get_dtbe" instead of "get_dtb_file" ?
> It's shorter and should be easier to remember (dt{b}e = DT{B} Entry).
>

Sorry, I like get_dtb more. It's .dtb file in the end, and it's called
exactly "dtb" in boot.img struct. So this is a keeper :)

> --
> Best Regards,
> Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

Hi,

On Wed, Dec 4, 2019 at 8:25 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi again,
>
> [I would be willing to take this discussion offline, if you consider it
> too noisy for ML]
>

Agreed. Please find me on freenode IRC, nick: joeskb7. There is
#u-boot channel, or #linaro-android, whichever suits you best.

> On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > +U_BOOT_CMD(
> > +     bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> > +     "manipulate Android Boot Image",
> > +     "set_addr <addr>\n"
> > +     "    - set the address in RAM where boot image is located\n"
> > +     "      ($loadaddr is used by default)\n"
> > +     "bootimg ver <varname>\n"
> > +     "    - get header version\n"
> > +     "bootimg get_dtbo <addr_var> [size_var]\n"
> > +     "    - get address and size (hex) of recovery DTBO area in the image\n"
> > +     "      <addr_var>: variable name to contain DTBO area address\n"
> > +     "      [size_var]: variable name to contain DTBO area size\n"
> > +     "bootimg dtb_dump\n"
> > +     "    - print info for all files in DTB area\n"
>
> I now see that "DTBO area" is used intermixed with "DTB area".
> I think it makes sense to use one of the two consistently and drop the
> other. Otherwise, users might think there are two distinct areas in
> the same Android image.
>

Those are, in fact, two different areas in boot.img. "DTBO area" is a
payload in boot.img where recovery dtbo file can be placed. "DTB area"
is a payload in boot.img where DTB files can be placed (either
concatenated or wrapped into DTBO image format).

> > +     "bootimg dtb_load_addr <varname>\n"
> > +     "    - get load address (hex) of DTB\n"
>
> This is actually not something retrieved from the DTB/DTBO area, but
> from the "dtb_addr" field of the Android Image itself, as defined in
> https://android.googlesource.com/platform/system/tools/mkbootimg/+/refs/heads/master/include/bootimg/bootimg.h
>
> I think this little bit of information is essential, but not sure how to
> make it more transparent to the user, since purely based on the
> available help message, I tend to infer that there is connection between
> "DTB" in "get load address (hex) of DTB" and the "DTBO area", while
> there is no connection whatsoever.
>

Let's keep command usage length reasonable. We are bloating U-Boot
footprint too much as it is already... I think user shouldn't care
where the load address is obtained from, really. Prefer to keep it
short, as it is.

> > +     "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
> > +     "    - get address and size (hex) of DTB file in the image\n"
> > +     "      <index>: index of desired DTB file in DTB area\n"
> > +     "      <addr_var>: variable name to contain DTB file address\n"
> > +     "      [size_var]: variable name to contain DTB file size\n"
>
> Would it make more sense to use "DTB entry" instead of "DTB file"
> since this is the wording used in the Google spec/header?
>

I'd argue that "DTB file" is more clear for user, than "entry". If you
don't have a strong preference on this one, let's keep it as is.

> Another general comment regarding the current sub-commands:
>  - set_addr
>  - ver
>  - get_dtbo
>  - dtb_dump
>  - dtb_load_addr
>  - get_dtb_file
>
> I observe the following (inconsistent) pattern:
>  - <do>_<what>
>  - <what>
>  - <do>_<what>
>  - <what>_<do>
>  - <what>_<do>_<what>
>  - <do>_<what>
>
> Looking at the "fdt" command, I find its sub-commands
> somehow better partitioned and easier to digest/remember:
>
> fdt addr [-c]  <addr> [<length>]
> fdt apply <addr>
> fdt move   <fdt> <newaddr> <length>
> fdt resize [<extrasize>]
> fdt print  <path> [<prop>]
> fdt list   <path> [<prop>]
> fdt get value <var> <path> <prop>
> fdt get name <var> <path> <index>
> fdt get addr <var> <path> <prop>
> fdt get size <var> <path> [<prop>]
> fdt set    <path> <prop> [<val>]
> fdt mknode <path> <node>
> fdt rm     <path> [<prop>]
> fdt header [get <var> <member>]
>
> Its syntax seems to be:
>  <command> <do> <what> [options]
>
> Would it make sense to borrow this naming style/principle?
> It could translate to the following for abootimg:
>
> abootimg (current sub-command name enclosed in brackets):
>  - addr (set_addr)
>  - ver
>  - dump dtbo (dtb_dump)
>  - get dtbo (get_dtbo)
>  - get dtbe (get_dtb_file)
>  - get dtla (dtb_load_addr)
>

Makes sense. I'll think about it.

Thanks for review!

> > +);
>
> --
> Best Regards,
> Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Aleksandr Bulyshchenko]

Hello Sam,

I'd like to add my 5 cents regarding separating dtimg start|size into 3
subcommands

> dtimg start index <num> <addr> [varname]
> dtimg start id <num> <addr> [varname]
> dtimg start rev <num> <addr> [varname]
>
> While I don't see real usecases for combining index with id or rev (if
someone applies metainformation to dtb entries for meaningful lookup,
identical entries most probably mean copy-paste error),
but at the same time I see space for at least two-factor identification
(e.g. model and revision).
Thus API should allow (but not require) combining id and rev.

The same remains relevant for abootimg as well.

Thanks,
Aleksandr Bulyshchenko

On Wed, Dec 4, 2019 at 9:12 PM Sam Protsenko <semen.protsenko@linaro.org>
wrote:

> Hi,
>
> On Wed, Dec 4, 2019 at 7:33 PM Eugeniu Rosca <erosca@de.adit-jv.com>
> wrote:
> >
> > Hello Sam,
> > Please, see one more suggestion below.
> >
> > On Tue, Dec 03, 2019 at 08:29:10PM +0100, Eugeniu Rosca wrote:
> > > Hi Sam,
> > > Cc: Aleksandr, Roman
> > >
> > > As expressed in the attached e-mail, to minimize the headaches
> extending
> > > the argument list of "bootimg" in future, can we please agree on below?
> > >
> > > On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > > > +U_BOOT_CMD(
> > > > +   bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> > > > +   "manipulate Android Boot Image",
> > > > +   "set_addr <addr>\n"
> > > > +   "    - set the address in RAM where boot image is located\n"
> > > > +   "      ($loadaddr is used by default)\n"
> > > > +   "bootimg ver <varname>\n"
> > >
> > > Can we make <varname> optional, with the background provided in [1]?
> > >
> > > > +   "    - get header version\n"
> > > > +   "bootimg get_dtbo <addr_var> [size_var]\n"
> > >
> > > How about converting <addr_var> to an optional argument too?
> > >
> > > > +   "    - get address and size (hex) of recovery DTBO area in the
> image\n"
> > > > +   "      <addr_var>: variable name to contain DTBO area address\n"
> > > > +   "      [size_var]: variable name to contain DTBO area size\n"
> > > > +   "bootimg dtb_dump\n"
> > > > +   "    - print info for all files in DTB area\n"
> > > > +   "bootimg dtb_load_addr <varname>\n"
> > >
> > > Same as above w.r.t. <varname>.
> > >
> > > > +   "    - get load address (hex) of DTB\n"
> > > > +   "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
> >
> > How about "get_dte" or "get_dtbe" instead of "get_dtb_file" ?
> > It's shorter and should be easier to remember (dt{b}e = DT{B} Entry).
> >
>
> Sorry, I like get_dtb more. It's .dtb file in the end, and it's called
> exactly "dtb" in boot.img struct. So this is a keeper :)
>
> > --
> > Best Regards,
> > Eugeniu
>

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

On Thu, Dec 05, 2019 at 04:17:38AM +0200, Aleksandr Bulyshchenko wrote:
>    Hello Sam,
>    I'd like to add my 5 cents regarding separating dtimg start|size into 3
>    subcommands
> 
>  dtimg start index <num> <addr> [varname]
>  dtimg start id <num> <addr> [varname]
>  dtimg start rev <num> <addr> [varname]
> 
>    While I don't see real usecases for combining index with id or rev (if
>    someone applies metainformation to dtb entries for meaningful lookup,
>    identical entries most probably mean copy-paste error),
>    but at the same time I see space for at least two-factor identification
>    (e.g. model and revision).
>    Thus API should allow (but not require) combining id and rev.
>    The same remains relevant for abootimg as well.

[shameless plug] Agreed with Aleksandr. Users will want to provide <id>
and <rev> (and possibly cust[0-4] in future) in one go/command launch.

-- 
Best Regards,
Eugeniu

[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

Hi Aleksandr,

On Thu, Dec 5, 2019 at 4:17 AM Aleksandr Bulyshchenko
<a.bulyshchenko@globallogic.com> wrote:
>
> Hello Sam,
>
> I'd like to add my 5 cents regarding separating dtimg start|size into 3 subcommands
>>
>> dtimg start index <num> <addr> [varname]
>> dtimg start id <num> <addr> [varname]
>> dtimg start rev <num> <addr> [varname]
>
> While I don't see real usecases for combining index with id or rev (if someone applies metainformation to dtb entries for meaningful lookup, identical entries most probably mean copy-paste error),
> but at the same time I see space for at least two-factor identification (e.g. model and revision).
> Thus API should allow (but not require) combining id and rev.
>

Agreed on id+rev usage. Can we still try and keep the interface as
simple as possible, e.g. like this:

    dtimg start index <num> <addr> [varname]
    dtimg start id <id_num> <rev_num> <custom_num> <addr> [varname]

In case when user wants to use only "id" or only  "rev", other bit
should be specified as "-":

    dtimg start id 10 - - <addr> [varname]
    dtimg start id - 10 - <addr> [varname]

It's similar to what is done in 'bootm' command:

    To boot that kernel without an initrd image,use a '-' for the
second argument.

This way we can keep away the 'index' usage, making two things possible:
  1. Code will be easier (we can provide one function for 'index' case
and one function for 'id/rev/custom' case)
  2. Easier for us to split the work and avoid dependencies between
our patch series

If everyone agrees on usage I suggested above, we can go ahead and
change it correspondingly for 'dtimg' and 'abootimg' patch series.

> The same remains relevant for abootimg as well.
>
> Thanks,
> Aleksandr Bulyshchenko
>
> On Wed, Dec 4, 2019 at 9:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>>
>> Hi,
>>
>> On Wed, Dec 4, 2019 at 7:33 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>> >
>> > Hello Sam,
>> > Please, see one more suggestion below.
>> >
>> > On Tue, Dec 03, 2019 at 08:29:10PM +0100, Eugeniu Rosca wrote:
>> > > Hi Sam,
>> > > Cc: Aleksandr, Roman
>> > >
>> > > As expressed in the attached e-mail, to minimize the headaches extending
>> > > the argument list of "bootimg" in future, can we please agree on below?
>> > >
>> > > On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
>> > > > +U_BOOT_CMD(
>> > > > +   bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
>> > > > +   "manipulate Android Boot Image",
>> > > > +   "set_addr <addr>\n"
>> > > > +   "    - set the address in RAM where boot image is located\n"
>> > > > +   "      ($loadaddr is used by default)\n"
>> > > > +   "bootimg ver <varname>\n"
>> > >
>> > > Can we make <varname> optional, with the background provided in [1]?
>> > >
>> > > > +   "    - get header version\n"
>> > > > +   "bootimg get_dtbo <addr_var> [size_var]\n"
>> > >
>> > > How about converting <addr_var> to an optional argument too?
>> > >
>> > > > +   "    - get address and size (hex) of recovery DTBO area in the image\n"
>> > > > +   "      <addr_var>: variable name to contain DTBO area address\n"
>> > > > +   "      [size_var]: variable name to contain DTBO area size\n"
>> > > > +   "bootimg dtb_dump\n"
>> > > > +   "    - print info for all files in DTB area\n"
>> > > > +   "bootimg dtb_load_addr <varname>\n"
>> > >
>> > > Same as above w.r.t. <varname>.
>> > >
>> > > > +   "    - get load address (hex) of DTB\n"
>> > > > +   "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
>> >
>> > How about "get_dte" or "get_dtbe" instead of "get_dtb_file" ?
>> > It's shorter and should be easier to remember (dt{b}e = DT{B} Entry).
>> >
>>
>> Sorry, I like get_dtb more. It's .dtb file in the end, and it's called
>> exactly "dtb" in boot.img struct. So this is a keeper :)
>>
>> > --
>> > Best Regards,
>> > Eugeniu