[U-Boot] [PATCH 3/9] x86: Allow excluding reset handling code from u-boot.

[Graeme Russ]

Hi Simon,

On Wed, Oct 10, 2012 at 8:15 AM, Simon Glass <sjg at 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