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

[Simon Glass]

Hi Graeme,

On Tue, Oct 9, 2012 at 3:58 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> 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

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