Dear haiying.w...@freescale.com,

In message <1296499317-26616-4-git-send-email-haiying.w...@freescale.com> you 
wrote:
> From: Haiying Wang <haiying.w...@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: w...@denx.de
Any sufficiently advanced bug is indistinguishable from a feature.
                                                      - Rich Kulawiec
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to