[U-Boot] [PATCH 2/6] arm64: mvebu: Add bubt command for flash image burn

Stefan Roese
2016-11-24T08:58:33Z

Hi Kosta,

a general comment: Please use scripts/checkpatch before sending
the patches to the list to make sure that all (mostly style
related) issues are resolved.

Please find some more mostly nitpicking comments below.

On 20.11.2016 16:38, kostap at marvell.com wrote:
> From: Konstantin Porotchkin <kostap@marvell.com>
>
> Add support for mvebu bubt command for flash image
> load, check and burn on boot device.

Could you please extent the patch (cmd) description here a bit?
Perhaps by adding an example on how this command can be used
to load and update image (loading via tftp, buring into SPI...).

> Change-Id: If2b971069ae44232761b601bbbcf0162567f5603
> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Nadav Haklai <nadavh@marvell.com>
> Cc: Neta Zur Hershkovits <neta@marvell.com>
> Cc: Omri Itach <omrii@marvell.com>
> Cc: Igal Liberman <igall@marvell.com>
> Cc: Haim Boot <hayim@marvell.com>
> Cc: Hanna Hawa <hannah@marvell.com>
> ---
> arch/arm/mach-mvebu/Kconfig | 30 ++
> cmd/Kconfig | 3 +
> cmd/Makefile | 2 +
> cmd/mvebu/Kconfig | 10 +
> cmd/mvebu/Makefile | 19 ++
> cmd/mvebu/bubt.c | 699 ++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 763 insertions(+)
> create mode 100644 cmd/mvebu/Kconfig
> create mode 100644 cmd/mvebu/Makefile
> create mode 100644 cmd/mvebu/bubt.c
>
> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> index 7248fd7..6de85d5 100644
> --- a/arch/arm/mach-mvebu/Kconfig
> +++ b/arch/arm/mach-mvebu/Kconfig
> @@ -99,6 +99,36 @@ config TARGET_THEADORABLE
>
> endchoice
>
> +choice
> + prompt "Flash for image"
> + default MVEBU_SPI_BOOT
> + depends on (TARGET_MVEBU_DB_88F3720 || TARGET_MVEBU_ARMADA_8K)
> +
> +config MVEBU_NAND_BOOT
> + bool "NAND flash boot"
> + depends on NAND_PXA3XX
> + help
> + Enable boot from NAND
> +
> +config MVEBU_SPI_BOOT
> + bool "SPI flash boot"
> + depends on SPI_FLASH
> +
> +config MVEBU_MMC_BOOT
> + bool "eMMC flash boot"
> + depends on MVEBU_MMC
> + help
> + Enable boot from eMMC boot partition
> +
> +endchoice
> +
> +config MVEBU_UBOOT_DFLT_NAME
> + string "Default image name for bubt command"
> + default "flash-image.bin"
> + help
> + This option should contain a default file name to be used with
> + MVEBU "bubt" command if the source file name is omitted
> +

I'm not sure if these Kconfig options really below in this file - please
see below...

> config SYS_BOARD
> default "clearfog" if TARGET_CLEARFOG
> default "mvebu_db-88f3720" if TARGET_MVEBU_DB_88F3720
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index e339d86..39dd0a0 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -610,6 +610,9 @@ config CMD_QFW
> This provides access to the QEMU firmware interface. The main
> feature is to allow easy loading of files passed to qemu-system
> via -kernel / -initrd
> +
> +source "cmd/mvebu/Kconfig"
> +
> endmenu
>
> config CMD_BOOTSTAGE
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 9c9a9d1..34bc544 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -163,3 +163,5 @@ obj-$(CONFIG_CMD_BLOB) += blob.o
>
> # core command
> obj-y += nvedit.o
> +
> +obj-$(CONFIG_ARCH_MVEBU) += mvebu/

Do you plan to add move mvebu specific commands?

> diff --git a/cmd/mvebu/Kconfig b/cmd/mvebu/Kconfig
> new file mode 100644
> index 0000000..e7fbd20
> --- /dev/null
> +++ b/cmd/mvebu/Kconfig
> @@ -0,0 +1,10 @@
> +menu "MVEBU commands"
> +depends on ARCH_MVEBU
> +
> +config CMD_MVEBU_BUBT
> + bool "bubt"
> + default n
> + help
> + bubt - Burn a u-boot image to flash
> +
> +endmenu

