Change in coreboot[master]: soc/intel/braswell: Attempt to fix the google FSP binary not booting
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Attempt to fix the google FSP binary not booting ......................................................................
soc/intel/braswell: Attempt to fix the google FSP binary not booting
Previously the romcc bootblock did set MTRR's to cache the rom and updated microcode. Try the same...
Change-Id: I3e81329854e823dc66fec191adbed617bb37d649 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S 2 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/36198/1
diff --git a/src/drivers/intel/fsp1_1/Makefile.inc b/src/drivers/intel/fsp1_1/Makefile.inc index a40ed85..bfdbd03 100644 --- a/src/drivers/intel/fsp1_1/Makefile.inc +++ b/src/drivers/intel/fsp1_1/Makefile.inc @@ -23,6 +23,7 @@ bootblock-y += bootblock.c bootblock-$(CONFIG_USE_GENERIC_FSP_CAR_INC) += cache_as_ram.S bootblock-y += fsp_util.c +bootblock-y += ../../../cpu/intel/microcode/microcode_asm.S
romstage-y += car.c romstage-y += fsp_util.c diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index 3460b9d..007b974 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -17,6 +17,8 @@ * GNU General Public License for more details. */
+#include <cpu/x86/mtrr.h> +#include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
/* @@ -49,6 +51,41 @@ cache_as_ram: post_code(0x20)
+ /* Cache the rom and update the microcode */ +cache_rom: + /* Disable cache */ + movl %cr0, %eax + orl $CR0_CacheDisable, %eax + movl %eax, %cr0 + + 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 + + /* Enable cache */ + movl %cr0, %eax + andl $(~(CR0_CacheDisable | CR0_NoWriteThrough)), %eax + invd + movl %eax, %cr0 + + /* Enable MTRR. */ + movl $MTRR_DEF_TYPE_MSR, %ecx + rdmsr + orl $MTRR_DEF_TYPE_EN, %eax + wrmsr + +update_microcode: + /* put the return address in %esp */ + movl $end_microcode_update, %esp + jmp update_bsp_microcode +end_microcode_update: + /* * Find the FSP binary in cbfs. * Make a fake stack that has the return value back to this code.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Attempt to fix the google FSP binary not booting ......................................................................
Patch Set 1: Code-Review+1
google/cyan and google/edgar verified booting
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Attempt to fix the google FSP binary not booting ......................................................................
Patch Set 1:
(2 comments)
Patchset works fine on Facebook FBG1701.
https://review.coreboot.org/c/coreboot/+/36198/1/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36198/1/src/drivers/intel/fsp1_1/ca... PS1, Line 83: update_microcode: Why not have FSP updating the microcode?
https://review.coreboot.org/c/coreboot/+/36198/1/src/drivers/intel/fsp1_1/ca... PS1, Line 244: .long CONFIG_CPU_MICROCODE_CBFS_LOC /* Microcode Location */ Is this real cause of the issue? If microcode is dynamically added, CPU_MICROCODE_CBFS_LOC is not correct. The updated_bsp_microcde uses CBFS location for microcode location.
Hello Patrick Rudolph, Huang Jin, Lee Leahy, Matt DeVillier, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36198
to look at the new patch set (#2).
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
soc/intel/braswell: Update microcode before FSP
The google FSP Braswell version has broken microcode update code and FSP checks at some point if the installed microcode version is non zero, so coreboot has to update it before calling FSP-T.
This is fixed with newer FSP releases by Intel, but doing updates won't hurt.
Tested with both Intel FSP and google FSP.
Change-Id: I3e81329854e823dc66fec191adbed617bb37d649 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S 2 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/36198/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3:
Any boot time penalty?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3:
Patch Set 3:
Any boot time penalty?
Typically you don't have many different MCU included on these SOC. So you have to loop through 2-4 offset and then do the update. Previously this was done in the romcc bootblock so it should not present a big regression. The follow up patch (with fake MCU) might actually provide a speedup since the FSP only has to loop once through the MCU.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3:
Any boot time penalty?
on google/cyan, it's actually 50ms faster than the previous working build (pre-C_ENVIRONMENT_BOOTBLOCK)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3:
Patch Set 3:
Any boot time penalty?
on google/cyan, it's actually 50ms faster than the previous working build (pre-C_ENVIRONMENT_BOOTBLOCK)
Previously ROM caching was only set up after microcode update while here it is done before. That probably explains the speedup. BTW FSP does not set up caching before microcode updates either so this patch series likely improves bootspeed in general.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3: Code-Review+1
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
Works fine on Facebook FBG1701
https://review.coreboot.org/c/coreboot/+/36198/1/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36198/1/src/drivers/intel/fsp1_1/ca... PS1, Line 83: update_microcode:
Why not have FSP updating the microcode?
Done
https://review.coreboot.org/c/coreboot/+/36198/1/src/drivers/intel/fsp1_1/ca... PS1, Line 244: .long CONFIG_CPU_MICROCODE_CBFS_LOC /* Microcode Location */
Is this real cause of the issue? If microcode is dynamically added, CPU_MICROCODE_CBFS_LOC is not co […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36198/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36198/3//COMMIT_MSG@9 PS3, Line 9: The google FSP Braswell version has broken microcode update code and : FSP checks at some point if the installed microcode version is non : zero, so coreboot has to update it before calling FSP-T. : : This is fixed with newer FSP releases by Intel, but doing updates : won't hurt. This should probably go into the code to explain why things are done 'twice'.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 3: Code-Review+2
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, Matt DeVillier, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36198
to look at the new patch set (#4).
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
soc/intel/braswell: Update microcode before FSP
The google FSP Braswell version has broken microcode update code and FSP checks at some point if the installed microcode version is non zero, so coreboot has to update it before calling FSP-T.
This is fixed with newer FSP releases by Intel, but doing updates in coreboot won't hurt.
Tested with both Intel FSP and google FSP.
Change-Id: I3e81329854e823dc66fec191adbed617bb37d649 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S 2 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/36198/4
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36198/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36198/3//COMMIT_MSG@9 PS3, Line 9: The google FSP Braswell version has broken microcode update code and : FSP checks at some point if the installed microcode version is non : zero, so coreboot has to update it before calling FSP-T. : : This is fixed with newer FSP releases by Intel, but doing updates : won't hurt.
This should probably go into the code to explain why things are done 'twice'.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36198/4/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36198/4/src/drivers/intel/fsp1_1/ca... PS4, Line 86: blog blob?
Hello Patrick Rudolph, Huang Jin, Frans Hendriks, Lee Leahy, Matt DeVillier, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36198
to look at the new patch set (#5).
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
soc/intel/braswell: Update microcode before FSP
The google FSP Braswell version has broken microcode update code and FSP checks at some point if the installed microcode version is non zero, so coreboot has to update it before calling FSP-T.
This is fixed with newer FSP releases by Intel, but doing updates in coreboot won't hurt.
Tested with both Intel FSP and google FSP.
Change-Id: I3e81329854e823dc66fec191adbed617bb37d649 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S 2 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/36198/5
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36198/4/src/drivers/intel/fsp1_1/ca... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36198/4/src/drivers/intel/fsp1_1/ca... PS4, Line 86: blog
blob?
Thx. done.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
Patch Set 5: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36198 )
Change subject: soc/intel/braswell: Update microcode before FSP ......................................................................
soc/intel/braswell: Update microcode before FSP
The google FSP Braswell version has broken microcode update code and FSP checks at some point if the installed microcode version is non zero, so coreboot has to update it before calling FSP-T.
This is fixed with newer FSP releases by Intel, but doing updates in coreboot won't hurt.
Tested with both Intel FSP and google FSP.
Change-Id: I3e81329854e823dc66fec191adbed617bb37d649 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36198 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/fsp1_1/Makefile.inc M src/drivers/intel/fsp1_1/cache_as_ram.S 2 files changed, 42 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/Makefile.inc b/src/drivers/intel/fsp1_1/Makefile.inc index 85c4e0e..0143118 100644 --- a/src/drivers/intel/fsp1_1/Makefile.inc +++ b/src/drivers/intel/fsp1_1/Makefile.inc @@ -19,6 +19,7 @@
bootblock-$(CONFIG_USE_GENERIC_FSP_CAR_INC) += cache_as_ram.S bootblock-y += fsp_util.c +bootblock-y += ../../../cpu/intel/microcode/microcode_asm.S
romstage-y += car.c romstage-y += fsp_util.c diff --git a/src/drivers/intel/fsp1_1/cache_as_ram.S b/src/drivers/intel/fsp1_1/cache_as_ram.S index ffafe9b..f4638d9 100644 --- a/src/drivers/intel/fsp1_1/cache_as_ram.S +++ b/src/drivers/intel/fsp1_1/cache_as_ram.S @@ -11,6 +11,8 @@ * GNU General Public License for more details. */
+#include <cpu/x86/mtrr.h> +#include <cpu/x86/cache.h> #include <cpu/x86/post_code.h>
/* @@ -43,6 +45,45 @@ cache_as_ram: post_code(0x20)
+ /* Cache the rom and update the microcode */ +cache_rom: + /* Disable cache */ + movl %cr0, %eax + orl $CR0_CacheDisable, %eax + movl %eax, %cr0 + + 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 + + /* Enable cache */ + movl %cr0, %eax + andl $(~(CR0_CacheDisable | CR0_NoWriteThrough)), %eax + invd + movl %eax, %cr0 + + /* Enable MTRR. */ + movl $MTRR_DEF_TYPE_MSR, %ecx + rdmsr + orl $MTRR_DEF_TYPE_EN, %eax + wrmsr + + /* The Google FSP release for Braswell has broken microcode update + code and FSP needs the installed microcode revision to be non zero. + It is better to have coreboot do it instead of relying on a fragile + blob. */ +update_microcode: + /* put the return address in %esp */ + movl $end_microcode_update, %esp + jmp update_bsp_microcode +end_microcode_update: + /* * Find the FSP binary in cbfs. * Make a fake stack that has the return value back to this code.
-
Arthur Heymans (Code Review) -
Frans Hendriks (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)