[U-Boot] [PATCH] AT91SAM9263-pm9263-support

[Wolfgang Denk]

Dear Ilko Iliev,

In message <48FCB0CB.8000304 at ronetix.at> you wrote:
> This patch adds support for the PM9263 board of Ronetix GmbH 
> (www.ronetix.at)
> 
> Signed-off-by: Ilko Iliev <iliev at ronetix.at>
> 
> ---
>  MAKEALL                              |    1 +
>  Makefile                             |    3 +
>  board/ronetix/pm9263/Makefile        |   60 +++++
>  board/ronetix/pm9263/config.mk       |    1 +
>  board/ronetix/pm9263/led.c           |   68 ++++++
>  board/ronetix/pm9263/lowlevel_init.S |  375 
> ++++++++++++++++++++++++++++++++

Ouch - line wrapped?

>  board/ronetix/pm9263/partition.c     |   47 ++++
>  board/ronetix/pm9263/pm9263.c        |  393 
> ++++++++++++++++++++++++++++++++++

Ditto?

> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -536,6 +536,7 @@ LIST_at91="		\
>  	at91sam9260ek	\
>  	at91sam9261ek	\
>  	at91sam9263ek	\
> +	pm9263	\
>  	at91sam9rlek	\
>  	cmc_pu2		\
>  	csb637		\

Please keep lists sorted, and vertical alignment of the backslashes
intact.

> diff --git a/Makefile b/Makefile
> index 9055747..2772dd5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2493,6 +2493,9 @@ at91sam9261ek_config	:	unconfig
>  at91sam9263ek_config	:	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm926ejs at91sam9263ek atmel at91
>  
> +pm9263_config	:	unconfig
> +	@$(MKCONFIG) $(@:_config=) arm arm926ejs pm9263 ronetix at91
> +

Please keep lists sorted.

>  at91sam9rlek_config	:	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm926ejs at91sam9rlek atmel at91
>  
...
> --- /dev/null
> +++ b/board/ronetix/pm9263/led.c
> @@ -0,0 +1,68 @@
...
> +void green_LED_off(void)
> +{
> +	at91_set_gpio_value(GREEN_LED, 1);
> +}
> +
> +
> +

Too many empty lines. One should be sufficient here.

> diff --git a/board/ronetix/pm9263/lowlevel_init.S b/board/ronetix/pm9263/lowlevel_init.S
> new file mode 100644
> index 0000000..de240b7
> --- /dev/null
> +++ b/board/ronetix/pm9263/lowlevel_init.S
> @@ -0,0 +1,375 @@
...
> +#define PIOD_PDR_VAL1 0xFFFF0000	/* define PDC[31:16] as DATA[31:16] */
> +#define PIOD_PPUDR_VAL 0xFFFF0000	/* no pull-up for D[31:16] */
> +#define MATRIX_EBI0CSA_VAL 0x0001010A	/* EBI0_CSA, CS1 SDRAM, CS3 NAND Flash,	3.3V memories */

Maximum line length exceeded. Actually many lines are too  long,  but
here it is obvious.


> diff --git a/common/lcd.c b/common/lcd.c
> index 25f8664..9f81031 100644
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -824,14 +824,23 @@ static void *lcd_logo (void)
>  
>  #ifdef CONFIG_ATMEL_LCD
>  # ifdef CONFIG_LCD_INFO
> +
>  	sprintf (info, "%s", U_BOOT_VERSION);

Please do not change common files witout need, and without even
mentioning it!

>  	lcd_drawchars (LCD_INFO_X, LCD_INFO_Y, (uchar *)info, strlen(info));
>  
> +#ifdef CONFIG_LCD_LOGO_TEXT1
> +	sprintf (info, CONFIG_LCD_LOGO_TEXT1);
> +#else
>  	sprintf (info, "(C) 2008 ATMEL Corp");
> +#endif	
>  	lcd_drawchars (LCD_INFO_X, LCD_INFO_Y + VIDEO_FONT_HEIGHT,
>  					(uchar *)info, strlen(info));

This change is unrelated to your board support. Please split it out
into a separate patch.

> +#ifdef CONFIG_LCD_LOGO_TEXT2
> +	sprintf (info, CONFIG_LCD_LOGO_TEXT2);
> +#else
>  	sprintf (info, "at91support at atmel.com");
> +#endif	

Ditto.

And instead of the #idfef's in the code,  please  consider  something
like this:

#ifndef CONFIG_LCD_LOGO_TEXT2
# define CONFIG_LCD_LOGO_TEXT2 "at91support at atmel.com"
#endif
...
	sprintf (info, CONFIG_LCD_LOGO_TEXT2);

Same for above.

> diff --git a/cpu/arm926ejs/at91/lowlevel_init.S b/cpu/arm926ejs/at91/lowlevel_init.S
> index ec6ad5d..7882e89 100644
> --- a/cpu/arm926ejs/at91/lowlevel_init.S
> +++ b/cpu/arm926ejs/at91/lowlevel_init.S
> @@ -27,7 +27,7 @@
>  #include <config.h>
>  #include <version.h>
>  
> -#ifndef CONFIG_SKIP_LOWLEVEL_INIT
> +#if !defined(CONFIG_SKIP_LOWLEVEL_INIT) && !defined(CONFIG_USER_LOWLEVEL_INIT)

This is unrelated to your board support, so please split it out into a
separate patch.

> diff --git a/drivers/mtd/dataflash.c b/drivers/mtd/dataflash.c
> index 049da69..e926b38 100644
> --- a/drivers/mtd/dataflash.c
> +++ b/drivers/mtd/dataflash.c
> @@ -130,8 +130,9 @@ int AT91F_DataflashInit (void)
>  			dfcode = 0;
>  			break;
>  		}
> +
>  		/* set the last area end to the dataflash size*/

Please don;t change common files without need.

> -		area_list[NB_DATAFLASH_AREA -1].end =
> +		dataflash_info[i].end_address = 

This change is unrelated to you board support and affects all boards.
Modifications by this must explicitely mentioned in the description
and discussed.

> diff --git a/include/configs/pm9263.h b/include/configs/pm9263.h
> new file mode 100644
> index 0000000..1646e9f
> --- /dev/null
> +++ b/include/configs/pm9263.h
> @@ -0,0 +1,285 @@
...
> +/* NOR flash, if populated */
> +#if 0

Please do not add dead code.

> +#define CONFIG_IPADDR		192.168.3.222
> +#define CONFIG_GATEWAYIP	192.168.3.1
> +#define CONFIG_ETHADDR		02:DE:AD:BE:EF:01
> +#define CONFIG_NETMASK		255.255.255.0
> +#define CONFIG_SERVERIP		192.168.3.1

Please never encode network paramters in the default environment.
Delete this.


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
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is  a  nanocentury.
                                               -- Tom Duff, Bell Labs