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 28:
(7 comments)
A bit outside the scope of this patch but the romstage codeflow is pretty messy with FSP1.1 with many functions doing nothing but calling the next: car_stage_entry -> romstage_c_entry -> cache_as_ram_stage_main -> romstage_main... It might be good to clean that up too. cpu/intel/car/romstage.c has a clean interface to start from.
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 38: : /* Mainboard and SoC initialization prior to console. */ : void car_mainboard_pre_console_init(void); : void car_soc_pre_console_init(void); : /* Mainboard and SoC initialization post console initialization. */ : void car_mainboard_post_console_init(void); : void car_soc_post_console_init(void); Those are unused now?
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/bo... File src/soc/intel/braswell/bootblock/bootblock.c:
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/bo... PS28, Line 35: : void program_base_addresses(void) Is this called somewhere else? Why not static void?
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/bo... PS28, Line 127: setup_mmconfig(); Doing this before programming base addresses would be more logical.
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 19: /* : * Replacement for cache_as_ram.inc when using the FSP binary. This code : * locates the FSP binary, initializes the cache as RAM and performs the : * first stage of initialization. Next this code switches the stack from : * the cache to RAM and then disables the cache as RAM. Finally this code : * performs the final stage of initialization. : */ : : /* I/O delay between post codes on failure */ : #define LHLT_DELAY 0x50000 : /* : * Per FSP1.1 specs, following registers are preserved: : * EBX, EDI, ESI, EBP, MM0, MM1 : * : * Shift values to release MM2. : * mm0 -> edi: BIST value : * mm1 -> mm0: low 32-bits of TSC value : * mm2 -> mm1: high 32-bits of TSC value : */ : movd %mm0, %edi : movd %mm1, %eax : movd %eax, %mm0 : movd %mm2, %eax : movd %eax, %mm1 Why is this removed?
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) */ Your removed the code that moved the timestamps in ebp:mm1...
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/pm... File src/soc/intel/braswell/bootblock/pmc.c:
https://review.coreboot.org/#/c/29662/28/src/soc/intel/braswell/bootblock/pm... PS28, Line 21: tco_disable Feel free to move it to the bootblock.c file
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 Looks like skylake does this too, but IMO it makes little sense. Just use rename the romstage entry in the C code to car_stage_entry. (maybe in a separate patch for skylake first)
-
Arthur Heymans (Code Review)