[PATCH 1/3] abootcmd: Add load subcommand
Mattijs Korpershoek
mkorpershoek at baylibre.com
Tue May 28 09:22:37 CEST 2024
Hi Roman,
Thank you for the patch.
On dim., mai 19, 2024 at 19:18, Roman Stratiienko <r.stratiienko at gmail.com> wrote:
> What it does:
> 1. Allocates the memory in HEAP to fit the partition
> 2. Loads partition into memory. In the following patch of this series,
> loading will be optimized to avoid loading an empty space.
> 3. Sets buffer start and buffer size value into environment variables
> abootimg_<partition>_ptr and abootimg_<partition>_size, respectively.
> and duplicate them as
> abootimg_<partition>_<slot>_ptr and abootimg_<partition>_<slot>_size.
> The latter two are needed to access by the AVB get_preloaded_partition.
> (see the next patch).
>
> Before this command, the boot script developer was responsible for
> allocating the memory manually by choosing the start and the end,
> which is far from good.
>
> Usage example:
>
> abootcmd load mmc 0 boot a
Should this be: abootimg load mmc 0 boot a ?
>
> Signed-off-by: Roman Stratiienko <r.stratiienko at gmail.com>
> ---
> cmd/abootimg.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
> index a5321bab6a..808c9c4941 100644
> --- a/cmd/abootimg.c
> +++ b/cmd/abootimg.c
> @@ -8,7 +8,9 @@
> #include <common.h>
> #include <command.h>
> #include <image.h>
> +#include <malloc.h>
> #include <mapmem.h>
> +#include <part.h>
>
> #define abootimg_addr() \
> (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr)
> @@ -259,10 +261,81 @@ static int do_abootimg_dump(struct cmd_tbl *cmdtp, int flag, int argc,
> return CMD_RET_SUCCESS;
> }
>
> +static int do_abootimg_load(struct cmd_tbl *cmdtp, int flag, int argc,
> + char *const argv[])
> +{
> + int time_start = get_timer(0);
On -next branch (where the patch will be applied), I see the following
build error:
cmd/abootimg.c: In function ‘do_abootimg_load’:
cmd/abootimg.c:279:26: error: implicit declaration of function ‘get_timer’ [-Wimplicit-function-declaration]
279 | int time_start = get_timer(0);
| ^~~~~~~~~
Please add the missing include: #include <time.h>
Base: 7e52d6ccfb76 ("Merge patch series "FWU: Add support for FWU metadata version 2"")
> + struct blk_desc *desc;
> + struct disk_partition info;
> + char buf[512] = { 0 };
> + void *addr;
> + int ret;
> +
> + if (argc < 4)
> + return CMD_RET_USAGE;
> + if (argc > 5)
> + return CMD_RET_USAGE;
Can we please do this in a single line, like done in other commands such
as do_abootimg_addr()?
> +
> + ret = blk_get_device_by_str(argv[1], argv[2], &desc);
> + if (ret < 0) {
> + printf("Error: Failed to get device %s %s\n", argv[1], argv[2]);
> + return CMD_RET_FAILURE;
> + }
> +
> + if (argc == 5)
> + sprintf(buf, "%s_%s", argv[3], argv[4]);
> + else
> + sprintf(buf, "%s", argv[3]);
> +
> + ret = part_get_info_by_name(desc, buf, &info);
> + if (ret < 0) {
> + printf("Error: Failed to get partition %s\n", buf);
> + return CMD_RET_FAILURE;
> + }
> +
> + addr = (void *)memalign(4096, info.size * info.blksz);
Why 4096? If this is a known number, can we use a define for it please?
> + if (!addr) {
> + printf("Error: Failed to allocate memory\n");
> + return CMD_RET_FAILURE;
> + }
> +
> + ret = blk_dread(desc, info.start, info.size, addr);
> + if (ret < 0) {
> + printf("Error: Failed to read partition %s\n", buf);
> + goto fail;
> + }
> +
> + sprintf(buf, "abootimg_%s_ptr", argv[3]);
> + env_set_hex(buf, (ulong)addr);
> +
> + sprintf(buf, "abootimg_%s_size", argv[3]);
> + env_set_hex(buf, info.size * info.blksz);
> +
> + if (argc == 5) {
> + sprintf(buf, "abootimg_%s_%s_ptr", argv[3], argv[4]);
> + env_set_hex(buf, (ulong)addr);
> +
> + sprintf(buf, "abootimg_%s_%s_size", argv[3], argv[4]);
> + env_set_hex(buf, info.size * info.blksz);
> + }
> +
> + int time_end = get_timer(0);
> +
> + printf("Loaded '%s' partition to address 0x%p (size: 0x%x) in %lu ms\n",
> + argv[3], addr, info.size * info.blksz, time_end - time_start);
This causes warnings on -next when building sandbox:
$ make sandbox_defconfig
$ make
cmd/abootimg.c:339:65: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘long unsigned int’ [-Wformat=]
339 | printf("Loaded '%s' partition to address 0x%p (size: 0x%x) in %lu ms\n",
| ~^
| |
| unsigned int
| %lx
340 | argv[3], addr, info.size * info.blksz, time_end - time_start);
| ~~~~~~~~~~~~~~~~~~~~~~
| |
> +
> + return CMD_RET_SUCCESS;
> +
> +fail:
> + free(addr);
> + return CMD_RET_FAILURE;
> +}
> +
> static struct cmd_tbl cmd_abootimg_sub[] = {
> U_BOOT_CMD_MKENT(addr, 4, 1, do_abootimg_addr, "", ""),
> U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""),
> U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""),
> + U_BOOT_CMD_MKENT(load, 5, 1, do_abootimg_load, "", ""),
> };
>
> static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc,
> @@ -305,5 +378,14 @@ U_BOOT_CMD(
> " - get address and size (hex) of DT blob in the image by index\n"
> " <num>: index number of desired DT blob in DTB area\n"
> " [addr_var]: variable name to contain DT blob address\n"
> - " [size_var]: variable name to contain DT blob size"
> + " [size_var]: variable name to contain DT blob size\n"
> + "abootimg load interface dev partition [slot_name]\n"
> + " - load boot image from device partition\n"
> + " memory is allocated in heap\n"
> + " address is stored in $abootimg_<partition>_ptr\n"
> + " size is stored in $abootimg_<partition>_size\n"
> + " interface: interface type (e.g. mmc, usb)\n"
> + " dev: device number (e.g. 0, 1)\n"
> + " partition: partition number (e.g. boot, dtb)\n"
"partition name" instead of "partition number"?
> + " slot_suffix: slot name (e.g. a, b)"
> );
> --
> 2.40.1
More information about the U-Boot
mailing list