[U-Boot] [PATCH] powerpc/p1022ds: boot from SD card/SPI flash with SPL

[Scott Wood <scottwood@freescale.com>]

On 05/08/2013 05:04:23 AM, ying.zhang at freescale.com wrote:
> From: Ying Zhang <b40530@freescale.com>
> 
> This patch introduces SPL to enable a loader stub that runs in the L2  
> SRAM,
> after being loaded by the code from the internal on-chip ROM. It  
> loads the
> final uboot image into DDR, then jump to it to begin execution.
> 
> The SPL'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 two stage uboot images:
>     * spl_boot, 96KB size. The env variables are copied to offset  
> 96KB.
>         in L2 SRAM, so that ddr spd code can get the interleaving  
> mode setting
>         in env. It loads final uboot image from offset 96KB.
>     * final uboot image, size is variable depends on the functions  
> enabled.
> 
> Signed-off-by: Ying Zhang <b40530@freescale.com>
> ---

Andy Fleming <afleming@gmail.com> is the MMC and mpc85xx maintainer;  
please CC him on these
patches.

This patch needs to be broken into several patches that each take care  
of
one logical problem; it's too hard to properly review (and have the  
right
people pay attention to certain parts) in its current state.

Is this on top of the already-posted P1022DS NAND patch?  If so, please
say so.

> diff --git a/README b/README
> index 862bb3e..f974bca 100644
> --- a/README
> +++ b/README
> @@ -2887,6 +2887,10 @@ FIT uImage format:
>  		CONFIG_SPL_INIT_MINIMAL
>  		Arch init code should be built for a very small image
> 
> +		CONFIG_SPL_INIT_NORMAL
> +		This is relative to MINIMAL, this init code contains  
> some
> +		features that the minimal SPL doesn't contains.

Why do we need a symbol for this?  The only place you use it is already
limited to CONFIG_SPL_BUILD and !CONFIG_SPL_INIT_MINIMAL.

> diff --git a/arch/powerpc/cpu/mpc85xx/tlb.c  
> b/arch/powerpc/cpu/mpc85xx/tlb.c
> index 0dff37f..d21b324 100644
> --- a/arch/powerpc/cpu/mpc85xx/tlb.c
> +++ b/arch/powerpc/cpu/mpc85xx/tlb.c
> @@ -55,7 +55,7 @@ void init_tlbs(void)
>  	return ;
>  }
> 
> -#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD)
> +#if !defined(CONFIG_NAND_SPL) && !defined(CONFIG_SPL_BUILD_MINIMAL)
>  void read_tlbcam_entry(int idx, u32 *valid, u32 *tsize, unsigned  
> long *epn,
>  		       phys_addr_t *rpn)
>  {

Aren't you breaking the existing minimal targets?
CONFIG_SPL_BUILD_MINIMAL does not currently exist, and you add it only
for P1022DS.

> diff --git a/arch/powerpc/cpu/mpc85xx/u-boot-spl.lds  
> b/arch/powerpc/cpu/mpc85xx/u-boot-spl.lds
> index f2b7bff..f1a9ac9 100644
> --- a/arch/powerpc/cpu/mpc85xx/u-boot-spl.lds
> +++ b/arch/powerpc/cpu/mpc85xx/u-boot-spl.lds
> @@ -26,6 +26,13 @@
>  #include "config.h"	/* CONFIG_BOARDDIR */
> 
>  OUTPUT_ARCH(powerpc)
> +#ifdef CONFIG_SYS_MPC85XX_NO_RESETVEC
> +PHDRS
> +{
> +  text PT_LOAD;
> +  bss PT_LOAD;
> +}
> +#endif

Whitespace.

> @@ -68,9 +80,21 @@ SECTIONS
>  #else
>  #error unknown NAND controller
>  #endif
> +
> +#ifndef CONFIG_FSL_IFC
> +#ifdef CONFIG_SYS_MPC85XX_NO_RESETVEC
> +	.bootpg ADDR(.text) - 0x1000 :
> +	{
> +		KEEP(*(.bootpg))
> +	} :text = 0xffff
> +#endif
> +#endif
> +
> +#ifndef CONFIG_SYS_MPC85XX_NO_RESETVEC
>  	.resetvec ADDR(.text) + RESET_VECTOR_OFFSET : {
>  		KEEP(*(.resetvec))
>  	} = 0xffff
> +#endif

Get rid of the IFC ifdef -- this should apply equally well to IFC.

Can you make the "no resetvec" stuff a separate patch?

> @@ -83,5 +107,6 @@ SECTIONS
>  		*(.sbss*)
>  		*(.bss*)
>  	}
> +	. = ALIGN(4);
>  	__bss_end = .;
>  }

