Subrata Banik 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. […]
i guess the idea is to do limited programming in bootblock and move the not that urgent bootblock programming into romstage?
will add the same in commit msg
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?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 17: <device/device.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 18: <device/pci_ops.h>
Is this required?
Done
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. […]
Done
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. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 22: intelblocks/p2sb.h
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 28: #include <soc/bootblock.h>
Why is bootblock. […]
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 31: #include <soc/p2sb.h>
Why is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 32: #include <soc/pch.h>
Not required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 33: #include <soc/pci_devs.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 35: #include <soc/pm.h>
Is this required?
Done
https://review.coreboot.org/c/coreboot/+/36627/2/src/soc/intel/icelake/romst... PS2, Line 36: #include <soc/smbus.h>
Is this required?
Done
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 initializat […]
it was added to maintain more align name with system_early_init but make sense to remove _early_ from function
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 a […]
yes, no issue
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?
i believe this is valid because vboot might use cmos during verstage while creating vboot_buffer. Although i don't see any error