Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36627 )
Change subject: soc/intel/icelake: Move pch_early_init from bootblock to romstage ......................................................................
Patch Set 2:
(17 comments)
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36627/2//COMMIT_MSG@10 PS2, Line 10: to romstage/pch.c. Commit message describes what is being done but not why it is being done. Can you please add the reasoning behind making this change?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/pch.c:
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 16: <device/mmio.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 17: <device/device.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 18: <device/pci_ops.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 19: intelblocks/fast_spi.h No Fast SPI operations in this file. Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 20: intelblocks/gspi.h No GSPI operations in this file. Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 21: intelblocks/lpc_lib.h No LPC operations in this file. Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 22: intelblocks/p2sb.h Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 28: #include <soc/bootblock.h> Why is bootblock.h included in a file for romstage?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 31: #include <soc/p2sb.h> Why is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 32: #include <soc/pch.h> Not required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 33: #include <soc/pci_devs.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 35: #include <soc/pm.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 36: #include <soc/smbus.h> Is this required?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 70: early What does "early" signify in case of romstage? In bootblock, it is used to differentiate initialization that is done before console init v/s after. What is the definition for romstage?
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 85: pmc_gpe_init Have you evaluated/tested the impact of making this change? If GPE init is moved to romstage, then anything that relies on having the GPE routing setup will fail. One example that comes to mind is TPM driver accessing the state of GPE for TPM IRQ line in verstage.
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 87: enable_rtc_upper_bank Is anything stored in CMOS in upper bank that is accessed in early stages before romstage?