Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30414 )
Change subject: mainboard/facebook/fbg1701: Do initial mainboard commit ......................................................................
Patch Set 17:
(22 comments)
Regarding the measured boot, I have looked a little bit at Your patches Frans and I saw crypto lib that is already implemented in the vboot submodule. IMO it should be more generic and reuse as much as possible from vboot submodule. Duplicating code is rather not necessary. I understand that Your implementation was submitted first to gerrit, however, it would be great if You could integrate Your work into the current measured boot implementation. As it still is not perfect and complete (for example in terms of the TPM2.0 support), You have the field to shine on. :) If You really have something in Your implementation that makes Your measured boot implementation the only option for this mainboard, please tell us.
I am adding Philipp Deppenwiese on CC so he can advise how to proceed with integration.
Philipp could You please point the right direction to Frans?
https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/fb... File Documentation/mainboard/facebook/fbg1701.md:
https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/fb... PS17, Line 7: This board currently requires Braswell fsp blob Is microcode required?
https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/fb... PS17, Line 21: The system has an external flash chip which is a 8 MiB soldered SOIC-8 chip. It What is the purpose of the external flash? Which one is used as firmware storage?
https://review.coreboot.org/#/c/30414/17/Documentation/mainboard/facebook/fb... PS17, Line 32: - flashing coreboot Why untested?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Kcon... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Kcon... PS17, Line 1: ## Consider committing a miniconfig file to get rid of a few configuration settings here
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Kcon... PS17, Line 43: config MAINBOARD_SPD0_FILE_NAME Please use GENERIC_SPD_BIN option in board specific options
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Make... File src/mainboard/facebook/fbg1701/Makefile.inc:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Make... PS17, Line 27: ramstage-y += logo.c Why not use such notation:
ramstage-$(CONFIG_FSP1_1_DISPLAY_LOGO) += logo.c
And remove the #if IS_ENABLED(CONFIG_FSP1_1_DISPLAY_LOGO) macro from logo.c ?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/Make... PS17, Line 41: ## Please use GENERIC_SPD_BIn and define SPD_SOURCES to point to SPD files.
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acpi... File src/mainboard/facebook/fbg1701/acpi/dptf.asl:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acpi... PS17, Line 1: /* Is this ASL file included anywhere?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acpi... File src/mainboard/facebook/fbg1701/acpi/superio.asl:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/acpi... PS17, Line 21: Device (COM1) { Any reason for the indentation?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boar... File src/mainboard/facebook/fbg1701/board_info.txt:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boar... PS17, Line 3: Category: misc Does the board really not fit into any category?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boot... File src/mainboard/facebook/fbg1701/bootblock.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/boot... PS17, Line 27: #if IS_ENABLED(CONFIG_VERIFIED_BOOT) coreboot uses now CONFIG() macro.
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/com_... File src/mainboard/facebook/fbg1701/com_init.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/com_... PS17, Line 24: #if !IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) Please use CONFIG() macro
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/devi... File src/mainboard/facebook/fbg1701/devicetree.cb:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/devi... PS17, Line 75: register "DptfDisable" = "1" Why DPTF is disabled here but enabled in domain 0?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/devi... PS17, Line 102: device pci 14.0 on end # 8086 22b5 - USB XHCI - Only 1 USB controller at a time Consider adding SATA device with off state
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/fmap... File src/mainboard/facebook/fbg1701/fmap.fmd:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/fmap... PS17, Line 1: # layout for firmware residing at top of 4GB address space If the mainboard is intended to use VBOOT I also recommend to provide second fmd file for that case.
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpio... File src/mainboard/facebook/fbg1701/gpio.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpio... PS17, Line 116: NATIVE_PU1K_CSEN_INVTX(1), /* 61 I2C0_SDA */ All I2C devices are disabled, why configuring the pins for I2C?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpio... PS17, Line 120: NATIVE_PU1K_CSEN_INVTX(1), /* 65 I2C0_SCL */ a/a
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/gpio... PS17, Line 256: Empty line, please remove
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/logo... File src/mainboard/facebook/fbg1701/logo.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/logo... PS17, Line 36: size_t file_size = cbfs_boot_load_file(filename, logo_data, Why not cbfs_boot_map_with_leak? This hardcoded logo_data array looks a little bit terrible.
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/rams... File src/mainboard/facebook/fbg1701/ramstage.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/rams... PS17, Line 21: #if IS_ENABLED(CONFIG_FSP1_1_DISPLAY_LOGO) Why is this split into two blocks with macro?
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/roms... File src/mainboard/facebook/fbg1701/romstage.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/roms... PS17, Line 41: ps->spd_data_ch0 = cbfs_boot_map_with_leak(buf, CBFS_TYPE_SPD, NULL); Change filename to spd.bin after selecting GENERIC_SPD_BIN
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/smih... File src/mainboard/facebook/fbg1701/smihandler.c:
https://review.coreboot.org/#/c/30414/17/src/mainboard/facebook/fbg1701/smih... PS17, Line 60: case ACPI_S3: Board does not seem to support S3, why is this case implemented?
-
Michał Żygowski (Code Review)