From: Scott Wood Date: Tue, 16 Jul 2013 13:06:44 -0500 Subject: [U-Boot] [PATCH 6/7 v8] NAND: TPL : introduce the TPL based on the SPL In-Reply-To: (from B40530@freescale.com on Tue Jul 16 05:04:55 2013) Message-ID: <1373998004.8183.333@snotra> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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