[U-Boot] [PATCH 6/7 v8] NAND: TPL : introduce the TPL based on the SPL

[Scott Wood <scottwood@freescale.com>]

On 07/15/2013 04:36:29 AM, ying.zhang at freescale.com wrote:
> +ifdef CONFIG_TPL
> +$(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin  
> $(obj)spl/u-boot-tpl.bin \
> +		$(obj)u-boot.bin
> +		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_PAD_TO) \
> +			-I binary -O binary \
> +			$(obj)spl/u-boot-spl.bin  
> $(obj)spl/u-boot-spl-pad.bin
> +		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_PAD_TO) \
> +			-I binary -O binary \
> +			$(obj)spl/u-boot-tpl.bin  
> $(obj)spl/u-boot-tpl-pad.bin
> +		cat $(obj)spl/u-boot-spl-pad.bin  
> $(obj)spl/u-boot-tpl-pad.bin \
> +			$(obj)u-boot.bin > $@
> +		rm $(obj)spl/u-boot-spl-pad.bin  
> $(obj)spl/u-boot-tpl-pad.bin
> +else
>  $(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin $(obj)u-boot.bin
>  		$(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_PAD_TO) \
>  			-I binary -O binary $<  
> $(obj)spl/u-boot-spl-pad.bin
>  		cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@
>  		rm $(obj)spl/u-boot-spl-pad.bin
> +endif

Are you sure CONFIG_SPL_PAD_TO will always be the same for both stages?

How about something like:

# $@ is output, $(1) and $(2) are inputs, $(3) is padded intermediate,  
$(4) is pad-to
SPL_PAD_APPEND = \
                 $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(4) -I binary -O  
binary \
                         $(1) $(obj)$(3); \
                 cat $(obj)$(3) $(obj)$(2) > $@; \
                 rm $(obj)$(3)

$(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin  
$(obj)tpl/u-boot-with-tpl.bin
                 $(call  
SPL_PAD_APPEND,$<,u-boot-with-tpl.bin,spl/u-boot-spl-pad.bin,$(CONFIG_SPL_PAD_TO))

$(obj)u-boot-with-tpl.bin: $(obj)tpl/u-boot-tpl.bin $(obj)u-boot.bin
                 $(call  
SPL_PAD_APPEND,$<,u-boot.bin,tpl/u-boot-tpl-pad.bin,$(CONFIG_TPL_PAD_TO))

> @@ -621,7 +635,12 @@ $(obj)u-boot-nand.bin:	nand_spl  
> $(obj)u-boot.bin
>  		cat $(obj)nand_spl/u-boot-spl-16k.bin  
> $(obj)u-boot.bin > $(obj)u-boot-nand.bin
> 
>  $(obj)spl/u-boot-spl.bin:	$(SUBDIR_TOOLS) depend
> -		$(MAKE) -C spl all
> +		$(MAKE) -C spl clean
> +		$(MAKE) -C spl all CONFIG_SPL_BUILD=y
> +
> +$(obj)spl/u-boot-tpl.bin:	$(SUBDIR_TOOLS) depend
> +		$(MAKE) -C spl clean
> +		$(MAKE) -C spl all CONFIG_TPL_BUILD=y CONFIG_SPL_BUILD=y

This will break in a parallel build, among other problems.  Please use  
a separate tpl directory.

> +ifeq ($(CONFIG_TPL_BUILD),y)
> +LDFLAGS_u-boot-tpl += -T $(obj)u-boot-spl.lds $(LDFLAGS_FINAL)
> +ifneq ($(CONFIG_SPL_TEXT_BASE),)
> +LDFLAGS_u-boot-tpl += -Ttext $(CONFIG_SPL_TEXT_BASE)
> +endif
> +else
> +ifeq ($(CONFIG_SPL_BUILD),y)
>  LDFLAGS_u-boot-spl += -T $(obj)u-boot-spl.lds $(LDFLAGS_FINAL)
>  ifneq ($(CONFIG_SPL_TEXT_BASE),)
>  LDFLAGS_u-boot-spl += -Ttext $(CONFIG_SPL_TEXT_BASE)
>  endif
> +endif
> +endif
> 

Why do we need separate LDFLAGS for SPL and TPL?  It doesn't look like  
you define them any differently.  Can you use $(SPL_BIN) (a.k.a.  
$(FILENAME)) here?  Making sure to ifdef CONFIG_SPL_BUILD so that we  
don't define $(LDFLAGS_) in other contexts.

>  # Linus' kernel sanity checking tool
>  CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
> diff --git a/doc/README.TPL b/doc/README.TPL
> new file mode 100644
> index 0000000..3056696
> --- /dev/null
> +++ b/doc/README.TPL
> @@ -0,0 +1,71 @@
> +Generic TPL framework
> +=====================
> +
> +Overview
> +--------
> +
> +TPL---Third Program Loader.
> +
> +Due to the SPL on some boards(powerpc mpc85xx) has a size limit and  
> cannot
> +be compatible with all the external device(e.g. DDR). So add a  
> tertiary
> +program loader (TPL) to enable a loader stub loaded by the code from  
> the
> +SPL. It loads the final uboot image into DDR, then jump to it to  
> begin
> +execution. Now, only the powerpc mpc85xx has this requirement and  
> will
> +implemente it.
> +
> +Keep consistent with SPL, with this framework almost all source  
> files for a
> +board can be reused. No code duplication or symlinking is necessary  
> anymore.
> +
> +How it works
> +------------
> +
> +There has been a directory TOPDIR/spl which contains only a  
> Makefile. It is
> +shared by SPL and TPL. By the way, TPL will share something with  
> SPL, such
> +as options defined in the board config files.
> +
> +The object files are built separately for SPL/TPL and placed in this
> +directory. The final binaries which are generated are  
> u-boot-{spl|tpl},
> +u-boot-{spl|tpl}.bin and u-boot-{spl|tpl}.map.
> +
> +During the TPL build a variable named CONFIG_TPL_BUILD is exported  
> in the
> +make environment and also appended to CPPFLAGS with  
> -DCONFIG_TPL_BUILD.
> +Source files can be compiled for TPL with options choosed in the  
> board
> +config file, based on whether CONFIG_TPL_BUILD is set.
> +
> +For example:
> +
> +drivers/mtd/nand/Makefile:
> +COBJS-$(CONFIG_SPL_NAND_INIT) += nand.o
> +
> +CONFIG_SPL_NAND_INIT is set in the include/configs/P1022DS.h:
> +#ifdef CONFIG_TPL_BUILD
> +#define CONFIG_SPL_NAND_INIT
> +#endif
> +
> +The building of TPL images can be with:
> +
> +#define CONFIG_TPL
> +
> +Because TPL images normally have a different text base, one has to be
> +configured by defining CONFIG_SPL_TEXT_BASE. The linker script has  
> to be
> +defined with CONFIG_SPL_LDSCRIPT. Likewise, these symbols are all  
> shared
> +with SPL, base on whether CONFIG_SPL_BUILD or CONFIG_TPL_BUILD is  
> set.
> +
> +To support generic U-Boot libraries and drivers in the TPL binary  
> one can
> +optionally define CONFIG_SPL_XXX_SUPPORT. Currently following options
> +are supported:
> +
> +CONFIG_SPL_SLIBCOMMON_SUPPORT (common/libcommon.o)
> +CONFIG_SPL_LIBDISK_SUPPORT (disk/libdisk.o)
> +CONFIG_SPL_I2C_SUPPORT (drivers/i2c/libi2c.o)
> +CONFIG_SPL_GPIO_SUPPORT (drivers/gpio/libgpio.o)
> +CONFIG_SPL_MMC_SUPPORT (drivers/mmc/libmmc.o)
> +CONFIG_SPL_SERIAL_SUPPORT (drivers/serial/libserial.o)
> +CONFIG_SPL_SPI_FLASH_SUPPORT (drivers/mtd/spi/libspi_flash.o)
> +CONFIG_SPL_SPI_SUPPORT (drivers/spi/libspi.o)
> +CONFIG_SPL_FAT_SUPPORT (fs/fat/libfat.o)
> +CONFIG_SPL_LIBGENERIC_SUPPORT (lib/libgeneric.o)
> +CONFIG_SPL_POWER_SUPPORT (drivers/power/libpower.o)
> +CONFIG_SPL_NAND_SUPPORT (drivers/mtd/nand/libnand.o)
> +CONFIG_SPL_DMA_SUPPORT (drivers/dma/libdma.o)
> +CONFIG_SPL_POST_MEM_SUPPORT (post/drivers/memory.o)

Please don't duplicate all this.  Only talk about what's different from  
normal SPL.

> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index bb81e84..e1f817a 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -39,6 +39,7 @@ COBJS-$(CONFIG_SPL_NAND_SIMPLE) += nand_spl_simple.o
>  COBJS-$(CONFIG_SPL_NAND_LOAD) += nand_spl_load.o
>  COBJS-$(CONFIG_SPL_NAND_ECC) += nand_ecc.o
>  COBJS-$(CONFIG_SPL_NAND_BASE) += nand_base.o
> +COBJS-$(CONFIG_SPL_NAND_INIT) += nand.o

Thank you for not reorganizing the makefile, but could you explain why  
you need nand.o when none of the other SPLs (even non-minimal) do?  Why  
can't platform code call board_nand_init() directly?  Where is  
nand_init() being called from?

> diff --git a/include/nand.h b/include/nand.h
> index 228d871..2aa7238 100644
> --- a/include/nand.h
> +++ b/include/nand.h
> @@ -153,6 +153,9 @@ int nand_unlock(nand_info_t *meminfo, loff_t  
> start, size_t length,
>  int nand_get_lock_status(nand_info_t *meminfo, loff_t offset);
> 
>  int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst);
> +#ifdef CONFIG_TPL_BUILD
> +int nand_load_image(uint32_t offs, unsigned int uboot_size, void  
> *vdst);
> +#endif
>  void nand_deselect(void);
> 

Don't ifdef prototypes.  Plus, some other platforms may want this in  
other configurations.

> +ifeq ($(CONFIG_SPL_BUILD),y)
>  export CONFIG_SPL_BUILD
> +endif

Why is this conditional?  Remember, we set CONFIG_SPL_BUILD regardless  
of whether we have CONFIG_TPL_BUILD.

In any case, is there any problem exporting variables that aren't  
defined?  Isn't that a no-op (at most, affecting the behavior if the  
variable is defined in the future)?

>  # We want the final binaries in this directory
>  obj := $(OBJTREE)/spl/
> 
> +clean:
> +	@find $(obj) -type f \
> +		\( -name 'core' -o -name '*.bak' -o -name '*~' \
> +		-o -name '*.o'  -o -name '*.a' \) -print \
> +		| xargs rm -f

What does this have to do with TPL?

> +ifndef CONFIG_TPL_BUILD
>  $(OBJTREE)/MLO:	$(obj)u-boot-spl.bin
>  	$(OBJTREE)/tools/mkimage -T omapimage \
>  		-a $(CONFIG_SPL_TEXT_BASE) -d $< $@
> @@ -157,11 +171,12 @@ $(OBJTREE)/MLO:	$(obj)u-boot-spl.bin
>  $(OBJTREE)/MLO.byteswap: $(obj)u-boot-spl.bin
>  	$(OBJTREE)/tools/mkimage -T omapimage -n byteswap \
>  		-a $(CONFIG_SPL_TEXT_BASE) -d $< $@
> +endif

Is the ifndef really needed?

>  $(OBJTREE)/SPL : $(obj)u-boot-spl.bin depend
>  		$(MAKE) -C $(SRCTREE)/arch/arm/imx-common $@
> 
> -ALL-y	+= $(obj)u-boot-spl.bin
> +ALL-y	+= $(obj)$(FILENAME).bin

The makefile deals with many filenames... could you be more specific  
with something like $(SPL_NAME)?

-Scott