[PATCH v1 2/3] cmd: bcm: add nitro image load commands

[Simon Glass]

On Sun, 17 May 2020 at 02:37, Rayagonda Kokatanur
<rayagonda.kokatanur at broadcom.com> wrote:
>
> From: Vikas Gupta <vikas.gupta at broadcom.com>
>
> Add nitro image load commands.
>
> Signed-off-by: Vikas Gupta <vikas.gupta at broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>
> ---
>  cmd/bcm/Makefile           |  1 +
>  cmd/bcm/nitro_image_load.c | 99 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 cmd/bcm/nitro_image_load.c
>

Reviewed-by: Simon Glass <sjg at chromium.org>

> diff --git a/cmd/bcm/Makefile b/cmd/bcm/Makefile
> index dc274f6b96..671c0fbd43 100644
> --- a/cmd/bcm/Makefile
> +++ b/cmd/bcm/Makefile
> @@ -3,3 +3,4 @@
>
>  obj-$(CONFIG_CMD_BCM_LOGSETUP) += logsetup.o
>  obj-y += chimp_boot.o
> +obj-y += nitro_image_load.o
> diff --git a/cmd/bcm/nitro_image_load.c b/cmd/bcm/nitro_image_load.c
> new file mode 100644
> index 0000000000..e460b91338
> --- /dev/null
> +++ b/cmd/bcm/nitro_image_load.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2020 Broadcom
> + */
> +
> +#include <command.h>
> +#include <common.h>

switch order

> +
> +#define NITRO_FW_IMAGE_SIG 0xFF123456
> +#define NITRO_NS3_CFG_IMAGE_SIG 0xCF54321A

lower-case hex

> +
> +/*structure for Nitro bin file
> + *  signature: Nitro fw itb file
> + *  size: Nitro fw itb file
> + *  signature: Nitro NS3 config file
> + *  size: Nitro NS3 config file
> + *  Data: Nitro fw itb file
> + *  ............................
> + *  ............................
> + *  Data: Nitro NS3 config file
> + *  ............................
> + *  ............................
> + */
> +
> +static struct nitro_img_header {
> +       u32 nitro_fw_bin_sig;
> +       u32 nitro_fw_bin_size;
> +       u32 nitro_fw_cfg1_sig;
> +       u32 nitro_fw_cfg1_size;
> +       u32 nitro_fw_cfg2_sig;
> +       u32 nitro_fw_cfg2_size;
> +} *img_header;

So do you need the nitro_fw_ prefix on these?

> +
> +static int do_spi_nitro_images_addr(cmd_tbl_t *cmdtp, int flag, int argc,
> +                                   char *const argv[])
> +{
> +       uintptr_t images_load_addr;
> +       uintptr_t spi_load_addr;
> +       u32 len;
> +       u32 spi_data_offset = sizeof(struct nitro_img_header);
> +
> +       if (argc != 3)
> +               return CMD_RET_USAGE;
> +
> +       /* convert command parameter to fastboot address (base 16), i.e. hex */
> +       images_load_addr = (uintptr_t)simple_strtoul(argv[1], NULL, 16);
> +       if (!images_load_addr) {
> +               pr_err("Invalid load address\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       spi_load_addr = (uintptr_t)simple_strtoul(argv[2], NULL, 16);

Drop cast

> +       if (!spi_load_addr) {
> +               pr_err("Invalid spi load address\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       img_header = (struct nitro_img_header *)images_load_addr;
> +
> +       if (img_header->nitro_fw_bin_sig != NITRO_FW_IMAGE_SIG) {
> +               pr_err("Invalid Nitro bin file\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       env_set_hex("spi_nitro_fw_itb_start_addr", (ulong)0);

Please drop the casts here and everywhere else.

Note that any of these can fail. Do you want to check for errors?
Could perhaps just check the final one?

> +       env_set_hex("spi_nitro_fw_itb_len", (ulong)0);
> +       env_set_hex("spi_nitro_fw_ns3_cfg_start_addr", (ulong)0);
> +       env_set_hex("spi_nitro_fw_ns3_cfg_len", (ulong)0);
> +
> +       len = img_header->nitro_fw_bin_size;
> +
> +       env_set_hex("spi_nitro_fw_itb_start_addr", (ulong)
> +                  (spi_load_addr + spi_data_offset));
> +       env_set_hex("spi_nitro_fw_itb_len", (ulong)
> +                   img_header->nitro_fw_bin_size);
> +
> +       spi_data_offset += len;
> +
> +       if (img_header->nitro_fw_cfg1_sig == NITRO_NS3_CFG_IMAGE_SIG) {
> +               len = img_header->nitro_fw_cfg1_size;
> +
> +               env_set_hex("spi_nitro_fw_ns3_cfg_start_addr", (ulong)
> +                          (spi_load_addr + spi_data_offset));
> +               env_set_hex("spi_nitro_fw_ns3_cfg_len", (ulong)len);
> +
> +               spi_data_offset += len;
> +       }
> +
> +       /* disable nitro secure boot */
> +       env_set_hex("nitro_fastboot_secure", (ulong)0);
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +U_BOOT_CMD
> +       (spi_nitro_images_addr, 3, 1, do_spi_nitro_images_addr,
> +        "Load the nitro bin header and sets envs ",
> +        "spi_nitro_images_addr <load_addr> <spi_base_addr>\n"
> +);
> --
> 2.17.1
>

Regards,
Simon