Hi Sam,
On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
Android Boot Image v2 adds "DTB" payload (and corresponding field in the image header). Provide functions for its handling:
I believe this totally new degree of freedom brought by "Android Boot Image v2" might unintentionally make some users more unhappy [0], since it enables yet another way of passing a DTB to the Android kernel.
It looks to me that there are at least three valid design choices for DTB storage/passing which platform maintainers have to consider: A. Android Image second area [1-2] B. Dedicated DTB/DTBO partitions on a block device C. DTB area in Android Image v2
While there are some major disadvantages for [A][1-2], [B] and [C] look more or less equivalent and will most likely only differ in the tooling used to generate and extract the useful/relevant artifacts.
Not to mention that hybrids [B+C] are also possible. Which solution should we pick as a long-term one?
[0] https://en.wikipedia.org/wiki/The_Paradox_of_Choice [1] 104816142f9c6a ("parse the second area of android image") [2] 6a7b406aa8b981 ("fdt: support booting with dtb in Android image")
[..]
common/Makefile | 2 +- common/image-android.c | 215 +++++++++++++++++++++++++++++++++++++++++ include/image.h | 5 + 3 files changed, 221 insertions(+), 1 deletion(-)
diff --git a/common/Makefile b/common/Makefile index 302d8beaf3..7e5f5058b3 100644 --- a/common/Makefile +++ b/common/Makefile @@ -108,7 +108,7 @@ endif
obj-y += image.o obj-$(CONFIG_ANDROID_AB) += android_ab.o -obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o +obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o image-android-dt.o
I expect that in a not so distant future, Android users who care about performance aspects of their system (e.g. in automotive sector, where boot time is paramount to achieve ~2s to Rear View Camera) will attempt optimizing U-Boot by compiling out features they don't need.
With this background: - Would it make sense to allow users to selectively enable and disable support for A/B-capable and non-A/B devices? My guess is that it is currently not an important development/review criteria, since particularly image-android-dt.c implements functions, some of which are A/B-specific, some non-A/B-specific (e.g. android_image_get_dtbo) and some are common. - I would try to avoid wiring the same compilation unit to Kbuild (e.g. image-android-dt.o) via multiple Kconfig symbols (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes the relationship between the CONFIG symbols unclear. As a user, I would like to see a simple mapping between compiled objects and Kconfig symbols, eventually reflecting a hierarchy/dependency:
config ANDROID_BOOT_IMAGE select ANDROID_BOOT_IMAGE_DT
config DTIMG select ANDROID_BOOT_IMAGE_DT
[..]
diff --git a/common/image-android.c b/common/image-android.c index 3564a64221..5e721a27a7 100644 --- a/common/image-android.c +++ b/common/image-android.c @@ -6,10 +6,12 @@ #include <common.h> #include <env.h> #include <image.h> +#include <image-android-dt.h> #include <android_image.h> #include <malloc.h> #include <errno.h> #include <asm/unaligned.h> +#include <mapmem.h>
#define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR 0x10008000
@@ -195,6 +197,122 @@ int android_image_get_second(const struct andr_img_hdr *hdr, return 0; }
+/**
- android_image_get_dtb_img_addr() - Get the address of DTB area in boot image.
- @hdr_addr: Boot image header address
- @addr: Will contain the address of DTB area in boot image
- Return: true on success or false on fail.
- */
+static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong *addr) +{
- const struct andr_img_hdr *hdr;
- ulong dtb_img_addr;
- bool res = true;
- hdr = map_sysmem(hdr_addr, sizeof(*hdr));
- if (android_image_check_header(hdr)) {
printf("Error: Boot Image header is incorrect\n");res = false;goto exit;- }
- if (hdr->header_version < 2) {
printf("Error: header_version must be >= 2 to get dtb\n");res = false;goto exit;- }
- if (hdr->dtb_size == 0) {
printf("Error: dtb_size is 0\n");res = false;goto exit;- }
- /* Calculate the address of DTB area in boot image */
- dtb_img_addr = hdr_addr;
- dtb_img_addr += hdr->page_size;
- dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
- dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
- dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
- dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
- if (addr)
*addr = dtb_img_addr;
In this recent series, I noticed a consistent preference towards doing a lot of processing with returning nothing to the caller in case of an invalid argument. Is this by choice?