Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32076 )
Change subject: mb/protectli/vault: Add FW2B and FW4B boards support ......................................................................
Patch Set 14:
(7 comments)
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 9: #include "onboard.h" I'm not seeing this used below
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 77: # LPE audio codec settings : register "lpe_codec_clk_src" = "LPE_CLK_SRC_PLL" # 19.2MHz clock is LPE audio being used?
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 80: Disable devices in ACPI mode perhaps 'Disable use of ACPI mode' since the devices aren't being disabled, just set to use PCI mode
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 131: device pnp 2e.4 off # Environment Controller : end : device pnp 2e.5 off # Keyboard : end : device pnp 2e.6 off # Mouse : end : device pnp 2e.7 off # GPIO : end : device pnp 2e.a off # CIR : end seems like these could all be single line, like 2e.0 above
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 25: Scope (_SB) { : Device (PCI0) : { : #include <acpi/southcluster.asl> : } : } : Scope (_SB.PCI0) : { : Device (RP03) : { : Name (_ADR, 0x001C0002) // _ADR: Address : OperationRegion(RPXX, PCI_Config, 0x00, 0x10) : } : } combine since same scope?
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/fadt.c:
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 19: 3 static, or use get_acpi_table_revision() ?
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 1: /* is this used? I wouldn't think so since SOC_INTEL_COMMON_BLOCK_HDA_VERB isn't selected anywhere