[U-Boot] [PATCH v4] Add initial support for Wandboard dual lite and solo.

[Wolfgang Denk]

Dear Fabio Estevam,

In message <1363228354-29534-1-git-send-email-festevam at gmail.com> you wrote:
> From: Fabio Estevam <fabio.estevam at 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 at denx.de>
Tested-by:	Wolfgang Denk <wd at 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: wd at denx.de
 The software required `Windows 95 or better', so I installed Linux.