On Mon, 2010-16-08 at 12:23 +0200, Wolfgang Denk wrote: > Dear Haiying Wang, > > In message <1281945897.24612.17.ca...@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.w...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot