Change in coreboot[master]: soc/intel/common/cache_as_ram.S: Add macro to clear CAR

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 15 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37191/1

diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S
index d1ae6ca..6a0728d 100644
--- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S
+++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S
@@ -48,6 +48,17 @@
 3:
    .endm

+	.macro clear_car
+	/* Clear the cache memory region. This will also fill up the cache */
+	movl	$CONFIG_DCACHE_RAM_BASE, %edi
+	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
+	shr	$0x02, %ecx
+	xor	%eax, %eax
+	cld
+	rep	stosl
+	.endm
+
+
 .global bootblock_pre_c_entry
 bootblock_pre_c_entry:

@@ -256,13 +267,7 @@

post_code(0x26)

-	/* Clear the cache memory region. This will also fill up the cache */
-	movl	$CONFIG_DCACHE_RAM_BASE, %edi
-	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
-	shr	$0x02, %ecx
-	xor	%eax, %eax
-	cld
-	rep	stosl
+	clear_car

post_code(0x27)

@@ -353,13 +358,7 @@

post_code(0x26)

-	/* Clear the cache memory region. This will also fill up the cache */
-	movl	$CONFIG_DCACHE_RAM_BASE, %edi
-	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
-	shr	$0x02, %ecx
-	xor	%eax, %eax
-	cld
-	rep	stosl
+	clear_car

post_code(0x27)

@@ -462,12 +461,8 @@
    xorl	%edx, %edx
    wrmsr

-	movl	$CONFIG_DCACHE_RAM_BASE, %edi
-	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
-	shr	$0x02, %ecx
-	xor	%eax, %eax
-	cld
-	rep	stosl
+	clear_car
+
    /*
     * Set IA32_PQR_ASSOC = 0x01
     * At this stage we apply LLC_WAY_MASK_1 to the cache.

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37191

to look at the new patch set (#2).

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

TEST: with BUILD_TIMELESS=1 the resulting binary is identical.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 14 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37191/2

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37191

to look at the new patch set (#3).

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

TEST: with BUILD_TIMELESS=1 the resulting binary is identical.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 21 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37191/3

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 4: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37191/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37191/4/src/soc/intel/common/block/... 
PS4, Line 58: .macro clear_car
Maybe don't indent this? it would make things more consistent

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, Angel Pons, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37191

to look at the new patch set (#5).

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

TEST: with BUILD_TIMELESS=1 the resulting binary is identical.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 21 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37191/5

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 6: Code-Review+2

[Arthur Heymans (Code Review)]

Attention is currently required from: Arthur Heymans.
Arthur Heymans has uploaded a new patch set (#7) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

TEST: with BUILD_TIMELESS=1 the resulting binary is identical.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 22 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37191/7

[Angel Pons (Code Review)]

Attention is currently required from: Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 7:

(2 comments)

File src/soc/intel/common/block/cpu/car/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37191/comment/1f55f2b2_d628db98 
PS4, Line 58: .macro clear_car
> Maybe don't indent this? it would make things more consistent

Ack

File src/soc/intel/common/block/cpu/car/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37191/comment/8b920345_a24bf5d2 
PS7, Line 44: 	/*
nit: drop indentation for comment?

[Arthur Heymans (Code Review)]

Attention is currently required from: Arthur Heymans, Angel Pons.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 7:

(1 comment)

File src/soc/intel/common/block/cpu/car/cache_as_ram.S:

https://review.coreboot.org/c/coreboot/+/37191/comment/ac1646ba_79393293 
PS7, Line 44: 	/*
> nit: drop indentation for comment?

Done

[Arthur Heymans (Code Review)]

Attention is currently required from: Arthur Heymans, Angel Pons.
Hello build bot (Jenkins), Patrick Georgi, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37191

to look at the new patch set (#8).

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

TEST: with BUILD_TIMELESS=1 the resulting binary is identical.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 22 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37191/8

[Angel Pons (Code Review)]

Attention is currently required from: Arthur Heymans.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 9:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/37191/comment/30566ba1_6d26bc65 
PS9, Line 11: the resulting binary
For which board?

[Arthur Heymans (Code Review)]

Attention is currently required from: Angel Pons.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 9:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/37191/comment/71045dc4_2376b1fe 
PS9, Line 11: the resulting binary
> For which board?

I don't remember. I just copied the message from something like 2 years ago. It should be the same for all code using cache_as_ram.S

[Patrick Rudolph (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 9: Code-Review+2

[Patrick Georgi (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

Patch Set 9: Code-Review+2

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/37191/comment/c4db32f9_c0a38cb7 
PS9, Line 11: the resulting binary
> > For which board? […]

Looking at the code, I see little room for divergence, so "all boards" seems to be both right and redundant, so let's leave it the way it is.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37191 )

Change subject: soc/intel/common/cache_as_ram.S: Add macro to clear CAR
......................................................................

soc/intel/common/cache_as_ram.S: Add macro to clear CAR

Add a macro to clear CAR which is replicated 3 times in this code.

TEST: with BUILD_TIMELESS=1 the resulting binary is identical.

Change-Id: Iec28e3f393c4fe222bfb0d5358f815691ec199ae
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37191
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Patrick Georgi 
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 22 insertions(+), 20 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S
index db1345a..5da453b 100644
--- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S
+++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S
@@ -41,6 +41,23 @@
    decl	%ecx
 .endm

+/*
+ * macro: clear_car
+ * Clears the region between CONFIG_DCACHE_RAM_BASE and
+ * CONFIG_DCACHE_RAM_BASE + CONFIG_DCACHE_RAM_SIZE to populate
+ * cachelines.
+ * Clobbers %eax, %ecx, %edi.
+ */
+.macro clear_car
+	/* Clear the cache memory region. This will also fill up the cache */
+	movl	$CONFIG_DCACHE_RAM_BASE, %edi
+	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
+	shr	$0x02, %ecx
+	xor	%eax, %eax
+	cld
+	rep	stosl
+.endm
+
 .global bootblock_pre_c_entry
 bootblock_pre_c_entry:

@@ -256,13 +273,7 @@

post_code(0x26)

-	/* Clear the cache memory region. This will also fill up the cache */
-	movl	$CONFIG_DCACHE_RAM_BASE, %edi
-	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
-	shr	$0x02, %ecx
-	xor	%eax, %eax
-	cld
-	rep	stosl
+	clear_car

post_code(0x27)

@@ -353,13 +364,7 @@

post_code(0x26)

-	/* Clear the cache memory region. This will also fill up the cache */
-	movl	$CONFIG_DCACHE_RAM_BASE, %edi
-	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
-	shr	$0x02, %ecx
-	xor	%eax, %eax
-	cld
-	rep	stosl
+	clear_car

post_code(0x27)

@@ -518,12 +523,9 @@
    movl	$0x02, %eax
 #endif
    wrmsr
-	movl	$CONFIG_DCACHE_RAM_BASE, %edi
-	movl	$CONFIG_DCACHE_RAM_SIZE, %ecx
-	shr	$0x02, %ecx
-	xor	%eax, %eax
-	cld
-	rep	stosl
+
+	clear_car
+
    /*
     * Set IA32_PQR_ASSOC
     * At this stage we apply LLC_WAY_MASK_1 to the cache.