Dear Marcel Janssen,
> From: Marcel<korg...@home.nl>
>
> Signed-off-by: Marcel<korg...@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.50...@emk-elektronik.de>
Date: Tue, 08 Feb 2011 16:35:16 +0100
Best Regards,
Reinhard
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
