Change in coreboot[master]: [UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer arit...
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: [UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
[UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic
The size of the data used is fixed in this function so there is no need for this aritmetic.
The function signature will be changed in a followup commit.
Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/amd/agesa/s3_mtrr.c 1 file changed, 53 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/44293/1
diff --git a/src/drivers/amd/agesa/s3_mtrr.c b/src/drivers/amd/agesa/s3_mtrr.c index 6c7ba5f..672eb20 100644 --- a/src/drivers/amd/agesa/s3_mtrr.c +++ b/src/drivers/amd/agesa/s3_mtrr.c @@ -8,114 +8,82 @@ #include <string.h> #include <northbridge/amd/agesa/agesa_helper.h>
-static void write_mtrr(u8 **p_nvram_pos, unsigned int idx) -{ - msr_t msr_data; - msr_data = rdmsr(idx); - - memcpy(*p_nvram_pos, &msr_data, sizeof(msr_data)); - *p_nvram_pos += sizeof(msr_data); -} +struct mtrr_backup { + msr_t mtrr_fix_64k_00000; + msr_t mtrr_fix_16k_80000; + msr_t mtrr_fix_16k_a0000; + msr_t mtrr_fix_4k_xx000[8]; /* 4K_C0000 -- 4K_F8000 */ + msr_t mtrr_phys_base[8]; + msr_t mtrr_phys_mask[8]; + msr_t syscfg; + msr_t top_mem; + msr_t top_mem2; +};
void backup_mtrr(void *mtrr_store, u32 *mtrr_store_size) { - u8 *nvram_pos = mtrr_store; - msr_t msr_data; - u32 i; + msr_t syscfg_msr; + struct mtrr_backup *mtrr_save = (struct mtrr_backup *)mtrr_store; + uint32_t i;
/* Enable access to AMD RdDram and WrDram extension bits */ - msr_data = rdmsr(SYSCFG_MSR); - msr_data.lo |= SYSCFG_MSR_MtrrFixDramModEn; - wrmsr(SYSCFG_MSR, msr_data); + syscfg_msr = rdmsr(SYSCFG_MSR); + syscfg_msr.lo |= SYSCFG_MSR_MtrrFixDramModEn; + wrmsr(SYSCFG_MSR, syscfg_msr);
- /* Fixed MTRRs */ - write_mtrr(&nvram_pos, MTRR_FIX_64K_00000); - write_mtrr(&nvram_pos, MTRR_FIX_16K_80000); - write_mtrr(&nvram_pos, MTRR_FIX_16K_A0000); + mtrr_save->mtrr_fix_64k_00000 = rdmsr(MTRR_FIX_64K_00000); + mtrr_save->mtrr_fix_16k_80000 = rdmsr(MTRR_FIX_16K_80000); + mtrr_save->mtrr_fix_16k_a0000 = rdmsr(MTRR_FIX_16K_A0000);
- for (i = MTRR_FIX_4K_C0000; i <= MTRR_FIX_4K_F8000; i++) - write_mtrr(&nvram_pos, i); + for (i = 0; i < 8; i++) + mtrr_save->mtrr_fix_4k_xx000[i] = rdmsr(MTRR_FIX_4K_C0000 + i);
/* Disable access to AMD RdDram and WrDram extension bits */ - msr_data = rdmsr(SYSCFG_MSR); - msr_data.lo &= ~SYSCFG_MSR_MtrrFixDramModEn; - wrmsr(SYSCFG_MSR, msr_data); + syscfg_msr = rdmsr(SYSCFG_MSR); + syscfg_msr.lo &= ~SYSCFG_MSR_MtrrFixDramModEn; + wrmsr(SYSCFG_MSR, syscfg_msr);
- /* Variable MTRRs */ - for (i = MTRR_PHYS_BASE(0); i < MTRR_PHYS_BASE(8); i++) - write_mtrr(&nvram_pos, i); + for (i = 0; i < 8; i++) { + mtrr_save->mtrr_phys_mask[i] = rdmsr(MTRR_PHYS_BASE(i)); + mtrr_save->mtrr_phys_base[i] = rdmsr(MTRR_PHYS_BASE(i)); + }
- /* SYSCFG_MSR */ - write_mtrr(&nvram_pos, SYSCFG_MSR); - /* TOM */ - write_mtrr(&nvram_pos, TOP_MEM); - /* TOM2 */ - write_mtrr(&nvram_pos, TOP_MEM2); + mtrr_save->syscfg = rdmsr(SYSCFG_MSR); + mtrr_save->top_mem = rdmsr(TOP_MEM); + mtrr_save->top_mem2 = rdmsr(TOP_MEM2);
- *mtrr_store_size = nvram_pos - (u8*) mtrr_store; + *mtrr_store_size = sizeof(struct mtrr_backup); }
void restore_mtrr(void) { - volatile u32 *msrPtr = (u32 *) OemS3Saved_MTRR_Storage(); - u32 msr; - msr_t msr_data; - - if (!msrPtr) - return; - - disable_cache(); + msr_t syscfg_msr; + struct mtrr_backup *mtrr_save = (struct mtrr_backup *)OemS3Saved_MTRR_Storage(); + uint32_t i;
/* Enable access to AMD RdDram and WrDram extension bits */ - msr_data = rdmsr(SYSCFG_MSR); - msr_data.lo |= SYSCFG_MSR_MtrrFixDramModEn; - wrmsr(SYSCFG_MSR, msr_data); + syscfg_msr = rdmsr(SYSCFG_MSR); + syscfg_msr.lo |= SYSCFG_MSR_MtrrFixDramModEn; + wrmsr(SYSCFG_MSR, syscfg_msr);
- /* Now restore the Fixed MTRRs */ - msr_data.lo = *msrPtr; - msrPtr ++; - msr_data.hi = *msrPtr; - msrPtr ++; - wrmsr(MTRR_FIX_64K_00000, msr_data); + wrmsr(MTRR_FIX_64K_00000, mtrr_save->mtrr_fix_64k_00000); + wrmsr(MTRR_FIX_16K_80000, mtrr_save->mtrr_fix_16k_80000); + wrmsr(MTRR_FIX_16K_A0000, mtrr_save->mtrr_fix_16k_a0000);
- msr_data.lo = *msrPtr; - msrPtr ++; - msr_data.hi = *msrPtr; - msrPtr ++; - wrmsr(MTRR_FIX_16K_80000, msr_data); - - msr_data.lo = *msrPtr; - msrPtr ++; - msr_data.hi = *msrPtr; - msrPtr ++; - wrmsr(MTRR_FIX_16K_A0000, msr_data); - - for (msr = MTRR_FIX_4K_C0000; msr <= MTRR_FIX_4K_F8000; msr++) { - msr_data.lo = *msrPtr; - msrPtr ++; - msr_data.hi = *msrPtr; - msrPtr ++; - wrmsr(msr, msr_data); - } + for (i = 0; i < 8; i++) + wrmsr(MTRR_FIX_4K_C0000 + i, mtrr_save->mtrr_fix_4k_xx000[i]);
/* Disable access to AMD RdDram and WrDram extension bits */ - msr_data = rdmsr(SYSCFG_MSR); - msr_data.lo &= ~SYSCFG_MSR_MtrrFixDramModEn; - wrmsr(SYSCFG_MSR, msr_data); + syscfg_msr = rdmsr(SYSCFG_MSR); + syscfg_msr.lo &= ~SYSCFG_MSR_MtrrFixDramModEn; + wrmsr(SYSCFG_MSR, syscfg_msr);
- /* Restore the Variable MTRRs */ - for (msr = MTRR_PHYS_BASE(0); msr <= MTRR_PHYS_MASK(7); msr++) { - msr_data.lo = *msrPtr; - msrPtr ++; - msr_data.hi = *msrPtr; - msrPtr ++; - wrmsr(msr, msr_data); + for (i = 0; i < 8; i++) { + wrmsr(MTRR_PHYS_BASE(i), mtrr_save->mtrr_phys_mask[i]); + wrmsr(MTRR_PHYS_BASE(i), mtrr_save->mtrr_phys_base[i]); }
- /* Restore SYSCFG MTRR */ - msr_data.lo = *msrPtr; - msrPtr ++; - msr_data.hi = *msrPtr; - msrPtr ++; - wrmsr(SYSCFG_MSR, msr_data); + wrmsr(SYSCFG_MSR, mtrr_save->syscfg); + wrmsr(TOP_MEM, mtrr_save->top_mem); + wrmsr(TOP_MEM2, mtrr_save->top_mem2); }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: [UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44293/1/src/drivers/amd/agesa/s3_mt... File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/1/src/drivers/amd/agesa/s3_mt... PS1, Line 61: struct mtrr_backup *mtrr_save = (struct mtrr_backup *)OemS3Saved_MTRR_Storage(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/44293/1/src/drivers/amd/agesa/s3_mt... PS1, Line 61: struct mtrr_backup *mtrr_save = (struct mtrr_backup *)OemS3Saved_MTRR_Storage(); please, no spaces at the start of a line
Hello Mike Banon, Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44293
to look at the new patch set (#2).
Change subject: [UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
[UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic
The size of the data used is fixed in this function so there is no need for this aritmetic.
The function signature will be changed in a followup commit.
Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/amd/agesa/s3_mtrr.c 1 file changed, 53 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/44293/2
Hello Mike Banon, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44293
to look at the new patch set (#3).
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic
The size of the data used is fixed in this function so there is no need for this aritmetic.
The function signature will be changed in a followup commit.
UNTESTED.
Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/amd/agesa/s3_mtrr.c 1 file changed, 53 insertions(+), 85 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/44293/3
Attention is currently required from: Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(3 comments)
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/205de028_aa257c05 PS4, Line 67: disable_cache(); Disappears without argumentation.
https://review.coreboot.org/c/coreboot/+/44293/comment/2966db87_25e69f34 PS4, Line 47: mtrr_save->mtrr_phys_mask[i] = rdmsr(MTRR_PHYS_BASE(i)); MTRR_PHYS_MASK(i)
Maybe you want to read BASE() first, althought it does not matter
https://review.coreboot.org/c/coreboot/+/44293/comment/c4167b25_7e6f209b PS4, Line 82: wrmsr(MTRR_PHYS_BASE(i), mtrr_save->mtrr_phys_mask[i]); MTRR_PHYS_MASK(i)
Since valid bit is in MASK register, BASE should be programmed first.
Attention is currently required from: Kyösti Mälkki. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS4: It looks like this code would be obsolete with https://review.coreboot.org/c/coreboot/+/52781/21
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/2592af69_03b3bc55 PS4, Line 67: disable_cache();
Disappears without argumentation.
Probably not intended but actually it's not needed.
Attention is currently required from: Kyösti Mälkki. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(1 comment)
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/77f59c52_c6f0959d PS4, Line 47: mtrr_save->mtrr_phys_mask[i] = rdmsr(MTRR_PHYS_BASE(i));
MTRR_PHYS_MASK(i)
Maybe you want to read BASE() first, althought it does not matter
You are right. It also looks like the default MTRR_DEF_TYPE_MSR is not backup/set?
Attention is currently required from: Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS4:
It looks like this code would be obsolete with https://review.coreboot. […]
Perhaps, but 3mdeb might not have intention to fix fam15tn or fam16kb.
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/125f1856_692006a5 PS4, Line 47: mtrr_save->mtrr_phys_mask[i] = rdmsr(MTRR_PHYS_BASE(i));
MTRR_PHYS_MASK(i) […]
Hmm.. yes, MTRR solution with default type as WB might resume incorrectly?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(1 comment)
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/0b40c753_7982ad9d PS4, Line 10: : struct mtrr_backup { : msr_t mtrr_fix_64k_00000; : msr_t mtrr_fix_16k_80000; : msr_t mtrr_fix_16k_a0000; : msr_t mtrr_fix_4k_xx000[8]; /* 4K_C0000 -- 4K_F8000 */ : msr_t mtrr_phys_base[8]; : msr_t mtrr_phys_mask[8]; : msr_t syscfg; : msr_t top_mem; : msr_t top_mem2; : }; The code probably looks a lot simpler using a MSR address array.
Attention is currently required from: Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(1 comment)
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/fecf643e_3183d67f PS4, Line 10: : struct mtrr_backup { : msr_t mtrr_fix_64k_00000; : msr_t mtrr_fix_16k_80000; : msr_t mtrr_fix_16k_a0000; : msr_t mtrr_fix_4k_xx000[8]; /* 4K_C0000 -- 4K_F8000 */ : msr_t mtrr_phys_base[8]; : msr_t mtrr_phys_mask[8]; : msr_t syscfg; : msr_t top_mem; : msr_t top_mem2; : };
The code probably looks a lot simpler using a MSR address array.
Maybe it is wiser to put effort in PARALLEL_MP for fam15tn/16kb if you already made it compatible with ASEG_SMM. Looks like save_bsp_msrs() and load_msr in sipi_vector.S achieves the same thing.
Attention is currently required from: Kyösti Mälkki. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44293 )
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
Patch Set 4:
(1 comment)
File src/drivers/amd/agesa/s3_mtrr.c:
https://review.coreboot.org/c/coreboot/+/44293/comment/e2417a12_b191e5dc PS4, Line 10: : struct mtrr_backup { : msr_t mtrr_fix_64k_00000; : msr_t mtrr_fix_16k_80000; : msr_t mtrr_fix_16k_a0000; : msr_t mtrr_fix_4k_xx000[8]; /* 4K_C0000 -- 4K_F8000 */ : msr_t mtrr_phys_base[8]; : msr_t mtrr_phys_mask[8]; : msr_t syscfg; : msr_t top_mem; : msr_t top_mem2; : };
Maybe it is wiser to put effort in PARALLEL_MP for fam15tn/16kb if you already made it compatible with ASEG_SMM. Looks like save_bsp_msrs() and load_msr in sipi_vector.S achieves the same thing.
Agreed.
Attention is currently required from: Kyösti Mälkki. Hello Mike Banon, build bot (Jenkins), Michał Żygowski, Kyösti Mälkki,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44293
to look at the new patch set (#5).
Change subject: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic ......................................................................
drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic
The size of the data used is fixed in this function so there is no need for this aritmetic.
The function signature will be changed in a followup commit.
The cache_disable call is dropped as all the codepaths calling the restore_mtrr function do this already.
UNTESTED.
Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/amd/agesa/oem_s3.c M src/drivers/amd/agesa/s3_mtrr.c 2 files changed, 54 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/44293/5
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)