On Mon, Aug 3, 2015 at 8:10 AM, Simon Glass sjg@chromium.org wrote:

This is declared but no-longer exists. Drop it.

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

arch/x86/include/asm/u-boot-x86.h | 1 - 1 file changed, 1 deletion(-)

diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h index 4dae365..1c459d5 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -14,7 +14,6 @@ extern char gdt_rom[]; int arch_cpu_init(void); int x86_cpu_init_f(void); int cpu_init_f(void); -void init_gd(gd_t *id, u64 *gdt_addr); void setup_gdt(gd_t *id, u64 *gdt_addr); /*

• Setup FSP execution environment GDT to use the one we used in

--

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

Hi Simon,

On Mon, Aug 3, 2015 at 8:10 AM, Simon Glass sjg@chromium.org wrote:

Some archs like to have larger alignment for their global data. Use 16 bytes which suits all current archs.

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

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

diff --git a/common/board_f.c b/common/board_f.c index c596083..a947770 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -497,7 +497,7 @@ static int setup_machine(void)

static int reserve_global_data(void) {

•   gd->start_addr_sp -= sizeof(gd_t);
  

•   gd->start_addr_sp = ALIGN(gd->start_addr_sp - sizeof(gd_t), 16);
  

This logic is wrong. This alignment adjustment makes there is no enough room for global data.

    gd->new_gd = (gd_t *)map_sysmem(gd->start_addr_sp, sizeof(gd_t));
    debug("Reserving %zu Bytes for Global Data at: %08lx\n",
                    sizeof(gd_t), gd->start_addr_sp);

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 2155265..147f646 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -99,7 +99,7 @@ typedef struct global_data { int pcidelay_done; #endif struct udevice *cur_serial_dev; /* current serial device */

•   struct arch_global_data arch;   /* architecture-specific data */
  

•   struct arch_global_data arch __aligned(16);     /* arch-specific data */
  

} gd_t; #endif

--

Regards, Bin

On 08/02/2015 05:10 PM, Simon Glass wrote:

At present we have a simple assignment to gd. With some archs this is implemented as a register or through some other means; a simple assignment does not suit in all cases.

Change this to a function and add documentation to describe how this all works.

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

common/board_f.c | 14 +++++++++++--- include/common.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c index a947770..3d1f305 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -1024,15 +1024,23 @@ void board_init_f_r(void) #endif /* CONFIG_X86 */

#ifndef CONFIG_X86 -ulong board_init_f_mem(ulong top) +__weak void arch_setup_gdt(struct global_data *gd_ptr) {

• gd = gd_ptr;

+}

+ulong board_init_f_mem(ulong top, ulong reserve_size) +{

• struct global_data *gd_ptr;

• /* Leave space for the stack we are running with now */ top -= 0x40;

  top -= sizeof(struct global_data); top = ALIGN(top, 16);

• gd = (struct global_data *)top;
• memset((void *)gd, '\0', sizeof(*gd));

• gd_ptr = (struct global_data *)top;
• memset(gd_ptr, '\0', sizeof(*gd));
• arch_setup_gdt(gd_ptr);

#ifdef CONFIG_SYS_MALLOC_F_LEN top -= CONFIG_SYS_MALLOC_F_LEN; diff --git a/include/common.h b/include/common.h index fcc9ae7..029eb1f 100644 --- a/include/common.h +++ b/include/common.h @@ -217,6 +217,46 @@ extern char console_buffer[]; /* arch/$(ARCH)/lib/board.c */ void board_init_f(ulong); void board_init_r(gd_t *, ulong) __attribute__ ((noreturn));

+/**

• • board_init_f_mem() - Allocate global data and set stack position
• • This function is called by each architecture very early in the start-up
• • code to set up the environment for board_init_f(). It allocates space for
• • global_data (see include/asm-generic/global_data.h) and places the stack
• • below this.
• • This function requires a stack[1] Normally this is at @top. The function
• • starts allocating space from 64 bytes below @top. First it creates space
• • for global_data. then it calls arch_setup_gd() which sets gd to point to
• • the global_data space and can reserve additional bytes of space if
• • required). Finally it allocates early malloc() memory
• • (CONFIG_SYS_MALLOC_F_LEN). The new top of the stack is just below this,
• • and it returned by this function.
• • [1] Strictly speaking it would be possible to implement this function
• • in C on many archs such that it does not require a stack. However this
• • does not seem hugely important as only 64 byte are wasted.
• • @top: Top of available memory, also normally the top of the stack
• • @return: New stack location
• */

+ulong board_init_f_mem(ulong top, ulong reserve_size);

+/**

• • arch_setup_gdt() - Set up the global_data pointer
• • This pointer is special in some architectures and cannot easily be assigned
• • to. For example on x86 it is implemented by adding a specific record to its
• • Global Descriptor Table! So we we provide a function to carry out this task.
• • For most architectures this can simply be:
• • gd = gd_ptr;
• • @gd_ptr: Pointer to global data
• */

+void arch_setup_gdt(gd_t *gd_ptr);

int checkboard(void); int show_board_info(void); int checkflash(void);

This looks good to me. Reviewed-by: York Sun yorksun@freescale.com

York

Hi Bin,

On 6 August 2015 at 01:15, Bin Meng bmeng.cn@gmail.com wrote:

Hi Simon,

On Mon, Aug 3, 2015 at 8:10 AM, Simon Glass sjg@chromium.org wrote:

...

At present we have a simple assignment to gd. With some archs this is implemented as a register or through some other means; a simple assignment does not suit in all cases.

Change this to a function and add documentation to describe how this all works.

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

common/board_f.c | 14 +++++++++++--- include/common.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c index a947770..3d1f305 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -1024,15 +1024,23 @@ void board_init_f_r(void) #endif /* CONFIG_X86 */

#ifndef CONFIG_X86 -ulong board_init_f_mem(ulong top) +__weak void arch_setup_gdt(struct global_data *gd_ptr) {

•   gd = gd_ptr;
  

+}

