[U-Boot] [PATCH] Davinci: Utility for MMC boot

[Wolfgang Denk]

Dear Prabhakar Lad,

In message <1340708826-26707-1-git-send-email-prabhakar.lad at ti.com> you wrote:
> From: Alagu Sankar <alagusankar at embwise.com>
> 
> This is a Linux command line tool specific to TI's Davinci platforms, for
> flashing UBL (User Boot Loader), u-boot and u-boot Environment in the MMC/SD
> card. This MMC/SD card can be used for booting Davinci platforms that supports
> MMC/SD boot option.

Do we also build UBL as part of the U-Boot source tree?

If not, then why is this tool supposed to be part of the U-Boot tree?

How does this work with a SPL?

> --- a/Makefile
> +++ b/Makefile
> @@ -726,7 +726,7 @@ clean:
>  	@rm -f $(obj)examples/api/demo{,.bin}
>  	@rm -f $(obj)tools/bmp_logo	   $(obj)tools/easylogo/easylogo  \
>  	       $(obj)tools/env/{fw_printenv,fw_setenv}			  \
> -	       $(obj)tools/envcrc					  \
> +	       $(obj)tools/envcrc	   $(obj)tools/uflash/uflash	  \
>  	       $(obj)tools/gdb/{astest,gdbcont,gdbsend}			  \
>  	       $(obj)tools/gen_eth_addr    $(obj)tools/img2srec		  \
>  	       $(obj)tools/mk{env,}image   $(obj)tools/mpc86x_clk	  \

Please keep list sorted.


> +e. Using the 'uflash' utility, place the UBL and u-uoot binaries on the MMC
> +   card. Copy the u-boot.bin to tools/uflash directory

Why is this copy operation needed?

And where is the UBL binary coming from?



> diff --git a/tools/uflash/config.txt b/tools/uflash/config.txt
> new file mode 100644
> index 0000000..f6acb22
> --- /dev/null
> +++ b/tools/uflash/config.txt
> @@ -0,0 +1,11 @@
> +bootargs=console=ttyS0,115200n8 root=/dev/mmcblk0p1 rootwait rootfstype=ext3 rw
> +bootcmd=ext2load mmc 0 0x80700000 boot/uImage; bootm 0x80700000
> +bootdelay=1
> +baudrate=115200
> +bootfile="uImage"
> +stdin=serial
> +stdout=serial
> +stderr=serial
> +ethact=dm9000
> +videostd=ntsc

This looks like U-Boot environment settings?  Why are these in the
tools/uflash/ directory?  I would expect these are board specific?
For example, what in case a board uses a different baud rate?

Is this really supposed to be board independent?  It doesn't look
so...

> +

And please, no trailing empty lines!

...
> +	if (!strcmp(platform, "DM3XX")) {
> +		if (!uboot_load_address)
> +			uboot_load_address = DM3XX_UBOOT_LOAD_ADDRESS;
> +		if (!uboot_entry_point)
> +			uboot_entry_point = DM3XX_UBOOT_LOAD_ADDRESS;
> +	}
> +
> +	if (!strcmp(platform, "OMAPL138")) {
> +		if (!uboot_load_address)
> +			uboot_load_address = DA850_UBOOT_LOAD_ADDRESS;
> +		if (!uboot_entry_point)
> +			uboot_entry_point = DA850_UBOOT_LOAD_ADDRESS;
> +	}

So this is actually all hardwired for a few very specific board
configurations, right?

.
> +static int get_file_size(char *fname)
> +{
> +	FILE *fp;
> +	int size;
> +
> +	fp = fopen(fname, "rb");
> +	if (fp == NULL) {
> +		fprintf(stdout, "File %s Open Error : %s\n",
> +					fname, strerror(errno));
> +		return -1;
> +	}
> +
> +	fseek(fp, 0, SEEK_END);
> +	size = ftell(fp);
> +	fclose(fp);

Why not simply using stat() ?

> +static int write_file(int devfd, char *fname)
> +{
> +	FILE *fp;
> +	int readlen, writelen;
> +
> +	fp = fopen(fname, "rb");
> +	if (fp == NULL) {
> +		fprintf(stderr, "File %s Open Error: %s",
> +				fname, strerror(errno));
> +		return -1;
> +	}
> +
> +	while ((readlen = fread(readbuf, 1, BLOCK_SIZE, fp)) > 0) {
> +		if (readlen < BLOCK_SIZE)
> +			memset(&readbuf[readlen], 0, BLOCK_SIZE-readlen);
> +
> +		writelen = write(devfd, readbuf, BLOCK_SIZE);
> +		if (writelen < BLOCK_SIZE) {
> +			close(devfd);
> +			return -1;
> +		}
> +	}
> +
> +	fclose(fp);

You don't even print a warning or error message in case of read
errors?  Ouch...


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
C makes it easy for you to shoot yourself in the foot. C++ makes that
harder, but when you do, it blows away your whole leg.
                                                 -- Bjarne Stroustrup