Change in ...coreboot[master]: cpu/intel/car/core2: Update microcode in CAR setup

[Arthur Heymans (Code Review)]

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

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

cpu/intel/car/core2: Update microcode in CAR setup

This updates the BSP microcode during CAR setup.

Change-Id: I8ecd1b99e18c7e73ae47f850fdc4870751dc6e8c
Signed-off-by: Arthur Heymans 
---
M src/cpu/intel/car/core2/cache_as_ram.S
1 file changed, 28 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/30775/1

diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S
index 981b12d..4fb8c66 100644
--- a/src/cpu/intel/car/core2/cache_as_ram.S
+++ b/src/cpu/intel/car/core2/cache_as_ram.S
@@ -111,6 +111,17 @@
    movl	$(~(CACHE_AS_RAM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax
    wrmsr

+	/* Cache the whole rom to fetch microcode updates */
+	movl	$MTRR_PHYS_BASE(1), %ecx
+	xorl	%edx, %edx
+	movl	$CACHE_ROM_BASE | MTRR_TYPE_WRPROT, %eax
+	wrmsr
+
+	movl	$MTRR_PHYS_MASK(1), %ecx
+	rdmsr
+	movl	$(~(CACHE_ROM_SIZE - 1) | MTRR_PHYS_MASK_VALID), %eax
+	wrmsr
+
    post_code(0x25)

/* Enable MTRR. */
@@ -131,15 +142,17 @@
    invd
    movl	%eax, %cr0

-	/* Clear the cache memory region. This will also fill up the cache. */
-	movl	$CACHE_AS_RAM_BASE, %esi
-	movl	%esi, %edi
-	movl	$(CACHE_AS_RAM_SIZE >> 2), %ecx
-	xorl	%eax, %eax
-	rep	stosl
+#if IS_ENABLED(CONFIG_MICROCODE_UPDATE_PRE_RAM)
+update_microcode:
+	/* put the return address in %esp */
+	movl	$end_microcode_update, %esp
+	jmp	update_bsp_microcode
+end_microcode_update:
+#endif

+cache_xip:
    post_code(0x26)
-	/* Enable Cache-as-RAM mode by disabling cache. */
+	/* Disable caching to set final MTRRs for speeding up XIP. */
    movl	%cr0, %eax
    orl	$CR0_CacheDisable, %eax
    movl	%eax, %cr0
@@ -165,8 +178,16 @@
    /* Enable cache. */
    movl	%cr0, %eax
    andl	$(~(CR0_CacheDisable | CR0_NoWriteThrough)), %eax
+	invd
    movl	%eax, %cr0

+	/* Clear the cache memory region. This will also fill up the cache. */
+	movl	$CACHE_AS_RAM_BASE, %esi
+	movl	%esi, %edi
+	movl	$(CACHE_AS_RAM_SIZE >> 2), %ecx
+	xorl	%eax, %eax
+	rep	stosl
+
    /* Setup the stack. */
    mov	$_car_stack_end, %esp

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

Kyösti Mälkki has uploaded a new patch set (#3) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

cpu/intel/car/core2: Update microcode in CAR setup

This updates the BSP microcode during CAR setup.

Change-Id: I8ecd1b99e18c7e73ae47f850fdc4870751dc6e8c
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/30682
---
M src/cpu/intel/car/core2/cache_as_ram.S
1 file changed, 28 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/30775/3

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/30775/3/src/cpu/intel/car/core2/cache_as_ram... 
File src/cpu/intel/car/core2/cache_as_ram.S:

https://review.coreboot.org/#/c/30775/3/src/cpu/intel/car/core2/cache_as_ram... 
PS3, Line 117: CACHE_ROM_BASE | MTRR_TYPE_WRPROT
CACHE_ROM_BASE | MTRR_TYPE_WRPROT

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Patch Set 3:
> (1 comment)

() are missing

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

Kyösti Mälkki has uploaded a new patch set (#4) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

cpu/intel/car/core2: Update microcode in CAR setup

This updates the BSP microcode during CAR setup.

Change-Id: I8ecd1b99e18c7e73ae47f850fdc4870751dc6e8c
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/30682
---
M src/cpu/intel/car/core2/cache_as_ram.S
1 file changed, 28 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/30775/4

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/30775/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30775/4//COMMIT_MSG@9 
PS4, Line 9: This updates the BSP microcode during CAR setup.
Why?

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/30775/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30775/4//COMMIT_MSG@9 
PS4, Line 9: This updates the BSP microcode during CAR setup.
> Why?

Updating microcode during CAR hangs the system. Updating the BSP microcode seems to be done by most vendor firmware quite early on. On later systems it is required but these older ones it seems to be ok not to do it...

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/30775/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30775/4//COMMIT_MSG@9 
PS4, Line 9: This updates the BSP microcode during CAR setup.
> Updating microcode during CAR hangs the system. […]

I just wondered if it's worth to have a conditional
path here. If we know that updating ucode in ramstage
is good enough, why not do it there? We'd not only
save some lines but also the additional CBFS walk.

Change in coreboot[master]: cpu/intel/car/core2: Update microcode in CAR setup

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

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Patch Set 4:

abandon?

Change in coreboot[master]: cpu/intel/car/core2: Update microcode in CAR setup

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/30775 )

Change subject: cpu/intel/car/core2: Update microcode in CAR setup
......................................................................

Abandoned

It is observed to work fine until CPU init in ramstage.