Change in coreboot[master]: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_TABLE

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_TABLE
......................................................................

soc/amd/common: Avoid stage_cache if !HAVE_ACPI_TABLE

This patch skips stage cache function calls if HAVE_ACPI_TABLE
not enable.

Change-Id: I620429d05f924b457d7d1b34fc242a621f273191
Signed-off-by: Subrata Banik 
---
M src/soc/amd/common/block/pi/agesawrapper.c
M src/soc/amd/common/block/s3/Makefile.inc
2 files changed, 21 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34369/1

diff --git a/src/soc/amd/common/block/pi/agesawrapper.c b/src/soc/amd/common/block/pi/agesawrapper.c
index a39e29f..2996290 100644
--- a/src/soc/amd/common/block/pi/agesawrapper.c
+++ b/src/soc/amd/common/block/pi/agesawrapper.c
@@ -312,12 +312,13 @@
    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))
-		printk(BIOS_ERR, "S3 data not saved, resuming impossible\n");
-
+	if (CONFIG(HAVE_ACPI_RESUME)) {
+		if (save_s3_info(RtbParams->S3DataBlock.NvStorage,
+				 RtbParams->S3DataBlock.NvStorageSize,
+				 RtbParams->S3DataBlock.VolatileStorage,
+				 RtbParams->S3DataBlock.VolatileStorageSize))
+			printk(BIOS_ERR, "S3 data not saved, resuming impossible\n");
+	}
    return Status;
 }

@@ -326,8 +327,10 @@
    AGESA_STATUS status;
    size_t nv_size;

-	get_s3nv_info(&InitResumeParams->S3DataBlock.NvStorage, &nv_size);
-	InitResumeParams->S3DataBlock.NvStorageSize = nv_size;
+	if (CONFIG(HAVE_ACPI_RESUME)) {
+		get_s3nv_info(&InitResumeParams->S3DataBlock.NvStorage, &nv_size);
+		InitResumeParams->S3DataBlock.NvStorageSize = nv_size;
+	}

timestamp_add_now(TS_AGESA_INIT_RESUME_START);
    status = amd_dispatch(InitResumeParams);
@@ -343,8 +346,10 @@

amd_initcpuio();

-	get_s3vol_info(&S3LateParams->S3DataBlock.VolatileStorage, &vol_size);
-	S3LateParams->S3DataBlock.VolatileStorageSize = vol_size;
+	if (CONFIG(HAVE_ACPI_RESUME)) {
+		get_s3vol_info(&S3LateParams->S3DataBlock.VolatileStorage, &vol_size);
+		S3LateParams->S3DataBlock.VolatileStorageSize = vol_size;
+	}

timestamp_add_now(TS_AGESA_S3_LATE_START);
    Status = amd_dispatch(S3LateParams);
@@ -358,8 +363,10 @@
    AGESA_STATUS Status;
    size_t vol_size;

-	get_s3vol_info(&S3FinalParams->S3DataBlock.VolatileStorage, &vol_size);
-	S3FinalParams->S3DataBlock.VolatileStorageSize = vol_size;
+	if (CONFIG(HAVE_ACPI_RESUME)) {	
+		get_s3vol_info(&S3FinalParams->S3DataBlock.VolatileStorage, &vol_size);
+		S3FinalParams->S3DataBlock.VolatileStorageSize = vol_size;
+	}

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 9efc6bc..7d950b0 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-y += s3_resume.c
-ramstage-y += s3_resume.c
+romstage-$(CONFIG_HAVE_ACPI_RESUME) += s3_resume.c
+ramstage-$(CONFIG_HAVE_ACPI_RESUME) += s3_resume.c

endif

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34369

