diff mbox

[U-Boot,v5,2/9] x86: Allow excluding reset vector code from u-boot

Message ID 1353961997-32762-3-git-send-email-sjg@chromium.org
State Superseded, archived
Headers show

Commit Message

Simon Glass Nov. 26, 2012, 8:33 p.m. UTC 
From: Gabe Black <gabeblack@chromium.org>

When running from coreboot we don't want this code.

This version works by ifdef-ing out all of the code that would go
into those sections and all the code that refers to it. The sections are
then empty, and the linker will either leave them empty for the loader
to ignore or remove them entirely.

Signed-off-by: Gabe Black <gabeblack@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v5:
- Use CONFIG_NO_RESET_CODE again (as v3), check it in arch config.h

Changes in v4:
- Use CONFIG_SYS_X86_RESET_VECTOR instead CONFIG_NO_RESET_CODE
- Add note about CONFIG_SYS_X86_RESET_VECTOR to README

Changes in v3:
- Fix incorrect repeated line in Makefile

Changes in v2:
- Put CONFIG_NO_RESET_CODE into Makefile instead of source files

 Makefile                      |    8 +++-----
 README                        |    4 ++++
 arch/x86/cpu/Makefile         |    5 +++--
 arch/x86/cpu/u-boot.lds       |    3 +++
 arch/x86/include/asm/config.h |    5 +++++
 5 files changed, 18 insertions(+), 7 deletions(-)

Comments

Wolfgang Denk Nov. 26, 2012, 11:03 p.m. UTC | #1 
Dear Simon Glass,

In message <1353961997-32762-3-git-send-email-sjg@chromium.org> you wrote:
> 
> +- CONFIG_NO_RESET_CODE
> +		If defined, the x86 reset vector code is excluded. You will need
> +		to do this when U-Boot is running from Coreboot.

Sorry fr never ending nitpicking - but either this is some x86
specific stuff, then it would probably make sense to have such
indication in the name of the define, or it is not, then the comment
should be fixed.

> +START-$(CONFIG_SYS_X86_RESET_VECTOR) += resetvec.o start16.o

CONFIG_SYS_X86_RESET_VECTOR - is this just another new, undocumented
variable?

Or a mismatch between documentation and code?

Best regards,

Wolfgang Denk
Simon Glass Nov. 26, 2012, 11:14 p.m. UTC | #2 
Hi Wolfgang,

On Mon, Nov 26, 2012 at 3:03 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1353961997-32762-3-git-send-email-sjg@chromium.org> you wrote:
>>
>> +- CONFIG_NO_RESET_CODE
>> +             If defined, the x86 reset vector code is excluded. You will need
>> +             to do this when U-Boot is running from Coreboot.
>
> Sorry fr never ending nitpicking - but either this is some x86
> specific stuff, then it would probably make sense to have such
> indication in the name of the define, or it is not, then the comment
> should be fixed.
>
>> +START-$(CONFIG_SYS_X86_RESET_VECTOR) += resetvec.o start16.o
>
> CONFIG_SYS_X86_RESET_VECTOR - is this just another new, undocumented
> variable?

Yes it is new and undocumented, because it is internal to the x86
implementation and I don't want people to set it.

>
> Or a mismatch between documentation and code?

Well there are now two options:

1. The user-facing CONFIG_NO_RESET_CODE option which, if not defined,
asserts CONFIG_SYS_X86_RESET_VECTOR
2. CONFIG_SYS_X86_RESET_VECTOR which is used in the Makefiles

I would rather have a single positive option
(CONFIG_SYS_X86_RESET_VECTOR, as series v4). Failing that I would
rather have ifneq in the Makefile (and just use CONFIG_NO_RESET_CODE).
I am not sure how to have a negative option without an ifneq in the
Makefile. I looked pretty hard but could not find an example in
U-Boot.

Re CONFIG_NO_RESET_CODE, it probably should be
CONFIG_SYS_X86_NO_RESET_CODE, but it would be even better if we could
just have one option. I worry that people will be confused by one that
they set, which affects another that is set if they don't see the
first...In a month I will probably find it confusing.

This is possible a minor point since I think Graeme said that the only
other x86 board will move to Coreboot soon.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "What if" is a trademark of Hewlett Packard, so stop using it in your
> sentences without permission, or risk being sued.
Graeme Russ Nov. 26, 2012, 11:24 p.m. UTC | #3 
Hi Simon,

On Tue, Nov 27, 2012 at 10:14 AM, Simon Glass <sjg@chromium.org> wrote:

