[U-Boot] [PATCH 1/4] keymile: rework headerfiles for keymile boards

[Wolfgang Denk]

Dear Heiko Schocher,


sorry for the late review.

In message <4BC2CCBA.9090002 at denx.de> you wrote:
> - This patch reworks all headerfiles for keymile boards (coge, supx4,
>   eter1, suen3).
>   Furthermore, a refactoring on the whole environment variables has been
>   acomplished.

> - introduces the variable "initial_boot_bank" to the default
>   environment. The "actual_bank" is set to this value on first
>   startup.
>   It is normally set to 0. So the behaviour is as before.
>   If set to != 0, the first actual_bank is set to this
>   value as well. Thus a bootpackage can define a boot
>   bank other than 0.

Please define what a "boot bank" or a "bootpackage" is. I have no clue
what you might mean.

> +/* protected RAM */
> +#define CONFIG_PRAM		0		/* enable PRAM */

This makes little sense to me. If you enable the feature, you probably
also want to define a non-zero amount of PRAM reserved, or why do you
enable this at all?

> +/* pseudo-non volatile RAM [hex] */
> +#define CONFIG_KM_PNVRAM	0x80000
> +/* physical RAM MTD size [hex] */
> +#define CONFIG_KM_PHRAM		0x100000
> +/* resereved pram area at the end of memroy [hex] */
> +#define CONFIG_KM_RESERVED_PRAM	0x0

What's this? PRAM support is a standard feature, with well-defined
behaviour, including the location of the PRAM area.

...
> +/* resereved pram area at the end of memroy [hex] */

[typo!]

> +/* 8Mbytes for switch + 4Kbytes for bootcount */
> +#define CONFIG_KM_RESERVED_PRAM	0x801000

???

> +#define CONFIG_KM_DEF_ENV_CPU						\
> +	"addbootcount="							\
> +		"setexpr bootcountaddr ${memsize} - ${reservedpram} && "\

Argh...

...
> + at END_OF_RAM: denotes the RAM size found in variable 'memsize'
> + at reserved: reserved pram for special purpose (switch, bootcount, ...)
> +		reserved space: 'reservedpram'
...
> +This addresses are computed when running one of the boot targets:
> + - 'release': for a standalone system		kernel/rootfs from flash
> + - 'develop': for development			kernel(tftp)/rootfs(NFS)
> + - 'ramfs': rootfilesystem in RAM 		kernel(tftp)/rootfs(RAM)
> +The variable 'pram' is set accordingly.

???

> +	"addmem="							\
> +		"setexpr value ${pram} * 0x400 && "			\
> +		"setexpr value ${memsize} - 0x${value} && "		\
> +		"setenv bootargs ${bootargs} mem=0x${value}\0"		\
...


NAK.

I think this code is based on a serious misconception about how PRAM
is supposed to work.

Please re-read section "Protected RAM" in the README. None of what you
are doing here should be needed, as this is inherent functionality of
the PRAM feature. It seems to me that you are trying to use this on a
ARM system, where PRAM support seems to be missing. If so, it should
be implemented in a clean and generic way (similar to how it's done
for PowerPC, and according to the documentation in the REAME).
Manually computing this and that in a board-specific way is not
acceptable.



Other remarks:


> --- a/include/configs/keymile-common.h
> +++ b/include/configs/keymile-common.h

This is suposed to be a common file for Keymile boards, right?

> +#ifndef CONFIG_MACH_SUEN3
...
> +#else
...
> +#endif

Then this has no place here. The SUEN3 specific code should be moved
to the SUEN3 board config file.

> +/******************************************************************************/
> +/*********************************** COMMON ***********************************/
> +/******************************************************************************/

Incorrect multiline comment style. And hey, why do we need this? I
think all of this is common stuff?


....
> + at END_OF_RAM: denotes the RAM size found in variable 'memsize'

This should not be needed either.

...
> + * - 'falshargs': defaults arguments for flash base boot

This looks "fals[c]h" to me :-)

> +/*
> + * compute_addr
> + * - compute addresses and sizes
> + * - addresses are calculated form the end of memory 'memsize'
> + *
> + * - 'setpnvramaddr': compute PNVRAM address
> + * - 'setpram': compute PRAM size for develop/release target
> + * - 'setramfspram': compute PRAM size for ramfs target
> + * - 'setrootfsaddr': compute rootfilesystem address for phram
> + * - 'setvaraddr': compute /var address for phram

Please clean up after fixing PRAM support.


In general, I fail to see what the purpose of this "refactoring" is.
The statistics are pretty clear:

>  include/configs/keymile-common.h |  577 ++++++++++++++++++++++++++++++--------
>  include/configs/km8xx.h          |   35 ++--
>  include/configs/km_arm.h         |    2 -
>  include/configs/kmeter1.h        |   40 ++--
>  include/configs/mgcoge.h         |   35 ++--
>  5 files changed, 512 insertions(+), 177 deletions(-)

For each line you delete you have to insert 3 new lines. In general,
this is not an improvment.  Maybe you want to reconsider.

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
Hokey religions and ancient weapons are  no  substitute  for  a  good
blaster at your side.                                      - Han Solo