Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33839 )
Change subject: mb/protectli/vault_kbl: Add FW6 boards support ......................................................................
Patch Set 7:
(32 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/33839/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33839/4//COMMIT_MSG@7 PS4, Line 7: src/mainboard
mb
Done
https://review.coreboot.org/c/coreboot/+/33839/4//COMMIT_MSG@7 PS4, Line 7: boards
There are three boards... […]
Added an entry in Documentation which explains what are the board variants etc.
https://review.coreboot.org/c/coreboot/+/33839/4//COMMIT_MSG@8 PS4, Line 8:
Tested how?
What is tested and not tested was mentioned in Documentation
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/Kconfig:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 22: FW6
Is the mainboard name "FW6" or "FW6x"? (x is either A, B or C)
Board is FW6 and I do not want to change this Kconfig option, because it is propagated to SMBIOS. Vendor BIOS had FW6 as part number for all variants. I know from my own experience that careless modification of SMBIOS causes a huge mess in a long time.
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 36: config USE_INTEL_FSP_MP_INIT : bool : default n : : config USE_COREBOOT_NATIVE_MP_INIT : bool : default y
Aren't these the default values already?
Indeed. Removed
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 50: default "8086,5916" # 8086,5906 for FW6A
Use variants?
Not necessary. Actually I would like to avoid it because of SMBIOS as mentioned in other comment. Resolved/workarounded VGA BIOS ID with libgfxinit
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... PS2, Line 4:
Extra newline
Done
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... PS2, Line 16:
Extra newline
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 4: * Copyright (C) 2015 Google Inc.
Remove.
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 5: * Copyright (C) 2016 Intel Corporation.
IANAL, but I don't believe changing copyright notices of others is a good idea.
Removed the Copyrights as coreboot uses AUTHORS file now
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 17: Scope (_SB)
Note that this file is already included inside the _SB scope in dsdt. […]
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/acpi/superio.asl:
PS4:
Do empty files need copyright headers?
IIRC Jenkins complains on files without a proper header.
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 4: * Copyright (C) 2015 Google Inc.
Remove.
Done
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/acpi_tables.c:
PS2:
Copyright on what? If this file is not going to be used, please remove it, or leave it empty.
IIRC Jenkins complains on files without the header
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/board_info.txt:
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... PS2, Line 1: Vendor name: Intel : Board name: Kabylake RVP Reference Board
oops
Ack
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/board_info.txt:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 2: Board name: FW6A FW6B FW6C
How about "FW6 series" or "FW6x series" ?
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 235: CHIPSET_LOCKDOWN_FSP
Really?
Corrected
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 20: "dsdt.aml",
Do we use tabs in coreboot to indent ASL files?
AFAIK yes.
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 28: // Some generic macros
Please drop. […]
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 34: // CPU
This comment does not add much value, so I would also drop it
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 37: Scope (_SB) { : Device (PCI0)
Device (_SB. […]
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 45: // Chipset specific sleep states
This comment was also dropped
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/gpio.h:
PS4:
If possible, consider using a gpio. […]
Not currently possible on skylake SoC IIRC. it would result in C file inclusion.
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/mainboard.c:
PS4:
Do you even need this file?
coreboot won't build without it
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 6: * Copyright (C) 2016 Intel Corporation.
Why?
Ack
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/ramstage.c:
https://review.coreboot.org/c/coreboot/+/33839/2/src/mainboard/protectli/vau... PS2, Line 35:
Extra newline
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/ramstage.c:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 25: *
Ack
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 26: gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table));
Isn't it indented with tabs already?
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/romstage.c:
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 50: sizeof(RcompResistor));
One line.
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 57: 100, 40, 20, 20, 26 };
This fits on a single line as well.
Done
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... File src/mainboard/protectli/vault_kbl/smihandler.c:
PS4:
Does this board actually need a SMI handler?
Actually it doesn't. Removed file.
https://review.coreboot.org/c/coreboot/+/33839/4/src/mainboard/protectli/vau... PS4, Line 35: /* On success, the IO Trap Handler returns 0
Please use the comment style below: […]
File removed.