From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Wed, 8 May 2013 19:12:57 -0500 Subject: [U-Boot] [PATCH] powerpc/p1022ds: boot from SD card/SPI flash with SPL In-Reply-To: <1368007463-5760-1-git-send-email-ying.zhang@freescale.com> (from ying.zhang@freescale.com on Wed May 8 05:04:23 2013) Message-ID: <1368058377.3398.62@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 05/08/2013 05:04:23 AM, ying.zhang at freescale.com wrote: > From: Ying Zhang > > 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 > --- Andy Fleming 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 > #include > > +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 > +#include > +#include > +#include > +#include > +#include > +#ifdef CONFIG_SDCARD > +#include > +#include > +#include "../common/sdhc_boot.h" > +#endif > +#ifdef CONFIG_SPIFLASH > +#include > +#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