Dear Ronen,

in message 3FD74E1D.1010308@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