Dear Wolfgang Denk, Here is my comments to your notices, the updated patch is in the following letter.
On Tue, 30 Mar 2010 23:15:45 +0200 Wolfgang Denk <w...@denx.de> wrote: > Dear Mikhail Kshevetskiy, > > In message <20100329162420.40f54...@laska.campus-ws.pu.ru> you wrote: > > This patch is based on custom u-boot-1.1.2 version produced by voipac > > (http://www.voipac.com) and board/trizepsiv files from current u-boot. > > > > Up to now only PXA270 DIMM module with NOR flash is tested. > > > > Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevets...@gmail.com> > > --- > > Makefile | 3 + > > board/vpac270/Makefile | 51 +++ > > board/vpac270/config.mk | 3 + > > board/vpac270/lowlevel_init.S | 740 > > +++++++++++++++++++++++++++++++++++++++++ > > board/vpac270/vpac270.c | 101 ++++++ > > include/configs/vpac270.h | 329 ++++++++++++++++++ > > 6 files changed, 1227 insertions(+), 0 deletions(-) > > create mode 100644 board/vpac270/Makefile > > create mode 100644 board/vpac270/config.mk > > create mode 100644 board/vpac270/lowlevel_init.S > > create mode 100644 board/vpac270/vpac270.c > > create mode 100644 include/configs/vpac270.h > > Entries to MAKEALL and MAINTAINERS missing. fixed > > diff --git a/board/vpac270/config.mk b/board/vpac270/config.mk > > new file mode 100644 > > index 0000000..4486f6b > > --- /dev/null > > +++ b/board/vpac270/config.mk > > @@ -0,0 +1,3 @@ > > +TEXT_BASE =0xa1f00000 > > +# 0xa1700000 > > +#TEXT_BASE = 0 > > Please do not add dead code. Remove it. fixed > > diff --git a/board/vpac270/lowlevel_init.S b/board/vpac270/lowlevel_init.S > > new file mode 100644 > > index 0000000..1df381c > ... > > +/* wait for coprocessor write complete */ > > + .macro CPWAIT reg > > + mrc p15,0,\reg,c2,c0,0 > > + mov \reg,\reg > > + sub pc,pc,#4 > > + .endm > > Indentation and vertial alignment by TAB only. Please fix globally. fixed, I try to keep lowlevel_init.S as much close to the same file from board/trizeptiv as it was possible, so initially i keep formatting and comments style from the upstream version of board/trizeptiv/lowlevel_init.S. > > > + /* Set up GPIO pins first ----------------------------------------- */ > > + > > + ldr r0, =GPSR0 > > + ldr r1, =CONFIG_SYS_GPSR0_VAL > > + str r1, [r0] > > One TAB between instruction and arguments should be sufficient. same as above > > + /* ---------------------------------------------------------------- */ > > + /* Enable memory interface */ > > + /* */ > > + /* The sequence below is based on the recommended init steps */ > > + /* detailed in the Intel PXA250 Operating Systems Developers Guide, */ > > + /* Chapter 10. */ > > + /* ---------------------------------------------------------------- */ > > Incorrect multiline comment style. Please fix globally. same as above > ... > > + /* MECR: Memory Expansion Card Register */ > > + ldr r2, =CONFIG_SYS_MECR_VAL > > + str r2, [r1, #MECR_OFFSET] > > + ldr r2, [r1, #MECR_OFFSET] > > Inconsistent indentation - please fix globally. same as above > > + /* MCMEM0: Card Interface slot 0 timing */ > > + ldr r2, =CONFIG_SYS_MCMEM0_VAL > > + str r2, [r1, #MCMEM0_OFFSET] > > + ldr r2, [r1, #MCMEM0_OFFSET] > > + > > + /* MCMEM1: Card Interface slot 1 timing */ > > + ldr r2, =CONFIG_SYS_MCMEM1_VAL > > + str r2, [r1, #MCMEM1_OFFSET] > > + ldr r2, [r1, #MCMEM1_OFFSET] > > + > > + /* MCATT0: Card Interface Attribute Space Timing, slot 0 */ > > + ldr r2, =CONFIG_SYS_MCATT0_VAL > > + str r2, [r1, #MCATT0_OFFSET] > > + ldr r2, [r1, #MCATT0_OFFSET] > > + > > + /* MCATT1: Card Interface Attribute Space Timing, slot 1 */ > > + ldr r2, =CONFIG_SYS_MCATT1_VAL > > + str r2, [r1, #MCATT1_OFFSET] > > + ldr r2, [r1, #MCATT1_OFFSET] > > + > > + /* MCIO0: Card Interface I/O Space Timing, slot 0 */ > > + ldr r2, =CONFIG_SYS_MCIO0_VAL > > + str r2, [r1, #MCIO0_OFFSET] > > + ldr r2, [r1, #MCIO0_OFFSET] > > + > > + /* MCIO1: Card Interface I/O Space Timing, slot 1 */ > > + ldr r2, =CONFIG_SYS_MCIO1_VAL > > + str r2, [r1, #MCIO1_OFFSET] > > + ldr r2, [r1, #MCIO1_OFFSET] > > + > > + /* ---------------------------------------------------------------- */ > > + /* Step 2c: Write FLYCNFG FIXME: what's that??? */ > > + /* ---------------------------------------------------------------- */ > > +@ ldr r2, =CONFIG_SYS_FLYCNFG_VAL > > +@ str r2, [r1, #FLYCNFG_OFFSET] > > +@ str r2, [r1, #FLYCNFG_OFFSET] > > + > > + /* ---------------------------------------------------------------- */ > > + /* Step 2d: Initialize Timing for Sync Memory (SDCLK0) */ > > + /* ---------------------------------------------------------------- */ > > + > > + /* Before accessing MDREFR we need a valid DRI field, so we set */ > > + /* this to power on defaults + DRI field. */ > > + > > + ldr r4, [r1, #MDREFR_OFFSET] > > + ldr r2, =0xFFF > > + bic r4, r4, r2 > > + > > + ldr r3, =CONFIG_SYS_MDREFR_VAL > > + and r3, r3, r2 > > + > > + orr r4, r4, r3 > > + str r4, [r1, #MDREFR_OFFSET] /* write back MDREFR */ > > > Hm... I think most of this does not belong into lowlevel_init.S which > really should be kept minimal. Most of this should be rewritten in C. the similar or exactly the same peace of code can be found in lowlevel initialization of almost all arm/pxa boards, so i think it is reasonable to keep this code in place. > > diff --git a/board/vpac270/vpac270.c b/board/vpac270/vpac270.c > > new file mode 100644 > > index 0000000..723e9d8 > > --- /dev/null > > +++ b/board/vpac270/vpac270.c > ... > > +int board_late_init(void) > > +{ > > +#if defined(CONFIG_SERIAL_MULTI) > > + char *console=getenv("boot_console"); > > + > > + if (console == NULL) console = BOOT_CONSOLE; > > Incorrect indentation. Please check and fix globally. fixed, The same as early, i try to keep this file similar to board/trizeptiv/conxs.c file from the upstream. > > > diff --git a/include/configs/vpac270.h b/include/configs/vpac270.h > > new file mode 100644 > > index 0000000..969d6d3 > > --- /dev/null > > +++ b/include/configs/vpac270.h > ... > > +//#define DEBUG 15 > > No C++ comments please. And don't add dead code. fixed > ... > > +#define CONFIG_BOOTDELAY 3 > > +#define CONFIG_BOOTCOMMAND "FIXME" > > +#define CONFIG_BOOTARGS "root=/dev/mtdblock2 > > rootfstype=jffs2 console=ttyS0,115200" > > Line too long. Please fix globally. fixed > > +#define CONFIG_STACKSIZE (64*1024) /* regular stack */ > > +#ifdef CONFIG_USE_IRQ > > + #define CONFIG_STACKSIZE_IRQ (4*1024) /* IRQ stack */ > > + #define CONFIG_STACKSIZE_FIQ (4*1024) /* FIQ stack */ > > +#endif > > Indentation by TAB only! fixed > > +/* > > + * GPIO settings > > + */ > > +#if 1 /* minimal vpac270 settings, includes FFUART and Ethernet */ > ... > > +#else /* maximal vpac270 settings */ > ... > > Do not add dead code. Remove the "#if 1" and the whole "else" block. fixed _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
