Change in coreboot[master]: soc/amd/picasso: Cache romstage in RAM

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

soc/amd/picasso: Cache romstage in RAM

Picasso's romstage is non-XIP and therefore is not automatically
cached by the WP type applied to the flash storage.  Determine
its location in RAM and type it with an MTRR.

Signed-off-by: Marshall Dawson 
Change-Id: I431c63553c6eabafc4f3512a2484f350cd07003b
---
M src/soc/amd/picasso/romstage.c
1 file changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38692/1

diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c
index 8b8d329..4655d95 100644
--- a/src/soc/amd/picasso/romstage.c
+++ b/src/soc/amd/picasso/romstage.c
@@ -21,6 +21,7 @@
 #include <console/console.h>
 #include <program_loading.h>
 #include <romstage_handoff.h>
+#include <symbols.h>
 #include <elog.h>
 #include <soc/romstage.h>

@@ -29,34 +30,47 @@
    /* By default, don't do anything */
 }

+static void cache_romstage(void)
+{
+	int mtrr;
+
+	mtrr = get_free_var_mtrr();
+	if (mtrr < 0)
+		return;
+	set_var_mtrr(mtrr, (unsigned int)_romstage, _eromstage - _romstage, MTRR_TYPE_WRBACK);
+}
+
 asmlinkage void car_stage_entry(void)
 {
    uintptr_t top_of_mem;
    int s3_resume;

post_code(0x40);
-	console_init();
+	cache_romstage();

post_code(0x41);
+	console_init();
+
+	post_code(0x42);
    s3_resume = acpi_s3_resume_allowed() && acpi_is_wakeup_s3();
    mainboard_romstage_entry_s3(s3_resume);
    elog_boot_notify(s3_resume);

-	post_code(0x42);
+	post_code(0x43);
    u32 val = cpuid_eax(1);
    printk(BIOS_DEBUG, "Family_Model: %08x\n", val);

-	post_code(0x43);
+	post_code(0x44);
    top_of_mem = ALIGN_DOWN(rdmsr(TOP_MEM).lo, 8 * MiB);
    backup_top_of_low_cacheable(top_of_mem);

-	post_code(0x44);
+	post_code(0x45);
    if (cbmem_recovery(s3_resume))
    	printk(BIOS_CRIT, "Failed to recover cbmem\n");
    if (romstage_handoff_init(s3_resume))
    	printk(BIOS_ERR, "Failed to set romstage handoff data\n");

-	post_code(0x45);
+	post_code(0x46);
    run_ramstage();

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

[Arthur Heymans (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 1:

Is doing this in the bootblock as part of the platform_prog_run hook in bootblock not a better idea? That way the romstage is cached for the full extend of its execution.

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 1:
> Is doing this in the bootblock as part of the platform_prog_run hook in bootblock not a better idea? That way the romstage is cached for the full extend of its execution.

Hmm, they the hook seems very promising.  The types, name and entry look like what I expect but I'm not seeing the stage size anywhere.  The comment in program_loading.h says "The region_device is the source of the program content to load." however in looking at parameters passed for loading romstage, the rdev.offset = entry = CONFIG_ROMSTAGE_ADDR.  The rdev.size isn't particularly recognizable.

Maybe it's acceptable to artificially use a sufficiently large size to never worry about outgrowing it.

BTW, using the hook looks to be better for caching the ramstage target area and TSEG as well.

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 1:
> Patch Set 1:
> > Is doing this in the bootblock as part of the platform_prog_run hook in bootblock not a better idea? That way the romstage is cached for the full extend of its execution.
>
> Hmm, they the hook seems very promising.  The types, name and entry look like what I expect but I'm not seeing the stage size anywhere.  The comment in program_loading.h says "The region_device is the source of the program content to load." however in looking at parameters passed for loading romstage, the rdev.offset = entry = CONFIG_ROMSTAGE_ADDR.  The rdev.size isn't particularly recognizable.
> BTW, using the hook looks to be better for caching the ramstage target area and TSEG as well.

What do you think about CB:38728 as a different approach?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38692 )

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38692/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38692/1//COMMIT_MSG@12 
PS1, Line 12: 
Please add that POST codes change too.

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

soc/amd/picasso: Cache romstage in RAM

Picasso's romstage is non-XIP and therefore is not automatically
cached by the WP type applied to the flash storage.  Determine
its location in RAM and type it with an MTRR.

Since this adds an initialization step, the post codes after this step
are incremented by one.

Signed-off-by: Marshall Dawson 
Change-Id: I431c63553c6eabafc4f3512a2484f350cd07003b
---
M src/soc/amd/picasso/romstage.c
1 file changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38692/2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38692 )

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38692/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38692/1//COMMIT_MSG@12 
PS1, Line 12:
> Please add that POST codes change too.

Done

[Arthur Heymans (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 2:
> Patch Set 1:
> > Patch Set 1:
> > > Is doing this in the bootblock as part of the platform_prog_run hook in bootblock not a better idea? That way the romstage is cached for the full extend of its execution.
> >
> > Hmm, they the hook seems very promising.  The types, name and entry look like what I expect but I'm not seeing the stage size anywhere.  The comment in program_loading.h says "The region_device is the source of the program content to load." however in looking at parameters passed for loading romstage, the rdev.offset = entry = CONFIG_ROMSTAGE_ADDR.  The rdev.size isn't particularly recognizable.
> > BTW, using the hook looks to be better for caching the ramstage target area and TSEG as well.
>
> What do you think about CB:38728 as a different approach?

I like that approach better. It allows to compress all stages after the bootblock.

[Raul Rangel (Code Review)]

Raul Rangel has uploaded a new patch set (#7) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38692 )

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

soc/amd/picasso: Cache romstage in RAM

Picasso's romstage is non-XIP and therefore is not automatically
cached by the WP type applied to the flash storage.  Determine
its location in RAM and type it with an MTRR.

Since this adds an initialization step, the post codes after this step
are incremented by one.

BUG=b:153675909
TEST=Boot trembyle and make sure that romstage isn't abnormally slow.
Verified romstage MTRRs have valid parameters:
Adding MTRR: base 0x2000000 , size 0x100000

Signed-off-by: Marshall Dawson 
Change-Id: I431c63553c6eabafc4f3512a2484f350cd07003b
---
M src/soc/amd/picasso/romstage.c
1 file changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/38692/7

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38692 )

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Patch Set 7: Code-Review-1

Looks like this CL is superseded by https://review.coreboot.org/c/coreboot/+/38728/2

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Cache romstage in RAM
......................................................................

Abandoned

CB:42103