Change in coreboot[master]: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME

[Subrata Banik (Code Review)]

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME

This patch fixes inconsistent issue with stage cache enabling with
HAVE_ACPI_RESUME config enable. Only enable stage cache if
CONFIG_HAVE_ACPI_RESUME=y

Change-Id: I7c3b3ec4642a615e17fb3dbdedca6af8ca95ea2b
Signed-off-by: Subrata Banik 
---
M src/soc/amd/stoneyridge/Kconfig
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/34368/1

diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig
index 78b89e3..73869ec 100644
--- a/src/soc/amd/stoneyridge/Kconfig
+++ b/src/soc/amd/stoneyridge/Kconfig
@@ -60,7 +60,7 @@
    select C_ENVIRONMENT_BOOTBLOCK
    select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
    select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH
-	select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
+	select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if HAVE_ACPI_RESUME
    select PARALLEL_MP
    select PARALLEL_MP_AP_WORK
    select HAVE_SMI_HANDLER

[Subrata Banik (Code Review)]

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1:

looks to me that this amd board is broken too much.. let me explain the problem that i'm seeing,

when i'm building amd/gardenia board, here is my .config

CONFIG_RELOCATABLE_RAMSTAGE=y
CONFIG_RELOCATABLE_MODULES=y
CONFIG_NO_STAGE_CACHE=y

now src/lib/Makefile.inc +174 will include cbmem_stage_cache.c although NO_STAGE_CACHE=y

else
ramstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
romstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
postcar-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
endif

hence below stage_cache function call from src/soc/amd/common/block/s3/s3_resume.c +63 is getting resolve from cbmem_stage_cache.c. do you think this is correct ?

stage_cache_get_raw(STAGE_S3_DATA, base, size);

when i have modified src/lib/Makefile.inc +174 as below,then i'm getting compilation error as stage_cache_get_raw() function is undefined :(

else ifeq ($(CONFIG_USE_CBMEM_STAGE_CACHE),y)
ramstage-y += cbmem_stage_cache.c
romstage-y += cbmem_stage_cache.c
postcar-y += cbmem_stage_cache.c
endif

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34368 )

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1:
> Patch Set 1:
>
> looks to me that this amd board is broken too much.. let me explain the problem that i'm seeing,
>
> when i'm building amd/gardenia board, here is my .config
>
> CONFIG_RELOCATABLE_RAMSTAGE=y
> CONFIG_RELOCATABLE_MODULES=y
> CONFIG_NO_STAGE_CACHE=y
>
> now src/lib/Makefile.inc +174 will include cbmem_stage_cache.c although NO_STAGE_CACHE=y
>
> else
> ramstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
> romstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
> postcar-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
> endif
>
> hence below stage_cache function call from src/soc/amd/common/block/s3/s3_resume.c +63 is getting resolve from cbmem_stage_cache.c. do you think this is correct ?
>
> stage_cache_get_raw(STAGE_S3_DATA, base, size);
>
> when i have modified src/lib/Makefile.inc +174 as below,then i'm getting compilation error as stage_cache_get_raw() function is undefined :(
>
> else ifeq ($(CONFIG_USE_CBMEM_STAGE_CACHE),y)
> ramstage-y += cbmem_stage_cache.c
> romstage-y += cbmem_stage_cache.c
> postcar-y += cbmem_stage_cache.c
> endif

