[U-Boot-Users] U-Boot success .. Linux big test

[Wolfgang Denk]

Dear Ronen,

in message <3FD74E1D.1010308 at il.marvell.com> you wrote:
> 
> O.K. I'm ready ...
> I want to submit a patch for the U-Boot project.

I'm ttrying to check this in, but an iteration will be needed as some
parts are not acceptable as implemented.


I have a few requests for future contributions:

* Please don't use C++ style (//) comments

* Please remove trailing white space

* Please stick to the "Coding Rules" (see README)

* Please write READABLE code. Something like this:
	...
	/*  RESET_REG_BITS(regOffset,bits) -
	   gets register offset and bits: a 32bit value. It set to logic '0' in the
	   internal register the bits which given as an input example:
	   RESET_REG_BITS(0x840,BIT3 | BIT24 | BIT30) - set bits: 3,24 and 30 to logic
	   '0' in register 0x840 while the other bits stays as is. */
	#define RESET_REG_BITS(regOffset,bits) \
		*(unsigned int*)(NONE_CACHEABLE | INTERNAL_REG_BASE_ADDR   \
		| regOffset) &= ~( (unsigned int)WORD_SWAP(bits) )
	/* gets register offset and bits: a 32bit value. It set to logic '1' in the
	   internal register the bits which given as an input example:
	   GT_SET_REG_BITS(0x840,BIT3 | BIT24 | BIT30) - set bits: 3,24 and 30 to logic
	   '1' in register 0x840 while the other bits stays as is. */
	 /*#define GT_SET_REG_BITS(regOffset,bits)      ((*((volatile unsigned int*)(NONE_CACHEABLE(INTERNAL_REG_BASE_ADDR) | (regOffset)))) |= ((unsigned int)WORD_SWAP(bits)))1 */
	 /*#define GT_SET_REG_BITS(regOffset,bits) RESET_REG_BITS(regOffset,bits)1 */
	#define GT_SET_REG_BITS(regOffset,bits) SET_REG_BITS(regOffset,bits)
	/* gets register offset and bits: a 32bit value. It set to logic '0' in the
	   internal register the bits which given as an input example:
	   GT_RESET_REG_BITS(0x840,BIT3 | BIT24 | BIT30) - set bits: 3,24 and 30 to
	   logic '0' in register 0x840 while the other bits stays as is. */
	 /*#define GT_RESET_REG_BITS(regOffset,bits)    ((*((volatile unsigned int*)(NONE_CACHEABLE(INTERNAL_REG_BASE_ADDR) |  (regOffset)))) &= ~((unsigned int)WORD_SWAP(bits)))1 */
	#define GT_RESET_REG_BITS(regOffset,bits) RESET_REG_BITS(regOffset,bits)
	...
  is _NOT_ readable. It is a nightmare!!!

* Please write useful comments. If the code reads
	GT_SET_REG_BITS(0x840,BIT3 | BIT24 | BIT30)
  then a comment like your "set bits: 3,24 and 30 to logic '1' in register 0x840"
  is not only redundand and useless, it just makes the code unreadable.

* Please don't use too long lines; normally the maximum is 72...80
  characters per line


I checked in most of your code now, but I ask you to clean up the following issues:

* Most files in your board directories are more or less identical; if
  there are differences these are only in names (64360 vs. 64460)
  like this:
	<       MV_REG_WRITE (MV64360_ETH_TX_FIFO_URGENT_THRESHOLD_REG (eth_port_num),
	---
	>       MV_REG_WRITE (MV64460_ETH_TX_FIFO_URGENT_THRESHOLD_REG (eth_port_num),

  The following files are 100% identical:

	board/Marvell/db64360/pci.c		board/Marvell/db64460/pci.c
	board/Marvell/db64360/Makefile		board/Marvell/db64460/Makefile
	board/Marvell/db64360/u-boot.lds	board/Marvell/db64460/u-boot.lds

  The following files are logically identical (i. e. just trivial
  differences in some identifiers):

	board/Marvell/db64360/64360.h		board/Marvell/db64460/64460.h
	board/Marvell/db64360/mv_eth.c		board/Marvell/db64460/mv_eth.c
	board/Marvell/db64360/config.mk		board/Marvell/db64460/config.mk
	board/Marvell/db64360/db64360.c		board/Marvell/db64460/db64460.c
	board/Marvell/db64360/eth.h		board/Marvell/db64460/eth.h
	board/Marvell/db64360/mpsc.c		board/Marvell/db64460/mpsc.c
	board/Marvell/db64360/mpsc.h		board/Marvell/db64460/mpsc.h
	arch/ppc/galileo/EVB64360/mv64360_eth.c	arch/ppc/galileo/EVB64460/mv64460_eth.c
	board/Marvell/db64360/mv_eth.h		board/Marvell/db64460/mv_eth.h
	board/Marvell/db64360/mv_regs.h		board/Marvell/db64460/mv_regs.h

  Such a duplication of code is not acceptable.

  Please clean this up - remove the board  specific  files,  and  use
  common  files  instead (and check that you don't duplicate existing
  code like 64360 network drivers).

* board/Marvell/include/mv_gen_reg.h contains more or less  the  same
  definitions  like  include/galileo/gt64260R.h  -- please clean this
  up (include file conflict in "cpu/74xx_7xx/start.S").

* "board/Marvell/common/serial.c" looks pretty generic to me; I think
  it can (and should) be removed

> Attached you can find a zip file containing:
> 1. board/Marvell/* - Files to support Marvell development boards db64360 
> and db64460.

Added for now, but cleanup is required. Please see above.

> 2. include/configs/* - Configuration files for db64360 and db64460

Added.

> 3. common/cmd_bootm.c, drivers/eepro100.c and net/eth.c are 3 files that 
> I added #ifdef inside them:
>     cmd_bootm - the ifdef is needed for booting of the linux kernel.

Rejected. No such #ifdef is needed or acceptable. Please perform  all
necessary  board  initializations  where apropriate, i. e. as part of
the board init sequence (see lib_ppc/board.c).

Note: this means that your board_prebootm_init() function (which must
be renamed, of course) will never be called in the  current  code  in
CVS.

>     eepro100 - without this define our PCI NIC will not function.
>     eth.c - simple initialization of the network interface.

Added.

> 4. include/74xx_7xx.h and cpu/* - Files to support the following CPUs: 
> MPC7455, MPC7457 and 750GX.

Added.

> 5. Makefile - An updated Makefile that include the Marvel development 
> boards.
> 6. MAKEALL - An updated MAKEALL file that include the Marvel development 
> boards.
> 7. README - An updated README for the new boards only. (I didn't 
> mentioned the new cpus support)
> 8. doc/*- 2 files describing the features that are supported on the 
> Marvell development boards and a list of
>             the main test cases that we run.

Added.

> Our compilation is clean  from warning or errors.
> If you find anything inappropriate in this patch, please let me know.

Also missing: entries for the MAINTAINERS and CREDITS files.


Best regards, and a Happy New Year!

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
I haven't lost my mind -- it's backed up on tape somewhere.