Here you introduce a new Kconfig file. Wouldn't it be better, to
move all Kconfig option into this file instead of having most
of them in the mach-mvebu file?

> diff --git a/cmd/mvebu/Makefile b/cmd/mvebu/Makefile
> new file mode 100644
> index 0000000..b0817e3
> --- /dev/null
> +++ b/cmd/mvebu/Makefile
> @@ -0,0 +1,19 @@
> +#
> +# ***************************************************************************
> +# Copyright (C) 2015 Marvell International Ltd.
> +# ***************************************************************************
> +# This program is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License as published by the Free
> +# Software Foundation, either version 2 of the License, or any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +# ***************************************************************************
> +#

As already mentioned by Simon, please move to using SPDX
license headers. You can and should of course keep your copyright
notice. Please fix this globally. For SPDX in U-Boot I suggest
to take a look at this page:

http://www.denx.de/wiki/U-Boot/Licensing

> +
> +obj-$(CONFIG_CMD_MVEBU_BUBT) += bubt.o
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> new file mode 100644
> index 0000000..6539063
> --- /dev/null
> +++ b/cmd/mvebu/bubt.c
> @@ -0,0 +1,699 @@
> +/*
> + * ***************************************************************************
> + * Copyright (C) 2015 Marvell International Ltd.
> + * ***************************************************************************
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation, either version 2 of the License, or any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + * ***************************************************************************
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <command.h>
> +#include <vsprintf.h>
> +#include <errno.h>
> +#include <dm.h>
> +
> +#include <spi_flash.h>
> +#include <spi.h>
> +#include <nand.h>
> +#include <usb.h>
> +#include <fs.h>
> +#include <mmc.h>
> +#include <u-boot/sha1.h>
> +#include <u-boot/sha256.h>
> +
> +#if defined(CONFIG_TARGET_MVEBU_ARMADA_8K)
> +#define MAIN_HDR_MAGIC 0xB105B002

As already mentioned by Simon, please move to using lower
case characters for hex numbers.

> +struct mvebu_image_header {
> + uint32_t magic; /* 0-3 */
> + uint32_t prolog_size; /* 4-7 */
> + uint32_t prolog_checksum; /* 8-11 */
> + uint32_t boot_image_size; /* 12-15 */
> + uint32_t boot_image_checksum; /* 16-19 */
> + uint32_t rsrvd0; /* 20-23 */
> + uint32_t load_addr; /* 24-27 */
> + uint32_t exec_addr; /* 28-31 */
> + uint8_t uart_cfg; /* 32 */
> + uint8_t baudrate; /* 33 */
> + uint8_t ext_count; /* 34 */
> + uint8_t aux_flags; /* 35 */
> + uint32_t io_arg_0; /* 36-39 */
> + uint32_t io_arg_1; /* 40-43 */
> + uint32_t io_arg_2; /* 43-47 */
> + uint32_t io_arg_3; /* 48-51 */
> + uint32_t rsrvd1; /* 52-55 */
> + uint32_t rsrvd2; /* 56-59 */
> + uint32_t rsrvd3; /* 60-63 */
> +};
> +#elif defined(CONFIG_TARGET_MVEBU_DB_88F3720) /* A3700 */
> +#define HASH_SUM_LEN 16
> +#define IMAGE_VERSION_3_6_0 0x030600
> +#define IMAGE_VERSION_3_5_0 0x030500
> +
> +struct common_tim_data {
> + u32 version;
> + u32 identifier;
> + u32 trusted;
> + u32 issue_date;
> + u32 oem_unique_id;
> + u32 reserved[5]; /* Reserve 20 bytes */
> + u32 boot_flash_sign;
> + u32 num_images;
> + u32 num_keys;
> + u32 size_of_reserved;
> +};
> +
> +struct mvebu_image_info {
> + u32 image_id;
> + u32 next_image_id;
> + u32 flash_entry_addr;
> + u32 load_addr;
> + u32 image_size;
> + u32 image_size_to_hash;
> + u32 hash_algorithm_id;
> + u32 hash[HASH_SUM_LEN]; /* Reserve 512 bits for the hash */
> + u32 partition_number;
> + u32 enc_algorithm_id;
> + u32 encrypt_start_offset;
> + u32 encrypt_size;
> +};
> +#endif
> +
> +struct bubt_dev {
> + char name[8];
> + size_t (*read)(const char *file_name);
> + int (*write)(size_t image_size);
> + int (*active)(void);
> +};
> +
> +static ulong get_load_addr(void)
> +{
> + const char *addr_str;
> + unsigned long addr;
> +
> + addr_str = getenv("loadaddr");
> + if (addr_str != NULL)
> + addr = simple_strtoul(addr_str, NULL, 16);
> + else
> + addr = CONFIG_SYS_LOAD_ADDR;
> +
> + return addr;
> +}
> +
> +/********************************************************************
> + * eMMC services
> + ********************************************************************/
> +#ifdef CONFIG_DM_MMC
> +static int mmc_burn_image(size_t image_size)
> +{
> + struct mmc *mmc;
> + lbaint_t start_lba;
> + lbaint_t blk_count;
> + ulong blk_written;
> +#ifdef CONFIG_SYS_MMC_ENV_DEV
> + const u8 mmc_dev_num = CONFIG_SYS_MMC_ENV_DEV;
> +#else
> + const u8 mmc_dev_num = 0;
> +#endif

I suggest to move this one up in this file to make the code look a bit
"cleaner", like this:

#ifndef CONFIG_SYS_MMC_ENV_DEV
#define CONFIG_SYS_MMC_ENV_DEV 0
#endif

Then you can remove the #ifdef from the code here.

> +
> + mmc = find_mmc_device(mmc_dev_num);
> + if (!mmc) {
> + printf("No SD/MMC/eMMC card found\n");
> + return 1;
> + }

Does this bubt command only support eMMC or SD/MMC as well? If so,
please mention this in the Kconfig options too, as they only list
eMMC (IIRC).

> +
> + if (mmc_init(mmc)) {
> + printf("%s(%d) init failed\n", IS_SD(mmc) ? "SD" : "MMC", mmc_dev_num);
> + return 1;
> + }
> +
> +#ifdef CONFIG_SYS_MMC_ENV_PART
> + if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {
> + if (mmc_switch_part(mmc_dev_num, CONFIG_SYS_MMC_ENV_PART)) {
> + printf("MMC partition switch failed\n");
> + return 1;
> + }
> + }
> +#endif
> +
> + /* SD reserves LBA-0 for MBR and boots from LBA-1, MMC/eMMC boots from LBA-0 */
> + start_lba = IS_SD(mmc) ? 1 : 0;
> + blk_count = image_size / mmc->block_dev.blksz;
> + if (image_size % mmc->block_dev.blksz)
> + blk_count += 1;
> +
> + blk_written = mmc->block_dev.block_write(mmc_dev_num,
> + start_lba, blk_count, (void *)get_load_addr());
> + if (blk_written != blk_count) {
> + printf("Error - written %#lx blocks\n", blk_written);
> + return 1;
> + } else {
> + printf("Done!\n");
> + }
> +
> +#ifdef CONFIG_SYS_MMC_ENV_PART
> + if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)
> + mmc_switch_part(mmc_dev_num, mmc->part_num);
> +#endif
> +
> + return 0;
> +}
> +
> +static size_t mmc_read_file(const char *file_name)
> +{
> + loff_t act_read = 0;
> + int rc;
> + struct mmc *mmc;
> +#ifdef CONFIG_SYS_MMC_ENV_DEV
> + const u8 mmc_dev_num = CONFIG_SYS_MMC_ENV_DEV;
> +#else
> + const u8 mmc_dev_num = 0;
> +#endif

Again, please use the construct suggested above.

> + mmc = find_mmc_device(mmc_dev_num);
> + if (!mmc) {
> + printf("No SD/MMC/eMMC card found\n");
> + return 0;
> + }
> +
> + if (mmc_init(mmc)) {
> + printf("%s(%d) init failed\n", IS_SD(mmc) ? "SD" : "MMC", mmc_dev_num);
> + return 0;
> + }
> +
> + /* Load from data partition (0) */
> + if (fs_set_blk_dev("mmc", "0", FS_TYPE_ANY)) {
> + printf("Error: MMC 0 not found\n");
> + return 0;
> + }
> +
> + /* Perfrom file read */
> + rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
> + if (rc)
> + return 0;
> +
> + return act_read;
> +}
> +
> +int is_mmc_active(void)
> +{
> + return 1;
> +}
> +#else /* CONFIG_DM_MMC */
> +#define mmc_burn_image 0
> +#define mmc_read_file 0
> +#define is_mmc_active 0

