Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33956
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
drivers/amd/agesa: Drop redundant stack allocation
The removed call was there to support case LATE_CBMEM_INIT=y, HAVE_ACPI_RESUME=y. Same stack space is already allocated with postcar_frame_init() call.
Change-Id: I03a44bc3252f553b1769d362b2f442d3e6ab73f4 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/amd/agesa/oem_s3.c 1 file changed, 0 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/33956/1
diff --git a/src/drivers/amd/agesa/oem_s3.c b/src/drivers/amd/agesa/oem_s3.c index acd34b8..9514bcd 100644 --- a/src/drivers/amd/agesa/oem_s3.c +++ b/src/drivers/amd/agesa/oem_s3.c @@ -18,7 +18,6 @@ #include <string.h> #include <cbmem.h> #include <console/console.h> -#include <program_loading.h> #include <northbridge/amd/agesa/state_machine.h> #include <AGESA.h> #include <northbridge/amd/agesa/agesa_helper.h> @@ -122,8 +121,6 @@ u32 MTRRStorageSize = 0; uintptr_t pos, size;
- romstage_ram_stack_base(HIGH_ROMSTAGE_STACK_SIZE, ROMSTAGE_STACK_CBMEM); - /* To be consumed in AmdInitResume. */ get_s3nv_data(S3DataTypeNonVolatile, &pos, &size); if (size && dataBlock->NvStorageSize)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2:
AGESA resume from S3 still works with apu1 with this.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2: Code-Review+1
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2: Code-Review+1
AGESA resume from S3 still works with G505S with this. Although - if combined with a new "CONFIG_SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT" - S3 stops working (haven't tested this config alone yet).
Always clear the DRAM after DRAM initialization regardless of additional security implementations in use.
This increases boot time depending on the amount of DRAM installed.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+1
AGESA resume from S3 still works with G505S with this. Although - if combined with a new "CONFIG_SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT" - S3 stops working (haven't tested this config alone yet).
Always clear the DRAM after DRAM initialization regardless of additional security implementations in use.
This increases boot time depending on the amount of DRAM installed.
AGESA has valuables in low memory and coreboot does not really keep account of them. I think it's BIOS_HEAP_START_ADDRESS + _SIZE you are not allowed to clear. So I expect S3 resume with that CLEAR_DRAM feature enabled to fail regardless of this commit.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review+1
AGESA resume from S3 still works with G505S with this. Although - if combined with a new "CONFIG_SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT" - S3 stops working (haven't tested this config alone yet).
Always clear the DRAM after DRAM initialization regardless of additional security implementations in use.
This increases boot time depending on the amount of DRAM installed.
AGESA has valuables in low memory and coreboot does not really keep account of them. I think it's BIOS_HEAP_START_ADDRESS + _SIZE you are not allowed to clear. So I expect S3 resume with that CLEAR_DRAM feature enabled to fail regardless of this commit.
Yes, indeed S3 is always failing if DRAM clearing is enabled, even without your commit.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review+1
AGESA resume from S3 still works with G505S with this. Although - if combined with a new "CONFIG_SECURITY_CLEAR_DRAM_ON_REGULAR_BOOT" - S3 stops working (haven't tested this config alone yet).
Always clear the DRAM after DRAM initialization regardless of additional security implementations in use.
This increases boot time depending on the amount of DRAM installed.
AGESA has valuables in low memory and coreboot does not really keep account of them. I think it's BIOS_HEAP_START_ADDRESS + _SIZE you are not allowed to clear. So I expect S3 resume with that CLEAR_DRAM feature enabled to fail regardless of this commit.
Yes, indeed S3 is always failing if DRAM clearing is enabled, even without your commit.
Thanks for testing!
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33956 )
Change subject: drivers/amd/agesa: Drop redundant stack allocation ......................................................................
drivers/amd/agesa: Drop redundant stack allocation
The removed call was there to support case LATE_CBMEM_INIT=y, HAVE_ACPI_RESUME=y. Same stack space is already allocated with postcar_frame_init() call.
Change-Id: I03a44bc3252f553b1769d362b2f442d3e6ab73f4 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33956 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Mike Banon mikebdp2@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/amd/agesa/oem_s3.c 1 file changed, 0 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Mike Banon: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/drivers/amd/agesa/oem_s3.c b/src/drivers/amd/agesa/oem_s3.c index acd34b8..9514bcd 100644 --- a/src/drivers/amd/agesa/oem_s3.c +++ b/src/drivers/amd/agesa/oem_s3.c @@ -18,7 +18,6 @@ #include <string.h> #include <cbmem.h> #include <console/console.h> -#include <program_loading.h> #include <northbridge/amd/agesa/state_machine.h> #include <AGESA.h> #include <northbridge/amd/agesa/agesa_helper.h> @@ -122,8 +121,6 @@ u32 MTRRStorageSize = 0; uintptr_t pos, size;
- romstage_ram_stack_base(HIGH_ROMSTAGE_STACK_SIZE, ROMSTAGE_STACK_CBMEM); - /* To be consumed in AmdInitResume. */ get_s3nv_data(S3DataTypeNonVolatile, &pos, &size); if (size && dataBlock->NvStorageSize)