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

Tue Aug 17 07:46:51 CEST 2010

On Mon, 2010-16-08 at 12:23 +0200, Wolfgang Denk wrote:
> 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_)
OK. will rename it.

> > 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.
will fix it.

> > 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.
Should we let it structured as $(TOPDIR)/sram_boot/board/freescale....?
At least current, the above code is mostly only used for 85xx. The only
common part I can tell is the changes in Makefile. 

> ...
> > --- 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.
Does SRAM_BOOT/sram_boot sound bad? :)

> ...
> > 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.
For this case, I need to call some functions like getenv, hwconfig,
printf, strcmp etc. which are needed in ddr spd code, but I don't want
to link the libs for those file because if so, the 2nd stage uboot will
be larger. It might also not be a good idea to copy all those functions
into some new files which are really duplicate. I agree it is
unmaintainable here. As Scott pointed, we need to find a better way. Any
suggestion?

> 
> > 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.
The only use is to make board_hwconfig and cpu_hwconfig from sbss to sdata section.

> Please fix!
The problem here is: 
The commit 79e4e6480b359cb28129cecfa2cae0ef9cccf803:
     "powerpc/8xxx: Enabled hwconfig for memory interleaving"
makes changes as:
-       if ((p = getenv("memctl_intlv_ctl")) != NULL) {
+       if (hwconfig_sub("fsl_ddr", "ctlr_intlv")) {

Thus the hwconfig is called before ddr initialized, and the system hangs
at "
         if (board_hwconfig)
                return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
                                      opt, ";", ':', arglen);
" in common/hwconfig.c.
I did not get it yet, but just copied above code from mpc8641hpcn.c to
make the system boot up. 

> > +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.
Sorry, will fix it.

> 
> > 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.
> 
Again, if those are not acceptable, do you have any suggestion on how to
pick the code for the functions I need to use in sram boot?

Thanks.

Haiying