[U-Boot] [PATCH 11/11] ventana: switch to SPL
Tim Harvey
tharvey at gateworks.com
Thu Apr 24 10:06:24 CEST 2014
On Wed, Apr 23, 2014 at 11:03 AM, Stefano Babic <sbabic at denx.de> wrote:
> Hi Tim,
Hi Stefano,
>
> On 03/04/2014 08:01, Tim Harvey wrote:
>> Switch to an SPL image. The SPL for Ventana does the following:
>> - setup i2c and read the factory programmed EEPROM to obtain DRAM config
>> and model for board-specific calibration data
>> - configure DRAM per CPU/size/layout/devices/calibration
>> - load u-boot.img from NAND and jump to it
>>
>> This allows for a single SPL+u-boot.img to replace the previous multiple board
>> configurations.
>>
>> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
>> ---
>> board/gateworks/gw_ventana/Makefile | 2 +-
>> board/gateworks/gw_ventana/README | 91 +++---
>> board/gateworks/gw_ventana/gw_ventana.c | 5 +-
>> board/gateworks/gw_ventana/gw_ventana.cfg | 15 -
>> board/gateworks/gw_ventana/gw_ventana_spl.c | 394 ++++++++++++++++++++++++++
>> board/gateworks/gw_ventana/gw_ventana_spl.cfg | 29 ++
>> boards.cfg | 6 +-
>> include/config/uboot.release | 1 +
>> include/configs/gw_ventana.h | 13 +-
>> 9 files changed, 500 insertions(+), 56 deletions(-)
>> create mode 100644 board/gateworks/gw_ventana/gw_ventana_spl.c
>> create mode 100644 board/gateworks/gw_ventana/gw_ventana_spl.cfg
>> create mode 100644 include/config/uboot.release
>>
>> diff --git a/board/gateworks/gw_ventana/Makefile b/board/gateworks/gw_ventana/Makefile
>> index e8dab89..8b239ae 100644
>> --- a/board/gateworks/gw_ventana/Makefile
>> +++ b/board/gateworks/gw_ventana/Makefile
>> @@ -6,5 +6,5 @@
>> # SPDX-License-Identifier: GPL-2.0+
>> #
>>
>> -obj-y := gw_ventana.o gsc.o
>> +obj-y := gw_ventana.o gsc.o gw_ventana_spl.o
>
> I think that gw_ventana_spl.o is always built. Should we use instead :
>
> obj-$(CONFIG_SPL_BUILD) += gw_ventana_spl.o
>
yes, you are right. I will change this
>>
>> diff --git a/board/gateworks/gw_ventana/README b/board/gateworks/gw_ventana/README
>> index 9e697d6..c45d4b8 100644
>> --- a/board/gateworks/gw_ventana/README
>> +++ b/board/gateworks/gw_ventana/README
>> @@ -3,53 +3,80 @@ U-Boot for the Gateworks Ventana Product Family boards
>> This file contains information for the port of U-Boot to the Gateworks
>> Ventana Product family boards.
>>
>> -1. Boot source, boot from NAND
>> +1. Secondary Program Loader (SPL)
>> +---------------------------------
>> +
>> +The i.MX6 has a BOOT ROM PPL (Primary Program Loader) which supports loading
>> +an executable image from various boot devices.
>> +
>> +The Gateworks Ventana board config uses an SPL build configuration. This
>> +will build the following artifacts from u-boot source:
>> + - SPL - Secondary Program Loader that the i.MX6 BOOT ROM (Primary Program
>> + Loader) boots. This detects CPU/DRAM configuration, configures
>> + The DRAM controller, loads u-boot.img from the detected boot device,
>> + and jumps to it. As this is booted from the PPL, it has an IVT/DCD
>> + table.
>> + - u-boot.img - The main u-boot core which is u-boot.bin with a image header.
>> +
>> +
>> +2. Build
>> +--------
>> +
>> +To build U-Boot for the Gateworks Ventana product family:
>> +
>> + make gwventana_config
>> + make
>> +
>> +
>> +3. Boot source, boot from NAND
>> ------------------------------
>>
>> The i.MX6 BOOT ROM expects some structures that provide details of NAND layout
>> and bad block information (referred to as 'bootstreams') which are replicated
>> -multiple times in NAND. The number of replications is configurable through
>> -board strapping options and eFUSE settings. The Freescale 'kobs-ng'
>> -application from the Freescale LTIB BSP, which runs under Linux, must be used
>> -to program the bootstream in order to setup the replicated headers correctly.
>> +multiple times in NAND. The number of replications and their spacing (referred
>> +to as search stride) is configurable through board strapping options and/or
>> +eFUSE settings (BOOT_SEARCH_COUNT / Pages in block from BOOT_CFG2). In
>> +addition, the i.MX6 BOOT ROM Flash Configuration Block (FCB) supports two
>> +copies of a bootloader in flash in the case that a bad block has corrupted one. The Freescale 'kobs-ng' application from the Freescale LTIB BSP, which runs
>
> Line too long ?
will fix
>
<snip>
>> --- a/board/gateworks/gw_ventana/gw_ventana.c
>> +++ b/board/gateworks/gw_ventana/gw_ventana.c
>> @@ -1019,8 +1019,9 @@ int board_early_init_f(void)
>>
>> int dram_init(void)
>> {
>> - gd->ram_size = get_ram_size((void *)PHYS_SDRAM,
>> - CONFIG_DDR_MB*1024*1024);
>> + struct mx6_spl_data *data = (struct mx6_spl_data *)
>> + ((CONFIG_SPL_TEXT_BASE - sizeof(struct mx6_spl_data)) & ~0xf);
>
> Ok, you have *data on the stack, you initialize to a magic number and
> then you do nothing with it.
>
>> + gd->ram_size = data->mem_dram_size;
>
> We have already talked about it. A runtime detection is absolutely
> preferable, as suggested by Igor.
Yes, as discussed in the other thread I will be changing this to
runtime detection and dropping the shared struct between SPL and
u-boot.img.
>
>>
>> return 0;
>> }
<snip>
>> diff --git a/board/gateworks/gw_ventana/gw_ventana_spl.c b/board/gateworks/gw_ventana/gw_ventana_spl.c
>> new file mode 100644
>> index 0000000..492c814
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/gw_ventana_spl.c
>> @@ -0,0 +1,394 @@
>> +/*
>> + * Author: Tim Harvey <tharvey at gateworks.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <i2c.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/iomux.h>
>> +#include <asm/arch/mx6_ddr_regs.h>
>> +#include <asm/arch/mx6-ddr.h>
>> +#include <asm/arch/mx6-pins.h>
>> +#include <asm/arch/sys_proto.h>
>> +#include <asm/imx-common/boot_mode.h>
>> +#include <asm/imx-common/iomux-v3.h>
>> +#include <asm/imx-common/mxc_i2c.h>
>> +#ifdef CONFIG_SPL
>> +#include <spl.h>
>> +#endif
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#if defined(CONFIG_SPL_BUILD)
>
> I will drop it from here and conditionally compile this file if
> SPL_BUILD is set.
Agreed - will do this
>
<snip>
>> +static u16
>> +read_eeprom(void)
>> +{
>> + u8 data[3];
>> +
>> + /*
>> + * On a board with a missing/depleted backup battery for GSC, the
>> + * board may be ready to probe the GSC before its firmware is
>> + * running. We will wait here indefinately for the GSC/EEPROM.
>> + */
>> + while (1) {
>> + if (0 == i2c_set_bus_num(I2C_GSC) &&
>> + 0 == i2c_probe(GSC_EEPROM_ADDR))
>> + break;
>> + /* TODO: mdelay hangs at this point
>> + mdelay(1);
>> + */
>> + }
>> +
>> + i2c_read(GSC_EEPROM_ADDR, GSC_EEPROM_DDR_SIZE, 1, data, 3);
>> +
>> + return (data[0] << 8) | data[2];
>> +}
>
> This is a simplified version of the same function in gw_ventana.c. By
> submitting the original patch, you convinced me that due to hardware
> issues the read can fails the first time and you add gsc_i2c_read(). Why
> do we have no problem with i2c in SPL ? It should be even worse.
>
You are absolutely right - I will split out the read_eeprom from
gw_ventana.c and re-use it here as well.
>> +
>> +/* configure mx6 mmdc io registers */
>> +struct mx6_mmdc_ioregs mmdc_ioregs = {
>> + /* DDR3 */
>> + .mmdc_grp_ddr_type = 0x000c0000,
>> + /* disable DDR pullups */
>> + .mmdc_grp_ddrpke = 0x00000000,
>> + /* SDCLK[0:1], CAS, RAS, Reset: Differential input, 40ohm */
>> + .mmdc_dram_sdclk_0 = 0x00020030,
>> + .mmdc_dram_sdclk_1 = 0x00020030,
>> + .mmdc_dram_cas = 0x00020030,
>> + .mmdc_dram_ras = 0x00020030,
>> + .mmdc_dram_reset = 0x00020030,
>> + /* ADDR[00:16], SDBA[0:1]: 40 ohm */
>> + .mmdc_grp_addds = 0x00000030,
>> + /* SDCKE[0:1]: 100k pull-up */
>> + .mmdc_dram_sdcke0 = 0x00003000,
>> + .mmdc_dram_sdcke1 = 0x00003000,
>> + /* SDBA2: pull-up disabled */
>> + .mmdc_dram_sdba2 = 0x00000000,
>> + /* SDODT[0:1]: 100k pull-up, 40 ohm */
>> + .mmdc_dram_sdodt0 = 0x00003030,
>> + .mmdc_dram_sdodt1 = 0x00003030,
>> + /* CS0/CS1/SDBA2/CKE0/CKE1/SDWE: 40 ohm */
>> + .mmdc_grp_ctlds = 0x00000030,
>> + /* SDQS[0:7]: Differential input, 40 ohm */
>> + .mmdc_ddrmode_ctl = 0x00020000,
>> + .mmdc_dram_sdqs0 = 0x00000030,
>> + .mmdc_dram_sdqs1 = 0x00000030,
>> + .mmdc_dram_sdqs2 = 0x00000030,
>> + .mmdc_dram_sdqs3 = 0x00000030,
>> + .mmdc_dram_sdqs4 = 0x00000030,
>> + .mmdc_dram_sdqs5 = 0x00000030,
>> + .mmdc_dram_sdqs6 = 0x00000030,
>> + .mmdc_dram_sdqs7 = 0x00000030,
>> +
>> + /* DATA[00:63]: Differential input, 40 ohm */
>> + .mmdc_grp_ddrmode = 0x00020000,
>> + .mmdc_grp_b0ds = 0x00000030,
>> + .mmdc_grp_b1ds = 0x00000030,
>> + .mmdc_grp_b2ds = 0x00000030,
>> + .mmdc_grp_b3ds = 0x00000030,
>> + .mmdc_grp_b4ds = 0x00000030,
>> + .mmdc_grp_b5ds = 0x00000030,
>> + .mmdc_grp_b6ds = 0x00000030,
>> + .mmdc_grp_b7ds = 0x00000030,
>> +
>> + /* DQM[0:7]: Differential input, 40 ohm */
>> + .mmdc_dram_dqm0 = 0x00020030,
>> + .mmdc_dram_dqm1 = 0x00020030,
>> + .mmdc_dram_dqm2 = 0x00020030,
>> + .mmdc_dram_dqm3 = 0x00020030,
>> + .mmdc_dram_dqm4 = 0x00020030,
>> + .mmdc_dram_dqm5 = 0x00020030,
>> + .mmdc_dram_dqm6 = 0x00020030,
>> + .mmdc_dram_dqm7 = 0x00020030,
>> +};
>
> I will suggest you move these structure in a separate file. It is then
> easier for a board developer to understand what is very board specific.
hmmm... they are all very board specific but ok.
>
<snip>
>> +/*
>> + * called from C runtime startup code (arch/arm/lib/crt0.S:_main)
>> + * - we have a stack and a place to store GD, both in SRAM
>> + * - no variable global data is available
>> + */
>> +void board_init_f(ulong dummy)
>> +{
>> + int ddrcfg;
>> +
>> + /* iomux and setup of i2c */
>> + i2c_setup_iomux();
>> +
>> + /* TODO: possible to get console support at this point for debugging? */
>> + timer_init();
>> + ddrcfg = read_eeprom();
>> + spl_dram_init(ddrcfg);
>> +
>> + arch_cpu_init();
>> +
>> + /* Clear the BSS. */
>> + memset(__bss_start, 0, __bss_end - __bss_start);
>> +
>> + /* Set global data pointer. */
>> + gd = &gdata;
>> +
>> + board_early_init_f();
>> +
>> + preloader_console_init();
>> +
>> + if (is_cpu_type(MXC_CPU_MX6Q))
>> + puts("CPU: IMX6Q\n");
>> + else if (is_cpu_type(MXC_CPU_MX6DL))
>> + puts("CPU: IMX6DL\n");
>> + else
>> + puts("CPU: unsupported\n");
>> + printf("SDRAM: %dMB %dbit\n", (16 << (ddrcfg>>8)),
>> + (8 << (ddrcfg & 0xff)));
>
> Really do we need this in SPL ? Is not enough in U-Boot ?
not needed - I used it mainly for debugging and will remove it.
>
>> +
>> + board_init_r(NULL, 0);
>> +}
>
> Mmhhh...apart the access to the eeprom to get the ram size, this
> function should be common.
maybe, but I think we should wait to see what other boards come up
with SPL support to see what actually ends up being common. An SPL
that supports SPI or some of the other boot devices may need to do
some additional things.
>
>
>> +
>> +/* called from board_init_r() to decide what spl_*_load_image() to call */
>> +u32 spl_boot_device(void)
>> +{
>> + puts("Boot Device: ");
>> + switch (get_boot_device()) {
>> + case MX6_MMC_BOOT:
>> + printf("uSD\n");
>> + return BOOT_DEVICE_MMC1;
>> + case MX6_NAND_BOOT:
>> + printf("NAND\n");
>> + return BOOT_DEVICE_NAND;
>> + case MX6_SATA_BOOT:
>> + printf("SATA\n");
>> + return BOOT_DEVICE_SATA;
>> + default:
>> + printf("UNKNOWN\n");
>> + return BOOT_DEVICE_NONE;
>> + }
>> +}
>
> This function (without unneeded printf) should be made common. Feel free
> to add a spl.c file inside ./arch/arm/imx-common
I'm removing this function and instead using what was
'get_boot_device()' from my other patch for it, which I've moved into
arch/arm/imx-common/misc.c. I'm not sure I see the need to display the
boot device so I'm removing that as well.
>
>> +
>> +#if defined(CONFIG_SPL_MMC_SUPPORT)
>> +/* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
>> +u32 spl_boot_mode(void)
>> +{
>> + switch (get_boot_device()) {
>> + /* for MMC return either RAW or FAT mode */
>> + case BOOT_DEVICE_MMC1:
>> +#ifdef CONFIG_SPL_FAT_SUPPORT
>> + return MMCSD_MODE_FAT;
>> +#else
>> + return MMCSD_MODE_RAW;
>> +#endif
>> + break;
>> + default:
>> + puts("spl: ERROR: unsupported device\n");
>> + hang();
>> + }
>> +}
>> +#endif
>> +
>> +void reset_cpu(ulong addr)
>> +{
>> +}
>> +#endif
>
> Ditto
yes, I'll make this common as well.
>
>
>> +
>> diff --git a/board/gateworks/gw_ventana/gw_ventana_spl.cfg b/board/gateworks/gw_ventana/gw_ventana_spl.cfg
>> new file mode 100644
>> index 0000000..9ab95f5
>> --- /dev/null
>> +++ b/board/gateworks/gw_ventana/gw_ventana_spl.cfg
>> @@ -0,0 +1,29 @@
>> +/*
>> + * Copyright (C) 2013 Gateworks Corporation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + *
>> + * Refer doc/README.imximage for more details about how-to configure
>> + * and create imximage boot image
>> + *
>> + * The syntax is taken as close as possible with the kwbimage
>> + */
>> +
>> +/* image version */
>> +IMAGE_VERSION 2
>> +
>> +/*
>> + * Boot Device : one of
>> + * spi, sd, nand, sata
>> + */
>> +#ifdef CONFIG_SPI_FLASH
>> +BOOT_FROM spi
>> +#else
>> +BOOT_FROM nand
>> +#endif
>> +
>> +#define __ASSEMBLY__
>> +#include <config.h>
>> +#include "asm/arch/iomux.h"
>> +#include "asm/arch/crm_regs.h"
>> +#include "clocks.cfg"
>
> Mmhhh...which is the reason to have gw_ventana.cfg and gw_ventana_spl.cfg ?
>
> You need the .cfg file to generate the IVT header only for SPL. It is
> not required for U-Boot, because the boot ROM has already processed the
> first i.MX header.
>
oops - gw_ventana_spl.cfg is not used so I'll remove it
>
>> diff --git a/boards.cfg b/boards.cfg
>> index a7be5a3..aa48f89 100644
>> --- a/boards.cfg
>> +++ b/boards.cfg
>> @@ -320,11 +320,7 @@ Active arm armv7 mx6 freescale mx6qsabreauto
>> Active arm armv7 mx6 freescale mx6sabresd mx6dlsabresd mx6sabresd:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL Fabio Estevam <fabio.estevam at freescale.com>
>> Active arm armv7 mx6 freescale mx6sabresd mx6qsabresd mx6sabresd:IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg,MX6Q Fabio Estevam <fabio.estevam at freescale.com>
>> Active arm armv7 mx6 freescale mx6slevk mx6slevk mx6slevk:IMX_CONFIG=board/freescale/mx6slevk/imximage.cfg,MX6SL Fabio Estevam <fabio.estevam at freescale.com>
>> -Active arm armv7 mx6 gateworks gw_ventana gwventanadl gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6DL,DDR_MB=512 Tim Harvey <tharvey at gateworks.com>
>> -Active arm armv7 mx6 gateworks gw_ventana gwventanadl1g gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6DL,DDR_MB=1024 Tim Harvey <tharvey at gateworks.com>
>> -Active arm armv7 mx6 gateworks gw_ventana gwventanaq gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6Q,DDR_MB=512 Tim Harvey <tharvey at gateworks.com>
>> -Active arm armv7 mx6 gateworks gw_ventana gwventanaq1g gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6Q,DDR_MB=1024 Tim Harvey <tharvey at gateworks.com>
>> -Active arm armv7 mx6 gateworks gw_ventana gwventanaq1gspi gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6Q,DDR_MB=1024,SPI_FLASH Tim Harvey <tharvey at gateworks.com>
>> +Active arm armv7 mx6 gateworks gw_ventana gwventana gw_ventana:IMX_CONFIG=board/gateworks/gw_ventana/gw_ventana.cfg,MX6QDL,SPL Tim Harvey <tharvey at gateworks.com>
>
> Nice, only one entry for all of your boards.
yes... considering we currently have 4 baseboards (a fifth on the
way), supporting both IMX6DL or IMX6Q on each, and have 2 memory
densities (a third on the way) SPL is a must for my sanity to
eliminate what would be something like 5*2*3 various build-time
configurations.
The dynamic ddr configuration functionality I'm proposing helps out
tremendously as well because the DDR3 calibration and testing I've
done tells me that each board design (varied layout between SoC and
DDR3) needs its own calibration values due to propagation delays and
IMX6Q/D vs IMX6DL/SOLO need different calibration values on each board
as well. This can all be handled with minimal tables and easily
configured at runtime via baseboard detection, cpu detection, and
memory density information.
>
<snip>
>> diff --git a/include/configs/gw_ventana.h b/include/configs/gw_ventana.h
>> index 3398390..f6dc52a 100644
>> --- a/include/configs/gw_ventana.h
>> +++ b/include/configs/gw_ventana.h
>> @@ -7,6 +7,17 @@
>> #ifndef __CONFIG_H
>> #define __CONFIG_H
>>
>> +/* SPL */
>> +#define CONFIG_SPL_NAND_SUPPORT
>> +/*
>> +#define CONFIG_SPL_MMC_SUPPORT
>> +#define CONFIG_SPL_SATA_SUPPORT
>> +#define CONFIG_SPL_FAT_SUPPORT
>> +*/
>> +/* Location in NAND to read U-Boot from */
>> +#define CONFIG_SYS_NAND_U_BOOT_OFFS (14 * 1024 * 1024)
>
> This is ok - it is your decision where to put it.
>
> May I ask why do you need 14 MB at the beginning ? It seems you lose a
> lot of place. NAND is cheap nowadays, but...
it was perhaps a poor decision I made early on when I was following
too much of the reference design without understanding all the
details. They used a 16MB area in NAND for the bootstreams so that is
what I used as well. I worked through the calculation once and the
16MB wasn't all that crazy considering at the time I was wanting
enough room to support a 1MB u-boot. When you add the blocks and
redundant blocks for FCB/DBBT (default is to have 2 copies of these),
then double the bootloader size (because IMX BOOT ROM supports 2
firmware images for redundancy), and factor in 20% headroom to allow
for bad blocks, the 16MB wasn't all that crazy. Now that I'm talking
about using 14MB for a <64KB SPL image, its overkill for sure.
I don't want to impose a partition layout change on our existing users
that I want to update to the SPL bootloader (with all the improved
DDR3 calibration values) so I figure we'll put u-boot.img in the last
2MB of the 16MB partition for the 'bootloader' and leave the first
14MB for SPL to be flashed according to the IMX6 BOOT ROM.
>
>> +
>> +#include "imx6_spl.h" /* common IMX6 SPL configuration */
>> #include "mx6_common.h"
>> #define CONFIG_MX6
>> #define CONFIG_DISPLAY_CPUINFO /* display cpu info */
>> @@ -242,7 +253,7 @@
>> "mtdparts=nor:512k(uboot),64k(env),2m(kernel),-(rootfs)"
>> #else
>> #define MTDIDS_DEFAULT "nand0=nand"
>> -#define MTDPARTS_DEFAULT "mtdparts=nand:16m(uboot),1m(env),-(rootfs)"
>> +#define MTDPARTS_DEFAULT "mtdparts=nand:14m(spl),2m(uboot),1m(env),-(rootfs)"
and I'll be reverting that last change as well and leaving the
original 16M partition for the 'bootloader' meaning the 14MB area
flashed by kobs-ng according to the IMX6 BOOT ROM requirements for
NAND boot as well as the 2MB area I'm reserving for u-boot.img.
If I were to split the partitions like the above change, it will cause
some grief for existing users that are using the 3.0.35 (non
device-tree) vendor kernel that has the mtd partitions hard coded in
the board support. Instead, I decided to patch the kobs-ng application
used to flash the SPL so that it can be passed a max size to use
within /dev/mtd0 and users that need to upgrade to the SPL bootloader
will need to use that patched kobs-ng to flash the SPL to the first
14MB, then use std mtd utils to flash u-boot.img in the area between
14M and 16M.
Don't forget, at some point soon I hope to add some functionality to
u-boot to flash the SPL portion to nand (the way kobs-ng does) so that
you don't need to boot to linux and use kobs-ng or use our JTAG tool.
Thanks for the reviews. I have a few more things to catch up on, but
hope to post a v2 by the end of the week or early next week.
Regards,
Tim
More information about the U-Boot
mailing list