Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T
It is a requirement for Firmware to have Firmware Interface Table (FIT), which contains pointers to each microcode update. The microcode update is loaded for all logical processors before reset vector.
FSPT_UPD.MicrocodeRegionBase∗∗ and FSPT_UPD.MicrocodeRegionLength are input parameters to TempRamInit API. If these values are 0, FSP will not attempt to update microcode.
Since Gen-6 all IA-SoC has FIT loading ucode even before cpu reset in place hence skipping FSP-T loading ucode after CPU reset options.
Change-Id: I3a406fa0e2e62e3363c2960e173dc5f5f5ca0455 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/fspcar.c M src/soc/intel/cannonlake/bootblock/bootblock.c M src/soc/intel/denverton_ns/bootblock/bootblock.c M src/soc/intel/skylake/fspcar.c 4 files changed, 50 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/37187/1
diff --git a/src/soc/intel/apollolake/fspcar.c b/src/soc/intel/apollolake/fspcar.c index 8b1089f..129ff55 100644 --- a/src/soc/intel/apollolake/fspcar.c +++ b/src/soc/intel/apollolake/fspcar.c @@ -25,6 +25,17 @@ .FsptCommonUpd = { .Revision = 0, .Reserved = {0}, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), whioch contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-6 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ .MicrocodeRegionBase = 0, .MicrocodeRegionLength = 0, .CodeRegionBase = diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c index 653ba30..9826748 100644 --- a/src/soc/intel/cannonlake/bootblock/bootblock.c +++ b/src/soc/intel/cannonlake/bootblock/bootblock.c @@ -30,10 +30,19 @@ .Reserved = {0}, }, .FsptCoreUpd = { - .MicrocodeRegionBase = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LOC, - .MicrocodeRegionSize = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LEN, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), whioch contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-6 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ + .MicrocodeRegionBase = 0, + .MicrocodeRegionLength = 0, .CodeRegionBase = (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE), .CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE, diff --git a/src/soc/intel/denverton_ns/bootblock/bootblock.c b/src/soc/intel/denverton_ns/bootblock/bootblock.c index f75de1f..58ce3de 100644 --- a/src/soc/intel/denverton_ns/bootblock/bootblock.c +++ b/src/soc/intel/denverton_ns/bootblock/bootblock.c @@ -31,10 +31,19 @@ .Reserved = {0}, }, .FsptCoreUpd = { - .MicrocodeRegionBase = - (UINT32)CONFIG_CPU_MICROCODE_CBFS_LOC, - .MicrocodeRegionLength = - (UINT32)CONFIG_CPU_MICROCODE_CBFS_LEN, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), whioch contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-6 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ + .MicrocodeRegionBase = 0, + .MicrocodeRegionLength = 0, .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_ROM_SIZE), .CodeRegionLength = (UINT32)CONFIG_ROM_SIZE, diff --git a/src/soc/intel/skylake/fspcar.c b/src/soc/intel/skylake/fspcar.c index a4c3726..fff240a 100644 --- a/src/soc/intel/skylake/fspcar.c +++ b/src/soc/intel/skylake/fspcar.c @@ -23,10 +23,19 @@ .Reserved = {0}, }, .FsptCoreUpd = { - .MicrocodeRegionBase = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LOC, - .MicrocodeRegionSize = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LEN, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), whioch contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-6 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ + .MicrocodeRegionBase = 0, + .MicrocodeRegionLength = 0, .CodeRegionBase = (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE), .CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE,
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 1:
(3 comments)
The FSP-T code is pretty bad/buggy especially in this regard. Did you test this?
https://review.coreboot.org/c/coreboot/+/37187/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37187/1//COMMIT_MSG@17 PS1, Line 17: Gen-6 Gen 4 actually.
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/apollolake/fs... File src/soc/intel/apollolake/fspcar.c:
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/apollolake/fs... PS1, Line 30: whioch which.
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/denverton_ns/... File src/soc/intel/denverton_ns/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/denverton_ns/... PS1, Line 35: CONFIG_CPU_MICROCODE_CBFS_LOC Drop the Kconfig symbols, they are unused now?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 1:
Patch Set 1:
(3 comments)
The FSP-T code is pretty bad/buggy especially in this regard. Did you test this?
Yes Arthur, we are testing across all FSP-T users inside Intel. @Sheng is doing on IOT platform @David is looking at server platform.
will get +1/+2 from them
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
(3 comments)
The FSP-T code is pretty bad/buggy especially in this regard. Did you test this?
Yes Arthur, we are testing across all FSP-T users inside Intel. @Sheng is doing on IOT platform @David is looking at server platform.
will get +1/+2 from them
Nice. Could you also have CB:36682 tested so FSP-T can dropped altogether?
Hello Patrick Rudolph, Vanny E, Lean Sheng Tan, Vincent Zimmer, build bot (Jenkins), David Guckian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37187
to look at the new patch set (#2).
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T
It is a requirement for Firmware to have Firmware Interface Table (FIT), which contains pointers to each microcode update. The microcode update is loaded for all logical processors before reset vector.
FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength are input parameters to TempRamInit API. If these values are 0, FSP will not attempt to update microcode.
Since Gen-4 all IA-SoC has FIT loading ucode even before cpu reset in place hence skipping FSP-T loading ucode after CPU reset options.
Also removed unused kconfig CONFIG_CPU_MICROCODE_CBFS_LOC and CONFIG_CPU_MICROCODE_CBFS_LEN
Change-Id: I3a406fa0e2e62e3363c2960e173dc5f5f5ca0455 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/cpu/Makefile.inc M src/drivers/intel/fsp2_0/Kconfig M src/soc/intel/apollolake/fspcar.c M src/soc/intel/cannonlake/bootblock/bootblock.c M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/denverton_ns/bootblock/bootblock.c M src/soc/intel/skylake/fspcar.c 7 files changed, 50 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/37187/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37187/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37187/1//COMMIT_MSG@17 PS1, Line 17: Gen-6
Gen 4 actually.
Done
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/apollolake/fs... File src/soc/intel/apollolake/fspcar.c:
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/apollolake/fs... PS1, Line 30: whioch
which.
Done
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/denverton_ns/... File src/soc/intel/denverton_ns/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37187/1/src/soc/intel/denverton_ns/... PS1, Line 35: CONFIG_CPU_MICROCODE_CBFS_LOC
Drop the Kconfig symbols, they are unused now?
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
Patch Set 1:
(3 comments)
The FSP-T code is pretty bad/buggy especially in this regard. Did you test this?
Yes Arthur, we are testing across all FSP-T users inside Intel. @Sheng is doing on IOT platform @David is looking at server platform.
will get +1/+2 from them
Nice. Could you also have CB:36682 tested so FSP-T can dropped altogether?
Yes regarding BtG enabling, i have requested Sachin from intel to take a look and guide what needs to be done.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 2: Code-Review+2
David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 2: Code-Review+1
This patch is working on my systems. Thanks Subrata
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T
It is a requirement for Firmware to have Firmware Interface Table (FIT), which contains pointers to each microcode update. The microcode update is loaded for all logical processors before reset vector.
FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength are input parameters to TempRamInit API. If these values are 0, FSP will not attempt to update microcode.
Since Gen-4 all IA-SoC has FIT loading ucode even before cpu reset in place hence skipping FSP-T loading ucode after CPU reset options.
Also removed unused kconfig CONFIG_CPU_MICROCODE_CBFS_LOC and CONFIG_CPU_MICROCODE_CBFS_LEN
Change-Id: I3a406fa0e2e62e3363c2960e173dc5f5f5ca0455 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37187 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: David Guckian Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/Makefile.inc M src/drivers/intel/fsp2_0/Kconfig M src/soc/intel/apollolake/fspcar.c M src/soc/intel/cannonlake/bootblock/bootblock.c M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/denverton_ns/bootblock/bootblock.c M src/soc/intel/skylake/fspcar.c 7 files changed, 50 insertions(+), 40 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved David Guckian: Looks good to me, but someone else must approve
diff --git a/src/cpu/Makefile.inc b/src/cpu/Makefile.inc index b80c30d..4b5d67b 100644 --- a/src/cpu/Makefile.inc +++ b/src/cpu/Makefile.inc @@ -59,9 +59,4 @@
cpu_microcode_blob.bin-file ?= $(obj)/cpu_microcode_blob.bin cpu_microcode_blob.bin-type := microcode - -ifneq ($(CONFIG_CPU_MICROCODE_CBFS_LOC),) -cpu_microcode_blob.bin-COREBOOT-position := $(CONFIG_CPU_MICROCODE_CBFS_LOC) -else cpu_microcode_blob.bin-align := 16 -endif diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 1fd4b0c..77382d3 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -37,21 +37,6 @@ Add the FSP-M and FSP-S binaries to CBFS. Currently coreboot does not use the FSP-T binary and it is not added.
-config CPU_MICROCODE_CBFS_LEN - hex "Microcode update region length in bytes" - depends on FSP_CAR - default 0x0 - help - The length in bytes of the microcode update region. - -config CPU_MICROCODE_CBFS_LOC - hex "Microcode update base address in CBFS" - depends on FSP_CAR - default 0x0 - help - The location (base address) in CBFS that contains the - microcode update binary. - config FSP_T_CBFS string "Name of FSP-T in CBFS" depends on FSP_CAR diff --git a/src/soc/intel/apollolake/fspcar.c b/src/soc/intel/apollolake/fspcar.c index 8b1089f..a284116 100644 --- a/src/soc/intel/apollolake/fspcar.c +++ b/src/soc/intel/apollolake/fspcar.c @@ -25,6 +25,17 @@ .FsptCommonUpd = { .Revision = 0, .Reserved = {0}, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), which contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-4 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ .MicrocodeRegionBase = 0, .MicrocodeRegionLength = 0, .CodeRegionBase = diff --git a/src/soc/intel/cannonlake/bootblock/bootblock.c b/src/soc/intel/cannonlake/bootblock/bootblock.c index 653ba30..9f85397 100644 --- a/src/soc/intel/cannonlake/bootblock/bootblock.c +++ b/src/soc/intel/cannonlake/bootblock/bootblock.c @@ -30,10 +30,19 @@ .Reserved = {0}, }, .FsptCoreUpd = { - .MicrocodeRegionBase = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LOC, - .MicrocodeRegionSize = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LEN, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), which contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-4 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ + .MicrocodeRegionBase = 0, + .MicrocodeRegionLength = 0, .CodeRegionBase = (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE), .CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE, diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig index 0ce0d5b..713aae6 100644 --- a/src/soc/intel/denverton_ns/Kconfig +++ b/src/soc/intel/denverton_ns/Kconfig @@ -108,14 +108,6 @@ hex default 0x10000
-config CPU_MICROCODE_CBFS_LOC - hex - default 0xfff20040 - -config CPU_MICROCODE_CBFS_LEN - hex - default 0x0ff80 - config CPU_BCLK_MHZ int default 100 diff --git a/src/soc/intel/denverton_ns/bootblock/bootblock.c b/src/soc/intel/denverton_ns/bootblock/bootblock.c index f75de1f..47c76b5 100644 --- a/src/soc/intel/denverton_ns/bootblock/bootblock.c +++ b/src/soc/intel/denverton_ns/bootblock/bootblock.c @@ -31,10 +31,19 @@ .Reserved = {0}, }, .FsptCoreUpd = { - .MicrocodeRegionBase = - (UINT32)CONFIG_CPU_MICROCODE_CBFS_LOC, - .MicrocodeRegionLength = - (UINT32)CONFIG_CPU_MICROCODE_CBFS_LEN, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), which contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-4 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ + .MicrocodeRegionBase = 0, + .MicrocodeRegionLength = 0, .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_ROM_SIZE), .CodeRegionLength = (UINT32)CONFIG_ROM_SIZE, diff --git a/src/soc/intel/skylake/fspcar.c b/src/soc/intel/skylake/fspcar.c index a4c3726..0d27f57 100644 --- a/src/soc/intel/skylake/fspcar.c +++ b/src/soc/intel/skylake/fspcar.c @@ -23,10 +23,19 @@ .Reserved = {0}, }, .FsptCoreUpd = { - .MicrocodeRegionBase = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LOC, - .MicrocodeRegionSize = - (uint32_t)CONFIG_CPU_MICROCODE_CBFS_LEN, + /* + * It is a requirement for firmware to have Firmware Interface Table + * (FIT), which contains pointers to each microcode update. + * The microcode update is loaded for all logical processors before + * cpu reset vector. + * + * All SoC since Gen-4 has above mechanism in place to load microcode + * even before hitting CPU reset vector. Hence skipping FSP-T loading + * microcode after CPU reset by passing '0' value to + * FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength. + */ + .MicrocodeRegionBase = 0, + .MicrocodeRegionLength = 0, .CodeRegionBase = (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE), .CodeRegionSize = (uint32_t)CONFIG_ROM_SIZE,
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/c/coreboot/+/37187/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37187/2//COMMIT_MSG@18 PS2, Line 18: skipping skip
https://review.coreboot.org/c/coreboot/+/37187/2//COMMIT_MSG@20 PS2, Line 20: removed remove
https://review.coreboot.org/c/coreboot/+/37187/2//COMMIT_MSG@9 PS2, Line 9: It is a requirement for Firmware to have Firmware Interface Table (FIT), : which contains pointers to each microcode update. : The microcode update is loaded for all logical processors before reset vector. : : FSPT_UPD.MicrocodeRegionBase and FSPT_UPD.MicrocodeRegionLength are : input parameters to TempRamInit API. : If these values are 0, FSP will not attempt to update microcode. : : Since Gen-4 all IA-SoC has FIT loading ucode even before cpu reset in place : hence skipping FSP-T loading ucode after CPU reset options. : : Also removed unused kconfig CONFIG_CPU_MICROCODE_CBFS_LOC and : CONFIG_CPU_MICROCODE_CBFS_LEN Please reformat for the 75 characters text width.
https://review.coreboot.org/c/coreboot/+/37187/2//COMMIT_MSG@22 PS2, Line 22: Tested how? Is any time saved?
https://review.coreboot.org/c/coreboot/+/37187/2/src/soc/intel/apollolake/fs... File src/soc/intel/apollolake/fspcar.c:
https://review.coreboot.org/c/coreboot/+/37187/2/src/soc/intel/apollolake/fs... PS2, Line 32: cpu CPU
https://review.coreboot.org/c/coreboot/+/37187/2/src/soc/intel/apollolake/fs... PS2, Line 34: * All SoC since Gen-4 has above mechanism in place to load microcode microcode updates
https://review.coreboot.org/c/coreboot/+/37187/2/src/soc/intel/apollolake/fs... PS2, Line 35: skipping skip
https://review.coreboot.org/c/coreboot/+/37187/2/src/soc/intel/apollolake/fs... PS2, Line 36: * microcode after CPU reset by passing '0' value to microcode updates
https://review.coreboot.org/c/coreboot/+/37187/2/src/soc/intel/apollolake/fs... PS2, Line 40: .MicrocodeRegionLength = 0, So it was already disabled for APL? Mention that in the commit message, and maybe reference the commit doing that?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37187/3/src/soc/intel/cannonlake/bo... File src/soc/intel/cannonlake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37187/3/src/soc/intel/cannonlake/bo... PS3, Line 45: Length Wanted to give this a shot, because last time the CFL FSP died with 0 values. Heh, doesn't even compile.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37187/3/src/soc/intel/cannonlake/bo... File src/soc/intel/cannonlake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37187/3/src/soc/intel/cannonlake/bo... PS3, Line 45: Length
Wanted to give this a shot, because last time the CFL FSP died with 0 values. […]
For what it's worth, it still doesn't work. Neither with this line fixed nor without this patch and also no matter if MICROCODE_CBFS_LOC/SIZE are set or not (with the currently linked 3rdparty/fsp/CoffeeLakeFspBinPkg).
I've a strange feeling, the people testing at Intel use downstream patches or different FSP builds. Or maybe board ports need to be adapted for this?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37187/3/src/soc/intel/cannonlake/bo... File src/soc/intel/cannonlake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37187/3/src/soc/intel/cannonlake/bo... PS3, Line 45: Length
For what it's worth, it still doesn't work. Neither with this line fixed nor […]
@Sheng/Praveen, can you please help Nico to unblock FSP-T usage. as per David and my test, i don't see any issue on our platform.
PraveenX Hodagatta Pranesh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
compilation error is due to coffeelake FsptUpd.h has MicrocodeRegionSize instead of MicrocodeRegionLength. fixed here https://review.coreboot.org/c/coreboot/+/37265
PraveenX Hodagatta Pranesh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
Tested on Coffeelake RVP11 with FSP_CAR selected, can boot successfully.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Tested on Coffeelake RVP11 with FSP_CAR selected, can boot successfully.
Thanks. Can you share your defconfig and tell us which commit and what FSP binary you used?
PraveenX Hodagatta Pranesh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Patch Set 3:
Tested on Coffeelake RVP11 with FSP_CAR selected, can boot successfully.
Thanks. Can you share your defconfig and tell us which commit and what FSP binary you used?
i have tested on latest commit "a4f5954159531cfbe22a0b54acab331231440915" and FSP binaries are from "3rdparty/fsp/CoffeeLakeFspBinPkg/". please suggest How to share defconfig ?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Tested on Coffeelake RVP11 with FSP_CAR selected, can boot successfully.
Thanks. Can you share your defconfig and tell us which commit and what FSP binary you used?
i have tested on latest commit "a4f5954159531cfbe22a0b54acab331231440915" and FSP binaries are from "3rdparty/fsp/CoffeeLakeFspBinPkg/".
You mean a4f59541 with your patch on top?
please suggest How to share defconfig ?
`make savedefconfig` generates a file called `defconfig` in the source root. Usually it's not long, so you can paste it here, or put it on dpaste.com or alike.
PraveenX Hodagatta Pranesh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Patch Set 3:
Tested on Coffeelake RVP11 with FSP_CAR selected, can boot successfully.
Thanks. Can you share your defconfig and tell us which commit and what FSP binary you used?
i have tested on latest commit "a4f5954159531cfbe22a0b54acab331231440915" and FSP binaries are from "3rdparty/fsp/CoffeeLakeFspBinPkg/".
You mean a4f59541 with your patch on top?
please suggest How to share defconfig ?
`make savedefconfig` generates a file called `defconfig` in the source root. Usually it's not long, so you can paste it here, or put it on dpaste.com or alike.
yes, my patch on top of a4f59541.
defconfig: http://dpaste.com/2093NBB
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37187 )
Change subject: soc/intel/{apl,cnl,dnv,skl}: Skip ucode loading by FSP-T ......................................................................
Patch Set 3:
Tested on Coffeelake RVP11 with FSP_CAR selected, can boot successfully.
Thanks. Can you share your defconfig and tell us which commit and what FSP binary you used?
i have tested on latest commit "a4f5954159531cfbe22a0b54acab331231440915" and FSP binaries are from "3rdparty/fsp/CoffeeLakeFspBinPkg/".
You mean a4f59541 with your patch on top?
please suggest How to share defconfig ?
`make savedefconfig` generates a file called `defconfig` in the source root. Usually it's not long, so you can paste it here, or put it on dpaste.com or alike.
yes, my patch on top of a4f59541.
defconfig: http://dpaste.com/2093NBB
Thanks for the details. For what it's worth, I wasn't able to reproduce your success. Somehow it hangs and I have little means to debug things pre-console. We have no business interest in FSP-T, so I'll just leave it be.
-
Arthur Heymans (Code Review) -
David Guckian (Code Review)
-
Lean Sheng Tan (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
PraveenX Hodagatta Pranesh (Code Review)
-
Subrata Banik (Code Review)