Change in coreboot[master]: Makefile: Simplify calculation of region base with default fmd files
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40373 )
Change subject: Makefile: Simplify calculation of region base with default fmd files ......................................................................
Makefile: Simplify calculation of region base with default fmd files
When using default fmd files, base of the fmap region is currently calculated based on the size and base of previous fmap regions. Since the existence of any fmap region is dependent on the selection of certain CONFIG_* parameters, these calculations get complicated. Every time base is calculated for a region, there need to be checks to see which of the previous regions really exist. As the regions in default fmd file are increased, these calculations and the conditional checks get even more complicated.
This change introduces a Makefile variable FMAP_CURRENT_BASE which is updated every time a new region is allocated space. This allows using the same steps for determining the base of any fmap region irrespective of the state of previous regions.
The way the code is organized it should be possible in the future to also add a macro to perform the same steps (in case that is possible).
TEST=Verified that coreboot image generated remains unchanged for x86 and ARM boards using the default fmd files.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I2a109462928b6e8b7930bbcc1a1ba45fa85de6ac --- M Makefile.inc 1 file changed, 30 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/40373/1
diff --git a/Makefile.inc b/Makefile.inc index dbf95d4..076c139 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -896,52 +896,42 @@ # X86 CONSOLE FMAP region # # position, size and entry line of CONSOLE relative to BIOS_BASE, if enabled -FMAP_CONSOLE_BASE := 0 + +FMAP_CURRENT_BASE := 0 + ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) +FMAP_CONSOLE_BASE := $(FMAP_CURRENT_BASE) FMAP_CONSOLE_SIZE := $(CONFIG_CONSOLE_SPI_FLASH_BUFFER_SIZE) FMAP_CONSOLE_ENTRY := CONSOLE@$(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) -else # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_CONSOLE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE)) +else FMAP_CONSOLE_ENTRY := -endif # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) +endif
-# -# X86 RW_MRC_CACHE FMAP region -# -# position, size and entry line of MRC_CACHE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -FMAP_MRC_CACHE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ - $(FMAP_CONSOLE_SIZE)), 0x10000) +FMAP_MRC_CACHE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10000) FMAP_MRC_CACHE_SIZE := $(CONFIG_MRC_SETTINGS_CACHE_SIZE) FMAP_MRC_CACHE_ENTRY := RW_MRC_CACHE@$(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE) -else # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -FMAP_MRC_CACHE_BASE := 0 -FMAP_MRC_CACHE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE)) +else FMAP_MRC_CACHE_ENTRY := -endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) +endif
-# -# X86 SMMSTORE FMAP region -# -# position, size and entry line of SMMSTORE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_SMMSTORE),y) -FMAP_SMMSTORE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ - $(FMAP_CONSOLE_SIZE) $(FMAP_MRC_CACHE_SIZE)), 0x10000) +FMAP_SMMSTORE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10000) FMAP_SMMSTORE_SIZE := $(CONFIG_SMMSTORE_SIZE) FMAP_SMMSTORE_ENTRY := SMMSTORE@$(FMAP_SMMSTORE_BASE) $(FMAP_SMMSTORE_SIZE) -else # ifeq ($(CONFIG_SMMSTORE),y) -FMAP_SMMSTORE_BASE := 0 -FMAP_SMMSTORE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_SMMSTORE_BASE) $(FMAP_SMMSTORE_SIZE)) +else FMAP_SMMSTORE_ENTRY := -endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) +endif
# # X86 FMAP region # # # position, size -FMAP_FMAP_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) \ - $(FMAP_MRC_CACHE_SIZE) $(FMAP_SMMSTORE_SIZE)) +FMAP_FMAP_BASE := $(FMAP_CURRENT_BASE) FMAP_FMAP_SIZE := 0x200
# @@ -950,7 +940,9 @@ # position and size of CBFS, relative to BIOS_BASE FMAP_CBFS_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE)) FMAP_CBFS_SIZE := $(call int-subtract, $(FMAP_BIOS_SIZE) $(FMAP_CBFS_BASE)) + else # ifeq ($(CONFIG_ARCH_X86),y) + DEFAULT_FLASHMAP:=$(top)/util/cbfstool/default.fmd # entire flash FMAP_ROM_ADDR := 0 @@ -963,47 +955,41 @@ FMAP_FMAP_BASE := 0x20000 FMAP_FMAP_SIZE := 0x100
+FMAP_CURRENT_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE)) + # # NON-X86 CONSOLE FMAP region # # position, size and entry line of CONSOLE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_CONSOLE_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE)) +FMAP_CONSOLE_BASE := $(FMAP_CURRENT_BASE) FMAP_CONSOLE_SIZE := $(CONFIG_CONSOLE_SPI_FLASH_BUFFER_SIZE) FMAP_CONSOLE_ENTRY := CONSOLE@$(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) -else # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_CONSOLE_BASE := 0 -FMAP_CONSOLE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE)) +else FMAP_CONSOLE_ENTRY := -endif # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) +endif
# # NON-X86 RW_MRC_CACHE FMAP region # # position, size and entry line of MRC_CACHE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_MRC_CACHE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ - $(FMAP_CONSOLE_SIZE)), 0x10000) -else -FMAP_MRC_CACHE_BASE := $(call int-align, $(call int-add, $(FMAP_FMAP_BASE) \ - $(FMAP_FMAP_SIZE)), 0x10000) -endif +FMAP_MRC_CACHE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10000) FMAP_MRC_CACHE_SIZE := $(CONFIG_MRC_SETTINGS_CACHE_SIZE) FMAP_MRC_CACHE_ENTRY := RW_MRC_CACHE@$(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE) -else # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -FMAP_MRC_CACHE_BASE := 0 -FMAP_MRC_CACHE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE)) +else FMAP_MRC_CACHE_ENTRY := -endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) +endif
# # NON-X86 COREBOOT default cbfs FMAP region # # position and size of CBFS, relative to BIOS_BASE -FMAP_CBFS_BASE := $(call int-add,$(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE) $(FMAP_CONSOLE_SIZE) \ - $(FMAP_MRC_CACHE_SIZE)) +FMAP_CBFS_BASE := $(FMAP_CURRENT_BASE) FMAP_CBFS_SIZE := $(call int-subtract,$(FMAP_BIOS_SIZE) $(FMAP_CBFS_BASE)) + endif # ifeq ($(CONFIG_ARCH_X86),y)
$(obj)/fmap.fmd: $(top)/Makefile.inc $(DEFAULT_FLASHMAP) $(obj)/config.h
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40373 )
Change subject: Makefile: Simplify calculation of region base with default fmd files ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40373/1/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40373/1/Makefile.inc@984 PS1, Line 984: endif nit: maybe if you reshuffled this a bit most of these could be shared between x86 and non-x86 now?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40373 )
Change subject: Makefile: Simplify calculation of region base with default fmd files ......................................................................
Patch Set 1: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40373 )
Change subject: Makefile: Simplify calculation of region base with default fmd files ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40373 )
Change subject: Makefile: Simplify calculation of region base with default fmd files ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40373/1/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40373/1/Makefile.inc@984 PS1, Line 984: endif
nit: maybe if you reshuffled this a bit most of these could be shared between x86 and non-x86 now?
Sure, probably better done as a follow-up change. I wanted to keep the same behavior as before here.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40373 )
Change subject: Makefile: Simplify calculation of region base with default fmd files ......................................................................
Makefile: Simplify calculation of region base with default fmd files
When using default fmd files, base of the fmap region is currently calculated based on the size and base of previous fmap regions. Since the existence of any fmap region is dependent on the selection of certain CONFIG_* parameters, these calculations get complicated. Every time base is calculated for a region, there need to be checks to see which of the previous regions really exist. As the regions in default fmd file are increased, these calculations and the conditional checks get even more complicated.
This change introduces a Makefile variable FMAP_CURRENT_BASE which is updated every time a new region is allocated space. This allows using the same steps for determining the base of any fmap region irrespective of the state of previous regions.
The way the code is organized it should be possible in the future to also add a macro to perform the same steps (in case that is possible).
TEST=Verified that coreboot image generated remains unchanged for x86 and ARM boards using the default fmd files.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I2a109462928b6e8b7930bbcc1a1ba45fa85de6ac Reviewed-on: https://review.coreboot.org/c/coreboot/+/40373 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Aaron Durbin adurbin@chromium.org --- M Makefile.inc 1 file changed, 30 insertions(+), 44 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Julius Werner: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index 39dd6dd..8835ec3 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -896,52 +896,42 @@ # X86 CONSOLE FMAP region # # position, size and entry line of CONSOLE relative to BIOS_BASE, if enabled -FMAP_CONSOLE_BASE := 0 + +FMAP_CURRENT_BASE := 0 + ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) +FMAP_CONSOLE_BASE := $(FMAP_CURRENT_BASE) FMAP_CONSOLE_SIZE := $(CONFIG_CONSOLE_SPI_FLASH_BUFFER_SIZE) FMAP_CONSOLE_ENTRY := CONSOLE@$(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) -else # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_CONSOLE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE)) +else FMAP_CONSOLE_ENTRY := -endif # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) +endif
-# -# X86 RW_MRC_CACHE FMAP region -# -# position, size and entry line of MRC_CACHE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -FMAP_MRC_CACHE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ - $(FMAP_CONSOLE_SIZE)), 0x10000) +FMAP_MRC_CACHE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10000) FMAP_MRC_CACHE_SIZE := $(CONFIG_MRC_SETTINGS_CACHE_SIZE) FMAP_MRC_CACHE_ENTRY := RW_MRC_CACHE@$(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE) -else # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -FMAP_MRC_CACHE_BASE := 0 -FMAP_MRC_CACHE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE)) +else FMAP_MRC_CACHE_ENTRY := -endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) +endif
-# -# X86 SMMSTORE FMAP region -# -# position, size and entry line of SMMSTORE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_SMMSTORE),y) -FMAP_SMMSTORE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ - $(FMAP_CONSOLE_SIZE) $(FMAP_MRC_CACHE_SIZE)), 0x10000) +FMAP_SMMSTORE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10000) FMAP_SMMSTORE_SIZE := $(CONFIG_SMMSTORE_SIZE) FMAP_SMMSTORE_ENTRY := SMMSTORE@$(FMAP_SMMSTORE_BASE) $(FMAP_SMMSTORE_SIZE) -else # ifeq ($(CONFIG_SMMSTORE),y) -FMAP_SMMSTORE_BASE := 0 -FMAP_SMMSTORE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_SMMSTORE_BASE) $(FMAP_SMMSTORE_SIZE)) +else FMAP_SMMSTORE_ENTRY := -endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) +endif
# # X86 FMAP region # # # position, size -FMAP_FMAP_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) \ - $(FMAP_MRC_CACHE_SIZE) $(FMAP_SMMSTORE_SIZE)) +FMAP_FMAP_BASE := $(FMAP_CURRENT_BASE) FMAP_FMAP_SIZE := 0x200
# @@ -950,7 +940,9 @@ # position and size of CBFS, relative to BIOS_BASE FMAP_CBFS_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE)) FMAP_CBFS_SIZE := $(call int-subtract, $(FMAP_BIOS_SIZE) $(FMAP_CBFS_BASE)) + else # ifeq ($(CONFIG_ARCH_X86),y) + DEFAULT_FLASHMAP:=$(top)/util/cbfstool/default.fmd # entire flash FMAP_ROM_ADDR := 0 @@ -963,47 +955,41 @@ FMAP_FMAP_BASE := 0x20000 FMAP_FMAP_SIZE := 0x100
+FMAP_CURRENT_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE)) + # # NON-X86 CONSOLE FMAP region # # position, size and entry line of CONSOLE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_CONSOLE_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE)) +FMAP_CONSOLE_BASE := $(FMAP_CURRENT_BASE) FMAP_CONSOLE_SIZE := $(CONFIG_CONSOLE_SPI_FLASH_BUFFER_SIZE) FMAP_CONSOLE_ENTRY := CONSOLE@$(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) -else # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_CONSOLE_BASE := 0 -FMAP_CONSOLE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE)) +else FMAP_CONSOLE_ENTRY := -endif # ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) +endif
# # NON-X86 RW_MRC_CACHE FMAP region # # position, size and entry line of MRC_CACHE relative to BIOS_BASE, if enabled ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -ifeq ($(CONFIG_CONSOLE_SPI_FLASH),y) -FMAP_MRC_CACHE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ - $(FMAP_CONSOLE_SIZE)), 0x10000) -else -FMAP_MRC_CACHE_BASE := $(call int-align, $(call int-add, $(FMAP_FMAP_BASE) \ - $(FMAP_FMAP_SIZE)), 0x10000) -endif +FMAP_MRC_CACHE_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10000) FMAP_MRC_CACHE_SIZE := $(CONFIG_MRC_SETTINGS_CACHE_SIZE) FMAP_MRC_CACHE_ENTRY := RW_MRC_CACHE@$(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE) -else # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) -FMAP_MRC_CACHE_BASE := 0 -FMAP_MRC_CACHE_SIZE := 0 +FMAP_CURRENT_BASE := $(call int-add, $(FMAP_MRC_CACHE_BASE) $(FMAP_MRC_CACHE_SIZE)) +else FMAP_MRC_CACHE_ENTRY := -endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) +endif
# # NON-X86 COREBOOT default cbfs FMAP region # # position and size of CBFS, relative to BIOS_BASE -FMAP_CBFS_BASE := $(call int-add,$(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE) $(FMAP_CONSOLE_SIZE) \ - $(FMAP_MRC_CACHE_SIZE)) +FMAP_CBFS_BASE := $(FMAP_CURRENT_BASE) FMAP_CBFS_SIZE := $(call int-subtract,$(FMAP_BIOS_SIZE) $(FMAP_CBFS_BASE)) + endif # ifeq ($(CONFIG_ARCH_X86),y)
$(obj)/fmap.fmd: $(top)/Makefile.inc $(DEFAULT_FLASHMAP) $(obj)/config.h
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)