[PATCH v3 0/9] am57xx: Implement Android 10 boot flow

[PATCH v3 0/9] 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 (see "abootimg" command and associated C API).

This patch series must be applied on top of these recently sent patches
by Eugeniu:

    [PATCH 0/3] cmd: dtimg: Rename to adtimg and refactor usage style

Changes in v3:
 - rename command to "abootimg" (requested by Simon Glass)
 - rework command interface (as discussed with Eugeniu)
 - add command documentation
 - address other comments

[1] https://source.android.com/devices/bootloader

Sam Protsenko (9):
  image: android: Add functions for handling dtb field
  image: android: Add routine to get dtbo params
  cmd: abootimg: Add abootimg command
  doc: android: Add documentation for Android Boot Image
  test/py: android: Add test for abootimg
  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                                 |  10 +
 cmd/Makefile                                |   1 +
 cmd/abootimg.c                              | 242 +++++++++++++++++
 common/Makefile                             |   2 +-
 common/image-android.c                      | 275 ++++++++++++++++++++
 configs/am57xx_evm_defconfig                |   6 +
 configs/am57xx_hs_evm_defconfig             |   6 +
 configs/am57xx_hs_evm_usb_defconfig         |   6 +
 configs/sandbox_defconfig                   |   1 +
 doc/android/boot-image.rst                  | 154 +++++++++++
 include/configs/ti_armv7_common.h           |   7 +
 include/environment/ti/boot.h               | 146 ++++++-----
 include/image.h                             |   6 +
 test/py/tests/test_android/test_abootimg.py | 159 +++++++++++
 14 files changed, 954 insertions(+), 67 deletions(-)
 create mode 100644 cmd/abootimg.c
 create mode 100644 doc/android/boot-image.rst
 create mode 100644 test/py/tests/test_android/test_abootimg.py

-- 
2.24.0

[PATCH v3 1/9] 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 blob from "DTB" part of
    boot image, by blob's index
  - android_image_print_dtb_contents(): Iterate over all DTB blobs in
    "DTB" part of boot image and print those blobs info

