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

[Wolfgang Denk]

Dear Mingkai Hu,

In message <1252649951-28543-2-git-send-email-Mingkai.hu at freescale.com> you wrote:
> MPC8536E can support booting from NAND flash which uses the
> image u-boot-nand.bin. This image contains two parts: a 4K
> NAND loader and a main U-Boot image. The former is appended
> to the latter to produce u-boot-nand.bin. The 4K NAND loader
> includes the corresponding nand_spl directory, along with the
> code twisted by CONFIG_NAND_SPL. The main U-Boot image just
> like a general U-Boot image except the parts that included by
> CONFIG_SYS_RAMBOOT.
...
> diff --git a/cpu/mpc85xx/nand_init.c b/cpu/mpc85xx/nand_init.c
> new file mode 100644
> index 0000000..c29b22d
...
> +void cpu_init_early_f(void)
> +{
> +	int i;
> +
> +	/* Pointer is writable since we allocated a register for it */
> +	gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
> +
> +	/* Clear initial global data */
> +	for (i = 0; i < sizeof(gd_t); i++)
> +		((char *)gd)[i] = 0;
> +
> +	set_tlb(0, CONFIG_SYS_CCSRBAR, CONFIG_SYS_CCSRBAR_PHYS,
> +		MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +		1, 0, BOOKE_PAGESZ_4K, 0);
> +
> +	/* 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.

> 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.

...
>  /*
> + * 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.

> 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)start.S:
> +	@rm -f $(obj)start.S
> +	ln -sf $(SRCTREE)/cpu/mpc85xx/start.S $(obj)start.S
> +
> +$(obj)resetvec.S:
> +	@rm -f $(obj)resetvec.S
> +	ln -s $(SRCTREE)/cpu/$(CPU)/resetvec.S $(obj)resetvec.S
> +
> +$(obj)nand_boot_fsl_elbc.c:
> +	@rm -f $(obj)nand_boot_fsl_elbc.c
> +	ln -sf $(SRCTREE)/nand_spl/nand_boot_fsl_elbc.c \
> +	       $(obj)nand_boot_fsl_elbc.c
> +
> +$(obj)ns16550.c:
> +	@rm -f $(obj)ns16550.c
> +	ln -sf $(SRCTREE)/drivers/serial/ns16550.c $(obj)ns16550.c
> +
> +$(obj)nand_init.c:
> +	@rm -f $(obj)nand_init.c
> +	ln -sf $(SRCTREE)/cpu/mpc85xx/nand_init.c $(obj)nand_init.c
> +
> +$(obj)cache.c:
> +	@rm -f $(obj)cache.c
> +	ln -sf $(SRCTREE)/lib_ppc/cache.c $(obj)cache.c
> +
> +$(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) ?

> +#########################################################################
> +
> +$(obj)%.o:	$(obj)%.S
> +	$(CC) $(AFLAGS) -c -o $@ $<
> +
> +$(obj)%.o:	$(obj)%.c
> +	$(CC) $(CFLAGS) -c -o $@ $<
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/nand_spl/board/freescale/mpc8536ds/nand_boot.c b/nand_spl/board/freescale/mpc8536ds/nand_boot.c
> new file mode 100644
> index 0000000..77973d1
> --- /dev/null
> +++ b/nand_spl/board/freescale/mpc8536ds/nand_boot.c
...
> +void board_init_f(ulong bootflag)
> +{
> +	u8 sysclk_ratio;
> +	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));

Please use I/O accessors.

...
> +	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?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Real Programmers always confuse Christmas and Halloween because
OCT 31 == DEC 25 !  - Andrew Rutherford (andrewr at ucs.adelaide.edu.au)