[U-Boot] [PATCH] ARM AT91 new board EB+CPUx9K2

[Wolfgang Denk]

Dear Jens Scharsig,

In message <4AE6B186.9030301 at bus-elektronik.de> you wrote:
> This patch adds a new ARM AT91RM9200 board, named EB+CPUx9K2.
> 
> * support for EB+CPUx9K2 board by BuS Elektronik GmbH & Co. KG 
> * select via make EB_CPUx9K2_config 
> 
> Signed-off-by: Jens Scharsig <esw at bus-elektronik.de>
> ---
> This patch needs the [PATCH] AT91RM9200 BGA port D defines (http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/50667)

Please use shorter lines.

...
> diff --git a/board/BuS/EB+CPUx9K2/cpux9k2.c b/board/BuS/EB+CPUx9K2/cpux9k2.c
> new file mode 100644
> index 0000000..6c29d91
> --- /dev/null
> +++ b/board/BuS/EB+CPUx9K2/cpux9k2.c
...
> +int board_init(void)
> +{
> +	/* Enable Ctrlc */
> +	console_init_f();
> +
> +	/* Correct IRDA resistor problem / Set PA23_TXD in Output */
> +	((AT91PS_PIO) AT91C_BASE_PIOA)->PIO_OER = AT91C_PA23_TXD2;

Please use I/O accessors to access device registers. Please fix
globally.

> +int misc_init_r(void)
> +{
...
> +			if (tm) {
> +				sprintf(str, "%02x:%02x:%02x:%02x:%02x:%02x",
> +					mac[0], mac[1], mac[2],
> +					mac[3], mac[4], mac[5]);
> +				setenv("ethaddr", str);

Please don't reinvent the wheel. Use eth_setenv_enetaddr().

> +/*
> + * DRAM initialisations
> + */
> +
> +int dram_init(void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
> +	return 0;

Please consider using get_ram_size() for auto-sizing the memor and
perfoming at least a basic RAM test.

> +unsigned int lxt972_IsPhyConnected(AT91PS_EMAC p_mac);
> +UCHAR lxt972_GetLinkSpeed(AT91PS_EMAC p_mac);
> +UCHAR lxt972_InitPhy(AT91PS_EMAC p_mac);
> +UCHAR lxt972_AutoNegotiate(AT91PS_EMAC p_mac, int *status);

Please don't use non-stanrard data types like UCHAR.


> +void cpux9k2_nand_hw_init(void)
> +{
> +	/* Setup Smart Media, fitst enable the address range of CS3 */
> +	*AT91C_EBI_CSA |= AT91C_EBI_CS3A_SMC_SmartMedia;
> +	/* set the bus interface characteristics based on
> +	   tDS Data Set up Time 30 - ns
> +	   tDH Data Hold Time 20 - ns
> +	   tALS ALE Set up Time 20 - ns
> +	   16ns at 60 MHz ~= 3  */
> +	/*memory mapping structures */
> +#define SM_ID_RWH	(5 << 28)
> +#define SM_RWH		(1 << 28)
> +#define SM_RWS		(0 << 24)
> +#define SM_TDF		(1 << 8)
> +#define SM_NWS		(3)

Please don't add #defines right in the middle of the code. Move to
header file, or at least to head of file.

And see comment above - use I/O accessors here and everywhere.

> +#if defined(CONFIG_VIDEO)
> +/*
> + ****h* EB+CPUx9K2/drv_video_init
> + * FUNCTION
> + ***
> + */

Incorrect multiline comment style.

> +int drv_video_init(void)
> +{
> +	char *s;
> +	unsigned long splash;
> +
> +	printf("Init Video as ");
> +	s = getenv("displaywidth");
> +	if (s != NULL)
> +		display_width = simple_strtoul(s, NULL, 10);
> +	else
> +		display_width = 256;
> +	s = getenv("displayheight");
> +	if (s != NULL)
> +		display_height = simple_strtoul(s, NULL, 10);
> +	else
> +		display_height = 256;
> +	printf("%ld x %ld pixel matrix\n", display_width, display_height);
> +
> +	#define SM2_RWH		(0x7 << 28)
> +	#define SM2_RWS		(0x7 << 24)
> +	#define SM2_TDF		(15 << 8)
> +	#define SM2_NWS		(0x7F)

Please don't add #defines right in the middle of the code. Move to
header file, or at least to head of file.

> diff --git a/board/BuS/EB+CPUx9K2/u-boot.lds b/board/BuS/EB+CPUx9K2/u-boot.lds
> new file mode 100644
> index 0000000..f4fbf96
> --- /dev/null
> +++ b/board/BuS/EB+CPUx9K2/u-boot.lds

Do you really need a private linker script? Doesn't look so...

> diff --git a/include/configs/EB_CPUx9K2.h b/include/configs/EB_CPUx9K2.h
> new file mode 100644
> index 0000000..9ff743d
> --- /dev/null
> +++ b/include/configs/EB_CPUx9K2.h
...
> +
> +/*--------------------------------------------------------------------------*/
> +
> +#undef DEBUG
> +
> +/*--------------------------------------------------------------------------*/

Get rid of all this stuff. Don't undef what is not defined in the
beginning, and don't meddle with command line options.


> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_NAND
> +#define CONFIG_CMD_BMP
> +#define	CONFIG_CMD_I2C
> +#define	CONFIG_I2C_CMD_TREE
> +#define	CONFIG_I2C_CMD_NO_FLAT
> +#define	CONFIG_CMD_DATE
> +#define	CONFIG_CMD_JFFS2

You may want to keep such lists sorted. And please use a consistent
style - either always follow #define with a TAB, or (probably better)
always with a space.


> +#define CONFIG_BAUDRATE 115200
> +#define CONFIG_AT91RM9200_USART
> +#define CONFIG_DBGU			/* define DBGU as console */
> +#undef CONFIG_USART0
> +#undef CONFIG_USART1
> +
> +#undef	CONFIG_HWFLOW
> +#undef	CONFIG_MODEM_SUPPORT

Don;t undefine what is not defined.

> +/*
> + * network
> + */
> +
> +#if 0
> +#define	CONFIG_NET_MULTI		1
> +#define	CONFIG_DRIVER_AT91EMAC		1
> +#define	CONFIG_DRIVER_AT91EMAC_QUIET	1
> +#define	CONFIG_SYS_RX_ETH_BUFFER	8
> +#undef	CONFIG_RMII
> +#define	CONFIG_MII			1
> +#define	CONFIG_CMD_MII			1

Don't add dead code.

> +#define CONFIG_SYS_FLASH_ERASE_TOUT	(6*CONFIG_SYS_HZ)
> +#define CONFIG_SYS_FLASH_WRITE_TOUT	(2*CONFIG_SYS_HZ)

This looks wrong to me. A timeout is a time, but CONFIG_SYS_HZ is a
frequency, i. e. the inverse of a time. These don't mix.


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
This all sounds complicated, but it mostly does excatly what you  ex-
pect. It's just difficult for us to explain what you expect...
                       - L. Wall & R. L. Schwartz, _Programming Perl_