[U-Boot] [PATCH v2 01/22] mkimage: Add OMAP boot image support

Wolfgang Denk
2011-05-15T19:06:14Z

Dear Aneesh V,

In message <1305472900-4004-2-git-send-email-aneesh@ti.com> you wrote:
> From: John Rigby <john.rigby@linaro.org>
>
> Signed-off-by: John Rigby <john.rigby@linaro.org>
> ---
> common/image.c | 1 +
> include/image.h | 1 +
> tools/Makefile | 2 +
> tools/mkimage.c | 2 +
> tools/mkimage.h | 1 +
> tools/omapimage.c | 229 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> tools/omapimage.h | 50 ++++++++++++
> 7 files changed, 286 insertions(+), 0 deletions(-)
> create mode 100644 tools/omapimage.c
> create mode 100644 tools/omapimage.h
>
> diff --git a/common/image.c b/common/image.c
> index e542a57..7f6fe1c 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -141,6 +141,7 @@ static const table_entry_t uimage_type[] = {
> { IH_TYPE_FLATDT, "flat_dt", "Flat Device Tree", },
> { IH_TYPE_KWBIMAGE, "kwbimage", "Kirkwood Boot Image",},
> { IH_TYPE_IMXIMAGE, "imximage", "Freescale i.MX Boot Image",},
> + { IH_TYPE_OMAPIMAGE, "omapimage", "TI OMAP CH/GP Boot Image",},
> { -1, "", "", },

Please keep list sorted / sort list.

> diff --git a/tools/Makefile b/tools/Makefile
> index 623f908..a1c4ed7 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -84,6 +84,7 @@ OBJ_FILES-$(CONFIG_CMD_LOADS) += img2srec.o
> OBJ_FILES-$(CONFIG_INCA_IP) += inca-swap-bytes.o
> NOPED_OBJ_FILES-y += kwbimage.o
> NOPED_OBJ_FILES-y += imximage.o
> +NOPED_OBJ_FILES-y += omapimage.o
> NOPED_OBJ_FILES-y += mkimage.o
> OBJ_FILES-$(CONFIG_NETCONSOLE) += ncb.o
> NOPED_OBJ_FILES-y += os_support.o
> @@ -180,6 +181,7 @@ $(obj)mkimage$(SFX): $(obj)crc32.o \
> $(obj)fit_image.o \
> $(obj)image.o \
> $(obj)imximage.o \
> + $(obj)omapimage.o \
> $(obj)kwbimage.o \
> $(obj)md5.o \
> $(obj)mkimage.o \

Please keep lists sorted.

> --- /dev/null
> +++ b/tools/omapimage.c
> @@ -0,0 +1,229 @@
> +/*
> + * (C) Copyright 2010
> + * Linaro LTD, www.linaro.org
> + * Author: John Rigby <john.rigby@linaro.org>
> + * Based on TI's signGP.c
> + *
> + * (C) Copyright 2009
> + * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
> + *
> + * (C) Copyright 2008
> + * Marvell Semiconductor <www.marvell.com>
> + * Written-by: Prafulla Wadaskar <prafulla@marvell.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * 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 (at your option) 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +/* Required to obtain the getline prototype from stdio.h */
> +#define _GNU_SOURCE
> +
> +#include "mkimage.h"
> +#include <image.h>
> +#include "omapimage.h"
> +
> +/* Header size is CH header rounded up to 512 bytes plus GP header */
> +#define OMAP_CH_HDR_SIZE 512
> +#define OMAP_GP_HDR_SIZE (sizeof(struct gp_header))
> +#define OMAP_FILE_HDR_SIZE (OMAP_CH_HDR_SIZE+OMAP_GP_HDR_SIZE)
> +
> +static uint8_t omapimage_header[OMAP_FILE_HDR_SIZE];
> +
> +static int omapimage_check_image_types(uint8_t type)
> +{
> + if (type == IH_TYPE_OMAPIMAGE)
> + return EXIT_SUCCESS;
> + else
> + return EXIT_FAILURE;

Maybe an error message would be helpful?

> +static void omapimage_print_section(struct ch_settings *chs)
> +{
> + switch (chs->section_key) {
> + case KEY_CHSETTINGS:
> + printf("CHSETTINGS (%x) "
> + "valid:%x "
> + "version:%x "
> + "reserved:%x "
> + "flags:%x\n",
> + chs->section_key,
> + chs->valid,
> + chs->version,
> + chs->reserved,
> + chs->flags);
> + break;
> + default:
> + printf("UNKNOWNKEY (%x) "
> + "valid:%x "
> + "version:%x "
> + "reserved:%x "
> + "flags:%x\n",
> + chs->section_key,
> + chs->valid,
> + chs->version,
> + chs->reserved,
> + chs->flags);
> + break;

How about unifying these cases, and passing "CHSETTINGS" resp.
"UNKNOWNKEY" as argument?

...
> +struct ch_toc {
> + uint32_t section_offset;
> + uint32_t section_size;
> + uint8_t unused[12];
> + uint8_t section_name[12];
> +} __attribute__ ((__packed__));
> +
> +struct ch_settings {
> + uint32_t section_key;
> + uint8_t valid;
> + uint8_t version;
> + uint16_t reserved;
> + uint32_t flags;
> +} __attribute__ ((__packed__));
> +
> +struct gp_header {
> + uint32_t size;
> + uint32_t load_addr;
> +} __attribute__ ((__packed__));

Is there any good reason to have these "__attribute__ ((__packed__))"
here? In general, these are only known to cause trouble and pain, and
I cannot see a need here.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"To IBM, 'open' means there is a modicum of interoperability among
some of their equipment." - Harv Masterson