Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
src/lib: Add Kconfig option for SPD cache in FMAP
Currently, the option to cache sodimm SPD data in an FMAP region is closely coupled to a single board (google/hatch) and requires a custom FMAP to utilize.
Loosen this coupling by introducing a Kconfig option which adds a correctly sized and aligned RW_SPD_CACHE region to the default FMAP. Change the inclusion of spd_cache.c to use this new Kconfig, rather than the board-specific one currently used. Lastly, have google/hatch select the new Kconfig when appropriate to ensure no change in current functionality.
Test: build/boot WYVERN google/hatch variant with default FMAP, verify FMAP contains RW_SPD_CACHE, verify SPD cache used via cbmem log.
Change-Id: Iee0e7acb01e238d7ed354e3dbab1207903e3a4fc Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M Makefile.inc M src/lib/Kconfig M src/lib/Makefile.inc M src/mainboard/google/hatch/Kconfig M util/cbfstool/default-x86.fmd 5 files changed, 24 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/48520/1
diff --git a/Makefile.inc b/Makefile.inc index dee4a2e..f8beae0 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -976,6 +976,16 @@ FMAP_SMMSTORE_ENTRY := endif
+ifeq ($(CONFIG_SPD_CACHE_IN_FMAP),y) +FMAP_SPD_CACHE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x4000) +FMAP_SPD_CACHE_SIZE := $(call int-multiply, $(CONFIG_DIMM_MAX) $(CONFIG_DIMM_SPD_SIZE)) +FMAP_SPD_CACHE_SIZE := $(call int-align, $(FMAP_SPD_CACHE_SIZE), 0x1000) +FMAP_SPD_CACHE_ENTRY := RW_SPD_CACHE@$(FMAP_SPD_CACHE_BASE) $(FMAP_SPD_CACHE_SIZE) +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_SPD_CACHE_BASE) $(FMAP_SPD_CACHE_SIZE)) +else +FMAP_SPD_CACHE_ENTRY := +endif + # # X86 FMAP region # @@ -1052,6 +1062,7 @@ -e "s,##CONSOLE_ENTRY##,$(FMAP_CONSOLE_ENTRY)," \ -e "s,##MRC_CACHE_ENTRY##,$(FMAP_MRC_CACHE_ENTRY)," \ -e "s,##SMMSTORE_ENTRY##,$(FMAP_SMMSTORE_ENTRY)," \ + -e "s,##SPD_CACHE_ENTRY##,$(FMAP_SPD_CACHE_ENTRY)," \ -e "s,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \ -e "s,##CBFS_SIZE##,$(FMAP_CBFS_SIZE)," \ $(DEFAULT_FLASHMAP) > $@.tmp diff --git a/src/lib/Kconfig b/src/lib/Kconfig index ab0182c..9bc0b2d 100644 --- a/src/lib/Kconfig +++ b/src/lib/Kconfig @@ -54,6 +54,16 @@ config SPD_READ_BY_WORD bool
+config SPD_CACHE_IN_FMAP + bool + default n + help + Enables capability to cache SODIMM SPDs in a dedicated FMAP region + to speed loading of SPD data. Requires board-level implementation to + read/write/utilize cached SPD data. + When the default FMAP is used, will create a region named RW_SPD_CACHE + to store the cached SPD data. + if RAMSTAGE_LIBHWBASE
config HWBASE_DYNAMIC_MMIO diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 9e601eb..8b2ec2d 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -374,4 +374,4 @@
ramstage-y += uuid.c
-romstage-$(CONFIG_ROMSTAGE_SPD_SMBUS) += spd_cache.c +romstage-$(CONFIG_SPD_CACHE_IN_FMAP) += spd_cache.c diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 20b7103..a4e91b6 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -86,6 +86,7 @@ config ROMSTAGE_SPD_SMBUS bool default n + select SPD_CACHE_IN_FMAP
config DRIVER_TPM_SPI_BUS default 0x1 diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd index f0143e9..25c5096 100644 --- a/util/cbfstool/default-x86.fmd +++ b/util/cbfstool/default-x86.fmd @@ -12,6 +12,7 @@ ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## ##SMMSTORE_ENTRY## + ##SPD_CACHE_ENTRY## FMAP@##FMAP_BASE## ##FMAP_SIZE## COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE## }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 1: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@57 PS1, Line 57: _IN_FMAP Do we need the _IN_FMAP part? Also, should SODIMM be in the name? SODIMM_SPD_CACHE?
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@62 PS1, Line 62: board-level implementation I think this should be something that a common library or SoC code handles. Anyways, it is out of scope for this CL.
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@64 PS1, Line 64: RW_SPD_CACHE Just a note: Since the default is being set to RW_SPD_CACHE, any board using this in their own custom fmd file will also have to use the same name.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@57 PS1, Line 57: _IN_FMAP
Do we need the _IN_FMAP part? Also, should SODIMM be in the name? SODIMM_SPD_CACHE?
I'm not sure how else one might cache SPD data, but felt like being more explicit was better than less. I have no problem changing if another symbol is preferred
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@62 PS1, Line 62: board-level implementation
I think this should be something that a common library or SoC code handles. […]
I'll ponder how to do that for a follow up :)
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@64 PS1, Line 64: RW_SPD_CACHE
Just a note: Since the default is being set to RW_SPD_CACHE, any board using this in their own custo […]
this is actually hardcoded in spd_cache.h; I tried finding a way to make it a Kconfig symbol so it could be set dynamically, but came up empty due to the limitations of Makefiles (I believe SMMSTORE is handled similarly for the same reason).
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@57 PS1, Line 57: _IN_FMAP
I'm not sure how else one might cache SPD data, but felt like being more explicit was better than le […]
It's a minor nit since it felt redundant. I am okay if you want to go ahead with this name.
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@64 PS1, Line 64: RW_SPD_CACHE
this is actually hardcoded in spd_cache. […]
That is strange. Came out empty in Makefile.inc where you are updating the default fmd file?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/48520/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48520/1//COMMIT_MSG@9 PS1, Line 9: sodimm nit: I'd say `DIMM` instead
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@57 PS1, Line 57: _IN_FMAP
It's a minor nit since it felt redundant. I am okay if you want to go ahead with this name.
I wouldn't use SODIMM in the option name. After all, it would work just fine with desktop-size DIMMs, wouldn't it? 😊
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@61 PS1, Line 61: SODIMM nit: DIMM
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 1: Code-Review+2
Nice! lgtm once the nits are fixed
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@64 PS1, Line 64: RW_SPD_CACHE
That is strange. Came out empty in Makefile. […]
just tried it again and seemed to work fine, so I'll add that in as well
Hello build bot (Jenkins), Furquan Shaikh, Jamie Chen, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48520
to look at the new patch set (#2).
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
src/lib: Add Kconfig option for SPD cache in FMAP
Currently, the option to cache dimm SPD data in an FMAP region is closely coupled to a single board (google/hatch) and requires a custom FMAP to utilize.
Loosen this coupling by introducing a Kconfig option which adds a correctly sized and aligned RW_SPD_CACHE region to the default FMAP. Add a Kconfig option for the region name, replacing the existing hard- coded instance in spd_cache.h. Change the inclusion of spd_cache.c to use this new Kconfig, rather than the board-specific one currently used. Lastly, have google/hatch select the new Kconfig when appropriate to ensure no change in current functionality.
Test: build/boot WYVERN google/hatch variant with default FMAP, verify FMAP contains RW_SPD_CACHE, verify SPD cache used via cbmem log.
Also tested on an out-of-tree Purism board.
Change-Id: Iee0e7acb01e238d7ed354e3dbab1207903e3a4fc Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M Makefile.inc M src/include/spd_cache.h M src/lib/Kconfig M src/lib/Makefile.inc M src/mainboard/google/hatch/Kconfig M util/cbfstool/default-x86.fmd 6 files changed, 32 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/48520/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@57 PS1, Line 57: _IN_FMAP
I wouldn't use SODIMM in the option name. […]
Woops. You are right. I should have said DIMM_SPD_CACHE.
Hello build bot (Jenkins), Furquan Shaikh, Jamie Chen, Patrick Georgi, Martin Roth, Tim Wawrzynczak, Angel Pons, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48520
to look at the new patch set (#3).
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
src/lib: Add Kconfig option for SPD cache in FMAP
Currently, the option to cache DIMM SPD data in an FMAP region is closely coupled to a single board (google/hatch) and requires a custom FMAP to utilize.
Loosen this coupling by introducing a Kconfig option which adds a correctly sized and aligned RW_SPD_CACHE region to the default FMAP. Add a Kconfig option for the region name, replacing the existing hard- coded instance in spd_cache.h. Change the inclusion of spd_cache.c to use this new Kconfig, rather than the board-specific one currently used. Lastly, have google/hatch select the new Kconfig when appropriate to ensure no change in current functionality.
Test: build/boot WYVERN google/hatch variant with default FMAP, verify FMAP contains RW_SPD_CACHE, verify SPD cache used via cbmem log.
Also tested on an out-of-tree Purism board.
Change-Id: Iee0e7acb01e238d7ed354e3dbab1207903e3a4fc Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M Makefile.inc M src/include/spd_cache.h M src/lib/Kconfig M src/lib/Makefile.inc M src/mainboard/google/hatch/Kconfig M util/cbfstool/default-x86.fmd 6 files changed, 32 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/48520/3
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48520/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48520/1//COMMIT_MSG@9 PS1, Line 9: sodimm
nit: I'd say `DIMM` instead
Done
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/48520/1/src/lib/Kconfig@61 PS1, Line 61: SODIMM
nit: DIMM
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
Patch Set 3: Code-Review+2
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48520 )
Change subject: src/lib: Add Kconfig option for SPD cache in FMAP ......................................................................
src/lib: Add Kconfig option for SPD cache in FMAP
Currently, the option to cache DIMM SPD data in an FMAP region is closely coupled to a single board (google/hatch) and requires a custom FMAP to utilize.
Loosen this coupling by introducing a Kconfig option which adds a correctly sized and aligned RW_SPD_CACHE region to the default FMAP. Add a Kconfig option for the region name, replacing the existing hard- coded instance in spd_cache.h. Change the inclusion of spd_cache.c to use this new Kconfig, rather than the board-specific one currently used. Lastly, have google/hatch select the new Kconfig when appropriate to ensure no change in current functionality.
Test: build/boot WYVERN google/hatch variant with default FMAP, verify FMAP contains RW_SPD_CACHE, verify SPD cache used via cbmem log.
Also tested on an out-of-tree Purism board.
Change-Id: Iee0e7acb01e238d7ed354e3dbab1207903e3a4fc Signed-off-by: Matt DeVillier matt.devillier@puri.sm Reviewed-on: https://review.coreboot.org/c/coreboot/+/48520 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Makefile.inc M src/include/spd_cache.h M src/lib/Kconfig M src/lib/Makefile.inc M src/mainboard/google/hatch/Kconfig M util/cbfstool/default-x86.fmd 6 files changed, 32 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index 90280df..90c2494 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -966,6 +966,16 @@ FMAP_SMMSTORE_ENTRY := endif
+ifeq ($(CONFIG_SPD_CACHE_IN_FMAP),y) +FMAP_SPD_CACHE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x4000) +FMAP_SPD_CACHE_SIZE := $(call int-multiply, $(CONFIG_DIMM_MAX) $(CONFIG_DIMM_SPD_SIZE)) +FMAP_SPD_CACHE_SIZE := $(call int-align, $(FMAP_SPD_CACHE_SIZE), 0x1000) +FMAP_SPD_CACHE_ENTRY := $(CONFIG_SPD_CACHE_FMAP_NAME)@$(FMAP_SPD_CACHE_BASE) $(FMAP_SPD_CACHE_SIZE) +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_SPD_CACHE_BASE) $(FMAP_SPD_CACHE_SIZE)) +else +FMAP_SPD_CACHE_ENTRY := +endif + # # X86 FMAP region # @@ -1042,6 +1052,7 @@ -e "s,##CONSOLE_ENTRY##,$(FMAP_CONSOLE_ENTRY)," \ -e "s,##MRC_CACHE_ENTRY##,$(FMAP_MRC_CACHE_ENTRY)," \ -e "s,##SMMSTORE_ENTRY##,$(FMAP_SMMSTORE_ENTRY)," \ + -e "s,##SPD_CACHE_ENTRY##,$(FMAP_SPD_CACHE_ENTRY)," \ -e "s,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \ -e "s,##CBFS_SIZE##,$(FMAP_CBFS_SIZE)," \ $(DEFAULT_FLASHMAP) > $@.tmp diff --git a/src/include/spd_cache.h b/src/include/spd_cache.h index 5465aad..a53b8eb 100644 --- a/src/include/spd_cache.h +++ b/src/include/spd_cache.h @@ -7,7 +7,7 @@ #include <stddef.h> #include <stdint.h>
-#define SPD_CACHE_FMAP_NAME "RW_SPD_CACHE" +#define SPD_CACHE_FMAP_NAME (CONFIG_SPD_CACHE_FMAP_NAME) #define SC_SPD_NUMS (CONFIG_DIMM_MAX) #define SC_SPD_OFFSET(n) (CONFIG_DIMM_SPD_SIZE * n) #define SC_CRC_OFFSET (CONFIG_DIMM_MAX * CONFIG_DIMM_SPD_SIZE) diff --git a/src/lib/Kconfig b/src/lib/Kconfig index 168a068..e1d56fe 100644 --- a/src/lib/Kconfig +++ b/src/lib/Kconfig @@ -46,6 +46,23 @@ config SPD_READ_BY_WORD bool
+config SPD_CACHE_IN_FMAP + bool + default n + help + Enables capability to cache DIMM SPDs in a dedicated FMAP region + to speed loading of SPD data. Currently requires board-level + romstage implementation to read/write/utilize cached SPD data. + When the default FMAP is used, will create a region named RW_SPD_CACHE + to store the cached SPD data. + +config SPD_CACHE_FMAP_NAME + string + depends on SPD_CACHE_IN_FMAP + default "RW_SPD_CACHE" + help + Name of the FMAP region created in the default FMAP to cache SPD data. + if RAMSTAGE_LIBHWBASE
config HWBASE_DYNAMIC_MMIO diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index 07555a7..8424cbf 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -372,4 +372,4 @@
ramstage-y += uuid.c
-romstage-$(CONFIG_ROMSTAGE_SPD_SMBUS) += spd_cache.c +romstage-$(CONFIG_SPD_CACHE_IN_FMAP) += spd_cache.c diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 20b7103..a4e91b6 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -86,6 +86,7 @@ config ROMSTAGE_SPD_SMBUS bool default n + select SPD_CACHE_IN_FMAP
config DRIVER_TPM_SPI_BUS default 0x1 diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd index f0143e9..25c5096 100644 --- a/util/cbfstool/default-x86.fmd +++ b/util/cbfstool/default-x86.fmd @@ -12,6 +12,7 @@ ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## ##SMMSTORE_ENTRY## + ##SPD_CACHE_ENTRY## FMAP@##FMAP_BASE## ##FMAP_SIZE## COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE## }
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Hung-Te Lin (Code Review)
-
Matt DeVillier (Code Review)
-
Tim Wawrzynczak (Code Review)