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

[Simon Glass]

Hi Graeme,

On Wed, Oct 3, 2012 at 6:01 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> Hi Simon,
>
> At first I thought this patch dealt with the 'board reset' code but
> then realised it deals with the 'reset vector' - Can you fix the patch
> subject please

Will do.

>
> On Thu, Oct 4, 2012 at 10:39 AM, Simon Glass <sjg at chromium.org> wrote:
>> From: Gabe Black <gabeblack at 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.
>
> Could this be done by #ifdef'ing the section in the linker script?

Yes, in fact that is already in this patch.

>
>> Signed-off-by: Gabe Black <gabeblack at chromium.org>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>  arch/x86/cpu/resetvec.S                          |    6 ++++++
>>  arch/x86/cpu/start16.S                           |    4 ++++
>>  arch/x86/cpu/u-boot.lds                          |    3 +++
>>  board/chromebook-x86/coreboot/coreboot_start16.S |    6 ++++++
>>  board/eNET/eNET_start16.S                        |    4 ++++
>>  5 files changed, 23 insertions(+), 0 deletions(-)
>>
>> 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


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:

OBJS  = $(CPUDIR)/start.o
ifeq ($(CPU),x86)
	ifneq ($(CONFIG_NO_RESET_CODE),y)
		OBJS += $(CPUDIR)/start16.o
		OBJS += $(CPUDIR)/resetvec.o
	endif
endif


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

>
>> diff --git a/arch/x86/cpu/start16.S b/arch/x86/cpu/start16.S
>> index cc393ff..d944840 100644
>> --- a/arch/x86/cpu/start16.S
>> +++ b/arch/x86/cpu/start16.S
>> @@ -28,11 +28,14 @@
>>
>>  #include <asm/global_data.h>
>>  #include <asm/processor-flags.h>
>> +#include <config.h>
>>
>>  #define BOOT_SEG       0xffff0000      /* linear segment of boot code */
>>  #define a32            .byte 0x67;
>>  #define o32            .byte 0x66;
>>
>> +#ifndef CONFIG_NO_RESET_CODE
>> +
>>  .section .start16, "ax"
>>  .code16
>>  .globl start16
>> @@ -141,3 +144,4 @@ gdt:
>>         .byte   0x93            /* access */
>>         .byte   0xcf            /* flags + limit_high */
>>         .byte   0x00            /* base_high */
>> +#endif
>
> Ditto
>
>> diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds
>> index fe28030..2a90a01 100644
>> --- a/arch/x86/cpu/u-boot.lds
>> +++ b/arch/x86/cpu/u-boot.lds
>> @@ -85,6 +85,8 @@ SECTIONS
>>         __bios_start = LOADADDR(.bios);
>>         __bios_size = SIZEOF(.bios);
>>
>> +#ifndef CONFIG_NO_RESET_CODE
>> +
>>         /*
>>          * The following expressions place the 16-bit Real-Mode code and
>>          * Reset Vector at the end of the Flash ROM
>> @@ -94,4 +96,5 @@ SECTIONS
>>
>>         . = RESET_VEC_LOC;
>>         .resetvec : AT (CONFIG_SYS_TEXT_BASE + (CONFIG_SYS_MONITOR_LEN - RESET_SEG_SIZE + RESET_VEC_LOC)) { KEEP(*(.resetvec)); }
>> +#endif
>>  }
>
> The commit comment states "and the linker will either leave them empty
> for the loader to ignore or remove them entirely" but you are actually
> explicitly remove them anyway - If they are not compiled, is this
> necessary?

We need either or.

>
>> diff --git a/board/chromebook-x86/coreboot/coreboot_start16.S b/board/chromebook-x86/coreboot/coreboot_start16.S
>> index 9ad06df..6fac3d6 100644
>> --- a/board/chromebook-x86/coreboot/coreboot_start16.S
>> +++ b/board/chromebook-x86/coreboot/coreboot_start16.S
>> @@ -28,6 +28,10 @@
>>   * that is used by U-boot to its final destination.
>>   */
>>
>> +#include <config.h>
>> +
>> +#ifndef CONFIG_NO_RESET_CODE
>> +
>>  .text
>>  .section .start16, "ax"
>>  .code16
>> @@ -35,6 +39,8 @@
>>  board_init16:
>>         jmp     board_init16_ret
>>
>> +#endif
>> +
>>  .section .bios, "ax"
>>  .code16
>>  .globl realmode_reset
>
> Hmm, I doubt coreboot really need a board level start16.S and (quite
> frankly) the whole 'realmode reset' code (i.e. BIOS reset) is crap and
> should be globally tossed (no one will ever call it)

Will drop this.

>
>> diff --git a/board/eNET/eNET_start16.S b/board/eNET/eNET_start16.S
>> index 5e3f44c..43dda2f 100644
>> --- a/board/eNET/eNET_start16.S
>> +++ b/board/eNET/eNET_start16.S
>> @@ -32,6 +32,8 @@
>>  #include <asm/arch/sc520.h>
>>  #include <generated/asm-offsets.h>
>>
>> +#ifndef CONFIG_NO_RESET_CODE
>> +
>>  .text
>>  .section .start16, "ax"
>>  .code16
>> @@ -63,6 +65,8 @@ board_init16:
>>
>>         jmp     board_init16_ret
>>
>> +#endif
>> +
>>  .section .bios, "ax"
>>  .code16
>>  .globl realmode_reset
>
> All the above should mean there is no reason to touch the eNET code

Will remove this change.

>
> Regards,
>
> Graeme

Regards,
Simon