Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41680 )
Change subject: soc/intel/xeon_sp: Early programming of ACPI bar ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41680/3/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/bootblock.h:
https://review.coreboot.org/c/coreboot/+/41680/3/src/soc/intel/xeon_sp/inclu... PS3, Line 3: _SOC_SKYLAKE_BOOTBLOCK_H_ not nice _SOC_SKYLAKE_BOOTBLOCK_H_ already exists in src/soc/intel/skylake/include/soc/bootblock.h may be _XEON_SP_BOOTBLOCK_H_?
https://review.coreboot.org/c/coreboot/+/41680/3/src/soc/intel/xeon_sp/inclu... PS3, Line 8: /* Bootblock pre console init programming */ : void bootblock_cpu_init(void); : void bootblock_pch_early_init(void); : : /* Bootblock post console init programming */ : void i2c_early_init(void); In this patch, only the bootblock_pch_init function is added, others aren't needed.
https://review.coreboot.org/c/coreboot/+/41680/3/src/soc/intel/xeon_sp/inclu... PS3, Line 15: void pch_early_iorange_init(void); : void report_platform_info(void); : void report_memory_config(void); Do we really need these function now?
https://review.coreboot.org/c/coreboot/+/41680/3/src/soc/intel/xeon_sp/pch.c File src/soc/intel/xeon_sp/pch.c:
https://review.coreboot.org/c/coreboot/+/41680/3/src/soc/intel/xeon_sp/pch.c... PS3, Line 51: enable_rtc_upper_bank(); I think it would be better to do this in a separate patch.