Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 66: : set_var_mtrr(mtrr, EARLY_DRAM_MTRR_BASE, EARLY_DRAM_MTRR_SIZE, : MTRR_TYPE_WRBACK); : : mtrr = get_free_var_mtrr(); : if (mtrr < 0) : return -1; : : set_var_mtrr(mtrr, FLASH_BASE_ADDR, CONFIG_ROM_SIZE, : MTRR_TYPE_WRPROT);
Does this all work? What is the state of CR0 and MTRR_DEF_TYPE_MSR at this point?
Hmm, not as well as I thought. Re. CR0, I assume you're asking about CD and NW. I cleared those in CB:35035 reset_in_dram_crt0.S line 27. However, the default type was not set, and setting it results it in quick_ram_check_or_die() failing due to cbmem_top returning a goofed value. Will debug tomorrow.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 79: set_mtrrs_for_ramstage
You want to cache a sufficiently large area below cbmem_top and also TSEG above since the stage cach […]
Yes. This replaces the postcar_frame stuff. FWIW in CB:36114, TSEG moves inside cbmem instead of above it.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 94: /* Cache anticipated ramstage location through the top of TSEG */
Don't you want to cache TSEG too? Usually we set up a stage cache there?
That's the idea. This was intended to be analogous to what postcar_enable_tseg_cache() would aim to do.