Change in coreboot[master]: soc/amd/picasso: Convert TSEG and BERT info to cbmem

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Convert TSEG and BERT info to cbmem
......................................................................

soc/amd/picasso: Convert TSEG and BERT info to cbmem

Remove the convoluted memory map calculation for cbmem_top() and
put the two reserved regions into cbmem.  There is no compellng
reason to keep these regions above cbmem.  AMD can lock any region
with Tvalid and the appropriate mask.  The memory pointed to by
the BERT table simply needs to be reserved from the OS.

Change-Id: I6fa0544d642bbd8e2b3c21c3d186459ac8f6d477
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/picasso/include/soc/cpu.h
M src/soc/amd/picasso/memmap.c
M src/soc/amd/picasso/romstage.c
3 files changed, 52 insertions(+), 43 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/36114/1

diff --git a/src/soc/amd/picasso/include/soc/cpu.h b/src/soc/amd/picasso/include/soc/cpu.h
index b54a8ef..cf1d2d4 100644
--- a/src/soc/amd/picasso/include/soc/cpu.h
+++ b/src/soc/amd/picasso/include/soc/cpu.h
@@ -41,6 +41,6 @@
 #define EARLY_DRAM_MTRR_TOP \
    	(EARLY_DRAM_MTRR_BASE + EARLY_DRAM_MTRR_SIZE)

-#define EARLY_RAMSTAGE_MTRR_SZ (32 * MiB)
+#define EARLY_RAMSTAGE_MTRR_SZ (32 * MiB + CONFIG_SMM_TSEG_SIZE)

#endif /* __PICASSO_CPU_H__ */
diff --git a/src/soc/amd/picasso/memmap.c b/src/soc/amd/picasso/memmap.c
index 09af7e4..7b21bb8 100644
--- a/src/soc/amd/picasso/memmap.c
+++ b/src/soc/amd/picasso/memmap.c
@@ -28,6 +28,8 @@
 #include <soc/iomap.h>
 #include <amdblocks/acpimmio.h>

+#define BERT_REGION_MAX_SIZE 0x100000
+
 void backup_top_of_low_cacheable(uintptr_t ramtop)
 {
    biosram_write32(BIOSRAM_CBMEM_TOP, ramtop);
@@ -38,24 +40,19 @@
    return biosram_read32(BIOSRAM_CBMEM_TOP);
 }

-#if CONFIG(ACPI_BERT)
- #if CONFIG_SMM_TSEG_SIZE == 0x0
-  #define BERT_REGION_MAX_SIZE 0x100000
- #else
-  /* SMM_TSEG_SIZE must stay on a boundary appropriate for its granularity */
-  #define BERT_REGION_MAX_SIZE CONFIG_SMM_TSEG_SIZE
- #endif
-#else
- #define BERT_REGION_MAX_SIZE 0
-#endif
-
 void bert_reserved_region(void **start, size_t *size)
 {
-	if (CONFIG(ACPI_BERT))
-		*start = cbmem_top();
-	else
-		start = NULL;
-	*size = BERT_REGION_MAX_SIZE;
+	const struct cbmem_entry *bert;
+
+	*start = 0;
+	*size = 0;
+
+	bert = cbmem_entry_find(CBMEM_ID_BERT_RAW_DATA);
+	if (!bert)
+		return;
+
+	*start = cbmem_entry_start(bert);
+	*size = cbmem_entry_size(bert);
 }

void *cbmem_top(void)
@@ -66,19 +63,7 @@
    	return 0;

/* 8MB alignment to keep MTRR usage low */
-	return (void *)ALIGN_DOWN(restore_top_of_low_cacheable()
-			- CONFIG_SMM_TSEG_SIZE
-			- BERT_REGION_MAX_SIZE, 8*MiB);
-}
-
-static uintptr_t smm_region_start(void)
-{
-	return (uintptr_t)cbmem_top() + BERT_REGION_MAX_SIZE;
-}
-
-static size_t smm_region_size(void)
-{
-	return CONFIG_SMM_TSEG_SIZE;
+	return (void *)ALIGN_DOWN(restore_top_of_low_cacheable(), 8 * MiB);
 }

