Frans Hendriks 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:
(10 comments)
Have implemented the comment. Will update with new patch
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: […]
Done
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?
UART of onboard IT8528
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?
Change to sbc
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
This IS_ENABLED can be remove, since C_ENVIRONMENT_BOOTBLOCK is defaault
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?
DPTF should be disabled
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.
This file is used for VENDORCODE_ELTAN_VBOOT. It can be used with VENDORCODE_ELTAN_VBOOT disabled.
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?
Configure the pins in not used state. Need to configure any state.
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?
Mistake. The IS_ENABLED will be covered by 1 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. […]
Done
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?
Copy/paste result from Intel Strago. Removed unused code in next patch set.