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?

View Change

22 comments:

• File Documentation/mainboard/facebook/fbg1701.md:

  • Patch Set #17, Line 7: This board currently requires Braswell fsp blob

    Is microcode required?

  • Patch Set #17, 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?

  • Patch Set #17, Line 32: - flashing coreboot

    Why untested?

• File src/mainboard/facebook/fbg1701/Kconfig:

  • Patch Set #17, Line 1: ##

    Consider committing a miniconfig file to get rid of a few configuration settings here

  • Patch Set #17, Line 43: config MAINBOARD_SPD0_FILE_NAME

    Please use GENERIC_SPD_BIN option in board specific options

• File src/mainboard/facebook/fbg1701/Makefile.inc:

  • Patch Set #17, 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 ?

  • Patch Set #17, Line 41: ##

    Please use GENERIC_SPD_BIn and define SPD_SOURCES to point to SPD files.

• File src/mainboard/facebook/fbg1701/acpi/dptf.asl:

  • Patch Set #17, Line 1: /*

    Is this ASL file included anywhere?

• File src/mainboard/facebook/fbg1701/acpi/superio.asl:

  • Patch Set #17, Line 21: Device (COM1) {

    Any reason for the indentation?

• File src/mainboard/facebook/fbg1701/board_info.txt:

  • Patch Set #17, Line 3: Category: misc

    Does the board really not fit into any category?

• File src/mainboard/facebook/fbg1701/bootblock.c:

  • Patch Set #17, Line 27: #if IS_ENABLED(CONFIG_VERIFIED_BOOT)

    coreboot uses now CONFIG() macro.

• File src/mainboard/facebook/fbg1701/com_init.c:

  • Patch Set #17, Line 24: #if !IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK)

    Please use CONFIG() macro

• File src/mainboard/facebook/fbg1701/devicetree.cb:

  • Patch Set #17, Line 75: register "DptfDisable" = "1"

    Why DPTF is disabled here but enabled in domain 0?

  • Patch Set #17, 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

• File src/mainboard/facebook/fbg1701/fmap.fmd:

  • Patch Set #17, 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.

• File src/mainboard/facebook/fbg1701/gpio.c:

  • Patch Set #17, Line 116: NATIVE_PU1K_CSEN_INVTX(1), /* 61 I2C0_SDA */

    All I2C devices are disabled, why configuring the pins for I2C?

  • Patch Set #17, Line 120: NATIVE_PU1K_CSEN_INVTX(1), /* 65 I2C0_SCL */

    a/a

  • Patch Set #17, Line 256:

    Empty line, please remove

• File src/mainboard/facebook/fbg1701/logo.c:

  • Patch Set #17, 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.

• File src/mainboard/facebook/fbg1701/ramstage.c:

  • Patch Set #17, Line 21: #if IS_ENABLED(CONFIG_FSP1_1_DISPLAY_LOGO)

    Why is this split into two blocks with macro?

• File src/mainboard/facebook/fbg1701/romstage.c:

  • Patch Set #17, 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

• File src/mainboard/facebook/fbg1701/smihandler.c:

  • Patch Set #17, Line 60: case ACPI_S3:

    Board does not seem to support S3, why is this case implemented?

To view, visit change 30414. To unsubscribe, or for help writing mail filters, visit settings.