Patchwork [U-Boot,3/4] nand_spl/spiboot/sdboot: Add ability to have smaller middle loader

login
register
mail settings
Submitter y@right.am.freescale.net
Date June 2, 2011, 7:27 p.m.
Message ID <1307042852-10385-3-git-send-email-y>
Download mbox | patch
Permalink /patch/98469/
State Superseded
Headers show

Comments

y@right.am.freescale.net - June 2, 2011, 7:27 p.m.
From: Matthew McClintock <msm@freescale.com>

This patch allows some boards do define CONFIG_BOOTSTRAP to let them
build a MPL or middle program loader to setup the board before booting
to the full u-boot build. The advantage is that we are using the same
build system and linker scripts that would be used in a normal u-boot
build.

This is used on powerpc/85xx parts that only have 256kB of cache
and also need to perform DDR SPD. So, the nand_spl will load the
MPL to L2 SRAM and there DDR will be configured. Afterwards the
MPL uses the CONFIG_BOOTCOMMAND environment variable to boot to
the appropriate media. This also works with SD and SPI via the
on chip rom but instead it goes from on-chip-rom to MPL to full
version of u-boot.

Depends on previous icache/dcache command changes and various
build fixes for add cases that occur when trying to build an
extremely minimal image

Signed-off-by: Matthew McClintock <msm@freescale.com>
---
 Makefile |   38 +++++++++++++++++++++++++++++++++++++-
 mkconfig |    1 +
 2 files changed, 38 insertions(+), 1 deletions(-)
Wolfgang Denk - June 4, 2011, 12:32 p.m.
Dear y@right.am.freescale.net,

In message <1307042852-10385-3-git-send-email-y> you wrote:
> 
> This patch allows some boards do define CONFIG_BOOTSTRAP to let them
> build a MPL or middle program loader to setup the board before booting
> to the full u-boot build. The advantage is that we are using the same
> build system and linker scripts that would be used in a normal u-boot
> build.

The intention is OK, but I disagree with the implementation.

> +ifndef CONFIG_IN_BOOTSTRAP
> +ifeq ($(CONFIG_SPIFLASH), y)
> +ALL += $(obj)u-boot-spi.bin
> +endif
> +
> +ifeq ($(CONFIG_SDCARD), y)
> +ALL += $(obj)u-boot-sd.bin
> +endif
> +endif

I really dislike to have this in the top level Makefile. We want to
make it simpler, less board specific. and this is the opposite.

I doubt you always need all these images.  If you need u-boot-sd.bin,
then just type "make u-boot-sd.bin".

>  nand_spl:	$(TIMESTAMP_FILE) $(VERSION_FILE) depend
>  		$(MAKE) -C nand_spl/board/$(BOARDDIR) all
>  
> +NAND_SPL_OBJS-y += $(obj)nand_spl/u-boot-spl-16k.bin
> +NAND_SPL_OBJS-$(CONFIG_BOOTSTRAP) += $(obj)u-boot-bootstrap.bin
> +NAND_SPL_OBJS-y += $(obj)u-boot.bin

This looks pretty much board specific to me.  I doubt all boards can
use u-boot-spl-16k.bin here.

> +ifeq ($(CONFIG_BOOTSTRAP),y)
> +$(obj)u-boot-nand.bin:	nand_spl $(obj)u-boot-bootstrap.bin $(obj)u-boot.bin
> +else
...
> +$(obj)u-boot-bootstrap.bin:
> +		rm -rf $(BUILD_DIR)bootstrap
> +		$(MAKE) $(CONFIG_BOARD_NAME) O=bootstrap/ EXTRA_OPTS=IN_BOOTSTRAP
> +		cp $(BUILD_DIR)bootstrap/u-boot.bin u-boot-bootstrap.bin

Argh.. how many different images are you going to add?  This is
becoming a mess.

Do we really need all this in the top level Makefile?

> diff --git a/mkconfig b/mkconfig
> index e72ec3d..74294ec 100755
> --- a/mkconfig
> +++ b/mkconfig
> @@ -159,6 +159,7 @@ for i in ${TARGETS} ; do
>  done
>  
>  cat << EOF >> config.h
> +#define CONFIG_BOARD_NAME $BOARD_NAME
>  #define CONFIG_BOARDDIR board/$BOARDDIR
>  #include <config_cmd_defaults.h>
>  #include <config_defaults.h>

