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

[Lad, Prabhakar]

Hi Wolfgang,

On Wed, Jun 27, 2012 at 03:12:07, Wolfgang Denk wrote:
> 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?
> 
  No, we do not build UBL as part of U-Boot.

> If not, then why is this tool supposed to be part of the U-Boot tree?
> 
> How does this work with a SPL?
> 
  This command has options to flash u-boot images to MMC/SD card. When SPL
  is supported, this command can be used to flash the single SPL image to
  MMC/SD card.

> > --- 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.
> 
  Ok.

> 
> > +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?
> 
  This copy is not needed as long as the path to u-boot.bin is specified
  Correctly in command line.

> And where is the UBL binary coming from?
> 
  UBL binary is optional. We can flash only u-boot.bin.
> 
> 
> > 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...
> 
 I agree with this. Can you think of any other scenario?

> > +
> 
> And please, no trailing empty lines!
> 
  Ok.

> ...
> > +	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?
> 
  Yes.

> .
> > +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() ?
> 
   Yes that makes sense.

> > +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...
> 
  Ok , I'll fix it in V2 version.

Thx,
--Prabhakar Lad

> 
> 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
>