From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 29 Oct 2013 13:41:46 +0100 Subject: [U-Boot] [PATCH v1 3/3] arm, at91: add siemens corvus board In-Reply-To: <526F875E.3060609@gmail.com> References: <1382421100-28378-1-git-send-email-hs@denx.de> <1382421100-28378-4-git-send-email-hs@denx.de> <526DFA08.7060609@atmel.com> <526F875E.3060609@gmail.com> Message-ID: <526FAD0A.8010403@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Andreas, Am 29.10.2013 11:01, schrieb Andreas Bie?mann: > Hi Heiko, > > some additional comments on top of Bo's. > > On 10/28/2013 06:45 AM, Bo Shen wrote: >> Hi Heiko Schocher, >> Please add commit message. >> >> On 10/22/2013 13:51, Heiko Schocher wrote: >>> Signed-off-by: Boris Schmidt >>> Reviewed-by: Heiko Schocher >>> Cc: Andreas Bie?mann >>> --- >>> board/siemens/corvus/Makefile | 39 +++++ >>> board/siemens/corvus/board.c | 345 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> boards.cfg | 1 + >>> include/configs/corvus.h | 185 ++++++++++++++++++++++ >>> 4 files changed, 570 insertions(+) >>> create mode 100644 board/siemens/corvus/Makefile >>> create mode 100644 board/siemens/corvus/board.c >>> create mode 100644 include/configs/corvus.h >>> >>> diff --git a/board/siemens/corvus/board.c b/board/siemens/corvus/board.c >>> new file mode 100644 >>> index 0000000..1940da7 >>> --- /dev/null >>> +++ b/board/siemens/corvus/board.c [...] >>> +int board_init(void) >>> +{ >>> + /* Enable Ctrlc */ >>> + console_init_f(); >> >> Remove it. >> >>> + /* arch number of corvus-Board */ >>> + gd->bd->bi_arch_number = MACH_TYPE_CORVUS; >> >> move to board related configuration file. >> >>> + /* adress of boot parameters */ >> >> s/adress/address >> >>> + gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100; + >>> + >>> + board_early_init_f(); > > Why call board_early_init_f() here? Isn't it called bby generic board > code in the init_sequence_f[] when CONFIG_BOARD_EARLY_INIT_F is defined? removed. >>> +#ifdef CONFIG_CMD_NAND >>> + corvus_nand_hw_init(); >>> +#endif >>> +#ifdef CONFIG_HAS_DATAFLASH > > Isn't SPI required for dataflash? Consider rearrange the ifdiffery here. rearranged. [...] >>> +int board_eth_init(bd_t *bis) >>> +{ >>> + int rc = 0; >>> +#ifdef CONFIG_MACB >>> + rc = macb_eth_initialize(0, (void *)ATMEL_BASE_EMAC, 0x00); >>> +#endif >>> + return rc; >>> +} >>> + >>> +/* SPI chip select control */ >>> +#ifdef CONFIG_ATMEL_SPI > > I think this is not required, should be catched by garbage collector. removed this ifdef [...] >>> diff --git a/include/configs/corvus.h b/include/configs/corvus.h >>> new file mode 100644 >>> index 0000000..09513f9 >>> --- /dev/null >>> +++ b/include/configs/corvus.h >>> @@ -0,0 +1,185 @@ >>> +/* >>> + * Common board functions for siemens AT91SAM9G45 based boards >>> + * (C) Copyright 2013 Siemens AG >>> + * >>> + * Based on: >>> + * U-Boot file: include/configs/at91sam9m10g45ek.h >>> + * (C) Copyright 2007-2008 >>> + * Stelian Pop >>> + * Lead Tech Design >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> + >>> + > > remove some of the new lines here. Done. >>> +#ifndef __CONFIG_H >>> +#define __CONFIG_H >>> + >>> +#include >>> + >>> +#define MACH_TYPE_CORVUS 2066 >>> + >>> +/* >>> + * Warning: changing CONFIG_SYS_TEXT_BASE requires >>> + * adapting the initial boot program. >>> + * Since the linker has to swallow that define, we must use a pure >>> + * hex number here! >>> + */ >>> + >>> +#define CONFIG_SYS_TEXT_BASE 0x73f00000 > > Is'nt the RAM placed @0x20000000 for this devices? Ah got it, The > sam9m10 has DDR1 @CS0 and DDR0 @CS6 Yep. >>> + >>> +#define CONFIG_AT91_LEGACY >>> +#define CONFIG_ATMEL_LEGACY /* required until (g)pio is fixed */ > > The ATMEL_LEGACY is required for dataflash, please consider using the > generic mtd sf stuff instead. Ok, think about that ... [...] >>> +/* LED */ >>> +#define CONFIG_AT91_LED >>> +#define CONFIG_RED_LED AT91_PIN_PD31 /* this is the >>> user1 led */ >>> +#define CONFIG_GREEN_LED AT91_PIN_PD0 /* this is the user2 >>> led */ > > Would you mind to use the generic gpio led stuff instead? You mean ./drivers/misc/gpio_led.c ? I check this. [...] >>> +/* Ethernet */ >>> +#define CONFIG_MACB >>> +#define CONFIG_RMII >>> +#define CONFIG_NET_RETRY_COUNT 20 >>> +#define CONFIG_RESET_PHY_R >>> + >>> +/* USB */ >>> +#define CONFIG_USB_EHCI >>> +#define CONFIG_USB_EHCI_ATMEL >>> +#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 2 >>> +#define CONFIG_DOS_PARTITION >>> +#define CONFIG_USB_STORAGE >>> + >>> +#define CONFIG_SYS_LOAD_ADDR 0x22000000 /* load address */ >> >> Does this work on your board? > > ;) good catch ... Fixed. >>> + >>> +#define CONFIG_SYS_MEMTEST_START CONFIG_SYS_SDRAM_BASE >>> +#define CONFIG_SYS_MEMTEST_END 0x23e00000 >> >> Ditto. >> >>> +/* bootstrap + u-boot + env in nandflash */ >>> +#define CONFIG_ENV_IS_IN_NAND >>> +#define CONFIG_ENV_OFFSET 0x100000 >>> +#define CONFIG_ENV_OFFSET_REDUND 0x180000 >>> +#define CONFIG_ENV_SIZE 0x20000 >>> + >>> +#define CONFIG_BOOTCOMMAND \ >>> + "nand read 0x70000000 0x200000 0x300000;" \ > > nand read ${laodaddr} kernel ? > > When using mtdparts in u-boot ... just my 2? You mean: nand read ${loadaddr} kernel ;-) bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany