[U-Boot] [PATCH] powerpc/p1022ds: nand: introduce the TPL based on the SPL

[Scott Wood <scottwood@freescale.com>]

On 06/13/2013 02:27:44 AM, Zhang Ying-B40530 wrote:
> 
> 
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, June 13, 2013 6:29 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] powerpc/p1022ds: nand: introduce the TPL based  
> on the SPL
> 
> On 06/09/2013 12:54:43 AM, ying.zhang at freescale.com wrote:
> > From: Ying Zhang <b40530@freescale.com>
> >
> > Due to the nand SPL on the board 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 runs in the  
> L2
> > SRAM,
> > after being loaded by the code from the SPL. It initializes the DDR
> > with
> > the SPD.
> >
> > The TPL's size is sizeable, the maximum size must not exceed the  
> size
> > of L2
> > SRAM. It initializes the DDR through SPD code, and copys final uboot
> > image
> > to DDR. So there are three stage uboot images:
> > 	* spl_boot, 4KB size, pad to 128K byte.
> > 	* tpl_boot, 88K size, pad to 128K size. The env variables are
> > 	copied to L2 SRAM, so that ddr SPD code can get the interleaving
> > 	mode setting in env. It loads final uboot image from offset
> > 256KB.
> > 	* final uboot image, size is variable depends on the functions
> >         enabled.
> >
> > This patch is on top of the patch:
> > powerpc/p1022ds: boot from SD Card with SPL
> >
> > Signed-off-by: Ying Zhang <b40530@freescale.com>
> > ---
> >  Makefile                                           |   25 +++-
> >  README                                             |   55 ++++++-
> >  arch/powerpc/config.mk                             |    2 +
> >  arch/powerpc/cpu/mpc85xx/spl_minimal.c             |   16 ++
> >  arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds            |   80 +++++++++
> >  .../cpu/mpc8xxx/ddr/lc_common_dimm_params.c        |    4 +-
> >  arch/powerpc/lib/Makefile                          |    2 +
> >  board/freescale/p1022ds/Makefile                   |    3 +
> >  board/freescale/p1022ds/spl_minimal.c              |   56 +------
> >  board/freescale/p1022ds/tlb.c                      |    4 +-
> >  board/freescale/p1022ds/tpl.c                      |  101
> > ++++++++++++
> >  common/Makefile                                    |    9 +
> >  common/cmd_nvedit.c                                |    8 +-
> >  config.mk                                          |   32 ++++
> >  doc/README.TPL                                     |   93  
> +++++++++++
> >  drivers/mtd/nand/Makefile                          |    8 +
> >  drivers/mtd/nand/fsl_elbc_tpl.c                    |  168
> > ++++++++++++++++++++
> >  drivers/serial/serial.c                            |    2 +-
> >  include/bootstage.h                                |    3 +-
> >  include/configs/P1022DS.h                          |   75 +++++++--
> >  tpl/Makefile                                       |  161
> > +++++++++++++++++++
> >  21 files changed, 824 insertions(+), 83 deletions(-)
> >  create mode 100644 arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds
> >  create mode 100644 board/freescale/p1022ds/tpl.c
> >  create mode 100644 doc/README.TPL
> >  create mode 100644 drivers/mtd/nand/fsl_elbc_tpl.c
> >  create mode 100644 tpl/Makefile
> >
> > diff --git a/README b/README
> > index 7add6d4..04f9aa5 100644
> > --- a/README
> > +++ b/README
> > @@ -2985,9 +2985,10 @@ FIT uImage format:
> >  		Set for the SPL on PPC mpc8xxx targets, support for
> >  		arch/powerpc/cpu/mpc8xxx/ddr/libddr.o in SPL binary.
> >
> > -		CONFIG_SPL_COMMON_INIT_DDR
> > +		CONFIG_COMMON_INIT_DDR
> >  		Set for common ddr init with serial presence detect in
> > -		SPL binary.
> > +		SPL binary or TPL binary.
> > +
> >  		CONFIG_SYS_NAND_5_ADDR_CYCLE,
> > CONFIG_SYS_NAND_PAGE_COUNT,
> >  		CONFIG_SYS_NAND_PAGE_SIZE, CONFIG_SYS_NAND_OOBSIZE,
> >  		CONFIG_SYS_NAND_BLOCK_SIZE,
> > CONFIG_SYS_NAND_BAD_BLOCK_POS,
> > @@ -3058,6 +3059,56 @@ FIT uImage format:
> >  		option to re-enable it. This will affect the output of
> > the
> >  		bootm command when booting a FIT image.
> >
> > +- TPL framework
> > +		CONFIG_TPL
> > +		Enable building of TPL globally.
> > +
> > +		CONFIG_TPL_LDSCRIPT
> > +		LDSCRIPT for linking the TPL binary.
> > +
> > +		CONFIG_TPL_MAX_SIZE
> > +		Maximum size of the TPL image (text, data, rodata, and
> > +		linker lists sections), BSS excluded.
> > +		When defined, the linker checks that the actual size
> > does
> > +		not exceed it.
> > +
> > +		CONFIG_SPL_TEXT_BASE
> > +		TEXT_BASE for linking the TPL binary.
> > +
> > +		CONFIG_TPL_LIBCOMMON_SUPPORT
> > +		Support for common/libcommon.o in TPL binary
> > +
> > +		CONFIG_TPL_LIBDISK_SUPPORT
> > +		Support for disk/libdisk.o in TPL binary
> > +
> > +		CONFIG_TPL_I2C_SUPPORT
> > +		Support for drivers/i2c/libi2c.o in TPL binary
> > +
> > +		CONFIG_TPL_MPC8XXX_INIT_DDR_SUPPORT
> > +		Set for the TPL on PPC mpc8xxx targets, support for
> > +		arch/powerpc/cpu/mpc8xxx/ddr/libddr.o in TPL binary.
> > +
> > +		CONFIG_TPL_SERIAL_SUPPORT
> > +		Support for drivers/serial/libserial.o in TPL binary
> > +
> > +		CONFIG_TPL_LIBGENERIC_SUPPORT
> > +		Support for lib/libgeneric.o in TPL binary
> > +
> > +		CONFIG_TPL_ENV_SUPPORT
> > +		Support for the environment operating in TPL binary
> > +
> > +		CONFIG_TPL_PAD_TO
> > +		Image offset to which the TPL should be padded before
> > appending
> > +		the TPL payload. By default, this is defined as
> > +		CONFIG_TPL_MAX_SIZE, or 0 if CONFIG_TPL_MAX_SIZE is
> > undefined.
> > +		CONFIG_TPL_PAD_TO must be either 0, meaning to append
> > the TPL
> > +		payload without any padding, or >= CONFIG_TPL_MAX_SIZE.
> > +
> > +		CONFIG_TPL_TARGET
> > +		Final target image containing SPL and payload.  Some
> > TPLs
> > +		use an arch-specific makefile fragment instead, for
> > +		example if more than one image needs to be produced.
> 
> Let's please not just duplicate all the SPL stuff with s/SPL/TPL/.   
> Now
> that the concept of a separate autoconf.mk is apparently less
> controversial than when I suggested it a while back, let's generalize
> it into multiple U-Boot phases, each of which has its own autoconf.mk.
> The same set of symbols would be used to build any phase (ideally
> including the main phase) -- the symbols would just be declared under
> different ifdefs.
> [Zhang Ying]
> I know the concept of a separate autoconf.mk for multiple U-Boot  
> phases.
> If these symbols are only used in the C files, should do as you say.
> But most of these symbols were used in the Makefile to contain the
> Library. The patch "separate autoconf.mk" has not be merged, in this
> case, they can only declared separately.

