Re: [PATCH v5 10/23] FWU: cmd: Add a command to read FWU metadata

On 6/9/22 14:29, Sughosh Ganu wrote:

Add a command to read the metadata as specified in the FWU specification and print the fields of the metadata.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

cmd/Kconfig | 7 +++++ cmd/Makefile | 1 + cmd/fwu_mdata.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 cmd/fwu_mdata.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index 09193b61b9..275becd837 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -144,6 +144,13 @@ config CMD_CPU internal name) and clock frequency. Other information may be available depending on the CPU driver.

+config CMD_FWU_METADATA

• bool "fwu metadata read"
• depends on FWU_MULTI_BANK_UPDATE
• default y if FWU_MULTI_BANK_UPDATE

+ default y

it is enough with the depends on

• help

•  Command to read the metadata and dump it's contents
  

• config CMD_LICENSE bool "license" select BUILD_BIN2C

diff --git a/cmd/Makefile b/cmd/Makefile index 5e43a1e022..259a93bc65 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_CMD_FPGA) += fpga.o obj-$(CONFIG_CMD_FPGAD) += fpgad.o obj-$(CONFIG_CMD_FS_GENERIC) += fs.o obj-$(CONFIG_CMD_FUSE) += fuse.o +obj-$(CONFIG_CMD_FWU_METADATA) += fwu_mdata.o obj-$(CONFIG_CMD_GETTIME) += gettime.o obj-$(CONFIG_CMD_GPIO) += gpio.o obj-$(CONFIG_CMD_HVC) += smccc.o diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c new file mode 100644 index 0000000000..bc20ca26a3 --- /dev/null +++ b/cmd/fwu_mdata.c @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#include <command.h> +#include <dm.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <stdio.h> +#include <stdlib.h>

+#include <linux/types.h>

+static void print_mdata(struct fwu_mdata *mdata) +{

• int i, j;
• struct fwu_image_entry *img_entry;
• struct fwu_image_bank_info *img_info;
• u32 nimages, nbanks;
• printf("\tFWU Metadata\n");
• printf("crc32: %#x\n", mdata->crc32);
• printf("version: %#x\n", mdata->version);
• printf("active_index: %#x\n", mdata->active_index);
• printf("previous_active_index: %#x\n", mdata->previous_active_index);
• nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
• nbanks = CONFIG_FWU_NUM_BANKS;
• printf("\tImage Info\n");
• for (i = 0; i < nimages; i++) {

• img_entry = &mdata->img_entry[i];
  

• printf("\nImage Type Guid: %pUL\n", &img_entry->image_type_uuid);
  

• printf("Location Guid: %pUL\n", &img_entry->location_uuid);
  

• for (j = 0; j < nbanks; j++) {
  

• 	img_info = &img_entry->img_bank_info[j];
  

• 	printf("Image Guid:  %pUL\n", &img_info->image_uuid);
  

• 	printf("Image Acceptance: %#x\n", img_info->accepted);
  

• }
  

• }

+}

+int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,

•      int argc, char * const argv[])
  

+{

• struct udevice *dev;
• int ret = CMD_RET_SUCCESS;
• struct fwu_mdata *mdata = NULL;
• if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {

• log_err("Unable to get FWU metadata device\n");
  

• return CMD_RET_FAILURE;
  

• }

ret is reused here ?

so the default value is overwrite

I think it is better to have 2 variables

ret => cmd result

res => result of fwu_get_mdata() ?

• ret = fwu_get_mdata(&mdata);
• if (ret < 0) {

• log_err("Unable to get valid FWU metadata\n");
  

• ret = CMD_RET_FAILURE;
  

• goto out;
  

• }

+ res = fwu_get_mdata(&mdata); + if (res < 0) { + log_err("Unable to get valid FWU metadata\n"); + ret = CMD_RET_FAILURE; + goto out; + }

• print_mdata(mdata);

+out:

• free(mdata);
• return ret;

+}

+U_BOOT_CMD(

• fwu_mdata_read, 1, 1, do_fwu_mdata_read,
• "Read and print FWU metadata",
• ""

+);

regards

Patrick