Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31490
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
soc/amd/common: Refactor S3 helpers
Change-Id: I1506ee2f7ecf3cb6ec4cce37a030c05f78ec6d59 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/s3_resume.h M src/soc/amd/common/block/pi/agesawrapper.c M src/soc/amd/common/block/s3/Makefile.inc M src/soc/amd/common/block/s3/s3_resume.c 4 files changed, 53 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/31490/1
diff --git a/src/soc/amd/common/block/include/amdblocks/s3_resume.h b/src/soc/amd/common/block/include/amdblocks/s3_resume.h index 13f8010..9323baf 100644 --- a/src/soc/amd/common/block/include/amdblocks/s3_resume.h +++ b/src/soc/amd/common/block/include/amdblocks/s3_resume.h @@ -17,10 +17,10 @@ #define __AMD_S3_RESUME_H__
#include <stdint.h> +#include <agesa_headers.h>
-int save_s3_info(void *nv_base, size_t nv_size, - void *vol_base, size_t vol_size); -void get_s3nv_info(void **base, size_t *size); -void get_s3vol_info(void **base, size_t *size); +AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); +AGESA_STATUS OemS3LateRestore(S3_DATA_BLOCK *dataBlock); +AGESA_STATUS OemS3Save(S3_DATA_BLOCK *dataBlock);
#endif /* __AMD_S3_RESUME_H__ */ diff --git a/src/soc/amd/common/block/pi/agesawrapper.c b/src/soc/amd/common/block/pi/agesawrapper.c index e769a45..1579b0b 100644 --- a/src/soc/amd/common/block/pi/agesawrapper.c +++ b/src/soc/amd/common/block/pi/agesawrapper.c @@ -313,10 +313,8 @@ Status = amd_dispatch(RtbParams); timestamp_add_now(TS_AGESA_INIT_RTB_DONE);
- if (save_s3_info(RtbParams->S3DataBlock.NvStorage, - RtbParams->S3DataBlock.NvStorageSize, - RtbParams->S3DataBlock.VolatileStorage, - RtbParams->S3DataBlock.VolatileStorageSize)) + Status = OemS3Save(&RtbParams->S3DataBlock); + if (Status != AGESA_SUCCESS) printk(BIOS_ERR, "S3 data not saved, resuming impossible\n");
return Status; @@ -325,10 +323,8 @@ static AGESA_STATUS amd_init_resume(AMD_RESUME_PARAMS *InitResumeParams) { AGESA_STATUS status; - size_t nv_size;
- get_s3nv_info(&InitResumeParams->S3DataBlock.NvStorage, &nv_size); - InitResumeParams->S3DataBlock.NvStorageSize = nv_size; + OemInitResume(&InitResumeParams->S3DataBlock);
timestamp_add_now(TS_AGESA_INIT_RESUME_START); status = amd_dispatch(InitResumeParams); @@ -340,12 +336,10 @@ static AGESA_STATUS amd_s3late_restore(AMD_S3LATE_PARAMS *S3LateParams) { AGESA_STATUS Status; - size_t vol_size;
amd_initcpuio();
- get_s3vol_info(&S3LateParams->S3DataBlock.VolatileStorage, &vol_size); - S3LateParams->S3DataBlock.VolatileStorageSize = vol_size; + OemS3LateRestore(&S3LateParams->S3DataBlock);
timestamp_add_now(TS_AGESA_S3_LATE_START); Status = amd_dispatch(S3LateParams); @@ -357,10 +351,8 @@ static AGESA_STATUS amd_s3final_restore(AMD_S3FINAL_PARAMS *S3FinalParams) { AGESA_STATUS Status; - size_t vol_size;
- get_s3vol_info(&S3FinalParams->S3DataBlock.VolatileStorage, &vol_size); - S3FinalParams->S3DataBlock.VolatileStorageSize = vol_size; + OemS3LateRestore(&S3FinalParams->S3DataBlock);
timestamp_add_now(TS_AGESA_S3_FINAL_START); Status = amd_dispatch(S3FinalParams); diff --git a/src/soc/amd/common/block/s3/Makefile.inc b/src/soc/amd/common/block/s3/Makefile.inc index 7d950b0..9efc6bc 100644 --- a/src/soc/amd/common/block/s3/Makefile.inc +++ b/src/soc/amd/common/block/s3/Makefile.inc @@ -1,6 +1,6 @@ ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_S3),y)
-romstage-$(CONFIG_HAVE_ACPI_RESUME) += s3_resume.c -ramstage-$(CONFIG_HAVE_ACPI_RESUME) += s3_resume.c +romstage-y += s3_resume.c +ramstage-y += s3_resume.c
endif diff --git a/src/soc/amd/common/block/s3/s3_resume.c b/src/soc/amd/common/block/s3/s3_resume.c index eb0148f..818bde2 100644 --- a/src/soc/amd/common/block/s3/s3_resume.c +++ b/src/soc/amd/common/block/s3/s3_resume.c @@ -31,49 +31,66 @@ board_reset(); }
-void get_s3nv_info(void **base, size_t *size) +AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock) { + void *base; + size_t size; + int i; + uint32_t erased = 0xffffffff; struct region_device rdev;
if (mrc_cache_get_current(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, &rdev)) reboot_from_resume("mrc_cache_get_current error, rebooting.\n");
- *base = rdev_mmap_full(&rdev); - *size = region_device_sz(&rdev); - if (!*base || !*size) + base = rdev_mmap_full(&rdev); + size = region_device_sz(&rdev); + if (!base || !size) reboot_from_resume("Error: S3 NV data not found, rebooting.\n");
/* Read 16 bytes to infer if the NV has been erased from flash. */ - int i; - uint32_t erased = 0xffffffff; for (i = 0; i < 4; i++) - erased &= read32((uint32_t *)*base + i); - + erased &= read32((uint32_t *)base + i); if (erased == 0xffffffff) reboot_from_resume("Error: S3 NV data invalid, rebooting.\n");
- printk(BIOS_SPEW, "S3 NV data @0x%p, 0x%0zx bytes\n", *base, *size); + dataBlock->NvStorage = base; + dataBlock->NvStorageSize = size; + printk(BIOS_SPEW, "S3 NV data @0x%p, 0x%0zx bytes\n", + dataBlock->NvStorage, (size_t)dataBlock->NvStorageSize); + + return AGESA_SUCCESS; }
-void get_s3vol_info(void **base, size_t *size) +AGESA_STATUS OemS3LateRestore(S3_DATA_BLOCK *dataBlock) { - stage_cache_get_raw(STAGE_S3_DATA, base, size); - if (!*base || !*size) + void *base; + size_t size; + + stage_cache_get_raw(STAGE_S3_DATA, &base, &size); + if (!base || !size) { printk(BIOS_ERR, "Error: S3 volatile data not found\n"); - else - printk(BIOS_SPEW, "S3 volatile data @0x%p 0x%0zx total bytes\n", - *base, *size); -} - -int save_s3_info(void *nv_base, size_t nv_size, void *vol_base, size_t vol_size) -{ - if (mrc_cache_stash_data(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, - nv_base, nv_size) < 0) { - printk(BIOS_ERR, "Failed to stash MRC data\n"); - return -1; + return AGESA_FATAL; }
- stage_cache_add_raw(STAGE_S3_DATA, vol_base, vol_size); - return 0; + dataBlock->VolatileStorage = base; + dataBlock->VolatileStorageSize = size; + printk(BIOS_SPEW, "S3 volatile data @0x%p, 0x%0zx bytes\n", + dataBlock->VolatileStorage, (size_t)dataBlock->VolatileStorageSize); + + return AGESA_SUCCESS; +} + +AGESA_STATUS OemS3Save(S3_DATA_BLOCK *dataBlock) +{ + if (mrc_cache_stash_data(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, + dataBlock->NvStorage, dataBlock->NvStorageSize) < 0) { + printk(BIOS_ERR, "Failed to stash MRC data\n"); + return AGESA_CRITICAL; + } + + stage_cache_add_raw(STAGE_S3_DATA, dataBlock->VolatileStorage, + dataBlock->VolatileStorageSize); + + return AGESA_SUCCESS; }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31490/2/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/#/c/31490/2/src/soc/amd/common/block/include/amd... PS2, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
https://review.coreboot.org/#/c/31490/2/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31490/2/src/soc/amd/common/block/s3/s3_resum... PS2, Line 79: dataBlock->VolatileStorage, (size_t)dataBlock->VolatileStorageSize); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31490/3/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/#/c/31490/3/src/soc/amd/common/block/include/amd... PS3, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
https://review.coreboot.org/#/c/31490/3/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31490/3/src/soc/amd/common/block/s3/s3_resum... PS3, Line 80: dataBlock->VolatileStorage, (size_t)dataBlock->VolatileStorageSize); line over 80 characters
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 3: Code-Review+1
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/31490/4/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/#/c/31490/4/src/soc/amd/common/block/include/amd... PS4, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
https://review.coreboot.org/#/c/31490/4/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31490/4/src/soc/amd/common/block/s3/s3_resum... PS4, Line 80: dataBlock->VolatileStorage, (size_t)dataBlock->VolatileStorageSize); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/include/amd... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/include/amd... PS5, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/s3/s3_resum... PS5, Line 80: dataBlock->VolatileStorage, (size_t)dataBlock->VolatileStorageSize); line over 80 characters
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 5:
(1 comment)
Hi Marshall, this code was created by you. Please review.
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 315: Status = OemS3Save(&RtbParams->S3DataBlock); Status is supposed to save the return of amd_dispatch, don't use it here. Instead use: if(OemS3Save(&RtbParams->S3DataBlock)) AGESA_SUCCESS = 0.
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 5:
Ping Marshall? This code is yours, and I'm not comfortable approving changes to it.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 5:
(1 comment)
I'm fine with the rewrite, subject to what Richard identified.
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 315: Status = OemS3Save(&RtbParams->S3DataBlock);
Status is supposed to save the return of amd_dispatch, don't use it here. Instead use: […]
I'd be reluctant to rely on AGESA_SUCCESS being 0 like that. Maybe instead, check Status after amd_dispatch() and if not SUCCESS, print an error and return early.
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 5:
(1 comment)
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 315: Status = OemS3Save(&RtbParams->S3DataBlock);
I'd be reluctant to rely on AGESA_SUCCESS being 0 like that. […]
I believe most codes consider 0 as success, because it leaves a wide range for different errors. As fs for what I wrote, I was only explaining why that particular if form would work. That form was how original code has been written. OTOH, as Marshall properly pointed out, the only reason a status is returned is for a common print if error, so I would be happy with a print after amd_dispatch.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 5:
(1 comment)
Does Stoney currently update the S3DataBlock in SPI on every cold boot? I remember discussion before that MRC cache fastboot is not supported.
Maybe it would be weird/risky to resume from S3 with a different set of parameters compared to what the cold boot used though. Nevertheless, those who have no need for S3 might prefer the cold boots with SPI part write-protected, skipping AMD_INIT_RTB.
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 315: Status = OemS3Save(&RtbParams->S3DataBlock);
I believe most codes consider 0 as success, because it leaves a wide range for different errors. […]
Right, OemS3Save() should only be called if amd_dispatch() does not return errors so we know S3DataBlock is valid
In an odd case where OemS3Save fails (unable to write SPI) it would be preferred to runtime disable S3 feature in ACPI. But we don't have the necessary logic ready for that (has been on my TODO for long time).
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 5:
(1 comment)
ou
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/#/c/31490/5/src/soc/amd/common/block/pi/agesawra... PS5, Line 315: Status = OemS3Save(&RtbParams->S3DataBlock);
Right, OemS3Save() should only be called if amd_dispatch() does not return errors so we know S3DataB […]
That would be acceptable. Please implement code in which OemS3Save is only called if dispatch did not returned error.
Hello Marshall Dawson, Richard Spiegel, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31490
to look at the new patch set (#6).
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
soc/amd/common: Refactor S3 helpers
Make the prototypes match what drivers/amd/agesa would rather see, in preparation to use the same code with open-source AGESA.
Change-Id: I1506ee2f7ecf3cb6ec4cce37a030c05f78ec6d59 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/s3_resume.h M src/soc/amd/common/block/pi/agesawrapper.c M src/soc/amd/common/block/s3/s3_resume.c 3 files changed, 53 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/31490/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/in... PS6, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
(4 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 HAVE_ACPI_RESUME=y and definetly not only about stage_cache.
(Yes, PI build may be limited for us to be able to use it, we should still follow the specs.)
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.
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/s3... PS6, Line 88: if (mrc_cache_stash_data(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, This part is about SPI flash, for MRC cache. Maybe someone did poor decisions and accepted AMD not to support this with stoneyridge, but it is part of the specs regardless.
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/s3... PS6, Line 94: stage_cache_add_raw(STAGE_S3_DATA, dataBlock->VolatileStorage, Empty stub function in stage_cache.h so this becomes no-op, just fine as-is.
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?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
(1 comment)
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)
I believe the spec has to do with systems where memory DIMM can be changed. […]
Memory training data is also useful on cold boots: if the memory is the same, why waste time retraining the same thing again?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
(1 comment)
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)
Memory training data is also useful on cold boots: if the memory is the same, why waste time retrain […]
Unfortunately, stoney had no way of loading the timing data back into one of the memory components on a cold boot, so the memory training data was only useful on resume from S3. I don't remember if it was the PHY or something else, but we tried to reuse the memory timings at boot as we do on intel platform, and were told that it wasn't possible.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
This file is just misplaced under soc/ when it is common to AGESAv5. While named s3_resume, functionality of storing non-volatile section and use it for cold boots is used with family16kb.
For the record; I shall be blocking attempts to place AGESAv9 (?) support code elsewhere other than under src/drivers/amd/. We have established directory layout with FSP, follow it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
Just a reminder!
do we have some sight on this CL. stage_cache original implementation is pending on this
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 6:
Patch Set 6:
Just a reminder!
do we have some sight on this CL. stage_cache original implementation is pending on this
Ping! do we know the next step here? This CL is pending for a while and blocked remaining CL in tree. can we go ahead with https://review.coreboot.org/c/coreboot/+/34369 and unblock original purpose ?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31490/7/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/c/coreboot/+/31490/7/src/soc/amd/common/block/in... PS7, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/31490/5/src/soc/amd/common/block/pi... File src/soc/amd/common/block/pi/agesawrapper.c:
https://review.coreboot.org/c/coreboot/+/31490/5/src/soc/amd/common/block/pi... PS5, Line 315: Status = OemS3Save(&RtbParams->S3DataBlock);
That would be acceptable. […]
Done
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)
Unfortunately, stoney had no way of loading the timing data back into one of the memory components o […]
The code here should replace AGESA OemS3Save() which also works for fastboot aka MRC cache with fam16kb.
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);
I believe the linker will not include it on the image if it's never called, with end result similar […]
There is no change here. My comment was for Subrata to tell that there is no need to add any if (CONFIG(HAVE_ACPI_RESUME)) condition here.
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/s3... PS6, Line 88: if (mrc_cache_stash_data(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION,
This part is about SPI flash, for MRC cache. […]
Done
https://review.coreboot.org/c/coreboot/+/31490/6/src/soc/amd/common/block/s3... PS6, Line 94: stage_cache_add_raw(STAGE_S3_DATA, dataBlock->VolatileStorage,
Empty stub function in stage_cache.h so this becomes no-op, just fine as-is.
Comment for Subrata.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 7:
Patch Set 6:
Just a reminder!
do we have some sight on this CL. stage_cache original implementation is pending on this
Which change is pending? Link please.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 6:
Just a reminder!
do we have some sight on this CL. stage_cache original implementation is pending on this
Which change is pending? Link please.
here you go https://review.coreboot.org/c/coreboot/+/33116
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
Just a reminder!
do we have some sight on this CL. stage_cache original implementation is pending on this
Which change is pending? Link please.
here you go https://review.coreboot.org/c/coreboot/+/33116
CB:34651 should take care of that. I Rebased CB:33116 on that.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 6:
Just a reminder!
do we have some sight on this CL. stage_cache original implementation is pending on this
Which change is pending? Link please.
here you go https://review.coreboot.org/c/coreboot/+/33116
CB:34651 should take care of that. I Rebased CB:33116 on that.
Thanks a lot. I will wait for bot +1 then i have 1 small comment to address from Furquan on patchset 24
Kyösti Mälkki has removed Subrata Banik from this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Removed reviewer Subrata Banik.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31490/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/c/coreboot/+/31490/8/src/soc/amd/common/block/in... PS8, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
Hello Marshall Dawson, Richard Spiegel, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31490
to look at the new patch set (#9).
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
soc/amd/common: Refactor S3 helpers
Make the prototypes match what drivers/amd/agesa would rather see, in preparation to use the same code with open-source AGESA.
Change-Id: I1506ee2f7ecf3cb6ec4cce37a030c05f78ec6d59 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/s3_resume.h M src/soc/amd/common/block/pi/agesawrapper.c M src/soc/amd/common/block/s3/s3_resume.c 3 files changed, 54 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/31490/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31490/9/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/c/coreboot/+/31490/9/src/soc/amd/common/block/in... PS9, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 9: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31490/10/src/soc/amd/common/block/i... File src/soc/amd/common/block/include/amdblocks/s3_resume.h:
https://review.coreboot.org/c/coreboot/+/31490/10/src/soc/amd/common/block/i... PS10, Line 22: AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); need consistent spacing around '*' (ctx:WxV)
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31490 )
Change subject: soc/amd/common: Refactor S3 helpers ......................................................................
soc/amd/common: Refactor S3 helpers
Make the prototypes match what drivers/amd/agesa would rather see, in preparation to use the same code with open-source AGESA.
Change-Id: I1506ee2f7ecf3cb6ec4cce37a030c05f78ec6d59 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31490 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/common/block/include/amdblocks/s3_resume.h M src/soc/amd/common/block/pi/agesawrapper.c M src/soc/amd/common/block/s3/s3_resume.c 3 files changed, 54 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/common/block/include/amdblocks/s3_resume.h b/src/soc/amd/common/block/include/amdblocks/s3_resume.h index 13f8010..9323baf 100644 --- a/src/soc/amd/common/block/include/amdblocks/s3_resume.h +++ b/src/soc/amd/common/block/include/amdblocks/s3_resume.h @@ -17,10 +17,10 @@ #define __AMD_S3_RESUME_H__
#include <stdint.h> +#include <agesa_headers.h>
-int save_s3_info(void *nv_base, size_t nv_size, - void *vol_base, size_t vol_size); -void get_s3nv_info(void **base, size_t *size); -void get_s3vol_info(void **base, size_t *size); +AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock); +AGESA_STATUS OemS3LateRestore(S3_DATA_BLOCK *dataBlock); +AGESA_STATUS OemS3Save(S3_DATA_BLOCK *dataBlock);
#endif /* __AMD_S3_RESUME_H__ */ diff --git a/src/soc/amd/common/block/pi/agesawrapper.c b/src/soc/amd/common/block/pi/agesawrapper.c index a39e29f..c5464df 100644 --- a/src/soc/amd/common/block/pi/agesawrapper.c +++ b/src/soc/amd/common/block/pi/agesawrapper.c @@ -312,10 +312,10 @@ Status = amd_dispatch(RtbParams); timestamp_add_now(TS_AGESA_INIT_RTB_DONE);
- if (save_s3_info(RtbParams->S3DataBlock.NvStorage, - RtbParams->S3DataBlock.NvStorageSize, - RtbParams->S3DataBlock.VolatileStorage, - RtbParams->S3DataBlock.VolatileStorageSize)) + if (Status != AGESA_SUCCESS) + return Status; + + if (OemS3Save(&RtbParams->S3DataBlock) != AGESA_SUCCESS) printk(BIOS_ERR, "S3 data not saved, resuming impossible\n");
return Status; @@ -324,10 +324,8 @@ static AGESA_STATUS amd_init_resume(AMD_RESUME_PARAMS *InitResumeParams) { AGESA_STATUS status; - size_t nv_size;
- get_s3nv_info(&InitResumeParams->S3DataBlock.NvStorage, &nv_size); - InitResumeParams->S3DataBlock.NvStorageSize = nv_size; + OemInitResume(&InitResumeParams->S3DataBlock);
timestamp_add_now(TS_AGESA_INIT_RESUME_START); status = amd_dispatch(InitResumeParams); @@ -339,12 +337,10 @@ static AGESA_STATUS amd_s3late_restore(AMD_S3LATE_PARAMS *S3LateParams) { AGESA_STATUS Status; - size_t vol_size;
amd_initcpuio();
- get_s3vol_info(&S3LateParams->S3DataBlock.VolatileStorage, &vol_size); - S3LateParams->S3DataBlock.VolatileStorageSize = vol_size; + OemS3LateRestore(&S3LateParams->S3DataBlock);
timestamp_add_now(TS_AGESA_S3_LATE_START); Status = amd_dispatch(S3LateParams); @@ -356,10 +352,8 @@ static AGESA_STATUS amd_s3final_restore(AMD_S3FINAL_PARAMS *S3FinalParams) { AGESA_STATUS Status; - size_t vol_size;
- get_s3vol_info(&S3FinalParams->S3DataBlock.VolatileStorage, &vol_size); - S3FinalParams->S3DataBlock.VolatileStorageSize = vol_size; + OemS3LateRestore(&S3FinalParams->S3DataBlock);
timestamp_add_now(TS_AGESA_S3_FINAL_START); Status = amd_dispatch(S3FinalParams); diff --git a/src/soc/amd/common/block/s3/s3_resume.c b/src/soc/amd/common/block/s3/s3_resume.c index 74aa79c..598036a 100644 --- a/src/soc/amd/common/block/s3/s3_resume.c +++ b/src/soc/amd/common/block/s3/s3_resume.c @@ -26,56 +26,73 @@ /* Training data versioning is not supported or tracked. */ #define DEFAULT_MRC_VERSION 0
-static void reboot_from_resume(const char *message) /* Does not return */ +static void __noreturn reboot_from_resume(const char *message) { printk(BIOS_ERR, "%s", message); set_pm1cnt_s5(); board_reset(); }
-void get_s3nv_info(void **base, size_t *size) +AGESA_STATUS OemInitResume(S3_DATA_BLOCK *dataBlock) { + void *base; + size_t size; + int i; + uint32_t erased = 0xffffffff; struct region_device rdev;
if (mrc_cache_get_current(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, &rdev)) reboot_from_resume("mrc_cache_get_current error, rebooting.\n");
- *base = rdev_mmap_full(&rdev); - *size = region_device_sz(&rdev); - if (!*base || !*size) + base = rdev_mmap_full(&rdev); + size = region_device_sz(&rdev); + if (!base || !size) reboot_from_resume("Error: S3 NV data not found, rebooting.\n");
/* Read 16 bytes to infer if the NV has been erased from flash. */ - int i; - uint32_t erased = 0xffffffff; for (i = 0; i < 4; i++) - erased &= read32((uint32_t *)*base + i); - + erased &= read32((uint32_t *)base + i); if (erased == 0xffffffff) reboot_from_resume("Error: S3 NV data invalid, rebooting.\n");
- printk(BIOS_SPEW, "S3 NV data @0x%p, 0x%0zx bytes\n", *base, *size); + dataBlock->NvStorage = base; + dataBlock->NvStorageSize = size; + printk(BIOS_SPEW, "S3 NV data @0x%p, 0x%0zx bytes\n", + dataBlock->NvStorage, (size_t)dataBlock->NvStorageSize); + + return AGESA_SUCCESS; }
-void get_s3vol_info(void **base, size_t *size) +AGESA_STATUS OemS3LateRestore(S3_DATA_BLOCK *dataBlock) { - stage_cache_get_raw(STAGE_S3_DATA, base, size); - if (!*base || !*size) + void *base = NULL; + size_t size = 0; + + stage_cache_get_raw(STAGE_S3_DATA, &base, &size); + if (!base || !size) { printk(BIOS_ERR, "Error: S3 volatile data not found\n"); - else - printk(BIOS_SPEW, "S3 volatile data @0x%p 0x%0zx total bytes\n", - *base, *size); -} - -int save_s3_info(void *nv_base, size_t nv_size, void *vol_base, size_t vol_size) -{ - if (mrc_cache_stash_data(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, - nv_base, nv_size) < 0) { - printk(BIOS_ERR, "Failed to stash MRC data\n"); - return -1; + return AGESA_FATAL; }
- stage_cache_add_raw(STAGE_S3_DATA, vol_base, vol_size); - return 0; + dataBlock->VolatileStorage = base; + dataBlock->VolatileStorageSize = size; + printk(BIOS_SPEW, "S3 volatile data @0x%p, 0x%0zx bytes\n", + dataBlock->VolatileStorage, (size_t)dataBlock->VolatileStorageSize); + + return AGESA_SUCCESS; +} + +AGESA_STATUS OemS3Save(S3_DATA_BLOCK *dataBlock) +{ + if (mrc_cache_stash_data(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, + dataBlock->NvStorage, dataBlock->NvStorageSize) < 0) { + printk(BIOS_ERR, "Failed to stash MRC data\n"); + return AGESA_CRITICAL; + } + + stage_cache_add_raw(STAGE_S3_DATA, dataBlock->VolatileStorage, + dataBlock->VolatileStorageSize); + + return AGESA_SUCCESS; }
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Richard Spiegel (Code Review)
-
Subrata Banik (Code Review)