Dear Fabio Estevam,
In message 1363228354-29534-1-git-send-email-festevam@gmail.com you wrote:
From: Fabio Estevam fabio.estevam@freescale.com
Add initial support for Wandboard.
Subject and commit message are redundant, resulting in text like this:
Add initial support for Wandboard dual lite and solo.
Add initial support for Wandboard.
Please remove the second line.
MAINTAINERS | 1 + arch/arm/include/asm/arch-mx6/mx6dl_pins.h | 3 + board/wandboard/Makefile | 29 ++++ board/wandboard/wandboard.c | 181 ++++++++++++++++++++++++ boards.cfg | 2 + include/configs/wandboard.h | 207 ++++++++++++++++++++++++++++ 6 files changed, 423 insertions(+) create mode 100644 board/wandboard/Makefile create mode 100644 board/wandboard/wandboard.c create mode 100644 include/configs/wandboard.h
The patch does not apply cleanly against current u-boot-imx#master; there are conflicts for boards.cfg
It would be nice if there was some board/wandboard/README that describes how to install a new U-Boot image on a running system.
+u32 get_board_rev(void) +{
- return 0x61011;
+}
Umm... This is very ugly. Where is this magic number coming froim, and what does it mean? It makes no sense to me to hardcode the board revision number into the U-Boot image??
+int checkboard(void) +{
- puts("Board: Wandboard\n");
Should we also add information about DL / Solo here? It would probably make sense...
...
+#define CONFIG_MACH_TYPE 4412
Should this not rather be:
#define MACH_TYPE_WANDBOARD 4412 #define CONFIG_MACH_TYPE MACH_TYPE_WANDBOARD
so we can remove this when machtypes gets updated?
Is there only a single MACH_TYPE for the DL and Solo variants?
+#include <asm/arch/imx-regs.h> +#include <asm/imx-common/gpio.h>
Please move includes to the start of the file.
+/* Size of malloc() pool */ +#define CONFIG_SYS_MALLOC_LEN (10 * 1024 * 1024)
Why do we need that much?
+#define CONFIG_BOOTDELAY 1
Is there any reason for not chosing the more standard 5 second delay?
+#define CONFIG_PREBOOT ""
Omit this?
+#define CONFIG_EXTRA_ENV_SETTINGS \
- "script=boot.scr\0" \
- "uimage=uImage\0" \
- "console=ttymxc0\0" \
- "fdt_high=0xffffffff\0" \
- "initrd_high=0xffffffff\0" \
- "fdt_file=imx6dl-wandboard.dtb\0" \
Should this not be adjusted for the Solo variant?
+#define CONFIG_SYS_MEMTEST_START 0x10000000 +#define CONFIG_SYS_MEMTEST_END 0x10010000
This makes no sense. Please see doc/README.memory-test
I can confirm that the code boots on a wanboard_dl system, but it does not find the environment as used by the original Technixion port. Is this intentional?
Can we please remove the "Reset cause: WDOG" line in production mode?
Thanks.
Reviewed-by: Wolfgang Denk wd@denx.de Tested-by: Wolfgang Denk wd@denx.de
Best regards,
Wolfgang Denk