Skip to content

Commit

Permalink
cmd: adtimg: Refactor usage style
Browse files Browse the repository at this point in the history 
Trying to extend 'adtimg' functionality [1], we've been severely hit
by a major limitation in the command's usage scheme. Specifically, the
command's user interface appears to be too centric to getting the
DTB/DTBO entry [3] based on the index of the desired DT in the image,
which makes it really difficult retrieving the DT entry based on
alternative criteria (e.g. filtering by id/rev fields), the
latter being demanded by real life customer use-cases [1].

This went to the point of receiving below feedback from Sam [2]:

 -- snip --
 As for 'dtimg' command: after giving it some thought, I think not much
 people using it yet. So in this particular case I don't have some
 strong preference, and if you think the 'dtimg' interface is ugly, and
 it overcomes "don't break interfaces" rule, maybe now is a good time
 to rework it (before it gets widely used).
 -- snip --

Given the above, rework the usage pattern from [4] to [5], in order to
allow an intuitive enablement of "by id|rev" DT search [6].

[1] https://patchwork.ozlabs.org/cover/1202575/
    ("cmd: dtimg: Enhance with --id and --rev options (take #1)")
[2] https://patchwork.ozlabs.org/patch/1182207/#2317020
[3] https://source.android.com/devices/architecture/dto/partitions
[4] Old usage
adtimg dump <addr>                    - Print image contents
adtimg start <addr> <index> <varname> - Get DT address by index
adtimg size <addr> <index> <varname>  - Get DT size by index

[5] New usage
adtimg addr <addr>                      - Set image location to <addr>
adtimg dump                             - Print out image contents
adtimg get dt --index=<i> [avar [svar]] - Get DT address and size by index

[6] Soon-to-be-provided "by id|rev" add-on functionality
adtimg get dt --id=<id> --rev=<rev> [avar [svar [ivar]]]
 - Get DT address/size/index by id|rev fields

Signed-off-by: Eugeniu Rosca <[email protected]>
Reviewed-by: Sam Protsenko <[email protected]>
  • Loading branch information
@erosca @trini
erosca authored and trini committed Jan 10, 2020
1 parent 4c6edc2 commit 4f731c7
Showing 1 changed file with 158 additions and 59 deletions.
217 changes: 158 additions & 59 deletions cmd/adtimg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,22 @@
/*
* (C) Copyright 2018 Linaro Ltd.
* Sam Protsenko <[email protected]>
* Eugeniu Rosca <[email protected]>
*/

#include <env.h>
#include <image-android-dt.h>
#include <common.h>

enum cmd_adtimg_info {
CMD_ADTIMG_START = 0,
CMD_ADTIMG_SIZE,
};
#define OPT_INDEX "--index"

static int do_adtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
/*
* Current/working DTB/DTBO Android image address.
* Similar to 'working_fdt' variable in 'fdt' command.
*/
static ulong working_img;

