Jonathan Zhang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option
FSPM MemRefreshWatermark option support is present in FB's CPX-SP FSP binary, but not in Intel's CPX-SP FSP binary.
Add a Kconfig HAVE_FSPM_DIMM_WATERMARK_OPTION to indicate whether the FSP binary supports such option.
When such option is set, if corresponding VPD variable is set, use it to control the behavior.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I7592dfc8836f457d64f1976cba3b59f8251c7abe --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/romstage.c M src/mainboard/ocp/deltalake/vpd.h M src/soc/intel/xeon_sp/cpx/Kconfig M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h 5 files changed, 88 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/46480/1
diff --git a/src/mainboard/ocp/deltalake/Kconfig b/src/mainboard/ocp/deltalake/Kconfig index ff5599c..0777ad3 100644 --- a/src/mainboard/ocp/deltalake/Kconfig +++ b/src/mainboard/ocp/deltalake/Kconfig @@ -4,6 +4,7 @@ def_bool y select BOARD_ROMSIZE_KB_65536 select HAVE_ACPI_TABLES + select HAVE_FSPM_DIMM_WATERMARK_OPTION select MAINBOARD_USES_FSP2_0 select SOC_INTEL_COOPERLAKE_SP select SUPERIO_ASPEED_AST2400 diff --git a/src/mainboard/ocp/deltalake/romstage.c b/src/mainboard/ocp/deltalake/romstage.c index 71a26c8..3cb514b 100644 --- a/src/mainboard/ocp/deltalake/romstage.c +++ b/src/mainboard/ocp/deltalake/romstage.c @@ -59,6 +59,25 @@ "DciEn to %d\n", FSP_DCI, FSP_DCI_DEFAULT); mupd->FspmConfig.PchDciEn = FSP_DCI_DEFAULT; } + + /* Set MemRefreshWaterMark */ + if (CONFIG(HAVE_FSPM_DIMM_WATERMARK_OPTION)) { + if (vpd_gets(FSPM_MEMREFRESHWATERMARK, val_str, VPD_LEN, VPD_RW_THEN_RO)) { + val = (uint8_t)atol(val_str); + if (val > 2) { + printk(BIOS_DEBUG, "Invalid MemRefreshWatermark value from VPD: " + "%d\n", val); + val = FSPM_MEMREFRESHWATERMARK_DEFAULT; + } + printk(BIOS_DEBUG, "Setting MemRefershWatermark %d from VPD\n", val); + mupd->FspmConfig.MemRefreshWaterMark = val; + } else { + printk(BIOS_INFO, "Not able to get VPD %s, default set " + "MemRefreshWatermark to %d\n", FSPM_MEMREFRESHWATERMARK, + FSPM_MEMREFRESHWATERMARK_DEFAULT); + mupd->FspmConfig.MemRefreshWaterMark = FSPM_MEMREFRESHWATERMARK_DEFAULT; + } + } }
/* Update bifurcation settings according to different Configs */ diff --git a/src/mainboard/ocp/deltalake/vpd.h b/src/mainboard/ocp/deltalake/vpd.h index ae2099d..2db9851 100644 --- a/src/mainboard/ocp/deltalake/vpd.h +++ b/src/mainboard/ocp/deltalake/vpd.h @@ -32,4 +32,8 @@ #define FSP_DCI "fsp_dci_enable" /* 1 or 0: enable or disable DCI */ #define FSP_DCI_DEFAULT 0 /* Default value when the VPD variable is not found */
+/* FSPM MemRefreshWatermark: 0:Auto (default), 1: high, 2: low */ +#define FSPM_MEMREFRESHWATERMARK "fspm_mem_refresh_watermark" +#define FSPM_MEMREFRESHWATERMARK_DEFAULT 1 /* Default value when the VPD variable is not found */ + #endif diff --git a/src/soc/intel/xeon_sp/cpx/Kconfig b/src/soc/intel/xeon_sp/cpx/Kconfig index 975afc9..1d69889 100644 --- a/src/soc/intel/xeon_sp/cpx/Kconfig +++ b/src/soc/intel/xeon_sp/cpx/Kconfig @@ -91,4 +91,7 @@ int default 512
+config HAVE_FSPM_DIMM_WATERMARK_OPTION + def_bool n + endif diff --git a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h index 37ff1bd..89922ef 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h +++ b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h @@ -495,237 +495,248 @@ **/ UINT8 serialDebugMsgLvl;
-/** Offset 0x00C4 - IIO ConfigIOU0 +#if CONFIG_HAVE_FSPM_DIMM_WATERMARK_OPTION +/** Offset 0x00C4 - Usage type for Memory Refresh Watermark + Select Memory Refresh Watermark, 0: Auto (default), 1: High, 2: Low +**/ + UINT8 MemRefreshWaterMark; +#endif + +/** Offset 0x00C5 - IIO ConfigIOU0 ConfigIOU[MAX_SOCKET][0]: MAX_SOCKET=8, 0x00:x4x4x4x4, 0x01:x4x4xxx8, 0x02:xxx8x4x4, 0x03:xxx8xxx8, 0x04:xxxxxx16, 0xFF:AUTO **/ UINT8 IioConfigIOU0[8];
-/** Offset 0x00CC - IIO ConfigIOU1 +/** Offset 0x00CD - IIO ConfigIOU1 ConfigIOU[MAX_SOCKET][1]: MAX_SOCKET=8, 0x00:x4x4x4x4, 0x01:x4x4xxx8, 0x02:xxx8x4x4, 0x03:xxx8xxx8, 0x04:xxxxxx16, 0xFF:AUTO **/ UINT8 IioConfigIOU1[8];
-/** Offset 0x00D4 - IIO ConfigIOU2 +/** Offset 0x00D5 - IIO ConfigIOU2 ConfigIOU[MAX_SOCKET][2]: MAX_SOCKET=8, 0x00:x4x4x4x4, 0x01:x4x4xxx8, 0x02:xxx8x4x4, 0x03:xxx8xxx8, 0x04:xxxxxx16, 0xFF:AUTO **/ UINT8 IioConfigIOU2[8];
-/** Offset 0x00DC - IIO ConfigIOU3 +/** Offset 0x00DD - IIO ConfigIOU3 ConfigIOU[MAX_SOCKET][3]: MAX_SOCKET=8, 0x00:x4x4x4x4, 0x01:x4x4xxx8, 0x02:xxx8x4x4, 0x03:xxx8xxx8, 0x04:xxxxxx16, 0xFF:AUTO **/ UINT8 IioConfigIOU3[8];
-/** Offset 0x00E4 - IIO ConfigIOU4 +/** Offset 0x00E5 - IIO ConfigIOU4 ConfigIOU[MAX_SOCKET][4]: MAX_SOCKET=8, 0x00:x4x4x4x4, 0x01:x4x4xxx8, 0x02:xxx8x4x4, 0x03:xxx8xxx8, 0x04:xxxxxx16, 0xFF:AUTO **/ UINT8 IioConfigIOU4[8];
-/** Offset 0x00EC - Usage type for IIO PCIE Config Table Ptr +/** Offset 0x00ED +**/ + UINT8 UnusedUpdSpace2[3]; + +/** Offset 0x00F0 - Usage type for IIO PCIE Config Table Ptr IIO PCIE Config Table Ptr **/ UINT32 IioPcieConfigTablePtr;
-/** Offset 0x00F0 - Usage type for IIO PCIE Config Table Number +/** Offset 0x00F4 - Usage type for IIO PCIE Config Table Number IIO PCIE Config Table Number **/ UINT32 IioPcieConfigTableNumber;
-/** Offset 0x00F4 - Usage type for IIO PCIE Root Port Enable or Disable +/** Offset 0x00F8 - Usage type for IIO PCIE Root Port Enable or Disable IIO PCH rootport, if port is enabled, the value is 0x01, if the port is disabled, the value is 0x00 **/ UINT8 IIOPcieRootPortEnable;
-/** Offset 0x00F5 - Usage type for IIO DeEmphasis +/** Offset 0x00F9 - Usage type for IIO DeEmphasis IIO DeEmphasis **/ UINT8 DeEmphasis;
-/** Offset 0x00F6 - Usage type for IIO PCIE Root Port link speed +/** Offset 0x00FA - Usage type for IIO PCIE Root Port link speed IIO root port link speed **/ UINT8 IIOPciePortLinkSpeed;
-/** Offset 0x00F7 - Usage type for IIO PCIE Root Port Max Payload +/** Offset 0x00FB - Usage type for IIO PCIE Root Port Max Payload IIO Root Port Max Payload **/ UINT8 IIOPcieMaxPayload;
-/** Offset 0x00F8 - Usage type for IIO DfxDnTxPreset +/** Offset 0x00FC - Usage type for IIO DfxDnTxPreset IIO DfxDnTxPreset **/ UINT8 DfxDnTxPreset;
-/** Offset 0x00F9 - Usage type for IIO DfxRxPreset +/** Offset 0x00FD - Usage type for IIO DfxRxPreset IIO DfxRxPreset **/ UINT8 DfxRxPreset;
-/** Offset 0x00FA - Usage type for IIO DfxUpTxPreset +/** Offset 0x00FE - Usage type for IIO DfxUpTxPreset IIO DfxUpTxPreset **/ UINT8 DfxUpTxPreset;
-/** Offset 0x00FB - Usage type for IIO PcieCommonClock +/** Offset 0x00FF - Usage type for IIO PcieCommonClock IIO PcieCommonClock **/ UINT8 PcieCommonClock;
-/** Offset 0x00FC - Usage type for IIO NtbPpd +/** Offset 0x0100 - Usage type for IIO NtbPpd IIO NtbPpd **/ UINT8 NtbPpd;
-/** Offset 0x00FD - Usage type for IIO NtbBarSizeOverride +/** Offset 0x0101 - Usage type for IIO NtbBarSizeOverride IIO NtbBarSizeOverride **/ UINT8 NtbBarSizeOverride;
-/** Offset 0x00FE - Usage type for IIO NtbSplitBar +/** Offset 0x0102 - Usage type for IIO NtbSplitBar IIO NtbSplitBar **/ UINT8 NtbSplitBar;
-/** Offset 0x00FF - Usage type for IIO NtbBarSizeImBar1 +/** Offset 0x0103 - Usage type for IIO NtbBarSizeImBar1 IIO NtbBarSizeImBar1 **/ UINT8 NtbBarSizeImBar1;
-/** Offset 0x0100 - Usage type for IIO NtbBarSizeImBar2 +/** Offset 0x0104 - Usage type for IIO NtbBarSizeImBar2 IIO PNtbBarSizeImBar2 **/ UINT8 NtbBarSizeImBar2;
-/** Offset 0x0101 - Usage type for IIO NtbBarSizeImBar2_0 +/** Offset 0x0105 - Usage type for IIO NtbBarSizeImBar2_0 IIO PNtbBarSizeImBar2_0 **/ UINT8 NtbBarSizeImBar2_0;
-/** Offset 0x0102 - Usage type for IIO NtbBarSizeImBar2_1 +/** Offset 0x0106 - Usage type for IIO NtbBarSizeImBar2_1 IIO NtbBarSizeImBar2_1 **/ UINT8 NtbBarSizeImBar2_1;
-/** Offset 0x0103 - Usage type for IIO NtbBarSizeEmBarSZ1 +/** Offset 0x0107 - Usage type for IIO NtbBarSizeEmBarSZ1 IIO NtbBarSizeEmBarSZ1 **/ UINT8 NtbBarSizeEmBarSZ1;
-/** Offset 0x0104 - Usage type for IIO NtbBarSizeEmBarSZ2 +/** Offset 0x0108 - Usage type for IIO NtbBarSizeEmBarSZ2 IIO NtbBarSizeEmBarSZ2 **/ UINT8 NtbBarSizeEmBarSZ2;
-/** Offset 0x0105 - Usage type for IIO NtbBarSizeEmBarSZ2_0 +/** Offset 0x0109 - Usage type for IIO NtbBarSizeEmBarSZ2_0 IIO NtbBarSizeEmBarSZ2_0 **/ UINT8 NtbBarSizeEmBarSZ2_0;
-/** Offset 0x0106 - Usage type for IIO NtbBarSizeEmBarSZ2_1 +/** Offset 0x010A - Usage type for IIO NtbBarSizeEmBarSZ2_1 IIO NtbBarSizeEmBarSZ2_1 **/ UINT8 NtbBarSizeEmBarSZ2_1;
-/** Offset 0x0107 - Usage type for IIO NtbXlinkCtlOverride +/** Offset 0x010B - Usage type for IIO NtbXlinkCtlOverride IIO NtbXlinkCtlOverride **/ UINT8 NtbXlinkCtlOverride;
-/** Offset 0x0108 - Usage type for IIO VT-D Function +/** Offset 0x010C - Usage type for IIO VT-D Function IIO VT-D Function, if supported, the value is 0x01, if not supported, the value is 0x00 **/ UINT8 VtdSupport;
-/** Offset 0x0109 - Usage type for IIO Pcie Port Hide +/** Offset 0x010D - Usage type for IIO Pcie Port Hide Hide or visible for IIO Pcie Port, 1 : Hide, 0 : Visible **/ UINT8 PEXPHIDE;
-/** Offset 0x010A - Usage type for IIO Pcie Port Menu Hide +/** Offset 0x010E - Usage type for IIO Pcie Port Menu Hide Hide or visible for IIO Pcie Port Menu, 1 : Hide, 0 : Visible **/ UINT8 HidePEXPMenu;
-/** Offset 0x010B - PchSirqMode +/** Offset 0x010F - PchSirqMode Enable or Disable PchSirqMode **/ UINT8 PchSirqMode;
-/** Offset 0x010C - PchAdrEn +/** Offset 0x0110 - PchAdrEn Enable or Disable PchAdr **/ UINT8 PchAdrEn;
-/** Offset 0x010D - ThermalDeviceEnable +/** Offset 0x0111 - ThermalDeviceEnable Enable or Disable ThermalDeviceEnable with PCI or ACPI mode **/ UINT8 ThermalDeviceEnable;
-/** Offset 0x010E - } TYPE:{Combo +/** Offset 0x0112 - } TYPE:{Combo Root port swapping based on device connection status : TRUE or FALSE TRUE : 0x01, FALSE : 0x00 **/ UINT8 PchPcieRootPortFunctionSwap;
-/** Offset 0x010F - PCH PCIE PLL Ssc +/** Offset 0x0113 - PCH PCIE PLL Ssc Valid spread range : 0x00-0x14 (A value of 0 is SSC of 0.0%. A value of 20 is SSC of 2.0%), Auto : 0xFE(Set to hardware default), Disable : 0xFF **/ UINT8 PchPciePllSsc;
-/** Offset 0x0110 - Usage type for PCH PCIE Root Port Index +/** Offset 0x0114 - Usage type for PCH PCIE Root Port Index Index assigned to every PCH PCIE Root Port **/ UINT8 PchPciePortIndex[20];
-/** Offset 0x0124 - Usage type for PCH PCIE Root Port Enable or Disable +/** Offset 0x0128 - Usage type for PCH PCIE Root Port Enable or Disable 0-19: PCH rootport, if port is enabled, the value is 0x01, if the port is disabled, the value is 0x00 **/ UINT8 PchPcieForceEnable[20];
-/** Offset 0x0138 - Usage type for PCH PCIE Root Port Link Speed +/** Offset 0x013C - Usage type for PCH PCIE Root Port Link Speed 0-19: PCH rootport, 0x00 : Pcie Auto Speed, 0x01 : Pcie Gen1 Speed, 0x02 : Pcie Gen2 Speed, 0x03 : Pcie Gen3 Speed **/ UINT8 PchPciePortLinkSpeed[20];
-/** Offset 0x014C - PchDciEn +/** Offset 0x0150 - PchDciEn Enable or Disable Pch DciEn **/ UINT8 PchDciEn;
-/** Offset 0x014D - MeUmaEnable +/** Offset 0x0151 - MeUmaEnable Enable or disable ME UMA feature **/ UINT8 MeUmaEnable;
-/** Offset 0x014E - SerialIoUartDebugEnable +/** Offset 0x0152 - SerialIoUartDebugEnable Enable SerialIo Uart debug library in FSP. 0:Disable, 1:Enable **/ UINT8 SerialIoUartDebugEnable;
-/** Offset 0x014F +/** Offset 0x0153 **/ - UINT8 UnusedUpdSpace2; + UINT8 UnusedUpdSpace3;
-/** Offset 0x0150 - ISA Serial Base selection +/** Offset 0x0154 - ISA Serial Base selection Select ISA Serial Base address could be initialized by boot loader. Default is 0x3F8 0x3F8, 0x2F8 **/ UINT16 SerialIoUartDebugIoBase;
-/** Offset 0x0152 +/** Offset 0x0156 **/ - UINT8 UnusedUpdSpace3[2]; + UINT8 UnusedUpdSpace4[2];
-/** Offset 0x0154 +/** Offset 0x0158 **/ UINT8 ReservedMemoryInitUpd[16]; } FSPM_CONFIG; @@ -746,11 +757,11 @@ **/ FSPM_CONFIG FspmConfig;
-/** Offset 0x0164 +/** Offset 0x0168 **/ - UINT8 UnusedUpdSpace4[2]; + UINT8 UnusedUpdSpace5[6];
-/** Offset 0x0166 +/** Offset 0x016E **/ UINT16 UpdTerminator; } FSPM_UPD;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46480/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/vpd.h:
https://review.coreboot.org/c/coreboot/+/46480/1/src/mainboard/ocp/deltalake... PS1, Line 37: #define FSPM_MEMREFRESHWATERMARK_DEFAULT 1 /* Default value when the VPD variable is not found */ line over 96 characters
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46480/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46480/1//COMMIT_MSG@10 PS1, Line 10: FSP binary, but not in Intel's CPX-SP FSP binary. Interesting
https://review.coreboot.org/c/coreboot/+/46480/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/vpd.h:
https://review.coreboot.org/c/coreboot/+/46480/1/src/mainboard/ocp/deltalake... PS1, Line 37: #define FSPM_MEMREFRESHWATERMARK_DEFAULT 1 /* Default value when the VPD variable is not found */
line over 96 characters
I'd just drop the comment
https://review.coreboot.org/c/coreboot/+/46480/1/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h:
PS1: I'd say the changes to this UPD break the ABI when using the Intel CPX-SP FSP binary
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46480/1/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/vpd.h:
https://review.coreboot.org/c/coreboot/+/46480/1/src/mainboard/ocp/deltalake... PS1, Line 37: #define FSPM_MEMREFRESHWATERMARK_DEFAULT 1 /* Default value when the VPD variable is not found */
I'd just drop the comment
makes sense
https://review.coreboot.org/c/coreboot/+/46480/1/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h:
PS1:
I'd say the changes to this UPD break the ABI when using the Intel CPX-SP FSP binary
Yes. When using the Intel (to-be-released) CPX-SP FSP binary, CONFIG_HAVE_FSPM_DIMM_WATERMARK_OPTION can not be set; on the other hand, it has to be set when using FB internal CPX-SP FSP binary.
Hello Marc Jones, build bot (Jenkins), Johnny Lin, Angel Pons, Subrata Banik, Patrick Rudolph, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46480
to look at the new patch set (#2).
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option
FSPM MemRefreshWatermark option support is present in FB's CPX-SP FSP binary, but not in Intel's CPX-SP FSP binary.
In CPX-SP soc code, add a Kconfig HAVE_FSPM_DIMM_WATERMARK_OPTION to indicate whether the FSP binary supports such option.
For DeltaLake mainboard, when such option is set, if corresponding VPD variable is set, use it to control the behavior.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I7592dfc8836f457d64f1976cba3b59f8251c7abe --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/romstage.c M src/mainboard/ocp/deltalake/vpd.h M src/soc/intel/xeon_sp/cpx/Kconfig M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h 5 files changed, 88 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/46480/2
Hello build bot (Jenkins), Marc Jones, Johnny Lin, Angel Pons, Subrata Banik, Patrick Rudolph, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46480
to look at the new patch set (#3).
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option
FSPM MemRefreshWatermark option support is present in FB's CPX-SP FSP binary, but not in Intel's CPX-SP FSP binary.
In CPX-SP soc code, add a Kconfig HAVE_FSPM_DIMM_WATERMARK_OPTION to indicate whether the FSP binary supports such option.
For DeltaLake mainboard, when such option is set, if corresponding VPD variable is set, use it to control the behavior.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I7592dfc8836f457d64f1976cba3b59f8251c7abe --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/romstage.c M src/mainboard/ocp/deltalake/vpd.h M src/soc/intel/xeon_sp/cpx/Kconfig M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h 5 files changed, 92 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/46480/3
Hello build bot (Jenkins), Marc Jones, Johnny Lin, Angel Pons, Subrata Banik, Patrick Rudolph, Tim Chu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46480
to look at the new patch set (#4).
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option
FSPM MemRefreshWatermark option support is present in FB's CPX-SP FSP binary, but not in Intel's CPX-SP FSP binary.
In CPX-SP soc code, add a Kconfig HAVE_FSPM_DIMM_WATERMARK_OPTION to indicate whether the FSP binary supports such option.
For DeltaLake mainboard, when such option is set, if corresponding VPD variable is set, use it to control the behavior.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I7592dfc8836f457d64f1976cba3b59f8251c7abe --- M src/mainboard/ocp/deltalake/Kconfig M src/mainboard/ocp/deltalake/romstage.c M src/mainboard/ocp/deltalake/vpd.h M src/soc/intel/xeon_sp/cpx/Kconfig M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h 5 files changed, 92 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/46480/4
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46480/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46480/5//COMMIT_MSG@10 PS5, Line 10: FSP binary, but not in Intel's CPX-SP FSP binary. Do we want to support such inconsistencies in coreboot? I think coreboot should only support one type of FSP. Supporting different versions only opens a can of worms.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46480/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46480/5//COMMIT_MSG@10 PS5, Line 10: FSP binary, but not in Intel's CPX-SP FSP binary.
Do we want to support such inconsistencies in coreboot? I think coreboot should only support one typ […]
We do not want to and we want to minimize the difference. That being said, this become unavoidable. Since Intel CPX-SP FSP binary is not released yet, and since FB is probably the only Intel customer who uses CPX-SP FSP binary, we could go with not adding a Kconfig, and not do conditional compilation at this time. But sooner or later we will need to deal with such situation.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 6: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/46480/1/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/46480/1/src/vendorcode/intel/fsp/fs... PS1, Line 498: #if CONFIG_HAVE_FSPM_DIMM_WATERMARK_OPTION : /** Offset 0x00C4 - Usage type for Memory Refresh Watermark : Select Memory Refresh Watermark, 0: Auto (default), 1: High, 2: Low : **/ : UINT8 MemRefreshWaterMark; : #endif Any reason to put this here and not in one of the UnusedUpdSpace areas where you would get around needing a config guard to protect against incompatibilities between FSPs?
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46480/1/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/46480/1/src/vendorcode/intel/fsp/fs... PS1, Line 498: #if CONFIG_HAVE_FSPM_DIMM_WATERMARK_OPTION : /** Offset 0x00C4 - Usage type for Memory Refresh Watermark : Select Memory Refresh Watermark, 0: Auto (default), 1: High, 2: Low : **/ : UINT8 MemRefreshWaterMark; : #endif
Any reason to put this here and not in one of the UnusedUpdSpace areas where you would get around ne […]
Intel groups configurations together (this is a good practice), and hence this option belongs to Memory Pre-Mem group. Good idea to use UnusedUpdSpace. There are several ones scattered. I will use the top most one (and sacrifice the grouping), so that I do not have to rebase the patch too often. I will mark this patch and [CB: 46609] as private as they are used by the team. I will create another patch instead.
Jonathan Zhang has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/46480 )
Change subject: vc/intel/FSP2_0/CPX-SP: add FSPM DIMM watermark option ......................................................................
Abandoned
superceded by [CB:46640]
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Jonathan Zhang (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)