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

From: Ying Zhang <b40530@freescale.com>

Due to the nand SPL on some board(e.g. P1022DS)has a size limit, it can
not be more than 4K. So, the SPL cannot initialize the DDR with the SPD
code. This patch introduces TPL to enable a loader stub that is loaded
by the code from the SPL. It initializes the DDR with the SPD or other
operations.

The TPL's size is sizeable, the maximum size is decided by the memory's
size that TPL runs. It initializes the DDR through SPD code, and copys
final uboot image to DDR. So there are three stage uboot images:
	* spl_boot, * tpl_boot, * final uboot image

This patch is on top of the patch:
	SPL: Makefile: Build a separate autoconf.mk for SPL

Signed-off-by: Ying Zhang <b40530@freescale.com>
---
Change from v7:
- Modify the doc/README.TPL
- Modify the spl/Makefile.
Change from v6:
- Modify the description of the patch.
- Add the separate the autoconf.mk for TPL.
- Delete the file tpl/Makefile and the directory tpl.
- Reuse the spl/Makefie in TPL.
Change from v5:
- Use ifdef to define "nand_load_image" to non-static for non-SPL.
Change from v4:
- No change.
Change from v3:
- No change.
Change from v2:
- No change.
Change from v1:
- Split from "powerpc/p1022ds: nand: introduce the TPL based on the SPL".

 Makefile                        |   44 +++++++++++++++++++++---
 README                          |    9 +++++
 config.mk                       |   17 +++++++++
 doc/README.TPL                  |   71 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/Makefile       |    1 +
 drivers/mtd/nand/fsl_elbc_spl.c |    5 ++-
 include/nand.h                  |    3 ++
 spl/Makefile                    |   25 +++++++++++---
 8 files changed, 164 insertions(+), 11 deletions(-)
 create mode 100644 doc/README.TPL

-----Original Message-----
From: Wood Scott-B07421 
Sent: Tuesday, July 16, 2013 7:56 AM
To: Zhang Ying-B40530
Cc: u-boot@lists.denx.de; afleming@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@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.

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?


> @@ -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.
[Zhang Ying]
Ok.

> +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.
[Zhang Ying] 
I think best to separate, even if they are the same. I can use $(SPL_BIN) or $(SPL_NAME) to rename it.

>  # 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_*?

> 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?
[Zhang Ying] 
This can be deleted.

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

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.

> +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)?
[Zhang Ying] 
Ok, remove this ifeq.

>  # 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?
[Zhang Ying] 
If share the directory spl, it is necessary. If create the new directory tpl, this is not useful.

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

>  $(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)?
[Zhang Ying] 
OK.

-Scott

-----Original Message-----
From: Wood Scott-B07421 
Sent: Wednesday, July 17, 2013 2:07 AM
To: Zhang Ying-B40530
Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com; Xie Xiaobo-R63061
Subject: Re: [PATCH 6/7 v8] NAND: TPL : introduce the TPL based on the SPL

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@lists.denx.de; afleming@gmail.com; Xie Xiaobo-R63061;  
> Zhang Ying-B40530
> Subject: Re: [PATCH 6/7 v8] NAND: TPL : introduce the TPL based on  
> the SPL
> 

> 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.
[Zhang Ying] 
But, Don't u-boot-with-spl.bin contain tpl/u-boot-with-tpl.bin in the following rule?

$(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))


> > 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.
[Zhang Ying] 
There has a prolem:
nand_spl_simple.c: In function 'nand_init':
nand_spl_simple.c:265:2: error: too many arguments to function 'board_nand_init'

This because the functionality "board_nand_init" is called and parameter is not null, 
but board_nand_init prototype is declared to "void board_nand_init(void)".

In "include/nand.h":

#ifdef CONFIG_SYS_NAND_SELF_INIT
void board_nand_init(void);
int nand_register(int devnum);
#else
extern int board_nand_init(struct nand_chip *nand);
#endif

There need ifdef CONFIG_SYS_NAND_SELF_INIT in nand_init(), it's ok?

#ifdef CONFIG_SYS_NAND_SELF_INIT
        board_nand_init();
#else
        board_nand_init(&nand_chip);
#endif