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