> Hi Wolfgang,
>
> On Mon, Nov 26, 2012 at 3:03 PM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Simon Glass,
> >
> > In message <1353961997-32762-3-git-send-email-sjg@chromium.org> you
> wrote:
> >>
> >> +- CONFIG_NO_RESET_CODE
> >> +             If defined, the x86 reset vector code is excluded. You
> will need
> >> +             to do this when U-Boot is running from Coreboot.
> >
> > Sorry fr never ending nitpicking - but either this is some x86
> > specific stuff, then it would probably make sense to have such
> > indication in the name of the define, or it is not, then the comment
> > should be fixed.
> >
> >> +START-$(CONFIG_SYS_X86_RESET_VECTOR) += resetvec.o start16.o
> >
> > CONFIG_SYS_X86_RESET_VECTOR - is this just another new, undocumented
> > variable?
>
> Yes it is new and undocumented, because it is internal to the x86
> implementation and I don't want people to set it.
>
> >
> > Or a mismatch between documentation and code?
>
> Well there are now two options:
>
> 1. The user-facing CONFIG_NO_RESET_CODE option which, if not defined,
> asserts CONFIG_SYS_X86_RESET_VECTOR
> 2. CONFIG_SYS_X86_RESET_VECTOR which is used in the Makefiles
>
> I would rather have a single positive option
> (CONFIG_SYS_X86_RESET_VECTOR, as series v4). Failing that I would
> rather have ifneq in the Makefile (and just use CONFIG_NO_RESET_CODE).
> I am not sure how to have a negative option without an ifneq in the
> Makefile. I looked pretty hard but could not find an example in
> U-Boot.
>

That is the crux of the problem - we want to have a config define which
_excludes_  the compilation of some code (in this case the reset vector).
This should be expanded to optionally exclude the 16-bit 'Real Mode' code
(with the ultimate goal of dropping that code altogether).

My vote would be for a CONFIG_X86_NO_RESET_VECTOR (and
CONFIG_X86_NO_REAL_MODE) and just use ifneq in the Makefile


> Re CONFIG_NO_RESET_CODE, it probably should be
> CONFIG_SYS_X86_NO_RESET_CODE, but it would be even better if we could
> just have one option. I worry that people will be confused by one that
> they set, which affects another that is set if they don't see the
> first...In a month I will probably find it confusing.
>
> This is possible a minor point since I think Graeme said that the only
> other x86 board will move to Coreboot soon.
>
>
Actually, I said that the only other x86 board will be dropped from U-Boot
as the processor is no longer available.

Regards,

Graeme
Wolfgang Denk Nov. 27, 2012, 1:41 p.m. UTC | #4 
Dear Simon Glass,

In message <CAPnjgZ2EjgHHnNj-0dyHgMMHhomYLuVJE=KF7pFCpwRSbDgY8g@mail.gmail.com> you wrote:
> 
> > CONFIG_SYS_X86_RESET_VECTOR - is this just another new, undocumented
> > variable?
> 
> Yes it is new and undocumented, because it is internal to the x86
> implementation and I don't want people to set it.

Then document it as internal and not to be touched, but documented it
must be.

> Well there are now two options:
> 
> 1. The user-facing CONFIG_NO_RESET_CODE option which, if not defined,
> asserts CONFIG_SYS_X86_RESET_VECTOR
> 2. CONFIG_SYS_X86_RESET_VECTOR which is used in the Makefiles
> 
> I would rather have a single positive option
> (CONFIG_SYS_X86_RESET_VECTOR, as series v4). Failing that I would
> rather have ifneq in the Makefile (and just use CONFIG_NO_RESET_CODE).
> I am not sure how to have a negative option without an ifneq in the
> Makefile. I looked pretty hard but could not find an example in
> U-Boot.

First, you can #define CONFIG_SYS_X86_RESET_VECTOR in some global
header file, and in the boards that don't want it add an #undef in the
board config file.

Second, you can have some

	RESET_OBJS-$(CONFIG_SYS_X86_RESET_VECTOR) = <your_list>

in your Makefile, and then use
	
	COBJS   := $(sort $(COBJS-y) $(RESET_OBJS-))

which will include <your_list> only if CONFIG_SYS_X86_RESET_VECTOR is
_not_ set or empty.

etc.

> This is possible a minor point since I think Graeme said that the only
> other x86 board will move to Coreboot soon.

...for the time being, maybe.  But we should not bar other options
without serious need.

Best regards,

Wolfgang Denk
Wolfgang Denk Nov. 27, 2012, 1:42 p.m. UTC | #5 
Dear Graeme Russ,

In message <CALButCJ43_3d_PgNwnC3w9q4Fxy0rgZAXd1KTzb+SDytqE3Y1Q@mail.gmail.com> you wrote:
>
> My vote would be for a CONFIG_X86_NO_RESET_VECTOR (and
> CONFIG_X86_NO_REAL_MODE) and just use ifneq in the Makefile

ACK for the names, but ther eis no need for ifneq or such.


Best regards,

Wolfgang Denk
Simon Glass Nov. 27, 2012, 3:12 p.m. UTC | #6 
Hi,

