Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as- RAM setup code to properly enable MTRRs. Add that capabability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 56 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/1
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index 1d3a98f..00f8416 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -12,10 +12,14 @@ */
#include <stdint.h> +#include <symbols.h> #include <bootblock_common.h> #include <console/console.h> +#include <cpu/x86/cache.h> +#include <cpu/x86/msr.h> #include <cpu/amd/msr.h> #include <cpu/x86/mtrr.h> +#include <cpu/amd/mtrr.h> #include <timestamp.h> #include <soc/southbridge.h> #include <soc/i2c.h> @@ -31,10 +35,62 @@ wrmsr(MMIO_CONF_BASE, mmconf); }
+static const unsigned int fixed_mtrrs[] = { + MTRR_FIX_64K_00000, + MTRR_FIX_16K_80000, + MTRR_FIX_16K_A0000, + MTRR_FIX_4K_C0000, + MTRR_FIX_4K_C8000, + MTRR_FIX_4K_D0000, + MTRR_FIX_4K_D8000, + MTRR_FIX_4K_E0000, + MTRR_FIX_4K_E8000, + MTRR_FIX_4K_F0000, + MTRR_FIX_4K_F8000, +}; + +static void set_caching(void) +{ + msr_t deftype, syscfg, rwmem; + int mtrr; + int i; + + syscfg = rdmsr(SYSCFG_MSR); + syscfg.lo |= SYSCFG_MSR_MtrrFixDramModEn | SYSCFG_MSR_MtrrFixDramEn + | SYSCFG_MSR_MtrrVarDramEn; + wrmsr(SYSCFG_MSR, syscfg); + + /* Write all as MTRR_READ_MEM | MTRR_WRITE_MEM to send 0-1M cycles to DRAM */ + rwmem.hi = rwmem.lo = 0x18181818; + for (i = 0 ; i < ARRAY_SIZE(fixed_mtrrs) ; i++) + wrmsr(fixed_mtrrs[i], rwmem); + + syscfg.lo &= ~(SYSCFG_MSR_MtrrFixDramModEn | SYSCFG_MSR_MtrrFixDramEn); + wrmsr(SYSCFG_MSR, syscfg); + + deftype = rdmsr(MTRR_DEF_TYPE_MSR); + deftype.lo &= ~MTRR_DEF_TYPE_MASK; + deftype.lo |= MTRR_DEF_TYPE_EN | MTRR_DEF_TYPE_FIX_EN | MTRR_TYPE_UNCACHEABLE; + wrmsr(MTRR_DEF_TYPE_MSR, deftype); + + mtrr = get_free_var_mtrr(); + if (mtrr < 0) + return; + set_var_mtrr(mtrr, FLASH_BASE_ADDR, CONFIG_ROM_SIZE, MTRR_TYPE_WRPROT); + + mtrr = get_free_var_mtrr(); + if (mtrr < 0) + return; + set_var_mtrr(mtrr, (unsigned int)_bootblock, _ebootblock - _bootblock, MTRR_TYPE_WRBACK); + + enable_cache(); +} + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { enable_pci_mmconf(); amd_initmmio(); + set_caching();
bootblock_main_with_basetime(base_timestamp); }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38691/1/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38691/1/src/soc/amd/picasso/bootblo... PS1, Line 84: set_var_mtrr(mtrr, (unsigned int)_bootblock, _ebootblock - _bootblock, MTRR_TYPE_WRBACK); line over 96 characters
Felix Held has uploaded a new patch set (#2) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as- RAM setup code to properly enable MTRRs. Add that capabability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 2:
(1 comment)
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 75: MTRR_TYPE_WRBACK); code indent should use tabs where possible
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 2:
(1 comment)
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 75: MTRR_TYPE_WRBACK);
code indent should use tabs where possible
wasn't it always indent with tabs and align with spaces? or should i just indent the second half with one more tab than the first half?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 2:
(5 comments)
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 What does this exactly do?
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 CACHE_ROM_BASE/_SIZE?
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 69: CONFIG_ROM_SIZE What if CONFIG_ROM_SIZE is >16MiB?
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?
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 75: MTRR_TYPE_WRBACK);
wasn't it always indent with tabs and align with spaces? or should i just indent the second half wit […]
I guess it complains if there are more than 7 spaces.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 2:
(3 comments)
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 66: get_free_var_mtrr() Does the system always come out of reset with cleared variable MTRR's? On Intel systems those are always cleared before setting MTRR_DEF_TYPE_MSR and setting up CAR and bootblock/romstage caching.
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.
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.
Felix Held has uploaded a new patch set (#3) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as- RAM setup code to properly enable MTRRs. Add that capabability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38691/3/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38691/3/src/soc/amd/picasso/bootblo... PS3, Line 65: MTRR_TYPE_WRBACK); code indent should use tabs where possible
Felix Held has uploaded a new patch set (#4) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as- RAM setup code to properly enable MTRRs. Add that capabability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/4
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 4:
(3 comments)
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
What does this exactly do?
1=Enables the RdDram and WrDram attributes in Core::X86::Msr::MtrrFix_64K through Core::X86::Msr::MtrrFix_4K_7.
Not sure why it's being turned off.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 66: get_free_var_mtrr() The PPR says:
Valid: Read-write. Reset: X. 1=The variable-size MTRR pair is enabled.
Looks like it's undefined.
https://review.coreboot.org/c/coreboot/+/38691/2/src/soc/amd/picasso/bootblo... PS2, Line 69: CONFIG_ROM_SIZE
What if CONFIG_ROM_SIZE is >16MiB?
47:12 PhyMask: address mask. Read-write. Reset: X_XXXX_XXXXh.
I think that's plenty of bits.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 4:
(1 comment)
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 69: CONFIG_ROM_SIZE
47:12 PhyMask: address mask. Read-write. Reset: X_XXXX_XXXXh. […]
Usually not more than 16MiB are memory mapped because there are some resources with fixed address below `4GiB-16MiB`, e.g. APICs. I don't know about Picasso (no datasheet).
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 4:
(1 comment)
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 69: CONFIG_ROM_SIZE
Usually not more than 16MiB are memory mapped because there are some […]
https://developer.amd.com/wp-content/resources/55570-B1_PUB.zip
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 4:
(1 comment)
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 69: CONFIG_ROM_SIZE
https://developer.amd.com/wp-content/resources/55570-B1_PUB. […]
Nice! Thank you :)
I had already given up downloading their documents because earlier versions left out too much to be useful. I guess that's the first fam17 one that mentions that there is more that just the cores ;)
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 4:
(1 comment)
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 69: CONFIG_ROM_SIZE
Nice! Thank you :) […]
If you find any registers that aren't documented call it out. We should push on AMD to make them public.
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(...); }
Raul Rangel has uploaded a new patch set (#7) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as-RAM setup code to properly enable MTRRs. Add that capability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/bootblock/bootblock.c 2 files changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38691/7/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/38691/7/src/soc/amd/picasso/bootblo... PS7, Line 28: if (mtrr >= 0) { braces {} are not necessary for single statement blocks
Raul Rangel has uploaded a new patch set (#8) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as-RAM setup code to properly enable MTRRs. Add that capability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
BUG=b:147042464 TEST=Boot trembyle to payload
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/bootblock/bootblock.c 2 files changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/8
Raul Rangel has uploaded a new patch set (#9) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD programs, picasso cannot rely on the cache-as-RAM setup code to properly enable MTRRs. Add that capability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
BUG=b:147042464 TEST=Boot trembyle to payload
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/bootblock/bootblock.c 2 files changed, 39 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/9
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
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 9: Code-Review+1
There is a comment on previous CL from Julius about using _Static_assert instead of the power of 2 check in Makefile. Other than that, LGTM.
Not for this change, but I think we want to set up the caching for romstage as well here?
Paul Menzel 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38691/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38691/9//COMMIT_MSG@9 PS9, Line 9: programs devices?
https://review.coreboot.org/c/coreboot/+/38691/9//COMMIT_MSG@16 PS9, Line 16: TEST=Boot trembyle to payload Any changes with this commit (boot time, …)?
Raul Rangel has uploaded a new patch set (#10) to the change originally created by Marshall Dawson. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD devices, picasso cannot rely on the cache-as-RAM setup code to properly enable MTRRs. Add that capability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
BUG=b:147042464 TEST=Boot trembyle to payload and make sure bootblock isn't abnormally slow.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/38691/10
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:
(2 comments)
Patch Set 9: Code-Review+1
There is a comment on previous CL from Julius about using _Static_assert instead of the power of 2 check in Makefile. Other than that, LGTM.
Not for this change, but I think we want to set up the caching for romstage as well here?
Split the alignment check into a different CL so it doesn't block this.
There is another patch in the train for setting up romstage cache. I'll be working on that next.
https://review.coreboot.org/c/coreboot/+/38691/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38691/9//COMMIT_MSG@9 PS9, Line 9: programs
devices?
Done
https://review.coreboot.org/c/coreboot/+/38691/9//COMMIT_MSG@16 PS9, Line 16: TEST=Boot trembyle to payload
Any changes with this commit (boot time, …)?
Done
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 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
soc/amd/picasso: Enable cache in bootblock
Unlike prior AMD devices, picasso cannot rely on the cache-as-RAM setup code to properly enable MTRRs. Add that capability to the bootblock_c_entry() function. In addition, enable an MTRR to cache (WP) the flash boot device and another for WB of the non-XIP bootblock running in DRAM.
BUG=b:147042464 TEST=Boot trembyle to payload and make sure bootblock isn't abnormally slow.
Change-Id: I5615ff60ca196e622a939b46276a4a0940076ebe Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38691 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/bootblock/bootblock.c 1 file changed, 33 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c index 8ae4db3..6a0fd85 100644 --- a/src/soc/amd/picasso/bootblock/bootblock.c +++ b/src/soc/amd/picasso/bootblock/bootblock.c @@ -2,14 +2,47 @@ /* This file is part of the coreboot project. */
#include <stdint.h> +#include <symbols.h> #include <bootblock_common.h> #include <console/console.h> +#include <cpu/x86/cache.h> +#include <cpu/x86/msr.h> +#include <cpu/amd/msr.h> +#include <cpu/x86/mtrr.h> +#include <cpu/amd/mtrr.h> #include <soc/southbridge.h> #include <soc/i2c.h> #include <amdblocks/amd_pci_mmconf.h>
+static void set_caching(void) +{ + msr_t deftype = {0, 0}; + int mtrr; + + /* Disable fixed and variable MTRRs while we setup */ + wrmsr(MTRR_DEF_TYPE_MSR, deftype); + + clear_all_var_mtrr(); + + mtrr = get_free_var_mtrr(); + if (mtrr >= 0) + set_var_mtrr(mtrr, FLASH_BASE_ADDR, CONFIG_ROM_SIZE, MTRR_TYPE_WRPROT); + + mtrr = get_free_var_mtrr(); + if (mtrr >= 0) + set_var_mtrr(mtrr, (unsigned int)_bootblock, REGION_SIZE(bootblock), + MTRR_TYPE_WRBACK); + + /* Enable variable MTRRs. Fixed MTRRs are left disabled since they are not used. */ + deftype.lo |= MTRR_DEF_TYPE_EN | MTRR_TYPE_UNCACHEABLE; + wrmsr(MTRR_DEF_TYPE_MSR, deftype); + + enable_cache(); +} + asmlinkage void bootblock_c_entry(uint64_t base_timestamp) { + set_caching(); enable_pci_mmconf();
bootblock_main_with_basetime(base_timestamp);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 13:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2994 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2993 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2992 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2991
Please note: This test is under development and might not be accurate at all!
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 13:
when testing against a fairly old fsp some changes between changeset 6 and 13 break boot on mandolin for me. does this require an updated fsp version?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 13:
Patch Set 13:
when testing against a fairly old fsp some changes between changeset 6 and 13 break boot on mandolin for me. does this require an updated fsp version?
can also try to redo my local rebase to see if i just messed up something in there
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 13:
Patch Set 13:
Patch Set 13:
when testing against a fairly old fsp some changes between changeset 6 and 13 break boot on mandolin for me. does this require an updated fsp version?
can also try to redo my local rebase to see if i just messed up something in there
I only test with the latest FSP.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13:
Patch Set 13:
when testing against a fairly old fsp some changes between changeset 6 and 13 break boot on mandolin for me. does this require an updated fsp version?
can also try to redo my local rebase to see if i just messed up something in there
I only test with the latest FSP.
ok, i'll try with a fresh fsp again. just wanted to change as little as possible at a time
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38691 )
Change subject: soc/amd/picasso: Enable cache in bootblock ......................................................................
Patch Set 13: Code-Review+2
does this require an updated fsp version?
a fresh fsp build fixed the issue for me
-
9elements QA (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Marshall Dawson (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)