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

[Eugeniu Rosca]

Hi Sam,

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

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

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

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

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

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

[..]

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

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

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

   config ANDROID_BOOT_IMAGE
      select ANDROID_BOOT_IMAGE_DT

   config DTIMG
      select ANDROID_BOOT_IMAGE_DT

[..]

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

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

-- 
Best Regards,
Eugeniu