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
Regards,
Graeme
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