NAK. This breaks existing code, also it should not be needed.

Best regards,

Wolfgang Denk
McClintock Matthew-B29882 - June 7, 2011, 7:35 p.m.
On Sat, Jun 4, 2011 at 7:32 AM, Wolfgang Denk <wd@denx.de> wrote:
>> +ifndef CONFIG_IN_BOOTSTRAP
>> +ifeq ($(CONFIG_SPIFLASH), y)
>> +ALL += $(obj)u-boot-spi.bin
>> +endif
>> +
>> +ifeq ($(CONFIG_SDCARD), y)
>> +ALL += $(obj)u-boot-sd.bin
>> +endif
>> +endif
>
> I really dislike to have this in the top level Makefile. We want to
> make it simpler, less board specific. and this is the opposite.

Perhaps having a CONFIG_TARGET_IMAGE available and having just one
generic TARGET available?

ifdef CONFIG_TARGET_IMAGE
ALL += $(CONFIG_TARGET_IMAGE)
endif

TARGET_IMAGE_OBJS-y += various.bin
TARGET_IMAGE_OBJS-y += required.bin
TARGET_IMAGE_OBJS-y += blobs.bin

$(obj)$(CONFIG_TARGET_IMAGE):   $(TARGET_IMAGE_OBJS-y)
                cat $(TARGET_IMAGE_OBJS-y) > $(obj)$(CONFIG_TARGET_IMAGE)

> I doubt you always need all these images.  If you need u-boot-sd.bin,
> then just type "make u-boot-sd.bin".

We choose these targets by adding a config line in boards.cfg. We
don't build them unless we do a make BOARD_{NAND,SDCARD,SPIFLASH}. We
could optionally just require the user to type out the target manually
or do what I mentioned above however, I strongly believe these images
should always be built as we don't want end user confusion about which
image to program.

>>  nand_spl:    $(TIMESTAMP_FILE) $(VERSION_FILE) depend
>>               $(MAKE) -C nand_spl/board/$(BOARDDIR) all
>>
>> +NAND_SPL_OBJS-y += $(obj)nand_spl/u-boot-spl-16k.bin
>> +NAND_SPL_OBJS-$(CONFIG_BOOTSTRAP) += $(obj)u-boot-bootstrap.bin
>> +NAND_SPL_OBJS-y += $(obj)u-boot.bin
>
> This looks pretty much board specific to me.  I doubt all boards can
> use u-boot-spl-16k.bin here.

This u-boot-spl-16k.bin reference has been here before this patch. It
is typically just the u-boot-nand image to be used and not actually
required to be 16k. It could probably be updated to just be
u-boot-spl.bin except that for some boards this is not the right
image. Usually the 16k one points at the right image.

>
>> +ifeq ($(CONFIG_BOOTSTRAP),y)
>> +$(obj)u-boot-nand.bin:       nand_spl $(obj)u-boot-bootstrap.bin $(obj)u-boot.bin
>> +else
> ...
>> +$(obj)u-boot-bootstrap.bin:
>> +             rm -rf $(BUILD_DIR)bootstrap
>> +             $(MAKE) $(CONFIG_BOARD_NAME) O=bootstrap/ EXTRA_OPTS=IN_BOOTSTRAP
>> +             cp $(BUILD_DIR)bootstrap/u-boot.bin u-boot-bootstrap.bin
>
> Argh.. how many different images are you going to add?  This is
> becoming a mess.
>
> Do we really need all this in the top level Makefile?

