Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40300 )
Change subject: mainboard: add Dell Latitude E7240 ......................................................................
Patch Set 3:
(21 comments)
https://review.coreboot.org/c/coreboot/+/40300/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40300/3//COMMIT_MSG@11 PS3, Line 11: It boots from USB and mSATA
Microsoft Windows and GNU/Linux?
Probably Linux for now. Windows in BIOS mode is very picky. Given that ACPI is still WIP, I'd say Windows is still untested.
https://review.coreboot.org/c/coreboot/+/40300/3/Documentation/mainboard/del... File Documentation/mainboard/dell/latitude_e7240.md:
https://review.coreboot.org/c/coreboot/+/40300/3/Documentation/mainboard/del... PS3, Line 31: when the IFD is unlocked That shouldn't be necessary to flash coreboot. Even flashrom says which command-line switch one should use to flash when a region is read-protected (--noverify-all).
However, if the vendor firmware sets protected ranges, please mention so.
https://review.coreboot.org/c/coreboot/+/40300/3/Documentation/mainboard/del... PS3, Line 37: not supported yet nit: `not yet supported` sounds better and seems to be more frequent (has over four times as many search results in Google)
https://review.coreboot.org/c/coreboot/+/40300/3/Documentation/mainboard/del... PS3, Line 48: No internal display before booting to OS when connected with a dock Does this happen when using legacy VGA text mode with libgfxinit?
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
CB:44976 removes this from in-tree mainboards
Done
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 20: default dell/latitude_e7240
that is, guard the board directory with quotes, because newer Kconfig dislikes unquoted slashes: CB: […]
Done
https://review.coreboot.org/c/coreboot/+/40300/3/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/board_info.txt:
https://review.coreboot.org/c/coreboot/+/40300/3/src/mainboard/dell/latitude... PS3, Line 6: n flashrom works, though?
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) […]
Another option would be to use LPC_GEN4_DEC for this. It's not used in devicetree, so once you're done you can just clear it.
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 101: Host bridge Host bridge
one Host bridge too much?
Ack
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 104: device pci 01.0 off # PCIe Bridge for discrete graphics : end
This does not exist on HSW-LP
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 8: register "gpu_panel_port_select" = "0"
I'm going to kill this (it's reserved MBZ on haswell)
Done
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 47: (eSATA?)
Schematics say DOCK
Done
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 d […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/40300/3/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40300/3/src/mainboard/dell/latitude... PS3, Line 29: Host bridge Host bridge One host bridge too many?
https://review.coreboot.org/c/coreboot/+/40300/3/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/40300/3/src/mainboard/dell/latitude... PS3, Line 14: eDP, You can try to place this first, so that the internal display is always the first to light up. Maybe it helps with the dock issue.
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/gpio.c:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 8: LP_GPIO_OUT_LOW, /* 0: OUTPUT LOW */
Maybe align with tabs?
Ack
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 compon […]
Done
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 */
I'd define macros for these before the GPIO array, so as to make Jenkins happy and use one line per […]
Using two lines is also reasonable, especially with the explicit array indices
https://review.coreboot.org/c/coreboot/+/40300/2/src/mainboard/dell/latitude... PS2, Line 102: };
Just add a final newline to make Jenkins happy.
Done
https://review.coreboot.org/c/coreboot/+/40300/2/src/superio/smsc/Makefile.i... File src/superio/smsc/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40300/2/src/superio/smsc/Makefile.i... PS2, Line 16: subdirs-y += ece5048
gosh, I'm not going to add this yet...
Done
-
Angel Pons (Code Review)