> -----Original Message----- > From: Wolfgang Denk [mailto:w...@denx.de] > Sent: Wednesday, September 23, 2009 3:16 AM > To: Hu Mingkai-B21284 > Cc: u-boot@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