This seems unrelated.

> diff --git a/board/freescale/common/sdhc_boot.c  
> b/board/freescale/common/sdhc_boot.c
> index e432318..96b0680 100644
> --- a/board/freescale/common/sdhc_boot.c
> +++ b/board/freescale/common/sdhc_boot.c
> @@ -24,6 +24,7 @@
>  #include <mmc.h>
>  #include <malloc.h>
> 
> +DECLARE_GLOBAL_DATA_PTR;
>  /*
>   * The environment variables are written to just after the u-boot  
> image
>   * on SDCard, so we must read the MBR to get the start address and  
> code
> @@ -31,6 +32,9 @@
>   */
>  #define ESDHC_BOOT_IMAGE_SIZE	0x48
>  #define ESDHC_BOOT_IMAGE_ADDR	0x50
> +#define ESDHC_BOOT_IMAGE_SIGN   0x55AA
> +#define ESDHC_BOOT_IMAGE_SIGN_ADDR      0x1FE
> +#define CONFIG_CFG_DATA_SECTOR  0
> 
>  int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
>  {
> @@ -61,3 +65,122 @@ int mmc_get_env_addr(struct mmc *mmc, u32  
> *env_addr)
> 
>  	return 0;
>  }
> +
> +#ifdef CONFIG_SPL_BUILD
> +void mmc_get_env(void)
> +{
> +	/* load environment */
> +	struct mmc *mmc;
> +	int err;
> +	u32 offset;
> +	uint blk_start, blk_cnt, ret;
> +
> +	mmc_initialize(gd->bd);
> +	/* We register only one device. So, the dev id is always 0 */
> +	mmc = find_mmc_device(0);
> +	if (!mmc) {
> +		puts("spl: mmc device not found!!\n");
> +		hang();
> +	}
> +	err = mmc_init(mmc);
> +	if (err) {
> +		puts("spl: mmc init failed!");
> +		hang();
> +	}
> +	if (1 == mmc_get_env_addr(mmc, &offset))
> +		puts("spl: mmc get env error!!\n");

Constants go on the right hand side of comparisons.

> +	val = *(u16 *)(tmp_buf + ESDHC_BOOT_IMAGE_SIGN_ADDR);
> +	if ((u16)ESDHC_BOOT_IMAGE_SIGN != val) {
> +		free(tmp_buf);
> +		return;
> +	}

Why do you need this cast?

> +	offset = *(u32 *)(tmp_buf + ESDHC_BOOT_IMAGE_ADDR);
> +	offset += CONFIG_SYS_MMC_U_BOOT_OFFS;
> +	/* Get the code size from offset 0x48 */
> +	code_len = *(u32 *)(tmp_buf + ESDHC_BOOT_IMAGE_SIZE);
> +	code_len -= CONFIG_SYS_MMC_U_BOOT_OFFS;
> +	/*
> +	 * Load U-Boot image from mmc into RAM
> +	*/
> +	/*
> +	SDHC card: code offset and length is stored in block units  
> rather
> +	* than a single byte
> +	*/

	/*
	 * U-Boot multiline
	 * comment style is
	 * like this.
	 */

> +	blk_start = ALIGN(offset, mmc->read_bl_len) / mmc->read_bl_len;
> +	blk_cnt = ALIGN(code_len, mmc->read_bl_len) / mmc->read_bl_len;
> +
> +	err = mmc->block_dev.block_read(0, blk_start, blk_cnt,
> +					(uchar  
> *)CONFIG_SYS_MMC_U_BOOT_DST);
> +	if (err != blk_cnt) {
> +		free(tmp_buf);
> +		return ;
> +	}
> +}
> +void mmc_boot(void)

No space before ;

That return is pointless since you're at the end of the function anyway.

Please put a blank line between functions.