Please use empty functions instead.

> +#endif /* CONFIG_DM_MMC */
> +
> +
> +/********************************************************************
> + * SPI services
> + ********************************************************************/
> +#ifdef CONFIG_SPI_FLASH
> +static int spi_burn_image(size_t image_size)
> +{
> + int ret;
> + struct spi_flash *flash;
> + uint32_t erase_bytes;
> +
> + /* Probe the SPI bus to get the flash device */
> + flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> + CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE);
> + if (!flash) {
> + printf("Failed to probe SPI Flash\n");
> + return 1;
> + }
> +
> +#ifdef CONFIG_SPI_FLASH_PROTECTION
> + spi_flash_protect(flash, 0);
> +#endif
> + erase_bytes = image_size + (flash->erase_size - image_size % flash->erase_size);
> + printf("Erasing %d bytes (%d blocks) at offset 0 ...", erase_bytes, erase_bytes/flash->erase_size);
> + ret = spi_flash_erase(flash, 0, erase_bytes);
> + if (ret)
> + printf("Error!\n");
> + else
> + printf("Done!\n");
> +
> + printf("Writing %d bytes from 0x%lx to offset 0 ...", (int)image_size, get_load_addr());
> + ret = spi_flash_write(flash, 0, image_size, (void *)get_load_addr());
> + if (ret)
> + printf("Error!\n");
> + else
> + printf("Done!\n");
> +
> +#ifdef CONFIG_SPI_FLASH_PROTECTION
> + spi_flash_protect(flash, 1);
> +#endif
> +
> + return ret;
> +}
> +
> +int is_spi_active(void)
> +{
> + return 1;
> +}
> +#else /* CONFIG_SPI_FLASH */
> +#define spi_burn_image 0
> +#define is_spi_active 0

