Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 9:
(14 comments)
PTAL
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
Done
https://review.coreboot.org/c/coreboot/+/38691/6//COMMIT_MSG@14 PS6, Line 14:
Can you please add BUG=?
Done
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 58: SYSCFG_MSR_MtrrFixDramEn
1=Enables the RdDram and WrDram attributes in Core::X86::Msr::MtrrFix_64K through […]
Reworked the patch.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 66: get_free_var_mtrr()
The PPR says: […]
Cleared now.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 69: FLASH_BASE_ADDR
What is this? Is the flash not mapped below 4GiB as usual? Can't we use […]
We don't define CAR_CACHE_ROM_SIZE so we can't use it.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 69: CONFIG_ROM_SIZE
If you find any registers that aren't documented call it out. […]
Ack
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 74: set_var_mtrr
You might want to cover .earlyram.data with a variable MTRR too.
It's a part of bootblock, so it gets covered.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 74: _ebootblock - _bootblock
Will there be a guarantee this is a power of 2?
It is now.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 84: set_caching();
Do this first? It does not seem to depend on the previous ones and should provide a speed up.
Done
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.
Done
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?
Done
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?
Turns out we don't need to do this.
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 igno […]
Done
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? […]
Done