[U-Boot] [PATCH 4/4] add icnova sam9g45 board

[Reinhard Meyer]

Dear Marcel Janssen,
> From: Marcel<korgull at home.nl>
>
> Signed-off-by: Marcel<korgull at home.nl>
> ---

Sorry, this patch has to be rejected for several reasond, see below.

>   Makefile                             |    4 +

Nope, boards must be added to boards.cfg.

>   board/in-circuit/icnova/Makefile     |   54 ++++++
>   board/in-circuit/icnova/at91_nand.c  |  131 +++++++++++++++

Can you elaborate why the common atmel_nand code will not do?

>   board/in-circuit/icnova/config.mk    |    1 +

config.mk is not needed anymore.

>   board/in-circuit/icnova/flash2x8.c   |  242 +++++++++++++++++++++++++++

Same here: why can't the CFI code be used?

>   board/in-circuit/icnova/icnova_arm.c |  259 +++++++++++++++++++++++++++++
>   board/in-circuit/icnova/nand.h       |    2 +
>   board/in-circuit/icnova/u-boot.lds   |   73 ++++++++
>   include/configs/icnova_sam9g45.h     |  305 ++++++++++++++++++++++++++++++++++


>   9 files changed, 1071 insertions(+), 0 deletions(-)
>   create mode 100644 board/in-circuit/icnova/Makefile
>   create mode 100644 board/in-circuit/icnova/at91_nand.c
>   create mode 100644 board/in-circuit/icnova/config.mk
>   create mode 100644 board/in-circuit/icnova/flash2x8.c
>   create mode 100644 board/in-circuit/icnova/icnova_arm.c
>   create mode 100644 board/in-circuit/icnova/nand.h
>   create mode 100644 board/in-circuit/icnova/u-boot.lds
>   create mode 100644 include/configs/icnova_sam9g45.h
>
> diff --git a/board/in-circuit/icnova/at91_nand.c b/board/in-circuit/icnova/at91_nand.c
> new file mode 100644
> +#define CFG_NAND_ALE	21
> +#define CFG_NAND_CLE	22
> +#define	MASK_ALE	(1<<  CFG_NAND_ALE)
> +#define	MASK_CLE	(1<<  CFG_NAND_CLE)
> +#define CFG_NAND_CE	AT91_PIN_PC8
> +#define CFG_NAND_RDY	AT91_PIN_PC11

This clearly belongs into the board-configuration .h file.

> +void icnova_nand_hw_init(void)
> +{
> +	unsigned long csa;
> +
> +	/* Enable CS3 */
> +	csa = at91_sys_read(AT91_MATRIX_EBICSA);
> +	at91_sys_write(AT91_MATRIX_EBICSA,
> +		       csa | AT91_MATRIX_EBI_CS3A_SMC_SMARTMEDIA);
> +
> +	/* Configure SMC CS3 for NAND/SmartMedia */
> +	at91_sys_write(AT91_SMC_SETUP(3),
> +		       AT91_SMC_NWESETUP_(8) | AT91_SMC_NCS_WRSETUP_(0) |
> +		       AT91_SMC_NRDSETUP_(8) | AT91_SMC_NCS_RDSETUP_(0));
> +	at91_sys_write(AT91_SMC_PULSE(3),
> +		       AT91_SMC_NWEPULSE_(28) | AT91_SMC_NCS_WRPULSE_(20) |
> +		       AT91_SMC_NRDPULSE_(28) | AT91_SMC_NCS_RDPULSE_(20));
> +	at91_sys_write(AT91_SMC_CYCLE(3),
> +		       AT91_SMC_NWECYCLE_(36) | AT91_SMC_NRDCYCLE_(36));
> +	at91_sys_write(AT91_SMC_MODE(3),
> +		       AT91_SMC_READMODE | AT91_SMC_WRITEMODE |
> +		       AT91_SMC_EXNWMODE_DISABLE |
> +#ifdef CONFIG_SYS_NAND_DBW_16
> +		       AT91_SMC_DBW_16 |
> +#else /* CONFIG_SYS_NAND_DBW_8 */
> +		       AT91_SMC_DBW_8 |
> +#endif
> +		       AT91_SMC_TDF_(3));
> +
> +	at91_sys_write(AT91_PMC_PCER, 1<<  AT91SAM9G45_ID_PIOC);
> +
> +	/* Configure RDY/BSY */
> +	at91_set_gpio_input(CFG_NAND_RDY, 1);
> +
> +	/* Enable NandFlash */
> +	at91_set_gpio_output(CFG_NAND_CE, 1);
> +}
> +