+ulong board_init_f_mem(ulong top, ulong reserve_size)

What is reserve_size? I don't see it is used by this function. If we change the signature, we need also update arc and ppc4xx start.S otherwise they will break.

Actually that should have been reverted. I decided to introduce it later if needed, but found a way around it for x86.

...

+{

•   struct global_data *gd_ptr;
  

•   /* Leave space for the stack we are running with now */
    top -= 0x40;
  
    top -= sizeof(struct global_data);
    top = ALIGN(top, 16);
  

•   gd = (struct global_data *)top;
  

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

•   gd_ptr = (struct global_data *)top;
  

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

•   arch_setup_gdt(gd_ptr);
  

gdt? Should be just arch_setup_gd()?

Yes

...

#ifdef CONFIG_SYS_MALLOC_F_LEN top -= CONFIG_SYS_MALLOC_F_LEN; diff --git a/include/common.h b/include/common.h index fcc9ae7..029eb1f 100644 --- a/include/common.h +++ b/include/common.h @@ -217,6 +217,46 @@ extern char console_buffer[]; /* arch/$(ARCH)/lib/board.c */ void board_init_f(ulong); void board_init_r(gd_t *, ulong) __attribute__ ((noreturn));

+/**

• • board_init_f_mem() - Allocate global data and set stack position
• • This function is called by each architecture very early in the start-up
• • code to set up the environment for board_init_f(). It allocates space for
• • global_data (see include/asm-generic/global_data.h) and places the stack
• • below this.
• • This function requires a stack[1] Normally this is at @top. The function
• • starts allocating space from 64 bytes below @top. First it creates space
• • for global_data. then it calls arch_setup_gd() which sets gd to point to

Nits: . -> ,

...

• • the global_data space and can reserve additional bytes of space if
• • required). Finally it allocates early malloc() memory
• • (CONFIG_SYS_MALLOC_F_LEN). The new top of the stack is just below this,
• • and it returned by this function.
• • [1] Strictly speaking it would be possible to implement this function
• • in C on many archs such that it does not require a stack. However this
• • does not seem hugely important as only 64 byte are wasted.

Where is the 64 bytes it uses? I only see a variable gd_ptr on the stack.

It's the subtraction at the top of board_init_f_mem().

...

• • @top: Top of available memory, also normally the top of the stack
• • @return: New stack location
• */

+ulong board_init_f_mem(ulong top, ulong reserve_size);

I don't like the name board_init_f_mem(). It actually does two things: creates global data pointer and reserve the early malloc() memory size. Also it looks like it's more like an arch thing instead of a board's. How about setup_gd_f_mem()?

The name is the same as before. It's intended to imply that it gets the memory ready for board_init_f(). It may expand in future to do additional setup (e.g. to allocate some other type of space). If we change the name we would perhaps have to change it again later. At least this name describes its purpose.

...

+/**

• • arch_setup_gdt() - Set up the global_data pointer
• • This pointer is special in some architectures and cannot easily be assigned
• • to. For example on x86 it is implemented by adding a specific record to its
• • Global Descriptor Table! So we we provide a function to carry out this task.
• • For most architectures this can simply be:
• • gd = gd_ptr;
• • @gd_ptr: Pointer to global data
• */

+void arch_setup_gdt(gd_t *gd_ptr);

int checkboard(void); int show_board_info(void); int checkflash(void); --

Regards, Bin

Regards, Simon

On Mon, Aug 3, 2015 at 8:10 AM, Simon Glass sjg@chromium.org wrote:

Rather than keeping track of the Global Descriptor Table in its own memory we may as well put it in global_data with everything else. As a first step, stop using the separately allocated GDT.

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

arch/x86/cpu/cpu.c | 9 +++++---- arch/x86/include/asm/global_data.h | 4 +++- 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index 129777c..4f57145 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -136,9 +136,10 @@ static void load_gdt(const u64 *boot_gdt, u16 num_entries) asm volatile("lgdtl %0\n" : : "m" (gdt)); }

