Dear Fabio Estevam,
In message <1363228354-29534-1-git-send-email-feste...@gmail.com> you wrote:
> From: Fabio Estevam <fabio.este...@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 <w...@denx.de>
Tested-by: Wolfgang Denk <w...@denx.de>
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The software required `Windows 95 or better', so I installed Linux.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