static int do_adtimg_addr(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
{
char *endp;
Expand All @@ -24,86 +28,185 @@ static int do_adtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,

hdr_addr = simple_strtoul(argv[1], &endp, 16);
if (*endp != '\0') {
printf("Error: Wrong image address\n");
printf("Error: Wrong image address '%s'\n", argv[1]);
return CMD_RET_FAILURE;
}

if (!android_dt_check_header(hdr_addr)) {
printf("Error: DT image header is incorrect\n");
/*
* Allow users to set an address prior to copying the DTB/DTBO
* image to that same address, i.e. skip header verification.
*/

working_img = hdr_addr;
return CMD_RET_SUCCESS;
}

static int adtimg_check_working_img(void)
{
if (!working_img) {
printf("Error: Please, call 'adtimg addr <addr>'. Aborting!\n");
return CMD_RET_FAILURE;
}

android_dt_print_contents(hdr_addr);
if (!android_dt_check_header(working_img)) {
printf("Error: Invalid image header at 0x%lx\n", working_img);
return CMD_RET_FAILURE;
}

return CMD_RET_SUCCESS;
}

static int adtimg_get_fdt(int argc, char * const argv[],
enum cmd_adtimg_info cmd)
static int do_adtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
{
ulong hdr_addr;
u32 index;
char *endp;
ulong fdt_addr;
u32 fdt_size;
char buf[65];

if (argc != 4)
if (argc != 1)
return CMD_RET_USAGE;

hdr_addr = simple_strtoul(argv[1], &endp, 16);
if (*endp != '\0') {
printf("Error: Wrong image address\n");
if (adtimg_check_working_img() != CMD_RET_SUCCESS)
return CMD_RET_FAILURE;

android_dt_print_contents(working_img);

return CMD_RET_SUCCESS;
}

static int adtimg_getopt_u32(char * const opt, char * const name, u32 *optval)
{
char *endp, *str;
u32 val;

if (!opt || !name || !optval)
return CMD_RET_FAILURE;

str = strchr(opt, '=');
if (!str) {
printf("Error: Option '%s' not followed by '='\n", name);
return CMD_RET_FAILURE;
}

if (!android_dt_check_header(hdr_addr)) {
printf("Error: DT image header is incorrect\n");
if (*++str == '\0') {
printf("Error: Option '%s=' not followed by value\n", name);
return CMD_RET_FAILURE;
}

index = simple_strtoul(argv[2], &endp, 0);
val = simple_strtoul(str, &endp, 0);
if (*endp != '\0') {
printf("Error: Wrong index\n");
printf("Error: Wrong integer value '%s=%s'\n", name, str);
return CMD_RET_FAILURE;
}

if (!android_dt_get_fdt_by_index(hdr_addr, index, &fdt_addr, &fdt_size))
*optval = val;
return CMD_RET_SUCCESS;
}

static int adtimg_getopt_index(int argc, char * const argv[], u32 *index,
char **avar, char **svar)
{
int ret;

if (!argv || !avar || !svar)
return CMD_RET_FAILURE;

switch (cmd) {
case CMD_ADTIMG_START:
snprintf(buf, sizeof(buf), "%lx", fdt_addr);
break;
case CMD_ADTIMG_SIZE:
snprintf(buf, sizeof(buf), "%x", fdt_size);
break;
default:
printf("Error: Unknown cmd_adtimg_info value: %d\n", cmd);
if (argc > 3) {
printf("Error: Unexpected argument '%s'\n", argv[3]);
return CMD_RET_FAILURE;
}

env_set(argv[3], buf);
ret = adtimg_getopt_u32(argv[0], OPT_INDEX, index);
if (ret != CMD_RET_SUCCESS)
return ret;

if (argc > 1)
*avar = argv[1];
if (argc > 2)
*svar = argv[2];

return CMD_RET_SUCCESS;
}

static int do_adtimg_start(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
static int adtimg_get_dt_by_index(int argc, char * const argv[])
{
return adtimg_get_fdt(argc, argv, CMD_ADTIMG_START);
ulong addr;
u32 index, size;
int ret;
char *avar = NULL, *svar = NULL;

ret = adtimg_getopt_index(argc, argv, &index, &avar, &svar);
if (ret != CMD_RET_SUCCESS)
return ret;

if (!android_dt_get_fdt_by_index(working_img, index, &addr, &size))
return CMD_RET_FAILURE;

if (avar && svar) {
ret = env_set_hex(avar, addr);
if (ret) {
printf("Error: Can't set '%s' to 0x%lx\n", avar, addr);
return CMD_RET_FAILURE;
}
ret = env_set_hex(svar, size);
if (ret) {
printf("Error: Can't set '%s' to 0x%x\n", svar, size);
return CMD_RET_FAILURE;
}
} else if (avar) {
ret = env_set_hex(avar, addr);
if (ret) {
printf("Error: Can't set '%s' to 0x%lx\n", avar, addr);
return CMD_RET_FAILURE;
}
printf("0x%x (%d)\n", size, size);
} else {
printf("0x%lx, 0x%x (%d)\n", addr, size, size);
}

return CMD_RET_SUCCESS;
}

static int do_adtimg_size(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
static int adtimg_get_dt(int argc, char * const argv[])
{
return adtimg_get_fdt(argc, argv, CMD_ADTIMG_SIZE);
if (argc < 2) {
printf("Error: No options passed to '%s'\n", argv[0]);
return CMD_RET_FAILURE;
}

/* Strip off leading 'dt' command argument */
argc--;
argv++;

if (!strncmp(argv[0], OPT_INDEX, sizeof(OPT_INDEX) - 1))
return adtimg_get_dt_by_index(argc, argv);

printf("Error: Option '%s' not supported\n", argv[0]);
return CMD_RET_FAILURE;
}

static int do_adtimg_get(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
{
if (argc < 2) {
printf("Error: No arguments passed to '%s'\n", argv[0]);
return CMD_RET_FAILURE;
}

if (adtimg_check_working_img() != CMD_RET_SUCCESS)
return CMD_RET_FAILURE;

/* Strip off leading 'get' command argument */
argc--;
argv++;

if (!strcmp(argv[0], "dt"))
return adtimg_get_dt(argc, argv);

printf("Error: Wrong argument '%s'\n", argv[0]);
return CMD_RET_FAILURE;
}

static cmd_tbl_t cmd_adtimg_sub[] = {
U_BOOT_CMD_MKENT(dump, 2, 0, do_adtimg_dump, "", ""),
U_BOOT_CMD_MKENT(start, 4, 0, do_adtimg_start, "", ""),
U_BOOT_CMD_MKENT(size, 4, 0, do_adtimg_size, "", ""),
U_BOOT_CMD_MKENT(addr, CONFIG_SYS_MAXARGS, 1, do_adtimg_addr, "", ""),
U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_adtimg_dump, "", ""),
U_BOOT_CMD_MKENT(get, CONFIG_SYS_MAXARGS, 1, do_adtimg_get, "", ""),
};

static int do_adtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
Expand All @@ -127,17 +230,13 @@ static int do_adtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
U_BOOT_CMD(
adtimg, CONFIG_SYS_MAXARGS, 0, do_adtimg,
"manipulate dtb/dtbo Android image",
"dump <addr>\n"
" - parse specified image and print its structure info\n"
" <addr>: image address in RAM, in hex\n"
"adtimg 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"
"adtimg 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"
" <varname>: name of variable where to store size of FDT"
"addr <addr> - Set image location to <addr>\n"
"adtimg dump - Print out image contents\n"
"adtimg get dt --index=<index> [avar [svar]] - Get DT address/size by index\n"
"\n"
"Legend:\n"
" - <addr>: DTB/DTBO image address (hex) in RAM\n"
" - <index>: index (hex/dec) of desired DT in the image\n"
" - <avar>: variable name to contain DT address (hex)\n"
" - <svar>: variable name to contain DT size (hex)"
);

0 comments on commit 4f731c7

Please sign in to comment.