Jamie Ryu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
soc/intel/tgl: Disable hybrid storage mode in recovery mode
This is WA to initialize NVME with CSE Lite in recovery mode. CSME Lite does not support hybrid storage dynamic configuration in RO. Hence, hybrid storage mode is disabled in recovery mode for CSME Lite until the functionality is supported by CSE Lite RO.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/1
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c old mode 100644 new mode 100755 index 926d8eb..e931a22 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -10,6 +10,7 @@ #include <intelblocks/lpss.h> #include <intelblocks/xdci.h> #include <intelpch/lockdown.h> +#include <security/vboot/vboot_common.h> #include <soc/gpio_soc_defs.h> #include <soc/intel/common/vbt.h> #include <soc/pci_devs.h> @@ -251,7 +252,17 @@ params->Enable8254ClockGatingOnS3 = !CONFIG_USE_LEGACY_8254_TIMER;
/* Enable Hybrid storage auto detection */ - params->HybridStorageMode = config->HybridStorageMode; + if (CONFIG(SOC_INTEL_CSE_LITE_SKU) && vboot_recovery_mode_enabled()) { + /* + * Since CSME Lite SKU does not support hybrid storage dynamic + * configuration in recovery boot mode, dynamic configuration is + * disabled as a temporary WA until the fix is available. + */ + printk(BIOS_DEBUG, "cse_lite: recovery mode enabled\n"); + params->HybridStorageMode = 0; + } else { + params->HybridStorageMode = config->HybridStorageMode; + }
/* USB4/TBT */ for (i = 0; i < ARRAY_SIZE(params->ITbtPcieRootPortEn); i++) {
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#2).
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
soc/intel/tgl: Disable hybrid storage mode in recovery mode
This is WA to initialize NVME with CSE Lite in recovery mode. CSME Lite does not support hybrid storage dynamic configuration in RO. Hence, hybrid storage mode is disabled in recovery mode for CSME Lite until the functionality is supported by CSE Lite RO.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
Patch Set 2: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@9 PS2, Line 9: CSE I see CSE and CSME being used interchangeably. Are they the same? Or does CSE refer to the hardware and CSME to the firmware (software) SKU?
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@9 PS2, Line 9: WA This is Washington! 😄
Jokes aside, I've usually seen "workaround" abbreviated as `W/A`, so I'd use that
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@16 PS2, Line 16: Cq-Depend: chrome-internal:3100721 This tag doesn't mean anything upstream (unlike Chromium Gerrit, we don't have a CQ bot)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 259: the fix about the fix: is it something that needs to be changed in the CSME Lite SKU firmware?
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 259: WA W/A
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 255: vboot_recovery_mode_enabled() The dynamic hybrid storage configuration involves updating straps. Since the "CSE" RO code does not support updating the straps, the check should be to see if CSE has booted from RO partition instead of checking Chrome/VBOOT recovery mode.
if (CONFIG(SOC_INTEL_CSE_LITE_SKU) && cse_is_rw())
cse_is_rw() could be implemented in src/soc/intel/common/block/cse/cse_lite.c
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 255: vboot_recovery_mode_enabled()
The dynamic hybrid storage configuration involves updating straps. […]
I will update this as you suggested.
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@9 PS2, Line 9: CSE
I see CSE and CSME being used interchangeably. […]
oh I thought I changed all to CSME but missed this. Thanks for pointing this out.
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@9 PS2, Line 9: WA
This is Washington! 😄 […]
Thanks for information! I will update this accordingly.
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@16 PS2, Line 16: Cq-Depend: chrome-internal:3100721
This tag doesn't mean anything upstream (unlike Chromium Gerrit, we don't have a CQ bot)
do I need to remove this? I wanted to mention this has a dependency on chrome-internal:3100721, and is there any other way to do it?
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 259: the fix
about the fix: is it something that needs to be changed in the CSME Lite SKU firmware?
the final fix is still in discussion, so it is not decided yet.
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 259: WA
W/A
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in recovery mode ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@16 PS2, Line 16: Cq-Depend: chrome-internal:3100721
do I need to remove this? I wanted to mention this has a dependency on chrome-internal:3100721, and […]
Well, I don't think chrome-internal stuff is publicly visible, so I don't know what it is about (I have no access). I imagine it has to do with the CSME Lite firmware.
In any case, I would leave it as-is. It can be helpful for those with access to chrome-internal
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Angel Pons, Sridhar Siricilla, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#3).
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE RO boot ......................................................................
soc/intel/tgl: Disable hybrid storage mode in CSE RO boot
This is a W/A to initialize NVME with CSE Lite in CSE RO boot. CSE Lite does not support hybrid storage dynamic configuration in CSE RO; hence, hybrid storage mode is disabled in case of CSE RO boot until the functionality is supported.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 12 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/3
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Angel Pons, Sridhar Siricilla, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#4).
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE RO boot ......................................................................
soc/intel/tgl: Disable hybrid storage mode in CSE RO boot
This is a W/A to initialize NVME with CSE Lite in CSE RO boot. CSE Lite does not support hybrid storage dynamic configuration in CSE RO; hence, hybrid storage mode is disabled in case of CSE RO boot in recovery mode until the functionality is supported.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/4
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE RO boot ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/4/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/4/src/soc/intel/tigerlake/fsp... PS4, Line 265: params->HybridStorageMode = 0; Doesn't this mean that optane won't work in recovery mode?
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE RO boot ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/4/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/4/src/soc/intel/tigerlake/fsp... PS4, Line 265: params->HybridStorageMode = 0;
Doesn't this mean that optane won't work in recovery mode?
Nick, Both NVMe and Optane work with this W/A, but with Optane, only one NVME will be configured as x2. I verified this works with Optane and was able to install OS to Optane in recovery mode and then boot from Optane in normal mode.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE RO boot ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/4/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/4/src/soc/intel/tigerlake/fsp... PS4, Line 265: params->HybridStorageMode = 0;
Nick, Both NVMe and Optane work with this W/A, but with Optane, only one NVME will be configured as […]
Thanks, Jamie.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE RO boot ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 255: vboot_recovery_mode_enabled()
I will update this as you suggested.
Does this still need changed or is the recovery mode check ok?
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Angel Pons, Sridhar Siricilla, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#5).
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tgl: Disable hybrid storage mode in CSE Lite RO boot
This is a W/A to initialize NVME with CSE Lite in CSE RO boot. CSE Lite does not support hybrid storage dynamic configuration in CSE RO; hence, hybrid storage mode is disabled in case of CSE RO boot in recovery mode until the functionality is supported.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/5
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 255: vboot_recovery_mode_enabled()
Does this still need changed or is the recovery mode check ok?
Patch#5 is the updated by the latest internal discussion and considering current CSE Lite and FSP implementaion. Rizwan, please add your comments if you have any. Thanks.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tgl: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 5: Code-Review+1
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
This is a W/A to initialize NVME with CSE Lite in CSE RO boot. CSE Lite does not support hybrid storage dynamic configuration in CSE RO; hence, hybrid storage mode is disabled in case of CSE RO boot in recovery mode until the functionality is supported.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/8
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#11).
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
This is a W/A to initialize NVME with CSE Lite in CSE RO boot. CSE Lite does not support hybrid storage dynamic configuration in CSE RO; hence, hybrid storage mode is disabled in case of CSE RO boot in recovery mode until the functionality is supported.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 14 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/11
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 12: Code-Review+1
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 12: Code-Review+2
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#13).
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
This is to initialize NVME with CSE Lite in CSE RO boot. CSE Lite does not support hybrid storage dynamic configuration in CSE RO; hence, hybrid storage mode is disabled in case of CSE RO boot in recovery mode.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 13 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/13
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#14).
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
A UPD HybridStorageMode allows a platform to dynamically configure the PCIe strap configuration required if a Optane device is connected. The strap configuration is done by HECI commands between FSP and CSE to override the default PCIe strap value, and the updated strap value is stored in SPI RW data to be used on the next boot. CSE Lite supports the strap override when running on CSE RW partition, while CSE RO partition does not support it because CSE RO is not allowed to access SPI RW data. The strap override failure on CSE RO causes FSP not initializing PCH Clkreq and PCIe port mapping and this results NVMe and Optane initialization failure.
By disabling HybridStorageMode in case of CSE RO boot, NVMe detection is done by the default PCIe configuration and Optane is detected as a single NVMe storage device on CSE RO boot in recovery mode. Both NVMe and Optane devices detection as well as OS installation to these storage devices are verified on CSE RO boot in recovery mode.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 13 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/14
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 14: Code-Review+2
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 14: Code-Review+1
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 14: Code-Review+1
Can you address all unresolved comments?
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 14: -Code-Review
(2 comments)
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 260: (CONFIG(SOC_INTEL_CSE_LITE_SKU) && cse_is_hfs3_fw_sku_lite() it looks it's same check, why do we need to check both case?
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 261: !cse_is_hfs1_com_normal() Could you explain why this checking is added?
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 260: (CONFIG(SOC_INTEL_CSE_LITE_SKU) && cse_is_hfs3_fw_sku_lite()
it looks it's same check, why do we need to check both case?
There is a chance CONFIG_SOC_INTEL_CSE_LITE_SKU is enabled in coreboot while CSE Consumer SKU is integrated in ME blob. Current implementation checks CSE SKU information before runs any CSE Lite specific code flow, so that CSE Lite specific code does not affect running CSE Consumer SKU. This code is added by the same assumption and to be fail-safe.
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 261: !cse_is_hfs1_com_normal()
Could you explain why this checking is added?
Current FSP implementation checks CSE state to see which HECI commands are supported in current CSE state, and the HECI commands to override PCIe straps to support hybrid storage dynamic configuration are only supported in CSE Normal state; hence, we are checking CSE Normal state to make sure CSE is in state that cannot support the strap override. Please let me know if more description is needed.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 14: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 261: !cse_is_hfs1_com_normal()
Current FSP implementation checks CSE state to see which HECI commands are supported in current CSE […]
Could you add this info in code as comments?
Hello V Sowmya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Rizwan Qureshi, Sridhar Siricilla, Angel Pons, Nick Vaccaro, Raj Astekar, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#15).
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
A UPD HybridStorageMode allows a platform to dynamically configure the PCIe strap configuration required if a Optane device is connected. The strap configuration is done by HECI commands between FSP and CSE to override the default PCIe strap value, and the updated strap value is stored in SPI RW data to be used on the next boot. CSE Lite supports the strap override when running on CSE RW partition, while CSE RO partition does not support it because CSE RO is not allowed to access SPI RW data. The strap override failure on CSE RO causes FSP not initializing PCH Clkreq and PCIe port mapping and this results NVMe and Optane initialization failure.
By disabling HybridStorageMode in case of CSE RO boot, NVMe detection is done by the default PCIe configuration and Optane is detected as a single NVMe storage device on CSE RO boot in recovery mode. Both NVMe and Optane devices detection as well as OS installation to these storage devices are verified on CSE RO boot in recovery mode.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/15
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 261: !cse_is_hfs1_com_normal()
Could you add this info in code as comments?
Thanks, I added more comments to patch#15.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 260: (CONFIG(SOC_INTEL_CSE_LITE_SKU) && cse_is_hfs3_fw_sku_lite()
There is a chance CONFIG_SOC_INTEL_CSE_LITE_SKU is enabled in coreboot while CSE Consumer SKU is int […]
Done
https://review.coreboot.org/c/coreboot/+/42282/14/src/soc/intel/tigerlake/fs... PS14, Line 261: !cse_is_hfs1_com_normal()
Thanks, I added more comments to patch#15.
Done
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15: Code-Review+2
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@9 PS2, Line 9: CSE
oh I thought I changed all to CSME but missed this. Thanks for pointing this out.
Done
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@9 PS2, Line 9: WA
Thanks for information! I will update this accordingly.
Done
https://review.coreboot.org/c/coreboot/+/42282/2//COMMIT_MSG@16 PS2, Line 16: Cq-Depend: chrome-internal:3100721
Well, I don't think chrome-internal stuff is publicly visible, so I don't know what it is about (I h […]
I will leave chrome-internal id here to keep the record. Thanks.
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 255: vboot_recovery_mode_enabled()
Patch#5 is the updated by the latest internal discussion and considering current CSE Lite and FSP im […]
Done
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 259: the fix
the final fix is still in discussion, so it is not decided yet.
After having a discussion, it is decided that this will be the final fix.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 255: vboot_recovery_mode_enabled()
Patch#5 is the updated by the latest internal discussion and considering current CSE Lite and FSP im […]
Done
https://review.coreboot.org/c/coreboot/+/42282/2/src/soc/intel/tigerlake/fsp... PS2, Line 259: the fix
the final fix is still in discussion, so it is not decided yet.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG@10 PS15, Line 10: a Optane an
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG@10 PS15, Line 10: strap configuration Please use just one space.
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG@14 PS15, Line 14: CSE Lite supports the strap override when running on CSE RW partition, Please add a blank line above to separate paragraphs visibly.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42282/15/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/15/src/soc/intel/tigerlake/fs... PS15, Line 268: printk(BIOS_DEBUG, "cse_lite: CSE RO boot in recovery mode\n"); Make it level INFO, and mention that hybrid storage mode is disabled?
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Rizwan Qureshi, Duncan Laurie, Sridhar Siricilla, Angel Pons, Patrick Rudolph, V Sowmya, Jamie Ryu, Nick Vaccaro, Raj Astekar,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42282
to look at the new patch set (#16).
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
A UPD HybridStorageMode allows a platform to dynamically configure the PCIe strap configuration required if an Optane device is connected. The strap configuration is done by HECI commands between FSP and CSE to override the default PCIe strap value, and the updated strap value is stored in SPI RW data to be used on the next boot.
CSE Lite supports the strap override when running on CSE RW partition, while CSE RO partition does not support it because CSE RO is not allowed to access SPI RW data. The strap override failure on CSE RO causes FSP not initializing PCH Clkreq and PCIe port mapping and this results NVMe and Optane initialization failure.
By disabling HybridStorageMode in case of CSE RO boot, NVMe detection is done by the default PCIe configuration and Optane is detected as a single NVMe storage device on CSE RO boot in recovery mode. Both NVMe and Optane devices detection as well as OS installation to these storage devices are verified on CSE RO boot in recovery mode.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/42282/16
Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 16: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG@10 PS15, Line 10: a Optane
an
Done
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG@10 PS15, Line 10: strap configuration
Please use just one space.
Done
https://review.coreboot.org/c/coreboot/+/42282/15//COMMIT_MSG@14 PS15, Line 14: CSE Lite supports the strap override when running on CSE RW partition,
Please add a blank line above to separate paragraphs visibly.
Done
https://review.coreboot.org/c/coreboot/+/42282/15/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42282/15/src/soc/intel/tigerlake/fs... PS15, Line 268: printk(BIOS_DEBUG, "cse_lite: CSE RO boot in recovery mode\n");
Make it level INFO, and mention that hybrid storage mode is disabled?
Done. Thanks for comments!
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
Patch Set 16: Code-Review+2
Nick Vaccaro has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42282 )
Change subject: soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot ......................................................................
soc/intel/tigerlake: Disable hybrid storage mode in CSE Lite RO boot
A UPD HybridStorageMode allows a platform to dynamically configure the PCIe strap configuration required if an Optane device is connected. The strap configuration is done by HECI commands between FSP and CSE to override the default PCIe strap value, and the updated strap value is stored in SPI RW data to be used on the next boot.
CSE Lite supports the strap override when running on CSE RW partition, while CSE RO partition does not support it because CSE RO is not allowed to access SPI RW data. The strap override failure on CSE RO causes FSP not initializing PCH Clkreq and PCIe port mapping and this results NVMe and Optane initialization failure.
By disabling HybridStorageMode in case of CSE RO boot, NVMe detection is done by the default PCIe configuration and Optane is detected as a single NVMe storage device on CSE RO boot in recovery mode. Both NVMe and Optane devices detection as well as OS installation to these storage devices are verified on CSE RO boot in recovery mode.
BUG=b:158643194 TEST=boot and verified with tglrvp and volteer in recovery mode Cq-Depend: chrome-internal:3100721
Signed-off-by: Jamie Ryu jamie.m.ryu@intel.com Change-Id: I5397cfc007069debe3701bf1e38e81bd17a29f0c Reviewed-on: https://review.coreboot.org/c/coreboot/+/42282 Reviewed-by: Nick Vaccaro nvaccaro@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/tigerlake/fsp_params.c 1 file changed, 15 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nick Vaccaro: Looks good to me, approved Jamie Ryu: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c index a612427..3187a33 100644 --- a/src/soc/intel/tigerlake/fsp_params.c +++ b/src/soc/intel/tigerlake/fsp_params.c @@ -7,9 +7,11 @@ #include <fsp/api.h> #include <fsp/ppi/mp_service_ppi.h> #include <fsp/util.h> +#include <intelblocks/cse.h> #include <intelblocks/lpss.h> #include <intelblocks/xdci.h> #include <intelpch/lockdown.h> +#include <security/vboot/vboot_common.h> #include <soc/gpio_soc_defs.h> #include <soc/intel/common/vbt.h> #include <soc/pci_devs.h> @@ -255,7 +257,19 @@ params->Enable8254ClockGatingOnS3 = !CONFIG_USE_LEGACY_8254_TIMER;
/* Enable Hybrid storage auto detection */ - params->HybridStorageMode = config->HybridStorageMode; + if (CONFIG(SOC_INTEL_CSE_LITE_SKU) && cse_is_hfs3_fw_sku_lite() + && vboot_recovery_mode_enabled() && !cse_is_hfs1_com_normal()) { + /* + * CSE Lite SKU does not support hybrid storage dynamic configuration + * in CSE RO boot, and FSP does not allow to send the strap override + * HECI commands if CSE is not in normal mode; hence, hybrid storage + * mode is disabled on CSE RO boot in recovery boot mode. + */ + printk(BIOS_INFO, "cse_lite: CSE RO boot. HybridStorageMode disabled\n"); + params->HybridStorageMode = 0; + } else { + params->HybridStorageMode = config->HybridStorageMode; + }
/* USB4/TBT */ for (i = 0; i < ARRAY_SIZE(params->ITbtPcieRootPortEn); i++) {
-
Angel Pons (Code Review) -
Duncan Laurie (Code Review)
-
Jamie Ryu (Code Review)
-
Nick Vaccaro (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
Wonkyu Kim (Code Review)