Hello Kim, Kim Phillips wrote: > On Mon, 5 Jul 2010 12:23:10 +0200 > Heiko Schocher <[email protected]> wrote: > >> --- >> board/ve8313/Makefile | 50 +++++ >> board/ve8313/config.mk | 10 + >> board/ve8313/ve8313.c | 212 ++++++++++++++++++ >> boards.cfg | 1 + >> include/configs/ve8313.h | 534 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 807 insertions(+), 0 deletions(-) > > missing MAKEALL, MAINTAINERS entries, and, if there's anything unique > about this board, a doc/README.ve8313.
There is nothing unique on this board, so don;t think a doc/README.ve8313 is useful. >> diff --git a/board/ve8313/config.mk b/board/ve8313/config.mk > >> +ifndef TEXT_BASE >> +#TEXT_BASE = 0x100000 >> +TEXT_BASE = 0xfe000000 >> +endif >> + >> +#PLATFORM_CPPFLAGS += -DDEBUG > > please clean this up - remove commented out code. fixed. >> diff --git a/board/ve8313/ve8313.c b/board/ve8313/ve8313.c >> new file mode 100644 >> index 0000000..b13d1f3 >> --- /dev/null >> +++ b/board/ve8313/ve8313.c >> @@ -0,0 +1,212 @@ >> +/* >> + * (C) Copyright 2010 >> + * Heiko Schocher, DENX Software Engineering, [email protected]. > > this file contains code that looks like it was copied from files with > Freescale copyrights; why are you deleting Freescale's copyrights? Hups, Sorry for that. >> +#if defined(CONFIG_OF_LIBFDT) >> +#include <libfdt.h> >> +#endif > > I realise this was copied in, but we don't need the ifdef anymore. done. >> +/* Fixed sdram init -- doesn't use serial presence detect. >> + * >> + * This is useful for faster booting in configs where the RAM is unlikely >> + * to be changed, or for things like NAND booting where space is tight. >> + */ > > Is the memory soldered on to the board? If not, does the serial > presence detect code not work for you? It is soldered. >> +#if defined(CONFIG_HW_WATCHDOG) >> + clrbits_be32(&gpio->dat, (VE8313_WDT_EN | VE8313_WDT_TRIG)); >> + /* set WDT pins as output */ >> + setbits_be32(&gpio->dir, (VE8313_WDT_EN | VE8313_WDT_TRIG)); >> +#else >> + /* disable WDT */ >> + setbits_be32(&gpio->dat, (VE8313_WDT_EN | VE8313_WDT_TRIG)); > > inner parens not necessary, also the last WDT_EN bit setting doesn't > match the comment above it (ENable vs. disable) - perhaps nothing > need be done if HW_WATCHDOG is not set? I must explicitely disable the WDT. >> + setbits_be32(&gpio->dat, (VE8313_WDT_TRIG)); >> + clrbits_be32(&gpio->dat, (VE8313_WDT_TRIG)); > > inner parens not necessary. done. > >> +#undef CONFIG_HW_WATCHDOG > > no #undefs please. removed. >> +#define CONFIG_SYS_LBC_LBCR (0x00040000) > > parens not necessary. fixed. >> +#define CONFIG_SYS_PCI_SUBSYS_VENDORID 0x1057 /* Motorola */ > > It's Freescale: 0x1957. fixed. Thanks for your review. bye Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
