| Message ID | BLU436-SMTP2178D8EEFE6DA6DA049DF12BFB60@phx.gbl |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 06/01 20:31, Bin Meng wrote: > The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far. > It worked pretty well but looks not that good. Apart from doing too > much work than just enabling CAR, it cannot read the configuration > data from device tree at that time. Now we want to move it a little > bit later as part of init_sequence_f[] being called by board_init_f(). > This way it looks and works better in the U-Boot initialization path. > > Due to FSP's design, after calling FspInitEntry it will not return to > its caller, instead it jumps to a continuation function which is given > by bootloader with a new stack in system memory. The original stack in > the CAR is gone, but its content is perserved by FSP and described by > a bootloader temporary memory HOB. Technically we can recover anything > we had before in the previous stack, but that is way too complicated. > To make life much easier, in the FSP continuation routine we just > simply call fsp_init_done() and jump back to car_init_ret() to redo > the whole board_init_f() initialization, but this time with a non-zero > HOB list pointer saved in U-Boot's global data so that we can bypass > the FspInitEntry for the second time. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > arch/x86/cpu/start.S | 6 +++++- > arch/x86/include/asm/u-boot-x86.h | 4 ++++ > arch/x86/lib/fsp/fsp_car.S | 26 +++++--------------------- > arch/x86/lib/fsp/fsp_common.c | 8 ++++++++ > common/board_f.c | 3 +++ > 5 files changed, 25 insertions(+), 22 deletions(-) > > diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S > index 2e5f9da..00e585e 100644 > --- a/arch/x86/cpu/start.S > +++ b/arch/x86/cpu/start.S > @@ -116,12 +116,16 @@ car_init_ret: > rep stosb > > #ifdef CONFIG_HAVE_FSP > + test %esi, %esi > + jz skip_hob > + > /* Store HOB list */ > movl %esp, %edx > addl $GD_HOB_LIST, %edx > movl %esi, (%edx) > -#endif > > +skip_hob: > +#endif > /* Setup first parameter to setup_gdt, pointer to global_data */ > movl %esp, %eax > > diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h > index faa5182..9ee4104 100644 > --- a/arch/x86/include/asm/u-boot-x86.h > +++ b/arch/x86/include/asm/u-boot-x86.h > @@ -54,6 +54,10 @@ u32 isa_map_rom(u32 bus_addr, int size); > /* arch/x86/lib/... */ > int video_bios_init(void); > > +#ifdef CONFIG_HAVE_FSP > +int x86_fsp_init(void); > +#endif > + > void board_init_f_r_trampoline(ulong) __attribute__ ((noreturn)); > void board_init_f_r(void) __attribute__ ((noreturn)); > > diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S > index 5e09568..afbf3f9 100644 > --- a/arch/x86/lib/fsp/fsp_car.S > +++ b/arch/x86/lib/fsp/fsp_car.S > @@ -56,28 +56,10 @@ temp_ram_init_ret: > > /* stack grows down from top of CAR */ > movl %edx, %esp > + subl $4, %esp > > - /* > - * TODO: > - * > - * According to FSP architecture spec, the fsp_init() will not return > - * to its caller, instead it requires the bootloader to provide a > - * so-called continuation function to pass into the FSP as a parameter > - * of fsp_init, and fsp_init() will call that continuation function > - * directly. > - * > - * The call to fsp_init() may need to be moved out of the car_init() > - * to cpu_init_f() with the help of some inline assembly codes. > - * Note there is another issue that fsp_init() will setup another stack > - * using the fsp_init parameter stack_top after DRAM is initialized, > - * which means any data on the previous stack (on the CAR) gets lost > - * (ie: U-Boot global_data). FSP is supposed to support such scenario, > - * however it does not work. This should be revisited in the future. > - */ > - movl $CONFIG_FSP_TEMP_RAM_ADDR, %eax > - xorl %edx, %edx > - xorl %ecx, %ecx > - call fsp_init > + xor %esi, %esi > + jmp car_init_done > > .global fsp_init_done > fsp_init_done: > @@ -86,6 +68,8 @@ fsp_init_done: > * Save eax to esi temporarily. > */ > movl %eax, %esi > + > +car_init_done: > /* > * Re-initialize the ebp (BIST) to zero, as we already reach here > * which means we passed BIST testing before. > diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c > index 001494d..5b25632 100644 > --- a/arch/x86/lib/fsp/fsp_common.c > +++ b/arch/x86/lib/fsp/fsp_common.c > @@ -46,3 +46,11 @@ void board_final_cleanup(void) > > return; > } > + > +int x86_fsp_init(void) > +{ > + if (!gd->arch.hob_list) > + fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL); > + > + return 0; > +} > diff --git a/common/board_f.c b/common/board_f.c > index fbbad1b..21be26f 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = { > #ifdef CONFIG_OF_CONTROL > fdtdec_setup, > #endif > +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP) > + x86_fsp_init, > +#endif > #ifdef CONFIG_TRACE > trace_early_init, > #endif Overall, I like the idea of these two patches! In my quick testing on my Valley Island, I now appear to have access to the gd and device tree blob prior to calling the fsp to setup SDRAM. This will be a huge help (assuming I'm testing correctly) for migrating the memory configuration into the dts files. :) I'll try to spend more time testing this week, sorry, I've been quite busy so far with other things. Thanks! -Andrew
Hi Andrew, On Wed, Jun 3, 2015 at 4:05 AM, Andrew Bradford <andrew@bradfordembedded.com> wrote: > Hi Bin, > > On 06/01 20:31, Bin Meng wrote: >> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far. >> It worked pretty well but looks not that good. Apart from doing too >> much work than just enabling CAR, it cannot read the configuration >> data from device tree at that time. Now we want to move it a little >> bit later as part of init_sequence_f[] being called by board_init_f(). >> This way it looks and works better in the U-Boot initialization path. >> >> Due to FSP's design, after calling FspInitEntry it will not return to >> its caller, instead it jumps to a continuation function which is given >> by bootloader with a new stack in system memory. The original stack in >> the CAR is gone, but its content is perserved by FSP and described by >> a bootloader temporary memory HOB. Technically we can recover anything >> we had before in the previous stack, but that is way too complicated. >> To make life much easier, in the FSP continuation routine we just >> simply call fsp_init_done() and jump back to car_init_ret() to redo >> the whole board_init_f() initialization, but this time with a non-zero >> HOB list pointer saved in U-Boot's global data so that we can bypass >> the FspInitEntry for the second time. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> --- >> >> arch/x86/cpu/start.S | 6 +++++- >> arch/x86/include/asm/u-boot-x86.h | 4 ++++ >> arch/x86/lib/fsp/fsp_car.S | 26 +++++--------------------- >> arch/x86/lib/fsp/fsp_common.c | 8 ++++++++ >> common/board_f.c | 3 +++ >> 5 files changed, 25 insertions(+), 22 deletions(-) >> >> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S >> index 2e5f9da..00e585e 100644 >> --- a/arch/x86/cpu/start.S >> +++ b/arch/x86/cpu/start.S >> @@ -116,12 +116,16 @@ car_init_ret: >> rep stosb >> >> #ifdef CONFIG_HAVE_FSP >> + test %esi, %esi >> + jz skip_hob >> + >> /* Store HOB list */ >> movl %esp, %edx >> addl $GD_HOB_LIST, %edx >> movl %esi, (%edx) >> -#endif >> >> +skip_hob: >> +#endif >> /* Setup first parameter to setup_gdt, pointer to global_data */ >> movl %esp, %eax >> >> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h >> index faa5182..9ee4104 100644 >> --- a/arch/x86/include/asm/u-boot-x86.h >> +++ b/arch/x86/include/asm/u-boot-x86.h >> @@ -54,6 +54,10 @@ u32 isa_map_rom(u32 bus_addr, int size); >> /* arch/x86/lib/... */ >> int video_bios_init(void); >> >> +#ifdef CONFIG_HAVE_FSP >> +int x86_fsp_init(void); >> +#endif >> + >> void board_init_f_r_trampoline(ulong) __attribute__ ((noreturn)); >> void board_init_f_r(void) __attribute__ ((noreturn)); >> >> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S >> index 5e09568..afbf3f9 100644 >> --- a/arch/x86/lib/fsp/fsp_car.S >> +++ b/arch/x86/lib/fsp/fsp_car.S >> @@ -56,28 +56,10 @@ temp_ram_init_ret: >> >> /* stack grows down from top of CAR */ >> movl %edx, %esp >> + subl $4, %esp >> >> - /* >> - * TODO: >> - * >> - * According to FSP architecture spec, the fsp_init() will not return >> - * to its caller, instead it requires the bootloader to provide a >> - * so-called continuation function to pass into the FSP as a parameter >> - * of fsp_init, and fsp_init() will call that continuation function >> - * directly. >> - * >> - * The call to fsp_init() may need to be moved out of the car_init() >> - * to cpu_init_f() with the help of some inline assembly codes. >> - * Note there is another issue that fsp_init() will setup another stack >> - * using the fsp_init parameter stack_top after DRAM is initialized, >> - * which means any data on the previous stack (on the CAR) gets lost >> - * (ie: U-Boot global_data). FSP is supposed to support such scenario, >> - * however it does not work. This should be revisited in the future. >> - */ >> - movl $CONFIG_FSP_TEMP_RAM_ADDR, %eax >> - xorl %edx, %edx >> - xorl %ecx, %ecx >> - call fsp_init >> + xor %esi, %esi >> + jmp car_init_done >> >> .global fsp_init_done >> fsp_init_done: >> @@ -86,6 +68,8 @@ fsp_init_done: >> * Save eax to esi temporarily. >> */ >> movl %eax, %esi >> + >> +car_init_done: >> /* >> * Re-initialize the ebp (BIST) to zero, as we already reach here >> * which means we passed BIST testing before. >> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c >> index 001494d..5b25632 100644 >> --- a/arch/x86/lib/fsp/fsp_common.c >> +++ b/arch/x86/lib/fsp/fsp_common.c >> @@ -46,3 +46,11 @@ void board_final_cleanup(void) >> >> return; >> } >> + >> +int x86_fsp_init(void) >> +{ >> + if (!gd->arch.hob_list) >> + fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL); >> + >> + return 0; >> +} >> diff --git a/common/board_f.c b/common/board_f.c >> index fbbad1b..21be26f 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = { >> #ifdef CONFIG_OF_CONTROL >> fdtdec_setup, >> #endif >> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP) >> + x86_fsp_init, >> +#endif >> #ifdef CONFIG_TRACE >> trace_early_init, >> #endif > > Overall, I like the idea of these two patches! > > In my quick testing on my Valley Island, I now appear to have access to > the gd and device tree blob prior to calling the fsp to setup SDRAM. Yes, that's expected. Besides the device tree access, are you able to boot the Valley Island successfully with these two patches? > This will be a huge help (assuming I'm testing correctly) for migrating > the memory configuration into the dts files. :) > > I'll try to spend more time testing this week, sorry, I've been quite > busy so far with other things. > Thanks for testing. Regards, Bin
On 06/03 07:17, Bin Meng wrote: > Hi Andrew, > > On Wed, Jun 3, 2015 at 4:05 AM, Andrew Bradford > <andrew@bradfordembedded.com> wrote: > > Hi Bin, > > > > On 06/01 20:31, Bin Meng wrote: > >> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far. > >> It worked pretty well but looks not that good. Apart from doing too > >> much work than just enabling CAR, it cannot read the configuration > >> data from device tree at that time. Now we want to move it a little > >> bit later as part of init_sequence_f[] being called by board_init_f(). > >> This way it looks and works better in the U-Boot initialization path. > >> > >> Due to FSP's design, after calling FspInitEntry it will not return to > >> its caller, instead it jumps to a continuation function which is given > >> by bootloader with a new stack in system memory. The original stack in > >> the CAR is gone, but its content is perserved by FSP and described by > >> a bootloader temporary memory HOB. Technically we can recover anything > >> we had before in the previous stack, but that is way too complicated. > >> To make life much easier, in the FSP continuation routine we just > >> simply call fsp_init_done() and jump back to car_init_ret() to redo > >> the whole board_init_f() initialization, but this time with a non-zero > >> HOB list pointer saved in U-Boot's global data so that we can bypass > >> the FspInitEntry for the second time. > >> > >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > >> > >> --- > >> > >> arch/x86/cpu/start.S | 6 +++++- > >> arch/x86/include/asm/u-boot-x86.h | 4 ++++ > >> arch/x86/lib/fsp/fsp_car.S | 26 +++++--------------------- > >> arch/x86/lib/fsp/fsp_common.c | 8 ++++++++ > >> common/board_f.c | 3 +++ > >> 5 files changed, 25 insertions(+), 22 deletions(-) > >> > >> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S > >> index 2e5f9da..00e585e 100644 > >> --- a/arch/x86/cpu/start.S > >> +++ b/arch/x86/cpu/start.S > >> @@ -116,12 +116,16 @@ car_init_ret: > >> rep stosb > >> > >> #ifdef CONFIG_HAVE_FSP > >> + test %esi, %esi > >> + jz skip_hob > >> + > >> /* Store HOB list */ > >> movl %esp, %edx > >> addl $GD_HOB_LIST, %edx > >> movl %esi, (%edx) > >> -#endif > >> > >> +skip_hob: > >> +#endif > >> /* Setup first parameter to setup_gdt, pointer to global_data */ > >> movl %esp, %eax > >> > >> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h > >> index faa5182..9ee4104 100644 > >> --- a/arch/x86/include/asm/u-boot-x86.h > >> +++ b/arch/x86/include/asm/u-boot-x86.h > >> @@ -54,6 +54,10 @@ u32 isa_map_rom(u32 bus_addr, int size); > >> /* arch/x86/lib/... */ > >> int video_bios_init(void); > >> > >> +#ifdef CONFIG_HAVE_FSP > >> +int x86_fsp_init(void); > >> +#endif > >> + > >> void board_init_f_r_trampoline(ulong) __attribute__ ((noreturn)); > >> void board_init_f_r(void) __attribute__ ((noreturn)); > >> > >> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S > >> index 5e09568..afbf3f9 100644 > >> --- a/arch/x86/lib/fsp/fsp_car.S > >> +++ b/arch/x86/lib/fsp/fsp_car.S > >> @@ -56,28 +56,10 @@ temp_ram_init_ret: > >> > >> /* stack grows down from top of CAR */ > >> movl %edx, %esp > >> + subl $4, %esp > >> > >> - /* > >> - * TODO: > >> - * > >> - * According to FSP architecture spec, the fsp_init() will not return > >> - * to its caller, instead it requires the bootloader to provide a > >> - * so-called continuation function to pass into the FSP as a parameter > >> - * of fsp_init, and fsp_init() will call that continuation function > >> - * directly. > >> - * > >> - * The call to fsp_init() may need to be moved out of the car_init() > >> - * to cpu_init_f() with the help of some inline assembly codes. > >> - * Note there is another issue that fsp_init() will setup another stack > >> - * using the fsp_init parameter stack_top after DRAM is initialized, > >> - * which means any data on the previous stack (on the CAR) gets lost > >> - * (ie: U-Boot global_data). FSP is supposed to support such scenario, > >> - * however it does not work. This should be revisited in the future. > >> - */ > >> - movl $CONFIG_FSP_TEMP_RAM_ADDR, %eax > >> - xorl %edx, %edx > >> - xorl %ecx, %ecx > >> - call fsp_init > >> + xor %esi, %esi > >> + jmp car_init_done > >> > >> .global fsp_init_done > >> fsp_init_done: > >> @@ -86,6 +68,8 @@ fsp_init_done: > >> * Save eax to esi temporarily. > >> */ > >> movl %eax, %esi > >> + > >> +car_init_done: > >> /* > >> * Re-initialize the ebp (BIST) to zero, as we already reach here > >> * which means we passed BIST testing before. > >> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c > >> index 001494d..5b25632 100644 > >> --- a/arch/x86/lib/fsp/fsp_common.c > >> +++ b/arch/x86/lib/fsp/fsp_common.c > >> @@ -46,3 +46,11 @@ void board_final_cleanup(void) > >> > >> return; > >> } > >> + > >> +int x86_fsp_init(void) > >> +{ > >> + if (!gd->arch.hob_list) > >> + fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL); > >> + > >> + return 0; > >> +} > >> diff --git a/common/board_f.c b/common/board_f.c > >> index fbbad1b..21be26f 100644 > >> --- a/common/board_f.c > >> +++ b/common/board_f.c > >> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = { > >> #ifdef CONFIG_OF_CONTROL > >> fdtdec_setup, > >> #endif > >> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP) > >> + x86_fsp_init, > >> +#endif > >> #ifdef CONFIG_TRACE > >> trace_early_init, > >> #endif > > > > Overall, I like the idea of these two patches! > > > > In my quick testing on my Valley Island, I now appear to have access to > > the gd and device tree blob prior to calling the fsp to setup SDRAM. > > Yes, that's expected. Besides the device tree access, are you able to > boot the Valley Island successfully with these two patches? Yes! :) I'm able to boot and to extract data from the device tree prior to calling into the FSP. So with a patchset like this one that you've proposed I could then implement moving the fsp_config vpd data into device tree to much more easily support additional baytrail boards. Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com> Thanks! -Andrew
Hi Bin, On 3 June 2015 at 06:59, Andrew Bradford <andrew@bradfordembedded.com> wrote: > On 06/03 07:17, Bin Meng wrote: >> Hi Andrew, >> >> On Wed, Jun 3, 2015 at 4:05 AM, Andrew Bradford >> <andrew@bradfordembedded.com> wrote: >> > Hi Bin, >> > >> > On 06/01 20:31, Bin Meng wrote: >> >> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far. >> >> It worked pretty well but looks not that good. Apart from doing too >> >> much work than just enabling CAR, it cannot read the configuration >> >> data from device tree at that time. Now we want to move it a little >> >> bit later as part of init_sequence_f[] being called by board_init_f(). >> >> This way it looks and works better in the U-Boot initialization path. >> >> >> >> Due to FSP's design, after calling FspInitEntry it will not return to >> >> its caller, instead it jumps to a continuation function which is given >> >> by bootloader with a new stack in system memory. The original stack in >> >> the CAR is gone, but its content is perserved by FSP and described by >> >> a bootloader temporary memory HOB. Technically we can recover anything >> >> we had before in the previous stack, but that is way too complicated. >> >> To make life much easier, in the FSP continuation routine we just >> >> simply call fsp_init_done() and jump back to car_init_ret() to redo >> >> the whole board_init_f() initialization, but this time with a non-zero >> >> HOB list pointer saved in U-Boot's global data so that we can bypass >> >> the FspInitEntry for the second time. >> >> >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> >> >> --- >> >> >> >> arch/x86/cpu/start.S | 6 +++++- >> >> arch/x86/include/asm/u-boot-x86.h | 4 ++++ >> >> arch/x86/lib/fsp/fsp_car.S | 26 +++++--------------------- >> >> arch/x86/lib/fsp/fsp_common.c | 8 ++++++++ >> >> common/board_f.c | 3 +++ >> >> 5 files changed, 25 insertions(+), 22 deletions(-) >> >> >> >> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S >> >> index 2e5f9da..00e585e 100644 >> >> --- a/arch/x86/cpu/start.S >> >> +++ b/arch/x86/cpu/start.S >> >> @@ -116,12 +116,16 @@ car_init_ret: >> >> rep stosb >> >> >> >> #ifdef CONFIG_HAVE_FSP >> >> + test %esi, %esi >> >> + jz skip_hob >> >> + >> >> /* Store HOB list */ >> >> movl %esp, %edx >> >> addl $GD_HOB_LIST, %edx >> >> movl %esi, (%edx) >> >> -#endif >> >> >> >> +skip_hob: >> >> +#endif >> >> /* Setup first parameter to setup_gdt, pointer to global_data */ >> >> movl %esp, %eax >> >> >> >> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h >> >> index faa5182..9ee4104 100644 >> >> --- a/arch/x86/include/asm/u-boot-x86.h >> >> +++ b/arch/x86/include/asm/u-boot-x86.h >> >> @@ -54,6 +54,10 @@ u32 isa_map_rom(u32 bus_addr, int size); >> >> /* arch/x86/lib/... */ >> >> int video_bios_init(void); >> >> >> >> +#ifdef CONFIG_HAVE_FSP >> >> +int x86_fsp_init(void); >> >> +#endif >> >> + >> >> void board_init_f_r_trampoline(ulong) __attribute__ ((noreturn)); >> >> void board_init_f_r(void) __attribute__ ((noreturn)); >> >> >> >> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S >> >> index 5e09568..afbf3f9 100644 >> >> --- a/arch/x86/lib/fsp/fsp_car.S >> >> +++ b/arch/x86/lib/fsp/fsp_car.S >> >> @@ -56,28 +56,10 @@ temp_ram_init_ret: >> >> >> >> /* stack grows down from top of CAR */ >> >> movl %edx, %esp >> >> + subl $4, %esp >> >> >> >> - /* >> >> - * TODO: >> >> - * >> >> - * According to FSP architecture spec, the fsp_init() will not return >> >> - * to its caller, instead it requires the bootloader to provide a >> >> - * so-called continuation function to pass into the FSP as a parameter >> >> - * of fsp_init, and fsp_init() will call that continuation function >> >> - * directly. >> >> - * >> >> - * The call to fsp_init() may need to be moved out of the car_init() >> >> - * to cpu_init_f() with the help of some inline assembly codes. >> >> - * Note there is another issue that fsp_init() will setup another stack >> >> - * using the fsp_init parameter stack_top after DRAM is initialized, >> >> - * which means any data on the previous stack (on the CAR) gets lost >> >> - * (ie: U-Boot global_data). FSP is supposed to support such scenario, >> >> - * however it does not work. This should be revisited in the future. >> >> - */ >> >> - movl $CONFIG_FSP_TEMP_RAM_ADDR, %eax >> >> - xorl %edx, %edx >> >> - xorl %ecx, %ecx >> >> - call fsp_init >> >> + xor %esi, %esi >> >> + jmp car_init_done >> >> >> >> .global fsp_init_done >> >> fsp_init_done: >> >> @@ -86,6 +68,8 @@ fsp_init_done: >> >> * Save eax to esi temporarily. >> >> */ >> >> movl %eax, %esi >> >> + >> >> +car_init_done: >> >> /* >> >> * Re-initialize the ebp (BIST) to zero, as we already reach here >> >> * which means we passed BIST testing before. >> >> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c >> >> index 001494d..5b25632 100644 >> >> --- a/arch/x86/lib/fsp/fsp_common.c >> >> +++ b/arch/x86/lib/fsp/fsp_common.c >> >> @@ -46,3 +46,11 @@ void board_final_cleanup(void) >> >> >> >> return; >> >> } >> >> + >> >> +int x86_fsp_init(void) >> >> +{ >> >> + if (!gd->arch.hob_list) >> >> + fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL); >> >> + >> >> + return 0; >> >> +} >> >> diff --git a/common/board_f.c b/common/board_f.c >> >> index fbbad1b..21be26f 100644 >> >> --- a/common/board_f.c >> >> +++ b/common/board_f.c >> >> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = { >> >> #ifdef CONFIG_OF_CONTROL >> >> fdtdec_setup, >> >> #endif >> >> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP) >> >> + x86_fsp_init, >> >> +#endif >> >> #ifdef CONFIG_TRACE >> >> trace_early_init, >> >> #endif >> > >> > Overall, I like the idea of these two patches! >> > >> > In my quick testing on my Valley Island, I now appear to have access to >> > the gd and device tree blob prior to calling the fsp to setup SDRAM. >> >> Yes, that's expected. Besides the device tree access, are you able to >> boot the Valley Island successfully with these two patches? > > Yes! :) > > I'm able to boot and to extract data from the device tree prior to > calling into the FSP. So with a patchset like this one that you've > proposed I could then implement moving the fsp_config vpd data into > device tree to much more easily support additional baytrail boards. > > Tested-by: Andrew Bradford <andrew.bradford@kodakalaris.com> > > Thanks! > -Andrew This is a big step forward - congrats on getting this running! I feel that ultimately the FSP init fits better in dram_init(), but by that time we have emitted serial output and the 'second pass' logic gets a lot more messy. So let's go with what you have for now. Acked-by: Simon Glass <sjg@chromium.org> Regards, Simon
diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S index 2e5f9da..00e585e 100644 --- a/arch/x86/cpu/start.S +++ b/arch/x86/cpu/start.S @@ -116,12 +116,16 @@ car_init_ret: rep stosb #ifdef CONFIG_HAVE_FSP + test %esi, %esi + jz skip_hob + /* Store HOB list */ movl %esp, %edx addl $GD_HOB_LIST, %edx movl %esi, (%edx) -#endif +skip_hob: +#endif /* Setup first parameter to setup_gdt, pointer to global_data */ movl %esp, %eax diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h index faa5182..9ee4104 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -54,6 +54,10 @@ u32 isa_map_rom(u32 bus_addr, int size); /* arch/x86/lib/... */ int video_bios_init(void); +#ifdef CONFIG_HAVE_FSP +int x86_fsp_init(void); +#endif + void board_init_f_r_trampoline(ulong) __attribute__ ((noreturn)); void board_init_f_r(void) __attribute__ ((noreturn)); diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S index 5e09568..afbf3f9 100644 --- a/arch/x86/lib/fsp/fsp_car.S +++ b/arch/x86/lib/fsp/fsp_car.S @@ -56,28 +56,10 @@ temp_ram_init_ret: /* stack grows down from top of CAR */ movl %edx, %esp + subl $4, %esp - /* - * TODO: - * - * According to FSP architecture spec, the fsp_init() will not return - * to its caller, instead it requires the bootloader to provide a - * so-called continuation function to pass into the FSP as a parameter - * of fsp_init, and fsp_init() will call that continuation function - * directly. - * - * The call to fsp_init() may need to be moved out of the car_init() - * to cpu_init_f() with the help of some inline assembly codes. - * Note there is another issue that fsp_init() will setup another stack - * using the fsp_init parameter stack_top after DRAM is initialized, - * which means any data on the previous stack (on the CAR) gets lost - * (ie: U-Boot global_data). FSP is supposed to support such scenario, - * however it does not work. This should be revisited in the future. - */ - movl $CONFIG_FSP_TEMP_RAM_ADDR, %eax - xorl %edx, %edx - xorl %ecx, %ecx - call fsp_init + xor %esi, %esi + jmp car_init_done .global fsp_init_done fsp_init_done: @@ -86,6 +68,8 @@ fsp_init_done: * Save eax to esi temporarily. */ movl %eax, %esi + +car_init_done: /* * Re-initialize the ebp (BIST) to zero, as we already reach here * which means we passed BIST testing before. diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c index 001494d..5b25632 100644 --- a/arch/x86/lib/fsp/fsp_common.c +++ b/arch/x86/lib/fsp/fsp_common.c @@ -46,3 +46,11 @@ void board_final_cleanup(void) return; } + +int x86_fsp_init(void) +{ + if (!gd->arch.hob_list) + fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL); + + return 0; +} diff --git a/common/board_f.c b/common/board_f.c index fbbad1b..21be26f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = { #ifdef CONFIG_OF_CONTROL fdtdec_setup, #endif +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP) + x86_fsp_init, +#endif #ifdef CONFIG_TRACE trace_early_init, #endif
The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far. It worked pretty well but looks not that good. Apart from doing too much work than just enabling CAR, it cannot read the configuration data from device tree at that time. Now we want to move it a little bit later as part of init_sequence_f[] being called by board_init_f(). This way it looks and works better in the U-Boot initialization path. Due to FSP's design, after calling FspInitEntry it will not return to its caller, instead it jumps to a continuation function which is given by bootloader with a new stack in system memory. The original stack in the CAR is gone, but its content is perserved by FSP and described by a bootloader temporary memory HOB. Technically we can recover anything we had before in the previous stack, but that is way too complicated. To make life much easier, in the FSP continuation routine we just simply call fsp_init_done() and jump back to car_init_ret() to redo the whole board_init_f() initialization, but this time with a non-zero HOB list pointer saved in U-Boot's global data so that we can bypass the FspInitEntry for the second time. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- arch/x86/cpu/start.S | 6 +++++- arch/x86/include/asm/u-boot-x86.h | 4 ++++ arch/x86/lib/fsp/fsp_car.S | 26 +++++--------------------- arch/x86/lib/fsp/fsp_common.c | 8 ++++++++ common/board_f.c | 3 +++ 5 files changed, 25 insertions(+), 22 deletions(-)