> diff --git a/board/freescale/common/sdhc_boot.h  
> b/board/freescale/common/sdhc_boot.h
> new file mode 100644
> index 0000000..2b5dcb9
> --- /dev/null
> +++ b/board/freescale/common/sdhc_boot.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef __SDHC_BOOT_H_
> +#define __SDHC_BOOT_H_    1
> +
> +
> +int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr);
> +void mmc_get_env(void);
> +void mmc_boot(void);
> +
> +#endif  /* __SDHC_BOOT_H_ */

Does this stuff really belong in board/freescale?  Should probably at
least be in arch/powerpc/cpu/mpc85xx, if not more generic.

> +#include <common.h>
> +#include <ns16550.h>
> +#include <nand.h>
> +#include <asm/fsl_law.h>
> +#include <asm/fsl_ddr_sdram.h>
> +#include <malloc.h>
> +#ifdef CONFIG_SDCARD
> +#include <mmc.h>
> +#include <i2c.h>
> +#include "../common/sdhc_boot.h"
> +#endif
> +#ifdef CONFIG_SPIFLASH
> +#include <i2c.h>
> +#include "../common/ngpixis.h"
> +#include "../common/spi_boot.h"
> +#endif

Don't ifdef includes.

Don't use boards.cfg-defined config symbols outside the board config
header, unless they're proper U-Boot config symbols which requires  
better
naming, and documentation in README.

> +void hang(void)
> +{
> +	puts("### ERROR ### Please RESET the board ###\n");
> +	for (;;)
> +		;
> +}

Whitespace

