Change in coreboot[master]: lib/fmap: Disable pre-RAM cache for FSP 1.0

[Julius Werner (Code Review)]

Hello Arthur Heymans, Patrick Georgi,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

lib/fmap: Disable pre-RAM cache for FSP 1.0

Due to the way CAR teardown is handled in FSP 1.0, the results of
car_get_var_ptr() aren't always reliable, which can break things when
running with FMAP cache. It might be possible to fix this but would make
the code rather complicated, so let's just disable the feature on these
platforms and hope they die out soon.

Change-Id: I7ffb1b8b08a7ca3fe8d53dc827e2c8521da064c7
Signed-off-by: Julius Werner 
---
M src/lib/fmap.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/36937/1

diff --git a/src/lib/fmap.c b/src/lib/fmap.c
index 4b4179c..b54322c 100644
--- a/src/lib/fmap.c
+++ b/src/lib/fmap.c
@@ -60,6 +60,9 @@

static void setup_preram_cache(struct mem_region_device *cache_mrdev)
 {
+	if (CONFIG(PLATFORM_USES_FSP1_0))
+		return;	/* FSP 1.0's CAR_GLOBAL support cannot handle this. */
+
    if (!ENV_ROMSTAGE_OR_BEFORE) {
    	/* We get here if ramstage makes an FMAP access before calling
    	   cbmem_initialize(). We should avoid letting it come to that,

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

Patch Set 1: Code-Review+2

[Julius Werner (Code Review)]

Hello Arthur Heymans, build bot (Jenkins), Patrick Georgi,

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

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

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

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

lib/fmap: Disable pre-RAM cache for FSP 1.0

Due to the way CAR teardown is handled in FSP 1.0, the results of
car_get_var_ptr() aren't always reliable, which can break things when
running with FMAP cache. It might be possible to fix this but would make
the code rather complicated, so let's just disable the feature on these
platforms and hope they die out soon.

Also allow this option to be used by platforms that don't have space for
the cache and want to save a little more code.

Change-Id: I7ffb1b8b08a7ca3fe8d53dc827e2c8521da064c7
Signed-off-by: Julius Werner 
---
M src/drivers/intel/fsp1_0/Kconfig
M src/lib/Kconfig
M src/lib/fmap.c
M src/soc/rockchip/rk3288/Kconfig
4 files changed, 16 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/36937/2

[Julius Werner (Code Review)]

Hello Patrick Rudolph, Huang Jin, Arthur Heymans, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,

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

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

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

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

lib/fmap: Disable pre-RAM cache for FSP 1.0

Due to the way CAR teardown is handled in FSP 1.0, the results of
car_get_var_ptr() aren't always reliable, which can break things when
running with FMAP cache. It might be possible to fix this but would make
the code rather complicated, so let's just disable the feature on these
platforms and hope they die out soon.

Also allow this option to be used by platforms that don't have space for
the cache and want to save a little more code.

Change-Id: I7ffb1b8b08a7ca3fe8d53dc827e2c8521da064c7
Signed-off-by: Julius Werner 
---
M src/arch/x86/car.ld
M src/drivers/intel/fsp1_0/Kconfig
M src/lib/Kconfig
M src/lib/fmap.c
M src/soc/rockchip/rk3288/Kconfig
5 files changed, 19 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/36937/3

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

Patch Set 3: Code-Review+1

(2 comments)

Waiting for Siemens folks to confirm, some nits, but otherwise looks good. Thanks!

https://review.coreboot.org/c/coreboot/+/36937/3/src/lib/fmap.c 
File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/36937/3/src/lib/fmap.c@77 
PS3, Line 77: affort
afford

https://review.coreboot.org/c/coreboot/+/36937/3/src/lib/fmap.c@78 
PS3, Line 78: 		print_once(BIOS_ERR,
single line?

[Julius Werner (Code Review)]

Hello Werner Zeh, Patrick Rudolph, Huang Jin, Arthur Heymans, uwe taz, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,

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

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

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

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

lib/fmap: Disable pre-RAM cache for FSP 1.0

Due to the way CAR teardown is handled in FSP 1.0, the results of
car_get_var_ptr() aren't always reliable, which can break things when
running with FMAP cache. It might be possible to fix this but would make
the code rather complicated, so let's just disable the feature on these
platforms and hope they die out soon.

Also allow this option to be used by platforms that don't have space for
the cache and want to save a little more code.

Change-Id: I7ffb1b8b08a7ca3fe8d53dc827e2c8521da064c7
Signed-off-by: Julius Werner 
---
M src/arch/x86/car.ld
M src/drivers/intel/fsp1_0/Kconfig
M src/lib/Kconfig
M src/lib/fmap.c
M src/soc/rockchip/rk3288/Kconfig
5 files changed, 19 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/36937/4

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36937/3/src/lib/fmap.c 
File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/36937/3/src/lib/fmap.c@77 
PS3, Line 77: affort
> afford

Done

https://review.coreboot.org/c/coreboot/+/36937/3/src/lib/fmap.c@78 
PS3, Line 78: 		print_once(BIOS_ERR,
> single line?

Not in 80 chars (which is what the whole rest of the file uses for now).

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

Patch Set 4: Code-Review+2

This patch fixes the issues I was seeing on mc_tcu3 (fsp_baytrail, FSP1.0) with FMAP cache. The board is now booting again. This is true for mc_bdx1 (fsp_broadwell_de, FSP1.0), too.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

Patch Set 4: Code-Review+2

getting it in quicker as this fixes a regression on FSP1.0

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................

lib/fmap: Disable pre-RAM cache for FSP 1.0

Due to the way CAR teardown is handled in FSP 1.0, the results of
car_get_var_ptr() aren't always reliable, which can break things when
running with FMAP cache. It might be possible to fix this but would make
the code rather complicated, so let's just disable the feature on these
platforms and hope they die out soon.

Also allow this option to be used by platforms that don't have space for
the cache and want to save a little more code.

Change-Id: I7ffb1b8b08a7ca3fe8d53dc827e2c8521da064c7
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36937
Tested-by: build bot (Jenkins) 
Reviewed-by: Werner Zeh 
Reviewed-by: Patrick Georgi 
---
M src/arch/x86/car.ld
M src/drivers/intel/fsp1_0/Kconfig
M src/lib/Kconfig
M src/lib/fmap.c
M src/soc/rockchip/rk3288/Kconfig
5 files changed, 19 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Werner Zeh: Looks good to me, approved

diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld
index 972cb52..3680250 100644
--- a/src/arch/x86/car.ld
+++ b/src/arch/x86/car.ld
@@ -58,7 +58,10 @@
 #endif

TIMESTAMP(., 0x200)
+
+#if !CONFIG(NO_FMAP_CACHE)
    FMAP_CACHE(., FMAP_SIZE)
+#endif

_car_ehci_dbg_info = .;
    /* Reserve sizeof(struct ehci_dbg_info). */
diff --git a/src/drivers/intel/fsp1_0/Kconfig b/src/drivers/intel/fsp1_0/Kconfig
index 32a0777..1a1d4f7 100644
--- a/src/drivers/intel/fsp1_0/Kconfig
+++ b/src/drivers/intel/fsp1_0/Kconfig
@@ -15,6 +15,7 @@
    bool
    default n
    select CAR_GLOBAL_MIGRATION
+	select NO_FMAP_CACHE	# doesn't work with CAR_GLOBAL restrictions
    help
      Selected for Intel processors/platform combinations that use the
      Intel Firmware Support Package (FSP) 1.0 for initialization.
diff --git a/src/lib/Kconfig b/src/lib/Kconfig
index cb1e4a5..dd9974a 100644
--- a/src/lib/Kconfig
+++ b/src/lib/Kconfig
@@ -68,3 +68,10 @@
    def_bool y

endif
+
+config NO_FMAP_CACHE
+	bool
+	help
+	  If your platform really doesn't want to use an FMAP cache (e.g. due to
+	  space constraints), you can select this to disable warnings and save
+	  a bit more code.
diff --git a/src/lib/fmap.c b/src/lib/fmap.c
index 4b4179c..48aab8f 100644
--- a/src/lib/fmap.c
+++ b/src/lib/fmap.c
@@ -60,6 +60,9 @@

static void setup_preram_cache(struct mem_region_device *cache_mrdev)
 {
+	if (CONFIG(NO_FMAP_CACHE))
+		return;
+
    if (!ENV_ROMSTAGE_OR_BEFORE) {
    	/* We get here if ramstage makes an FMAP access before calling
    	   cbmem_initialize(). We should avoid letting it come to that,
@@ -70,10 +73,10 @@
    }

if (REGION_SIZE(fmap_cache) == 0) {
-		/* If you see this you really want to add an FMAP_CACHE to your
-		   memlayout, unless you absolutely can't affort the 2K. */
-		print_once(BIOS_NOTICE,
-			"NOTE: Running without FMAP_CACHE, should add it!\n");
+		/* If you see this you should add FMAP_CACHE() to your memlayout
+		   (or select NO_FMAP_CACHE if you can't afford the 2K). */
+		print_once(BIOS_ERR,
+			"ERROR: FMAP_CACHE enabled but no region provided!\n");
    	return;
    }

diff --git a/src/soc/rockchip/rk3288/Kconfig b/src/soc/rockchip/rk3288/Kconfig
index 3aebab9..6a44ccd 100644
--- a/src/soc/rockchip/rk3288/Kconfig
+++ b/src/soc/rockchip/rk3288/Kconfig
@@ -29,6 +29,7 @@
    select MAINBOARD_FORCE_NATIVE_VGA_INIT
    select HAVE_LINEAR_FRAMEBUFFER
    select NO_BOOTBLOCK_CONSOLE
+	select NO_FMAP_CACHE

if SOC_ROCKCHIP_RK3288

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

Kyösti Mälkki has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/36937 )

Change subject: lib/fmap: Disable pre-RAM cache for FSP 1.0
......................................................................