[U-Boot] [PATCH] Add support for NetusG20

[Wolfgang Denk]

Dear Claudio Mignanti,

In message <1277651361-26448-1-git-send-email-c.mignanti at gmail.com> you wrote:
> Add support for the NetusG20 board by Acmesystems srl.
> This board is based on AT91SAM9G20 SoC.
> 
> Signed-off-by: Claudio Mignanti <c.mignanti at gmail.com>
> ---
>  MAKEALL                                           |    1 +
>  Makefile                                          |    3 +
>  arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c |    5 +
>  board/acmesystems/netusg20/Makefile               |   56 +++++++
>  board/acmesystems/netusg20/config.mk              |    1 +
>  board/acmesystems/netusg20/led.c                  |   40 +++++
>  board/acmesystems/netusg20/netusg20.c             |  152 +++++++++++++++++
>  board/acmesystems/netusg20/partition.c            |   39 +++++
>  include/configs/netusg20.h                        |  181 +++++++++++++++++++++
>  9 files changed, 478 insertions(+), 0 deletions(-)
>  create mode 100644 board/acmesystems/netusg20/Makefile
>  create mode 100644 board/acmesystems/netusg20/config.mk
>  create mode 100644 board/acmesystems/netusg20/led.c
>  create mode 100644 board/acmesystems/netusg20/netusg20.c
>  create mode 100644 board/acmesystems/netusg20/partition.c
>  create mode 100644 include/configs/netusg20.h

Entry to MAINTAINERS missing.

> diff --git a/Makefile b/Makefile
> index 87d5214..b73659f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2867,6 +2867,9 @@ at91sam9g45ekes_config	:	unconfig
>  	fi;
>  	@$(MKCONFIG) -a at91sam9m10g45ek arm arm926ejs at91sam9m10g45ek atmel at91
>  
> +netusg20_config:	unconfig
> +	@$(MKCONFIG) $(@:_config=) arm arm926ejs netusg20 acmesystems at91
> +
>  otc570_config	:	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm926ejs otc570 esd at91

NAK. Please rebase your patch against current code. We don't add
boards to the top level Makefile any more. Add the definition to
boards.cfg instead.

> diff --git a/arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c b/arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
> index 77d49ab..87ec531 100644
> --- a/arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
> +++ b/arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
> @@ -59,7 +59,12 @@ void at91_serial3_hw_init(void)
>  {
>  	at91_pmc_t	*pmc	= (at91_pmc_t *) AT91_PMC_BASE;
>  
> +#ifdef CONFIG_NETUSG20
> +	/* pull-up active on DRXD*/
> +	at91_set_a_periph(AT91_PIO_PORTB, 14, 1);
> +#else
>  	at91_set_a_periph(AT91_PIO_PORTB, 14, 0);		/* DRXD */
> +#endif
>  	at91_set_a_periph(AT91_PIO_PORTB, 15, 1);		/* DTXD */
>  	writel(1 << AT91_ID_SYS, &pmc->pcer);
>  }

Please do not add board specific defines to common code. If really
needed, add a feature-specific #define.

...
> +#ifdef CONFIG_RESET_PHY_R
> +void reset_phy(void)
> +{
> +#ifdef CONFIG_MACB
> +	/*
> +	 * Initialize ethernet HW addr prior to starting Linux,
> +	 * needed for nfsroot
> +	 */
> +	eth_init(gd->bd);
> +#endif
> +}
> +#endif

This doesn't look right to me.

...
> +/*
> + * Hardware drivers
> + */
> +#define CONFIG_AT91_GPIO	1
> +#define CONFIG_ATMEL_USART	1
> +#undef CONFIG_USART0
> +#undef CONFIG_USART1
> +#undef CONFIG_USART2
> +#define CONFIG_USART3		1	/* USART 3 is DBGU */

Do not undef what is not defined anyway.

> +/* LED */
> +#define CONFIG_AT91_LED
> +#define	CONFIG_RED_LED		AT91_PIN_PA9	/* this is the power led */
> +#define	CONFIG_GREEN_LED	AT91_PIN_PA6	/* this is the user led */

Please use consistent code. Either ALWAYS use a TAB after a #define,
or (better) always use a SPACE.

> +/*
> + * Command line configuration.
> + */
> +#include <config_cmd_default.h>
> +#undef CONFIG_CMD_BDI
> +#undef CONFIG_CMD_FPGA
> +#undef CONFIG_CMD_IMI
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_LOADS
> +#undef CONFIG_CMD_SOURCE

What is the exact reason for undefing pretty useful commands like bdi,
imi, or source?

> +/* MMC */
> +#define CONFIG_MMC
> +#define CONFIG_CMD_MMC
> +#define CONFIG_ATMEL_MCI
> +#define CONFIG_CMD_AUTOSCRIPT
> +#define CONFIG_CMD_IMI
> +#define CONFIG_CMD_SOURCE

...especially, if you enable these later, connected to a completely
misleading comment (imi and source are in no way related to MMC
support).

> +#define CONFIG_ENV_IS_IN_DATAFLASH	1
> +#define CONFIG_SYS_MONITOR_BASE	(CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS1 + 0x8400)
> +#define CONFIG_ENV_OFFSET		0x4200
> +#define CONFIG_ENV_ADDR		(CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS1 + CONFIG_ENV_OFFSET)

Line too long. 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
'What shall we do?' said Twoflower.  'Panic?'  said  Rincewind  hope-
fully. He always held that panic was the best means of survival; back
in  the  olden days, his theory went, people faced with hungry sabre-
toothed tigers could be divided very simply in those who panicked and
those who stood there saying 'What a magnificent  brute!'  or  'Here,
pussy.'                      - Terry Pratchett, _The Light Fantastic_