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 
---
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) (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

[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 
---
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

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 
---
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