Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/pi... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/pi... PS6, Line 318: if (OemS3Save(&RtbParams->S3DataBlock) != AGESA_SUCCESS)
While it is called OemS3Save, this step saves memory training data to SPI and is not strictly about […]
I believe the spec has to do with systems where memory DIMM can be changed. Not always the case with embedded, but who knows? Some OEM/ODM might want to allow it. When resuming from S3 it's assumed HW did not change... no guarantee with normal cold boot. That's the reason it's called OemS3Save... it'll not be used in cold boots. So I believe the name is ok.
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/s3... PS6, Line 72: stage_cache_get_raw(STAGE_S3_DATA, &base, &size);
Never reached with HAVE_ACPI_RESUME=n, ahould link just fine with empty stubs in stage_cache.h.
I believe the linker will not include it on the image if it's never called, with end result similar to using empty stubs. SO what do you actually want to achieve with the change (which I believe would be harmless)? Just make it obvious?