/*
@@ -106,12 +91,39 @@
 void smm_region(uintptr_t *start, size_t *size)
 {
    static int once;
+	const struct cbmem_entry *smm;

-	*start = smm_region_start();
-	*size = smm_region_size();
+	*start = 0;
+	*size = 0;
+
+	smm = cbmem_entry_find(CBMEM_ID_SMM_TSEG_SPACE);
+	if (!smm)
+		return;
+
+	*start = ALIGN_UP((uintptr_t)cbmem_entry_start(smm), CONFIG_SMM_TSEG_SIZE);
+	*size = CONFIG_SMM_TSEG_SIZE;

if (!once) {
    	clear_tvalid();
    	once = 1;
    }
 }
+
+/* Add or find TSEG and BERT storage prior to ramstage.  ramstage may need
+ * to be recovered from cached space in TSEG.  BERT storage is consumed at
+ * ROMSTAGE_CBMEM_INIT_HOOK and ordering can't be enforced for a hook.
+ */
+static void alloc_reserved_in_cbmem(int unused)
+{
+	void *p;
+
+	/* Make large enough so TSEG can have alignment = size, allowing a
+	 * good mask.
+	 */
+	p = cbmem_add(CBMEM_ID_SMM_TSEG_SPACE, 2 * CONFIG_SMM_TSEG_SIZE);
+
+	if (CONFIG(ACPI_BERT))
+		p = cbmem_add(CBMEM_ID_BERT_RAW_DATA, BERT_REGION_MAX_SIZE);
+}
+
+ROMSTAGE_CBMEM_INIT_HOOK(alloc_reserved_in_cbmem)
diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c
index eb39cc4..f4ae24b 100644
--- a/src/soc/amd/picasso/romstage.c
+++ b/src/soc/amd/picasso/romstage.c
@@ -78,27 +78,24 @@

static void set_mtrrs_for_ramstage(void)
 {
-	uintptr_t smm_base;
-	size_t smm_size;
-	uintptr_t smm_top;
    uintptr_t mem_top;
    uintptr_t ramstage_wb_base;
    size_t ramstage_wb_size;
    int mtrr;

-	smm_region(&smm_base, &smm_size);
-	smm_top = smm_base + smm_size;
-
    mem_top = (uintptr_t)cbmem_top();

-	/* Cache anticipated ramstage location through the top of TSEG */
-	ramstage_wb_base = mem_top - EARLY_RAMSTAGE_MTRR_SZ;
-	ramstage_wb_size = smm_top - ramstage_wb_base;
+	/* Cache anticipated ramstage location through the top of cbmem.
+	 * Unlike some other implementations, TSEG is in cbmem so it will
+	 * be cached as well.
+	 */
+	ramstage_wb_size = EARLY_RAMSTAGE_MTRR_SZ;
+	ramstage_wb_base = mem_top - ramstage_wb_size;

-	/* Make sure MTRR base and size are usable */
+	/* Ensure base and size are usable in a single MTRR pair */
    if (ramstage_wb_size != 1 << fms(ramstage_wb_size)) {
    	ramstage_wb_size = 1 << (1 + fms(ramstage_wb_size));
-		ramstage_wb_base = smm_top - ramstage_wb_size;
+		ramstage_wb_base = mem_top - ramstage_wb_size;
    }
    if (mem_top - EARLY_RAMSTAGE_MTRR_SZ < EARLY_DRAM_MTRR_TOP) {
    	printk(BIOS_WARNING, "Warning: Skipping ramstage cacheable due to configuration\n");

[Martin Roth (Code Review)]

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

Change subject: soc/amd/picasso: Convert TSEG and BERT info to cbmem
......................................................................

Patch Set 2: Code-Review+1

Looks good to me, but I wouldn't mind another review on this one.

[Marshall Dawson (Code Review)]

Hello build bot (Jenkins), Martin Roth,

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

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

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

Change subject: soc/amd/picasso: Convert TSEG and BERT info to cbmem
......................................................................

soc/amd/picasso: Convert TSEG and BERT info to cbmem

Remove the convoluted memory map calculation for cbmem_top() and
put the two reserved regions into cbmem.  There is no compellng
reason to keep these regions above cbmem.  AMD can lock any region
with Tvalid and the appropriate mask.  The memory pointed to by
the BERT table simply needs to be reserved from the OS.

Change-Id: I6fa0544d642bbd8e2b3c21c3d186459ac8f6d477
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/picasso/memmap.c
1 file changed, 42 insertions(+), 30 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/36114/4

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Convert TSEG and BERT info to cbmem
......................................................................

Abandoned

TSEG cannot move to cbmem due to not being able to guarantee ordering of init hooks.  A replacement for BERT is in cb:38694