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

[Hu Mingkai-B21284]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Wednesday, September 23, 2009 3:16 AM
> To: Hu Mingkai-B21284
> Cc: u-boot at lists.denx.de; Wood Scott-B07421; Gala Kumar-B11780
> Subject: Re: [U-Boot] [PATCH v3 3/5] NAND boot: MPC8536DS support
> 
> > +	/* set up CCSR if we want it moved */ #if 
> > +(CONFIG_SYS_CCSRBAR_DEFAULT != CONFIG_SYS_CCSRBAR_PHYS)
> > +	{
> > +		volatile u32 *ccsr_virt =
> > +			(volatile u32 *)(CONFIG_SYS_CCSRBAR + 0x1000);
> 
> Please do not declare vaiables righjt in the middle of the code.
> Consider moving this code into a separate function instead.
>
OK.
 
> > diff --git a/cpu/mpc85xx/u-boot-nand.lds 
> b/cpu/mpc85xx/u-boot-nand.lds 
> > index c14e946..a0fc8f1 100644
> > --- a/cpu/mpc85xx/u-boot-nand.lds
> > +++ b/cpu/mpc85xx/u-boot-nand.lds
> > @@ -65,10 +65,8 @@ SECTIONS
> >      PROVIDE (etext = .);
> >      .rodata    :
> >     {
> > -    *(.rodata)
> > -    *(.rodata1)
> > -    *(.rodata.str1.4)
> >      *(.eh_frame)
> > +    *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*)))
> 
> Please rebase your patch against current code.
> 
OK.

> ...
> >  /*
> > + * Config the L2 Cache as L2 SRAM
> > + */
> > +#define CONFIG_SYS_INIT_L2_ADDR		0xf8f80000
> > +#ifdef CONFIG_PHYS_64BIT
> > +#define CONFIG_SYS_INIT_L2_ADDR_PHYS	0xff8f80000ull
> > +#else
> > +#define CONFIG_SYS_INIT_L2_ADDR_PHYS	CONFIG_SYS_INIT_L2_ADDR
> > +#endif
> > +#define CONFIG_SYS_L2_SIZE		(512 << 10)
> > +#define CONFIG_SYS_INIT_L2_END		
> (CONFIG_SYS_INIT_L2_ADDR + CONFIG_SYS_L2_SIZE)
> 
> Line too long, please fix globally.
OK.

> 
> > diff --git a/nand_spl/board/freescale/mpc8536ds/Makefile 
> > b/nand_spl/board/freescale/mpc8536ds/Makefile
> > new file mode 100644
> > index 0000000..c9104d3
> > --- /dev/null
> > +++ b/nand_spl/board/freescale/mpc8536ds/Makefile
> ...
> > +$(obj)tlb.c:
> > +	@rm -f $(obj)tlb.c
> > +	ln -sf $(SRCTREE)/cpu/mpc85xx/tlb.c $(obj)tlb.c
> > +
> > +$(obj)tlb_table.c:
> > +	@rm -f $(obj)tlb_table.c
> > +	ln -sf $(SRCTREE)/board/$(BOARDDIR)/tlb.c $(obj)tlb_table.c
> 
> Consider using the same file name here; it will simplify the 
> Makefile rules.
> 
> > +
> > +$(obj)law.c:
> > +	@rm -f $(obj)law.c
> > +	ln -sf $(SRCTREE)/drivers/misc/fsl_law.c $(obj)law.c
> 
> Ditto.
> 
> > +
> > +$(obj)law_table.c:
> > +	@rm -f $(obj)law_table.c
> > +	ln -sf $(SRCTREE)/board/$(BOARDDIR)/law.c $(obj)law_table.c
> 
> Ditto.
> 
> Please sort list.
> 
> And why do you need a separate rule everywhere? They look all 
> the same to me  (except for the inconsistent file names) ?
> 

Ok, I'll change it. But for some files, the name is same for different
files in the different
directory, for example,  cpu/mpc85xx/tlb.c and
board/freescale/mpc8536/tlb.c, so
I changed the linked name.

> > 
> > +	/* initialize selected port with appropriate baud rate */
> > +	sysclk_ratio = *((volatile unsigned char *)(PIXIS_BASE + 
> > +PIXIS_SPD));
> 
> Please use I/O accessors.
> 
Thanks, I've modified this, and prepared a new version patch.

> ...
> > +	NS16550_init((NS16550_t)(CONFIG_SYS_CCSRBAR + 0x4500),
> > +			bus_clk / 16 / CONFIG_BAUDRATE);
> 
> Smells like I/O accessors should be used here (and further 
> down below), too?
> 
Ditto.

Thanks,
Mingkai