Hi Wolfgang and ML,
On Sat, Dec 4, 2010 at 11:47 PM, Wolfgang Denk <w...@denx.de> wrote:
> Dear Luigi 'Comio' Mantellini,
>
> Not only the commit message, also the remaining text is full of
> typos. Please run through a spell checker.
you are right. I will check better my english. sorry!
>
>> Signed-off-by: Luigi 'Comio' Mantellini <luigi.mantell...@idf-hit.com>
>> ---
>> .gitignore | 25 ++++++-
>> Makefile | 174
>> +++++++++++++++++++++++++++++++++++++++++++-
>
> Please consider if this can be handled separately, without adding tons
> of new code to the top level Makefile.
>
>> diff --git a/.gitignore b/.gitignore
>> index e71f6ac..8db8f0f 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -23,6 +23,12 @@
>> /u-boot.hex
>> /u-boot.map
>> /u-boot.bin
>> +/u-boot.bin.bz2
>> +/u-boot.bin.gz
>> +/u-boot.bin.lzma
>> +/u-boot.bin.lzo
...
>
> Please explain why all these new images are needed?
The u-boot.bin.* images will used as payload. u-boot-bootstrap.bin.*
can be dropped. can A rule u-boot.bin.* be considered safe?
>
>> @@ -39,7 +59,7 @@
>> /LOG
>> /errlog
>> /reloc_off
>> -
>> +/.payload.s
>
.payload.s is an assembler file generated at compile time to include
the payload file (using .incbin directive).
> ???
>
>> +examples/standalone/
>> +
>> +setvars
>
> ???
Sorry... these are wrongs. I will remove.
>
>> +ifeq ($(CONFIG_BOOTSTRAP),y)
...
>> +BOOTSTRAP_LIBS-$(CONFIG_BOOTSTRAP_LZO) += lib/lzo/liblzo.o
>> +BOOTSTRAP_LIBS-$(CONFIG_BOOTSTRAP_XZ) += lib/xz/libxz.o
>> +BOOTSTRAP_LIBS += $(BOOTSTRAP_LIBS-y)
>> +
>> +.PHONY : $(BOOTSTRAP_LIBS)
>> +
>> +BOOTSTRAP_LIBBOARD = board/$(BOARDDIR)/lib$(BOARD)_bootstrap.o
>> +BOOTSTRAP_LIBBOARD := $(addprefix $(obj),$(BOOTSTRAP_LIBBOARD))
>> +endif
>
> Can this not go to a separate, new directory?
I would like to re-factorize again the patch(es) (if you consider this
feature useful...) and I would like to better understand a possible
directory structure.
To cerate bootstrap, I need some (1) bootstrap specific files, some
(2) non specific bootstrap files (like decompression libraries) and
some (3) cut-down files (that in this patch have name like
*_bootstrap.c).
Which is your suggestion to organize these files?
Regarding the (1) bootstrap specific files, like main makefile, the
bootstrap.c file, I will move into a single directory. I have 2
choices: /bootsrap in the rootdir or /lib/bootstrap... I prefer the
second choice. Furthermore I noticed that nand_spl and onenand_ipl
code are placed into the rootdir.
Regarding the (2) non specific code, following the nand_spl and
onenand_ipl examples, I should create sym links to the real code
sources. Honestly, I don't like this approach but if you thinks that
is a good way, I will follow. I will create a ghost u-boot tree into
the bootstrap directory with little-bit different makefiles, in order
to check CONFIG_BOOTSTRAP_* symbols.
Regarding the (3) cut-down fles (for example strat_bootstrap.S), I
will avoid to use directly the u-boot original files, because the code
is similar but not equal and I will introduce a lot of ifdef/endif to
adapt the behaviour. In this case, also following the nand_spl and
onenand_ipl examples, I will create a ghost tree in the /bootstrap
directory for each cpu (even if I will send work for only mips)
The last question is regarding the boards that should add some
bootstrap specific code... where should this code be placed? into the
board/BOARD/ directory or into a directory like
/bootstrap/board/BOARD/?
Sorry for these stupid questions, but I would avoid to iterate a lot
of time on this work... and of course I will spend time only if this
can be considered useful by community (this code is from my job and my
boss asked by already to do other activities...).
>
>> +__BOOTSTRAP_LIBS := $(subst $(obj),,$(BOOTSTRAP_LIBS)) $(subst
>> $(obj),,$(BOOTSTRAP_LIBBOARD))
>
> Lines too long... Please fix globally.
>
>> +$(obj)u-boot.bin.gz: $(obj)u-boot.bin
>> + gzip -c $< > $@
>> +
...
>> +
>> +$(obj)u-boot.bin.bz2: $(obj)u-boot.bin
>> + bzip2 --best -z -c $< > $@
>> +
>
> Do we need this in the top level Makefile?
Create compressed files can be useful. I can move into a different
makefile or create suffix rules like
%.bin.bz2: %.bin
bzip2 --best -z -c $< > $@
>
>> @@ -373,7 +423,7 @@ $(obj)u-boot.dis: $(obj)u-boot
>> GEN_UBOOT = \
..
>> - cd $(LNDIR) && $(LD) $(LDFLAGS) $$UNDEF_SYM $(__OBJS) \
>> + cd $(LNDIR) && $(LD) --gc-sections $(LDFLAGS) $$UNDEF_SYM
>> $(__OBJS) \
..
>
> Be careful!! This will break toins of boards as you did not adapt the
> linker scripts!
Fixed. it was from previous experiments. I removed this from my sources.
>
>> +# Bootstrap targets
>> +
>> +ifeq ($(CONFIG_BOOTSTRAP),y)
>> +$(obj)u-boot-bootstrap.hex: $(obj)u-boot-bootstrap
>> + $(OBJCOPY) ${OBJCFLAGS} -O ihex $< $@
>> +
>> +$(obj)u-boot-bootstrap.srec: $(obj)u-boot-bootstrap
>> + $(OBJCOPY) -O srec $< $@
>> +
>> +$(obj)u-boot-bootstrap.bin: $(obj)u-boot-bootstrap
>> + $(OBJCOPY) ${OBJCFLAGS} -O binary $< $@
>> + $(BOARD_SIZE_CHECK)
>> +
>> +$(obj)u-boot-bootstrap.bin.gz: $(obj)u-boot-bootstrap.bin
>> + gzip -c $< > $@
>> +
>> +$(obj)u-boot-bootstrap.bin.lzma: $(obj)u-boot-bootstrap.bin
>> + lzma -e -z -c $< > $@
>> +
>> +$(obj)u-boot.bin-bootstrap.lzo: $(obj)u-boot-bootstrap.bin
>> + lzop -9 -c $< > $@
>> +
>> +$(obj)u-boot.bin-bootstrap.bz2: $(obj)u-boot-bootstrap.bin
>> + bzip2 --best -z -c $< > $@
>> +
>> +$(obj)u-boot-bootstrap.ldr: $(obj)u-boot-bootstrap
>> + $(CREATE_LDR_ENV)
>> + $(LDR) -T $(CONFIG_BFIN_CPU) -c $@ $< $(LDR_FLAGS)
>> + $(BOARD_SIZE_CHECK)
>> +
>> +$(obj)u-boot-bootstrap.ldr.hex: $(obj)u-boot-bootstrap.ldr
>> + $(OBJCOPY) ${OBJCFLAGS} -O ihex $< $@ -I binary
>> +
>> +$(obj)u-boot-bootstrap.ldr.srec: $(obj)u-boot-bootstrap.ldr
>> + $(OBJCOPY) ${OBJCFLAGS} -O srec $< $@ -I binary
>> +
>> +$(obj)u-boot-bootstrap.img: $(obj)u-boot-bootstrap.bin
>> + $(obj)tools/mkimage -A $(ARCH) -T firmware -C none \
>> + -a $(CONFIG_BOOTSTRAP_BASE) -e 0 \
>> + -n $(shell sed -n -e 's/.*U_BOOT_VERSION//p' $(VERSION_FILE) |
>> \
>> + sed -e 's/"[ ]*$$/ for $(BOARD) board"/') \
>> + -d $< $@
>> +
>> +$(obj)u-boot-bootstrap.imx: $(obj)u-boot-bootstrap.bin
>> + $(obj)tools/mkimage -n $(IMX_CONFIG) -T imximage \
>> + -e $(CONFIG_BOOTSTRAP_BASE) -d $< $@
>> +
>> +$(obj)u-boot-bootstrap.kwb: $(obj)u-boot-bootstrap.bin
>> + $(obj)tools/mkimage -n $(CONFIG_SYS_KWD_CONFIG) -T kwbimage \
>> + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_TEXT_BASE) -d $< $@
>> +
>> +$(obj)u-boot-bootstrap.sha1: $(obj)u-boot-bootstrap.bin
>> + $(obj)tools/ubsha1 $(obj)u-boot-bootstrap.bin
>> +
>> +$(obj)u-boot-bootstrap.dis: $(obj)u-boot-bootstrap
>> + $(OBJDUMP) -d $< > $@
>> +
>> +PAYLOAD_FILE_BASE=$(obj)u-boot.bin
>> +ifeq ($(CONFIG_BOOTSTRAP_GZIP),y)
>> +PAYLOAD_FILE_EXT:=.gz
>> +endif
>> +ifeq ($(CONFIG_BOOTSTRAP_LZMA),y)
>> +PAYLOAD_FILE_EXT:=.lzma
>> +endif
>> +ifeq ($(CONFIG_BOOTSTRAP_LZO),y)
>> +PAYLOAD_FILE_EXT:=.lzo
>> +endif
>> +ifeq ($(CONFIG_BOOTSTRAP_BZIP2),y)
>> +PAYLOAD_FILE_EXT:=.bz2
>> +endif
>> +ifeq ($(CONFIG_BOOTSTRAP_XZ),y)
>> +PAYLOAD_FILE_EXT:=.xz
>> +endif
>> +
>> +PAYLOAD_FILE := $(PAYLOAD_FILE_BASE)$(PAYLOAD_FILE_EXT)
>> +
>> +$(obj).payload.s: $(PAYLOAD_FILE)
>> + echo ".globl payload_start" > $@
>> + echo ".globl payload_end" >> $@
>> + echo ".globl payload_size" >> $@
>> + echo ".globl payload_uncsize" >> $@
>> + echo .section .payload,\"a\",@progbits >> $@
>> + echo "payload_size:" >> $@
>> + echo -n ".word " >> $@
>> + wc -c $(PAYLOAD_FILE) | cut -f1 -d' ' >> $@
>> + echo "payload_uncsize:" >> $@
>> + echo -n ".word " >> $@
>> + wc -c $(obj)u-boot.bin | cut -f1 -d' ' >> $@
>> + echo "payload_start:" >> $@
>> + echo .incbin \"$(PAYLOAD_FILE)\" >> $@
>> + echo "payload_end:" >> $@
>> +
>> +GEN_UBOOT_BOOTSTRAP = \
>> + UNDEF_SYM=`$(OBJDUMP) -x $(BOOTSTRAP_LIBBOARD)
>> $(BOOTSTRAP_LIBS) | \
>> + sed -n -e
>> 's/.*\($(SYM_PREFIX)__u_boot_cmd_.*\)/-u\1/p'|sort|uniq`;\
>> + cd $(LNDIR) && $(LD) --gc-sections $(BOOTSTRAP_LDFLAGS)
>> $$UNDEF_SYM $(obj).payload.o $(__BOOTSTRAP_OBJS) \
>> + --start-group $(__BOOTSTRAP_LIBS) --end-group
>> $(BOOTSTRAP_PLATFORM_LIBS) \
>> + -Map u-boot-bootstrap.map -o u-boot-bootstrap
>> +$(obj)u-boot-bootstrap: depend $(SUBDIRS) $(BOOTSTRAP_OBJS)
>> $(BOOTSTRAP_LIBBOARD) $(BOOTSTRAP_LIBS) $(BOOTSTRAP_LDSCRIPT)
>> $(obj)u-boot-bootstrap.lds $(obj).payload.o
>> + $(GEN_UBOOT_BOOTSTRAP)
>> +ifeq ($(CONFIG_KALLSYMS),y)
>> + smap=`$(call SYSTEM_MAP,u-boot-bootstrap) | \
>> + awk '$$2 ~ /[tTwW]/ {printf $$1 $$3 "\\\\000"}'` ; \
>> + $(CC) $(CFLAGS) -DSYSTEM_MAP="\"$${smap}\"" \
>> + -c common/system_map.c -o $(obj)common/system_map.o
>> + $(GEN_UBOOT_BOOTSTRAP) $(obj)common/system_map.o
>> +endif
>> +
>> +$(BOOTSTRAP_LIBBOARD): depend $(BOOTSTRAP_LIBS)
>> + $(MAKE) -C $(dir $(subst $(obj),,$@)) $(notdir $@)
>> +endif
>
>
> NAK!! Please find a way to implement this without adding all that
> code to the top level Makefile.
ok
>
>> +- Compressed U-Boot support:
>> + CONFIG_BOOTSTRAP_BASE
>> +
>> + This option enables the Boostrap infrastructure.
>> + The bootstrap code consists of a small binary that is able
>> + to decompress an attached payload (a full U-Boot image),
>> + allowing to have a small but also full featured U-Boot
>> + bootloader.
>
> That means you have to make adjustments to the init code of all
> architectures / boards. I do not see this in this patch, nor are
> there any preceeding patches that prepare the ground. In other words,
> this will not work at best, or more likely break zillions of boards.
I can propose code only for mips (that is the platform that use every day).
>
>
> On which architectures / boards has this been tested?
>
>> + BOOTSTRAP_LDSCRIPT
>> +
>> + LD script for bootstrap code. If not defined, a board specific
>> script will be
>> + used.
>
> This is the wrong way around. Only if a board specific script is
> needed there should be need for such a definition; in all other cases
> a default script should be used.
>
>> +ifneq ($(CONFIG_BOOTSTRAP_TEXT_BASE),)
>> +CPPFLAGS += -DCONFIG_BOOTSTRAP_TEXT_BASE=$(CONFIG_BOOTSTRAP_TEXT_BASE)
>> +endif
>
> This should never be necessary.
I added for symmetry respect SYS_TEXT_BASE.
>
>
>> +#ifdef CONFIG_BOOTSTRAP_BZIP2
>> +#define _CONFIG_BOOTSTRAP_RELOCATE
>> +#define _CONFIG_BOOTSTRAP_MALLOC
>> +// #define _CONFIG_BOOTSTRAP_FAKEMALLOC
>> +#endif
>
> C++ comments are not allowed. Please fix globally.
>
C99 :) ok
>> +static const char *algo =
>> +#if defined(CONFIG_BOOTSTRAP_GZIP)
>> + "gzip";
>> +#elif defined(CONFIG_BOOTSTRAP_LZMA)
>> + "lzma";
>> +#elif defined(CONFIG_BOOTSTRAP_LZO)
>> + "lzo";
>> +#elif defined(CONFIG_BOOTSTRAP_BZIP2)
>> + "bzip2";
>> +#elif defined(CONFIG_BOOTSTRAP_XZ)
>> + "xz";
>> +#else
>> + "flat";
>> +#endif
>
> Previous descriptions sounded as if all these formats were supported
> in parallel. Not it seems only one is at a time, and there is a search
> order. This must at least be documented. It would be better if an
> error was raised if two options are selected.
The bootstrap supports just the algo used by the linked payload. I
will add a coherency code to avoid multiple algo definitions
>
>
>> + printf("Uncompressing payload (%s)...", algo);
>> +#if defined(CONFIG_BOOTSTRAP_GZIP)
>> + ret = gunzip(dst, unc_size, src, &size);
>> +#elif defined(CONFIG_BOOTSTRAP_LZMA)
>> + SizeT outsize = unc_size;
>
> What's "SizeT" ?? We don;t allow such names in U-Boot.
>
ok
SizeT is used by lzma library
>> + if (ret) {
>> + printf("failed with error %d.\n", ret);
>> + bootstrap_hang();
>> + } else {
>> + puts("done.\n");
>> + }
>> + return ret;
>
> Indentation by TAB, please!
>
>> \ No newline at end of file
>
> And fix this, too.
ok
>
>> diff --git a/tools/xz_wrap.sh b/tools/xz_wrap.sh
>> new file mode 100755
>> index 0000000..dbe5e55
>> --- /dev/null
>> +++ b/tools/xz_wrap.sh
>> @@ -0,0 +1,45 @@
>> +#!/bin/sh
>> +#
>> +# This is a wrapper for xz to use appropriate compression options depending
>> +# on what is being compressed. The only argument to this script should be
>> +# "kernel" or "misc" to indicate what is being compressed.
>
> What do we use?
>
>> +# Author: Lasse Collin <lasse.col...@tukaani.org>
>> +#
>> +# This file has been put into the public domain.
>> +# You can do whatever you want with this file.
>> +#
>> +
>> +# Defaults: No BCJ filter and no extra LZMA2 options.
>> +BCJ=
>> +LZMA2OPTS=
>> +
>> +# Big dictionary is OK for the kernel image, but it's not OK
>> +# for other things.
>> +#
>> +# BCJ filter is used only for the kernel, at least for now.
>> +# It could be useful for non-trivial initramfs too, but it
>> +# depends on the exact content of the initramfs image.
>> +case $1 in
>> + kernel)
>> + DICT=16MiB
>> + case $ARCH in
>> + x86|x86_64) BCJ=--x86 ;;
>> + powerpc) BCJ=--powerpc ;;
>> + ia64) BCJ=--ia64; LZMA2OPTS=pb=4 ;;
>> + arm) BCJ=--arm ;;
>> + sparc) BCJ=--sparc ;;
>> + esac
>
> Error handling for other architectures?
>
This is from xz embedded module and I just copied into scripts
directory. I will remove xz in the next iteration.
Thanks again for your detailed review.
best regards,
luigi
>
> 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: w...@denx.de
> ACHTUNG!!!
>
> Das machine is nicht fur gefingerpoken und mittengrabben. Ist easy
> schnappen der springenwerk, blowenfusen und corkenpoppen mit spitzen-
> sparken. Ist nicht fur gewerken by das dummkopfen. Das rubbernecken
> sightseeren keepen hands in das pockets. Relaxen und vatch das
> blinkenlights!!!
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
--
Luigi 'Comio' Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantell...@idf-hit.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
