[U-Boot] [PATCH 3/7] Add support for SRAM Boot

Mon Aug 16 12:23:56 CEST 2010

Dear Haiying Wang,

In message <1281945897.24612.17.camel at localhost.localdomain> you wrote:
> Once CONFIG_MIDDLE_STAGE_SRAM_BOOT is defined, CONFIG_SRAM_BOOT is enabled to
> generate u-boot-sram.bin which will run in the l2/l3 sram. This middle stage
> uboot will init ddr sdram with ddr spd code and load the final uboot image to
> ddr and start from there. It is useful for the silicons which have small l2/l3
> size under the two scenarios:
> 1. NAND boot. The 4k NAND SPL uboot can not enable the spd ddr code to
> initialize the ddr because of the 4k size limitation, and the l2/l3 as SRAM also
> is not large enough to acoommodate the final uboot image.
> 2. SD/eSPI boot. we don't want to statically init ddr in SD/eSPI's configuration
> part, but l2/l3 as SRAM is small for final uboot.

The concept may be useful for other situations as well, so we should
try and make this as generic as possible.

First, the name CONFIG_MIDDLE_STAGE_SRAM_BOOT is too long and too
specific to your case. Please use a more generic name, for example
CONFIG_SYS_2ND_STAGE_BOOT or similar (I don't think this is a user
configurable option, hence the CONFIG_SYS_)

> This patch has nand boot support, SD/eSPI support will be submited later.
> 
> Because ddr spd code calls some functions defined the files in common/ and lib/,#ifndef CONFIG_SRAM_BOOT is used in those files to keep the sram uboot size as
> small as possible.

Line too long.

> Signed-off-by: Haiying Wang <Haiying.Wang at freescale.com>
> ---
>  Makefile                                           |   18 ++-
>  arch/powerpc/cpu/mpc85xx/cpu_init_nand.c           |   31 +++-
>  arch/powerpc/cpu/mpc85xx/sram_boot/Makefile        |  190 ++++++++++++++++++++
>  arch/powerpc/cpu/mpc85xx/sram_boot/sram_boot.c     |   76 ++++++++
>  .../cpu/mpc85xx/sram_boot/u-boot-sram-boot.lds     |  101 +++++++++++

The code for this should not live in some specific 85xx directory, but
instead be generalized similar to what we have with nand_spl.

...
> --- a/Makefile
> +++ b/Makefile
...
> +$(SRAM_BOOT):	$(TIMESTAMP_FILE) $(VERSION_FILE) $(obj)include/autoconf.mk
> +		$(MAKE) -C $(CPUDIR)/sram_boot all
> +
> +$(U_BOOT_NAND_SRAM): $(NAND_SPL) $(SRAM_BOOT) $(obj)u-boot.bin
> +		cat $(obj)nand_spl/u-boot-spl-16k.bin $(obj)$(CPUDIR)/u-boot-sram.bin $(obj)u-boot.bin > $(obj)u-boot-nand.bin

We really need bette rnames here, too.

...
> diff --git a/arch/powerpc/cpu/mpc85xx/sram_boot/Makefile b/arch/powerpc/cpu/mpc85xx/sram_boot/Makefile
> new file mode 100644
> index 0000000..7c86095
> --- /dev/null
> +++ b/arch/powerpc/cpu/mpc85xx/sram_boot/Makefile
> @@ -0,0 +1,190 @@
> +#
> +# Copyright (C) 2010 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.
> +#
> +
> +SRAM_BOOT := y
> +
> +include $(TOPDIR)/config.mk
> +
> +LDSCRIPT= $(TOPDIR)/$(CPUDIR)/sram_boot/u-boot-sram-boot.lds
> +LDFLAGS	= -Bstatic -T $(LDSCRIPT) -Ttext $(TEXT_BASE) $(PLATFORM_LDFLAGS)
> +AFLAGS	+= -DCONFIG_SRAM_BOOT
> +CFLAGS	+= -DCONFIG_SRAM_BOOT
> +
> +SOBJS	= start.o ticks.o ppcstring.o
> +COBJS	= cache.o cpu_init_early.o cpu_init_nand.o fsl_law.o law.o speed.o \
> +	  sram_boot.o ns16550.o tlb.o tlb_table.o string.o hwconfig.o ddr.o \
> +	  time.o time_lib.o ddr-gen3.o ddr_spd.o ctype.o div64.o console.o \
> +	  cmd_nvedit.o env_common.o env_nand.o vsprintf.o display_options.o

You do not want to duplicate all this stuff here.

This makes no sense.  Also, it is unmaintainable.

> diff --git a/arch/powerpc/cpu/mpc85xx/sram_boot/sram_boot.c b/arch/powerpc/cpu/mpc85xx/sram_boot/sram_boot.c
> new file mode 100644
> index 0000000..7b90eee
> --- /dev/null
> +++ b/arch/powerpc/cpu/mpc85xx/sram_boot/sram_boot.c
...
> +const char *board_hwconfig = "foo:bar=baz";
> +const char *cpu_hwconfig = "foo:bar=baz";

This does not exactly look like useful values to me.

Please fix!

> +void board_init_f(ulong bootflag)
> +{
> +	uint plat_ratio, bus_clk, sys_clk = 0;
> +	ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
...
> +void putc(char c)
> +{
...
> +void puts(const char *str)
> +{

Argh... 

Please do not reimplement everything. This will result in a terribke
mess of unmaintainable code.

> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> index fd5320d..af24491 100644
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
...
> diff --git a/common/console.c b/common/console.c
> index 7e01886..3dfc8e8 100644
> --- a/common/console.c
> +++ b/common/console.c
...
> diff --git a/common/env_common.c b/common/env_common.c
> index 460309b..e04a985 100644
> --- a/common/env_common.c
> +++ b/common/env_common.c
...
> diff --git a/common/env_nand.c b/common/env_nand.c
> index a5e1038..0f7b83c 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
...
> diff --git a/lib/display_options.c b/lib/display_options.c
> index 20319e6..240ad62 100644
> --- a/lib/display_options.c
> +++ b/lib/display_options.c
...
> +#endif /* !CONFIG_SRAM_BOOT */
> diff --git a/lib/string.c b/lib/string.c
> index b375b81..255496a 100644
> --- a/lib/string.c
> +++ b/lib/string.c
...
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index aa214dd..f28ee5b 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c

Full NAK to all these modifications of common code. This is not
acceptable.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If it happens once, it's a bug.
If it happens twice, it's a feature.
If it happens more than twice, it's a design philosophy.