On Tue, Nov 27, 2012 at 5:42 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme Russ,
>
> In message <CALButCJ43_3d_PgNwnC3w9q4Fxy0rgZAXd1KTzb+SDytqE3Y1Q@mail.gmail.com> you wrote:
>>
>> My vote would be for a CONFIG_X86_NO_RESET_VECTOR (and
>> CONFIG_X86_NO_REAL_MODE) and just use ifneq in the Makefile
>
> ACK for the names, but ther eis no need for ifneq or such.

Thank you both, I will make it so.

Regards,
Simon

>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> A good marriage would be between a blind wife and deaf husband.
>                                                -- Michel de Montaigne
diff mbox

Patch

diff --git a/Makefile b/Makefile
index 1a17be9..dcd3a0c 100644
--- a/Makefile
+++ b/Makefile
@@ -230,10 +230,8 @@  endif
 # U-Boot objects....order is important (i.e. start must be first)
 
 OBJS  = $(CPUDIR)/start.o
-ifeq ($(CPU),x86)
-OBJS += $(CPUDIR)/start16.o
-OBJS += $(CPUDIR)/resetvec.o
-endif
+OBJS-$(CONFIG_SYS_X86_RESET_VECTOR) += $(CPUDIR)/start16.o
+OBJS-$(CONFIG_SYS_X86_RESET_VECTOR) += $(CPUDIR)/resetvec.o
 ifeq ($(CPU),ppc4xx)
 OBJS += $(CPUDIR)/resetvec.o
 endif
@@ -241,7 +239,7 @@  ifeq ($(CPU),mpc85xx)
 OBJS += $(CPUDIR)/resetvec.o
 endif
 
-OBJS := $(addprefix $(obj),$(OBJS))
+OBJS := $(addprefix $(obj),$(OBJS) $(OBJS-y))
 
 HAVE_VENDOR_COMMON_LIB = $(if $(wildcard board/$(VENDOR)/common/Makefile),y,n)
 
diff --git a/README b/README
index 2dc0984..dd7fb9d 100644
--- a/README
+++ b/README
@@ -3621,6 +3621,10 @@  Low Level (hardware related) configuration options:
 		be used if available. These functions may be faster under some
 		conditions but may increase the binary size.
 
+- CONFIG_NO_RESET_CODE
+		If defined, the x86 reset vector code is excluded. You will need
+		to do this when U-Boot is running from Coreboot.
+
 Freescale QE/FMAN Firmware Support:
 -----------------------------------
 
diff --git a/arch/x86/cpu/Makefile b/arch/x86/cpu/Makefile
index 7f1fc18..b0c350c 100644
--- a/arch/x86/cpu/Makefile
+++ b/arch/x86/cpu/Makefile
@@ -28,12 +28,13 @@  include $(TOPDIR)/config.mk
 
 LIB	= $(obj)lib$(CPU).o
 
-START	= start.o start16.o resetvec.o
+START-y	= start.o
+START-$(CONFIG_SYS_X86_RESET_VECTOR) += resetvec.o start16.o
 COBJS	= interrupts.o cpu.o
 
 SRCS	:= $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
-START	:= $(addprefix $(obj),$(START))
+START	:= $(addprefix $(obj),$(START-y))
 
 all:	$(obj).depend $(START) $(LIB)
 
diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds
index a1ecefa..de6dfdc 100644
--- a/arch/x86/cpu/u-boot.lds
+++ b/arch/x86/cpu/u-boot.lds
@@ -86,6 +86,8 @@  SECTIONS
 	__bios_start = LOADADDR(.bios);
 	__bios_size = SIZEOF(.bios);
 
+#ifdef CONFIG_SYS_X86_RESET_VECTOR
+
 	/*
 	 * The following expressions place the 16-bit Real-Mode code and
 	 * Reset Vector at the end of the Flash ROM
@@ -95,4 +97,5 @@  SECTIONS
 
 	. = RESET_VEC_LOC;
 	.resetvec : AT (CONFIG_SYS_TEXT_BASE + (CONFIG_SYS_MONITOR_LEN - RESET_SEG_SIZE + RESET_VEC_LOC)) { KEEP(*(.resetvec)); }
+#endif
 }
diff --git a/arch/x86/include/asm/config.h b/arch/x86/include/asm/config.h
index 049c44e..e3cae0b 100644
--- a/arch/x86/include/asm/config.h
+++ b/arch/x86/include/asm/config.h
@@ -21,4 +21,9 @@ 
 #ifndef _ASM_CONFIG_H_
 #define _ASM_CONFIG_H_
 
+/* Include the reset fector code unless the board configure doesn't want it */
+#ifndef CONFIG_NO_RESET_CODE
+#define CONFIG_SYS_X86_RESET_VECTOR
+#endif
+
 #endif