-void setup_gdt(gd_t *id, u64 *gdt_addr) +void setup_gdt(gd_t *new_gd, u64 *gdt_addr) {

•   id->arch.gdt = gdt_addr;
  

•   gdt_addr = new_gd->arch.gdt;
  

•   /* CS: code, read/execute, 4 GB, base 0 */
    gdt_addr[X86_GDT_ENTRY_32BIT_CS] = GDT_ENTRY(0xc09b, 0, 0xfffff);
  
  

@@ -146,9 +147,9 @@ void setup_gdt(gd_t *id, u64 *gdt_addr) gdt_addr[X86_GDT_ENTRY_32BIT_DS] = GDT_ENTRY(0xc093, 0, 0xfffff);

    /* FS: data, read/write, 4 GB, base (Global Data Pointer) */

•   id->arch.gd_addr = id;
  

•   new_gd->arch.gd_addr = new_gd;
    gdt_addr[X86_GDT_ENTRY_32BIT_FS] = GDT_ENTRY(0xc093,
  

•                (ulong)&id->arch.gd_addr, 0xfffff);
  

•                (ulong)&new_gd->arch.gd_addr, 0xfffff);
  
    /* 16-bit CS: code, read/execute, 64 kB, base 0 */
    gdt_addr[X86_GDT_ENTRY_16BIT_CS] = GDT_ENTRY(0x009b, 0, 0x0ffff);
  

diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h index f7e3889..35148ab 100644 --- a/arch/x86/include/asm/global_data.h +++ b/arch/x86/include/asm/global_data.h @@ -10,6 +10,8 @@

#ifndef __ASSEMBLY__

+#include <asm/processor.h>

