Change in ...coreboot[master]: {drivers,soc/intel/braswell}: Implement C_ENVIRONMENT_BOOTBLOCK support
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29662 )
Change subject: {drivers,soc/intel/braswell}: Implement C_ENVIRONMENT_BOOTBLOCK support ......................................................................
Patch Set 29:
(4 comments)
https://review.coreboot.org/#/c/29662/28/src/drivers/intel/fsp1_1/include/fs... File src/drivers/intel/fsp1_1/include/fsp/car.h:
https://review.coreboot.org/#/c/29662/28/src/drivers/intel/fsp1_1/include/fs... PS28, Line 24: struct cache_as_ram_params { : uint64_t tsc; : uint32_t bist; : FSP_INFO_HEADER *fih; : uintptr_t bootloader_car_start; : uintptr_t bootloader_car_end; : }; You might want to clean this up, as most gets unused now.
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/ca... File src/soc/intel/braswell/bootblock/cache_as_ram.S:
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/ca... PS28, Line 93: : /* : * ebp: FSP_INFO_HEADER address : * ecx: Temp RAM base : * edx: Temp RAM top : * edi: BIST value : * esp: Top of stack in temp RAM : * mm0: low 32-bits of TSC value : * mm1: high 32-bits of TSC value : */ : : /* Create cache_as_ram_params on stack */ : pushl %edx /* bootloader CAR end */ : pushl %ecx /* bootloader CAR begin */ : pushl %ebp /* FSP_INFO_HEADER */ : pushl %edi /* bist */ : movd %mm1, %eax : pushl %eax /* tsc[63:32] */ : movd %mm0, %eax : pushl %eax /* tsc[31:0] */ : pushl %esp /* pointer to cache_as_ram_params */ The bootblock function does not use this anymore.
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/ca... PS28, Line 128: /* Restore the timestamp from bootblock_crt0.S (ebp:mm1) */
in bootblock_protectd_mode_entry: (file src\arch\x86\bootblock_ctr0. […]
Yes but here you push %ebp to to the stack, but %ebp does not contain the upper timestamp (%mm2) (the function argument is 64bit long). %ebp contains the FSP location afaict.
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/romstage/car... File src/soc/intel/braswell/romstage/car_stage_entry.S:
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/romstage/car... PS28, Line 21: .global car_stage_entry : car_stage_entry: : call romstage_c_entry : : movb $0x69, %ah : jmp .Lhlt
Would separate patch be better? […]
I pushed some work that cleans this up. See https://review.coreboot.org/c/coreboot/+/32962 and https://review.coreboot.org/c/coreboot/+/32963/
-
Arthur Heymans (Code Review)