Hi Bin,

On 22 July 2015 at 21:41, Bin Meng bmeng.cn@gmail.com wrote:

Hi Simon,

On Wed, Jul 22, 2015 at 11:48 PM, Simon Glass sjg@chromium.org wrote:

...

When running U-Boot as an EFI application we cannot relocate since we do not have relocation information. U-Boot has already been relocated to a suitable address.

Add a global_data flag to control skipping relocation.

Signed-off-by: Simon Glass sjg@chromium.org

common/board_f.c | 7 +++++++ include/asm-generic/global_data.h | 1 + 2 files changed, 8 insertions(+)

diff --git a/common/board_f.c b/common/board_f.c index 21be26f..5e09c5f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -664,6 +664,11 @@ static int reloc_fdt(void)

static int setup_reloc(void) {

•   if (gd->flags & GD_FLG_SKIP_RELOC) {
  

•           debug("Skipping relocation due to flag\n");
  

•           return 0;
  

•   }
  

#ifdef CONFIG_SYS_TEXT_BASE gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE; #ifdef CONFIG_M68K @@ -689,6 +694,8 @@ static int setup_reloc(void)

static int jump_to_copy(void) {

•   if (gd->flags & GD_FLG_SKIP_RELOC)
  

•           return 0;
    /*
     * x86 is special, but in a nice way. It uses a trampoline which
     * enables the dcache if possible.
  

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index db0550b..086ee96 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -116,5 +116,6 @@ typedef struct global_data { #define GD_FLG_ENV_READY 0x00080 /* Env. imported into hash table */ #define GD_FLG_SERIAL_READY 0x00100 /* Pre-reloc serial console ready */ #define GD_FLG_FULL_MALLOC_INIT 0x00200 /* Full malloc() is ready */ +#define GD_FLG_SKIP_RELOC 0x00400 /* Don't relocate */

#endif /* __ASM_GENERIC_GBL_DATA_H */

What about reloc_fdt() and do_elf_reloc_fixups()?

I'll add the first one. The second one is not called and I have a patch to remove it.

Regards, Bin

On Wed, Jul 22, 2015 at 11:48 PM, Simon Glass sjg@chromium.org wrote:

This is currently done for all targets, since 0xff is the default erased value for most flash devices. In some cases this is not what we want (e.g. for EFI images) so provide a command to do a vanilla objcopy.

Signed-off-by: Simon Glass sjg@chromium.org

Makefile | 8 +++++++- config.mk | 2 -- 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile index b6f83a5..e0218b3 100644 --- a/Makefile +++ b/Makefile @@ -778,8 +778,14 @@ ifneq ($(CONFIG_SYS_TEXT_BASE),) LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE) endif

+# Normally we fill empty space with 0xff quiet_cmd_objcopy = OBJCOPY $@ -cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $(OBJCOPYFLAGS_$(@F)) $< $@ +cmd_objcopy = $(OBJCOPY) --gap-fill=0xff $(OBJCOPYFLAGS) \

•   $(OBJCOPYFLAGS_$(@F)) $< $@
  

+# Provide a version which does not do this, for use by EFI +quiet_cmd_zobjcopy = OBJCOPY $@ +cmd_zobjcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $(OBJCOPYFLAGS_$(@F)) $< $@

quiet_cmd_mkimage = MKIMAGE $@ cmd_mkimage = $(objtree)/tools/mkimage $(MKIMAGEFLAGS_$(@F)) -d $< $@ \ diff --git a/config.mk b/config.mk index 6282919..b77d589 100644 --- a/config.mk +++ b/config.mk @@ -73,8 +73,6 @@ endif

RELFLAGS := $(PLATFORM_RELFLAGS)

-OBJCOPYFLAGS += --gap-fill=0xff

PLATFORM_CPPFLAGS += $(RELFLAGS) PLATFORM_CPPFLAGS += -pipe

--

Reviewed-by: Bin Meng bmeng.cn@gmail.com

Hi Simon,

On Wed, Jul 22, 2015 at 11:48 PM, Simon Glass sjg@chromium.org wrote:

These will be used for efi.h both for U-Boot running as an EFI application and as a payload. They come from Linux 4.1.

Signed-off-by: Simon Glass sjg@chromium.org

include/linux/types.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/types.h b/include/linux/types.h index c9a8d9a..255967f 100644 --- a/include/linux/types.h +++ b/include/linux/types.h @@ -113,6 +113,11 @@ typedef __s64 int64_t;

#endif /* __KERNEL_STRICT_NAMES */

+/* this is a special 64bit data type that is 8-byte aligned */ +#define aligned_u64 __u64 __aligned(8) +#define aligned_be64 __be64 _aligned(8) +#define aligned_le64 __le64 _aligned(8)

The above two should be __aligned(8)

#if defined(CONFIG_USE_STDINT) && defined(__INT64_TYPE__) typedef __UINT64_TYPE__ uint64_t; typedef __UINT64_TYPE__ u_int64_t; @@ -145,7 +150,6 @@ typedef __u64 __bitwise __be64; typedef __u16 __bitwise __sum16; typedef __u32 __bitwise __wsum;

typedef unsigned __bitwise__ gfp_t;

struct ustat {

Regards, Bin

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

Fix a typo, improve some comments and add a little more detail in some cases.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/cpu/start.S | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S index 00e585e..7ef8b88 100644 --- a/arch/x86/cpu/start.S +++ b/arch/x86/cpu/start.S @@ -25,11 +25,11 @@ .globl _x86boot_start _x86boot_start: /*

•    * This is the fail safe 32-bit bootstrap entry point. The
  

•    * following code is not executed from a cold-reset (actually, a
  

•    * lot of it is, but from real-mode after cold reset. It is
  

•    * repeated here to put the board into a state as close to cold
  

•    * reset as necessary)
  

•    * This is the fail-safe 32-bit bootstrap entry point.
  

•    *
  

•    * This code is used when booting from another boot loader like
  

•    * coreboot or EFI. So we repeat some of the same init found in
  

•    * start16.
     */
    cli
    cld
  

@@ -45,15 +45,15 @@ _x86boot_start: jmp 1f _start: /*

•    * This is the 32-bit cold-reset entry point. Initialize %bx to 0
  

•    * in case we're preceeded by some sort of boot stub.
  

•    * This is the 32-bit cold-reset entry point, coming from start16.
  

•    * Set %bx to 0 to indicate this.
     */
    movw    $GD_FLG_COLD_BOOT, %bx
  

1: /* Save BIST */ movl %eax, %ebp

•   /* Load the segement registes to match the gdt loaded in start16.S */
  

•   /* Load the segement registers to match the GDT loaded in start16.S */
    movl    $(X86_GDT_ENTRY_32BIT_DS * X86_GDT_ENTRY_SIZE), %eax
    movw    %ax, %fs
    movw    %ax, %ds
  

@@ -64,7 +64,11 @@ _start: /* Clear the interrupt vectors */ lidt blank_idt_ptr

•   /* Early platform init (setup gpio, etc ) */
  

•   /*
  

•    * Critical early platform init - generally not used, we prefer init
  

•    * to happen later when we have a console, in case something goes
  

•    * wrong.
  

•    */
    jmp     early_board_init
  

.globl early_board_init_ret early_board_init_ret: @@ -79,7 +83,7 @@ car_init_ret: * We now have CONFIG_SYS_CAR_SIZE bytes of Cache-As-RAM (or SRAM, * or fully initialised SDRAM - we really don't care which) * starting at CONFIG_SYS_CAR_ADDR to be used as a temporary stack

•    * and early malloc area. The MRC requires some space at the top.
  

•    * and early malloc() area. The MRC requires some space at the top.
     *
     * Stack grows down from top of CAR. We have:
     *
  

@@ -97,7 +101,7 @@ car_init_ret: #endif #else /*

•    * When we get here after car_init, esp points to a temporary stack
  

•    * When we get here after car_init(), esp points to a temporary stack
     * and esi holds the HOB list address returned by the FSP.
     */
  

#endif @@ -137,17 +141,18 @@ skip_hob: movl %esp, %ecx

#if defined(CONFIG_SYS_MALLOC_F_LEN)

•   /* Set up the pre-relocation malloc pool */
    subl    $CONFIG_SYS_MALLOC_F_LEN, %esp
    movl    %eax, %edx
    addl    $GD_MALLOC_BASE, %edx
    movl    %esp, (%edx)
  

#endif

•   /* Store BIST */
  

•   /* Store BIST into global_data */
    movl    %eax, %edx
    addl    $GD_BIST, %edx
    movl    %ebp, (%edx)
  
  

•   /* Set second parameter to setup_gdt */
  

•   /* Set second parameter to setup_gdt() */
    movl    %ecx, %edx
  
    /* Setup global descriptor table so gd->xyz works */
  

@@ -157,7 +162,7 @@ skip_hob: post_code(POST_START_DONE) xorl %eax, %eax

•   /* Enter, U-boot! */
  

•   /* Enter, U-Boot! */
    call    board_init_f
  
    /* indicate (lack of) progress */
  

@@ -184,13 +189,13 @@ board_init_f_r_trampoline: /* Align global data to 16-byte boundary */ andl $0xfffffff0, %esp

•   /* Setup first parameter to memcpy (and setup_gdt) */
  

•   /* Setup first parameter to memcpy() and setup_gdt() */
    movl    %esp, %eax
  
  

•   /* Setup second parameter to memcpy */
  

•   /* Setup second parameter to memcpy() */
    fs movl 0, %edx
  
  

•   /* Set third parameter to memcpy */
  

•   /* Set third parameter to memcpy() */
    movl    $GENERATED_GBL_DATA_SIZE, %ecx
  
    /* Copy global data from CAR to SDRAM stack */
  

@@ -202,7 +207,7 @@ board_init_f_r_trampoline: /* Align global descriptor table to 16-byte boundary */ andl $0xfffffff0, %esp

•   /* Set second parameter to setup_gdt */
  

•   /* Set second parameter to setup_gdt() */
    movl    %esp, %edx
  
    /* Setup global descriptor table so gd->xyz works */
  

@@ -216,7 +221,7 @@ board_init_f_r_trampoline:

    call    car_uninit

1:

•   /* Re-enter U-Boot by calling board_init_f_r */
  

•   /* Re-enter U-Boot by calling board_init_f_r() */
    call    board_init_f_r
  
  

die: @@ -230,9 +235,10 @@ blank_idt_ptr:

    .p2align        2       /* force 4-byte alignment */

•   /* Add a multiboot header so U-Boot can be loaded by GRUB2 */
  

multiboot_header: /* magic */

•   .long   0x1BADB002
  

•   .long   0x1badb002
    /* flags */
    .long   (1 << 16)
    /* checksum */
  

--

Reviewed-by: Bin Meng bmeng.cn@gmail.com

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

We should use these constants where possible.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/cpu/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index b9134cf..d233a45 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -456,7 +456,7 @@ void x86_full_reset(void)

int dcache_status(void) {

•   return !(read_cr0() & 0x40000000);
  

•   return !(read_cr0() & X86_CR0_CD);
  

}

/* Define these functions to allow ehch-hcd to function */

Reviewed-by: Bin Meng bmeng.cn@gmail.com

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

These flags now overlap some global ones. Adjust the x86-specific flags to avoid this.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/include/asm/global_data.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h index 4d9eac6..abd70b9 100644 --- a/arch/x86/include/asm/global_data.h +++ b/arch/x86/include/asm/global_data.h @@ -87,14 +87,14 @@ static inline __attribute__((no_instrument_function)) gd_t *get_fs_gd_ptr(void)

#define gd get_fs_gd_ptr()

+#define DECLARE_GLOBAL_DATA_PTR

#endif

/*

• Our private Global Data Flags

*/ -#define GD_FLG_COLD_BOOT 0x00100 /* Cold Boot */ -#define GD_FLG_WARM_BOOT 0x00200 /* Warm Boot */

-#define DECLARE_GLOBAL_DATA_PTR +#define GD_FLG_COLD_BOOT 0x1000 /* Cold Boot */ +#define GD_FLG_WARM_BOOT 0x2000 /* Warm Boot */

To avoid future conflicts, can we define flags high 16-bits as architecture-specific flags while leaving low 16-bits as the generic ones?

#endif /* __ASM_GBL_DATA_H */

Regards, Bin

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

This is now handled by generic U-Boot code so we do not need an x86 version. It is no-longer called, so remove it.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/include/asm/relocate.h | 1 - arch/x86/lib/relocate.c | 18 ------------------ 2 files changed, 19 deletions(-)

diff --git a/arch/x86/include/asm/relocate.h b/arch/x86/include/asm/relocate.h index eb186b9..cff3abc 100644 --- a/arch/x86/include/asm/relocate.h +++ b/arch/x86/include/asm/relocate.h @@ -11,7 +11,6 @@ #include <common.h>

int copy_uboot_to_ram(void); -int copy_fdt_to_ram(void); int clear_bss(void); int do_elf_reloc_fixups(void);

diff --git a/arch/x86/lib/relocate.c b/arch/x86/lib/relocate.c index b33586b..1a62142 100644 --- a/arch/x86/lib/relocate.c +++ b/arch/x86/lib/relocate.c @@ -17,8 +17,6 @@

#include <common.h> #include <inttypes.h> -#include <libfdt.h> -#include <malloc.h> #include <asm/u-boot-x86.h> #include <asm/relocate.h> #include <asm/sections.h> @@ -35,22 +33,6 @@ int copy_uboot_to_ram(void) return 0; }

-int copy_fdt_to_ram(void) -{

•   if (gd->new_fdt) {
  

•           ulong fdt_size;
  

•           fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
  

•           memcpy(gd->new_fdt, gd->fdt_blob, fdt_size);
  

•           debug("Relocated fdt from %p to %p, size %lx\n",
  

•                  gd->fdt_blob, gd->new_fdt, fdt_size);
  

•           gd->fdt_blob = gd->new_fdt;
  

•   }
  

•   return 0;
  

-}

int clear_bss(void) { ulong dst_addr = (ulong)&__bss_start + gd->reloc_off; --

Reviewed-by: Bin Meng bmeng.cn@gmail.com

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

The GDT works but technically the length is incorrect. Fix this and add a comment.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/cpu/call64.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/cpu/call64.S b/arch/x86/cpu/call64.S index 74dd5a8..08dc473 100644 --- a/arch/x86/cpu/call64.S +++ b/arch/x86/cpu/call64.S @@ -82,8 +82,8 @@ lret_target:

    .data

gdt:

•   .word   gdt_end - gdt
  

•   .long   gdt
  

•   .word   gdt_end - gdt - 1
  

•   .long   gdt                     /* Fixed up by code above */
    .word   0
    .quad   0x0000000000000000      /* NULL descriptor */
    .quad   0x00af9a000000ffff      /* __KERNEL_CS */
  

--

Reviewed-by: Bin Meng bmeng.cn@gmail.com

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

Some files use global_data but don't declare it. Fix this.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/cpu/mp_init.c | 2 ++ arch/x86/lib/bootm.c | 2 ++ arch/x86/lib/fsp/fsp_common.c | 2 ++ arch/x86/lib/lpc-uclass.c | 2 ++ arch/x86/lib/pch-uclass.c | 2 ++ drivers/gpio/intel_ich6_gpio.c | 2 ++ 6 files changed, 12 insertions(+)

diff --git a/arch/x86/cpu/mp_init.c b/arch/x86/cpu/mp_init.c index e686b28..3294a50 100644 --- a/arch/x86/cpu/mp_init.c +++ b/arch/x86/cpu/mp_init.c @@ -24,6 +24,8 @@ #include <dm/uclass-internal.h> #include <linux/linkage.h>

+DECLARE_GLOBAL_DATA_PTR;

/* Total CPUs include BSP */ static int num_cpus;

diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index 86030cf..445ee6e 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -22,6 +22,8 @@ #include <asm/arch/timestamp.h> #endif

+DECLARE_GLOBAL_DATA_PTR;

#define COMMAND_LINE_OFFSET 0x9000

/* diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c index 5b25632..c585710 100644 --- a/arch/x86/lib/fsp/fsp_common.c +++ b/arch/x86/lib/fsp/fsp_common.c @@ -11,6 +11,8 @@ #include <asm/processor.h> #include <asm/fsp/fsp_support.h>

+DECLARE_GLOBAL_DATA_PTR;

int print_cpuinfo(void) { post_code(POST_CPU_INFO); diff --git a/arch/x86/lib/lpc-uclass.c b/arch/x86/lib/lpc-uclass.c index 6aeb4d4..c6e8f73 100644 --- a/arch/x86/lib/lpc-uclass.c +++ b/arch/x86/lib/lpc-uclass.c @@ -9,6 +9,8 @@ #include <dm.h> #include <dm/root.h>

+DECLARE_GLOBAL_DATA_PTR;

static int lpc_uclass_post_bind(struct udevice *bus) { /* diff --git a/arch/x86/lib/pch-uclass.c b/arch/x86/lib/pch-uclass.c index d1082e1..20dfa81 100644 --- a/arch/x86/lib/pch-uclass.c +++ b/arch/x86/lib/pch-uclass.c @@ -9,6 +9,8 @@ #include <dm.h> #include <dm/root.h>

+DECLARE_GLOBAL_DATA_PTR;

static int pch_uclass_post_bind(struct udevice *bus) { /* diff --git a/drivers/gpio/intel_ich6_gpio.c b/drivers/gpio/intel_ich6_gpio.c index 8a108f3..cb408a4 100644 --- a/drivers/gpio/intel_ich6_gpio.c +++ b/drivers/gpio/intel_ich6_gpio.c @@ -35,6 +35,8 @@ #include <asm/io.h> #include <asm/pci.h>

+DECLARE_GLOBAL_DATA_PTR;

#define GPIO_PER_BANK 32

struct ich6_bank_priv {

Reviewed-by: Bin Meng bmeng.cn@gmail.com

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

Fix a typo, remove an unused field and make sure to use existing #define constants instead of open-coded values.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/cpu/interrupts.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/cpu/interrupts.c b/arch/x86/cpu/interrupts.c index 853c82f..3a9c2d4 100644 --- a/arch/x86/cpu/interrupts.c +++ b/arch/x86/cpu/interrupts.c @@ -19,6 +19,7 @@ #include <asm/processor-flags.h> #include <linux/compiler.h> #include <asm/msr.h> +#include <asm/processor.h> #include <asm/u-boot-x86.h> #include <asm/i8259.h>

@@ -46,7 +47,7 @@ static char *exceptions[] = { "Invalid TSS", "Segment Not Present", "Stack Segment Fault",

•   "Gerneral Protection",
  

•   "General Protection",
    "Page Fault",
    "Reserved",
    "x87 FPU Floating-Point Error",
  

@@ -165,7 +166,6 @@ struct idt_entry { struct desc_ptr { unsigned short size; unsigned long address;

•   unsigned short segment;
  

} __packed;

struct idt_entry idt[256] __aligned(16); @@ -202,14 +202,13 @@ int cpu_init_interrupts(void) for (i = 0; i < 256; i++) { idt[i].access = 0x8e; idt[i].res = 0;

•           idt[i].selector = 0x10;
  

•           idt[i].selector = X86_GDT_ENTRY_32BIT_CS * X86_GDT_ENTRY_SIZE;
            set_vector(i, irq_entry);
            irq_entry += irq_entry_size;
    }
  
  

•   idt_ptr.size = 256 * 8;
  

•   idt_ptr.size = 256 * 8 - 1;
    idt_ptr.address = (unsigned long) idt;
  

•   idt_ptr.segment = 0x18;
  
    load_idt(&idt_ptr);
  
  

--

Reviewed-by: Bin Meng bmeng.cn@gmail.com

Hi Bin,

On 23 July 2015 at 00:46, Bin Meng bmeng.cn@gmail.com wrote:

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

...

This option is not used by any board but appears to still be useful, at least for testing. With recent commits it does not build, so fix it.

It looks like CONFIG_PCI_ENUM_ONLY does similar things as CONFIG_PCI_PNP. Can we just use CONFIG_PCI_PNP?

My understanding is that:

- CONFIG_PCI_PNP enables auto-configuration of devices - CONFIG_PCI_ENUM_ONLY changes the auto-config code to just scan and print out what is there, not change it

So I believe both are useful, even if the current code structure could be improved...

...

Signed-off-by: Simon Glass sjg@chromium.org

drivers/pci/pci_auto.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c index 7ff282b..51d2ac0 100644 --- a/drivers/pci/pci_auto.c +++ b/drivers/pci/pci_auto.c @@ -82,9 +82,9 @@ void pciauto_setup_device(struct pci_controller *hose, pci_size_t bar_size; u16 cmdstat = 0; int bar, bar_nr = 0; +#ifndef CONFIG_PCI_ENUM_ONLY u8 header_type; int rom_addr; -#ifndef CONFIG_PCI_ENUM_ONLY pci_addr_t bar_value; struct pci_region *bar_res; int found_mem64 = 0; @@ -181,6 +181,7 @@ void pciauto_setup_device(struct pci_controller *hose, bar_nr++; }

+#ifndef CONFIG_PCI_ENUM_ONLY /* Configure the expansion ROM address */ pci_hose_read_config_byte(hose, dev, PCI_HEADER_TYPE, &header_type); if (header_type != PCI_HEADER_TYPE_CARDBUS) { @@ -201,6 +202,7 @@ void pciauto_setup_device(struct pci_controller *hose, debug("\n"); } } +#endif

    pci_hose_write_config_word(hose, dev, PCI_COMMAND, cmdstat);
    pci_hose_write_config_byte(hose, dev, PCI_CACHE_LINE_SIZE,

--

Regards, Bin

Regards, Simon

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

From: Ben Stoltz stoltz@google.com

When U-Boot runs as an EFI application is does not have a definition of CONFIG_SYS_TEXT_BASE. U-Boot is a relocatable application and the relocation is done by EFI. U-Boot can be loaded at any address.

This is similar to how sandbox works. Adjust the early board init to deal with this.

Signed-off-by: Ben Stoltz stoltz@google.com Signed-off-by: Simon Glass sjg@chromium.org

---

common/board_f.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c index 5e09c5f..ad5500f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -144,7 +144,7 @@ static int init_baud_rate(void)

static int display_text_info(void) { -#ifndef CONFIG_SANDBOX +#if !defined(CONFIG_SANDBOX) && !defined(CONFIG_ARCH_EFI)

I think this patch should come after CONFIG_ARCH_EFI is introduced. And per your commit message, I think here CONFIG_ARCH_EFI means EFI application. ARCH_xxx is confusing. Can we use the name CONFIG_EFI_APP?

    ulong bss_start, bss_end, text_base;

    bss_start = (ulong)&__bss_start;

@@ -267,7 +267,7 @@ static int setup_mon_len(void) { #if defined(__ARM__) || defined(__MICROBLAZE__) gd->mon_len = (ulong)&__bss_end - (ulong)_start; -#elif defined(CONFIG_SANDBOX) +#elif defined(CONFIG_SANDBOX) || defined(CONFIG_ARCH_EFI) gd->mon_len = (ulong)&_end - (ulong)_init; #elif defined(CONFIG_BLACKFIN) || defined(CONFIG_NIOS2) gd->mon_len = CONFIG_SYS_MONITOR_LEN; @@ -975,7 +975,8 @@ void board_init_f(ulong boot_flags) if (initcall_run_list(init_sequence_f)) hang();

-#if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) +#if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \

•           !defined(CONFIG_ARCH_EFI)
    /* NOTREACHED - jump_to_copy() does not return */
    hang();
  

#endif

Regards, Bin

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

When running as an EFI application, U-Boot must request memory from EFI, and provide access to the boot services U-Boot needs.

Add library code to perform these tasks. This includes efi_main() which is the entry point from EFI. U-Boot is built as a shared library.

Signed-off-by: Simon Glass sjg@chromium.org

arch/x86/include/asm/fsp/fsp_hob.h | 59 +----- include/efi.h | 356 +++++++++++++++++++++++++++++++++++++ include/efi_api.h | 252 ++++++++++++++++++++++++++ include/part_efi.h | 9 +- lib/Kconfig | 2 + lib/Makefile | 1 + lib/efi/Kconfig | 33 ++++ lib/efi/Makefile | 7 + lib/efi/efi.c | 91 ++++++++++ lib/efi/efi_app.c | 125 +++++++++++++ 10 files changed, 871 insertions(+), 64 deletions(-) create mode 100644 include/efi.h create mode 100644 include/efi_api.h create mode 100644 lib/efi/Kconfig create mode 100644 lib/efi/Makefile create mode 100644 lib/efi/efi.c create mode 100644 lib/efi/efi_app.c

diff --git a/arch/x86/include/asm/fsp/fsp_hob.h b/arch/x86/include/asm/fsp/fsp_hob.h index 6cca7f5..3fb3546 100644 --- a/arch/x86/include/asm/fsp/fsp_hob.h +++ b/arch/x86/include/asm/fsp/fsp_hob.h @@ -8,6 +8,8 @@ #ifndef __FSP_HOB_H__ #define __FSP_HOB_H__

+#include <efi.h>

/* Type of HOB Header */ #define HOB_TYPE_MEM_ALLOC 0x0002 #define HOB_TYPE_RES_DESC 0x0003 @@ -25,63 +27,6 @@ struct hob_header { u32 reserved; /* always zero */ };

-/* Enumeration of memory types introduced in UEFI */ -enum efi_mem_type {

•   EFI_RESERVED_MEMORY_TYPE,
  

•   /*
  

•    * The code portions of a loaded application.
  

•    * (Note that UEFI OS loaders are UEFI applications.)
  

•    */
  

•   EFI_LOADER_CODE,
  

•   /*
  

•    * The data portions of a loaded application and
  

•    * the default data allocation type used by an application
  

•    * to allocate pool memory.
  

•    */
  

•   EFI_LOADER_DATA,
  

•   /* The code portions of a loaded Boot Services Driver */
  

•   EFI_BOOT_SERVICES_CODE,
  

•   /*
  

•    * The data portions of a loaded Boot Serves Driver and
  

•    * the default data allocation type used by a Boot Services
  

•    * Driver to allocate pool memory.
  

•    */
  

•   EFI_BOOT_SERVICES_DATA,
  

•   /* The code portions of a loaded Runtime Services Driver */
  

•   EFI_RUNTIME_SERVICES_CODE,
  

•   /*
  

•    * The data portions of a loaded Runtime Services Driver and
  

•    * the default data allocation type used by a Runtime Services
  

•    * Driver to allocate pool memory.
  

•    */
  

•   EFI_RUNTIME_SERVICES_DATA,
  

•   /* Free (unallocated) memory */
  

•   EFI_CONVENTIONAL_MEMORY,
  

•   /* Memory in which errors have been detected */
  

•   EFI_UNUSABLE_MEMORY,
  

•   /* Memory that holds the ACPI tables */
  

•   EFI_ACPI_RECLAIM_MEMORY,
  

•   /* Address space reserved for use by the firmware */
  

•   EFI_ACPI_MEMORY_NVS,
  

•   /*
  

•    * Used by system firmware to request that a memory-mapped IO region
  

•    * be mapped by the OS to a virtual address so it can be accessed by
  

•    * EFI runtime services.
  

•    */
  

•   EFI_MMAP_IO,
  

•   /*
  

•    * System memory-mapped IO region that is used to translate
  

•    * memory cycles to IO cycles by the processor.
  

•    */
  

•   EFI_MMAP_IO_PORT,
  

•   /*
  

•    * Address space reserved by the firmware for code that is
  

•    * part of the processor.
  

•    */
  

•   EFI_PAL_CODE,
  

•   EFI_MAX_MEMORY_TYPE
  

-};

/*

• Describes all memory ranges used during the HOB producer phase that
• exist outside the HOB list. This HOB type describes how memory is used,

diff --git a/include/efi.h b/include/efi.h new file mode 100644 index 0000000..66ef6c3 --- /dev/null +++ b/include/efi.h @@ -0,0 +1,356 @@ +/*

• • Extensible Firmware Interface
• • Based on 'Extensible Firmware Interface Specification' version 0.9,
• • April 30, 1999
• • Copyright (C) 1999 VA Linux Systems
• • Copyright (C) 1999 Walt Drummond drummond@valinux.com
• • Copyright (C) 1999, 2002-2003 Hewlett-Packard Co.

• • David Mosberger-Tang <davidm@hpl.hp.com>
    

• • Stephane Eranian <eranian@hpl.hp.com>
    

• • From include/linux/efi.h in kernel 4.1 with some additions/subtractions
• */

+#ifndef _EFI_H +#define _EFI_H

+#include <linux/string.h> +#include <linux/types.h>

+#ifdef CONFIG_EFI_STUB_64BIT

I believe this CONFIG_EFI_STUB_64BIT is introduced in later patches, can we remove this to later patch where you add this CONFIG_EFI_STUB_64BIT?

+#define EFIAPI __attribute__((ms_abi))

This is worth a comment block to describe the fact that EFI 64-bit is using M$ ABI which is different from ours.

+#else +#define EFIAPI +#endif

+struct efi_device_path;

+#define EFI_SUCCESS 0 +#define EFI_LOAD_ERROR (1 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_INVALID_PARAMETER (2 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_UNSUPPORTED (3 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_BAD_BUFFER_SIZE (4 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_BUFFER_TOO_SMALL (5 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_NOT_READY (6 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_DEVICE_ERROR (7 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_WRITE_PROTECTED (8 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_OUT_OF_RESOURCES (9 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_NOT_FOUND (14 | (1UL << (BITS_PER_LONG - 1))) +#define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG - 1)))

Can we also remove those duplicated defines in arch/x86/include/asm/fsp/fsp_types.h so that FSP codes can use the one in efi.h?

+typedef unsigned long efi_status_t; +typedef u64 efi_physical_addr_t; +typedef u64 efi_virtual_addr_t; +typedef void *efi_handle_t;

+#define EFI_GUID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \

•   ((efi_guid_t) \
  

•   {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, \
  

•           ((a) >> 24) & 0xff, \
  

•           (b) & 0xff, ((b) >> 8) & 0xff, \
  

•           (c) & 0xff, ((c) >> 8) & 0xff, \
  

•           (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) } })
  

+/*

• • Generic EFI table header
• */

Please use one-line comment.

+struct efi_table_hdr {

•   u64 signature;
  

•   u32 revision;
  

•   u32 headersize;
  

•   u32 crc32;
  

•   u32 reserved;
  

+};

+/* Enumeration of memory types introduced in UEFI */ +enum efi_mem_type {

•   EFI_RESERVED_MEMORY_TYPE,
  

•   /*
  

•    * The code portions of a loaded application.
  

•    * (Note that UEFI OS loaders are UEFI applications.)
  

•    */
  

•   EFI_LOADER_CODE,
  

•   /*
  

•    * The data portions of a loaded application and
  

•    * the default data allocation type used by an application
  

•    * to allocate pool memory.
  

•    */
  

•   EFI_LOADER_DATA,
  

•   /* The code portions of a loaded Boot Services Driver */
  

•   EFI_BOOT_SERVICES_CODE,
  

•   /*
  

•    * The data portions of a loaded Boot Serves Driver and
  

•    * the default data allocation type used by a Boot Services
  

•    * Driver to allocate pool memory.
  

•    */
  

•   EFI_BOOT_SERVICES_DATA,
  

•   /* The code portions of a loaded Runtime Services Driver */
  

•   EFI_RUNTIME_SERVICES_CODE,
  

•   /*
  

•    * The data portions of a loaded Runtime Services Driver and
  

•    * the default data allocation type used by a Runtime Services
  

•    * Driver to allocate pool memory.
  

•    */
  

•   EFI_RUNTIME_SERVICES_DATA,
  

•   /* Free (unallocated) memory */
  

•   EFI_CONVENTIONAL_MEMORY,
  

•   /* Memory in which errors have been detected */
  

•   EFI_UNUSABLE_MEMORY,
  

•   /* Memory that holds the ACPI tables */
  

•   EFI_ACPI_RECLAIM_MEMORY,
  

•   /* Address space reserved for use by the firmware */
  

•   EFI_ACPI_MEMORY_NVS,
  

•   /*
  

•    * Used by system firmware to request that a memory-mapped IO region
  

•    * be mapped by the OS to a virtual address so it can be accessed by
  

•    * EFI runtime services.
  

•    */
  

•   EFI_MMAP_IO,
  

•   /*
  

•    * System memory-mapped IO region that is used to translate
  

•    * memory cycles to IO cycles by the processor.
  

•    */
  

•   EFI_MMAP_IO_PORT,
  

•   /*
  

•    * Address space reserved by the firmware for code that is
  

•    * part of the processor.
  

•    */
  

•   EFI_PAL_CODE,
  

•   EFI_MAX_MEMORY_TYPE,
  

•   EFI_TABLE_END,  /* For efi_build_mem_table() */
  

+};

+/* Attribute values */ +enum {

•   EFI_MEMORY_UC_SHIFT     = 0,    /* uncached */
  

•   EFI_MEMORY_WC_SHIFT     = 1,    /* write-coalescing */
  

•   EFI_MEMORY_WT_SHIFT     = 2,    /* write-through */
  

•   EFI_MEMORY_WB_SHIFT     = 3,    /* write-back */
  

•   EFI_MEMORY_UCE_SHIFT    = 4,    /* uncached, exported */
  

•   EFI_MEMORY_WP_SHIFT     = 12,   /* write-protect */
  

•   EFI_MEMORY_RP_SHIFT     = 13,   /* read-protect */
  

•   EFI_MEMORY_XP_SHIFT     = 14,   /* execute-protect */
  

•   EFI_MEMORY_RUNTIME_SHIFT = 63,  /* range requires runtime mapping */
  

•   EFI_MEMORY_RUNTIME = 1ULL << EFI_MEMORY_RUNTIME_SHIFT,
  

•   efi_mem_desc_VERSION    = 1,
  

What is efi_mem_desc_VERSION? The name should be all capital letters.

+};

+#define EFI_PAGE_SHIFT 12 +#define EFI_PAGE_SIZE (1UL << EFI_PAGE_SHIFT)

+struct efi_mem_desc {

•   u32 type;
  

•   u32 reserved;
  

•   efi_physical_addr_t physical_start;
  

•   efi_virtual_addr_t virtual_start;
  

•   u64 num_pages;
  

•   u64 attribute;
  

+};

+/*

• • Allocation types for calls to boottime->allocate_pages.
• */

Please use one-line comment.

+#define EFI_ALLOCATE_ANY_PAGES 0 +#define EFI_ALLOCATE_MAX_ADDRESS 1 +#define EFI_ALLOCATE_ADDRESS 2 +#define EFI_MAX_ALLOCATE_TYPE 3

+/*

• • Types and defines for Time Services
• */

Please use one-line comment.

+#define EFI_TIME_ADJUST_DAYLIGHT 0x1 +#define EFI_TIME_IN_DAYLIGHT 0x2 +#define EFI_UNSPECIFIED_TIMEZONE 0x07ff

+struct efi_time {

•   u16 year;
  

•   u8 month;
  

•   u8 day;
  

•   u8 hour;
  

•   u8 minute;
  

•   u8 second;
  

•   u8 pad1;
  

•   u32 nanosecond;
  

•   s16 timezone;
  

•   u8 daylight;
  

•   u8 pad2;
  

+};

+struct efi_time_cap {

•   u32 resolution;
  

•   u32 accuracy;
  

•   u8 sets_to_zero;
  

+};

+enum efi_locate_search_type {

•   all_handles,
  

•   by_register_notify,
  

•   by_protocol
  

+};

+struct efi_open_protocol_info_entry {

•   efi_handle_t agent_handle;
  

•   efi_handle_t controller_handle;
  

•   u32 attributes;
  

•   u32 open_count;
  

+};

+enum efi_entry_t {

•   EFIET_END,      /* Signals this is the last (empty) entry */
  

•   EFIET_MEMORY_MAP,
  

Please remove this blank line.

•   EFIET_MEMORY_COUNT,
  

+};

+#define EFI_TABLE_VERSION 1

+/**

• • struct efi_info_hdr - Header for the EFI info table
• • @version: EFI_TABLE_VERSION
• • @hdr_size: Size of this struct in bytes

Missing description for total_size and spare[5]

• */

+struct efi_info_hdr {

•   u32 version;
  

•   u32 hdr_size;
  

•   u32 total_size;
  

•   u32 spare[5];
  

+};

+/**

• • struct efi_entry_hdr - Header for a table entry
• • @type: enum eft_entry_t
• • @size size of entry bytes excluding header and padding
• • @addr: address of this entry (0 if it follows the header )
• • @link: size of entry including header and padding
• • @

Missing description for spare1 and spare2.

• */

+struct efi_entry_hdr {

•   u32 type;
  

•   u32 size;
  

•   u64 addr;
  

•   u32 link;
  

•   u32 spare1;
  

•   u64 spare22;
  

Why 22? Should it be spare2?

+};

+struct efi_entry_memmap {

Can we add comment block for this structure too?

•   u32 version;
  

•   u32 desc_size;
  

•   u64 spare;
  

•   struct efi_mem_desc desc[];
  

Nits: should it be "struct efi_mem_desc *desc"?

+};

+static inline struct efi_mem_desc *efi_get_next_mem_desc(

•           struct efi_entry_memmap *map, struct efi_mem_desc *desc)
  

+{

•   return (struct efi_mem_desc *)((ulong)desc + map->desc_size);
  

+}

+struct efi_priv {

•   efi_handle_t parent_image;
  

•   struct efi_device_path *device_path;
  

•   struct efi_system_table *sys_table;
  

•   struct efi_boot_services *boot;
  

•   struct efi_runtime_services *run;
  

•   bool use_pool_for_malloc;
  

•   unsigned long ram_base;
  

•   unsigned int image_data_type;
  

•   struct efi_info_hdr *info;
  

•   unsigned int info_size;
  

•   void *next_hdr;
  

+};

+/* Base address of the EFI image */ +extern char ImageBase[];

Can we avoid CamelCase?

+/**

• • efi_get_sys_table() - Get access to the main EFI system table
• • @return pointer to EFI system table
• */

+struct efi_system_table *efi_get_sys_table(void);

+/**

• • efi_get_ram_base() - Find the base of RAM
• • This is used when U-Boot is built as an EFI application.
• • @return the base of RAM as know to U-Boot

as 'known'?

• */

+unsigned long efi_get_ram_base(void);

+/**

• • efi_init() - Set up ready for use of EFI boot services
• • @priv: Pointer to our private EFI structure to fill in
• • @banner: Banner to display when starting
• • @image: The image handle passed to efi_main()
• • @sys_table: The EFI system table pointer passed to efi_main()
• */

+int efi_init(struct efi_priv *priv, const char *banner, efi_handle_t image,

•        struct efi_system_table *sys_table);
  

+/**

• • efi_malloc() - Allocate some memory from EFI
• • @priv: Pointer to private EFI structure
• • @size: Number of bytes to allocate
• • @retp: Return EFI status resuilt

Typo of 'result'

• • @return pointer to memory allocated, or NULL on error
• */

+void *efi_malloc(struct efi_priv *priv, int size, efi_status_t *retp);

+/**

• • efi_free() - Free memory allocated from EFI
• • @priv: Pointer to private EFI structure
• • @ptr: Pointer to memory to free
• */

+void efi_free(struct efi_priv *priv, void *ptr);

+/**

• • efi_puts() - Write out a string to the EFI console
• • @priv: Pointer to private EFI structure
• • @str: String to write (note this is ASCII, not unicode)
• */

+void efi_puts(struct efi_priv *priv, const char *str);

+/**

• • efi_putc() - Write out a character to the EFI console
• • @priv: Pointer to private EFI structure
• • @ch: Character to write (note this is ASCII, not unicode)

A character can only be ASCII.

• */

+void efi_putc(struct efi_priv *priv, const char ch);

+/**

• • efi_info_get() - get an entry from an EFI table
• • @type: Entry type to search for
• • @datap: Returns pointer to entry data
• • @sizep: Returns pointer to entry size
• • @return 0 if OK, -ENODATA if there is no table, -ENOENT if there is no entry
• • of the requested type, -EPROTONOSUPPORT if the table has the wrong version
• */

+int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);

+/**

• • efi_build_mem_table() - make a sorted copy of the memory table
• • @map: Pointer to EFI memory map table
• • @size: Size of table in bytes
• • @skip_bs: True to skip boot-time memory and merge it with conventional

• •         memory. This will significantly reduce the number of table
    

• •         entries.
    

• • @return pointer to the new table. It should be freed with free() by the

• •    caller
    

• */

+void *efi_build_mem_table(struct efi_entry_memmap *map, int size, bool skip_bs);

+#endif /* _LINUX_EFI_H */ diff --git a/include/efi_api.h b/include/efi_api.h new file mode 100644 index 0000000..d4528f6 --- /dev/null +++ b/include/efi_api.h @@ -0,0 +1,252 @@ +/*

• • Extensible Firmware Interface
• • Based on 'Extensible Firmware Interface Specification' version 0.9,
• • April 30, 1999
• • Copyright (C) 1999 VA Linux Systems
• • Copyright (C) 1999 Walt Drummond drummond@valinux.com
• • Copyright (C) 1999, 2002-2003 Hewlett-Packard Co.

• • David Mosberger-Tang <davidm@hpl.hp.com>
    

• • Stephane Eranian <eranian@hpl.hp.com>
    

• • From include/linux/efi.h in kernel 4.1 with some additions/subtractions
• */

+#ifndef _EFI_API_H +#define _EFI_API_H

+#include <efi.h>

+/*

• • EFI Boot Services table
• */

Please use one-line comment.

+struct efi_boot_services {

•   struct efi_table_hdr hdr;
  

•   void *raise_tpl;
  

•   void *restore_tpl;
  

•   efi_status_t (EFIAPI *allocate_pages)(int, int, unsigned long,
  

•                                         efi_physical_addr_t *);
  

•   efi_status_t (EFIAPI *free_pages)(efi_physical_addr_t, unsigned long);
  

•   efi_status_t (EFIAPI *get_memory_map)(unsigned long *memory_map_size,
  

•                   struct efi_mem_desc *desc, unsigned long *key,
  

•                   unsigned long *desc_size, u32 *desc_version);
  

•   efi_status_t (EFIAPI *allocate_pool)(int, unsigned long, void **);
  

•   efi_status_t (EFIAPI *free_pool)(void *);
  

•   void *create_event;
  

•   void *set_timer;
  

•   efi_status_t(EFIAPI *wait_for_event)(unsigned long number_of_events,
  

•                                        void *event, unsigned long *index);
  

•   void *signal_event;
  

•   void *close_event;
  

•   void *check_event;
  

•   void *install_protocol_interface;
  

•   void *reinstall_protocol_interface;
  

•   void *uninstall_protocol_interface;
  

•   efi_status_t (EFIAPI *handle_protocol)(efi_handle_t, efi_guid_t *,
  

•                                          void **);
  

•   void *__reserved;
  

Can it be just 'reserved'?

•   void *register_protocol_notify;
  

•   efi_status_t (EFIAPI *locate_handle)(
  

•                   enum efi_locate_search_type search_type,
  

•                   efi_guid_t *protocol, void *search_key,
  

•                   unsigned long *buffer_size, efi_handle_t *buffer);
  

•   efi_status_t (EFIAPI *locate_device_path)(efi_guid_t *protocol,
  

•                   struct efi_device_path **device_path,
  

•                   efi_handle_t *device);
  

•   void *install_configuration_table;
  

•   efi_status_t (EFIAPI *load_image)(bool boot_policiy,
  

•                   efi_handle_t parent_image,
  

•                   struct efi_device_path *file_path, void *source_buffer,
  

•                   unsigned long source_size, efi_handle_t *image);
  

•   efi_status_t (EFIAPI *start_image)(efi_handle_t handle,
  

•                                      unsigned long *exitdata_size,
  

•                                      s16 **exitdata);
  

•   efi_status_t (EFIAPI *exit)(efi_handle_t handle,
  

•                               efi_status_t exit_status,
  

•                               unsigned long exitdata_size, s16 *exitdata);
  

•   void *unload_image;
  

•   efi_status_t (EFIAPI *exit_boot_services)(efi_handle_t, unsigned long);
  

•   efi_status_t (EFIAPI *get_next_monotonic_count)(u64 *count);
  

•   efi_status_t (EFIAPI *stall)(unsigned long usecs);
  

•   void *set_watchdog_timer;
  

•   efi_status_t(EFIAPI *connect_controller)(efi_handle_t controller_handle,
  

•                   efi_handle_t *driver_image_handle,
  

•                   struct efi_device_path *remaining_device_path,
  

•                   bool recursive);
  

•   void *disconnect_controller;
  

+#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL 0x00000001 +#define EFI_OPEN_PROTOCOL_GET_PROTOCOL 0x00000002 +#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL 0x00000004 +#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008 +#define EFI_OPEN_PROTOCOL_BY_DRIVER 0x00000010 +#define EFI_OPEN_PROTOCOL_EXCLUSIVE 0x00000020

Can we move these to efi.h?

•   efi_status_t (EFIAPI *open_protocol)(efi_handle_t handle,
  

•                   efi_guid_t *protocol, void **interface,
  

•                   efi_handle_t agent_handle,
  

•                   efi_handle_t controller_handle, u32 attributes);
  

•   void *close_protocol;
  

•   efi_status_t(EFIAPI *open_protocol_information)(efi_handle_t handle,
  

•                   efi_guid_t *protocol,
  

•                   struct efi_open_protocol_info_entry **entry_buffer,
  

•                   unsigned long *entry_count);
  

•   efi_status_t (EFIAPI *protocols_per_handle)(efi_handle_t handle,
  

•                   efi_guid_t ***protocol_buffer,
  

•                   unsigned long *protocols_buffer_count);
  

•   efi_status_t (EFIAPI *locate_handle_buffer) (
  

•                   enum efi_locate_search_type search_type,
  

•                   efi_guid_t *protocol, void *search_key,
  

•                   unsigned long *no_handles, efi_handle_t **buffer);
  

•   void *locate_protocol;
  

•   void *install_multiple_protocol_interfaces;
  

•   void *uninstall_multiple_protocol_interfaces;
  

•   void *calculate_crc32;
  

•   void *copy_mem;
  

•   void *set_mem;
  

•   void *create_event_ex;
  

+};

+/*

• • Types and defines for EFI ResetSystem
• */

Please use one-line comment.

+enum efi_reset_type {

•   EFI_RESET_COLD = 0,
  

•   EFI_RESET_WARM = 1,
  

•   EFI_RESET_SHUTDOWN = 2
  

+};

Nits: can we move this to efi.h?

+/*

• • EFI Runtime Services table
• */

Please use one-line comment.

+#define EFI_RUNTIME_SERVICES_SIGNATURE ((u64)0x5652453544e5552ULL)

Is the (u64) cast necessary?

+#define EFI_RUNTIME_SERVICES_REVISION 0x00010000

Nits: can we move the above two to efi.h?

+struct efi_runtime_services {

•   struct efi_table_hdr hdr;
  

•   void *get_time;
  

•   void *set_time;
  

•   void *get_wakeup_time;
  

•   void *set_wakeup_time;
  

•   void *set_virtual_address_map;
  

•   void *convert_pointer;
  

•   efi_status_t (EFIAPI *get_variable)(s16 *variable_name,
  

•                   efi_guid_t *vendor, u32 *attributes,
  

•                   unsigned long *data_size, void *data);
  

•   efi_status_t (EFIAPI *get_next_variable)(
  

•                   unsigned long *variable_name_size,
  

•                   s16 *variable_name, efi_guid_t *vendor);
  

•   efi_status_t (EFIAPI *set_variable)(s16 *variable_name,
  

•                   efi_guid_t *vendor, u32 attributes,
  

•                   unsigned long data_size, void *data);
  

•   void *get_next_high_mono_count;
  

•   void (EFIAPI *reset_system)(enum efi_reset_type reset_type,
  

•                               efi_status_t reset_status,
  

•                               unsigned long data_size, void *reset_data);
  

•   void *update_capsule;
  

•   void *query_capsule_caps;
  

•   void *query_variable_info;
  

+};

+/*

• • EFI Configuration Table and GUID definitions
• */

Please use one-line comment.

+#define NULL_GUID \

•   EFI_GUID(0x00000000, 0x0000, 0x0000, 0x00, 0x00, \
  

•            0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
  

+#define LOADED_IMAGE_PROTOCOL_GUID \

•   EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, 0x8e, 0x3f, \
  

•            0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
  

Nits: can we move the above two to efi.h?

+struct efi_system_table {

•   struct efi_table_hdr hdr;
  

•   unsigned long fw_vendor;   /* physical addr of CHAR16 vendor string */
  

What's CHAR16? Is it wchar_t?

•   u32 fw_revision;
  

•   unsigned long con_in_handle;
  

•   struct efi_simple_input_interface *con_in;
  

•   unsigned long con_out_handle;
  

•   struct efi_simple_text_output_protocol *con_out;
  

•   unsigned long stderr_handle;
  

•   unsigned long std_err;
  

•   struct efi_runtime_services *runtime;
  

•   struct efi_boot_services *boottime;
  

•   unsigned long nr_tables;
  

•   unsigned long tables;
  

+};

+struct efi_loaded_image {

•   u32 revision;
  

•   void *parent_handle;
  

•   struct efi_system_table *system_table;
  

•   void *device_handle;
  

•   void *file_path;
  

•   void *reserved;
  

•   u32 load_options_size;
  

•   void *load_options;
  

•   void *image_base;
  

•   aligned_u64 image_size;
  

•   unsigned int image_code_type;
  

•   unsigned int image_data_type;
  

•   unsigned long unload;
  

+};

+struct __packed efi_device_path {

__packed is not needed here.

•   u8 type;
  

•   u8 sub_type;
  

•   u16 length;
  

+};

+struct simple_text_output_mode {

•   s32 max_mode;
  

•   s32 mode;
  

•   s32 attribute;
  

•   s32 cursor_column;
  

•   s32 cursor_row;
  

•   bool cursor_visible;
  

+};

+struct efi_simple_text_output_protocol {

•   void *reset;
  

•   efi_status_t (EFIAPI *output_string)(
  

•                   struct efi_simple_text_output_protocol *this,
  

•                   const unsigned short *str);
  

•   void *test_string;
  

•   efi_status_t(EFIAPI *query_mode)(
  

•                   struct efi_simple_text_output_protocol *this,
  

•                   unsigned long mode_number, unsigned long *columns,
  

•                   unsigned long *rows);
  

•   efi_status_t(EFIAPI *set_mode)(
  

•                   struct efi_simple_text_output_protocol *this,
  

•                   unsigned long mode_number);
  

•   efi_status_t(EFIAPI *set_attribute)(
  

•                   struct efi_simple_text_output_protocol *this,
  

•                   unsigned long attribute);
  

•   efi_status_t(EFIAPI *clear_screen) (
  

•                   struct efi_simple_text_output_protocol *this);
  

•   efi_status_t(EFIAPI *set_cursor_position) (
  

•                   struct efi_simple_text_output_protocol *this,
  

•                   unsigned long column, unsigned long row);
  

•   efi_status_t(EFIAPI *enable_cursor)(void *, bool enable);
  

•   struct simple_text_output_mode *mode;
  

+};

+struct efi_input_key {

•   u16 scan_code;
  

•   s16 unicode_char;
  

+};

+struct efi_simple_input_interface {

•   efi_status_t(EFIAPI *reset)(struct efi_simple_input_interface *this,
  

•                   bool ExtendedVerification);
  

•   efi_status_t(EFIAPI *read_key_stroke)(
  

•                   struct efi_simple_input_interface *this,
  

•                   struct efi_input_key *key);
  

•   void *wait_for_key;
  

+};

+#endif diff --git a/include/part_efi.h b/include/part_efi.h index d68ef3b..3012b91 100644 --- a/include/part_efi.h +++ b/include/part_efi.h @@ -18,6 +18,8 @@ #ifndef _DISK_PART_EFI_H #define _DISK_PART_EFI_H

+#include <efi.h>

#define MSDOS_MBR_SIGNATURE 0xAA55 #define EFI_PMBR_OSTYPE_EFI 0xEF #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE @@ -29,13 +31,6 @@ #define GPT_ENTRY_NUMBERS 128 #define GPT_ENTRY_SIZE 128

-#define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \

•   ((efi_guid_t) \
  

•   {{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
  

•           (b) & 0xff, ((b) >> 8) & 0xff, \
  

•           (c) & 0xff, ((c) >> 8) & 0xff, \
  

•           (d0), (d1), (d2), (d3), (d4), (d5), (d6), (d7) }})
  

#define PARTITION_SYSTEM_GUID \ EFI_GUID( 0xC12A7328, 0xF81F, 0x11d2, \ 0xBA, 0x4B, 0x00, 0xA0, 0xC9, 0x3E, 0xC9, 0x3B) diff --git a/lib/Kconfig b/lib/Kconfig index c98d399..4c8d38e 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -96,4 +96,6 @@ config ERRNO_STR - if errno is null or positive number - a pointer to "Success" message - if errno is negative - a pointer to errno related message

+source lib/efi/Kconfig

endmenu diff --git a/lib/Makefile b/lib/Makefile index 97ed398..4821779 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -7,6 +7,7 @@

ifndef CONFIG_SPL_BUILD

+obj-$(CONFIG_EFI) += efi/ obj-$(CONFIG_RSA) += rsa/ obj-$(CONFIG_LZMA) += lzma/ obj-$(CONFIG_LZO) += lzo/ diff --git a/lib/efi/Kconfig b/lib/efi/Kconfig new file mode 100644 index 0000000..2b3dbd4 --- /dev/null +++ b/lib/efi/Kconfig @@ -0,0 +1,33 @@ +config EFI

•   bool "Support running U-Boot from EFI"
  

•   depends on X86
  

•   help
  

•     U-Boot can be started from EFI on certain platforms. This allows
  

•     EFI to perform most of the system init and then jump to U-Boot for
  

•     final system boot. Another option is to run U-Boot as an EFI
  

•     application, with U-Boot using EFI's drivers instead of its own.
  

+choice

•   prompt "Select EFI mode to use"
  

•   depends on X86 && EFI
  

+config ARCH_EFI

Can we change this to EFI_APP?

•   bool "Support running U-Boot from EFI"
  

Nits: better to be "Support running as an EFI application".

•   help
  

•     Build U-Boot as an application which can be started from EFI. This
  

•     is useful for examining a platform in the early stages of porting
  

•     U-Boot to it. It allows only very basic functionality, such as a
  

•     command problem and memory and I/O functions. Use 'reset' to return
  

I don't understand what is 'a command problem'.

•     to EFI.
  

+config EFI_RAM_SIZE

•   hex "Amount of EFI RAM for U-Boot"
  

•   depends on ARCH_EFI
  

•   default 0x2000000
  

•   help
  

•     Set the amount of EFI RAM which is claimed by U-Boot for its own
  

•     use. U-Boot allocates this from EFI on start-up (along with a few
  

•     other smaller amounts) and it can never be increased after that.
  

•     It is used as the RAM size in withU-Boot.
  

Missing space between 'with' and 'U-Boot'

+endchoice diff --git a/lib/efi/Makefile b/lib/efi/Makefile new file mode 100644 index 0000000..137d2f9 --- /dev/null +++ b/lib/efi/Makefile @@ -0,0 +1,7 @@ +# +# (C) Copyright 2015 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +#

+obj-$(CONFIG_ARCH_EFI) += efi_app.o efi.o diff --git a/lib/efi/efi.c b/lib/efi/efi.c new file mode 100644 index 0000000..acd276c --- /dev/null +++ b/lib/efi/efi.c @@ -0,0 +1,91 @@ +/*

• • Copyright (c) 2015 Google, Inc
• • SPDX-License-Identifier: GPL-2.0+
• • EFI information obtained here:
• • http://wiki.phoenix.com/wiki/index.php/EFI_BOOT_SERVICES
• • Common EFI functions
• */

+#include <common.h> +#include <debug_uart.h> +#include <errno.h> +#include <linux/err.h> +#include <linux/types.h> +#include <efi.h> +#include <efi_api.h>

+DECLARE_GLOBAL_DATA_PTR;

+static void efi_memset(void *ptr, int ch, int size) +{

•   char *dest = ptr;
  

•   while (size-- > 0)
  

•           *dest++ = ch;
  

+}

Why do we need this? Can we use memset() here?

+void efi_putc(struct efi_priv *priv, const char ch) +{

•   struct efi_simple_text_output_protocol *con = priv->sys_table->con_out;
  

•   uint16_t ucode[2];
  

•   ucode[0] = ch;
  

•   ucode[1] = '\0';
  

•   con->output_string(con, ucode);
  

+}

+void efi_puts(struct efi_priv *priv, const char *str) +{

•   while (*str)
  

•           efi_putc(priv, *str++);
  

+}

+int efi_init(struct efi_priv *priv, const char *banner, efi_handle_t image,

•        struct efi_system_table *sys_table)
  

+{

•   efi_guid_t loaded_image_guid = LOADED_IMAGE_PROTOCOL_GUID;
  

•   struct efi_boot_services *boot = sys_table->boottime;
  

•   struct efi_loaded_image *loaded_image;
  

•   int ret;
  

•   efi_memset(priv, '\0', sizeof(*priv));
  

•   priv->sys_table = sys_table;
  

•   priv->boot = sys_table->boottime;
  

•   priv->parent_image = image;
  

•   priv->run = sys_table->runtime;
  

•   efi_puts(priv, "U-Boot EFI ");
  

•   efi_puts(priv, banner);
  

•   efi_putc(priv, ' ');
  

•   ret = boot->open_protocol(priv->parent_image, &loaded_image_guid,
  

•                             (void **)&loaded_image, &priv->parent_image,
  

•                             NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

•   if (ret) {
  

•           efi_puts(priv, "Failed to get loaded image protocol\n");
  

•           return ret;
  

•   }
  

•   priv->image_data_type = loaded_image->image_data_type;
  

•   return 0;
  

+}

+void *efi_malloc(struct efi_priv *priv, int size, efi_status_t *retp) +{

•   struct efi_boot_services *boot = priv->boot;
  

•   void *buf = NULL;
  

•   *retp = boot->allocate_pool(priv->image_data_type, size, &buf);
  

•   return buf;
  

+}

+void efi_free(struct efi_priv *priv, void *ptr) +{

•   struct efi_boot_services *boot = priv->boot;
  

•   boot->free_pool(ptr);
  

+} diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c new file mode 100644 index 0000000..b71669c --- /dev/null +++ b/lib/efi/efi_app.c @@ -0,0 +1,125 @@ +/*

• • Copyright (c) 2015 Google, Inc
• • SPDX-License-Identifier: GPL-2.0+
• • EFI information obtained here:
• • http://wiki.phoenix.com/wiki/index.php/EFI_BOOT_SERVICES
• • This file implements U-Boot running as an EFI application.
• */

+#include <common.h> +#include <debug_uart.h> +#include <errno.h> +#include <linux/err.h> +#include <linux/types.h> +#include <efi.h> +#include <efi_api.h>

+DECLARE_GLOBAL_DATA_PTR;

+static struct efi_priv *global_priv;

+struct efi_system_table *efi_get_sys_table(void) +{

•   return global_priv->sys_table;
  

+}

+unsigned long efi_get_ram_base(void) +{

•   return global_priv->ram_base;
  

+}

+static efi_status_t setup_memory(struct efi_priv *priv) +{

•   struct efi_boot_services *boot = priv->boot;
  

•   efi_physical_addr_t addr;
  

•   efi_status_t ret;
  

•   int pages;
  

•   global_data_ptr = efi_malloc(priv, sizeof(struct global_data), &ret);
  

Where is this global_data_ptr defined? I guess this is introduced in later patches. The introduction of global_data_ptr should be moved to this patch.

•   if (!global_data_ptr)
  

•           return ret;
  

•   memset(gd, '\0', sizeof(*gd));
  

And gd here? If gd == global_data_ptr, why no just use gd without adding global_data_ptr?

•   gd->malloc_base = (ulong)efi_malloc(priv, CONFIG_SYS_MALLOC_F_LEN,
  

•                                       &ret);
  

•   if (!gd->malloc_base)
  

•           return ret;
  

•   pages = CONFIG_EFI_RAM_SIZE >> 12;
  

•   addr = 1ULL << 32;
  

Please add a comment to describe what we are trying to do here, eg: addr points to 4GiB, that means the allocated address should not exceed that.

•   ret = boot->allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
  

•                              priv->image_data_type, pages, &addr);
  

•   if (ret) {
  

•           printf("(using pool %lx) ", ret);
  

•           priv->ram_base = (ulong)efi_malloc(priv, CONFIG_EFI_RAM_SIZE,
  

•                                              &ret);
  

•           if (!priv->ram_base)
  

•                   return ret;
  

•           priv->use_pool_for_malloc = true;
  

•   } else {
  

•           priv->ram_base = addr;
  

•   }
  

•   gd->ram_size = pages << 12;
  

•   return 0;
  

+}

+static void free_memory(struct efi_priv *priv) +{

•   struct efi_boot_services *boot = priv->boot;
  

•   if (priv->use_pool_for_malloc)
  

•           efi_free(priv, (void *)priv->ram_base);
  

•   else
  

•           boot->free_pages(priv->ram_base, gd->ram_size >> 12);
  

•   efi_free(priv, (void *)gd->malloc_base);
  

•   efi_free(priv, gd);
  

•   global_data_ptr = NULL;
  

+}

+/**

• • efi_main() - Start an EFI image
• • This function is called by our EFI start-up code. It handles running
• • U-Boot. If it returns, EFI will continue. Another way to get back to EFI
• • is via reset_cpu().
• */

+efi_status_t efi_main(efi_handle_t image, struct efi_system_table *sys_table) +{

•   struct efi_priv local_priv, *priv = &local_priv;
  

•   efi_status_t ret;
  

•   /* Set up access to EFI data structures */
  

•   efi_init(priv, "App", image, sys_table);
  

•   global_priv = priv;
  

•   /* Set up the EFI debug UART so that printf() works */
  

•   debug_uart_init();
  

Where is this implemented for efi app?

•   ret = setup_memory(priv);
  

•   if (ret) {
  

•           printf("Failed to set up memory: ret=%lx\n", ret);
  

Is printf usable here? If yes, why do we create efi_puts() and efi_putc()?

•           return ret;
  

•   }
  

•   printf("starting\n");
  

•   board_init_f(GD_FLG_SKIP_RELOC);
  

•   board_init_r(NULL, 0);
  

•   free_memory(priv);
  

•   return EFI_SUCCESS;
  

+}

+void reset_cpu(ulong addr) +{

•   struct efi_priv *priv = global_priv;
  

•   free_memory(priv);
  

•   printf("U-Boot EFI exiting\n");
  

•   priv->boot->exit(priv->parent_image, EFI_SUCCESS, 0, NULL);
  

+}

Regards, Bin

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

With EFI the start address of U-Boot is specified differently. We could consider just setting GD_FLG_RELOC and then setting up reloc_off. But that flag has other implementations and we are not able to use U-Boot relocation which this flag implies.

Instead, just add a special case for EFI.

Signed-off-by: Simon Glass sjg@chromium.org

Please check previous review comments regarding to CONFIG_ARCH_EFI and ImageBase, otherwise:

Reviewed-by: Bin Meng bmeng.cn@gmail.com

lib/initcall.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/lib/initcall.c b/lib/initcall.c index 7142744..1931655 100644 --- a/lib/initcall.c +++ b/lib/initcall.c @@ -6,6 +6,7 @@

#include <common.h> #include <initcall.h> +#include <efi.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -19,6 +20,9 @@ int initcall_run_list(const init_fnc_t init_sequence[])

            if (gd->flags & GD_FLG_RELOC)
                    reloc_ofs = gd->reloc_off;

+#ifdef CONFIG_ARCH_EFI

•           reloc_ofs = (unsigned long)ImageBase;
  

+#endif debug("initcall: %p", (char *)*init_fnc_ptr - reloc_ofs); if (gd->flags & GD_FLG_RELOC) debug(" (relocated to %p)\n", (char *)*init_fnc_ptr); --

Regards, Bin

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

Add a serial driver which makes use of EFI's console in/out service.

Signed-off-by: Simon Glass sjg@chromium.org

drivers/serial/Kconfig | 8 +++ drivers/serial/Makefile | 1 + drivers/serial/serial_efi.c | 149 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 drivers/serial/serial_efi.c

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index 4829284..f874837 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -44,6 +44,14 @@ config DEBUG_UART_NS16550 will need to provide parameters to make this work. The driver will be available until the real driver model serial is running.

+config DEBUG_UART_EFI

This driver looks like a normal serial driver, except one debug uart api _debug_uart_putc(). Also it can not only support UART, but also support any console input/output devices that EFI supports. Should we rename it to: EFI_CONSOLE?

•   bool "EFI"
  

depends on EFI_APP?

•   help
  

•     Select this to enable a debug UART which calls back to EFI to output
  

•     to the console. This can be useful for early debugging of U-Boot
  

•     when running on top of EFI (Extensive Firmware Interface). This is
  

•     a type of BIOS used by PCs.
  

endchoice

config DEBUG_UART_BASE diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile index d183eed..92c3808 100644 --- a/drivers/serial/Makefile +++ b/drivers/serial/Makefile @@ -21,6 +21,7 @@ obj-$(CONFIG_ALTERA_JTAG_UART) += altera_jtag_uart.o obj-$(CONFIG_ARM_DCC) += arm_dcc.o obj-$(CONFIG_ATMEL_USART) += atmel_usart.o obj-$(CONFIG_DW_SERIAL) += serial_dw.o +obj-$(CONFIG_ARCH_EFI) += serial_efi.o obj-$(CONFIG_LPC32XX_HSUART) += lpc32xx_hsuart.o obj-$(CONFIG_MCFUART) += mcfuart.o obj-$(CONFIG_OPENCORES_YANU) += opencores_yanu.o diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c new file mode 100644 index 0000000..15e541b --- /dev/null +++ b/drivers/serial/serial_efi.c @@ -0,0 +1,149 @@ +/*

• • Copyright (c) 2015 Google, Inc
• • Written by Simon Glass sjg@chromium.org
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <debug_uart.h> +#include <dm.h> +#include <efi.h> +#include <efi_api.h> +#include <errno.h> +#include <fdtdec.h> +#include <linux/compiler.h> +#include <asm/io.h> +#include <serial.h>

+DECLARE_GLOBAL_DATA_PTR;

Is this needed? I don't see gd is referenced.

+/* Information about the efi console */ +struct serial_efi_priv {

•   struct efi_simple_input_interface *con_in;
  

•   struct efi_simple_text_output_protocol *con_out;
  

•   struct efi_input_key key;
  

•   bool have_key;
  

+};

+int serial_efi_setbrg(struct udevice *dev, int baudrate) +{

•   return 0;
  

+}

+static int serial_efi_get_key(struct serial_efi_priv *priv) +{

•   int ret;
  

•   if (priv->have_key)
  

•           return 0;
  

•   ret = priv->con_in->read_key_stroke(priv->con_in, &priv->key);
  

•   if (ret == EFI_NOT_READY)
  

•           return -EAGAIN;
  

•   else if (ret != EFI_SUCCESS)
  

•           return -EIO;
  

•   priv->have_key = true;
  

•   return 0;
  

+}

+static int serial_efi_getc(struct udevice *dev) +{

•   struct serial_efi_priv *priv = dev_get_priv(dev);
  

•   int ret, ch;
  

•   ret = serial_efi_get_key(priv);
  

•   if (ret)
  

•           return ret;
  

•   priv->have_key = false;
  

•   ch = priv->key.unicode_char;
  

What is 8 here? Can we add a comment here?

•   if (!ch && priv->key.scan_code == 8)
  

•           ch = 8;
  

•   debug(" [%x %x %x] ", ch, priv->key.unicode_char, priv->key.scan_code);
  

•   return ch;
  

+}

+static int serial_efi_putc(struct udevice *dev, const char ch) +{

•   struct serial_efi_priv *priv = dev_get_priv(dev);
  

•   uint16_t ucode[2];
  

•   int ret;
  

•   ucode[0] = ch;
  

•   ucode[1] = '\0';
  

•   ret = priv->con_out->output_string(priv->con_out, ucode);
  

•   if (ret)
  

•           return -EIO;
  

•   return 0;
  

+}

+static int serial_efi_pending(struct udevice *dev, bool input) +{

•   struct serial_efi_priv *priv = dev_get_priv(dev);
  

•   int ret;
  

•   /* We assume that EFI will stall if its output buffer fills up */
  

•   if (!input)
  

•           return 0;
  

•   ret = serial_efi_get_key(priv);
  

•   if (ret == -EAGAIN)
  

•           return 0;
  

•   else if (ret)
  

•           return ret;
  

•   return 1;
  

+}

+void debug_uart_init(void) +{ +}

Can we add come comments here to mention nothing needs to be done here for EFI?

+static inline void _debug_uart_putc(int ch) +{

•   struct efi_system_table *sys_table = efi_get_sys_table();
  

•   uint16_t ucode[2];
  

•   ucode[0] = ch;
  

•   ucode[1] = '\0';
  

•   sys_table->con_out->output_string(sys_table->con_out, ucode);
  

+}

+DEBUG_UART_FUNCS

+static int serial_efi_probe(struct udevice *dev) +{

•   struct efi_system_table *table = efi_get_sys_table();
  

•   struct serial_efi_priv *priv = dev_get_priv(dev);
  

•   priv->con_in = table->con_in;
  

•   priv->con_out = table->con_out;
  

•   return 0;
  

+}

+static const struct dm_serial_ops serial_efi_ops = {

•   .putc = serial_efi_putc,
  

•   .getc = serial_efi_getc,
  

•   .pending = serial_efi_pending,
  

•   .setbrg = serial_efi_setbrg,
  

+};

+static const struct udevice_id serial_efi_ids[] = {

•   { .compatible = "efi,uart" },
  

•   { }
  

+};

+U_BOOT_DRIVER(serial_s5p) = {

Should be serial_efi.

•   .name   = "serial_s5p",
  

The same here.

•   .id     = UCLASS_SERIAL,
  

•   .of_match = serial_efi_ids,
  

•   .priv_auto_alloc_size = sizeof(struct serial_efi_priv),
  

•   .probe = serial_efi_probe,
  

•   .ops    = &serial_efi_ops,
  

•   .flags = DM_FLAG_PRE_RELOC,
  

+};

Regards, Bin

Hi Simon,

On Wed, Jul 22, 2015 at 11:49 PM, Simon Glass sjg@chromium.org wrote:

From: Ben Stoltz stoltz@google.com

When U-Boot runs as an EFI application is does not have a definition of CONFIG_SYS_TEXT_BASE. U-Boot is a relocatable application and the relocation is done by EFI. U-Boot can be loaded at any address.

Ensure that this CONFIG option is not set in this case.

Signed-off-by: Ben Stoltz stoltz@google.com Signed-off-by: Simon Glass sjg@chromium.org

Except the name of ARCH_EFI per my previous comments:

Reviewed-by: Bin Meng bmeng.cn@gmail.com

---

Kconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/Kconfig b/Kconfig index 15e15af..c1e7d22 100644 --- a/Kconfig +++ b/Kconfig @@ -179,6 +179,7 @@ config SYS_EXTRA_OPTIONS

config SYS_TEXT_BASE depends on SPARC || ARC || X86 || ARCH_UNIPHIER

•   depends on !ARCH_EFI
    hex "Text Base"
    help
      TODO: Move CONFIG_SYS_TEXT_BASE for all the architecture
  

--