enum pei_boot_mode_t { PEI_BOOT_NONE = 0, PEI_BOOT_SOFT_RESET, @@ -44,6 +46,7 @@ struct mtrr_request {

/* Architecture-specific global data */ struct arch_global_data {

•   u64 gdt[X86_GDT_NUM_ENTRIES] __aligned(16);
    struct global_data *gd_addr;    /* Location of Global Data */
    uint8_t x86;                    /* CPU family */
    uint8_t x86_vendor;             /* CPU vendor */
  

@@ -68,7 +71,6 @@ struct arch_global_data { /* MRC training data to save for the next boot */ char *mrc_output; unsigned int mrc_output_len;

•   void *gdt;                      /* Global descriptor table */
    ulong table;                    /* Table pointer from previous loader */
  

};

--

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

Hi Simon,

On Mon, Aug 3, 2015 at 8:10 AM, Simon Glass sjg@chromium.org wrote:

There is quite a bit of assembler code that can be removed if we use the generic global_data setup. Less arch-specific code makes it easier to add new features and maintain the start-up code.

Drop the unneeded code and adjust the hooks in board_f.c to cope.

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

arch/x86/cpu/cpu.c | 4 ++- arch/x86/cpu/start.S | 86 ++++++---------------------------------------------- common/board_f.c | 3 +- 3 files changed, 15 insertions(+), 78 deletions(-)

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index 4f57145..b2e0323 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -136,8 +136,10 @@ static void load_gdt(const u64 *boot_gdt, u16 num_entries) asm volatile("lgdtl %0\n" : : "m" (gdt)); }

-void setup_gdt(gd_t *new_gd, u64 *gdt_addr) +void arch_setup_gdt(gd_t *new_gd) {

•   u64 *gdt_addr;
  

•   gdt_addr = new_gd->arch.gdt;
  
    /* CS: code, read/execute, 4 GB, base 0 */
  

diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S index ac711ed..d93bdfc 100644 --- a/arch/x86/cpu/start.S +++ b/arch/x86/cpu/start.S @@ -104,8 +104,7 @@ car_init_ret: * * top-> CONFIG_SYS_CAR_ADDR + CONFIG_SYS_CAR_SIZE * MRC area

•    *      global_data
  

•    *      x86 global descriptor table
  

•    *      global_data with x86 global descriptor table
     *      early malloc area
     *      stack
     * bottom-> CONFIG_SYS_CAR_ADDR
  

@@ -120,13 +119,10 @@ car_init_ret: * and esi holds the HOB list address returned by the FSP. */ #endif

•   /* Reserve space on stack for global data */
  

•   subl    $GENERATED_GBL_DATA_SIZE, %esp
  

•   /* Align global data to 16-byte boundary */
  

•   andl    $0xfffffff0, %esp
  

•   post_code(POST_START_STACK)
  

•   /* Set up global data */
  

•   mov     %esp, %eax
  

•   call    board_init_f_mem
  

•   mov     %eax, %esp
  
    /*
     * Debug UART is available here although it may not be plumbed out
  

@@ -137,19 +133,12 @@ car_init_ret: * call printch */

•   /* Zero the global data since it won't happen later */
  

•   xorl    %eax, %eax
  

•   movl    $GENERATED_GBL_DATA_SIZE, %ecx
  

•   movl    %esp, %edi
  

•   rep     stosb
  

#ifdef CONFIG_HAVE_FSP

•   /* Store the HOB list if we have one */
    test    %esi, %esi
    jz      skip_hob
  
  

•   /* Store HOB list */
  

•   movl    %esp, %edx
  

•   addl    $GD_HOB_LIST, %edx
  

+fs addl $GD_HOB_LIST, %edx

I don't see %edx is initialized anywhere?

    movl    %esi, (%edx)

skip_hob: @@ -159,35 +148,10 @@ skip_hob: addl $GD_TABLE, %edx movl %esi, (%edx) #endif

•   /* Setup first parameter to setup_gdt, pointer to global_data */
  

•   movl    %esp, %eax
  

•   /* Reserve space for global descriptor table */
  

•   subl    $X86_GDT_SIZE, %esp
  

•   /* Align temporary global descriptor table to 16-byte boundary */
  

•   andl    $0xfffffff0, %esp
  

•   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 into global_data */
  

•   movl    %eax, %edx
  

•   addl    $GD_BIST, %edx
  

•   /* Store BIST */
  

+fs addl $GD_BIST, %edx

Ditto.

    movl    %ebp, (%edx)

•   /* Set second parameter to setup_gdt() */
  

•   movl    %ecx, %edx
  

•   /* Setup global descriptor table so gd->xyz works */
  

•   call    setup_gdt
  

•   /* Set parameter to board_init_f() to boot flags */
    post_code(POST_START_DONE)
    xorl    %eax, %eax
  

@@ -213,37 +177,7 @@ board_init_f_r_trampoline: /* Stack grows down from top of SDRAM */ movl %eax, %esp

•   /* Reserve space on stack for global data */
  

•   subl    $GENERATED_GBL_DATA_SIZE, %esp
  

•   /* Align global data to 16-byte boundary */
  

•   andl    $0xfffffff0, %esp
  

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

•   movl    %esp, %eax
  

•   /* Setup second parameter to memcpy() */
  

•   fs movl 0, %edx
  

•   /* Set third parameter to memcpy() */
  

•   movl    $GENERATED_GBL_DATA_SIZE, %ecx
  

•   /* Copy global data from CAR to SDRAM stack */
  

•   call    memcpy
  

•   /* Reserve space for global descriptor table */
  

•   subl    $X86_GDT_SIZE, %esp
  

•   /* Align global descriptor table to 16-byte boundary */
  

•   andl    $0xfffffff0, %esp
  

•   /* Set second parameter to setup_gdt() */
  

•   movl    %esp, %edx
  

•   /* Setup global descriptor table so gd->xyz works */
  

•   call    setup_gdt
  

•   /* Set if we need to disable CAR */
  

•   /* See if we need to disable CAR */
  

.weak car_uninit movl $car_uninit, %eax cmpl $0, %eax diff --git a/common/board_f.c b/common/board_f.c index 3d1f305..ec9d2a8 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -711,6 +711,7 @@ static int jump_to_copy(void) * with the stack in SDRAM and Global Data in temporary memory * (CPU cache) */

•   arch_setup_gdt(gd->new_gd);
    board_init_f_r_trampoline(gd->start_addr_sp);
  

#else relocate_code(gd->start_addr_sp, gd->new_gd, gd->relocaddr); @@ -1028,6 +1029,7 @@ __weak void arch_setup_gdt(struct global_data *gd_ptr) { gd = gd_ptr; } +#endif /* !CONFIG_X86 */

ulong board_init_f_mem(ulong top, ulong reserve_size) { @@ -1049,4 +1051,3 @@ ulong board_init_f_mem(ulong top, ulong reserve_size)

    return top;

} -#endif /* !CONFIG_X86 */

Can we remove the CONFIG_X86 around arch_set_gdt() given it is a __weak and any arch can implement their own?

--

Regards, Bin