Change in coreboot[master]: intel/block/cpu: Keep flash region cached until the payload is loaded
Attention is currently required from: Arthur Heymans, Patrick Georgi, Subrata Banik, Maulik V Vaghela, Rizwan Qureshi, Angel Pons, Sridhar Siricilla, Patrick Rudolph. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62566 )
Change subject: intel/block/cpu: Keep flash region cached until the payload is loaded ......................................................................
Patch Set 2:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/62566/comment/f9cfcce1_c017cda6 PS2, Line 10: done by FSP
@Werner, few clarification questions: […]
Yes Subrata, sorry for being unprecise. On Eklhart Lake (where I have discovered this issue) the MP init is executed in hybrid mode where FSP do the init and coreboot provides the callbacks.
https://review.coreboot.org/c/coreboot/+/62566/comment/1450a901_bf5c963f PS2, Line 15: Now, in this scenario, the SPI : flash linear address range is not registered as a resource (since the : common SPI driver in src/soc/intel/common/block/spi is shared across : multiple SPI controllers and therefore cannot distinguish where the : flash is actually located at)
I don't understand this. […]
Yes, like Angel mentioned already the fast_spi driver is not bound to a PCI device. I have had a quick look at the generic SPI driver and the bus_ops are only used for ACPI use cases. Doing this properly needs to refactor the SPI driver. But again, the painful thing here is the broad usage of the common driver. We would need a dedicated SPI bus driver just for the bus where the flash is attached to. Or we can add a callback into soc code to ask it _this_ SPI controller owns the flash and add the needed resource for this controller in the common code.
I though a bit about placing a resource in the system agent but it looks weird as the SPI flash is not part of it.
https://review.coreboot.org/c/coreboot/+/62566/comment/adabdce2_01addbca PS2, Line 19: uncached flash : range
I thought flashed mapped range is always cached and DRAM resource allocation is not clearing that. […]
The thing here is like described: Once coreboot MTRR setup is executed, the ranges are computed based on the resources already registered. If there is no resource around covering the flash region, it will not be taken into account and hence the MTRR will not be set up for it (according to the debug logs I have enabled for MTRR debugging).
https://review.coreboot.org/c/coreboot/+/62566/comment/f7f35f80_60388570 PS2, Line 20: The result of this chain is : that loading the payload from flash takes much longer now (on mc_ehl1 it : takes ~12 seconds for 4.5 MB).
Interesting. […]
There is a chance that this is indeed platform related. So if somebody can do some test on affected platforms that would be super helpful. For instance the same mainboard (meaning same SPI bus routing and speed settings) with a Broadwell-DE loads the same payload in 1.6 seconds. My first though was: maybe due to the larger cache (6 MB Broadwell-DE vs. 1.5 Elkhart Lake).
File src/soc/intel/common/block/cpu/mp_init.c:
https://review.coreboot.org/c/coreboot/+/62566/comment/1e1509e9_78d3570b PS2, Line 164: MP_SERVICES_PPI
Adding Intel ADL team to check if they are also seeing this regression. […]
At least what I have heared from Sheng Alder Lake is affected, too.
https://review.coreboot.org/c/coreboot/+/62566/comment/abfc18fe_2e6d2fac PS2, Line 165: mtrr_use_temp_range(OPTIMAL_CACHE_ROM_BASE, OPTIMAL_CACHE_ROM_SIZE,
The only platform that does this is amd/pi/00730F01. […]
It was just handy to re-use the macros defined for this purpose there.
-
Werner Zeh (Code Review)