Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29662 )
Change subject: soc/intel/braswell: Add C_ENVIRONMENT_BOOTBLOCK support ......................................................................
Patch Set 9:
(7 comments)
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/Kconfig File src/soc/intel/braswell/Kconfig:
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/Kconfig@68 PS9, Line 68: VBOOT_STARTS_IN_ROMSTAGE Could be changed.
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/Kconfig@70 PS9, Line 70: config BOOTBLOCK_CPU_INIT : string : default "soc/intel/braswell/bootblock/bootblock.c" unused now
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/Makefile.inc File src/soc/intel/braswell/Makefile.inc:
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/Makefile.inc@... PS9, Line 29: cache_as_ram_cbootblock.S to be consistent, cache_as_ram.S would be better.
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/bootblock/boo... File src/soc/intel/braswell/bootblock/bootblock.c:
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/bootblock/boo... PS9, Line 76: Microcode needs to be updated before running CAR. You can use cpu/intel/microcode/microcode_asm.S for that purpose.
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/bootblock/cac... File src/soc/intel/braswell/bootblock/cache_as_ram_cbootblock.S:
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/bootblock/cac... PS9, Line 146: * 0x03 - FSP INFO Header not found : * 0x04 - ImageBase does not equal CONFIG_FSP_LOC - Is the FSP : rebased to a different location, or does it need to be? Could you put cosmetic changes in a different patch?
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/romstage/car_... File src/soc/intel/braswell/romstage/car_stage_entry.S:
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/romstage/car_... PS9, Line 24: #include "src/drivers/intel/fsp1_1/after_raminit.S" It is better to link files than to include them like this.
https://review.coreboot.org/#/c/29662/9/src/soc/intel/braswell/romstage/car_... PS9, Line 31: #if IS_ENABLED(CONFIG_POST_IO) : outb %al, $CONFIG_POST_IO_PORT : #else : post_code(POST_DEAD_CODE) : #endif Why is this needed?
-
Arthur Heymans (Code Review)