Dear angelo,
In message 20121104200021.GB5141@angel3 you wrote:
Add support for amcore board.
Signed-off-by: Angelo Dureghello sysamfw@gmail.com Cc: Jason Jin jason.jin@freescale.com
Changes for v2:
- None
Changes for v3:
- Fix code format issues
board/sysam/amcore/Makefile | 43 ++++ board/sysam/amcore/amcore.c | 168 ++++++++++++++ board/sysam/amcore/config.mk | 23 ++ board/sysam/amcore/flash.c | 444 ++++++++++++++++++++++++++++++++++++ board/sysam/amcore/u-boot.lds | 101 ++++++++ boards.cfg | 1 + include/configs/amcore.h | 213 +++++++++++++++++ include/flash.h | 1 + 8 files changed, 994 insertions(+), 0 deletions(-)
Entry to MAINTAINERS missing.
--- /dev/null +++ b/board/sysam/amcore/amcore.c
...
+#include <common.h> +#include <command.h> +#include <malloc.h> +#include <asm/immap.h>
+int init_lcd (void)
Only one blank line in places like that, please. Please fix globally.
+{
- /*
* board can have a K0108 lcd connected on the parallel port,* wired as below:** fc cpu P0 P1 P2 P3 P4 P5 P6 P7 P10 P11 P12 P13 P14* lcd D0 D1 D2 D3 D4 D5 D6 D7 CS1 CS2 RW DI E** Starting up setting lines in high impedance*/- mbar_writeShort(MCFSIM_PAR, 0x300);
- mbar_writeShort(MCFSIM_PADDR, 0xfcff);
- mbar_writeShort(MCFSIM_PADAT, 0x0c00);
+}
Did you bother to compile your code? This is a function returning "int", but I don't see any return statement. I would expect to see compiler warnings here?
+phys_size_t initdram (int board_type) +{
Please make sure to use get_mem_size() !
+int testdram (void)
...
+}
We have plenty of memory tests already. Chose one, but don't implement yet another one.
diff --git a/board/sysam/amcore/config.mk b/board/sysam/amcore/config.mk
...
+CONFIG_SYS_TEXT_BASE = 0xffc00000
This should never be needed. Please move this to board congih file instead.
diff --git a/board/sysam/amcore/flash.c b/board/sysam/amcore/flash.c new file mode 100644 index 0000000..971b091 --- /dev/null +++ b/board/sysam/amcore/flash.c
This looks like a standard CFI flash. Why cannot you use the standard CFI driver, then?
if (remainder) {remainder >>= 10;remainder = (int)((float)(((float)remainder/(float)1024)*10000));
Also please note that we do not use any kind of FP operations in U-Boot. Please get rid of thise - fix globally, please.
+void inline spin_wheel(void)
We don't use such stuff, either.
diff --git a/board/sysam/amcore/u-boot.lds b/board/sysam/amcore/u-boot.lds new file mode 100644 index 0000000..ccb770d --- /dev/null +++ b/board/sysam/amcore/u-boot.lds
Why exactly do you need a board specific linker script?
diff --git a/include/configs/amcore.h b/include/configs/amcore.h new file mode 100644 index 0000000..92127ea --- /dev/null +++ b/include/configs/amcore.h
...
+#define CONFIG_SYS_UART_PORT (0)
Please no parens around simple valies - p[lease fix globally.
+#define CONFIG_BOOTDELAY 1 /* autoboot after 5 seconds */
Comment and code disagree.
+#undef CONFIG_WATCHDOG +#undef CONFIG_MONITOR_IS_IN_RAM
PLease do not undefine what is not defiend anyway.
+#define CONFIG_SYS_DEVICE_NULLDEV 1 /* include nulldev device */ +#define CONFIG_SYS_CONSOLE_INFO_QUIET 1 /* no console @ startup */ +#define CONFIG_AUTO_COMPLETE 1 /* add autocompletion support */ +#define CONFIG_LOOPW 1 /* enable loopw command */ +#define CONFIG_MX_CYCLIC 1 /* enable mdc/mwc commands */
Please do not define values for any macros which are just tested for existence. Please fix globally.
+#define CONFIG_SYS_BOOTPARAMS_LEN 64*1024
Now macros like this do need parens!!
+/*
- For booting Linux, the board info and command line data
- have to be in the first 8 MB of memory, since this is
- the maximum mapped by the Linux kernel during initialization ??
- */
Is this really the case?
diff --git a/include/flash.h b/include/flash.h index 7db599e..a04ce90 100644 --- a/include/flash.h +++ b/include/flash.h @@ -282,6 +282,7 @@ extern flash_info_t *flash_get_info(ulong base); #define SST_ID_xF1601 0x234B234B /* 39xF1601 ID (16M = 1M x 16 ) */ #define SST_ID_xF1602 0x234A234A /* 39xF1602 ID (16M = 1M x 16 ) */ #define SST_ID_xF3201 0x235B235B /* 39xF3201 ID (32M = 2M x 16 ) */ +#define SST_ID_xF3201B 0x235D235D /* 39xF3201B ID (32M = 2M x 16 ) */ #define SST_ID_xF3202 0x235A235A /* 39xF3202 ID (32M = 2M x 16 ) */ #define SST_ID_xF6401 0x236B236B /* 39xF6401 ID (64M = 4M x 16 ) */ #define SST_ID_xF6402 0x236A236A /* 39xF6402 ID (64M = 4M x 16 ) */
Note - whenever you have to add anything to include/flash.h you should ask yourself what you are doing wrong.
Best regards,
Wolfgang Denk