Hi Jean Thanks for your comments, Please see my reply inlined...
> -----Original Message----- > From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagn...@jcrosoft.com] > Sent: Friday, April 17, 2009 1:15 PM > To: Prafulla Wadaskar > Cc: u-boot@lists.denx.de; Ashish Karkare; Ronen Shitrit > Subject: Re: [U-Boot] [PATCH v2] Marvell MV88F6281GTW_GE Board support > > On 21:48 Wed 08 Apr , Prafulla Wadaskar wrote: > > From: prafulla_wadaskar <prafu...@marvell.com> > > > > This is Marvell's 88F6281_A0 based custom board developed > for wireless > > access point product > > > > This patch is tested for- > > 1. Boot from DRAM/SPI flash/NFS > > 2. File transfer using tftp and loadb > > 3. SPI flash read/write/erase > > 4. Booting Linux kernel and RFS from SPI flash > > Note: doImage utility needed to convert u-boot.bin to > > u-boot-spiflash.bin, DRAM configuration will be part of this utility > btw where is the spi driver? Drivers/spi/kirkwood_spi.c through Kirkwood SOC support patch :-) > > +#define MV88F6281GTW_GE_OE_HIGH > (~((BIT4)|(BIT6)|(BIT7)|(BIT12) \ > > + |(BIT13)|(BIT16)|(BIT17))) > > +#define MV88F6281GTW_GE_OE_VAL_LOW (BIT20) /*make GLED on */ > > +#define MV88F6281GTW_GE_OE_VAL_HIGH > ((BIT6)|(BIT13)|(BIT16)|(BIT17)) > plese remove the BITxx Okay.... > > + > > +/* > > + * Default values for MPP registers > > + */ > > +#define MV88F6281GTW_GE_MPP0_7 0x01112222 > > +#define MV88F6281GTW_GE_MPP8_15 0x11103311 > > +#define MV88F6281GTW_GE_MPP16_23 0x00001111 > > +#define MV88F6281GTW_GE_MPP24_31 0x22222222 > > +#define MV88F6281GTW_GE_MPP32_39 0x40440222 > > +#define MV88F6281GTW_GE_MPP40_47 0x00004444 > > +#define MV88F6281GTW_GE_MPP48_55 0x00000000 > please move all this define to a header > and if possible please use macro to describe the content Okay I will creat and move them to header > > + /* init serial */ > > + gd->baudrate = CONFIG_BAUDRATE; > > + gd->have_console = 1; > > + serial_init(); > no need please remove the serial init is done by the lib_arm/board.c Okay I will remove it > > + > > +#endif /* CONFIG_MISC_INIT_R */ > > diff --git a/board/Marvell/mv88f6281gtw_ge/u-boot.lds > > b/board/Marvell/mv88f6281gtw_ge/u-boot.lds > is it possible to have a shorter name for the board? No Jean, not possible, kernel patches also represents the same name and machine is also register with the same name, pleas bear with this, thanks.. > > + .rodata : { *(.rodata) } > please replace by this > .rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) } Okay I will do it > > + . = ALIGN(4); > > + .data : { *(.data) } > > + . = ALIGN(4); > > + .got : { *(.got) } > > +/* > > +#define CONFIG_FEROCEON_88FR131 1 /* CPU Core > subversion */ > > +#define LE 1 /* Specify LE/BE operation */ > why? Because SOC can be initialized to work in both the modes. > > +#define CONFIG_KIRKWOOD 1 /* SOC Family Name */ > > +#define CONFIG_KW88F6281 1 /* SOC Name */ > > +#define CONFIG_KW88F6281_A0 1 /* SOC Revision */ > is is not possible to detect it? I will try to detect it. > > +#define CONFIG_BAUDRATE 115200 /* console baudrate */ > ^^^^^^^^^ > whitespace please remove You mean spaces and tabs combination, I wll remove them > > + > > +#define CONFIG_SYS_PROMPT "Marvell>> " /* > Command Prompt > why not Marvell> or a board name? This is to sync up with our current u-boot and the automation tools/documentation based on it Changing it to Marvell> is not a big deal but will involve lot of unwanted efforts. > > +#define CONFIG_SYS_CBSIZE 1024 /* Console I/O > Buff Size */ > > +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE \ > > + +sizeof(CONFIG_SYS_PROMPT)+16) /* Print Buff */ > please add space before and after '+' Okay.. > > +#define CONFIG_SYS_MALLOC_LEN 0x00400000 /* 4M */ > 4M? What it should be? > > +/* size in bytes reserved for initial data */ > > +#define CONFIG_SYS_GBL_DATA_SIZE 128 > > + > > +/* > > + * Other required minimal configurations */ > > +#define CONFIG_CONSOLE_INFO_QUIET /* some code reduction */ > > +#define CONFIG_MISC_INIT_R 1 /* call misc_init_r() */ > > +#define CONFIG_NR_DRAM_BANKS 4 > ^ > whitespace please remove Okay.. > > +#define CONFIG_STACKSIZE 0x00100000 /* regular stack- 1M */ > > +#define CONFIG_SYS_LOAD_ADDR 0x00800000 /* > default load adr- 8M */ > > +#define CONFIG_SYS_MEMTEST_START 0x00400000 /* 4M */ > > +#define CONFIG_SYS_MEMTEST_END 0x007fffff /*(_8M -1) */ > _8M? What it should be ? > > + */ > > +#ifdef CONFIG_CMD_NET > > +#define CONFIG_NETCONSOLE /* include NetConsole support */ > whitespace please remove Okay .. > > +#define CONFIG_NET_MULTI /* specify more that one ports > available */ > > +#define CONFIG_KIRKWOOD_EGIGA /* Enable SOC specific > Ethernet Gigabit > > + Controller Driver */ > please use this style of multiple comment > /* > * > */ Okay.. > > +#undef CONFIG_PHY_LINK_DETECT /* detect link always on */ > > + /* specify ports to be used */ > > +#define CONFIG_KIRKWOOD_EGIGA_PORTS {TRUE,FALSE} > > + /* phy base addr for multi-chip > addressing */ > > +#define CONFIG_IPADDR 192.168.5.44 > > +#define CONFIG_SERVERIP 192.168.5.30 > > +#define CONFIG_NETMASK 255.255.255.0 > please remove the IP params Why ? > > +#define CONFIG_ENV_OVERWRITE /* ethaddr can be > reprogrammed */ > > +#endif /* CONFIG_CMD_NET */ > > + > > +/* > > + * Marvell 88Exxxx Switch configurations */ > > +#define CONFIG_RESET_PHY_R /* use reset_phy() to init > phy/swtich */ > whitespace please remove Okay.. > > +#define CONFIG_SWITCH_88E61XX /* Enable mv88e61xx > switch driver */ > > +#define CONFIG_SWITCH_MV88E6165 /* Used Switch is 88E6165 */ > > + /* p5 of 88E6165 connceted to CPU */ > > +#define CONFIG_SWITCH_88E61XX_CPU_PORT 5 > > +#define CONFIG_SWITCH_88E61XX_ENABLED_PORTS (BIT0 | > BIT1 | BIT2 | \ > > + BIT3 | BIT4 | BIT5) > please remobe this BITx Okay.. > > +#endif /* _CONFIG_MV88F6281GTW_GE_H */ > Best Regards, > J. > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
