[U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

[Wolfgang Denk]

Dear Haiying.Wang at freescale.com,

In message <1296499317-26616-4-git-send-email-Haiying.Wang at freescale.com> you wrote:
> From: Haiying Wang <Haiying.Wang at freescale.com>
> 
> Support P1021MDS board to boot from NAND flash (No NOR flash on this
> board). And because P1021 only has 256K L2 SRAM, which can not used for final
> uboot image, this patch also enables the TPL BOOT on P1021MDS so that DDR can
> be initialized in L2 SRAM through SPD code. So there are three stage uboot
> images:
> * nand_spl, pad from 4KB size to 16KB, load tpl_boot from offset 16KB in NAND.
> * tpl_boot, 112KB size. The env variables are copied to offset 128KB
>   in L2 SRAM, so that ddr spd code can get the interleaving mode setting in env.
>   It loads final uboot image from offset 128KB in NAND.
> * final uboot image, size is variable depends on the functions enabled.


> diff --git a/board/freescale/p1021mds/config.mk b/board/freescale/p1021mds/config.mk
> new file mode 100644
> index 0000000..3888f61
> --- /dev/null
> +++ b/board/freescale/p1021mds/config.mk
...
> +ifndef NAND_SPL
> +ifndef IN_TPL
> +ifeq ($(CONFIG_NAND), y)
> +LDSCRIPT := $(TOPDIR)/$(CPUDIR)/u-boot-nand.lds
> +endif
> +endif
> +endif

Why is this config.mk needed?  Can you not do all this in the board
config file instead?

> diff --git a/board/freescale/p1021mds/ddr.c b/board/freescale/p1021mds/ddr.c
> new file mode 100644
> index 0000000..594a4a8
> --- /dev/null
> +++ b/board/freescale/p1021mds/ddr.c

It seems there are a number of functions here which ar actually shared
with other files, for example board/freescale/p1022ds/ddr.c.

I wonder if it is not possible to use more common code here - especially
given the fact that we already have a nice collection of such files:

	board/freescale/corenet_ds/ddr.c
	board/freescale/mpc8536ds/ddr.c
	board/freescale/mpc8540ads/ddr.c
	board/freescale/mpc8541cds/ddr.c
	board/freescale/mpc8544ds/ddr.c
	board/freescale/mpc8548cds/ddr.c
	board/freescale/mpc8555cds/ddr.c
	board/freescale/mpc8560ads/ddr.c
	board/freescale/mpc8568mds/ddr.c
	board/freescale/mpc8569mds/ddr.c
	board/freescale/mpc8572ds/ddr.c
	board/freescale/mpc8610hpcd/ddr.c
	board/freescale/mpc8641hpcn/ddr.c
	board/freescale/p1022ds/ddr.c
	board/freescale/p1_p2_rdb/ddr.c
	board/freescale/p2020ds/ddr.c
	
> diff --git a/board/freescale/p1021mds/p1021mds.c b/board/freescale/p1021mds/p1021mds.c
> new file mode 100644
> index 0000000..c7a7e57
> --- /dev/null
> +++ b/board/freescale/p1021mds/p1021mds.c
...
> +extern void cpu_mp_lmb_reserve(struct lmb *lmb);

Please move prototypes to header file.

> +void board_lmb_reserve(struct lmb *lmb)
> +{
> +	cpu_mp_lmb_reserve(lmb);
> +}

How many board/freescale/<name>/<name>.c file share this same code?


> diff --git a/board/freescale/p1021mds/tlb.c b/board/freescale/p1021mds/tlb.c
> new file mode 100644
> index 0000000..30af6dd
> --- /dev/null
> +++ b/board/freescale/p1021mds/tlb.c

How much of this is actually different from - say -
board/freescale/p1022ds/tlb.c ?


...
> +/*
> + * Environment Configuration
> + */
> +#define CONFIG_HOSTNAME	p1021mds
> +#define CONFIG_ROOTPATH	/nfsroot
> +#define CONFIG_BOOTFILE	your.uImage

Please rather omit the setting instead of using fillers that are of no
practical value.

> +#define CONFIG_LOADADDR	1000000   /*default location for tftp and bootm*/
> +
> +#define CONFIG_BOOTDELAY 10       /* -1 disables auto-boot */
> +#undef  CONFIG_BOOTARGS           /* the boot command will set bootargs*/
> +
> +#define	CONFIG_EXTRA_ENV_SETTINGS					\
> +	"netdev=eth0\0"							\
> +	"consoledev=ttyS0\0"						\
> +	"ramdiskaddr=2000000\0"						\
> +	"ramdiskfile=your.ramdisk.u-boot\0"				\

Ditto. [BTW: why "....ramdisk.u-boot"? U-Boot does not use ramdisks.
The ramdisk is only used for some OS, so that should probably be
"...ramdisk.linux" instead?]

> +	"fdtaddr=c00000\0"						\
> +	"fdtfile=your.fdt.dtb\0"					\

Ditto. [Are "fdt" and "dtb" not redundant?]

> diff --git a/tpl/board/freescale/p1021mds/tpl_boot.c b/tpl/board/freescale/p1021mds/tpl_boot.c
> new file mode 100644
> index 0000000..386d76c
> --- /dev/null
> +++ b/tpl/board/freescale/p1021mds/tpl_boot.c
...
> +extern void nand_load(unsigned int offs, int uboot_size, uchar *dst);
> +extern phys_size_t init_ddr_dram(void);

Please move prototypes to header files.


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
Any sufficiently advanced bug is indistinguishable from a feature.
                                                      - Rich Kulawiec