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

[Wolfgang Denk <wd@denx.de>]

Dear "Lad, Prabhakar",

In message <4665BC9CC4253445B213A010E6DC7B35CDD4FE@DBDE01.ent.ti.com> you wrote:
> 
> > > This is a Linux command line tool specific to TI's Davinci platforms, f> or
> > > flashing UBL (User Boot Loader), u-boot and u-boot Environment in the M> MC/SD
> > > card. This MMC/SD card can be used for booting Davinci platforms that s> upports
> > > 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.

Why not?

I assume by adding this tool to the U-Boot tree you want to provide a
single, self-contained build system for the boot loader on such
boards.  But this doesn;t work if we are not also building the UBL
image.

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

How exactly does this work?  You do not document this use case.

> > > +e. Using the 'uflash' utility, place the UBL and u-uoot binaries on th> e 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.

I did not understand this from your documentation.  Can you please add
this information?

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

I have no idea what you are trying to do here.  The envrionment
settings are inherently part of U-Boot, and I don't understand why you
are adding another set of settings here.

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

Sorry, but this doesn't make sense then.  It doesn't scale.  It should
be possible to use such a tool with any such board - and without
changing the code of the tool.  Instead of gard-coding all this
information, you should make the tool data driven, so that it's
sufficient to provide respective configuration data - actually I think
all this information is already present in the existing header files,
especially the board configuration file. So why not just reuse this
information?

> > You don't even print a warning or error message in case of read
> > errors?  Ouch...
> > 
>   Ok , I'll fix it in V2 version.

Please check error checking / handling for all system calls and calls
to library functions.

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
I have often regretted my speech, never my silence.