Change in ...coreboot[master]: {drivers,soc/intel/braswell}: Implement C_ENVIRONMENT_BOOTBLOCK support
Frans Hendriks 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:
(5 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 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?
Yes.
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?
Ack
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.
Agree
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 128: /* Restore the timestamp from bootblock_crt0.S (ebp:mm1) */
Your removed the code that moved the timestamps in ebp:mm1...
in bootblock_protectd_mode_entry: (file src\arch\x86\bootblock_ctr0.S) mm1 is filled, before calling bootblock_pre_c_entry().
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. […]
Would separate patch be better? Maybe after merge of this patch doing skylake and braswell in one patch set
-
Frans Hendriks (Code Review)