Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46855 )
Change subject: mrc_cache: Move code for triggering memory training into mrc_cache ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... PS6, Line 659: const struct cache_region *cr; : : cr = lookup_region_type(type); Not for this change, but I think this whole `lookup_region_type()` can be moved within the if block for (CONFIG(MRC_STASH_TO_CBMEM)) below on line 677. If we are not stashing to CBMEM, there is no use looking up the region type.
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... PS6, Line 663: failed to I think this should say "skipped" because it is not necessarily a failure if the region type is not found.
https://review.coreboot.org/c/coreboot/+/46855/6/src/drivers/mrc_cache/mrc_c... PS6, Line 665: 0 It is confusing that this returns 0 even though lookup_region_type() returns NULL. Should we instead drop the return type for mrc_cache_stash_data()?