| Message ID | 1307042852-10385-3-git-send-email-y |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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?
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>