to look at the new patch set (#2).

Change subject: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_TABLE
......................................................................

soc/amd/common: Avoid stage_cache if !HAVE_ACPI_TABLE

This patch skips stage cache function calls if HAVE_ACPI_TABLE
not enable.

Change-Id: I620429d05f924b457d7d1b34fc242a621f273191
Signed-off-by: Subrata Banik 
---
M src/soc/amd/common/block/pi/agesawrapper.c
M src/soc/amd/common/block/s3/Makefile.inc
2 files changed, 21 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34369/2

Change in coreboot[master]: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Marshall Dawson, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34369

to look at the new patch set (#3).

Change subject: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME
......................................................................

soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME

This patch skips stage cache function calls if HAVE_ACPI_RESUME
not enable.

Change-Id: I620429d05f924b457d7d1b34fc242a621f273191
Signed-off-by: Subrata Banik 
---
M src/soc/amd/common/block/pi/agesawrapper.c
M src/soc/amd/common/block/s3/Makefile.inc
2 files changed, 21 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34369/3

Change in coreboot[master]: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
File src/soc/amd/common/block/pi/agesawrapper.c:

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 314: 
How about instead, insert:

if (!CONFIG(HAVE_ACPI_RESUME)
    return Status;

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 329: 
How about instead, insert:

if (!CONFIG(HAVE_ACPI_RESUME)
    return AGESA_UNSUPPORTED;

I'm not too worried about an error message, BTW.  The call to this function gets removed from romstage when CONFIG_HAVE_ACPI_RESUME is not y.

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 346: 
How about instead, insert:

if (!CONFIG(HAVE_ACPI_RESUME)
    return AGESA_UNSUPPORTED;

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 365: 
same as above.

Change in coreboot[master]: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME
......................................................................

Patch Set 3: Code-Review-1

Those are not stage cache function calls you touch here, and they are not strictly S3 related, despite the names.

Change in coreboot[master]: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Avoid stage_cache if !HAVE_ACPI_RESUME
......................................................................

Patch Set 3:
> Those are not stage cache function calls you touch here, and they are not strictly S3 related, despite the names.

Kyosti, was it the commit message you don't like?  Or is there a problem with the nature of the changes WRT HAVE_ACPI_RESUME?  I don't follow your concern about not strictly S3 related, with the exception of InitRtb as I'd mentioned.

Change in coreboot[master]: soc/amd/common: Skip S3 related function call if !HAVE_ACPI_RESUME

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Marshall Dawson, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34369

to look at the new patch set (#4).

Change subject: soc/amd/common: Skip S3 related function call if !HAVE_ACPI_RESUME
......................................................................

soc/amd/common: Skip S3 related function call if !HAVE_ACPI_RESUME

This patch skips save_s3_info()/get_s3nv_info()/get_s3vol_info()
function calls if HAVE_ACPI_RESUME not enable.

Change-Id: I620429d05f924b457d7d1b34fc242a621f273191
Signed-off-by: Subrata Banik 
---
M src/soc/amd/common/block/pi/agesawrapper.c
M src/soc/amd/common/block/s3/Makefile.inc
2 files changed, 14 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34369/4

Change in coreboot[master]: soc/amd/common: Skip S3 related function call if !HAVE_ACPI_RESUME

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Skip S3 related function call if !HAVE_ACPI_RESUME
......................................................................

Patch Set 4:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
File src/soc/amd/common/block/pi/agesawrapper.c:

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 314:
> How about instead, insert: […]

Done

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 329:
> How about instead, insert: […]

Done

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 346:
> How about instead, insert: […]

Done

https://review.coreboot.org/c/coreboot/+/34369/3/src/soc/amd/common/block/pi... 
PS3, Line 365:
> same as above.

Done

Change in coreboot[master]: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_...

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Marshall Dawson, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34369

to look at the new patch set (#5).

Change subject: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_ACPI_RESUME
......................................................................

soc/amd/common: Skip S3 save/restore related function calls if !HAVE_ACPI_RESUME

This patch skips save_s3_info()/get_s3nv_info()/get_s3vol_info() [Referred
from s3_resume.c] function calls if HAVE_ACPI_RESUME not enable.

Change-Id: I620429d05f924b457d7d1b34fc242a621f273191
Signed-off-by: Subrata Banik 
---
M src/soc/amd/common/block/pi/agesawrapper.c
M src/soc/amd/common/block/s3/Makefile.inc
2 files changed, 14 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/34369/5

Change in coreboot[master]: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_...

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_ACPI_RESUME
......................................................................

Patch Set 5: Code-Review-1

There are empty stubs in stage_cache.h for the case with cache disabled, no need for the extra 
guards here.

Further comments in CB:31490

Change in coreboot[master]: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_...

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_ACPI_RESUME
......................................................................

Patch Set 5:
> Patch Set 5: Code-Review-1
>
> There are empty stubs in stage_cache.h for the case with cache disabled, no need for the extra 
> guards here.
>
> Further comments in CB:31490

i'm happy to abandon this CL. Kindly consider my CB:33116 is now dependent on your CL CB:31490

Change in coreboot[master]: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_...

[Subrata Banik (Code Review)]

Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34369 )

Change subject: soc/amd/common: Skip S3 save/restore related function calls if !HAVE_ACPI_RESUME
......................................................................

Abandoned