Empty functions please.

> +#endif /* CONFIG_SPI_FLASH */
> +
> +/********************************************************************
> + * NAND services
> + ********************************************************************/
> +#ifdef CONFIG_CMD_NAND
> +static int nand_burn_image(size_t image_size)
> +{
> + int ret, block_size;
> + nand_info_t *nand;
> + int dev = nand_curr_device;
> +
> + if ((dev < 0) || (dev >= CONFIG_SYS_MAX_NAND_DEVICE) ||
> + (!nand_info[dev].name)) {
> + puts("\nno devices available\n");
> + return 1;
> + }
> + nand = &nand_info[dev];
> + block_size = nand->erasesize;
> +
> + /* Align U-Boot size to currently used blocksize */
> + image_size = ((image_size + (block_size - 1)) & (~(block_size-1)));

checkpatch will most likely complain about the missing space
before and after the "-" above.

> +
> + /* Erase the U-BOOT image space */
> + printf("Erasing 0x%x - 0x%x:...", 0, (int)image_size);
> + ret = nand_erase(nand, 0, image_size);
> + if (ret) {
> + printf("Error!\n");
> + goto error;
> + }
> + printf("Done!\n");
> +
> + /* Write the image to flash */
> + printf("Writing image:...");
> + printf("&image_size = 0x%p\n", (void*)&image_size);
> + ret = nand_write(nand, 0, &image_size, (void *)get_load_addr());
> + if (ret)
> + printf("Error!\n");
> + else
> + printf("Done!\n");
> +
> +error:
> + return ret;
> +}
> +
> +int is_nand_active(void)
> +{
> + return 1;
> +}
> +#else /* CONFIG_CMD_NAND */
> +#define nand_burn_image 0
> +#define is_nand_active 0

Empty functions please.

