| Message ID | 1438560612-29910-6-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
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
Hi Bin, On 6 August 2015 at 01:16, Bin Meng <bmeng.cn@gmail.com> wrote: > 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? What I really want to do is: movl fs:GD_HOB_LIST, %edx That doesn't seem to do the right thing, although I'm still testing fiddling with it. > >> 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? Unfortunately I get a compile error when assigning to gd - it is not a register like on ARM, etc. Regards, Simon
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 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 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 */
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(-)