Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 53: (CONFIG(CACHE_MRC_SETTINGS)
Ack, the only remaining FSP 1.1 platform (Braswell) selects CACHE_MRC_SETTINGS anyway. […]
But, I mean, we can't just leave it like this, right? That's just wrong. It shouldn't be calling the MRC cache code if it's supposed to be disabled.
You can just use an extra set of parenthesis to make the GCC warning disappear, like this:
} else if (CONFIG(CACHE_MRC_SETTINGS) && (params->saved_data = mrc_cache_current_mmap_leak(...))) {
Alternatively you can do something like
} else { if (CONFIG(...)) params->saved_data = mrc_cache_current_mmap_leak(...); else params->saved_data = NULL;
if (params->saved_data) { ... } else if (s3wake) { ... } else { ... } }
Or, if we're saying that all boards using this code must have that Kconfig enabled, just take the check for it out and describe it via Kconfig depends on.
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 292: __weak void *mrc_cache_current_mmap_leak(int type, uint32_t version, I... am very confused why this is here. The way the old code is written (and the way I think you should write the new version), this shouldn't be needed because it's never calling the function if that Kconfig is disabled. Maybe this is left over from before we had --gc-sections enabled on x86 and can just be removed?
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/ironl... PS7, Line 1626: &mrc_size Are you not allowed to pass NULL here if you don't need it? We should write the function so it's legal to pass NULL here, for convenience in cases like this.
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/sandy... PS7, Line 278: This is missing the size check?
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/amd/common/block/s3... PS7, Line 36: size = mrc_size; Just pass `size` in directly?
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/intel/apollolake/ro... PS7, Line 319: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); There was never a reason for these assert()s to be guarded, I'm not sure why a lot of this code is ordering it so weirdly (probably all copy&pasted from the same). You can just move this above (or below or wherever).