[U-Boot-Users] [PATCH 3/5] Adds support for RedBoot

[Wolfgang Denk]

Dear Pantelis,

in message <20061129172559.24638.5730.stgit at pantathon.hol.gr> you wrote:
> 
> +		- Print either all the RedBoot config variables or a specific one
> +
> +                  > redboot config printenv [config-var] [member]
> +
> +		  member is one of value, type, enable-sense, enable-key with value
> +		  being the default one.
> +
> +		- Set an environment variable from a RedBoot config variable
> +
> +                  > redboot config setenv var config-var [member]
> +
> +		  For example when given a config variable of 
> +
> +		  tsec1_esa=00:08:e5:11:32:33                                                     
> +		          type=esa enable-sense=true        
> +
> +		  > redboot config setenv ethaddr tsec1_esa
> +
> +		  would set the ethaddr environment variable to 00:08:e5:11:32:33

Can you please explain why you  need  a  separate  implementation  of
printenv  /  setenv  commands?  Why  do you make a difference between
"RedBoot config variables" and normal (U-Boot) environment variables?

> +	        > redboot exec address	
> +
> +		The PPC bd_t differs for kernels compiled for redboot; this command
> +		lets you boot such a kernel.

I have to admit that I don't like this. I'd rathwer see you using the
standard bootm command, and have the necessary  adaption  done  there
based  on  the  image  type (which should indicate that thgis is a RB
image). You have a lot of #ifdef's there anyway.


Also, please cleanup your code (indentation by TABs, trailing white
space, C++ comments, line length).

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If all the Chinese simultaneously jumped into the Pacific  off  a  10
foot platform erected 10 feet off their coast, it would cause a tidal
wave that would destroy everything in this country west of Nebraska.