[U-Boot] [PATCH v3 1/3] NAND boot: MPC8536DS support

[Hu Mingkai-B21284]

> -----Original Message-----
> From: Wood Scott-B07421 
> Sent: Saturday, September 19, 2009 12:37 AM
> To: Hu Mingkai-B21284
> Cc: u-boot at lists.denx.de; galak at 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