While I think this patch is great and we definitely need it, there are some things I'd like to discuss and improve, or back out if possible...
On 3/27/10 6:18 PM, repository service wrote: > -all: $(obj)/config.h coreboot > +all: $(obj)/config.h $(obj)/build.h coreboot > endif > This will make $(obj)/build.h and coreboot in parallel I think... is that intended? > -$(obj)/$$(1)%$(3).o: src/$$(1)%.$(2) $(obj)/config.h > +$(obj)/$$(1)%$(3).o: src/$$(1)%.$(2) | $(obj)/build.h $(obj)/config.h > printf " CC $$$$(subst $$$$(obj)/,,$$$$(@))\n" > Uh what's that | $(obj)/build.h ? > OBJS := $(patsubst %,$(obj)/%,$(TARGETS-y)) > -INCLUDES := -I$(top)/src -I$(top)/src/include -I$(obj) > -I$(top)/src/arch/$(ARCHDIR-y)/include > -INCLUDES += -I$(top)/src/devices/oprom/include > -INCLUDES += -include $(obj)/config.h > +INCLUDES := -Isrc -Isrc/include -I$(obj) -Isrc/arch/$(ARCHDIR-y)/include > +INCLUDES += -Isrc/devices/oprom/include > +# abspath is a workaround for romcc > +INCLUDES += -include $(abspath $(obj)/config.h) -include $(abspath > $(obj)/build.h) Why is build.h added to INCLUDES? This basically renders the dependency system unusable because all of the C files are depending on build.h (which they are not) Only two files should depend on build.h and that's console.o and romstage.inc > @@ -195,7 +193,7 @@ > > $(obj)/mainboard/$(MAINBOARDDIR)/romstage.inc: > $(src)/mainboard/$(MAINBOARDDIR)/romstage.c $(obj)/romcc $(OPTION_TABLE_H) > $(obj)/build.h > printf " ROMCC romstage.inc\n" > - $(ROMCC) -c -S $(ROMCCFLAGS) -include $(obj)/build.h -I. $(INCLUDES) $< > -o $@ > + $(ROMCC) -c -S $(ROMCCFLAGS) -I. $(INCLUDES) $< -o $@ > Why is build.h dropped here? > > @@ -205,7 +203,7 @@ > > $(obj)/mainboard/$(MAINBOARDDIR)/romstage.pre.inc: > $(src)/mainboard/$(MAINBOARDDIR)/romstage.c $(OPTION_TABLE_H) $(obj)/build.h > printf " CC romstage.inc\n" > - $(CC) -MMD $(CFLAGS) -include $(obj)/build.h -I$(src) -I. -c -S $< -o $@ > + $(CC) -MMD $(CFLAGS) -I$(src) -I. -c -S $< -o $@ > Why is build.h dropped here? > Modified: trunk/src/arch/i386/lib/Makefile.inc > ============================================================================== > --- trunk/src/arch/i386/lib/Makefile.inc Fri Mar 26 19:31:12 2010 > (r5303) > +++ trunk/src/arch/i386/lib/Makefile.inc Sat Mar 27 18:18:39 2010 > (r5304) > @@ -8,8 +8,3 @@ > > initobj-y += printk_init.o > initobj-y += cbfs_and_run.o > - > -ifdef POST_EVALUATION > -$(obj)/arch/i386/lib/console.o :: $(obj)/build.h > -endif > - > I don't think it's OK to drop this dependency, as it needs to be known before gcc is run on the binary. Stefan -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
