Change in coreboot[master]: default.fmd: reserve space for the ffs headers

[Marty E. Plummer (Code Review)]

Marty E. Plummer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

default.fmd: reserve space for the ffs headers

Change-Id: I184efae6860a9a2e3cf6a39b9d0df2a6c3e943d4
Signed-off-by: Marty E. Plummer 
---
M Makefile.inc
M src/Kconfig
M util/cbfstool/default.fmd
3 files changed, 39 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/35009/1

diff --git a/Makefile.inc b/Makefile.inc
index 9ad0c59..722e180 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -964,6 +964,22 @@
 FMAP_FMAP_SIZE := 0x100

#
+# NON_X86 FFS_HDR_1 REGION
+#
+#
+ifeq ($(CONFIG_FFS_HEADER),y)
+FMAP_FFS_HDR_1_BASE := 0
+FMAP_FFS_HDR_SIZE := 0x8000
+FMAP_FFS_HDR_1_ENTRY := FFS_HDR_1@0 $(FMAP_FFS_HDR_1_SIZE)
+FMAP_FMAP_BASE := $(call int-add, $(FMAP_FMAP_BASE) $(FMAP_FFS_HDR_SIZE))
+FMAP_FMAP_SIZE := 0x180
+else # ifeq ($(CONFIG_FFS_HEADER),y)
+FMAP_FFS_HDR_1_BASE := 0
+FMAP_FFS_HDR_1_SIZE := 0
+FMAP_FFS_HDR_1_ENTRY :=
+endif # ifeq ($(CONFIG_FFS_HEADER),y)
+
+#
 # NON-X86 CONSOLE FMAP region
 #
 # position, size and entry line of CONSOLE relative to BIOS_BASE, if enabled
@@ -1004,8 +1020,21 @@
 FMAP_CBFS_BASE := $(call int-add,$(FMAP_FMAP_BASE) $(FMAP_FMAP_SIZE) $(FMAP_CONSOLE_SIZE) \
    $(FMAP_MRC_CACHE_SIZE))
 FMAP_CBFS_SIZE := $(call int-subtract,$(FMAP_BIOS_SIZE) $(FMAP_CBFS_BASE))
+FMAP_CBFS_SIZE := $(call int-subtract, $(FMAP_CBFS_SIZE) $(FMAP_FFS_HDR_SIZE))
 endif # ifeq ($(CONFIG_ARCH_X86),y)

+#
+# NON_X86 FFS_HDR_2 REGION
+#
+#
+ifeq ($(CONFIG_FFS_HEADER),y)
+FMAP_FFS_HDR_2_BASE := $(call int-add, $(FMAP_CBFS_BASE) $(FMAP_CBFS_SIZE))
+FMAP_FFS_HDR_2_ENTRY := FFS_HDR_2@$(FMAP_FFS_HDR_2_BASE) $(FMAP_FFS_HDR_SIZE)
+else # ifeq ($(CONFIG_FFS_HEADER),y)
+FMAP_FFS_HDR_2_BASE := 0
+FMAP_FFS_HDR_1_ENTRY :=
+endif # ifeq ($(CONFIG_FFS_HEADER),y)
+
 $(obj)/fmap.fmd: $(top)/Makefile.inc $(DEFAULT_FLASHMAP) $(obj)/config.h
    sed -e "s,##ROM_BASE##,$(FMAP_ROM_ADDR)," \
        -e "s,##ROM_SIZE##,$(FMAP_ROM_SIZE)," \
@@ -1013,11 +1042,13 @@
        -e "s,##BIOS_SIZE##,$(FMAP_BIOS_SIZE)," \
        -e "s,##FMAP_BASE##,$(FMAP_FMAP_BASE)," \
        -e "s,##FMAP_SIZE##,$(FMAP_FMAP_SIZE)," \
+	    -e "s,##FFS_HDR_1_ENTRY##,$(FMAP_FFS_HDR_1_ENTRY)," \
        -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,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \
        -e "s,##CBFS_SIZE##,$(FMAP_CBFS_SIZE)," \
+	    -e "s,##FFS_HDR_2_ENTRY##,$(FMAP_FFS_HDR_2_ENTRY)," \
    	$(DEFAULT_FLASHMAP) > $@.tmp
    mv $@.tmp $@
 else # ifeq ($(CONFIG_FMDFILE),)
