Arthur Heymans 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:
(8 comments)
I think it is worth splitting the romstage cache setup and the ramstage cache setup to speed up the review. About the ramstage caching set up. It might be useful to have a look at CB:35993, which dealt with a somewhat similar issue about finding the right base/size for one MTRR to cover a region. Another path would be to reuse the functions doing the computations for setting up MTRR to set up in postcar stage, it dealt with the logic of having to cache regions quite well and was flexible in the amount of MTRR it used.
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 67: EARLY_DRAM_MTRR_BASE Check if aligned to EARLY_DRAM_MTRR_SIZE.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 67: EARLY_DRAM_MTRR_SIZE Check with preprocessor IS_POWER_OF_2() (from commonlib).
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?
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 cache is often put there I suppose?
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?
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 99: amstage_wb_size != 1 << fms(ramstage_wb_size) IS_POWER_OF_2() from commonlib.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 101: smm_top - ramstage_wb_size how do you know this is aligned to ramstage_wb_size?
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 113: Warning: Unable to make ramstage cacheable\n Be a little more specific? "No MTRR available to make ramstage cacheable?"