"DTB" payload might be in one of the following formats:
  1. concatenated DTB blobs
  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 blob(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 "abootimg" to extract dtb blob
    from boot image (using functions from this patch)
  - extract desired dtbo blobs from "dtbo" partition using "adtimg"
    command
  - merge dtbo blobs into dtb blob using "fdt apply" command
  - pass resulting dtb blob 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>
---
 common/Makefile        |   2 +-
 common/image-android.c | 214 +++++++++++++++++++++++++++++++++++++++++
 include/image.h        |   5 +
 3 files changed, 220 insertions(+), 1 deletion(-)

diff --git a/common/Makefile b/common/Makefile
index 029cc0f2ce..1ffddc2f94 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..1ccad6c556 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,121 @@ 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);
+
+	*addr = dtb_img_addr;
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
+/**
+ * android_image_get_dtb_by_index() - Get address and size of blob 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 blob 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 blob 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 +363,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 blobs 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 blobs
+ *   2. Android DTBO format (see CONFIG_CMD_ADTIMG for details)
+ *
+ * This function does next:
+ *   1. Prints out the format used in DTB area
+ *   2. Iterates over all DTB blobs in DTB area and prints out the info for
+ *      each blob.
+ *
+ * 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 blob 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 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;
+		}
+
+		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 f4d2aaf53e..8e81166be4 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.24.0

[PATCH v3 2/9] 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 'abootimg' command to provide the user a way
to get the address of recovery dtbo from U-Boot shell, which can be
further parsed using 'adtimg' 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>
---
 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 1ccad6c556..5d6669ceab 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 'adtimg' U-Boot
+ * command or android_dt_*() functions to extract desired DTBO blob.
+ *
+ * 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 8e81166be4..b8d821605b 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.24.0

[PATCH v3 3/9] cmd: abootimg: Add abootimg 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 blob 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 (for non-A/B devices only,
see [1,2] for details).

It can be tested like this:

    => mmc dev 1
    => part start mmc 1 boot_a boot_start
    => part size mmc 1 boot_a boot_size
    => mmc read $loadaddr $boot_start $boot_size
    => abootimg get ver
    => abootimg dump dtb

[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>
---
 cmd/Kconfig    |   8 ++
 cmd/Makefile   |   1 +
 cmd/abootimg.c | 242 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 251 insertions(+)
 create mode 100644 cmd/abootimg.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index f63adbdc3a..e25ac7ee57 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -356,6 +356,14 @@ config CMD_ADTIMG
 	  files should be merged in one dtb further, which needs to be passed to
 	  the kernel, as part of a boot process.
 
+config CMD_ABOOTIMG
+	bool "abootimg"
+	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 c17ee20b25..8ccd8c7b87 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -48,6 +48,7 @@ ifdef CONFIG_POST
 obj-$(CONFIG_CMD_DIAG) += diag.o
 endif
 obj-$(CONFIG_CMD_ADTIMG) += adtimg.o
+obj-$(CONFIG_CMD_ABOOTIMG) += abootimg.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/abootimg.c b/cmd/abootimg.c
new file mode 100644
index 0000000000..d4675c275b
--- /dev/null
+++ b/cmd/abootimg.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2019 Linaro Ltd.
+ * Sam Protsenko <semen.protsenko@linaro.org>
+ */
+
+#include <android_image.h>
+#include <common.h>
+#include <mapmem.h>
+
+#define abootimg_addr() (_abootimg_addr == -1 ? load_addr : _abootimg_addr)
+
+/* Please use abootimg_addr() macro to obtain the boot image address */
+static ulong _abootimg_addr = -1;
+
+static int abootimg_get_ver(int argc, char * const argv[])
+{
+	const struct andr_img_hdr *hdr;
+	int res = CMD_RET_SUCCESS;
+
+	if (argc > 1)
+		return CMD_RET_USAGE;
+
+	hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
+	if (android_image_check_header(hdr)) {
+		printf("Error: Boot Image header is incorrect\n");
+		res = CMD_RET_FAILURE;
+		goto exit;
+	}
+
+	if (argc == 0)
+		printf("%u\n", hdr->header_version);
+	else
+		env_set_ulong(argv[0], hdr->header_version);
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
+static int abootimg_get_recovery_dtbo(int argc, char * const argv[])
+{
+	ulong addr;
+	u32 size;
+
+	if (argc > 2)
+		return CMD_RET_USAGE;
+
+	if (!android_image_get_dtbo(abootimg_addr(), &addr, &size))
+		return CMD_RET_FAILURE;
+
+	if (argc == 0) {
+		printf("%lx\n", addr);
+	} else {
+		env_set_hex(argv[0], addr);
+		if (argc == 2)
+			env_set_hex(argv[1], size);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int abootimg_get_dtb_load_addr(int argc, char * const argv[])
+{
+	const struct andr_img_hdr *hdr;
+	int res = CMD_RET_SUCCESS;
+
+	if (argc > 1)
+		return CMD_RET_USAGE;
+
+	hdr = map_sysmem(abootimg_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;
+	}
+
+	if (argc == 0)
+		printf("%lx\n", (ulong)hdr->dtb_addr);
+	else
+		env_set_hex(argv[0], (ulong)hdr->dtb_addr);
+
+exit:
+	unmap_sysmem(hdr);
+	return res;
+}
+
+static int abootimg_get_dtb_by_index(int argc, char * const argv[])
+{
+	u32 num;
+	char *endp;
+	ulong addr;
+	u32 size;
+
+	if (argc < 1 || argc > 3)
+		return CMD_RET_USAGE;
+
+	num = simple_strtoul(argv[0] + strlen("--index="), &endp, 0);
+	if (*endp != '\0') {
+		printf("Error: Wrong num\n");
+		return CMD_RET_FAILURE;
+	}
+
+	if (!android_image_get_dtb_by_index(abootimg_addr(), num,
+					    &addr, &size)) {
+		return CMD_RET_FAILURE;
+	}
+
+	if (argc == 1) {
+		printf("%lx\n", addr);
+	} else {
+		env_set_hex(argv[1], addr);
+		if (argc == 3)
+			env_set_hex(argv[2], size);
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static int abootimg_get_dtb(int argc, char * const argv[])
+{
+	if (argc < 1)
+		return CMD_RET_USAGE;
+
+	if (strstr(argv[0], "--index="))
+		return abootimg_get_dtb_by_index(argc, argv);
+
+	return CMD_RET_USAGE;
+}
+
+static int do_abootimg_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;
+	}
+
+	_abootimg_addr = img_addr;
+	return CMD_RET_SUCCESS;
+}
+
+static int do_abootimg_get(cmd_tbl_t *cmdtp, int flag, int argc,
+			   char * const argv[])
+{
+	const char *param;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	param = argv[1];
+	argc -= 2;
+	argv += 2;
+	if (!strcmp(param, "ver"))
+		return abootimg_get_ver(argc, argv);
+	else if (!strcmp(param, "recovery_dtbo"))
+		return abootimg_get_recovery_dtbo(argc, argv);
+	else if (!strcmp(param, "dtb_load_addr"))
+		return abootimg_get_dtb_load_addr(argc, argv);
+	else if (!strcmp(param, "dtb"))
+		return abootimg_get_dtb(argc, argv);
+
+	return CMD_RET_USAGE;
+}
+
+static int do_abootimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+			    char * const argv[])
+{
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	if (!strcmp(argv[1], "dtb")) {
+		if (android_image_print_dtb_contents(abootimg_addr()))
+			return CMD_RET_FAILURE;
+	} else {
+		return CMD_RET_USAGE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static cmd_tbl_t cmd_abootimg_sub[] = {
+	U_BOOT_CMD_MKENT(addr, 2, 1, do_abootimg_addr, "", ""),
+	U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""),
+	U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""),
+};
+
+static int do_abootimg(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
+{
+	cmd_tbl_t *cp;
+
+	cp = find_cmd_tbl(argv[1], cmd_abootimg_sub,
+			  ARRAY_SIZE(cmd_abootimg_sub));
+
+	/* Strip off leading 'abootimg' 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(
+	abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg,
+	"manipulate Android Boot Image",
+	"addr <addr>\n"
+	"    - set the address in RAM where boot image is located\n"
+	"      ($loadaddr is used by default)\n"
+	"abootimg dump dtb\n"
+	"    - print info for all DT blobs in DTB area\n"
+	"abootimg get ver [varname]\n"
+	"    - get header version\n"
+	"abootimg get recovery_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"
+	"abootimg get dtb_load_addr [varname]\n"
+	"    - get load address (hex) of DTB, from image header\n"
+	"abootimg get dtb --index=<num> [addr_var [size_var]]\n"
+	"    - get address and size (hex) of DT blob in the image by index\n"
+	"      <num>: index number of desired DT blob in DTB area\n"
+	"      [addr_var]: variable name to contain DT blob address\n"
+	"      [size_var]: variable name to contain DT blob size\n"
+);
-- 
2.24.0

[PATCH v3 4/9] doc: android: Add documentation for Android Boot Image

[Sam Protsenko]

Describe Android Boot Image format, how its support is implemented in
U-Boot and associated commands usage.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 cmd/Kconfig                |   2 +
 doc/android/boot-image.rst | 154 +++++++++++++++++++++++++++++++++++++
 2 files changed, 156 insertions(+)
 create mode 100644 doc/android/boot-image.rst

diff --git a/cmd/Kconfig b/cmd/Kconfig
index e25ac7ee57..18c1db3fd4 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -364,6 +364,8 @@ config CMD_ABOOTIMG
 	  images contained in boot.img, like kernel, ramdisk, dtb, etc, and
 	  obtain corresponding meta-information from boot.img.
 
+	  See doc/android/boot-image.rst for details.
+
 config CMD_ELF
 	bool "bootelf, bootvx"
 	default y
diff --git a/doc/android/boot-image.rst b/doc/android/boot-image.rst
new file mode 100644
index 0000000000..74937e4bfa
--- /dev/null
+++ b/doc/android/boot-image.rst
@@ -0,0 +1,154 @@
+.. SPDX-License-Identifier: GPL-2.0+
+.. sectionauthor:: Sam Protsenko <semen.protsenko@linaro.org>
+
+Android Boot Image
+==================
+
+Overview
+--------
+
+Android Boot Image is used to boot Android OS. It usually contains kernel image
+(like ``zImage`` file) and ramdisk. Sometimes it can contain additional
+binaries. This image is built as a part of AOSP (called ``boot.img``), and being
+flashed into ``boot`` partition on eMMC. Bootloader then reads that image from
+``boot`` partition to RAM and boots the kernel from it. Kernel than starts
+``init`` process from the ramdisk. It should be mentioned that recovery image
+(``recovery.img``) also has Android Boot Image format.
+
+Android Boot Image format is described at [1]_. At the moment it can have one of
+next image headers:
+
+* v0: it's called *legacy* boot image header; used in devices launched before
+  Android 9; contains kernel image, ramdisk and second stage bootloader
+  (usually unused)
+* v1: used in devices launched with Android 9; adds ``recovery_dtbo`` field,
+  which should be used for non-A/B devices in ``recovery.img`` (see [2]_ for
+  details)
+* v2: used in devices launched with Android 10; adds ``dtb`` field, which
+  references payload containing DTB blobs (either concatenated one after the
+  other, or in Android DTBO image format)
+
+v2, v1 and v0 formats are backward compatible.
+
+Android Boot Image format is represented by :c:type:`struct andr_img_hdr` in
+U-Boot, and can be seen in ``include/android_image.h``. U-Boot supports booting
+Android Boot Image and also has associated command
+
+Booting
+-------
+
+U-Boot is able to boot the Android OS from Android Boot Image using ``bootm``
+command. In order to use Android Boot Image format support, next option should
+be enabled::
+
+    CONFIG_ANDROID_BOOT_IMAGE=y
+
+Then one can use next ``bootm`` command call to run Android:
+
+.. code-block:: bash
+
+    => bootm $loadaddr $loadaddr $fdtaddr
+
+where ``$loadaddr`` - address in RAM where boot image was loaded; ``$fdtaddr`` -
+address in RAM where DTB blob was loaded.
+
+And parameters are, correspondingly:
+
+  1. Where kernel image is located in RAM
+  2. Where ramdisk is located in RAM (can be ``"-"`` if not applicable)
+  3. Where DTB blob is located in RAM
+
+``bootm`` command will figure out that image located in ``$loadaddr`` has
+Android Boot Image format, will parse that and boot the kernel from it,
+providing DTB blob to kernel (from 3rd parameter), passing info about ramdisk to
+kernel via DTB.
+
+DTB and DTBO blobs
+------------------
+
+``bootm`` command can't just use DTB blob from Android Boot Image (``dtb``
+field), because:
+
+* there is no DTB area in Android Boot Image before v2
+* there may be several DTB blobs in DTB area (e.g. for different SoCs)
+* some DTBO blobs may have to be merged in DTB blobs before booting
+  (e.g. for different boards)
+
+So user has to prepare DTB blob manually and provide it in a 3rd parameter
+of ``bootm`` command. Next commands can be used to do so:
+
+1. ``abootimg``: manipulates Anroid Boot Image, allows one to extract
+   meta-information and payloads from it
+2. ``adtimg``: manipulates Android DTB/DTBO image [3]_, allows one to extract
+   DTB/DTBO blobs from it
+
+In order to use those, please enable next config options::
+
+    CONFIG_CMD_ABOOTIMG=y
+    CONFIG_CMD_ADTIMG=y
+
+For example, let's assume we have next Android partitions on eMMC:
+
+* ``boot``: contains Android Boot Image v2 (including DTB blobs)
+* ``dtbo``: contains DTBO blobs
+
+Then next command sequence can be used to boot Android:
+
+.. code-block:: bash
+
+    => mmc dev 1
+
+       # Read boot image to RAM (into $loadaddr)
+    => part start mmc 1 boot boot_start
+    => part size mmc 1 boot boot_size
+    => mmc read $loadaddr $boot_start $boot_size
+
+       # Read DTBO image to RAM (into $dtboaddr)
+    => part start mmc 1 dtbo dtbo_start
+    => part size mmc 1 dtbo dtbo_size
+    => mmc read $dtboaddr $dtbo_start $dtbo_size
+
+       # Copy required DTB blob (into $fdtaddr)
+    => abootimg get dtb --index=0 dtb0_start dtb0_size
+    => cp.b $dtb0_start $fdtaddr $dtb0_size
+
+       # Merge required DTBO blobs into DTB blob
+    => fdt addr $fdtaddr 0x100000
+    => adtimg addr $dtboaddr
+    => adtimg get dt --index=0 $dtbo0_addr
+    => fdt apply $dtbo0_addr
+
+       # Boot Android
+    => bootm $loadaddr $loadaddr $fdtaddr
+
+This sequence should be used for Android 10 boot. Of course, the whole Android
+boot procedure includes much more actions, like:
+
+* obtaining reboot reason from BCB (see [4]_)
+* implementing recovery boot
+* implementing fastboot boot
+* implementing A/B slotting (see [5]_)
+* implementing AVB2.0 (see [6]_)
+
+But Android Boot Image booting is the most crucial part in Android boot scheme.
+
+All Android bootloader requirements documentation is available at [7]_. Some
+overview on the whole Android 10 boot process can be found at [8]_.
+
+C API for working with Android Boot Image format
+------------------------------------------------
+
+.. kernel-doc:: common/image-android.c
+   :internal:
+
+References
+----------
+
+.. [1] https://source.android.com/devices/bootloader/boot-image-header
+.. [2] https://source.android.com/devices/bootloader/recovery-image
+.. [3] https://source.android.com/devices/architecture/dto/partitions
+.. [4] :doc:`bcb`
+.. [5] :doc:`ab`
+.. [6] :doc:`avb2`
+.. [7] https://source.android.com/devices/bootloader
+.. [8] https://connect.linaro.org/resources/san19/san19-217/
-- 
2.24.0

[PATCH v3 5/9] test/py: android: Add test for abootimg

[Sam Protsenko]

Unit test for 'abootimg' 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_abootimg

shows that 1/1 tests passes successfully.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 configs/sandbox_defconfig                   |   1 +
 test/py/tests/test_android/test_abootimg.py | 159 ++++++++++++++++++++
 2 files changed, 160 insertions(+)
 create mode 100644 test/py/tests/test_android/test_abootimg.py

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 64245f7cdc..df5aad54bf 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -25,6 +25,7 @@ CONFIG_ANDROID_AB=y
 CONFIG_CMD_CPU=y
 CONFIG_CMD_LICENSE=y
 CONFIG_CMD_BOOTZ=y
+CONFIG_CMD_ABOOTIMG=y
 # CONFIG_CMD_ELF is not set
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
diff --git a/test/py/tests/test_android/test_abootimg.py b/test/py/tests/test_android/test_abootimg.py
new file mode 100644
index 0000000000..8c0939afe9
--- /dev/null
+++ b/test/py/tests/test_android/test_abootimg.py
@@ -0,0 +1,159 @@
+# SPDX-License-Identifier:  GPL-2.0+
+# Copyright (c) 2019, Linaro Limited
+# Author: Sam Protsenko <semen.protsenko@linaro.org>
+
+# Test U-Boot's "abootimg" 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 "abootimg 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 ('abootimg' 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 AbootimgTestDiskImage(object):
+    """Disk image used by abootimg tests."""
+
+    def __init__(self, u_boot_console):
+        """Initialize a new AbootimgDiskImage 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 abootimg_disk_image(u_boot_console):
+    """pytest fixture to provide a AbootimgTestDiskImage 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 = AbootimgTestDiskImage(u_boot_console)
+    return gtdi
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.buildconfigspec('android_boot_image')
+ at pytest.mark.buildconfigspec('cmd_abootimg')
+ at pytest.mark.buildconfigspec('cmd_fdt')
+ at pytest.mark.requiredtool('xxd')
+ at pytest.mark.requiredtool('gunzip')
+def test_abootimg(abootimg_disk_image, u_boot_console):
+    """Test the 'abootimg' 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,
+        abootimg_disk_image.path))
+
+    u_boot_console.log.action('Testing \'abootimg get ver\'...')
+    response = u_boot_console.run_command('abootimg get ver')
+    assert response == "2"
+    u_boot_console.run_command('abootimg get ver v')
+    response = u_boot_console.run_command('env print v')
+    assert response == 'v=2'
+
+    u_boot_console.log.action('Testing \'abootimg get recovery_dtbo\'...')
+    response = u_boot_console.run_command('abootimg get recovery_dtbo a')
+    assert response == 'Error: recovery_dtbo_size is 0'
+
+    u_boot_console.log.action('Testing \'abootimg dump dtb\'...')
+    response = u_boot_console.run_command('abootimg dump dtb').replace('\r', '')
+    assert response == dtb_dump_resp
+
+    u_boot_console.log.action('Testing \'abootimg get dtb_load_addr\'...')
+    u_boot_console.run_command('abootimg get dtb_load_addr a')
+    response = u_boot_console.run_command('env print a')
+    assert response == 'a=11f00000'
+
+    u_boot_console.log.action('Testing \'abootimg get dtb --index\'...')
+    u_boot_console.run_command('abootimg get dtb --index=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.24.0

[PATCH v3 6/9] 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. 'abootimg':
     - CONFIG_CMD_ABOOTIMG=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>
---
 configs/am57xx_evm_defconfig        | 6 ++++++
 configs/am57xx_hs_evm_defconfig     | 6 ++++++
 configs/am57xx_hs_evm_usb_defconfig | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/configs/am57xx_evm_defconfig b/configs/am57xx_evm_defconfig
index 0c6a2e9193..22eb58fc29 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_ADTIMG=y
+CONFIG_CMD_ABOOTIMG=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"
@@ -102,3 +107,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_defconfig b/configs/am57xx_hs_evm_defconfig
index 3c57dfb031..bb26d46bfa 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_ADTIMG=y
+CONFIG_CMD_ABOOTIMG=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"
@@ -98,3 +103,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 87f391c2b0..73e42f9dcc 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_ADTIMG=y
+CONFIG_CMD_ABOOTIMG=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"
@@ -105,3 +110,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.24.0

[PATCH v3 7/9] env: ti: boot: Respect slot_suffix in AVB commands

[Sam Protsenko]

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 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.24.0

[PATCH v3 8/9] 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 blobs from dtbo partition.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 include/environment/ti/boot.h | 107 +++++++++++++---------------------
 1 file changed, 42 insertions(+), 65 deletions(-)

diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
index da99215fbd..3ac8e3ff08 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,20 +65,14 @@
 		"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 ""
+#define AB_SELECT_SLOT ""
+#define AB_SELECT_ARGS ""
 #endif
 
 #define FASTBOOT_CMD \
@@ -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; " \
+			"abootimg get dtb --index=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.24.0

[PATCH v3 9/9] arm: ti: boot: Use correct dtb and dtbo on Android boot

[Sam Protsenko]

Read correct dtb blob from boot.img/recovery.img and apply correct dtbo
blobs from dtbo partition.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 include/configs/ti_armv7_common.h |  7 +++++
 include/environment/ti/boot.h     | 43 ++++++++++++++++++++++++++++---
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/include/configs/ti_armv7_common.h b/include/configs/ti_armv7_common.h
index adc7861539..b36526b8f3 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 blobs (which should be applied into DTB
+ * blob), 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
+ * blob 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 3ac8e3ff08..0bb670eec7 100644
--- a/include/environment/ti/boot.h
+++ b/include/environment/ti/boot.h
@@ -75,6 +75,45 @@
 #define AB_SELECT_ARGS ""
 #endif
 
+/*
+ * Prepares complete device tree blob 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 for current SoC/board from boot image in $loadaddr
+ *      to $fdtaddr
+ *   2. Merge all needed DTBO for current board from 'dtbo' partition into read
+ *      DTB
+ *   3. User should provide $fdtaddr as 3rd argument to 'bootm'
+ */
+#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 for AM57x EVM RevA3...\"; " \
+		"abootimg get dtb --index=0 dtb_start dtb_size; " \
+		"cp.b $dtb_start $fdtaddr $dtb_size; " \
+		"fdt addr $fdtaddr; " \
+		"echo \"  Applying DTBOs for AM57x EVM RevA3...\"; " \
+		"adtimg addr $dtboaddr; " \
+		"adtimg get dt --index=0 dtbo0_addr; " \
+		"fdt apply $dtbo0_addr; " \
+		"adtimg get dt --index=1 dtbo1_addr; " \
+		"fdt apply $dtbo1_addr; " \
+	"elif test $board_name = beagle_x15_revc; then " \
+		"echo \"  Reading DTB for Beagle X15 RevC...\"; " \
+		"abootimg get dtb --index=0 dtb_start dtb_size; " \
+		"cp.b $dtb_start $fdtaddr $dtb_size; " \
+		"fdt addr $fdtaddr; " \
+	"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 +170,7 @@
 		"if part start mmc $mmcdev $apart boot_start; then " \
 			"part size mmc $mmcdev $apart boot_size; " \
 			"mmc read $loadaddr $boot_start $boot_size; " \
-			"abootimg get dtb --index=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.24.0

[PATCH v3 3/9] cmd: abootimg: Add abootimg command

[Eugeniu Rosca]

Hi,

With below nits, I have provided the Reviewed-by/Tested-by signatures.

On Tue, Dec 24, 2019 at 09:54:49PM +0200, Sam Protsenko 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 blob from
> boot.img in scripting manner, and then apply needed dtbo's (from "dtbo"

FWIW, 'scripted manner' is more common, as suggested by Ngram:
https://tinyurl.com/rck3ysj

> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
> +static int abootimg_get_dtb_by_index(int argc, char * const argv[])
> +{
> +	u32 num;
> +	char *endp;
> +	ulong addr;
> +	u32 size;
> +
> +	if (argc < 1 || argc > 3)
> +		return CMD_RET_USAGE;
> +
> +	num = simple_strtoul(argv[0] + strlen("--index="), &endp, 0);
> +	if (*endp != '\0') {
> +		printf("Error: Wrong num\n");
> +		return CMD_RET_FAILURE;
> +	}

The way '--index=' option is processed right now results in below
inconsistencies:

=> abootimg get dtb --index=0
1980
 `- expected output

=> abootimg get dtb --index=
1980
 `- unexpected output (expected error)

=> abootimg get dtb --index=0 --index=1
=>
 `- unexpected output (expected error)

> +U_BOOT_CMD(
> +	abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg,
> +	"manipulate Android Boot Image",
> +	"addr <addr>\n"
> +	"    - set the address in RAM where boot image is located\n"
> +	"      ($loadaddr is used by default)\n"
> +	"abootimg dump dtb\n"
> +	"    - print info for all DT blobs in DTB area\n"
> +	"abootimg get ver [varname]\n"
> +	"    - get header version\n"
> +	"abootimg get recovery_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"
> +	"abootimg get dtb_load_addr [varname]\n"
> +	"    - get load address (hex) of DTB, from image header\n"
> +	"abootimg get dtb --index=<num> [addr_var [size_var]]\n"
> +	"    - get address and size (hex) of DT blob in the image by index\n"
> +	"      <num>: index number of desired DT blob in DTB area\n"
> +	"      [addr_var]: variable name to contain DT blob address\n"
> +	"      [size_var]: variable name to contain DT blob size\n"

[minor] Superfluous blank line at the end of help.

Apart from the above, testing two Android v2 images containing the
DTB in so called "concat" and DTBO formats didn't reveal any issues.

Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

-- 
Best Regards,
Eugeniu

[PATCH v3 1/9] image: android: Add functions for handling dtb field

[Simon Glass]

Hi Sam,

On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> 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 blob from "DTB" part of
>     boot image, by blob's index
>   - android_image_print_dtb_contents(): Iterate over all DTB blobs in
>     "DTB" part of boot image and print those blobs info
>
> "DTB" payload might be in one of the following formats:
>   1. concatenated DTB blobs
>   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 blob(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 "abootimg" to extract dtb blob
>     from boot image (using functions from this patch)
>   - extract desired dtbo blobs from "dtbo" partition using "adtimg"
>     command
>   - merge dtbo blobs into dtb blob using "fdt apply" command
>   - pass resulting dtb blob 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>
> ---
>  common/Makefile        |   2 +-
>  common/image-android.c | 214 +++++++++++++++++++++++++++++++++++++++++
>  include/image.h        |   5 +
>  3 files changed, 220 insertions(+), 1 deletion(-)
>
> diff --git a/common/Makefile b/common/Makefile
> index 029cc0f2ce..1ffddc2f94 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..1ccad6c556 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>

Prefer underscores if possible.

>  #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,121 @@ 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;

Perhaps this isn't universal but at least with DM we use 'ret' for
return code instead of 'res' for result.

> +
> +       hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> +       if (android_image_check_header(hdr)) {
> +               printf("Error: Boot Image header is incorrect\n");
> +               res = false;

Could this function return an error code?

> +               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) {

if (!hdr...)

> +               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);
> +
> +       *addr = dtb_img_addr;
> +
> +exit:
> +       unmap_sysmem(hdr);
> +       return res;
> +}
> +
> +/**
> + * android_image_get_dtb_by_index() - Get address and size of blob 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 blob by its index in DTB area of Android
> + * Boot Image in RAM.
> + *
> + * Return: true on success or false on error.

Let's return an error code.

> + */
> +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
> +                                   u32 *size)

Suggest adding a 'p' suffix to return values, i.e. addrp and sizep.

> +{
> +       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 blob with specified index  */
> +       u32 i;                  /* index iterator */

Why use u32 for these variables? Natural types should be used where
possible, i.e. uint or int.

> +
> +       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;are these are these are these
> +       }
> +
> +       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 +363,101 @@ void android_print_contents(const struct andr_img_hdr *hdr)
>                 printf("%sdtb addr:             %llx\n", p, hdr->dtb_addr);
>         }
>  }
> +

Function comment...what is index?

> +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 blobs 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 blobs
> + *   2. Android DTBO format (see CONFIG_CMD_ADTIMG for details)
> + *
> + * This function does next:
> + *   1. Prints out the format used in DTB area
> + *   2. Iterates over all DTB blobs in DTB area and prints out the info for
> + *      each blob.
> + *
> + * Return: true on success or false on error.

Again I think an error code is better.

> + */
> +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 blob 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 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;
> +               }
> +
> +               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 f4d2aaf53e..8e81166be4 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);

Function comment

>  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.24.0
>

Regards,
SImon

[PATCH v3 2/9] image: android: Add routine to get dtbo params

[Simon Glass]

Hi Sam,

On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> 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 'abootimg' command to provide the user a way
> to get the address of recovery dtbo from U-Boot shell, which can be
> further parsed using 'adtimg' 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>
> ---
>  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 1ccad6c556..5d6669ceab 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 'adtimg' U-Boot
> + * command or android_dt_*() functions to extract desired DTBO blob.
> + *
> + * 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.

Same comments as previous patch on error codes, etc.

Also can we have a test for all this code?

> + */
> +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 8e81166be4..b8d821605b 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);

function comment again

>  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.24.0
>

Regards,
SImon

[PATCH v3 3/9] cmd: abootimg: Add abootimg command

[Simon Glass]

On Tue, 24 Dec 2019 at 12:55, 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 blob 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 (for non-A/B devices only,
> see [1,2] for details).
>
> It can be tested like this:
>
>     => mmc dev 1
>     => part start mmc 1 boot_a boot_start
>     => part size mmc 1 boot_a boot_size
>     => mmc read $loadaddr $boot_start $boot_size
>     => abootimg get ver
>     => abootimg dump dtb
>
> [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>
> ---
>  cmd/Kconfig    |   8 ++
>  cmd/Makefile   |   1 +
>  cmd/abootimg.c | 242 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 251 insertions(+)
>  create mode 100644 cmd/abootimg.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Can we have a test please?

[PATCH v3 4/9] doc: android: Add documentation for Android Boot Image

[Simon Glass]

On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Describe Android Boot Image format, how its support is implemented in
> U-Boot and associated commands usage.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  cmd/Kconfig                |   2 +
>  doc/android/boot-image.rst | 154 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 156 insertions(+)
>  create mode 100644 doc/android/boot-image.rst

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v3 5/9] test/py: android: Add test for abootimg

[Simon Glass]

On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Unit test for 'abootimg' 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_abootimg
>
> shows that 1/1 tests passes successfully.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  configs/sandbox_defconfig                   |   1 +
>  test/py/tests/test_android/test_abootimg.py | 159 ++++++++++++++++++++
>  2 files changed, 160 insertions(+)
>  create mode 100644 test/py/tests/test_android/test_abootimg.py

OK here is the test -)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v3 6/9] configs: am57xx_evm: Enable Android commands

[Simon Glass]

On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Enable Android commands that will be needed for Android 10 boot flow
> implementation, for all AM57x variants. Commands enabled:
>
>   1. 'abootimg':
>      - CONFIG_CMD_ABOOTIMG=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>
> ---
>  configs/am57xx_evm_defconfig        | 6 ++++++
>  configs/am57xx_hs_evm_defconfig     | 6 ++++++
>  configs/am57xx_hs_evm_usb_defconfig | 6 ++++++
>  3 files changed, 18 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v3 1/9] image: android: Add functions for handling dtb field

[Sam Protsenko]

Hi Simon,

On Wed, Jan 8, 2020 at 7:39 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sam,
>
> On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> >
> > 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 blob from "DTB" part of
> >     boot image, by blob's index
> >   - android_image_print_dtb_contents(): Iterate over all DTB blobs in
> >     "DTB" part of boot image and print those blobs info
> >
> > "DTB" payload might be in one of the following formats:
> >   1. concatenated DTB blobs
> >   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 blob(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 "abootimg" to extract dtb blob
> >     from boot image (using functions from this patch)
> >   - extract desired dtbo blobs from "dtbo" partition using "adtimg"
> >     command
> >   - merge dtbo blobs into dtb blob using "fdt apply" command
> >   - pass resulting dtb blob 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>
> > ---
> >  common/Makefile        |   2 +-
> >  common/image-android.c | 214 +++++++++++++++++++++++++++++++++++++++++
> >  include/image.h        |   5 +
> >  3 files changed, 220 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/Makefile b/common/Makefile
> > index 029cc0f2ce..1ffddc2f94 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..1ccad6c556 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>
>
> Prefer underscores if possible.
>

It's existing header file, related to another feature. So it can be
renamed in some separate patch outside of this patch series. Btw, is
there any background on why we should stick to underscore in file
names?

> >  #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,121 @@ 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;
>
> Perhaps this isn't universal but at least with DM we use 'ret' for
> return code instead of 'res' for result.
>

I think it's just a matter of taste. Not a major one, right?

> > +
> > +       hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> > +       if (android_image_check_header(hdr)) {
> > +               printf("Error: Boot Image header is incorrect\n");
> > +               res = false;
>
> Could this function return an error code?
>

Frankly, don't see much value in error code in this particular case.
All we can do to handle any error in this function further is just to
print corresponding error message, which I do in this function already
(sticking to principle to print error messages where we actually know
what happened). So I'd stick to just bool, if you don't mind, to not
over-complicate this without actual reason.

> > +               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) {
>
> if (!hdr...)
>

Actually I really wanted to emphasize here that we test if size is 0,
so I'd keep that as is, if you don't mind.

> > +               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);
> > +
> > +       *addr = dtb_img_addr;
> > +
> > +exit:
> > +       unmap_sysmem(hdr);
> > +       return res;
> > +}
> > +
> > +/**
> > + * android_image_get_dtb_by_index() - Get address and size of blob 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 blob by its index in DTB area of Android
> > + * Boot Image in RAM.
> > + *
> > + * Return: true on success or false on error.
>
> Let's return an error code.
>

Sorry, I don't agree. Just by looking how we handle (or can handle in
future) error cases when executing this function, I can't see any
value in error code. Error message is already printed on each
particular error case in this function. So if you don't mind, I'd keep
"bool" here (because I don't see some really good reason why not to).

> > + */
> > +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
> > +                                   u32 *size)
>
> Suggest adding a 'p' suffix to return values, i.e. addrp and sizep.
>

Sorry, I really don't like cluttering variable names with suffixes. We
already know that those are pointers, just by looking at function
signature. I'd better stay away from stuff like Hungarian notation,
but maybe it's just my Windows development induced trauma is talking
;)

> > +{
> > +       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 blob with specified index  */
> > +       u32 i;                  /* index iterator */
>
> Why use u32 for these variables? Natural types should be used where
> possible, i.e. uint or int.
>

'dtb_img_size' should be u32, because struct andr_img_hdr::dtb_size is
u32, and it's being assigned to 'dtb_img_size'.

As for 'i', it's used to check the 'index', which in turn is used to
check struct dt_table_header::dt_entry_count, in
android_dt_get_fdt_by_index() function.

Although I agree that in common case we should stick to int/ulong, in
this particular case I guess u32 choice is valid.

> > +
> > +       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;are these are these are these
> > +       }
> > +
> > +       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 +363,101 @@ void android_print_contents(const struct andr_img_hdr *hdr)
> >                 printf("%sdtb addr:             %llx\n", p, hdr->dtb_addr);
> >         }
> >  }
> > +
>
> Function comment...what is index?
>

It's static, so I decided the comment can be ommited in this case. But
ok, I'll add it in v4, for consistency.

> > +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 blobs 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 blobs
> > + *   2. Android DTBO format (see CONFIG_CMD_ADTIMG for details)
> > + *
> > + * This function does next:
> > + *   1. Prints out the format used in DTB area
> > + *   2. Iterates over all DTB blobs in DTB area and prints out the info for
> > + *      each blob.
> > + *
> > + * Return: true on success or false on error.
>
> Again I think an error code is better.
>

As stated above, I don't see any actual usages of error code value...
So I'd stick to bool as more simplistic choice, if it's not critical
with you.

> > + */
> > +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 blob 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 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;
> > +               }
> > +
> > +               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 f4d2aaf53e..8e81166be4 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);
>
> Function comment
>

It's present next to function implementation, in
common/image-android.c (like it's usually done in Linux kernel).

> >  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.24.0
> >
>
> Regards,
> SImon

[PATCH v3 0/9] am57xx: Implement Android 10 boot flow

[Bajjuri, Praneeth]

Tom,

On 12/24/2019 1:54 PM, Sam Protsenko wrote:
> 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 (see "abootimg" command and associated C API).
>
> This patch series must be applied on top of these recently sent patches
> by Eugeniu:
>
>      [PATCH 0/3] cmd: dtimg: Rename to adtimg and refactor usage style
>
> Changes in v3:
>   - rename command to "abootimg" (requested by Simon Glass)
>   - rework command interface (as discussed with Eugeniu)
>   - add command documentation
>   - address other comments


Since this series is functionally mature,
can this series be pulled as-is in the next merge window,
verified v3 with current aosp master and functionality is working
as expected.

the comments on changing to a more suitable names , adding a
new test script for the functionality can be done as a incremental
update on the top during the next cycle.


>
> [1] https://source.android.com/devices/bootloader
>
> Sam Protsenko (9):
>    image: android: Add functions for handling dtb field
>    image: android: Add routine to get dtbo params
>    cmd: abootimg: Add abootimg command
>    doc: android: Add documentation for Android Boot Image
>    test/py: android: Add test for abootimg
>    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                                 |  10 +
>   cmd/Makefile                                |   1 +
>   cmd/abootimg.c                              | 242 +++++++++++++++++
>   common/Makefile                             |   2 +-
>   common/image-android.c                      | 275 ++++++++++++++++++++
>   configs/am57xx_evm_defconfig                |   6 +
>   configs/am57xx_hs_evm_defconfig             |   6 +
>   configs/am57xx_hs_evm_usb_defconfig         |   6 +
>   configs/sandbox_defconfig                   |   1 +
>   doc/android/boot-image.rst                  | 154 +++++++++++
>   include/configs/ti_armv7_common.h           |   7 +
>   include/environment/ti/boot.h               | 146 ++++++-----
>   include/image.h                             |   6 +
>   test/py/tests/test_android/test_abootimg.py | 159 +++++++++++
>   14 files changed, 954 insertions(+), 67 deletions(-)
>   create mode 100644 cmd/abootimg.c
>   create mode 100644 doc/android/boot-image.rst
>   create mode 100644 test/py/tests/test_android/test_abootimg.py
>

[PATCH v3 0/9] am57xx: Implement Android 10 boot flow

[Bajjuri, Praneeth]

+ Sam Protsenko's current email address

On 1/22/2020 11:51 AM, Bajjuri, Praneeth wrote:
> Tom,
>
> On 12/24/2019 1:54 PM, Sam Protsenko wrote:
>> 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 (see "abootimg" command and associated C API).
>>
>> This patch series must be applied on top of these recently sent patches
>> by Eugeniu:
>>
>>      [PATCH 0/3] cmd: dtimg: Rename to adtimg and refactor usage style
>>
>> Changes in v3:
>>   - rename command to "abootimg" (requested by Simon Glass)
>>   - rework command interface (as discussed with Eugeniu)
>>   - add command documentation
>>   - address other comments
>
>
> Since this series is functionally mature,
> can this series be pulled as-is in the next merge window,
> verified v3 with current aosp master and functionality is working
> as expected.
>
> the comments on changing to a more suitable names , adding a
> new test script for the functionality can be done as a incremental
> update on the top during the next cycle.
>
>
>>
>> [1] https://source.android.com/devices/bootloader
>>
>> Sam Protsenko (9):
>>    image: android: Add functions for handling dtb field
>>    image: android: Add routine to get dtbo params
>>    cmd: abootimg: Add abootimg command
>>    doc: android: Add documentation for Android Boot Image
>>    test/py: android: Add test for abootimg
>>    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                                 |  10 +
>>   cmd/Makefile                                |   1 +
>>   cmd/abootimg.c                              | 242 +++++++++++++++++
>>   common/Makefile                             |   2 +-
>>   common/image-android.c                      | 275 ++++++++++++++++++++
>>   configs/am57xx_evm_defconfig                |   6 +
>>   configs/am57xx_hs_evm_defconfig             |   6 +
>>   configs/am57xx_hs_evm_usb_defconfig         |   6 +
>>   configs/sandbox_defconfig                   |   1 +
>>   doc/android/boot-image.rst                  | 154 +++++++++++
>>   include/configs/ti_armv7_common.h           |   7 +
>>   include/environment/ti/boot.h               | 146 ++++++-----
>>   include/image.h                             |   6 +
>>   test/py/tests/test_android/test_abootimg.py | 159 +++++++++++
>>   14 files changed, 954 insertions(+), 67 deletions(-)
>>   create mode 100644 cmd/abootimg.c
>>   create mode 100644 doc/android/boot-image.rst
>>   create mode 100644 test/py/tests/test_android/test_abootimg.py
>>

[PATCH v3 1/9] image: android: Add functions for handling dtb field

[Simon Glass]

Hi Sam,

On Fri, 10 Jan 2020 at 08:33, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, Jan 8, 2020 at 7:39 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sam,
> >
> > On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> > >
> > > 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 blob from "DTB" part of
> > >     boot image, by blob's index
> > >   - android_image_print_dtb_contents(): Iterate over all DTB blobs in
> > >     "DTB" part of boot image and print those blobs info
> > >
> > > "DTB" payload might be in one of the following formats:
> > >   1. concatenated DTB blobs
> > >   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 blob(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 "abootimg" to extract dtb blob
> > >     from boot image (using functions from this patch)
> > >   - extract desired dtbo blobs from "dtbo" partition using "adtimg"
> > >     command
> > >   - merge dtbo blobs into dtb blob using "fdt apply" command
> > >   - pass resulting dtb blob 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>
> > > ---
> > >  common/Makefile        |   2 +-
> > >  common/image-android.c | 214 +++++++++++++++++++++++++++++++++++++++++
> > >  include/image.h        |   5 +
> > >  3 files changed, 220 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/common/Makefile b/common/Makefile
> > > index 029cc0f2ce..1ffddc2f94 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..1ccad6c556 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>
> >
> > Prefer underscores if possible.
> >
>
> It's existing header file, related to another feature. So it can be
> renamed in some separate patch outside of this patch series. Btw, is
> there any background on why we should stick to underscore in file
> names?

I added a comment here:

https://www.denx.de/wiki/U-Boot/CodingStyle

>
> > >  #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,121 @@ 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;
> >
> > Perhaps this isn't universal but at least with DM we use 'ret' for
> > return code instead of 'res' for result.
> >
>
> I think it's just a matter of taste. Not a major one, right?

Not major perhaps, but this sort of consistency makes the code much
easier to read, IMO. Using error codes as return values is pretty
universal in U-Boot so I think you should do that instead of
true/false.

>
> > > +
> > > +       hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> > > +       if (android_image_check_header(hdr)) {
> > > +               printf("Error: Boot Image header is incorrect\n");
> > > +               res = false;
> >
> > Could this function return an error code?
> >
>
> Frankly, don't see much value in error code in this particular case.
> All we can do to handle any error in this function further is just to
> print corresponding error message, which I do in this function already
> (sticking to principle to print error messages where we actually know
> what happened). So I'd stick to just bool, if you don't mind, to not
> over-complicate this without actual reason.

It is not a great idea to print an error message in a leaf function.
Ideally we would do all of this in the command that calls the
function, if it can be determined by the error code.

Also, 'boot image header is incorrect' doesn't help that much. What is
incorrect about it? You could print the return code so you know.
Following your philosophy you should add lots of printf() calls to
android_image_check_header().

Also log_err() is better than printf()


>
> > > +               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) {
> >
> > if (!hdr...)
> >
>
> Actually I really wanted to emphasize here that we test if size is 0,
> so I'd keep that as is, if you don't mind.

OK I guess it is just what you are used to.

>
> > > +               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);
> > > +
> > > +       *addr = dtb_img_addr;
> > > +
> > > +exit:
> > > +       unmap_sysmem(hdr);
> > > +       return res;
> > > +}
> > > +
> > > +/**
> > > + * android_image_get_dtb_by_index() - Get address and size of blob 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 blob by its index in DTB area of Android
> > > + * Boot Image in RAM.
> > > + *
> > > + * Return: true on success or false on error.
> >
> > Let's return an error code.
> >
>
> Sorry, I don't agree. Just by looking how we handle (or can handle in
> future) error cases when executing this function, I can't see any
> value in error code. Error message is already printed on each
> particular error case in this function. So if you don't mind, I'd keep
> "bool" here (because I don't see some really good reason why not to).

See above.

>
> > > + */
> > > +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
> > > +                                   u32 *size)
> >
> > Suggest adding a 'p' suffix to return values, i.e. addrp and sizep.
> >
>
> Sorry, I really don't like cluttering variable names with suffixes. We
> already know that those are pointers, just by looking at function
> signature. I'd better stay away from stuff like Hungarian notation,
> but maybe it's just my Windows development induced trauma is talking
> ;)

Again this is just a matter of style. Driver model uses 'p' and some
of image.h. I like Hungarians but not their notation either, except in
this case where is a really good visual indication of what is going
on.

>
> > > +{
> > > +       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 blob with specified index  */
> > > +       u32 i;                  /* index iterator */
> >
> > Why use u32 for these variables? Natural types should be used where
> > possible, i.e. uint or int.
> >
>
> 'dtb_img_size' should be u32, because struct andr_img_hdr::dtb_size is
> u32, and it's being assigned to 'dtb_img_size'.

I'm not sure why.

>
> As for 'i', it's used to check the 'index', which in turn is used to
> check struct dt_table_header::dt_entry_count, in
> android_dt_get_fdt_by_index() function.
>
> Although I agree that in common case we should stick to int/ulong, in
> this particular case I guess u32 choice is valid.

In general we should use natural sizes to avoid shifts/masks, etc. on
machines with a larger word size. The exception is structures, etc.
where you need a certain binary format.

>
> > > +
> > > +       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;are these are these are these
> > > +       }
> > > +
> > > +       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 +363,101 @@ void android_print_contents(const struct andr_img_hdr *hdr)
> > >                 printf("%sdtb addr:             %llx\n", p, hdr->dtb_addr);
> > >         }
> > >  }
> > > +
> >
> > Function comment...what is index?
> >
>
> It's static, so I decided the comment can be ommited in this case. But
> ok, I'll add it in v4, for consistency.

Yes please. Function comments should be added for non-trivial
functions even if they are static.

>
> > > +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 blobs 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 blobs
> > > + *   2. Android DTBO format (see CONFIG_CMD_ADTIMG for details)
> > > + *
> > > + * This function does next:
> > > + *   1. Prints out the format used in DTB area
> > > + *   2. Iterates over all DTB blobs in DTB area and prints out the info for
> > > + *      each blob.
> > > + *
> > > + * Return: true on success or false on error.
> >
> > Again I think an error code is better.
> >
>
> As stated above, I don't see any actual usages of error code value...
> So I'd stick to bool as more simplistic choice, if it's not critical
> with you.

See above, but I suggest you add some error codes, so the caller can
see what went wrong.

>
> > > + */
> > > +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 blob 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 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;
> > > +               }
> > > +
> > > +               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 f4d2aaf53e..8e81166be4 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);
> >
> > Function comment
> >
>
> It's present next to function implementation, in
> common/image-android.c (like it's usually done in Linux kernel).

OK but please put it in the header, which is where people look and
ctags takes you for the definition. The header file describes the API
for image.h


>
> > >  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.24.0
> > >
> >

Regards,
Simon