Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33484
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
mainboard/facebook/fbg1701: Add FMAP file
No FMAP file is availabe for this mainboard. Add fmap.fmd and add file location to Kconfig
BUG=N/A TEST=Boot Embedded Linux 4.20 and verify logging on Facebook FBG-1701
Change-Id: Iecfae4573100c6787b6e8b1c4f2583a7fb3d95a3 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/mainboard/facebook/fbg1701/Kconfig A src/mainboard/facebook/fbg1701/fmap.fmd 2 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/33484/1
diff --git a/src/mainboard/facebook/fbg1701/Kconfig b/src/mainboard/facebook/fbg1701/Kconfig index 3f45194..801a99a 100644 --- a/src/mainboard/facebook/fbg1701/Kconfig +++ b/src/mainboard/facebook/fbg1701/Kconfig @@ -70,6 +70,10 @@ hex default 0x08000
+config FMDFILE + string + default "$(top)/src/mainboard/$(CONFIG_MAINBOARD_DIR)/fmap.fmd" + config FSP_LOC hex default 0xfff9c000 diff --git a/src/mainboard/facebook/fbg1701/fmap.fmd b/src/mainboard/facebook/fbg1701/fmap.fmd new file mode 100644 index 0000000..dca6838 --- /dev/null +++ b/src/mainboard/facebook/fbg1701/fmap.fmd @@ -0,0 +1,21 @@ +# layout for firmware residing at top of 4GB address space +# +-------------+ <-- 4GB - ROM_SIZE / start of flash +# | unspecified | +# +-------------+ <-- 4GB - BIOS_SIZE +# | FMAP | +# +-------------+ <-- 4GB - BIOS_SIZE + FMAP_SIZE +# | CBFS | +# +-------------+ <-- 4GB / end of flash + +FLASH@0xFF800000 0x800000 { + SI_ALL@0x0 0x200000 { + SI_DESC@0x0 0x1000 + SI_ME@0x1000 0x1ff000 + } + BIOS@0x200000 0x600000 { + FMAP@0 0x200 + RW_MRC_CACHE@0x200 0x8000 + + COREBOOT(CBFS)@0x8200 0x5F7E00 + } +}
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 1: Code-Review-1
It's not needed and prevents the use of features that automatically update the generated a fmap.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/Kconf... PS1, Line 57: 0x00800000 Set this to 0x600000 and the default fmap will be very similar
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/fmap.... File src/mainboard/facebook/fbg1701/fmap.fmd:
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/fmap.... PS1, Line 17: RW_MRC_CACHE@0x200 0x8000 should be 64K aligned iirc.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/Kconf... PS1, Line 57: 0x00800000
Set this to 0x600000 and the default fmap will be very similar
Will change it to 0x5F700 which is CBFS size with 4K alignment for MRC cache.
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/fmap.... File src/mainboard/facebook/fbg1701/fmap.fmd:
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/fmap.... PS1, Line 17: RW_MRC_CACHE@0x200 0x8000
should be 64K aligned iirc.
Expect the alignment is related to SPI layout/routines. If SPI routines update the modified bytes only and saves bytes in same block(s), there is not issue. Will align to 4K which is sector size of used SPI.
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33484
to look at the new patch set (#2).
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
mainboard/facebook/fbg1701: Add FMAP file
No FMAP file is availabe for this mainboard. Add fmap.fmd and add file location to Kconfig
BUG=N/A TEST=Boot Embedded Linux 4.20 and verify logging on Facebook FBG-1701
Change-Id: Iecfae4573100c6787b6e8b1c4f2583a7fb3d95a3 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/mainboard/facebook/fbg1701/Kconfig A src/mainboard/facebook/fbg1701/fmap.fmd 2 files changed, 25 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/33484/2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/Kconf... PS1, Line 57: 0x00800000
Will change it to 0x5F700 which is CBFS size with 4K alignment for MRC cache.
Done
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/fmap.... File src/mainboard/facebook/fbg1701/fmap.fmd:
https://review.coreboot.org/#/c/33484/1/src/mainboard/facebook/fbg1701/fmap.... PS1, Line 17: RW_MRC_CACHE@0x200 0x8000
Expect the alignment is related to SPI layout/routines. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... PS2, Line 57: This should be the size of the BIOS region at most.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
Clean. Size should be 0x60000.
IMO there should be fmap.fmd to describe the layout of the SPI including the descriptor and ME location.
Previous comment was that fmap.fmd must not be supplied. My idea was to have a fmd file available, but rename it to some 'none default' name. Since main features this project are verified boot and measured boot my suggestion is using secure.fmp
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... PS2, Line 57:
This should be the size of the BIOS region at most.
My assumption is tht the CBFS_SIZE is 0x5F700 which is available for coreboot. Should this be the BIOS region (0x600000 in fmap.fmd) for BIOS or size?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
Clean. Size should be 0x60000.
IMO there should be fmap.fmd to describe the layout of the SPI including the descriptor and ME location.
Well, are these regions used anywhere in coreboot? If not, it would just add redundant information that already exists in the flash descriptor. Which generally makes maintenance harder.
Previous comment was that fmap.fmd must not be supplied. My idea was to have a fmd file available, but rename it to some 'none default' name. Since main features this project are verified boot and measured boot my suggestion is using secure.fmp
Well, if that file adds information necessary for verified/measured boot, that makes sense. But I don't see that yet.
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... PS2, Line 57:
My assumption is tht the CBFS_SIZE is 0x5F700 which is available for coreboot. […]
Yes, 0x600000 seems correct in your case. You can also try it and inspect the generated FMAP for yourself, simply set this value to 0x600000 and FMDFILE to an empty string, then build. `build/fmap.fmd` should be auto- matically generated. It's not easy to read because of some decimal numbers, but should roughly reflect what you have in your handcrafted file (wrt. the BIOS region).
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
Clean. Size should be 0x60000.
IMO there should be fmap.fmd to describe the layout of the SPI including the descriptor and ME location.
Well, are these regions used anywhere in coreboot? If not, it would just add redundant information that already exists in the flash descriptor. Which generally makes maintenance harder.
Previous comment was that fmap.fmd must not be supplied. My idea was to have a fmd file available, but rename it to some 'none default' name. Since main features this project are verified boot and measured boot my suggestion is using secure.fmp
Well, if that file adds information necessary for verified/measured boot, that makes sense. But I don't see that yet.
We generate a full SPI image. Please let me check if removing fmap.fmp still works.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
Clean. Size should be 0x60000.
IMO there should be fmap.fmd to describe the layout of the SPI including the descriptor and ME location.
Well, are these regions used anywhere in coreboot? If not, it would just add redundant information that already exists in the flash descriptor. Which generally makes maintenance harder.
Previous comment was that fmap.fmd must not be supplied. My idea was to have a fmd file available, but rename it to some 'none default' name. Since main features this project are verified boot and measured boot my suggestion is using secure.fmp
Well, if that file adds information necessary for verified/measured boot, that makes sense. But I don't see that yet.
The solution: abondon this patchset.
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... PS2, Line 57:
Yes, 0x600000 seems correct in your case. You can also try it and inspect […]
Build without custom FMAP and CBFS_SIZE to 0x600000 and the sizes are equal. The generated ROM works fine.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
Clean. Size should be 0x60000.
IMO there should be fmap.fmd to describe the layout of the SPI including the descriptor and ME location.
Well, are these regions used anywhere in coreboot? If not, it would just add redundant information that already exists in the flash descriptor. Which generally makes maintenance harder.
Previous comment was that fmap.fmd must not be supplied. My idea was to have a fmd file available, but rename it to some 'none default' name. Since main features this project are verified boot and measured boot my suggestion is using secure.fmp
Well, if that file adds information necessary for verified/measured boot, that makes sense. But I don't see that yet.
The solution: abondon this patchset.
Sorry, the CBFS_SIZE needs to be corrected.
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33484
to look at the new patch set (#3).
Change subject: mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000 ......................................................................
mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000
CBFS_SIZE equals size of whole SPI device. The descriptor and ME need to be placed in bottom part. Reduce the CBFS_SIZE to maximum avalaible size.
BUG=N/A TEST=Boot Embedded Linux 4.20 on Facebook FBG-1701
Change-Id: Iecfae4573100c6787b6e8b1c4f2583a7fb3d95a3 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/mainboard/facebook/fbg1701/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/33484/3
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000 ......................................................................
Patch Set 3:
(1 comment)
This patchset only changes the CBFS_SIZE now.
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... PS2, Line 57:
Build without custom FMAP and CBFS_SIZE to 0x600000 and the sizes are equal. […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000 ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000 ......................................................................
Patch Set 3: Code-Review+1
Felix Held has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000 ......................................................................
mainboard/facebook/fbg1701: Set CBFS_SIZE to 0x600000
CBFS_SIZE equals size of whole SPI device. The descriptor and ME need to be placed in bottom part. Reduce the CBFS_SIZE to maximum avalaible size.
BUG=N/A TEST=Boot Embedded Linux 4.20 on Facebook FBG-1701
Change-Id: Iecfae4573100c6787b6e8b1c4f2583a7fb3d95a3 Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33484 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/mainboard/facebook/fbg1701/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved
diff --git a/src/mainboard/facebook/fbg1701/Kconfig b/src/mainboard/facebook/fbg1701/Kconfig index b3c589d..95d8f6c 100644 --- a/src/mainboard/facebook/fbg1701/Kconfig +++ b/src/mainboard/facebook/fbg1701/Kconfig @@ -54,7 +54,7 @@
config CBFS_SIZE hex - default 0x00800000 + default 0x00600000
config CPU_MICROCODE_CBFS_LEN hex
-
Arthur Heymans (Code Review) -
Felix Held (Code Review)
-
Frans Hendriks (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)