Hi Graeme, On Tue, Oct 9, 2012 at 3:58 PM, Graeme Russ <graeme.r...@gmail.com> wrote: > Hi Simon, > > On Wed, Oct 10, 2012 at 8:15 AM, Simon Glass <s...@chromium.org> wrote: > >>>> diff --git a/arch/x86/cpu/resetvec.S b/arch/x86/cpu/resetvec.S >>>> index 44aee5f..5b359ff 100644 >>>> --- a/arch/x86/cpu/resetvec.S >>>> +++ b/arch/x86/cpu/resetvec.S >>>> @@ -25,6 +25,10 @@ >>>> >>>> /* Reset vector, jumps to start16.S */ >>>> >>>> +#include <config.h> >>>> + >>>> +#ifndef CONFIG_NO_RESET_CODE >>>> + >>>> .extern start16 >>>> >>>> .section .resetvec, "ax" >>>> @@ -36,3 +40,5 @@ reset_vector: >>>> >>>> .org 0xf >>>> nop >>>> + >>>> +#endif >>> >>> Condition it out in the Makefile instead >> >> I suspect the reason it was done here is these lines in the top-level >> Makefile. >> >> OBJS = $(CPUDIR)/start.o >> ifeq ($(CPU),x86) >> OBJS += $(CPUDIR)/start16.o >> OBJS += $(CPUDIR)/resetvec.o >> endif > > I have often wondered about these lines in the top-level Makefile > considering they are also in arch/x86/cpu/Makefile. I keep meaning to test > if they are actually needed in the top-level Makefile but keep forgetting > > (I see why now - see below) > >> If we just take it out of the .lds file then start16.o and resetvec.o >> are not included in the image. But they will still be built. We could >> add an additional condition here perhaps, like: > > I don't see a huge problem with that. Yes, it's a waste of CPU cycles > during the build but really, who cares. > >> OBJS = $(CPUDIR)/start.o >> ifeq ($(CPU),x86) >> ifneq ($(CONFIG_NO_RESET_CODE),y) >> OBJS += $(CPUDIR)/start16.o >> OBJS += $(CPUDIR)/resetvec.o >> endif >> endif > > Looks good for the time being (again, see beloW). > >> Here is the menu as I see it - what would you prefer? >> - top level Makefile change >> - arch/arm/cpu/Makefile change (pointless if top level Makefile >> includes these files anyway) >> - building everything but removing unneeded object files in the link script > > Can we not invert the logic of CONFIG_X86_NO_RESET_VECTOR using some > Makefile magic and then do this in arch/x86/cpu/Makefile: > > START-$(INCLUDE_X86_RESET_VECTOR) += resetvec.o > START-y = start.o > START-$(INCLUDE_X86_RESET_VECTOR) += start16.o > > Actuall, to be honest, it should be: > > SOBJS-y += start.o > > SOBJS16-$(INCLUDE_X86_RESET_VECTOR) += resetvec.o > SOBJS16-$(INCLUDE_X86_RESET_VECTOR) += start16.o > > SRCS := $(SOBJS16-y:.o=.S) $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c) > OBJS := $(addprefix $(obj),$(SOBJS) $(COBJS)) > OBJS16 := $(addprefix $(obj),$(SOBJS16)) > > all: $(obj).depend $(OBJS16) $(LIB) > > start.S is not at all related to the reset vector / protected mode switch, > and so can safely be moved into the main (32-bit) lib. ENTRY(_start) in > the linker script and: > > .section .text > .code32 > .globl _start > .type _start, @function .globl _start > .type _start, @function > > in start.S will always guarantee that the code in start.S appears first > in u-boot.bin. > > Ah Ha! now I get it - Now I see why the top-level Makefile includes: > > OBJS = $(CPUDIR)/start.o > ifeq ($(CPU),x86) > OBJS += $(CPUDIR)/start16.o > OBJS += $(CPUDIR)/resetvec.o > endif > > These files are not in $(CPUDIR)/lib$(CPU).o so they must be pulled in > individually! > > OK, by moving start.o into the lib we can drop the first line... > > Now, if we create a 16-bit lib in arch/x86/cpu/Makefile: > > LIB = $(obj)lib$(CPU).o > LIB16 = $(obj)lib16$(CPU).o > > SOBJS16-$(INCLUDE_X86_RESET_VECTOR) += resetvec.o > SOBJS16-$(INCLUDE_X86_RESET_VECTOR) += start16.o > > SOBJS-y += start.o > COBJS-y += cpu.o > COBJS-y += interrupts.o > > SRCS := $(SOBJS16-y:.o=.S) $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c) > OBJS16 := $(addprefix $(obj),$(SOBJS16)) > OBJS := $(addprefix $(obj),$(SOBJS) $(COBJS)) > > all: $(obj).depend $(LIB) $(LIB16) > > $(LIB): $(OBJS) > $(call cmd_link_o_target, $(OBJS)) > > $(LIB16): $(OBJS16) > $(call cmd_link_o_target, $(OBJS16)) > > And then in the top-level Makefile: > > LIBS-$(INCLUDE_X86_RESET_VECTOR) += $(CPUDIR)/lib16$(CPU).o > > Much cleaner :) > > (Hmmm, looking at arch/x86/lib/Makefile is appears that it is safe to mix > 16- and 32-bit code in the same lib - maybe that is a better solution...) > > But don't worry too much about all that in these patches - Make the changes > as you have already suggested and I will tweak the rest later
Looks good - yes I will prepare a new series to send likely on Thursday. Not that it matters for now, but who will define INCLUDE_X86_RESET_VECTOR? Regards, Simon > > Regards, > > Graeme _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot