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

[Jean-Christophe PLAGNIOL-VILLARD]

> > > +#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..
ok if possible next time try a shorter name
> 
> 
> > > +	.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.
so the binary will be compile as LE or BE so __ARMEL__ or __ARMEB__ will be
defined 
> 
> > > +#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.
ok
> 
> > > +#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?
just ask why do you need 4M of malloc?
> 
> > > +/* 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 ?
8M maybe
> 
> 
> > > + */
> > > +#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 ?
it's board instance specific

Best Regards,
J.