diff --git a/src/Kconfig b/src/Kconfig
index 6288d0b..0fe8445 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -367,6 +367,12 @@
      but in some cases more complex setups are required.
      When an fmd is specified, it overrides the default format.

+config FFS_HEADER
+	bool "Add ffs header"
+	default n
+	help
+	  Generate an open-power compatible ffs header. Required for booting
+	  coreboot on open-power systems with their original seeprom firmware.
 endmenu

# load site-local kconfig to allow user specific defaults and overrides
diff --git a/util/cbfstool/default.fmd b/util/cbfstool/default.fmd
index 8f4819e..b0e82ea 100644
--- a/util/cbfstool/default.fmd
+++ b/util/cbfstool/default.fmd
@@ -11,10 +11,12 @@

FLASH@##ROM_BASE## ##ROM_SIZE## {
    BIOS@##BIOS_BASE## ##BIOS_SIZE## {
+		##FFS_HDR_1_ENTRY##
    	BOOTBLOCK 128K
    	FMAP@##FMAP_BASE## ##FMAP_SIZE##
    	##CONSOLE_ENTRY##
    	##MRC_CACHE_ENTRY##
    	COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE##
+		##FFS_HDR_2_ENTRY##
    }
 }

[Timothy Pearson (Code Review)]

Timothy Pearson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

Patch Set 4: Code-Review+1

Looks good aside from the comment nits.

[Timothy Pearson (Code Review)]

Timothy Pearson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

Patch Set 4:

(2 comments)
> Patch Set 4: Code-Review+1
>
> Looks good aside from the comment nits.

...which didn't save the first time around... :)

https://review.coreboot.org/c/coreboot/+/35009/4/Makefile.inc 
File Makefile.inc:

https://review.coreboot.org/c/coreboot/+/35009/4/Makefile.inc@967 
PS4, Line 967: NON_X86
Will this be used for all non-x86 systems or should it call out ppc explicitly?

https://review.coreboot.org/c/coreboot/+/35009/4/Makefile.inc@1027 
PS4, Line 1027: NON_X86
Ditto

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35009/4/Makefile.inc 
File Makefile.inc:

https://review.coreboot.org/c/coreboot/+/35009/4/Makefile.inc@967 
PS4, Line 967: NON_X86
> Will this be used for all non-x86 systems or should it call out ppc explicitly?

Was slightly unsure, actually. I've only seen ppc systems which used this format so far,
but I suppose there is no reason some other vendor and arch couldn't use this.

https://review.coreboot.org/c/coreboot/+/35009/4/Makefile.inc@1027 
PS4, Line 1027: NON_X86
> Ditto

Ditto

[Timothy Pearson (Code Review)]

Timothy Pearson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

Patch Set 4: Code-Review+2

Fair enough.  Technically there's no reason even x86 systems couldn't use the format, but I consider the possibility so remote given x86's rather insular / idiosyncratic nature that I'm OK with the labels as-is.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

Patch Set 4:
> Patch Set 4: Code-Review+2
>
> Fair enough.  Technically there's no reason even x86 systems couldn't use the format, but I consider the possibility so remote given x86's rather insular / idiosyncratic nature that I'm OK with the labels as-is.

It all looks as if it's primarily for Power, so maybe create a new default-power.fmd template and work from there?

[Marty E. Plummer (Code Review)]

Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35009 )

Change subject: default.fmd: reserve space for the ffs headers
......................................................................

Patch Set 6:
> Patch Set 4:
> > Patch Set 4: Code-Review+2
> >
> > Fair enough.  Technically there's no reason even x86 systems couldn't use the format, but I consider the possibility so remote given x86's rather insular / idiosyncratic nature that I'm OK with the labels as-is.
>
> It all looks as if it's primarily for Power, so maybe create a new default-power.fmd template and work from there?

I've considered this, but I'm thinking to hold off on that for now because as
best as I can tell after the initial boot/flash of a coreboot.rom image one would
then need to flash an image which is (more or less) entirely cbfs, with the
bootblock 'outside' of it to be written the the p9 seeproms, so perhaps that would
be the one to have the new default-ppc64.fmd file.