Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33839 )
Change subject: mb/protectli/vault_kbl: Add FW6 support ......................................................................
Patch Set 11:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... File Documentation/mainboard/protectli/fw6.jpg:
PS10:
Where's the flash chip? I wouldn't want to hear about people reflashing their MOSFETs
On the bottom side. Described in Documentation
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... File Documentation/mainboard/protectli/fw6.md:
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... PS10, Line 25: by coreboot
by *the* coreboot
Done
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... PS10, Line 49: Use a clip
Is the flash chip voltage rail isolated from the rest of the system with a diode?
Not, it is not.
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... PS10, Line 76: with VGA Option ROM : - HDMI port with libgfxinit
This can go on a single line: […]
Done
https://review.coreboot.org/c/coreboot/+/33839/10/MAINTAINERS File MAINTAINERS:
https://review.coreboot.org/c/coreboot/+/33839/10/MAINTAINERS@330 PS10, Line 330: F: src/mainboard/protectli/
Why so?
Done
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... File src/mainboard/protectli/vault_kbl/bootblock.c:
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 14: CLKIN_DEV
GPIO_DEV maybe?
Done
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... File src/mainboard/protectli/vault_kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 100: .ac_loadline = 1030, : .dc_loadline = 1030,
Can we use the values for these parameters from SoC code? as for Iccmax […]
Done
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 292: q
Trailing `q` ?
Leftovers of hotkeys... Removed
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... File src/mainboard/protectli/vault_kbl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 30: #include "acpi/mainboard.asl"
I don't think we want to include an empty file
Done
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... File src/mainboard/protectli/vault_kbl/ramstage.c:
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 26: params->GmmEnable = 0;
disable it in the devicetree: […]
Done