Hi Jean
Thanks for your comments, Please see my reply inlined...
-----Original Message----- From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@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 prafulla@marvell.com
This is Marvell's 88F6281_A0 based custom board developed
for wireless
access point product
This patch is tested for-
- Boot from DRAM/SPI flash/NFS
- File transfer using tftp and loadb
- SPI flash read/write/erase
- 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-chipaddressing */
+#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.