I'm not sure there is a great way around this. I played around with
trying to include them in the config.mk or a board/cpu/arch Makefile
(I'll admit my lack of Makefile mastery here) but what seems to work
best is something like this to add "other" targets that don't need to
reside in the top level Makefile. This is best because targets depend
on things in the toplevel Makefile.

sinclude $(obj)arch/$(ARCH)/Makefile.targets
sinclude $(obj)arch/$(ARCH)/cpu/$(CPU)/Makefile.targets

>> diff --git a/mkconfig b/mkconfig
>> index e72ec3d..74294ec 100755
>> --- a/mkconfig
>> +++ b/mkconfig
>> @@ -159,6 +159,7 @@ for i in ${TARGETS} ; do
>>  done
>>
>>  cat << EOF >> config.h
>> +#define CONFIG_BOARD_NAME $BOARD_NAME
>>  #define CONFIG_BOARDDIR board/$BOARDDIR
>>  #include <config_cmd_defaults.h>
>>  #include <config_defaults.h>
>
> NAK. This breaks existing code, also it should not be needed.

How does this break existing code? Because a specific board might also
defined this? I can drop it and add it to the board config file
instead, but it seemed easier to programmatically do this so one
target can build another target. This combined with the previous patch
lets one build invoke another build (then we can add an additional
option via EXTRA_OPTS from the previous patch)

-M
Wolfgang Denk - June 7, 2011, 8:19 p.m.
Dear McClintock Matthew-B29882,

In message <BANLkTikgy1nw8rwrk=ex6N9oVSfAfhEgZQ@mail.gmail.com> you wrote:
>
> Perhaps having a CONFIG_TARGET_IMAGE available and having just one
> generic TARGET available?
>
> ifdef CONFIG_TARGET_IMAGE
> ALL += $(CONFIG_TARGET_IMAGE)
> endif
>
> TARGET_IMAGE_OBJS-y += various.bin
> TARGET_IMAGE_OBJS-y += required.bin
> TARGET_IMAGE_OBJS-y += blobs.bin
>
> $(obj)$(CONFIG_TARGET_IMAGE):   $(TARGET_IMAGE_OBJS-y)
>                 cat $(TARGET_IMAGE_OBJS-y) > $(obj)$(CONFIG_TARGET_IMAGE)

THis will probably not work, as different images may require different
levels of alignment / padding.  Also, some architectures require more
fancy image building capabilities (provided for example by the mkimage
tool).

> > I doubt you always need all these images.  If you need u-boot-sd.bin,
> > then just type "make u-boot-sd.bin".
>
> We choose these targets by adding a config line in boards.cfg. We
> don't build them unless we do a make BOARD_{NAND,SDCARD,SPIFLASH}. We

Ok, so you do run individual builds for one selected image type each.
This is IMHO OK.

> could optionally just require the user to type out the target manually

What's the difference?  Writing "make BOARD_{NAND,SDCARD,SPIFLASH}" is
typing out the target manually, isn't it?

> or do what I mentioned above however, I strongly believe these images
> should always be built as we don't want end user confusion about which
> image to program.

In which way would that prevent any confusion? In any case you have to
know which image you want to program.  It's poretty much the same to
me if I have to select a name from a list of images that resault from
a "make" run, or if I select a name from a list of possible make
target names.  Actually, the latter seems more logical to me.

> >> +NAND_SPL_OBJS-y += $(obj)nand_spl/u-boot-spl-16k.bin
> >> +NAND_SPL_OBJS-$(CONFIG_BOOTSTRAP) += $(obj)u-boot-bootstrap.bin
> >> +NAND_SPL_OBJS-y += $(obj)u-boot.bin
> >
> > This looks pretty much board specific to me.  I doubt all boards can
> > use u-boot-spl-16k.bin here.
>
> This u-boot-spl-16k.bin reference has been here before this patch. It

Yes, I know.

> is typically just the u-boot-nand image to be used and not actually
> required to be 16k. It could probably be updated to just be
> u-boot-spl.bin except that for some boards this is not the right
> image. Usually the 16k one points at the right image.

In general, it's wrong and should be fixed.

> > Do we really need all this in the top level Makefile?
>
> I'm not sure there is a great way around this. I played around with
> trying to include them in the config.mk or a board/cpu/arch Makefile
> (I'll admit my lack of Makefile mastery here) but what seems to work
> best is something like this to add "other" targets that don't need to
> reside in the top level Makefile. This is best because targets depend
> on things in the toplevel Makefile.


I think we should step back a bit.  Assume I forgot everything I've
read so far, and try to explain me in simple words (completely
independent from your suggested implementation) what exactly you want
to do.

> >>  cat << EOF >> config.h
> >> +#define CONFIG_BOARD_NAME $BOARD_NAME
> >>  #define CONFIG_BOARDDIR board/$BOARDDIR
> >>  #include <config_cmd_defaults.h>
> >>  #include <config_defaults.h>
> >
> > NAK. This breaks existing code, also it should not be needed.
>
> How does this break existing code? Because a specific board might also
> defined this? I can drop it and add it to the board config file

Not might, bot actually does.

> instead, but it seemed easier to programmatically do this so one
> target can build another target. This combined with the previous patch
> lets one build invoke another build (then we can add an additional
> option via EXTRA_OPTS from the previous patch)

All this is way too obscure to me.  I fail to see what you are trying
to acchieve.


Best regards,

Wolfgang Denk
McClintock Matthew-B29882 - June 8, 2011, 6:38 p.m.
On Tue, Jun 7, 2011 at 3:19 PM, Wolfgang Denk <wd@denx.de> wrote:
>> Perhaps having a CONFIG_TARGET_IMAGE available and having just one
>> generic TARGET available?
>>
>> ifdef CONFIG_TARGET_IMAGE
>> ALL += $(CONFIG_TARGET_IMAGE)
>> endif
>>
>> TARGET_IMAGE_OBJS-y += various.bin
>> TARGET_IMAGE_OBJS-y += required.bin
>> TARGET_IMAGE_OBJS-y += blobs.bin
>>
>> $(obj)$(CONFIG_TARGET_IMAGE):   $(TARGET_IMAGE_OBJS-y)
>>                 cat $(TARGET_IMAGE_OBJS-y) > $(obj)$(CONFIG_TARGET_IMAGE)
>
> THis will probably not work, as different images may require different
> levels of alignment / padding.  Also, some architectures require more
> fancy image building capabilities (provided for example by the mkimage
> tool).

It works for the images in the Makefile currently, but in the future
more complicated and board specific targets could live in
board/arch/cpu specific files that are included into the top level
makefile.

sinclude $(obj)arch/$(ARCH)/Makefile.targets
sinclude $(obj)arch/$(ARCH)/cpu/$(CPU)/Makefile.targets

In fact we could move all image creation (besides u-boot.bin) out to
the files and clean out the top level makefile.

>
>> > Do we really need all this in the top level Makefile?
>>
>> I'm not sure there is a great way around this. I played around with
>> trying to include them in the config.mk or a board/cpu/arch Makefile
>> (I'll admit my lack of Makefile mastery here) but what seems to work
>> best is something like this to add "other" targets that don't need to
>> reside in the top level Makefile. This is best because targets depend
>> on things in the toplevel Makefile.
>
> I think we should step back a bit.  Assume I forgot everything I've
> read so far, and try to explain me in simple words (completely
> independent from your suggested implementation) what exactly you want
> to do.

I think my other reply to your email on patch 2/4 should provide a
good summary on what I am trying to accomplish.

>
>> >>  cat << EOF >> config.h
>> >> +#define CONFIG_BOARD_NAME $BOARD_NAME
>> >>  #define CONFIG_BOARDDIR board/$BOARDDIR
>> >>  #include <config_cmd_defaults.h>
>> >>  #include <config_defaults.h>
>> >
>> > NAK. This breaks existing code, also it should not be needed.
>>
>> How does this break existing code? Because a specific board might also
>> defined this? I can drop it and add it to the board config file
>
> Not might, bot actually does.

Still not sure how this is breaking anything. Can you point me at a
build that fails?

Patch

diff --git a/Makefile b/Makefile
index 0400930..7a9cae7 100644
--- a/Makefile
+++ b/Makefile
@@ -317,6 +317,16 @@  ifeq ($(CONFIG_NAND_U_BOOT),y)
 ALL += $(obj)u-boot-nand.bin
 endif
 
+ifndef CONFIG_IN_BOOTSTRAP
+ifeq ($(CONFIG_SPIFLASH), y)
+ALL += $(obj)u-boot-spi.bin
+endif
+
+ifeq ($(CONFIG_SDCARD), y)
+ALL += $(obj)u-boot-sd.bin
+endif
+endif
+
 ifeq ($(CONFIG_ONENAND_U_BOOT),y)
 ALL += $(obj)u-boot-onenand.bin
 ONENAND_BIN ?= $(obj)onenand_ipl/onenand-ipl-2k.bin
@@ -404,8 +414,16 @@  $(obj)u-boot.lds: $(LDSCRIPT)
 nand_spl:	$(TIMESTAMP_FILE) $(VERSION_FILE) depend
 		$(MAKE) -C nand_spl/board/$(BOARDDIR) all
 
+NAND_SPL_OBJS-y += $(obj)nand_spl/u-boot-spl-16k.bin
+NAND_SPL_OBJS-$(CONFIG_BOOTSTRAP) += $(obj)u-boot-bootstrap.bin
+NAND_SPL_OBJS-y += $(obj)u-boot.bin
+
+ifeq ($(CONFIG_BOOTSTRAP),y)
+$(obj)u-boot-nand.bin:	nand_spl $(obj)u-boot-bootstrap.bin $(obj)u-boot.bin
+else
 $(obj)u-boot-nand.bin:	nand_spl $(obj)u-boot.bin
-		cat $(obj)nand_spl/u-boot-spl-16k.bin $(obj)u-boot.bin > $(obj)u-boot-nand.bin
+endif
+		cat $(NAND_SPL_OBJS-y) > $(obj)u-boot-nand.bin
 
 onenand_ipl:	$(TIMESTAMP_FILE) $(VERSION_FILE) $(obj)include/autoconf.mk
 		$(MAKE) -C onenand_ipl/board/$(BOARDDIR) all
@@ -413,6 +431,23 @@  onenand_ipl:	$(TIMESTAMP_FILE) $(VERSION_FILE) $(obj)include/autoconf.mk
 $(obj)u-boot-onenand.bin:	onenand_ipl $(obj)u-boot.bin
 		cat $(ONENAND_BIN) $(obj)u-boot.bin > $(obj)u-boot-onenand.bin
 
+ifdef CONFIG_BOOTSTRAP
+$(obj)u-boot-spi.bin:		$(obj)u-boot.bin $(obj)u-boot-bootstrap.bin
+		cat $(obj)u-boot-bootstrap.bin $(obj)u-boot.bin > $(obj)u-boot-spi.bin
+$(obj)u-boot-sd.bin:		$(obj)u-boot.bin $(obj)u-boot-bootstrap.bin
+		cat $(obj)u-boot-bootstrap.bin $(obj)u-boot.bin > $(obj)u-boot-sd.bin
+else
+$(obj)u-boot-spi.bin:		$(obj)u-boot.bin
+		ln -s u-boot.bin u-boot-spi.bin
+$(obj)u-boot-sd.bin:		$(obj)u-boot.bin
+		ln -s u-boot.bin u-boot-sd.bin
+endif
+
+$(obj)u-boot-bootstrap.bin:
+		rm -rf $(BUILD_DIR)bootstrap
+		$(MAKE) $(CONFIG_BOARD_NAME) O=bootstrap/ EXTRA_OPTS=IN_BOOTSTRAP
+		cp $(BUILD_DIR)bootstrap/u-boot.bin u-boot-bootstrap.bin
+
 $(VERSION_FILE):
 		@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \
 		   printf '#define PLAIN_VERSION "%s%s"\n' \
@@ -1130,6 +1165,7 @@  clean:
 		\( -name 'core' -o -name '*.bak' -o -name '*~' \
 		-o -name '*.o'	-o -name '*.a' -o -name '*.exe'	\) -print \
 		| xargs rm -f
+	@rm -fr $(BUILD_DIR)bootstrap/
 
 clobber:	clean
 	@find $(OBJTREE) -type f \( -name '*.depend' \
diff --git a/mkconfig b/mkconfig
index e72ec3d..74294ec 100755
--- a/mkconfig
+++ b/mkconfig
@@ -159,6 +159,7 @@  for i in ${TARGETS} ; do
 done
 
 cat << EOF >> config.h
+#define CONFIG_BOARD_NAME $BOARD_NAME
 #define CONFIG_BOARDDIR board/$BOARDDIR
 #include <config_cmd_defaults.h>
 #include <config_defaults.h>