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

[Prafulla Wadaskar]

Hi Jean
Thanks for your review feedback
Pls see my in lined comments 

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj at jcrosoft.com] 
> Sent: Monday, April 27, 2009 4:06 AM
> To: Prafulla Wadaskar
> Cc: u-boot at 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 at 512k(psm),2m at 1m(kernel),13m at 3m(root
> > +fs)\0" \
> he could be usefull to use CONFIG_MTDPARTS
I will check this and use it

Regards..
Prafulla . .