[U-Boot] [PATCH v3 2/2] amcore: add support for amcore board
Angelo Dureghello
sysamfw at gmail.com
Mon Nov 5 22:10:20 CET 2012
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 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
More information about the U-Boot
mailing list