[U-Boot] [PATCH] Add support for the Calao QIL-A9G20 board

[Wolfgang Denk]

Dear Albin Tonnerre,

In message <1252688802-29403-1-git-send-email-albin.tonnerre at free-electrons.com> you wrote:
> The Calao SBC35-A9G20 board is manufactured and sold by Calao Systems
> <http://www.calao-systems.com>. It is built around an AT91SAM9G20 ARM SoC
> running at 400MHz. It features an Ethernet port, an SPI RTC backed by an onboard
> battery , an SD/MMC slot, a CompactFlash slot, 64Mo of SDRAM, 256Mo of NAND
> flash, two USB host ports, and an USB device port. More informations can be
> found at <http://www.calao-systems.com/articles.php?lng=en&pg=5936>
> 
> Signed-off-by: Albin Tonnerre <albin.tonnerre at free-electrons.com>
> ---
>  MAINTAINERS                       |    2 +
>  MAKEALL                           |    2 +
>  Makefile                          |   10 ++
>  board/calao/qil_a9g20/Makefile    |   55 ++++++++++
>  board/calao/qil_a9g20/config.mk   |    1 +
>  board/calao/qil_a9g20/qil_a9g20.c |  201 +++++++++++++++++++++++++++++++++++++
>  board/calao/qil_a9g20/spi.c       |   57 +++++++++++
>  include/configs/qil_a9g20.h       |  200 ++++++++++++++++++++++++++++++++++++
>  8 files changed, 528 insertions(+), 0 deletions(-)
>  create mode 100644 board/calao/qil_a9g20/Makefile
>  create mode 100644 board/calao/qil_a9g20/config.mk
>  create mode 100644 board/calao/qil_a9g20/qil_a9g20.c
>  create mode 100644 board/calao/qil_a9g20/spi.c
>  create mode 100644 include/configs/qil_a9g20.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9db278..b1e82f4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -696,6 +696,8 @@ Andrea Scian <andrea.scian at dave-tech.it>
>  
>  Albin Tonnerre <albin.tonnerre at free-electrons.com>
>  
> +	qil_a9260	ARM926EJS (AT91SAM9260 SoC)
> +	qil_a9g20	ARM926EJS (AT91SAM9G20 SoC)
>  	sbc35_a9g20	ARM926EJS (AT91SAM9G20 SoC)
>  	tny_a9260	ARM926EJS (AT91SAM9260 SoC)
>  	tny_a9g20	ARM926EJS (AT91SAM9G20 SoC)

This is a mess.

The Subject mentions adding the QIL-A9G20 board; the commit message
says you were adding the SBC35-A9G20 board (which already exists),
but actually you seem to be adding the QIL-A9G20 and QIL-A9260
boards?

Please make comments and code match.


> diff --git a/MAKEALL b/MAKEALL
> index f0ed8ea..531a667 100755
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -614,6 +614,8 @@ LIST_at91="			\
>  	m501sk			\
>  	pm9261			\
>  	pm9263			\
> +	QIL_A9260		\
> +	QIL_A9G20		\
>  	SBC35_A9G20		\
>  	TNY_A9260		\
>  	TNY_A9G20		\

Ditto.

> diff --git a/Makefile b/Makefile
> index 0449a5b..eb542a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2897,6 +2897,16 @@ at91sam9g45ekes_config	:	unconfig
>  pm9263_config	:	unconfig
>  	@$(MKCONFIG) $(@:_config=) arm arm926ejs pm9263 ronetix at91
>  
> +QIL_A9G20_NANDFLASH_config \
> +QIL_A9G20_EEPROM_config \
> +QIL_A9G20_config \
> +QIL_A9260_NANDFLASH_config \
> +QIL_A9260_EEPROM_config \
> +QIL_A9260_config	:	unconfig
> +	@mkdir -p $(obj)include
> +	@echo "#define CONFIG_$(@:_config=) 1" >$(obj)include/config.h
> +	@$(MKCONFIG) -a qil_a9g20 arm arm926ejs qil_a9g20 calao at91

Ditto.

...
> diff --git a/board/calao/qil_a9g20/qil_a9g20.c b/board/calao/qil_a9g20/qil_a9g20.c
> new file mode 100644
> index 0000000..38f4cce
> --- /dev/null
> +++ b/board/calao/qil_a9g20/qil_a9g20.c
...
> +int board_init(void)
> +{
> +	/* Enable Ctrlc */
> +	console_init_f();
> +
> +#ifdef CONFIG_QIL_A9G20
> +	gd->bd->bi_arch_number = MACH_TYPE_QIL_A9G20;
> +#else
> +	gd->bd->bi_arch_number = MACH_TYPE_QIL_A9260;
> +#endif

Please avoid the #ifdef here; use for example a #define in the board
config file instead.

...
> +#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

You must not initialize the Ethernet interface unless U-Boot is
running a network coimmand. And you should then disable it again
before booting Linux. Please read the FAQ and fix the Linux driver
issues in Linux.

...
> +/*
> + * 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

You are undefining some pretty useful commands here (like bdinfo,
iminfo, source, etc.) Is ther eany special reason for doing this?


...
> +/* USB */
> +#define CONFIG_USB_ATMEL
> +#define CONFIG_USB_OHCI_NEW		1
> +#define CONFIG_DOS_PARTITION		1
> +#define CONFIG_SYS_USB_OHCI_CPU_INIT	1
> +#define CONFIG_SYS_USB_OHCI_REGS_BASE	0x00500000	/* AT91SAM9260_UHP_BASE */

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
It would seem that evil retreats when forcibly confronted
	-- Yarnek of Excalbia, "The Savage Curtain", stardate 5906.5