[U-Boot] [PATCH v2] Marvell MV88F6281GTW_GE Board support

[Prafulla Wadaskar]

Hi Jean

Thanks for your comments,
Please see my reply inlined...

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj at jcrosoft.com] 
> Sent: Friday, April 17, 2009 1:15 PM
> To: Prafulla Wadaskar
> Cc: u-boot at 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 <prafulla at 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.
>