Aaron Durbin 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:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35289/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35289/2//COMMIT_MSG@8 PS2, Line 8: I know this is a WIP patch, but please fill out the description with the motivation behind this patch.
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 252: const struct memranges *memmap This should be removed and all the StackBase/StackSize handling for the various scenarios should be in one function and called from fsp_memory_init(). This function likely needs to be split as well for some initial validation of header prior to doing other things. That said, I don't think the change looks too too bad.
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... PS2, Line 335: const struct memranges *memmap) Or pass NULL for memmap here.
Or, which I think is more appropriate, provide an FSP-S-like loading path for FSP-M. It can be relocated or one can attempt to statically link things, but the latter problematic in the S3 case.
I suggest we add a Kconfig which is FSPM_RUNS_FROM_DRAM which would handle all the complex conditionals and loading, tbh.
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... PS2, Line 404: memranges_init_empty(&memmap, &freeranges[0], ARRAY_SIZE(freeranges)); this line needs to be unconditional even if memmap isn't used.
https://review.coreboot.org/c/coreboot/+/35289/2/src/drivers/intel/fsp2_0/me... PS2, Line 405: _car_region_start Don't we have a ENV_CAR to use to guard all of this appropriately? I don't think the Kconfigs above are accurate. FSP_USES_CB_STACK is completely orthogonal to CAR. !FSP_M_XIP along with ENV_CAR would be the cleanest. However, I think my suggestion above is cleaner.