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

[Wolfgang Denk]

Dear Aneesh V,

In message <1305472900-4004-2-git-send-email-aneesh at ti.com> you wrote:
> From: John Rigby <john.rigby at linaro.org>
> 
> Signed-off-by: John Rigby <john.rigby at 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 at 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 at 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