[U-Boot] [PATCH v3 2/2] amcore: add support for amcore board

[Wolfgang Denk]

Dear angelo,

In message <20121104200021.GB5141 at angel3> you wrote:
> Add support for amcore board.
> 
> Signed-off-by: Angelo Dureghello <sysamfw at gmail.com>
> Cc: Jason Jin <jason.jin at 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

-- 
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
Roses are red
Violets are blue
Some poems rhyme