Michał Żygowski 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:
(11 comments)
https://review.coreboot.org/c/coreboot/+/32076/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32076/14//COMMIT_MSG@8 PS14, Line 8:
Despite the added documentation, I’d still mention the *Braswell* in the commit message, and explain […]
Done
https://review.coreboot.org/c/coreboot/+/32076/7/src/mainboard/protectli/vau... File src/mainboard/protectli/vault/variants/fw2b/devicetree.cb:
PS7:
Please use an overridetree if most settings are the same.
All settings are the same except one disabled PCIe RP. See variants directory
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
Done
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... File src/mainboard/protectli/vault_bsw/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 38: /* Disable USB ports in S5 */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0; : : /* Disable DPTF */ : gnvs->dpte = 0;
But you just zeroed the whole thing a few lines ago?
Done
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?
it's not, removed
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
Done
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. […]
Done
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?
Done
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() ?
Done
https://review.coreboot.org/c/coreboot/+/32076/14/src/mainboard/protectli/va... PS14, Line 39: acpi_checksum((void *) fadt, header->length);
Fits in one line
Done
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
Removed. Will implement it later