Attention is currently required from: Andrey Petrov, Arthur Heymans, Chen, Gang C, David Hendricks, Johnny Lin, Lean Sheng Tan, Martin L Roth, Nico Huber, Patrick Rudolph, Paul Menzel, Ronak Kanabar.
Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80328?usp=email )
Change subject: drivers/intel/fsp2_0: Add FSP_ALLOCATES_TEMP_RAM ......................................................................
Patch Set 4:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80328/comment/e28318f4_dacf97a1 : PS1, Line 9: FSP allocates temporary ram in its reserved memory ranges : instead of requesting coreboot to do the allocation. This : is supported by some FSP implementations for Xeon SP.
I second this. If we used the defaults, we would still have to assert that […]
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/80328/comment/da3758cc_8d89e504 : PS3, Line 8:
Wouldn't it be easier to select `PLATFORM_USES_FSP2_0` instead of `PLATFORM_USES_FSP2_2`?
Not sure if I correctly get your points, Rudolph. I didn't use PLATFORM_USES_FSP2_0 or PLATFORM_USES_FSP2_2 as the selector due to 2 reasons.
1) PLATFORM_USES_FSP2_0/2 will be enabled by default by FSP2_3/4 forwardly, but we would like to keep this kconfig to be explicitly and optionally selected.
2) We cannot cover CPX sue to CPX's FSP doesn't update StackBase/Size's default value to fit for this usage. P.S. new logics is added in memory_init.c to check StackBase/Size's range validity before move ahead.
https://review.coreboot.org/c/coreboot/+/80328/comment/d52e3098_f20e0d4e : PS3, Line 11: some
Good catch, if CPX needs, maybe community could help to test CPX as well? Anyway ... […]
Done
Patchset:
PS3:
I hope we are not stuck here. Shuo, if you could draw a small temp-ram memory […]
Good suggestion Nico. I'm tentatively adding a memory map into src/drivers/intel/fsp2_0/memory_init.c where there are more space to put long contents. Please review and let us discuss the next step and if it is more preferred to move the memory map into Kconfig.
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/80328/comment/8d0c4c00_559a7a84 : PS3, Line 42: FSP_DOES_NOT_NEED_TEMP_RAM
Maybe "FSP_ALLOCATES_TEMP_RAM"? […]
Done
https://review.coreboot.org/c/coreboot/+/80328/comment/8bbd3172_1d08d72c : PS3, Line 46: FSP allocates temporary ram in its reserved memory ranges : instead of requesting coreboot to do the allocation.
The option says `NOT_NEED`? Isn’t that a contradiction?
Acknowledged
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/80328/comment/340d9d0b_c3d2e10b : PS1, Line 156: goto temp_ram_assigned;
Yeah, for the RC sample fixes (thanks for providing the inputs :-)), we made careful evaluations. […]
Arthur, I'm trying to cover the needs above in the updated codes. Let us review and discuss.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/80328/comment/59dd1bee_972c6cca : PS3, Line 156: goto temp_ram_assigned;
Please review together with https://review.coreboot. […]
Done