[U-Boot] [PATCH 0/4] cmd: dtimg: Enhance with --id and --rev options (take #1)

[U-Boot] [PATCH 0/4] cmd: dtimg: Enhance with --id and --rev options (take #1)

[Eugeniu Rosca]

Dear U-Boot community,

As summarized in the title, the motivation behind the series is to
primarly allow the Android "dtimg" command (credits to Sam for having
it) to actually leverage the specification described in [*].

There are a few collateral improvements and optimizations added as well.
The individual patches should be verbose and descriptive enough.
The series has been tested on R-Car H3ULCB.
All available static analysis tools [**] have been silenced.

Your comments would be highly appreciated.

[*] https://source.android.com/devices/architecture/dto/partitions
[**] smatch, cppecheck, sparse, make W=1

Eugeniu Rosca (4):
  cmd: dtimg: Report invalid index argument
  cmd: dtimg: Merge duplicated prints
  cmd: dtimg: Make <varname> an optional argument
  cmd: dtimg: Get start and size based on --id and --rev

 cmd/dtimg.c                | 133 +++++++++++++++++++++++++++++--------
 common/image-android-dt.c  |  64 ++++++++++++++++--
 include/image-android-dt.h |   5 +-
 3 files changed, 169 insertions(+), 33 deletions(-)

-- 
2.24.0

[U-Boot] [PATCH 1/4] cmd: dtimg: Report invalid index argument

[Eugeniu Rosca]

Being user-friendly is paramount to make any product likeable and
easy to use. Hence, instead of [1], print [2].

[1] dtimg start 0x48000000 not-a-number myvar
Error: Wrong index

[2] dtimg start 0x48000000 not-a-number myvar
Error: Wrong index 'not-a-number'

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 cmd/dtimg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/dtimg.c b/cmd/dtimg.c
index 6c5d53cc6808..2317c859953d 100644
--- a/cmd/dtimg.c
+++ b/cmd/dtimg.c
@@ -63,7 +63,7 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 
 	index = simple_strtoul(argv[2], &endp, 0);
 	if (*endp != '\0') {
-		printf("Error: Wrong index\n");
+		printf("Error: Wrong index '%s'\n", argv[2]);
 		return CMD_RET_FAILURE;
 	}
 
-- 
2.24.0

[U-Boot] [PATCH 2/4] cmd: dtimg: Merge duplicated prints

[Eugeniu Rosca]

Getting DTB/DTBO header address happens twice (in do_dtimg_dump and
in dtimg_get_fdt) with duplicating below error messages:
 - Error: Wrong image address
 - Error: DT image header is incorrect

Reduce the duplication and improve the error message by appending
the faulty address value:
 - Error: Wrong image address '0x48000000z'

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 cmd/dtimg.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/cmd/dtimg.c b/cmd/dtimg.c
index 2317c859953d..5989081b0c14 100644
--- a/cmd/dtimg.c
+++ b/cmd/dtimg.c
@@ -13,18 +13,13 @@ enum cmd_dtimg_info {
 	CMD_DTIMG_SIZE,
 };
 
-static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
-			 char * const argv[])
+static int dtimg_get_argv_addr(char * const str, ulong *hdr_addrp)
 {
 	char *endp;
-	ulong hdr_addr;
+	ulong hdr_addr = simple_strtoul(str, &endp, 16);
 
-	if (argc != 2)
-		return CMD_RET_USAGE;
-
-	hdr_addr = simple_strtoul(argv[1], &endp, 16);
 	if (*endp != '\0') {
-		printf("Error: Wrong image address\n");
+		printf("Error: Wrong image address '%s'\n", str);
 		return CMD_RET_FAILURE;
 	}
 
@@ -32,6 +27,21 @@ static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
 		printf("Error: DT image header is incorrect\n");
 		return CMD_RET_FAILURE;
 	}
+	*hdr_addrp = hdr_addr;
+
+	return CMD_RET_SUCCESS;
+}
+
+static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+			 char * const argv[])
+{
+	ulong hdr_addr;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
+		return CMD_RET_FAILURE;
 
 	android_dt_print_contents(hdr_addr);
 
@@ -50,16 +60,8 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 	if (argc != 4)
 		return CMD_RET_USAGE;
 
-	hdr_addr = simple_strtoul(argv[1], &endp, 16);
-	if (*endp != '\0') {
-		printf("Error: Wrong image address\n");
+	if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
 		return CMD_RET_FAILURE;
-	}
-
-	if (!android_dt_check_header(hdr_addr)) {
-		printf("Error: DT image header is incorrect\n");
-		return CMD_RET_FAILURE;
-	}
 
 	index = simple_strtoul(argv[2], &endp, 0);
 	if (*endp != '\0') {
-- 
2.24.0

[U-Boot] [PATCH 3/4] cmd: dtimg: Make <varname> an optional argument

[Eugeniu Rosca]

Unlike dtimg, U-Boot commands like part [1], fstype [2] and uuid [3]
accept an _optional_ <varname> parameter, which means that they will
output the result to console whenever <varname> is skipped. This is
extremely useful during development.

Allow "dtimg" to behave in a similar fashion [4]. In addition:
 - replace env_set() by env_set_hex()
 - track and report the failures of env_set_hex()
 - amend command's help/usage text

[1] => part start mmc 0 1
    800
    => part start mmc 0 1 myvar; print myvar
    myvar=800
[2] => fstype mmc 0:1
    ext4
    => fstype mmc 0:1 myvar; print myvar
    myvar=ext4
[3] => uuid
    b3909b50-55df-4173-b83c-b05343d2d5d2
    => uuid myvar; print myvar
    myvar=4c04b15f-d0c1-4f98-9aca-ab62a66be864
[4] => dtimg start 0x48000000 0
    0x480000e0
    => dtimg start 0x48000000 0 myvar; print myvar
    myvar=480000e0

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 cmd/dtimg.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/cmd/dtimg.c b/cmd/dtimg.c
index 5989081b0c14..5348a4ad46e8 100644
--- a/cmd/dtimg.c
+++ b/cmd/dtimg.c
@@ -55,9 +55,10 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 	char *endp;
 	ulong fdt_addr;
 	u32 fdt_size;
-	char buf[65];
+	ulong envval;
+	int ret;
 
-	if (argc != 4)
+	if (argc < 3)
 		return CMD_RET_USAGE;
 
 	if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
@@ -74,17 +75,24 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 
 	switch (cmd) {
 	case CMD_DTIMG_START:
-		snprintf(buf, sizeof(buf), "%lx", fdt_addr);
+		envval = fdt_addr;
 		break;
 	case CMD_DTIMG_SIZE:
-		snprintf(buf, sizeof(buf), "%x", fdt_size);
+		envval = fdt_size;
 		break;
 	default:
 		printf("Error: Unknown cmd_dtimg_info value: %d\n", cmd);
 		return CMD_RET_FAILURE;
 	}
 
-	env_set(argv[3], buf);
+	if (argv[3]) {
+		ret = env_set_hex(argv[3], envval);
+		if (ret)
+			printf("Error(%d) env-setting '%s=0x%lx'\n",
+			       ret, argv[3], envval);
+	} else {
+		printf("0x%lx\n", envval);
+	}
 
 	return CMD_RET_SUCCESS;
 }
@@ -131,12 +139,12 @@ U_BOOT_CMD(
 	"dump <addr>\n"
 	"    - parse specified image and print its structure info\n"
 	"      <addr>: image address in RAM, in hex\n"
-	"dtimg start <addr> <index> <varname>\n"
+	"dtimg start <addr> <index> [<varname>]\n"
 	"    - get address (hex) of FDT in the image, by index\n"
 	"      <addr>: image address in RAM, in hex\n"
 	"      <index>: index of desired FDT in the image\n"
 	"      <varname>: name of variable where to store address of FDT\n"
-	"dtimg size <addr> <index> <varname>\n"
+	"dtimg size <addr> <index> [<varname>]\n"
 	"    - get size (hex, bytes) of FDT in the image, by index\n"
 	"      <addr>: image address in RAM, in hex\n"
 	"      <index>: index of desired FDT in the image\n"
-- 
2.24.0

[U-Boot] [PATCH 4/4] cmd: dtimg: Get start and size based on --id and --rev

[Eugeniu Rosca]

Currently, it is only possible to get the ${index}'s entry of a DTB/DTBO
image [*]. The "dtimg" command is agnostic on the "id" and "rev" fields
and is unable to take them as input for a more fine-grained DTB/DTBO
search/retrieval.

This is a major limitation, as users would like [**] to employ the
"id"/"rev" fields to e.g. differentiate between DTBs/DTBOs
associated to multiple HW revisions or several platforms.

Given a sample DTBO image [***], the new options work like below:

 => dtimg start 0x48000000 0 --id invalid
    Error: Bad value '--id=invalid'
 => dtimg start 0x48000000 0 --id 0x100
    Error: No #0 entry having id=0x100 && rev=0x0
 => dtimg start 0x48000000 0 --id 00779000
    0x480006ac
 => dtimg start 0x48000000 1 --id 00779000
    0x48000b46
 => dtimg start 0x48000000 2 --id 00779000
    Error: No #2 entry having id=0x779000 && rev=0x0
 => dtimg start 0x48000000 99 --id 00779000
    Error: index >= dt_entry_count (99 >= 6)
 => dtimg start 0x48000000 0
    0x480000e0
 => dtimg start 0x48000000 1
    0x480000e0
 => dtimg start 0x48000000 2
    0x480004d0
 => dtimg start 0x48000000 3
    0x480005be
 => dtimg start 0x48000000 4
    0x480006ac
 => dtimg start 0x48000000 5
    0x48000b46
 => dtimg start 0x48000000 6
    Error: index >= dt_entry_count (6 >= 6)
 => dtimg size 0x48000000 0 --id 00779000
    0x49a
 => dtimg size 0x48000000 1 --id 00779000
    0x248

[*] https://source.android.com/devices/architecture/dto/partitions
[**] https://patchwork.ozlabs.org/patch/958594/#2302310
[***] Sample/dummy DTBO image:
 => dtimg dump 0x48000000
 dt_table_header:
                magic = d7b7ab1e
           total_size = 3470
          header_size = 32
        dt_entry_size = 32
       dt_entry_count = 6
    dt_entries_offset = 32
            page_size = 4096
              version = 0
 dt_table_entry[0]:
              dt_size = 1008
            dt_offset = 224
                   id = 0b779530
                  rev = 00000000
            custom[0] = 00000000
            custom[1] = 00000000
            custom[2] = 00000000
            custom[3] = 00000000
            (FDT)size = 1008
      (FDT)compatible = (unknown)
 dt_table_entry[1]:
              dt_size = 1008
            dt_offset = 224
                   id = 0b779520
                  rev = 00000000
            custom[0] = 00000000
            custom[1] = 00000000
            custom[2] = 00000000
            custom[3] = 00000000
            (FDT)size = 1008
      (FDT)compatible = (unknown)
 dt_table_entry[2]:
              dt_size = 238
            dt_offset = 1232
                   id = 0b779530
                  rev = 00000000
            custom[0] = 00000000
            custom[1] = 00000000
            custom[2] = 00000000
            custom[3] = 00000000
            (FDT)size = 238
      (FDT)compatible = (unknown)
 dt_table_entry[3]:
              dt_size = 238
            dt_offset = 1470
                   id = 0b779520
                  rev = 00000000
            custom[0] = 00000000
            custom[1] = 00000000
            custom[2] = 00000000
            custom[3] = 00000000
            (FDT)size = 238
      (FDT)compatible = (unknown)
 dt_table_entry[4]:
              dt_size = 1178
            dt_offset = 1708
                   id = 00779000
                  rev = 00000000
            custom[0] = 00000000
            custom[1] = 00000000
            custom[2] = 00000000
            custom[3] = 00000000
            (FDT)size = 1178
      (FDT)compatible = (unknown)
 dt_table_entry[5]:
              dt_size = 584
            dt_offset = 2886
                   id = 00779000
                  rev = 00000000
            custom[0] = 00000000
            custom[1] = 00000000
            custom[2] = 00000000
            custom[3] = 00000000
            (FDT)size = 584
      (FDT)compatible = (unknown)

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 cmd/dtimg.c                | 81 +++++++++++++++++++++++++++++++++++---
 common/image-android-dt.c  | 64 ++++++++++++++++++++++++++++--
 include/image-android-dt.h |  5 ++-
 3 files changed, 138 insertions(+), 12 deletions(-)

diff --git a/cmd/dtimg.c b/cmd/dtimg.c
index 5348a4ad46e8..10e909ce551b 100644
--- a/cmd/dtimg.c
+++ b/cmd/dtimg.c
@@ -7,6 +7,7 @@
 #include <env.h>
 #include <image-android-dt.h>
 #include <common.h>
+#include <linux/ctype.h>
 
 enum cmd_dtimg_info {
 	CMD_DTIMG_START = 0,
@@ -48,6 +49,61 @@ static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+static int dtimg_get_opthex(int *argcp, char * const *argvp[], u32 *valp)
+{
+	char *endp;
+	u32 val;
+
+	if (!argcp || !argvp || !valp)
+		return CMD_RET_FAILURE;
+
+	if (*argcp < 2) {
+		printf("Error: Option '%s' expects an argument\n", (*argvp)[0]);
+		return CMD_RET_FAILURE;
+	}
+
+	val = simple_strtoul((*argvp)[1], &endp, 16);
+	if (*endp != '\0') {
+		printf("Error: Bad value '%s=%s'\n", (*argvp)[0], (*argvp)[1]);
+		return CMD_RET_FAILURE;
+	}
+
+	*valp = val;
+	(*argcp)--;
+	(*argvp)++;
+
+	return CMD_RET_SUCCESS;
+}
+
+static int dtimg_get_opt(int argc, char * const argv[],
+			 struct dt_table_entry *ep, char **envstrp)
+{
+	if (!ep || argc < 0 || !argv || !envstrp)
+		return CMD_RET_FAILURE;
+
+	for (; argc > 0; argc--, argv++) {
+		int ret;
+
+		if (!strcmp(argv[0], "--id")) {
+			ret = dtimg_get_opthex(&argc, &argv, &ep->id);
+			if (ret != CMD_RET_SUCCESS)
+				return ret;
+		} else if (!strcmp(argv[0], "--rev")) {
+			ret = dtimg_get_opthex(&argc, &argv, &ep->rev);
+			if (ret != CMD_RET_SUCCESS)
+				return ret;
+		} else if (argc == 1 && argv[0][0] != '-' &&
+			   !isdigit(argv[0][0])) {
+			*envstrp = argv[0];
+		} else {
+			printf("Error: Option '%s' not supported\n", argv[0]);
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 {
 	ulong hdr_addr;
@@ -55,6 +111,8 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 	char *endp;
 	ulong fdt_addr;
 	u32 fdt_size;
+	struct dt_table_entry entry = { 0 };
+	char *envstr = NULL;
 	ulong envval;
 	int ret;
 
@@ -70,7 +128,18 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 		return CMD_RET_FAILURE;
 	}
 
-	if (!android_dt_get_fdt_by_index(hdr_addr, index, &fdt_addr, &fdt_size))
+	/*
+	 * Consume all processed mandatory arguments.
+	 * Prepare for parsing the optional ones.
+	 */
+	argc -= 3;
+	argv += 3;
+
+	ret = dtimg_get_opt(argc, argv, &entry, &envstr);
+	if (ret != CMD_RET_SUCCESS)
+		return ret;
+
+	if (!android_dt_get_fdt(hdr_addr, index, &entry, &fdt_addr, &fdt_size))
 		return CMD_RET_FAILURE;
 
 	switch (cmd) {
@@ -85,11 +154,11 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
 		return CMD_RET_FAILURE;
 	}
 
-	if (argv[3]) {
-		ret = env_set_hex(argv[3], envval);
+	if (envstr) {
+		ret = env_set_hex(envstr, envval);
 		if (ret)
 			printf("Error(%d) env-setting '%s=0x%lx'\n",
-			       ret, argv[3], envval);
+			       ret, envstr, envval);
 	} else {
 		printf("0x%lx\n", envval);
 	}
@@ -111,8 +180,8 @@ static int do_dtimg_size(cmd_tbl_t *cmdtp, int flag, int argc,
 
 static cmd_tbl_t cmd_dtimg_sub[] = {
 	U_BOOT_CMD_MKENT(dump, 2, 0, do_dtimg_dump, "", ""),
-	U_BOOT_CMD_MKENT(start, 4, 0, do_dtimg_start, "", ""),
-	U_BOOT_CMD_MKENT(size, 4, 0, do_dtimg_size, "", ""),
+	U_BOOT_CMD_MKENT(start, 8, 0, do_dtimg_start, "", ""),
+	U_BOOT_CMD_MKENT(size, 8, 0, do_dtimg_size, "", ""),
 };
 
 static int do_dtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
diff --git a/common/image-android-dt.c b/common/image-android-dt.c
index a2d52df4a2a9..91e6b4eca23a 100644
--- a/common/image-android-dt.c
+++ b/common/image-android-dt.c
@@ -5,7 +5,6 @@
  */
 
 #include <image-android-dt.h>
-#include <dt_table.h>
 #include <common.h>
 #include <linux/libfdt.h>
 #include <mapmem.h>
@@ -38,14 +37,15 @@ bool android_dt_check_header(ulong hdr_addr)
  *
  * @return true on success or false on error
  */
-bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
-				 u32 *size)
+bool android_dt_get_fdt(ulong hdr_addr, u32 index,
+			struct dt_table_entry *ep, ulong *addr, u32 *size)
 {
 	const struct dt_table_header *hdr;
 	const struct dt_table_entry *e;
 	u32 entry_count, entries_offset, entry_size;
 	ulong e_addr;
-	u32 dt_offset, dt_size;
+	u32 dt_offset, dt_size, dt_id, dt_rev;
+	int i, cnt;
 
 	hdr = map_sysmem(hdr_addr, sizeof(*hdr));
 	entry_count = fdt32_to_cpu(hdr->dt_entry_count);
@@ -59,6 +59,62 @@ bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
 		return false;
 	}
 
+	/*
+	 * In case of a NULL dt_table_entry _or_ if both its 'id' and 'rev'
+	 * fields are empty/zero, return the ${index}'th DTB/DTBO entry
+	 */
+	if (!ep || (!ep->id && !ep->rev))
+		goto found;
+
+	/*
+	 * - In case of non-zero value received in both 'id' and 'rev' fields,
+	 *   return the ${cnt}'th DTB/DTBO entry matching both fields
+	 * - In case of non-zero value received in 'id' and zero in 'rev',
+	 *   return the ${cnt}'th DTB/DTBO entry matching the 'id' field only
+	 * - In case of non-zero value received in 'rev' and zero in 'id',
+	 *   return the ${cnt}'th DTB/DTBO entry matching the 'rev' field only
+	 * In any of the above cases: 0 <= ${cnt} <= ${index} < entry_count
+	 */
+	for (i = 0, cnt = -1; i < entry_count; i++) {
+		e_addr = hdr_addr + entries_offset + i * entry_size;
+		e = map_sysmem(e_addr, sizeof(*e));
+		dt_id = fdt32_to_cpu(e->id);
+		dt_rev = fdt32_to_cpu(e->rev);
+		unmap_sysmem(e);
+
+		if (ep->id && ep->rev) {
+			if (ep->id == dt_id && ep->rev == dt_rev) {
+				cnt++;
+				if (cnt == index) {
+					index = i;
+					goto found;
+				}
+			}
+		} else if (ep->id) {
+			if (ep->id == dt_id) {
+				cnt++;
+				if (cnt == index) {
+					index = i;
+					goto found;
+				}
+			}
+		} else if (ep->rev) {
+			if (ep->rev == dt_rev) {
+				cnt++;
+				if (cnt == index) {
+					index = i;
+					goto found;
+				}
+			}
+		}
+	}
+
+	printf("Error: No #%d entry having id=0x%x && rev=0x%x\n",
+	       index, ep->id, ep->rev);
+
+	return false;
+
+found:
 	e_addr = hdr_addr + entries_offset + index * entry_size;
 	e = map_sysmem(e_addr, sizeof(*e));
 	dt_offset = fdt32_to_cpu(e->dt_offset);
diff --git a/include/image-android-dt.h b/include/image-android-dt.h
index 9a3aa8fa30fb..15f6115eedf0 100644
--- a/include/image-android-dt.h
+++ b/include/image-android-dt.h
@@ -8,10 +8,11 @@
 #define IMAGE_ANDROID_DT_H
 
 #include <linux/types.h>
+#include <dt_table.h>
 
 bool android_dt_check_header(ulong hdr_addr);
-bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
-				 u32 *size);
+bool android_dt_get_fdt(ulong hdr_addr, u32 index,
+			struct dt_table_entry *ep, ulong *addr, u32 *size);
 
 #if !defined(CONFIG_SPL_BUILD)
 void android_dt_print_contents(ulong hdr_addr);
-- 
2.24.0

[PATCH 1/4] cmd: dtimg: Report invalid index argument

[Sam Protsenko]

Hi,

On Fri, Nov 29, 2019 at 9:30 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Being user-friendly is paramount to make any product likeable and
> easy to use. Hence, instead of [1], print [2].
>
> [1] dtimg start 0x48000000 not-a-number myvar
> Error: Wrong index
>
> [2] dtimg start 0x48000000 not-a-number myvar
> Error: Wrong index 'not-a-number'
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  cmd/dtimg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 6c5d53cc6808..2317c859953d 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -63,7 +63,7 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>
>         index = simple_strtoul(argv[2], &endp, 0);
>         if (*endp != '\0') {
> -               printf("Error: Wrong index\n");
> +               printf("Error: Wrong index '%s'\n", argv[2]);
>                 return CMD_RET_FAILURE;
>         }
>
> --
> 2.24.0
>

[PATCH 2/4] cmd: dtimg: Merge duplicated prints

[Sam Protsenko]

Hi,

On Fri, Nov 29, 2019 at 9:30 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Getting DTB/DTBO header address happens twice (in do_dtimg_dump and
> in dtimg_get_fdt) with duplicating below error messages:
>  - Error: Wrong image address
>  - Error: DT image header is incorrect
>

Actually, I intentionally left it that way for the sake of code
simplicity. Didn't want to introduce one more function: more lines of
code, more entities, doesn't solve any issues in this particular case,
as I see it. Speaking of which, have you tried to compare how the
binary footprint changes (.text, .data. .rodata, using buildman) when
you apply this patch? I have a feeling those duplicated strings are in
fact optimized out by compiler. Relied on that fact actually. As they
say: all problems in computer science can be solved by another level
of indirection... except for the problem of too many layers of
indirection :)

In one word: if you feel it's better, I don't mind:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

More detailed review is below.

> Reduce the duplication and improve the error message by appending
> the faulty address value:
>  - Error: Wrong image address '0x48000000z'
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  cmd/dtimg.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 2317c859953d..5989081b0c14 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -13,18 +13,13 @@ enum cmd_dtimg_info {
>         CMD_DTIMG_SIZE,
>  };
>
> -static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
> -                        char * const argv[])
> +static int dtimg_get_argv_addr(char * const str, ulong *hdr_addrp)
>  {
>         char *endp;
> -       ulong hdr_addr;
> +       ulong hdr_addr = simple_strtoul(str, &endp, 16);
>
> -       if (argc != 2)
> -               return CMD_RET_USAGE;
> -
> -       hdr_addr = simple_strtoul(argv[1], &endp, 16);
>         if (*endp != '\0') {
> -               printf("Error: Wrong image address\n");
> +               printf("Error: Wrong image address '%s'\n", str);
>                 return CMD_RET_FAILURE;

As now this function is not a command handler anymore, but just an
internal function, I suggest using 'bool' as its return type, and
return true/false instead of CMD_RET_* (because further you're
checking its return value anyway, it might be prettier this way).

>         }
>
> @@ -32,6 +27,21 @@ static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
>                 printf("Error: DT image header is incorrect\n");
>                 return CMD_RET_FAILURE;
>         }
> +       *hdr_addrp = hdr_addr;
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
> +                        char * const argv[])
> +{
> +       ulong hdr_addr;
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
> +               return CMD_RET_FAILURE;
>
>         android_dt_print_contents(hdr_addr);
>
> @@ -50,16 +60,8 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>         if (argc != 4)
>                 return CMD_RET_USAGE;
>
> -       hdr_addr = simple_strtoul(argv[1], &endp, 16);
> -       if (*endp != '\0') {
> -               printf("Error: Wrong image address\n");
> +       if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
>                 return CMD_RET_FAILURE;
> -       }
> -
> -       if (!android_dt_check_header(hdr_addr)) {
> -               printf("Error: DT image header is incorrect\n");
> -               return CMD_RET_FAILURE;
> -       }
>
>         index = simple_strtoul(argv[2], &endp, 0);
>         if (*endp != '\0') {
> --
> 2.24.0
>

[PATCH 3/4] cmd: dtimg: Make <varname> an optional argument

[Sam Protsenko]

Hi,

On Fri, Nov 29, 2019 at 9:30 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Unlike dtimg, U-Boot commands like part [1], fstype [2] and uuid [3]
> accept an _optional_ <varname> parameter, which means that they will
> output the result to console whenever <varname> is skipped. This is
> extremely useful during development.
>
> Allow "dtimg" to behave in a similar fashion [4]. In addition:
>  - replace env_set() by env_set_hex()

Thanks, didn't know that existed. I like it.

>  - track and report the failures of env_set_hex()

"grep -Ir env_set cmd/" shows nobody really cares to check env_set*
error codes. Probably it's very unlikely that environment is broken at
the point of commands execution?

>  - amend command's help/usage text
>
> [1] => part start mmc 0 1
>     800
>     => part start mmc 0 1 myvar; print myvar
>     myvar=800
> [2] => fstype mmc 0:1
>     ext4
>     => fstype mmc 0:1 myvar; print myvar
>     myvar=ext4
> [3] => uuid
>     b3909b50-55df-4173-b83c-b05343d2d5d2
>     => uuid myvar; print myvar
>     myvar=4c04b15f-d0c1-4f98-9aca-ab62a66be864
> [4] => dtimg start 0x48000000 0
>     0x480000e0
>     => dtimg start 0x48000000 0 myvar; print myvar
>     myvar=480000e0
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  cmd/dtimg.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 5989081b0c14..5348a4ad46e8 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -55,9 +55,10 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>         char *endp;
>         ulong fdt_addr;
>         u32 fdt_size;
> -       char buf[65];
> +       ulong envval;
> +       int ret;
>
> -       if (argc != 4)
> +       if (argc < 3)
>                 return CMD_RET_USAGE;
>
>         if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
> @@ -74,17 +75,24 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>
>         switch (cmd) {
>         case CMD_DTIMG_START:
> -               snprintf(buf, sizeof(buf), "%lx", fdt_addr);
> +               envval = fdt_addr;
>                 break;
>         case CMD_DTIMG_SIZE:
> -               snprintf(buf, sizeof(buf), "%x", fdt_size);
> +               envval = fdt_size;
>                 break;
>         default:
>                 printf("Error: Unknown cmd_dtimg_info value: %d\n", cmd);
>                 return CMD_RET_FAILURE;
>         }
>
> -       env_set(argv[3], buf);
> +       if (argv[3]) {
> +               ret = env_set_hex(argv[3], envval);
> +               if (ret)
> +                       printf("Error(%d) env-setting '%s=0x%lx'\n",
> +                              ret, argv[3], envval);
> +       } else {
> +               printf("0x%lx\n", envval);
> +       }
>
>         return CMD_RET_SUCCESS;
>  }
> @@ -131,12 +139,12 @@ U_BOOT_CMD(
>         "dump <addr>\n"
>         "    - parse specified image and print its structure info\n"
>         "      <addr>: image address in RAM, in hex\n"
> -       "dtimg start <addr> <index> <varname>\n"
> +       "dtimg start <addr> <index> [<varname>]\n"

Bikeshedding: maybe use just [varname]?

>         "    - get address (hex) of FDT in the image, by index\n"
>         "      <addr>: image address in RAM, in hex\n"
>         "      <index>: index of desired FDT in the image\n"
>         "      <varname>: name of variable where to store address of FDT\n"
> -       "dtimg size <addr> <index> <varname>\n"
> +       "dtimg size <addr> <index> [<varname>]\n"
>         "    - get size (hex, bytes) of FDT in the image, by index\n"
>         "      <addr>: image address in RAM, in hex\n"
>         "      <index>: index of desired FDT in the image\n"
> --
> 2.24.0
>

Other than those minor comments:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

[PATCH 4/4] cmd: dtimg: Get start and size based on --id and --rev

[Sam Protsenko]

Hi,

On Fri, Nov 29, 2019 at 9:31 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Currently, it is only possible to get the ${index}'s entry of a DTB/DTBO
> image [*]. The "dtimg" command is agnostic on the "id" and "rev" fields
> and is unable to take them as input for a more fine-grained DTB/DTBO
> search/retrieval.
>
> This is a major limitation, as users would like [**] to employ the
> "id"/"rev" fields to e.g. differentiate between DTBs/DTBOs
> associated to multiple HW revisions or several platforms.
>
> Given a sample DTBO image [***], the new options work like below:
>
>  => dtimg start 0x48000000 0 --id invalid
>     Error: Bad value '--id=invalid'
>  => dtimg start 0x48000000 0 --id 0x100
>     Error: No #0 entry having id=0x100 && rev=0x0
>  => dtimg start 0x48000000 0 --id 00779000
>     0x480006ac
>  => dtimg start 0x48000000 1 --id 00779000
>     0x48000b46
>  => dtimg start 0x48000000 2 --id 00779000
>     Error: No #2 entry having id=0x779000 && rev=0x0
>  => dtimg start 0x48000000 99 --id 00779000
>     Error: index >= dt_entry_count (99 >= 6)
>  => dtimg start 0x48000000 0
>     0x480000e0
>  => dtimg start 0x48000000 1
>     0x480000e0
>  => dtimg start 0x48000000 2
>     0x480004d0
>  => dtimg start 0x48000000 3
>     0x480005be
>  => dtimg start 0x48000000 4
>     0x480006ac
>  => dtimg start 0x48000000 5
>     0x48000b46
>  => dtimg start 0x48000000 6
>     Error: index >= dt_entry_count (6 >= 6)
>  => dtimg size 0x48000000 0 --id 00779000
>     0x49a
>  => dtimg size 0x48000000 1 --id 00779000
>     0x248
>
> [*] https://source.android.com/devices/architecture/dto/partitions
> [**] https://patchwork.ozlabs.org/patch/958594/#2302310
> [***] Sample/dummy DTBO image:
>  => dtimg dump 0x48000000
>  dt_table_header:
>                 magic = d7b7ab1e
>            total_size = 3470
>           header_size = 32
>         dt_entry_size = 32
>        dt_entry_count = 6
>     dt_entries_offset = 32
>             page_size = 4096
>               version = 0
>  dt_table_entry[0]:
>               dt_size = 1008
>             dt_offset = 224
>                    id = 0b779530
>                   rev = 00000000
>             custom[0] = 00000000
>             custom[1] = 00000000
>             custom[2] = 00000000
>             custom[3] = 00000000
>             (FDT)size = 1008
>       (FDT)compatible = (unknown)
>  dt_table_entry[1]:
>               dt_size = 1008
>             dt_offset = 224
>                    id = 0b779520
>                   rev = 00000000
>             custom[0] = 00000000
>             custom[1] = 00000000
>             custom[2] = 00000000
>             custom[3] = 00000000
>             (FDT)size = 1008
>       (FDT)compatible = (unknown)
>  dt_table_entry[2]:
>               dt_size = 238
>             dt_offset = 1232
>                    id = 0b779530
>                   rev = 00000000
>             custom[0] = 00000000
>             custom[1] = 00000000
>             custom[2] = 00000000
>             custom[3] = 00000000
>             (FDT)size = 238
>       (FDT)compatible = (unknown)
>  dt_table_entry[3]:
>               dt_size = 238
>             dt_offset = 1470
>                    id = 0b779520
>                   rev = 00000000
>             custom[0] = 00000000
>             custom[1] = 00000000
>             custom[2] = 00000000
>             custom[3] = 00000000
>             (FDT)size = 238
>       (FDT)compatible = (unknown)
>  dt_table_entry[4]:
>               dt_size = 1178
>             dt_offset = 1708
>                    id = 00779000
>                   rev = 00000000
>             custom[0] = 00000000
>             custom[1] = 00000000
>             custom[2] = 00000000
>             custom[3] = 00000000
>             (FDT)size = 1178
>       (FDT)compatible = (unknown)
>  dt_table_entry[5]:
>               dt_size = 584
>             dt_offset = 2886
>                    id = 00779000
>                   rev = 00000000
>             custom[0] = 00000000
>             custom[1] = 00000000
>             custom[2] = 00000000
>             custom[3] = 00000000
>             (FDT)size = 584
>       (FDT)compatible = (unknown)
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  cmd/dtimg.c                | 81 +++++++++++++++++++++++++++++++++++---
>  common/image-android-dt.c  | 64 ++++++++++++++++++++++++++++--
>  include/image-android-dt.h |  5 ++-

[strong opinion] Can you please split it into two patches? First to
introduce new functionality to common/image-android-dt.c, second one
to use it in dtimg. Atomicity reasons... I understand that we don't
introduce unused code, but it will be used in the same patch series,
though later (in some hypothetical scenario) we would be able to e.g.
revert cmd related changes, if someone else (see 'abootimg') already
uses API from common/image-android-dt.c. Also easier to review this
way.

>  3 files changed, 138 insertions(+), 12 deletions(-)
>
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 5348a4ad46e8..10e909ce551b 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -7,6 +7,7 @@
>  #include <env.h>
>  #include <image-android-dt.h>
>  #include <common.h>
> +#include <linux/ctype.h>
>
>  enum cmd_dtimg_info {
>         CMD_DTIMG_START = 0,
> @@ -48,6 +49,61 @@ static int do_dtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
>         return CMD_RET_SUCCESS;
>  }
>
> +static int dtimg_get_opthex(int *argcp, char * const *argvp[], u32 *valp)

Maybe make it _hex? For naming uniformity reasons, like in env_set_hex().

Also, (but that's only my opinion), don't really like "p" sufficies
for "pointer", I'd avoid using it (didn't see that much in U-Boot or
in kernel sources). For my taste, it only clutters the name (similar
to Hungarian notation).

Also, maybe it would be prettier to change the return type to 'bool'
and return true/false instead of CMD_RET* (as you don't "return
dtimg_get_opthex() further).

Last bit: as we don't change argcp and argvp, maybe it's better to
make them const. And I'd probably add some kernel-doc comment for such
function, mentioning, which parameters are [in] and which are [out].
Minor though.

> +{
> +       char *endp;
> +       u32 val;
> +
> +       if (!argcp || !argvp || !valp)

Hmm, do we have something like assert() in U-Boot? Seems like it fits
here. Just a thought.

> +               return CMD_RET_FAILURE;
> +
> +       if (*argcp < 2) {
> +               printf("Error: Option '%s' expects an argument\n", (*argvp)[0]);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       val = simple_strtoul((*argvp)[1], &endp, 16);
> +       if (*endp != '\0') {
> +               printf("Error: Bad value '%s=%s'\n", (*argvp)[0], (*argvp)[1]);

Maybe would be easier to read if using alias variables. Compiler
should optimize that I guess.

> +               return CMD_RET_FAILURE;
> +       }
> +
> +       *valp = val;
> +       (*argcp)--;
> +       (*argvp)++;
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int dtimg_get_opt(int argc, char * const argv[],
> +                        struct dt_table_entry *ep, char **envstrp)
> +{
> +       if (!ep || argc < 0 || !argv || !envstrp)
> +               return CMD_RET_FAILURE;
> +
> +       for (; argc > 0; argc--, argv++) {
> +               int ret;
> +
> +               if (!strcmp(argv[0], "--id")) {

After giving it some thought, not a lot of people are using 'dtimg'
yet. So if you think that existing interface is ugly, maybe now is a
good time to change it. Or after merging this patch series and my
Android Boot Flow patch series, to make development easier. Looking at
that '--id'... I never saw such params in other U-Boot commands, so
probably it's better to avoid using that, for consistency.

Even the code itself gets hairy because I failed to think ahead when
coming up with proper extensible interface for 'dtimg'... Can we
really go ahead and rework it like this:

    dtimg start index <num> <addr> [varname]
    dtimg start id <num> <addr> [varname]
    dtimg start rev <num> <addr> [varname]

Not only it will be more consistent with what I'm trying to do in
'abootimg' right now, even the command code should become easier (I
guess). Again, I don't have *strong* preference here. Methodologically
it's bad to change existing interface, but it wasn't too long since I
introduced it, so maybe not a lot of people are using it at this
moment. Your decision.

Also, probably sandbox unit test should be added for new
functionality. The 'abootimg' test can be used as a template. Later we
should add some proper documentation to doc/ as well...

> +                       ret = dtimg_get_opthex(&argc, &argv, &ep->id);
> +                       if (ret != CMD_RET_SUCCESS)
> +                               return ret;
> +               } else if (!strcmp(argv[0], "--rev")) {
> +                       ret = dtimg_get_opthex(&argc, &argv, &ep->rev);
> +                       if (ret != CMD_RET_SUCCESS)
> +                               return ret;
> +               } else if (argc == 1 && argv[0][0] != '-' &&
> +                          !isdigit(argv[0][0])) {
> +                       *envstrp = argv[0];
> +               } else {
> +                       printf("Error: Option '%s' not supported\n", argv[0]);
> +                       return CMD_RET_FAILURE;
> +               }
> +       }
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
>  static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>  {
>         ulong hdr_addr;
> @@ -55,6 +111,8 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>         char *endp;
>         ulong fdt_addr;
>         u32 fdt_size;
> +       struct dt_table_entry entry = { 0 };
> +       char *envstr = NULL;
>         ulong envval;
>         int ret;
>
> @@ -70,7 +128,18 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>                 return CMD_RET_FAILURE;
>         }
>
> -       if (!android_dt_get_fdt_by_index(hdr_addr, index, &fdt_addr, &fdt_size))
> +       /*
> +        * Consume all processed mandatory arguments.
> +        * Prepare for parsing the optional ones.
> +        */
> +       argc -= 3;
> +       argv += 3;
> +
> +       ret = dtimg_get_opt(argc, argv, &entry, &envstr);
> +       if (ret != CMD_RET_SUCCESS)
> +               return ret;
> +
> +       if (!android_dt_get_fdt(hdr_addr, index, &entry, &fdt_addr, &fdt_size))
>                 return CMD_RET_FAILURE;
>
>         switch (cmd) {
> @@ -85,11 +154,11 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)
>                 return CMD_RET_FAILURE;
>         }
>
> -       if (argv[3]) {
> -               ret = env_set_hex(argv[3], envval);
> +       if (envstr) {
> +               ret = env_set_hex(envstr, envval);
>                 if (ret)
>                         printf("Error(%d) env-setting '%s=0x%lx'\n",
> -                              ret, argv[3], envval);
> +                              ret, envstr, envval);
>         } else {
>                 printf("0x%lx\n", envval);
>         }
> @@ -111,8 +180,8 @@ static int do_dtimg_size(cmd_tbl_t *cmdtp, int flag, int argc,
>
>  static cmd_tbl_t cmd_dtimg_sub[] = {
>         U_BOOT_CMD_MKENT(dump, 2, 0, do_dtimg_dump, "", ""),
> -       U_BOOT_CMD_MKENT(start, 4, 0, do_dtimg_start, "", ""),
> -       U_BOOT_CMD_MKENT(size, 4, 0, do_dtimg_size, "", ""),
> +       U_BOOT_CMD_MKENT(start, 8, 0, do_dtimg_start, "", ""),
> +       U_BOOT_CMD_MKENT(size, 8, 0, do_dtimg_size, "", ""),
>  };
>

Have you fixed command usage (help) strings as well? Can't find it in
this patch... If it's somewhere else, please point me out.

>  static int do_dtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> diff --git a/common/image-android-dt.c b/common/image-android-dt.c
> index a2d52df4a2a9..91e6b4eca23a 100644
> --- a/common/image-android-dt.c
> +++ b/common/image-android-dt.c
> @@ -5,7 +5,6 @@
>   */
>
>  #include <image-android-dt.h>
> -#include <dt_table.h>
>  #include <common.h>
>  #include <linux/libfdt.h>
>  #include <mapmem.h>
> @@ -38,14 +37,15 @@ bool android_dt_check_header(ulong hdr_addr)
>   *
>   * @return true on success or false on error
>   */
> -bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
> -                                u32 *size)
> +bool android_dt_get_fdt(ulong hdr_addr, u32 index,
> +                       struct dt_table_entry *ep, ulong *addr, u32 *size)

[strong opinion] I don't really like the way index/id/rev is handled
in this function... Would really like to see 3 different functions to
handle getting DTB/DTBO file by index/id/rev correspondingly. *OR* one
single function, but providing some flag, telling what to use
(index/id/rev). What I don't like in current implementation is a lot
of suggesting about which criteria to use for DTB lookup. As I see it,
user must know exactly what should be used. And function shouldn't be
too smart in that regard. If you have a different opinion, please
elaborate.

Also, please fix the Doxygen/kernel-doc comment for this function
(above) accordingly, describing its usage, parameters, etc. User
should only read the comment to understand how to use the function,
not the code itself. At least in an ideal world :)

>  {
>         const struct dt_table_header *hdr;
>         const struct dt_table_entry *e;
>         u32 entry_count, entries_offset, entry_size;
>         ulong e_addr;
> -       u32 dt_offset, dt_size;
> +       u32 dt_offset, dt_size, dt_id, dt_rev;
> +       int i, cnt;
>
>         hdr = map_sysmem(hdr_addr, sizeof(*hdr));
>         entry_count = fdt32_to_cpu(hdr->dt_entry_count);
> @@ -59,6 +59,62 @@ bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
>                 return false;
>         }
>
> +       /*
> +        * In case of a NULL dt_table_entry _or_ if both its 'id' and 'rev'
> +        * fields are empty/zero, return the ${index}'th DTB/DTBO entry
> +        */
> +       if (!ep || (!ep->id && !ep->rev))
> +               goto found;
> +
> +       /*
> +        * - In case of non-zero value received in both 'id' and 'rev' fields,
> +        *   return the ${cnt}'th DTB/DTBO entry matching both fields
> +        * - In case of non-zero value received in 'id' and zero in 'rev',
> +        *   return the ${cnt}'th DTB/DTBO entry matching the 'id' field only
> +        * - In case of non-zero value received in 'rev' and zero in 'id',
> +        *   return the ${cnt}'th DTB/DTBO entry matching the 'rev' field only
> +        * In any of the above cases: 0 <= ${cnt} <= ${index} < entry_count
> +        */

Comments like this should go to Doxygen comment above...

> +       for (i = 0, cnt = -1; i < entry_count; i++) {
> +               e_addr = hdr_addr + entries_offset + i * entry_size;
> +               e = map_sysmem(e_addr, sizeof(*e));
> +               dt_id = fdt32_to_cpu(e->id);
> +               dt_rev = fdt32_to_cpu(e->rev);
> +               unmap_sysmem(e);
> +
> +               if (ep->id && ep->rev) {
> +                       if (ep->id == dt_id && ep->rev == dt_rev) {
> +                               cnt++;
> +                               if (cnt == index) {
> +                                       index = i;
> +                                       goto found;
> +                               }
> +                       }
> +               } else if (ep->id) {
> +                       if (ep->id == dt_id) {
> +                               cnt++;
> +                               if (cnt == index) {
> +                                       index = i;
> +                                       goto found;
> +                               }
> +                       }
> +               } else if (ep->rev) {
> +                       if (ep->rev == dt_rev) {
> +                               cnt++;
> +                               if (cnt == index) {
> +                                       index = i;
> +                                       goto found;
> +                               }
> +                       }
> +               }
> +       }
> +
> +       printf("Error: No #%d entry having id=0x%x && rev=0x%x\n",
> +              index, ep->id, ep->rev);
> +
> +       return false;
> +
> +found:
>         e_addr = hdr_addr + entries_offset + index * entry_size;
>         e = map_sysmem(e_addr, sizeof(*e));
>         dt_offset = fdt32_to_cpu(e->dt_offset);
> diff --git a/include/image-android-dt.h b/include/image-android-dt.h
> index 9a3aa8fa30fb..15f6115eedf0 100644
> --- a/include/image-android-dt.h
> +++ b/include/image-android-dt.h
> @@ -8,10 +8,11 @@
>  #define IMAGE_ANDROID_DT_H
>
>  #include <linux/types.h>
> +#include <dt_table.h>
>
>  bool android_dt_check_header(ulong hdr_addr);
> -bool android_dt_get_fdt_by_index(ulong hdr_addr, u32 index, ulong *addr,
> -                                u32 *size);
> +bool android_dt_get_fdt(ulong hdr_addr, u32 index,
> +                       struct dt_table_entry *ep, ulong *addr, u32 *size);
>
>  #if !defined(CONFIG_SPL_BUILD)
>  void android_dt_print_contents(ulong hdr_addr);
> --

Sorry, right now I don't agree with overall design decisions made in
this patch. Although I can see how my failure to provide decent
'dtimg' interface from the beginning led to these implications, I
still think we shouldn't do it this way. Let's discuss.

To repeat myself, that's how I see the better solution:

1. Interface: 3 different uses:

    dtimg start index <num> <addr> [varname]
    dtimg start id <num> <addr> [varname]
    dtimg start rev <num> <addr> [varname]

and the same for dtimg size probably. Anyway, the point is, those
should be separated, in a way that no guessing should be done in
programming.

2. No guesswork in programming. That means we only look for index, if
"index" specified, etc. My preference is "stupid" functions, and smart
user :)

Hope it makes sense. Please let me know what you think (I could easily
missed something, patch is quite big and I'm also focusing on Boot
Flow series in parallel).

Another thing we should discuss is the process, i.e. how exactly our
series should intertwine, their dependencies on each other, and merge
order. Right now I can't finish the "abootimg get_dtb_file id/rev ..."
sub-command implementation, as it depends on this patch of yours... So
we have two choices here:
  1. I only implement

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

  and don't implement next two usages at all (for now):

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

We can easily add those once your series is merged.

2. Your series *must* be merged first, so my series can use it.

I prefer (1), as it makes it possible to break the unnecessary
dependency. What do you think?

> 2.24.0
>