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

[Angelo Dureghello <sysamfw@gmail.com>]

Hi Wolfgang,

sorry but i have still a note here.

About 

#define VALUE (0)

you said to fix it globally, without parens.

Working for the fixes i found several "accepted" files (i.e. m5282.h)
full of 

#define  XYZ1 (1)
#define  XYZ2 (2)

etc etc.

CodingStyle says parens around expressions must be used, but don't seems
to set any rule for this case.

I know this is a detail, but i need to know the reason for the fix.

Best Regards
Angelo Dureghello


On Sun, Nov 04, 2012 at 11:02:36PM +0100, Wolfgang Denk wrote:
> 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
> 
> -- 
> 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