Change in coreboot[master]: mb/google/volteer: Disable HybridStorageMode for Volteer EVT and Delbin
Shaunak Saha has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Volteer EVT and Delbin ......................................................................
mb/google/volteer: Disable HybridStorageMode for Volteer EVT and Delbin
HybridStorageMode FSP UPD needs to be set only for optane storage. Enabling HybridStorageMode causes some extra delay in FspSiliconInit due to HECI command and hence is avoided for NVMe and SATA scenerios. This change disables "HybridStorageMode" for volteer EVT and Delbin.
BUG=b:158573805 TEST=Build and boot volteer evt and confirm that FspSiliconInit time is reduced. In Volteer this saves ~100ms.
Signed-off-by: Shaunak Saha shaunak.saha@intel.com Change-Id: I54fc78e3f888d4f2a02ba0ad6b9aef33eb872a9c --- M src/mainboard/google/volteer/variants/delbin/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 2 files changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45643/1
diff --git a/src/mainboard/google/volteer/variants/delbin/overridetree.cb b/src/mainboard/google/volteer/variants/delbin/overridetree.cb index 05c8a34..7624d40 100644 --- a/src/mainboard/google/volteer/variants/delbin/overridetree.cb +++ b/src/mainboard/google/volteer/variants/delbin/overridetree.cb @@ -2,6 +2,8 @@ register "DdiPort1Hpd" = "0" register "DdiPort2Hpd" = "0"
+ register "HybridStorageMode" = "0" + device domain 0 on device pci 15.0 on chip drivers/i2c/generic diff --git a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb index 0bb82f1..b6d5c8a 100644 --- a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb @@ -5,6 +5,8 @@ register "DdiPort1Hpd" = "0" register "DdiPort2Hpd" = "0"
+ register "HybridStorageMode" = "0" + device domain 0 on device pci 05.0 on end # IPU 0x9A19 device pci 15.0 on
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Volteer EVT and Delbin ......................................................................
Patch Set 1:
Sorry maybe I am misremembering, but doesn't one of the volteer2 SKUs use hybrid storage?
Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Volteer EVT and Delbin ......................................................................
Patch Set 1:
Patch Set 1:
Sorry maybe I am misremembering, but doesn't one of the volteer2 SKUs use hybrid storage?
You are remembering it well Tim. Its my mistake, apologies. I will keep this patch only for Delbin then. Will deal with volteer proto2 and EVT in a separate CL.
Hello build bot (Jenkins), Caveh Jalali, Ravishankar Sarawadi, Tim Wawrzynczak, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45643
to look at the new patch set (#2).
Change subject: mb/google/volteer: Disable HybridStorageMode for Delbin ......................................................................
mb/google/volteer: Disable HybridStorageMode for Delbin
HybridStorageMode FSP UPD needs to be set only for optane storage. Enabling HybridStorageMode causes some extra delay in FspSiliconInit due to HECI command and hence is avoided for NVMe and SATA scenerios. This change disables "HybridStorageMode" for Delbin.
BUG=b:158573805 TEST=Build and boot delbin and confirm that FspSiliconInit time is reduced. This saves ~100ms.
Signed-off-by: Shaunak Saha shaunak.saha@intel.com Change-Id: I54fc78e3f888d4f2a02ba0ad6b9aef33eb872a9c --- M src/mainboard/google/volteer/variants/delbin/overridetree.cb 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45643/2
Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Delbin ......................................................................
Patch Set 2: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Delbin ......................................................................
Patch Set 2:
How about we change the baseboard (which is the default) to be 0, and only enable the option in variants that require it?
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Delbin ......................................................................
Patch Set 2:
Patch Set 2:
How about we change the baseboard (which is the default) to be 0, and only enable the option in variants that require it?
+1 to Tim's suggestion to default to 0.
Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Delbin ......................................................................
Patch Set 2:
Patch Set 2:
How about we change the baseboard (which is the default) to be 0, and only enable the option in variants that require it?
Agreed. I had this patch before "https://review.coreboot.org/c/coreboot/+/44261". But then we decided to work on dynamically detecting optane just like FSP. On checking FSP code they are doing it through HECI messages. Do you suggest i fix that old patch by adding dynamic detection or its not worth adding that code for volteer?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for Delbin ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
How about we change the baseboard (which is the default) to be 0, and only enable the option in variants that require it?
Agreed. I had this patch before "https://review.coreboot.org/c/coreboot/+/44261". But then we decided to work on dynamically detecting optane just like FSP. On checking FSP code they are doing it through HECI messages. Do you suggest i fix that old patch by adding dynamic detection or its not worth adding that code for volteer?
I think for now we should do the following: 1) Set HybridStorageMode to 0 in the baseboard 2) Enable it in the devicetrees for boards that will potentially use it 3) If any shipping devices have SKUs that use optane and SKUs that don't, then we may want to consider dynamic detection SG?
Hello build bot (Jenkins), Caveh Jalali, Ravishankar Sarawadi, Tim Wawrzynczak, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45643
to look at the new patch set (#3).
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
mb/google/volteer: Disable HybridStorageMode for volteer baseboard
HybridStorageMode FSP UPD needs to be set only for optane storage. Enabling HybridStorageMode causes some extra delay in FspSiliconInit due to HECI command and hence is avoided for NVMe and SATA scenerios. This change disables "HybridStorageMode" for volteer baseboard. For boards using optane HybridStorage needs to be enabled from overwrite devicetree.
BUG=b:158573805 TEST=Build and boot non optane device and confirm that FspSiliconInit time is reduced. This saves ~100ms.
Signed-off-by: Shaunak Saha shaunak.saha@intel.com Change-Id: I54fc78e3f888d4f2a02ba0ad6b9aef33eb872a9c --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45643/3
Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
Patch Set 2:
How about we change the baseboard (which is the default) to be 0, and only enable the option in variants that require it?
Agreed. I had this patch before "https://review.coreboot.org/c/coreboot/+/44261". But then we decided to work on dynamically detecting optane just like FSP. On checking FSP code they are doing it through HECI messages. Do you suggest i fix that old patch by adding dynamic detection or its not worth adding that code for volteer?
I think for now we should do the following:
- Set HybridStorageMode to 0 in the baseboard
- Enable it in the devicetrees for boards that will potentially use it
- If any shipping devices have SKUs that use optane and SKUs that don't, then we may want to consider dynamic detection
SG?
Agreed. In this patch i am setting HybridStorageMode to 0 in the baseboard. Will introduce the dynamic detection in a separate patch. Hope that is ok.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 3:
In order not to break any devices with hybrid storage, I think you'll need to add `register "HybridStorageMode" = "1"` to volteer & volteer2 overridetree.cb files
Hello build bot (Jenkins), Caveh Jalali, Ravishankar Sarawadi, Tim Wawrzynczak, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45643
to look at the new patch set (#4).
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
mb/google/volteer: Disable HybridStorageMode for volteer baseboard
HybridStorageMode FSP UPD needs to be set only for optane storage. Enabling HybridStorageMode causes some extra delay in FspSiliconInit due to HECI command and hence is avoided for NVMe and SATA scenerios. This change disables "HybridStorageMode" for volteer baseboard. For boards using optane HybridStorage needs to be enabled from overwrite devicetree. We are enabling HybridStorage for volteer and volteer2 as those plaforms have SKU's with optane storage.
BUG=b:158573805 TEST=Build and boot non optane device and confirm that FspSiliconInit time is reduced. This saves ~100ms.
Signed-off-by: Shaunak Saha shaunak.saha@intel.com Change-Id: I54fc78e3f888d4f2a02ba0ad6b9aef33eb872a9c --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 3 files changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45643/4
Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
Patch Set 3:
In order not to break any devices with hybrid storage, I think you'll need to add `register "HybridStorageMode" = "1"` to volteer & volteer2 overridetree.cb files
Done.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4: Code-Review+2
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
Patch Set 3:
In order not to break any devices with hybrid storage, I think you'll need to add `register "HybridStorageMode" = "1"` to volteer & volteer2 overridetree.cb files
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1" Won't this still break volteer SKUs that don't have octane?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1"
Won't this still break volteer SKUs that don't have octane?
No, this enables detection of hybrid storage modules in FSP (which is time consuming), and currently only volteer/volteer2 have SKUs with optane
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1"
No, this enables detection of hybrid storage modules in FSP (which is time consuming), and currently […]
I thought the extra time consumption was breaking FAFT?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1"
I thought the extra time consumption was breaking FAFT?
It causes our boot time to increase ~150ms IIRC. However, if you don't have this, you won't be able to boot from optane storage (which some volteer/volteer2 SKUs do have).
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1"
It causes our boot time to increase ~150ms IIRC. […]
Is there a way to address this with a probe routine that checks SKU and then sets HybridStorageMode accordingly?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1"
Is there a way to address this with a probe routine that checks SKU and then sets HybridStorageMode […]
We have some things planned for if/when an OEM decides to go with the hybrid storage option, none have so far so it's lower priority; see the bug for some more details
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45643/4/src/mainboard/google/voltee... PS4, Line 50: register "HybridStorageMode" = "1"
We have some things planned for if/when an OEM decides to go with the hybrid storage option, none ha […]
Ack
Nick Vaccaro has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
mb/google/volteer: Disable HybridStorageMode for volteer baseboard
HybridStorageMode FSP UPD needs to be set only for optane storage. Enabling HybridStorageMode causes some extra delay in FspSiliconInit due to HECI command and hence is avoided for NVMe and SATA scenerios. This change disables "HybridStorageMode" for volteer baseboard. For boards using optane HybridStorage needs to be enabled from overwrite devicetree. We are enabling HybridStorage for volteer and volteer2 as those plaforms have SKU's with optane storage.
BUG=b:158573805 TEST=Build and boot non optane device and confirm that FspSiliconInit time is reduced. This saves ~100ms.
Signed-off-by: Shaunak Saha shaunak.saha@intel.com Change-Id: I54fc78e3f888d4f2a02ba0ad6b9aef33eb872a9c Reviewed-on: https://review.coreboot.org/c/coreboot/+/45643 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb M src/mainboard/google/volteer/variants/volteer/overridetree.cb M src/mainboard/google/volteer/variants/volteer2/overridetree.cb 3 files changed, 5 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index 330accc..28d903f 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -87,7 +87,7 @@ # Enable Optane PCIE 11 using clk 0 register "PcieRpEnable[10]" = "1" register "PcieRpLtrEnable[10]" = "1" - register "HybridStorageMode" = "1" + register "HybridStorageMode" = "0"
# Enable SD Card PCIE 8 using clk 3 register "PcieRpEnable[7]" = "1" diff --git a/src/mainboard/google/volteer/variants/volteer/overridetree.cb b/src/mainboard/google/volteer/variants/volteer/overridetree.cb index 5079493..2c120d5 100644 --- a/src/mainboard/google/volteer/variants/volteer/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer/overridetree.cb @@ -47,6 +47,8 @@ register "IomTypeCPortPadCfg[0]" = "0x090E000A" register "IomTypeCPortPadCfg[1]" = "0x090E000D"
+ register "HybridStorageMode" = "1" + device domain 0 on device ref ipu on end device ref i2c0 on diff --git a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb index f8dcd58..d49a52a 100644 --- a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb @@ -5,6 +5,8 @@ register "DdiPort1Hpd" = "0" register "DdiPort2Hpd" = "0"
+ register "HybridStorageMode" = "1" + device domain 0 on device ref dptf on chip drivers/intel/dptf
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45643 )
Change subject: mb/google/volteer: Disable HybridStorageMode for volteer baseboard ......................................................................
Patch Set 5: Code-Review+2
-
Nick Vaccaro (Code Review) -
Shaunak Saha (Code Review)
-
Tim Wawrzynczak (Code Review)