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); } -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: newchange

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 -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 07 Aug 2020 20:55:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

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 -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-CC: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset

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 -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-MessageType: newpatchset

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. -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Comment-Date: Wed, 18 May 2022 09:06:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

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. -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Comment-Date: Wed, 18 May 2022 09:17:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-MessageType: comment

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? -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Comment-Date: Wed, 18 May 2022 09:20:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-MessageType: comment

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?
-- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Comment-Date: Wed, 18 May 2022 11:50:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-MessageType: comment

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. -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Comment-Date: Wed, 18 May 2022 12:13:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

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.
-- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Comment-Date: Wed, 18 May 2022 12:59:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment

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. -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Comment-Date: Wed, 18 May 2022 12:59:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-MessageType: comment

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 -- To view, visit https://review.coreboot.org/c/coreboot/+/44293 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c6df8951d39695cddd4635360d6407d4d001b0a Gerrit-Change-Number: 44293 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski@3mdeb.com> Gerrit-Reviewer: Mike Banon <mikebdp2@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans@9elements.com> Gerrit-CC: Michał Kopeć <michal.kopec@3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-MessageType: newpatchset
participants (3)
-
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)