Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38691/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38691/6//COMMIT_MSG@9 PS6, Line 9: nit: space not required
https://review.coreboot.org/c/coreboot/+/38691/6//COMMIT_MSG@14 PS6, Line 14: Can you please add BUG=?
https://review.coreboot.org/c/coreboot/+/38691/6/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38691/6/src/soc/amd/picasso/bootblo... PS6, Line 37: I think its safer to clear all variable MTRRs before actually using them.
https://review.coreboot.org/c/coreboot/+/38691/6/src/soc/amd/picasso/bootblo... PS6, Line 39: SYSCFG_MSR_MtrrFixDramEn Shouldn't this be enabled after fixed_mtrss are initialized with RdMem/WrMem bits?
https://review.coreboot.org/c/coreboot/+/38691/6/src/soc/amd/picasso/bootblo... PS6, Line 43: /* Write all as MTRR_READ_MEM | MTRR_WRITE_MEM to send 0-1M cycles to DRAM */ Why is that?
https://review.coreboot.org/c/coreboot/+/38691/6/src/soc/amd/picasso/bootblo... PS6, Line 48: ~(SYSCFG_MSR_MtrrFixDramModEn | SYSCFG_MSR_MtrrFixDramEn) Clearing of SYSCFG_MSR_MtrrFixDramModEn is going to result in bits 3 and 4 in Fixed MTRRs to be ignored. So, what is the use of lines 43-46?
https://review.coreboot.org/c/coreboot/+/38691/6/src/soc/amd/picasso/bootblo... PS6, Line 57: if (mtrr < 0) : return; return early? Don't you still want to enable caching based on the rest of the setup?
if (mtrr != 1) { set_var_mtrr(...); }