> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, September 19, 2009 12:37 AM
> To: Hu Mingkai-B21284
> Cc: u-boot@lists.denx.de; ga...@kernel.crashing.org
> Subject: Re: [PATCH v3 1/3] NAND boot: MPC8536DS support
>
> On Fri, Sep 18, 2009 at 11:35:33AM +0800, Mingkai Hu wrote:
> > diff --git a/Makefile b/Makefile
> > index 99837a3..4d18a9f 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2446,6 +2446,7 @@ vme8349_config: unconfig
> > ATUM8548_config: unconfig
> > @$(MKCONFIG) $(@:_config=) ppc mpc85xx atum8548
> >
> > +MPC8536DS_NAND_config \
> > MPC8536DS_36BIT_config \
> > MPC8536DS_config: unconfig
>
> NAND and 36BIT are orthogonal.
>
> How about changing it to:
>
> # Options: NAND, 36BIT
> MPC8536DS_%_config MPC8536DS_config: unconfig
>
I don't get it. what's the '%'? or how to use it?
> > +#if defined(CONFIG_NAND_BR_PRELIM) \
> > + && defined(CONFIG_NAND_OR_PRELIM)
> > + out_be32(&lbc->br0, CONFIG_NAND_BR_PRELIM);
> > + out_be32(&lbc->or0, CONFIG_NAND_OR_PRELIM);
> > + /* for FPGA */
> > + out_be32(&lbc->br3, CONFIG_SYS_BR3_PRELIM);
> > + out_be32(&lbc->or3, CONFIG_SYS_OR3_PRELIM);
>
> Those last two lines should probably be #ifdef CONFIG_SYS_BR3_PRELIM.
>
Ok.
> > +#if defined(CONFIG_SYS_RAMBOOT) && defined(CONFIG_SYS_INIT_L2_ADDR)
> > + ccsr_l2cache_t *l2cache = (void *)CONFIG_SYS_MPC85xx_L2_ADDR;
> > + uint l2srbar;
> > + int i;
> > +
> > + l2srbar = CONFIG_SYS_INIT_L2_ADDR;
> > + out_be32(&l2cache->l2srbar0, l2srbar);
> > +
> > + /* set MBECCDIS=1, SBECCDIS=1 */
> > + out_be32(&l2cache->l2errdis,
> > + (MPC85xx_L2ERRDIS_MBECC |
> > + MPC85xx_L2ERRDIS_SBECC));
> > +
> > + /* set L2E=1 & L2SRAM=001 */
> > + out_be32(&l2cache->l2ctl,
> > + (MPC85xx_L2CTL_L2E |
> > + MPC85xx_L2CTL_L2SRAM_ENTIRE));
> > +
> > + /* Initialize L2 SRAM to zero */
> > + for (i = 0; i < CONFIG_SYS_L2_SIZE; i++)
> > + ((char *)l2srbar)[i] = 0;
>
> "uint" is not a valid type for either virtual or physical addresses.
>
> Use a pointer (or uintptr_t if you must) for the former, and
> phys_addr_t for the latter.
>
> You're using it as char *, so why not just declare it that way?
>
Ok, change to char *.
> > +void board_init_f(ulong bootflag)
> > +{
> > + u8 sysclk_ratio;
>
> You're not saving any space over plain int/uint...
>
> > + uint plat_ratio, bus_clk, sys_clk;
> > + volatile ccsr_gur_t *gur = (void
> *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
> > +
> > + /* initialize selected port with appropriate baud rate */
> > + sysclk_ratio = *((volatile unsigned char *)(PIXIS_BASE
> + PIXIS_SPD));
> > + sysclk_ratio &= 0x7;
> > + switch (sysclk_ratio) {
> > + case 0:
> > + sys_clk = 33333000;
> > + break;
> > + case 1:
> > + sys_clk = 39999600;
> > + break;
> > + case 2:
> > + sys_clk = 49999500;
> > + break;
> > + case 3:
> > + sys_clk = 66666000;
> > + break;
> > + case 4:
> > + sys_clk = 83332500;
> > + break;
> > + case 5:
> > + sys_clk = 99999000;
> > + break;
> > + case 6:
> > + sys_clk = 133332000;
> > + break;
> > + case 7:
> > + sys_clk = 166665000;
> > + break;
> > + default:
> > + sys_clk = 0;
>
> This default: case is impossible to reach.
>
> > + break;
> > + }
>
> We could save some space by putting this in a table.
>
> > + plat_ratio = (gur->porpllsr) & 0x0000003e;
>
> Unnecessary parens.
>
> > + plat_ratio >>= 1;
>
> plat_ratio /= 2 is more readable and should generate identical code.
>
Reshaped this function(board_init_f) by table-driven method.
Thanks,
Mingkai
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