Have a look at the at91sam9260ek port in the rework tree how this should look like.

> diff --git a/board/in-circuit/icnova/config.mk b/board/in-circuit/icnova/config.mk
> new file mode 100644
> +CONFIG_SYS_TEXT_BASE = 0x73f00000

Belongs into board-configuration .h

> diff --git a/board/in-circuit/icnova/flash2x8.c b/board/in-circuit/icnova/flash2x8.c
> new file mode 100644
> +#ifndef CONFIG_ICNOVA_ARM9

What is that supposed to mean? Are there non-ARM9 boards
handled by the same software?

> +#include<asm/cacheflush.h>
> +#include<asm/sections.h>
> +#else
> +#define dcache_flush_unlocked() while(0)
> +#define sync_write_buffer() while(0)
> +#define uncached(addr)	addr
> +#endif

All those defines don't seem to make sense or belong into this file.

> +static void flash_identify(uint16_t *flash, flash_info_t *info)
...
> +unsigned long flash_init(void)
...
> +void flash_print_info(flash_info_t *info)
...
> +int flash_erase(flash_info_t *info, int s_first, int s_last)
...
> +int write_buff(flash_info_t *info, uchar *src,
... etc ...

All those seem to be more or less extract from the common flash code.

> diff --git a/board/in-circuit/icnova/icnova_arm.c b/board/in-circuit/icnova/icnova_arm.c
> new file mode 100644

Again: why emphase on the _arm here?


> +//#include<usb.h>

No C++ style comments, and no dead code!

> +#if defined(CONFIG_RESET_PHY_R)&&  defined(CONFIG_MACB)

We see the RESET_PHY stuff criss-cross copied all over AT91, but it appears
that is required only by a few boards.
Make sure that, when you copy code, that you understands its function and
the applicability to your hardware.

> +#if 0

Thought as much... RESET_PHY apparently is not needed.
And #if dead code is not allowed.

> +	rstc = readl(AT91_BASE_SYS + AT91_RSTC_MR);
> +
> +	/* Need to reset PHY ->  500ms reset */
...
> +#endif
> +int checkboard (void)
> +{
> +	char myboard = 0;
> +#ifdef  CONFIG_ICNOVA_ARM9
> +	printf ("Board : SAM9G45oem\n");
> +#endif
> +	printf ("------------------------\n");
> +	printf("memory \n");
> +	printf("SDRAM base 0x%08x\n",CONFIG_SYS_SDRAM_BASE);
> +	printf("NAND  base 0x%08x\n",CONFIG_SYS_NAND_BASE);
> +	printf("NOR   base 0x%08x\n",CONFIG_SYS_FLASH_BASE);
> +	printf("ENV addr   0x%08x\n",CONFIG_ENV_ADDR);	
> +	printf ("------------------------\n");
> +	printf("Interfaces \n");
> +	#ifdef CONFIG_MACB
> +	printf("Ethernet\n");
> +	#endif
> +	#ifdef CONFIG_USB_ATMEL
> +	printf("USB host\n");
> +	#endif
> +	#ifdef CONFIG_USB_GADGET
> +	printf("USB gadget : ");
> +	#ifdef CONFIG_USBD_DFU
> +	printf("DFU\n");
> +	#endif
> +	#ifdef CONFIG_USB_ETHER
> +	printf("CDC\n");
> +	#endif
> +	#endif
> +        printf ("------------------------\n");
> +	printf ("\n");
> +	return 0;

Such verbatim output is not required and most of it is
handled by generic code already.

> +int board_init(void)
> +{
> +	/* Enable Ctrlc */
> +	console_init_f();
> +	
> +        gd->bd->bi_arch_number = MACH_TYPE_AT91SAM9G45EKES;	

Excuse me, is that board identical to at91sam9g45ek/es?
If not, you need your own mach-id!
Wrong indentation with spaces.

> +int dram_init(void)
> +{
> +	gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE;

This is already broken with relocation!

> +	gd->ram_size = get_ram_size(
> +		(void *)CONFIG_SYS_SDRAM_BASE,
> +		CONFIG_SYS_SDRAM_SIZE);
> +	return 0;
> +}
> +
> +#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 is a completely dead function, and again C++ style
comments are not allowed.

> +
> +int board_eth_init(bd_t *bis)
> +{
> +	int rc = 0;
> +
> +#if defined(CONFIG_USB_ETHER)&&  defined(CONFIG_USB_GADGET_ATMEL_USBA)
> +	rc = usba_udc_probe(&brd);
> +	if (!rc)
> +	  rc = usb_eth_initialize(bis);

Wrong indentation.

> diff --git a/board/in-circuit/icnova/nand.h b/board/in-circuit/icnova/nand.h
> new file mode 100644
> index 0000000..9b0b6ea
> --- /dev/null
> +++ b/board/in-circuit/icnova/nand.h
> @@ -0,0 +1,2 @@
> +
> +void icnova_nand_hw_init(void);

A file for only one define? Probably that could go
into the board-configuration. Or becomes obsolete if common
NAND code is used. Or, like in other board ports, the NAND
init goes into the main board file.

> diff --git a/board/in-circuit/icnova/u-boot.lds b/board/in-circuit/icnova/u-boot.lds
> new file mode 100644

Again, why does the common .lds file not do?

> diff --git a/include/configs/icnova_sam9g45.h b/include/configs/icnova_sam9g45.h
> new file mode 100644
> +//#define DEBUG

Nope. (//)

> +#define CONFIG_ARM926EJS		/* This is an ARM926EJS Core */
> +#define CONFIG_ICNOVA_ARM9		/* It's an  ICnova SAM9G45 OEM boardR */
> +#define CONFIG_AT91SAM9G45		/* It's an Atmel AT91SAM9G45 SoC */
> +#define CONFIG_AT91FAMILY
> +#define CONFIG_ARCH_AT91	
> +#define CONFIG_AT91_LEGACY              /* needed for some defines */
> +#define CONFIG_ARCH_CPU_INIT
> +#undef CONFIG_USE_IRQ			/* we don't need IRQ/FIQ stuff	*/
> +
> +/* this General purpose register is used to trigger
> + * u-boot to wait for connection. For example, this can
> + * be used to upgrade the application image in th efollowing way
> + * 1) host sets the GPBR via Linux.
> + * 2) host reboots the device
> + * 3) u-boot reads the GPBR
> + * 4) u-boot will wait for new firmware
> +*/
> +#define CONFIG_USE_ATMEL_GPBR1 		1

Does the mean to define GPBR1 to be defined or to the value of 1?
We could add that GPBR1 use to at91_gpbr.h. Discussion is open here.

> +//#define CONFIG_CMD_PORTIO

No //, no dead stuff.

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

This is broken.

> +#define CONFIG_SYS_GBL_DATA_SIZE	128 /* 128 bytes for initial data */

Broken.

Please have a look at the at91sam9260ek port how it should look like and read
my post

Subject: [U-Boot] [STATUS: AT91/AVR32]
Message-ID: <4D5162B4.50606 at emk-elektronik.de>
Date: Tue, 08 Feb 2011 16:35:16 +0100

Best Regards,
Reinhard