Treat the autoconf.mk patch as a dependency.  Don't try to hack around  
its absence.

> > diff --git a/arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds
> > b/arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds
> > new file mode 100644
> > index 0000000..344c325
> > --- /dev/null
> > +++ b/arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds
> 
> What is different about this compared to every other 85xx linker  
> script
> that justifies a separate copy?
> [Zhang Ying]
> It was a little different from the u-boot-spl.lds:
> There is no .resetvec in u-boot-tpl.lds and the start address of .text
> is different.
> if share, need to change the file name. It will be used by SPL and  
> TPL.
> I don't know whether someone will put forward different opinions.

The name doesn't need to change.  Consider TPL as being a "second SPL"  
and then the name works fine. :-)

Resetvec can be handled via an ifdef.

> > diff --git a/drivers/mtd/nand/fsl_elbc_tpl.c
> > b/drivers/mtd/nand/fsl_elbc_tpl.c
> > new file mode 100644
> > index 0000000..09868c4
> > --- /dev/null
> > +++ b/drivers/mtd/nand/fsl_elbc_tpl.c
> 
> ...and here you duplicated an entire driver!
> [Zhang Ying]
> There are two purposes to do so:
> 1. As mentioned above that u-boot-tpl.lds, if share, need to change
> the file name.

File naming is not a good reason to duplicate a big chunk of code.

> 2. Make the functionality "nand_load" to non-static, because it will
> be called by the external function to get the environment from the
> nand flash.
> Why use this? Because the common nand drivers need malloc the large
> memory that large than the size of L2 SRAM(256K). There was unable
> to use common nand drivers in the TPL.

Does making nand_load non-static in the main SPL cause size constrants  
to be violated?  If so, the staticness can be determined via an ifdef  
(use ifdef to define something like MAYBE_STATIC -- don't ifdef the  
prototype itself).

-Scott