Change in coreboot[master]: [UNTESTED]drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer arit...

[Arthur Heymans (Code Review)]

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 <[e-mail]>
---
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 <[e-mail]>
Gerrit-MessageType: newchange

[build bot (Jenkins) (Code Review)]

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 <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-CC: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Fri, 07 Aug 2020 20:55:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Arthur Heymans (Code Review)]

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 <[e-mail]>
---
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 <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-CC: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Arthur Heymans (Code Review)]

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 <[e-mail]>
---
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 <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Kyösti Mälkki (Code Review)]

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 <[e-mail]>
Gerrit-Reviewer: Kyösti Mälkki <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Michał Kopeć <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Arthur Heymans <[e-mail]>
Gerrit-Comment-Date: Wed, 18 May 2022 09:06:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Arthur Heymans (Code Review)]

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 <[e-mail]>
Gerrit-Reviewer: Kyösti Mälkki <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Michał Kopeć <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Kyösti Mälkki <[e-mail]>
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 <[e-mail]>
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Arthur Heymans (Code Review)]

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 <[e-mail]>
Gerrit-Reviewer: Kyösti Mälkki <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Michał Kopeć <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Kyösti Mälkki <[e-mail]>
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 <[e-mail]>
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Kyösti Mälkki (Code Review)]

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?

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Arthur Heymans (Code Review)]

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 <[e-mail]>
Gerrit-Reviewer: Kyösti Mälkki <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Michał Kopeć <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 18 May 2022 12:13:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Kyösti Mälkki (Code Review)]

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.

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Arthur Heymans (Code Review)]

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 <[e-mail]>
Gerrit-Reviewer: Kyösti Mälkki <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Michał Kopeć <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Kyösti Mälkki <[e-mail]>
Gerrit-Comment-Date: Wed, 18 May 2022 12:59:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <[e-mail]>
Comment-In-Reply-To: Kyösti Mälkki <[e-mail]>
Gerrit-MessageType: comment

Change in coreboot[master]: drivers/amd/agesa/s3_mtrr.c: Use a struct over pointer aritmetic

[Arthur Heymans (Code Review)]

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 <[e-mail]>
---
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 <[e-mail]>
Gerrit-Reviewer: Kyösti Mälkki <[e-mail]>
Gerrit-Reviewer: Michał Żygowski <[e-mail]>
Gerrit-Reviewer: Mike Banon <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Michał Kopeć <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Kyösti Mälkki <[e-mail]>
Gerrit-MessageType: newpatchset