[U-Boot] [PATCH] add new board nas62x0

[Wolfgang Denk]

Dear Luka Perkov,

In message <20120315235958.GA16846 at w500.iskon.local> you wrote:
>
> Add support for new boards RaidSonic ICY BOX NAS6210 and NAS6220 boards.

Checkpatch reports a number of issues:

WARNING: please, no space before tabs
ERROR: trailing whitespace
WARNING: please, no spaces at the start of a line

Please fix these.

> Only difference between boards is number of SATA ports. By default we
> use only one SATA port. In order to use both SATA ports on NAS6220
> define CONFIG_NAS6220 in board config file.

You should provide a second entry to boards.cfg then which sets this
define, so that editing of the board config file is not needed.

> If you want to test the boot loader without touching nand you can boot
> u-boot using UART. In order to create UART image you need to change
> image configuration from nand to uart:
> 
> sed -i 's/nand/uart/' board/Marvell/ib62x0/kwbimage.cfg
> 
> To boot your kirkwood board from UART you will need this tool:
> 
> http://www.solinno.co.uk/public/kwuartboot/
> http://www.solinno.co.uk/public/kwuartboot/kwuartboot-0.1.tar.gz
> 
> Compile and run it like this:
> 
> ./kwuartboot /dev/ttyUSB0 /path/to/u-boot.kwb
> 
> And when it's finished open the console.

This text should probably be added to a README and checked in.  As is,
this information gets lost for the users.


> +#ifdef CONFIG_RESET_PHY_R
> +/*
> + * This must be here because of the compile issue if missing
> + */
> +void reset_phy(void) {}
> +#endif /* CONFIG_RESET_PHY_R */

This makes no sense.  If you don't need reset_phy(), then don't define
CONFIG_RESET_PHY


> +/*
> + * High Level Configuration Options (easy to change)
> + */
> +#define CONFIG_FEROCEON_88FR131	1	/* CPU Core subversion */
> +#define CONFIG_KIRKWOOD		1	/* SOC Family Name */
> +#define CONFIG_KW88F6281	1	/* SOC Name */

Please don't define values for macros that enable features only.
Please fix globally.

...
> +/*
> + * Memory Info
> + */
> +#define CONFIG_NR_DRAM_BANKS	1 	/* we have 1 bank of DRAM */
> +#define PHYS_SDRAM_1_SIZE		(256 << 20) /* SDRAM size 256MB */

please use get_ram_size() instead and auto-size the memory.

> +#define CONFIG_MAX_RAM_BANK_SIZE	(256 << 20)
...

> +/*
> + * max 4k env size is enough, but in case of nand
> + * it has to be rounded to sector size
> + */

I think this is not correct.


> +#define CONFIG_ENV_ADDR			0x80000
> +#define CONFIG_ENV_OFFSET		0x80000	/* env starts here */

Using both ADDR and OFFSET is redundant at best.  Drop one.

> +/* Data, registers and alternate blocks are at the same offset */
> +#define CONFIG_SYS_ATA_DATA_OFFSET	(0x0100)
> +#define CONFIG_SYS_ATA_REG_OFFSET	(0x0100)
> +#define CONFIG_SYS_ATA_ALT_OFFSET	(0x0100)

Please do not use parens for simple parameters.  Please fix globally.


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
Speed of a tortoise breaking the sound barrier         = 1 Machturtle