This is second step towards cleaning mkimage code for kwbimage support in clean way. In this patch- 1. The image_get_header_size function call is replaced by sizeof(image_header_t) in default_image.c 2. image header code moved form mkimage.c to default_image .c

Signed-off-by: Prafulla Wadaskar prafulla@marvell.com --- tools/default_image.c | 22 ++++++++++++++++++---- tools/default_image.h | 1 + tools/mkimage.c | 24 ++++++++++++------------ tools/mkimage.h | 8 ++++++++ 4 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/tools/default_image.c b/tools/default_image.c index 45afde1..e1a9bc7 100644 --- a/tools/default_image.c +++ b/tools/default_image.c @@ -29,6 +29,20 @@ #include <image.h> #include "default_image.h"

+image_header_t header; + +/* + * Default image parameters + */ +struct image_type_params defimage_params = { + .header_size = sizeof(image_header_t), + .hdr = (void*)&header, +}; + +void *image_get_tparams (void){ + return (void *)&defimage_params; +} + int image_check_params (struct mkimage_params *params) { return ((params->dflag && (params->fflag || params->lflag)) || @@ -100,13 +114,13 @@ void image_set_header (char *ptr, struct stat *sbuf, image_header_t * hdr = (image_header_t *)ptr;

checksum = crc32 (0, - (const char *)(ptr + image_get_header_size ()), - sbuf->st_size - image_get_header_size ()); + (const char *)(ptr + sizeof(image_header_t)), + sbuf->st_size - sizeof(image_header_t));

/* Build new header */ image_set_magic (hdr, IH_MAGIC); image_set_time (hdr, sbuf->st_mtime); - image_set_size (hdr, sbuf->st_size - image_get_header_size ()); + image_set_size (hdr, sbuf->st_size - sizeof(image_header_t)); image_set_load (hdr, params->addr); image_set_ep (hdr, params->ep); image_set_dcrc (hdr, checksum); @@ -117,7 +131,7 @@ void image_set_header (char *ptr, struct stat *sbuf,

image_set_name (hdr, params->imagename);

- checksum = crc32 (0, (const char *)hdr, image_get_header_size ()); + checksum = crc32 (0, (const char *)hdr, sizeof(image_header_t));

image_set_hcrc (hdr, checksum); } diff --git a/tools/default_image.h b/tools/default_image.h index 6ebc689..2311ede 100644 --- a/tools/default_image.h +++ b/tools/default_image.h @@ -25,6 +25,7 @@ #ifndef _DEFAULT_IMAGE_H_ #define _DEFAULT_IMAGE_H_

+void *image_get_tparams (void); int image_check_params (struct mkimage_params *params); int image_verify_header (char *ptr, int image_size, struct mkimage_params *params); diff --git a/tools/mkimage.c b/tools/mkimage.c index 66d6c8e..d1636d8 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -49,9 +49,6 @@ struct mkimage_params params = { .cmdname = "", };

-image_header_t header; -image_header_t *hdr = &header; - int main (int argc, char **argv) { @@ -59,6 +56,7 @@ main (int argc, char **argv) struct stat sbuf; unsigned char *ptr; int retval; + struct image_type_params *tparams = image_get_tparams ();

params.cmdname = *argv;

@@ -163,7 +161,7 @@ NXTARG: ; params.ep = params.addr; /* If XIP, entry point must be after the U-Boot header */ if (params.xflag) - params.ep += image_get_header_size (); + params.ep += tparams->header_size; }

/* @@ -171,11 +169,11 @@ NXTARG: ; * the size of the U-Boot header. */ if (params.xflag) { - if (params.ep != params.addr + image_get_header_size ()) { + if (params.ep != params.addr + tparams->header_size) { fprintf (stderr, "%s: For XIP, the entry point must be the load addr + %lu\n", params.cmdname, - (unsigned long)image_get_header_size ()); + (unsigned long)tparams->header_size); exit (EXIT_FAILURE); } } @@ -209,7 +207,7 @@ NXTARG: ; exit (EXIT_FAILURE); }

- if ((unsigned)sbuf.st_size < image_get_header_size ()) { + if ((unsigned)sbuf.st_size < tparams->header_size) { fprintf (stderr, "%s: Bad size: "%s" is no valid image\n", params.cmdname, params.imagefile); @@ -247,9 +245,10 @@ NXTARG: ; * * write dummy header, to be fixed later */ - memset (hdr, 0, image_get_header_size ()); + memset (tparams->hdr, 0, tparams->header_size);

- if (write(ifd, hdr, image_get_header_size ()) != image_get_header_size ()) { + if (write(ifd, tparams->hdr, tparams->header_size) + != tparams->header_size) { fprintf (stderr, "%s: Write error on %s: %s\n", params.cmdname, params.imagefile, strerror(errno)); exit (EXIT_FAILURE); @@ -366,6 +365,7 @@ copy_file (int ifd, const char *datafile, int pad) int zero = 0; int offset = 0; int size; + struct image_type_params *tparams = image_get_tparams ();

if (params.vflag) { fprintf (stderr, "Adding Image %s\n", datafile); @@ -398,14 +398,14 @@ copy_file (int ifd, const char *datafile, int pad) * reserved for it. */

- if ((unsigned)sbuf.st_size < image_get_header_size ()) { + if ((unsigned)sbuf.st_size < tparams->header_size) { fprintf (stderr, "%s: Bad size: "%s" is too small for XIP\n", params.cmdname, datafile); exit (EXIT_FAILURE); }

- for (p = ptr; p < ptr + image_get_header_size (); p++) { + for (p = ptr; p < ptr + tparams->header_size; p++) { if ( *p != 0xff ) { fprintf (stderr, "%s: Bad file: "%s" has invalid buffer for XIP\n", @@ -414,7 +414,7 @@ copy_file (int ifd, const char *datafile, int pad) } }

- offset = image_get_header_size (); + offset = tparams->header_size; }

size = sbuf.st_size - offset; diff --git a/tools/mkimage.h b/tools/mkimage.h index 0418644..3e94911 100644 --- a/tools/mkimage.h +++ b/tools/mkimage.h @@ -69,4 +69,12 @@ struct mkimage_params { char *cmdname; };

+/* + * These are image type specific variables + */ +struct image_type_params { + uint32_t header_size; + void *hdr; +}; + #endif /* _MKIIMAGE_H_ */

Currently it is used by image.c only This API can be used by additional mkimage types supports for ex. kwbimage, to use it the API is made global

Signed-off-by: Prafulla Wadaskar prafulla@marvell.com --- common/image.c | 3 +-- include/image.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/image.c b/common/image.c index e22c974..232348c 100644 --- a/common/image.c +++ b/common/image.c @@ -158,7 +158,6 @@ static table_entry_t uimage_comp[] = {

uint32_t crc32 (uint32_t, const unsigned char *, uint); uint32_t crc32_wd (uint32_t, const unsigned char *, uint, uint); -static void genimg_print_size (uint32_t size); #if defined(CONFIG_TIMESTAMP) || defined(CONFIG_CMD_DATE) || defined(USE_HOSTCC) static void genimg_print_time (time_t timestamp); #endif @@ -472,7 +471,7 @@ void memmove_wd (void *to, void *from, size_t len, ulong chunksz) } #endif /* !USE_HOSTCC */

-static void genimg_print_size (uint32_t size) +void genimg_print_size (uint32_t size) { #ifndef USE_HOSTCC printf ("%d Bytes = ", size); diff --git a/include/image.h b/include/image.h index b519e5e..88a13ab 100644 --- a/include/image.h +++ b/include/image.h @@ -300,6 +300,7 @@ int genimg_get_comp_id (const char *name); #define IMAGE_FORMAT_LEGACY 0x01 /* legacy image_header based format */ #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */

+void genimg_print_size (uint32_t size); int genimg_get_format (void *img_addr); int genimg_has_config (bootm_headers_t *images); ulong genimg_get_image (ulong img_addr);

This is Third step towards cleaning mkimage code for kwbimage support in clean way.

This patch adds type kwbimabe support for mkimage For more details refer docs/README.kwbimage

This patch is tested with Wheevaplub board

Signed-off-by: Prafulla Wadaskar prafulla@marvell.com --- Makefile | 5 + common/image.c | 1 + doc/README.kwbimage | 93 +++++++++++++ include/image.h | 3 +- tools/Makefile | 4 + tools/kwbimage.c | 364 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/kwbimage.h | 118 +++++++++++++++++ tools/mkimage.c | 4 +- 8 files changed, 590 insertions(+), 2 deletions(-) create mode 100644 doc/README.kwbimage create mode 100644 tools/kwbimage.c create mode 100644 tools/kwbimage.h

diff --git a/Makefile b/Makefile index ef535ed..336dcd4 100644 --- a/Makefile +++ b/Makefile @@ -313,6 +313,10 @@ $(obj)u-boot.img: $(obj)u-boot.bin sed -e 's/"[ ]*$$/ for $(BOARD) board"/') \ -d $< $@

+$(obj)u-boot.kwb: $(obj)u-boot.bin + $(obj)tools/mkimage -n $(KWD_CONFIG) -T kwbimage \ + -a $(TEXT_BASE) -e $(TEXT_BASE) -d $< $@ + $(obj)u-boot.sha1: $(obj)u-boot.bin $(obj)tools/ubsha1 $(obj)u-boot.bin

@@ -3660,6 +3664,7 @@ clobber: clean @rm -f $(OBJS) $(obj)*.bak $(obj)ctags $(obj)etags $(obj)TAGS \ $(obj)cscope.* $(obj)*.*~ @rm -f $(obj)u-boot $(obj)u-boot.map $(obj)u-boot.hex $(ALL) + @rm -f $(obj)u-boot.kwb @rm -f $(obj)tools/{env/crc32.c,inca-swap-bytes} @rm -f $(obj)cpu/mpc824x/bedbug_603e.c @rm -f $(obj)include/asm/proc $(obj)include/asm/arch $(obj)include/asm diff --git a/common/image.c b/common/image.c index 8cf675d..10b5deb 100644 --- a/common/image.c +++ b/common/image.c @@ -139,6 +139,7 @@ static table_entry_t uimage_type[] = { { IH_TYPE_SCRIPT, "script", "Script", }, { IH_TYPE_STANDALONE, "standalone", "Standalone Program", }, { IH_TYPE_FLATDT, "flat_dt", "Flat Device Tree", }, + { IH_TYPE_KWBIMAGE, "kwbimage", "Kirkwood Boot Image",}, { -1, "", "", }, };

diff --git a/doc/README.kwbimage b/doc/README.kwbimage new file mode 100644 index 0000000..ca5b228 --- /dev/null +++ b/doc/README.kwbimage @@ -0,0 +1,93 @@ +--------------------------------------------- +Kirkwood Boot Image generation using mkimage +--------------------------------------------- + +This document describes the U-Boot feature as it +is implemented for the Kirkwood family of SoCs. + +The Kirkwood SoC's can boot directly from NAND FLASH, +SPI FLASH, SATA etc. using its internal bootRom support. + +for more details refer section 24.2 of Kirkwood functional specifications. +ref: www.marvell.com/products/embedded.../kirkwood/index.jsp + +Commad syntax: +-------------- +./tools/mkimage -l <kwboot_file> + to list the kwb image file details + +./tools/mkimage -n <board specific configuration file> \ + -T kwbimage -a <start address> -e <execution address> \ + -d <input_raw_binary> <output_kwboot_file> + +for ex. +./tools/mkimage -n ./board/Marvell/openrd_base/kwbimage.cfg \ + -T kwbimage -a 0x00600000 -e 0x00600000 \ + -d u-boot.bin u-boot.kwb + +kwimage support available with mkimage utility will generate kirkwood boot +image that can be flashed on the board NAND/SPI flash + +Board specific configuration file specifications: +------------------------------------------------ +1. This file must present in the $(BOARDDIR) and the name should be + kwbimage.cfg (since this is used in Makefile) +2. This file can have empty lines and lines starting with "#" as first + character to put comments +3. This file can have configuration command lines as mentioned below, + any other information in this file is treated as invalid. + +Configuration command line syntax: +--------------------------------- +1. Each command line is must have two strings, first one command or address + and second one data string +2. Following are the valid command strings and associated data strings:- + Command string data string + -------------- ----------- + BOOT_FROM nand/spi/sata + NAND_ECC_MODE default/rs/hamming/disabled + NAND_PAGE_SIZE any uint16_t hex value + SATA_PIO_MODE any uint32_t hex value + DDR_INIT_DELAY any uint32_t hex value + DATA regaddr and regdara hex value + you can have maximum 55 such register programming commands + +3. All commands are optional to program + +Typical example of kwimage.cfg file: +----------------------------------- + +# Boot Media configurations +BOOT_FROM nand +NAND_ECC_MODE default +NAND_PAGE_SIZE 0x0800 + +# Configure RGMII-0 interface pad voltage to 1.8V +DATA 0xFFD100e0 0x1b1b1b9b +# DRAM Configuration +DATA 0xFFD01400 0x43000c30 +DATA 0xFFD01404 0x37543000 +DATA 0xFFD01408 0x22125451 +DATA 0xFFD0140C 0x00000a33 +DATA 0xFFD01410 0x000000cc +DATA 0xFFD01414 0x00000000 +DATA 0xFFD01418 0x00000000 +DATA 0xFFD0141C 0x00000C52 +DATA 0xFFD01420 0x00000040 +DATA 0xFFD01424 0x0000F17F +DATA 0xFFD01428 0x00085520 +DATA 0xFFD0147C 0x00008552 +DATA 0xFFD01504 0x0FFFFFF1 +DATA 0xFFD01508 0x10000000 +DATA 0xFFD0150C 0x0FFFFFF5 +DATA 0xFFD01514 0x00000000 +DATA 0xFFD0151C 0x00000000 +DATA 0xFFD01494 0x00030000 +DATA 0xFFD01498 0x00000000 +DATA 0xFFD0149C 0x0000E803 +DATA 0xFFD01480 0x00000001 +# End of Header extension +DATA 0x0 0x0 + +------------------------------------------------ +Author: Prafulla Wadaskar prafulla@marvell.com diff --git a/include/image.h b/include/image.h index f119cee..513f285 100644 --- a/include/image.h +++ b/include/image.h @@ -155,7 +155,8 @@ #define IH_TYPE_SCRIPT 6 /* Script file */ #define IH_TYPE_FILESYSTEM 7 /* Filesystem Image (any type) */ #define IH_TYPE_FLATDT 8 /* Binary Flat Device Tree Blob */ -#define IH_TYPE_MAX IH_TYPE_FLATDT /* Max supported types */ +#define IH_TYPE_KWBIMAGE 9 /* Kirkwood Boot Image */ +#define IH_TYPE_MAX IH_TYPE_KWBIMAGE

/* * Compression Types diff --git a/tools/Makefile b/tools/Makefile index 6ef15d9..07a1c27 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -172,6 +172,7 @@ $(obj)img2srec$(SFX): $(obj)img2srec.o

$(obj)mkimage$(SFX): $(obj)mkimage.o $(obj)crc32.o $(obj)image.o $(obj)md5.o \ $(obj)default_image.o \ + $(obj)kwbimage.o \ $(obj)sha1.o $(LIBFDT_OBJS) $(obj)os_support.o $(CC) $(CFLAGS) $(HOST_LDFLAGS) -o $@ $^ $(STRIP) $@ @@ -207,6 +208,9 @@ $(obj)image.o: $(SRCTREE)/common/image.c $(obj)default_image.o: $(SRCTREE)/tools/default_image.c $(CC) -g $(FIT_CFLAGS) -c -o $@ $<

+$(obj)kwbimage.o: $(SRCTREE)/tools/kwbimage.c + $(CC) -g $(FIT_CFLAGS) -c -o $@ $< + $(obj)mkimage.o: $(SRCTREE)/tools/mkimage.c $(CC) -g $(FIT_CFLAGS) -c -o $@ $<

diff --git a/tools/kwbimage.c b/tools/kwbimage.c new file mode 100644 index 0000000..bebe142 --- /dev/null +++ b/tools/kwbimage.c @@ -0,0 +1,364 @@ +/* + * (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 + */ + +#include "mkimage.h" +#include <image.h> +#include "kwbimage.h" + +/* + * Suported commands for configuration file + */ +static table_entry_t kwbimage_cmds[] = { + { CMD_BOOT_FROM, "BOOT_FROM", "", }, + { CMD_NAND_ECC_MODE, "NAND_ECC_MODE", "", }, + { CMD_NAND_PAGE_SIZE, "NAND_PAGE_SIZE", "", }, + { CMD_SATA_PIO_MODE, "SATA_PIO_MODE", "", }, + { CMD_DDR_INIT_DELAY, "DDR_INIT_DELAY", "", }, + { CMD_DATA, "DATA", "", }, + { CMD_INVALID, "", "", }, +}; + +/* + * Suported Boot options for configuration file + */ +static table_entry_t kwbimage_bootops[] = { + { IBR_HDR_SPI_ID, "spi", "SPI Flash", }, + { IBR_HDR_NAND_ID, "nand", "NAND Flash", }, + { IBR_HDR_SATA_ID, "sata", "Sata port", }, + { IBR_HDR_PEX_ID, "pex", "PCIe port", }, + { IBR_HDR_UART_ID, "uart", "Serial port", }, + { -1, "", "Invalid", }, +}; + +/* + * Suported NAND ecc options configuration file + */ +static table_entry_t kwbimage_eccmodes[] = { + { IBR_HDR_ECC_DEFAULT, "default", "", }, + { IBR_HDR_ECC_FORCED_HAMMING,"hamming", "", }, + { IBR_HDR_ECC_FORCED_RS, "rs", "", }, + { IBR_HDR_ECC_DISABLED, "disabled", "", }, + { -1, "", "", }, +}; + +static struct kwb_header kwbimage_header; +static int datacmd_cnt = 0; +static char * fname = "Unknown"; +static int lineno = -1; + +struct image_type_params kwbimage_params = { + .header_size = sizeof(struct kwb_header), + .hdr = (void*)&kwbimage_header, + .check_params = kwbimage_check_params, + .verify_header = kwbimage_verify_header, + .print_header = kwbimage_print_header, + .set_header = kwbimage_set_header, +}; + +void *kwbimage_get_tparams (void){ + return (void *)&kwbimage_params; +} + +/* + * Report Error if xflag is set in addition to default + */ +int kwbimage_check_params (struct mkimage_params *params) +{ + if (!strlen (params->imagename)) { + printf ("Error:%s - Configuration file not specified, " + "it is needed for kwbimage generation\n", + params->cmdname); + return CFG_INVALID; + } + return ((params->dflag && (params->fflag || params->lflag)) || + (params->fflag && (params->dflag || params->lflag)) || + (params->lflag && (params->dflag || params->fflag)) || + (params->xflag) || !(strlen (params->imagename))); +} + +uint32_t check_get_hexval (char *token) +{ + uint32_t hexval; + + if (!sscanf (token, "%x", &hexval)) { + printf ("Error:%s[%d] - Invalid hex data\n", fname, lineno); + exit (EXIT_FAILURE); + } + return hexval; +} + +/* + * Generates 8 bit checksum + */ +uint8_t kwbimage_checksum8 (void *start, uint32_t len, uint8_t csum) +{ + register uint8_t sum = csum; + volatile uint8_t *startp = (volatile uint8_t *)start; + + do { + sum += *startp; + startp++; + } while (--len); + return (sum); +} + +/* + * Generates 32 bit checksum + */ +uint32_t kwbimage_checksum32 (uint32_t *start, uint32_t len, uint32_t csum) +{ + register uint32_t sum = csum; + volatile uint32_t *startp = start; + + do { + sum += *startp; + startp++; + len -= sizeof(uint32_t); + } while (len > 0); + return (sum); +} + +void kwbimage_check_cfgdata (char *token, enum kwbimage_cmd cmdsw, + struct kwb_header *kwbhdr) +{ + bhr_t *mhdr = &kwbhdr->kwb_hdr; + extbhr_t *exthdr = &kwbhdr->kwb_exthdr; + int i; + + switch (cmdsw) { + case CMD_BOOT_FROM: + i = get_table_entry_id (kwbimage_bootops, + "Kwbimage boot option", token); + + if (i < 0) + goto INVL_DATA; + + mhdr->blockid = i; + printf ("Preparing kirkwood boot image to boot " + "from %s\n", token); + break; + case CMD_NAND_ECC_MODE: + i = get_table_entry_id (kwbimage_eccmodes, + "NAND ecc mode", token); + + if (i < 0) + goto INVL_DATA; + + mhdr->nandeccmode = i; + printf ("Nand ECC mode = %s\n", token); + break; + case CMD_NAND_PAGE_SIZE: + mhdr->nandpagesize = + (uint16_t) check_get_hexval (token); + printf ("Nand page size = 0x%x\n", + mhdr->nandpagesize); + break; + case CMD_SATA_PIO_MODE: + mhdr->satapiomode = + (uint8_t) check_get_hexval (token); + printf ("Sata PIO mode = 0x%x\n", + mhdr->satapiomode); + break; + case CMD_DDR_INIT_DELAY: + mhdr->ddrinitdelay = + (uint16_t) check_get_hexval (token); + printf ("DDR init delay = %d msec\n", + mhdr->ddrinitdelay); + break; + case CMD_DATA: + exthdr->rcfg[datacmd_cnt].raddr = + (uint32_t) check_get_hexval (token); + + break; + case CMD_INVALID: + default: +INVL_DATA: + printf ("Error:%s[%d] - Invalid data\n", fname, lineno); + exit (EXIT_FAILURE); + } +} + +void kwdimage_set_ext_header (struct kwb_header *kwbhdr, char* name) { + bhr_t *mhdr = &kwbhdr->kwb_hdr; + extbhr_t *exthdr = &kwbhdr->kwb_exthdr; + FILE *fd = NULL; + int j; + char *line = NULL; + char * token, *saveptr1, *saveptr2; + size_t len = 0; + enum kwbimage_cmd cmd; + + fname = name; + /* set dram register offset */ + exthdr->dramregsoffs = (uint32_t)&exthdr->rcfg - (uint32_t)mhdr; + + if ((fd = fopen (name, "r")) == 0) { + printf ("Error:%s - Can't open\n", fname); + exit (EXIT_FAILURE); + } + + /* Simple kwimage.cfg file parser */ + lineno=0; + while ((getline (&line, &len, fd)) != -1) { + lineno++; + token = strtok_r (line, "\r\n", &saveptr1); + /* drop all lines with zero tokens (= empty lines) */ + if (token == NULL) + continue; + + for (j = 0, cmd = CMD_INVALID, line = token; ; line = NULL) { + token = strtok_r (line, " \t", &saveptr2); + if (token == NULL) + break; + /* Drop all text starting with '#' as comments */ + if (token[0] == '#') + break; + + /* Process rest as valid config command line */ + switch (j) { + case CFG_COMMAND: + cmd = get_table_entry_id (kwbimage_cmds, + "Kwbimage command", token); + + if (cmd == CMD_INVALID) + goto INVL_CMD; + break; + + case CFG_DATA0: + kwbimage_check_cfgdata (token, cmd, kwbhdr); + break; + + case CFG_DATA1: + if (cmd != CMD_DATA) + goto INVL_CMD; + + exthdr->rcfg[datacmd_cnt].rdata = + (uint32_t) check_get_hexval (token); + + if (datacmd_cnt > KWBIMAGE_MAX_CONFIG ) { + printf ("Error:%s[%d] - Found more " + "than max(%d) allowed " + "data configurations\n", + fname, lineno, + KWBIMAGE_MAX_CONFIG); + exit (EXIT_FAILURE); + } else + datacmd_cnt++; + break; + + default: + /* + * Command format permits maximum three + * strings on a line, i.e. command and data + * if more than two are observed, then error + * will be reported + */ +INVL_CMD: + printf ("Error:%s[%d] - Invalid command\n", + fname, lineno); + exit (EXIT_FAILURE); + } + j++; + } + } + if (line) + free (line); + + fclose (fd); +} + +void kwbimage_set_header (char *ptr, struct stat *sbuf, int ifd, + struct mkimage_params *params) +{ + struct kwb_header *hdr = (struct kwb_header *)ptr; + bhr_t *mhdr = &hdr->kwb_hdr; + extbhr_t *exthdr = &hdr->kwb_exthdr; + uint32_t checksum; + int size; + + /* Build and add image checksum header */ + checksum = kwbimage_checksum32 ((uint32_t *)ptr, sbuf->st_size, 0); + + size = write (ifd, &checksum, sizeof(uint32_t)); + if (size != sizeof(uint32_t)) { + printf ("Error:%s - Checksum write %d bytes %s\n", + params->cmdname, params->imagefile, size); + exit (EXIT_FAILURE); + } + + sbuf->st_size += sizeof(uint32_t); + + mhdr->blocksize = sbuf->st_size - sizeof(struct kwb_header); + mhdr->srcaddr = sizeof(struct kwb_header); + mhdr->destaddr= params->addr; + mhdr->execaddr =params->ep; + mhdr->ext = 0x1; /* header extension appended */ + + kwdimage_set_ext_header (hdr, params->imagename); + /* calculate checksums */ + mhdr->checkSum = kwbimage_checksum8 ((void *)mhdr, sizeof(bhr_t), 0); + exthdr->checkSum = kwbimage_checksum8 ((void *)exthdr, + sizeof(extbhr_t), 0); +} + + +/* -l support functions */ +int kwbimage_verify_header (char *ptr, int image_size, + struct mkimage_params *params) +{ + struct kwb_header *hdr = (struct kwb_header *)ptr; + bhr_t *mhdr = &hdr->kwb_hdr; + extbhr_t *exthdr = &hdr->kwb_exthdr; + uint8_t calc_hdrcsum; + uint8_t calc_exthdrcsum; + + calc_hdrcsum = kwbimage_checksum8 ((void *)mhdr, + sizeof(bhr_t) - sizeof(uint8_t), 0); + if (calc_hdrcsum != mhdr->checkSum) { + return -FDT_ERR_BADSTRUCTURE; /* mhdr csum not matched */ + } + calc_exthdrcsum = kwbimage_checksum8 ((void *)exthdr, + sizeof(extbhr_t) - sizeof(uint8_t), 0); + if (calc_hdrcsum != mhdr->checkSum) { + return -FDT_ERR_BADSTRUCTURE; /* exthdr csum not matched */ + } + return 0; +} + +void kwbimage_print_header (char *ptr) +{ + struct kwb_header *hdr = (struct kwb_header *) ptr; + bhr_t *mhdr = &hdr->kwb_hdr; + + printf ("Image Type: Kirkwood Boot from %s Image\n", + get_table_entry_name (kwbimage_bootops, + "Kwbimage boot option", + (int) mhdr->blockid)); + + printf ("Data Size: "); + genimg_print_size (mhdr->blocksize - sizeof(uint32_t)); + printf ("Load Address: %08x\n", mhdr->destaddr); + printf ("Entry Point: %08x\n", mhdr->execaddr); +} + diff --git a/tools/kwbimage.h b/tools/kwbimage.h new file mode 100644 index 0000000..8a24cb4 --- /dev/null +++ b/tools/kwbimage.h @@ -0,0 +1,118 @@ +/* + * (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 + */ + +#ifndef _KWBIMAGE_H_ +#define _KWBIMAGE_H_ + +#include <stdint.h> + +#define KWBIMAGE_MAX_CONFIG ((0x1dc - 0x20)/sizeof(struct reg_config)) +#define MAX_TEMPBUF_LEN 32 + +/* NAND ECC Mode */ +#define IBR_HDR_ECC_DEFAULT 0x00 +#define IBR_HDR_ECC_FORCED_HAMMING 0x01 +#define IBR_HDR_ECC_FORCED_RS 0x02 +#define IBR_HDR_ECC_DISABLED 0x03 + +/* Boot Type - block ID */ +#define IBR_HDR_I2C_ID 0x4D +#define IBR_HDR_SPI_ID 0x5A +#define IBR_HDR_NAND_ID 0x8B +#define IBR_HDR_SATA_ID 0x78 +#define IBR_HDR_PEX_ID 0x9C +#define IBR_HDR_UART_ID 0x69 +#define IBR_DEF_ATTRIB 0x00 + +enum kwbimage_cmd { + CMD_INVALID, + CMD_BOOT_FROM, + CMD_NAND_ECC_MODE, + CMD_NAND_PAGE_SIZE, + CMD_SATA_PIO_MODE, + CMD_DDR_INIT_DELAY, + CMD_DATA +}; + +enum kwbimage_cmd_types { + CFG_INVALID = -1, + CFG_COMMAND, + CFG_DATA0, + CFG_DATA1 +}; + +/* typedefs */ +typedef struct bhr_t { + uint8_t blockid; /*0 */ + uint8_t nandeccmode; /*1 */ + uint16_t nandpagesize; /*2-3 */ + uint32_t blocksize; /*4-7 */ + uint32_t rsvd1; /*8-11 */ + uint32_t srcaddr; /*12-15 */ + uint32_t destaddr; /*16-19 */ + uint32_t execaddr; /*20-23 */ + uint8_t satapiomode; /*24 */ + uint8_t rsvd3; /*25 */ + uint16_t ddrinitdelay; /*26-27 */ + uint16_t rsvd2; /*28-29 */ + uint8_t ext; /*30 */ + uint8_t checkSum; /*31 */ +} bhr_t, *pbhr_t; + +struct reg_config { + uint32_t raddr; + uint32_t rdata; +}; + +typedef struct extbhr_t { + uint32_t dramregsoffs; + uint8_t rsrvd1[0x20 - sizeof(uint32_t)]; + struct reg_config rcfg[KWBIMAGE_MAX_CONFIG]; + uint8_t rsrvd2[7]; + uint8_t checkSum; +} extbhr_t, *pextbhr_t; + +struct kwb_header { + bhr_t kwb_hdr; + extbhr_t kwb_exthdr; +}; + +/* + * functions + */ +uint8_t kwbimage_checksum8 (void *start, uint32_t len, uint8_t csum); +uint32_t kwbimage_checksum32 (uint32_t *start, uint32_t len, uint32_t csum); + +/* + * callback functions for mkimage + */ +void *kwbimage_get_tparams (void); +int kwbimage_check_params (struct mkimage_params *params); +int kwbimage_verify_header (char *ptr, int image_size, + struct mkimage_params *params); +void kwbimage_print_header (char *ptr); +void kwbimage_set_header (char *ptr, struct stat *sbuf, int ifd, + struct mkimage_params *params); + +#endif /* _KWBIMAGE_H_ */ diff --git a/tools/mkimage.c b/tools/mkimage.c index 3a3cb19..7e29610 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -25,6 +25,7 @@ #include "mkimage.h" #include <image.h> #include "default_image.h" +#include "kwbimage.h"

static void copy_file(int, const char *, int); static void usage(void); @@ -59,6 +60,7 @@ static struct image_functions image_ftable[] = { {image_get_tparams,}, /* for IH_TYPE_SCRIPT */ {image_get_tparams,}, /* for IH_TYPE_STANDALONE */ {fitimage_get_tparams,}, /* for IH_TYPE_FLATDT */ + {kwbimage_get_tparams,}, /* for IH_TYPE_KWBIMAGE */ };

int @@ -111,7 +113,7 @@ main (int argc, char **argv) (params.opt_type = genimg_get_type_id (*++argv)) < 0) usage (); - if (image_ftable_size <= params.opt_type) { + if (image_ftable_size < params.opt_type) { fprintf (stderr, "%s: Unsupported image type %s\n", params.cmdname, *argv);

Dear Prafulla Wadaskar,

In message 1248804270-13715-7-git-send-email-prafulla@marvell.com you wrote:

This is Third step towards cleaning mkimage code for kwbimage support in clean way.

Umm... The "Third step" was already in patch 4/8. I guess you have to check your commit messages better...

diff --git a/tools/Makefile b/tools/Makefile index 6ef15d9..07a1c27 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -172,6 +172,7 @@ $(obj)img2srec$(SFX): $(obj)img2srec.o

$(obj)mkimage$(SFX): $(obj)mkimage.o $(obj)crc32.o $(obj)image.o $(obj)md5.o \ $(obj)default_image.o \

• 	$(obj)kwbimage.o \
  $(obj)sha1.o $(LIBFDT_OBJS) $(obj)os_support.o
  

Please keep sorted.

--- /dev/null +++ b/tools/kwbimage.c @@ -0,0 +1,364 @@ +/*

• • (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
• */

+#include "mkimage.h" +#include <image.h> +#include "kwbimage.h"

+/*

• • Suported commands for configuration file
• */

+static table_entry_t kwbimage_cmds[] = {

• { CMD_BOOT_FROM, "BOOT_FROM", "", },
• { CMD_NAND_ECC_MODE, "NAND_ECC_MODE", "", },
• { CMD_NAND_PAGE_SIZE, "NAND_PAGE_SIZE", "", },
• { CMD_SATA_PIO_MODE, "SATA_PIO_MODE", "", },
• { CMD_DDR_INIT_DELAY, "DDR_INIT_DELAY", "", },
• { CMD_DATA, "DATA", "", },
• { CMD_INVALID, "", "", },

+};

Why don't these entries have any printable names associated?

+/*

• • Suported Boot options for configuration file

Typo.

...

+/*

• • Suported NAND ecc options configuration file
• */

+static table_entry_t kwbimage_eccmodes[] = {

• { IBR_HDR_ECC_DEFAULT, "default", "", },
• { IBR_HDR_ECC_FORCED_HAMMING,"hamming", "", },
• { IBR_HDR_ECC_FORCED_RS, "rs", "", },
• { IBR_HDR_ECC_DISABLED, "disabled", "", },
• { -1, "", "", },

+};

Why don't these entries have any printable names associated?

+uint32_t check_get_hexval (char *token) +{

• uint32_t hexval;
• if (!sscanf (token, "%x", &hexval)) {

• printf ("Error:%s[%d] - Invalid hex data\n", fname, lineno);
  

• exit (EXIT_FAILURE);
  

• }
• return hexval;

+}

scanf() is not a good or reliable conversion tool. Better avoid it. Use strto[u]l() instead.

In the error case, please also print the string that you cannot convert.

+/*

• • Generates 8 bit checksum
• */

+uint8_t kwbimage_checksum8 (void *start, uint32_t len, uint8_t csum) +{

• register uint8_t sum = csum;
• volatile uint8_t *startp = (volatile uint8_t *)start;
• do {

• sum += *startp;
  

• startp++;
  

• } while (--len);

"startp" is incorrectly named. It is only _start_ at the beginning. Just call it "p" :-)

Maybe you should add handling for the "len=0" case.

+/*

• • Generates 32 bit checksum
• */

+uint32_t kwbimage_checksum32 (uint32_t *start, uint32_t len, uint32_t csum) +{

• register uint32_t sum = csum;
• volatile uint32_t *startp = start;
• do {

• sum += *startp;
  

• startp++;
  

• len -= sizeof(uint32_t);
  

• } while (len > 0);
• return (sum);

You should error handling for the cas ethet len is not an even multiple of sizeof(uint32_t).

Maybe you should add handling for the "len=0" case.

...

• case CMD_NAND_PAGE_SIZE:

• mhdr->nandpagesize =
  

• 	(uint16_t) check_get_hexval (token);
  

• 	printf ("Nand page size = 0x%x\n",
  

• 			mhdr->nandpagesize);
  

Indentation incorrect.

• break;
  

• case CMD_SATA_PIO_MODE:

• mhdr->satapiomode =
  

• 	(uint8_t) check_get_hexval (token);
  

• 	printf ("Sata PIO mode = 0x%x\n",
  

• 			mhdr->satapiomode);
  

Indentation incorrect.

• break;
  

• case CMD_DDR_INIT_DELAY:

• mhdr->ddrinitdelay =
  

• 	(uint16_t) check_get_hexval (token);
  

• printf ("DDR init delay = %d msec\n",
  

• 			mhdr->ddrinitdelay);
  

Indentation incorrect.

• break;
  

• case CMD_DATA:

• exthdr->rcfg[datacmd_cnt].raddr =
  

• 	(uint32_t) check_get_hexval (token);
  

Umm... why are all these casts needed?

Please get rid of these.

• break;
  

• case CMD_INVALID:
• default:

+INVL_DATA:

• printf ("Error:%s[%d] - Invalid data\n", fname, lineno);
  

• exit (EXIT_FAILURE);
  

• }

I really dislike this jumping into another case. Please don;t. Move this code out of the switch().

+}

+void kwdimage_set_ext_header (struct kwb_header *kwbhdr, char* name) {

How about some comments what all these functions are supposed to do?

• 	case CFG_DATA1:
  

• 		if (cmd != CMD_DATA)
  

• 			goto INVL_CMD;
  

• 		exthdr->rcfg[datacmd_cnt].rdata =
  

• 			(uint32_t) check_get_hexval (token);
  

• 		if (datacmd_cnt > KWBIMAGE_MAX_CONFIG ) {
  

• 			printf ("Error:%s[%d] - Found more "
  

• 				"than max(%d) allowed "
  

• 				"data configurations\n",
  

• 				fname, lineno,
  

• 				KWBIMAGE_MAX_CONFIG);
  

• 		exit (EXIT_FAILURE);
  

Indentation incorrect.

• 		} else
  

• 			datacmd_cnt++;
  

• 		break;
  

• 	default:
  

• 		/*
  

• 		 * Command format permits maximum three
  

• 		 * strings on a line, i.e. command and data
  

• 		 * if more than two are observed, then error
  

• 		 * will be reported
  

• 		 */
  

That does not make sense. If three three stings are permitted, then why throwing an error when there are three?

And "three strings on a line, i.e. command and data" sounds incorrect, too.

+INVL_CMD:

• 		printf ("Error:%s[%d] - Invalid command\n",
  

• 			fname, lineno);
  

• 		exit (EXIT_FAILURE);
  

Please do not jump right in the middle of a switch. Move this error code out of the switch.

+/* -l support functions */ +int kwbimage_verify_header (char *ptr, int image_size,

• 	struct mkimage_params *params)
  

+{

• struct kwb_header *hdr = (struct kwb_header *)ptr;
• bhr_t *mhdr = &hdr->kwb_hdr;
• extbhr_t *exthdr = &hdr->kwb_exthdr;
• uint8_t calc_hdrcsum;
• uint8_t calc_exthdrcsum;
• calc_hdrcsum = kwbimage_checksum8 ((void *)mhdr,

• 	sizeof(bhr_t) - sizeof(uint8_t), 0);
  

• if (calc_hdrcsum != mhdr->checkSum) {

• return -FDT_ERR_BADSTRUCTURE;	/* mhdr csum not matched */
  

• }

No braces needed for single line statements.

• calc_exthdrcsum = kwbimage_checksum8 ((void *)exthdr,

• 	sizeof(extbhr_t) - sizeof(uint8_t), 0);
  

• if (calc_hdrcsum != mhdr->checkSum) {

• return -FDT_ERR_BADSTRUCTURE; /* exthdr csum not matched */
  

• }

No braces needed for single line statements.

+void kwbimage_print_header (char *ptr) +{

• struct kwb_header *hdr = (struct kwb_header *) ptr;
• bhr_t *mhdr = &hdr->kwb_hdr;
• printf ("Image Type: Kirkwood Boot from %s Image\n",

• get_table_entry_name (kwbimage_bootops,
  

• 	"Kwbimage boot option",
  

• 	(int) mhdr->blockid));
  

Indentation looks ugly.

...

diff --git a/tools/mkimage.c b/tools/mkimage.c index 3a3cb19..7e29610 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -25,6 +25,7 @@ #include "mkimage.h" #include <image.h> #include "default_image.h" +#include "kwbimage.h"

static void copy_file(int, const char *, int); static void usage(void); @@ -59,6 +60,7 @@ static struct image_functions image_ftable[] = { {image_get_tparams,}, /* for IH_TYPE_SCRIPT */ {image_get_tparams,}, /* for IH_TYPE_STANDALONE */ {fitimage_get_tparams,}, /* for IH_TYPE_FLATDT */

• {kwbimage_get_tparams,}, /* for IH_TYPE_KWBIMAGE */

};

int @@ -111,7 +113,7 @@ main (int argc, char **argv) (params.opt_type = genimg_get_type_id (*++argv)) < 0) usage ();

• 		if (image_ftable_size <= params.opt_type) {
  

• 		if (image_ftable_size < params.opt_type) {
  

Ah! What's going on here????

Best regards,

Wolfgang Denk

-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Saturday, August 08, 2009 4:32 AM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik Subject: Re: [U-Boot] [PATCH 6/8] tools: mkimage: Making table_entry code global

Dear Prafulla Wadaskar,

In message 1248804270-13715-6-git-send-email-prafulla@marvell.com you wrote:

...

diff --git a/include/image.h b/include/image.h index

88a13ab..f119cee

...

100644 --- a/include/image.h +++ b/include/image.h @@ -168,6 +168,15 @@ #define IH_MAGIC 0x27051956 /* Image Magic Number

*/

...

#define IH_NMLEN 32 /* Image Name Length

*/

...

+typedef struct table_entry {

• int id; /* as defined in image.h */
• char *sname; /* short (input) name */
• char *lname; /* long (output) name */

+} table_entry_t;

Now read this code again, with the distance of a couple of days, and tell me what you think.

"as defined in image.h" - hey, this _is_ image.h !

So, "struct table_entry" - what sort of table is this?

"short (input) name", "long (output) name" - what the heck is this about?

In the old version, the declaration of the struct was followed by the declarations of the actual tables, so we could _see_ what was meant.

You rip the code out of context, which makes it unreadable and ununderstandable.

Dear Wolfgang, I have gone through all the feedback that you have provided for this entire patch series. Thanks a lot...

Now I need to take care of issues raised by you on the top of presenting them, with comments wherever necessary to make it better readable and understandable.

I tried to maintain the flow in the series for easy understanding, but I failed :-( With these patches, new code looks differently compared to original code. So I feel instead of providing incremental patches- I should provide 1. basic mkimge framework patch, 2. mkimage: add: uimage support patch, 3. mkimage: add: fit image support patch, 4. mkimage: add: Kirkwood boot image support patch. I will do this in my next post. I hope you will like it. If you have any inputs in this regard or you think this is bad plan, pls kindly let me know.

Regards.. Prafulla . .

This is a really bad idea, it seems.

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@denx.de "He only drinks when he gets depressed." "Why does he get depressed?" "Sometimes it's because he hasn't had a drink." - Terry Pratchett, _Men at Arms_

Dear Prafulla Wadaskar,

In message 1248804270-13715-3-git-send-email-prafulla@marvell.com you wrote:

This is second step towards cleaning mkimage code for kwbimage support in clean way. In this patch-

1. The image_get_header_size function call is replaced by sizeof(image_header_t) in default_image.c

I still fail to understand why this would be needed, or even wanted.

2. image header code moved form mkimage.c to default_image .c

This looks incorrect to me (not to mention the "form/from" typo): I see no code being moved, only variable declarations.

+/*

• • Default image parameters
• */

+struct image_type_params defimage_params = {

• .header_size = sizeof(image_header_t),
• .hdr = (void*)&header,

+};

+void *image_get_tparams (void){

• return (void *)&defimage_params;

+}

We need better documentation. Why would that function be needed?

int image_check_params (struct mkimage_params *params) { return ((params->dflag && (params->fflag || params->lflag)) || @@ -100,13 +114,13 @@ void image_set_header (char *ptr, struct stat *sbuf, image_header_t * hdr = (image_header_t *)ptr;

checksum = crc32 (0,

• 	(const char *)(ptr + image_get_header_size ()),
  

• 	sbuf->st_size - image_get_header_size ());
  

• 	(const char *)(ptr + sizeof(image_header_t)),
  

• 	sbuf->st_size - sizeof(image_header_t));
  

I cannot help it, but this change looks like a step backward to me. Why don't we use the header_size entry from the image_type_params here, too?

...

--- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -49,9 +49,6 @@ struct mkimage_params params = { .cmdname = "", };

-image_header_t header; -image_header_t *hdr = &header;

int main (int argc, char **argv) { @@ -59,6 +56,7 @@ main (int argc, char **argv) struct stat sbuf; unsigned char *ptr; int retval;

• struct image_type_params *tparams = image_get_tparams ();

  params.cmdname = *argv;

@@ -163,7 +161,7 @@ NXTARG: ; params.ep = params.addr; /* If XIP, entry point must be after the U-Boot header */ if (params.xflag)

• 	params.ep += image_get_header_size ();
  

• 	params.ep += tparams->header_size;
  

Would it not be better to stick with the image_get_header_size() function, and instead make it aware which image type we are asking for?

The old code was homnogenuous: it consistently used image_get_header_size(); now we have a sizeof() here and a ptr->header_size there.

I really dislike that.

Best regards,

Wolfgang Denk