Hi Jean Thanks for your review feedback Pls see my in lined comments
-----Original Message----- From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj@jcrosoft.com] Sent: Monday, April 27, 2009 4:06 AM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ashish Karkare; Ronen Shitrit; Prabhanjan Sarnaik Subject: Re: [PATCH v3] Marvell MV88F6281GTW_GE Board support
On 07:19 Thu 23 Apr , Prafulla Wadaskar wrote:
This is Marvell's 88F6281_A0 based custom board developed
for wireless
access point product v3: updated as per review comments for v2 added
mv88f6281gtw_ge.h file
removed BITxx macros
first a general comment I do not known if it's your mailer but all tab are converted in whitespace please fix it
This should be mailer problem, I will fix it for all next patch send
+DECLARE_GLOBAL_DATA_PTR;
+int board_init(void) +{
/* Board Parameters initializations */could explain what you do a few more?
This is explained in detail in Kirkwood support file (i.e. cpu/arm926ejs/kirkwood/kwcore.c) in Kirkwood SOC support patch I will put some explanation here too for each init call
kw_window_ctrl_reg_init();kw_gpio_init(MV88F6281GTW_GE_OE_VAL_LOW,MV88F6281GTW_GE_OE_VAL_HIGH,MV88F6281GTW_GE_OE_LOW,MV88F6281GTW_GE_OE_HIGH);
kw_mpp_control_init(MV88F6281GTW_GE_MPP0_7,MV88F6281GTW_GE_MPP8_15,MV88F6281GTW_GE_MPP16_23,MV88F6281GTW_GE_MPP24_31,MV88F6281GTW_GE_MPP32_39,MV88F6281GTW_GE_MPP40_47,- MV88F6281GTW_GE_MPP48_55);
from
/* serial config */gd->baudrate = CONFIG_BAUDRATE;gd->have_console = 1;no need please remove
Okay I will remove this
/** arch number of USED SOC*/gd->bd->bi_arch_number = MACH_TYPE_MV88F6281GTW_GE;/* adress of boot parameters */gd->bd->bi_boot_params = 0x00000100;please be more consistant with the other arm boards RAM_BASE + 0x100
Okay..
return 0;+}
+int dram_init(void) +{
int i;for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {gd->bd->bi_dram[i].start = kw_sdram_bar(i);gd->bd->bi_dram[i].size = kw_sdram_bs(i);}return 0;+}
+int last_stage_init(void) +{
return 0;+}
no need please remove
Okay
+#if defined(CONFIG_MISC_INIT_R) +/* miscellaneous platform dependent init */ int misc_init_r(void) {
return kw_misc_init_r();+}
if it's really arch late init please create a generic function like arch_late_init or arch_misc_init and call it from lib_arm/board.c
+void reset_phy(void) +{ +#ifdef CONFIG_MV88E61XX_SWITCH
/* configure and initialize switch */struct mv88e61xx_config swcfg = {.name = "egiga0",.vlancfg = MV88E61XX_VLANCFG_ROUTER,.rgmii_delay = MV88E61XX_RGMII_DELAY_EN,.portstate = MV88E61XX_PORTSTT_FORWARDING,.cpuport = 5,.ports_enabled = (PORT(0) | PORT(1) | PORT(2)| PORT(3) | PORT(4) | PORT(5))};mv88e61xx_switch_initialize(&swcfg);+#endif +}
please only call reset_phy when the SWITCH is enable. it will reduce the size of u-boot whenyou do not use the switch
Yes... I will do it
+/*
- 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
could explain a few more these value
I will put explanation for this
GbePort0/1 for kernel */
+#define CONFIG_KIRKWOOD_PCIE_INIT /* Enable PCIE
Port0 for kernel */
+#define CONFIG_KIRKWOOD_RGMII_PAD_1V8 /* Set RGMII Pad voltage to +1.8V */ #endif
why? this boards is not a KIRWOOD?
No, This board is MV88F6281GTW_GE which uses 88F6281 belongs to kirkwood family of SoCs. Soc has provision to configure RGMII pad voltages to 1.8 or 3.3 Volts connected to Gbe interface (i.e. switch used on this board). Also kernel switch and Gbe controller driver does not take care of this since they are generic.
+#define CONFIG_BOOTCOMMAND
"$(x_bootcmd_kernel); setenv bootargs " \ IIRC please use ${} instead of $()
Okay
"$(x_bootargs) $(x_bootargs_root); bootm 0x6400000;"+#define CONFIG_EXTRA_ENV_SETTINGS
"x_bootargs=console=ttyS0,115200 " \
+"mtdparts=spi0.0:512k(uboot),512k@512k(psm),2m@1m(kernel),13m@3m(root
+fs)\0" \
he could be usefull to use CONFIG_MTDPARTS
I will check this and use it
Regards.. Prafulla . .