Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46312 )
Change subject: soc/intel/common: Move CSE RW into new FMAP region to optimize boot time ......................................................................
Patch Set 7:
(4 comments)
Patch Set 7:
Patch Set 5:
Patch Set 5:
We still need to evaluate the downstream effects of this change on the rest of our infrastructure, tooling, etc. That's not to say we won't go this route, but we need to be a little more thorough with making sure all of our ebuilds, other tools, etc. that rely on the current flash layout to make sure they are up to date with this as well.
+1 to what Tim said. There are a number of factors that will have to be evaluated to ensure that we do not silently break or cause incompatibilities with any of the existing infrastructure:
- Signer
- Futility
- Firmware updater (both RO+RW as well as RW only)
- Autotest lab firmware recovery
- FAFT
...
Sridhar/Rizwan - have you evaluated the impact of the change on rest of the infrastructure? Is this captured anywhere?
@Tim/@Furquan, We have evaluated and capured impact of FMAP change on coreboots' test and development infra @ https://docs.google.com/document/d/1WlYqzUbdexHvB_xEF7UZ_PGXZS_3-lQ6z0DWnIW4...
Please review and let us know your comments.
https://review.coreboot.org/c/coreboot/+/46312/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46312/4//COMMIT_MSG@7 PS4, Line 7: Boot time optimization
Please make it a statement by using a verb (in imperative mood). […]
Done
https://review.coreboot.org/c/coreboot/+/46312/4//COMMIT_MSG@12 PS4, Line 12: impacts
increases the boot time by around 300 ms.
Done
https://review.coreboot.org/c/coreboot/+/46312/4//COMMIT_MSG@28 PS4, Line 28: TEST=Verified on hatch and JSL platforms
What is the old boot time, and what is the new one?
Ack
https://review.coreboot.org/c/coreboot/+/46312/3/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/46312/3/src/soc/intel/common/block/... PS3, Line 793: uint8_t
Yes, it make sense to me.
Done