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

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

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

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