Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
mb/purism/librem_cnl: Use FMAP-based SPD cache
Use a FMAP region to cache SPD data, providing improvements in boot time and detection of change in DIMM population (whcih FSP will sometimes fail to detect / fail to invalidate the MRC cache).
Adapted from implemention used in google/hatch.
Test: build/boot Librem Mini v2, verify SPD cache used, changes in DIMM population properly detected.
Change-Id: I15cb9aa8b00d39d098a0f901aee026bac1161a80 Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M src/mainboard/purism/librem_cnl/Kconfig M src/mainboard/purism/librem_cnl/romstage.c 2 files changed, 93 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/48549/1
diff --git a/src/mainboard/purism/librem_cnl/Kconfig b/src/mainboard/purism/librem_cnl/Kconfig index 1d31ec6..d0282ad 100644 --- a/src/mainboard/purism/librem_cnl/Kconfig +++ b/src/mainboard/purism/librem_cnl/Kconfig @@ -8,6 +8,7 @@ select INTEL_GMA_HAVE_VBT select NO_UART_ON_SUPERIO select SOC_INTEL_COMMON_BLOCK_HDA_VERB + select SPD_CACHE_IN_FMAP select SPD_READ_BY_WORD select USE_LEGACY_8254_TIMER
diff --git a/src/mainboard/purism/librem_cnl/romstage.c b/src/mainboard/purism/librem_cnl/romstage.c index b7c57ee..44d8ad0 100644 --- a/src/mainboard/purism/librem_cnl/romstage.c +++ b/src/mainboard/purism/librem_cnl/romstage.c @@ -1,55 +1,104 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <console/console.h> #include <soc/cnl_memcfg_init.h> #include <soc/romstage.h> +#include <spd_bin.h> +#include <spd_cache.h> #include "variant.h"
-static const struct cnl_mb_cfg memcfg = { - - /* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */ - .spd[0] = { - .read_type = READ_SMBUS, - .spd_spec = {.spd_smbus_address = 0xa0}, - }, - .spd[1] = {.read_type = NOT_EXISTING}, - .spd[2] = { - .read_type = READ_SMBUS, - .spd_spec = {.spd_smbus_address = 0xa4}, - }, - .spd[3] = {.read_type = NOT_EXISTING}, - - /* - * Rcomp resistor values. These values represent the resistance in - * ohms of the three rcomp resistors attached to the DDR_COMP_0, - * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM. - */ - .rcomp_resistor = { 121, 81, 100 }, - - /* Rcomp target values */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, - - /* - * Indicates whether memory is interleaved. - * Set to 1 for an interleaved design, - * set to 0 for non-interleaved design. - */ - .dq_pins_interleaved = 1, - - /* - * VREF_CA configuration. - * Set to 0 VREF_CA goes to both CH_A and CH_B, - * set to 1 VREF_CA goes to CH_A and VREF_DQ_A goes to CH_B, - * set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. - */ - .vref_ca_config = 2, - - /* Early Command Training */ - .ect = 0, -}; - void mainboard_memory_init_params(FSPM_UPD *memupd) { FSP_M_CONFIG *mem_cfg = &memupd->FspmConfig; + + struct cnl_mb_cfg memcfg = { + + /* + * Rcomp resistor values. These values represent the resistance in + * ohms of the three rcomp resistors attached to the DDR_COMP_0, + * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM. + */ + .rcomp_resistor = { 121, 81, 100 }, + + /* Rcomp target values */ + .rcomp_targets = { 100, 40, 20, 20, 26 }, + + /* + * Indicates whether memory is interleaved. + * Set to 1 for an interleaved design, + * set to 0 for non-interleaved design. + */ + .dq_pins_interleaved = 1, + + /* + * VREF_CA configuration. + * Set to 0 VREF_CA goes to both CH_A and CH_B, + * set to 1 VREF_CA goes to CH_A and VREF_DQ_A goes to CH_B, + * set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. + */ + .vref_ca_config = 2, + + /* Early Command Training */ + .ect = 0, + }; + + + /* Read spd block to get memory config */ + struct spd_block blk = { + .addr_map = { 0x50, 0x52, }, + }; + + uint8_t *spd_cache; + size_t spd_cache_sz; + bool need_update_cache = false; + bool dimm_changed = true; + + /* load spd cache from RW_SPD_CACHE */ + if (load_spd_cache(&spd_cache, &spd_cache_sz) == CB_SUCCESS) { + if (!spd_cache_is_valid(spd_cache, spd_cache_sz)) { + printk(BIOS_WARNING, "Invalid SPD cache\n"); + } else { + dimm_changed = check_if_dimm_changed(spd_cache, &blk); + if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != NULL) { + /* Set mrc_cache as invalid */ + printk(BIOS_INFO, "Set mrc_cache as invalid\n"); + memupd->FspmArchUpd.NvsBufferPtr = NULL; + } + } + need_update_cache = true; + } + + if (!dimm_changed) { + spd_fill_from_cache(spd_cache, &blk); + } else { + /* Access memory info through SMBUS. */ + get_spd_smbus(&blk); + + if (need_update_cache && update_spd_cache(&blk) == CB_ERR) + printk(BIOS_WARNING, "update SPD cache failed\n"); + } + + if (blk.spd_array[0] == NULL) { + memcfg.spd[0].read_type = NOT_EXISTING; + } else { + memcfg.spd[0].read_type = READ_SPD_MEMPTR; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; + } + + memcfg.spd[1].read_type = NOT_EXISTING; + + if (blk.spd_array[1] == NULL) { + memcfg.spd[2].read_type = NOT_EXISTING; + } else { + memcfg.spd[2].read_type = READ_SPD_MEMPTR; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; + } + + memcfg.spd[3].read_type = NOT_EXISTING; + dump_spd_info(&blk); + cannonlake_memcfg_init(mem_cfg, &memcfg); variant_memory_init_params(mem_cfg); }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48549/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/1/src/mainboard/purism/librem... PS1, Line 86: memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; line over 96 characters
https://review.coreboot.org/c/coreboot/+/48549/1/src/mainboard/purism/librem... PS1, Line 96: memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; line over 96 characters
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48549
to look at the new patch set (#2).
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
mb/purism/librem_cnl: Use FMAP-based SPD cache
Use a FMAP region to cache SPD data, providing improvements in boot time and detection of change in DIMM population (whcih FSP will sometimes fail to detect / fail to invalidate the MRC cache).
Adapted from implemention used in google/hatch.
Test: build/boot Librem Mini v2, verify SPD cache used, changes in DIMM population properly detected.
Change-Id: I15cb9aa8b00d39d098a0f901aee026bac1161a80 Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M src/mainboard/purism/librem_cnl/Kconfig M src/mainboard/purism/librem_cnl/romstage.c 2 files changed, 95 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/48549/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48549/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48549/2//COMMIT_MSG@13 PS2, Line 13: implemention implement*at*ion
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 68: true nit: for clarity, maybe set this to `dimm_changed`. The value is only used when `dimm_changed` is true.
Unifying both variables can't be done, since it would break the case where `load_spd_cache` fails
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 104: cannonlake_memcfg_init(mem_cfg, &memcfg); Wouldn't it make more sense to move the spd cache code into `cannonlake_memcfg_init`?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 68: true
nit: for clarity, maybe set this to `dimm_changed`. […]
IMO, that's actually less clear, and I don't want to diverge the implementation here and in google/hatch unnecessarily if we're going to end up unifying it at the SoC or common level
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 104: cannonlake_memcfg_init(mem_cfg, &memcfg);
Wouldn't it make more sense to move the spd cache code into `cannonlake_memcfg_init`?
possibly, but one would at least need to pass in the channel config, SPD address map, etc. But open to ideas
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 104: cannonlake_memcfg_init(mem_cfg, &memcfg);
possibly, but one would at least need to pass in the channel config, SPD address map, etc. […]
You mean one would need to pass in a `struct cnl_mb_cfg` struct, like we already do?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 68: true
IMO, that's actually less clear, and I don't want to diverge the implementation here and in google/h […]
Ack, can be done in a follow-up if need be
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 104: cannonlake_memcfg_init(mem_cfg, &memcfg);
You mean one would need to pass in a `struct cnl_mb_cfg` struct, like we already do?
the SPD addressing is different when using coreboot to read the SPD via smbus vs FSP, so that alone seems like it would add some complexity, but maybe not
Hello build bot (Jenkins), Angel Pons, Michael Niewöhner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48549
to look at the new patch set (#3).
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
mb/purism/librem_cnl: Use FMAP-based SPD cache
Use a FMAP region to cache SPD data, providing improvements in boot time and detection of change in DIMM population (which FSP will sometimes fail to detect / fail to invalidate the MRC cache).
Adapted from implementation used in google/hatch.
Test: build/boot Librem Mini v2, verify SPD cache used, changes in DIMM population properly detected.
Change-Id: I15cb9aa8b00d39d098a0f901aee026bac1161a80 Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M src/mainboard/purism/librem_cnl/Kconfig M src/mainboard/purism/librem_cnl/romstage.c 2 files changed, 95 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/48549/3
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48549/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48549/2//COMMIT_MSG@13 PS2, Line 13: implemention
implement*at*ion
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/romstage.c:
https://review.coreboot.org/c/coreboot/+/48549/2/src/mainboard/purism/librem... PS2, Line 104: cannonlake_memcfg_init(mem_cfg, &memcfg);
the SPD addressing is different when using coreboot to read the SPD via smbus vs FSP, so that alone […]
It would need to be thought. Anyway, no reason to block this change.
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48549 )
Change subject: mb/purism/librem_cnl: Use FMAP-based SPD cache ......................................................................
mb/purism/librem_cnl: Use FMAP-based SPD cache
Use a FMAP region to cache SPD data, providing improvements in boot time and detection of change in DIMM population (which FSP will sometimes fail to detect / fail to invalidate the MRC cache).
Adapted from implementation used in google/hatch.
Test: build/boot Librem Mini v2, verify SPD cache used, changes in DIMM population properly detected.
Change-Id: I15cb9aa8b00d39d098a0f901aee026bac1161a80 Signed-off-by: Matt DeVillier matt.devillier@puri.sm Reviewed-on: https://review.coreboot.org/c/coreboot/+/48549 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/purism/librem_cnl/Kconfig M src/mainboard/purism/librem_cnl/romstage.c 2 files changed, 95 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/purism/librem_cnl/Kconfig b/src/mainboard/purism/librem_cnl/Kconfig index 1d31ec6..d0282ad 100644 --- a/src/mainboard/purism/librem_cnl/Kconfig +++ b/src/mainboard/purism/librem_cnl/Kconfig @@ -8,6 +8,7 @@ select INTEL_GMA_HAVE_VBT select NO_UART_ON_SUPERIO select SOC_INTEL_COMMON_BLOCK_HDA_VERB + select SPD_CACHE_IN_FMAP select SPD_READ_BY_WORD select USE_LEGACY_8254_TIMER
diff --git a/src/mainboard/purism/librem_cnl/romstage.c b/src/mainboard/purism/librem_cnl/romstage.c index b7c57ee..fd3154f 100644 --- a/src/mainboard/purism/librem_cnl/romstage.c +++ b/src/mainboard/purism/librem_cnl/romstage.c @@ -1,55 +1,106 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <console/console.h> #include <soc/cnl_memcfg_init.h> #include <soc/romstage.h> +#include <spd_bin.h> +#include <spd_cache.h> #include "variant.h"
-static const struct cnl_mb_cfg memcfg = { - - /* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */ - .spd[0] = { - .read_type = READ_SMBUS, - .spd_spec = {.spd_smbus_address = 0xa0}, - }, - .spd[1] = {.read_type = NOT_EXISTING}, - .spd[2] = { - .read_type = READ_SMBUS, - .spd_spec = {.spd_smbus_address = 0xa4}, - }, - .spd[3] = {.read_type = NOT_EXISTING}, - - /* - * Rcomp resistor values. These values represent the resistance in - * ohms of the three rcomp resistors attached to the DDR_COMP_0, - * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM. - */ - .rcomp_resistor = { 121, 81, 100 }, - - /* Rcomp target values */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, - - /* - * Indicates whether memory is interleaved. - * Set to 1 for an interleaved design, - * set to 0 for non-interleaved design. - */ - .dq_pins_interleaved = 1, - - /* - * VREF_CA configuration. - * Set to 0 VREF_CA goes to both CH_A and CH_B, - * set to 1 VREF_CA goes to CH_A and VREF_DQ_A goes to CH_B, - * set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. - */ - .vref_ca_config = 2, - - /* Early Command Training */ - .ect = 0, -}; - void mainboard_memory_init_params(FSPM_UPD *memupd) { FSP_M_CONFIG *mem_cfg = &memupd->FspmConfig; + + struct cnl_mb_cfg memcfg = { + + /* + * Rcomp resistor values. These values represent the resistance in + * ohms of the three rcomp resistors attached to the DDR_COMP_0, + * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM. + */ + .rcomp_resistor = { 121, 81, 100 }, + + /* Rcomp target values */ + .rcomp_targets = { 100, 40, 20, 20, 26 }, + + /* + * Indicates whether memory is interleaved. + * Set to 1 for an interleaved design, + * set to 0 for non-interleaved design. + */ + .dq_pins_interleaved = 1, + + /* + * VREF_CA configuration. + * Set to 0 VREF_CA goes to both CH_A and CH_B, + * set to 1 VREF_CA goes to CH_A and VREF_DQ_A goes to CH_B, + * set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. + */ + .vref_ca_config = 2, + + /* Early Command Training */ + .ect = 0, + }; + + + /* Read spd block to get memory config */ + struct spd_block blk = { + .addr_map = { 0x50, 0x52, }, + }; + + uint8_t *spd_cache; + size_t spd_cache_sz; + bool need_update_cache = false; + bool dimm_changed = true; + + /* load spd cache from RW_SPD_CACHE */ + if (load_spd_cache(&spd_cache, &spd_cache_sz) == CB_SUCCESS) { + if (!spd_cache_is_valid(spd_cache, spd_cache_sz)) { + printk(BIOS_WARNING, "Invalid SPD cache\n"); + } else { + dimm_changed = check_if_dimm_changed(spd_cache, &blk); + if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != NULL) { + /* Set mrc_cache as invalid */ + printk(BIOS_INFO, "Set mrc_cache as invalid\n"); + memupd->FspmArchUpd.NvsBufferPtr = NULL; + } + } + need_update_cache = true; + } + + if (!dimm_changed) { + spd_fill_from_cache(spd_cache, &blk); + } else { + /* Access memory info through SMBUS. */ + get_spd_smbus(&blk); + + if (need_update_cache && update_spd_cache(&blk) == CB_ERR) + printk(BIOS_WARNING, "update SPD cache failed\n"); + } + + if (blk.spd_array[0] == NULL) { + memcfg.spd[0].read_type = NOT_EXISTING; + } else { + memcfg.spd[0].read_type = READ_SPD_MEMPTR; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = + (uintptr_t)blk.spd_array[0]; + } + + memcfg.spd[1].read_type = NOT_EXISTING; + + if (blk.spd_array[1] == NULL) { + memcfg.spd[2].read_type = NOT_EXISTING; + } else { + memcfg.spd[2].read_type = READ_SPD_MEMPTR; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = + (uintptr_t)blk.spd_array[1]; + } + + memcfg.spd[3].read_type = NOT_EXISTING; + dump_spd_info(&blk); + cannonlake_memcfg_init(mem_cfg, &memcfg); variant_memory_init_params(mem_cfg); }
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Hung-Te Lin (Code Review)
-
Matt DeVillier (Code Review)