Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 33: Code-Review+1
(19 comments)
https://review.coreboot.org/c/coreboot/+/32531/33/Documentation/mainboard/51... File Documentation/mainboard/51nb/x210.jpg:
PS33: Maybe crop this image a bit, so that it is under 50 KiB?
https://review.coreboot.org/c/coreboot/+/32531/33/Documentation/mainboard/51... File Documentation/mainboard/51nb/x210.md:
https://review.coreboot.org/c/coreboot/+/32531/33/Documentation/mainboard/51... PS33, Line 48: Use libgfxinit once Kaby Lake is supported. Done, I guess?
https://review.coreboot.org/c/coreboot/+/32531/8/src/ec/51nb/51nb.c File src/ec/51nb/51nb.c:
https://review.coreboot.org/c/coreboot/+/32531/8/src/ec/51nb/51nb.c@23 PS8, Line 23: /* Enable function 5 (PS/2 AUX) */
Some ECs have a SuperIO-like part. See ec/roda/it8518 for an example.
Ack
https://review.coreboot.org/c/coreboot/+/32531/8/src/ec/51nb/51nb.c@43 PS8, Line 43: CHIP_NAME("51NB EC")
Not sure if all the 51nb devices use the same EC. I'd suggest specifying the EC model (NPCE9... […]
Done
https://review.coreboot.org/c/coreboot/+/32531/8/src/ec/51nb/Makefile.inc File src/ec/51nb/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32531/8/src/ec/51nb/Makefile.inc@25 PS8, Line 25: $(51NB_EC_INSERT) $(obj)/coreboot.pre \
No, vendor firmware doesn't use the IFD ec region.
Ack
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ac... File src/mainboard/51nb/x210/acpi_tables.c:
PS33: This file shouldn't be needed anymore
https://review.coreboot.org/c/coreboot/+/32531/21/src/mainboard/51nb/x210/de... File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32531/21/src/mainboard/51nb/x210/de... PS21, Line 149: register "PcieRpEnable[2]" = "1" # Ethernet controller : register "PcieRpLtrEnable[2]" = "1"
This is not RP1?
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/de... File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/de... PS33, Line 167: register "PcieRpHotPlug[3]" = "1" Hmmmmm... Why do we want hotplug for Wi-Fi only?
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/de... PS33, Line 179: webcam Please capitalize: `Webcam`
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... File src/mainboard/51nb/x210/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... PS33, Line 33: // CPU This comment does not add much value. I would drop it
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... PS33, Line 43: Unnecessary blank line
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... PS33, Line 46: // Chipset specific sleep states This comment was dropped not too long ago
https://review.coreboot.org/c/coreboot/+/32531/8/src/mainboard/51nb/x210/hda... File src/mainboard/51nb/x210/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32531/8/src/mainboard/51nb/x210/hda... PS8, Line 25:
Will do.
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... File src/mainboard/51nb/x210/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 22: 0x0000000c 12
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 34: 0x17aa2155 This value in the comment might rot away.
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 35: 0x0 0 here and on AZALIA_PIN_CFG macros
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 39: /* Pin Complex (NID 0x19) */ I would drop these as they don't add much info.
https://review.coreboot.org/c/coreboot/+/32531/21/src/mainboard/51nb/x210/ma... File src/mainboard/51nb/x210/mainboard.c:
https://review.coreboot.org/c/coreboot/+/32531/21/src/mainboard/51nb/x210/ma... PS21, Line 53: }
This seems like Purisms idea of how to store a serial number. […]
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ro... File src/mainboard/51nb/x210/romstage.c:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ro... PS33, Line 31: sizeof(RcompResistor)); Fits on the previous line