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

Scott Wood scottwood at freescale.com
Tue Jul 16 20:06:44 CEST 2013 

On 07/16/2013 05:04:55 AM, Zhang Ying-B40530 wrote:
> 
> 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 16, 2013 7:56 AM
> To: Zhang Ying-B40530
> Cc: u-boot at lists.denx.de; afleming at gmail.com; Xie Xiaobo-R63061;  
> Zhang Ying-B40530
> Subject: Re: [PATCH 6/7 v8] NAND: TPL : introduce the TPL based on  
> the SPL
> 
> 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?
> [Zhang Ying]
> It is not necessarily the same. Same here, because the nand block size
> is 128K. It doesn't matter that is not the same because  
> CONFIG_SPL_PAD_TO
> is defined based on the CONFIG_TPL_BUILD.

That doesn't work here, because we're not in spl/Makefile.  How would  
the value of CONFIG_SPL_PAD_TO change between the rule for

> 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))
> [Zhang Ying]
> According to your advice, how to do for those don't have TPL?

They would use the rule for u-boot-with-spl.bin, and the TPL rule would  
be ignored.  No ifdef needed.

> >  # 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.
> [Zhang Ying]
> Refers to the CONFIG_SPL_*?

Mainly yes, but also to anything that is duplicated really.  In fact,  
TPL should probably just be another paragraph in README.SPL that  
explains the additional mechanism.

> > 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.
> [Zhang Ying]
> Remove ifdef?  If remove, there was error:
> cmd_nand.c:889:12: error: conflicting types for 'nand_load_image'.

Hmm...  In that case, please rename the function when built static.

> The function nand_load_image is defined in two files:
> 1. /common/cmd_nand.c:
> static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand,
>                            ulong offset, ulong addr, char *cmd)
> 2. drivers/mtd/nand/fsl_elbc_spl.c
> #ifndef CONFIG_TPL_BUILD
> static
> #endif
> int nand_load_image(uint32_t offs, unsigned int uboot_size, void  
> *vdst)
> 
> This function only is called by outside in TPL. So, there should  
> ifdef.

You're defining a function called "nand_load_image" ind  
include/nand.h.  There's nothing eLBC-specific about that.  It is  
entirely possible that another implementation will want to export that  
function in an ordinary SPL -- or even possibly in the main U-Boot  
image.

In fact, there's already a common definition for this, which is  
nand_spl_load_image().  Use that.

> > +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?
> [Zhang Ying]
> To be honest, this I don't know. But you said before: there's not  
> even a user of MLO with TPL.

There's no harm in letting the rules sit there unused.  There are a lot  
of other config symbols as well that MLO is not used with -- should we  
ifndef them all? :-)

-Scott

More information about the U-Boot mailing list