> +ulong get_effective_memsize(void)
> +{
> +	return CONFIG_SYS_L2_SIZE;
> +}
> +
> +void board_init_f(ulong bootflag)
> +{
> +	int px_spd;
> +	u32 plat_ratio, sys_clk, bus_clk;
> +	ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
> +
> +	console_init_f();
> +
> +#if defined(CONFIG_SDCARD) || defined(CONFIG_SPIFLASH)
> +	/* Set pmuxcr to allow both i2c1 and i2c2 */
> +	setbits_be32(&gur->pmuxcr, in_be32(&gur->pmuxcr) | 0x1000);
> +	setbits_be32(&gur->pmuxcr,
> +		in_be32(&gur->pmuxcr) | MPC85xx_PMUXCR_SD_DATA);
> +
> +#ifdef CONFIG_SPIFLASH
> +	/* Enable the SPI */
> +	clrsetbits_8(&pixis->brdcfg0, PIXIS_ELBC_SPI_MASK, PIXIS_SPI);
> +#endif
> +	/* Read back the register to synchronize the write. */
> +	in_be32(&gur->pmuxcr);
> +#endif
> +
> +	/* initialize selected port with appropriate baud rate */
> +	px_spd = in_8((unsigned char *)(PIXIS_BASE + PIXIS_SPD));
> +	sys_clk = sysclk_tbl[px_spd & PIXIS_SPD_SYSCLK_MASK];
> +	plat_ratio = in_be32(&gur->porpllsr) &  
> MPC85xx_PORPLLSR_PLAT_RATIO;
> +	bus_clk = sys_clk * plat_ratio / 2;
> +
> +	NS16550_init((NS16550_t)CONFIG_SYS_NS16550_COM1,
> +			bus_clk / 16 / CONFIG_BAUDRATE);
> +#ifdef CONFIG_SDCARD
> +	puts("\nSD boot...\n");
> +#endif
> +#ifdef CONFIG_NAND
> +	puts("\nNAND boot...\n");
> +#endif
> +#ifdef CONFIG_SPIFLASH
> +	puts("\nSPI Flash boot...\n");
> +#endif

Is NAND handled by this file?  Isn't this on top of the patch that
already adds NAND boot support?  I don't see this print being removed
from somewhere else.

> diff --git a/common/Makefile b/common/Makefile
> index 0e0fff1..bc80414 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -225,6 +225,11 @@ COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_common.o
>  COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_flags.o
>  COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_nowhere.o
>  COBJS-$(CONFIG_SPL_NET_SUPPORT) += miiphyutil.o
> +COBJS-$(CONFIG_SPL_HWCONFIG_SUPPORT) += hwconfig.o
> +COBJS-$(CONFIG_SPL_INIT_DDR_SUPPORT) += ddr_spd.o
> +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_attr.o
> +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_flags.o
> +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_callback.o

CONFIG_SPL_ENV_SUPPORT should replace CONFIG_SPL_NET_SUPPORT here (and
add it to the boards that already have CONFIG_SPL_NET_SUPPORT).  This
sort of refactoring needs to be a separate patch, BTW.

Can ddr_spd.o and hwconfig just use their normal CONFIG symbols (i.e.
move the existing makefile line out of the !SPL ifdef)?  It's getting a
bit excessive with all the new SPL symbols.

Maybe one day we can redo it to be a separate config namespace, like I
suggested a while ago. :-P  It would certainly help with three-stage  
boot
support.

>  endif
>  COBJS-$(CONFIG_BOUNCE_BUFFER) += bouncebuf.o
>  COBJS-y += console.o
> diff --git a/doc/README.mpc85xx-sd-spi-boot  
> b/doc/README.mpc85xx-sd-spi-boot
> new file mode 100644
> index 0000000..79f91fc
> --- /dev/null
> +++ b/doc/README.mpc85xx-sd-spi-boot
> @@ -0,0 +1,82 @@
> +----------------------------------------
> +Booting from On-Chip ROM (eSDHC or eSPI)
> +----------------------------------------
> +
> +boot_format is a tool to write SD bootable images to a filesystem  
> and build
> +SD/SPI images to a binary file for writing later.
> +
> +When booting from an SD card/MMC, boot_format puts the configuration  
> file and
> +the RAM-based U-Boot image on the card.
> +When booting from an EEPROM, boot_format generates a binary image  
> that is used
> +to boot from this EEPROM.
> +
> +Where to get boot_format:
> +========================
> +
> +you can browse it online at:
> +http://git.freescale.com/git/cgit.cgi/ppc/sdk/boot-format.git/
> +
> +Building
> +========
> +
> +Run the following to build this project
> +
> +	$ make
> +
> +Execution
> +=========
> +
> +boot_format runs under a regular Linux machine and requires a super  
> user mode
> +to run. Execute boot_format as follows.
> +
> +For building SD images by writing directly to a file system on SD  
> media:
> +
> +	$ boot_format $config u-boot.bin -sd $device
> +
> +Where $config is the included config.dat file for your platform and  
> $device
> +is the target block device for the SD media on your computer.
> +
> +For build binary images directly a local file:
> +
> +	$ boot_format $config u-boot.bin -spi $file
> +
> +Where $file is the target file. Also keep in mind the u-boot.bin  
> file needs
> +to be the u-boot built for your particular platform and target media.
> +
> +Hint: To generate a u-boot.bin for a P1022DS booting from SD I would  
> run the
> +following in the u-boot repository:
> +
> +	$ make P1022DS_SDCARD

s/Hint/Example/ and s/from SD I would run/from SD, run/

> diff --git a/lib/Makefile b/lib/Makefile
> index e901cc7..ab12e63 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -66,6 +66,11 @@ endif
>  COBJS-$(CONFIG_SPL_NET_SUPPORT) += errno.o
>  COBJS-$(CONFIG_SPL_NET_SUPPORT) += hashtable.o
>  COBJS-$(CONFIG_SPL_NET_SUPPORT) += net_utils.o
> +COBJS-$(CONFIG_SPL_ADDR_MAP_SUPPORT) += addr_map.o


> +COBJS-$(CONFIG_SPL_UTILITYLIB_SUPPORT) += hashtable.o
> +COBJS-$(CONFIG_SPL_UTILITYLIB_SUPPORT) += display_options.o
> +COBJS-$(CONFIG_SPL_UTILITYLIB_SUPPORT) += errno.o

As with common/Makefile, Can these just be the normal makefile lines,
moved outside the SPL ifdef (and no duplications with
CONFIG_SPL_NET_SUPPORT)?

> diff --git a/spl/Makefile b/spl/Makefile
> index b5a8de7..3a3b868 100644
> --- a/spl/Makefile
> +++ b/spl/Makefile
> @@ -51,6 +51,9 @@ LIBS-y += arch/powerpc/cpu/mpc8xxx/lib8xxx.o
>  endif
>  ifeq ($(CPU),mpc85xx)
>  LIBS-y += arch/powerpc/cpu/mpc8xxx/lib8xxx.o
> +ifdef CONFIG_SPL_INIT_DDR_SUPPORT
> +LIBS-y += arch/powerpc/cpu/mpc8xxx/ddr/libddr.o
> +endif

Why isn't this handled as part of lib8xxx.o?  We should avoid putting
hardware-specific things in generic Makefiles.  There ones that are
already there should be fixed at some point.

-Scott