Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35289 )
Change subject: [WIP] intel/fsp2_0: Restrict references to CAR ......................................................................
Patch Set 2: Code-Review-2
(3 comments)
Also, this will be not-for-merge for some time.
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... PS2, Line 402: if (!(CONFIG(FSP_M_XIP) && CONFIG(FSP_USES_CB_STACK))) {
Why is FSP_USES_CB_STACK added at all?
I could write this '!FSP_M_XIP || !FSP_USES_CB_STACK' if that is clearer.
I believe there will be request to split this entire function further, so this if very much WIP now.
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... PS2, Line 413: status = load_fspm_mem(&hdr, &file_data, &memmap); memmap reference here
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... PS2, Line 424: do_fsp_memory_init(&hdr, s3wake, &memmap); Another memmap reference here. But I believe amd/picasso, with DRAM already up, would not want to make this call. That patchtrain actually already has some comments about avoidind fsp_memory_init() call, but they still want FSP-M loader (afaics!).