[U-Boot] [PATCH] powerpc/p1022ds: boot from SD card/SPI flash with SPL
Scott Wood
scottwood at freescale.com
Thu May 9 02:12:57 CEST 2013
On 05/08/2013 05:04:23 AM, ying.zhang at freescale.com wrote:
> From: Ying Zhang <b40530 at 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 at freescale.com>
> ---
Andy Fleming <afleming at 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
More information about the U-Boot
mailing list