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 10: Code-Review+2
(2 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 665: 0
I do think that we should keep the return type though. Otherwise how will we indicate errors?
The way the function is written, there is only one case where this function can fail i.e. when `cbmem_add()` fails. I see that most callers of this function don't really do anything with the return value. Some print out an error and only 2 platforms take action: 1. qualcomm - this asserts in case there is a failure 2. amd - this actually skips stashing of volatile data to stage cache.
I think we can take a common action within this function -- assert if cbmem_add fails. There is no other action taken by any other platform anyways.
We can go ahead with what you have right now i.e. printing out a message that cache region wasn't found and return 0. But, I think we should fix this in a separate CL.
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46855/10/src/drivers/mrc_cache/mrc_... PS10, Line 708: "MRC: No region type found. " : "Skip adding to cbmem for type %d.\n", No need to break print strings. They can be longer than the column limit.