Change in coreboot[master]: soc/amd/picasso: Add bootram reservations

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38702 )

Change subject: soc/amd/picasso: Add bootram reservations
......................................................................

soc/amd/picasso: Add bootram reservations

Picasso consumes DRAM to avoid running XIP from the boot flash.  This
must be reserved from use by the OS.  During bootblock, save the region
used by the stage (and reserve 0 for verstage).  Save romstage's
region in romstage.  Override the weak bootmem_platform_add_ranges() to
retrieve the bases and sizes, then add them as BM_MEM_RESERVED.

Note the design of verstage, as it applies to the x86, is T.B.D.  The
reservation of its bootram should be reevaluated later.

Change-Id: I7032117bbf12b845e06df8cfa7a62445e589ecf5
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/picasso/bootblock/bootblock.c
M src/soc/amd/picasso/northbridge.c
M src/soc/amd/picasso/romstage.c
3 files changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/38702/1

diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index 00f8416..e845437 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -24,6 +24,7 @@
 #include <soc/southbridge.h>
 #include <soc/i2c.h>
 #include <amdblocks/amd_pci_mmconf.h>
+#include <amdblocks/biosram.h>

static void amd_initmmio(void)
 {
@@ -108,4 +109,7 @@

fch_early_init();
    i2c_soc_early_init();
+
+	set_bootram_bootblock((uintptr_t)_bootblock, _ebootblock - _bootblock);
+	set_bootram_verstage(0, 0); /* TODO: load values once supported */
 }
diff --git a/src/soc/amd/picasso/northbridge.c b/src/soc/amd/picasso/northbridge.c
index c5bd9fa..9d3c98b 100644
--- a/src/soc/amd/picasso/northbridge.c
+++ b/src/soc/amd/picasso/northbridge.c
@@ -35,10 +35,29 @@
 #include <stdint.h>
 #include <string.h>
 #include <arch/bert_storage.h>
+#include <bootmem.h>
 #include <fsp/util.h>

#include "chip.h"

+void bootmem_platform_add_ranges(void)
+{
+	uint64_t base;
+	uint32_t size;
+
+	get_bootram_bootblock(&base, &size);
+	bootmem_add_range(base, size, BM_MEM_RESERVED);
+
+	get_bootram_verstage(&base, &size);
+	bootmem_add_range(base, size, BM_MEM_RESERVED);
+
+	get_bootram_romstage(&base, &size);
+	bootmem_add_range(base, size, BM_MEM_RESERVED);
+
+	get_bootram_fspm(&base, &size);
+	bootmem_add_range(base, size, BM_MEM_RESERVED);
+}
+
 static void read_resources(struct device *dev)
 {
    uint32_t mem_usable = (uintptr_t)cbmem_top();
diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c
index a8651fa..3c2343c 100644
--- a/src/soc/amd/picasso/romstage.c
+++ b/src/soc/amd/picasso/romstage.c
@@ -28,6 +28,7 @@
 #include <symbols.h>
 #include <elog.h>
 #include <soc/romstage.h>
+#include <amdblocks/biosram.h>
 #include "chip.h"
 #include <fsp/api.h>

@@ -160,6 +161,11 @@
    mainboard_fsp_memory_init_params_cb(mcfg, version);
 }

+void fsp_reserve_fspm_mem(uintptr_t base, size_t size)
+{
+	set_bootram_fspm(base, (uint32_t)size);
+}
+
 asmlinkage void car_stage_entry(void)
 {
    int s3_resume;
@@ -186,6 +192,9 @@
    set_mtrrs_for_ramstage();

post_code(0x46);
+	set_bootram_romstage((uintptr_t)_romstage, _eromstage - _romstage);
+
+	post_code(0x47);
    run_ramstage();

post_code(0x50); /* Should never see this post code. */

[Felix Held (Code Review)]

Felix Held has uploaded a new patch set (#5) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38702 )

Change subject: soc/amd/picasso: Add bootram reservations
......................................................................

soc/amd/picasso: Add bootram reservations

Picasso consumes DRAM to avoid running XIP from the boot flash.  This
must be reserved from use by the OS.  During bootblock, save the region
used by the stage (and reserve 0 for verstage).  Save romstage's
region in romstage.  Override the weak bootmem_platform_add_ranges() to
retrieve the bases and sizes, then add them as BM_MEM_RESERVED.

Note the design of verstage, as it applies to the x86, is T.B.D.  The
reservation of its bootram should be reevaluated later.

Change-Id: I7032117bbf12b845e06df8cfa7a62445e589ecf5
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/picasso/bootblock/bootblock.c
M src/soc/amd/picasso/northbridge.c
M src/soc/amd/picasso/romstage.c
3 files changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/38702/5

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

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

Change subject: soc/amd/picasso: Add bootram reservations
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38702/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38702/5//COMMIT_MSG@10 
PS5, Line 10: must be reserved from use by the OS.  During bootblock, save the region
Add reason why? (S3 resume would corrupt it).

Unless you are close to having working ACPI S3 there should be no rush to merge this without proper discussion.

https://review.coreboot.org/c/coreboot/+/38702/5//COMMIT_MSG@13 
PS5, Line 13: retrieve the bases and sizes, then add them as BM_MEM_RESERVED.
This is not really a picasso issue that would justify use of a vendor-specific solution via biosram. It is a generic problem, feature like wiping DRAM late in romstage would benefit of having bootmem accounting available, on every platform, also non-X86.

You can achieve this with CBMEM and place set_bootmem_() calls in common code.

If I could, this would be -2. Practise has shown that wiping out vendor-specific approaches in favour of generic ones is expensive and wastes review resources. And causes tensions allover.

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

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

Change subject: soc/amd/picasso: Add bootram reservations
......................................................................

Patch Set 5:

IMHO CB:38729 could achieve the same as CB:38701 + CB:38702. I had forgotten it existed.

[Marshall Dawson (Code Review)]

Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38702 )

Change subject: soc/amd/picasso: Add bootram reservations
......................................................................

Abandoned

No longer necessary CB:42264