Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32076 )
Change subject: mb/protectli/vault: Add FW2B and FW4B Braswell based boards support ......................................................................
Patch Set 15: Code-Review+1
(16 comments)
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 13: Method (_STA, 0, NotSerialized) { Should '{' be placed on newline?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 12: #include <arch/smp/mpspec.h> Are al these include files required?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/cmos.layout:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 60: 388 4 h 0 reboot_counter Compare with FB FBG1701: Is reboot_counter used? Otherwise add # reboot_counter reserved for core, not used by platform.
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 70: 400 1 e 2 hyper_threading unused
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 80: 416 128 r 0 vbnv ChromeOS and SandyBridge MRC not used!
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 12: register "PcdIgdDvmt50PreAlloc" = "1" Might use IDG_MEMSIZE_32MB
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 77: # Disable use of ACPI mode Comment what is done. Use #Enable devices in PCI mode
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 125: device pci 1f.0 on # 8086 229c - LPC bridge TAB missing for alignment
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 13: 0x02, // DSDT revision: ACPI v2.0 and up One line comment use '/*'
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 19: #include "onboard.h" Is onboard.h required here?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/fadt.c:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 23: header->asl_compiler_revision = 1; special reason not using = asl_revision?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/gpio.c:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 10: #include <soc/gpio.h> Is soc/gpio.h the only required include file?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 249: Remove empty line
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/ramstage.c:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 10: #include <boardid.h> Is boardid.h required?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/romstage.c:
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 10: #include <fsp/car.h> Is this car.h required?
https://review.coreboot.org/c/coreboot/+/32076/15/src/mainboard/protectli/va... PS15, Line 23: ite_enable_serial(SERIAL1_DEV, CONFIG_TTYS0_BASE); ite_enable_serial() is done in early stage. Required to re-enable again? (FSP only enables internal UART as COM1)