Shelley Chen 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 9:
(12 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp1_1/ro... PS8, Line 61: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
nit: I think this should be moved above the call to mrc_cache_current_mmap_leak().
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp2_0/me... PS8, Line 121: /* Assume boot device is memory mapped. */ : assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
nit: I think this should be moved above the call to mrc_cache_current_mmap_leak().
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: BIOS_ERR
Is this really an error? MRC might not have valid data and that is fine e.g. […]
In mrc_data_valid(), we originally pasted this error
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: on data verification
Why "on data verification"?
Sorry, copy/pasted the error message from mrc_data_valid() in the old driver.
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 436: if (mrc_cache_get_latest_slot_info(cr->name,
There is a change in logic here i.e. current mrc cache is not checked to be valid. […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@38 PS8, Line 38: so it is up to the : * caller to unmap if necessary.
Technically you could do rdev_unmap(boot_device_ro(), mapping) since rdev ops are always only tied t […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@43 PS8, Line 43: data_size
I think I would prefer keeping this consistent with the underlying rdev API to be honest. […]
I thought that we had agreed to return the void * in the past? Furquan, do you not agree with this anymore?
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/amd/common/block/s3... PS8, Line 32: if (!base)
This is checked again in line 35.
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/apollolake/ro... PS8, Line 315: &mrc_size
This is unused. […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/baytrail/roms... PS8, Line 140: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
Same here.
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... File src/soc/intel/broadwell/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... PS8, Line 46: !pei_data->saved_data
This check doesn't look right. This should be: `if (pei_data->saved_data)` i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... PS8, Line 50: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
Same comment as other files.
Done