> +#endif /* CONFIG_CMD_NAND */
> +
> +/********************************************************************
> + * USB services
> + ********************************************************************/
> +#if defined(CONFIG_USB_STORAGE) && defined (CONFIG_BLK)
> +static size_t usb_read_file(const char *file_name)
> +{
> + loff_t act_read = 0;
> + struct udevice *dev;
> + int rc;
> +
> + usb_stop();
> +
> + if (usb_init() < 0) {
> + printf("Error: usb_init failed\n");
> + return 0;
> + }
> +
> + /* Try to recognize storage devices immediately */
> + blk_first_device(IF_TYPE_USB, &dev);
> + if (dev == NULL) {
> + printf("Error: USB storage device not found\n");
> + return 0;
> + }
> +
> + /* Always load from usb 0 */
> + if (fs_set_blk_dev("usb", "0", FS_TYPE_ANY)) {
> + printf("Error: USB 0 not found\n");
> + return 0;
> + }
> +
> + /* Perfrom file read */
> + rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
> + if (rc)
> + return 0;
> +
> + return act_read;
> +}
> +
> +int is_usb_active(void)
> +{
> + return 1;
> +}
> +#else /* defined(CONFIG_USB_STORAGE) && defined (CONFIG_BLK) */
> +#define usb_read_file 0
> +#define is_usb_active 0
> +#endif /* defined(CONFIG_USB_STORAGE) && defined (CONFIG_BLK) */
> +
> +/********************************************************************
> + * Network services
> + ********************************************************************/
> +#ifdef CONFIG_CMD_NET
> +static size_t tftp_read_file(const char *file_name)
> +{
> + /* update global variable load_addr before tftp file from network */
> + load_addr = get_load_addr();
> + return net_loop(TFTPGET);
> +}
> +
> +int is_tftp_active(void)
> +{
> + return 1;
> +}
> +#else
> +#define tftp_read_file 0
> +#define is_tftp_active 0
> +#endif /* CONFIG_CMD_NET */
> +
> +
> +enum bubt_devices {
> + BUBT_DEV_NET = 0,
> + BUBT_DEV_USB,
> + BUBT_DEV_MMC,
> + BUBT_DEV_SPI,
> + BUBT_DEV_NAND,
> +
> + BUBT_MAX_DEV
> +};
> +
> +struct bubt_dev bubt_devs[BUBT_MAX_DEV] = {
> + {"tftp", tftp_read_file, NULL, is_tftp_active},
> + {"usb", usb_read_file, NULL, is_usb_active},
> + {"mmc", mmc_read_file, mmc_burn_image, is_mmc_active},
> + {"spi", NULL, spi_burn_image, is_spi_active},
> + {"nand", NULL, nand_burn_image, is_nand_active},
> +};
> +
> +static int bubt_write_file(struct bubt_dev *dst, size_t image_size)
> +{
> + if (!dst->write) {
> + printf("Error: Write not supported on device %s\n", dst->name);
> + return 1;
> + }
> +
> + return dst->write(image_size);
> +}
> +
> +#if defined(CONFIG_TARGET_MVEBU_ARMADA_8K)
> +uint32_t do_checksum32(uint32_t *start, int32_t len)
> +{
> + uint32_t sum = 0;
> + uint32_t *startp = start;
> +
> + do {
> + sum += *startp;
> + startp++;
> + len -= 4;
> + } while (len > 0);
> +
> + return sum;
> +}
> +
> +static int check_image_header(void)
> +{
> + struct mvebu_image_header *hdr = (struct mvebu_image_header *)get_load_addr();
> + uint32_t header_len = hdr->prolog_size;
> + uint32_t checksum;
> + uint32_t checksum_ref = hdr->prolog_checksum;
> +
> + /*
> + * For now compare checksum, and magic. Later we can
> + * verify more stuff on the header like interface type, etc
> + */
> + if (hdr->magic != MAIN_HDR_MAGIC) {
> + printf("ERROR: Bad MAGIC 0x%08x != 0x%08x\n", hdr->magic, MAIN_HDR_MAGIC);
> + return -ENOEXEC;
> + }
> +
> + /* The checksum value is discarded from checksum calculation */
> + hdr->prolog_checksum = 0;
> +
> + checksum = do_checksum32((uint32_t *)hdr, header_len);
> + if (checksum != checksum_ref) {
> + printf("Error: Bad Image checksum. 0x%x != 0x%x\n", checksum, checksum_ref);
> + return -ENOEXEC;
> + }
> +
> + /* Restore the checksum before writing */
> + hdr->prolog_checksum = checksum_ref;
> + printf("Image checksum...OK!\n");
> +
> + return 0;
> +}
> +#elif defined(CONFIG_TARGET_MVEBU_DB_88F3720) /* Armada 3700 */
> +static int check_image_header(void)
> +{
> + struct common_tim_data *hdr = (struct common_tim_data *)get_load_addr();
> + int image_num;
> + u8 hash_160_output[SHA1_SUM_LEN];
> + u8 hash_256_output[SHA256_SUM_LEN];
> + sha1_context hash1_text;
> + sha256_context hash256_text;
> + u8 *hash_output;
> + u32 hash_algorithm_id;
> + u32 image_size_to_hash;
> + u32 flash_entry_addr;
> + u32 *hash_value;
> + u32 internal_hash[HASH_SUM_LEN];
> + const uint8_t *buff;
> + u32 num_of_image = hdr->num_images;
> + u32 version = hdr->version;
> + u32 trusted = hdr->trusted;
> +
> + /* bubt checksum validation only supports nontrusted images */
> + if (trusted == 1) {
> + printf("bypass image validation, only untrusted image is supported now\n");
> + return 0;
> + }
> + /* only supports image version 3.5 and 3.6 */
> + if (version != IMAGE_VERSION_3_5_0 && version != IMAGE_VERSION_3_6_0) {
> + printf("Error: Unsupported Image version = 0x%08x\n", version);
> + return -ENOEXEC;
> + }
> + /* validate images hash value */
> + for (image_num = 0; image_num < num_of_image; image_num++) {
> + struct mvebu_image_info *info = (struct mvebu_image_info *)(get_load_addr()
> + + sizeof(struct common_tim_data) + image_num * sizeof(struct mvebu_image_info));
> + hash_algorithm_id = info->hash_algorithm_id;
> + image_size_to_hash = info->image_size_to_hash;
> + flash_entry_addr = info->flash_entry_addr;
> + hash_value = info->hash;
> + buff = (const uint8_t *)(get_load_addr() + flash_entry_addr);
> +
> + if (image_num == 0) {
> + /* The first image includes hash values in itself content. For hash calculation, we need
> + * to save original hash values to local variable that will be copied back for comparsion
> + * and set all zeros to replace orignal hash values to calculate its new hash value.
> + * First image original format : x...x (datum1) x...x(original hash values) x...x(datum2)
> + * Replaced first image format : x...x (datum1) 0...0(hash values) x...x(datum2)
> + */

Multicomment style is:

/*
*
*/

And please note the 80 colums limit.

> + memcpy(internal_hash, hash_value, sizeof(internal_hash));
> + memset(hash_value, 0, sizeof(internal_hash));
> + }
> + if (image_size_to_hash == 0) {
> + printf("Warning: Image_%d hash checksum is disable, skip the image validation.\n", image_num);
> + continue;
> + }
> + switch (hash_algorithm_id) {
> + case SHA1_SUM_LEN:
> + sha1_starts(&hash1_text);
> + sha1_update(&hash1_text, buff, image_size_to_hash);
> + sha1_finish(&hash1_text, hash_160_output);
> + hash_output = hash_160_output;
> + break;
> + case SHA256_SUM_LEN:
> + sha256_starts(&hash256_text);
> + sha256_update(&hash256_text, buff, image_size_to_hash);
> + sha256_finish(&hash256_text, hash_256_output);
> + hash_output = hash_256_output;
> + break;
> + default:
> + printf("Error: Unsupported hash_algorithm_id = %d\n", hash_algorithm_id);
> + return -ENOEXEC;
> + }
> + if (image_num == 0)
> + memcpy(hash_value, internal_hash, sizeof(internal_hash));
> + if (memcmp(hash_value, hash_output, hash_algorithm_id) != 0) {
> + printf("Error: Image_%d checksum is not correct\n", image_num);
> + return -ENOEXEC;
> + }
> + }
> + printf("Image checksum...OK!\n");
> + return 0;
> +}
> +#else
> +static int check_image_header(void)
> +{
> + printf("bubt cmd does not support this SoC device or family!\n");
> + return -ENOEXEC;
> +}
> +#endif
> +
> +static int bubt_verify(size_t image_size)
> +{
> +
> + /* Check a correct image header exists */
> + if (check_image_header()) {
> + printf("Error: Image header verification failed\n");
> + return 1;
> + }
> + return 0;
> +}
> +
> +
> +static int bubt_read_file(struct bubt_dev *src)
> +{
> + size_t image_size;
> +
> + if (!src->read) {
> + printf("Error: Read not supported on device \"%s\"\n", src->name);
> + return 0;
> + }
> +
> + image_size = src->read(net_boot_file_name);
> + if (image_size <= 0) {
> + printf("Error: Failed to read file %s from %s\n", net_boot_file_name, src->name);
> + return 0;
> + }
> +
> + return image_size;
> +}
> +
> +static int bubt_is_dev_active(struct bubt_dev *dev)
> +{
> + if (!dev->active) {
> + printf("Device \"%s\" not supported by U-BOOT image\n", dev->name);
> + return 0;
> + }
> +
> + if (!dev->active()) {
> + printf("Device \"%s\" is inactive\n", dev->name);
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +struct bubt_dev *find_bubt_dev(char *dev_name)
> +{
> + int dev;
> +
> + for (dev = 0; dev < BUBT_MAX_DEV; dev++) {
> + if (strcmp(bubt_devs[dev].name, dev_name) == 0)
> + return &bubt_devs[dev];
> + }
> +
> + return 0;
> +}
> +
> +#define DEFAULT_BUBT_SRC "tftp"
> +
> +#ifndef DEFAULT_BUBT_DST
> +#ifdef CONFIG_MVEBU_SPI_BOOT
> +#define DEFAULT_BUBT_DST "spi"
> +#elif defined(CONFIG_MVEBU_NAND_BOOT)
> +#define DEFAULT_BUBT_DST "nand"
> +#elif defined(CONFIG_MVEBU_MMC_BOOT)
> +#define DEFAULT_BUBT_DST "mmc"
> +else
> +#define DEFAULT_BUBT_DST "error"
> +#endif
> +#endif /* DEFAULT_BUBT_DST */
> +
> +int do_bubt_cmd(cmd_tbl_t *cmdtp, int flag, int argc,
> + char * const argv[])
> +{
> + struct bubt_dev *src, *dst;
> + size_t image_size;
> + char src_dev_name[8];
> + char dst_dev_name[8];
> + char *name;
> +
> + if (argc < 2)
> + copy_filename(net_boot_file_name, CONFIG_MVEBU_UBOOT_DFLT_NAME, sizeof(net_boot_file_name));
> + else
> + copy_filename(net_boot_file_name, argv[1], sizeof(net_boot_file_name));
> +
> + if (argc >= 3) {
> + strncpy(dst_dev_name, argv[2], 8);
> + } else {
> + name = DEFAULT_BUBT_DST;
> + strncpy(dst_dev_name, name, 8);
> + }
> +
> + if (argc >= 4)
> + strncpy(src_dev_name, argv[3], 8);
> + else
> + strncpy(src_dev_name, DEFAULT_BUBT_SRC, 8);
> +
> + /* Figure out the destination device */
> + dst = find_bubt_dev(dst_dev_name);
> + if (!dst) {
> + printf("Error: Unknown destination \"%s\"\n", dst_dev_name);
> + return 1;
> + }
> +
> + if (!bubt_is_dev_active(dst))
> + return 1;
> +
> + /* Figure out the source device */
> + src = find_bubt_dev(src_dev_name);
> + if (!src) {
> + printf("Error: Unknown source \"%s\"\n", src_dev_name);
> + return 1;
> + }
> +
> + if (!bubt_is_dev_active(src))
> + return 1;
> +
> + printf("Burning U-BOOT image \"%s\" from \"%s\" to \"%s\"\n", net_boot_file_name, src->name, dst->name);
> +
> + image_size = bubt_read_file(src);
> + if (!image_size)
> + return 1;
> +
> + if (bubt_verify(image_size))
> + return 1;
> +
> + if (bubt_write_file(dst, image_size))
> + return 1;
> +
> + return 0;
> +}
> +
> +U_BOOT_CMD(
> + bubt, 4, 0, do_bubt_cmd,
> + "Burn a u-boot image to flash",
> + "[file-name] [destination [source]]\n"
> + "\t-file-name The image file name to burn. Default = u-boot.bin\n"
> + "\t-destination Flash to burn to [spi, nand, mmc]. Default = active boot device\n"
> + "\t-source The source to load image from [tftp, usb, mmc]. Default = tftp\n"
> + "Examples:\n"
> + "\tbubt - Burn flash-image.bin from tftp to active boot device\n"
> + "\tbubt flash-image-new.bin nand - Burn flash-image-new.bin from tftp to NAND flash\n"
> + "\tbubt backup-flash-image.bin mmc usb - Burn backup-flash-image.bin from usb to MMC\n"

Ah, here you have some nice examples. Please add at least one best
with a log from running on a board to the commit text as
mentioned above.

Thanks,
Stefan