Does this help:
diff --git a/src/soc/amd/common/block/s3/Makefile.inc b/src/soc/amd/common/block/s3/Makefile.inc
index 9efc6bc414..7d950b0b3a 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)]

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > looks to me that this amd board is broken too much.. let me explain the problem that i'm seeing,
> >
> > when i'm building amd/gardenia board, here is my .config
> >
> > CONFIG_RELOCATABLE_RAMSTAGE=y
> > CONFIG_RELOCATABLE_MODULES=y
> > CONFIG_NO_STAGE_CACHE=y
> >
> > now src/lib/Makefile.inc +174 will include cbmem_stage_cache.c although NO_STAGE_CACHE=y
> >
> > else
> > ramstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
> > romstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
> > postcar-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
> > endif
> >
> > hence below stage_cache function call from src/soc/amd/common/block/s3/s3_resume.c +63 is getting resolve from cbmem_stage_cache.c. do you think this is correct ?
> >
> > stage_cache_get_raw(STAGE_S3_DATA, base, size);
> >
> > when i have modified src/lib/Makefile.inc +174 as below,then i'm getting compilation error as stage_cache_get_raw() function is undefined :(
> >
> > else ifeq ($(CONFIG_USE_CBMEM_STAGE_CACHE),y)
> > ramstage-y += cbmem_stage_cache.c
> > romstage-y += cbmem_stage_cache.c
> > postcar-y += cbmem_stage_cache.c
> > endif
>
> Does this help:
> diff --git a/src/soc/amd/common/block/s3/Makefile.inc b/src/soc/amd/common/block/s3/Makefile.inc
> index 9efc6bc414..7d950b0b3a 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

yes Furquan, it does, i'm just waiting to submit this CL before we all agrees that there are some fundamental issue exist in that board with stage_cache assumption ?

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34368 )

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34368 )

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1:

I agree that gardenia hasn't been well maintained.  We may have found a maintainer for the gardenia board, but the Stoney reference board may also get switched to Jadite.

I'll bring this issue up with Silverback & AMD.

Thanks for pointing out the problem.

[Subrata Banik (Code Review)]

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I agree that gardenia hasn't been well maintained.  We may have found a maintainer for the gardenia board, but the Stoney reference board may also get switched to Jadite.
>
> I'll bring this issue up with Silverback & AMD.
>
> Thanks for pointing out the problem.

Thanks Martin, i have submitted another CL https://review.coreboot.org/c/coreboot/+/34369 here to fix that stage_cache calls that today still went into although platform has !HAVE_ACPI_RESUME

[Marshall Dawson (Code Review)]

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1: Code-Review+2

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

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 1:

Not against this, but I expect followup work to put a 'depends on HAVE_ACPI_RESUME' to any stage cache so this one is superseded soon.

[Subrata Banik (Code Review)]

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

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

Patch Set 2:
> Patch Set 1:
>
> Not against this, but I expect followup work to put a 'depends on HAVE_ACPI_RESUME' to any stage cache so this one is superseded soon.

NO_STAGE_CACHE is depends on !HAVE_ACPI_RESUME and all stage cache is depends on !NO_STAGE_CACHE, so i believe your concern been taken care already

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34368 )

Change subject: amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME
......................................................................

amd/stoneyridge/Kconfig: Enable stage cache based on HAVE_ACPI_RESUME

This patch fixes inconsistent issue with stage cache enabling with
HAVE_ACPI_RESUME config enable. Only enable stage cache if
CONFIG_HAVE_ACPI_RESUME=y

Change-Id: I7c3b3ec4642a615e17fb3dbdedca6af8ca95ea2b
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34368
Tested-by: build bot (Jenkins) 
Reviewed-by: Martin Roth 
Reviewed-by: Marshall Dawson 
---
M src/soc/amd/stoneyridge/Kconfig
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Martin Roth: Looks good to me, approved
  Marshall Dawson: Looks good to me, approved

diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig
index 3a8fd05..ea0ad5f 100644
--- a/src/soc/amd/stoneyridge/Kconfig
+++ b/src/soc/amd/stoneyridge/Kconfig
@@ -73,7 +73,7 @@
    select C_ENVIRONMENT_BOOTBLOCK
    select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH
    select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH
-	select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
+	select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if HAVE_ACPI_RESUME
    select PARALLEL_MP
    select PARALLEL_MP_AP_WORK
    select HAVE_SMI_HANDLER