Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40300 )
Change subject: mainboard: add Dell Latitude E7240 ......................................................................
Patch Set 2:
(20 comments)
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/Kconfig:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 6: select CPU_INTEL_HASWELL
already selected by 'NORTHBRIDGE_INTEL_HASWELL'
CB:44976 removes this from in-tree mainboards
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 20: default dell/latitude_e7240
default "dell/latitude_e7240"
that is, guard the board directory with quotes, because newer Kconfig dislikes unquoted slashes: CB:42480
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 2: bootblock-y += gpio.c gpio.c shouldn't be required in bootblock.
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/bootblock.c:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 10: pci_write_config32(PCH_LPC_DEV, LPC_GEN2_DEC, 0x007c0901); Uh, this will overwrite what `pch_enable_lpc` programmed (happens just before calling this function). Why not use the correct the value in the devicetree instead?
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 90: Unsupported PCI device 8086:282a
Hmmm
Done
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 4: register "gpu_ddi_e_connected" = "0" N/A for Haswell-LP
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 7: register "gpu_dp_d_hotplug" = "4" N/A for Haswell-LP
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 8: register "gpu_panel_port_select" = "0" I'm going to kill this (it's reserved MBZ on haswell)
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 38: register "gen2_dec" = "0x005c0921" This value is programmed on the hardware, but then overwritten in `mainboard_config_superio`, so the hardware ends up with a different value than the one in the devicetree. The allocator will use the devicetree value when reading resources, even though the actual register contains a different value.
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 47: (eSATA?) Schematics say DOCK
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 50: device pci 14.0 on end # xHCI Controller nit: IMHO, having the `end` keywords aligned (one extra space for the "on" case) eases reading the devicetree:
device pci 14.0 on end # xHCI Controller device pci 16.0 on end # Management Engine Interface 1 device pci 16.1 off end # Management Engine Interface 2 device pci 16.2 off end # Management Engine IDE-R
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 63: device pci 1c.5 off end # PCIe Port #6 This can be muxed between PCIe and SATA, depending on the state of GPIO37 (called mCARD_PCIE_SATA# in the schematics and controlled by GPIOE3 of the SMSC ECE5048)
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 64: device pci 1c.6 off end # PCIe Port #7 : device pci 1c.7 off end # PCIe Port #8 These devices don't exist on HSW-LP
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 11: -- FIXME: check this The only ports that should be in here are DP2, HDMI1 and eDP:
DDI1 is used for HDMI, so DP1 isn't supported.
DDI2 is used for DP, and there's no hardware to support for HDMI alt mode, so HDMI2 is not possible.
DP3, HDMI3, Analog and LVDS do not exist on Haswell-LP.
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/gpio.c:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 6: LP_GPIO_OUT_LOW, /* 0: OUTPUT LOW */ Instead of adding the GPIO number in comments, just use positional initializers for the array components:
[0] = LP_GPIO_OUT_LOW, [1] = { .conf0 = GPIO_MODE_GPIO | GPIO_DIR_INPUT | GPIO_IRQ_LEVEL },
This is better because the numbers can't be wrong (and if they are, it won't build)
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 14: { .conf0 = GPIO_MODE_GPIO | GPIO_DIR_INPUT | GPIO_INVERT | GPIO_IRQ_LEVEL, .pirq = GPIO_PIRQ_APIC_ROUTE }, /* 8 */
line over 96 characters
I'd define macros for these before the GPIO array, so as to make Jenkins happy and use one line per GPIO.
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 102: };
adding a line without newline at end of file
Just add a final newline to make Jenkins happy.
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/romstage.c:
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 32: { 0x0080, 1, USB_OC_PIN_SKIP, USB_PORT_DOCK }, Schematics say this is WLAN + BT (that means, USB is wired to a mPCIe slot). The length seems rather long and the port type isn't accurate.
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 34: { 0x0040, 1, USB_OC_PIN_SKIP, USB_PORT_BACK_PANEL }, nit: schematics say this is USH / Smart Card
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 37: { 0x0040, 1, USB_OC_PIN_SKIP, USB_PORT_BACK_PANEL }, nit: schematics say this is for the touchscreen
-
Angel Pons (Code Review)